Re: [PATCH v4 1/2] vfio: move the function vfio_get_xlat_addr() to memory.c

[Peter Xu]

On Thu, Oct 27, 2022 at 03:40:31PM +0800, Cindy Lu wrote:
> Move the function vfio_get_xlat_addr to softmmu/memory.c, and
> change the name to memory_get_xlat_addr().So we can use this
> function in other devices,such as vDPA device.
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>

Acked-by: Peter Xu <peterx@redhat.com>

Trivial nit below.

[...]

> +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> +                          ram_addr_t *ram_addr, bool *read_only,
> +                          AddressSpace *as)
> +{
> +    MemoryRegion *mr;
> +    hwaddr xlat;
> +    hwaddr len = iotlb->addr_mask + 1;
> +    bool writable = iotlb->perm & IOMMU_WO;
> +
> +    /*
> +     * The IOMMU TLB entry we have just covers translation through
> +     * this IOMMU to its immediate target.  We need to translate
> +     * it the rest of the way through to memory.
> +     */
> +    mr = address_space_translate(as, iotlb->translated_addr, &xlat, &len,
> +                                 writable, MEMTXATTRS_UNSPECIFIED);

Can "as" be anything not address_space_memory in this case?

I had a feeling that you wanted to check iotlb->target_as only by peeking
at the next patch, but that can also be checked in this function too, and
the new parameter may not be needed.  Another benefit is we can also drop
the same check in vfio_iommu_map_notify() and
vfio_iommu_map_dirty_notify():

    if (iotlb->target_as != &address_space_memory) {
        error_report("Wrong target AS \"%s\", only system memory is allowed",
                     iotlb->target_as->name ? iotlb->target_as->name : "none");
        return;
    }

Thanks,

-- 
Peter Xu