Eduardo Habkost <ehabk...@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 <s...@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(µvm_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
>>
signature.asc
Description: PGP signature
