Re: [PATCH v2 11/17] qemu: Validate USB controllers earlier

On Fri, Feb 16, 2024 at 02:29:50 -0800, Andrea Bolognani wrote: You don't have 'CONFIG_MODULES' enabled and I do, see 'qdev_new'. I went through qemu to see were the '-usb' is actually used. '-usb' is converted to --machine TYPE,usb=on which is then accessible either via machine_usb() function or via MachineState type's 'usb' property. machine_usb() is used from: hw/arm/realview.c=static void realview_init(MachineState *machine, hw/arm/realview.c: if (machine_usb(machine)) { hw/arm/versatilepb.c=static void versatile_init(MachineState *machine, int board_id) hw/arm/versatilepb.c: if (machine_usb(machine)) { hw/i386/acpi-microvm.c=static void acpi_dsdt_add_xhci(Aml *scope, MicrovmMachineState *mms) hw/i386/acpi-microvm.c: if (machine_usb(MACHINE(mms))) { hw/i386/microvm.c=static void microvm_devices_init(MicrovmMachineState *mms) hw/i386/microvm.c: if (x86_machine_is_acpi_enabled(x86ms) && machine_usb(MACHINE(mms))) { hw/i386/pc_piix.c=static void pc_init1(MachineState *machine, hw/i386/pc_piix.c: machine_usb(machine), &error_abort); hw/i386/pc_q35.c=static void pc_q35_init(MachineState *machine) hw/i386/pc_q35.c: if (machine_usb(machine)) { hw/ppc/mac_oldworld.c=static void ppc_heathrow_init(MachineState *machine) hw/ppc/mac_oldworld.c: if (machine_usb(machine)) { The value is also directly read from the following places (I've renamed it to be able to grep for it). hw/ppc/mac_newworld.c: machine->xxusb |= defaults_enabled() && !machine->usb_disabled; hw/ppc/mac_newworld.c: if (machine->xxusb) { hw/ppc/spapr.c: machine->xxusb |= defaults_enabled() && !machine->usb_disabled; hw/ppc/spapr.c: if (machine->xxusb) { So if I didn't miss anything these are the only machines that we need to care about with -usb. For normal x86 the things are relatibvely simple: - isapc doesn't support USB - I440FX can't be compiled in without the proper default USB controller that would be picked by libvirt - for q35 libvirt explicitly doesn't ever use -usb - microvm machine is weird though as it seems to use TYPE_XHCI_SYSBUS USB controller which is MMIO?!? Then there are machines which create the default controller via: pci_create_simple(pci_bus, -1, "pci-ohci"); This is the case for: hw/arm/realview.c hw/arm/versatilepb.c hw/ppc/mac_oldworld.c hw/ppc/mac_newworld.c and hw/ppc/spapr.c: uses pci_create_simple(phb->bus, -1, "pci-ohci"); or pci_create_simple(phb->bus, -1, "nec-usb-xhci"); both of which our code should be able to detect and use properly at least after some tweaking. Currently we don't seem to ever consider 'pci-ohci' as the last resort controller type for any of the above machines. For arm both 'config REALVIEW' and 'config VERSATILE' require CONFIG_USB_OHCI. For PPC 'config MAC_NEWWORLD' implies USB_OHCI_PCI, but 'MAC_OLDWORLD does not. Either way we could add 'pci-ohci' as the very last resort at least for non-x86 machines. Thus the only open question is about the 'microvm' weird controller. I've defined a VM as: <domain type='kvm'> <name>microvm</name> <uuid>e739ac15-61b5-48c2-acc8-e7fb2b79774f</uuid> <memory unit='KiB'>1024000</memory> <currentMemory unit='KiB'>1024000</currentMemory> <vcpu placement='static'>1</vcpu> <os> <type arch='x86_64' machine='microvm'>hvm</type> <boot dev='hd'/> </os> <cpu mode='custom' match='exact' check='none'> <model fallback='forbid'>qemu64</model> </cpu> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> <on_reboot>restart</on_reboot> <on_crash>destroy</on_crash> <devices> <emulator>/home/pipo/git/qemu.git/build/x86_64-softmmu/qemu-system-x86_64</emulator> <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/alpine.qcow2'/> <target dev='vda' bus='virtio'/> <address type='virtio-mmio'/> </disk> <controller type='usb' index='0'> <address type='virtio-mmio'/> </controller> <input type='keyboard' bus='ps2'/> <input type='mouse' bus='ps2'/> <audio id='1' type='none'/> <memballoon model='none'/> </devices> </domain> Which results into: /home/pipo/git/qemu.git/build/x86_64-softmmu/qemu-system-x86_64 \ -name guest=microvm,debug-threads=on \ -S \ -object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain-17-microvm/master-key.aes"}' \ -machine microvm,usb=off,dump-guest-core=off,memory-backend=microvm.ram,acpi=off \ -accel kvm \ -cpu qemu64 \ -m size=1024000k \ -object '{"qom-type":"memory-backend-ram","id":"microvm.ram","size":1048576000}' \ -overcommit mem-lock=off \ -smp 1,sockets=1,cores=1,threads=1 \ -uuid e739ac15-61b5-48c2-acc8-e7fb2b79774f \ -display none \ -no-user-config \ -nodefaults \ -chardev socket,id=charmonitor,fd=40,server=on,wait=off \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ -no-shutdown \ -boot strict=on \ -usb \ -blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/alpine.qcow2","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2","file":"libvirt-1-storage","backing":null}' \ -device '{"driver":"virtio-blk-device","drive":"libvirt-1-format","id":"virtio-disk0","bootindex":1}' \ -audiodev '{"id":"audio1","driver":"none"}' \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ -msg timestamp=on Now as you've noted, the USB controller is not useful at all as no devices can't be added to the implicit controller via libvirt due to the bus name mismatch, but a definition like this is possible and starts (for a microvm machine to do something you also need to use direct kernel boot as the firmware alegedly doesn't support boot from block devices). The question now is whether we care about microvm. Specifically dropping '-usb' will most likely cause migration incompatibility. Or perhaps the question might be whether we care about migration compatibility when '-usb' as last resort was used as we didnt' control the USB controller in the first place. I've also added some tests for the versatilepb machine to see what we generate, those might make sense to be merged before this. I think that the path forward for everything except 'microvm' is to add 'pci-ohci' as last resort USB controller. If we decide that we do care about 'microvm's USB controller then we'll need to keep adding it via the machine property (so that we can at least drop all the legacy '-usb' stuff). At that point we should also fix the implicit bus name.