|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.13 v3] x86/passthrough: fix migration of MSI when using posted interrupts
On 08.11.2019 14:34, Roger Pau Monne wrote:
> When using posted interrupts and the guest migrates MSI from vCPUs Xen
> needs to flush any pending PIRR vectors on the previous vCPU, or else
> those vectors could get wrongly injected at a later point when the MSI
> fields are already updated.
>
> Rename sync_pir_to_irr to vlapic_sync_pir_to_irr and export it so it
> can be called when updating the binding of physical interrupts to
> guests.
>
> Note that PIRR is synced to IRR both in pt_irq_destroy_bind and
> pt_irq_create_bind when the interrupt delivery data is being updated.
>
> Also store the vCPU ID in multi-destination mode when using posted
> interrupts so that the interrupt is always injected to a known vCPU in
> order to be able to flush the PIRR when modifying the binding.
>
> Reported-by: Joe Jin <joe.jin@xxxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Cc: Joe Jin <joe.jin@xxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> ---
> I would like to see a bug fix for this issue in 4.13. The fix here only
> affects posted interrupts, hence I think the risk of breaking anything
> else is low.
> ---
> Changes since v2:
> - Also sync PIRR with IRR when using CPU posted interrupts.
> - Force the selection of a specific vCPU when using posted interrupts
> for multi-dest.
> - Change vmsi_deliver_pirq to honor dest_vcpu_id.
>
> Changes since v1:
> - Store the vcpu id also in multi-dest mode if the interrupt is bound
> to a vcpu for posted delivery.
> - s/#if/#ifdef/.
> ---
> xen/arch/x86/hvm/vlapic.c | 6 +++---
> xen/arch/x86/hvm/vmsi.c | 11 ++++++++++-
> xen/drivers/passthrough/io.c | 29 +++++++++++++++++++++++------
> xen/include/asm-x86/hvm/vlapic.h | 2 ++
> 4 files changed, 38 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 9466258d6f..d255ad8db7 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -106,7 +106,7 @@ static void vlapic_clear_irr(int vector, struct vlapic
> *vlapic)
> vlapic_clear_vector(vector, &vlapic->regs->data[APIC_IRR]);
> }
>
> -static void sync_pir_to_irr(struct vcpu *v)
> +void vlapic_sync_pir_to_irr(struct vcpu *v)
> {
> if ( hvm_funcs.sync_pir_to_irr )
> alternative_vcall(hvm_funcs.sync_pir_to_irr, v);
> @@ -114,7 +114,7 @@ static void sync_pir_to_irr(struct vcpu *v)
>
> static int vlapic_find_highest_irr(struct vlapic *vlapic)
> {
> - sync_pir_to_irr(vlapic_vcpu(vlapic));
> + vlapic_sync_pir_to_irr(vlapic_vcpu(vlapic));
>
> return vlapic_find_highest_vector(&vlapic->regs->data[APIC_IRR]);
> }
> @@ -1493,7 +1493,7 @@ static int lapic_save_regs(struct vcpu *v,
> hvm_domain_context_t *h)
> if ( !has_vlapic(v->domain) )
> return 0;
>
> - sync_pir_to_irr(v);
> + vlapic_sync_pir_to_irr(v);
>
> return hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, vcpu_vlapic(v)->regs);
> }
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index 6597d9f719..fe488ccc7d 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -118,7 +118,16 @@ void vmsi_deliver_pirq(struct domain *d, const struct
> hvm_pirq_dpci *pirq_dpci)
>
> ASSERT(pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI);
>
> - vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
> + if ( hvm_funcs.deliver_posted_intr && pirq_dpci->gmsi.dest_vcpu_id != -1
> )
> + /*
> + * When using posted interrupts multi-destination delivery mode is
> + * forced to select a specific vCPU so that the PIRR can be synced
> into
> + * IRR when the interrupt is destroyed or moved.
> + */
> + vmsi_inj_irq(vcpu_vlapic(d->vcpu[pirq_dpci->gmsi.dest_vcpu_id]),
> + vector, trig_mode, delivery_mode);
> + else
> + vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
> }
>
> /* Return value, -1 : multi-dests, non-negative value: dest_vcpu_id */
> diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
> index b292e79382..d3f1ae5c39 100644
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -341,7 +341,7 @@ int pt_irq_create_bind(
> {
> uint8_t dest, delivery_mode;
> bool dest_mode;
> - int dest_vcpu_id;
> + int dest_vcpu_id, prev_vcpu_id = -1;
> const struct vcpu *vcpu;
> uint32_t gflags = pt_irq_bind->u.msi.gflags &
> ~XEN_DOMCTL_VMSI_X86_UNMASKED;
> @@ -411,6 +411,7 @@ int pt_irq_create_bind(
>
> pirq_dpci->gmsi.gvec = pt_irq_bind->u.msi.gvec;
> pirq_dpci->gmsi.gflags = gflags;
> + prev_vcpu_id = pirq_dpci->gmsi.dest_vcpu_id;
> }
> }
> /* Calculate dest_vcpu_id for MSI-type pirq migration. */
> @@ -426,14 +427,24 @@ int pt_irq_create_bind(
>
> pirq_dpci->gmsi.posted = false;
> vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;
> - if ( iommu_intpost )
> + if ( hvm_funcs.deliver_posted_intr && delivery_mode ==
> dest_LowestPrio )
> {
> - if ( delivery_mode == dest_LowestPrio )
> - vcpu = vector_hashing_dest(d, dest, dest_mode,
> - pirq_dpci->gmsi.gvec);
> + /*
> + * NB: when using posted interrupts the vector is signaled
> + * on the PIRR, and hence Xen needs to force interrupts to be
> + * delivered to a specific vCPU in order to be able to sync PIRR
> + * with IRR when the interrupt binding is destroyed, or else
> + * pending interrupts in the previous vCPU PIRR field could be
> + * delivered after the update.
> + */
> + vcpu = vector_hashing_dest(d, dest, dest_mode,
> + pirq_dpci->gmsi.gvec);
> if ( vcpu )
> - pirq_dpci->gmsi.posted = true;
> + pirq_dpci->gmsi.dest_vcpu_id = vcpu->vcpu_id;
> }
> + if ( iommu_intpost && vcpu )
> + pirq_dpci->gmsi.posted = true;
> +
> if ( vcpu && is_iommu_enabled(d) )
> hvm_migrate_pirq(pirq_dpci, vcpu);
I'm afraid I don't really agree with this as a whole, but to a fair
part because of shortcomings of the original code. For one, I can't
figure how hvm_girq_dest_2_vcpu_id() can possibly produce a useful
result without it being passed the delivery mode. I think what the
function does should much more closely resemble vmsi_deliver(),
just without actually delivering anything.
Next you seem to be assuming that multi-dest can only be a result
of lowest-priority delivery mode. But look at vmsi_deliver(): Fixed
mode too can result in multiple destinations, just that in this
case it ends up being a multicast.
When there are multiple destinations, we simply shouldn't post the
interrupt. Note that lowest-priority delivery mode, once
hvm_girq_dest_2_vcpu_id() actually learns of honoring it, would not
have to result in multiple destinations, again as per vmsi_deliver()
(i.e. posting would still be possible in this case, just that
arbitration then happens in software and [unfortunately] ahead of
the time the interrupt actually occurs; this may still represent a
problem though, but it's unclear to me how this case is actually
intended to work with an IRTE only being able to point at a single
PI descriptor - Kevin?).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |