Re: [Xen-devel] [PATCH v2] x86/hvm: re-work viridian APIC assist code

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: David Woodhouse [mailto:dwmw2@xxxxxxxxxxxxx]
> Sent: 25 August 2018 00:38
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich
> <jbeulich@xxxxxxxx>; Eslam Elnikety <elnikety@xxxxxxxxx>; Shan Haitao
> <haitao.shan@xxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v2] x86/hvm: re-work viridian APIC assist
> code
> 
> On Thu, 2018-01-18 at 10:10 -0500, Paul Durrant wrote:
> > Lastly the previous code did not properly emulate an EOI if a missed EOI
> > was discovered in vlapic_has_pending_irq(); it merely cleared the bit in
> > the ISR. The new code instead calls vlapic_EOI_set().
> 
> Hm, this *halves* my observed performance running a 32-thread
> 'diskspd.exe' on a Windows box with attached NVME devices, which makes
> me sad.

Yes, that's clearly not what it is expected :-(

> 
> It's the call to hvm_dpci_msi_eoi() that does it.
> 
> Commenting out the call to pt_pirq_iterate() and leaving *just* the
> domain-global spinlock bouncing cache lines between all my CPUs, it's
> already down to 1.6MIOPS/s from 2.2M on my test box before it does
> *anything* at all.
> 
> Calling an *inline* version of pt_pirq_iterate so no retpoline for the
> indirect calls, and I'm down to 1.1M even when I've nopped out the
> whole of the _hvm_dpci_msi_eoi function that it's calling. Put it all
> back, and I'm down to about 1.0M. So it's worse than halved.
> 
> And what's all this for? The code here is making my eyes bleed but I
> believe it's for unmaskable MSIs, and these aren't unmaskable.
> 

I believe APIC assist is intended for fully synthetic interrupts. Is it 
definitely this patch that causes the problem? It was only intended to fix 
previous incorrectness but, if this is the culprit, then it's clearly caused 
collateral damage in a logically unrelated area.

  Paul

> Tempted to make it all go away by having a per-domain bitmap of vectors
> for which all this work is actually required, and bypassing the whole
> bloody lot in hvm_dpci_msi_eoi() if the corresponding in bit that
> bitmap isn't set.
> 
> The hackish version of that (which seems to work, but would probably
> want testing with an actual unmaskable MSI in the system, and I have
> absolutely no confidence I understand what's going on here) looks
> something like this:
> 
> diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
> index bab3aa3..24df008 100644
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -24,6 +24,7 @@
>  #include <asm/hvm/irq.h>
>  #include <asm/hvm/support.h>
>  #include <asm/io_apic.h>
> +#include <asm/msi.h>
> 
>  static DEFINE_PER_CPU(struct list_head, dpci_list);
> 
> @@ -282,6 +283,7 @@ int pt_irq_create_bind(
>      struct hvm_pirq_dpci *pirq_dpci;
>      struct pirq *info;
>      int rc, pirq = pt_irq_bind->machine_irq;
> +    irq_desc_t *desc;
> 
>      if ( pirq < 0 || pirq >= d->nr_pirqs )
>          return -EINVAL;
> @@ -422,6 +425,13 @@ int pt_irq_create_bind(
> 
>          dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
>          pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
> +        BUG_ON(!local_irq_is_enabled());
> +        desc = pirq_spin_lock_irq_desc(info, NULL);
> +        if ( desc && desc->msi_desc && !msi_maskable_irq(desc->msi_desc) )
> +            set_bit(pirq_dpci->gmsi.gvec,
> +                    hvm_domain_irq(d)->unmaskable_msi_vecs);
> +        spin_unlock_irq(&desc->lock);
> +
>          spin_unlock(&d->event_lock);
> 
>          pirq_dpci->gmsi.posted = false;
> @@ -869,7 +874,8 @@ static int _hvm_dpci_msi_eoi(struct domain *d,
> 
>  void hvm_dpci_msi_eoi(struct domain *d, int vector)
>  {
> -    if ( !iommu_enabled || !hvm_domain_irq(d)->dpci )
> +    if ( !iommu_enabled || !hvm_domain_irq(d)->dpci ||
> +         !test_bit(vector, hvm_domain_irq(d)->unmaskable_msi_vecs) )
>         return;
> 
>      spin_lock(&d->event_lock);
> diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-
> x86/hvm/irq.h
> index 8a43cb9..d9d4652 100644
> --- a/xen/include/asm-x86/hvm/irq.h
> +++ b/xen/include/asm-x86/hvm/irq.h
> @@ -78,6 +78,7 @@ struct hvm_irq {
>      u8 round_robin_prev_vcpu;
> 
>      struct hvm_irq_dpci *dpci;
> +    DECLARE_BITMAP(unmaskable_msi_vecs, 256);
> 
>      /*
>       * Number of wires asserting each GSI.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel