Re: [PATCH V1 16/26] vfio: return mr from vfio_get_xlat_addr
On Tue, Feb 04, 2025 at 12:42:20PM -0500, Steven Sistare wrote: > !---| > CAUTION: External Email > > |---! > > On 2/4/2025 10:47 AM, Cédric Le Goater wrote: > > + John (for vfio-user) > > > > On 1/29/25 15:43, Steve Sistare wrote: > > > Return the memory region that the translated address is found in, for > > > use in a subsequent patch. No functional change. > > > > Keeping a reference on this memory region could be risky. What for ? > > The returned mr is briefly used here in later patches: > > vfio_iommu_map_notify() > vfio_get_xlat_addr(&mr) > vfio_container_dma_map(mr->ram_block) ** > if ram_block is right > vioc->dma_map_file() > else > vioc->dma_map() The need for ->ram_block in dma map/unmap is exactly the case for vfio-user too. Cédric: > > There is a risk that the life cycle of the returned MemoryRegion > > doesn't match VFIO expectations. Can you perhaps explain in a bit more detail your concerns? Are you talking about current code, or possible future uses? Is there an alternative approach you could suggest? regards john
Re: [PATCH V1 16/26] vfio: return mr from vfio_get_xlat_addr
On 2/4/2025 10:47 AM, Cédric Le Goater wrote:
+ John (for vfio-user)
On 1/29/25 15:43, Steve Sistare wrote:
Return the memory region that the translated address is found in, for
use in a subsequent patch. No functional change.
Keeping a reference on this memory region could be risky. What for ?
The returned mr is briefly used here in later patches:
vfio_iommu_map_notify()
vfio_get_xlat_addr(&mr)
vfio_container_dma_map(mr->ram_block) **
if ram_block is right
vioc->dma_map_file()
else
vioc->dma_map()
Signed-off-by: Steve Sistare
---
hw/vfio/common.c | 9 ++---
hw/virtio/vhost-vdpa.c | 2 +-
include/exec/memory.h | 5 -
system/memory.c | 8 +++-
4 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index db0498e..4bbc29f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -248,12 +248,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,
+ MemoryRegion **mr_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);
+ &mr_has_discard_manager, mr_p, errp);
if (ret && mr_has_discard_manager) {
/*
* Malicious VMs might trigger discarding of IOMMU-mapped memory. The
@@ -300,7 +301,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, &local_err)) {
+ if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL,
+ &local_err)) {
error_report_err(local_err);
goto out;
}
@@ -1279,7 +1281,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, &local_err)) {
+ if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, NULL,
+ &local_err)) {
error_report_err(local_err);
goto out_unlock;
}
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 3cdaa12..a1866bb 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -228,7 +228,7 @@ 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,
+ if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL, NULL,
&local_err)) {
error_report_err(local_err);
return;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index ea5d33a..a2f1229 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -747,13 +747,16 @@ 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 *mr_has_discard_manager,
+ MemoryRegion **mr_p,
There is a risk that the life cycle of the returned MemoryRegion
doesn't match VFIO expectations.
Also, it seems that memory_get_xlat_addr() has reached a point
where the callers need refactoring. 'mr_p' would be the 5th out
parameter and 3 of these already depend on the MemoryRegion
returned by flatview_translate().
If we return mr plus xlat, then the caller could trivially derive
vaddr, ram_addr, and read_only.
- Steve
+ Error **errp);
typedef struct CoalescedMemoryRange CoalescedMemoryRange;
typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
diff --git a/system/memory.c b/system/memory.c
index 4c82979..4ec2b8f 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2185,7 +2185,9 @@ void
ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
/* Called with rcu_read_lock held. */
bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
ram_ad
Re: [PATCH V1 16/26] vfio: return mr from vfio_get_xlat_addr
+ John (for vfio-user)
On 1/29/25 15:43, Steve Sistare wrote:
Return the memory region that the translated address is found in, for
use in a subsequent patch. No functional change.
Keeping a reference on this memory region could be risky. What for ?
Signed-off-by: Steve Sistare
---
hw/vfio/common.c | 9 ++---
hw/virtio/vhost-vdpa.c | 2 +-
include/exec/memory.h | 5 -
system/memory.c| 8 +++-
4 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index db0498e..4bbc29f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -248,12 +248,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,
+ MemoryRegion **mr_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);
+ &mr_has_discard_manager, mr_p, errp);
if (ret && mr_has_discard_manager) {
/*
* Malicious VMs might trigger discarding of IOMMU-mapped memory. The
@@ -300,7 +301,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, &local_err)) {
+if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL,
+&local_err)) {
error_report_err(local_err);
goto out;
}
@@ -1279,7 +1281,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, &local_err)) {
+if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, NULL,
+&local_err)) {
error_report_err(local_err);
goto out_unlock;
}
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 3cdaa12..a1866bb 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -228,7 +228,7 @@ 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,
+if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL, NULL,
&local_err)) {
error_report_err(local_err);
return;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index ea5d33a..a2f1229 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -747,13 +747,16 @@ 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 *mr_has_discard_manager,
+ MemoryRegion **mr_p,
There is a risk that the life cycle of the returned MemoryRegion
doesn't match VFIO expectations.
Also, it seems that memory_get_xlat_addr() has reached a point
where the callers need refactoring. 'mr_p' would be the 5th out
parameter and 3 of these already depend on the MemoryRegion
returned by flatview_translate().
Thanks,
C.
+ Error **errp);
typedef struct CoalescedMemoryRange CoalescedMemoryRange;
typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
diff --git a/system/memory.c b/system/memory.c
index 4c82979..4ec2b8f 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2185,7 +2185,9 @@ 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 *mr_has_discard_manager,
+ MemoryRegion **mr_p,
+ Error **errp)
{
MemoryRegion *mr;
hwaddr xlat;
@@ -2250,6 +2252,10 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, voi
