Re: [PATCH v8 09/24] x86: refactor psr: set value: assemble features value array.

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

>>> Yi Sun <yi.y.sun@linux.intel.com> 03/14/17 3:20 AM >>>
>On 17-03-13 06:37:41, Jan Beulich wrote:
>> >>> On 13.03.17 at 03:43, <yi.y.sun@linux.intel.com> wrote:
>> > On 17-03-10 02:15:20, Jan Beulich wrote:
>> >> >>> On 10.03.17 at 04:21, <yi.y.sun@linux.intel.com> wrote:
>> >> > On 17-03-08 09:54:04, Jan Beulich wrote:
>> >> >> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote:
>> >> >> > @@ -207,6 +233,29 @@ static enum psr_feat_type 
>> > psr_cbm_type_to_feat_type(enum cbm_type type)
>> >> >> >      return feat_type;
>> >> >> >  }
>> >> >> >  
>> >> >> > +static bool psr_check_cbm(unsigned int cbm_len, uint64_t cbm)
>> >> >> > +{
>> >> >> > +    unsigned int first_bit, zero_bit;
>> >> >> > +
>> >> >> > +    /* Set bits should only in the range of [0, cbm_len). */
>> >> >> > +    if ( cbm & (~0ull << cbm_len) )
>> >> >> 
>> >> >> This will not do what you intend for cbm_len == 64.
>> >> >> 
>> >> > cbm_len is not 64. cbm_len means the CBM value length, how many bits. For L3
>> >> > CAT, it may be 11 bits. For L2 CAT, it may be 8 bits.
>> >> 
>> >> And there is an _architectural_ guarantee for them to never
>> >> reach 64 bits?
>> >> 
>> > Per SDM, 'EAX[4:0] reports the length of the capacity bitmask length'. So, we
>> > get cbm_len in '*_init_feature' function as below.
>> > 
>> > #define CAT_CBM_LEN_MASK 0x1f
>> > cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1;
>> > 
>> > So, the max cbm_len is '32'.
>> 
>> Great, yet in that case why do you use 64-bit calculations at all?
>> Avoiding the undefinedness when cbm_len == 32 can be
>> achieved without 64-bit arithmetic.
>> 
>Considering to support future feature, we use 64bit as input parameter type for
>psr_get_val/psr_set_val. So, we handle all values as 64bit.

But you quote the SDM above as limiting values to 32 bits. Plus I can't see why
making the code ready for 64-bit values is any help when you assume the
architecture might change - if it does, values may as well become wider. Allowing
for some slack in the public interface is certainly reasonable, but let's not make
internal code deal with needlessly wide numbers. After all dealing with 32-bit
quantities is more efficient.

Jan


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