Re: [PATCH v12 1/1] vhost-vdpa: add support for vIOMMU

[Eugenio Perez Martin]

On Tue, Dec 13, 2022 at 9:17 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/12/9 21:08, Cindy Lu 写道:
> > 1.Skip the check in vhost_vdpa_listener_skipped_section() while
> > MR is IOMMU, Move this check to  vhost_vdpa_iommu_map_notify()
> > 2.Add support for vIOMMU.
>
>
> So I think the changlog needs some tweak, we need to explain why you
> need to do the above.
>
>
> > Add the new function to deal with iommu MR.
> > - during iommu_region_add register a specific IOMMU notifier,
> >   and store all notifiers in a list.
> > - during iommu_region_del, compare and delete the IOMMU notifier from the 
> > list
>
>
> And try to describe how you implement it by avoid duplicating what the
> code did as below.
>
> E.g you can say you implement this through the IOMMU MAP notifier etc.
>
>
> >
> > Verified in vp_vdpa and vdpa_sim_net driver
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >   hw/virtio/vhost-vdpa.c         | 162 ++++++++++++++++++++++++++++++---
> >   include/hw/virtio/vhost-vdpa.h |  10 ++
> >   2 files changed, 161 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 7468e44b87..2b3920c2a1 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -26,6 +26,7 @@
> >   #include "cpu.h"
> >   #include "trace.h"
> >   #include "qapi/error.h"
> > +#include "hw/virtio/virtio-access.h"
>
>
> Any reason you need to use virtio accessors here?
>
>
> >
> >   /*
> >    * Return one past the end of the end of section. Be careful with uint64_t
> > @@ -60,15 +61,22 @@ static bool 
> > vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
> >                        iova_min, section->offset_within_address_space);
> >           return true;
> >       }
> > +    /*
> > +     * While using vIOMMU, Sometimes the section will be larger than 
> > iova_max
> > +     * but the memory that  actually mapping is smaller, So skip the check
> > +     * here. Will add the check in vhost_vdpa_iommu_map_notify,
> > +     *There is the real size that maps to the kernel
> > +     */
> >
> > -    llend = vhost_vdpa_section_end(section);
> > -    if (int128_gt(llend, int128_make64(iova_max))) {
> > -        error_report("RAM section out of device range (max=0x%" PRIx64
> > -                     ", end addr=0x%" PRIx64 ")",
> > -                     iova_max, int128_get64(llend));
> > -        return true;
> > +    if (!memory_region_is_iommu(section->mr)) {
> > +        llend = vhost_vdpa_section_end(section);
> > +        if (int128_gt(llend, int128_make64(iova_max))) {
> > +            error_report("RAM section out of device range (max=0x%" PRIx64
> > +                         ", end addr=0x%" PRIx64 ")",
> > +                         iova_max, int128_get64(llend));
> > +            return true;
> > +        }
> >       }
> > -
> >       return false;
> >   }
> >
> > @@ -173,6 +181,115 @@ static void vhost_vdpa_listener_commit(MemoryListener 
> > *listener)
> >       v->iotlb_batch_begin_sent = false;
> >   }
> >
> > +static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry 
> > *iotlb)
> > +{
> > +    struct vdpa_iommu *iommu = container_of(n, struct vdpa_iommu, n);
> > +
> > +    hwaddr iova = iotlb->iova + iommu->iommu_offset;
> > +    struct vhost_vdpa *v = iommu->dev;
> > +    void *vaddr;
> > +    int ret;
> > +    Int128 llend;
> > +
> > +    if (iotlb->target_as != &address_space_memory) {
> > +        error_report("Wrong target AS \"%s\", only system memory is 
> > allowed",
> > +                     iotlb->target_as->name ? iotlb->target_as->name : 
> > "none");
> > +        return;
> > +    }
> > +    RCU_READ_LOCK_GUARD();
> > +    /* check if RAM section out of device range */
> > +    llend = int128_add(int128_makes64(iotlb->addr_mask), 
> > int128_makes64(iova));
> > +    if (int128_gt(llend, int128_make64(v->iova_range.last))) {
> > +        error_report("RAM section out of device range (max=0x%" PRIx64
> > +                     ", end addr=0x%" PRIx64 ")",
> > +                     v->iova_range.last, int128_get64(llend));
> > +        return;
> > +    }
> > +
> > +    vhost_vdpa_iotlb_batch_begin_once(v);'
>
>
> Any reason we need to consider batching here? (or batching can be done
> via notifier?)
>
>
> > +
> > +    if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> > +        bool read_only;
> > +
> > +        if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL)) {
> > +            return;
> > +        }
> > +
> > +        ret = vhost_vdpa_dma_map(v, iova, iotlb->addr_mask + 1, vaddr,
> > +            read_only);
>
>
> Let's add some tracepoints for those as what is done in the
> region_add()/del().
>
>
> > +        if (ret) {
> > +            error_report("vhost_vdpa_dma_map(%p, 0x%" HWADDR_PRIx ", "
> > +                         "0x%" HWADDR_PRIx ", %p) = %d (%m)",
> > +                         v, iova, iotlb->addr_mask + 1, vaddr, ret);
> > +        }
> > +    } else {
> > +        ret = vhost_vdpa_dma_unmap(v, iova, iotlb->addr_mask + 1);
> > +        if (ret) {
> > +            error_report("vhost_vdpa_dma_unmap(%p, 0x%" HWADDR_PRIx ", "
> > +                         "0x%" HWADDR_PRIx ") = %d (%m)",
> > +                         v, iova, iotlb->addr_mask + 1, ret);
> > +        }
> > +    }
> > +}
> > +
> > +static void vhost_vdpa_iommu_region_add(MemoryListener *listener,
> > +                                        MemoryRegionSection *section)
> > +{
> > +    struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, 
> > listener);
> > +
> > +    struct vdpa_iommu *iommu;
> > +    Int128 end;
> > +    int iommu_idx;
> > +    IOMMUMemoryRegion *iommu_mr;
> > +    int ret;
> > +
> > +    iommu_mr = IOMMU_MEMORY_REGION(section->mr);
> > +
> > +    iommu = g_malloc0(sizeof(*iommu));
> > +    end = int128_add(int128_make64(section->offset_within_region),
> > +        section->size);
>
>
> Though checkpatch.pl doesn't complain, the indentation looks odd, e.g
> the 's' should be indent to below "i" of "int128" etc.
>
> You can tweak you editor to adopt Qemu coding style.
>
>
> > +    end = int128_sub(end, int128_one());
> > +    iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
> > +        MEMTXATTRS_UNSPECIFIED);
> > +    iommu->iommu_mr = iommu_mr;
> > +    iommu_notifier_init(&iommu->n, vhost_vdpa_iommu_map_notify,
> > +        IOMMU_NOTIFIER_IOTLB_EVENTS,
> > +        section->offset_within_region, int128_get64(end), iommu_idx);
> > +    iommu->iommu_offset = section->offset_within_address_space -
> > +        section->offset_within_region;
> > +    iommu->dev = v;
> > +
> > +    ret = memory_region_register_iommu_notifier(section->mr, &iommu->n, 
> > NULL);
> > +    if (ret) {
> > +        g_free(iommu);
> > +        return;
> > +    }
> > +
> > +    QLIST_INSERT_HEAD(&v->iommu_list, iommu, iommu_next);
> > +    memory_region_iommu_replay(iommu->iommu_mr, &iommu->n);
> > +
> > +    return;
> > +}
> > +
> > +static void vhost_vdpa_iommu_region_del(MemoryListener *listener,
> > +                                        MemoryRegionSection *section)
> > +{
> > +    struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, 
> > listener);
> > +
> > +    struct vdpa_iommu *iommu;
> > +
> > +    QLIST_FOREACH(iommu, &v->iommu_list, iommu_next)
> > +    {
> > +        if (MEMORY_REGION(iommu->iommu_mr) == section->mr &&
> > +            iommu->n.start == section->offset_within_region) {
> > +            memory_region_unregister_iommu_notifier(section->mr, 
> > &iommu->n);
> > +            QLIST_REMOVE(iommu, iommu_next);
> > +            g_free(iommu);
> > +            break;
> > +        }
> > +    }
> > +}
> > +
> >   static void vhost_vdpa_listener_region_add(MemoryListener *listener,
> >                                              MemoryRegionSection *section)
> >   {
> > @@ -187,6 +304,10 @@ static void 
> > vhost_vdpa_listener_region_add(MemoryListener *listener,
> >                                               v->iova_range.last)) {
> >           return;
> >       }
> > +    if (memory_region_is_iommu(section->mr)) {
> > +        vhost_vdpa_iommu_region_add(listener, section);
>
>
> Adding Eugenio.
>
> I think it need to populate iova_tree otherwise the vIOMMU may break
> shadow virtqueue (and we need to do it for region_del as well).
>

Populate the iova_tree could be the easiest and more convenient way.

Thinking out loud, does iommu offer a way to iterate through its iova
tree? This way SVQ would avoid the duplication of the entries and the
need of maintain it.

SVQ would still need some entries in its own tree (SVQ vrings etc) anyhow.

Thanks!

> E.g you can test your code with shadow virtqueue via x-svq=on.
>
> Thanks
>
>