* [PATCH v3 1/7] util/error: Introduce warn_report_err_once()
2025-02-06 13:14 [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails Cédric Le Goater
@ 2025-02-06 13:14 ` Cédric Le Goater
2025-02-10 16:26 ` Cédric Le Goater
2025-02-06 13:14 ` [PATCH v3 2/7] vfio/pci: Replace "iommu_device" by "vIOMMU" Cédric Le Goater
` (7 subsequent siblings)
8 siblings, 1 reply; 13+ messages in thread
From: Cédric Le Goater @ 2025-02-06 13:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Williamson, Cédric Le Goater, Markus Armbruster
Depending on the configuration of the host and VM, a passthrough
device may generate recurring DMA mapping errors at runtime. In such
cases, reporting the issue once is sufficient.
We have already the warn/error_report_once() routines taking a format
and arguments. Using the same design pattern, add a new warning
variant taking an 'Error *' parameter.
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
include/qapi/error.h | 12 ++++++++++++
util/error.c | 11 +++++++++++
2 files changed, 23 insertions(+)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 71f8fb2c50eee9a544992d0c05263c9793956fe1..f5fe2162623e5770d652f7415ebc25172d97616e 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -466,6 +466,18 @@ void warn_reportf_err(Error *err, const char *fmt, ...)
void error_reportf_err(Error *err, const char *fmt, ...)
G_GNUC_PRINTF(2, 3);
+/*
+ * Similar to warn_report_err(), except it prints the message just once.
+ * Return true when it prints, false otherwise.
+ */
+bool warn_report_err_once_cond(bool *printed, Error *err);
+
+#define warn_report_err_once(err) \
+ ({ \
+ static bool print_once_; \
+ warn_report_err_once_cond(&print_once_, err); \
+ })
+
/*
* Just like error_setg(), except you get to specify the error class.
* Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
diff --git a/util/error.c b/util/error.c
index e5e247209a9e0796074a9794f5598325f22f8d35..673011b89e95f488817b86c31cd389386b2558bb 100644
--- a/util/error.c
+++ b/util/error.c
@@ -247,6 +247,17 @@ void warn_report_err(Error *err)
error_free(err);
}
+bool warn_report_err_once_cond(bool *printed, Error *err)
+{
+ if (*printed) {
+ error_free(err);
+ return false;
+ }
+ *printed = true;
+ warn_report_err(err);
+ return true;
+}
+
void error_reportf_err(Error *err, const char *fmt, ...)
{
va_list ap;
--
2.48.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/7] util/error: Introduce warn_report_err_once()
2025-02-06 13:14 ` [PATCH v3 1/7] util/error: Introduce warn_report_err_once() Cédric Le Goater
@ 2025-02-10 16:26 ` Cédric Le Goater
2025-02-11 6:53 ` Markus Armbruster
0 siblings, 1 reply; 13+ messages in thread
From: Cédric Le Goater @ 2025-02-10 16:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Williamson, Markus Armbruster
On 2/6/25 14:14, Cédric Le Goater wrote:
> Depending on the configuration of the host and VM, a passthrough
> device may generate recurring DMA mapping errors at runtime. In such
> cases, reporting the issue once is sufficient.
>
> We have already the warn/error_report_once() routines taking a format
> and arguments. Using the same design pattern, add a new warning
> variant taking an 'Error *' parameter.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
> include/qapi/error.h | 12 ++++++++++++
> util/error.c | 11 +++++++++++
> 2 files changed, 23 insertions(+)
Hello Markus,
Are you ok with this change ? Should we take it through the vfio queue ?
Thanks,
C.
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 71f8fb2c50eee9a544992d0c05263c9793956fe1..f5fe2162623e5770d652f7415ebc25172d97616e 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -466,6 +466,18 @@ void warn_reportf_err(Error *err, const char *fmt, ...)
> void error_reportf_err(Error *err, const char *fmt, ...)
> G_GNUC_PRINTF(2, 3);
>
> +/*
> + * Similar to warn_report_err(), except it prints the message just once.
> + * Return true when it prints, false otherwise.
> + */
> +bool warn_report_err_once_cond(bool *printed, Error *err);
> +
> +#define warn_report_err_once(err) \
> + ({ \
> + static bool print_once_; \
> + warn_report_err_once_cond(&print_once_, err); \
> + })
> +
> /*
> * Just like error_setg(), except you get to specify the error class.
> * Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
> diff --git a/util/error.c b/util/error.c
> index e5e247209a9e0796074a9794f5598325f22f8d35..673011b89e95f488817b86c31cd389386b2558bb 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -247,6 +247,17 @@ void warn_report_err(Error *err)
> error_free(err);
> }
>
> +bool warn_report_err_once_cond(bool *printed, Error *err)
> +{
> + if (*printed) {
> + error_free(err);
> + return false;
> + }
> + *printed = true;
> + warn_report_err(err);
> + return true;
> +}
> +
> void error_reportf_err(Error *err, const char *fmt, ...)
> {
> va_list ap;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/7] util/error: Introduce warn_report_err_once()
2025-02-10 16:26 ` Cédric Le Goater
@ 2025-02-11 6:53 ` Markus Armbruster
0 siblings, 0 replies; 13+ messages in thread
From: Markus Armbruster @ 2025-02-11 6:53 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-devel, Alex Williamson
Cédric Le Goater <clg@redhat.com> writes:
> On 2/6/25 14:14, Cédric Le Goater wrote:
>> Depending on the configuration of the host and VM, a passthrough
>> device may generate recurring DMA mapping errors at runtime. In such
>> cases, reporting the issue once is sufficient.
>>
>> We have already the warn/error_report_once() routines taking a format
>> and arguments. Using the same design pattern, add a new warning
>> variant taking an 'Error *' parameter.
>>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>> include/qapi/error.h | 12 ++++++++++++
>> util/error.c | 11 +++++++++++
>> 2 files changed, 23 insertions(+)
>
> Hello Markus,
>
> Are you ok with this change ? Should we take it through the vfio queue ?
Yes, please.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 2/7] vfio/pci: Replace "iommu_device" by "vIOMMU"
2025-02-06 13:14 [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails Cédric Le Goater
2025-02-06 13:14 ` [PATCH v3 1/7] util/error: Introduce warn_report_err_once() Cédric Le Goater
@ 2025-02-06 13:14 ` Cédric Le Goater
2025-02-06 13:14 ` [PATCH v3 3/7] vfio: Rephrase comment in vfio_listener_region_add() error path Cédric Le Goater
` (6 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2025-02-06 13:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Williamson, Cédric Le Goater
This is to be consistent with other reported errors related to vIOMMU
devices.
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 1b90c78c5a1927b1553c59de7a470544bc07788a..90570e9e4ee571d3ec5a7f7b302b1e7e7f0c9a33 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3116,7 +3116,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
if (!vbasedev->mdev &&
!pci_device_set_iommu_device(pdev, vbasedev->hiod, errp)) {
- error_prepend(errp, "Failed to set iommu_device: ");
+ error_prepend(errp, "Failed to set vIOMMU: ");
goto out_teardown;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 3/7] vfio: Rephrase comment in vfio_listener_region_add() error path
2025-02-06 13:14 [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails Cédric Le Goater
2025-02-06 13:14 ` [PATCH v3 1/7] util/error: Introduce warn_report_err_once() Cédric Le Goater
2025-02-06 13:14 ` [PATCH v3 2/7] vfio/pci: Replace "iommu_device" by "vIOMMU" Cédric Le Goater
@ 2025-02-06 13:14 ` Cédric Le Goater
2025-02-06 13:14 ` [PATCH v3 4/7] vfio: Introduce vfio_get_vfio_device() Cédric Le Goater
` (5 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2025-02-06 13:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Williamson, Cédric Le Goater
Rephrase a bit the ending comment about how errors are handled
depending on the phase in which vfio_listener_region_add() is called.
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/common.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index f7499a9b7447a7593198e1523c50858b70a8bd85..62af1216fc5a9089fc718c2afe3a405d9381db32 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -683,12 +683,13 @@ fail:
error_reportf_err(err, "PCI p2p may not work: ");
return;
}
- /*
- * On the initfn path, store the first error in the container so we
- * can gracefully fail. Runtime, there's not much we can do other
- * than throw a hardware error.
- */
+
if (!bcontainer->initialized) {
+ /*
+ * At machine init time or when the device is attached to the
+ * VM, store the first error in the container so we can
+ * gracefully fail the device realize routine.
+ */
if (!bcontainer->error) {
error_propagate_prepend(&bcontainer->error, err,
"Region %s: ",
@@ -697,6 +698,10 @@ fail:
error_free(err);
}
} else {
+ /*
+ * At runtime, there's not much we can do other than throw a
+ * hardware error.
+ */
error_report_err(err);
hw_error("vfio: DMA mapping failed, unable to continue");
}
--
2.48.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 4/7] vfio: Introduce vfio_get_vfio_device()
2025-02-06 13:14 [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails Cédric Le Goater
` (2 preceding siblings ...)
2025-02-06 13:14 ` [PATCH v3 3/7] vfio: Rephrase comment in vfio_listener_region_add() error path Cédric Le Goater
@ 2025-02-06 13:14 ` Cédric Le Goater
2025-02-06 13:14 ` [PATCH v3 5/7] vfio: Improve error reporting when MMIO region mapping fails Cédric Le Goater
` (4 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2025-02-06 13:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Williamson, Cédric Le Goater
This helper will be useful in the listener handlers to extract the
VFIO device from a memory region using memory_region_owner(). At the
moment, we only care for PCI passthrough devices. If the need arises,
we will add more.
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
include/hw/vfio/vfio-common.h | 1 +
hw/vfio/helpers.c | 10 ++++++++++
2 files changed, 11 insertions(+)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 0c60be5b15c70168f4f94ad7054d9bd750a162d3..ac35136a11051b079cd9d04e6becd344a0e0f7e7 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -252,6 +252,7 @@ bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);
bool vfio_attach_device(char *name, VFIODevice *vbasedev,
AddressSpace *as, Error **errp);
void vfio_detach_device(VFIODevice *vbasedev);
+VFIODevice *vfio_get_vfio_device(Object *obj);
int vfio_kvm_device_add_fd(int fd, Error **errp);
int vfio_kvm_device_del_fd(int fd, Error **errp);
diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index 913796f437f84eece8711cb4b4b654a44040d17c..4b255d4f3a9e81f55df00c68fc71da769fd5bd04 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -23,6 +23,7 @@
#include <sys/ioctl.h>
#include "hw/vfio/vfio-common.h"
+#include "hw/vfio/pci.h"
#include "hw/hw.h"
#include "trace.h"
#include "qapi/error.h"
@@ -728,3 +729,12 @@ bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp)
return HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp);
}
+
+VFIODevice *vfio_get_vfio_device(Object *obj)
+{
+ if (object_dynamic_cast(obj, TYPE_VFIO_PCI)) {
+ return &VFIO_PCI(obj)->vbasedev;
+ } else {
+ return NULL;
+ }
+}
--
2.48.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 5/7] vfio: Improve error reporting when MMIO region mapping fails
2025-02-06 13:14 [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails Cédric Le Goater
` (3 preceding siblings ...)
2025-02-06 13:14 ` [PATCH v3 4/7] vfio: Introduce vfio_get_vfio_device() Cédric Le Goater
@ 2025-02-06 13:14 ` Cédric Le Goater
2025-02-06 13:14 ` [PATCH v3 6/7] vfio: Remove reports of DMA mapping errors in backends Cédric Le Goater
` (3 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2025-02-06 13:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Williamson, Cédric Le Goater
When the IOMMU address space width is smaller than the physical
address width, a MMIO region of a device can fail to map because the
region is outside the supported IOVA ranges of the VM. In this case,
PCI peer-to-peer transactions on BARs are not supported. This can
occur with the 39-bit IOMMU address space width, as can be the case on
some Intel consumer processors, or when using a vIOMMU device with
default settings.
The current error message is unclear, improve it and also change the
error report to a warning because it is a non fatal condition for the
VM. To prevent excessive log messages, restrict these recurring DMA
mapping errors to a single warning at runtime.
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/common.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 62af1216fc5a9089fc718c2afe3a405d9381db32..4fca4c6166f761acceb7b57b52e379603ea53876 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -555,6 +555,18 @@ static bool vfio_get_section_iova_range(VFIOContainerBase *bcontainer,
return true;
}
+static void vfio_device_error_append(VFIODevice *vbasedev, Error **errp)
+{
+ /*
+ * MMIO region mapping failures are not fatal but in this case PCI
+ * peer-to-peer transactions are broken.
+ */
+ if (vbasedev && vbasedev->type == VFIO_DEVICE_TYPE_PCI) {
+ error_append_hint(errp, "%s: PCI peer-to-peer transactions "
+ "on BARs are not supported.\n", vbasedev->name);
+ }
+}
+
static void vfio_listener_region_add(MemoryListener *listener,
MemoryRegionSection *section)
{
@@ -670,7 +682,10 @@ static void vfio_listener_region_add(MemoryListener *listener,
strerror(-ret));
if (memory_region_is_ram_device(section->mr)) {
/* Allow unexpected mappings not to be fatal for RAM devices */
- error_report_err(err);
+ VFIODevice *vbasedev =
+ vfio_get_vfio_device(memory_region_owner(section->mr));
+ vfio_device_error_append(vbasedev, &err);
+ warn_report_err_once(err);
return;
}
goto fail;
--
2.48.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 6/7] vfio: Remove reports of DMA mapping errors in backends
2025-02-06 13:14 [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails Cédric Le Goater
` (4 preceding siblings ...)
2025-02-06 13:14 ` [PATCH v3 5/7] vfio: Improve error reporting when MMIO region mapping fails Cédric Le Goater
@ 2025-02-06 13:14 ` Cédric Le Goater
2025-02-06 13:14 ` [PATCH v3 7/7] vfio: Remove superfluous error report in vfio_listener_region_add() Cédric Le Goater
` (2 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2025-02-06 13:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Williamson, Cédric Le Goater
Currently, the mapping handlers of the IOMMU backends, VFIO IOMMU Type
1 aka. legacy and IOMMUFD, return an errno and also report an error.
This can lead to excessive log messages at runtime for recurring DMA
mapping errors. Since these errors are already reported by the callers
in the vfio_container_dma_un/map() routines, simply remove them and
allow the callers to handle the reporting.
The mapping handler of the IOMMUFD backend has a comment suggesting
MMIO region mapping failures return EFAULT. I am not sure this is
entirely true, so keep the EFAULT case until the conditions are
clarified.
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
backends/iommufd.c | 3 ---
hw/vfio/container.c | 2 --
2 files changed, 5 deletions(-)
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 7b4fc8ec460ef635b9ed5ac7b201f124476b512a..d57da44755be3d7fdba74f7dbecfe6d1c89921ba 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -167,8 +167,6 @@ int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id, hwaddr iova,
/* TODO: Not support mapping hardware PCI BAR region for now. */
if (errno == EFAULT) {
warn_report("IOMMU_IOAS_MAP failed: %m, PCI BAR?");
- } else {
- error_report("IOMMU_IOAS_MAP failed: %m");
}
}
return ret;
@@ -203,7 +201,6 @@ int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
if (ret) {
ret = -errno;
- error_report("IOMMU_IOAS_UNMAP failed: %m");
}
return ret;
}
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 4ebb5268088d0a2006e0ed04afec0ee746ed2c1d..7c57bdd27b72731db5cf4f9446d954e143b4747e 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -159,7 +159,6 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer,
unmap.size -= 1ULL << ctz64(bcontainer->pgsizes);
continue;
}
- error_report("VFIO_UNMAP_DMA failed: %s", strerror(errno));
return -errno;
}
@@ -204,7 +203,6 @@ static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
return 0;
}
- error_report("VFIO_MAP_DMA failed: %s", strerror(errno));
return -errno;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 7/7] vfio: Remove superfluous error report in vfio_listener_region_add()
2025-02-06 13:14 [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails Cédric Le Goater
` (5 preceding siblings ...)
2025-02-06 13:14 ` [PATCH v3 6/7] vfio: Remove reports of DMA mapping errors in backends Cédric Le Goater
@ 2025-02-06 13:14 ` Cédric Le Goater
2025-02-06 15:07 ` Harsh Prateek Bora
2025-02-10 16:01 ` [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails Alex Williamson
2025-02-11 13:32 ` Cédric Le Goater
8 siblings, 1 reply; 13+ messages in thread
From: Cédric Le Goater @ 2025-02-06 13:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Williamson, Cédric Le Goater, Harsh Prateek Bora,
Shivaprasad G Bhat
For pseries machines, commit 567b5b309abe ("vfio/pci: Relax DMA map
errors for MMIO regions") introduced 2 error reports to notify the
user of MMIO mapping errors. Consolidate both code paths under one.
Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
Cc: Shivaprasad G Bhat <sbhat@linux.ibm.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/common.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 4fca4c6166f761acceb7b57b52e379603ea53876..abbdc56b6dbb5eed22e7a2d2d55ee5695279661e 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -594,8 +594,9 @@ static void vfio_listener_region_add(MemoryListener *listener,
return;
}
+ /* PPC64/pseries machine only */
if (!vfio_container_add_section_window(bcontainer, section, &err)) {
- goto fail;
+ goto mmio_dma_error;
}
memory_region_ref(section->mr);
@@ -680,6 +681,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
"0x%"HWADDR_PRIx", %p) = %d (%s)",
bcontainer, iova, int128_get64(llsize), vaddr, ret,
strerror(-ret));
+ mmio_dma_error:
if (memory_region_is_ram_device(section->mr)) {
/* Allow unexpected mappings not to be fatal for RAM devices */
VFIODevice *vbasedev =
@@ -694,11 +696,6 @@ static void vfio_listener_region_add(MemoryListener *listener,
return;
fail:
- if (memory_region_is_ram_device(section->mr)) {
- error_reportf_err(err, "PCI p2p may not work: ");
- return;
- }
-
if (!bcontainer->initialized) {
/*
* At machine init time or when the device is attached to the
@@ -806,6 +803,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
memory_region_unref(section->mr);
+ /* PPC64/pseries machine only */
vfio_container_del_section_window(bcontainer, section);
}
--
2.48.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 7/7] vfio: Remove superfluous error report in vfio_listener_region_add()
2025-02-06 13:14 ` [PATCH v3 7/7] vfio: Remove superfluous error report in vfio_listener_region_add() Cédric Le Goater
@ 2025-02-06 15:07 ` Harsh Prateek Bora
0 siblings, 0 replies; 13+ messages in thread
From: Harsh Prateek Bora @ 2025-02-06 15:07 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel; +Cc: Alex Williamson, Shivaprasad G Bhat
On 2/6/25 18:44, Cédric Le Goater wrote:
> For pseries machines, commit 567b5b309abe ("vfio/pci: Relax DMA map
> errors for MMIO regions") introduced 2 error reports to notify the
> user of MMIO mapping errors. Consolidate both code paths under one.
>
> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Cc: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
> hw/vfio/common.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 4fca4c6166f761acceb7b57b52e379603ea53876..abbdc56b6dbb5eed22e7a2d2d55ee5695279661e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -594,8 +594,9 @@ static void vfio_listener_region_add(MemoryListener *listener,
> return;
> }
>
> + /* PPC64/pseries machine only */
> if (!vfio_container_add_section_window(bcontainer, section, &err)) {
> - goto fail;
> + goto mmio_dma_error;
> }
>
> memory_region_ref(section->mr);
> @@ -680,6 +681,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
> "0x%"HWADDR_PRIx", %p) = %d (%s)",
> bcontainer, iova, int128_get64(llsize), vaddr, ret,
> strerror(-ret));
> + mmio_dma_error:
> if (memory_region_is_ram_device(section->mr)) {
> /* Allow unexpected mappings not to be fatal for RAM devices */
> VFIODevice *vbasedev =
> @@ -694,11 +696,6 @@ static void vfio_listener_region_add(MemoryListener *listener,
> return;
>
> fail:
> - if (memory_region_is_ram_device(section->mr)) {
> - error_reportf_err(err, "PCI p2p may not work: ");
Not sure if this specific error msg was intended for a specific case,
but since both ifs check for same condition before returning with error,
looks logically same.
Shiva, any concerns here from vfio perspective?
Otherwise,
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> - return;
> - }
> -
> if (!bcontainer->initialized) {
> /*
> * At machine init time or when the device is attached to the
> @@ -806,6 +803,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
>
> memory_region_unref(section->mr);
>
> + /* PPC64/pseries machine only */
> vfio_container_del_section_window(bcontainer, section);
> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails
2025-02-06 13:14 [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails Cédric Le Goater
` (6 preceding siblings ...)
2025-02-06 13:14 ` [PATCH v3 7/7] vfio: Remove superfluous error report in vfio_listener_region_add() Cédric Le Goater
@ 2025-02-10 16:01 ` Alex Williamson
2025-02-11 13:32 ` Cédric Le Goater
8 siblings, 0 replies; 13+ messages in thread
From: Alex Williamson @ 2025-02-10 16:01 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-devel
On Thu, 6 Feb 2025 14:14:28 +0100
Cédric Le Goater <clg@redhat.com> wrote:
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Hello,
>
> Under certain circumstances, a MMIO region of a device fails to map
> because the region is outside the supported IOVA ranges of the VM. In
> this case, PCI peer-to-peer transactions on BARs are not supported.
> This typically occurs when the IOMMU address space width is less than
> the physical address width, as can be the case on some Intel consumer
> processors or when using a vIOMMU device with default settings.
>
> This series tries to clarify the error message reported to the user.
>
> Thanks,
>
> C.
>
> Changes in v3:
>
> - Fixed warn_report_err_once()
> - Improved commit logs a bit
> - Dropped check on compatibility of CPU and IOMMU address space
> width. Will address later in its own series.
>
> Changes in v2:
>
> - Removed advice on how to resolve the issue. Diagnostic is enough.
> - Introduced helpers
> - Checked device type, since this only applies to PCI
> - Added cleanup
>
> Cédric Le Goater (7):
> util/error: Introduce warn_report_err_once()
> vfio/pci: Replace "iommu_device" by "vIOMMU"
> vfio: Rephrase comment in vfio_listener_region_add() error path
> vfio: Introduce vfio_get_vfio_device()
> vfio: Improve error reporting when MMIO region mapping fails
> vfio: Remove reports of DMA mapping errors in backends
> vfio: Remove superfluous error report in vfio_listener_region_add()
>
> include/hw/vfio/vfio-common.h | 1 +
> include/qapi/error.h | 12 +++++++++++
> backends/iommufd.c | 3 ---
> hw/vfio/common.c | 40 +++++++++++++++++++++++++----------
> hw/vfio/container.c | 2 --
> hw/vfio/helpers.c | 10 +++++++++
> hw/vfio/pci.c | 2 +-
> util/error.c | 11 ++++++++++
> 8 files changed, 64 insertions(+), 17 deletions(-)
>
Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails
2025-02-06 13:14 [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails Cédric Le Goater
` (7 preceding siblings ...)
2025-02-10 16:01 ` [PATCH v3 0/7] vfio: Improve error reporting when MMIO region mapping fails Alex Williamson
@ 2025-02-11 13:32 ` Cédric Le Goater
8 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2025-02-11 13:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Williamson
On 2/6/25 14:14, Cédric Le Goater wrote:
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Hello,
>
> Under certain circumstances, a MMIO region of a device fails to map
> because the region is outside the supported IOVA ranges of the VM. In
> this case, PCI peer-to-peer transactions on BARs are not supported.
> This typically occurs when the IOMMU address space width is less than
> the physical address width, as can be the case on some Intel consumer
> processors or when using a vIOMMU device with default settings.
>
> This series tries to clarify the error message reported to the user.
>
> Thanks,
>
> C.
>
> Changes in v3:
>
> - Fixed warn_report_err_once()
> - Improved commit logs a bit
> - Dropped check on compatibility of CPU and IOMMU address space
> width. Will address later in its own series.
>
> Changes in v2:
>
> - Removed advice on how to resolve the issue. Diagnostic is enough.
> - Introduced helpers
> - Checked device type, since this only applies to PCI
> - Added cleanup
>
> Cédric Le Goater (7):
> util/error: Introduce warn_report_err_once()
> vfio/pci: Replace "iommu_device" by "vIOMMU"
> vfio: Rephrase comment in vfio_listener_region_add() error path
> vfio: Introduce vfio_get_vfio_device()
> vfio: Improve error reporting when MMIO region mapping fails
> vfio: Remove reports of DMA mapping errors in backends
> vfio: Remove superfluous error report in vfio_listener_region_add()
>
> include/hw/vfio/vfio-common.h | 1 +
> include/qapi/error.h | 12 +++++++++++
> backends/iommufd.c | 3 ---
> hw/vfio/common.c | 40 +++++++++++++++++++++++++----------
> hw/vfio/container.c | 2 --
> hw/vfio/helpers.c | 10 +++++++++
> hw/vfio/pci.c | 2 +-
> util/error.c | 11 ++++++++++
> 8 files changed, 64 insertions(+), 17 deletions(-)
>
Applied to vfio-next.
Thanks,
C.
^ permalink raw reply [flat|nested] 13+ messages in thread