On Tue, 11 Dec 2018 11:10:13 -0700 Alex Williamson <alex.williamson@xxxxxxxxxx> wrote: > [cc +Halil] > > On Mon, 26 Nov 2018 16:48:50 +0800 > Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx> wrote: > > > Hi, > > > > This is respin of previous sending from > > https://www.spinics.net/lists/kvm/msg176447.html > > > > Current mdev device create interface depends on fixed mdev type, > > which get uuid from user to create instance of mdev device. If user > > wants to use customized number of resource for mdev device, then > > only can create new mdev type for that which may not be flexible. > > This requirement comes not only from to be able to allocate flexible > > resources for KVMGT, but also from Intel scalable IO virtualization > > which would use vfio/mdev to be able to allocate arbitrary resources > > on mdev instance. More info on [1] [2] [3]. > > > > To allow to create user defined resources for mdev, it trys to > > extend mdev create interface by adding new "aggregate=xxx" parameter > > following UUID, for target mdev type if aggregation is supported, it > > can create new mdev device which contains resources combined by > > number of instances, e.g > > > > echo "<uuid>,aggregate=10" > create > > > > VM manager e.g libvirt can check mdev type with "aggregation" > > attribute which can support this setting. If no "aggregation" > > attribute found for mdev type, previous behavior is still kept for > > one instance allocation. And new sysfs attribute > > "aggregated_instances" is created for each mdev device to show > > allocated number. > > Halil, I hope I'm not mis-remembering, but I think you mentioned some > apprehension of this aggregation attribute with regard to the spirit of > mdev devices at KVM Forum. If that's correct, do you have any comments > on this series or suggestions towards a better interface? Obviously > this is an optional attribute targeting devices which can scale with a > semi-arbitrary number of instances behind a single device, where > specifying an mdev type for each enumeration of those aggregate > instances in impractical. I don't know that there's anything similar > in AP, but if we're missing some alignment between interfaces, please > speak up. Thanks, > > Alex > Hi Alex! Thanks for asking. First things first: I'm happy we can squeeze in with our weird scenarios (like vfio-ap to some extent vfio-ccw) somehow, and I'm a bit reluctant to criticize or make to generalizations coming form those positions. My personal opinion is, with the mandatory attribute available_instances we got the assumption of homogeneous and functionally interchangeable resources represented by an mdev device of a certain mdev type into the DNA of mdev. Let me quote from Documentation/vfio-mediated-device.txt """ * available_instances This attribute should show the number of devices of type <type-id> that can be created. """ <digression> Please note it says *devices* (does not talk backing resources or whatever). This series however describes "aggregation" like """ * <aggregation> <aggregation> is an optional attributes to show max number that the instance resources of [<type-id>] can be aggregated to be assigned for one mdev device. No <aggregation> attribute means driver doesn't support to aggregate instance resoures for one mdev device. <aggregation> may be less than available_instances which depends on driver. <aggregation> number can't exceed available_instances. Set number of instances by appending "aggregate=N" parameter for create attribute. By default without "aggregate=N" parameter it will create one instance as normal. """ Please note that we talk about *instance resources* where instance is not an mdev device instance (when aggregating we have one mdev device instance). </digression> In my opinion, bringing in this assumption that 'the parent device provides <instances> homogeneous and functionally interchangeable resources, and when one does a successful create one is allocated and assigned to the created mdev device' was a needless limitation in the first place. I.e. it does not translate at all to vfio-ap. Thinking abstract OOP, mdev devices are just objects of some type (or with other words object class). Create corresponds to the constructor. Furthermore mdev is a proponent of the RAII (resource acquisition is initialization) pattern -- I like RAII. Now the fact that type of the object created determines every relevant aspect of the resources acquired is IMHO limiting. This series tries to mitigate that problem by introduces aggregate=N with the semantic 'Nay, don't acquire resources as the type would normally imply, but multiply what is normal with N -- kind of.' This is an optional mechanism, with optional facilities. The bulk of the code is added to core mdev, because aggregate is although optional but nevertheless core feature of mdev. Clearly aggregate makes no sense for vfio-ccw nor for vfio-ap: for the latter, the identity of the backing resources is crucial. But vfio-ap could, at least theoretically, benefit from a create parameter. Currently we acquire resources after the mdev device was created, because in the context of create there was no means to tell what resources are to be acquired. Coming from this position, I think, passing everything after the uuid to the vendor driver, and letting it handle 'constructor parameters' would be a more flexible option. If that flexibility is desirable, that is a different matter. IMHO if mdev is supposed to be about this homogeneous and at acquisition time interchangeable 'instance resources' use case, and everything different is considered oddball, then the aggregate 'attribute' and the mechanic behind it is kind of sound. Maybe if we change the description for available_instances, the conceptual discomfort I voiced in the digression can be entirely eliminated. On the other hand, if we want 'a slice of some device, where what exactly is included the slice actually matters' as a first class citizen of the mdev universe, then I have a bad feeling about the interface change series is proposing. Thanks again for prompting me to speak up. I hope I managed to formulate this as reluctant, as reluctant I feel speaking about the design decision being made here. Regards, Halil > > References: > > [1] > > https://software.intel.com/en-us/download/intel-virtualization-technology-for-directed-io-architecture-specification > > [2] > > https://software.intel.com/en-us/download/intel-scalable-io-virtualization-technical-specification > > [3] https://schd.ws/hosted_files/lc32018/00/LC3-SIOV-final.pdf > > > > v2: > > - Add new create_with_instances driver hook > > - Update doc for new attributes > > > > v3: > > - Rename parameter and attribute names from review > > - Make "aggregation" attribute to show maxium resource number > > for aggregation > > - Check driver hooks for attribute create validation > > - Update doc and ABI spec > > > > v4: > > - Rebase > > > > Zhenyu Wang (6): > > vfio/mdev: Add new "aggregate" parameter for mdev create > > vfio/mdev: Add "aggregation" attribute for supported mdev type > > vfio/mdev: Add "aggregated_instances" attribute for supported mdev > > device > > Documentation/vfio-mediated-device.txt: Update for vfio/mdev > > aggregation support > > Documentation/ABI/testing/sysfs-bus-vfio-mdev: Update for vfio/mdev > > aggregation support > > drm/i915/gvt: Add new type with aggregation support > > > > Documentation/ABI/testing/sysfs-bus-vfio-mdev | 25 +++++++ > > Documentation/vfio-mediated-device.txt | 44 +++++++++-- > > drivers/gpu/drm/i915/gvt/gvt.h | 11 ++- > > drivers/gpu/drm/i915/gvt/kvmgt.c | 53 ++++++++++++- > > drivers/gpu/drm/i915/gvt/vgpu.c | 56 +++++++++++++- > > drivers/vfio/mdev/mdev_core.c | 40 +++++++++- > > drivers/vfio/mdev/mdev_private.h | 6 +- > > drivers/vfio/mdev/mdev_sysfs.c | 74 > > ++++++++++++++++++- include/linux/mdev.h | > > 19 +++++ 9 files changed, 305 insertions(+), 23 deletions(-) > > >
