diff mbox series

[v1] psr: fix bug which may cause crash

Message ID 1574835871-5005-1-git-send-email-yi.y.sun@linux.intel.com (mailing list archive)
State Superseded
Headers show
Series [v1] psr: fix bug which may cause crash | expand

Commit Message

Yi Sun Nov. 27, 2019, 6:24 a.m. UTC 
During test, we found a crash on Xen with below trace.
(XEN) Xen call trace:
(XEN)    [<ffff82d0802a065a>] R psr.c#l3_cdp_write_msr+0x1e/0x22
(XEN)    [<ffff82d0802a0858>] F psr.c#do_write_psr_msrs+0x6d/0x109
(XEN)    [<ffff82d08023e000>] F smp_call_function_interrupt+0x5a/0xac
(XEN)    [<ffff82d0802a2b89>] F call_function_interrupt+0x20/0x34
(XEN)    [<ffff82d080282c64>] F do_IRQ+0x175/0x6ae
(XEN)    [<ffff82d08038b8ba>] F common_interrupt+0x10a/0x120
(XEN)    [<ffff82d0802ec616>] F cpu_idle.c#acpi_idle_do_entry+0x9d/0xb1
(XEN)    [<ffff82d0802ecc01>] F cpu_idle.c#acpi_processor_idle+0x41d/0x626
(XEN)    [<ffff82d08027353b>] F domain.c#idle_loop+0xa5/0xa7
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 20:
(XEN) GENERAL PROTECTION FAULT
(XEN) [error_code=0000]
(XEN) ****************************************

Root cause is that the cache of COS registers are not initialized
for CAT/CDP which have non-zero default value. That causes invalid
write to MSR when COS id has exceeded the max number.. So fix it by
initializing the cache.

Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
---
 xen/arch/x86/psr.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Jan Beulich Nov. 27, 2019, 10:06 a.m. UTC | #1 
On 27.11.2019 07:24, Yi Sun wrote:
> During test, we found a crash on Xen with below trace.
> (XEN) Xen call trace:
> (XEN)    [<ffff82d0802a065a>] R psr.c#l3_cdp_write_msr+0x1e/0x22
> (XEN)    [<ffff82d0802a0858>] F psr.c#do_write_psr_msrs+0x6d/0x109
> (XEN)    [<ffff82d08023e000>] F smp_call_function_interrupt+0x5a/0xac
> (XEN)    [<ffff82d0802a2b89>] F call_function_interrupt+0x20/0x34
> (XEN)    [<ffff82d080282c64>] F do_IRQ+0x175/0x6ae
> (XEN)    [<ffff82d08038b8ba>] F common_interrupt+0x10a/0x120
> (XEN)    [<ffff82d0802ec616>] F cpu_idle.c#acpi_idle_do_entry+0x9d/0xb1
> (XEN)    [<ffff82d0802ecc01>] F cpu_idle.c#acpi_processor_idle+0x41d/0x626
> (XEN)    [<ffff82d08027353b>] F domain.c#idle_loop+0xa5/0xa7
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 20:
> (XEN) GENERAL PROTECTION FAULT
> (XEN) [error_code=0000]
> (XEN) ****************************************
> 
> Root cause is that the cache of COS registers are not initialized
> for CAT/CDP which have non-zero default value. That causes invalid
> write to MSR when COS id has exceeded the max number.. So fix it by
> initializing the cache.

I'm struggling with this description, first and foremost because
there's no (recognizable to me) connection between the supposed
root cause and the crash. Exceeding the maximum number is a bug in
some loop's bounds I would say, not an omission of cached value
initialization. In particular I see in do_write_psr_msrs()

        for ( j = 0; j < cos_num; j++, index++ )
        {
            if ( feat->cos_reg_val[cos * cos_num + j] != info->val[index] )
            {
                feat->cos_reg_val[cos * cos_num + j] = info->val[index];
                props->write_msr(cos, info->val[index], props->type[j]);
            }
        }

Afaict the makes clear that values found in ->cos_reg_val[] would
never get written out (which fits it being just a cache). If
anything, a "random" match of the cache value and the two be
written value would _prevent_ an MSR write despite potentially
the MSR in fact currently holding a different value.

Nevertheless a few remarks on the patch itself, just in case
it's just the description that has misguided me.

> --- 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.

> @@ -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 ...

> +            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.

Jan
Yi Sun Nov. 28, 2019, 8:17 a.m. UTC | #2 
On 19-11-27 11:06:49, Jan Beulich wrote:
> On 27.11.2019 07:24, Yi Sun wrote:
> > During test, we found a crash on Xen with below trace.
> > (XEN) Xen call trace:
> > (XEN)    [<ffff82d0802a065a>] R psr.c#l3_cdp_write_msr+0x1e/0x22
> > (XEN)    [<ffff82d0802a0858>] F psr.c#do_write_psr_msrs+0x6d/0x109
> > (XEN)    [<ffff82d08023e000>] F smp_call_function_interrupt+0x5a/0xac
> > (XEN)    [<ffff82d0802a2b89>] F call_function_interrupt+0x20/0x34
> > (XEN)    [<ffff82d080282c64>] F do_IRQ+0x175/0x6ae
> > (XEN)    [<ffff82d08038b8ba>] F common_interrupt+0x10a/0x120
> > (XEN)    [<ffff82d0802ec616>] F cpu_idle.c#acpi_idle_do_entry+0x9d/0xb1
> > (XEN)    [<ffff82d0802ecc01>] F cpu_idle.c#acpi_processor_idle+0x41d/0x626
> > (XEN)    [<ffff82d08027353b>] F domain.c#idle_loop+0xa5/0xa7
> > (XEN)
> > (XEN)
> > (XEN) ****************************************
> > (XEN) Panic on CPU 20:
> > (XEN) GENERAL PROTECTION FAULT
> > (XEN) [error_code=0000]
> > (XEN) ****************************************
> > 
> > Root cause is that the cache of COS registers are not initialized
> > for CAT/CDP which have non-zero default value. That causes invalid
> > write to MSR when COS id has exceeded the max number.. So fix it by
> > initializing the cache.
> 
> I'm struggling with this description, first and foremost because
> there's no (recognizable to me) connection between the supposed
> root cause and the crash. Exceeding the maximum number is a bug in
> some loop's bounds I would say, not an omission of cached value
> initialization. In particular I see in do_write_psr_msrs()
> 
>         for ( j = 0; j < cos_num; j++, index++ )
>         {
>             if ( feat->cos_reg_val[cos * cos_num + j] != info->val[index] )
>             {
>                 feat->cos_reg_val[cos * cos_num + j] = info->val[index];
>                 props->write_msr(cos, info->val[index], props->type[j]);
>             }
>         }
> 
Sorry, my description is not clear. The bug happens when CDP and MBA
co-exist and MBA COS_MAX is bigger than CDP COS_MAX. E.g. MBA has 8
COS registers but CDP only have 6. When setting MBA throttling value
for the 7th guest, the value array would be:
    +------------------+------------------+--------------+
    | Data default val | Code default val | MBA throttle |
    +------------------+------------------+--------------+

We should avoid writting CDP data/code valules to MSR here. This should
be prevented by:
    if ( feat->cos_reg_val[cos * cos_num + j] != info->val[index] )

But the whole cos_reg_val[] is not initialized to default value so that
the check cannot prevent default value setting.

> Afaict the makes clear that values found in ->cos_reg_val[] would
> never get written out (which fits it being just a cache). If
> anything, a "random" match of the cache value and the two be
> written value would _prevent_ an MSR write despite potentially
> the MSR in fact currently holding a different value.
> 
> Nevertheless a few remarks on the patch itself, just in case
> it's just the description that has misguided me.
> 
> > --- 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.

> > @@ -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!

> Jan
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
Jan Beulich Nov. 28, 2019, 10:01 a.m. UTC | #3 
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
diff mbox series

Patch

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 5866a26..d3e7467 100644
--- 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;
 
     /* No valid value so do not enable feature. */
     if ( !regs->a || !regs->d )
@@ -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++)
+            feat->cos_reg_val[i] = cat_default_val(feat->cat.cbm_len);
 
         wrmsrl((type == FEAT_TYPE_L3_CAT ?
                 MSR_IA32_PSR_L3_MASK(0) :
@@ -352,8 +354,11 @@  static bool cat_init_feature(const struct cpuid_leaf *regs,
         feat->cos_max = (feat->cos_max - 1) >> 1;
 
         /* We reserve cos=0 as default cbm (all bits within cbm_len are 1). */
-        get_cdp_code(feat, 0) = cat_default_val(feat->cat.cbm_len);
-        get_cdp_data(feat, 0) = cat_default_val(feat->cat.cbm_len);
+        for(i = 0; i < MAX_COS_REG_CNT/2; i++)
+        {
+            get_cdp_code(feat, i) = cat_default_val(feat->cat.cbm_len);
+            get_cdp_data(feat, i) = cat_default_val(feat->cat.cbm_len);
+        }
 
         wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cat.cbm_len));
         wrmsrl(MSR_IA32_PSR_L3_MASK(1), cat_default_val(feat->cat.cbm_len));