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

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>
---
 hw/vfio/common.c      | 92 ++-----------------------------------------
 include/exec/memory.h |  4 ++
 softmmu/memory.c      | 84 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+), 88 deletions(-)

On Thu, Oct 27, 2022 at 3:41 PM Cindy Lu <lulu@redhat.com> 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: Jason Wang <jasowang@redhat.com>

> ---
>  hw/vfio/common.c      | 92 ++-----------------------------------------
>  include/exec/memory.h |  4 ++
>  softmmu/memory.c      | 84 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+), 88 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index ace9562a9b..2b5a9f3d8d 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -574,92 +574,6 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
>             section->offset_within_address_space & (1ULL << 63);
>  }
>
> -/* Called with rcu_read_lock held.  */
> -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> -                               ram_addr_t *ram_addr, bool *read_only)
> -{
> -    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(&address_space_memory,
> -                                 iotlb->translated_addr,
> -                                 &xlat, &len, writable,
> -                                 MEMTXATTRS_UNSPECIFIED);
> -    if (!memory_region_is_ram(mr)) {
> -        error_report("iommu map to non memory area %"HWADDR_PRIx"",
> -                     xlat);
> -        return false;
> -    } else if (memory_region_has_ram_discard_manager(mr)) {
> -        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> -        MemoryRegionSection tmp = {
> -            .mr = mr,
> -            .offset_within_region = xlat,
> -            .size = int128_make64(len),
> -        };
> -
> -        /*
> -         * Malicious VMs can map memory into the IOMMU, which is expected
> -         * to remain discarded. vfio will pin all pages, populating memory.
> -         * Disallow that. vmstate priorities make sure any RamDiscardManager
> -         * were already restored before IOMMUs are restored.
> -         */
> -        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> -            error_report("iommu map to discarded memory (e.g., unplugged via"
> -                         " virtio-mem): %"HWADDR_PRIx"",
> -                         iotlb->translated_addr);
> -            return false;
> -        }
> -
> -        /*
> -         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> -         * pages will remain pinned inside vfio until unmapped, resulting in a
> -         * higher memory consumption than expected. If memory would get
> -         * populated again later, there would be an inconsistency between pages
> -         * pinned by vfio and pages seen by QEMU. This is the case until
> -         * unmapped from the IOMMU (e.g., during device reset).
> -         *
> -         * With malicious guests, we really only care about pinning more memory
> -         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> -         * exceeded and can be used to mitigate this problem.
> -         */
> -        warn_report_once("Using vfio with vIOMMUs and coordinated discarding of"
> -                         " RAM (e.g., virtio-mem) works, however, malicious"
> -                         " guests can trigger pinning of more memory than"
> -                         " intended via an IOMMU. It's possible to mitigate "
> -                         " by setting/adjusting RLIMIT_MEMLOCK.");
> -    }
> -
> -    /*
> -     * Translation truncates length to the IOMMU page size,
> -     * check that it did not truncate too much.
> -     */
> -    if (len & iotlb->addr_mask) {
> -        error_report("iommu has granularity incompatible with target AS");
> -        return false;
> -    }
> -
> -    if (vaddr) {
> -        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> -    }
> -
> -    if (ram_addr) {
> -        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> -    }
> -
> -    if (read_only) {
> -        *read_only = !writable || mr->readonly;
> -    }
> -
> -    return true;
> -}
> -
>  static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>  {
>      VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> @@ -682,7 +596,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>      if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
>          bool read_only;
>
> -        if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
> +        if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only,
> +                                  &address_space_memory)) {
>              goto out;
>          }
>          /*
> @@ -1359,7 +1274,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>      }
>
>      rcu_read_lock();
> -    if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
> +    if (memory_get_xlat_addr(iotlb, NULL, &translated_addr, NULL,
> +                             &address_space_memory)) {
>          int ret;
>
>          ret = vfio_get_dirty_bitmap(container, iova, iotlb->addr_mask + 1,
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index bfb1de8eea..282de1d5ad 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -713,6 +713,10 @@ void ram_discard_manager_register_listener(RamDiscardManager *rdm,
>  void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
>                                               RamDiscardListener *rdl);
>
> +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> +                          ram_addr_t *ram_addr, bool *read_only,
> +                          AddressSpace *as);
> +
>  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
>  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 7ba2048836..8586863ffa 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -2121,6 +2121,90 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
>      rdmc->unregister_listener(rdm, rdl);
>  }
>
> +/* Called with rcu_read_lock held.  */
> +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);
> +    if (!memory_region_is_ram(mr)) {
> +        error_report("iommu map to non memory area %" HWADDR_PRIx "", xlat);
> +        return false;
> +    } else if (memory_region_has_ram_discard_manager(mr)) {
> +        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> +        MemoryRegionSection tmp = {
> +            .mr = mr,
> +            .offset_within_region = xlat,
> +            .size = int128_make64(len),
> +        };
> +
> +        /*
> +         * Malicious VMs can map memory into the IOMMU, which is expected
> +         * to remain discarded. device will pin all pages, populating memory.
> +         * Disallow that. vmstate priorities make sure any RamDiscardManager
> +         * were already restored before IOMMUs are restored.
> +         */
> +        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> +            error_report("iommu map to discarded memory (e.g., unplugged via"
> +                         " virtio-mem): %" HWADDR_PRIx "",
> +                         iotlb->translated_addr);
> +            return false;
> +        }
> +
> +        /*
> +         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> +         * pages will remain pinned inside device until unmapped, resulting in a
> +         * higher memory consumption than expected. If memory would get
> +         * populated again later, there would be an inconsistency between pages
> +         * pinned by device and pages seen by QEMU. This is the case until
> +         * unmapped from the IOMMU (e.g., during device reset).
> +         *
> +         * With malicious guests, we really only care about pinning more memory
> +         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> +         * exceeded and can be used to mitigate this problem.
> +         */
> +        warn_report_once("Using device with vIOMMUs and coordinated discarding"
> +                         " of RAM (e.g., virtio-mem) works, however, malicious"
> +                         " guests can trigger pinning of more memory than"
> +                         " intended via an IOMMU. It's possible to mitigate "
> +                         " by setting/adjusting RLIMIT_MEMLOCK.");
> +    }
> +
> +    /*
> +     * Translation truncates length to the IOMMU page size,
> +     * check that it did not truncate too much.
> +     */
> +    if (len & iotlb->addr_mask) {
> +        error_report("iommu has granularity incompatible with target AS");
> +        return false;
> +    }
> +
> +    if (vaddr) {
> +        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> +    }
> +
> +    if (ram_addr) {
> +        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> +    }
> +
> +    if (read_only) {
> +        *read_only = !writable || mr->readonly;
> +    }
> +
> +    return true;
> +}
> +
>  void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
>  {
>      uint8_t mask = 1 << client;
> --
> 2.34.3
>

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,

On Thu, 27 Oct 2022 15:40:31 +0800
Cindy Lu <lulu@redhat.com> 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>
> ---
>  hw/vfio/common.c      | 92 ++-----------------------------------------
>  include/exec/memory.h |  4 ++
>  softmmu/memory.c      | 84 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+), 88 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index ace9562a9b..2b5a9f3d8d 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -574,92 +574,6 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
>             section->offset_within_address_space & (1ULL << 63);
>  }
>  
> -/* Called with rcu_read_lock held.  */
> -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> -                               ram_addr_t *ram_addr, bool *read_only)
> -{
> -    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(&address_space_memory,
> -                                 iotlb->translated_addr,
> -                                 &xlat, &len, writable,
> -                                 MEMTXATTRS_UNSPECIFIED);
> -    if (!memory_region_is_ram(mr)) {
> -        error_report("iommu map to non memory area %"HWADDR_PRIx"",
> -                     xlat);
> -        return false;
> -    } else if (memory_region_has_ram_discard_manager(mr)) {
> -        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> -        MemoryRegionSection tmp = {
> -            .mr = mr,
> -            .offset_within_region = xlat,
> -            .size = int128_make64(len),
> -        };
> -
> -        /*
> -         * Malicious VMs can map memory into the IOMMU, which is expected
> -         * to remain discarded. vfio will pin all pages, populating memory.
> -         * Disallow that. vmstate priorities make sure any RamDiscardManager
> -         * were already restored before IOMMUs are restored.
> -         */
> -        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> -            error_report("iommu map to discarded memory (e.g., unplugged via"
> -                         " virtio-mem): %"HWADDR_PRIx"",
> -                         iotlb->translated_addr);
> -            return false;
> -        }
> -
> -        /*
> -         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> -         * pages will remain pinned inside vfio until unmapped, resulting in a
> -         * higher memory consumption than expected. If memory would get
> -         * populated again later, there would be an inconsistency between pages
> -         * pinned by vfio and pages seen by QEMU. This is the case until
> -         * unmapped from the IOMMU (e.g., during device reset).
> -         *
> -         * With malicious guests, we really only care about pinning more memory
> -         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> -         * exceeded and can be used to mitigate this problem.
> -         */
> -        warn_report_once("Using vfio with vIOMMUs and coordinated discarding of"
> -                         " RAM (e.g., virtio-mem) works, however, malicious"
> -                         " guests can trigger pinning of more memory than"
> -                         " intended via an IOMMU. It's possible to mitigate "
> -                         " by setting/adjusting RLIMIT_MEMLOCK.");
> -    }
> -
> -    /*
> -     * Translation truncates length to the IOMMU page size,
> -     * check that it did not truncate too much.
> -     */
> -    if (len & iotlb->addr_mask) {
> -        error_report("iommu has granularity incompatible with target AS");
> -        return false;
> -    }
> -
> -    if (vaddr) {
> -        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> -    }
> -
> -    if (ram_addr) {
> -        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> -    }
> -
> -    if (read_only) {
> -        *read_only = !writable || mr->readonly;
> -    }
> -
> -    return true;
> -}
> -
>  static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>  {
>      VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> @@ -682,7 +596,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>      if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
>          bool read_only;
>  
> -        if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
> +        if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only,
> +                                  &address_space_memory)) {
>              goto out;
>          }
>          /*
> @@ -1359,7 +1274,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>      }
>  
>      rcu_read_lock();
> -    if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
> +    if (memory_get_xlat_addr(iotlb, NULL, &translated_addr, NULL,
> +                             &address_space_memory)) {
>          int ret;
>  
>          ret = vfio_get_dirty_bitmap(container, iova, iotlb->addr_mask + 1,
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index bfb1de8eea..282de1d5ad 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -713,6 +713,10 @@ void ram_discard_manager_register_listener(RamDiscardManager *rdm,
>  void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
>                                               RamDiscardListener *rdl);
>  
> +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> +                          ram_addr_t *ram_addr, bool *read_only,
> +                          AddressSpace *as);
> +
>  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
>  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
>  
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 7ba2048836..8586863ffa 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -2121,6 +2121,90 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
>      rdmc->unregister_listener(rdm, rdl);
>  }
>  
> +/* Called with rcu_read_lock held.  */
> +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);
> +    if (!memory_region_is_ram(mr)) {
> +        error_report("iommu map to non memory area %" HWADDR_PRIx "", xlat);
> +        return false;
> +    } else if (memory_region_has_ram_discard_manager(mr)) {
> +        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> +        MemoryRegionSection tmp = {
> +            .mr = mr,
> +            .offset_within_region = xlat,
> +            .size = int128_make64(len),
> +        };
> +
> +        /*
> +         * Malicious VMs can map memory into the IOMMU, which is expected
> +         * to remain discarded. device will pin all pages, populating memory.
> +         * Disallow that. vmstate priorities make sure any RamDiscardManager
> +         * were already restored before IOMMUs are restored.
> +         */
> +        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> +            error_report("iommu map to discarded memory (e.g., unplugged via"
> +                         " virtio-mem): %" HWADDR_PRIx "",
> +                         iotlb->translated_addr);
> +            return false;
> +        }
> +
> +        /*
> +         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> +         * pages will remain pinned inside device until unmapped, resulting in a
> +         * higher memory consumption than expected. If memory would get
> +         * populated again later, there would be an inconsistency between pages
> +         * pinned by device and pages seen by QEMU. This is the case until
> +         * unmapped from the IOMMU (e.g., during device reset).
> +         *
> +         * With malicious guests, we really only care about pinning more memory
> +         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> +         * exceeded and can be used to mitigate this problem.
> +         */
> +        warn_report_once("Using device with vIOMMUs and coordinated discarding"
> +                         " of RAM (e.g., virtio-mem) works, however, malicious"
> +                         " guests can trigger pinning of more memory than"
> +                         " intended via an IOMMU. It's possible to mitigate "
> +                         " by setting/adjusting RLIMIT_MEMLOCK.");

