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

[Shalini Chellathurai Saroja]

On 6/2/21 7:45 PM, Cole Robinson wrote:
> 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.
ok, then I will update the code and send v3, thank you for the review.
>
> 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