diff mbox series

[2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id

Message ID 20220517194113.2574-3-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series Fix some problems with "arm/dom0less: assign dom0less guests to cpupools" | expand

Commit Message

Andrew Cooper May 17, 2022, 7:41 p.m. UTC
Sadly, cpupool IDs are chosen by the caller, not assigned sequentially, so
this does need to have a full 32 bits of range.

Also leave a BUILD_BUG_ON() to catch more obvious ABI changes in the future.

Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Edwin Török <edvin.torok@citrix.com>
CC: Luca Fancellu <luca.fancellu@arm.com>
---
 tools/ocaml/libs/xc/xenctrl.ml      | 1 +
 tools/ocaml/libs/xc/xenctrl.mli     | 1 +
 tools/ocaml/libs/xc/xenctrl_stubs.c | 8 +++++++-
 3 files changed, 9 insertions(+), 1 deletion(-)

Comments

Christian Lindig May 18, 2022, 8:04 a.m. UTC | #1
Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>


On 17 May 2022, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com<mailto:andrew.cooper3@citrix.com>> wrote:

Sadly, cpupool IDs are chosen by the caller, not assigned sequentially, so
this does need to have a full 32 bits of range.

Also leave a BUILD_BUG_ON() to catch more obvious ABI changes in the future.

Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com<mailto:andrew.cooper3@citrix.com>>
---
CC: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>
CC: Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>>
CC: Luca Fancellu <luca.fancellu@arm.com<mailto:luca.fancellu@arm.com>>
---
tools/ocaml/libs/xc/xenctrl.ml      | 1 +
tools/ocaml/libs/xc/xenctrl.mli     | 1 +
tools/ocaml/libs/xc/xenctrl_stubs.c | 8 +++++++-
3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 7503031d8f61..8eab6f60eb14 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -85,6 +85,7 @@ type domctl_create_config =
max_grant_frames: int;
max_maptrack_frames: int;
max_grant_version: int;
+ cpupool_id: int32;
arch: arch_domainconfig;
}

diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index d1d9c9247afc..d3014a2708d8 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -77,6 +77,7 @@ type domctl_create_config = {
  max_grant_frames: int;
  max_maptrack_frames: int;
  max_grant_version: int;
+  cpupool_id: int32;
  arch: arch_domainconfig;
}

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 5b4fe72c8dec..513ee142d2a0 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -189,7 +189,8 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
#define VAL_MAX_GRANT_FRAMES    Field(config, 6)
#define VAL_MAX_MAPTRACK_FRAMES Field(config, 7)
#define VAL_MAX_GRANT_VERSION   Field(config, 8)
-#define VAL_ARCH                Field(config, 9)
+#define VAL_CPUPOOL_ID          Field(config, 9)
+#define VAL_ARCH                Field(config, 10)

uint32_t domid = Int_val(wanted_domid);
int result;
@@ -201,6 +202,7 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
.max_maptrack_frames = Int_val(VAL_MAX_MAPTRACK_FRAMES),
.grant_opts =
   XEN_DOMCTL_GRANT_version(Int_val(VAL_MAX_GRANT_VERSION)),
+ .cpupool_id = Int32_val(VAL_CPUPOOL_ID),
};

domain_handle_of_uuid_string(cfg.handle, String_val(VAL_HANDLE));
@@ -225,6 +227,9 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
case 1: /* X86 - emulation flags in the block */
#if defined(__i386__) || defined(__x86_64__)

+ /* Quick & dirty check for ABI changes. */
+ BUILD_BUG_ON(sizeof(cfg) != 64);
+
        /* Mnemonics for the named fields inside xen_x86_arch_domainconfig */
#define VAL_EMUL_FLAGS          Field(arch_domconfig, 0)

@@ -254,6 +259,7 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
}

#undef VAL_ARCH
+#undef VAL_CPUPOOL_ID
#undef VAL_MAX_GRANT_VERSION
#undef VAL_MAX_MAPTRACK_FRAMES
#undef VAL_MAX_GRANT_FRAMES
--
2.11.0
Edwin Torok May 18, 2022, 9:51 a.m. UTC | #2
> On 17 May 2022, at 20:41, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> Sadly, cpupool IDs are chosen by the caller, not assigned sequentially, so
> this does need to have a full 32 bits of range.
> 
> Also leave a BUILD_BUG_ON() to catch more obvious ABI changes in the future.
> 
> Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks for the fix.


