On Tue Sep 30, 2025 at 9:05 PM CEST, Grygorii Strashko wrote: > From: Grygorii Strashko <[email protected]> > > 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 <[email protected]> > --- > Changes in v2: > - masks fixed in vlapic_lvt_mask[] > - commit msg reworded > > v1: > https://patchwork.kernel.org/project/xen-devel/patch/[email protected]/ > 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
