* [PATCH 0/2] Fix some problems with "arm/dom0less: assign dom0less guests to cpupools" @ 2022-05-17 19:41 Andrew Cooper 2022-05-17 19:41 ` [PATCH 1/2] xen/cpupool: Reject attempts to add a domain to CPUPOOLID_NONE Andrew Cooper 2022-05-17 19:41 ` [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id Andrew Cooper 0 siblings, 2 replies; 12+ messages in thread From: Andrew Cooper @ 2022-05-17 19:41 UTC (permalink / raw) To: Xen-devel Cc: Andrew Cooper, Juergen Gross, Dario Faggioli, Christian Lindig, Edwin Török, Luca Fancellu, Stefano Stabellini, Julien Grall, Volodymyr Babchuk, Bertrand Marquis, George Dunlap, Nick Rosbrook, Roger Pau Monné, Doug Goldstein ARM folks: Please be rather more careful when exposing hypervisor internals to arbitrary user input. Being domain_create, the fallout is unlikely to be an security issue if it had gotten into a release, but Xen will definitely have an unhappy time with unexpected scheduler state. George/Nick: The Golang bindings seem pre-existingly broken. I get the following spew which is unrelated to this change: ./helpers.gen.go:800[/tmp/go-build762104750/_/local/xen.git/tools/golang/xenlight/_obj/helpers.gen.cgo1.go:1320]: cannot use _Ctype_ulong(numVcpus) * _Cconst_sizeof_libxl_sched_params (type _Ctype_ulong) as type _Ctype_size_t in argument to _Cfunc__CMalloc ./helpers.gen.go:1292[/tmp/go-build762104750/_/local/xen.git/tools/golang/xenlight/_obj/helpers.gen.cgo1.go:1960]: cannot use _Ctype_ulong(numVcpuHardAffinity) * _Cconst_sizeof_libxl_bitmap (type _Ctype_ulong) as type _Ctype_size_t in argument to _Cfunc__CMalloc ./helpers.gen.go:1302[/tmp/go-build762104750/_/local/xen.git/tools/golang/xenlight/_obj/helpers.gen.cgo1.go:1970]: cannot use _Ctype_ulong(numVcpuSoftAffinity) * _Cconst_sizeof_libxl_bitmap (type _Ctype_ulong) as type _Ctype_size_t in argument to _Cfunc__CMalloc ./helpers.gen.go:1336[/tmp/go-build762104750/_/local/xen.git/tools/golang/xenlight/_obj/helpers.gen.cgo1.go:2008]: cannot use _Ctype_ulong(numVnumaNodes) * _Cconst_sizeof_libxl_vnode_info (type _Ctype_ulong) as type _Ctype_size_t in argument to _Cfunc__CMalloc ./helpers.gen.go:1379[/tmp/go-build762104750/_/local/xen.git/tools/golang/xenlight/_obj/helpers.gen.cgo1.go:2063]: cannot use _Ctype_ulong(numIoports) * _Cconst_sizeof_libxl_ioport_range (type _Ctype_ulong) as type _Ctype_size_t in argument to _Cfunc__CMalloc ./helpers.gen.go:1397[/tmp/go-build762104750/_/local/xen.git/tools/golang/xenlight/_obj/helpers.gen.cgo1.go:2081]: cannot use _Ctype_ulong(numIomem) * _Cconst_sizeof_libxl_iomem_range (type _Ctype_ulong) as type _Ctype_size_t in argument to _Cfunc__CMalloc ./helpers.gen.go:2518[/tmp/go-build762104750/_/local/xen.git/tools/golang/xenlight/_obj/helpers.gen.cgo1.go:3919]: cannot use _Ctype_ulong(numConnectors) * _Cconst_sizeof_libxl_connector_param (type _Ctype_ulong) as type _Ctype_size_t in argument to _Cfunc__CMalloc ./helpers.gen.go:2676[/tmp/go-build762104750/_/local/xen.git/tools/golang/xenlight/_obj/helpers.gen.cgo1.go:4182]: cannot use _Ctype_ulong(numVsndStreams) * _Cconst_sizeof_libxl_vsnd_stream (type _Ctype_ulong) as type _Ctype_size_t in argument to _Cfunc__CMalloc ./helpers.gen.go:2741[/tmp/go-build762104750/_/local/xen.git/tools/golang/xenlight/_obj/helpers.gen.cgo1.go:4288]: cannot use _Ctype_ulong(numVsndPcms) * _Cconst_sizeof_libxl_vsnd_pcm (type _Ctype_ulong) as type _Ctype_size_t in argument to _Cfunc__CMalloc ./helpers.gen.go:2930[/tmp/go-build762104750/_/local/xen.git/tools/golang/xenlight/_obj/helpers.gen.cgo1.go:4540]: cannot use _Ctype_ulong(numDisks) * _Cconst_sizeof_libxl_device_disk (type _Ctype_ulong) as type _Ctype_size_t in argument to _Cfunc__CMalloc ./helpers.gen.go:2930[/tmp/go-build762104750/_/local/xen.git/tools/golang/xenlight/_obj/helpers.gen.cgo1.go:4540]: too many errors but this is where my knowledge ends. Needless to say, the golang bindings haven't been regenerated with this change in place. Roger/Stefano/Doug: Given the golang breakage, we are presuamably lacking golang from any of the CI containers. Andrew Cooper (2): xen/cpupool: Reject attempts to add a domain to CPUPOOLID_NONE tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id tools/ocaml/libs/xc/xenctrl.ml | 1 + tools/ocaml/libs/xc/xenctrl.mli | 1 + tools/ocaml/libs/xc/xenctrl_stubs.c | 8 +++++++- xen/common/sched/cpupool.c | 2 -- 4 files changed, 9 insertions(+), 3 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] xen/cpupool: Reject attempts to add a domain to CPUPOOLID_NONE 2022-05-17 19:41 [PATCH 0/2] Fix some problems with "arm/dom0less: assign dom0less guests to cpupools" Andrew Cooper @ 2022-05-17 19:41 ` Andrew Cooper 2022-05-18 10:24 ` Juergen Gross 2022-05-18 10:27 ` Luca Fancellu 2022-05-17 19:41 ` [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id Andrew Cooper 1 sibling, 2 replies; 12+ messages in thread From: Andrew Cooper @ 2022-05-17 19:41 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Juergen Gross, Dario Faggioli, Luca Fancellu c/s cfc52148444f ("xen/domain: Reduce the quantity of initialisation for system domains") removed the path in domain_create() which called sched_init_domain() with CPUPOOLID_NONE for system domains. Arguably, that changeset should have cleaned up this path too. However, c/s 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") changed domain_create() from using a hardcoded poolid of 0, to using a value passed by the toolstack. While CPUPOOLID_NONE is an internal constant, userspace can pass -1 for the cpupool_id parameter and attempt to construct a real domain using default ops, which at a minimum will fail the assertion in dom_scheduler(). Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Juergen Gross <jgross@suse.com> CC: Dario Faggioli <dfaggioli@suse.com> CC: Luca Fancellu <luca.fancellu@arm.com> --- xen/common/sched/cpupool.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c index f6e3d97e5288..f1aa2db5f463 100644 --- a/xen/common/sched/cpupool.c +++ b/xen/common/sched/cpupool.c @@ -619,8 +619,6 @@ int cpupool_add_domain(struct domain *d, unsigned int poolid) int rc; int n_dom = 0; - if ( poolid == CPUPOOLID_NONE ) - return 0; spin_lock(&cpupool_lock); c = cpupool_find_by_id(poolid); if ( c == NULL ) -- 2.11.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] xen/cpupool: Reject attempts to add a domain to CPUPOOLID_NONE 2022-05-17 19:41 ` [PATCH 1/2] xen/cpupool: Reject attempts to add a domain to CPUPOOLID_NONE Andrew Cooper @ 2022-05-18 10:24 ` Juergen Gross 2022-05-18 10:27 ` Luca Fancellu 1 sibling, 0 replies; 12+ messages in thread From: Juergen Gross @ 2022-05-18 10:24 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: Dario Faggioli, Luca Fancellu [-- Attachment #1.1.1: Type: text/plain, Size: 945 bytes --] On 17.05.22 21:41, Andrew Cooper wrote: > c/s cfc52148444f ("xen/domain: Reduce the quantity of initialisation for > system domains") removed the path in domain_create() which called > sched_init_domain() with CPUPOOLID_NONE for system domains. > > Arguably, that changeset should have cleaned up this path too. > > However, c/s 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") > changed domain_create() from using a hardcoded poolid of 0, to using a value > passed by the toolstack. > > While CPUPOOLID_NONE is an internal constant, userspace can pass -1 for the > cpupool_id parameter and attempt to construct a real domain using default ops, > which at a minimum will fail the assertion in dom_scheduler(). > > Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen [-- Attachment #1.1.2: OpenPGP public key --] [-- Type: application/pgp-keys, Size: 3149 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 495 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] xen/cpupool: Reject attempts to add a domain to CPUPOOLID_NONE 2022-05-17 19:41 ` [PATCH 1/2] xen/cpupool: Reject attempts to add a domain to CPUPOOLID_NONE Andrew Cooper 2022-05-18 10:24 ` Juergen Gross @ 2022-05-18 10:27 ` Luca Fancellu 2022-05-18 11:14 ` Andrew Cooper 1 sibling, 1 reply; 12+ messages in thread From: Luca Fancellu @ 2022-05-18 10:27 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Juergen Gross, Dario Faggioli Hi Andrew, > On 17 May 2022, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > c/s cfc52148444f ("xen/domain: Reduce the quantity of initialisation for > system domains") removed the path in domain_create() which called > sched_init_domain() with CPUPOOLID_NONE for system domains. > > Arguably, that changeset should have cleaned up this path too. > > However, c/s 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") > changed domain_create() from using a hardcoded poolid of 0, to using a value > passed by the toolstack. > > While CPUPOOLID_NONE is an internal constant, userspace can pass -1 for the > cpupool_id parameter and attempt to construct a real domain using default ops, > which at a minimum will fail the assertion in dom_scheduler(). > > Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks for this fix, with the introduction of 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools”) we’ve checked all the path passing struct xen_domctl_createdomain, and at that time it seems to be that the new cpupool_id member would have been always zero when created from the tool stack, am I wrong? I’m asking so that I will keep in mind for the future. However with your second patch of this serie, the tool stack is able to write it, so I guess this fix now is mandatory. I’ve tested your patch, enabling boot time cpupools, on an arm machine and booting Xen+Dom0 and another DomU by dom0less feature, and all works. Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> Tested-by: Luca Fancellu <luca.fancellu@arm.com> Cheers, Luca ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] xen/cpupool: Reject attempts to add a domain to CPUPOOLID_NONE 2022-05-18 10:27 ` Luca Fancellu @ 2022-05-18 11:14 ` Andrew Cooper 0 siblings, 0 replies; 12+ messages in thread From: Andrew Cooper @ 2022-05-18 11:14 UTC (permalink / raw) To: Luca Fancellu; +Cc: Xen-devel, Juergen Gross, Dario Faggioli On 18/05/2022 11:27, Luca Fancellu wrote: > Hi Andrew, > >> On 17 May 2022, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> c/s cfc52148444f ("xen/domain: Reduce the quantity of initialisation for >> system domains") removed the path in domain_create() which called >> sched_init_domain() with CPUPOOLID_NONE for system domains. >> >> Arguably, that changeset should have cleaned up this path too. >> >> However, c/s 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") >> changed domain_create() from using a hardcoded poolid of 0, to using a value >> passed by the toolstack. >> >> While CPUPOOLID_NONE is an internal constant, userspace can pass -1 for the >> cpupool_id parameter and attempt to construct a real domain using default ops, >> which at a minimum will fail the assertion in dom_scheduler(). >> >> Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Thanks for this fix, with the introduction of 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools”) > we’ve checked all the path passing struct xen_domctl_createdomain, and at that time it seems to be that > the new cpupool_id member would have been always zero when created from the tool stack, am I wrong? Hypercalls are an entirely public API/ABI. Looking through xen.git gets you the common users, but it most definitely doesn't get you all users of the interface. This hypercall specifically gets fuzzed (there's a KFX PoC somewhere), but it's a bug for any hypercall to be able to hit an assertion/crash/etc. > I’m asking so that I will keep in mind for the future. > > However with your second patch of this serie, the tool stack is able to write it, so I guess this fix now is mandatory. > > I’ve tested your patch, enabling boot time cpupools, on an arm machine and booting Xen+Dom0 and another DomU > by dom0less feature, and all works. > > Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> > Tested-by: Luca Fancellu <luca.fancellu@arm.com> Thanks. ~Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id 2022-05-17 19:41 [PATCH 0/2] Fix some problems with "arm/dom0less: assign dom0less guests to cpupools" Andrew Cooper 2022-05-17 19:41 ` [PATCH 1/2] xen/cpupool: Reject attempts to add a domain to CPUPOOLID_NONE Andrew Cooper @ 2022-05-17 19:41 ` Andrew Cooper 2022-05-18 8:04 ` Christian Lindig 2022-05-18 9:51 ` Edwin Torok 1 sibling, 2 replies; 12+ messages in thread From: Andrew Cooper @ 2022-05-17 19:41 UTC (permalink / raw) To: Xen-devel Cc: Andrew Cooper, Christian Lindig, Edwin Török, Luca Fancellu 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(-) 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id 2022-05-17 19:41 ` [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id Andrew Cooper @ 2022-05-18 8:04 ` Christian Lindig 2022-05-18 9:51 ` Edwin Torok 1 sibling, 0 replies; 12+ messages in thread From: Christian Lindig @ 2022-05-18 8:04 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Edwin Torok, Luca Fancellu [-- Attachment #1: Type: text/plain, Size: 3483 bytes --] 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 [-- Attachment #2: Type: text/html, Size: 7264 bytes --] ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id 2022-05-17 19:41 ` [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id Andrew Cooper 2022-05-18 8:04 ` Christian Lindig @ 2022-05-18 9:51 ` Edwin Torok 2022-05-18 10:12 ` Andrew Cooper 1 sibling, 1 reply; 12+ messages in thread From: Edwin Torok @ 2022-05-18 9:51 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Christian Lindig, Luca Fancellu > 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id 2022-05-18 9:51 ` Edwin Torok @ 2022-05-18 10:12 ` Andrew Cooper 2022-05-18 10:30 ` Luca Fancellu 0 siblings, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2022-05-18 10:12 UTC (permalink / raw) To: Edwin Torok; +Cc: Xen-devel, Christian Lindig, Luca Fancellu 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id 2022-05-18 10:12 ` Andrew Cooper @ 2022-05-18 10:30 ` Luca Fancellu 2022-05-18 11:34 ` Andrew Cooper 0 siblings, 1 reply; 12+ messages in thread From: Luca Fancellu @ 2022-05-18 10:30 UTC (permalink / raw) To: Andrew Cooper; +Cc: Edwin Torok, Xen-devel, Christian Lindig > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id 2022-05-18 10:30 ` Luca Fancellu @ 2022-05-18 11:34 ` Andrew Cooper 2022-05-18 14:34 ` Luca Fancellu 0 siblings, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2022-05-18 11:34 UTC (permalink / raw) To: Luca Fancellu; +Cc: Edwin Torok, Xen-devel, Christian Lindig 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id 2022-05-18 11:34 ` Andrew Cooper @ 2022-05-18 14:34 ` Luca Fancellu 0 siblings, 0 replies; 12+ messages in thread From: Luca Fancellu @ 2022-05-18 14:34 UTC (permalink / raw) To: Andrew Cooper; +Cc: Edwin Torok, Xen-devel, Christian Lindig, Anthony PERARD + 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-05-18 14:34 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-17 19:41 [PATCH 0/2] Fix some problems with "arm/dom0less: assign dom0less guests to cpupools" Andrew Cooper 2022-05-17 19:41 ` [PATCH 1/2] xen/cpupool: Reject attempts to add a domain to CPUPOOLID_NONE Andrew Cooper 2022-05-18 10:24 ` Juergen Gross 2022-05-18 10:27 ` Luca Fancellu 2022-05-18 11:14 ` Andrew Cooper 2022-05-17 19:41 ` [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id Andrew Cooper 2022-05-18 8:04 ` Christian Lindig 2022-05-18 9:51 ` Edwin Torok 2022-05-18 10:12 ` Andrew Cooper 2022-05-18 10:30 ` Luca Fancellu 2022-05-18 11:34 ` Andrew Cooper 2022-05-18 14:34 ` Luca Fancellu
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.