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

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

Posted by Grygorii Strashko 4 days, 1 hour ago

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.

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@epam.com>
---
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@epam.com/
 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,
 };
 
 #define vlapic_lvtt_period(vlapic)                              \
-- 
2.34.1

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

Posted by Alejandro Vallejo 3 days, 5 hours ago

On Tue Sep 30, 2025 at 9:05 PM CEST, 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.
>
> 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@epam.com>
> ---
> 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@epam.com/
>  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

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

Posted by Grygorii Strashko 1 day, 6 hours ago


On 01.10.25 18:18, Alejandro Vallejo wrote:
> On Tue Sep 30, 2025 at 9:05 PM CEST, 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.
>>
>> 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@epam.com>
>> ---
>> 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@epam.com/
>>   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?

I've been thinking about the same and It can be done, np.
I always trying to make "fix" with as small diff as possible
considering back-porting.

How about "follow up" patch if there is an agreement to proceed this way on the Top level?


-- 
Best regards,
-grygorii