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

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 25/10/2026 à 15:27, Bernhard Kaindl a écrit :
> 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?
Looks good to me, just a slight adjustment I would add.

> 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);
> +}
> +
I would rather introduce pi_clear_on()

static inline void pi_clear_on(struct pi_desc *pi_desc)
{
    clear_bit(POSTED_INTR_ON, &pi_desc->control);
}

And write vmx_reset_pir() as :

/* 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;

    pi_clear_on(desc);

    for ( unsigned int i = 0; i < ARRAY_SIZE(desc->pir); ++i )
        write_atomic(&desc->pir[i], 0);
}


Not sure which is best here between write_atomic (movq) and xchg.
Alternatively, we can also use `ACCESS_ONCE(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
Teddy

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature