| Message ID | 20240812130606.90410-5-edgar.iglesias@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: pvh: Partial QOM:fication with new x86 PVH machine | expand |
On Mon, 12 Aug 2024, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> > > Add SMP support for Xen PVH ARM guests. Create max_cpus ioreq > servers to handle hotplug. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> > --- > hw/arm/xen_arm.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > index 5f75cc3779..ef8315969c 100644 > --- a/hw/arm/xen_arm.c > +++ b/hw/arm/xen_arm.c > @@ -173,7 +173,7 @@ static void xen_arm_init(MachineState *machine) > > xen_init_ram(machine); > > - xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener); > + xen_register_ioreq(xam->state, machine->smp.max_cpus, &xen_memory_listener); > > xen_create_virtio_mmio_devices(xam); > > @@ -218,7 +218,8 @@ static void xen_arm_machine_class_init(ObjectClass *oc, void *data) > MachineClass *mc = MACHINE_CLASS(oc); > mc->desc = "Xen PVH ARM machine"; > mc->init = xen_arm_init; > - mc->max_cpus = 1; > + /* MAX number of vcpus supported by Xen. */ > + mc->max_cpus = GUEST_MAX_VCPUS; Will this cause allocations of data structures with 128 elements? Looking at hw/xen/xen-hvm-common.c:xen_do_ioreq_register it seems possible? Or hw/xen/xen-hvm-common.c:xen_do_ioreq_register is called later on with the precise vCPU value which should be provided to QEMU via the -smp command line option (tools/libs/light/libxl_dm.c:libxl__build_device_model_args_new)?
On Mon, Aug 12, 2024 at 06:47:17PM -0700, Stefano Stabellini wrote: > On Mon, 12 Aug 2024, Edgar E. Iglesias wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> > > > > Add SMP support for Xen PVH ARM guests. Create max_cpus ioreq > > servers to handle hotplug. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> > > --- > > hw/arm/xen_arm.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > > index 5f75cc3779..ef8315969c 100644 > > --- a/hw/arm/xen_arm.c > > +++ b/hw/arm/xen_arm.c > > @@ -173,7 +173,7 @@ static void xen_arm_init(MachineState *machine) > > > > xen_init_ram(machine); > > > > - xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener); > > + xen_register_ioreq(xam->state, machine->smp.max_cpus, &xen_memory_listener); > > > > xen_create_virtio_mmio_devices(xam); > > > > @@ -218,7 +218,8 @@ static void xen_arm_machine_class_init(ObjectClass *oc, void *data) > > MachineClass *mc = MACHINE_CLASS(oc); > > mc->desc = "Xen PVH ARM machine"; > > mc->init = xen_arm_init; > > - mc->max_cpus = 1; > > + /* MAX number of vcpus supported by Xen. */ > > + mc->max_cpus = GUEST_MAX_VCPUS; > > Will this cause allocations of data structures with 128 elements? > Looking at hw/xen/xen-hvm-common.c:xen_do_ioreq_register it seems > possible? Or hw/xen/xen-hvm-common.c:xen_do_ioreq_register is called Yes, in theory there's probably overhead with this but as you correctly noted below, a PVH aware xl will set the max_cpus option to a lower value. With a non-pvh aware xl, I was a little worried about the overhead but I couldn't see any visible slow-down on ARM neither in boot or in network performance (I didn't run very sophisticated benchmarks). > later on with the precise vCPU value which should be provided to QEMU > via the -smp command line option > (tools/libs/light/libxl_dm.c:libxl__build_device_model_args_new)? Yes, a pvh aware xl will for example pass -smp 2,maxcpus=4 based on values from the xl.cfg. If the user doesn't set maxvcpus in xl.cfg, xl will set maxvcpus to the same value as vcpus. Best regards, Edgar
On 13/08/2024 6:02 pm, Edgar E. Iglesias wrote: > On Mon, Aug 12, 2024 at 06:47:17PM -0700, Stefano Stabellini wrote: >> On Mon, 12 Aug 2024, Edgar E. Iglesias wrote: >>> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> >>> >>> Add SMP support for Xen PVH ARM guests. Create max_cpus ioreq >>> servers to handle hotplug. >>> >>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> >>> --- >>> hw/arm/xen_arm.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c >>> index 5f75cc3779..ef8315969c 100644 >>> --- a/hw/arm/xen_arm.c >>> +++ b/hw/arm/xen_arm.c >>> @@ -173,7 +173,7 @@ static void xen_arm_init(MachineState *machine) >>> >>> xen_init_ram(machine); >>> >>> - xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener); >>> + xen_register_ioreq(xam->state, machine->smp.max_cpus, &xen_memory_listener); >>> >>> xen_create_virtio_mmio_devices(xam); >>> >>> @@ -218,7 +218,8 @@ static void xen_arm_machine_class_init(ObjectClass *oc, void *data) >>> MachineClass *mc = MACHINE_CLASS(oc); >>> mc->desc = "Xen PVH ARM machine"; >>> mc->init = xen_arm_init; >>> - mc->max_cpus = 1; >>> + /* MAX number of vcpus supported by Xen. */ >>> + mc->max_cpus = GUEST_MAX_VCPUS; The only suitable number here is the one you get back from XEN_DMOP_nr_vcpus GUEST_MAX_VCPUS is and has always been bogus as a compile time constant in the Xen public headers (yes, ARM inherited it from x86, but it should have never been copied to start with). Please do not introduce any further uses of it. ~Andrew
On Tue, 13 Aug 2024, Edgar E. Iglesias wrote: > On Mon, Aug 12, 2024 at 06:47:17PM -0700, Stefano Stabellini wrote: > > On Mon, 12 Aug 2024, Edgar E. Iglesias wrote: > > > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> > > > > > > Add SMP support for Xen PVH ARM guests. Create max_cpus ioreq > > > servers to handle hotplug. > > > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> > > > --- > > > hw/arm/xen_arm.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > > > index 5f75cc3779..ef8315969c 100644 > > > --- a/hw/arm/xen_arm.c > > > +++ b/hw/arm/xen_arm.c > > > @@ -173,7 +173,7 @@ static void xen_arm_init(MachineState *machine) > > > > > > xen_init_ram(machine); > > > > > > - xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener); > > > + xen_register_ioreq(xam->state, machine->smp.max_cpus, &xen_memory_listener); > > > > > > xen_create_virtio_mmio_devices(xam); > > > > > > @@ -218,7 +218,8 @@ static void xen_arm_machine_class_init(ObjectClass *oc, void *data) > > > MachineClass *mc = MACHINE_CLASS(oc); > > > mc->desc = "Xen PVH ARM machine"; > > > mc->init = xen_arm_init; > > > - mc->max_cpus = 1; > > > + /* MAX number of vcpus supported by Xen. */ > > > + mc->max_cpus = GUEST_MAX_VCPUS; > > > > Will this cause allocations of data structures with 128 elements? > > Looking at hw/xen/xen-hvm-common.c:xen_do_ioreq_register it seems > > possible? Or hw/xen/xen-hvm-common.c:xen_do_ioreq_register is called > > Yes, in theory there's probably overhead with this but as you correctly > noted below, a PVH aware xl will set the max_cpus option to a lower value. > > With a non-pvh aware xl, I was a little worried about the overhead > but I couldn't see any visible slow-down on ARM neither in boot or in network > performance (I didn't run very sophisticated benchmarks). What do you mean by "non-pvh aware xl"? All useful versions of xl support pvh? > > later on with the precise vCPU value which should be provided to QEMU > > via the -smp command line option > > (tools/libs/light/libxl_dm.c:libxl__build_device_model_args_new)? > > Yes, a pvh aware xl will for example pass -smp 2,maxcpus=4 based on > values from the xl.cfg. If the user doesn't set maxvcpus in xl.cfg, xl > will set maxvcpus to the same value as vcpus. OK good. In that case if this is just an initial value meant to be overwritten, I think it is best to keep it as 1.
On Tue, Aug 13, 2024 at 03:52:32PM -0700, Stefano Stabellini wrote: > On Tue, 13 Aug 2024, Edgar E. Iglesias wrote: > > On Mon, Aug 12, 2024 at 06:47:17PM -0700, Stefano Stabellini wrote: > > > On Mon, 12 Aug 2024, Edgar E. Iglesias wrote: > > > > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> > > > > > > > > Add SMP support for Xen PVH ARM guests. Create max_cpus ioreq > > > > servers to handle hotplug. > > > > > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> > > > > --- > > > > hw/arm/xen_arm.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > > > > index 5f75cc3779..ef8315969c 100644 > > > > --- a/hw/arm/xen_arm.c > > > > +++ b/hw/arm/xen_arm.c > > > > @@ -173,7 +173,7 @@ static void xen_arm_init(MachineState *machine) > > > > > > > > xen_init_ram(machine); > > > > > > > > - xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener); > > > > + xen_register_ioreq(xam->state, machine->smp.max_cpus, &xen_memory_listener); > > > > > > > > xen_create_virtio_mmio_devices(xam); > > > > > > > > @@ -218,7 +218,8 @@ static void xen_arm_machine_class_init(ObjectClass *oc, void *data) > > > > MachineClass *mc = MACHINE_CLASS(oc); > > > > mc->desc = "Xen PVH ARM machine"; > > > > mc->init = xen_arm_init; > > > > - mc->max_cpus = 1; > > > > + /* MAX number of vcpus supported by Xen. */ > > > > + mc->max_cpus = GUEST_MAX_VCPUS; > > > > > > Will this cause allocations of data structures with 128 elements? > > > Looking at hw/xen/xen-hvm-common.c:xen_do_ioreq_register it seems > > > possible? Or hw/xen/xen-hvm-common.c:xen_do_ioreq_register is called > > > > Yes, in theory there's probably overhead with this but as you correctly > > noted below, a PVH aware xl will set the max_cpus option to a lower value. > > > > With a non-pvh aware xl, I was a little worried about the overhead > > but I couldn't see any visible slow-down on ARM neither in boot or in network > > performance (I didn't run very sophisticated benchmarks). > > What do you mean by "non-pvh aware xl"? All useful versions of xl > support pvh? I mean an xl without our PVH patches merged. xl in upstream doesn't know much about PVH yet. Even for ARM, we're still carrying significant patches in our tree. > > > later on with the precise vCPU value which should be provided to QEMU > > > via the -smp command line option > > > (tools/libs/light/libxl_dm.c:libxl__build_device_model_args_new)? > > > > Yes, a pvh aware xl will for example pass -smp 2,maxcpus=4 based on > > values from the xl.cfg. If the user doesn't set maxvcpus in xl.cfg, xl > > will set maxvcpus to the same value as vcpus. > > OK good. In that case if this is just an initial value meant to be > overwritten, I think it is best to keep it as 1. Sorry but that won't work. I think the confusion here may be that it's easy to mix up mc->max_cpus and machine->smp.max_cpus, these are not the same. They have different purposes. I'll try to clarify the 3 values in play. machine-smp.cpus: Number of guest vcpus active at boot. Passed to QEMU via the -smp command-line option. We don't use this value in QEMU's ARM PVH machines. machine->smp.max_cpus: Max number of vcpus that the guest can use (equal or larger than machine-smp.cpus). Will be set by xl via the "-smp X,maxcpus=Y" command-line option to QEMU. Taken from maxvcpus from xl.cfg, same as XEN_DMOP_nr_vcpus. This is what we use for xen_register_ioreq(). mc->max_cpus: Absolute MAX in QEMU used to cap the -smp command-line options. If xl tries to set -smp (machine->smp.max_cpus) larger than this, QEMU will bail out. Used to setup xen_register_ioreq() ONLY if -smp maxcpus was NOT set (i.e by a non PVH aware xl). Cannot be 1 because that would limit QEMU to MAX 1 vcpu. I guess we could set mc->max_cpus to what XEN_DMOP_nr_vcpus returns but I'll have to check if we can even issue that hypercall this early in QEMU since mc->max_cpus is setup before we even parse the machine options. We may not yet know what domid we're attaching to yet. Cheers, Edgar
On Wed, 14 Aug 2024, Edgar E. Iglesias wrote: > On Tue, Aug 13, 2024 at 03:52:32PM -0700, Stefano Stabellini wrote: > > On Tue, 13 Aug 2024, Edgar E. Iglesias wrote: > > > On Mon, Aug 12, 2024 at 06:47:17PM -0700, Stefano Stabellini wrote: > > > > On Mon, 12 Aug 2024, Edgar E. Iglesias wrote: > > > > > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> > > > > > > > > > > Add SMP support for Xen PVH ARM guests. Create max_cpus ioreq > > > > > servers to handle hotplug. > > > > > > > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> > > > > > --- > > > > > hw/arm/xen_arm.c | 5 +++-- > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > > > > > index 5f75cc3779..ef8315969c 100644 > > > > > --- a/hw/arm/xen_arm.c > > > > > +++ b/hw/arm/xen_arm.c > > > > > @@ -173,7 +173,7 @@ static void xen_arm_init(MachineState *machine) > > > > > > > > > > xen_init_ram(machine); > > > > > > > > > > - xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener); > > > > > + xen_register_ioreq(xam->state, machine->smp.max_cpus, &xen_memory_listener); > > > > > > > > > > xen_create_virtio_mmio_devices(xam); > > > > > > > > > > @@ -218,7 +218,8 @@ static void xen_arm_machine_class_init(ObjectClass *oc, void *data) > > > > > MachineClass *mc = MACHINE_CLASS(oc); > > > > > mc->desc = "Xen PVH ARM machine"; > > > > > mc->init = xen_arm_init; > > > > > - mc->max_cpus = 1; > > > > > + /* MAX number of vcpus supported by Xen. */ > > > > > + mc->max_cpus = GUEST_MAX_VCPUS; > > > > > > > > Will this cause allocations of data structures with 128 elements? > > > > Looking at hw/xen/xen-hvm-common.c:xen_do_ioreq_register it seems > > > > possible? Or hw/xen/xen-hvm-common.c:xen_do_ioreq_register is called > > > > > > Yes, in theory there's probably overhead with this but as you correctly > > > noted below, a PVH aware xl will set the max_cpus option to a lower value. > > > > > > With a non-pvh aware xl, I was a little worried about the overhead > > > but I couldn't see any visible slow-down on ARM neither in boot or in network > > > performance (I didn't run very sophisticated benchmarks). > > > > What do you mean by "non-pvh aware xl"? All useful versions of xl > > support pvh? > > > I mean an xl without our PVH patches merged. > xl in upstream doesn't know much about PVH yet. > Even for ARM, we're still carrying significant patches in our tree. Oh I see. In that case, I don't think we need to support "non-pvh aware xl". > > > > later on with the precise vCPU value which should be provided to QEMU > > > > via the -smp command line option > > > > (tools/libs/light/libxl_dm.c:libxl__build_device_model_args_new)? > > > > > > Yes, a pvh aware xl will for example pass -smp 2,maxcpus=4 based on > > > values from the xl.cfg. If the user doesn't set maxvcpus in xl.cfg, xl > > > will set maxvcpus to the same value as vcpus. > > > > OK good. In that case if this is just an initial value meant to be > > overwritten, I think it is best to keep it as 1. > > Sorry but that won't work. I think the confusion here may be that > it's easy to mix up mc->max_cpus and machine->smp.max_cpus, these are > not the same. They have different purposes. > > I'll try to clarify the 3 values in play. > > machine-smp.cpus: > Number of guest vcpus active at boot. > Passed to QEMU via the -smp command-line option. > We don't use this value in QEMU's ARM PVH machines. > > machine->smp.max_cpus: > Max number of vcpus that the guest can use (equal or larger than machine-smp.cpus). > Will be set by xl via the "-smp X,maxcpus=Y" command-line option to QEMU. > Taken from maxvcpus from xl.cfg, same as XEN_DMOP_nr_vcpus. > This is what we use for xen_register_ioreq(). > > mc->max_cpus: > Absolute MAX in QEMU used to cap the -smp command-line options. > If xl tries to set -smp (machine->smp.max_cpus) larger than this, QEMU will bail out. > Used to setup xen_register_ioreq() ONLY if -smp maxcpus was NOT set (i.e by a non PVH aware xl). > Cannot be 1 because that would limit QEMU to MAX 1 vcpu. > > I guess we could set mc->max_cpus to what XEN_DMOP_nr_vcpus returns but I'll > have to check if we can even issue that hypercall this early in QEMU since > mc->max_cpus is setup before we even parse the machine options. We may > not yet know what domid we're attaching to yet. If mc->max_cpus is the absolute max and it will not be used if -smp is passed to QEMU, then I think it is OK to use GUEST_MAX_VCPUS
On Thu, Aug 15, 2024 at 2:30 AM Stefano Stabellini <sstabellini@kernel.org> wrote: > On Wed, 14 Aug 2024, Edgar E. Iglesias wrote: > > On Tue, Aug 13, 2024 at 03:52:32PM -0700, Stefano Stabellini wrote: > > > On Tue, 13 Aug 2024, Edgar E. Iglesias wrote: > > > > On Mon, Aug 12, 2024 at 06:47:17PM -0700, Stefano Stabellini wrote: > > > > > On Mon, 12 Aug 2024, Edgar E. Iglesias wrote: > > > > > > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> > > > > > > > > > > > > Add SMP support for Xen PVH ARM guests. Create max_cpus ioreq > > > > > > servers to handle hotplug. > > > > > > > > > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> > > > > > > --- > > > > > > hw/arm/xen_arm.c | 5 +++-- > > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > > > > > > index 5f75cc3779..ef8315969c 100644 > > > > > > --- a/hw/arm/xen_arm.c > > > > > > +++ b/hw/arm/xen_arm.c > > > > > > @@ -173,7 +173,7 @@ static void xen_arm_init(MachineState > *machine) > > > > > > > > > > > > xen_init_ram(machine); > > > > > > > > > > > > - xen_register_ioreq(xam->state, machine->smp.cpus, > &xen_memory_listener); > > > > > > + xen_register_ioreq(xam->state, machine->smp.max_cpus, > &xen_memory_listener); > > > > > > > > > > > > xen_create_virtio_mmio_devices(xam); > > > > > > > > > > > > @@ -218,7 +218,8 @@ static void > xen_arm_machine_class_init(ObjectClass *oc, void *data) > > > > > > MachineClass *mc = MACHINE_CLASS(oc); > > > > > > mc->desc = "Xen PVH ARM machine"; > > > > > > mc->init = xen_arm_init; > > > > > > - mc->max_cpus = 1; > > > > > > + /* MAX number of vcpus supported by Xen. */ > > > > > > + mc->max_cpus = GUEST_MAX_VCPUS; > > > > > > > > > > Will this cause allocations of data structures with 128 elements? > > > > > Looking at hw/xen/xen-hvm-common.c:xen_do_ioreq_register it seems > > > > > possible? Or hw/xen/xen-hvm-common.c:xen_do_ioreq_register is > called > > > > > > > > Yes, in theory there's probably overhead with this but as you > correctly > > > > noted below, a PVH aware xl will set the max_cpus option to a lower > value. > > > > > > > > With a non-pvh aware xl, I was a little worried about the overhead > > > > but I couldn't see any visible slow-down on ARM neither in boot or > in network > > > > performance (I didn't run very sophisticated benchmarks). > > > > > > What do you mean by "non-pvh aware xl"? All useful versions of xl > > > support pvh? > > > > > > I mean an xl without our PVH patches merged. > > xl in upstream doesn't know much about PVH yet. > > Even for ARM, we're still carrying significant patches in our tree. > > Oh I see. In that case, I don't think we need to support "non-pvh aware > xl". > > > > > > > later on with the precise vCPU value which should be provided to > QEMU > > > > > via the -smp command line option > > > > > (tools/libs/light/libxl_dm.c:libxl__build_device_model_args_new)? > > > > > > > > Yes, a pvh aware xl will for example pass -smp 2,maxcpus=4 based on > > > > values from the xl.cfg. If the user doesn't set maxvcpus in xl.cfg, > xl > > > > will set maxvcpus to the same value as vcpus. > > > > > > OK good. In that case if this is just an initial value meant to be > > > overwritten, I think it is best to keep it as 1. > > > > Sorry but that won't work. I think the confusion here may be that > > it's easy to mix up mc->max_cpus and machine->smp.max_cpus, these are > > not the same. They have different purposes. > > > > I'll try to clarify the 3 values in play. > > > > machine-smp.cpus: > > Number of guest vcpus active at boot. > > Passed to QEMU via the -smp command-line option. > > We don't use this value in QEMU's ARM PVH machines. > > > > machine->smp.max_cpus: > > Max number of vcpus that the guest can use (equal or larger than > machine-smp.cpus). > > Will be set by xl via the "-smp X,maxcpus=Y" command-line option to QEMU. > > Taken from maxvcpus from xl.cfg, same as XEN_DMOP_nr_vcpus. > > This is what we use for xen_register_ioreq(). > > > > mc->max_cpus: > > Absolute MAX in QEMU used to cap the -smp command-line options. > > If xl tries to set -smp (machine->smp.max_cpus) larger than this, QEMU > will bail out. > > Used to setup xen_register_ioreq() ONLY if -smp maxcpus was NOT set (i.e > by a non PVH aware xl). > > Cannot be 1 because that would limit QEMU to MAX 1 vcpu. > > > > I guess we could set mc->max_cpus to what XEN_DMOP_nr_vcpus returns but > I'll > > have to check if we can even issue that hypercall this early in QEMU > since > > mc->max_cpus is setup before we even parse the machine options. We may > > not yet know what domid we're attaching to yet. > > If mc->max_cpus is the absolute max and it will not be used if -smp is > passed to QEMU, then I think it is OK to use GUEST_MAX_VCPUS > Looking at this a little more. If users (xl) don't pass an -smp option we actually default to smp.max_cpus=1. So, another option is to simply remove the upper limit in QEMU (e.g we can set mc->max_cpus to something very large like UINT32_MAX). That would avoid early hypercalls, avoid using GUEST_MAX_VCPUS and always let xl dictate the max_cpus value using the -smp cmdline option.
On Fri, 16 Aug 2024, Edgar E. Iglesias wrote: > On Thu, Aug 15, 2024 at 2:30 AM Stefano Stabellini <sstabellini@kernel.org> wrote: > On Wed, 14 Aug 2024, Edgar E. Iglesias wrote: > > On Tue, Aug 13, 2024 at 03:52:32PM -0700, Stefano Stabellini wrote: > > > On Tue, 13 Aug 2024, Edgar E. Iglesias wrote: > > > > On Mon, Aug 12, 2024 at 06:47:17PM -0700, Stefano Stabellini wrote: > > > > > On Mon, 12 Aug 2024, Edgar E. Iglesias wrote: > > > > > > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> > > > > > > > > > > > > Add SMP support for Xen PVH ARM guests. Create max_cpus ioreq > > > > > > servers to handle hotplug. > > > > > > > > > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> > > > > > > --- > > > > > > hw/arm/xen_arm.c | 5 +++-- > > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > > > > > > index 5f75cc3779..ef8315969c 100644 > > > > > > --- a/hw/arm/xen_arm.c > > > > > > +++ b/hw/arm/xen_arm.c > > > > > > @@ -173,7 +173,7 @@ static void xen_arm_init(MachineState *machine) > > > > > > > > > > > > xen_init_ram(machine); > > > > > > > > > > > > - xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener); > > > > > > + xen_register_ioreq(xam->state, machine->smp.max_cpus, &xen_memory_listener); > > > > > > > > > > > > xen_create_virtio_mmio_devices(xam); > > > > > > > > > > > > @@ -218,7 +218,8 @@ static void xen_arm_machine_class_init(ObjectClass *oc, void *data) > > > > > > MachineClass *mc = MACHINE_CLASS(oc); > > > > > > mc->desc = "Xen PVH ARM machine"; > > > > > > mc->init = xen_arm_init; > > > > > > - mc->max_cpus = 1; > > > > > > + /* MAX number of vcpus supported by Xen. */ > > > > > > + mc->max_cpus = GUEST_MAX_VCPUS; > > > > > > > > > > Will this cause allocations of data structures with 128 elements? > > > > > Looking at hw/xen/xen-hvm-common.c:xen_do_ioreq_register it seems > > > > > possible? Or hw/xen/xen-hvm-common.c:xen_do_ioreq_register is called > > > > > > > > Yes, in theory there's probably overhead with this but as you correctly > > > > noted below, a PVH aware xl will set the max_cpus option to a lower value. > > > > > > > > With a non-pvh aware xl, I was a little worried about the overhead > > > > but I couldn't see any visible slow-down on ARM neither in boot or in network > > > > performance (I didn't run very sophisticated benchmarks). > > > > > > What do you mean by "non-pvh aware xl"? All useful versions of xl > > > support pvh? > > > > > > I mean an xl without our PVH patches merged. > > xl in upstream doesn't know much about PVH yet. > > Even for ARM, we're still carrying significant patches in our tree. > > Oh I see. In that case, I don't think we need to support "non-pvh aware xl". > > > > > > > later on with the precise vCPU value which should be provided to QEMU > > > > > via the -smp command line option > > > > > (tools/libs/light/libxl_dm.c:libxl__build_device_model_args_new)? > > > > > > > > Yes, a pvh aware xl will for example pass -smp 2,maxcpus=4 based on > > > > values from the xl.cfg. If the user doesn't set maxvcpus in xl.cfg, xl > > > > will set maxvcpus to the same value as vcpus. > > > > > > OK good. In that case if this is just an initial value meant to be > > > overwritten, I think it is best to keep it as 1. > > > > Sorry but that won't work. I think the confusion here may be that > > it's easy to mix up mc->max_cpus and machine->smp.max_cpus, these are > > not the same. They have different purposes. > > > > I'll try to clarify the 3 values in play. > > > > machine-smp.cpus: > > Number of guest vcpus active at boot. > > Passed to QEMU via the -smp command-line option. > > We don't use this value in QEMU's ARM PVH machines. > > > > machine->smp.max_cpus: > > Max number of vcpus that the guest can use (equal or larger than machine-smp.cpus). > > Will be set by xl via the "-smp X,maxcpus=Y" command-line option to QEMU. > > Taken from maxvcpus from xl.cfg, same as XEN_DMOP_nr_vcpus. > > This is what we use for xen_register_ioreq(). > > > > mc->max_cpus: > > Absolute MAX in QEMU used to cap the -smp command-line options. > > If xl tries to set -smp (machine->smp.max_cpus) larger than this, QEMU will bail out. > > Used to setup xen_register_ioreq() ONLY if -smp maxcpus was NOT set (i.e by a non PVH aware xl). > > Cannot be 1 because that would limit QEMU to MAX 1 vcpu. > > > > I guess we could set mc->max_cpus to what XEN_DMOP_nr_vcpus returns but I'll > > have to check if we can even issue that hypercall this early in QEMU since > > mc->max_cpus is setup before we even parse the machine options. We may > > not yet know what domid we're attaching to yet. > > If mc->max_cpus is the absolute max and it will not be used if -smp is > passed to QEMU, then I think it is OK to use GUEST_MAX_VCPUS > > Looking at this a little more. If users (xl) don't pass an -smp option we actually default to smp.max_cpus=1. > So, another option is to simply remove the upper limit in QEMU (e.g we can set mc->max_cpus to something very large like UINT32_MAX). > That would avoid early hypercalls, avoid using GUEST_MAX_VCPUS and always let xl dictate the max_cpus value using the -smp cmdline option. As the expectation is that there will be always a smp.max_cpus option passed to QEMU, I would avoid an extra early hypercall. For the initial value, I would use something static and large, but not unreasonably large as UINT32_MAX to be more resilient in (erroneous) cases where smp.max_cpus is not passed. So I would initialize it to GUEST_MAX_VCPUS, or if we don't want to use GUEST_MAX_VCPUS, something equivalent in the 64-256 range. Alternative we can have a runtime check and exit with a warning if smp.max_cpus is not set.
On 2024-08-16 12:53, Stefano Stabellini wrote: > On Fri, 16 Aug 2024, Edgar E. Iglesias wrote: >> On Thu, Aug 15, 2024 at 2:30 AM Stefano Stabellini <sstabellini@kernel.org> wrote: >> On Wed, 14 Aug 2024, Edgar E. Iglesias wrote: >> > On Tue, Aug 13, 2024 at 03:52:32PM -0700, Stefano Stabellini wrote: >> > > On Tue, 13 Aug 2024, Edgar E. Iglesias wrote: >> > > > On Mon, Aug 12, 2024 at 06:47:17PM -0700, Stefano Stabellini wrote: >> > > > > On Mon, 12 Aug 2024, Edgar E. Iglesias wrote: >> > > > > > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> >> > > > > > >> > > > > > Add SMP support for Xen PVH ARM guests. Create max_cpus ioreq >> > > > > > servers to handle hotplug. >> > > > > > >> > > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> >> > > > > > --- >> > > > > > hw/arm/xen_arm.c | 5 +++-- >> > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) >> > > > > > >> > > > > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c >> > > > > > index 5f75cc3779..ef8315969c 100644 >> > > > > > --- a/hw/arm/xen_arm.c >> > > > > > +++ b/hw/arm/xen_arm.c >> > > > > > @@ -173,7 +173,7 @@ static void xen_arm_init(MachineState *machine) >> > > > > > >> > > > > > xen_init_ram(machine); >> > > > > > >> > > > > > - xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener); >> > > > > > + xen_register_ioreq(xam->state, machine->smp.max_cpus, &xen_memory_listener); >> > > > > > >> > > > > > xen_create_virtio_mmio_devices(xam); >> > > > > > >> > > > > > @@ -218,7 +218,8 @@ static void xen_arm_machine_class_init(ObjectClass *oc, void *data) >> > > > > > MachineClass *mc = MACHINE_CLASS(oc); >> > > > > > mc->desc = "Xen PVH ARM machine"; >> > > > > > mc->init = xen_arm_init; >> > > > > > - mc->max_cpus = 1; >> > > > > > + /* MAX number of vcpus supported by Xen. */ >> > > > > > + mc->max_cpus = GUEST_MAX_VCPUS; >> > > > > >> > > > > Will this cause allocations of data structures with 128 elements? >> > > > > Looking at hw/xen/xen-hvm-common.c:xen_do_ioreq_register it seems >> > > > > possible? Or hw/xen/xen-hvm-common.c:xen_do_ioreq_register is called >> > > > >> > > > Yes, in theory there's probably overhead with this but as you correctly >> > > > noted below, a PVH aware xl will set the max_cpus option to a lower value. >> > > > >> > > > With a non-pvh aware xl, I was a little worried about the overhead >> > > > but I couldn't see any visible slow-down on ARM neither in boot or in network >> > > > performance (I didn't run very sophisticated benchmarks). >> > > >> > > What do you mean by "non-pvh aware xl"? All useful versions of xl >> > > support pvh? >> > >> > >> > I mean an xl without our PVH patches merged. >> > xl in upstream doesn't know much about PVH yet. >> > Even for ARM, we're still carrying significant patches in our tree. >> >> Oh I see. In that case, I don't think we need to support "non-pvh aware xl". >> >> >> > > > > later on with the precise vCPU value which should be provided to QEMU >> > > > > via the -smp command line option >> > > > > (tools/libs/light/libxl_dm.c:libxl__build_device_model_args_new)? >> > > > >> > > > Yes, a pvh aware xl will for example pass -smp 2,maxcpus=4 based on >> > > > values from the xl.cfg. If the user doesn't set maxvcpus in xl.cfg, xl >> > > > will set maxvcpus to the same value as vcpus. >> > > >> > > OK good. In that case if this is just an initial value meant to be >> > > overwritten, I think it is best to keep it as 1. >> > >> > Sorry but that won't work. I think the confusion here may be that >> > it's easy to mix up mc->max_cpus and machine->smp.max_cpus, these are >> > not the same. They have different purposes. >> > >> > I'll try to clarify the 3 values in play. >> > >> > machine-smp.cpus: >> > Number of guest vcpus active at boot. >> > Passed to QEMU via the -smp command-line option. >> > We don't use this value in QEMU's ARM PVH machines. >> > >> > machine->smp.max_cpus: >> > Max number of vcpus that the guest can use (equal or larger than machine-smp.cpus). >> > Will be set by xl via the "-smp X,maxcpus=Y" command-line option to QEMU. >> > Taken from maxvcpus from xl.cfg, same as XEN_DMOP_nr_vcpus. >> > This is what we use for xen_register_ioreq(). >> > >> > mc->max_cpus: >> > Absolute MAX in QEMU used to cap the -smp command-line options. >> > If xl tries to set -smp (machine->smp.max_cpus) larger than this, QEMU will bail out. >> > Used to setup xen_register_ioreq() ONLY if -smp maxcpus was NOT set (i.e by a non PVH aware xl). >> > Cannot be 1 because that would limit QEMU to MAX 1 vcpu. >> > >> > I guess we could set mc->max_cpus to what XEN_DMOP_nr_vcpus returns but I'll >> > have to check if we can even issue that hypercall this early in QEMU since >> > mc->max_cpus is setup before we even parse the machine options. We may >> > not yet know what domid we're attaching to yet. >> >> If mc->max_cpus is the absolute max and it will not be used if -smp is >> passed to QEMU, then I think it is OK to use GUEST_MAX_VCPUS >> >> Looking at this a little more. If users (xl) don't pass an -smp option we actually default to smp.max_cpus=1. >> So, another option is to simply remove the upper limit in QEMU (e.g we can set mc->max_cpus to something very large like UINT32_MAX). >> That would avoid early hypercalls, avoid using GUEST_MAX_VCPUS and always let xl dictate the max_cpus value using the -smp cmdline option. > > As the expectation is that there will be always a smp.max_cpus option > passed to QEMU, I would avoid an extra early hypercall. > > For the initial value, I would use something static and large, but not > unreasonably large as UINT32_MAX to be more resilient in (erroneous) > cases where smp.max_cpus is not passed. > > So I would initialize it to GUEST_MAX_VCPUS, or if we don't want to use > GUEST_MAX_VCPUS, something equivalent in the 64-256 range. > > Alternative we can have a runtime check and exit with a warning if > smp.max_cpus is not set. FYI, xl only passes a -smp option when the domU has more than 1 vcpu. Though that implies only a single vcpu. Regards, Jason
On Sat, Aug 17, 2024 at 2:45 AM Jason Andryuk <jason.andryuk@amd.com> wrote: > On 2024-08-16 12:53, Stefano Stabellini wrote: > > On Fri, 16 Aug 2024, Edgar E. Iglesias wrote: > >> On Thu, Aug 15, 2024 at 2:30 AM Stefano Stabellini < > sstabellini@kernel.org> wrote: > >> On Wed, 14 Aug 2024, Edgar E. Iglesias wrote: > >> > On Tue, Aug 13, 2024 at 03:52:32PM -0700, Stefano Stabellini > wrote: > >> > > On Tue, 13 Aug 2024, Edgar E. Iglesias wrote: > >> > > > On Mon, Aug 12, 2024 at 06:47:17PM -0700, Stefano > Stabellini wrote: > >> > > > > On Mon, 12 Aug 2024, Edgar E. Iglesias wrote: > >> > > > > > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> > >> > > > > > > >> > > > > > Add SMP support for Xen PVH ARM guests. Create > max_cpus ioreq > >> > > > > > servers to handle hotplug. > >> > > > > > > >> > > > > > Signed-off-by: Edgar E. Iglesias < > edgar.iglesias@amd.com> > >> > > > > > --- > >> > > > > > hw/arm/xen_arm.c | 5 +++-- > >> > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > >> > > > > > > >> > > > > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > >> > > > > > index 5f75cc3779..ef8315969c 100644 > >> > > > > > --- a/hw/arm/xen_arm.c > >> > > > > > +++ b/hw/arm/xen_arm.c > >> > > > > > @@ -173,7 +173,7 @@ static void > xen_arm_init(MachineState *machine) > >> > > > > > > >> > > > > > xen_init_ram(machine); > >> > > > > > > >> > > > > > - xen_register_ioreq(xam->state, machine->smp.cpus, > &xen_memory_listener); > >> > > > > > + xen_register_ioreq(xam->state, > machine->smp.max_cpus, &xen_memory_listener); > >> > > > > > > >> > > > > > xen_create_virtio_mmio_devices(xam); > >> > > > > > > >> > > > > > @@ -218,7 +218,8 @@ static void > xen_arm_machine_class_init(ObjectClass *oc, void *data) > >> > > > > > MachineClass *mc = MACHINE_CLASS(oc); > >> > > > > > mc->desc = "Xen PVH ARM machine"; > >> > > > > > mc->init = xen_arm_init; > >> > > > > > - mc->max_cpus = 1; > >> > > > > > + /* MAX number of vcpus supported by Xen. */ > >> > > > > > + mc->max_cpus = GUEST_MAX_VCPUS; > >> > > > > > >> > > > > Will this cause allocations of data structures with 128 > elements? > >> > > > > Looking at hw/xen/xen-hvm-common.c:xen_do_ioreq_register > it seems > >> > > > > possible? Or > hw/xen/xen-hvm-common.c:xen_do_ioreq_register is called > >> > > > > >> > > > Yes, in theory there's probably overhead with this but as > you correctly > >> > > > noted below, a PVH aware xl will set the max_cpus option > to a lower value. > >> > > > > >> > > > With a non-pvh aware xl, I was a little worried about the > overhead > >> > > > but I couldn't see any visible slow-down on ARM neither in > boot or in network > >> > > > performance (I didn't run very sophisticated benchmarks). > >> > > > >> > > What do you mean by "non-pvh aware xl"? All useful versions > of xl > >> > > support pvh? > >> > > >> > > >> > I mean an xl without our PVH patches merged. > >> > xl in upstream doesn't know much about PVH yet. > >> > Even for ARM, we're still carrying significant patches in our > tree. > >> > >> Oh I see. In that case, I don't think we need to support > "non-pvh aware xl". > >> > >> > >> > > > > later on with the precise vCPU value which should be > provided to QEMU > >> > > > > via the -smp command line option > >> > > > > > (tools/libs/light/libxl_dm.c:libxl__build_device_model_args_new)? > >> > > > > >> > > > Yes, a pvh aware xl will for example pass -smp 2,maxcpus=4 > based on > >> > > > values from the xl.cfg. If the user doesn't set maxvcpus > in xl.cfg, xl > >> > > > will set maxvcpus to the same value as vcpus. > >> > > > >> > > OK good. In that case if this is just an initial value meant > to be > >> > > overwritten, I think it is best to keep it as 1. > >> > > >> > Sorry but that won't work. I think the confusion here may be > that > >> > it's easy to mix up mc->max_cpus and machine->smp.max_cpus, > these are > >> > not the same. They have different purposes. > >> > > >> > I'll try to clarify the 3 values in play. > >> > > >> > machine-smp.cpus: > >> > Number of guest vcpus active at boot. > >> > Passed to QEMU via the -smp command-line option. > >> > We don't use this value in QEMU's ARM PVH machines. > >> > > >> > machine->smp.max_cpus: > >> > Max number of vcpus that the guest can use (equal or larger > than machine-smp.cpus). > >> > Will be set by xl via the "-smp X,maxcpus=Y" command-line > option to QEMU. > >> > Taken from maxvcpus from xl.cfg, same as XEN_DMOP_nr_vcpus. > >> > This is what we use for xen_register_ioreq(). > >> > > >> > mc->max_cpus: > >> > Absolute MAX in QEMU used to cap the -smp command-line options. > >> > If xl tries to set -smp (machine->smp.max_cpus) larger than > this, QEMU will bail out. > >> > Used to setup xen_register_ioreq() ONLY if -smp maxcpus was > NOT set (i.e by a non PVH aware xl). > >> > Cannot be 1 because that would limit QEMU to MAX 1 vcpu. > >> > > >> > I guess we could set mc->max_cpus to what XEN_DMOP_nr_vcpus > returns but I'll > >> > have to check if we can even issue that hypercall this early > in QEMU since > >> > mc->max_cpus is setup before we even parse the machine > options. We may > >> > not yet know what domid we're attaching to yet. > >> > >> If mc->max_cpus is the absolute max and it will not be used if > -smp is > >> passed to QEMU, then I think it is OK to use GUEST_MAX_VCPUS > >> > >> Looking at this a little more. If users (xl) don't pass an -smp option > we actually default to smp.max_cpus=1. > >> So, another option is to simply remove the upper limit in QEMU (e.g we > can set mc->max_cpus to something very large like UINT32_MAX). > >> That would avoid early hypercalls, avoid using GUEST_MAX_VCPUS and > always let xl dictate the max_cpus value using the -smp cmdline option. > > > > As the expectation is that there will be always a smp.max_cpus option > > passed to QEMU, I would avoid an extra early hypercall. > > > > For the initial value, I would use something static and large, but not > > unreasonably large as UINT32_MAX to be more resilient in (erroneous) > > cases where smp.max_cpus is not passed. > > > > So I would initialize it to GUEST_MAX_VCPUS, or if we don't want to use > > GUEST_MAX_VCPUS, something equivalent in the 64-256 range. > Thanks Stefano, I'm going to send a v2 following this suggestion of using GUEST_MAX_VCPUS. Will also add comments clarifying that this is a MAX value for the command-line option and not what gets passed to register_ioreq. We can continue the discussion from there to see if we want to change things, I don't have a strong opinion here so I'm happy to go either way. > > > > Alternative we can have a runtime check and exit with a warning if > > smp.max_cpus is not set. > > FYI, xl only passes a -smp option when the domU has more than 1 vcpu. > Though that implies only a single vcpu. > > Thanks Jason, yes, in that case the default of cpus=1, maxcpus=1 gets set. I was initially under the wrong assumption that without -smp options, the max would get set. This is what I was trying to clarify in my previous email: >> Looking at this a little more. If users (xl) don't pass an -smp option we actually default to smp.max_cpus=1. Best regards, Edgar
diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c index 5f75cc3779..ef8315969c 100644 --- a/hw/arm/xen_arm.c +++ b/hw/arm/xen_arm.c @@ -173,7 +173,7 @@ static void xen_arm_init(MachineState *machine) xen_init_ram(machine); - xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener); + xen_register_ioreq(xam->state, machine->smp.max_cpus, &xen_memory_listener); xen_create_virtio_mmio_devices(xam); @@ -218,7 +218,8 @@ static void xen_arm_machine_class_init(ObjectClass *oc, void *data) MachineClass *mc = MACHINE_CLASS(oc); mc->desc = "Xen PVH ARM machine"; mc->init = xen_arm_init; - mc->max_cpus = 1; + /* MAX number of vcpus supported by Xen. */ + mc->max_cpus = GUEST_MAX_VCPUS; mc->default_machine_opts = "accel=xen"; /* Set explicitly here to make sure that real ram_size is passed */ mc->default_ram_size = 0;