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 <[email protected]> >> --- >> 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
