> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Wednesday, May 04, 2016 6:43 AM
> > +
> > + if (gpu_dev->ops->write) {
> > + ret = gpu_dev->ops->write(vgpu_dev,
> > + user_data,
> > + count,
> > + vgpu_emul_space_config,
> > + pos);
> > + }
> > +
> > + memcpy((void *)(vdev->vconfig + pos), (void *)user_data, count);
>
> So write is expected to user_data to allow only the writable bits to be
> changed? What's really being saved in the vconfig here vs the vendor
> vgpu driver? It seems like we're only using it to cache the BAR
> values, but we're not providing the BAR emulation here, which seems
> like one of the few things we could provide so it's not duplicated in
> every vendor driver. But then we only need a few u32s to do that, not
> all of config space.
We can borrow same vconfig emulation from existing vfio-pci driver.
But doing so doesn't mean that vendor vgpu driver cannot have its
own vconfig emulation further. vGPU is not like a real device, since
there may be no physical config space implemented for each vGPU.
So anyway vendor vGPU driver needs to create/emulate the virtualized
config space while the way how is created might be vendor specific.
So better to keep the interface to access raw vconfig space from
vendor vGPU driver.
> > +static ssize_t vgpu_dev_rw(void *device_data, char __user *buf,
> > + size_t count, loff_t *ppos, bool iswrite)
> > +{
> > + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> > + struct vfio_vgpu_device *vdev = device_data;
> > +
> > + if (index >= VFIO_PCI_NUM_REGIONS)
> > + return -EINVAL;
> > +
> > + switch (index) {
> > + case VFIO_PCI_CONFIG_REGION_INDEX:
> > + return vgpu_dev_config_rw(vdev, buf, count, ppos, iswrite);
> > +
> > + case VFIO_PCI_BAR0_REGION_INDEX ... VFIO_PCI_BAR5_REGION_INDEX:
> > + return vgpu_dev_bar_rw(vdev, buf, count, ppos, iswrite);
> > +
> > + case VFIO_PCI_ROM_REGION_INDEX:
> > + case VFIO_PCI_VGA_REGION_INDEX:
>
> Wait a sec, who's doing the VGA emulation? We can't be claiming to
> support a VGA region and then fail to provide read/write access to it
> like we said it has.
For Intel side we plan to not support VGA region when upstreaming our
KVMGT work, which means Intel vGPU will be exposed only as a
secondary graphics card then so legacy VGA is not required. Also no
VBIOS/ROM requirement. Guess we can remove above two regions.
> > +
> > +static int vgpu_dev_mmio_fault(struct vm_area_struct *vma, struct vm_fault
> > *vmf)
> > +{
> > + int ret = 0;
> > + struct vfio_vgpu_device *vdev = vma->vm_private_data;
> > + struct vgpu_device *vgpu_dev;
> > + struct gpu_device *gpu_dev;
> > + u64 virtaddr = (u64)vmf->virtual_address;
> > + u64 offset, phyaddr;
> > + unsigned long req_size, pgoff;
> > + pgprot_t pg_prot;
> > +
> > + if (!vdev && !vdev->vgpu_dev)
> > + return -EINVAL;
> > +
> > + vgpu_dev = vdev->vgpu_dev;
> > + gpu_dev = vgpu_dev->gpu_dev;
> > +
> > + offset = vma->vm_pgoff << PAGE_SHIFT;
> > + phyaddr = virtaddr - vma->vm_start + offset;
> > + pgoff = phyaddr >> PAGE_SHIFT;
> > + req_size = vma->vm_end - virtaddr;
> > + pg_prot = vma->vm_page_prot;
> > +
> > + if (gpu_dev->ops->validate_map_request) {
> > + ret = gpu_dev->ops->validate_map_request(vgpu_dev, virtaddr,
> > &pgoff,
> > + &req_size, &pg_prot);
> > + if (ret)
> > + return ret;
> > +
> > + if (!req_size)
> > + return -EINVAL;
> > + }
> > +
> > + ret = remap_pfn_range(vma, virtaddr, pgoff, req_size, pg_prot);
>
> So not supporting validate_map_request() means that the user can
> directly mmap BARs of the host GPU and as shown below, we assume a 1:1
> mapping of vGPU BAR to host GPU BAR. Is that ever valid in a vGPU
> scenario or should this callback be required? It's not clear to me how
> the vendor driver determines what this maps to, do they compare it to
> the physical device's own BAR addresses?
I didn't quite understand too. Based on earlier discussion, do we need
something like this, or could achieve the purpose just by leveraging
recent sparse mmap support?
Thanks
Kevin
