[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: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH qemu v4 3/3] spapr: Support NVIDIA V100 GPU with NVLink2 |
Date: |
Fri, 8 Mar 2019 15:34:56 +1100 |
User-agent: |
Mutt/1.11.3 (2019-02-01) |
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