Is this really fit to be in shared code?  Simply replacing "vfio" with
"device" for comments and warnings that are really of concern for a
specific use case doesn't look much better to me.

I think translating an unpopulated address, as in the previous test
above, is generally invalid, but the comment is certainly trying to
frame the severity of this error relative to a specific use case.

Here we're generating an unconditional warning, assuming that this code
path has been triggered by device code, for the condition of simply
asking for a translation to a MemoryRegion under discard manager
control?  Again, isn't that an action that has implications for a
specific use case of a device that supports pinning host memory?

Should the shared code be generating this warning, or could an optional
pointer arg be updated to indicate a translation to discard manager
controlled memory and this comment and warning should remain in the
caller?  Thanks,

Alex

> +    }
> +
> +    /*
> +     * Translation truncates length to the IOMMU page size,
> +     * check that it did not truncate too much.
> +     */
> +    if (len & iotlb->addr_mask) {
> +        error_report("iommu has granularity incompatible with target AS");
> +        return false;
> +    }
> +
> +    if (vaddr) {
> +        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> +    }
> +
> +    if (ram_addr) {
> +        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> +    }
> +
> +    if (read_only) {
> +        *read_only = !writable || mr->readonly;
> +    }
> +
> +    return true;
> +}
> +
>  void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
>  {
>      uint8_t mask = 1 << client;

On Fri, Oct 28, 2022 at 5:11 AM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Thu, 27 Oct 2022 15:40:31 +0800
> Cindy Lu <lulu@redhat.com> 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>
> > ---
> >  hw/vfio/common.c      | 92 ++-----------------------------------------
> >  include/exec/memory.h |  4 ++
> >  softmmu/memory.c      | 84 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 92 insertions(+), 88 deletions(-)
> >
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index ace9562a9b..2b5a9f3d8d 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -574,92 +574,6 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> >             section->offset_within_address_space & (1ULL << 63);
> >  }
> >
> > -/* Called with rcu_read_lock held.  */
> > -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > -                               ram_addr_t *ram_addr, bool *read_only)
> > -{
> > -    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(&address_space_memory,
> > -                                 iotlb->translated_addr,
> > -                                 &xlat, &len, writable,
> > -                                 MEMTXATTRS_UNSPECIFIED);
> > -    if (!memory_region_is_ram(mr)) {
> > -        error_report("iommu map to non memory area %"HWADDR_PRIx"",
> > -                     xlat);
> > -        return false;
> > -    } else if (memory_region_has_ram_discard_manager(mr)) {
> > -        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> > -        MemoryRegionSection tmp = {
> > -            .mr = mr,
> > -            .offset_within_region = xlat,
> > -            .size = int128_make64(len),
> > -        };
> > -
> > -        /*
> > -         * Malicious VMs can map memory into the IOMMU, which is expected
> > -         * to remain discarded. vfio will pin all pages, populating memory.
> > -         * Disallow that. vmstate priorities make sure any RamDiscardManager
> > -         * were already restored before IOMMUs are restored.
> > -         */
> > -        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> > -            error_report("iommu map to discarded memory (e.g., unplugged via"
> > -                         " virtio-mem): %"HWADDR_PRIx"",
> > -                         iotlb->translated_addr);
> > -            return false;
> > -        }
> > -
> > -        /*
> > -         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> > -         * pages will remain pinned inside vfio until unmapped, resulting in a
> > -         * higher memory consumption than expected. If memory would get
> > -         * populated again later, there would be an inconsistency between pages
> > -         * pinned by vfio and pages seen by QEMU. This is the case until
> > -         * unmapped from the IOMMU (e.g., during device reset).
> > -         *
> > -         * With malicious guests, we really only care about pinning more memory
> > -         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> > -         * exceeded and can be used to mitigate this problem.
> > -         */
> > -        warn_report_once("Using vfio with vIOMMUs and coordinated discarding of"
> > -                         " RAM (e.g., virtio-mem) works, however, malicious"
> > -                         " guests can trigger pinning of more memory than"
> > -                         " intended via an IOMMU. It's possible to mitigate "
> > -                         " by setting/adjusting RLIMIT_MEMLOCK.");
> > -    }
> > -
> > -    /*
> > -     * Translation truncates length to the IOMMU page size,
> > -     * check that it did not truncate too much.
> > -     */
> > -    if (len & iotlb->addr_mask) {
> > -        error_report("iommu has granularity incompatible with target AS");
> > -        return false;
> > -    }
> > -
> > -    if (vaddr) {
> > -        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > -    }
> > -
> > -    if (ram_addr) {
> > -        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> > -    }
> > -
> > -    if (read_only) {
> > -        *read_only = !writable || mr->readonly;
> > -    }
> > -
> > -    return true;
> > -}
> > -
> >  static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> >  {
> >      VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> > @@ -682,7 +596,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> >      if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> >          bool read_only;
> >
> > -        if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
> > +        if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only,
> > +                                  &address_space_memory)) {
> >              goto out;
> >          }
> >          /*
> > @@ -1359,7 +1274,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> >      }
> >
> >      rcu_read_lock();
> > -    if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
> > +    if (memory_get_xlat_addr(iotlb, NULL, &translated_addr, NULL,
> > +                             &address_space_memory)) {
> >          int ret;
> >
> >          ret = vfio_get_dirty_bitmap(container, iova, iotlb->addr_mask + 1,
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index bfb1de8eea..282de1d5ad 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -713,6 +713,10 @@ void ram_discard_manager_register_listener(RamDiscardManager *rdm,
> >  void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> >                                               RamDiscardListener *rdl);
> >
> > +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > +                          ram_addr_t *ram_addr, bool *read_only,
> > +                          AddressSpace *as);
> > +
> >  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> >  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> >
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index 7ba2048836..8586863ffa 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -2121,6 +2121,90 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> >      rdmc->unregister_listener(rdm, rdl);
> >  }
> >
> > +/* Called with rcu_read_lock held.  */
> > +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);
> > +    if (!memory_region_is_ram(mr)) {
> > +        error_report("iommu map to non memory area %" HWADDR_PRIx "", xlat);
> > +        return false;
> > +    } else if (memory_region_has_ram_discard_manager(mr)) {
> > +        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> > +        MemoryRegionSection tmp = {
> > +            .mr = mr,
> > +            .offset_within_region = xlat,
> > +            .size = int128_make64(len),
> > +        };
> > +
> > +        /*
> > +         * Malicious VMs can map memory into the IOMMU, which is expected
> > +         * to remain discarded. device will pin all pages, populating memory.
> > +         * Disallow that. vmstate priorities make sure any RamDiscardManager
> > +         * were already restored before IOMMUs are restored.
> > +         */
> > +        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> > +            error_report("iommu map to discarded memory (e.g., unplugged via"
> > +                         " virtio-mem): %" HWADDR_PRIx "",
> > +                         iotlb->translated_addr);
> > +            return false;
> > +        }
> > +
> > +        /*
> > +         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> > +         * pages will remain pinned inside device until unmapped, resulting in a
> > +         * higher memory consumption than expected. If memory would get
> > +         * populated again later, there would be an inconsistency between pages
> > +         * pinned by device and pages seen by QEMU. This is the case until
> > +         * unmapped from the IOMMU (e.g., during device reset).
> > +         *
> > +         * With malicious guests, we really only care about pinning more memory
> > +         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> > +         * exceeded and can be used to mitigate this problem.
> > +         */
> > +        warn_report_once("Using device with vIOMMUs and coordinated discarding"
> > +                         " of RAM (e.g., virtio-mem) works, however, malicious"
> > +                         " guests can trigger pinning of more memory than"
> > +                         " intended via an IOMMU. It's possible to mitigate "
> > +                         " by setting/adjusting RLIMIT_MEMLOCK.");
>
> Is this really fit to be in shared code?  Simply replacing "vfio" with
> "device" for comments and warnings that are really of concern for a
> specific use case doesn't look much better to me.
>
> I think translating an unpopulated address, as in the previous test
> above, is generally invalid, but the comment is certainly trying to
> frame the severity of this error relative to a specific use case.
>
> Here we're generating an unconditional warning, assuming that this code
> path has been triggered by device code, for the condition of simply
> asking for a translation to a MemoryRegion under discard manager
> control?  Again, isn't that an action that has implications for a
> specific use case of a device that supports pinning host memory?


