Re: [Xen-devel] [PATCH RFC] pass-through: sync pir to irr after msix vector been updated

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.09.2019 22:33, Joe Jin wrote:
> On 9/24/19 8:42 AM, Roger Pau Monné wrote:
>> AFAICT you are draining any pending data from the posted interrupt
>> PIRR field into the IRR vlapic field, so that no stale interrupts are
>> left behind after the MSI fields have been updated by the guest. I
>> think this is correct, I wonder however whether you also need to
>> trigger a vcpu re-scheduling (pause/unpause the vpcu), so that pending
>> interrupts in IRR are injected by vmx_intr_assist.

You didn't address (perhaps just verbally) this remark from Roger
at all. I'm not convinced that a pause/unpause is an appropriate
action here - there surely should be a more direct way.

>> Also, I think you should do this syncing after the pi_update_irte
>> call, or else there's still a window (albeit small) where you can
>> still get posted interrupts delivered to the old vcpu.
> 
> I agree with you that we need to take care of this issue as well.
> 
> I created an additional patch but not tested yet for the test env was
> broken, post here for your comment firstly, I'll update later of test
> result when my test env back:

It would have been nice if you had at least build-tested it. In
its current shape it's hard to tell what value it is. Anyway, ...

> @@ -23,6 +23,7 @@
>  #include <xen/irq.h>
>  #include <asm/hvm/irq.h>
>  #include <asm/hvm/support.h>
> +#include <asm/hvm/vmx/vmx.h>

Why is this needed? It's not a good idea to include it here.

> @@ -443,9 +444,21 @@ int pt_irq_create_bind(
>              hvm_migrate_pirq(pirq_dpci, vcpu);
>  
>          /* Use interrupt posting if it is supported. */
> -        if ( iommu_intpost )
> -            pi_update_irte(vcpu ? &vcpu->arch.hvm.vmx.pi_desc : NULL,
> -                           info, pirq_dpci->gmsi.gvec);
> +        if ( iommu_intpost ) {
> +            unsigned int ndst = APIC_INVALID_DEST;
> +            struct irq_desc *desc;
> +
> +            desc = pirq_spin_lock_irq_desc(info, NULL);
> +            if ( irq_desc ) {
> +                ndst = irq_desc->msi_desc->pi_desc->ndst;
> +                spin_unlock_irq(&desc->lock);
> +            }

This redoes (in a much less careful way, i.e. prone to a crash)
what pi_update_irte() does. It would seem far easier if you
simply made the function return the value, or even make it do
the call right away (when needed).

> +            if ( pi_update_irte(vcpu ? &vcpu->arch.hvm.vmx.pi_desc : NULL,
> +                                    info, pirq_dpci->gmsi.gvec) == 0
> +                            && ndst != APIC_INVALID_DEST )
> +                vlapic_sync_pir_to_irr(d->vcpu[ndst]);

Aiui "ndst" is an APIC ID and hence can't be used to index
d->vcpu[]. Without a description it's not really clear to me
why you found it necessary to go via APIC ID here - in your
earlier patch variant this wasn't the case iirc.

Before you actually (re)post this patch you'll also want to
take care of numerous style issues.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel