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

[David Gibson]

On Thu, Mar 07, 2019 at 03:02:32PM -0700, Alex Williamson wrote:
> On Thu,  7 Mar 2019 16:05:18 +1100
> Alexey Kardashevskiy <address@hidden> 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).


-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature