Re: [PATCH v4 05/12] x86/hvm: allowing registering EOI callbacks for GSIs

2021-05-04 Thread Roger Pau Monné 
On Mon, May 03, 2021 at 05:50:39PM +0200, Jan Beulich wrote:
> On 20.04.2021 16:07, Roger Pau Monne wrote:
> > Such callbacks will be executed once a EOI is performed by the guest,
> > regardless of whether the interrupts are injected from the vIO-APIC or
> > the vPIC, as ISA IRQs are translated to GSIs and then the
> > corresponding callback is executed at EOI.
> > 
> > The vIO-APIC infrastructure for handling EOIs is build on top of the
> > existing vlapic EOI callback functionality, while the vPIC one is
> > handled when writing to the vPIC EOI register.
> > 
> > Note that such callbacks need to be registered and de-registered, and
> > that a single GSI can have multiple callbacks associated. That's
> > because GSIs can be level triggered and shared, as that's the case
> > with legacy PCI interrupts shared between several devices.
> > 
> > Strictly speaking this is a non-functional change, since there are no
> > users of this new interface introduced by this change.
> > 
> > Signed-off-by: Roger Pau Monné 
> 
> In principle, as everything looks functionally correct to me,
> Reviewed-by: Jan Beulich 
> 
> Nevertheless, besides a few remarks further down, I have to admit I'm
> concerned of the direct-to-indirect calls conversion (not just here,
> but also covering earlier patches), which (considering we're talking
> of EOI) I expect may occur quite frequently for at least some guests.

I would expect the vmexit cost for each EOI would shadow any gain
between using direct vs indirect calls.

> There aren't that many different callback functions which get
> registered, are there? Hence I wonder whether enumerating them and
> picking the right one via, say, an enum wouldn't be more efficient
> and still allow elimination of (in the case here) unconditional calls
> to hvm_dpci_eoi() for every EOI.

So for the vlapic (vector) callbacks we have the current consumers:
 - MSI passthrough.
 - vPT.
 - IO-APIC.

For GSI callbacks we have:
 - GSI passthrough.
 - vPT.

I could see about implementing this.

This is also kind of blocked on the RTC stuff, since vPT cannot be
migrated to this new model unless we remove strict_mode or changfe the
logic here to allow GSI callbacks to de-register themselves.

> 
> > --- a/xen/arch/x86/hvm/irq.c
> > +++ b/xen/arch/x86/hvm/irq.c
> > @@ -595,6 +595,81 @@ int hvm_local_events_need_delivery(struct vcpu *v)
> >  return !hvm_interrupt_blocked(v, intack);
> >  }
> >  
> > +int hvm_gsi_register_callback(struct domain *d, unsigned int gsi,
> > +  struct hvm_gsi_eoi_callback *cb)
> > +{
> > +struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> > +
> > +if ( gsi >= hvm_irq->nr_gsis )
> > +{
> > +ASSERT_UNREACHABLE();
> > +return -EINVAL;
> > +}
> > +
> > +write_lock(_irq->gsi_callbacks_lock);
> > +list_add(>list, _irq->gsi_callbacks[gsi]);
> > +write_unlock(_irq->gsi_callbacks_lock);
> > +
> > +return 0;
> > +}
> > +
> > +int hvm_gsi_unregister_callback(struct domain *d, unsigned int gsi,
> > +struct hvm_gsi_eoi_callback *cb)
> > +{
> > +struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> > +const struct list_head *tmp;
> > +bool found = false;
> > +
> > +if ( gsi >= hvm_irq->nr_gsis )
> > +{
> > +ASSERT_UNREACHABLE();
> > +return -EINVAL;
> > +}
> > +
> > +write_lock(_irq->gsi_callbacks_lock);
> > +list_for_each ( tmp, _irq->gsi_callbacks[gsi] )
> > +if ( tmp == >list )
> > +{
> > +list_del(>list);
> 
> Minor remark: Would passing "tmp" here lead to better generated
> code?

Maybe? I don't mind doing so.

> > @@ -419,13 +421,25 @@ static void eoi_callback(struct vcpu *v, unsigned int 
> > vector, void *data)
> >  if ( is_iommu_enabled(d) )
> >  {
> >  spin_unlock(>arch.hvm.irq_lock);
> > -hvm_dpci_eoi(d, vioapic->base_gsi + pin);
> > +hvm_dpci_eoi(d, gsi);
> >  spin_lock(>arch.hvm.irq_lock);
> >  }
> >  
> > +/*
> > + * Callbacks don't expect to be executed with any lock held, so
> > + * drop the lock that protects the vIO-APIC fields from 
> > changing.
> > + *
> > + * Note that the redirection entry itself cannot go away, so 
> > upon
> > + * retaking the lock we only need to avoid making assumptions 
> > on
> > + * redirection entry field values (ie: recheck the IRR field).
> > + */
> > +spin_unlock(>arch.hvm.irq_lock);
> > +hvm_gsi_execute_callbacks(d, gsi);
> > +spin_lock(>arch.hvm.irq_lock);
> 
> While this may be transient in the series, as said before I'm not
> happy about this double unlock/relock sequence. I didn't really
> understand what would be wrong with
> 
> spin_unlock(>arch.hvm.irq_lock);
> if ( is_iommu_enabled(d) )
>

Re: [PATCH v4 05/12] x86/hvm: allowing registering EOI callbacks for GSIs

2021-05-03 Thread Jan Beulich 
On 20.04.2021 16:07, Roger Pau Monne wrote:
> Such callbacks will be executed once a EOI is performed by the guest,
> regardless of whether the interrupts are injected from the vIO-APIC or
> the vPIC, as ISA IRQs are translated to GSIs and then the
> corresponding callback is executed at EOI.
> 
> The vIO-APIC infrastructure for handling EOIs is build on top of the
> existing vlapic EOI callback functionality, while the vPIC one is
> handled when writing to the vPIC EOI register.
> 
> Note that such callbacks need to be registered and de-registered, and
> that a single GSI can have multiple callbacks associated. That's
> because GSIs can be level triggered and shared, as that's the case
> with legacy PCI interrupts shared between several devices.
> 
> Strictly speaking this is a non-functional change, since there are no
> users of this new interface introduced by this change.
> 
> Signed-off-by: Roger Pau Monné 

In principle, as everything looks functionally correct to me,
Reviewed-by: Jan Beulich 

Nevertheless, besides a few remarks further down, I have to admit I'm
concerned of the direct-to-indirect calls conversion (not just here,
but also covering earlier patches), which (considering we're talking
of EOI) I expect may occur quite frequently for at least some guests.
There aren't that many different callback functions which get
registered, are there? Hence I wonder whether enumerating them and
picking the right one via, say, an enum wouldn't be more efficient
and still allow elimination of (in the case here) unconditional calls
to hvm_dpci_eoi() for every EOI.

> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -595,6 +595,81 @@ int hvm_local_events_need_delivery(struct vcpu *v)
>  return !hvm_interrupt_blocked(v, intack);
>  }
>  
> +int hvm_gsi_register_callback(struct domain *d, unsigned int gsi,
> +  struct hvm_gsi_eoi_callback *cb)
> +{
> +struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> +
> +if ( gsi >= hvm_irq->nr_gsis )
> +{
> +ASSERT_UNREACHABLE();
> +return -EINVAL;
> +}
> +
> +write_lock(_irq->gsi_callbacks_lock);
> +list_add(>list, _irq->gsi_callbacks[gsi]);
> +write_unlock(_irq->gsi_callbacks_lock);
> +
> +return 0;
> +}
> +
> +int hvm_gsi_unregister_callback(struct domain *d, unsigned int gsi,
> +struct hvm_gsi_eoi_callback *cb)
> +{
> +struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> +const struct list_head *tmp;
> +bool found = false;
> +
> +if ( gsi >= hvm_irq->nr_gsis )
> +{
> +ASSERT_UNREACHABLE();
> +return -EINVAL;
> +}
> +
> +write_lock(_irq->gsi_callbacks_lock);
> +list_for_each ( tmp, _irq->gsi_callbacks[gsi] )
> +if ( tmp == >list )
> +{
> +list_del(>list);

Minor remark: Would passing "tmp" here lead to better generated
code?

> @@ -419,13 +421,25 @@ static void eoi_callback(struct vcpu *v, unsigned int 
> vector, void *data)
>  if ( is_iommu_enabled(d) )
>  {
>  spin_unlock(>arch.hvm.irq_lock);
> -hvm_dpci_eoi(d, vioapic->base_gsi + pin);
> +hvm_dpci_eoi(d, gsi);
>  spin_lock(>arch.hvm.irq_lock);
>  }
>  
> +/*
> + * Callbacks don't expect to be executed with any lock held, so
> + * drop the lock that protects the vIO-APIC fields from changing.
> + *
> + * Note that the redirection entry itself cannot go away, so upon
> + * retaking the lock we only need to avoid making assumptions on
> + * redirection entry field values (ie: recheck the IRR field).
> + */
> +spin_unlock(>arch.hvm.irq_lock);
> +hvm_gsi_execute_callbacks(d, gsi);
> +spin_lock(>arch.hvm.irq_lock);

While this may be transient in the series, as said before I'm not
happy about this double unlock/relock sequence. I didn't really
understand what would be wrong with

spin_unlock(>arch.hvm.irq_lock);
if ( is_iommu_enabled(d) )
hvm_dpci_eoi(d, gsi);
hvm_gsi_execute_callbacks(d, gsi);
spin_lock(>arch.hvm.irq_lock);

This in particular wouldn't grow but even shrink the later patch
dropping the call to hvm_dpci_eoi().

> --- a/xen/arch/x86/hvm/vpic.c
> +++ b/xen/arch/x86/hvm/vpic.c
> @@ -235,6 +235,8 @@ static void vpic_ioport_write(
>  unsigned int pin = __scanbit(pending, 8);
>  
>  ASSERT(pin < 8);
> +hvm_gsi_execute_callbacks(current->domain,
> +hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin));
>  hvm_dpci_eoi(current->domain,
>   hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : 
> pin));
>  __clear_bit(pin, );
> @@ -285,6 +287,8 @@ static void vpic_ioport_write(
>  /* Release lock

[PATCH v4 05/12] x86/hvm: allowing registering EOI callbacks for GSIs

2021-04-20 Thread Roger Pau Monne 
Such callbacks will be executed once a EOI is performed by the guest,
regardless of whether the interrupts are injected from the vIO-APIC or
the vPIC, as ISA IRQs are translated to GSIs and then the
corresponding callback is executed at EOI.

The vIO-APIC infrastructure for handling EOIs is build on top of the
existing vlapic EOI callback functionality, while the vPIC one is
handled when writing to the vPIC EOI register.

Note that such callbacks need to be registered and de-registered, and
that a single GSI can have multiple callbacks associated. That's
because GSIs can be level triggered and shared, as that's the case
with legacy PCI interrupts shared between several devices.

Strictly speaking this is a non-functional change, since there are no
users of this new interface introduced by this change.

Signed-off-by: Roger Pau Monné 
---
Changes since v3:
 - Make callback take a domain parameter.
 - Return whether the unregistered callback was found.
 - Add a comment regarding the result of hvm_gsi_has_callbacks being
   stable.

Changes since v2:
 - Latch hvm_domain_irq in some functions.
 - Make domain parameter of hvm_gsi_has_callbacks const.
 - Add comment about dropping the lock around the
   hvm_gsi_execute_callbacks call.
 - Drop change to ioapic_load.

Changes since v1:
 - New in this version.
---
 xen/arch/x86/hvm/hvm.c| 15 ++-
 xen/arch/x86/hvm/irq.c| 75 +++
 xen/arch/x86/hvm/vioapic.c| 29 +++---
 xen/arch/x86/hvm/vpic.c   |  4 ++
 xen/include/asm-x86/hvm/irq.h | 21 ++
 5 files changed, 137 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ae37bc434ae..2c4dd1b86f2 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -608,7 +608,7 @@ static int hvm_print_line(
 
 int hvm_domain_initialise(struct domain *d)
 {
-unsigned int nr_gsis;
+unsigned int nr_gsis, i;
 int rc;
 
 if ( !hvm_enabled )
@@ -656,6 +656,14 @@ int hvm_domain_initialise(struct domain *d)
 BUILD_BUG_ON(NR_HVM_DOMU_IRQS < NR_ISAIRQS);
 ASSERT(hvm_domain_irq(d)->nr_gsis >= NR_ISAIRQS);
 
+/* Initialize the EOI callback list. */
+hvm_domain_irq(d)->gsi_callbacks = xmalloc_array(struct list_head, 
nr_gsis);
+if ( !hvm_domain_irq(d)->gsi_callbacks )
+goto fail1;
+rwlock_init(_domain_irq(d)->gsi_callbacks_lock);
+for ( i = 0; i < nr_gsis; i++ )
+INIT_LIST_HEAD(_domain_irq(d)->gsi_callbacks[i]);
+
 /* need link to containing domain */
 d->arch.hvm.pl_time->domain = d;
 
@@ -715,6 +723,8 @@ int hvm_domain_initialise(struct domain *d)
  fail1:
 if ( is_hardware_domain(d) )
 xfree(d->arch.hvm.io_bitmap);
+if ( hvm_domain_irq(d) )
+XFREE(hvm_domain_irq(d)->gsi_callbacks);
 XFREE(d->arch.hvm.params);
 XFREE(d->arch.hvm.irq);
  fail0:
@@ -777,6 +787,9 @@ void hvm_domain_destroy(struct domain *d)
 vioapic_deinit(d);
 
 XFREE(d->arch.hvm.pl_time);
+
+if ( hvm_domain_irq(d) )
+XFREE(hvm_domain_irq(d)->gsi_callbacks);
 XFREE(d->arch.hvm.irq);
 
 list_for_each_safe ( ioport_list, tmp, >arch.hvm.g2m_ioport_list )
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 38ac5fb6c7c..4825a387bdc 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -595,6 +595,81 @@ int hvm_local_events_need_delivery(struct vcpu *v)
 return !hvm_interrupt_blocked(v, intack);
 }
 
+int hvm_gsi_register_callback(struct domain *d, unsigned int gsi,
+  struct hvm_gsi_eoi_callback *cb)
+{
+struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+
+if ( gsi >= hvm_irq->nr_gsis )
+{
+ASSERT_UNREACHABLE();
+return -EINVAL;
+}
+
+write_lock(_irq->gsi_callbacks_lock);
+list_add(>list, _irq->gsi_callbacks[gsi]);
+write_unlock(_irq->gsi_callbacks_lock);
+
+return 0;
+}
+
+int hvm_gsi_unregister_callback(struct domain *d, unsigned int gsi,
+struct hvm_gsi_eoi_callback *cb)
+{
+struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+const struct list_head *tmp;
+bool found = false;
+
+if ( gsi >= hvm_irq->nr_gsis )
+{
+ASSERT_UNREACHABLE();
+return -EINVAL;
+}
+
+write_lock(_irq->gsi_callbacks_lock);
+list_for_each ( tmp, _irq->gsi_callbacks[gsi] )
+if ( tmp == >list )
+{
+list_del(>list);
+found = true;
+break;
+}
+write_unlock(_irq->gsi_callbacks_lock);
+
+return found ? 0 : -ENOENT;
+}
+
+void hvm_gsi_execute_callbacks(struct domain *d, unsigned int gsi)
+{
+struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+struct hvm_gsi_eoi_callback *cb;
+
+read_lock(_irq->gsi_callbacks_lock);
+list_for_each_entry ( cb, _irq->gsi_callbacks[gsi], list )
+cb->callback(d, gsi, cb->data);
+read_unlock(_irq->gsi_callbacks_lock);
+}
+
+bool hvm_gsi_has_callbacks(const struct domain *d,