> From: Kirti Wankhede [mailto:kwankhede@xxxxxxxxxx] > Sent: Thursday, May 05, 2016 6:45 PM > > > On 5/5/2016 2:36 PM, Tian, Kevin wrote: > >> From: Kirti Wankhede > >> Sent: Wednesday, May 04, 2016 9:32 PM > >> > >> Thanks Alex. > >> > >> >> +config VGPU_VFIO > >> >> + tristate > >> >> + depends on VGPU > >> >> + default n > >> >> + > >> > > >> > This is a little bit convoluted, it seems like everything added in this > >> > patch is vfio agnostic, it doesn't necessarily care what the consumer > >> > is. That makes me think we should only be adding CONFIG_VGPU here and > >> > it should not depend on CONFIG_VFIO or be enabling CONFIG_VGPU_VFIO. > >> > The middle config entry is also redundant to the first, just move the > >> > default line up to the first and remove the rest. > >> > >> CONFIG_VGPU doesn't directly depend on VFIO. CONFIG_VGPU_VFIO is > >> directly dependent on VFIO. But devices created by VGPU core module need > >> a driver to manage those devices. CONFIG_VGPU_VFIO is the driver which > >> will manage vgpu devices. So I think CONFIG_VGPU_VFIO should be enabled > >> by CONFIG_VGPU. > >> > >> This would look like: > >> menuconfig VGPU > >> tristate "VGPU driver framework" > >> select VGPU_VFIO > >> default n > >> help > >> VGPU provides a framework to virtualize GPU without SR-IOV cap > >> See Documentation/vgpu.txt for more details. > >> > >> If you don't know what do here, say N. > >> > >> config VGPU_VFIO > >> tristate > >> depends on VGPU > >> depends on VFIO > >> default n > >> > > > > There could be multiple drivers operating VGPU. Why do we restrict > > it to VFIO here? > > > > VGPU_VFIO uses VFIO APIs, it depends on VFIO. > I think since there is no other driver than VGPU_VFIO for VGPU devices, > we should keep default selection of VGPU_VFIO on VGPU. May be in future > if other driver is add ti operate vGPU devices, then default selection > can be removed. What's your plan to support Xen here? > > >> >> +create_attr_error: > >> >> + if (gpu_dev->ops->vgpu_destroy) { > >> >> + int ret = 0; > >> >> + ret = gpu_dev->ops->vgpu_destroy(gpu_dev->dev, > >> >> + vgpu_dev->uuid, > >> >> + vgpu_dev->vgpu_instance); > >> > > >> > Unnecessary initialization and we don't do anything with the result. > >> > Below indicates lack of vgpu_destroy indicates the vendor doesn't > >> > support unplug, but doesn't that break our error cleanup path here? > >> > > >> > >> Comment about vgpu_destroy: > >> If VM is running and vgpu_destroy is called that > >> means the vGPU is being hotunpluged. Return > >> error if VM is running and graphics driver > >> doesn't support vgpu hotplug. > >> > >> Its GPU drivers responsibility to check if VM is running and return > >> accordingly. This is vgpu creation path. Vgpu device would be hotplug to > >> VM on vgpu_start. > > > > How does GPU driver know whether VM is running? VM is managed > > by KVM here. > > > > Maybe it's clearer to say whether vGPU is busy which means some work > > has been loaded to vGPU. That's something GPU driver can tell. > > > > GPU driver can detect based on resources allocated for the VM from > vgpu_create/vgpu_start. Yes, in that case GPU driver only knows a vGPU is in-use, not who is using vGPU (now is VM, in the future it could be a container). Anyway my point is just not assuming VM to add limitation when it's not necessary. :-) > > >> > >> >> + * @vgpu_bar_info: Called to get BAR size and flags of vGPU device. > >> >> + * @vdev: vgpu device structure > >> >> + * @bar_index: BAR index > >> >> + * @bar_info: output, returns size and flags of > >> >> + * requested BAR > >> >> + * Returns integer: success (0) or error (< 0) > >> > > >> > This is called bar_info, but the bar_index is actually the vfio region > >> > index and things like the config region info is being overloaded > >> > through it. We already have a structure defined for getting a generic > >> > region index, why not use it? Maybe this should just be > >> > vgpu_vfio_get_region_info. > >> > > >> > >> Ok. Will do. > > > > As you commented earlier that GPU driver is required to provide config > > space (which I agree), then what's the point of introducing another > > bar specific structure? VFIO can use @write to get bar information > > from vgpu config space, just like how it's done on physical device today. > > > > It is required not only for size, but also to fetch flags. Region flags > could be combination of: > > #define VFIO_REGION_INFO_FLAG_READ (1 << 0) /* Region supports read */ > #define VFIO_REGION_INFO_FLAG_WRITE (1 << 1) /* Region supports write */ > #define VFIO_REGION_INFO_FLAG_MMAP (1 << 2) /* Region supports mmap */ > > Thanks, > Kirti. That makes sense. Thanks Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
