Re: [PATCH] tools/libxc: use uint32_t for pirq in xc_domain_irq_permission

[Julien Grall <julien@xxxxxxx>]

Hi Igor,

On 08/07/2021 03:06, Igor Druzhinin wrote:
> On 07/07/2021 14:21, Julien Grall wrote:
> > On 07/07/2021 14:14, Jan Beulich wrote:
> > > On 07.07.2021 14:59, Julien Grall wrote:
> > > > On 07/07/2021 13:54, Jan Beulich wrote:
> > > > > On 07.07.2021 14:51, Julien Grall wrote:
> > > > > > On 07/07/2021 02:02, Igor Druzhinin wrote:
> > > > > > > Current unit8_t for pirq argument in this interface is too 
> > > > > > > restrictive
> > > > > > > causing failures on modern hardware with lots of GSIs. That 
> > > > > > > extends down to
> > > > > > > XEN_DOMCTL_irq_permission ABI structure where it needs to be 
> > > > > > > fixed up
> > > > > > > as well. Internal Xen structures appear to be fine. Existing 
> > > > > > > users of
> > > > > > > the interface in tree (libxl, ocaml and python bindings) are 
> > > > > > > already using
> > > > > > > int for pirq representation that should be wide enough.
> > > > > > By "int", I am assuming you imply "signed int", is that correct?
> Yes, just "int" in the meaning "signed int" - I can clarify that in the 
> description.
> > > > > > If so, should the function xc_domain_irq_permission() interface 
> > > > > > take an
> > > > > > int in parameter and check it is not negative?
> > > > > Please let's not make things worse than they are, the more that
> > > > Well, what I am trying to prevent is surprise where the caller
> > > > mistakenly pass a negative value that will be interpreted as a positive
> > > > value...
> > > This happens all the time when converting from signed to unsigned
> > > perhaps just internally.
> > I am not sure what's your point... Yes there are place in Xen that 
> > switch between signed and unsigned. We likely have some (latent) 
> > problem because of that...
> Callers of libxc interface shouldn't have been using signed int at all.
> They just happen to do it at least in-tree - that's what I found and 
> mentioned
> in the description. At the same time "int" type is for now wide enough 
> so there
> is no immediate rush to fix them up.
>
> That gets a little bit tricky with bindings - they themselves expose pirq
> as int. So a negative value could be passed by the caller and, given other
> similar interace functions like xc_physdev_map_pirq() are using "int pirq"
> to signal an error as negative value, that could be misinterpreted by lower
> levels.
>
> We can add extra checks in bindings to avoid passing all negative values to
> libxc level. Would this be good enough?
>
> > > > Such issues are beyong annoying to debug...
> > > No worse than any other out-of-bounds value, I would say.
> > >
> > > >   > ./CODING_STYLE is unambiguous in cases like this one.
> > > >
> > > > Hmmm... The coding style mention the fixed size but nothing about the
> > > > signedness of the type...
> > > Oh, sorry, yes. The adjustment for this even pre-dates the two
> > > patches to ./CODING_STYLE that I've on record as pending for
> > > nearly two years.
> > >
> > > > The alternative suggestion is to keep a unsigned type but check the bit
> > > > 31 is not set.
> > > Why? Why not bit 30 or bit 27? There's nothing special about
> > > bit 31 in an unsigned number.
> > Bit 31 is the signed bit for signed number. The check would make sure 
> > that:
> >   1) The value will fit other hypercall (the PIRQ is described as int 
> > in a few of the structure)
> >   2) Catch potentially caller that would use the number that could 
> > potentially be interpreted as negative by other part of the hypervisor.
> > That said, I can live with the implicit signed -> unsigned convertion, 
> > however the commit message should at least be clarified because it is 
> > misleading.
> Could you specify which statement exactly is misleading (or needs 
> clariying)
> in the commit message?
The commit message is mentioning that all the callers are using "signed 
int" but then the patch will use "uint32_t" without really saying why...
I think adding something along the line to:

"While all the callers are using signed int, PIRQ indexes are not meant 
to be negative. Switch the type to unsigned 32-bit and leave the caller 
clean-up for future follow-up."
Cheers,

--
Julien Grall