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

[Grygorii Strashko <grygorii_strashko@epam.com>]

On 26.09.25 11:17, Jan Beulich wrote:
> On 25.09.2025 21:55, Grygorii Strashko wrote:
>> From: Grygorii Strashko <grygorii_strashko@epam.com>
>>
>> The LAPIC LVTx registers have two RO bits:
>> - all: Delivery Status (DS) bit 12
>> - LINT0/LINT1: Remote IRR Flag (RIR) bit 14.
>>    This bit is reserved for other LVTx regs with RAZ/WI access type (MMIO), while
>>    WRMSR (guest_wrmsr_x2apic()) has appropiate checks for reserved bits
>>    (MBZ access type).
> 
> Question is what the behavior is for writing the r/o (but not reserved) bits.
> I wasn't able to find any statement in the SDM.

Me too. Usually RO/WI on most HW.
For example, LAPIC MMIO "Write" will be ignored (WRMSR will trigger exception).

> 
>> and the current vLAPIC implementations allows guest to write to these RO bits.
>>
>> 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.
>>
>> Hence it is definitely wrong to allow guest to write to LVTx regs RO bits,
>> fix it by unconditionally cleaning up those bits in vlapic_reg_write().
>>
>> Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
> 
> Please also add a Fixes: tag when correcting code.

This is veeery old code. Probably
f7c8af3a6476e ("[XEN] HVM: Clean up and simplify vlapic device-model code")
or
50b3cef2eecb0 ("[HVM] Place all APIC registers into one page in native format.")

> 
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -880,6 +880,7 @@ void vlapic_reg_write(struct vcpu *v, unsigned int reg, uint32_t val)
>>           if ( vlapic_sw_disabled(vlapic) )
>>               val |= APIC_LVT_MASKED;
>>           val &= array_access_nospec(vlapic_lvt_mask, (reg - APIC_LVTT) >> 4);
>> +        val &= ~(APIC_LVT_REMOTE_IRR | APIC_SEND_PENDING);
> 
> There shouldn't be a 2nd &= here; what needs adding should imo be added to
> (really: removed from) vlapic_lvt_mask[].

I'll try it.

  (Orthogonal to this I wonder whether
> guest_wrmsr_x2apic() wouldn't better use that array, too.)

WRMSR checks for MBZ. RO bits are not MBZ, so masks are different.

> 
> While looking at this, don't we have an issue with CMCI as well?

I see no APIC_CMCI write emulation. only read.

> guest_{rd,wr}msr_x2apic() handle it, but vlapic_reg_write() doesn't. I.e. on
> AMD we would fail to deliver #GP when the guest accesses it, while on Intel
> we would lose the value written. And we also don't set its mask bit in
> vlapic_do_init(). I guess I need to make a patch ...

Is'n it depends on CMCI capability exposing to guest? (have no idea what's CMCI :)

-- 
Best regards,
-grygorii