Re: [PATCH 2/6] x86/vlapic: Fix hardware virtualized APIC state after reset

[Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>]

To: Teddy Astie <teddy.astie@xxxxxxxxxx>
Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx, Jan Beulich <jbeulich@xxxxxxxx>,
 Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, roger@xxxxxxxxxxxxxx,
 Denis Mukhin <dmukhin@xxxxxxx>
Subject: Re: [PATCH 2/6] x86/vlapic: Fix hardware virtualized APIC state after 
reset
In-Reply-To: 
<1792504425.8631fc262581453bbf619ec5b2062170.1a15996ec68000f79f@xxxxxxxxxx>
References: 
<1792504313.8631fc262581453bbf619ec5b2062170.1a1599536d8000f79f@xxxxxxxxxx>
 <20261020135333.38064-2-teddy.astie@xxxxxxxxxx>
 <1792504425.8631fc262581453bbf619ec5b2062170.1a15996ec68000f79f@xxxxxxxxxx>

Hi Teddy,

Thanks for the reset improvements. Could the new reset-time sync
call use a dedicated discard operation instead? That would also cover
suppressed postings without changing ordinary pending-interrupt queries.

xen/arch/x86/hvm/vlapic.c:
@@ -1429,6 +1432,13 @@ static void vlapic_do_init(struct vlapic *vlapic)
> +    /*
> +     * Drain the posted interrupt descriptor (and clear its ON bit) before
> +     * zeroing the IRR, so that vectors posted before the reset don't get
> +     * merged into the fresh IRR by a later sync.
> +     */
> +    hvm_sync_pir_to_irr(vlapic_vcpu(vlapic));

PIR (Posted Interrupt Request) records pending vectors. ON means
Outstanding Notification, while SN (Suppress Notification) suppresses
non-urgent notifications, not the recording of interrupts. The
switch-out hook sets SN for a non-blocked target. An IOMMU posting can
then leave PIR nonempty with ON=0 before reset starts.

The existing vmx_sync_pir_to_irr() in xen/arch/x86/hvm/vmx/vmx.c returns
before touching PIR in that state:

>     if ( !pi_test_on(desc) )
>         return;

Consequently the new call can leave an old vector in PIR while reset
clears IRR (Interrupt Request Register). It can later be transferred
into the fresh IRR. This is a pre-existing reset gap, not a new
regression introduced by this patch.

Checking PIR as well as ON in vmx_sync_pir_to_irr() would add reads
to ordinary pending queries, including hvm_vcpu_has_pending_irq().
Reset instead wants to discard the bits regardless of ON.

What do you think? Would replacing the new call with this optional
backend hook fit this patch better?

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1433,11 +1433,11 @@ static void vlapic_do_init(struct vlapic *vlapic)
         return;
 
     /*
-     * Drain the posted interrupt descriptor (and clear its ON bit) before
-     * zeroing the IRR, so that vectors posted before the reset don't get
-     * merged into the fresh IRR by a later sync.
+     * Discard posted interrupts before resetting the APIC registers,
+     * including suppressed postings which have not set ON.
      */
-    hvm_sync_pir_to_irr(vlapic_vcpu(vlapic));
+    if ( hvm_funcs.reset_pir )
+        alternative_vcall(hvm_funcs.reset_pir, vlapic_vcpu(vlapic));
 
     vlapic_set_reg(vlapic, APIC_LVR, VLAPIC_VERSION);
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2437,6 +2437,17 @@ static void cf_check vmx_sync_pir_to_irr(struct vcpu *v)
     }
 }
 
+/* Discard posted interrupts without depending on notification state. */
+static void cf_check vmx_reset_pir(struct vcpu *v)
+{
+    struct pi_desc *desc = &v->arch.hvm.vmx.pi_desc;
+
+    (void)pi_test_and_clear_on(desc);
+
+    for ( unsigned int i = 0; i < ARRAY_SIZE(desc->pir); ++i )
+        (void)xchg(&desc->pir[i], 0);
+}
+
 static bool cf_check vmx_test_pir(const struct vcpu *v, uint8_t vec)
 {
     return pi_test_pir(vec, &v->arch.hvm.vmx.pi_desc);
@@ -3176,6 +3187,7 @@ const struct hvm_function_table * __init start_vmx(void)
 
         vmx_function_table.deliver_posted_intr = vmx_deliver_posted_intr;
         vmx_function_table.sync_pir_to_irr     = vmx_sync_pir_to_irr;
+        vmx_function_table.reset_pir           = vmx_reset_pir;
         vmx_function_table.test_pir            = vmx_test_pir;
     }
 
@@ -3212,6 +3224,7 @@ void __init vmx_fill_funcs(void)
 
     vmx_function_table.deliver_posted_intr = vmx_deliver_posted_intr;
     vmx_function_table.sync_pir_to_irr     = vmx_sync_pir_to_irr;
+    vmx_function_table.reset_pir           = vmx_reset_pir;
     vmx_function_table.test_pir            = vmx_test_pir;
 }
 
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h 
b/xen/arch/x86/include/asm/hvm/hvm.h
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -218,6 +218,8 @@ struct hvm_function_table {
     void (*process_isr)(int isr, struct vcpu *v);
     void (*deliver_posted_intr)(struct vcpu *v, u8 vector);
     void (*sync_pir_to_irr)(struct vcpu *v);
+    /* Discard pending posted interrupts and notification state on reset. */
+    void (*reset_pir)(struct vcpu *v);
     bool (*test_pir)(const struct vcpu *v, uint8_t vector);
     void (*handle_eoi)(uint8_t vector, int isr);
     int (*pi_update_irte)(const struct vcpu *v, const struct pirq *pirq);

The ON clear is before clearing PIR, preserving the required
clear-ON-before-draining-PIR ordering (see vmx_sync_pir_to_irr()).

The helper needs only the inline descriptor, so initial LAPIC setup
can call it before VMCS creation. The hook is optional and installed
for VMX posted-interrupt support, including the vmx_fill_funcs() entry.

For the series as posted, and also with the suggested change above
incorporated:

Assisted-by: Copilot:gpt-6-astra-high, Copilot:gpt-6.1-sol-high-xhigh-max # 
review
Reviewed-by: Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>

cu, Bernhard

-- 
Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>