RE: [Question] x86/microvm: why has_hotpluggable_cpus = false but hot(ub)plug APIs exist?

[Salil Mehta]

> From: Igor Mammedov <imammedo@redhat.com>
> Sent: Thursday, October 26, 2023 1:09 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> 
> On Wed, 25 Oct 2023 09:54:07 +0000
> Salil Mehta <salil.mehta@huawei.com> wrote:
> 
> > > From: David Hildenbrand <david@redhat.com>
> > > Sent: Wednesday, October 25, 2023 10:32 AM
> > > To: Salil Mehta <salil.mehta@huawei.com>; Igor Mammedov
> > > <imammedo@redhat.com>; Salil Mehta <salil.mehta@opnsrc.net>
> > >
> > > On 25.10.23 11:16, Salil Mehta wrote:
> > > > Hi Igor,
> > > >
> > > >> From: Igor Mammedov <imammedo@redhat.com>
> > > >> Sent: Tuesday, October 24, 2023 9:06 AM
> > > >> To: Salil Mehta <salil.mehta@opnsrc.net>
> > > >>
> > > >> On Wed, 18 Oct 2023 17:48:36 +0100
> > > >> Salil Mehta <salil.mehta@opnsrc.net> wrote:
> > > >>
> > > >>> Hi Alex,
> > > >>>
> > > >>> On 18/10/2023 16:41, Alex Bennée wrote:
> > > >>>>
> > > >>>> Salil Mehta <salil.mehta@opnsrc.net> writes:
> > > >>>>
> > > >>>>> Hello,
> > > >>>>>
> > > >>>>> Came across below code excerpt in x86/microvm code and wanted to 
> > > >>>>> know
> > > >>>>> why 'has_hotpluggable_cpus' flag has been set to 'false' while 
> > > >>>>> various
> > > >>>>> hot(un)plug APIs have been defined?
> > > >>>>>
> > > >>>>> static void microvm_class_init(ObjectClass *oc, void *data)
> > > >>>>> {
> > > >>>>>       X86MachineClass *x86mc = X86_MACHINE_CLASS(oc);
> > > >>>>>       MachineClass *mc = MACHINE_CLASS(oc);
> > > >>>>>       HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> > > >>>>>
> > > >>>>>       mc->init = microvm_machine_state_init;
> > > >>>>>
> > > >>>>>       mc->family = "microvm_i386";
> > > >>>>>       [...]
> > > >>>>>       mc->max_cpus = 288;
> > > >>>>>       mc->has_hotpluggable_cpus = false;  --------> This one
> > > >>>>>       [...]
> > > >>>>
> > > >>>>   From the original commit that added it:
> > > >>>>
> > > >>>>     It's a minimalist machine type without PCI nor ACPI support, 
> > > >>>> designed
> > > >>>>     for short-lived guests. microvm also establishes a baseline for
> > > >>>>     benchmarking and optimizing both QEMU and guest operating 
> > > >>>> systems,
> > > >>>>     since it is optimized for both boot time and footprint.
> > > >>>
> > > >>>
> > > >>> Agreed. It looks like ACPI is supported but neither CPU/Memory Hotplug
> > > >>> is supported for this minimalist machine type.
> > > >>>
> > > >>>
> > > >>> static void microvm_devices_init(MicrovmMachineState *mms)
> > > >>> {
> > > >>>       const char *default_firmware;
> > > >>>       X86MachineState *x86ms = X86_MACHINE(mms);
> > > >>>
> > > >>>      [...]
> > > >>>
> > > >>>       /* Optional and legacy devices */
> > > >>>       if (x86_machine_is_acpi_enabled(x86ms)) {
> > > >>>           DeviceState *dev = qdev_new(TYPE_ACPI_GED_X86);
> > > >>>           qdev_prop_set_uint32(dev, "ged-event", 
> > > >>> ACPI_GED_PWR_DOWN_EVT);
> > > >>>           sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, GED_MMIO_BASE);
> > > >>>        /* sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, 
> > > >>> GED_MMIO_BASE_MEMHP);
> > > */
> > > >>>
> > > >>>           [...]
> > > >>>
> > > >>>           sysbus_realize(SYS_BUS_DEVICE(dev), &error_fatal);
> > > >>>           x86ms->acpi_dev = HOTPLUG_HANDLER(dev);
> > > >>>       }
> > > >>>      [...]
> > > >>> }
> > > >>>
> > > >>>>
> > > >>>> Generally hotplug requires a dance between the VMM and the firmware 
> > > >>>> to
> > > >>>> properly shutdown and restart hotplug devices. The principle
> > > >>>> communication mechanism for this is ACPI.
> > > >>
> > > >> firmware part in cpu/mem hoptlug usually provided by QEMU by the way of
> > > >> ACPI tables (which may contain AML code that handles dance with QEMU
> > > >> while exposing standard interface to guest OS to handle hotplug)
> > > >>
> > > >>>
> > > >>> Agreed.
> > > >>>
> > > >>>>>       /* hotplug (for cpu coldplug) */
> > > >>>>>       mc->get_hotplug_handler = microvm_get_hotplug_handler;
> > > >>>>>       hc->pre_plug = microvm_device_pre_plug_cb;
> > > >>>>>       hc->plug = microvm_device_plug_cb;
> > > >>>>>       hc->unplug_request = microvm_device_unplug_request_cb;
> > > >>>>>       hc->unplug = microvm_device_unplug_cb;
> > > >>>
> > > >>> sorry, I also missed the definitions of the last 2 functions which 
> > > >>> says
> > > >>> that unplug is not supported so perhaps these functions are only
> > > >>> required to support cold plugging which corroborates with the comment 
> > > >>> as
> > > >>> well.
> > > >>
> > > >> this function are usually used for both cold and hotplug of bus-less 
> > > >> devices.
> > > >> They provide an opt-in way for board to wire up such devices (incl. 
> > > >> CPU).
> > > >
> > > >
> > > > Sure. I understand but microvm does not support hotplug so presence of
> > > > unplug{_request} versions brought a doubt in my mind but I realized 
> > > > later
> > > > that their definitions were empty. Cold-plug does not require unplug
> > > > versions.
> > > >
> > > > Was there any plan to support hot-plug with microvm in future?
> > >
> > > At least virtio-mem for memory hotplug should be fairly straight- forward
> > > to wire-up I guess. The relation to ACPI are minimal: we currently only
> > > use ACPI SRAT to expose the maximum GPA range that e.g., Linux requires
> > > early during boot to properly prepare for memory hotplug (size the
> > > virtual memory space for the kernel accordingly). One could use
> > > alternative (paravirtualized) interfaces for that.
> >
> > Ok. Light weight VM more in lines of Firecracker to improve boot-up times?
> >
> > Also, presence of unplug versions gives a wrong impression about code?
> 
> unplug handlers could be theoretically used to undo what (pre)plug did
> during VM deconstruction. (though it's not used this way today unless
> hotplug controller requested it).


I understand the intent but it would be better to remove these and bring
then back again if they are required in future. 

  "/* hotplug (for cpu coldplug) */"

Above comment and the code are not exactly consistent even though
definitions of the {request_}unplug are empty.


> What is confusing is interface naming (HotplugHandler) which is not
> hotplug limited anymore.

{pre_}plug routines are common to {hot,cold}-plugged CPUs, therefore, it
makes sense to have them here but I am not sure it is necessary to have
{request_}unplug routines as part of this implementation. Absolutely, it
makes sense to have them part of the design/framework for future
extensibility in case even un-initialization of VM decides to use these.


> > > The question is whether any form of hotplug is "in the spirit" of microvm.
> >
> > Understand that. BTW, was it ever made to be used with kata-containers?
> >
> > Thanks
> > Salil.