Re: [Qemu-devel] [RFC v2] libvirt vGPU QEMU integration

[Paolo Bonzini]

On 19/09/2016 23:36, Alex Williamson wrote:
> On Tue, 20 Sep 2016 02:05:52 +0530
> Kirti Wankhede <address@hidden> wrote:
>> 'fb_length':
>>     Read-only file. Mandatory.
>>     Returns <number>{K,M,G}, size of framebuffer.
> 
> This can't be mandatory, it's only relevant to vGPU devices, vGPUs are
> just one user of mediated devices. [...]
> 
>> 'params'
>>     Write-Only file. Optional.
>>     String input. Libvirt would pass the string given in XML file to
>> this file and then create mdev device. Set empty string to clear params.
>> For example, set parameter 'frame_rate_limiter=0' to disable frame rate
>> limiter for performance benchmarking, then create device of type 11. The
>> device created would have that parameter set by vendor driver.
> 
> Might as well just call it "magic", there's absolutely no ability to
> introspect what parameters are allowed or even valid here.  Can all
> parameters be changed dynamically?  Do parameter changes apply to
> existing devices or only future devices?  This is a poorly specified
> interface.  I'd prefer this be done via module options on the vendor
> driver.

Or additional files in the mdev's sysfs directory?

>> The parent device would look like:
>>
>>    <device>
>>      <name>pci_0000_86_00_0</name>
>>      <capability type='pci'>
>>        <domain>0</domain>
>>        <bus>134</bus>
>>        <slot>0</slot>
>>        <function>0</function>
> 
> This is the parent device address?

(Since this is taken from an email of mine, this is libvirt's XML
interpretation of the udev database and the contents of sysfs.  Try
"virsh nodedev-dumpxml pci_0000_00_02_0" for an example that is
unrelated to mdev).

>> 2. Create/destroy mediated device
>>
>> With above example, vGPU device XML would look like:
>>
>>    <device>
>>      <name>my-vgpu</name>
>>      <parent>pci_0000_86_00_0</parent>
>>      <capability type='mdev'>
>>        <type id='11'/>
>>        <group>1</group>
>>        <params>'frame_rate_limiter=0'</params>
>>      </capability>
>>    </device>
>>
>> 'type id' is mandatory.
> 
> I was under the impression that the vendor supplied "name" was the one
> unique identifier.  We're certainly not going to create a registrar to
> hand out type ids to each vendor, so what makes type id unique?

Why does it have to be unique?  It's just parroting what libvirt said in
the <capability type='mdev'> block of the parent's nodedev-dumpxml.

(Though I guess going by name could be useful as well.  Perhaps both
could be allowed).

>> * Clear params, if set earlier:
>>
>>     echo "" > /sys/../0000\:86\:00.0/11/params
> 
> So params only applies to the devices created while those params are
> set?  This is inherently racy.

I agree, these extra attributes should be set on the mdev, not the type
directory.

Paolo