[libvirt] [PATCH 3/3] qemu: Forbid user aliases for USB controllers

Michal Privoznik posted 3 patches 7 years, 2 months ago
[libvirt] [PATCH 3/3] qemu: Forbid user aliases for USB controllers
Posted by Michal Privoznik 7 years, 2 months ago
https://bugzilla.redhat.com/show_bug.cgi?id=1552127

These are bit different than other devices. Their alias also
specify the name of the bus. For instance, if there are these
'joined' USB devices:

  <controller type='usb' index='0' model='ich9-ehci1'>
    <alias name='ua-myUSB'/>
    <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x7'/>
  </controller>
  <controller type='usb' index='0' model='ich9-uhci1'>
    <master startport='0'/>
    <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' multifunction='on'/>
  </controller>

which translates to cmd line as:

  -device ich9-usb-ehci1,id=ua-myUSB,bus=pci.0,addr=0x4.0x7
  -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4

The problem is that UHCI is still trying to serve 'usb.0' bus.
Rather than trying to come up with some complicated algorithm to
make everything work, lets forbid user aliases for USB
controllers.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_domain.c                      | 20 +++++++++++++++++++-
 tests/qemuxml2argvdata/user-aliases-usb.xml | 19 +++++++++++++++++++
 tests/qemuxml2argvdata/user-aliases.xml     |  1 -
 tests/qemuxml2argvtest.c                    |  1 +
 4 files changed, 39 insertions(+), 2 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/user-aliases-usb.xml

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b55013de6..67c145493 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4565,6 +4565,21 @@ qemuDomainDeviceDefValidateControllerSATA(const virDomainControllerDef *controll
 }
 
 
+static int
+qemuDomainDeviceDefValidateControllerUSB(const virDomainControllerDef *controller)
+{
+    if (controller->info.alias &&
+        virDomainDeviceAliasIsUserAlias(controller->info.alias)) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("Providing user alias to USB "
+                         "controllers is not supported yet"));
+        return -1;
+    }
+
+    return 0;
+}
+
+
 static int
 qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller,
                                       const virDomainDef *def,
@@ -4602,10 +4617,13 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller,
                                                         qemuCaps);
         break;
 
+    case VIR_DOMAIN_CONTROLLER_TYPE_USB:
+        ret = qemuDomainDeviceDefValidateControllerUSB(controller);
+        break;
+
     case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
     case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
     case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
-    case VIR_DOMAIN_CONTROLLER_TYPE_USB:
     case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
         break;
     }
diff --git a/tests/qemuxml2argvdata/user-aliases-usb.xml b/tests/qemuxml2argvdata/user-aliases-usb.xml
new file mode 100644
index 000000000..6dade4572
--- /dev/null
+++ b/tests/qemuxml2argvdata/user-aliases-usb.xml
@@ -0,0 +1,19 @@
+<domain type='kvm'>
+  <name>gentoo</name>
+  <uuid>a75aca4b-a02f-2bcb-4a91-c93cd848c34b</uuid>
+  <memory unit='KiB'>4194304</memory>
+  <currentMemory unit='KiB'>4194304</currentMemory>
+  <vcpu placement='static'>4</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc-i440fx-1.4'>hvm</type>
+    <boot dev='hd'/>
+    <boot dev='cdrom'/>
+  </os>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' index='0'>
+      <alias name='ua-SomeWeirdController'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+    </controller>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/user-aliases.xml b/tests/qemuxml2argvdata/user-aliases.xml
index 52132a82d..6f1fed636 100644
--- a/tests/qemuxml2argvdata/user-aliases.xml
+++ b/tests/qemuxml2argvdata/user-aliases.xml
@@ -71,7 +71,6 @@
       <address type='drive' controller='0' bus='1' target='0' unit='0'/>
     </disk>
     <controller type='usb' index='0'>
-      <alias name='ua-SomeWeirdController'/>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
     </controller>
     <controller type='pci' index='0' model='pci-root'>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 688846b9b..5758811b8 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2965,6 +2965,7 @@ mymain(void)
             QEMU_CAPS_DEVICE_ISA_SERIAL,
             QEMU_CAPS_HDA_DUPLEX);
     DO_TEST("user-aliases2", QEMU_CAPS_DEVICE_IOH3420, QEMU_CAPS_ICH9_AHCI);
+    DO_TEST_PARSE_ERROR("user-aliases-usb", QEMU_CAPS_KVM);
 
     if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
         virFileDeleteTree(fakerootdir);
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu: Forbid user aliases for USB controllers
Posted by Peter Krempa 7 years, 2 months ago
On Fri, Mar 09, 2018 at 12:56:13 +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1552127
> 
> These are bit different than other devices. Their alias also
> specify the name of the bus. For instance, if there are these
> 'joined' USB devices:
> 
>   <controller type='usb' index='0' model='ich9-ehci1'>
>     <alias name='ua-myUSB'/>
>     <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x7'/>
>   </controller>
>   <controller type='usb' index='0' model='ich9-uhci1'>
>     <master startport='0'/>
>     <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' multifunction='on'/>
>   </controller>
> 
> which translates to cmd line as:
> 
>   -device ich9-usb-ehci1,id=ua-myUSB,bus=pci.0,addr=0x4.0x7
>   -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4
> 
> The problem is that UHCI is still trying to serve 'usb.0' bus.
> Rather than trying to come up with some complicated algorithm to
> make everything work, lets forbid user aliases for USB
> controllers.

