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. > > > 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
