[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH qemu v4 3/3] spapr: Support NVIDIA V100 GPU with N
From: |
Alex Williamson |
Subject: |
Re: [Qemu-ppc] [PATCH qemu v4 3/3] spapr: Support NVIDIA V100 GPU with NVLink2 |
Date: |
Fri, 8 Mar 2019 07:52:57 -0700 |
On Fri, 8 Mar 2019 15:34:56 +1100
David Gibson <address@hidden> 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 <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).
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