Re: [Qemu-devel] [PATCH v4 RESEND 3/3] IOMMU: enable intel_iommu map and unmap notifiers

[Aviv B.D.]

On Fri, Oct 21, 2016 at 6:57 AM, Peter Xu <address@hidden> wrote:

> On Thu, Oct 20, 2016 at 10:11:15PM +0300, Aviv B.D. wrote:
>
> [...]
>
> > > > +static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> > > > +                                           uint16_t domain_id,
> hwaddr
> > > addr,
> > > > +                                           uint8_t am)
> > > > +{
> > >
> > > The logic of this function looks strange to me.
> > >
> > > > +    IntelIOMMUNotifierNode *node;
> > > > +
> > > > +    QLIST_FOREACH(node, &(s->notifiers_list), next) {
> > > > +        VTDAddressSpace *vtd_as = node->vtd_as;
> > > > +        uint16_t vfio_domain_id;
> > > > +        int ret = vtd_get_did_dev(s, pci_bus_num(vtd_as->bus),
> > > vtd_as->devfn,
> > > > +                                  &vfio_domain_id);
> > > > +        if (!ret && domain_id == vfio_domain_id) {
> > > > +            IOMMUTLBEntry entry;
> > > > +
> > > > +            /* notify unmap */
> > > > +            if (node->notifier_flag & IOMMU_NOTIFIER_UNMAP) {
> > >
> > > First of all, if we are talking about VFIO, notifier_flag should
> > > always be MAP|UNMAP. So in that case, for newly mapped entries, looks
> > > like we will first send an UNMAP, then a MAP?
> > >
> >
> > You are correct, there is no valid reason to have notifier_flag other
> than
> > MAP|UNMAP
> > at least for VFIO.
>
> Not sure whether you know about upstream vhost work, vhost is going to
> support Intel IOMMU, in that case, it will only register to UNMAP
> notifier, not MAP.
>
> > I'm not sure if in the feature there won't be good reason to do
> otherwise,
> > so my
> > code support this scenario...
>
> My point is, I think you should not send this notify unconditionally.
> IMO the logic should be simpler here, like:
>
>     foreach (page in invalidation range) {
>         IOTLBEntry entry = m->iommu_ops.translate();
>         if (entry.perm != IOMMU_NONE && notifier_flag & NOTIIFER_MAP) {
>             /* this is map, user requested MAP flag */
>             memory_region_iommu_notify(entry);
>         } else if (entry.perm == IOMMU_NONE && notifier_flag &
>                 NOTIFIER_UNMAP) {
>             /* this is unmap, user requested UNMAP */
>             entry = ... /* setup the entry */
>             memory_region_iommu_notify(entry);
>         }
>     }
>

This was my first algorithm, but VFIO do not support remapping of mapped
page.
Before each MAP operation in VFIO one must do unmap, and therefore I'm
sending
the unmap notifications blindly before.
I can rearrange my code closer to your suggestion.


>
> This is to follow your logic. I don't know whether this is efficient
> enough, maybe good for the first version. The problem is, when you
> call translate(), you will need to go over the page every time from
> root dir. A faster way may be: provide a function to walk specific
> address range. If you are going to implement the replay logic that
> Alex/David has mentioned, maybe that will help too (walk over the
> whole 64bit range).
>
> Interesting idea, but I prefer to add it in separate patch set after this
one committed, if it's OK.


> >
> >
> > > > +                VTD_DPRINTF(GENERAL, "Remove addr 0x%"PRIx64 " mask
> %d",
> > > > +                            addr, am);
> > > > +                entry.target_as = &address_space_memory;
> > > > +                entry.iova = addr & VTD_PAGE_MASK_4K;
> > > > +                entry.translated_addr = 0;
> > > > +                entry.addr_mask = ~VTD_PAGE_MASK(VTD_PAGE_SHIFT_4K
> +
> > > am);
> > > > +                entry.perm = IOMMU_NONE;
> > > > +                memory_region_notify_iommu(&node->vtd_as->iommu,
> > > entry);
> > > > +            }
> > > > +
> > > > +            /* notify map */
> > > > +            if (node->notifier_flag & IOMMU_NOTIFIER_MAP) {
> > > > +                hwaddr original_addr = addr;
> > > > +                VTD_DPRINTF(GENERAL, "add addr 0x%"PRIx64 " mask
> %d",
> > > addr, am);
> > > > +                while (addr < original_addr + (1 << am) *
> > > VTD_PAGE_SIZE) {
> > > > +                    /* call to vtd_iommu_translate */
> > > > +                    IOMMUTLBEntry entry = s->iommu_ops.translate(
> > > > +
> > >  &node->vtd_as->iommu,
> > > > +                                                         addr,
> > > > +
>  IOMMU_NO_FAIL);
> > > > +                    if (entry.perm != IOMMU_NONE) {
> > > > +                        addr += entry.addr_mask + 1;
> > > > +                        memory_region_notify_iommu(&
> node->vtd_as->iommu,
> > > entry);
> > > > +                    } else {
> > > > +                        addr += VTD_PAGE_SIZE;
> > >
> > > IIUC, here is the point that we found "the page is gone" (so this is
> > > an UNMAP invalidation), and we should do memory_region_iommu_notify()
> > > for the whole area with IOMMU_NONE. Then we just quit the loop since
> > > continuous translate()s should fail as well if the first page is
> > > missing.
> > >
> > > Please correct if I am wrong.
> > >
> >
> > If I remember correctly I encounter a few cases where there was hole of
> > unmaped
> > memory in the middle of otherwise mapped pages. If I remember correctly
> it
> > was
> > with linux kernel 4.4, but I'm not sure.
>
> I see. Thanks.
>
> -- peterx
>

Best regards,
Aviv.