Re: [PATCH v6 08/16] x86: implement set value flow for MBA

["Jan Beulich" <JBeulich@suse.com>]

>>> On 13.10.17 at 04:02, <yi.y.sun@linux.intel.com> wrote:
> On 17-10-12 03:43:26, Jan Beulich wrote:
>> >>> On 12.10.17 at 06:33, <yi.y.sun@linux.intel.com> wrote:
>> > On 17-10-11 07:38:52, Jan Beulich wrote:
>> >> >>> On 08.10.17 at 09:23, <yi.y.sun@linux.intel.com> wrote:
>> >> > --- a/xen/arch/x86/psr.c
>> >> > +++ b/xen/arch/x86/psr.c
>> >> > @@ -138,6 +138,12 @@ static const struct feat_props {
>> >> >  
>> >> >      /* write_msr is used to write out feature MSR register. */
>> >> >      void (*write_msr)(unsigned int cos, uint32_t val, enum psr_type type);
>> >> > +
>> >> > +    /*
>> >> > +     * check_val is used to check if input val fulfills SDM requirement.
>> >> > +     * Change it to valid value if SDM allows.
>> >> > +     */
>> >> > +    bool (*check_val)(const struct feat_node *feat, unsigned long *val);
>> >> 
>> >> I'm pretty sure I've said so before - "check" to me implies all r/o
>> >> inputs. Perhaps sanitize_val() or even just sanitize()?
>> >> 
>> >> And why unsigned long when the only caller has a uint32_t in its
>> >> hands?
>> >> 
>> > To be compatible with cat_check_cbm (old name is 'psr_check_cbm' in L2 series),
>> > the last parameter type is 'unsigned long'. We have discussed it in L2 patch set
>> > v9, patch 10.
>> 
>> Iirc (without checking the old thread) this was for calculations to
>> be done as unsigned long ones. If that's the only aspect here,
>> then imo this is not a valid reason for the hook's parameter type
>> to be unsigned long *.
>> 
> Because below macros used in cat_check_cbm require the input addr to be 
> unsigned
> long, we define the last parameter of cat_check_cbm to be unsigned long.
>     find_first_bit
>     find_next_zero_bit
>     find_next_bit
> 
> If you think the unsigned long is not appropriate for 'check_val', I think I
> have to define a local variable in cat_check_cbm to do the convertion.

Exactly - the use of unsigned long is specific to this function, not
generic for all implementations of the hook. The parameter type
change back then was only a simple way to avoid defining another
local variable.

Jan


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