I'm not a fan of this. This creates situations where the user is not
able to know which devices support user aliases and which don't. If
there isn't a technical problem preventing this from working we should
not forbid it.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu: Forbid user aliases for USB controllers
Posted by Michal Privoznik 7 years, 2 months ago
On 03/12/2018 10:04 AM, Peter Krempa wrote:
> On Fri, Mar 09, 2018 at 12:56:13 +0100, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1552127
>>
>> These are bit different than other devices. Their alias also
>> specify the name of the bus. For instance, if there are these
>> 'joined' USB devices:
>>
>>   <controller type='usb' index='0' model='ich9-ehci1'>
>>     <alias name='ua-myUSB'/>
>>     <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x7'/>
>>   </controller>
>>   <controller type='usb' index='0' model='ich9-uhci1'>
>>     <master startport='0'/>
>>     <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' multifunction='on'/>
>>   </controller>
>>
>> which translates to cmd line as:
>>
>>   -device ich9-usb-ehci1,id=ua-myUSB,bus=pci.0,addr=0x4.0x7
>>   -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4
>>
>> The problem is that UHCI is still trying to serve 'usb.0' bus.
>> Rather than trying to come up with some complicated algorithm to
>> make everything work, lets forbid user aliases for USB
>> controllers.
> 
> I'm not a fan of this. This creates situations where the user is not
> able to know which devices support user aliases and which don't. If
> there isn't a technical problem preventing this from working we should
> not forbid it.
> 

Thing is I think we are facing technical problem. Let me give you an example:

  <controller type='usb' index='0' model='ich9-ehci1'>
    <alias name='ua-myUSB1'/>
    <address type='pci' domain='0' bus='0' slot='4' function='7'/>
  </controller>
  <controller type='usb' index='0' model='ich9-uhci1'>
    <alias name='ua-myUSB2'/>
    <master startport='0'/>
    <address type='pci' domain='0' bus='0' slot='4' function='0' multifunction='on'/>
  </controller>
  <controller type='usb' index='0' model='ich9-uhci2'>
    <alias name='ua-myUSB3'/>
    <master startport='2'/>
    <address type='pci' domain='0' bus='0' slot='4' function='1'/>
  </controller>
  <controller type='usb' index='0' model='ich9-uhci3'>
    <alias name='ua-myUSB3'/>
    <master startport='4'/>
    <address type='pci' domain='0' bus='0' slot='4' function='2'/>
  </controller>

What should the generated command line look like? To my knowledge, these
ich9-XhciN devices are tricky and their id= attribute gives also name to
the bus they are creating.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu: Forbid user aliases for USB controllers
Posted by Peter Krempa 7 years, 2 months ago
On Mon, Mar 12, 2018 at 13:12:27 +0100, Michal Privoznik wrote:
> On 03/12/2018 10:04 AM, Peter Krempa wrote:
> > On Fri, Mar 09, 2018 at 12:56:13 +0100, Michal Privoznik wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1552127
> >>
> >> These are bit different than other devices. Their alias also
> >> specify the name of the bus. For instance, if there are these
> >> 'joined' USB devices:
> >>
> >>   <controller type='usb' index='0' model='ich9-ehci1'>
> >>     <alias name='ua-myUSB'/>
> >>     <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x7'/>
> >>   </controller>
> >>   <controller type='usb' index='0' model='ich9-uhci1'>
> >>     <master startport='0'/>
> >>     <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' multifunction='on'/>
> >>   </controller>
> >>
> >> which translates to cmd line as:
> >>
> >>   -device ich9-usb-ehci1,id=ua-myUSB,bus=pci.0,addr=0x4.0x7
> >>   -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4
> >>
> >> The problem is that UHCI is still trying to serve 'usb.0' bus.
> >> Rather than trying to come up with some complicated algorithm to
> >> make everything work, lets forbid user aliases for USB
> >> controllers.
> > 
> > I'm not a fan of this. This creates situations where the user is not
> > able to know which devices support user aliases and which don't. If
> > there isn't a technical problem preventing this from working we should
> > not forbid it.
> > 
> 
> Thing is I think we are facing technical problem. Let me give you an example:
> 
>   <controller type='usb' index='0' model='ich9-ehci1'>
>     <alias name='ua-myUSB1'/>
>     <address type='pci' domain='0' bus='0' slot='4' function='7'/>
>   </controller>
>   <controller type='usb' index='0' model='ich9-uhci1'>
>     <alias name='ua-myUSB2'/>
>     <master startport='0'/>
>     <address type='pci' domain='0' bus='0' slot='4' function='0' multifunction='on'/>
>   </controller>
>   <controller type='usb' index='0' model='ich9-uhci2'>
>     <alias name='ua-myUSB3'/>
>     <master startport='2'/>
>     <address type='pci' domain='0' bus='0' slot='4' function='1'/>
>   </controller>
>   <controller type='usb' index='0' model='ich9-uhci3'>
>     <alias name='ua-myUSB3'/>
>     <master startport='4'/>
>     <address type='pci' domain='0' bus='0' slot='4' function='2'/>
>   </controller>
> 
> What should the generated command line look like? To my knowledge, these
> ich9-XhciN devices are tricky and their id= attribute gives also name to
> the bus they are creating.

I don't have deep technical knowledge of how the USB sub-controllers
work, but allowing an user alias for the master controller should be
possible even now, only the code generating the commandline/alias for
the sub-controller needs to be fixed to use the proper one.

For the issue of using different alias for controllers which need to
have the same, you could forbid that configuration.

Another option would be to allow the same alias, but that would not be
really different from the situation where users need to know which
support duplicate aliases, so I'd not go that way.

Yet another option is to treat the alias of the sub-controllers as an
opaque string and use the proper master alias on the commandline. This
will turn the alias of the sub-controllers into a unused user-provided
identifier, but would not break the assumptions of unique aliases and
would allow the controllers to work properly.

Peter

P.S.: The best option would be not to pass the user alias on the
commandline at all and reat it opaque strings. But it's too late for
that.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list