Re: [Xen-devel] [PATCH v1] psr: fix bug which may cause crash

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.11.2019 09:17, Yi Sun wrote:
> On 19-11-27 11:06:49, Jan Beulich wrote:
>> On 27.11.2019 07:24, Yi Sun wrote:
>>> --- a/xen/arch/x86/psr.c
>>> +++ b/xen/arch/x86/psr.c
>>> @@ -316,6 +316,7 @@ static bool cat_init_feature(const struct cpuid_leaf 
>>> *regs,
>>>          [FEAT_TYPE_L3_CDP] = "L3 CDP",
>>>          [FEAT_TYPE_L2_CAT] = "L2 CAT",
>>>      };
>>> +    unsigned int i = 0;
>>
>> Unnecessary initializer and too wide a scope.
>>
> Ok, u8 is enough.

Did you perhaps mistake "scope" for "width"? You shouldn't use
fixed width types when there's no strict need to do so.

>>> @@ -332,7 +333,8 @@ static bool cat_init_feature(const struct cpuid_leaf 
>>> *regs,
>>>              return false;
>>>  
>>>          /* We reserve cos=0 as default cbm (all bits within cbm_len are 
>>> 1). */
>>> -        feat->cos_reg_val[0] = cat_default_val(feat->cat.cbm_len);
>>> +        for(i = 0; i < MAX_COS_REG_CNT; i++)
>>
>> There are number of blanks missing here (and even more ones in
>> the other instance below). It also seems to me that the comment
>> ends up misplaced now. If ...
>>
> Sorry, the comment should be modified.
> 
>>> +            feat->cos_reg_val[i] = cat_default_val(feat->cat.cbm_len);
>>>  
>>>          wrmsrl((type == FEAT_TYPE_L3_CAT ?
>>>                  MSR_IA32_PSR_L3_MASK(0) :
>>
>> ... this indeed is to remain a single write, it may want to move
>> here. But as per above keeping cached and actual values in sync
>> may make it necessary to move this write into the loop as well.
>>
> You are right, I missed to loop this sentence.
> 
> Another idea:
> I remembered that the original purpose to only write COS[0] here is to
> improve performance by not writing too many MSRs. So I am thinking to
> change the fix to below line in do_write_psr_msrs().
>     if ( feat->cos_reg_val[cos * cos_num + j] != info->val[index] &&
>          cos <= feat->cos_max )
> 
> What is your opinion? Thanks!

Looks reasonable, provided it gets accompanied by a brief but
precise comment. I'd also suggest switching around the two
sides of the && .

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel