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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > > > ---
> > > >  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel