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

[Julien Grall <julien@xxxxxxx>]

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?
> > > >
> > > > 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...
> > 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.
Cheers,

--
Julien Grall