Re: [PATCH 2/6] system/physmem: IOMMU: Invoke the translate_size function if it is implemented

[Peter Xu <peterx@redhat.com>]

On Mon, Oct 30, 2023 at 02:00:54PM +0800, Ethan Chen wrote:
> On Fri, Oct 27, 2023 at 12:13:50PM -0400, Peter Xu wrote:
> > Add cc list.
> > 
> > On Fri, Oct 27, 2023 at 12:02:24PM -0400, Peter Xu wrote:
> > > On Fri, Oct 27, 2023 at 11:28:36AM +0800, Ethan Chen wrote:
> > > > On Thu, Oct 26, 2023 at 10:20:41AM -0400, Peter Xu wrote:
> > > > > Could you elaborate why is that important?  In what use case?
> > > > I was not involved in the formulation of the IOPMP specification, but I'll try
> > > > to explain my perspective. IOPMP use the same the idea as PMP. "The matching 
> > > > PMP entry must match all bytes of an access, or the access fails."
> > > > 
> > > > > 
> > > > > Consider IOVA mapped for address range iova=[0, 4K] only, here we have a
> > > > > DMA request with range=[0, 8K].  Now my understanding is what you want to
> > > > > achieve is don't trigger the DMA to [0, 4K] and fail the whole [0, 8K]
> > > > > request.
> > > > > 
> > > > > Can we just fail at the latter DMA [4K, 8K] when it happens?  After all,
> > > > > IIUC a device can split the 0-8K DMA into two smaller DMAs, then the 1st
> > > > > chunk can succeed then if it falls in 0-4K.  Some further explanation of
> > > > > the failure use case could be helpful.
> > > > 
> > > > IOPMP can only detect partially hit in an access. DMA device will split a 
> > > > large DMA transfer to small DMA transfers base on target and DMA transfer 
> > > > width, so partially hit error only happens when an access cross the boundary.
> > > > But to ensure that an access is only within one entry is still important. 
> > > > For example, an entry may mean permission of a device memory region. We do 
> > > > not want to see one DMA transfer can access mutilple devices, although DMA 
> > > > have permissions from multiple entries.
> > > 
> > > I was expecting a DMA request can be fulfilled successfully as long as the
> > > DMA translations are valid for the whole range of the request, even if the
> > > requested range may include two separate translated targets or more, each
> > > point to different places (either RAM, or other devicie's MMIO regions).
> 
> IOPMP is used to check DMA translation is vaild or not. In IOPMP specification
> , a translation access more than one entry is not invalid.
> Though it is not recommand, user can create an IOPMP entry contains mutiple
> places to make this kind translations valid.
> 
> > > 
> > > AFAIK currently QEMU memory model will automatically split that large
> > > request into two or more smaller requests, and fulfill them separately by
> > > two/more IOMMU translations, with its memory access dispatched to the
> > > specific memory regions.
> 
> Because of requests may be split, I need a method to take the original request
> information to IOPMP.

I'm not sure whether translate() is the "original request" either.  The
problem is QEMU can split the request for various reasons already, afaict.

For example, address_space_translate_internal() has this:

    if (memory_region_is_ram(mr)) {
        diff = int128_sub(section->size, int128_make64(addr));
        *plen = int128_get64(int128_min(diff, int128_make64(*plen)));
    }

Which can already shrink the request size from the caller before reaching
translate().  So the length passed into translate() can already be
modified.

Another thing is, we have two other common call sites for translate():

        memory_region_iommu_replay
        address_space_translate_for_iotlb

I'm not sure whether you've looked into them and think they don't need to
be trapped: at least memory_region_iommu_replay() looks all fine in this
regard because it always translate in min page size granule.  But I think
the restriction should apply to all translate()s.

translate_size() is weird on its own. If the only purpose is to pass the
length into translate(), another option is to add that parameter into
current translate(), allowing the implementation to ignore it.  I think
that'll be better, but even if so, I'm not 100% sure it'll always do what
you wanted as discussed above.

Thanks,

-- 
Peter Xu