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

[Cole Robinson]

On 6/2/21 9:34 AM, Shalini Chellathurai Saroja wrote:
> 
> 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.

Yes if you think that's looks good on a real machine, then that works
for me too.

Thanks,
Cole