[PATCH for-4.21??? 0/3] x86/vLAPIC: CMCI LVT handling

[PATCH for-4.21??? 0/3] x86/vLAPIC: CMCI LVT handling

[Jan Beulich]

1: add indirection to LVT handling
2: drop VLAPIC_VERSION
3: properly support the CMCI LVT

Now having this coded up, I realize it may be too intrusive at this point of
the release cycle. Still I wanted to propose it ...

Jan

[PATCH for-4.21??? 1/3] x86/vLAPIC: add indirection to LVT handling

[Jan Beulich]

In preparation to add support for the CMCI LVT, which is discontiguous to
the other LVTs, add a level of indirection. Rename the prior
vlapic_lvt_mask[] while doing so (as subsequently a 2nd array will want
adding, for use by guest_wrmsr_x2apic()).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The new name (lvt_valid[]) reflects its present contents. When re-based on
top of "x86/hvm: vlapic: fix RO bits emulation in LVTx regs", the name
wants to change to lvt_writable[] (or the 2nd array be added right away,
with lvt_valid[] then used by guest_wrmsr_x2apic()). Alternatively the
order of patches may want changing.

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -32,7 +32,16 @@
 #include <public/hvm/params.h>
 
 #define VLAPIC_VERSION                  0x00050014
-#define VLAPIC_LVT_NUM                  6
+#define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)
+
+#define LVTS \
+    LVT(LVTT), LVT(LVTTHMR), LVT(LVTPC), LVT(LVT0), LVT(LVT1), LVT(LVTERR),
+
+static const unsigned int lvt_reg[] = {
+#define LVT(which) APIC_ ## which
+    LVTS
+#undef LVT
+};
 
 #define LVT_MASK \
     (APIC_LVT_MASKED | APIC_SEND_PENDING | APIC_VECTOR_MASK)