> ---
> CC: Christian Lindig <christian.lindig@citrix.com>
> CC: Edwin Török <edvin.torok@citrix.com>
> CC: Luca Fancellu <luca.fancellu@arm.com>
> ---
> tools/ocaml/libs/xc/xenctrl.ml      | 1 +
> tools/ocaml/libs/xc/xenctrl.mli     | 1 +
> tools/ocaml/libs/xc/xenctrl_stubs.c | 8 +++++++-
> 3 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index 7503031d8f61..8eab6f60eb14 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -85,6 +85,7 @@ type domctl_create_config =
> 	max_grant_frames: int;
> 	max_maptrack_frames: int;
> 	max_grant_version: int;
> +	cpupool_id: int32;

What are the valid values for a CPU pool id, in particular what value should be passed here to get back the behaviour prior to these changes in Xen?
(i.e. would it be cpu pool id 0 or -1 if cpu pools aren't otherwise explicitly configured on the system)

Thanks,
--Edwin

> 	arch: arch_domainconfig;
> }
> 
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
> index d1d9c9247afc..d3014a2708d8 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -77,6 +77,7 @@ type domctl_create_config = {
>   max_grant_frames: int;
>   max_maptrack_frames: int;
>   max_grant_version: int;
> +  cpupool_id: int32;
>   arch: arch_domainconfig;
> }
> 
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index 5b4fe72c8dec..513ee142d2a0 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -189,7 +189,8 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
> #define VAL_MAX_GRANT_FRAMES    Field(config, 6)
> #define VAL_MAX_MAPTRACK_FRAMES Field(config, 7)
> #define VAL_MAX_GRANT_VERSION   Field(config, 8)
> -#define VAL_ARCH                Field(config, 9)
> +#define VAL_CPUPOOL_ID          Field(config, 9)
> +#define VAL_ARCH                Field(config, 10)
> 
> 	uint32_t domid = Int_val(wanted_domid);
> 	int result;
> @@ -201,6 +202,7 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
> 		.max_maptrack_frames = Int_val(VAL_MAX_MAPTRACK_FRAMES),
> 		.grant_opts =
> 		    XEN_DOMCTL_GRANT_version(Int_val(VAL_MAX_GRANT_VERSION)),
> +		.cpupool_id = Int32_val(VAL_CPUPOOL_ID),
> 	};
> 
> 	domain_handle_of_uuid_string(cfg.handle, String_val(VAL_HANDLE));
> @@ -225,6 +227,9 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
> 	case 1: /* X86 - emulation flags in the block */
> #if defined(__i386__) || defined(__x86_64__)
> 
> +		/* Quick & dirty check for ABI changes. */
> +		BUILD_BUG_ON(sizeof(cfg) != 64);
> +
>         /* Mnemonics for the named fields inside xen_x86_arch_domainconfig */
> #define VAL_EMUL_FLAGS          Field(arch_domconfig, 0)
> 
> @@ -254,6 +259,7 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
> 	}
> 
> #undef VAL_ARCH
> +#undef VAL_CPUPOOL_ID
> #undef VAL_MAX_GRANT_VERSION
> #undef VAL_MAX_MAPTRACK_FRAMES
> #undef VAL_MAX_GRANT_FRAMES
> -- 
> 2.11.0
>
Andrew Cooper May 18, 2022, 10:12 a.m. UTC | #3
On 18/05/2022 10:51, Edwin Torok wrote:
>> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
>> index 7503031d8f61..8eab6f60eb14 100644
>> --- a/tools/ocaml/libs/xc/xenctrl.ml
>> +++ b/tools/ocaml/libs/xc/xenctrl.ml
>> @@ -85,6 +85,7 @@ type domctl_create_config =
>> 	max_grant_frames: int;
>> 	max_maptrack_frames: int;
>> 	max_grant_version: int;
>> +	cpupool_id: int32;
> What are the valid values for a CPU pool id, in particular what value should be passed here to get back the behaviour prior to these changes in Xen?
> (i.e. would it be cpu pool id 0 or -1 if cpu pools aren't otherwise explicitly configured on the system)

cpupools are a non-optional construct in Xen.

By default, one cpupool exists, with the id 0, using the default
scheduler covering all pCPUs, and dom0 is constructed in this cpupool.

Passing 0 here is the backwards compatible option.

And on that note, Luca, you ought to patch xl/libxl to apply the pool=
setting directly during domain create, rather than depending on cpupool
0 existing and moving the domain later.

~Andrew
Luca Fancellu May 18, 2022, 10:30 a.m. UTC | #4
> On 18 May 2022, at 11:12, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> On 18/05/2022 10:51, Edwin Torok wrote:
>>> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
>>> index 7503031d8f61..8eab6f60eb14 100644
>>> --- a/tools/ocaml/libs/xc/xenctrl.ml
>>> +++ b/tools/ocaml/libs/xc/xenctrl.ml
>>> @@ -85,6 +85,7 @@ type domctl_create_config =
>>> 	max_grant_frames: int;
>>> 	max_maptrack_frames: int;
>>> 	max_grant_version: int;
>>> +	cpupool_id: int32;
>> What are the valid values for a CPU pool id, in particular what value should be passed here to get back the behaviour prior to these changes in Xen?
>> (i.e. would it be cpu pool id 0 or -1 if cpu pools aren't otherwise explicitly configured on the system)
> 
> cpupools are a non-optional construct in Xen.
> 
> By default, one cpupool exists, with the id 0, using the default
> scheduler covering all pCPUs, and dom0 is constructed in this cpupool.
> 
> Passing 0 here is the backwards compatible option.
> 
> And on that note, Luca, you ought to patch xl/libxl to apply the pool=
> setting directly during domain create, rather than depending on cpupool
> 0 existing and moving the domain later.

Is it an enhancement or a bug fix? From what I know, please correct me if I’m wrong, cpupool0
Is always present, so there won’t be problem on assuming its existence

> 
> ~Andrew
Andrew Cooper May 18, 2022, 11:34 a.m. UTC | #5
On 18/05/2022 11:30, Luca Fancellu wrote:
>> On 18 May 2022, at 11:12, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
>>
>> On 18/05/2022 10:51, Edwin Torok wrote:
>>>> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
>>>> index 7503031d8f61..8eab6f60eb14 100644
>>>> --- a/tools/ocaml/libs/xc/xenctrl.ml
>>>> +++ b/tools/ocaml/libs/xc/xenctrl.ml
>>>> @@ -85,6 +85,7 @@ type domctl_create_config =
>>>> 	max_grant_frames: int;
>>>> 	max_maptrack_frames: int;
>>>> 	max_grant_version: int;
>>>> +	cpupool_id: int32;
>>> What are the valid values for a CPU pool id, in particular what value should be passed here to get back the behaviour prior to these changes in Xen?
>>> (i.e. would it be cpu pool id 0 or -1 if cpu pools aren't otherwise explicitly configured on the system)
>> cpupools are a non-optional construct in Xen.
>>
>> By default, one cpupool exists, with the id 0, using the default
>> scheduler covering all pCPUs, and dom0 is constructed in this cpupool.
>>
>> Passing 0 here is the backwards compatible option.
>>
>> And on that note, Luca, you ought to patch xl/libxl to apply the pool=
>> setting directly during domain create, rather than depending on cpupool
>> 0 existing and moving the domain later.
> Is it an enhancement or a bug fix?

This isn't a binary option.

Your series added an optimisation to DOMCTL_createdomain, then didn't
adjust libxl to use the optimisation (which would have reduced the
number of hypercalls to create the domain, and reduce the number of
dynamic memory allocations in the hypervisor.  Marginal, certainly, but
clearly a nice-to-have).

Therefore, you created technical debt, which is option 3.

By default, as the contributor, you are expected to address the
technical debt, because it is an important difference between hacking a
feature up for yourself, and integrating the feature nicely for everyone.

You can of course negotiate with the tools maintainer to see if they
care, and right now that's a bit difficult.  It's quite possible that
noone other than me cares, and I'm not libxl maintainer.

Either you need to pay off the technical debt, or someone else will have
to.  Someone else is going to have to start with digging into source
history, which means it's more expensive than you doing it now.

At an absolute minimum, you need to be aware of where/when you are
creating technical debt.

>  From what I know, please correct me if I’m wrong, cpupool0
> Is always present, so there won’t be problem on assuming its existence

From what I can see, your series has reduced the magic involved with
cpupool0, which is good.

But the fact that it still has magic properties is still technical debt
that someone is going to have to pay off eventually.

~Andrew
Luca Fancellu May 18, 2022, 2:34 p.m. UTC | #6
+ Adding toolstack maintainer

> On 18 May 2022, at 12:34, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 18/05/2022 11:30, Luca Fancellu wrote:
>>> On 18 May 2022, at 11:12, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
>>> 
>>> On 18/05/2022 10:51, Edwin Torok wrote:
>>>>> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
>>>>> index 7503031d8f61..8eab6f60eb14 100644
>>>>> --- a/tools/ocaml/libs/xc/xenctrl.ml
>>>>> +++ b/tools/ocaml/libs/xc/xenctrl.ml
>>>>> @@ -85,6 +85,7 @@ type domctl_create_config =
>>>>> 	max_grant_frames: int;
>>>>> 	max_maptrack_frames: int;
>>>>> 	max_grant_version: int;
>>>>> +	cpupool_id: int32;
>>>> What are the valid values for a CPU pool id, in particular what value should be passed here to get back the behaviour prior to these changes in Xen?
>>>> (i.e. would it be cpu pool id 0 or -1 if cpu pools aren't otherwise explicitly configured on the system)
>>> cpupools are a non-optional construct in Xen.
>>> 
>>> By default, one cpupool exists, with the id 0, using the default
>>> scheduler covering all pCPUs, and dom0 is constructed in this cpupool.
>>> 
>>> Passing 0 here is the backwards compatible option.
>>> 
>>> And on that note, Luca, you ought to patch xl/libxl to apply the pool=
>>> setting directly during domain create, rather than depending on cpupool
>>> 0 existing and moving the domain later.
>> Is it an enhancement or a bug fix?
> 
> This isn't a binary option.
> 
> Your series added an optimisation to DOMCTL_createdomain, then didn't
> adjust libxl to use the optimisation (which would have reduced the
> number of hypercalls to create the domain, and reduce the number of
> dynamic memory allocations in the hypervisor.  Marginal, certainly, but
> clearly a nice-to-have).
> 
> Therefore, you created technical debt, which is option 3.
> 
> By default, as the contributor, you are expected to address the
> technical debt, because it is an important difference between hacking a
> feature up for yourself, and integrating the feature nicely for everyone.
> 
> You can of course negotiate with the tools maintainer to see if they
> care, and right now that's a bit difficult.  It's quite possible that
> noone other than me cares, and I'm not libxl maintainer.
> 
> Either you need to pay off the technical debt, or someone else will have
> to.  Someone else is going to have to start with digging into source
> history, which means it's more expensive than you doing it now.
> 
> At an absolute minimum, you need to be aware of where/when you are
> creating technical debt.

Ok, we've just created a task to handle this work so that we can track it, we will
handle it in the future.

Cheers,
Luca

> 
>> From what I know, please correct me if I’m wrong, cpupool0
>> Is always present, so there won’t be problem on assuming its existence
> 
> From what I can see, your series has reduced the magic involved with
> cpupool0, which is good.
> 
> But the fact that it still has magic properties is still technical debt
> that someone is going to have to pay off eventually.
> 
> ~Andrew
diff mbox series

Patch

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 7503031d8f61..8eab6f60eb14 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -85,6 +85,7 @@  type domctl_create_config =
 	max_grant_frames: int;
 	max_maptrack_frames: int;
 	max_grant_version: int;
+	cpupool_id: int32;
 	arch: arch_domainconfig;
 }
 
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index d1d9c9247afc..d3014a2708d8 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -77,6 +77,7 @@  type domctl_create_config = {
   max_grant_frames: int;
   max_maptrack_frames: int;
   max_grant_version: int;
+  cpupool_id: int32;
   arch: arch_domainconfig;
 }
 
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 5b4fe72c8dec..513ee142d2a0 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -189,7 +189,8 @@  CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
 #define VAL_MAX_GRANT_FRAMES    Field(config, 6)
 #define VAL_MAX_MAPTRACK_FRAMES Field(config, 7)
 #define VAL_MAX_GRANT_VERSION   Field(config, 8)
-#define VAL_ARCH                Field(config, 9)
+#define VAL_CPUPOOL_ID          Field(config, 9)
+#define VAL_ARCH                Field(config, 10)
 
 	uint32_t domid = Int_val(wanted_domid);
 	int result;
@@ -201,6 +202,7 @@  CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
 		.max_maptrack_frames = Int_val(VAL_MAX_MAPTRACK_FRAMES),
 		.grant_opts =
 		    XEN_DOMCTL_GRANT_version(Int_val(VAL_MAX_GRANT_VERSION)),
+		.cpupool_id = Int32_val(VAL_CPUPOOL_ID),
 	};
 
 	domain_handle_of_uuid_string(cfg.handle, String_val(VAL_HANDLE));
@@ -225,6 +227,9 @@  CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
 	case 1: /* X86 - emulation flags in the block */
 #if defined(__i386__) || defined(__x86_64__)
 
+		/* Quick & dirty check for ABI changes. */
+		BUILD_BUG_ON(sizeof(cfg) != 64);
+
         /* Mnemonics for the named fields inside xen_x86_arch_domainconfig */
 #define VAL_EMUL_FLAGS          Field(arch_domconfig, 0)
 
@@ -254,6 +259,7 @@  CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
 	}
 
 #undef VAL_ARCH
+#undef VAL_CPUPOOL_ID
 #undef VAL_MAX_GRANT_VERSION
 #undef VAL_MAX_MAPTRACK_FRAMES
 #undef VAL_MAX_GRANT_FRAMES