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.
Thanks, Roger.