Or can we rename the function to memory_get_xlat_addr_no_discard()?
This looks more general and fit for the caller that doesn't want to
map region that has a discard manager.

>
> Should the shared code be generating this warning, or could an optional
> pointer arg be updated to indicate a translation to discard manager
> controlled memory and this comment and warning should remain in the
> caller?  Thanks,

I think this should also work.

Thanks

>
> Alex
>
> > +    }
> > +
> > +    /*
> > +     * Translation truncates length to the IOMMU page size,
> > +     * check that it did not truncate too much.
> > +     */
> > +    if (len & iotlb->addr_mask) {
> > +        error_report("iommu has granularity incompatible with target AS");
> > +        return false;
> > +    }
> > +
> > +    if (vaddr) {
> > +        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > +    }
> > +
> > +    if (ram_addr) {
> > +        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> > +    }
> > +
> > +    if (read_only) {
> > +        *read_only = !writable || mr->readonly;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >  void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
> >  {
> >      uint8_t mask = 1 << client;
>

On Fri, 28 Oct 2022 09:50:10 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On Fri, Oct 28, 2022 at 5:11 AM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Thu, 27 Oct 2022 15:40:31 +0800
> > Cindy Lu <lulu@redhat.com> 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>
> > > ---
> > >  hw/vfio/common.c      | 92 ++-----------------------------------------
> > >  include/exec/memory.h |  4 ++
> > >  softmmu/memory.c      | 84 +++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 92 insertions(+), 88 deletions(-)
> > >
> > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > index ace9562a9b..2b5a9f3d8d 100644
> > > --- a/hw/vfio/common.c
> > > +++ b/hw/vfio/common.c
> > > @@ -574,92 +574,6 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> > >             section->offset_within_address_space & (1ULL << 63);
> > >  }
> > >
> > > -/* Called with rcu_read_lock held.  */
> > > -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > > -                               ram_addr_t *ram_addr, bool *read_only)
> > > -{
> > > -    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(&address_space_memory,
> > > -                                 iotlb->translated_addr,
> > > -                                 &xlat, &len, writable,
> > > -                                 MEMTXATTRS_UNSPECIFIED);
> > > -    if (!memory_region_is_ram(mr)) {
> > > -        error_report("iommu map to non memory area %"HWADDR_PRIx"",
> > > -                     xlat);
> > > -        return false;
> > > -    } else if (memory_region_has_ram_discard_manager(mr)) {
> > > -        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> > > -        MemoryRegionSection tmp = {
> > > -            .mr = mr,
> > > -            .offset_within_region = xlat,
> > > -            .size = int128_make64(len),
> > > -        };
> > > -
> > > -        /*
> > > -         * Malicious VMs can map memory into the IOMMU, which is expected
> > > -         * to remain discarded. vfio will pin all pages, populating memory.
> > > -         * Disallow that. vmstate priorities make sure any RamDiscardManager
> > > -         * were already restored before IOMMUs are restored.
> > > -         */
> > > -        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> > > -            error_report("iommu map to discarded memory (e.g., unplugged via"
> > > -                         " virtio-mem): %"HWADDR_PRIx"",
> > > -                         iotlb->translated_addr);
> > > -            return false;
> > > -        }
> > > -
> > > -        /*
> > > -         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> > > -         * pages will remain pinned inside vfio until unmapped, resulting in a
> > > -         * higher memory consumption than expected. If memory would get
> > > -         * populated again later, there would be an inconsistency between pages
> > > -         * pinned by vfio and pages seen by QEMU. This is the case until
> > > -         * unmapped from the IOMMU (e.g., during device reset).
> > > -         *
> > > -         * With malicious guests, we really only care about pinning more memory
> > > -         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> > > -         * exceeded and can be used to mitigate this problem.
> > > -         */
> > > -        warn_report_once("Using vfio with vIOMMUs and coordinated discarding of"
> > > -                         " RAM (e.g., virtio-mem) works, however, malicious"
> > > -                         " guests can trigger pinning of more memory than"
> > > -                         " intended via an IOMMU. It's possible to mitigate "
> > > -                         " by setting/adjusting RLIMIT_MEMLOCK.");
> > > -    }
> > > -
> > > -    /*
> > > -     * Translation truncates length to the IOMMU page size,
> > > -     * check that it did not truncate too much.
> > > -     */
> > > -    if (len & iotlb->addr_mask) {
> > > -        error_report("iommu has granularity incompatible with target AS");
> > > -        return false;
> > > -    }
> > > -
> > > -    if (vaddr) {
> > > -        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > > -    }
> > > -
> > > -    if (ram_addr) {
> > > -        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> > > -    }
> > > -
> > > -    if (read_only) {
> > > -        *read_only = !writable || mr->readonly;
> > > -    }
> > > -
> > > -    return true;
> > > -}
> > > -
> > >  static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > >  {
> > >      VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> > > @@ -682,7 +596,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > >      if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> > >          bool read_only;
> > >
> > > -        if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
> > > +        if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only,
> > > +                                  &address_space_memory)) {
> > >              goto out;
> > >          }
> > >          /*
> > > @@ -1359,7 +1274,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > >      }
> > >
> > >      rcu_read_lock();
> > > -    if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
> > > +    if (memory_get_xlat_addr(iotlb, NULL, &translated_addr, NULL,
> > > +                             &address_space_memory)) {
> > >          int ret;
> > >
> > >          ret = vfio_get_dirty_bitmap(container, iova, iotlb->addr_mask + 1,
> > > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > > index bfb1de8eea..282de1d5ad 100644
> > > --- a/include/exec/memory.h
> > > +++ b/include/exec/memory.h
> > > @@ -713,6 +713,10 @@ void ram_discard_manager_register_listener(RamDiscardManager *rdm,
> > >  void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> > >                                               RamDiscardListener *rdl);
> > >
> > > +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > > +                          ram_addr_t *ram_addr, bool *read_only,
> > > +                          AddressSpace *as);
> > > +
> > >  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> > >  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> > >
> > > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > > index 7ba2048836..8586863ffa 100644
> > > --- a/softmmu/memory.c
> > > +++ b/softmmu/memory.c
> > > @@ -2121,6 +2121,90 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> > >      rdmc->unregister_listener(rdm, rdl);
> > >  }
> > >
> > > +/* Called with rcu_read_lock held.  */
> > > +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);
> > > +    if (!memory_region_is_ram(mr)) {
> > > +        error_report("iommu map to non memory area %" HWADDR_PRIx "", xlat);
> > > +        return false;
> > > +    } else if (memory_region_has_ram_discard_manager(mr)) {
> > > +        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> > > +        MemoryRegionSection tmp = {
> > > +            .mr = mr,
> > > +            .offset_within_region = xlat,
> > > +            .size = int128_make64(len),
> > > +        };
> > > +
> > > +        /*
> > > +         * Malicious VMs can map memory into the IOMMU, which is expected
> > > +         * to remain discarded. device will pin all pages, populating memory.
> > > +         * Disallow that. vmstate priorities make sure any RamDiscardManager
> > > +         * were already restored before IOMMUs are restored.
> > > +         */
> > > +        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> > > +            error_report("iommu map to discarded memory (e.g., unplugged via"
> > > +                         " virtio-mem): %" HWADDR_PRIx "",
> > > +                         iotlb->translated_addr);
> > > +            return false;
> > > +        }
> > > +
> > > +        /*
> > > +         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> > > +         * pages will remain pinned inside device until unmapped, resulting in a
> > > +         * higher memory consumption than expected. If memory would get
> > > +         * populated again later, there would be an inconsistency between pages
> > > +         * pinned by device and pages seen by QEMU. This is the case until
> > > +         * unmapped from the IOMMU (e.g., during device reset).
> > > +         *
> > > +         * With malicious guests, we really only care about pinning more memory
> > > +         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> > > +         * exceeded and can be used to mitigate this problem.
> > > +         */
> > > +        warn_report_once("Using device with vIOMMUs and coordinated discarding"
> > > +                         " of RAM (e.g., virtio-mem) works, however, malicious"
> > > +                         " guests can trigger pinning of more memory than"
> > > +                         " intended via an IOMMU. It's possible to mitigate "
> > > +                         " by setting/adjusting RLIMIT_MEMLOCK.");  
> >
> > Is this really fit to be in shared code?  Simply replacing "vfio" with
> > "device" for comments and warnings that are really of concern for a
> > specific use case doesn't look much better to me.
> >
> > I think translating an unpopulated address, as in the previous test
> > above, is generally invalid, but the comment is certainly trying to
> > frame the severity of this error relative to a specific use case.
> >
> > Here we're generating an unconditional warning, assuming that this code
> > path has been triggered by device code, for the condition of simply
> > asking for a translation to a MemoryRegion under discard manager
> > control?  Again, isn't that an action that has implications for a
> > specific use case of a device that supports pinning host memory?  
> 
> 
> Or can we rename the function to memory_get_xlat_addr_no_discard()?
> This looks more general and fit for the caller that doesn't want to
> map region that has a discard manager.

