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

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

[Igor Druzhinin]

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.

Domctl interface version is needed to be bumped with this change but that
was already done by 918b8842a8 ("arm64: Change type of hsr, cpsr, spsr_el1
to uint64_t") in this release cycle.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 tools/include/xenctrl.h             | 2 +-
 tools/libs/ctrl/xc_domain.c         | 2 +-
 tools/ocaml/libs/xc/xenctrl_stubs.c | 2 +-
 xen/include/public/domctl.h         | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 2a7c836..8974747 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1385,7 +1385,7 @@ int xc_domain_ioport_permission(xc_interface *xch,
 
 int xc_domain_irq_permission(xc_interface *xch,
                              uint32_t domid,
-                             uint8_t pirq,
+                             uint32_t pirq,
                              uint8_t allow_access);
 
 int xc_domain_iomem_permission(xc_interface *xch,
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index 7d11884..8e4ffd0 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -1384,7 +1384,7 @@ int xc_vcpu_setcontext(xc_interface *xch,
 
 int xc_domain_irq_permission(xc_interface *xch,
                              uint32_t domid,
-                             uint8_t pirq,
+                             uint32_t pirq,
                              uint8_t allow_access)
 {
     DECLARE_DOMCTL;
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 6e4bc56..e5837e6 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -1077,7 +1077,7 @@ CAMLprim value stub_xc_domain_irq_permission(value xch, value domid,
 					     value pirq, value allow)
 {
 	CAMLparam4(xch, domid, pirq, allow);
-	uint8_t c_pirq;
+	uint32_t c_pirq;
 	uint8_t c_allow;
 	int ret;
 
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 4dbf107..277478e 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -441,7 +441,7 @@ struct xen_domctl_setdebugging {
 
 /* 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 */
 };
 
-- 
2.7.4

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

[Jan Beulich]

On 07.07.2021 03: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.
> 
> Domctl interface version is needed to be bumped with this change but that
> was already done by 918b8842a8 ("arm64: Change type of hsr, cpsr, spsr_el1
> to uint64_t") in this release cycle.

Let's hope it's not going to get reverted for having broken the tools
build in multiple ways.

> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -1385,7 +1385,7 @@ int xc_domain_ioport_permission(xc_interface *xch,
>  
>  int xc_domain_irq_permission(xc_interface *xch,
>                               uint32_t domid,
> -                             uint8_t pirq,
> +                             uint32_t pirq,
>                               uint8_t allow_access);

Take the opportunity and also change "allow_access" to bool? Or is
use of bool prohibited in external interfaces?

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -441,7 +441,7 @@ struct xen_domctl_setdebugging {
>  
>  /* 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 */
>  };

Please can you make the now added padding explicit, like we strive
to do with any new additions / extensions? With at least this aspect
taken care of
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Andrew Cooper]

On 07/07/2021 08:46, Jan Beulich wrote:
>> --- a/tools/include/xenctrl.h
>> +++ b/tools/include/xenctrl.h
>> @@ -1385,7 +1385,7 @@ int xc_domain_ioport_permission(xc_interface *xch,
>>  
>>  int xc_domain_irq_permission(xc_interface *xch,
>>                               uint32_t domid,
>> -                             uint8_t pirq,
>> +                             uint32_t pirq,
>>                               uint8_t allow_access);
> Take the opportunity and also change "allow_access" to bool? Or is
> use of bool prohibited in external interfaces?

We've got bool's in the interface already.

~Andrew

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

[Igor Druzhinin]

On 07/07/2021 10:19, Andrew Cooper wrote:
> On 07/07/2021 08:46, Jan Beulich wrote:
>>> --- a/tools/include/xenctrl.h
>>> +++ b/tools/include/xenctrl.h
>>> @@ -1385,7 +1385,7 @@ int xc_domain_ioport_permission(xc_interface *xch,
>>>   
>>>   int xc_domain_irq_permission(xc_interface *xch,
>>>                                uint32_t domid,
>>> -                             uint8_t pirq,
>>> +                             uint32_t pirq,
>>>                                uint8_t allow_access);
>> Take the opportunity and also change "allow_access" to bool? Or is
>> use of bool prohibited in external interfaces?
> 
> We've got bool's in the interface already.

Where exactly? I couldn't find a single "bool".

Igor

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

[Andrew Cooper]

On 08/07/2021 02:08, Igor Druzhinin wrote:
> On 07/07/2021 10:19, Andrew Cooper wrote:
>> On 07/07/2021 08:46, Jan Beulich wrote:
>>>> --- a/tools/include/xenctrl.h
>>>> +++ b/tools/include/xenctrl.h
>>>> @@ -1385,7 +1385,7 @@ int xc_domain_ioport_permission(xc_interface
>>>> *xch,
>>>>     int xc_domain_irq_permission(xc_interface *xch,
>>>>                                uint32_t domid,
>>>> -                             uint8_t pirq,
>>>> +                             uint32_t pirq,
>>>>                                uint8_t allow_access);
>>> Take the opportunity and also change "allow_access" to bool? Or is
>>> use of bool prohibited in external interfaces?
>>
>> We've got bool's in the interface already.
>
> Where exactly? I couldn't find a single "bool".

$ git grep -w bool -- :/tools/include/xen*.h
../tools/include/xenctrl.h:1844:                          uint32_t
domid, bool restore,
../tools/include/xenctrl.h:1846:                          unsigned int
nr_features, bool pae, bool itsc,
../tools/include/xenctrl.h:1847:                          bool
nested_virt, const struct xc_xend_cpuid *xend);
../tools/include/xenctrl.h:1954:int
xc_altp2m_get_domain_state(xc_interface *handle, uint32_t dom, bool *state);
../tools/include/xenctrl.h:1955:int
xc_altp2m_set_domain_state(xc_interface *handle, uint32_t dom, bool state);

and loads more.

~Andrew

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

[Igor Druzhinin]

On 08/07/2021 02:11, Andrew Cooper wrote:
> On 08/07/2021 02:08, Igor Druzhinin wrote:
>> On 07/07/2021 10:19, Andrew Cooper wrote:
>>> On 07/07/2021 08:46, Jan Beulich wrote:
>>>>> --- a/tools/include/xenctrl.h
>>>>> +++ b/tools/include/xenctrl.h
>>>>> @@ -1385,7 +1385,7 @@ int xc_domain_ioport_permission(xc_interface
>>>>> *xch,
>>>>>      int xc_domain_irq_permission(xc_interface *xch,
>>>>>                                 uint32_t domid,
>>>>> -                             uint8_t pirq,
>>>>> +                             uint32_t pirq,
>>>>>                                 uint8_t allow_access);
>>>> Take the opportunity and also change "allow_access" to bool? Or is
>>>> use of bool prohibited in external interfaces?
>>>
>>> We've got bool's in the interface already.
>>
>> Where exactly? I couldn't find a single "bool".
> 
> $ git grep -w bool -- :/tools/include/xen*.h
> ../tools/include/xenctrl.h:1844:                          uint32_t
> domid, bool restore,
> ../tools/include/xenctrl.h:1846:                          unsigned int
> nr_features, bool pae, bool itsc,
> ../tools/include/xenctrl.h:1847:                          bool
> nested_virt, const struct xc_xend_cpuid *xend);
> ../tools/include/xenctrl.h:1954:int
> xc_altp2m_get_domain_state(xc_interface *handle, uint32_t dom, bool *state);
> ../tools/include/xenctrl.h:1955:int
> xc_altp2m_set_domain_state(xc_interface *handle, uint32_t dom, bool state);
> 
> and loads more.

Are we ok to have different types in ABI interface and in libxc
function prototype then? Because I was referring to ABI structures.

Igor

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

[Andrew Cooper]

On 08/07/2021 02:14, Igor Druzhinin wrote:
> On 08/07/2021 02:11, Andrew Cooper wrote:
>> On 08/07/2021 02:08, Igor Druzhinin wrote:
>>> On 07/07/2021 10:19, Andrew Cooper wrote:
>>>> On 07/07/2021 08:46, Jan Beulich wrote:
>>>>>> --- a/tools/include/xenctrl.h
>>>>>> +++ b/tools/include/xenctrl.h
>>>>>> @@ -1385,7 +1385,7 @@ int xc_domain_ioport_permission(xc_interface
>>>>>> *xch,
>>>>>>      int xc_domain_irq_permission(xc_interface *xch,
>>>>>>                                 uint32_t domid,
>>>>>> -                             uint8_t pirq,
>>>>>> +                             uint32_t pirq,
>>>>>>                                 uint8_t allow_access);
>>>>> Take the opportunity and also change "allow_access" to bool? Or is
>>>>> use of bool prohibited in external interfaces?
>>>>
>>>> We've got bool's in the interface already.
>>>
>>> Where exactly? I couldn't find a single "bool".
>>
>> $ git grep -w bool -- :/tools/include/xen*.h
>> ../tools/include/xenctrl.h:1844:                          uint32_t
>> domid, bool restore,
>> ../tools/include/xenctrl.h:1846:                          unsigned int
>> nr_features, bool pae, bool itsc,
>> ../tools/include/xenctrl.h:1847:                          bool
>> nested_virt, const struct xc_xend_cpuid *xend);
>> ../tools/include/xenctrl.h:1954:int
>> xc_altp2m_get_domain_state(xc_interface *handle, uint32_t dom, bool
>> *state);
>> ../tools/include/xenctrl.h:1955:int
>> xc_altp2m_set_domain_state(xc_interface *handle, uint32_t dom, bool
>> state);
>>
>> and loads more.
>
> Are we ok to have different types in ABI interface and in libxc
> function prototype then?

Yes.  Again, we've got plenty of examples like this.

> Because I was referring to ABI structures.

The hypercall structs can't contain bool.  bool has implementation
defined width in C, just like enum, and there is no requirement for
sizeof(bool) to be 1.

The pre-existing uint8_t here is correct, although the hypercall handler
ideally wants a further adjustment to reject non-boolean values.  This
hypercall clearly predates our more careful review practices...

~Andrew

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

[Igor Druzhinin]

On 08/07/2021 02:26, Andrew Cooper wrote:
> On 08/07/2021 02:14, Igor Druzhinin wrote:
>> On 08/07/2021 02:11, Andrew Cooper wrote:
>>> On 08/07/2021 02:08, Igor Druzhinin wrote:
>>>> On 07/07/2021 10:19, Andrew Cooper wrote:
>>>>> On 07/07/2021 08:46, Jan Beulich wrote:
>>>>>>> --- a/tools/include/xenctrl.h
>>>>>>> +++ b/tools/include/xenctrl.h
>>>>>>> @@ -1385,7 +1385,7 @@ int xc_domain_ioport_permission(xc_interface
>>>>>>> *xch,
>>>>>>>       int xc_domain_irq_permission(xc_interface *xch,
>>>>>>>                                  uint32_t domid,
>>>>>>> -                             uint8_t pirq,
>>>>>>> +                             uint32_t pirq,
>>>>>>>                                  uint8_t allow_access);
>>>>>> Take the opportunity and also change "allow_access" to bool? Or is
>>>>>> use of bool prohibited in external interfaces?
>>>>>
>>>>> We've got bool's in the interface already.
>>>>
>>>> Where exactly? I couldn't find a single "bool".
>>>
>>> $ git grep -w bool -- :/tools/include/xen*.h
>>> ../tools/include/xenctrl.h:1844:                          uint32_t
>>> domid, bool restore,
>>> ../tools/include/xenctrl.h:1846:                          unsigned int
>>> nr_features, bool pae, bool itsc,
>>> ../tools/include/xenctrl.h:1847:                          bool
>>> nested_virt, const struct xc_xend_cpuid *xend);
>>> ../tools/include/xenctrl.h:1954:int
>>> xc_altp2m_get_domain_state(xc_interface *handle, uint32_t dom, bool
>>> *state);
>>> ../tools/include/xenctrl.h:1955:int
>>> xc_altp2m_set_domain_state(xc_interface *handle, uint32_t dom, bool
>>> state);
>>>
>>> and loads more.
>>
>> Are we ok to have different types in ABI interface and in libxc
>> function prototype then?
> 
> Yes.  Again, we've got plenty of examples like this.
> 
>> Because I was referring to ABI structures.
> 
> The hypercall structs can't contain bool.  bool has implementation
> defined width in C, just like enum, and there is no requirement for
> sizeof(bool) to be 1.
> 
> The pre-existing uint8_t here is correct, although the hypercall handler
> ideally wants a further adjustment to reject non-boolean values.  This
> hypercall clearly predates our more careful review practices...

Sure. Get what you want now. I'm just not a fan of type conversions
for the sake of it - prefer a common type to be used pervasively.
But, of course, happy to follow Xen practises.

Igor

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

[Christian Lindig]

[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]



On 7 Jul 2021, at 02:02, Igor Druzhinin <igor.druzhinin@citrix.com<mailto:igor.druzhinin@citrix.com>> 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.

Domctl interface version is needed to be bumped with this change but that
was already done by 918b8842a8 ("arm64: Change type of hsr, cpsr, spsr_el1
to uint64_t") in this release cycle.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com<mailto:igor.druzhinin@citrix.com>>
---
tools/include/xenctrl.h             | 2 +-
tools/libs/ctrl/xc_domain.c         | 2 +-
tools/ocaml/libs/xc/xenctrl_stubs.c | 2 +-
xen/include/public/domctl.h         | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>


[-- Attachment #2: Type: text/html, Size: 2554 bytes --]

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

[Julien Grall]

Hi Igor,

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?

Cheers,

-- 
Julien Grall

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

[Jan Beulich]

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
./CODING_STYLE is unambiguous in cases like this one. If we mean
non-negative values, the type used should be an unsigned one. In
pre-existing code this will obviously only get changed over time.

Jan

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

[Julien Grall]

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

Such issues are beyong annoying to debug...

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

The alternative suggestion is to keep a unsigned type but check the bit 
31 is not set.

Cheers,

-- 
Julien Grall

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

[Jan Beulich]

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.

> 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. You'll get an error from the
underlying hypercall for any out of bounds values, not just
ones with bit 31, 30, or 27 set.

Jan

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

[Julien Grall]

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

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

[Jan Beulich]

On 07.07.2021 15:21, Julien Grall wrote:
> On 07/07/2021 14:14, Jan Beulich wrote:
>> On 07.07.2021 14:59, Julien Grall wrote:
>>> 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.

And getting refused equally as out of range. Plain int uses will
want replacing imo, but perhaps we don't have room to do so in the
public interface (outside of the tools-only part of it at least).

> That said, I can live with the implicit signed -> unsigned convertion, 
> however the commit message should at least be clarified because it is 
> misleading.

You'll have to work this out with Igor. I can't see anything that's
misleading.

Jan

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

[Igor Druzhinin]

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?

Igor

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

[Julien Grall]

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