* [PATCH] tools/arm: Fix nr_spis handling v2
@ 2025-03-18 9:00 Michal Orzel
2025-03-18 9:22 ` Luca Fancellu
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Michal Orzel @ 2025-03-18 9:00 UTC (permalink / raw)
To: xen-devel
Cc: Michal Orzel, Anthony PERARD, Juergen Gross, Luca Fancellu,
Stefano Stabellini, Julien Grall, Bertrand Marquis
We are missing a way to detect whether a user provided a value for
nr_spis equal to 0 or did not provide any value (default is also 0) which
can cause issues when calculated nr_spis is > 0 and the value from domain
config is 0. Fix it by setting default value for nr_spis to UINT32_MAX
(max supported nr of SPIs is 960 anyway).
Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value")
Reported-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
tools/libs/light/libxl_arm.c | 7 ++++---
tools/libs/light/libxl_types.idl | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 2d895408cac3..5bb5bac61def 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
libxl_domain_config *d_config,
struct xen_domctl_createdomain *config)
{
- uint32_t nr_spis = 0;
+ uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis;
unsigned int i;
uint32_t vuart_irq, virtio_irq = 0;
bool vuart_enabled = false, virtio_enabled = false;
@@ -181,13 +181,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
LOG(DEBUG, "Configure the domain");
- if (nr_spis > d_config->b_info.arch_arm.nr_spis) {
+ /* We use UINT32_MAX to denote if a user provided a value or not */
+ if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) {
LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n",
nr_spis);
return ERROR_FAIL;
}
- config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
+ config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis;
LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
switch (d_config->b_info.arch_arm.gic_version) {
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index bd4b8721ff19..129c00ce929c 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -723,7 +723,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
("vuart", libxl_vuart_type),
("sve_vl", libxl_sve_type),
- ("nr_spis", uint32),
+ ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}),
])),
("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
])),
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] tools/arm: Fix nr_spis handling v2
2025-03-18 9:00 [PATCH] tools/arm: Fix nr_spis handling v2 Michal Orzel
@ 2025-03-18 9:22 ` Luca Fancellu
2025-03-19 14:01 ` Alejandro Vallejo
2025-03-20 16:46 ` Anthony PERARD
2 siblings, 0 replies; 12+ messages in thread
From: Luca Fancellu @ 2025-03-18 9:22 UTC (permalink / raw)
To: Michal Orzel
Cc: xen-devel@lists.xenproject.org, Anthony PERARD, Juergen Gross,
Stefano Stabellini, Julien Grall, Bertrand Marquis
Hi Michal,
> On 18 Mar 2025, at 09:00, Michal Orzel <michal.orzel@amd.com> wrote:
>
> We are missing a way to detect whether a user provided a value for
> nr_spis equal to 0 or did not provide any value (default is also 0) which
> can cause issues when calculated nr_spis is > 0 and the value from domain
> config is 0. Fix it by setting default value for nr_spis to UINT32_MAX
> (max supported nr of SPIs is 960 anyway).
>
> Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value")
> Reported-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
I’ve tested this fix using our test that mounts a virtio mmio disk on the domain,
everything works fine.
Tested-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] tools/arm: Fix nr_spis handling v2
2025-03-18 9:00 [PATCH] tools/arm: Fix nr_spis handling v2 Michal Orzel
2025-03-18 9:22 ` Luca Fancellu
@ 2025-03-19 14:01 ` Alejandro Vallejo
2025-03-19 14:05 ` Andrew Cooper
2025-03-24 13:08 ` Orzel, Michal
2025-03-20 16:46 ` Anthony PERARD
2 siblings, 2 replies; 12+ messages in thread
From: Alejandro Vallejo @ 2025-03-19 14:01 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Anthony PERARD, Juergen Gross, Luca Fancellu, Stefano Stabellini,
Julien Grall, Bertrand Marquis
On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel wrote:
> We are missing a way to detect whether a user provided a value for
> nr_spis equal to 0 or did not provide any value (default is also 0) which
> can cause issues when calculated nr_spis is > 0 and the value from domain
> config is 0. Fix it by setting default value for nr_spis to UINT32_MAX
> (max supported nr of SPIs is 960 anyway).
>
> Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value")
> Reported-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
> tools/libs/light/libxl_arm.c | 7 ++++---
> tools/libs/light/libxl_types.idl | 2 +-
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 2d895408cac3..5bb5bac61def 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> libxl_domain_config *d_config,
> struct xen_domctl_createdomain *config)
> {
> - uint32_t nr_spis = 0;
> + uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis;
> unsigned int i;
> uint32_t vuart_irq, virtio_irq = 0;
> bool vuart_enabled = false, virtio_enabled = false;
> @@ -181,13 +181,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>
> LOG(DEBUG, "Configure the domain");
>
> - if (nr_spis > d_config->b_info.arch_arm.nr_spis) {
> + /* We use UINT32_MAX to denote if a user provided a value or not */
> + if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) {
> LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n",
> nr_spis);
> return ERROR_FAIL;
> }
>
> - config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
> + config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis;
> LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
>
> switch (d_config->b_info.arch_arm.gic_version) {
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index bd4b8721ff19..129c00ce929c 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -723,7 +723,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> ("vuart", libxl_vuart_type),
> ("sve_vl", libxl_sve_type),
> - ("nr_spis", uint32),
> + ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}),
> ])),
> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> ])),
Doesn't this regenerate the golang bindings?
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] tools/arm: Fix nr_spis handling v2
2025-03-19 14:01 ` Alejandro Vallejo
@ 2025-03-19 14:05 ` Andrew Cooper
2025-03-19 14:37 ` Orzel, Michal
2025-03-19 16:22 ` Alejandro Vallejo
2025-03-24 13:08 ` Orzel, Michal
1 sibling, 2 replies; 12+ messages in thread
From: Andrew Cooper @ 2025-03-19 14:05 UTC (permalink / raw)
To: Alejandro Vallejo, Michal Orzel, xen-devel
Cc: Anthony PERARD, Juergen Gross, Luca Fancellu, Stefano Stabellini,
Julien Grall, Bertrand Marquis
On 19/03/2025 2:01 pm, Alejandro Vallejo wrote:
> On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel wrote:
>> We are missing a way to detect whether a user provided a value for
>> nr_spis equal to 0 or did not provide any value (default is also 0) which
>> can cause issues when calculated nr_spis is > 0 and the value from domain
>> config is 0. Fix it by setting default value for nr_spis to UINT32_MAX
>> (max supported nr of SPIs is 960 anyway).
>>
>> Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value")
>> Reported-by: Luca Fancellu <luca.fancellu@arm.com>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>> tools/libs/light/libxl_arm.c | 7 ++++---
>> tools/libs/light/libxl_types.idl | 2 +-
>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 2d895408cac3..5bb5bac61def 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>> libxl_domain_config *d_config,
>> struct xen_domctl_createdomain *config)
>> {
>> - uint32_t nr_spis = 0;
>> + uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis;
>> unsigned int i;
>> uint32_t vuart_irq, virtio_irq = 0;
>> bool vuart_enabled = false, virtio_enabled = false;
>> @@ -181,13 +181,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>
>> LOG(DEBUG, "Configure the domain");
>>
>> - if (nr_spis > d_config->b_info.arch_arm.nr_spis) {
>> + /* We use UINT32_MAX to denote if a user provided a value or not */
>> + if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) {
>> LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n",
>> nr_spis);
>> return ERROR_FAIL;
>> }
>>
>> - config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
>> + config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis;
>> LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
>>
>> switch (d_config->b_info.arch_arm.gic_version) {
>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>> index bd4b8721ff19..129c00ce929c 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -723,7 +723,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>> ("vuart", libxl_vuart_type),
>> ("sve_vl", libxl_sve_type),
>> - ("nr_spis", uint32),
>> + ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}),
>> ])),
>> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>> ])),
> Doesn't this regenerate the golang bindings?
You need a build environment with golang for that to happen. It's easy
to miss.
In this case, best to ask the committer to regen.
~Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] tools/arm: Fix nr_spis handling v2
2025-03-19 14:05 ` Andrew Cooper
@ 2025-03-19 14:37 ` Orzel, Michal
2025-03-19 16:22 ` Alejandro Vallejo
1 sibling, 0 replies; 12+ messages in thread
From: Orzel, Michal @ 2025-03-19 14:37 UTC (permalink / raw)
To: Andrew Cooper, Alejandro Vallejo, xen-devel
Cc: Anthony PERARD, Juergen Gross, Luca Fancellu, Stefano Stabellini,
Julien Grall, Bertrand Marquis
On 19/03/2025 15:05, Andrew Cooper wrote:
>
>
> On 19/03/2025 2:01 pm, Alejandro Vallejo wrote:
>> On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel wrote:
>>> We are missing a way to detect whether a user provided a value for
>>> nr_spis equal to 0 or did not provide any value (default is also 0) which
>>> can cause issues when calculated nr_spis is > 0 and the value from domain
>>> config is 0. Fix it by setting default value for nr_spis to UINT32_MAX
>>> (max supported nr of SPIs is 960 anyway).
>>>
>>> Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value")
>>> Reported-by: Luca Fancellu <luca.fancellu@arm.com>
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>>> tools/libs/light/libxl_arm.c | 7 ++++---
>>> tools/libs/light/libxl_types.idl | 2 +-
>>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>> index 2d895408cac3..5bb5bac61def 100644
>>> --- a/tools/libs/light/libxl_arm.c
>>> +++ b/tools/libs/light/libxl_arm.c
>>> @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>> libxl_domain_config *d_config,
>>> struct xen_domctl_createdomain *config)
>>> {
>>> - uint32_t nr_spis = 0;
>>> + uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis;
>>> unsigned int i;
>>> uint32_t vuart_irq, virtio_irq = 0;
>>> bool vuart_enabled = false, virtio_enabled = false;
>>> @@ -181,13 +181,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>>
>>> LOG(DEBUG, "Configure the domain");
>>>
>>> - if (nr_spis > d_config->b_info.arch_arm.nr_spis) {
>>> + /* We use UINT32_MAX to denote if a user provided a value or not */
>>> + if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) {
>>> LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n",
>>> nr_spis);
>>> return ERROR_FAIL;
>>> }
>>>
>>> - config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
>>> + config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis;
>>> LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
>>>
>>> switch (d_config->b_info.arch_arm.gic_version) {
>>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>>> index bd4b8721ff19..129c00ce929c 100644
>>> --- a/tools/libs/light/libxl_types.idl
>>> +++ b/tools/libs/light/libxl_types.idl
>>> @@ -723,7 +723,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>>> ("vuart", libxl_vuart_type),
>>> ("sve_vl", libxl_sve_type),
>>> - ("nr_spis", uint32),
>>> + ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}),
>>> ])),
>>> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>>> ])),
>> Doesn't this regenerate the golang bindings?
>
> You need a build environment with golang for that to happen. It's easy
> to miss.
>
> In this case, best to ask the committer to regen.
This patch is already waiting for tools Ab, so maybe @Anthony could do that on
commit?
~Michal
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] tools/arm: Fix nr_spis handling v2
2025-03-19 14:05 ` Andrew Cooper
2025-03-19 14:37 ` Orzel, Michal
@ 2025-03-19 16:22 ` Alejandro Vallejo
1 sibling, 0 replies; 12+ messages in thread
From: Alejandro Vallejo @ 2025-03-19 16:22 UTC (permalink / raw)
To: Andrew Cooper, Michal Orzel, xen-devel
Cc: Anthony PERARD, Juergen Gross, Luca Fancellu, Stefano Stabellini,
Julien Grall, Bertrand Marquis
On Wed Mar 19, 2025 at 2:05 PM GMT, Andrew Cooper wrote:
> On 19/03/2025 2:01 pm, Alejandro Vallejo wrote:
> > On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel wrote:
> >> We are missing a way to detect whether a user provided a value for
> >> nr_spis equal to 0 or did not provide any value (default is also 0) which
> >> can cause issues when calculated nr_spis is > 0 and the value from domain
> >> config is 0. Fix it by setting default value for nr_spis to UINT32_MAX
> >> (max supported nr of SPIs is 960 anyway).
> >>
> >> Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value")
> >> Reported-by: Luca Fancellu <luca.fancellu@arm.com>
> >> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> >> ---
> >> tools/libs/light/libxl_arm.c | 7 ++++---
> >> tools/libs/light/libxl_types.idl | 2 +-
> >> 2 files changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> >> index 2d895408cac3..5bb5bac61def 100644
> >> --- a/tools/libs/light/libxl_arm.c
> >> +++ b/tools/libs/light/libxl_arm.c
> >> @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >> libxl_domain_config *d_config,
> >> struct xen_domctl_createdomain *config)
> >> {
> >> - uint32_t nr_spis = 0;
> >> + uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis;
> >> unsigned int i;
> >> uint32_t vuart_irq, virtio_irq = 0;
> >> bool vuart_enabled = false, virtio_enabled = false;
> >> @@ -181,13 +181,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >>
> >> LOG(DEBUG, "Configure the domain");
> >>
> >> - if (nr_spis > d_config->b_info.arch_arm.nr_spis) {
> >> + /* We use UINT32_MAX to denote if a user provided a value or not */
> >> + if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) {
> >> LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n",
> >> nr_spis);
> >> return ERROR_FAIL;
> >> }
> >>
> >> - config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
> >> + config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis;
> >> LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
> >>
> >> switch (d_config->b_info.arch_arm.gic_version) {
> >> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> >> index bd4b8721ff19..129c00ce929c 100644
> >> --- a/tools/libs/light/libxl_types.idl
> >> +++ b/tools/libs/light/libxl_types.idl
> >> @@ -723,7 +723,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> >> ("vuart", libxl_vuart_type),
> >> ("sve_vl", libxl_sve_type),
> >> - ("nr_spis", uint32),
> >> + ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}),
> >> ])),
> >> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> >> ])),
> > Doesn't this regenerate the golang bindings?
>
> You need a build environment with golang for that to happen. It's easy
> to miss.
>
> In this case, best to ask the committer to regen.
>
> ~Andrew
One more thing for the list of stuff I'd like to add to CI at some point.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tools/arm: Fix nr_spis handling v2
2025-03-19 14:01 ` Alejandro Vallejo
2025-03-19 14:05 ` Andrew Cooper
@ 2025-03-24 13:08 ` Orzel, Michal
2025-03-24 15:22 ` Alejandro Vallejo
1 sibling, 1 reply; 12+ messages in thread
From: Orzel, Michal @ 2025-03-24 13:08 UTC (permalink / raw)
To: Alejandro Vallejo, xen-devel
Cc: Anthony PERARD, Juergen Gross, Luca Fancellu, Stefano Stabellini,
Julien Grall, Bertrand Marquis
On 19/03/2025 15:01, Alejandro Vallejo wrote:
>
>
> On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel wrote:
>> We are missing a way to detect whether a user provided a value for
>> nr_spis equal to 0 or did not provide any value (default is also 0) which
>> can cause issues when calculated nr_spis is > 0 and the value from domain
>> config is 0. Fix it by setting default value for nr_spis to UINT32_MAX
>> (max supported nr of SPIs is 960 anyway).
>>
>> Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value")
>> Reported-by: Luca Fancellu <luca.fancellu@arm.com>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>> tools/libs/light/libxl_arm.c | 7 ++++---
>> tools/libs/light/libxl_types.idl | 2 +-
>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 2d895408cac3..5bb5bac61def 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>> libxl_domain_config *d_config,
>> struct xen_domctl_createdomain *config)
>> {
>> - uint32_t nr_spis = 0;
>> + uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis;
>> unsigned int i;
>> uint32_t vuart_irq, virtio_irq = 0;
>> bool vuart_enabled = false, virtio_enabled = false;
>> @@ -181,13 +181,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>
>> LOG(DEBUG, "Configure the domain");
>>
>> - if (nr_spis > d_config->b_info.arch_arm.nr_spis) {
>> + /* We use UINT32_MAX to denote if a user provided a value or not */
>> + if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) {
>> LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n",
>> nr_spis);
>> return ERROR_FAIL;
>> }
>>
>> - config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
>> + config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis;
>> LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
>>
>> switch (d_config->b_info.arch_arm.gic_version) {
>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>> index bd4b8721ff19..129c00ce929c 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -723,7 +723,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>> ("vuart", libxl_vuart_type),
>> ("sve_vl", libxl_sve_type),
>> - ("nr_spis", uint32),
>> + ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}),
>> ])),
>> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>> ])),
>
> Doesn't this regenerate the golang bindings?
FYI, it does not. The bindings are already there for NrSpis and default value is
does not result in a change (for verification I checked max_grant_frames that
uses LIBXL_MAX_GRANT_DEFAULT).
~Michal
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] tools/arm: Fix nr_spis handling v2
2025-03-24 13:08 ` Orzel, Michal
@ 2025-03-24 15:22 ` Alejandro Vallejo
2025-03-24 15:31 ` Orzel, Michal
0 siblings, 1 reply; 12+ messages in thread
From: Alejandro Vallejo @ 2025-03-24 15:22 UTC (permalink / raw)
To: Orzel, Michal, xen-devel
Cc: Anthony PERARD, Juergen Gross, Luca Fancellu, Stefano Stabellini,
Julien Grall, Bertrand Marquis
On Mon Mar 24, 2025 at 1:08 PM GMT, Michal Orzel wrote:
>
>
> On 19/03/2025 15:01, Alejandro Vallejo wrote:
> >
> > Doesn't this regenerate the golang bindings?
> FYI, it does not. The bindings are already there for NrSpis and default value is
> does not result in a change (for verification I checked max_grant_frames that
> uses LIBXL_MAX_GRANT_DEFAULT).
>
> ~Michal
Oh, good then. Though it does raise the (completely separate) question of how
correct those bindings might be if they ignore the IDL's default values... :/
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tools/arm: Fix nr_spis handling v2
2025-03-24 15:22 ` Alejandro Vallejo
@ 2025-03-24 15:31 ` Orzel, Michal
2025-03-25 14:10 ` Alejandro Vallejo
0 siblings, 1 reply; 12+ messages in thread
From: Orzel, Michal @ 2025-03-24 15:31 UTC (permalink / raw)
To: Alejandro Vallejo, xen-devel
Cc: Anthony PERARD, Juergen Gross, Luca Fancellu, Stefano Stabellini,
Julien Grall, Bertrand Marquis
On 24/03/2025 16:22, Alejandro Vallejo wrote:
>
>
> On Mon Mar 24, 2025 at 1:08 PM GMT, Michal Orzel wrote:
>>
>>
>> On 19/03/2025 15:01, Alejandro Vallejo wrote:
>>>
>>> Doesn't this regenerate the golang bindings?
>> FYI, it does not. The bindings are already there for NrSpis and default value is
>> does not result in a change (for verification I checked max_grant_frames that
>> uses LIBXL_MAX_GRANT_DEFAULT).
>>
>> ~Michal
>
> Oh, good then. Though it does raise the (completely separate) question of how
> correct those bindings might be if they ignore the IDL's default values... :/
Why ignore. AFAICS libxl_domain_build_info_init is called there to grab default
values.
~Michal
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tools/arm: Fix nr_spis handling v2
2025-03-24 15:31 ` Orzel, Michal
@ 2025-03-25 14:10 ` Alejandro Vallejo
0 siblings, 0 replies; 12+ messages in thread
From: Alejandro Vallejo @ 2025-03-25 14:10 UTC (permalink / raw)
To: Orzel, Michal, xen-devel
Cc: Anthony PERARD, Juergen Gross, Luca Fancellu, Stefano Stabellini,
Julien Grall, Bertrand Marquis
On Mon Mar 24, 2025 at 3:31 PM GMT, Michal Orzel wrote:
>
>
> On 24/03/2025 16:22, Alejandro Vallejo wrote:
> >
> >
> > On Mon Mar 24, 2025 at 1:08 PM GMT, Michal Orzel wrote:
> >>
> >>
> >> On 19/03/2025 15:01, Alejandro Vallejo wrote:
> >>>
> >>> Doesn't this regenerate the golang bindings?
> >> FYI, it does not. The bindings are already there for NrSpis and default value is
> >> does not result in a change (for verification I checked max_grant_frames that
> >> uses LIBXL_MAX_GRANT_DEFAULT).
> >>
> >> ~Michal
> >
> > Oh, good then. Though it does raise the (completely separate) question of how
> > correct those bindings might be if they ignore the IDL's default values... :/
> Why ignore. AFAICS libxl_domain_build_info_init is called there to grab default
> values.
>
> ~Michal
Oh, so the default populator is itself autogenerated C with a golang binding
rather than native golang. Fair enough. The more you learn.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tools/arm: Fix nr_spis handling v2
2025-03-18 9:00 [PATCH] tools/arm: Fix nr_spis handling v2 Michal Orzel
2025-03-18 9:22 ` Luca Fancellu
2025-03-19 14:01 ` Alejandro Vallejo
@ 2025-03-20 16:46 ` Anthony PERARD
2025-03-24 8:54 ` Orzel, Michal
2 siblings, 1 reply; 12+ messages in thread
From: Anthony PERARD @ 2025-03-20 16:46 UTC (permalink / raw)
To: Michal Orzel
Cc: xen-devel, Juergen Gross, Luca Fancellu, Stefano Stabellini,
Julien Grall, Bertrand Marquis
On Tue, Mar 18, 2025 at 10:00:13AM +0100, Michal Orzel wrote:
> We are missing a way to detect whether a user provided a value for
> nr_spis equal to 0 or did not provide any value (default is also 0) which
> can cause issues when calculated nr_spis is > 0 and the value from domain
> config is 0. Fix it by setting default value for nr_spis to UINT32_MAX
> (max supported nr of SPIs is 960 anyway).
>
> Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value")
> Reported-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
> tools/libs/light/libxl_arm.c | 7 ++++---
> tools/libs/light/libxl_types.idl | 2 +-
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 2d895408cac3..5bb5bac61def 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> libxl_domain_config *d_config,
> struct xen_domctl_createdomain *config)
> {
> - uint32_t nr_spis = 0;
> + uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis;
> unsigned int i;
> uint32_t vuart_irq, virtio_irq = 0;
> bool vuart_enabled = false, virtio_enabled = false;
> @@ -181,13 +181,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>
> LOG(DEBUG, "Configure the domain");
>
> - if (nr_spis > d_config->b_info.arch_arm.nr_spis) {
> + /* We use UINT32_MAX to denote if a user provided a value or not */
> + if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) {
> LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n",
> nr_spis);
> return ERROR_FAIL;
> }
>
> - config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
> + config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis;
Could you try to check only once if there is a user provided value for
nr_spis?
> LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
>
> switch (d_config->b_info.arch_arm.gic_version) {
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index bd4b8721ff19..129c00ce929c 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -723,7 +723,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> ("vuart", libxl_vuart_type),
> ("sve_vl", libxl_sve_type),
> - ("nr_spis", uint32),
> + ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}),
Could you introduce something like LIBXL_NR_SPIS_DEFAULT in libxl.h
instead? (Like we have LIBXL_MAX_GRANT_DEFAULT or LIBXL_MEMKB_DEFAULT)
Thanks,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] tools/arm: Fix nr_spis handling v2
2025-03-20 16:46 ` Anthony PERARD
@ 2025-03-24 8:54 ` Orzel, Michal
0 siblings, 0 replies; 12+ messages in thread
From: Orzel, Michal @ 2025-03-24 8:54 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, Juergen Gross, Luca Fancellu, Stefano Stabellini,
Julien Grall, Bertrand Marquis
On 20/03/2025 17:46, Anthony PERARD wrote:
>
>
> On Tue, Mar 18, 2025 at 10:00:13AM +0100, Michal Orzel wrote:
>> We are missing a way to detect whether a user provided a value for
>> nr_spis equal to 0 or did not provide any value (default is also 0) which
>> can cause issues when calculated nr_spis is > 0 and the value from domain
>> config is 0. Fix it by setting default value for nr_spis to UINT32_MAX
>> (max supported nr of SPIs is 960 anyway).
>>
>> Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value")
>> Reported-by: Luca Fancellu <luca.fancellu@arm.com>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>> tools/libs/light/libxl_arm.c | 7 ++++---
>> tools/libs/light/libxl_types.idl | 2 +-
>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 2d895408cac3..5bb5bac61def 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>> libxl_domain_config *d_config,
>> struct xen_domctl_createdomain *config)
>> {
>> - uint32_t nr_spis = 0;
>> + uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis;
>> unsigned int i;
>> uint32_t vuart_irq, virtio_irq = 0;
>> bool vuart_enabled = false, virtio_enabled = false;
>> @@ -181,13 +181,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>
>> LOG(DEBUG, "Configure the domain");
>>
>> - if (nr_spis > d_config->b_info.arch_arm.nr_spis) {
>> + /* We use UINT32_MAX to denote if a user provided a value or not */
>> + if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) {
>> LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n",
>> nr_spis);
>> return ERROR_FAIL;
>> }
>>
>> - config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
>> + config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis;
>
> Could you try to check only once if there is a user provided value for
> nr_spis?
Sure, but that's one extra variable or more if/else blocks.
>
>> LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
>>
>> switch (d_config->b_info.arch_arm.gic_version) {
>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>> index bd4b8721ff19..129c00ce929c 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -723,7 +723,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>> ("vuart", libxl_vuart_type),
>> ("sve_vl", libxl_sve_type),
>> - ("nr_spis", uint32),
>> + ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}),
>
> Could you introduce something like LIBXL_NR_SPIS_DEFAULT in libxl.h
> instead? (Like we have LIBXL_MAX_GRANT_DEFAULT or LIBXL_MEMKB_DEFAULT)
Sure, no problem.
~Michal
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-03-25 14:11 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-18 9:00 [PATCH] tools/arm: Fix nr_spis handling v2 Michal Orzel
2025-03-18 9:22 ` Luca Fancellu
2025-03-19 14:01 ` Alejandro Vallejo
2025-03-19 14:05 ` Andrew Cooper
2025-03-19 14:37 ` Orzel, Michal
2025-03-19 16:22 ` Alejandro Vallejo
2025-03-24 13:08 ` Orzel, Michal
2025-03-24 15:22 ` Alejandro Vallejo
2025-03-24 15:31 ` Orzel, Michal
2025-03-25 14:10 ` Alejandro Vallejo
2025-03-20 16:46 ` Anthony PERARD
2025-03-24 8:54 ` Orzel, Michal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.