Is a guest restricted from mapping virtio-mem regions to a device?
AFAIK, this is something that a guest can do and we can't restrict them
from doing it, it's just that it opens some potential for malicious
activity that we rely on things like locked memory limits to keep from
getting out of hand.  Thanks,

Alex

> > Should the shared code be generating this warning, or could an optional
> > pointer arg be updated to indicate a translation to discard manager
> > controlled memory and this comment and warning should remain in the
> > caller?  Thanks,  
> 
> I think this should also work.
> 
> Thanks
> 
> >
> > Alex
> >  
> > > +    }
> > > +
> > > +    /*
> > > +     * Translation truncates length to the IOMMU page size,
> > > +     * check that it did not truncate too much.
> > > +     */
> > > +    if (len & iotlb->addr_mask) {
> > > +        error_report("iommu has granularity incompatible with target AS");
> > > +        return false;
> > > +    }
> > > +
> > > +    if (vaddr) {
> > > +        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > > +    }
> > > +
> > > +    if (ram_addr) {
> > > +        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> > > +    }
> > > +
> > > +    if (read_only) {
> > > +        *read_only = !writable || mr->readonly;
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > > +
> > >  void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
> > >  {
> > >      uint8_t mask = 1 << client;  
> >  
>

On Fri, Oct 28, 2022 at 10:08 AM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Fri, 28 Oct 2022 09:50:10 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
> > On Fri, Oct 28, 2022 at 5:11 AM Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> > >
> > > On Thu, 27 Oct 2022 15:40:31 +0800
> > > Cindy Lu <lulu@redhat.com> 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>
> > > > ---
> > > >  hw/vfio/common.c      | 92 ++-----------------------------------------
> > > >  include/exec/memory.h |  4 ++
> > > >  softmmu/memory.c      | 84 +++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 92 insertions(+), 88 deletions(-)
> > > >
> > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > > index ace9562a9b..2b5a9f3d8d 100644
> > > > --- a/hw/vfio/common.c
> > > > +++ b/hw/vfio/common.c
> > > > @@ -574,92 +574,6 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> > > >             section->offset_within_address_space & (1ULL << 63);
> > > >  }
> > > >
> > > > -/* Called with rcu_read_lock held.  */
> > > > -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > > > -                               ram_addr_t *ram_addr, bool *read_only)
> > > > -{
> > > > -    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(&address_space_memory,
> > > > -                                 iotlb->translated_addr,
> > > > -                                 &xlat, &len, writable,
> > > > -                                 MEMTXATTRS_UNSPECIFIED);
> > > > -    if (!memory_region_is_ram(mr)) {
> > > > -        error_report("iommu map to non memory area %"HWADDR_PRIx"",
> > > > -                     xlat);
> > > > -        return false;
> > > > -    } else if (memory_region_has_ram_discard_manager(mr)) {
> > > > -        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> > > > -        MemoryRegionSection tmp = {
> > > > -            .mr = mr,
> > > > -            .offset_within_region = xlat,
> > > > -            .size = int128_make64(len),
> > > > -        };
> > > > -
> > > > -        /*
> > > > -         * Malicious VMs can map memory into the IOMMU, which is expected
> > > > -         * to remain discarded. vfio will pin all pages, populating memory.
> > > > -         * Disallow that. vmstate priorities make sure any RamDiscardManager
> > > > -         * were already restored before IOMMUs are restored.
> > > > -         */
> > > > -        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> > > > -            error_report("iommu map to discarded memory (e.g., unplugged via"
> > > > -                         " virtio-mem): %"HWADDR_PRIx"",
> > > > -                         iotlb->translated_addr);
> > > > -            return false;
> > > > -        }
> > > > -
> > > > -        /*
> > > > -         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> > > > -         * pages will remain pinned inside vfio until unmapped, resulting in a
> > > > -         * higher memory consumption than expected. If memory would get
> > > > -         * populated again later, there would be an inconsistency between pages
> > > > -         * pinned by vfio and pages seen by QEMU. This is the case until
> > > > -         * unmapped from the IOMMU (e.g., during device reset).
> > > > -         *
> > > > -         * With malicious guests, we really only care about pinning more memory
> > > > -         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> > > > -         * exceeded and can be used to mitigate this problem.
> > > > -         */
> > > > -        warn_report_once("Using vfio with vIOMMUs and coordinated discarding of"
> > > > -                         " RAM (e.g., virtio-mem) works, however, malicious"
> > > > -                         " guests can trigger pinning of more memory than"
> > > > -                         " intended via an IOMMU. It's possible to mitigate "
> > > > -                         " by setting/adjusting RLIMIT_MEMLOCK.");
> > > > -    }
> > > > -
> > > > -    /*
> > > > -     * Translation truncates length to the IOMMU page size,
> > > > -     * check that it did not truncate too much.
> > > > -     */
> > > > -    if (len & iotlb->addr_mask) {
> > > > -        error_report("iommu has granularity incompatible with target AS");
> > > > -        return false;
> > > > -    }
> > > > -
> > > > -    if (vaddr) {
> > > > -        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > > > -    }
> > > > -
> > > > -    if (ram_addr) {
> > > > -        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> > > > -    }
> > > > -
> > > > -    if (read_only) {
> > > > -        *read_only = !writable || mr->readonly;
> > > > -    }
> > > > -
> > > > -    return true;
> > > > -}
> > > > -
> > > >  static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > > >  {
> > > >      VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> > > > @@ -682,7 +596,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > > >      if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> > > >          bool read_only;
> > > >
> > > > -        if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
> > > > +        if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only,
> > > > +                                  &address_space_memory)) {
> > > >              goto out;
> > > >          }
> > > >          /*
> > > > @@ -1359,7 +1274,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > > >      }
> > > >
> > > >      rcu_read_lock();
> > > > -    if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
> > > > +    if (memory_get_xlat_addr(iotlb, NULL, &translated_addr, NULL,
> > > > +                             &address_space_memory)) {
> > > >          int ret;
> > > >
> > > >          ret = vfio_get_dirty_bitmap(container, iova, iotlb->addr_mask + 1,
> > > > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > > > index bfb1de8eea..282de1d5ad 100644
> > > > --- a/include/exec/memory.h
> > > > +++ b/include/exec/memory.h
> > > > @@ -713,6 +713,10 @@ void ram_discard_manager_register_listener(RamDiscardManager *rdm,
> > > >  void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> > > >                                               RamDiscardListener *rdl);
> > > >
> > > > +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > > > +                          ram_addr_t *ram_addr, bool *read_only,
> > > > +                          AddressSpace *as);
> > > > +
> > > >  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> > > >  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> > > >
> > > > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > > > index 7ba2048836..8586863ffa 100644
> > > > --- a/softmmu/memory.c
> > > > +++ b/softmmu/memory.c
> > > > @@ -2121,6 +2121,90 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> > > >      rdmc->unregister_listener(rdm, rdl);
> > > >  }
> > > >
> > > > +/* Called with rcu_read_lock held.  */
> > > > +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);
> > > > +    if (!memory_region_is_ram(mr)) {
> > > > +        error_report("iommu map to non memory area %" HWADDR_PRIx "", xlat);
> > > > +        return false;
> > > > +    } else if (memory_region_has_ram_discard_manager(mr)) {
> > > > +        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> > > > +        MemoryRegionSection tmp = {
> > > > +            .mr = mr,
> > > > +            .offset_within_region = xlat,
> > > > +            .size = int128_make64(len),
> > > > +        };
> > > > +
> > > > +        /*
> > > > +         * Malicious VMs can map memory into the IOMMU, which is expected
> > > > +         * to remain discarded. device will pin all pages, populating memory.
> > > > +         * Disallow that. vmstate priorities make sure any RamDiscardManager
> > > > +         * were already restored before IOMMUs are restored.
> > > > +         */
> > > > +        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> > > > +            error_report("iommu map to discarded memory (e.g., unplugged via"
> > > > +                         " virtio-mem): %" HWADDR_PRIx "",
> > > > +                         iotlb->translated_addr);
> > > > +            return false;
> > > > +        }
> > > > +
> > > > +        /*
> > > > +         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> > > > +         * pages will remain pinned inside device until unmapped, resulting in a
> > > > +         * higher memory consumption than expected. If memory would get
> > > > +         * populated again later, there would be an inconsistency between pages
> > > > +         * pinned by device and pages seen by QEMU. This is the case until
> > > > +         * unmapped from the IOMMU (e.g., during device reset).
> > > > +         *
> > > > +         * With malicious guests, we really only care about pinning more memory
> > > > +         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> > > > +         * exceeded and can be used to mitigate this problem.
> > > > +         */
> > > > +        warn_report_once("Using device with vIOMMUs and coordinated discarding"
> > > > +                         " of RAM (e.g., virtio-mem) works, however, malicious"
> > > > +                         " guests can trigger pinning of more memory than"
> > > > +                         " intended via an IOMMU. It's possible to mitigate "
> > > > +                         " by setting/adjusting RLIMIT_MEMLOCK.");
> > >
> > > Is this really fit to be in shared code?  Simply replacing "vfio" with
> > > "device" for comments and warnings that are really of concern for a
> > > specific use case doesn't look much better to me.
> > >
> > > I think translating an unpopulated address, as in the previous test
> > > above, is generally invalid, but the comment is certainly trying to
> > > frame the severity of this error relative to a specific use case.
> > >
> > > Here we're generating an unconditional warning, assuming that this code
> > > path has been triggered by device code, for the condition of simply
> > > asking for a translation to a MemoryRegion under discard manager
> > > control?  Again, isn't that an action that has implications for a
> > > specific use case of a device that supports pinning host memory?
> >
> >
> > Or can we rename the function to memory_get_xlat_addr_no_discard()?
> > This looks more general and fit for the caller that doesn't want to
> > map region that has a discard manager.
>
> Is a guest restricted from mapping virtio-mem regions to a device?

For the region that is not populated, it should be restricted. If this
is wrong, we need a separate fix.

> AFAIK, this is something that a guest can do and we can't restrict them
> from doing it, it's just that it opens some potential for malicious
> activity that we rely on things like locked memory limits to keep from
> getting out of hand.  Thanks,

So it's the fault of the name, it could be
memory_get_xlat_addr_no_unpopulated_discard().

Or as I replied, stick to what you've suggested.

Thanks

>
> Alex
>
> > > Should the shared code be generating this warning, or could an optional
> > > pointer arg be updated to indicate a translation to discard manager
> > > controlled memory and this comment and warning should remain in the
> > > caller?  Thanks,
> >
> > I think this should also work.
> >
> > Thanks
> >
> > >
> > > Alex
> > >
> > > > +    }
> > > > +
> > > > +    /*
> > > > +     * Translation truncates length to the IOMMU page size,
> > > > +     * check that it did not truncate too much.
> > > > +     */
> > > > +    if (len & iotlb->addr_mask) {
> > > > +        error_report("iommu has granularity incompatible with target AS");
> > > > +        return false;
> > > > +    }
> > > > +
> > > > +    if (vaddr) {
> > > > +        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > > > +    }
> > > > +
> > > > +    if (ram_addr) {
> > > > +        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> > > > +    }
> > > > +
> > > > +    if (read_only) {
> > > > +        *read_only = !writable || mr->readonly;
> > > > +    }
> > > > +
> > > > +    return true;
> > > > +}
> > > > +
> > > >  void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
> > > >  {
> > > >      uint8_t mask = 1 << client;
> > >
> >
>

On Fri, 28 Oct 2022 10:23:16 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On Fri, Oct 28, 2022 at 10:08 AM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Fri, 28 Oct 2022 09:50:10 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >  
> > > On Fri, Oct 28, 2022 at 5:11 AM Alex Williamson
> > > <alex.williamson@redhat.com> wrote:  
> > > >
> > > > On Thu, 27 Oct 2022 15:40:31 +0800
> > > > Cindy Lu <lulu@redhat.com> 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>
> > > > > ---
> > > > >  hw/vfio/common.c      | 92 ++-----------------------------------------
> > > > >  include/exec/memory.h |  4 ++
> > > > >  softmmu/memory.c      | 84 +++++++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 92 insertions(+), 88 deletions(-)
> > > > >
> > > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > > > index ace9562a9b..2b5a9f3d8d 100644
> > > > > --- a/hw/vfio/common.c
> > > > > +++ b/hw/vfio/common.c
> > > > > @@ -574,92 +574,6 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> > > > >             section->offset_within_address_space & (1ULL << 63);
> > > > >  }
> > > > >
> > > > > -/* Called with rcu_read_lock held.  */
> > > > > -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > > > > -                               ram_addr_t *ram_addr, bool *read_only)
> > > > > -{
> > > > > -    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(&address_space_memory,
> > > > > -                                 iotlb->translated_addr,
> > > > > -                                 &xlat, &len, writable,
> > > > > -                                 MEMTXATTRS_UNSPECIFIED);
> > > > > -    if (!memory_region_is_ram(mr)) {
> > > > > -        error_report("iommu map to non memory area %"HWADDR_PRIx"",
> > > > > -                     xlat);
> > > > > -        return false;
> > > > > -    } else if (memory_region_has_ram_discard_manager(mr)) {
> > > > > -        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> > > > > -        MemoryRegionSection tmp = {
> > > > > -            .mr = mr,
> > > > > -            .offset_within_region = xlat,
> > > > > -            .size = int128_make64(len),
> > > > > -        };
> > > > > -
> > > > > -        /*
> > > > > -         * Malicious VMs can map memory into the IOMMU, which is expected
> > > > > -         * to remain discarded. vfio will pin all pages, populating memory.
> > > > > -         * Disallow that. vmstate priorities make sure any RamDiscardManager
> > > > > -         * were already restored before IOMMUs are restored.
> > > > > -         */
> > > > > -        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> > > > > -            error_report("iommu map to discarded memory (e.g., unplugged via"
> > > > > -                         " virtio-mem): %"HWADDR_PRIx"",
> > > > > -                         iotlb->translated_addr);
> > > > > -            return false;
> > > > > -        }
> > > > > -
> > > > > -        /*
> > > > > -         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> > > > > -         * pages will remain pinned inside vfio until unmapped, resulting in a
> > > > > -         * higher memory consumption than expected. If memory would get
> > > > > -         * populated again later, there would be an inconsistency between pages
> > > > > -         * pinned by vfio and pages seen by QEMU. This is the case until
> > > > > -         * unmapped from the IOMMU (e.g., during device reset).
> > > > > -         *
> > > > > -         * With malicious guests, we really only care about pinning more memory
> > > > > -         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> > > > > -         * exceeded and can be used to mitigate this problem.
> > > > > -         */
> > > > > -        warn_report_once("Using vfio with vIOMMUs and coordinated discarding of"
> > > > > -                         " RAM (e.g., virtio-mem) works, however, malicious"
> > > > > -                         " guests can trigger pinning of more memory than"
> > > > > -                         " intended via an IOMMU. It's possible to mitigate "
> > > > > -                         " by setting/adjusting RLIMIT_MEMLOCK.");
> > > > > -    }
> > > > > -
> > > > > -    /*
> > > > > -     * Translation truncates length to the IOMMU page size,
> > > > > -     * check that it did not truncate too much.
> > > > > -     */
> > > > > -    if (len & iotlb->addr_mask) {
> > > > > -        error_report("iommu has granularity incompatible with target AS");
> > > > > -        return false;
> > > > > -    }
> > > > > -
> > > > > -    if (vaddr) {
> > > > > -        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > > > > -    }
> > > > > -
> > > > > -    if (ram_addr) {
> > > > > -        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> > > > > -    }
> > > > > -
> > > > > -    if (read_only) {
> > > > > -        *read_only = !writable || mr->readonly;
> > > > > -    }
> > > > > -
> > > > > -    return true;
> > > > > -}
> > > > > -
> > > > >  static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > > > >  {
> > > > >      VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> > > > > @@ -682,7 +596,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > > > >      if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> > > > >          bool read_only;
> > > > >
> > > > > -        if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
> > > > > +        if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only,
> > > > > +                                  &address_space_memory)) {
> > > > >              goto out;
> > > > >          }
> > > > >          /*
> > > > > @@ -1359,7 +1274,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > > > >      }
> > > > >
> > > > >      rcu_read_lock();
> > > > > -    if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
> > > > > +    if (memory_get_xlat_addr(iotlb, NULL, &translated_addr, NULL,
> > > > > +                             &address_space_memory)) {
> > > > >          int ret;
> > > > >
> > > > >          ret = vfio_get_dirty_bitmap(container, iova, iotlb->addr_mask + 1,
> > > > > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > > > > index bfb1de8eea..282de1d5ad 100644
> > > > > --- a/include/exec/memory.h
> > > > > +++ b/include/exec/memory.h
> > > > > @@ -713,6 +713,10 @@ void ram_discard_manager_register_listener(RamDiscardManager *rdm,
> > > > >  void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> > > > >                                               RamDiscardListener *rdl);
> > > > >
> > > > > +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > > > > +                          ram_addr_t *ram_addr, bool *read_only,
> > > > > +                          AddressSpace *as);
> > > > > +
> > > > >  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> > > > >  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> > > > >
> > > > > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > > > > index 7ba2048836..8586863ffa 100644
> > > > > --- a/softmmu/memory.c
> > > > > +++ b/softmmu/memory.c
> > > > > @@ -2121,6 +2121,90 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> > > > >      rdmc->unregister_listener(rdm, rdl);
> > > > >  }
> > > > >
> > > > > +/* Called with rcu_read_lock held.  */
> > > > > +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);
> > > > > +    if (!memory_region_is_ram(mr)) {
> > > > > +        error_report("iommu map to non memory area %" HWADDR_PRIx "", xlat);
> > > > > +        return false;
> > > > > +    } else if (memory_region_has_ram_discard_manager(mr)) {
> > > > > +        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> > > > > +        MemoryRegionSection tmp = {
> > > > > +            .mr = mr,
> > > > > +            .offset_within_region = xlat,
> > > > > +            .size = int128_make64(len),
> > > > > +        };
> > > > > +
> > > > > +        /*
> > > > > +         * Malicious VMs can map memory into the IOMMU, which is expected
> > > > > +         * to remain discarded. device will pin all pages, populating memory.
> > > > > +         * Disallow that. vmstate priorities make sure any RamDiscardManager
> > > > > +         * were already restored before IOMMUs are restored.
> > > > > +         */
> > > > > +        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> > > > > +            error_report("iommu map to discarded memory (e.g., unplugged via"
> > > > > +                         " virtio-mem): %" HWADDR_PRIx "",
> > > > > +                         iotlb->translated_addr);
> > > > > +            return false;
> > > > > +        }
> > > > > +
> > > > > +        /*
> > > > > +         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> > > > > +         * pages will remain pinned inside device until unmapped, resulting in a
> > > > > +         * higher memory consumption than expected. If memory would get
> > > > > +         * populated again later, there would be an inconsistency between pages
> > > > > +         * pinned by device and pages seen by QEMU. This is the case until
> > > > > +         * unmapped from the IOMMU (e.g., during device reset).
> > > > > +         *
> > > > > +         * With malicious guests, we really only care about pinning more memory
> > > > > +         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> > > > > +         * exceeded and can be used to mitigate this problem.
> > > > > +         */
> > > > > +        warn_report_once("Using device with vIOMMUs and coordinated discarding"
> > > > > +                         " of RAM (e.g., virtio-mem) works, however, malicious"
> > > > > +                         " guests can trigger pinning of more memory than"
> > > > > +                         " intended via an IOMMU. It's possible to mitigate "
> > > > > +                         " by setting/adjusting RLIMIT_MEMLOCK.");  
> > > >
> > > > Is this really fit to be in shared code?  Simply replacing "vfio" with
> > > > "device" for comments and warnings that are really of concern for a
> > > > specific use case doesn't look much better to me.
> > > >
> > > > I think translating an unpopulated address, as in the previous test
> > > > above, is generally invalid, but the comment is certainly trying to
> > > > frame the severity of this error relative to a specific use case.
> > > >
> > > > Here we're generating an unconditional warning, assuming that this code
> > > > path has been triggered by device code, for the condition of simply
> > > > asking for a translation to a MemoryRegion under discard manager
> > > > control?  Again, isn't that an action that has implications for a
> > > > specific use case of a device that supports pinning host memory?  
> > >
> > >
> > > Or can we rename the function to memory_get_xlat_addr_no_discard()?
> > > This looks more general and fit for the caller that doesn't want to
> > > map region that has a discard manager.  
> >
> > Is a guest restricted from mapping virtio-mem regions to a device?  
> 
> For the region that is not populated, it should be restricted. If this
> is wrong, we need a separate fix.
> 
> > AFAIK, this is something that a guest can do and we can't restrict them
> > from doing it, it's just that it opens some potential for malicious
> > activity that we rely on things like locked memory limits to keep from
> > getting out of hand.  Thanks,  
> 
> So it's the fault of the name, it could be
> memory_get_xlat_addr_no_unpopulated_discard().

