Re: [Xen-devel] [PATCH v3] arm: irq: increase size of irq from uint8_t to uint32_t

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2015-04-15 at 14:00 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 15/04/15 13:10, Ian Campbell wrote:
> > Switching Julien to his Citrix address which should probably be used in
> > the future.
> > 
> > On Wed, 2015-04-08 at 17:14 +0300, Iurii Konovalenko wrote:
> >> From: Iurii Konovalenko <iurii.konovalenko@xxxxxxxxxxxxxxx>
> >>
> >> Changes are dedicated to XEN_DOMCTL_irq_permission and
> >> IRQ pssthrough API functions.
> >>
> >> PHYSDEV_* operations already using 32 bits type but signed one.
> >>
> >> Although, PHYSDEV_* operations are not yet used on ARM and LPIs support
> >> (which are using very high number) are not supported yet, we don't need
> >> to care about theses for now.
> > 
> > I may have slightly lost track, but I think we decided in Julien's
> > passthrough thread not to use most of these interfaces on ARM, or am I
> > confused?
> 
> Only XEN_DOMCTL_bind_pt_irq will be used for ARM passthrough.
> 
> I asked Iurii to mention PHYSDEV_* because the prototype is already
> valid but use int rather than unsigned int.
> 
> > If I'm correct then I think we can avoid messing with many of other
> > ones, for example the ISA IRQ one doesn't need changing, does it? (ISA
> > only had 16 IRQs IIRC...)
> 
> Only the newly introduce function xc_domain_bind_pt_spi_irq will be used
> for ARM. I guess we can avoid to modify xc_domain_bind_pt*.

IMHO the only reason to modify the existing xc_domain_bind_pt* would be
to make the hypercall and libxc interfaces consistent, but in that case
the commit message should talk about that and not so much about ARM etc.

Actually as it is 2/3 of the commit message talks about something
unrelated which isn't changing, and the only other bit doesn't really
explain what or why, just that it is limited to certain interfaces.

So perhaps a clearer and more precise commit message might be all which
is needed to be changed here.

> > I think it would be best if whichever bits of this are still relevant
> > were folded into Julien's '[PATCH v5 p2 04/19] xen/arm: Implement
> > hypercall DOMCTL_{,un}bind_pt_pirq' or at least presented as a followup
> > to it.
> > 
> >> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> >> index 8803ab2..65fb866 100644
> >> --- a/xen/include/public/domctl.h
> >> +++ b/xen/include/public/domctl.h
> >> @@ -400,7 +400,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_setdebugging_t);
> >>  
> >>  /* XEN_DOMCTL_irq_permission */
> >>  struct xen_domctl_irq_permission {
> >> -    uint8_t pirq;
> >> +    uint32_t pirq;
> >>      uint8_t allow_access;    /* flag to specify enable/disable of IRQ 
> >> access */
> > 
> > I think we weren't going to end up using this one either, but again I
> > might not be remembering correctly.
> 
> Even though we won't support it on ARM for now, I think it's good to
> keep the interface consistent.
> 
> It would avoid us to forget the problem when this will be support it later.

OK, but in that case the cc list needs to include the other hypervisor
maintainers and not just the ARM ones.

And in fact this whole patch needs to go to the tools maintainers
really, not so much to the ARM maintainers. get_maintainers.pl would
probably have helped.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel