[PATCH 0/2] use pirq_eoi_map in modern Linux kernels

[PATCH 0/2] use pirq_eoi_map in modern Linux kernels

[Stefano Stabellini]

Hi all,
this small patch series consists of two patches: a patch for Xen and a
patch for Linux.

The Xen patch implements PHYSDEVOP_pirq_eoi_gmfn_new, a new hypercall
similar to PHYSDEVOP_pirq_eoi_gmfn but it does not change the semantics
of PHYSDEVOP_eoi.

The Linux patch introduces pirq_eoi_map in drivers/xen/events.c, using
PHYSDEVOP_pirq_eoi_gmfn_new.

Cheers,

Stefano

[PATCH 1/2] xen: Introduce PHYSDEVOP_pirq_eoi_gmfn_new

[Stefano Stabellini]

PHYSDEVOP_pirq_eoi_gmfn changes the semantics of PHYSDEVOP_eoi.
Introduce PHYSDEVOP_pirq_eoi_gmfn_new, that is like
PHYSDEVOP_pirq_eoi_gmfn but it doesn't modify the behaviour of another
hypercall.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/ia64/xen/domain.c    |    1 +
 xen/arch/ia64/xen/hypercall.c |    5 ++++-
 xen/arch/x86/domain.c         |    1 +
 xen/arch/x86/physdev.c        |    6 +++++-
 xen/include/asm-ia64/domain.h |    3 +++
 xen/include/asm-x86/domain.h  |    3 +++
 xen/include/public/physdev.h  |    8 ++++++++
 7 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/xen/arch/ia64/xen/domain.c b/xen/arch/ia64/xen/domain.c
index 1ea5a90..a31bd32 100644
--- a/xen/arch/ia64/xen/domain.c
+++ b/xen/arch/ia64/xen/domain.c
@@ -1731,6 +1731,7 @@ int domain_relinquish_resources(struct domain *d)
 		if (d->arch.pirq_eoi_map != NULL) {
 			put_page(virt_to_page(d->arch.pirq_eoi_map));
 			d->arch.pirq_eoi_map = NULL;
+			d->arch.auto_unmask = 0;
 		}
 
 		/* Tear down shadow mode stuff. */
diff --git a/xen/arch/ia64/xen/hypercall.c b/xen/arch/ia64/xen/hypercall.c
index 130675e..44c3407 100644
--- a/xen/arch/ia64/xen/hypercall.c
+++ b/xen/arch/ia64/xen/hypercall.c
@@ -65,7 +65,7 @@ static long __do_pirq_guest_eoi(struct domain *d, int pirq)
 {
 	if ( pirq < 0 || pirq >= NR_IRQS )
 		return -EINVAL;
-	if ( d->arch.pirq_eoi_map ) {
+	if ( d->arch.pirq_eoi_map  && d->arch.auto_unmask ) {
 		spin_lock(&d->event_lock);
 		evtchn_unmask(pirq_to_evtchn(d, pirq));
 		spin_unlock(&d->event_lock);
@@ -508,6 +508,7 @@ long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
         break;
     }
 
+    case PHYSDEVOP_pirq_eoi_gmfn_new:
     case PHYSDEVOP_pirq_eoi_gmfn: {
         struct physdev_pirq_eoi_gmfn info;
         unsigned long mfn;
@@ -531,6 +532,8 @@ long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
         }
 
         current->domain->arch.pirq_eoi_map = mfn_to_virt(mfn);
+		if ( cmd == PHYSDEVOP_pirq_eoi_gmfn )
+			current->domain->arch.auto_unmask = 1;
         ret = 0;
         break;
     }
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 61d83c8..a540af7 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2125,6 +2125,7 @@ int domain_relinquish_resources(struct domain *d)
                 put_page_and_type(
                     mfn_to_page(d->arch.pv_domain.pirq_eoi_map_mfn));
                 d->arch.pv_domain.pirq_eoi_map = NULL;
+                d->arch.pv_domain.auto_unmask = 0;
             }
         }
 
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index f280c28..efd517f 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -271,7 +271,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
             break;
         }
         if ( !is_hvm_domain(v->domain) &&
-             v->domain->arch.pv_domain.pirq_eoi_map )
+             v->domain->arch.pv_domain.pirq_eoi_map &&
+             v->domain->arch.pv_domain.auto_unmask )
             evtchn_unmask(pirq->evtchn);
         if ( !is_hvm_domain(v->domain) ||
              domain_pirq_to_irq(v->domain, eoi.irq) > 0 )
@@ -293,6 +294,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
         break;
     }
 
+    case PHYSDEVOP_pirq_eoi_gmfn_new:
     case PHYSDEVOP_pirq_eoi_gmfn: {
         struct physdev_pirq_eoi_gmfn info;
         unsigned long mfn;
@@ -329,6 +331,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
             ret = -ENOSPC;
             break;
         }
+        if ( cmd == PHYSDEVOP_pirq_eoi_gmfn )
+            v->domain->arch.pv_domain.auto_unmask = 1;
 
         put_gfn(current->domain, info.gmfn);
         ret = 0;
diff --git a/xen/include/asm-ia64/domain.h b/xen/include/asm-ia64/domain.h
index 12dc3bd..8163d67 100644
--- a/xen/include/asm-ia64/domain.h
+++ b/xen/include/asm-ia64/domain.h
@@ -186,6 +186,9 @@ struct arch_domain {
     /* Shared page for notifying that explicit PIRQ EOI is required. */
     unsigned long *pirq_eoi_map;
     unsigned long pirq_eoi_map_mfn;
+	/* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
+	 * unmask the event channel */
+	unsigned int auto_unmask;
 
     /* Address of efi_runtime_services_t (placed in domain memory)  */
     void *efi_runtime;
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 00bbaeb..b0cbe65 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -231,6 +231,9 @@ struct pv_domain
     /* Shared page for notifying that explicit PIRQ EOI is required. */
     unsigned long *pirq_eoi_map;
     unsigned long pirq_eoi_map_mfn;
+    /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
+     * unmask the event channel */
+    unsigned int auto_unmask;
 
     /* Pseudophysical e820 map (XENMEM_memory_map).  */
     spinlock_t e820_lock;
diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
index 6e23295..d555719 100644
--- a/xen/include/public/physdev.h
+++ b/xen/include/public/physdev.h
@@ -50,6 +50,14 @@ DEFINE_XEN_GUEST_HANDLE(physdev_eoi_t);
  * array indexed by Xen's PIRQ value.
  */
 #define PHYSDEVOP_pirq_eoi_gmfn         17
+/*
+ * Register a shared page for the hypervisor to indicate whether the
+ * guest must issue PHYSDEVOP_eoi. This hypercall is very similar to
+ * PHYSDEVOP_pirq_eoi_gmfn but it doesn't change the semantics of
+ * PHYSDEVOP_eoi. The page registered is used as a bit array indexed by
+ * Xen's PIRQ value.
+ */
+#define PHYSDEVOP_pirq_eoi_gmfn_new         28
 struct physdev_pirq_eoi_gmfn {
     /* IN */
     xen_pfn_t gmfn;
-- 
1.7.2.5

[PATCH 2/2] linux/xen: support pirq_eoi_map

[Stefano Stabellini]

The pirq_eoi_map is a bitmap offered by Xen to check which pirqs need to
be EOI'd without having to issue an hypercall every time.
We use the new PHYSDEVOP_pirq_eoi_gmfn_new to map the bitmap, so that
Xen does not change the semantics of PHYSDEVOP_eoi behind our backs.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/events.c            |   17 ++++++++++++++++-
 include/xen/interface/physdev.h |   22 ++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 6e075cd..2bca0b7 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -37,6 +37,7 @@
 #include <asm/idle.h>
 #include <asm/io_apic.h>
 #include <asm/sync_bitops.h>
+#include <asm/xen/page.h>
 #include <asm/xen/pci.h>
 #include <asm/xen/hypercall.h>
 #include <asm/xen/hypervisor.h>
@@ -108,6 +109,7 @@ struct irq_info {
 #define PIRQ_SHAREABLE	(1 << 1)
 
 static int *evtchn_to_irq;
+static unsigned long *pirq_eoi_map;
 
 static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
 		      cpu_evtchn_mask);
@@ -274,6 +276,9 @@ static bool pirq_needs_eoi(unsigned irq)
 
 	BUG_ON(info->type != IRQT_PIRQ);
 
+	if (pirq_eoi_map != NULL)
+		return test_bit(irq, pirq_eoi_map);
+
 	return info->u.pirq.flags & PIRQ_NEEDS_EOI;
 }
 
@@ -1693,7 +1698,7 @@ void xen_callback_vector(void) {}
 
 void __init xen_init_IRQ(void)
 {
-	int i;
+	int i, rc;
 
 	evtchn_to_irq = kcalloc(NR_EVENT_CHANNELS, sizeof(*evtchn_to_irq),
 				    GFP_KERNEL);
@@ -1714,8 +1719,18 @@ void __init xen_init_IRQ(void)
 		 * __acpi_register_gsi can point at the right function */
 		pci_xen_hvm_init();
 	} else {
+		struct physdev_pirq_eoi_gmfn eoi_gmfn;
+
 		irq_ctx_init(smp_processor_id());
 		if (xen_initial_domain())
 			pci_xen_initial_domain();
+
+		pirq_eoi_map = (void *)__get_free_page(GFP_KERNEL|__GFP_ZERO);
+		eoi_gmfn.gmfn = virt_to_mfn(pirq_eoi_map);
+		rc = HYPERVISOR_physdev_op(PHYSDEVOP_pirq_eoi_gmfn_new, &eoi_gmfn);
+		if (rc != 0) {
+			free_page((unsigned long) pirq_eoi_map);
+			pirq_eoi_map = NULL;
+		}
 	}
 }
diff --git a/include/xen/interface/physdev.h b/include/xen/interface/physdev.h
index c1080d9..ba11907 100644
--- a/include/xen/interface/physdev.h
+++ b/include/xen/interface/physdev.h
@@ -39,6 +39,28 @@ struct physdev_eoi {
 };
 
 /*
+ * Register a shared page for the hypervisor to indicate whether the guest
+ * must issue PHYSDEVOP_eoi. The semantics of PHYSDEVOP_eoi change slightly
+ * once the guest used this function in that the associated event channel
+ * will automatically get unmasked. The page registered is used as a bit
+ * array indexed by Xen's PIRQ value.
+ */
+#define PHYSDEVOP_pirq_eoi_gmfn         17
+/*
+ * Register a shared page for the hypervisor to indicate whether the
+ * guest must issue PHYSDEVOP_eoi. This hypercall is very similar to
+ * PHYSDEVOP_pirq_eoi_gmfn but it doesn't change the semantics of
+ * PHYSDEVOP_eoi. The page registered is used as a bit array indexed by
+ * Xen's PIRQ value.
+ */
+#define PHYSDEVOP_pirq_eoi_gmfn_new         28
+struct physdev_pirq_eoi_gmfn {
+    /* IN */
+    unsigned long gmfn;
+};
+
+
+/*
  * Query the status of an IRQ line.
  * @arg == pointer to physdev_irq_status_query structure.
  */
-- 
1.7.2.5

Re: [PATCH 1/2] xen: Introduce PHYSDEVOP_pirq_eoi_gmfn_new

[Jan Beulich]

>>> On 26.01.12 at 16:49, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> PHYSDEVOP_pirq_eoi_gmfn changes the semantics of PHYSDEVOP_eoi.
> Introduce PHYSDEVOP_pirq_eoi_gmfn_new, that is like
> PHYSDEVOP_pirq_eoi_gmfn but it doesn't modify the behaviour of another
> hypercall.

I keep forgetting why you think the auto-unmasking does any harm.
It was done that way to avoid an extra hypercall.

> @@ -531,6 +532,8 @@ long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>          }
>  
>          current->domain->arch.pirq_eoi_map = mfn_to_virt(mfn);
> +		if ( cmd == PHYSDEVOP_pirq_eoi_gmfn )
> +			current->domain->arch.auto_unmask = 1;

Indentation.,

>          ret = 0;
>          break;
>      }
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -271,7 +271,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>              break;
>          }
>          if ( !is_hvm_domain(v->domain) &&
> -             v->domain->arch.pv_domain.pirq_eoi_map )
> +             v->domain->arch.pv_domain.pirq_eoi_map &&
> +             v->domain->arch.pv_domain.auto_unmask )

Could you not avoid the checking of v->domain->arch.pv_domain.pirq_eoi_map
by making sure v->domain->arch.pv_domain.auto_unmask gets cleared
when the map gets destroyed (not sure if that is permitted at all).

>              evtchn_unmask(pirq->evtchn);
>          if ( !is_hvm_domain(v->domain) ||
>               domain_pirq_to_irq(v->domain, eoi.irq) > 0 )
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -231,6 +231,9 @@ struct pv_domain
>      /* Shared page for notifying that explicit PIRQ EOI is required. */
>      unsigned long *pirq_eoi_map;
>      unsigned long pirq_eoi_map_mfn;
> +    /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
> +     * unmask the event channel */
> +    unsigned int auto_unmask;

bool_t?

Jan

>  
>      /* Pseudophysical e820 map (XENMEM_memory_map).  */
>      spinlock_t e820_lock;

Re: [PATCH 1/2] xen: Introduce PHYSDEVOP_pirq_eoi_gmfn_new

[Keir Fraser]

On 26/01/2012 15:49, "Stefano Stabellini" <Stefano.Stabellini@eu.citrix.com>
wrote:

> PHYSDEVOP_pirq_eoi_gmfn changes the semantics of PHYSDEVOP_eoi.
> Introduce PHYSDEVOP_pirq_eoi_gmfn_new, that is like
> PHYSDEVOP_pirq_eoi_gmfn but it doesn't modify the behaviour of another
> hypercall.

It's nasty that pirq_eoi_gmfn has the side effect. I suggest add a PHYSDEVOP
to explicitly enable/disable unmask-on-eoi (i.e., the command accepts a
boolean parameter). Once it is explicitly enabled/disabled in this way,
pirq_eoi_gmfn no longer has the side effect (regardless of whether it is
called before or after the explicit setting). So e.g., pv_domain.auto_unmask
becomes an int where 0/1 means no/yes, and -1 means default (i.e., old
behavour where it depends on whether PHYSDEVOP_pirq_eoi_gmfn has been
called).

This seems to me to move a bad interface in a better direction.

 -- Keir

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/ia64/xen/domain.c    |    1 +
>  xen/arch/ia64/xen/hypercall.c |    5 ++++-
>  xen/arch/x86/domain.c         |    1 +
>  xen/arch/x86/physdev.c        |    6 +++++-
>  xen/include/asm-ia64/domain.h |    3 +++
>  xen/include/asm-x86/domain.h  |    3 +++
>  xen/include/public/physdev.h  |    8 ++++++++
>  7 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/ia64/xen/domain.c b/xen/arch/ia64/xen/domain.c
> index 1ea5a90..a31bd32 100644
> --- a/xen/arch/ia64/xen/domain.c
> +++ b/xen/arch/ia64/xen/domain.c
> @@ -1731,6 +1731,7 @@ int domain_relinquish_resources(struct domain *d)
> if (d->arch.pirq_eoi_map != NULL) {
> put_page(virt_to_page(d->arch.pirq_eoi_map));
> d->arch.pirq_eoi_map = NULL;
> +   d->arch.auto_unmask = 0;
> }
>  
> /* Tear down shadow mode stuff. */
> diff --git a/xen/arch/ia64/xen/hypercall.c b/xen/arch/ia64/xen/hypercall.c
> index 130675e..44c3407 100644
> --- a/xen/arch/ia64/xen/hypercall.c
> +++ b/xen/arch/ia64/xen/hypercall.c
> @@ -65,7 +65,7 @@ static long __do_pirq_guest_eoi(struct domain *d, int pirq)
>  {
> if ( pirq < 0 || pirq >= NR_IRQS )
> return -EINVAL;
> - if ( d->arch.pirq_eoi_map ) {
> + if ( d->arch.pirq_eoi_map  && d->arch.auto_unmask ) {
> spin_lock(&d->event_lock);
> evtchn_unmask(pirq_to_evtchn(d, pirq));
> spin_unlock(&d->event_lock);
> @@ -508,6 +508,7 @@ long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>          break;
>      }
>  
> +    case PHYSDEVOP_pirq_eoi_gmfn_new:
>      case PHYSDEVOP_pirq_eoi_gmfn: {
>          struct physdev_pirq_eoi_gmfn info;
>          unsigned long mfn;
> @@ -531,6 +532,8 @@ long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>          }
>  
>          current->domain->arch.pirq_eoi_map = mfn_to_virt(mfn);
> +  if ( cmd == PHYSDEVOP_pirq_eoi_gmfn )
> +   current->domain->arch.auto_unmask = 1;
>          ret = 0;
>          break;
>      }
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 61d83c8..a540af7 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -2125,6 +2125,7 @@ int domain_relinquish_resources(struct domain *d)
>                  put_page_and_type(
>                      mfn_to_page(d->arch.pv_domain.pirq_eoi_map_mfn));
>                  d->arch.pv_domain.pirq_eoi_map = NULL;
> +                d->arch.pv_domain.auto_unmask = 0;
>              }
>          }
>  
> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> index f280c28..efd517f 100644
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -271,7 +271,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>              break;
>          }
>          if ( !is_hvm_domain(v->domain) &&
> -             v->domain->arch.pv_domain.pirq_eoi_map )
> +             v->domain->arch.pv_domain.pirq_eoi_map &&
> +             v->domain->arch.pv_domain.auto_unmask )
>              evtchn_unmask(pirq->evtchn);
>          if ( !is_hvm_domain(v->domain) ||
>               domain_pirq_to_irq(v->domain, eoi.irq) > 0 )
> @@ -293,6 +294,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>          break;
>      }
>  
> +    case PHYSDEVOP_pirq_eoi_gmfn_new:
>      case PHYSDEVOP_pirq_eoi_gmfn: {
>          struct physdev_pirq_eoi_gmfn info;
>          unsigned long mfn;
> @@ -329,6 +331,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>              ret = -ENOSPC;
>              break;
>          }
> +        if ( cmd == PHYSDEVOP_pirq_eoi_gmfn )
> +            v->domain->arch.pv_domain.auto_unmask = 1;
>  
>          put_gfn(current->domain, info.gmfn);
>          ret = 0;
> diff --git a/xen/include/asm-ia64/domain.h b/xen/include/asm-ia64/domain.h
> index 12dc3bd..8163d67 100644
> --- a/xen/include/asm-ia64/domain.h
> +++ b/xen/include/asm-ia64/domain.h
> @@ -186,6 +186,9 @@ struct arch_domain {
>      /* Shared page for notifying that explicit PIRQ EOI is required. */
>      unsigned long *pirq_eoi_map;
>      unsigned long pirq_eoi_map_mfn;
> + /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
> +  * unmask the event channel */
> + unsigned int auto_unmask;
>  
>      /* Address of efi_runtime_services_t (placed in domain memory)  */
>      void *efi_runtime;
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 00bbaeb..b0cbe65 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -231,6 +231,9 @@ struct pv_domain
>      /* Shared page for notifying that explicit PIRQ EOI is required. */
>      unsigned long *pirq_eoi_map;
>      unsigned long pirq_eoi_map_mfn;
> +    /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
> +     * unmask the event channel */
> +    unsigned int auto_unmask;
>  
>      /* Pseudophysical e820 map (XENMEM_memory_map).  */
>      spinlock_t e820_lock;
> diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
> index 6e23295..d555719 100644
> --- a/xen/include/public/physdev.h
> +++ b/xen/include/public/physdev.h
> @@ -50,6 +50,14 @@ DEFINE_XEN_GUEST_HANDLE(physdev_eoi_t);
>   * array indexed by Xen's PIRQ value.
>   */
>  #define PHYSDEVOP_pirq_eoi_gmfn         17
> +/*
> + * Register a shared page for the hypervisor to indicate whether the
> + * guest must issue PHYSDEVOP_eoi. This hypercall is very similar to
> + * PHYSDEVOP_pirq_eoi_gmfn but it doesn't change the semantics of
> + * PHYSDEVOP_eoi. The page registered is used as a bit array indexed by
> + * Xen's PIRQ value.
> + */
> +#define PHYSDEVOP_pirq_eoi_gmfn_new         28
>  struct physdev_pirq_eoi_gmfn {
>      /* IN */
>      xen_pfn_t gmfn;

Re: [PATCH 1/2] xen: Introduce PHYSDEVOP_pirq_eoi_gmfn_new

[Stefano Stabellini]

On Thu, 26 Jan 2012, Jan Beulich wrote:
> >>> On 26.01.12 at 16:49, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > PHYSDEVOP_pirq_eoi_gmfn changes the semantics of PHYSDEVOP_eoi.
> > Introduce PHYSDEVOP_pirq_eoi_gmfn_new, that is like
> > PHYSDEVOP_pirq_eoi_gmfn but it doesn't modify the behaviour of another
> > hypercall.
> 
> I keep forgetting why you think the auto-unmasking does any harm.
> It was done that way to avoid an extra hypercall.

We let Linux mask/unmask the event channel whenever it would make/unmask
the irq, so an automatic unmask is equivalent to unmasking an irq
without Linux knowing or wanting to do so.


> > @@ -531,6 +532,8 @@ long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
> >          }
> >  
> >          current->domain->arch.pirq_eoi_map = mfn_to_virt(mfn);
> > +		if ( cmd == PHYSDEVOP_pirq_eoi_gmfn )
> > +			current->domain->arch.auto_unmask = 1;
> 
> Indentation.,
>
> >          ret = 0;
> >          break;
> >      }
> > --- a/xen/arch/x86/physdev.c
> > +++ b/xen/arch/x86/physdev.c
> > @@ -271,7 +271,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
> >              break;
> >          }
> >          if ( !is_hvm_domain(v->domain) &&
> > -             v->domain->arch.pv_domain.pirq_eoi_map )
> > +             v->domain->arch.pv_domain.pirq_eoi_map &&
> > +             v->domain->arch.pv_domain.auto_unmask )
> 
> Could you not avoid the checking of v->domain->arch.pv_domain.pirq_eoi_map
> by making sure v->domain->arch.pv_domain.auto_unmask gets cleared
> when the map gets destroyed (not sure if that is permitted at all).

I think I can, I'll change it that way.


> >              evtchn_unmask(pirq->evtchn);
> >          if ( !is_hvm_domain(v->domain) ||
> >               domain_pirq_to_irq(v->domain, eoi.irq) > 0 )
> > --- a/xen/include/asm-x86/domain.h
> > +++ b/xen/include/asm-x86/domain.h
> > @@ -231,6 +231,9 @@ struct pv_domain
> >      /* Shared page for notifying that explicit PIRQ EOI is required. */
> >      unsigned long *pirq_eoi_map;
> >      unsigned long pirq_eoi_map_mfn;
> > +    /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
> > +     * unmask the event channel */
> > +    unsigned int auto_unmask;
> 
> bool_t?

good idea

Re: [PATCH 1/2] xen: Introduce PHYSDEVOP_pirq_eoi_gmfn_new

[Keir Fraser]

On 26/01/2012 16:11, "Keir Fraser" <keir@xen.org> wrote:

> On 26/01/2012 15:49, "Stefano Stabellini" <Stefano.Stabellini@eu.citrix.com>
> wrote:
> 
>> PHYSDEVOP_pirq_eoi_gmfn changes the semantics of PHYSDEVOP_eoi.
>> Introduce PHYSDEVOP_pirq_eoi_gmfn_new, that is like
>> PHYSDEVOP_pirq_eoi_gmfn but it doesn't modify the behaviour of another
>> hypercall.
> 
> It's nasty that pirq_eoi_gmfn has the side effect. I suggest add a PHYSDEVOP
> to explicitly enable/disable unmask-on-eoi (i.e., the command accepts a
> boolean parameter). Once it is explicitly enabled/disabled in this way,
> pirq_eoi_gmfn no longer has the side effect (regardless of whether it is
> called before or after the explicit setting). So e.g., pv_domain.auto_unmask
> becomes an int where 0/1 means no/yes, and -1 means default (i.e., old
> behavour where it depends on whether PHYSDEVOP_pirq_eoi_gmfn has been
> called).
> 
> This seems to me to move a bad interface in a better direction.

...in that the auto-unmask behaviour becomes explicitly selectable, rather
than implicitly, via two different commands for setting *something else*
which only differ in a side effect. That's kind of as gross as what we have
already, or worse.

 -- Keir

>  -- Keir
> 
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> ---
>>  xen/arch/ia64/xen/domain.c    |    1 +
>>  xen/arch/ia64/xen/hypercall.c |    5 ++++-
>>  xen/arch/x86/domain.c         |    1 +
>>  xen/arch/x86/physdev.c        |    6 +++++-
>>  xen/include/asm-ia64/domain.h |    3 +++
>>  xen/include/asm-x86/domain.h  |    3 +++
>>  xen/include/public/physdev.h  |    8 ++++++++
>>  7 files changed, 25 insertions(+), 2 deletions(-)
>> 
>> diff --git a/xen/arch/ia64/xen/domain.c b/xen/arch/ia64/xen/domain.c
>> index 1ea5a90..a31bd32 100644
>> --- a/xen/arch/ia64/xen/domain.c
>> +++ b/xen/arch/ia64/xen/domain.c
>> @@ -1731,6 +1731,7 @@ int domain_relinquish_resources(struct domain *d)
>> if (d->arch.pirq_eoi_map != NULL) {
>> put_page(virt_to_page(d->arch.pirq_eoi_map));
>> d->arch.pirq_eoi_map = NULL;
>> +   d->arch.auto_unmask = 0;
>> }
>>  
>> /* Tear down shadow mode stuff. */
>> diff --git a/xen/arch/ia64/xen/hypercall.c b/xen/arch/ia64/xen/hypercall.c
>> index 130675e..44c3407 100644
>> --- a/xen/arch/ia64/xen/hypercall.c
>> +++ b/xen/arch/ia64/xen/hypercall.c
>> @@ -65,7 +65,7 @@ static long __do_pirq_guest_eoi(struct domain *d, int pirq)
>>  {
>> if ( pirq < 0 || pirq >= NR_IRQS )
>> return -EINVAL;
>> - if ( d->arch.pirq_eoi_map ) {
>> + if ( d->arch.pirq_eoi_map  && d->arch.auto_unmask ) {
>> spin_lock(&d->event_lock);
>> evtchn_unmask(pirq_to_evtchn(d, pirq));
>> spin_unlock(&d->event_lock);
>> @@ -508,6 +508,7 @@ long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>>          break;
>>      }
>>  
>> +    case PHYSDEVOP_pirq_eoi_gmfn_new:
>>      case PHYSDEVOP_pirq_eoi_gmfn: {
>>          struct physdev_pirq_eoi_gmfn info;
>>          unsigned long mfn;
>> @@ -531,6 +532,8 @@ long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>>          }
>>  
>>          current->domain->arch.pirq_eoi_map = mfn_to_virt(mfn);
>> +  if ( cmd == PHYSDEVOP_pirq_eoi_gmfn )
>> +   current->domain->arch.auto_unmask = 1;
>>          ret = 0;
>>          break;
>>      }
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index 61d83c8..a540af7 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -2125,6 +2125,7 @@ int domain_relinquish_resources(struct domain *d)
>>                  put_page_and_type(
>>                      mfn_to_page(d->arch.pv_domain.pirq_eoi_map_mfn));
>>                  d->arch.pv_domain.pirq_eoi_map = NULL;
>> +                d->arch.pv_domain.auto_unmask = 0;
>>              }
>>          }
>>  
>> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
>> index f280c28..efd517f 100644
>> --- a/xen/arch/x86/physdev.c
>> +++ b/xen/arch/x86/physdev.c
>> @@ -271,7 +271,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>>              break;
>>          }
>>          if ( !is_hvm_domain(v->domain) &&
>> -             v->domain->arch.pv_domain.pirq_eoi_map )
>> +             v->domain->arch.pv_domain.pirq_eoi_map &&
>> +             v->domain->arch.pv_domain.auto_unmask )
>>              evtchn_unmask(pirq->evtchn);
>>          if ( !is_hvm_domain(v->domain) ||
>>               domain_pirq_to_irq(v->domain, eoi.irq) > 0 )
>> @@ -293,6 +294,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>>          break;
>>      }
>>  
>> +    case PHYSDEVOP_pirq_eoi_gmfn_new:
>>      case PHYSDEVOP_pirq_eoi_gmfn: {
>>          struct physdev_pirq_eoi_gmfn info;
>>          unsigned long mfn;
>> @@ -329,6 +331,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>>              ret = -ENOSPC;
>>              break;
>>          }
>> +        if ( cmd == PHYSDEVOP_pirq_eoi_gmfn )
>> +            v->domain->arch.pv_domain.auto_unmask = 1;
>>  
>>          put_gfn(current->domain, info.gmfn);
>>          ret = 0;
>> diff --git a/xen/include/asm-ia64/domain.h b/xen/include/asm-ia64/domain.h
>> index 12dc3bd..8163d67 100644
>> --- a/xen/include/asm-ia64/domain.h
>> +++ b/xen/include/asm-ia64/domain.h
>> @@ -186,6 +186,9 @@ struct arch_domain {
>>      /* Shared page for notifying that explicit PIRQ EOI is required. */
>>      unsigned long *pirq_eoi_map;
>>      unsigned long pirq_eoi_map_mfn;
>> + /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
>> +  * unmask the event channel */
>> + unsigned int auto_unmask;
>>  
>>      /* Address of efi_runtime_services_t (placed in domain memory)  */
>>      void *efi_runtime;
>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>> index 00bbaeb..b0cbe65 100644
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -231,6 +231,9 @@ struct pv_domain
>>      /* Shared page for notifying that explicit PIRQ EOI is required. */
>>      unsigned long *pirq_eoi_map;
>>      unsigned long pirq_eoi_map_mfn;
>> +    /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
>> +     * unmask the event channel */
>> +    unsigned int auto_unmask;
>>  
>>      /* Pseudophysical e820 map (XENMEM_memory_map).  */
>>      spinlock_t e820_lock;
>> diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
>> index 6e23295..d555719 100644
>> --- a/xen/include/public/physdev.h
>> +++ b/xen/include/public/physdev.h
>> @@ -50,6 +50,14 @@ DEFINE_XEN_GUEST_HANDLE(physdev_eoi_t);
>>   * array indexed by Xen's PIRQ value.
>>   */
>>  #define PHYSDEVOP_pirq_eoi_gmfn         17
>> +/*
>> + * Register a shared page for the hypervisor to indicate whether the
>> + * guest must issue PHYSDEVOP_eoi. This hypercall is very similar to
>> + * PHYSDEVOP_pirq_eoi_gmfn but it doesn't change the semantics of
>> + * PHYSDEVOP_eoi. The page registered is used as a bit array indexed by
>> + * Xen's PIRQ value.
>> + */
>> +#define PHYSDEVOP_pirq_eoi_gmfn_new         28
>>  struct physdev_pirq_eoi_gmfn {
>>      /* IN */
>>      xen_pfn_t gmfn;
> 
> 

Re: [PATCH 1/2] xen: Introduce PHYSDEVOP_pirq_eoi_gmfn_new

[Stefano Stabellini]

On Thu, 26 Jan 2012, Keir Fraser wrote:
> On 26/01/2012 15:49, "Stefano Stabellini" <Stefano.Stabellini@eu.citrix.com>
> wrote:
> 
> > PHYSDEVOP_pirq_eoi_gmfn changes the semantics of PHYSDEVOP_eoi.
> > Introduce PHYSDEVOP_pirq_eoi_gmfn_new, that is like
> > PHYSDEVOP_pirq_eoi_gmfn but it doesn't modify the behaviour of another
> > hypercall.
> 
> It's nasty that pirq_eoi_gmfn has the side effect. I suggest add a PHYSDEVOP
> to explicitly enable/disable unmask-on-eoi (i.e., the command accepts a
> boolean parameter). Once it is explicitly enabled/disabled in this way,
> pirq_eoi_gmfn no longer has the side effect (regardless of whether it is
> called before or after the explicit setting). So e.g., pv_domain.auto_unmask
> becomes an int where 0/1 means no/yes, and -1 means default (i.e., old
> behavour where it depends on whether PHYSDEVOP_pirq_eoi_gmfn has been
> called).
> 
> This seems to me to move a bad interface in a better direction.

The problem with this approach is that by default we have an hypercall
(PHYSDEVOP_pirq_eoi_gmfn) changing the behaviour of another one
(PHYSDEVOP_eoi). Not only this but we have an hypercall
(PHYSDEVOP_pirq_eoi_gmfn) violating the public interface of shared_info
as documented in public/xen.h.

Introducing a new hypercall with the same name
(PHYSDEVOP_pirq_eoi_gmfn_new) is the first step in admitting that the
old hypercall was a mistake and should not be used.

I don't think we should ever change the semantics of PHYSDEVOP_eoi with
another hypercall. If we want a PHYSDEVOP that eoi and unmask and event
channel let's introduce PHYSDEVOP_eoi_unmask.

Re: [PATCH 1/2] xen: Introduce PHYSDEVOP_pirq_eoi_gmfn_new

[Ian Campbell]

On Thu, 2012-01-26 at 16:29 +0000, Stefano Stabellini wrote:
> 
> Introducing a new hypercall with the same name
> (PHYSDEVOP_pirq_eoi_gmfn_new) is the first step in admitting that the
> old hypercall was a mistake and should not be used. 

If that's the case then there is precedent (e.g. sched_op, physdev_op,
evtchn_op) for renaming the existing thing FOO_compat and taking over
the name with the new semantics.

That's certainly better than _new->_newer->really_new etc. If you must
go down that route then adding a number seems preferable.

Ian.

Re: [PATCH 1/2] xen: Introduce PHYSDEVOP_pirq_eoi_gmfn_new

[Stefano Stabellini]

On Thu, 26 Jan 2012, Ian Campbell wrote:
> On Thu, 2012-01-26 at 16:29 +0000, Stefano Stabellini wrote:
> > 
> > Introducing a new hypercall with the same name
> > (PHYSDEVOP_pirq_eoi_gmfn_new) is the first step in admitting that the
> > old hypercall was a mistake and should not be used. 
> 
> If that's the case then there is precedent (e.g. sched_op, physdev_op,
> evtchn_op) for renaming the existing thing FOO_compat and taking over
> the name with the new semantics.
> 
> That's certainly better than _new->_newer->really_new etc. If you must
> go down that route then adding a number seems preferable.

In that case, I vote for taking over the existing name with the new
hypercall.

Re: [PATCH 1/2] xen: Introduce PHYSDEVOP_pirq_eoi_gmfn_new

[Keir Fraser]

On 26/01/2012 16:45, "Stefano Stabellini" <stefano.stabellini@eu.citrix.com>
wrote:

>> If that's the case then there is precedent (e.g. sched_op, physdev_op,
>> evtchn_op) for renaming the existing thing FOO_compat and taking over
>> the name with the new semantics.
>> 
>> That's certainly better than _new->_newer->really_new etc. If you must
>> go down that route then adding a number seems preferable.
> 
> In that case, I vote for taking over the existing name with the new
> hypercall.

Agreed, but you have to be careful because other codebases expect to be able
to sync with our public headers without subtle side effects.

The correct thing to do here is probably to rename the old command to
PHYSDEVOP_pirq_eoi_gmfn_v1, and your new one ..._v2.

Then you bump XEN_LATEST_INTERFACE_VERSION to 0x00040200 (somewhat
arbitrarily!), and at the end of physdev.h you put something like:
#if __XEN_INTERFACE_VERSION < 0x00040200
#define PHYSDEVOP_pirq_eoi_gmfn PHYSDEVOP_pirq_eoi_gmfn_v1
#else
#define PHYSDEVOP_pirq_eoi_gmfn PHYSDEVOP_pirq_eoi_gmfn_v2
#endif

Perfect, those who want the explicitly versioned command can use it. Old
codebases can sync with new headers safely. Or codebases can be updated for
latest XEN_INTERFACE_VERSION and default to the latest sanest command
versions.

 -- Keir

Re: [PATCH 1/2] xen: Introduce PHYSDEVOP_pirq_eoi_gmfn_new

[Keir Fraser]

On 26/01/2012 16:29, "Stefano Stabellini" <stefano.stabellini@eu.citrix.com>
wrote:

> On Thu, 26 Jan 2012, Keir Fraser wrote:
>> On 26/01/2012 15:49, "Stefano Stabellini" <Stefano.Stabellini@eu.citrix.com>
>> wrote:
>> 
>>> PHYSDEVOP_pirq_eoi_gmfn changes the semantics of PHYSDEVOP_eoi.
>>> Introduce PHYSDEVOP_pirq_eoi_gmfn_new, that is like
>>> PHYSDEVOP_pirq_eoi_gmfn but it doesn't modify the behaviour of another
>>> hypercall.
>> 
>> It's nasty that pirq_eoi_gmfn has the side effect. I suggest add a PHYSDEVOP
>> to explicitly enable/disable unmask-on-eoi (i.e., the command accepts a
>> boolean parameter). Once it is explicitly enabled/disabled in this way,
>> pirq_eoi_gmfn no longer has the side effect (regardless of whether it is
>> called before or after the explicit setting). So e.g., pv_domain.auto_unmask
>> becomes an int where 0/1 means no/yes, and -1 means default (i.e., old
>> behavour where it depends on whether PHYSDEVOP_pirq_eoi_gmfn has been
>> called).
>> 
>> This seems to me to move a bad interface in a better direction.
> 
> The problem with this approach is that by default we have an hypercall
> (PHYSDEVOP_pirq_eoi_gmfn) changing the behaviour of another one
> (PHYSDEVOP_eoi). Not only this but we have an hypercall
> (PHYSDEVOP_pirq_eoi_gmfn) violating the public interface of shared_info
> as documented in public/xen.h.
> 
> Introducing a new hypercall with the same name
> (PHYSDEVOP_pirq_eoi_gmfn_new) is the first step in admitting that the
> old hypercall was a mistake and should not be used.
> 
> I don't think we should ever change the semantics of PHYSDEVOP_eoi with
> another hypercall. If we want a PHYSDEVOP that eoi and unmask and event
> channel let's introduce PHYSDEVOP_eoi_unmask.

Okay, fine by me. See my comments on naming in the email I sent just a sec
ago.

 -- Keir

Re: [PATCH 1/2] xen: Introduce PHYSDEVOP_pirq_eoi_gmfn_new

[Stefano Stabellini]

On Thu, 26 Jan 2012, Keir Fraser wrote:
> On 26/01/2012 16:45, "Stefano Stabellini" <stefano.stabellini@eu.citrix.com>
> wrote:
> 
> >> If that's the case then there is precedent (e.g. sched_op, physdev_op,
> >> evtchn_op) for renaming the existing thing FOO_compat and taking over
> >> the name with the new semantics.
> >> 
> >> That's certainly better than _new->_newer->really_new etc. If you must
> >> go down that route then adding a number seems preferable.
> > 
> > In that case, I vote for taking over the existing name with the new
> > hypercall.
> 
> Agreed, but you have to be careful because other codebases expect to be able
> to sync with our public headers without subtle side effects.
> 
> The correct thing to do here is probably to rename the old command to
> PHYSDEVOP_pirq_eoi_gmfn_v1, and your new one ..._v2.
> 
> Then you bump XEN_LATEST_INTERFACE_VERSION to 0x00040200 (somewhat
> arbitrarily!), and at the end of physdev.h you put something like:
> #if __XEN_INTERFACE_VERSION < 0x00040200
> #define PHYSDEVOP_pirq_eoi_gmfn PHYSDEVOP_pirq_eoi_gmfn_v1
> #else
> #define PHYSDEVOP_pirq_eoi_gmfn PHYSDEVOP_pirq_eoi_gmfn_v2
> #endif
> 
> Perfect, those who want the explicitly versioned command can use it. Old
> codebases can sync with new headers safely. Or codebases can be updated for
> latest XEN_INTERFACE_VERSION and default to the latest sanest command
> versions.

Great, I'll make the change and repost.

[PATCH v2 1/2] xen: introduce PHYSDEVOP_pirq_eoi_gmfn_v2

[Stefano Stabellini]

PHYSDEVOP_pirq_eoi_gmfn changes the semantics of PHYSDEVOP_eoi.
In order to improve the interface this patch:

- renames PHYSDEVOP_pirq_eoi_gmfn to PHYSDEVOP_pirq_eoi_gmfn_v1;

- introduces PHYSDEVOP_pirq_eoi_gmfn_v2, that is like
  PHYSDEVOP_pirq_eoi_gmfn_v1 but it doesn't modify the behaviour of
  another hypercall;

- bump __XEN_LATEST_INTERFACE_VERSION__;

- #define PHYSDEVOP_pirq_eoi_gmfn to PHYSDEVOP_pirq_eoi_gmfn_v1 or
  PHYSDEVOP_pirq_eoi_gmfn_v2 depending on the __XEN_INTERFACE_VERSION.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/ia64/xen/domain.c      |    1 +
 xen/arch/ia64/xen/hypercall.c   |    7 +++++--
 xen/arch/x86/domain.c           |    1 +
 xen/arch/x86/physdev.c          |    7 +++++--
 xen/include/asm-ia64/domain.h   |    3 +++
 xen/include/asm-x86/domain.h    |    3 +++
 xen/include/public/physdev.h    |   16 +++++++++++++++-
 xen/include/public/xen-compat.h |    2 +-
 8 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/xen/arch/ia64/xen/domain.c b/xen/arch/ia64/xen/domain.c
index 1ea5a90..a31bd32 100644
--- a/xen/arch/ia64/xen/domain.c
+++ b/xen/arch/ia64/xen/domain.c
@@ -1731,6 +1731,7 @@ int domain_relinquish_resources(struct domain *d)
 		if (d->arch.pirq_eoi_map != NULL) {
 			put_page(virt_to_page(d->arch.pirq_eoi_map));
 			d->arch.pirq_eoi_map = NULL;
+			d->arch.auto_unmask = 0;
 		}
 
 		/* Tear down shadow mode stuff. */
diff --git a/xen/arch/ia64/xen/hypercall.c b/xen/arch/ia64/xen/hypercall.c
index 130675e..18930bf 100644
--- a/xen/arch/ia64/xen/hypercall.c
+++ b/xen/arch/ia64/xen/hypercall.c
@@ -65,7 +65,7 @@ static long __do_pirq_guest_eoi(struct domain *d, int pirq)
 {
 	if ( pirq < 0 || pirq >= NR_IRQS )
 		return -EINVAL;
-	if ( d->arch.pirq_eoi_map ) {
+	if ( d->arch.auto_unmask ) {
 		spin_lock(&d->event_lock);
 		evtchn_unmask(pirq_to_evtchn(d, pirq));
 		spin_unlock(&d->event_lock);
@@ -508,7 +508,8 @@ long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
         break;
     }
 
-    case PHYSDEVOP_pirq_eoi_gmfn: {
+    case PHYSDEVOP_pirq_eoi_gmfn_v1:
+    case PHYSDEVOP_pirq_eoi_gmfn_v2: {
         struct physdev_pirq_eoi_gmfn info;
         unsigned long mfn;
 
@@ -531,6 +532,8 @@ long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
         }
 
         current->domain->arch.pirq_eoi_map = mfn_to_virt(mfn);
+        if ( cmd == PHYSDEVOP_pirq_eoi_gmfn_v1 )
+            current->domain->arch.auto_unmask = 1;
         ret = 0;
         break;
     }
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 61d83c8..a540af7 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2125,6 +2125,7 @@ int domain_relinquish_resources(struct domain *d)
                 put_page_and_type(
                     mfn_to_page(d->arch.pv_domain.pirq_eoi_map_mfn));
                 d->arch.pv_domain.pirq_eoi_map = NULL;
+                d->arch.pv_domain.auto_unmask = 0;
             }
         }
 
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index f280c28..df92cc7 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -271,7 +271,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
             break;
         }
         if ( !is_hvm_domain(v->domain) &&
-             v->domain->arch.pv_domain.pirq_eoi_map )
+             v->domain->arch.pv_domain.auto_unmask )
             evtchn_unmask(pirq->evtchn);
         if ( !is_hvm_domain(v->domain) ||
              domain_pirq_to_irq(v->domain, eoi.irq) > 0 )
@@ -293,7 +293,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
         break;
     }
 
-    case PHYSDEVOP_pirq_eoi_gmfn: {
+    case PHYSDEVOP_pirq_eoi_gmfn_v2:
+    case PHYSDEVOP_pirq_eoi_gmfn_v1: {
         struct physdev_pirq_eoi_gmfn info;
         unsigned long mfn;
 
@@ -329,6 +330,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
             ret = -ENOSPC;
             break;
         }
+        if ( cmd == PHYSDEVOP_pirq_eoi_gmfn_v1 )
+            v->domain->arch.pv_domain.auto_unmask = 1;
 
         put_gfn(current->domain, info.gmfn);
         ret = 0;
diff --git a/xen/include/asm-ia64/domain.h b/xen/include/asm-ia64/domain.h
index 12dc3bd..31d7d32 100644
--- a/xen/include/asm-ia64/domain.h
+++ b/xen/include/asm-ia64/domain.h
@@ -186,6 +186,9 @@ struct arch_domain {
     /* Shared page for notifying that explicit PIRQ EOI is required. */
     unsigned long *pirq_eoi_map;
     unsigned long pirq_eoi_map_mfn;
+    /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
+     * unmask the event channel */
+    bool_t auto_unmask;
 
     /* Address of efi_runtime_services_t (placed in domain memory)  */
     void *efi_runtime;
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 00bbaeb..fb2cfd2 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -231,6 +231,9 @@ struct pv_domain
     /* Shared page for notifying that explicit PIRQ EOI is required. */
     unsigned long *pirq_eoi_map;
     unsigned long pirq_eoi_map_mfn;
+    /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
+     * unmask the event channel */
+    bool_t auto_unmask;
 
     /* Pseudophysical e820 map (XENMEM_memory_map).  */
     spinlock_t e820_lock;
diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
index 6e23295..16d800f 100644
--- a/xen/include/public/physdev.h
+++ b/xen/include/public/physdev.h
@@ -49,7 +49,15 @@ DEFINE_XEN_GUEST_HANDLE(physdev_eoi_t);
  * will automatically get unmasked. The page registered is used as a bit
  * array indexed by Xen's PIRQ value.
  */
-#define PHYSDEVOP_pirq_eoi_gmfn         17
+#define PHYSDEVOP_pirq_eoi_gmfn_v1       17
+/*
+ * Register a shared page for the hypervisor to indicate whether the
+ * guest must issue PHYSDEVOP_eoi. This hypercall is very similar to
+ * PHYSDEVOP_pirq_eoi_gmfn_v1 but it doesn't change the semantics of
+ * PHYSDEVOP_eoi. The page registered is used as a bit array indexed by
+ * Xen's PIRQ value.
+ */
+#define PHYSDEVOP_pirq_eoi_gmfn_v2       28
 struct physdev_pirq_eoi_gmfn {
     /* IN */
     xen_pfn_t gmfn;
@@ -325,6 +333,12 @@ DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_t);
 #define PHYSDEVOP_IRQ_NEEDS_UNMASK_NOTIFY XENIRQSTAT_needs_eoi
 #define PHYSDEVOP_IRQ_SHARED             XENIRQSTAT_shared
 
+#if __XEN_INTERFACE_VERSION < 0x00040200
+#define PHYSDEVOP_pirq_eoi_gmfn PHYSDEVOP_pirq_eoi_gmfn_v1
+#else
+#define PHYSDEVOP_pirq_eoi_gmfn PHYSDEVOP_pirq_eoi_gmfn_v2
+#endif
+
 #endif /* __XEN_PUBLIC_PHYSDEV_H__ */
 
 /*
diff --git a/xen/include/public/xen-compat.h b/xen/include/public/xen-compat.h
index 2e38003..d8c55bf 100644
--- a/xen/include/public/xen-compat.h
+++ b/xen/include/public/xen-compat.h
@@ -27,7 +27,7 @@
 #ifndef __XEN_PUBLIC_XEN_COMPAT_H__
 #define __XEN_PUBLIC_XEN_COMPAT_H__
 
-#define __XEN_LATEST_INTERFACE_VERSION__ 0x0003020a
+#define __XEN_LATEST_INTERFACE_VERSION__ 0x00040200
 
 #if defined(__XEN__) || defined(__XEN_TOOLS__)
 /* Xen is built with matching headers and implements the latest interface. */
-- 
1.7.2.5

[PATCH v2 2/2] linux/xen: support pirq_eoi_map

[Stefano Stabellini]

The pirq_eoi_map is a bitmap offered by Xen to check which pirqs need to
be EOI'd without having to issue an hypercall every time.
We use PHYSDEVOP_pirq_eoi_gmfn (v2) to map the bitmap, then, if we
succeed, we use pirq_eoi_map to check whether pirqs need eoi.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/events.c            |   17 ++++++++++++++++-
 include/xen/interface/physdev.h |   12 ++++++++++++
 2 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 6e075cd..7fdc738 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -37,6 +37,7 @@
 #include <asm/idle.h>
 #include <asm/io_apic.h>
 #include <asm/sync_bitops.h>
+#include <asm/xen/page.h>
 #include <asm/xen/pci.h>
 #include <asm/xen/hypercall.h>
 #include <asm/xen/hypervisor.h>
@@ -108,6 +109,7 @@ struct irq_info {
 #define PIRQ_SHAREABLE	(1 << 1)
 
 static int *evtchn_to_irq;
+static unsigned long *pirq_eoi_map;
 
 static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
 		      cpu_evtchn_mask);
@@ -274,6 +276,9 @@ static bool pirq_needs_eoi(unsigned irq)
 
 	BUG_ON(info->type != IRQT_PIRQ);
 
+	if (pirq_eoi_map != NULL)
+		return test_bit(irq, pirq_eoi_map);
+
 	return info->u.pirq.flags & PIRQ_NEEDS_EOI;
 }
 
@@ -1693,7 +1698,7 @@ void xen_callback_vector(void) {}
 
 void __init xen_init_IRQ(void)
 {
-	int i;
+	int i, rc;
 
 	evtchn_to_irq = kcalloc(NR_EVENT_CHANNELS, sizeof(*evtchn_to_irq),
 				    GFP_KERNEL);
@@ -1714,8 +1719,18 @@ void __init xen_init_IRQ(void)
 		 * __acpi_register_gsi can point at the right function */
 		pci_xen_hvm_init();
 	} else {
+		struct physdev_pirq_eoi_gmfn eoi_gmfn;
+
 		irq_ctx_init(smp_processor_id());
 		if (xen_initial_domain())
 			pci_xen_initial_domain();
+
+		pirq_eoi_map = (void *)__get_free_page(GFP_KERNEL|__GFP_ZERO);
+		eoi_gmfn.gmfn = virt_to_mfn(pirq_eoi_map);
+		rc = HYPERVISOR_physdev_op(PHYSDEVOP_pirq_eoi_gmfn, &eoi_gmfn);
+		if (rc != 0) {
+			free_page((unsigned long) pirq_eoi_map);
+			pirq_eoi_map = NULL;
+		}
 	}
 }
diff --git a/include/xen/interface/physdev.h b/include/xen/interface/physdev.h
index c1080d9..132c61f 100644
--- a/include/xen/interface/physdev.h
+++ b/include/xen/interface/physdev.h
@@ -39,6 +39,18 @@ struct physdev_eoi {
 };
 
 /*
+ * Register a shared page for the hypervisor to indicate whether the
+ * guest must issue PHYSDEVOP_eoi. The page registered is used as a bit
+ * array indexed by Xen's PIRQ value.
+ */
+#define PHYSDEVOP_pirq_eoi_gmfn      28
+struct physdev_pirq_eoi_gmfn {
+    /* IN */
+    unsigned long gmfn;
+};
+
+
+/*
  * Query the status of an IRQ line.
  * @arg == pointer to physdev_irq_status_query structure.
  */
-- 
1.7.2.5

Re: [PATCH v2 2/2] linux/xen: support pirq_eoi_map

[Konrad Rzeszutek Wilk]

On Thu, Jan 26, 2012 at 06:00:39PM +0000, Stefano Stabellini wrote:
> The pirq_eoi_map is a bitmap offered by Xen to check which pirqs need to
> be EOI'd without having to issue an hypercall every time.
> We use PHYSDEVOP_pirq_eoi_gmfn (v2) to map the bitmap, then, if we
> succeed, we use pirq_eoi_map to check whether pirqs need eoi.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  drivers/xen/events.c            |   17 ++++++++++++++++-
>  include/xen/interface/physdev.h |   12 ++++++++++++
>  2 files changed, 28 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 6e075cd..7fdc738 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -37,6 +37,7 @@
>  #include <asm/idle.h>
>  #include <asm/io_apic.h>
>  #include <asm/sync_bitops.h>
> +#include <asm/xen/page.h>
>  #include <asm/xen/pci.h>
>  #include <asm/xen/hypercall.h>
>  #include <asm/xen/hypervisor.h>
> @@ -108,6 +109,7 @@ struct irq_info {
>  #define PIRQ_SHAREABLE	(1 << 1)
>  
>  static int *evtchn_to_irq;
> +static unsigned long *pirq_eoi_map;
>  
>  static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
>  		      cpu_evtchn_mask);
> @@ -274,6 +276,9 @@ static bool pirq_needs_eoi(unsigned irq)
>  
>  	BUG_ON(info->type != IRQT_PIRQ);
>  
> +	if (pirq_eoi_map != NULL)
> +		return test_bit(irq, pirq_eoi_map);
> +

How about just having a different function called
pirq_needs_eoi_v2 which will just do

 return test_bit(irq, pirq_eoi_map)?

And then set the pirq_needs_eoi_v2 in the function table?


>  	return info->u.pirq.flags & PIRQ_NEEDS_EOI;
>  }
>  
> @@ -1693,7 +1698,7 @@ void xen_callback_vector(void) {}
>  
>  void __init xen_init_IRQ(void)
>  {
> -	int i;
> +	int i, rc;
>  
>  	evtchn_to_irq = kcalloc(NR_EVENT_CHANNELS, sizeof(*evtchn_to_irq),
>  				    GFP_KERNEL);
> @@ -1714,8 +1719,18 @@ void __init xen_init_IRQ(void)
>  		 * __acpi_register_gsi can point at the right function */
>  		pci_xen_hvm_init();
>  	} else {
> +		struct physdev_pirq_eoi_gmfn eoi_gmfn;
> +
>  		irq_ctx_init(smp_processor_id());
>  		if (xen_initial_domain())
>  			pci_xen_initial_domain();
> +
> +		pirq_eoi_map = (void *)__get_free_page(GFP_KERNEL|__GFP_ZERO);
> +		eoi_gmfn.gmfn = virt_to_mfn(pirq_eoi_map);
> +		rc = HYPERVISOR_physdev_op(PHYSDEVOP_pirq_eoi_gmfn, &eoi_gmfn);


Don't we want the v2 version?

> +		if (rc != 0) {
> +			free_page((unsigned long) pirq_eoi_map);
> +			pirq_eoi_map = NULL;
> +		}
>  	}
>  }
> diff --git a/include/xen/interface/physdev.h b/include/xen/interface/physdev.h
> index c1080d9..132c61f 100644
> --- a/include/xen/interface/physdev.h
> +++ b/include/xen/interface/physdev.h
> @@ -39,6 +39,18 @@ struct physdev_eoi {
>  };
>  
>  /*
> + * Register a shared page for the hypervisor to indicate whether the
> + * guest must issue PHYSDEVOP_eoi. The page registered is used as a bit
> + * array indexed by Xen's PIRQ value.
> + */
> +#define PHYSDEVOP_pirq_eoi_gmfn      28

Ah, the number is right, but the name is the generic one.

We should really mention that this is different from v1 or just

#define PHYSDEVOP_pirq_eoi_gmfn_v2 28
and use that in the code?

> +struct physdev_pirq_eoi_gmfn {
> +    /* IN */
> +    unsigned long gmfn;
> +};
> +
> +
> +/*
>   * Query the status of an IRQ line.
>   * @arg == pointer to physdev_irq_status_query structure.
>   */
> -- 
> 1.7.2.5

Re: [PATCH v2 2/2] linux/xen: support pirq_eoi_map

[Stefano Stabellini]

On Thu, 26 Jan 2012, Konrad Rzeszutek Wilk wrote:
> On Thu, Jan 26, 2012 at 06:00:39PM +0000, Stefano Stabellini wrote:
> > The pirq_eoi_map is a bitmap offered by Xen to check which pirqs need to
> > be EOI'd without having to issue an hypercall every time.
> > We use PHYSDEVOP_pirq_eoi_gmfn (v2) to map the bitmap, then, if we
> > succeed, we use pirq_eoi_map to check whether pirqs need eoi.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  drivers/xen/events.c            |   17 ++++++++++++++++-
> >  include/xen/interface/physdev.h |   12 ++++++++++++
> >  2 files changed, 28 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > index 6e075cd..7fdc738 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -37,6 +37,7 @@
> >  #include <asm/idle.h>
> >  #include <asm/io_apic.h>
> >  #include <asm/sync_bitops.h>
> > +#include <asm/xen/page.h>
> >  #include <asm/xen/pci.h>
> >  #include <asm/xen/hypercall.h>
> >  #include <asm/xen/hypervisor.h>
> > @@ -108,6 +109,7 @@ struct irq_info {
> >  #define PIRQ_SHAREABLE	(1 << 1)
> >  
> >  static int *evtchn_to_irq;
> > +static unsigned long *pirq_eoi_map;
> >  
> >  static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
> >  		      cpu_evtchn_mask);
> > @@ -274,6 +276,9 @@ static bool pirq_needs_eoi(unsigned irq)
> >  
> >  	BUG_ON(info->type != IRQT_PIRQ);
> >  
> > +	if (pirq_eoi_map != NULL)
> > +		return test_bit(irq, pirq_eoi_map);
> > +
> 
> How about just having a different function called
> pirq_needs_eoi_v2 which will just do
> 
>  return test_bit(irq, pirq_eoi_map)?
> 
> And then set the pirq_needs_eoi_v2 in the function table?

Ok, but the name "pirq_needs_eoi_v2" is misleading because
PHYSDEVOP_pirq_eoi_gmfn only works for PV guests at the moment.
How about I call the new function "pirq_check_eoi_map" and rename the old
one "pirq_needs_eoi_flag" and the generic name of the function pointer
would remain "pirq_needs_eoi"?


> >  	return info->u.pirq.flags & PIRQ_NEEDS_EOI;
> >  }
> >  
> > @@ -1693,7 +1698,7 @@ void xen_callback_vector(void) {}
> >  
> >  void __init xen_init_IRQ(void)
> >  {
> > -	int i;
> > +	int i, rc;
> >  
> >  	evtchn_to_irq = kcalloc(NR_EVENT_CHANNELS, sizeof(*evtchn_to_irq),
> >  				    GFP_KERNEL);
> > @@ -1714,8 +1719,18 @@ void __init xen_init_IRQ(void)
> >  		 * __acpi_register_gsi can point at the right function */
> >  		pci_xen_hvm_init();
> >  	} else {
> > +		struct physdev_pirq_eoi_gmfn eoi_gmfn;
> > +
> >  		irq_ctx_init(smp_processor_id());
> >  		if (xen_initial_domain())
> >  			pci_xen_initial_domain();
> > +
> > +		pirq_eoi_map = (void *)__get_free_page(GFP_KERNEL|__GFP_ZERO);
> > +		eoi_gmfn.gmfn = virt_to_mfn(pirq_eoi_map);
> > +		rc = HYPERVISOR_physdev_op(PHYSDEVOP_pirq_eoi_gmfn, &eoi_gmfn);
> 
> 
> Don't we want the v2 version?
> 
> > +		if (rc != 0) {
> > +			free_page((unsigned long) pirq_eoi_map);
> > +			pirq_eoi_map = NULL;
> > +		}
> >  	}
> >  }
> > diff --git a/include/xen/interface/physdev.h b/include/xen/interface/physdev.h
> > index c1080d9..132c61f 100644
> > --- a/include/xen/interface/physdev.h
> > +++ b/include/xen/interface/physdev.h
> > @@ -39,6 +39,18 @@ struct physdev_eoi {
> >  };
> >  
> >  /*
> > + * Register a shared page for the hypervisor to indicate whether the
> > + * guest must issue PHYSDEVOP_eoi. The page registered is used as a bit
> > + * array indexed by Xen's PIRQ value.
> > + */
> > +#define PHYSDEVOP_pirq_eoi_gmfn      28
> 
> Ah, the number is right, but the name is the generic one.
> 
> We should really mention that this is different from v1 or just
> 
> #define PHYSDEVOP_pirq_eoi_gmfn_v2 28
> and use that in the code?

Maybe we should:

#define PHYSDEVOP_pirq_eoi_gmfn_v2 28

in order not to hide the fact that there are two versions of this
hypercall.
Then we do:

/* we use the second version of the hypercall */
#define PHYSDEVOP_pirq_eoi_gmfn PHYSDEVOP_pirq_eoi_gmfn_v2


This way we just have PHYSDEVOP_pirq_eoi_gmfn in the code but we don't
hide the version with are using.

Re: [PATCH v2 2/2] linux/xen: support pirq_eoi_map

[Konrad Rzeszutek Wilk]

On Fri, Jan 27, 2012 at 11:03:41AM +0000, Stefano Stabellini wrote:
> On Thu, 26 Jan 2012, Konrad Rzeszutek Wilk wrote:
> > On Thu, Jan 26, 2012 at 06:00:39PM +0000, Stefano Stabellini wrote:
> > > The pirq_eoi_map is a bitmap offered by Xen to check which pirqs need to
> > > be EOI'd without having to issue an hypercall every time.
> > > We use PHYSDEVOP_pirq_eoi_gmfn (v2) to map the bitmap, then, if we
> > > succeed, we use pirq_eoi_map to check whether pirqs need eoi.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > ---
> > >  drivers/xen/events.c            |   17 ++++++++++++++++-
> > >  include/xen/interface/physdev.h |   12 ++++++++++++
> > >  2 files changed, 28 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > > index 6e075cd..7fdc738 100644
> > > --- a/drivers/xen/events.c
> > > +++ b/drivers/xen/events.c
> > > @@ -37,6 +37,7 @@
> > >  #include <asm/idle.h>
> > >  #include <asm/io_apic.h>
> > >  #include <asm/sync_bitops.h>
> > > +#include <asm/xen/page.h>
> > >  #include <asm/xen/pci.h>
> > >  #include <asm/xen/hypercall.h>
> > >  #include <asm/xen/hypervisor.h>
> > > @@ -108,6 +109,7 @@ struct irq_info {
> > >  #define PIRQ_SHAREABLE	(1 << 1)
> > >  
> > >  static int *evtchn_to_irq;
> > > +static unsigned long *pirq_eoi_map;
> > >  
> > >  static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
> > >  		      cpu_evtchn_mask);
> > > @@ -274,6 +276,9 @@ static bool pirq_needs_eoi(unsigned irq)
> > >  
> > >  	BUG_ON(info->type != IRQT_PIRQ);
> > >  
> > > +	if (pirq_eoi_map != NULL)
> > > +		return test_bit(irq, pirq_eoi_map);
> > > +
> > 
> > How about just having a different function called
> > pirq_needs_eoi_v2 which will just do
> > 
> >  return test_bit(irq, pirq_eoi_map)?
> > 
> > And then set the pirq_needs_eoi_v2 in the function table?
> 
> Ok, but the name "pirq_needs_eoi_v2" is misleading because
> PHYSDEVOP_pirq_eoi_gmfn only works for PV guests at the moment.
> How about I call the new function "pirq_check_eoi_map" and rename the old
> one "pirq_needs_eoi_flag" and the generic name of the function pointer
> would remain "pirq_needs_eoi"?

Even better!
> 
> 
> > >  	return info->u.pirq.flags & PIRQ_NEEDS_EOI;
> > >  }
> > >  
> > > @@ -1693,7 +1698,7 @@ void xen_callback_vector(void) {}
> > >  
> > >  void __init xen_init_IRQ(void)
> > >  {
> > > -	int i;
> > > +	int i, rc;
> > >  
> > >  	evtchn_to_irq = kcalloc(NR_EVENT_CHANNELS, sizeof(*evtchn_to_irq),
> > >  				    GFP_KERNEL);
> > > @@ -1714,8 +1719,18 @@ void __init xen_init_IRQ(void)
> > >  		 * __acpi_register_gsi can point at the right function */
> > >  		pci_xen_hvm_init();
> > >  	} else {
> > > +		struct physdev_pirq_eoi_gmfn eoi_gmfn;
> > > +
> > >  		irq_ctx_init(smp_processor_id());
> > >  		if (xen_initial_domain())
> > >  			pci_xen_initial_domain();
> > > +
> > > +		pirq_eoi_map = (void *)__get_free_page(GFP_KERNEL|__GFP_ZERO);
> > > +		eoi_gmfn.gmfn = virt_to_mfn(pirq_eoi_map);
> > > +		rc = HYPERVISOR_physdev_op(PHYSDEVOP_pirq_eoi_gmfn, &eoi_gmfn);
> > 
> > 
> > Don't we want the v2 version?
> > 
> > > +		if (rc != 0) {
> > > +			free_page((unsigned long) pirq_eoi_map);
> > > +			pirq_eoi_map = NULL;
> > > +		}
> > >  	}
> > >  }
> > > diff --git a/include/xen/interface/physdev.h b/include/xen/interface/physdev.h
> > > index c1080d9..132c61f 100644
> > > --- a/include/xen/interface/physdev.h
> > > +++ b/include/xen/interface/physdev.h
> > > @@ -39,6 +39,18 @@ struct physdev_eoi {
> > >  };
> > >  
> > >  /*
> > > + * Register a shared page for the hypervisor to indicate whether the
> > > + * guest must issue PHYSDEVOP_eoi. The page registered is used as a bit
> > > + * array indexed by Xen's PIRQ value.
> > > + */
> > > +#define PHYSDEVOP_pirq_eoi_gmfn      28
> > 
> > Ah, the number is right, but the name is the generic one.
> > 
> > We should really mention that this is different from v1 or just
> > 
> > #define PHYSDEVOP_pirq_eoi_gmfn_v2 28
> > and use that in the code?
> 
> Maybe we should:
> 
> #define PHYSDEVOP_pirq_eoi_gmfn_v2 28
> 
> in order not to hide the fact that there are two versions of this
> hypercall.
> Then we do:
> 
> /* we use the second version of the hypercall */
> #define PHYSDEVOP_pirq_eoi_gmfn PHYSDEVOP_pirq_eoi_gmfn_v2
> 
> 
> This way we just have PHYSDEVOP_pirq_eoi_gmfn in the code but we don't
> hide the version with are using.

That could work. However using a v2 everywhere will clearly show to
to somebody why it won't work with say Xen 4.0 (if they are trying to run it
under it and wonder why that hypercall did not work). Which reminds me, once the
hypervisor patch is in, we should definitly mention that in this git commit.

Re: [PATCH v2 2/2] linux/xen: support pirq_eoi_map

[Stefano Stabellini]

On Fri, 27 Jan 2012, Konrad Rzeszutek Wilk wrote:
> On Fri, Jan 27, 2012 at 11:03:41AM +0000, Stefano Stabellini wrote:
> > On Thu, 26 Jan 2012, Konrad Rzeszutek Wilk wrote:
> > > On Thu, Jan 26, 2012 at 06:00:39PM +0000, Stefano Stabellini wrote:
> > > > The pirq_eoi_map is a bitmap offered by Xen to check which pirqs need to
> > > > be EOI'd without having to issue an hypercall every time.
> > > > We use PHYSDEVOP_pirq_eoi_gmfn (v2) to map the bitmap, then, if we
> > > > succeed, we use pirq_eoi_map to check whether pirqs need eoi.
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > ---
> > > >  drivers/xen/events.c            |   17 ++++++++++++++++-
> > > >  include/xen/interface/physdev.h |   12 ++++++++++++
> > > >  2 files changed, 28 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > > > index 6e075cd..7fdc738 100644
> > > > --- a/drivers/xen/events.c
> > > > +++ b/drivers/xen/events.c
> > > > @@ -37,6 +37,7 @@
> > > >  #include <asm/idle.h>
> > > >  #include <asm/io_apic.h>
> > > >  #include <asm/sync_bitops.h>
> > > > +#include <asm/xen/page.h>
> > > >  #include <asm/xen/pci.h>
> > > >  #include <asm/xen/hypercall.h>
> > > >  #include <asm/xen/hypervisor.h>
> > > > @@ -108,6 +109,7 @@ struct irq_info {
> > > >  #define PIRQ_SHAREABLE	(1 << 1)
> > > >  
> > > >  static int *evtchn_to_irq;
> > > > +static unsigned long *pirq_eoi_map;
> > > >  
> > > >  static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
> > > >  		      cpu_evtchn_mask);
> > > > @@ -274,6 +276,9 @@ static bool pirq_needs_eoi(unsigned irq)
> > > >  
> > > >  	BUG_ON(info->type != IRQT_PIRQ);
> > > >  
> > > > +	if (pirq_eoi_map != NULL)
> > > > +		return test_bit(irq, pirq_eoi_map);
> > > > +
> > > 
> > > How about just having a different function called
> > > pirq_needs_eoi_v2 which will just do
> > > 
> > >  return test_bit(irq, pirq_eoi_map)?
> > > 
> > > And then set the pirq_needs_eoi_v2 in the function table?
> > 
> > Ok, but the name "pirq_needs_eoi_v2" is misleading because
> > PHYSDEVOP_pirq_eoi_gmfn only works for PV guests at the moment.
> > How about I call the new function "pirq_check_eoi_map" and rename the old
> > one "pirq_needs_eoi_flag" and the generic name of the function pointer
> > would remain "pirq_needs_eoi"?
> 
> Even better!
> > 
> > 
> > > >  	return info->u.pirq.flags & PIRQ_NEEDS_EOI;
> > > >  }
> > > >  
> > > > @@ -1693,7 +1698,7 @@ void xen_callback_vector(void) {}
> > > >  
> > > >  void __init xen_init_IRQ(void)
> > > >  {
> > > > -	int i;
> > > > +	int i, rc;
> > > >  
> > > >  	evtchn_to_irq = kcalloc(NR_EVENT_CHANNELS, sizeof(*evtchn_to_irq),
> > > >  				    GFP_KERNEL);
> > > > @@ -1714,8 +1719,18 @@ void __init xen_init_IRQ(void)
> > > >  		 * __acpi_register_gsi can point at the right function */
> > > >  		pci_xen_hvm_init();
> > > >  	} else {
> > > > +		struct physdev_pirq_eoi_gmfn eoi_gmfn;
> > > > +
> > > >  		irq_ctx_init(smp_processor_id());
> > > >  		if (xen_initial_domain())
> > > >  			pci_xen_initial_domain();
> > > > +
> > > > +		pirq_eoi_map = (void *)__get_free_page(GFP_KERNEL|__GFP_ZERO);
> > > > +		eoi_gmfn.gmfn = virt_to_mfn(pirq_eoi_map);
> > > > +		rc = HYPERVISOR_physdev_op(PHYSDEVOP_pirq_eoi_gmfn, &eoi_gmfn);
> > > 
> > > 
> > > Don't we want the v2 version?
> > > 
> > > > +		if (rc != 0) {
> > > > +			free_page((unsigned long) pirq_eoi_map);
> > > > +			pirq_eoi_map = NULL;
> > > > +		}
> > > >  	}
> > > >  }
> > > > diff --git a/include/xen/interface/physdev.h b/include/xen/interface/physdev.h
> > > > index c1080d9..132c61f 100644
> > > > --- a/include/xen/interface/physdev.h
> > > > +++ b/include/xen/interface/physdev.h
> > > > @@ -39,6 +39,18 @@ struct physdev_eoi {
> > > >  };
> > > >  
> > > >  /*
> > > > + * Register a shared page for the hypervisor to indicate whether the
> > > > + * guest must issue PHYSDEVOP_eoi. The page registered is used as a bit
> > > > + * array indexed by Xen's PIRQ value.
> > > > + */
> > > > +#define PHYSDEVOP_pirq_eoi_gmfn      28
> > > 
> > > Ah, the number is right, but the name is the generic one.
> > > 
> > > We should really mention that this is different from v1 or just
> > > 
> > > #define PHYSDEVOP_pirq_eoi_gmfn_v2 28
> > > and use that in the code?
> > 
> > Maybe we should:
> > 
> > #define PHYSDEVOP_pirq_eoi_gmfn_v2 28
> > 
> > in order not to hide the fact that there are two versions of this
> > hypercall.
> > Then we do:
> > 
> > /* we use the second version of the hypercall */
> > #define PHYSDEVOP_pirq_eoi_gmfn PHYSDEVOP_pirq_eoi_gmfn_v2
> > 
> > 
> > This way we just have PHYSDEVOP_pirq_eoi_gmfn in the code but we don't
> > hide the version with are using.
> 
> That could work. However using a v2 everywhere will clearly show to
> to somebody why it won't work with say Xen 4.0 (if they are trying to run it
> under it and wonder why that hypercall did not work). Which reminds me, once the
> hypervisor patch is in, we should definitly mention that in this git commit.

OK then, let's go with PHYSDEVOP_pirq_eoi_gmfn_v2