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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Feb 16, 2024 at 02:29:50 -0800, Andrea Bolognani wrote:
> On Fri, Feb 16, 2024 at 09:23:12AM +0100, Peter Krempa wrote:
> > I'll investigate a bit more in terms of implications to machines. One
> > thing I've observed is that e.g. with q35, which doesn't require you
> > breaking the machine type definitions you can get a qemu without the
> > UHCI subsystem by setting just:
> >
> >  CONFIG_ISAPC=n
> >  CONFIG_I440FX=n
> >  CONFIG_USB_UHCI=n
> >
> > Which then fails with similar error as above:
> >
> > $ ./build/qemu-system-x86_64  -M q35 -usb
> > qemu-system-x86_64: could not find a module for type 'ich9-usb-uhci1'
> 
> Interestingly, when I apply that configuration I get a slighly
> different behavior:
> 
>   $ ./build/qemu-system-x86_64 -M q35 -usb
>   qemu-system-x86_64: unknown type 'ich9-usb-uhci1'
>   Aborted (core dumped)
> 
> You've made those changes to configs/devices/i386-softmmu/default.mak,
> right? I wonder why our builds don't act the same.

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.
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux