Re: [PATCH v8 4/6] docs: Add Documentation for Mediated devices

Tian, Kevin
2016-10-13T03:27:50Z

> From: Kirti Wankhede [mailto:kwankhede@nvidia.com]
> Sent: Thursday, October 13, 2016 3:03 AM
>
>
> On 10/12/2016 9:29 PM, Alex Williamson wrote:
> > On Wed, 12 Oct 2016 20:43:48 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >
> >> On 10/12/2016 7:22 AM, Tian, Kevin wrote:
> >>>> From: Kirti Wankhede [mailto:kwankhede@nvidia.com]
> >>>> Sent: Wednesday, October 12, 2016 4:45 AM
> >>>>>> +* mdev_supported_types:
> >>>>>> + List of current supported mediated device types and its details are added
> >>>>>> +in this directory in following format:
> >>>>>> +
> >>>>>> +|- <parent phy device>
> >>>>>> +|--- Vendor-specific-attributes [optional]
> >>>>>> +|--- mdev_supported_types
> >>>>>> +| |--- <type id>
> >>>>>> +| | |--- create
> >>>>>> +| | |--- name
> >>>>>> +| | |--- available_instances
> >>>>>> +| | |--- description /class
> >>>>>> +| | |--- [devices]
> >>>>>> +| |--- <type id>
> >>>>>> +| | |--- create
> >>>>>> +| | |--- name
> >>>>>> +| | |--- available_instances
> >>>>>> +| | |--- description /class
> >>>>>> +| | |--- [devices]
> >>>>>> +| |--- <type id>
> >>>>>> +| |--- create
> >>>>>> +| |--- name
> >>>>>> +| |--- available_instances
> >>>>>> +| |--- description /class
> >>>>>> +| |--- [devices]
> >>>>>> +
> >>>>>> +[TBD : description or class is yet to be decided. This will change.]
> >>>>>
> >>>>> I thought that in previous discussions we had agreed to drop
> >>>>> the <type id> concept and use the name as the unique identifier.
> >>>>> When reporting these types in libvirt we won't want to report
> >>>>> the type id values - we'll want the name strings to be unique.
> >>>>>
> >>>>
> >>>> The 'name' might not be unique but type_id will be. For example that Neo
> >>>> pointed out in earlier discussion, virtual devices can come from two
> >>>> different physical devices, end user would be presented with what they
> >>>> had selected but there will be internal implementation differences. In
> >>>> that case 'type_id' will be unique.
> >>>>
> >>>
> >>> Hi, Kirti, my understanding is that Neo agreed to use an unique type
> >>> string (if you still called it <type id>), and then no need of additional
> >>> 'name' field which can be put inside 'description' field. See below quote:
> >>>
> >>
> >> We had internal discussions about this within NVIDIA and found that
> >> 'name' might not be unique where as 'type_id' would be unique. I'm
> >> refering to Neo's mail after that, where Neo do pointed that out.
> >>
> >> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg07714.html
> >
> > Everyone not privy to those internal discussions, including me, seems to
> > think we dropped type_id and that if a vendor does not have a stable
> > name, they can compose some sort of stable type description based on the
> > name+id, or even vendor+id, ex. NVIDIA-11. So please share why we
> > haven't managed to kill off type_id yet. No matter what internal
> > representation each vendor driver has of "type_id" it seems possible
> > for it to come up with stable string to define a given configuration.
>
>
> The 'type_id' is unique and the 'name' are not, the name is just a
> virtual device name/ human readable name. Because at this moment Intel
> can't define a proper GPU class, we have to add a 'description' field
> there as well to represent the features of this virtual device, once we
> have all agreed with the GPU class and its mandatory attributes, the
> 'description' field can be removed. Here is an example,
> type_id/type_name = NVIDIA_11,
> name=M60-M0Q,
> description=2560x1600, 2 displays, 512MB"

As I commented earlier, I didn't see how above attributes can be defined
mandatory:

- #displays, is concerned only for VDI usage, where remote user may
care about how many virtual displays it could be use. What about using
vGPU in non-VDI usage, e.g. purely media transcoding case where
#displays is just nothing? Then for media transcoding do we want to
further introduce attributes like H.265?

- framebuffer size (512MB) might make sense to discrete card like
NVIDIA. In your case the graphics memory is on-card, so the memory
size is critical to performance so user might want to know. However for
integrated card like Intel, we just use system memory as 'virtual' graphics
memory through GPU page tables. There is one global GPU page table
(GGTT) partitioned between vGPUs, but the majority of rendering happens
on per-process GPU page table (PPGTT) which can be fully managed by
each VM. In this sense, the size of GGTT resource has little performance
implication (mostly an indirect functionality sense, such as #displays) User
cannot make clear expectation on it, so we don't have plan to expose it.

>
> Neo's previous comment only applies to the situation where we will have
> the GPU class or optional attributes defined and recognized by libvirt,
> since that is not going to happen any time soon, we will have to have
> the new 'description' field, and we don't want to have it mixed up with
> 'name' field.

As explained above, I don't see necessity of defining a GPU class now,
at least not based on your examples (except resolution which might be
one). If required in the future, it must include mandatory attributes
cross virtualizations usages and hardware vendors. Vendor can choose
to document the relationship between vGPU type and applied virtualization
usages in their own user manual.

>
> We can definitely have something like name+id as Alex recommended to
> remove the 'name' field, but it will just require libvirt to have more
> logic to parse that string.
>

In this manner I prefer to have name+id style. libvirt doesn't need to
parse it, just treat the whole name+id as the unique type identification.
'description' field is kept for vendor specific descriptive purpose where
you can add above attributes. libvirt doesn't need to parse too, just
pass it up.

Thanks
Kevin