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

[Jan Beulich <jbeulich@xxxxxxxx>]

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