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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

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:
> > > --- 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.
The order in lvt_x_masks[] arrays are guaranteed by "[x] = y,".

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.

> > >    #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;


--
Best regards,
-grygorii