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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@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)
> +
> +#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.