@@ -41,20 +50,21 @@
     (LVT_MASK | APIC_DM_MASK | APIC_INPUT_POLARITY |\
     APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
 
-static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
+static const unsigned int lvt_valid[] =
 {
-     /* LVTT */
-     LVT_MASK | APIC_TIMER_MODE_MASK,
-     /* LVTTHMR */
-     LVT_MASK | APIC_DM_MASK,
-     /* LVTPC */
-     LVT_MASK | APIC_DM_MASK,
-     /* LVT0-1 */
-     LINT_MASK, LINT_MASK,
-     /* LVTERR */
-     LVT_MASK
+#define LVTT_VALID    (LVT_MASK | APIC_TIMER_MODE_MASK)
+#define LVTTHMR_VALID (LVT_MASK | APIC_DM_MASK)
+#define LVTPC_VALID   (LVT_MASK | APIC_DM_MASK)
+#define LVT0_VALID    LINT_MASK
+#define LVT1_VALID    LINT_MASK
+#define LVTERR_VALID  LVT_MASK
+#define LVT(which)    [LVT_BIAS(APIC_ ## which)] = which ## _VALID
+    LVTS
+#undef LVT
 };
 
+#undef LVTS
+
 #define vlapic_lvtt_period(vlapic)                              \
     ((vlapic_get_reg(vlapic, APIC_LVTT) & APIC_TIMER_MODE_MASK) \
      == APIC_TIMER_MODE_PERIODIC)
@@ -827,16 +837,16 @@ void vlapic_reg_write(struct vcpu *v, un
 
         if ( !(val & APIC_SPIV_APIC_ENABLED) )
         {
-            int i;
+            unsigned int i,
+                nr = GET_APIC_MAXLVT(vlapic_get_reg(vlapic, APIC_LVR)) + 1;
             uint32_t lvt_val;
 
             vlapic->hw.disabled |= VLAPIC_SW_DISABLED;
 
-            for ( i = 0; i < VLAPIC_LVT_NUM; i++ )
+            for ( i = 0; i < nr; i++ )
             {
-                lvt_val = vlapic_get_reg(vlapic, APIC_LVTT + 0x10 * i);
-                vlapic_set_reg(vlapic, APIC_LVTT + 0x10 * i,
-                               lvt_val | APIC_LVT_MASKED);
+                lvt_val = vlapic_get_reg(vlapic, lvt_reg[i]);
+                vlapic_set_reg(vlapic, lvt_reg[i], lvt_val | APIC_LVT_MASKED);
             }
         }
         else
@@ -878,7 +888,7 @@ void vlapic_reg_write(struct vcpu *v, un
     case APIC_LVTERR:       /* LVT Error Reg */
         if ( vlapic_sw_disabled(vlapic) )
             val |= APIC_LVT_MASKED;
-        val &= array_access_nospec(vlapic_lvt_mask, (reg - APIC_LVTT) >> 4);
+        val &= array_access_nospec(lvt_valid, LVT_BIAS(reg));
         vlapic_set_reg(vlapic, reg, val);
         if ( reg == APIC_LVT0 )
         {
@@ -1424,7 +1434,7 @@ bool is_vlapic_lvtpc_enabled(struct vlap
 /* Reset the VLAPIC back to its init state. */
 static void vlapic_do_init(struct vlapic *vlapic)
 {
-    int i;
+    unsigned int i, nr;
 
     if ( !has_vlapic(vlapic_vcpu(vlapic)->domain) )
         return;
@@ -1452,8 +1462,9 @@ static void vlapic_do_init(struct vlapic
 
     vlapic_set_reg(vlapic, APIC_DFR, 0xffffffffU);
 
-    for ( i = 0; i < VLAPIC_LVT_NUM; i++ )
-        vlapic_set_reg(vlapic, APIC_LVTT + 0x10 * i, APIC_LVT_MASKED);
+    nr = GET_APIC_MAXLVT(vlapic_get_reg(vlapic, APIC_LVR)) + 1;
+    for ( i = 0; i < nr; i++ )
+        vlapic_set_reg(vlapic, lvt_reg[i], APIC_LVT_MASKED);
 
     vlapic_set_reg(vlapic, APIC_SPIV, 0xff);
     vlapic->hw.disabled |= VLAPIC_SW_DISABLED;

Re: [PATCH for-4.21??? 1/3] x86/vLAPIC: add indirection to LVT handling

[Andrew Cooper]

On 08/10/2025 1:08 pm, Jan Beulich wrote:
> In preparation to add support for the CMCI LVT, which is discontiguous to
> the other LVTs, add a level of indirection.

It's not the only extra LVT.

AMD have Extended LVTs, which are necessary if we want to get virt-PMU
working.

https://sandpile.org/x86/apic.htm is a recent addition which covers all
of this.

>  Rename the prior
> vlapic_lvt_mask[] while doing so (as subsequently a 2nd array will want
> adding, for use by guest_wrmsr_x2apic()).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I'm afraid this introduces a vulnerability.

APIC_LVR is a toolstack-provided value.  Nothing bounds checks the
MAX_LVT value in it AFAICT, and previously this did not matter (from a
security point of view at least) because the loop bounds were constant.

~Andrew

Re: [PATCH for-4.21??? 1/3] x86/vLAPIC: add indirection to LVT handling

[Jan Beulich]

On 08.10.2025 15:04, Andrew Cooper wrote:
> On 08/10/2025 1:08 pm, Jan Beulich wrote:
>> In preparation to add support for the CMCI LVT, which is discontiguous to
>> the other LVTs, add a level of indirection.
> 
> It's not the only extra LVT.
> 
> AMD have Extended LVTs, which are necessary if we want to get virt-PMU
> working.
> 
> https://sandpile.org/x86/apic.htm is a recent addition which covers all
> of this.

Hmm, yes, but these will need taking care of separately anyway.

>>  Rename the prior
>> vlapic_lvt_mask[] while doing so (as subsequently a 2nd array will want
>> adding, for use by guest_wrmsr_x2apic()).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I'm afraid this introduces a vulnerability.
> 
> APIC_LVR is a toolstack-provided value.  Nothing bounds checks the
> MAX_LVT value in it AFAICT, and previously this did not matter (from a
> security point of view at least) because the loop bounds were constant.

Oh, right, I should have thought of that. As you don't suggest anything,
I'm going to simply add a check that the incoming value matches the one
that's there already. There will still be a quirk due to MCG_CAP and
APIC registers being loaded separately, with no defined ordering between
them. Within our current infrastructure I don't really see how to deal
with this kind of inter-dependency.

Jan

Re: [PATCH for-4.21??? 1/3] x86/vLAPIC: add indirection to LVT handling

[Jan Beulich]

On 08.10.2025 16:05, Jan Beulich wrote:
> On 08.10.2025 15:04, Andrew Cooper wrote:
>> I'm afraid this introduces a vulnerability.
>>
>> APIC_LVR is a toolstack-provided value.  Nothing bounds checks the
>> MAX_LVT value in it AFAICT, and previously this did not matter (from a
>> security point of view at least) because the loop bounds were constant.
> 
> Oh, right, I should have thought of that. As you don't suggest anything,
> I'm going to simply add a check that the incoming value matches the one
> that's there already.

Actually - no, that won't fly. We just need to bounds-check MAXLVT.

Jan

Re: [PATCH for-4.21??? 1/3] x86/vLAPIC: add indirection to LVT handling

[Grygorii Strashko]

Hi Jan,

On 08.10.25 15:08, Jan Beulich wrote:
> In preparation to add support for the CMCI LVT, which is discontiguous to
> the other LVTs, add a level of indirection. Rename the prior
> vlapic_lvt_mask[] while doing so (as subsequently a 2nd array will want
> adding, for use by guest_wrmsr_x2apic()).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The new name (lvt_valid[]) reflects its present contents. When re-based on
> top of "x86/hvm: vlapic: fix RO bits emulation in LVTx regs", the name
> wants to change to lvt_writable[] (or the 2nd array be added right away,
> with lvt_valid[] then used by guest_wrmsr_x2apic()). Alternatively the
> order of patches may want changing.
> 
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -32,7 +32,16 @@
>   #include <public/hvm/params.h>
>   
>   #define VLAPIC_VERSION                  0x00050014
> -#define VLAPIC_LVT_NUM                  6
> +#define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)

LVT_REG_IDX is more meaningful.

> +
> +#define LVTS \
> +    LVT(LVTT), LVT(LVTTHMR), LVT(LVTPC), LVT(LVT0), LVT(LVT1), LVT(LVTERR),
> +
> +static const unsigned int lvt_reg[] = {

this is going to be used by vlapic_get_reg()/vlapic_set_reg()
which both accept "uint32_t reg", so wouldn't it be reasonable
to use "uint32_t" here too.

> +#define LVT(which) APIC_ ## which
> +    LVTS
> +#undef LVT
> +};
>   
>   #define LVT_MASK \
>       (APIC_LVT_MASKED | APIC_SEND_PENDING | APIC_VECTOR_MASK)
> @@ -41,20 +50,21 @@
>       (LVT_MASK | APIC_DM_MASK | APIC_INPUT_POLARITY |\
>       APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>   
> -static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
> +static const unsigned int lvt_valid[] =
>   {
> -     /* LVTT */
> -     LVT_MASK | APIC_TIMER_MODE_MASK,
> -     /* LVTTHMR */
> -     LVT_MASK | APIC_DM_MASK,
> -     /* LVTPC */
> -     LVT_MASK | APIC_DM_MASK,
> -     /* LVT0-1 */
> -     LINT_MASK, LINT_MASK,
> -     /* LVTERR */
> -     LVT_MASK
> +#define LVTT_VALID    (LVT_MASK | APIC_TIMER_MODE_MASK)
> +#define LVTTHMR_VALID (LVT_MASK | APIC_DM_MASK)
> +#define LVTPC_VALID   (LVT_MASK | APIC_DM_MASK)
> +#define LVT0_VALID    LINT_MASK
> +#define LVT1_VALID    LINT_MASK
> +#define LVTERR_VALID  LVT_MASK
> +#define LVT(which)    [LVT_BIAS(APIC_ ## which)] = which ## _VALID
> +    LVTS
> +#undef LVT
>   };
>   
> +#undef LVTS
> +

I know people have different coding style/approaches...
But using self expanding macro-magic in this particular case is over-kill
- it breaks grep (grep APIC_LVTT will not give all occurrences)
- it complicates code analyzes and readability
    - What is array size?
    - Which array elements actually initialized?
    - what is the actual element's values?
- in this particular case - no benefits in terms of code lines.

It might be reasonable if there would be few dozen of regs (or more),
but there are only 6(7) and HW spec is old and stable.

So could there just be:
static const unsigned int lvt_reg[] = {
  APIC_LVTT,
  APIC_LVTTHMR
  ...

and

static const unsigned int lvt_valid[] = {
  [LVT_REG_IDX(APIC_LVTT)] = (LVT_MASK | APIC_TIMER_MODE_MASK),
  [LVT_REG_IDX(APIC_LVTTHMR)] = (LVT_MASK | APIC_DM_MASK),
  ..

Just fast look at above code gives all info without need to parse all
these recursive macro.

>   #define vlapic_lvtt_period(vlapic)                              \
>       ((vlapic_get_reg(vlapic, APIC_LVTT) & APIC_TIMER_MODE_MASK) \
>        == APIC_TIMER_MODE_PERIODIC)
> @@ -827,16 +837,16 @@ void vlapic_reg_write(struct vcpu *v, un
>   
>           if ( !(val & APIC_SPIV_APIC_ENABLED) )
>           {
> -            int i;
> +            unsigned int i,

uint32_t?

> +                nr = GET_APIC_MAXLVT(vlapic_get_reg(vlapic, APIC_LVR)) + 1;

This deserves wrapper (may be static inline)
Defining multiple vars on the same line makes code less readable as for me.

>               uint32_t lvt_val;
>   
>               vlapic->hw.disabled |= VLAPIC_SW_DISABLED;
>   
> -            for ( i = 0; i < VLAPIC_LVT_NUM; i++ )
> +            for ( i = 0; i < nr; i++ )
>               {
> -                lvt_val = vlapic_get_reg(vlapic, APIC_LVTT + 0x10 * i);
> -                vlapic_set_reg(vlapic, APIC_LVTT + 0x10 * i,
> -                               lvt_val | APIC_LVT_MASKED);
> +                lvt_val = vlapic_get_reg(vlapic, lvt_reg[i]);
> +                vlapic_set_reg(vlapic, lvt_reg[i], lvt_val | APIC_LVT_MASKED);
>               }
>           }
>           else
> @@ -878,7 +888,7 @@ void vlapic_reg_write(struct vcpu *v, un
>       case APIC_LVTERR:       /* LVT Error Reg */
>           if ( vlapic_sw_disabled(vlapic) )
>               val |= APIC_LVT_MASKED;
> -        val &= array_access_nospec(vlapic_lvt_mask, (reg - APIC_LVTT) >> 4);
> +        val &= array_access_nospec(lvt_valid, LVT_BIAS(reg));
>           vlapic_set_reg(vlapic, reg, val);
>           if ( reg == APIC_LVT0 )
>           {
> @@ -1424,7 +1434,7 @@ bool is_vlapic_lvtpc_enabled(struct vlap
>   /* Reset the VLAPIC back to its init state. */
>   static void vlapic_do_init(struct vlapic *vlapic)
>   {
> -    int i;
> +    unsigned int i, nr;

uint32_t?

>   
>       if ( !has_vlapic(vlapic_vcpu(vlapic)->domain) )
>           return;
> @@ -1452,8 +1462,9 @@ static void vlapic_do_init(struct vlapic
>   
>       vlapic_set_reg(vlapic, APIC_DFR, 0xffffffffU);
>   
> -    for ( i = 0; i < VLAPIC_LVT_NUM; i++ )
> -        vlapic_set_reg(vlapic, APIC_LVTT + 0x10 * i, APIC_LVT_MASKED);
> +    nr = GET_APIC_MAXLVT(vlapic_get_reg(vlapic, APIC_LVR)) + 1;
> +    for ( i = 0; i < nr; i++ )
> +        vlapic_set_reg(vlapic, lvt_reg[i], APIC_LVT_MASKED);
>   
>       vlapic_set_reg(vlapic, APIC_SPIV, 0xff);
>       vlapic->hw.disabled |= VLAPIC_SW_DISABLED;
> 

-- 
Best regards,
-grygorii

Re: [PATCH for-4.21??? 1/3] x86/vLAPIC: add indirection to LVT handling

[Jan Beulich]

On 09.10.2025 16:56, Grygorii Strashko wrote:
> On 08.10.25 15:08, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -32,7 +32,16 @@
>>   #include <public/hvm/params.h>
>>   
>>   #define VLAPIC_VERSION                  0x00050014
>> -#define VLAPIC_LVT_NUM                  6
>> +#define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)
> 
> LVT_REG_IDX is more meaningful.

Not to me. I don't like LVT_BIAS() very much as a name, but if anything I'd
want to replace it by something clearly better (and unambiguous).

>> +
>> +#define LVTS \
>> +    LVT(LVTT), LVT(LVTTHMR), LVT(LVTPC), LVT(LVT0), LVT(LVT1), LVT(LVTERR),
>> +
>> +static const unsigned int lvt_reg[] = {
> 
> this is going to be used by vlapic_get_reg()/vlapic_set_reg()
> which both accept "uint32_t reg", so wouldn't it be reasonable
> to use "uint32_t" here too.

Possible, but against ./CODING_STYLE (applies to your other uint32_t remarks,
too).

>> @@ -41,20 +50,21 @@
>>       (LVT_MASK | APIC_DM_MASK | APIC_INPUT_POLARITY |\
>>       APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>>   
>> -static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
>> +static const unsigned int lvt_valid[] =
>>   {
>> -     /* LVTT */
>> -     LVT_MASK | APIC_TIMER_MODE_MASK,
>> -     /* LVTTHMR */
>> -     LVT_MASK | APIC_DM_MASK,
>> -     /* LVTPC */
>> -     LVT_MASK | APIC_DM_MASK,
>> -     /* LVT0-1 */
>> -     LINT_MASK, LINT_MASK,
>> -     /* LVTERR */
>> -     LVT_MASK
>> +#define LVTT_VALID    (LVT_MASK | APIC_TIMER_MODE_MASK)
>> +#define LVTTHMR_VALID (LVT_MASK | APIC_DM_MASK)
>> +#define LVTPC_VALID   (LVT_MASK | APIC_DM_MASK)
>> +#define LVT0_VALID    LINT_MASK
>> +#define LVT1_VALID    LINT_MASK
>> +#define LVTERR_VALID  LVT_MASK
>> +#define LVT(which)    [LVT_BIAS(APIC_ ## which)] = which ## _VALID
>> +    LVTS
>> +#undef LVT
>>   };
>>   
>> +#undef LVTS
>> +
> 
> I know people have different coding style/approaches...
> But using self expanding macro-magic in this particular case is over-kill
> - it breaks grep (grep APIC_LVTT will not give all occurrences)
> - it complicates code analyzes and readability
>     - What is array size?
>     - Which array elements actually initialized?
>     - what is the actual element's values?
> - in this particular case - no benefits in terms of code lines.
> 
> It might be reasonable if there would be few dozen of regs (or more),
> but there are only 6(7) and HW spec is old and stable.
> 
> So could there just be:
> static const unsigned int lvt_reg[] = {
>   APIC_LVTT,
>   APIC_LVTTHMR
>   ...
> 
> and
> 
> static const unsigned int lvt_valid[] = {
>   [LVT_REG_IDX(APIC_LVTT)] = (LVT_MASK | APIC_TIMER_MODE_MASK),
>   [LVT_REG_IDX(APIC_LVTTHMR)] = (LVT_MASK | APIC_DM_MASK),
>   ..
> 
> Just fast look at above code gives all info without need to parse all
> these recursive macro.

And with no guarantee at all that the order of entries remains in sync
between all (two now, three later) uses.

>>   #define vlapic_lvtt_period(vlapic)                              \
>>       ((vlapic_get_reg(vlapic, APIC_LVTT) & APIC_TIMER_MODE_MASK) \
>>        == APIC_TIMER_MODE_PERIODIC)
>> @@ -827,16 +837,16 @@ void vlapic_reg_write(struct vcpu *v, un
>>   
>>           if ( !(val & APIC_SPIV_APIC_ENABLED) )
>>           {
>> -            int i;
>> +            unsigned int i,
> 
> uint32_t?
> 
>> +                nr = GET_APIC_MAXLVT(vlapic_get_reg(vlapic, APIC_LVR)) + 1;
> 
> This deserves wrapper (may be static inline)
> Defining multiple vars on the same line makes code less readable as for me.

I don't see multiple variables being defined on this line.

Jan

Re: [PATCH for-4.21??? 1/3] x86/vLAPIC: add indirection to LVT handling

[Grygorii Strashko]

On 09.10.25 18:31, Jan Beulich wrote:
> On 09.10.2025 16:56, Grygorii Strashko wrote:
>> On 08.10.25 15:08, Jan Beulich wrote:
>>> --- a/xen/arch/x86/hvm/vlapic.c
>>> +++ b/xen/arch/x86/hvm/vlapic.c
>>> @@ -32,7 +32,16 @@
>>>    #include <public/hvm/params.h>
>>>    
>>>    #define VLAPIC_VERSION                  0x00050014
>>> -#define VLAPIC_LVT_NUM                  6
>>> +#define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)
>>
>> LVT_REG_IDX is more meaningful.
> 
> Not to me. I don't like LVT_BIAS() very much as a name, but if anything I'd
> want to replace it by something clearly better (and unambiguous).
> 
>>> +
>>> +#define LVTS \
>>> +    LVT(LVTT), LVT(LVTTHMR), LVT(LVTPC), LVT(LVT0), LVT(LVT1), LVT(LVTERR),
>>> +
>>> +static const unsigned int lvt_reg[] = {
>>
>> this is going to be used by vlapic_get_reg()/vlapic_set_reg()
>> which both accept "uint32_t reg", so wouldn't it be reasonable
>> to use "uint32_t" here too.
> 
> Possible, but against ./CODING_STYLE (applies to your other uint32_t remarks,
> too).
> 
>>> @@ -41,20 +50,21 @@
>>>        (LVT_MASK | APIC_DM_MASK | APIC_INPUT_POLARITY |\
>>>        APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>>>    
>>> -static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
>>> +static const unsigned int lvt_valid[] =
>>>    {
>>> -     /* LVTT */
>>> -     LVT_MASK | APIC_TIMER_MODE_MASK,
>>> -     /* LVTTHMR */
>>> -     LVT_MASK | APIC_DM_MASK,
>>> -     /* LVTPC */
>>> -     LVT_MASK | APIC_DM_MASK,
>>> -     /* LVT0-1 */
>>> -     LINT_MASK, LINT_MASK,
>>> -     /* LVTERR */
>>> -     LVT_MASK
>>> +#define LVTT_VALID    (LVT_MASK | APIC_TIMER_MODE_MASK)
>>> +#define LVTTHMR_VALID (LVT_MASK | APIC_DM_MASK)
>>> +#define LVTPC_VALID   (LVT_MASK | APIC_DM_MASK)
>>> +#define LVT0_VALID    LINT_MASK
>>> +#define LVT1_VALID    LINT_MASK
>>> +#define LVTERR_VALID  LVT_MASK
>>> +#define LVT(which)    [LVT_BIAS(APIC_ ## which)] = which ## _VALID
>>> +    LVTS
>>> +#undef LVT
>>>    };
>>>    
>>> +#undef LVTS
>>> +
>>
>> I know people have different coding style/approaches...
>> But using self expanding macro-magic in this particular case is over-kill
>> - it breaks grep (grep APIC_LVTT will not give all occurrences)
>> - it complicates code analyzes and readability
>>      - What is array size?
>>      - Which array elements actually initialized?
>>      - what is the actual element's values?
>> - in this particular case - no benefits in terms of code lines.
>>
>> It might be reasonable if there would be few dozen of regs (or more),
>> but there are only 6(7) and HW spec is old and stable.
>>
>> So could there just be:
>> static const unsigned int lvt_reg[] = {
>>    APIC_LVTT,
>>    APIC_LVTTHMR
>>    ...
>>
>> and
>>
>> static const unsigned int lvt_valid[] = {
>>    [LVT_REG_IDX(APIC_LVTT)] = (LVT_MASK | APIC_TIMER_MODE_MASK),
>>    [LVT_REG_IDX(APIC_LVTTHMR)] = (LVT_MASK | APIC_DM_MASK),
>>    ..
>>
>> Just fast look at above code gives all info without need to parse all
>> these recursive macro.
> 
> And with no guarantee at all that the order of entries remains in sync
> between all (two now, three later) uses.

The order in lvt_x_masks[] arrays are guaranteed by "[x] = y,".

Comparing or syncing lvt_reg[] array with with lvt_x_masks[]
would not be exactly correct as they are used in a different way and
have different sizes (after patch 3).

if somebody decide to add AMD Extended LVTs which have offsets 500-530h
then lvt_x_masks[] will grow even more and will contain more unused wholes.

> 
>>>    #define vlapic_lvtt_period(vlapic)                              \
>>>        ((vlapic_get_reg(vlapic, APIC_LVTT) & APIC_TIMER_MODE_MASK) \
>>>         == APIC_TIMER_MODE_PERIODIC)
>>> @@ -827,16 +837,16 @@ void vlapic_reg_write(struct vcpu *v, un
>>>    
>>>            if ( !(val & APIC_SPIV_APIC_ENABLED) )
>>>            {
>>> -            int i;
>>> +            unsigned int i,
>>
>> uint32_t?
>>
>>> +                nr = GET_APIC_MAXLVT(vlapic_get_reg(vlapic, APIC_LVR)) + 1;
>>
>> This deserves wrapper (may be static inline)
>> Defining multiple vars on the same line makes code less readable as for me.
> 
> I don't see multiple variables being defined on this line.

     unsigned int i;
     unsigned int nr = GET_APIC_MAXLVT(vlapic_get_reg(vlapic, APIC_LVR)) + 1;


-- 
Best regards,
-grygorii

Re: [PATCH for-4.21??? 1/3] x86/vLAPIC: add indirection to LVT handling

[Jan Beulich]

On 09.10.2025 18:16, Grygorii Strashko wrote:
> On 09.10.25 18:31, Jan Beulich wrote:
>> On 09.10.2025 16:56, Grygorii Strashko wrote:
>>> On 08.10.25 15:08, Jan Beulich wrote:
>>>> @@ -41,20 +50,21 @@
>>>>        (LVT_MASK | APIC_DM_MASK | APIC_INPUT_POLARITY |\
>>>>        APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>>>>    -static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
>>>> +static const unsigned int lvt_valid[] =
>>>>    {
>>>> -     /* LVTT */
>>>> -     LVT_MASK | APIC_TIMER_MODE_MASK,
>>>> -     /* LVTTHMR */
>>>> -     LVT_MASK | APIC_DM_MASK,
>>>> -     /* LVTPC */
>>>> -     LVT_MASK | APIC_DM_MASK,
>>>> -     /* LVT0-1 */
>>>> -     LINT_MASK, LINT_MASK,
>>>> -     /* LVTERR */
>>>> -     LVT_MASK
>>>> +#define LVTT_VALID    (LVT_MASK | APIC_TIMER_MODE_MASK)
>>>> +#define LVTTHMR_VALID (LVT_MASK | APIC_DM_MASK)
>>>> +#define LVTPC_VALID   (LVT_MASK | APIC_DM_MASK)
>>>> +#define LVT0_VALID    LINT_MASK
>>>> +#define LVT1_VALID    LINT_MASK
>>>> +#define LVTERR_VALID  LVT_MASK
>>>> +#define LVT(which)    [LVT_BIAS(APIC_ ## which)] = which ## _VALID
>>>> +    LVTS
>>>> +#undef LVT
>>>>    };
>>>>    +#undef LVTS
>>>> +
>>>
>>> I know people have different coding style/approaches...
>>> But using self expanding macro-magic in this particular case is over-kill
>>> - it breaks grep (grep APIC_LVTT will not give all occurrences)
>>> - it complicates code analyzes and readability
>>>      - What is array size?
>>>      - Which array elements actually initialized?
>>>      - what is the actual element's values?
>>> - in this particular case - no benefits in terms of code lines.
>>>
>>> It might be reasonable if there would be few dozen of regs (or more),
>>> but there are only 6(7) and HW spec is old and stable.
>>>
>>> So could there just be:
>>> static const unsigned int lvt_reg[] = {
>>>    APIC_LVTT,
>>>    APIC_LVTTHMR
>>>    ...
>>>
>>> and
>>>
>>> static const unsigned int lvt_valid[] = {
>>>    [LVT_REG_IDX(APIC_LVTT)] = (LVT_MASK | APIC_TIMER_MODE_MASK),
>>>    [LVT_REG_IDX(APIC_LVTTHMR)] = (LVT_MASK | APIC_DM_MASK),
>>>    ..
>>>
>>> Just fast look at above code gives all info without need to parse all
>>> these recursive macro.
>>
>> And with no guarantee at all that the order of entries remains in sync
>> between all (two now, three later) uses.
> 
> The order in lvt_x_masks[] arrays are guaranteed by "[x] = y,".

Hmm, yes, sorry - not sure what I was thinking. What then remains is a
readability concern towards the longish lines you propose. I'll have to
think about it some more.

> Comparing or syncing lvt_reg[] array with with lvt_x_masks[]
> would not be exactly correct as they are used in a different way and
> have different sizes (after patch 3).
> 
> if somebody decide to add AMD Extended LVTs which have offsets 500-530h
> then lvt_x_masks[] will grow even more and will contain more unused wholes.

Yes, but (a) what do you do (of course a solution can be found, but
likely at the expense of adding yet another layer of indirection) and
(b) there are other (harder?) problems to be sorted for supporting
them.

>>>>    #define vlapic_lvtt_period(vlapic)                              \
>>>>        ((vlapic_get_reg(vlapic, APIC_LVTT) & APIC_TIMER_MODE_MASK) \
>>>>         == APIC_TIMER_MODE_PERIODIC)
>>>> @@ -827,16 +837,16 @@ void vlapic_reg_write(struct vcpu *v, un
>>>>               if ( !(val & APIC_SPIV_APIC_ENABLED) )
>>>>            {
>>>> -            int i;
>>>> +            unsigned int i,
>>>
>>> uint32_t?
>>>
>>>> +                nr = GET_APIC_MAXLVT(vlapic_get_reg(vlapic, APIC_LVR)) + 1;
>>>
>>> This deserves wrapper (may be static inline)
>>> Defining multiple vars on the same line makes code less readable as for me.
>>
>> I don't see multiple variables being defined on this line.
> 
>     unsigned int i;
>     unsigned int nr = GET_APIC_MAXLVT(vlapic_get_reg(vlapic, APIC_LVR)) + 1;

Hmm, I see now what you mean, but then my take is that your variant is
less readable (and too long a line afaict; once properly line-wrapped
it'll become more similar to what I had, I think).

Jan

Re: [PATCH for-4.21??? 1/3] x86/vLAPIC: add indirection to LVT handling

[Roger Pau Monné]

On Wed, Oct 08, 2025 at 02:08:26PM +0200, Jan Beulich wrote:
> In preparation to add support for the CMCI LVT, which is discontiguous to
> the other LVTs, add a level of indirection. Rename the prior
> vlapic_lvt_mask[] while doing so (as subsequently a 2nd array will want
> adding, for use by guest_wrmsr_x2apic()).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The new name (lvt_valid[]) reflects its present contents. When re-based on
> top of "x86/hvm: vlapic: fix RO bits emulation in LVTx regs", the name
> wants to change to lvt_writable[] (or the 2nd array be added right away,
> with lvt_valid[] then used by guest_wrmsr_x2apic()). Alternatively the
> order of patches may want changing.
> 
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -32,7 +32,16 @@
>  #include <public/hvm/params.h>
>  
>  #define VLAPIC_VERSION                  0x00050014
> -#define VLAPIC_LVT_NUM                  6
> +#define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)
> +
> +#define LVTS \
> +    LVT(LVTT), LVT(LVTTHMR), LVT(LVTPC), LVT(LVT0), LVT(LVT1), LVT(LVTERR),
> +
> +static const unsigned int lvt_reg[] = {
> +#define LVT(which) APIC_ ## which
> +    LVTS
> +#undef LVT
> +};
>  
>  #define LVT_MASK \
>      (APIC_LVT_MASKED | APIC_SEND_PENDING | APIC_VECTOR_MASK)
> @@ -41,20 +50,21 @@
>      (LVT_MASK | APIC_DM_MASK | APIC_INPUT_POLARITY |\
>      APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>  
> -static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
> +static const unsigned int lvt_valid[] =
>  {
> -     /* LVTT */
> -     LVT_MASK | APIC_TIMER_MODE_MASK,
> -     /* LVTTHMR */
> -     LVT_MASK | APIC_DM_MASK,
> -     /* LVTPC */
> -     LVT_MASK | APIC_DM_MASK,
> -     /* LVT0-1 */
> -     LINT_MASK, LINT_MASK,
> -     /* LVTERR */
> -     LVT_MASK
> +#define LVTT_VALID    (LVT_MASK | APIC_TIMER_MODE_MASK)
> +#define LVTTHMR_VALID (LVT_MASK | APIC_DM_MASK)
> +#define LVTPC_VALID   (LVT_MASK | APIC_DM_MASK)
> +#define LVT0_VALID    LINT_MASK
> +#define LVT1_VALID    LINT_MASK
> +#define LVTERR_VALID  LVT_MASK
> +#define LVT(which)    [LVT_BIAS(APIC_ ## which)] = which ## _VALID
> +    LVTS
> +#undef LVT
>  };
>  
> +#undef LVTS

I've been thinking about this, as I agree with Grygorii here that the
construct seems to complex.  What about using something like:

static const unsigned int lvt_regs[] = {
    APIC_LVTT, APIC_LVTTHMR, APIC_LVTPC, APIC_LVT0, APIC_LVT1, APIC_LVTERR,
};

static unsigned int lvt_valid(unsigned int reg)
{
    switch ( reg )
    {
    case APIC_LVTT:
        return LVT_MASK | APIC_TIMER_MODE_MASK;

    case APIC_LVTTHMR:
    case APIC_LVTPC:
        return LVT_MASK | APIC_DM_MASK;

    case APIC_LVT0:
    case APIC_LVT1:
        return LINT_MASK;

    case APIC_LVTERR:
        return LVT_MASK;
    }

    ASSERT_UNREACHABLE();
    return 0;
}

That uses a function instead of a directly indexed array, so it's
going to be slower.  I think the compiler will possibly inline it,
plus the clarity is worth the cost.

Thanks, Roger.

Re: [PATCH for-4.21??? 1/3] x86/vLAPIC: add indirection to LVT handling

[Jan Beulich]

On 10.10.2025 16:44, Roger Pau Monné wrote:
> On Wed, Oct 08, 2025 at 02:08:26PM +0200, Jan Beulich wrote:
>> In preparation to add support for the CMCI LVT, which is discontiguous to
>> the other LVTs, add a level of indirection. Rename the prior
>> vlapic_lvt_mask[] while doing so (as subsequently a 2nd array will want
>> adding, for use by guest_wrmsr_x2apic()).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> The new name (lvt_valid[]) reflects its present contents. When re-based on
>> top of "x86/hvm: vlapic: fix RO bits emulation in LVTx regs", the name
>> wants to change to lvt_writable[] (or the 2nd array be added right away,
>> with lvt_valid[] then used by guest_wrmsr_x2apic()). Alternatively the
>> order of patches may want changing.
>>
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -32,7 +32,16 @@
>>  #include <public/hvm/params.h>
>>  
>>  #define VLAPIC_VERSION                  0x00050014
>> -#define VLAPIC_LVT_NUM                  6
>> +#define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)
>> +
>> +#define LVTS \
>> +    LVT(LVTT), LVT(LVTTHMR), LVT(LVTPC), LVT(LVT0), LVT(LVT1), LVT(LVTERR),
>> +
>> +static const unsigned int lvt_reg[] = {
>> +#define LVT(which) APIC_ ## which
>> +    LVTS
>> +#undef LVT
>> +};
>>  
>>  #define LVT_MASK \
>>      (APIC_LVT_MASKED | APIC_SEND_PENDING | APIC_VECTOR_MASK)
>> @@ -41,20 +50,21 @@
>>      (LVT_MASK | APIC_DM_MASK | APIC_INPUT_POLARITY |\
>>      APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>>  
>> -static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
>> +static const unsigned int lvt_valid[] =
>>  {
>> -     /* LVTT */
>> -     LVT_MASK | APIC_TIMER_MODE_MASK,
>> -     /* LVTTHMR */
>> -     LVT_MASK | APIC_DM_MASK,
>> -     /* LVTPC */
>> -     LVT_MASK | APIC_DM_MASK,
>> -     /* LVT0-1 */
>> -     LINT_MASK, LINT_MASK,
>> -     /* LVTERR */
>> -     LVT_MASK
>> +#define LVTT_VALID    (LVT_MASK | APIC_TIMER_MODE_MASK)
>> +#define LVTTHMR_VALID (LVT_MASK | APIC_DM_MASK)
>> +#define LVTPC_VALID   (LVT_MASK | APIC_DM_MASK)
>> +#define LVT0_VALID    LINT_MASK
>> +#define LVT1_VALID    LINT_MASK
>> +#define LVTERR_VALID  LVT_MASK
>> +#define LVT(which)    [LVT_BIAS(APIC_ ## which)] = which ## _VALID
>> +    LVTS
>> +#undef LVT
>>  };
>>  
>> +#undef LVTS
> 
> I've been thinking about this, as I agree with Grygorii here that the
> construct seems to complex.  What about using something like:
> 
> static const unsigned int lvt_regs[] = {
>     APIC_LVTT, APIC_LVTTHMR, APIC_LVTPC, APIC_LVT0, APIC_LVT1, APIC_LVTERR,
> };
> 
> static unsigned int lvt_valid(unsigned int reg)
> {
>     switch ( reg )
>     {
>     case APIC_LVTT:
>         return LVT_MASK | APIC_TIMER_MODE_MASK;
> 
>     case APIC_LVTTHMR:
>     case APIC_LVTPC:
>         return LVT_MASK | APIC_DM_MASK;
> 
>     case APIC_LVT0:
>     case APIC_LVT1:
>         return LINT_MASK;
> 
>     case APIC_LVTERR:
>         return LVT_MASK;
>     }
> 
>     ASSERT_UNREACHABLE();
>     return 0;
> }
> 
> That uses a function instead of a directly indexed array, so it's
> going to be slower.  I think the compiler will possibly inline it,
> plus the clarity is worth the cost.

I don't agree; I see no clarity issue with the table approach. In fact I
view that one as more "clear". Instead of the above, if anything, I'd
be (somewhat reluctantly) willing to make the (currently follow-on)
change the other way around: Rather than switching guest_wrmsr_x2apic()
to a table-based approach as well, do away with the table-based approach
in vlapic_reg_write() by splitting the respective case blocks some more.
To limit redundancy, that may then involve the (imo undesirable) use of
"goto".

Jan

[PATCH for-4.21??? 2/3] x86/vLAPIC: drop VLAPIC_VERSION

[Jan Beulich]

In preparation of making the value somewhat dynamic drop the constant.
Replace its use in guest_wrmsr_x2apic() by actually fetching the LVR
value.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -31,7 +31,6 @@
 #include <public/hvm/ioreq.h>
 #include <public/hvm/params.h>
 
-#define VLAPIC_VERSION                  0x00050014
 #define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)
 
 #define LVTS \
@@ -1015,7 +1014,7 @@ int guest_wrmsr_x2apic(struct vcpu *v, u
     case APIC_SPIV:
         if ( val & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
                      APIC_SPIV_FOCUS_DISABLED |
-                     (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
+                     (vlapic_get_reg(vlapic, APIC_LVR) & APIC_LVR_DIRECTED_EOI
                       ? APIC_SPIV_DIRECTED_EOI : 0)) )
             return X86EMUL_EXCEPTION;
         break;
@@ -1439,7 +1438,7 @@ static void vlapic_do_init(struct vlapic
     if ( !has_vlapic(vlapic_vcpu(vlapic)->domain) )
         return;
 
-    vlapic_set_reg(vlapic, APIC_LVR, VLAPIC_VERSION);
+    vlapic_set_reg(vlapic, APIC_LVR, 0x00050014);
 
     for ( i = 0; i < 8; i++ )
     {

Re: [PATCH for-4.21??? 2/3] x86/vLAPIC: drop VLAPIC_VERSION

[Roger Pau Monné]

On Wed, Oct 08, 2025 at 02:08:48PM +0200, Jan Beulich wrote:
> In preparation of making the value somewhat dynamic drop the constant.
> Replace its use in guest_wrmsr_x2apic() by actually fetching the LVR
> value.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -31,7 +31,6 @@
>  #include <public/hvm/ioreq.h>
>  #include <public/hvm/params.h>
>  
> -#define VLAPIC_VERSION                  0x00050014
>  #define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)
>  
>  #define LVTS \
> @@ -1015,7 +1014,7 @@ int guest_wrmsr_x2apic(struct vcpu *v, u
>      case APIC_SPIV:
>          if ( val & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
>                       APIC_SPIV_FOCUS_DISABLED |
> -                     (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
> +                     (vlapic_get_reg(vlapic, APIC_LVR) & APIC_LVR_DIRECTED_EOI
>                        ? APIC_SPIV_DIRECTED_EOI : 0)) )
>              return X86EMUL_EXCEPTION;
>          break;
> @@ -1439,7 +1438,7 @@ static void vlapic_do_init(struct vlapic
>      if ( !has_vlapic(vlapic_vcpu(vlapic)->domain) )
>          return;
>  
> -    vlapic_set_reg(vlapic, APIC_LVR, VLAPIC_VERSION);
> +    vlapic_set_reg(vlapic, APIC_LVR, 0x00050014);

(Maybe I'm getting ahead of patch 3, as I haven't looked yet)

Don't we want to use some kind of macros to build this in a more
friendly way?

We could have a pair of SET_APIC_{VERSION,MAXLVT}()?

Thanks, Roger.

Re: [PATCH for-4.21??? 2/3] x86/vLAPIC: drop VLAPIC_VERSION

[Jan Beulich]

On 08.10.2025 18:23, Roger Pau Monné wrote:
> On Wed, Oct 08, 2025 at 02:08:48PM +0200, Jan Beulich wrote:
>> @@ -1439,7 +1438,7 @@ static void vlapic_do_init(struct vlapic
>>      if ( !has_vlapic(vlapic_vcpu(vlapic)->domain) )
>>          return;
>>  
>> -    vlapic_set_reg(vlapic, APIC_LVR, VLAPIC_VERSION);
>> +    vlapic_set_reg(vlapic, APIC_LVR, 0x00050014);
> 
> (Maybe I'm getting ahead of patch 3, as I haven't looked yet)
> 
> Don't we want to use some kind of macros to build this in a more
> friendly way?
> 
> We could have a pair of SET_APIC_{VERSION,MAXLVT}()?

With what patch 3 does to apicdef.h, I was rather wondering whether to simply
use two MASK_INSR() here (patch 3 already uses one right now).

Jan

Re: [PATCH for-4.21??? 2/3] x86/vLAPIC: drop VLAPIC_VERSION

[Roger Pau Monné]

On Thu, Oct 09, 2025 at 09:21:32AM +0200, Jan Beulich wrote:
> On 08.10.2025 18:23, Roger Pau Monné wrote:
> > On Wed, Oct 08, 2025 at 02:08:48PM +0200, Jan Beulich wrote:
> >> @@ -1439,7 +1438,7 @@ static void vlapic_do_init(struct vlapic
> >>      if ( !has_vlapic(vlapic_vcpu(vlapic)->domain) )
> >>          return;
> >>  
> >> -    vlapic_set_reg(vlapic, APIC_LVR, VLAPIC_VERSION);
> >> +    vlapic_set_reg(vlapic, APIC_LVR, 0x00050014);
> > 
> > (Maybe I'm getting ahead of patch 3, as I haven't looked yet)
> > 
> > Don't we want to use some kind of macros to build this in a more
> > friendly way?
> > 
> > We could have a pair of SET_APIC_{VERSION,MAXLVT}()?
> 
> With what patch 3 does to apicdef.h, I was rather wondering whether to simply
> use two MASK_INSR() here (patch 3 already uses one right now).

That would be fine for me.

Thanks, Roger.

[PATCH for-4.21??? 3/3] x86/vLAPIC: properly support the CMCI LVT

[Jan Beulich]

Rather than unconditionally accepting reads and writes while discarding
the value written, make accesses properly conditional upon CMCI being
exposed via MCG_CAP, and arrange to actually retain the value written.
Also reflect the extra LVT in LVR.

Note that this doesn't change the status quo of us never delivering any
interrupt through this LVT.

Fixes: 70173dbb9948 ("x86/HVM: fix miscellaneous aspects of x2APIC emulation")
Fixes: 8d0a20587e4e ("x86/hvm: further restrict access to x2apic MSRs")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The Fixes: tags are referencing where the explicit mentioning of APIC_CMCI
in what are now guest_{rd,wr}msr_x2apic() was introduced; the mis-handling
really pre-dates that, though.

In principle the later assignment to "nr" in vlapic_do_init() could now be
dropped again. I wasn't quite sure though whether that's a good idea.

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -31,10 +31,13 @@
 #include <public/hvm/ioreq.h>
 #include <public/hvm/params.h>
 
-#define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)
+#include <../cpu/mcheck/x86_mca.h> /* MCG_CMCI_P */
+
+#define LVT_BIAS(reg)                   (((reg) - APIC_CMCI) >> 4)
 
 #define LVTS \
-    LVT(LVTT), LVT(LVTTHMR), LVT(LVTPC), LVT(LVT0), LVT(LVT1), LVT(LVTERR),
+    LVT(LVTT), LVT(LVTTHMR), LVT(LVTPC), LVT(LVT0), LVT(LVT1), LVT(LVTERR), \
+    LVT(CMCI),
 
 static const unsigned int lvt_reg[] = {
 #define LVT(which) APIC_ ## which
@@ -57,6 +60,7 @@ static const unsigned int lvt_valid[] =
 #define LVT0_VALID    LINT_MASK
 #define LVT1_VALID    LINT_MASK
 #define LVTERR_VALID  LVT_MASK
+#define CMCI_VALID    (LVT_MASK | APIC_DM_MASK)
 #define LVT(which)    [LVT_BIAS(APIC_ ## which)] = which ## _VALID
     LVTS
 #undef LVT
@@ -697,8 +701,17 @@ int guest_rdmsr_x2apic(const struct vcpu
         return X86EMUL_EXCEPTION;
 
     offset = reg << 4;
-    if ( offset == APIC_ICR )
+    switch ( offset )
+    {
+    case APIC_ICR:
         high = (uint64_t)vlapic_read_aligned(vlapic, APIC_ICR2) << 32;
+        break;
+
+    case APIC_CMCI:
+        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
+            return X86EMUL_EXCEPTION;
+        break;
+    }
 
     *val = high | vlapic_read_aligned(vlapic, offset);
 
@@ -868,6 +881,10 @@ void vlapic_reg_write(struct vcpu *v, un
         vlapic_set_reg(vlapic, APIC_ICR2, val & 0xff000000U);
         break;
 
+    case APIC_CMCI:         /* LVT CMCI */
+        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
+            break;
+        fallthrough;
     case APIC_LVTT:         /* LVT Timer Reg */
         if ( vlapic_lvtt_tdt(vlapic) !=
              ((val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_TSC_DEADLINE) )
@@ -1024,9 +1041,12 @@ int guest_wrmsr_x2apic(struct vcpu *v, u
             return X86EMUL_EXCEPTION;
         break;
 
+    case APIC_CMCI:
+        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
+            return X86EMUL_EXCEPTION;
+        fallthrough;
     case APIC_LVTTHMR:
     case APIC_LVTPC:
-    case APIC_CMCI:
         if ( val & ~(LVT_MASK | APIC_DM_MASK) )
             return X86EMUL_EXCEPTION;
         break;
@@ -1438,7 +1458,9 @@ static void vlapic_do_init(struct vlapic
     if ( !has_vlapic(vlapic_vcpu(vlapic)->domain) )
         return;
 
-    vlapic_set_reg(vlapic, APIC_LVR, 0x00050014);
+    nr = 6 + !!(vlapic_vcpu(vlapic)->arch.vmce.mcg_cap & MCG_CMCI_P);
+    vlapic_set_reg(vlapic, APIC_LVR,
+                   0x00000014 | MASK_INSR(nr - 1, APIC_LVR_MAXLVT_MASK));
 
     for ( i = 0; i < 8; i++ )
     {
--- a/xen/arch/x86/include/asm/apicdef.h
+++ b/xen/arch/x86/include/asm/apicdef.h
@@ -15,7 +15,10 @@
 #define			GET_xAPIC_ID(x)		(((x)>>24)&0xFFu)
 #define			SET_xAPIC_ID(x)		(((x)<<24))
 #define		APIC_LVR	0x30
-#define			APIC_LVR_MASK		0xFF00FF
+#define			APIC_LVR_VERSION_MASK	0xff
+#define			APIC_LVR_MAXLVT_MASK	0xff0000
+#define			APIC_LVR_MASK		(APIC_LVR_VERSION_MASK | \
+						 APIC_LVR_MAXLVT_MASK)
 #define			APIC_LVR_DIRECTED_EOI	(1 << 24)
 #define			GET_APIC_VERSION(x)	((x)&0xFF)
 #define			GET_APIC_MAXLVT(x)	(((x)>>16)&0xFF)

Re: [PATCH for-4.21??? 3/3] x86/vLAPIC: properly support the CMCI LVT

[Grygorii Strashko]

Hi Jan

On 08.10.25 15:09, Jan Beulich wrote:
> Rather than unconditionally accepting reads and writes while discarding
> the value written, make accesses properly conditional upon CMCI being
> exposed via MCG_CAP, and arrange to actually retain the value written.
> Also reflect the extra LVT in LVR.
> 
> Note that this doesn't change the status quo of us never delivering any
> interrupt through this LVT.
> 
> Fixes: 70173dbb9948 ("x86/HVM: fix miscellaneous aspects of x2APIC emulation")
> Fixes: 8d0a20587e4e ("x86/hvm: further restrict access to x2apic MSRs")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The Fixes: tags are referencing where the explicit mentioning of APIC_CMCI
> in what are now guest_{rd,wr}msr_x2apic() was introduced; the mis-handling
> really pre-dates that, though.
> 
> In principle the later assignment to "nr" in vlapic_do_init() could now be
> dropped again. I wasn't quite sure though whether that's a good idea.
> 
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -31,10 +31,13 @@
>   #include <public/hvm/ioreq.h>
>   #include <public/hvm/params.h>
>   
> -#define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)
> +#include <../cpu/mcheck/x86_mca.h> /* MCG_CMCI_P */

This include... You probably do not like it also
It is dependency outside HVM code.

I've been thinking about something like vlapic->caps which can be filed before vlapic_init()
or passed as parameter, but seems x86 toolstack is considered to be able overwrite anything,
including v->arch.vmce.

Seems, no better options here.

> +
> +#define LVT_BIAS(reg)                   (((reg) - APIC_CMCI) >> 4)
>   
>   #define LVTS \
> -    LVT(LVTT), LVT(LVTTHMR), LVT(LVTPC), LVT(LVT0), LVT(LVT1), LVT(LVTERR),
> +    LVT(LVTT), LVT(LVTTHMR), LVT(LVTPC), LVT(LVT0), LVT(LVT1), LVT(LVTERR), \
> +    LVT(CMCI),
>   
>   static const unsigned int lvt_reg[] = {
>   #define LVT(which) APIC_ ## which
> @@ -57,6 +60,7 @@ static const unsigned int lvt_valid[] =
>   #define LVT0_VALID    LINT_MASK
>   #define LVT1_VALID    LINT_MASK
>   #define LVTERR_VALID  LVT_MASK
> +#define CMCI_VALID    (LVT_MASK | APIC_DM_MASK)
>   #define LVT(which)    [LVT_BIAS(APIC_ ## which)] = which ## _VALID
>       LVTS
>   #undef LVT
> @@ -697,8 +701,17 @@ int guest_rdmsr_x2apic(const struct vcpu
>           return X86EMUL_EXCEPTION;
>   
>       offset = reg << 4;
> -    if ( offset == APIC_ICR )
> +    switch ( offset )
> +    {
> +    case APIC_ICR:
>           high = (uint64_t)vlapic_read_aligned(vlapic, APIC_ICR2) << 32;
> +        break;
> +
> +    case APIC_CMCI:
> +        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )

Could it be done using wrapper, like vmce_has_cmci()?
As this is Intel specific it's candidate to be opt-out eventually.

> +            return X86EMUL_EXCEPTION;
> +        break;
> +    }
>   
>       *val = high | vlapic_read_aligned(vlapic, offset);
>   
> @@ -868,6 +881,10 @@ void vlapic_reg_write(struct vcpu *v, un
>           vlapic_set_reg(vlapic, APIC_ICR2, val & 0xff000000U);
>           break;
>   
> +    case APIC_CMCI:         /* LVT CMCI */
> +        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
> +            break;
> +        fallthrough;
>       case APIC_LVTT:         /* LVT Timer Reg */
>           if ( vlapic_lvtt_tdt(vlapic) !=
>                ((val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_TSC_DEADLINE) )
> @@ -1024,9 +1041,12 @@ int guest_wrmsr_x2apic(struct vcpu *v, u
>               return X86EMUL_EXCEPTION;
>           break;
>   
> +    case APIC_CMCI:
> +        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
> +            return X86EMUL_EXCEPTION;
> +        fallthrough;
>       case APIC_LVTTHMR:
>       case APIC_LVTPC:
> -    case APIC_CMCI:
>           if ( val & ~(LVT_MASK | APIC_DM_MASK) )
>               return X86EMUL_EXCEPTION;
>           break;
> @@ -1438,7 +1458,9 @@ static void vlapic_do_init(struct vlapic
>       if ( !has_vlapic(vlapic_vcpu(vlapic)->domain) )
>           return;
>   
> -    vlapic_set_reg(vlapic, APIC_LVR, 0x00050014);
> +    nr = 6 + !!(vlapic_vcpu(vlapic)->arch.vmce.mcg_cap & MCG_CMCI_P);
> +    vlapic_set_reg(vlapic, APIC_LVR,
> +                   0x00000014 | MASK_INSR(nr - 1, APIC_LVR_MAXLVT_MASK));
>   
>       for ( i = 0; i < 8; i++ )
>       {
> --- a/xen/arch/x86/include/asm/apicdef.h
> +++ b/xen/arch/x86/include/asm/apicdef.h
> @@ -15,7 +15,10 @@
>   #define			GET_xAPIC_ID(x)		(((x)>>24)&0xFFu)
>   #define			SET_xAPIC_ID(x)		(((x)<<24))
>   #define		APIC_LVR	0x30
> -#define			APIC_LVR_MASK		0xFF00FF
> +#define			APIC_LVR_VERSION_MASK	0xff
> +#define			APIC_LVR_MAXLVT_MASK	0xff0000
> +#define			APIC_LVR_MASK		(APIC_LVR_VERSION_MASK | \
> +						 APIC_LVR_MAXLVT_MASK)
>   #define			APIC_LVR_DIRECTED_EOI	(1 << 24)
>   #define			GET_APIC_VERSION(x)	((x)&0xFF)
>   #define			GET_APIC_MAXLVT(x)	(((x)>>16)&0xFF)
> 

-- 
Best regards,
-grygorii

Re: [PATCH for-4.21??? 3/3] x86/vLAPIC: properly support the CMCI LVT

[Jan Beulich]

On 09.10.2025 17:08, Grygorii Strashko wrote:
> On 08.10.25 15:09, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -31,10 +31,13 @@
>>   #include <public/hvm/ioreq.h>
>>   #include <public/hvm/params.h>
>>   
>> -#define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)
>> +#include <../cpu/mcheck/x86_mca.h> /* MCG_CMCI_P */
> 
> This include... You probably do not like it also
> It is dependency outside HVM code.
> 
> I've been thinking about something like vlapic->caps which can be filed before vlapic_init()
> or passed as parameter, but seems x86 toolstack is considered to be able overwrite anything,
> including v->arch.vmce.
> 
> Seems, no better options here.

Same here, hence why I used it despite not liking it.

>> @@ -697,8 +701,17 @@ int guest_rdmsr_x2apic(const struct vcpu
>>           return X86EMUL_EXCEPTION;
>>   
>>       offset = reg << 4;
>> -    if ( offset == APIC_ICR )
>> +    switch ( offset )
>> +    {
>> +    case APIC_ICR:
>>           high = (uint64_t)vlapic_read_aligned(vlapic, APIC_ICR2) << 32;
>> +        break;
>> +
>> +    case APIC_CMCI:
>> +        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
> 
> Could it be done using wrapper, like vmce_has_cmci()?
> As this is Intel specific it's candidate to be opt-out eventually.

Possible. I wanted to limit the churn, hence why I preferred not to introduce
a wrapper. Such an abstraction I wouldn't like to be a function taking a vCPU;
really this should be a domain property imo.

Jan

Re: [PATCH for-4.21??? 3/3] x86/vLAPIC: properly support the CMCI LVT

[Grygorii Strashko]

On 09.10.25 18:37, Jan Beulich wrote:
> On 09.10.2025 17:08, Grygorii Strashko wrote:
>> On 08.10.25 15:09, Jan Beulich wrote:
>>> --- a/xen/arch/x86/hvm/vlapic.c
>>> +++ b/xen/arch/x86/hvm/vlapic.c
>>> @@ -31,10 +31,13 @@
>>>    #include <public/hvm/ioreq.h>
>>>    #include <public/hvm/params.h>
>>>    
>>> -#define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)
>>> +#include <../cpu/mcheck/x86_mca.h> /* MCG_CMCI_P */
>>
>> This include... You probably do not like it also
>> It is dependency outside HVM code.
>>
>> I've been thinking about something like vlapic->caps which can be filed before vlapic_init()
>> or passed as parameter, but seems x86 toolstack is considered to be able overwrite anything,
>> including v->arch.vmce.
>>
>> Seems, no better options here.
> 
> Same here, hence why I used it despite not liking it.
> 
>>> @@ -697,8 +701,17 @@ int guest_rdmsr_x2apic(const struct vcpu
>>>            return X86EMUL_EXCEPTION;
>>>    
>>>        offset = reg << 4;
>>> -    if ( offset == APIC_ICR )
>>> +    switch ( offset )
>>> +    {
>>> +    case APIC_ICR:
>>>            high = (uint64_t)vlapic_read_aligned(vlapic, APIC_ICR2) << 32;
>>> +        break;
>>> +
>>> +    case APIC_CMCI:
>>> +        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
>>
>> Could it be done using wrapper, like vmce_has_cmci()?
>> As this is Intel specific it's candidate to be opt-out eventually.
> 
> Possible. I wanted to limit the churn, hence why I preferred not to introduce
> a wrapper. Such an abstraction I wouldn't like to be a function taking a vCPU;
> really this should be a domain property imo.

My intention was to limit spreading direct access to "vmce" data over vlapic code:

static bool vlapic_has_cmci(const struct vcpu *v)
{
	return v->arch.vmce.mcg_cap & MCG_CMCI_P;
}

Just expressing opinion.

-- 
Best regards,
-grygorii

Re: [PATCH for-4.21??? 3/3] x86/vLAPIC: properly support the CMCI LVT

[Jan Beulich]

On 14.10.2025 16:12, Grygorii Strashko wrote:
> 
> 
> On 09.10.25 18:37, Jan Beulich wrote:
>> On 09.10.2025 17:08, Grygorii Strashko wrote:
>>> On 08.10.25 15:09, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/hvm/vlapic.c
>>>> +++ b/xen/arch/x86/hvm/vlapic.c
>>>> @@ -31,10 +31,13 @@
>>>>    #include <public/hvm/ioreq.h>
>>>>    #include <public/hvm/params.h>
>>>>    
>>>> -#define LVT_BIAS(reg)                   (((reg) - APIC_LVTT) >> 4)
>>>> +#include <../cpu/mcheck/x86_mca.h> /* MCG_CMCI_P */
>>>
>>> This include... You probably do not like it also
>>> It is dependency outside HVM code.
>>>
>>> I've been thinking about something like vlapic->caps which can be filed before vlapic_init()
>>> or passed as parameter, but seems x86 toolstack is considered to be able overwrite anything,
>>> including v->arch.vmce.
>>>
>>> Seems, no better options here.
>>
>> Same here, hence why I used it despite not liking it.
>>
>>>> @@ -697,8 +701,17 @@ int guest_rdmsr_x2apic(const struct vcpu
>>>>            return X86EMUL_EXCEPTION;
>>>>    
>>>>        offset = reg << 4;
>>>> -    if ( offset == APIC_ICR )
>>>> +    switch ( offset )
>>>> +    {
>>>> +    case APIC_ICR:
>>>>            high = (uint64_t)vlapic_read_aligned(vlapic, APIC_ICR2) << 32;
>>>> +        break;
>>>> +
>>>> +    case APIC_CMCI:
>>>> +        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
>>>
>>> Could it be done using wrapper, like vmce_has_cmci()?
>>> As this is Intel specific it's candidate to be opt-out eventually.
>>
>> Possible. I wanted to limit the churn, hence why I preferred not to introduce
>> a wrapper. Such an abstraction I wouldn't like to be a function taking a vCPU;
>> really this should be a domain property imo.
> 
> My intention was to limit spreading direct access to "vmce" data over vlapic code:
> 
> static bool vlapic_has_cmci(const struct vcpu *v)
> {
> 	return v->arch.vmce.mcg_cap & MCG_CMCI_P;
> }

"vlapic" in the name makes it seemingly better, but not really. As before: I
think such a predicate should be taking a const struct domain * as input.
Unless of course we expected that different vCPU-s in a guest may have
different settings of the MCG_CMCI_P bit.

Jan

Re: [PATCH for-4.21??? 3/3] x86/vLAPIC: properly support the CMCI LVT

[Andrew Cooper]

On 08/10/2025 1:09 pm, Jan Beulich wrote:
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -697,8 +701,17 @@ int guest_rdmsr_x2apic(const struct vcpu
>          return X86EMUL_EXCEPTION;
>  
>      offset = reg << 4;
> -    if ( offset == APIC_ICR )
> +    switch ( offset )
> +    {
> +    case APIC_ICR:
>          high = (uint64_t)vlapic_read_aligned(vlapic, APIC_ICR2) << 32;
> +        break;
> +
> +    case APIC_CMCI:
> +        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
> +            return X86EMUL_EXCEPTION;
> +        break;
> +    }
>  
>      *val = high | vlapic_read_aligned(vlapic, offset);
>  
> @@ -868,6 +881,10 @@ void vlapic_reg_write(struct vcpu *v, un
>          vlapic_set_reg(vlapic, APIC_ICR2, val & 0xff000000U);
>          break;
>  
> +    case APIC_CMCI:         /* LVT CMCI */
> +        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
> +            break;
> +        fallthrough;
>      case APIC_LVTT:         /* LVT Timer Reg */
>          if ( vlapic_lvtt_tdt(vlapic) !=
>               ((val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_TSC_DEADLINE) )
> @@ -1024,9 +1041,12 @@ int guest_wrmsr_x2apic(struct vcpu *v, u
>              return X86EMUL_EXCEPTION;
>          break;
>  
> +    case APIC_CMCI:
> +        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
> +            return X86EMUL_EXCEPTION;
> +        fallthrough;
>      case APIC_LVTTHMR:
>      case APIC_LVTPC:
> -    case APIC_CMCI:
>          if ( val & ~(LVT_MASK | APIC_DM_MASK) )
>              return X86EMUL_EXCEPTION;
>          break;

This is almost certainly not how real hardware behaves.

The APIC is a discrete block of logic, whether it's integrated into the
core or not.  A new LVT is "just" another interrupt source, and if
nothing is wired into that pin, then it's just a register which never
produces an interrupt.

Accessibility of LVT_CMCI will depend on MAXLVT and nothing else.  In
silicon, I'm pretty sure it will be hardcoded as fully absent or
present, because I can't see any reason to make this configurable.

At this point, things get more complicated.

On Intel, there's no such thing as x2APIC capable (irrespective of
x2APIC enabled) without LVT_CMCI which is why there are no additional
access constraints on the register.

On AMD, there's no LVT_CMCI even on systems which support x2APIC. 
Instead, ELVTs are used and it is MCE-configuration based which ELVT the
interrupt is delivered through.

Choosing a default MAXLVT based on MCG_CMCI_P is probably fine (although
it certainly is ugly to tie APIC and vMCE together), but controls on the
access to APIC_CMCI should be based on MAXLVT.

~Andrew

Re: [PATCH for-4.21??? 3/3] x86/vLAPIC: properly support the CMCI LVT

[Jan Beulich]

On 14.10.2025 21:36, Andrew Cooper wrote:
> On 08/10/2025 1:09 pm, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -697,8 +701,17 @@ int guest_rdmsr_x2apic(const struct vcpu
>>          return X86EMUL_EXCEPTION;
>>  
>>      offset = reg << 4;
>> -    if ( offset == APIC_ICR )
>> +    switch ( offset )
>> +    {
>> +    case APIC_ICR:
>>          high = (uint64_t)vlapic_read_aligned(vlapic, APIC_ICR2) << 32;
>> +        break;
>> +
>> +    case APIC_CMCI:
>> +        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
>> +            return X86EMUL_EXCEPTION;
>> +        break;
>> +    }
>>  
>>      *val = high | vlapic_read_aligned(vlapic, offset);
>>  
>> @@ -868,6 +881,10 @@ void vlapic_reg_write(struct vcpu *v, un
>>          vlapic_set_reg(vlapic, APIC_ICR2, val & 0xff000000U);
>>          break;
>>  
>> +    case APIC_CMCI:         /* LVT CMCI */
>> +        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
>> +            break;
>> +        fallthrough;
>>      case APIC_LVTT:         /* LVT Timer Reg */
>>          if ( vlapic_lvtt_tdt(vlapic) !=
>>               ((val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_TSC_DEADLINE) )
>> @@ -1024,9 +1041,12 @@ int guest_wrmsr_x2apic(struct vcpu *v, u
>>              return X86EMUL_EXCEPTION;
>>          break;
>>  
>> +    case APIC_CMCI:
>> +        if ( !(v->arch.vmce.mcg_cap & MCG_CMCI_P) )
>> +            return X86EMUL_EXCEPTION;
>> +        fallthrough;
>>      case APIC_LVTTHMR:
>>      case APIC_LVTPC:
>> -    case APIC_CMCI:
>>          if ( val & ~(LVT_MASK | APIC_DM_MASK) )
>>              return X86EMUL_EXCEPTION;
>>          break;
> 
> This is almost certainly not how real hardware behaves.
> 
> The APIC is a discrete block of logic, whether it's integrated into the
> core or not.  A new LVT is "just" another interrupt source, and if
> nothing is wired into that pin, then it's just a register which never
> produces an interrupt.
> 
> Accessibility of LVT_CMCI will depend on MAXLVT and nothing else.  In
> silicon, I'm pretty sure it will be hardcoded as fully absent or
> present, because I can't see any reason to make this configurable.
> 
> At this point, things get more complicated.
> 
> On Intel, there's no such thing as x2APIC capable (irrespective of
> x2APIC enabled) without LVT_CMCI which is why there are no additional
> access constraints on the register.
> 
> On AMD, there's no LVT_CMCI even on systems which support x2APIC. 
> Instead, ELVTs are used and it is MCE-configuration based which ELVT the
> interrupt is delivered through.
> 
> Choosing a default MAXLVT based on MCG_CMCI_P is probably fine (although
> it certainly is ugly to tie APIC and vMCE together), but controls on the
> access to APIC_CMCI should be based on MAXLVT.

As you ask for it, I can certainly do so. I didn't because the way it's
done now the checks are cheaper overall.

Jan

Re: [PATCH for-4.21??? 0/3] x86/vLAPIC: CMCI LVT handling

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]


On 10/8/25 2:07 PM, Jan Beulich wrote:
> 1: add indirection to LVT handling
> 2: drop VLAPIC_VERSION
> 3: properly support the CMCI LVT
>
> Now having this coded up, I realize it may be too intrusive at this point of
> the release cycle. Still I wanted to propose it ...

IIUC (not being an x86 expert, please take that into account),
Xen advertises CMCI support but doesn’t emulate it properly, which means
the guest stops noticing corrected errors. This could result in lost error
reports or potential guest kernel warnings, right?
Would that lead to inconsistencies in the MCE subsystem?

I’m not entirely sure this issue is critical, as the related Linux
configurations can be disabled, or MCE can be turned off using|mce=off|.
At the very least, CMCI can be disabled with|mce=no_cmci|. So, unless there are
other objections, I think this can be deferred until 4.22.

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 1455 bytes --]