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
