Re: [libvirt] [RFC PATCH v1 1/1] Add attribute multiple_mdev_support for mdev type-id

From: Tian, Kevin Sent: Friday, September 28, 2018 9:27 AM > From: Alex Williamson [mailto:alex.williamson@redhat.com] > Sent: Friday, September 28, 2018 4:01 AM > > On Fri, 28 Sep 2018 00:53:00 +0530 > Kirti Wankhede <kwankhede(a)nvidia.com&gt; wrote: > > > On 9/27/2018 9:29 PM, Alex Williamson wrote: > > > On Thu, 27 Sep 2018 06:48:27 +0000 > > > "Tian, Kevin" <kevin.tian(a)intel.com&gt; wrote: > > > > > >>> From: Kirti Wankhede > > >>> Sent: Thursday, September 27, 2018 2:22 PM > > >>> > > >>> Generally a single instance of mdev device, a share of physical device, > is > > >>> assigned to user space application or a VM. There are cases when > multiple > > >>> instances of mdev devices of same or different types are required by > User > > >>> space application or VM. For example in case of vGPU, multiple mdev > > >>> devices > > >>> of type which represents whole GPU can be assigned to one instance > of > > >>> application or VM. > > >>> > > >>> All types of mdev devices may not support assigning multiple mdev > devices > > >>> to a user space application. In that case vendor driver can fail open() > > >>> call of mdev device. But there is no way to know User space > application > > >>> about the configuration supported by vendor driver. > > >>> > > >>> To expose supported configuration, vendor driver should add > > >>> 'multiple_mdev_support' attribute to type-id directory if vendor > driver > > >>> supports assigning multiple mdev devices of a particular type-id to > one > > >>> instance of user space application or a VM. > > >>> > > >>> User space application should read if 'multiple_mdev_support' > attibute is > > >>> present in type-id directory of all mdev devices which are going to be > > >>> used. If all read 1 then user space application can proceed with > multiple > > >>> mdev devices. > > >>> > > >>> This is optional and readonly attribute. > > >> > > >> I didn't get what is the exact problem from above description. Isn't it > the > > >> basic point behind mdev concept that parent driver can create multiple > > >> mdev instances on a physical device? VFIO usually doesn't care > whether > > >> multiple devices (pci or mdev) are assigned to same process/VM or not. > > >> Can you give some example of actual problem behind this change? > > > > > > The situation here is that mdev imposes no restrictions regarding how > > > mdev devices can be used by the user. Multiple mdevs, regardless of > > > type, can be combined and used in any way the user sees fit, at least > > > that's the theory. However, in practice, certain vendor drivers impose > > > a limitation that prevents multiple mdev devices from being used in the > > > same VM. This is done by returning an error when the user attempts to > > > open those additional devices. Now we're in the very predictable > > > situation that we want to consider that some of the vendor's mdev > types > > > might be combined in the same userspace instance, while others still > > > impose a restriction, and how can we optionally expose such per mdev > > > type restrictions to the userspace so we have something better than > > > "try it and see if it works". > > > > > > The below proposal assumes that a single instance per VM is the norm > > > and that we add something to the API to indicate multiple instances are > > > supported. However, that's really never been the position of the mdev > > > core, where there's no concept of limiting devices per user instance; > > > it's a vendor driver restriction. So I think the question is then why > > > should we burden vendor drivers who do not impose a restriction with > an > > > additional field? Does the below extension provide a better backwards > > > compatibility scenario? > > > > The vendor driver who do not want to impose restriction, doesn't need to > > provide this attribute. In that case, behavior would remain same as it > > is now, i.e. multiple mdev instances can be used by one instance of > > application. > > > > > > > With the current code, I think it's reasonable for userspace to assume > > > there are no mdev limits per userspace instance, those that exist are > > > vendor specific. The below proposal is for an optional field, what > > > does the management tool assume when it's not supplied? We have > both > > > cases currently, mdev devices that allow multiple instances per VM and > > > those that do not, so I don't see that the user is more informed with > > > this addition and no attempt is made here to synchronously update all > > > drivers to expose this new attribute. > > > > > > > When this attribute is not provided, behavior should remain same as it > > is now. But if this attribute is provided then management tool should > > read this attribute to verify that such combination is supported by > > vendor driver. > > > > > Does it make more sense then to add an attribute to expose the > > > restriction rather than the lack of restriction. Perhaps something > > > like "singleton_usage_restriction". > > > > I would prefer to expose what is supported than what is restricted. > > Above, it's stated that vendors who do not impose a restriction do not > need to implement this. So it's designed to expose a restriction. > We're stating that exposing multiple_mdev_support=1/Y is the equivalent > of not reporting the attribute at all, so the only reason to implement > it would be because there is a restriction. So why masquerade as a > feature? Thanks, > Agree it looks more like a restriction than feature. Especially a natural thought of missing a feature knob means no support of the feature, which doesn't fit the intention here. Also what default behavior should high level mgmt.. stack assume if the attribute is missing? some vendor drivers allows while other doesn't today. Then do we ask to include vendor-specific knowledge for "remain same behavior"?