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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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"?


> >     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.

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