netdev - Re: Re: [PATCH v5 08/11] vduse: Implement an MMU-based IOMMU driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 Mar 2021 14:56:05 +0800
From:   Yongji Xie <xieyongji@...edance.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     "Michael S. Tsirkin" <mst@...hat.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Parav Pandit <parav@...dia.com>,
        Christoph Hellwig <hch@...radead.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        Matthew Wilcox <willy@...radead.org>, viro@...iv.linux.org.uk,
        Jens Axboe <axboe@...nel.dk>, bcrl@...ck.org,
        Jonathan Corbet <corbet@....net>,
        Mika Penttilä <mika.penttila@...tfour.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
        kvm@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: Re: [PATCH v5 08/11] vduse: Implement an MMU-based IOMMU driver

On Fri, Mar 26, 2021 at 2:16 PM Jason Wang <jasowang@...hat.com> wrote:
>
>
> 在 2021/3/26 下午1:14, Yongji Xie 写道:
>
> +     }
> +     map->bounce_page = page;
> +
> +     /* paired with vduse_domain_map_page() */
> +     smp_mb();
>
> So this is suspicious. It's better to explain like, we need make sure A
> must be done after B.
>
> OK. I see. It's used to protect this pattern:
>
>      vduse_domain_alloc_bounce_page:          vduse_domain_map_page:
>      write map->bounce_page                           write map->orig_phys
>      mb()                                                            mb()
>      read map->orig_phys                                 read map->bounce_page
>
> Make sure there will always be a path to do bouncing.
>
> Ok.
>
>
> And it looks to me the iotlb_lock is sufficnet to do the synchronization
> here. E.g any reason that you don't take it in
> vduse_domain_map_bounce_page().
>
> Yes, we can. But the performance in multi-queue cases will go down if
> we use iotlb_lock on this critical path.
>
> And what's more, is there anyway to aovid holding the spinlock during
> bouncing?
>
> Looks like we can't. In the case that multiple page faults happen on
> the same page, we should make sure the bouncing is done before any
> page fault handler returns.
>
> So it looks to me all those extra complexitiy comes from the fact that
> the bounce_page and orig_phys are set by different places so we need to
> do the bouncing in two places.
>
> I wonder how much we can gain from the "lazy" boucning in page fault.
> The buffer mapped via dma_ops from virtio driver is expected to be
> accessed by the userspace soon.  It looks to me we can do all those
> stuffs during dma_map() then things would be greatly simplified.
>
> If so, we need to allocate lots of pages from the pool reserved for
> atomic memory allocation requests.
>
> This should be fine, a lot of drivers tries to allocate pages in atomic
> context. The point is to simplify the codes to make it easy to
> determince the correctness so we can add optimization on top simply by
> benchmarking the difference.
>
> OK. I will use this way in the next version.
>
> E.g we have serveral places that accesses orig_phys:
>
> 1) map_page(), write
> 2) unmap_page(), write
> 3) page fault handler, read
>
> It's not clear to me how they were synchronized. Or if it was
> synchronzied implicitly (via iova allocator?), we'd better document it.
>
> Yes.
>
> Or simply use spinlock (which is the preferrable way I'd like to go). We
> probably don't need to worry too much about the cost of spinlock since
> iova allocater use it heavily.
>
> Actually iova allocator implements a per-CPU cache to optimize it.
>
> Thanks,
> Yongji
>
>
> Right, but have a quick glance, I guess what you meant is that usually there's no lock contention unless cpu hot-plug. This can work but the problem is that such synchornization depends on the internal implementation of IOVA allocator which is kind of fragile. I still think we should do that on our own.
>

I might miss something. Looks like we don't need any synchronization
if the page fault handler is removed as you suggested. We should not
access the same orig_phys concurrently (in map_page() and
unmap_page()) unless we free the iova before accessing.

Thanks,
Yongji