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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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.