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

[Jan Beulich <jbeulich@xxxxxxxx>]

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