Unpopulated discard has no translation, it's invalid.  That's the
previous test above where we return false.  The comment there is a bit
vfio specific, but I think the behavior is universal.  That doesn't
resolve this warn_report_once for simply trying to translate something
backed by virtio-mem though.  Thanks,

Alex
 
> Or as I replied, stick to what you've suggested.
> 
> Thanks
> 
> >
> > Alex
> >  
> > > > Should the shared code be generating this warning, or could an optional
> > > > pointer arg be updated to indicate a translation to discard manager
> > > > controlled memory and this comment and warning should remain in the
> > > > caller?  Thanks,  
> > >
> > > I think this should also work.
> > >
> > > Thanks
> > >  
> > > >
> > > > Alex
> > > >  
> > > > > +    }
> > > > > +
> > > > > +    /*
> > > > > +     * Translation truncates length to the IOMMU page size,
> > > > > +     * check that it did not truncate too much.
> > > > > +     */
> > > > > +    if (len & iotlb->addr_mask) {
> > > > > +        error_report("iommu has granularity incompatible with target AS");
> > > > > +        return false;
> > > > > +    }
> > > > > +
> > > > > +    if (vaddr) {
> > > > > +        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > > > > +    }
> > > > > +
> > > > > +    if (ram_addr) {
> > > > > +        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> > > > > +    }
> > > > > +
> > > > > +    if (read_only) {
> > > > > +        *read_only = !writable || mr->readonly;
> > > > > +    }
> > > > > +
> > > > > +    return true;
> > > > > +}
> > > > > +
> > > > >  void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
> > > > >  {
> > > > >      uint8_t mask = 1 << client;  
> > > >  
> > >  
> >  
>

On Fri, Oct 28, 2022 at 10:35 AM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Fri, 28 Oct 2022 10:23:16 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
> > On Fri, Oct 28, 2022 at 10:08 AM Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> > >
> > > On Fri, 28 Oct 2022 09:50:10 +0800
> > > Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > > On Fri, Oct 28, 2022 at 5:11 AM Alex Williamson
> > > > <alex.williamson@redhat.com> wrote:
> > > > >
> > > > > On Thu, 27 Oct 2022 15:40:31 +0800
> > > > > Cindy Lu <lulu@redhat.com> 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>
> > > > > > ---
> > > > > >  hw/vfio/common.c      | 92 ++-----------------------------------------
> > > > > >  include/exec/memory.h |  4 ++
> > > > > >  softmmu/memory.c      | 84 +++++++++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 92 insertions(+), 88 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > > > > index ace9562a9b..2b5a9f3d8d 100644
> > > > > > --- a/hw/vfio/common.c
> > > > > > +++ b/hw/vfio/common.c
> > > > > > @@ -574,92 +574,6 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> > > > > >             section->offset_within_address_space & (1ULL << 63);
> > > > > >  }
> > > > > >
> > > > > > -/* Called with rcu_read_lock held.  */
> > > > > > -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > > > > > -                               ram_addr_t *ram_addr, bool *read_only)
> > > > > > -{
> > > > > > -    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(&address_space_memory,
> > > > > > -                                 iotlb->translated_addr,
> > > > > > -                                 &xlat, &len, writable,
> > > > > > -                                 MEMTXATTRS_UNSPECIFIED);
> > > > > > -    if (!memory_region_is_ram(mr)) {
> > > > > > -        error_report("iommu map to non memory area %"HWADDR_PRIx"",
> > > > > > -                     xlat);
> > > > > > -        return false;
> > > > > > -    } else if (memory_region_has_ram_discard_manager(mr)) {
> > > > > > -        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> > > > > > -        MemoryRegionSection tmp = {
> > > > > > -            .mr = mr,
> > > > > > -            .offset_within_region = xlat,
> > > > > > -            .size = int128_make64(len),
> > > > > > -        };
> > > > > > -
> > > > > > -        /*
> > > > > > -         * Malicious VMs can map memory into the IOMMU, which is expected
> > > > > > -         * to remain discarded. vfio will pin all pages, populating memory.
> > > > > > -         * Disallow that. vmstate priorities make sure any RamDiscardManager
> > > > > > -         * were already restored before IOMMUs are restored.
> > > > > > -         */
> > > > > > -        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> > > > > > -            error_report("iommu map to discarded memory (e.g., unplugged via"
> > > > > > -                         " virtio-mem): %"HWADDR_PRIx"",
> > > > > > -                         iotlb->translated_addr);
> > > > > > -            return false;
> > > > > > -        }
> > > > > > -
> > > > > > -        /*
> > > > > > -         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> > > > > > -         * pages will remain pinned inside vfio until unmapped, resulting in a
> > > > > > -         * higher memory consumption than expected. If memory would get
> > > > > > -         * populated again later, there would be an inconsistency between pages
> > > > > > -         * pinned by vfio and pages seen by QEMU. This is the case until
> > > > > > -         * unmapped from the IOMMU (e.g., during device reset).
> > > > > > -         *
> > > > > > -         * With malicious guests, we really only care about pinning more memory
> > > > > > -         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> > > > > > -         * exceeded and can be used to mitigate this problem.
> > > > > > -         */
> > > > > > -        warn_report_once("Using vfio with vIOMMUs and coordinated discarding of"
> > > > > > -                         " RAM (e.g., virtio-mem) works, however, malicious"
> > > > > > -                         " guests can trigger pinning of more memory than"
> > > > > > -                         " intended via an IOMMU. It's possible to mitigate "
> > > > > > -                         " by setting/adjusting RLIMIT_MEMLOCK.");
> > > > > > -    }
> > > > > > -
> > > > > > -    /*
> > > > > > -     * Translation truncates length to the IOMMU page size,
> > > > > > -     * check that it did not truncate too much.
> > > > > > -     */
> > > > > > -    if (len & iotlb->addr_mask) {
> > > > > > -        error_report("iommu has granularity incompatible with target AS");
> > > > > > -        return false;
> > > > > > -    }
> > > > > > -
> > > > > > -    if (vaddr) {
> > > > > > -        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > > > > > -    }
> > > > > > -
> > > > > > -    if (ram_addr) {
> > > > > > -        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> > > > > > -    }
> > > > > > -
> > > > > > -    if (read_only) {
> > > > > > -        *read_only = !writable || mr->readonly;
> > > > > > -    }
> > > > > > -
> > > > > > -    return true;
> > > > > > -}
> > > > > > -
> > > > > >  static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > > > > >  {
> > > > > >      VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> > > > > > @@ -682,7 +596,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > > > > >      if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> > > > > >          bool read_only;
> > > > > >
> > > > > > -        if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
> > > > > > +        if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only,
> > > > > > +                                  &address_space_memory)) {
> > > > > >              goto out;
> > > > > >          }
> > > > > >          /*
> > > > > > @@ -1359,7 +1274,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > > > > >      }
> > > > > >
> > > > > >      rcu_read_lock();
> > > > > > -    if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
> > > > > > +    if (memory_get_xlat_addr(iotlb, NULL, &translated_addr, NULL,
> > > > > > +                             &address_space_memory)) {
> > > > > >          int ret;
> > > > > >
> > > > > >          ret = vfio_get_dirty_bitmap(container, iova, iotlb->addr_mask + 1,
> > > > > > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > > > > > index bfb1de8eea..282de1d5ad 100644
> > > > > > --- a/include/exec/memory.h
> > > > > > +++ b/include/exec/memory.h
> > > > > > @@ -713,6 +713,10 @@ void ram_discard_manager_register_listener(RamDiscardManager *rdm,
> > > > > >  void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> > > > > >                                               RamDiscardListener *rdl);
> > > > > >
> > > > > > +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > > > > > +                          ram_addr_t *ram_addr, bool *read_only,
> > > > > > +                          AddressSpace *as);
> > > > > > +
> > > > > >  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> > > > > >  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> > > > > >
> > > > > > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > > > > > index 7ba2048836..8586863ffa 100644
> > > > > > --- a/softmmu/memory.c
> > > > > > +++ b/softmmu/memory.c
> > > > > > @@ -2121,6 +2121,90 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> > > > > >      rdmc->unregister_listener(rdm, rdl);
> > > > > >  }
> > > > > >
> > > > > > +/* Called with rcu_read_lock held.  */
> > > > > > +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);
> > > > > > +    if (!memory_region_is_ram(mr)) {
> > > > > > +        error_report("iommu map to non memory area %" HWADDR_PRIx "", xlat);
> > > > > > +        return false;
> > > > > > +    } else if (memory_region_has_ram_discard_manager(mr)) {
> > > > > > +        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> > > > > > +        MemoryRegionSection tmp = {
> > > > > > +            .mr = mr,
> > > > > > +            .offset_within_region = xlat,
> > > > > > +            .size = int128_make64(len),
> > > > > > +        };
> > > > > > +
> > > > > > +        /*
> > > > > > +         * Malicious VMs can map memory into the IOMMU, which is expected
> > > > > > +         * to remain discarded. device will pin all pages, populating memory.
> > > > > > +         * Disallow that. vmstate priorities make sure any RamDiscardManager
> > > > > > +         * were already restored before IOMMUs are restored.
> > > > > > +         */
> > > > > > +        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> > > > > > +            error_report("iommu map to discarded memory (e.g., unplugged via"
> > > > > > +                         " virtio-mem): %" HWADDR_PRIx "",
> > > > > > +                         iotlb->translated_addr);
> > > > > > +            return false;
> > > > > > +        }
> > > > > > +
> > > > > > +        /*
> > > > > > +         * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> > > > > > +         * pages will remain pinned inside device until unmapped, resulting in a
> > > > > > +         * higher memory consumption than expected. If memory would get
> > > > > > +         * populated again later, there would be an inconsistency between pages
> > > > > > +         * pinned by device and pages seen by QEMU. This is the case until
> > > > > > +         * unmapped from the IOMMU (e.g., during device reset).
> > > > > > +         *
> > > > > > +         * With malicious guests, we really only care about pinning more memory
> > > > > > +         * than expected. RLIMIT_MEMLOCK set for the user/process can never be
> > > > > > +         * exceeded and can be used to mitigate this problem.
> > > > > > +         */
> > > > > > +        warn_report_once("Using device with vIOMMUs and coordinated discarding"
> > > > > > +                         " of RAM (e.g., virtio-mem) works, however, malicious"
> > > > > > +                         " guests can trigger pinning of more memory than"
> > > > > > +                         " intended via an IOMMU. It's possible to mitigate "
> > > > > > +                         " by setting/adjusting RLIMIT_MEMLOCK.");
> > > > >
> > > > > Is this really fit to be in shared code?  Simply replacing "vfio" with
> > > > > "device" for comments and warnings that are really of concern for a
> > > > > specific use case doesn't look much better to me.
> > > > >
> > > > > I think translating an unpopulated address, as in the previous test
> > > > > above, is generally invalid, but the comment is certainly trying to
> > > > > frame the severity of this error relative to a specific use case.
> > > > >
> > > > > Here we're generating an unconditional warning, assuming that this code
> > > > > path has been triggered by device code, for the condition of simply
> > > > > asking for a translation to a MemoryRegion under discard manager
> > > > > control?  Again, isn't that an action that has implications for a
> > > > > specific use case of a device that supports pinning host memory?
> > > >
> > > >
> > > > Or can we rename the function to memory_get_xlat_addr_no_discard()?
> > > > This looks more general and fit for the caller that doesn't want to
> > > > map region that has a discard manager.
> > >
> > > Is a guest restricted from mapping virtio-mem regions to a device?
> >
> > For the region that is not populated, it should be restricted. If this
> > is wrong, we need a separate fix.
> >
> > > AFAIK, this is something that a guest can do and we can't restrict them
> > > from doing it, it's just that it opens some potential for malicious
> > > activity that we rely on things like locked memory limits to keep from
> > > getting out of hand.  Thanks,
> >
> > So it's the fault of the name, it could be
> > memory_get_xlat_addr_no_unpopulated_discard().
>
> Unpopulated discard has no translation, it's invalid.  That's the
> previous test above where we return false.  The comment there is a bit
> vfio specific, but I think the behavior is universal.  That doesn't
> resolve this warn_report_once for simply trying to translate something
> backed by virtio-mem though.  Thanks,

