Re: [Qemu-devel] [PATCH 4/4] hw/i386: Introduce the microvm machine type

[Eduardo Habkost]

Hi,

This looks good, overall, I'm just confused by the versioning
system.  Comments below:


On Fri, Jun 28, 2019 at 01:53:49PM +0200, Sergio Lopez wrote:
> Microvm is a machine type inspired by both NEMU and Firecracker, and
> constructed after the machine model implemented by the latter.
> 
> It's main purpose is providing users a KVM-only machine type with fast
> boot times, minimal attack surface (measured as the number of IO ports
> and MMIO regions exposed to the Guest) and small footprint (specially
> when combined with the ongoing QEMU modularization effort).
> 
> Normally, other than the device support provided by KVM itself,
> microvm only supports virtio-mmio devices. Microvm also includes a
> legacy mode, which adds an ISA bus with a 16550A serial port, useful
> for being able to see the early boot kernel messages.
> 
> Signed-off-by: Sergio Lopez <address@hidden>
[...]
> +static const TypeInfo microvm_machine_info = {
> +    .name          = TYPE_MICROVM_MACHINE,
> +    .parent        = TYPE_MACHINE,
> +    .abstract      = true,
> +    .instance_size = sizeof(MicrovmMachineState),
> +    .instance_init = microvm_machine_instance_init,
> +    .class_size    = sizeof(MicrovmMachineClass),
> +    .class_init    = microvm_class_init,

[1]

> +    .interfaces = (InterfaceInfo[]) {
> +         { TYPE_NMI },
> +         { }
> +    },
> +};
> +
> +static void microvm_machine_init(void)
> +{
> +    type_register_static(&microvm_machine_info);
> +}
> +type_init(microvm_machine_init);
> +
> +static void microvm_1_0_instance_init(Object *obj)
> +{
> +}

You shouldn't need a instance_init function if it's empty, I
believe you can delete it.

> +
> +static void microvm_machine_class_init(MachineClass *mc)

Why do you need both microvm_machine_class_init() [1] and
microvm_class_init()?

> +{
> +    mc->init = microvm_machine_state_init;
> +
> +    mc->family = "microvm_i386";
> +    mc->desc = "Microvm (i386)";
> +    mc->units_per_default_bus = 1;
> +    mc->no_floppy = 1;
> +    machine_class_allow_dynamic_sysbus_dev(mc, "sysbus-debugcon");
> +    machine_class_allow_dynamic_sysbus_dev(mc, "sysbus-debugexit");
> +    mc->max_cpus = 288;

Where does this limit come from?

> +    mc->has_hotpluggable_cpus = false;
> +    mc->auto_enable_numa_with_memhp = false;
> +    mc->default_cpu_type = X86_CPU_TYPE_NAME ("host");
> +    mc->nvdimm_supported = false;
> +    mc->default_machine_opts = "accel=kvm";
> +
> +    /* Machine class handlers */
> +    mc->cpu_index_to_instance_props = cpu_index_to_props;
> +    mc->get_default_cpu_node_id = cpu_get_default_cpu_node_id;
> +    mc->possible_cpu_arch_ids = cpu_possible_cpu_arch_ids;;

I don't think these methods should be mandatory if you don't
support NUMA or CPU hotplug.  Do you really need them?

(If the core machine code makes them mandatory, it's probably not
intentional).


> +    mc->reset = microvm_machine_reset;
> +}
> +
> +static void microvm_1_0_machine_class_init(MachineClass *mc)
> +{
> +    microvm_machine_class_init(mc);
> +}
> +DEFINE_MICROVM_MACHINE_AS_LATEST(1, 0)


We only have multiple versions of some machine types (pc-*,
virt-*, pseries-*, s390-ccw-virtio-*) because of Guest ABI
compatibility (which you are not implementing here).  What's the
reason behind having multiple microvm machine versions?


[...]
> +#define TYPE_MICROVM_MACHINE   MACHINE_TYPE_NAME("microvm")

Using MACHINE_TYPE_NAME("microvm") might eventually cause
conflicts with the "microvm" alias you are registering.  I
suggest using something like "microvm-machine-base".

A separate base class will only be necessary if you are really
planning to provide multiple versions of the machine type,
though.


> +#define MICROVM_MACHINE(obj) \
> +    OBJECT_CHECK(MicrovmMachineState, (obj), TYPE_MICROVM_MACHINE)
> +#define MICROVM_MACHINE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(MicrovmMachineClass, obj, TYPE_MICROVM_MACHINE)
> +#define MICROVM_MACHINE_CLASS(class) \
> +    OBJECT_CLASS_CHECK(MicrovmMachineClass, class, TYPE_MICROVM_MACHINE)
> +
> +#endif
> -- 
> 2.21.0
> 

-- 
Eduardo