Re: [Xen-devel] [v2 1/3] x86: refactor psr implementation in hypervisor.

2016-10-10 Thread Yi Sun 
On 16-10-10 01:34:47, Jan Beulich wrote:
> >>> On 09.10.16 at 08:43,  wrote:
> > On 16-09-30 17:18:33, Konrad Rzeszutek Wilk wrote:
> >> On Thu, Sep 22, 2016 at 10:15:20AM +0800, Yi Sun wrote:
> >> > Current psr.c is designed for supporting L3 CAT/CDP. It has many
> >> > limitations to add new feature. Considering to support more PSR
> >> > features, we need refactor PSR implementation to make it more
> >> > flexible and fulfill the principle, open for extension but closed
> >> > for modification.
> >> > 
> >> > The core of the refactoring is to abstract the common actions and
> >> > encapsulate them into "struct feat_ops". The detailed steps to add
> >> > a new feature are described at the head of psr.c.
> >> > 
> >> > Signed-off-by: Yi Sun 
> >> > 
> >> > ---
> >> > Changed since v1:
> >> >  * sysctl.c
> >> > - Interface change for abstraction requirement.
> >> >  * psr.c
> >> > - Function and variables names are changed to express accurately.
> >> > - Fix code style issues.
> >> > - Fix imprecise comments.
> >> > - Add one callback function, get_cos_num(), to fulfill the
> >> >   abstraction requirement.
> >> > - Divide get_old_set_new() callback functions into two functions:
> >> >   get_old_val() and set_new_val() make it more primitive.
> >> > - Change feat_info type from an array to union.
> >> > - Adjust some functions to replace if to switch to make them
> >> >   clearer.
> >> > - Replace custom list management to system.
> >> > - Use 'const' to make codes more safe.
> >> >  * psr.h
> >> > - Change 'enum mask_type' to 'enum psr_val_type' to express
> >> >   more accurate.
> >> > - Change parameters of psr_get_info() to fulfill abstraction
> >> >   requirement.
> >> > ---
> >> >  xen/arch/x86/domctl.c |   36 +-
> >> >  xen/arch/x86/psr.c| 1105 
> >> > +++--
> >> 
> >> Whoa. 1K changes. This is a dense patch.
> >> 
> > Yes, a little big because it refactors psr.c. :-)
> 
> Please nevertheless strive to break it up some, to aid reviewing. I
> have to admit that I'm on the edge of NAK-ing such a hard to review
> change to code that is of no core interest (at least as long as all of
> this remains Atom-only).
> 
Very sorry for this. I will try my best to split this big patch to
some small patches in V3.

> Also to both of you: Please limit the quoting in your replies.

Got it. Thank you for guidance!

> 
> Jan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [v2 1/3] x86: refactor psr implementation in hypervisor.

2016-10-10 Thread Jan Beulich 
>>> On 09.10.16 at 08:43,  wrote:
> On 16-09-30 17:18:33, Konrad Rzeszutek Wilk wrote:
>> On Thu, Sep 22, 2016 at 10:15:20AM +0800, Yi Sun wrote:
>> > Current psr.c is designed for supporting L3 CAT/CDP. It has many
>> > limitations to add new feature. Considering to support more PSR
>> > features, we need refactor PSR implementation to make it more
>> > flexible and fulfill the principle, open for extension but closed
>> > for modification.
>> > 
>> > The core of the refactoring is to abstract the common actions and
>> > encapsulate them into "struct feat_ops". The detailed steps to add
>> > a new feature are described at the head of psr.c.
>> > 
>> > Signed-off-by: Yi Sun 
>> > 
>> > ---
>> > Changed since v1:
>> >  * sysctl.c
>> > - Interface change for abstraction requirement.
>> >  * psr.c
>> > - Function and variables names are changed to express accurately.
>> > - Fix code style issues.
>> > - Fix imprecise comments.
>> > - Add one callback function, get_cos_num(), to fulfill the
>> >   abstraction requirement.
>> > - Divide get_old_set_new() callback functions into two functions:
>> >   get_old_val() and set_new_val() make it more primitive.
>> > - Change feat_info type from an array to union.
>> > - Adjust some functions to replace if to switch to make them
>> >   clearer.
>> > - Replace custom list management to system.
>> > - Use 'const' to make codes more safe.
>> >  * psr.h
>> > - Change 'enum mask_type' to 'enum psr_val_type' to express
>> >   more accurate.
>> > - Change parameters of psr_get_info() to fulfill abstraction
>> >   requirement.
>> > ---
>> >  xen/arch/x86/domctl.c |   36 +-
>> >  xen/arch/x86/psr.c| 1105 
>> > +++--
>> 
>> Whoa. 1K changes. This is a dense patch.
>> 
> Yes, a little big because it refactors psr.c. :-)

Please nevertheless strive to break it up some, to aid reviewing. I
have to admit that I'm on the edge of NAK-ing such a hard to review
change to code that is of no core interest (at least as long as all of
this remains Atom-only).

Also to both of you: Please limit the quoting in your replies.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [v2 1/3] x86: refactor psr implementation in hypervisor.

2016-10-09 Thread Yi Sun 
Thanks for reviewing the patches! Sorry for late to reply because Oct 1 to 7
is China National Holiday.

On 16-09-30 17:18:33, Konrad Rzeszutek Wilk wrote:
> On Thu, Sep 22, 2016 at 10:15:20AM +0800, Yi Sun wrote:
> > Current psr.c is designed for supporting L3 CAT/CDP. It has many
> > limitations to add new feature. Considering to support more PSR
> > features, we need refactor PSR implementation to make it more
> > flexible and fulfill the principle, open for extension but closed
> > for modification.
> > 
> > The core of the refactoring is to abstract the common actions and
> > encapsulate them into "struct feat_ops". The detailed steps to add
> > a new feature are described at the head of psr.c.
> > 
> > Signed-off-by: Yi Sun 
> > 
> > ---
> > Changed since v1:
> >  * sysctl.c
> > - Interface change for abstraction requirement.
> >  * psr.c
> > - Function and variables names are changed to express accurately.
> > - Fix code style issues.
> > - Fix imprecise comments.
> > - Add one callback function, get_cos_num(), to fulfill the
> >   abstraction requirement.
> > - Divide get_old_set_new() callback functions into two functions:
> >   get_old_val() and set_new_val() make it more primitive.
> > - Change feat_info type from an array to union.
> > - Adjust some functions to replace if to switch to make them
> >   clearer.
> > - Replace custom list management to system.
> > - Use 'const' to make codes more safe.
> >  * psr.h
> > - Change 'enum mask_type' to 'enum psr_val_type' to express
> >   more accurate.
> > - Change parameters of psr_get_info() to fulfill abstraction
> >   requirement.
> > ---
> >  xen/arch/x86/domctl.c |   36 +-
> >  xen/arch/x86/psr.c| 1105 
> > +++--
> 
> Whoa. 1K changes. This is a dense patch.
> 
Yes, a little big because it refactors psr.c. :-)

> >  xen/arch/x86/sysctl.c |   14 +-
> >  xen/include/asm-x86/psr.h |   20 +-
> >  4 files changed, 903 insertions(+), 272 deletions(-)
> > 
> > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> > index 2a2fe04..c53d819 100644
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -1386,41 +1386,41 @@ long arch_do_domctl(
> >  switch ( domctl->u.psr_cat_op.cmd )
> >  {
> >  case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM:
> > -ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
> > - domctl->u.psr_cat_op.data,
> > - PSR_CBM_TYPE_L3);
> > +ret = psr_set_val(d, domctl->u.psr_cat_op.target,
> > +  domctl->u.psr_cat_op.data,
> > +  PSR_MASK_TYPE_L3_CBM);
> >  break;
> >  
> >  case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CODE:
> > -ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
> > - domctl->u.psr_cat_op.data,
> > - PSR_CBM_TYPE_L3_CODE);
> > +ret = psr_set_val(d, domctl->u.psr_cat_op.target,
> > +  domctl->u.psr_cat_op.data,
> > +  PSR_MASK_TYPE_L3_CODE);
> >  break;
> >  
> >  case XEN_DOMCTL_PSR_CAT_OP_SET_L3_DATA:
> > -ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
> > - domctl->u.psr_cat_op.data,
> > - PSR_CBM_TYPE_L3_DATA);
> > +ret = psr_set_val(d, domctl->u.psr_cat_op.target,
> > +  domctl->u.psr_cat_op.data,
> > +  PSR_MASK_TYPE_L3_DATA);
> >  break;
> >  
> >  case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM:
> > -ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
> > - >u.psr_cat_op.data,
> > - PSR_CBM_TYPE_L3);
> > +ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> > +  >u.psr_cat_op.data,
> > +  PSR_MASK_TYPE_L3_CBM);
> >  copyback = 1;
> >  break;
> >  
> >  case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE:
> > -ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
> > - >u.psr_cat_op.data,
> > - PSR_CBM_TYPE_L3_CODE);
> > +ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> > +  >u.psr_cat_op.data,
> > +  PSR_MASK_TYPE_L3_CODE);
> >  copyback = 1;
> >  break;
> >  
> >  case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA:
> > -ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
> > - >u.psr_cat_op.data,
> > - PSR_CBM_TYPE_L3_DATA);
> > +ret =

