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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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