Ok, fine, let's add an optional arg then.

Thanks

>
> Alex
>
> > Or as I replied, stick to what you've suggested.
> >
> > Thanks
> >
> > >
> > > Alex
> > >
> > > > > Should the shared code be generating this warning, or could an optional
> > > > > pointer arg be updated to indicate a translation to discard manager
> > > > > controlled memory and this comment and warning should remain in the
> > > > > caller?  Thanks,
> > > >
> > > > I think this should also work.
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Alex
> > > > >
> > > > > > +    }
> > > > > > +
> > > > > > +    /*
> > > > > > +     * Translation truncates length to the IOMMU page size,
> > > > > > +     * check that it did not truncate too much.
> > > > > > +     */
> > > > > > +    if (len & iotlb->addr_mask) {
> > > > > > +        error_report("iommu has granularity incompatible with target AS");
> > > > > > +        return false;
> > > > > > +    }
> > > > > > +
> > > > > > +    if (vaddr) {
> > > > > > +        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > > > > > +    }
> > > > > > +
> > > > > > +    if (ram_addr) {
> > > > > > +        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> > > > > > +    }
> > > > > > +
> > > > > > +    if (read_only) {
> > > > > > +        *read_only = !writable || mr->readonly;
> > > > > > +    }
> > > > > > +
> > > > > > +    return true;
> > > > > > +}
> > > > > > +
> > > > > >  void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
> > > > > >  {
> > > > > >      uint8_t mask = 1 << client;
> > > > >
> > > >
> > >
> >
>