Re: [Xen-devel] [v2 1/3] x86: refactor psr implementation in hypervisor.

2016-09-30 Thread Konrad Rzeszutek Wilk 
On Thu, Sep 22, 2016 at 10:15:20AM +0800, Yi Sun wrote:
> Current psr.c is designed for supporting L3 CAT/CDP. It has many
> limitations to add new feature. Considering to support more PSR
> features, we need refactor PSR implementation to make it more
> flexible and fulfill the principle, open for extension but closed
> for modification.
> 
> The core of the refactoring is to abstract the common actions and
> encapsulate them into "struct feat_ops". The detailed steps to add
> a new feature are described at the head of psr.c.
> 
> Signed-off-by: Yi Sun 
> 
> ---
> Changed since v1:
>  * sysctl.c
> - Interface change for abstraction requirement.
>  * psr.c
> - Function and variables names are changed to express accurately.
> - Fix code style issues.
> - Fix imprecise comments.
> - Add one callback function, get_cos_num(), to fulfill the
>   abstraction requirement.
> - Divide get_old_set_new() callback functions into two functions:
>   get_old_val() and set_new_val() make it more primitive.
> - Change feat_info type from an array to union.
> - Adjust some functions to replace if to switch to make them
>   clearer.
> - Replace custom list management to system.
> - Use 'const' to make codes more safe.
>  * psr.h
> - Change 'enum mask_type' to 'enum psr_val_type' to express
>   more accurate.
> - Change parameters of psr_get_info() to fulfill abstraction
>   requirement.
> ---
>  xen/arch/x86/domctl.c |   36 +-
>  xen/arch/x86/psr.c| 1105 
> +++--

Whoa. 1K changes. This is a dense patch.

>  xen/arch/x86/sysctl.c |   14 +-
>  xen/include/asm-x86/psr.h |   20 +-
>  4 files changed, 903 insertions(+), 272 deletions(-)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 2a2fe04..c53d819 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1386,41 +1386,41 @@ long arch_do_domctl(
>  switch ( domctl->u.psr_cat_op.cmd )
>  {
>  case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM:
> -ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
> - domctl->u.psr_cat_op.data,
> - PSR_CBM_TYPE_L3);
> +ret = psr_set_val(d, domctl->u.psr_cat_op.target,
> +  domctl->u.psr_cat_op.data,
> +  PSR_MASK_TYPE_L3_CBM);
>  break;
>  
>  case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CODE:
> -ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
> - domctl->u.psr_cat_op.data,
> - PSR_CBM_TYPE_L3_CODE);
> +ret = psr_set_val(d, domctl->u.psr_cat_op.target,
> +  domctl->u.psr_cat_op.data,
> +  PSR_MASK_TYPE_L3_CODE);
>  break;
>  
>  case XEN_DOMCTL_PSR_CAT_OP_SET_L3_DATA:
> -ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
> - domctl->u.psr_cat_op.data,
> - PSR_CBM_TYPE_L3_DATA);
> +ret = psr_set_val(d, domctl->u.psr_cat_op.target,
> +  domctl->u.psr_cat_op.data,
> +  PSR_MASK_TYPE_L3_DATA);
>  break;
>  
>  case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM:
> -ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
> - >u.psr_cat_op.data,
> - PSR_CBM_TYPE_L3);
> +ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> +  >u.psr_cat_op.data,
> +  PSR_MASK_TYPE_L3_CBM);
>  copyback = 1;
>  break;
>  
>  case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE:
> -ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
> - >u.psr_cat_op.data,
> - PSR_CBM_TYPE_L3_CODE);
> +ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> +  >u.psr_cat_op.data,
> +  PSR_MASK_TYPE_L3_CODE);
>  copyback = 1;
>  break;
>  
>  case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA:
> -ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
> - >u.psr_cat_op.data,
> - PSR_CBM_TYPE_L3_DATA);
> +ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> +  >u.psr_cat_op.data,
> +  PSR_MASK_TYPE_L3_DATA);
>  copyback = 1;
>  break;
>  
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index 0b5073c..99e4c78 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -17,28 +17,159 @@
>  #include 
>  #include 
>  #include 
> +#include 
>