On 2018.07.24 11:44:40 -0600, Alex Williamson wrote: > On Fri, 20 Jul 2018 10:19:24 +0800 > Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx> wrote: > > > 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 "instances=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>,instances=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 "instances" is created for each mdev device to show allocated number. > > > > This trys to create new KVMGT type with minimal vGPU resources which can be > > combined with "instances=x" setting to allocate for user wanted resources. > > "instances" makes me think this is arg helps to create multiple mdev > instances rather than consuming multiple instances for a single mdev. > You're already exposing the "aggregation" attribute, so doesn't > "aggregate" perhaps make more sense as the create option? We're asking > the driver to aggregate $NUM instances into a single mdev. The mdev > attribute should then perhaps also be "aggregated_instances". yeah that seems better. > > The next user question for the interface might be what aspect of the > device gets multiplied by this aggregation? In i915 I see you're > multiplying the memory sizes by the instance, but clearly the > resolution doesn't change. I assume this is sort of like mdev types > themselves, ie. some degree of what a type means is buried in the > implementation and some degree of what some number of those types > aggregated together means is impossible to describe generically. > yeah, the purpose was to increase memory resource only, but due to current free formatted 'description', can't have a meaningful expression for that, and not sure if libvirt likes to understand vendor specific behavior e.g for intel vGPU? > We're also going to need to add aggregation to the checklist for device > compatibility for migration, for example 1) is it the same mdev_type, > 1a) are the aggregation counts the same (new), 2) is the host driver > compatible (TBD). > Right, will check with Zhi on that. > The count handling in create_store(), particularly MDEV_CREATE_OPT_LEN > looks a little suspicious. I think we should just be validating that > the string before the ',' or the entire string if there is no comma is > UUID length. Pass only that to uuid_le_to_bin(). We can then strncmp > as you have for "instances=" (or "aggregate=") but then let's be sure > to end the string we pass to kstrtouint(), ie. assume there could be > further args. Original purpose was to limit the length of string to accept, but can take this way without issue I think. > > Documentation/ABI/testing/sysfs-bus-vfio-mdev also needs to be updated > with this series. Ok. Thanks > > I'm curious what libvirt folks and Kirti think of this, it looks like > it has a nice degree of backwards compatibility, both in the sysfs > interface and the vendor driver interface. Thanks, > > Alex -- Open Source Technology Center, Intel ltd. $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
Attachment:
signature.asc
Description: PGP signature
