On Sat, Aug 17, 2024 at 2:45 AM Jason Andryuk <jason.andr...@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 < > sstabell...@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.igles...@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.igles...@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
