Re: [PATCH virt-manager v2 0/4] Enable MDEV support

[Shalini Chellathurai Saroja <shalini@xxxxxxxxxxxxx>]

On 6/1/21 11:13 PM, Cole Robinson wrote:

> On 5/31/21 3:54 PM, Shalini Chellathurai Saroja wrote:
> > Enable support for mediated devices in virt-xml tool and virt-manager
> > GUI tool.
> >
> >       
> Thanks! I pushed 1-3 with a small tweak to patch #1 to keep code
> coverage at 100%.
>

Good morning Cole,

Thank you for reviewing and pushing the virt-xml patch.

> In order to get the parent naming stuff in addhardware.py to trigger I
> needed to change things a bit:
>
> diff --git a/virtManager/addhardware.py b/virtManager/addhardware.py
> index 2df84bf5..13b899c3 100644
> --- a/virtManager/addhardware.py
> +++ b/virtManager/addhardware.py
> @@ -783,9 +783,10 @@ class vmmAddHardware(vmmGObjectUI):
>                          prettyname += " (%s)" % subdev.pretty_name()
>
>              if devtype == "mdev":
> -                for subdev in self.conn.filter_nodedevs("mdev"):
> -                    if dev.xmlobj.name == subdev.xmlobj.parent:
> -                        prettyname += " (%s)" % subdev.pretty_name()
> +                for parentdev in self.conn.list_nodedevs():
> +                    if dev.xmlobj.parent == parentdev.xmlobj.name:
> +                        prettyname = "%s %s" % (
> +                                parentdev.pretty_name(), prettyname)
>
>              model.append([dev.xmlobj, prettyname])
>
>
> Was it working in your code before that, like on a real system with mdev
> devices? Or was the Add Hardware->MDEV only listing devices with name
> `mdev_$UUID` ?
>     
    MDEV devices are listed as mdev_$UUID in my system with MDEV
    devices. 
    I have provided a screenshot with my code below.

> The point of this code is to give more descriptive names to what we list
> in the UI, if we can help it. The names for 'mdev' devices are not
> descriptive.
>     
    Yes, your code provides a more descriptive name and helps to 
    identify the device type (like CCW/PCI/AP device) . We can use it. 
    I have provided a screenshot with your code below.

> Their type id=XXX values are a bit better:
> vfio_ap-passthrough, nvidia-11, etc. Maybe we can just use that? The
> parent devices have a bit more info under <capability><type><name> but
> it's not clear if that's always available. Maybe we can get away with
> just using the <type id=XXX> value in the mdev pretty_name() definition
>     
    More than one MDEV device can have the same type id as shown below.
    
    May be, we can use type id in combination with mdev_$uuid. 
    I feel like your suggested code is better, as we could identify the
    parent 
    device and MDEV device type with this code.

# virsh nodedev-dumpxml mdev_8e782fea_e5f4_45fa_a0f9_024cf66e5009
<device>
  <name>mdev_8e782fea_e5f4_45fa_a0f9_024cf66e5009</name>
  <path>/sys/devices/css0/0.0.0024/8e782fea-e5f4-45fa-a0f9-024cf66e5009</path>
  <parent>css_0_0_0024</parent>
  <driver>
    <name>vfio_mdev</name>
  </driver>
  <capability type='mdev'>
    <type id='vfio_ccw-io'/>
    <uuid>8e782fea-e5f4-45fa-a0f9-024cf66e5009</uuid>
    <iommuGroup number='1'/>
  </capability>
</device>

# virsh nodedev-dumpxml mdev_92cc4b96_95d8_47ce_923c_c688d061bc41
<device>
  <name>mdev_92cc4b96_95d8_47ce_923c_c688d061bc41</name>
  <path>/sys/devices/css0/0.0.0023/92cc4b96-95d8-47ce-923c-c688d061bc41</path>
  <parent>css_0_0_0023</parent>
  <driver>
    <name>vfio_mdev</name>
  </driver>
  <capability type='mdev'>
    <type id='vfio_ccw-io'/>
    <uuid>92cc4b96-95d8-47ce-923c-c688d061bc41</uuid>
    <iommuGroup number='2'/>
  </capability>
</device>

    Similarly, <capability><type><name> info in MDEV
    parent device 
    can also be same for more than one MDEV device.

# virsh nodedev-dumpxml css_0_0_0023
<device>
  <name>css_0_0_0023</name>
  <path>/sys/devices/css0/0.0.0023</path>
  <parent>computer</parent>
  <driver>
    <name>vfio_ccw</name>
  </driver>
  <capability type='css'>
    <cssid>0x0</cssid>
    <ssid>0x0</ssid>
    <devno>0x0023</devno>
    <capability type='mdev_types'>
      <type id='vfio_ccw-io'>
        <name>I/O subchannel (Non-QDIO)</name>
        <deviceAPI>vfio-ccw</deviceAPI>
        <availableInstances>0</availableInstances>
      </type>
    </capability>
  </capability>
</device>

# virsh nodedev-dumpxml css_0_0_0024
<device>
  <name>css_0_0_0024</name>
  <path>/sys/devices/css0/0.0.0024</path>
  <parent>computer</parent>
  <driver>
    <name>vfio_ccw</name>
  </driver>
  <capability type='css'>
    <cssid>0x0</cssid>
    <ssid>0x0</ssid>
    <devno>0x0024</devno>
    <capability type='mdev_types'>
      <type id='vfio_ccw-io'>
        <name>I/O subchannel (Non-QDIO)</name>
        <deviceAPI>vfio-ccw</deviceAPI>
        <availableInstances>0</availableInstances>
      </type>
    </capability>
  </capability>
</device>

    Are you ok with using your suggested code (parent name mdev_$uuid) 
    for prettyname? Please let me know your feedback. 
> Thanks,
> Cole
>
>     
-- 
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294