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

[Sergio Lopez <slp@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4975 bytes --]


Eduardo Habkost <ehabkost@redhat.com> writes:

> 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 <slp@redhat.com>
> [...]
>> +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.

Ack.

>> +
>> +static void microvm_machine_class_init(MachineClass *mc)
>
> Why do you need both microvm_machine_class_init() [1] and
> microvm_class_init()?

No idea. To be honest, I took the boilerplate from NEMU's virt machine
type (hence the copyright notice), and I assumed that was actually
mandatory.

>> +{
>> +    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?

From pc_q35.c:366. Apparently, having this limit defined is mandatory,
and I wasn't which value would make sense for microvm.

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

Ack, I'll check whether this is actually needed or not.

>> +    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?

I though it could be a good idea to have versioning already in place, in
case we need it in the future. But, perhaps we can do a simple machine
definition and just add versioning when it's really needed?

> [...]
>> +#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.

Ack.

Thanks!
Sergio.

>> +#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
>> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]