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;

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.

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

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

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

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

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

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

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

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