Re: [Qemu-devel] [PATCH qemu v4 3/3] spapr: Support NVIDIA V100 GPU with NVLink2

[Alex Williamson <alex.williamson@redhat.com>]

On Fri, 8 Mar 2019 15:34:56 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Mar 07, 2019 at 03:02:32PM -0700, Alex Williamson wrote:
> > On Thu,  7 Mar 2019 16:05:18 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:  
> > > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> > > index 40a12001f580..15ec0b4c2723 100644
> > > --- a/hw/vfio/pci-quirks.c
> > > +++ b/hw/vfio/pci-quirks.c
> > > @@ -2180,3 +2180,123 @@ int vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp)
> > >  
> > >      return 0;
> > >  }
> > > +
> > > +static void vfio_pci_nvlink2_get_tgt(Object *obj, Visitor *v,
> > > +                                     const char *name,
> > > +                                     void *opaque, Error **errp)
> > > +{
> > > +    uint64_t tgt = (uint64_t) opaque;
> > > +    visit_type_uint64(v, name, &tgt, errp);
> > > +}
> > > +
> > > +static void vfio_pci_nvlink2_get_link_speed(Object *obj, Visitor *v,
> > > +                                                 const char *name,
> > > +                                                 void *opaque, Error **errp)
> > > +{
> > > +    uint32_t link_speed = (uint32_t)(uint64_t) opaque;
> > > +    visit_type_uint32(v, name, &link_speed, errp);
> > > +}
> > > +
> > > +int vfio_pci_nvidia_v100_ram_init(VFIOPCIDevice *vdev, Error **errp)
> > > +{
> > > +    int ret;
> > > +    void *p;
> > > +    struct vfio_region_info *nv2region = NULL;
> > > +    struct vfio_info_cap_header *hdr;
> > > +    MemoryRegion *nv2mr = g_malloc0(sizeof(*nv2mr));  
> > 
> > This is leaked in the below error paths and there's no cleanup on
> > finalize.  I assume these devices don't support hotplug, but they could
> > at least use the existing quirk infrastructure so as not to set a bad
> > precedent. 
> >   
> > > +
> > > +    ret = vfio_get_dev_region_info(&vdev->vbasedev,
> > > +                                   VFIO_REGION_TYPE_PCI_VENDOR_TYPE |
> > > +                                   PCI_VENDOR_ID_NVIDIA,
> > > +                                   VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM,
> > > +                                   &nv2region);
> > > +    if (ret) {
> > > +        return ret;
> > > +    }
> > > +
> > > +    p = mmap(NULL, nv2region->size, PROT_READ | PROT_WRITE | PROT_EXEC,
> > > +             MAP_SHARED, vdev->vbasedev.fd, nv2region->offset);
> > > +
> > > +    if (!p) {
> > > +        return -errno;
> > > +    }  
> > 
> > I think the above suggestion requires simply defining a quirk above:
> > 
> > VFIOQuirk *quirk;
> > 
> > Initializing it with one MemoryRegion here:
> > 
> > quirk = vfio_quirk_alloc(1);
> >   
> > > +
> > > +    memory_region_init_ram_ptr(nv2mr, OBJECT(vdev), "nvlink2-mr",  
> > 
> > s/nv2mr/quirk->mem/
> >   
> > > +                               nv2region->size, p);  
> > 
> > Then adding it to the device, for instance assuming there's always a
> > BAR0, attach it there:
> > 
> > QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
> > 
> > At least then it pretends to support cleanup.  
> 
> This does simplify the cleanup of the extra MRs.  It is a bit odd to
> attach it specifically to a BAR that's not otherwise tied to these
> resources (both the NV2 memory and ATSD are special NVLink extensions,
> not attached to a PCI BAR).

Yep, it's not perfect, we could add a list of non-BAR, non-VGA quirks
to the device itself, but I'm not sure it's necessary.  A comment to
track this discontinuity would be nice at least though.  Thanks,

Alex