Re: [PATCH 21/31] util: Add iova_tree_alloc

[Eugenio Perez Martin]

On Thu, Jan 27, 2022 at 9:06 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Jan 25, 2022 at 10:40:01AM +0100, Eugenio Perez Martin wrote:
> > So I think that the first step to remove complexity from the old one
> > is to remove iova_begin and iova_end.
> >
> > As Jason points out, removing iova_end is easier. It has the drawback
> > of having to traverse all the list beyond iova_end, but a well formed
> > iova tree should contain none. If the guest can manipulate it, it's
> > only hurting itself adding nodes to it.
> >
> > It's possible to extract the check for hole_right (or this in Jason's
> > proposal) as a special case too.
> >
> > But removing the iova_begin parameter is more complicated. We cannot
> > know if it's a valid hole without knowing iova_begin, and we cannot
> > resume traversing. Could we assume iova_begin will always be 0? I
> > think not, the vdpa device can return anything through syscall.
>
> Frankly I don't know what's the syscall you're talking about,

I meant VHOST_VDPA_GET_IOVA_RANGE, which allows qemu to know the valid
range of iova addresses. We get a pair of uint64_t from it, that
indicates the minimum and maximum iova address the device (or iommu)
supports.

We must allocate iova ranges within that address range, which
complicates this algorithm a little bit. Since the SVQ iova addresses
are not GPA, qemu needs extra code to be able to allocate and free
them, creating a new custom iova as.

Please let me know if you want more details or if you prefer me to
give more context in the patch message.

> but after a 2nd
> thought and after I went back and re-read your previous version more carefully
> (the one without the list) I think it seems working to me in general.  I 
> should
> have tried harder when reviewing the first time!
>

I guess I should have added more context so this particular change can
be better understood in isolation.

> I mean this one:
>
> https://lore.kernel.org/qemu-devel/20211029183525.1776416-24-eperezma@redhat.com/
>
> Though this time I have some comments on the details.
>
> Personally I like that one (probably with some amendment upon the old version)
> more than the current list-based approach.  But I'd like to know your thoughts
> too (including Jason).  I'll further comment in that thread soon.
>

Sure, I'm fine with whatever solution we choose, but I'm just running
out of ideas to simplify it. Reading your suggestions on old RFC now.

Overall I feel list-based one is both more convenient and easy to
delete when qemu raises the minimal glib version, but it adds a lot
more code.

It could add less code with this less elegant changes:
* If we just put the list entry in the DMAMap itself, although it
exposes unneeded implementation details.
* We force the iova tree either to be an allocation-based or an
insertion-based, but not both. In other words, you can only either use
iova_tree_alloc or iova_tree_insert on the same tree.

I have a few tests to check the algorithms, but they are not in the
qemu test format. I will post them so we all can understand better
what is expected from this.

Thanks!



Thanks!

> Thanks,
>
> --
> Peter Xu
>