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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

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