Re: [PATCH v2] x86/hvm: vlapic: fix RO bits emulation in LVTx regs

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Tue Sep 30, 2025 at 9:05 PM CEST, Grygorii Strashko wrote:
> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
>
> The LAPIC LVTx registers have two RO bits:
> - all: Delivery Status (DS) bit 12
> - LINT0/LINT1: Remote IRR Flag (RIR) bit 14.
>
> The Delivery Status (DS) is not emulated by Xen - there is no IRQ msg bus,
> and the IRQ is:
> - or accepted at destination and appears as pending
>   (vLAPIC Interrupt Request Register (IRR))
> - or get rejected immediately.
>
> The Remote IRR Flag (RIR) behavior emulation is not implemented for
> LINT0/LINT1 in Xen for now.
>
> The current vLAPIC implementations allows guest to write to these RO bits.
>
> The vLAPIC LVTx registers write happens in vlapic_reg_write() which expect
> to implement "Write ignore" access type for RO bits by applying masks from
> vlapic_lvt_mask[], but vlapic_lvt_mask[] contains incorrect masks which
> allows writing to RO fields.
>
> Hence it is definitely wrong to allow guest to write to LVTx regs RO bits,
> fix it by fixing LVTx registers masks in vlapic_lvt_mask[].
>
> In case of WRMSR (guest_wrmsr_x2apic()) access to LVTx registers, the SDM
> clearly defines access type for "Reserved" bits as RsvdZ (Non-zero writes
> to reserved bits should cause #GP exception), but contains no statements
> for RO bits except that they are not "Reserved". So, guest_wrmsr_x2apic()
> now uses different masks (than vlapic_reg_write()) for checking LVTx
> registers values for "Reserved" bit settings, which include RO bits and
> do not cause #GP exception.
>
> Fixes: d1bd157fbc9b ("Big merge the HVM full-virtualisation abstractions.")
> Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> ---
> Changes in v2:
> - masks fixed in vlapic_lvt_mask[]
> - commit msg reworded
>
> v1: 
> https://patchwork.kernel.org/project/xen-devel/patch/20250925195558.519568-1-grygorii_strashko@xxxxxxxx/
>  xen/arch/x86/hvm/vlapic.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 79697487ba90..2ecba8163f48 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -44,15 +44,17 @@
>  static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
>  {
>       /* LVTT */
> -     LVT_MASK | APIC_TIMER_MODE_MASK,
> +     (LVT_MASK | APIC_TIMER_MODE_MASK) & ~APIC_SEND_PENDING,
>       /* LVTTHMR */
> -     LVT_MASK | APIC_DM_MASK,
> +     (LVT_MASK | APIC_DM_MASK) & ~APIC_SEND_PENDING,
>       /* LVTPC */
> -     LVT_MASK | APIC_DM_MASK,
> -     /* LVT0-1 */
> -     LINT_MASK, LINT_MASK,
> +     (LVT_MASK | APIC_DM_MASK) & ~APIC_SEND_PENDING,
> +     /* LVT0 */
> +     LINT_MASK & ~(APIC_LVT_REMOTE_IRR | APIC_SEND_PENDING),
> +     /* LVT1 */
> +     LINT_MASK & ~(APIC_LVT_REMOTE_IRR | APIC_SEND_PENDING),
>       /* LVTERR */
> -     LVT_MASK
> +     LVT_MASK & ~APIC_SEND_PENDING,
>  };

This is a bit messy. Why not have 2 masks? One for rsvdZ bits, and one
for RO?

That ought to simplify the logic in both the MSR and MMIO cases.

MMIO would do RAZ/WI on the OR of both, while the MSR interface would gate
#GP(0) on the mask for rsvd bits only and ensure all RO bits are preserved on
writes.

Thoughts?

Cheers,
Alejandro