On Mon, Oct 7, 2024 at 5:38 PM Jonah Palmer <jonah.pal...@oracle.com> wrote: > > > > On 10/7/24 9:51 AM, Eugenio Perez Martin wrote: > > On Fri, Oct 4, 2024 at 8:48 PM Jonah Palmer <jonah.pal...@oracle.com> wrote: > >> > >> > >> > >> On 10/4/24 11:17 AM, Eugenio Perez Martin wrote: > >>> On Fri, Oct 4, 2024 at 2:45 PM Jonah Palmer <jonah.pal...@oracle.com> > >>> wrote: > >>>> > >>>> Implements the IOVA->GPA tree for handling mapping, unmapping, and > >>>> translations for guest memory regions. > >>>> > >>>> When the guest has overlapping memory regions, an HVA to IOVA translation > >>>> may return an incorrect IOVA when searching the IOVA->HVA tree. This is > >>>> due to one HVA range being contained (overlapping) in another HVA range > >>>> in the IOVA->HVA tree. By creating an IOVA->GPA tree, we can use GPAs to > >>>> translate and find the correct IOVA for guest memory regions. > >>>> > >>> > >>> Yes, this first patch is super close to what I meant, just one issue > >>> and a pair of nits here and there. > >>> > >>> I'd leave the second patch as an optimization on top, if the numbers > >>> prove that adding the code is worth it. > >>> > >> > >> Ah okay, gotcha. I also wasn't sure if what you mentioned below on the > >> previous series you also wanted implemented or if these would also be > >> optimizations on top. > >> > >> [Adding code to the vhost_iova_tree layer for handling multiple buffers > >> returned from translation for the memory area where each iovec covers]: > >> ----------------------------------------------------------------------- > >> "Let's say that SVQ wants to translate the HVA range > >> 0xfeda0000-0xfedd0000. So it makes available for the device two > >> chained buffers: One with addr=0x1000 len=0x20000 and the other one > >> with addr=(0x20000c1000 len=0x10000). > >> > >> The VirtIO device should be able to translate these two buffers in > >> isolation and chain them. Not optimal but it helps to keep QEMU source > >> clean, as the device already must support it." > >> > > > > This is 100% in the device and QEMU is already able to split the > > buffers that way, so we don't need any change in QEMU. > > > >> [Adding a permission check to iova_tree_find_address_iterator and match > >> the range by permissions]: > >> ----------------------------------------------------------------------- > >> "About the permissions, maybe we can make the permissions to be part of > >> the lookup? Instead of returning them at iova_tree_find_iova, make > >> them match at iova_tree_find_address_iterator." > >> > > > > Ouch, I forgot this part. This is a small change we also need, can you > > add it for the next version? Thanks for remind it! > > > > Sure can! > > I apologize for my lack of understanding on this, but for example in > vhost_svq_translate_addr, how do we know what the permissions are for > the addresses we're translating? >
No need to apologize :). The calling function, vhost_svq_vring_write_descs, knows if they are RW (write=true) or RO (write=false). We need to pass the parameter to vhost_svq_translate_addr. > I'm not sure if this is always true or not, but if the address that > we're translating is backed by guest memory, then can we always say that > the permission for the mapping would be IOMMU_RW (since the OS needs to > be able to modify it)?. Likewise, for addresses backed by host-only > memory, can we always say that the permission for the mapping would be > IOMMU_RO to avoid accidental modifications by the guest? > Not really, there are parts of the guest memory that are RO and the SVQ's used ring in the host memory is RW. It should be enough to check that the write bit is enabled in the flags & is requested. > If so, this would mean that these mappings would never have the > IOMMU_NONE or IOMMU_WO permissions, right? > That's right.
