| Message ID | 1739542467-226739-29-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Live update: vfio and iommufd | expand |
cc memory reviewers. The series is here: https://lore.kernel.org/qemu-devel/1739542467-226739-1-git-send-email-steven.sistare@oracle.com/ - Steve On 2/14/2025 9:14 AM, Steve Sistare wrote: > Modify memory_get_xlat_addr and vfio_get_xlat_addr to return the memory > region that the translated address is found in. This will be needed by > CPR in a subsequent patch to map blocks using IOMMU_IOAS_MAP_FILE. > > Also return the xlat offset, so we can simplify the interface by removing > the out parameters that can be trivially derived from mr and xlat. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > hw/vfio/common.c | 21 ++++++++++++++------- > hw/virtio/vhost-vdpa.c | 8 ++++++-- > include/exec/memory.h | 6 +++--- > system/memory.c | 19 ++++--------------- > 4 files changed, 27 insertions(+), 27 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index c536698..3b0c520 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -246,14 +246,13 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section) > } > > /* Called with rcu_read_lock held. */ > -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, > - ram_addr_t *ram_addr, bool *read_only, > - Error **errp) > +static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, MemoryRegion **mr_p, > + hwaddr *xlat_p, Error **errp) > { > bool ret, mr_has_discard_manager; > > - ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only, > - &mr_has_discard_manager, errp); > + ret = memory_get_xlat_addr(iotlb, &mr_has_discard_manager, mr_p, xlat_p, > + errp); > if (ret && mr_has_discard_manager) { > /* > * Malicious VMs might trigger discarding of IOMMU-mapped memory. The > @@ -281,6 +280,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n); > VFIOContainerBase *bcontainer = giommu->bcontainer; > hwaddr iova = iotlb->iova + giommu->iommu_offset; > + MemoryRegion *mr; > + hwaddr xlat; > void *vaddr; > int ret; > Error *local_err = NULL; > @@ -300,10 +301,13 @@ 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, &local_err)) { > + if (!vfio_get_xlat_addr(iotlb, &mr, &xlat, &local_err)) { > error_report_err(local_err); > goto out; > } > + vaddr = memory_region_get_ram_ptr(mr) + xlat; > + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly; > + > /* > * vaddr is only valid until rcu_read_unlock(). But after > * vfio_dma_map has set up the mapping the pages will be > @@ -1259,6 +1263,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > ram_addr_t translated_addr; > Error *local_err = NULL; > int ret = -EINVAL; > + MemoryRegion *mr; > + ram_addr_t xlat; > > trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask); > > @@ -1269,10 +1275,11 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > } > > rcu_read_lock(); > - if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) { > + if (!vfio_get_xlat_addr(iotlb, &mr, &xlat, &local_err)) { > error_report_err(local_err); > goto out_unlock; > } > + translated_addr = memory_region_get_ram_addr(mr) + xlat; > > ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, > translated_addr, &local_err); > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 3cdaa12..5dfe51e 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -209,6 +209,8 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > int ret; > Int128 llend; > Error *local_err = NULL; > + MemoryRegion *mr; > + hwaddr xlat; > > if (iotlb->target_as != &address_space_memory) { > error_report("Wrong target AS \"%s\", only system memory is allowed", > @@ -228,11 +230,13 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) { > bool read_only; > > - if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL, > - &local_err)) { > + if (!memory_get_xlat_addr(iotlb, NULL, &mr, &xlat, &local_err)) { > error_report_err(local_err); > return; > } > + vaddr = memory_region_get_ram_ptr(mr) + xlat; > + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly; > + > ret = vhost_vdpa_dma_map(s, VHOST_VDPA_GUEST_PA_ASID, iova, > iotlb->addr_mask + 1, vaddr, read_only); > if (ret) { > diff --git a/include/exec/memory.h b/include/exec/memory.h > index ea5d33a..8590838 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -747,13 +747,13 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm, > * @read_only: indicates if writes are allowed > * @mr_has_discard_manager: indicates memory is controlled by a > * RamDiscardManager > + * @mr_p: return the MemoryRegion containing the @iotlb translated addr > * @errp: pointer to Error*, to store an error if it happens. > * > * Return: true on success, else false setting @errp with error. > */ > -bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, > - ram_addr_t *ram_addr, bool *read_only, > - bool *mr_has_discard_manager, Error **errp); > +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, bool *mr_has_discard_manager, > + MemoryRegion **mr_p, hwaddr *xlat_p, Error **errp); > > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd; > diff --git a/system/memory.c b/system/memory.c > index 4c82979..755eafe 100644 > --- a/system/memory.c > +++ b/system/memory.c > @@ -2183,9 +2183,8 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm, > } > > /* Called with rcu_read_lock held. */ > -bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, > - ram_addr_t *ram_addr, bool *read_only, > - bool *mr_has_discard_manager, Error **errp) > +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, bool *mr_has_discard_manager, > + MemoryRegion **mr_p, hwaddr *xlat_p, Error **errp) > { > MemoryRegion *mr; > hwaddr xlat; > @@ -2238,18 +2237,8 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, > 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; > - } > - > + *xlat_p = xlat; > + *mr_p = mr; > return true; > } >
On Fri, Feb 14, 2025 at 06:14:10AM -0800, Steve Sistare wrote: > Modify memory_get_xlat_addr and vfio_get_xlat_addr to return the memory > region that the translated address is found in. This will be needed by > CPR in a subsequent patch to map blocks using IOMMU_IOAS_MAP_FILE. > > Also return the xlat offset, so we can simplify the interface by removing > the out parameters that can be trivially derived from mr and xlat. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > hw/vfio/common.c | 21 ++++++++++++++------- > hw/virtio/vhost-vdpa.c | 8 ++++++-- > include/exec/memory.h | 6 +++--- > system/memory.c | 19 ++++--------------- > 4 files changed, 27 insertions(+), 27 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index c536698..3b0c520 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -246,14 +246,13 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section) > } > > /* Called with rcu_read_lock held. */ > -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, > - ram_addr_t *ram_addr, bool *read_only, > - Error **errp) > +static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, MemoryRegion **mr_p, > + hwaddr *xlat_p, Error **errp) > { > bool ret, mr_has_discard_manager; > > - ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only, > - &mr_has_discard_manager, errp); > + ret = memory_get_xlat_addr(iotlb, &mr_has_discard_manager, mr_p, xlat_p, > + errp); > if (ret && mr_has_discard_manager) { > /* > * Malicious VMs might trigger discarding of IOMMU-mapped memory. The > @@ -281,6 +280,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n); > VFIOContainerBase *bcontainer = giommu->bcontainer; > hwaddr iova = iotlb->iova + giommu->iommu_offset; > + MemoryRegion *mr; > + hwaddr xlat; > void *vaddr; > int ret; > Error *local_err = NULL; > @@ -300,10 +301,13 @@ 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, &local_err)) { > + if (!vfio_get_xlat_addr(iotlb, &mr, &xlat, &local_err)) { > error_report_err(local_err); > goto out; > } > + vaddr = memory_region_get_ram_ptr(mr) + xlat; > + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly; > + > /* > * vaddr is only valid until rcu_read_unlock(). But after > * vfio_dma_map has set up the mapping the pages will be > @@ -1259,6 +1263,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > ram_addr_t translated_addr; > Error *local_err = NULL; > int ret = -EINVAL; > + MemoryRegion *mr; > + ram_addr_t xlat; > > trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask); > > @@ -1269,10 +1275,11 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > } > > rcu_read_lock(); > - if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) { > + if (!vfio_get_xlat_addr(iotlb, &mr, &xlat, &local_err)) { > error_report_err(local_err); > goto out_unlock; > } > + translated_addr = memory_region_get_ram_addr(mr) + xlat; > > ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, > translated_addr, &local_err); > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 3cdaa12..5dfe51e 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -209,6 +209,8 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > int ret; > Int128 llend; > Error *local_err = NULL; > + MemoryRegion *mr; > + hwaddr xlat; > > if (iotlb->target_as != &address_space_memory) { > error_report("Wrong target AS \"%s\", only system memory is allowed", > @@ -228,11 +230,13 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) { > bool read_only; > > - if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL, > - &local_err)) { > + if (!memory_get_xlat_addr(iotlb, NULL, &mr, &xlat, &local_err)) { > error_report_err(local_err); > return; > } > + vaddr = memory_region_get_ram_ptr(mr) + xlat; > + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly; > + > ret = vhost_vdpa_dma_map(s, VHOST_VDPA_GUEST_PA_ASID, iova, > iotlb->addr_mask + 1, vaddr, read_only); > if (ret) { > diff --git a/include/exec/memory.h b/include/exec/memory.h > index ea5d33a..8590838 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -747,13 +747,13 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm, > * @read_only: indicates if writes are allowed > * @mr_has_discard_manager: indicates memory is controlled by a > * RamDiscardManager (some prior fields are prone to removal)) > + * @mr_p: return the MemoryRegion containing the @iotlb translated addr > * @errp: pointer to Error*, to store an error if it happens. > * > * Return: true on success, else false setting @errp with error. > */ > -bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, > - ram_addr_t *ram_addr, bool *read_only, > - bool *mr_has_discard_manager, Error **errp); > +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, bool *mr_has_discard_manager, > + MemoryRegion **mr_p, hwaddr *xlat_p, Error **errp); > > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd; > diff --git a/system/memory.c b/system/memory.c > index 4c82979..755eafe 100644 > --- a/system/memory.c > +++ b/system/memory.c > @@ -2183,9 +2183,8 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm, > } > > /* Called with rcu_read_lock held. */ > -bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, > - ram_addr_t *ram_addr, bool *read_only, > - bool *mr_has_discard_manager, Error **errp) > +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, bool *mr_has_discard_manager, > + MemoryRegion **mr_p, hwaddr *xlat_p, Error **errp) If we're going to return the MR anyway, probably we can drop mr_has_discard_manager altogether.. > { > MemoryRegion *mr; > hwaddr xlat; > @@ -2238,18 +2237,8 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, > 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; > - } > - > + *xlat_p = xlat; > + *mr_p = mr; I suppose current use on the callers are still under RCU so looks ok, but that'll need to be rich-documented. Better way is always taking a MR reference when the MR pointer is returned, with memory_region_ref(). Then it is even valid if by accident accessed after rcu_read_unlock(), and caller should unref() after use. > return true; > } > > -- > 1.8.3.1 >
On 2/14/2025 11:48 AM, Peter Xu wrote: > On Fri, Feb 14, 2025 at 06:14:10AM -0800, Steve Sistare wrote: >> Modify memory_get_xlat_addr and vfio_get_xlat_addr to return the memory >> region that the translated address is found in. This will be needed by >> CPR in a subsequent patch to map blocks using IOMMU_IOAS_MAP_FILE. >> >> Also return the xlat offset, so we can simplify the interface by removing >> the out parameters that can be trivially derived from mr and xlat. >> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> hw/vfio/common.c | 21 ++++++++++++++------- >> hw/virtio/vhost-vdpa.c | 8 ++++++-- >> include/exec/memory.h | 6 +++--- >> system/memory.c | 19 ++++--------------- >> 4 files changed, 27 insertions(+), 27 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index c536698..3b0c520 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -246,14 +246,13 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section) >> } >> >> /* Called with rcu_read_lock held. */ >> -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, >> - ram_addr_t *ram_addr, bool *read_only, >> - Error **errp) >> +static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, MemoryRegion **mr_p, >> + hwaddr *xlat_p, Error **errp) >> { >> bool ret, mr_has_discard_manager; >> >> - ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only, >> - &mr_has_discard_manager, errp); >> + ret = memory_get_xlat_addr(iotlb, &mr_has_discard_manager, mr_p, xlat_p, >> + errp); >> if (ret && mr_has_discard_manager) { >> /* >> * Malicious VMs might trigger discarding of IOMMU-mapped memory. The >> @@ -281,6 +280,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >> VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n); >> VFIOContainerBase *bcontainer = giommu->bcontainer; >> hwaddr iova = iotlb->iova + giommu->iommu_offset; >> + MemoryRegion *mr; >> + hwaddr xlat; >> void *vaddr; >> int ret; >> Error *local_err = NULL; >> @@ -300,10 +301,13 @@ 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, &local_err)) { >> + if (!vfio_get_xlat_addr(iotlb, &mr, &xlat, &local_err)) { >> error_report_err(local_err); >> goto out; >> } >> + vaddr = memory_region_get_ram_ptr(mr) + xlat; >> + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly; >> + >> /* >> * vaddr is only valid until rcu_read_unlock(). But after >> * vfio_dma_map has set up the mapping the pages will be >> @@ -1259,6 +1263,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >> ram_addr_t translated_addr; >> Error *local_err = NULL; >> int ret = -EINVAL; >> + MemoryRegion *mr; >> + ram_addr_t xlat; >> >> trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask); >> >> @@ -1269,10 +1275,11 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >> } >> >> rcu_read_lock(); >> - if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) { >> + if (!vfio_get_xlat_addr(iotlb, &mr, &xlat, &local_err)) { >> error_report_err(local_err); >> goto out_unlock; >> } >> + translated_addr = memory_region_get_ram_addr(mr) + xlat; >> >> ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, >> translated_addr, &local_err); >> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c >> index 3cdaa12..5dfe51e 100644 >> --- a/hw/virtio/vhost-vdpa.c >> +++ b/hw/virtio/vhost-vdpa.c >> @@ -209,6 +209,8 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >> int ret; >> Int128 llend; >> Error *local_err = NULL; >> + MemoryRegion *mr; >> + hwaddr xlat; >> >> if (iotlb->target_as != &address_space_memory) { >> error_report("Wrong target AS \"%s\", only system memory is allowed", >> @@ -228,11 +230,13 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >> if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) { >> bool read_only; >> >> - if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL, >> - &local_err)) { >> + if (!memory_get_xlat_addr(iotlb, NULL, &mr, &xlat, &local_err)) { >> error_report_err(local_err); >> return; >> } >> + vaddr = memory_region_get_ram_ptr(mr) + xlat; >> + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly; >> + >> ret = vhost_vdpa_dma_map(s, VHOST_VDPA_GUEST_PA_ASID, iova, >> iotlb->addr_mask + 1, vaddr, read_only); >> if (ret) { >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index ea5d33a..8590838 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -747,13 +747,13 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm, >> * @read_only: indicates if writes are allowed >> * @mr_has_discard_manager: indicates memory is controlled by a >> * RamDiscardManager > > (some prior fields are prone to removal)) My bad, thanks. I'll delete vaddr, ram_addr, read_only, and add xlat. >> + * @mr_p: return the MemoryRegion containing the @iotlb translated addr >> * @errp: pointer to Error*, to store an error if it happens. >> * >> * Return: true on success, else false setting @errp with error. >> */ >> -bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, >> - ram_addr_t *ram_addr, bool *read_only, >> - bool *mr_has_discard_manager, Error **errp); >> +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, bool *mr_has_discard_manager, >> + MemoryRegion **mr_p, hwaddr *xlat_p, Error **errp); >> >> typedef struct CoalescedMemoryRange CoalescedMemoryRange; >> typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd; >> diff --git a/system/memory.c b/system/memory.c >> index 4c82979..755eafe 100644 >> --- a/system/memory.c >> +++ b/system/memory.c >> @@ -2183,9 +2183,8 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm, >> } >> >> /* Called with rcu_read_lock held. */ >> -bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, >> - ram_addr_t *ram_addr, bool *read_only, >> - bool *mr_has_discard_manager, Error **errp) >> +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, bool *mr_has_discard_manager, >> + MemoryRegion **mr_p, hwaddr *xlat_p, Error **errp) > > If we're going to return the MR anyway, probably we can drop > mr_has_discard_manager altogether.. To hoist mr_has_discard_manager to the vfio caller, I would need to return len. Your call. >> { >> MemoryRegion *mr; >> hwaddr xlat; >> @@ -2238,18 +2237,8 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, >> 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; >> - } >> - >> + *xlat_p = xlat; >> + *mr_p = mr; > > I suppose current use on the callers are still under RCU so looks ok, but > that'll need to be rich-documented. I can do that, or ... > Better way is always taking a MR reference when the MR pointer is returned, > with memory_region_ref(). Then it is even valid if by accident accessed > after rcu_read_unlock(), and caller should unref() after use. I can do that, but it would add cycles. Is this considered a high performance path that may be called frequently? - Steve > >> return true; >> } >> >> -- >> 1.8.3.1 >> >
On Fri, Feb 14, 2025 at 03:40:57PM -0500, Steven Sistare wrote: > > > diff --git a/system/memory.c b/system/memory.c > > > index 4c82979..755eafe 100644 > > > --- a/system/memory.c > > > +++ b/system/memory.c > > > @@ -2183,9 +2183,8 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm, > > > } > > > /* Called with rcu_read_lock held. */ > > > -bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, > > > - ram_addr_t *ram_addr, bool *read_only, > > > - bool *mr_has_discard_manager, Error **errp) > > > +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, bool *mr_has_discard_manager, > > > + MemoryRegion **mr_p, hwaddr *xlat_p, Error **errp) > > > > If we're going to return the MR anyway, probably we can drop > > mr_has_discard_manager altogether.. > > To hoist mr_has_discard_manager to the vfio caller, I would need to return len. > Your call. I meant only dropping mr_has_discard_manager parameter from the function interface, not the ram_discard_manager_is_populated() check. > > > > { > > > MemoryRegion *mr; > > > hwaddr xlat; > > > @@ -2238,18 +2237,8 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, > > > 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; > > > - } > > > - > > > + *xlat_p = xlat; > > > + *mr_p = mr; > > > > I suppose current use on the callers are still under RCU so looks ok, but > > that'll need to be rich-documented. > > I can do that, or ... > > > Better way is always taking a MR reference when the MR pointer is returned, > > with memory_region_ref(). Then it is even valid if by accident accessed > > after rcu_read_unlock(), and caller should unref() after use. > > I can do that, but it would add cycles. Is this considered a high performance > path that may be called frequently? AFAICT, any vIOMMU mapping isn't high perf path. In this specific path, the refcount op should be buried in any dma map operations.. Personally I slightly prefer this one because it's always safer to take a refcount along with a pointer.. easier to follow.
diff --git a/hw/vfio/common.c b/hw/vfio/common.c index c536698..3b0c520 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -246,14 +246,13 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section) } /* Called with rcu_read_lock held. */ -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, - ram_addr_t *ram_addr, bool *read_only, - Error **errp) +static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, MemoryRegion **mr_p, + hwaddr *xlat_p, Error **errp) { bool ret, mr_has_discard_manager; - ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only, - &mr_has_discard_manager, errp); + ret = memory_get_xlat_addr(iotlb, &mr_has_discard_manager, mr_p, xlat_p, + errp); if (ret && mr_has_discard_manager) { /* * Malicious VMs might trigger discarding of IOMMU-mapped memory. The @@ -281,6 +280,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n); VFIOContainerBase *bcontainer = giommu->bcontainer; hwaddr iova = iotlb->iova + giommu->iommu_offset; + MemoryRegion *mr; + hwaddr xlat; void *vaddr; int ret; Error *local_err = NULL; @@ -300,10 +301,13 @@ 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, &local_err)) { + if (!vfio_get_xlat_addr(iotlb, &mr, &xlat, &local_err)) { error_report_err(local_err); goto out; } + vaddr = memory_region_get_ram_ptr(mr) + xlat; + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly; + /* * vaddr is only valid until rcu_read_unlock(). But after * vfio_dma_map has set up the mapping the pages will be @@ -1259,6 +1263,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) ram_addr_t translated_addr; Error *local_err = NULL; int ret = -EINVAL; + MemoryRegion *mr; + ram_addr_t xlat; trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask); @@ -1269,10 +1275,11 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) } rcu_read_lock(); - if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) { + if (!vfio_get_xlat_addr(iotlb, &mr, &xlat, &local_err)) { error_report_err(local_err); goto out_unlock; } + translated_addr = memory_region_get_ram_addr(mr) + xlat; ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, translated_addr, &local_err); diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 3cdaa12..5dfe51e 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -209,6 +209,8 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) int ret; Int128 llend; Error *local_err = NULL; + MemoryRegion *mr; + hwaddr xlat; if (iotlb->target_as != &address_space_memory) { error_report("Wrong target AS \"%s\", only system memory is allowed", @@ -228,11 +230,13 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) { bool read_only; - if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL, - &local_err)) { + if (!memory_get_xlat_addr(iotlb, NULL, &mr, &xlat, &local_err)) { error_report_err(local_err); return; } + vaddr = memory_region_get_ram_ptr(mr) + xlat; + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly; + ret = vhost_vdpa_dma_map(s, VHOST_VDPA_GUEST_PA_ASID, iova, iotlb->addr_mask + 1, vaddr, read_only); if (ret) { diff --git a/include/exec/memory.h b/include/exec/memory.h index ea5d33a..8590838 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -747,13 +747,13 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm, * @read_only: indicates if writes are allowed * @mr_has_discard_manager: indicates memory is controlled by a * RamDiscardManager + * @mr_p: return the MemoryRegion containing the @iotlb translated addr * @errp: pointer to Error*, to store an error if it happens. * * Return: true on success, else false setting @errp with error. */ -bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, - ram_addr_t *ram_addr, bool *read_only, - bool *mr_has_discard_manager, Error **errp); +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, bool *mr_has_discard_manager, + MemoryRegion **mr_p, hwaddr *xlat_p, Error **errp); typedef struct CoalescedMemoryRange CoalescedMemoryRange; typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd; diff --git a/system/memory.c b/system/memory.c index 4c82979..755eafe 100644 --- a/system/memory.c +++ b/system/memory.c @@ -2183,9 +2183,8 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm, } /* Called with rcu_read_lock held. */ -bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, - ram_addr_t *ram_addr, bool *read_only, - bool *mr_has_discard_manager, Error **errp) +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, bool *mr_has_discard_manager, + MemoryRegion **mr_p, hwaddr *xlat_p, Error **errp) { MemoryRegion *mr; hwaddr xlat; @@ -2238,18 +2237,8 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, 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; - } - + *xlat_p = xlat; + *mr_p = mr; return true; }
Modify memory_get_xlat_addr and vfio_get_xlat_addr to return the memory region that the translated address is found in. This will be needed by CPR in a subsequent patch to map blocks using IOMMU_IOAS_MAP_FILE. Also return the xlat offset, so we can simplify the interface by removing the out parameters that can be trivially derived from mr and xlat. Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- hw/vfio/common.c | 21 ++++++++++++++------- hw/virtio/vhost-vdpa.c | 8 ++++++-- include/exec/memory.h | 6 +++--- system/memory.c | 19 ++++--------------- 4 files changed, 27 insertions(+), 27 deletions(-)