Re: [PATCH v3 for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning
On Wed, 23 Mar 2022 21:31:19 +0100 Eric Auger wrote: > The CRB command buffer currently is a RAM MemoryRegion and given > its base address alignment, it causes an error report on > vfio_listener_region_add(). This region could have been a RAM device > region, easing the detection of such safe situation but this option > was not well received. So let's add a helper function that uses the > memory region owner type to detect the situation is safe wrt > the assignment. Other device types can be checked here if such kind > of problem occurs again. > > Signed-off-by: Eric Auger > Reviewed-by: Philippe Mathieu-Daudé > > --- > > v2 -> v3: > - Use TPM_IS_CRB() > > v1 -> v2: > - do not check the MR name but rather the owner type > --- > hw/vfio/common.c | 27 ++- > hw/vfio/trace-events | 1 + > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 080046e3f51..55bc116473e 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -40,6 +40,7 @@ > #include "trace.h" > #include "qapi/error.h" > #include "migration/migration.h" > +#include "sysemu/tpm.h" > > VFIOGroupList vfio_group_list = > QLIST_HEAD_INITIALIZER(vfio_group_list); > @@ -861,6 +862,22 @@ static void > vfio_unregister_ram_discard_listener(VFIOContainer *container, > g_free(vrdl); > } > > +static bool vfio_known_safe_misalignment(MemoryRegionSection *section) > +{ > +MemoryRegion *mr = section->mr; > + > +if (!TPM_IS_CRB(mr->owner)) { > +return false; > +} > + > +/* this is a known safe misaligned region, just trace for debug purpose > */ > +trace_vfio_known_safe_misalignment(memory_region_name(mr), > + section->offset_within_address_space, > + section->offset_within_region, > + qemu_real_host_page_size); qemu_real_host_page_size and qemu_real_host_page_mask are now functions. I thought I'd just append "()" in each case, but then the 32-bit build breaks... > +return true; > +} > + > static void vfio_listener_region_add(MemoryListener *listener, > MemoryRegionSection *section) > { > @@ -884,7 +901,15 @@ static void vfio_listener_region_add(MemoryListener > *listener, > if (unlikely((section->offset_within_address_space & >~qemu_real_host_page_mask) != > (section->offset_within_region & > ~qemu_real_host_page_mask))) { > -error_report("%s received unaligned region", __func__); > +if (!vfio_known_safe_misalignment(section)) { > +error_report("%s received unaligned region %s iova=0x%"PRIx64 > + " offset_within_region=0x%"PRIx64 > + " qemu_real_host_page_mask=0x%"PRIxPTR, > + __func__, memory_region_name(section->mr), > + section->offset_within_address_space, > + section->offset_within_region, > + qemu_real_host_page_mask); Note how here we're very verbosely printing "qemu_real_host_page_mask=0x%..." and we're passing the qemu_real_host_page_mask value. In the previous trace command we're passing qemu_real_host_page_size. > +} > return; > } > > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events > index 0ef1b5f4a65..6f38a2e6991 100644 > --- a/hw/vfio/trace-events > +++ b/hw/vfio/trace-events > @@ -100,6 +100,7 @@ vfio_listener_region_add_skip(uint64_t start, uint64_t > end) "SKIPPING region_add > vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to > liobn fd %d" > vfio_listener_region_add_iommu(uint64_t start, uint64_t end) "region_add > [iommu] 0x%"PRIx64" - 0x%"PRIx64 > vfio_listener_region_add_ram(uint64_t iova_start, uint64_t iova_end, void > *vaddr) "region_add [ram] 0x%"PRIx64" - 0x%"PRIx64" [%p]" > +vfio_known_safe_misalignment(const char *name, uint64_t iova, uint64_t > offset_within_region, uint64_t page_size) "Region \"%s\" iova=0x%"PRIx64" > offset_within_region=0x%"PRIx64" qemu_real_host_page_mask=0x%"PRIxPTR ": > cannot be mapped for DMA" So here we've been passed qemu_real_host_page_size but we're again printing "qemu_real_host_page_mask=0x%...". To make things slightly more complicated, qemu_real_host_page_mask is now an intptr_t, which is arbitrarily not supported in trace commands, while qemu_real_host_page_size is a uintptr_t which is supported in trace commands :-\ I'll let you decide how you want to resolve this. Thanks, Alex
Re: [PATCH v3 for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning
On Wed, 23 Mar 2022 21:31:19 +0100 Eric Auger wrote: > The CRB command buffer currently is a RAM MemoryRegion and given > its base address alignment, it causes an error report on > vfio_listener_region_add(). This region could have been a RAM device > region, easing the detection of such safe situation but this option > was not well received. So let's add a helper function that uses the > memory region owner type to detect the situation is safe wrt > the assignment. Other device types can be checked here if such kind > of problem occurs again. > > Signed-off-by: Eric Auger > Reviewed-by: Philippe Mathieu-Daudé > > --- Thanks Eric! I'll queue this for after the v7.0 release with Connie's and Stefan's reviews. Thanks, Alex > > v2 -> v3: > - Use TPM_IS_CRB() > > v1 -> v2: > - do not check the MR name but rather the owner type > --- > hw/vfio/common.c | 27 ++- > hw/vfio/trace-events | 1 + > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 080046e3f51..55bc116473e 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -40,6 +40,7 @@ > #include "trace.h" > #include "qapi/error.h" > #include "migration/migration.h" > +#include "sysemu/tpm.h" > > VFIOGroupList vfio_group_list = > QLIST_HEAD_INITIALIZER(vfio_group_list); > @@ -861,6 +862,22 @@ static void > vfio_unregister_ram_discard_listener(VFIOContainer *container, > g_free(vrdl); > } > > +static bool vfio_known_safe_misalignment(MemoryRegionSection *section) > +{ > +MemoryRegion *mr = section->mr; > + > +if (!TPM_IS_CRB(mr->owner)) { > +return false; > +} > + > +/* this is a known safe misaligned region, just trace for debug purpose > */ > +trace_vfio_known_safe_misalignment(memory_region_name(mr), > + section->offset_within_address_space, > + section->offset_within_region, > + qemu_real_host_page_size); > +return true; > +} > + > static void vfio_listener_region_add(MemoryListener *listener, > MemoryRegionSection *section) > { > @@ -884,7 +901,15 @@ static void vfio_listener_region_add(MemoryListener > *listener, > if (unlikely((section->offset_within_address_space & >~qemu_real_host_page_mask) != > (section->offset_within_region & > ~qemu_real_host_page_mask))) { > -error_report("%s received unaligned region", __func__); > +if (!vfio_known_safe_misalignment(section)) { > +error_report("%s received unaligned region %s iova=0x%"PRIx64 > + " offset_within_region=0x%"PRIx64 > + " qemu_real_host_page_mask=0x%"PRIxPTR, > + __func__, memory_region_name(section->mr), > + section->offset_within_address_space, > + section->offset_within_region, > + qemu_real_host_page_mask); > +} > return; > } > > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events > index 0ef1b5f4a65..6f38a2e6991 100644 > --- a/hw/vfio/trace-events > +++ b/hw/vfio/trace-events > @@ -100,6 +100,7 @@ vfio_listener_region_add_skip(uint64_t start, uint64_t > end) "SKIPPING region_add > vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to > liobn fd %d" > vfio_listener_region_add_iommu(uint64_t start, uint64_t end) "region_add > [iommu] 0x%"PRIx64" - 0x%"PRIx64 > vfio_listener_region_add_ram(uint64_t iova_start, uint64_t iova_end, void > *vaddr) "region_add [ram] 0x%"PRIx64" - 0x%"PRIx64" [%p]" > +vfio_known_safe_misalignment(const char *name, uint64_t iova, uint64_t > offset_within_region, uint64_t page_size) "Region \"%s\" iova=0x%"PRIx64" > offset_within_region=0x%"PRIx64" qemu_real_host_page_mask=0x%"PRIxPTR ": > cannot be mapped for DMA" > vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, > uint64_t size, uint64_t page_size) "Region \"%s\" 0x%"PRIx64" > size=0x%"PRIx64" is not aligned to 0x%"PRIx64" and cannot be mapped for DMA" > vfio_listener_region_del_skip(uint64_t start, uint64_t end) "SKIPPING > region_del 0x%"PRIx64" - 0x%"PRIx64 > vfio_listener_region_del(uint64_t start, uint64_t end) "region_del > 0x%"PRIx64" - 0x%"PRIx64
Re: [PATCH v3 for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning
On 3/24/22 07:50, Cornelia Huck wrote: On Wed, Mar 23 2022, Eric Auger wrote: The CRB command buffer currently is a RAM MemoryRegion and given its base address alignment, it causes an error report on vfio_listener_region_add(). This region could have been a RAM device region, easing the detection of such safe situation but this option was not well received. So let's add a helper function that uses the memory region owner type to detect the situation is safe wrt the assignment. Other device types can be checked here if such kind of problem occurs again. Signed-off-by: Eric Auger Reviewed-by: Philippe Mathieu-Daudé --- v2 -> v3: - Use TPM_IS_CRB() v1 -> v2: - do not check the MR name but rather the owner type --- hw/vfio/common.c | 27 ++- hw/vfio/trace-events | 1 + 2 files changed, 27 insertions(+), 1 deletion(-) Reviewed-by: Cornelia Huck Acked-by: Stefan Berger
Re: [PATCH v3 for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning
On Wed, Mar 23 2022, Eric Auger wrote: > The CRB command buffer currently is a RAM MemoryRegion and given > its base address alignment, it causes an error report on > vfio_listener_region_add(). This region could have been a RAM device > region, easing the detection of such safe situation but this option > was not well received. So let's add a helper function that uses the > memory region owner type to detect the situation is safe wrt > the assignment. Other device types can be checked here if such kind > of problem occurs again. > > Signed-off-by: Eric Auger > Reviewed-by: Philippe Mathieu-Daudé > > --- > > v2 -> v3: > - Use TPM_IS_CRB() > > v1 -> v2: > - do not check the MR name but rather the owner type > --- > hw/vfio/common.c | 27 ++- > hw/vfio/trace-events | 1 + > 2 files changed, 27 insertions(+), 1 deletion(-) Reviewed-by: Cornelia Huck
[PATCH v3 for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning
The CRB command buffer currently is a RAM MemoryRegion and given its base address alignment, it causes an error report on vfio_listener_region_add(). This region could have been a RAM device region, easing the detection of such safe situation but this option was not well received. So let's add a helper function that uses the memory region owner type to detect the situation is safe wrt the assignment. Other device types can be checked here if such kind of problem occurs again. Signed-off-by: Eric Auger Reviewed-by: Philippe Mathieu-Daudé --- v2 -> v3: - Use TPM_IS_CRB() v1 -> v2: - do not check the MR name but rather the owner type --- hw/vfio/common.c | 27 ++- hw/vfio/trace-events | 1 + 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 080046e3f51..55bc116473e 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -40,6 +40,7 @@ #include "trace.h" #include "qapi/error.h" #include "migration/migration.h" +#include "sysemu/tpm.h" VFIOGroupList vfio_group_list = QLIST_HEAD_INITIALIZER(vfio_group_list); @@ -861,6 +862,22 @@ static void vfio_unregister_ram_discard_listener(VFIOContainer *container, g_free(vrdl); } +static bool vfio_known_safe_misalignment(MemoryRegionSection *section) +{ +MemoryRegion *mr = section->mr; + +if (!TPM_IS_CRB(mr->owner)) { +return false; +} + +/* this is a known safe misaligned region, just trace for debug purpose */ +trace_vfio_known_safe_misalignment(memory_region_name(mr), + section->offset_within_address_space, + section->offset_within_region, + qemu_real_host_page_size); +return true; +} + static void vfio_listener_region_add(MemoryListener *listener, MemoryRegionSection *section) { @@ -884,7 +901,15 @@ static void vfio_listener_region_add(MemoryListener *listener, if (unlikely((section->offset_within_address_space & ~qemu_real_host_page_mask) != (section->offset_within_region & ~qemu_real_host_page_mask))) { -error_report("%s received unaligned region", __func__); +if (!vfio_known_safe_misalignment(section)) { +error_report("%s received unaligned region %s iova=0x%"PRIx64 + " offset_within_region=0x%"PRIx64 + " qemu_real_host_page_mask=0x%"PRIxPTR, + __func__, memory_region_name(section->mr), + section->offset_within_address_space, + section->offset_within_region, + qemu_real_host_page_mask); +} return; } diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index 0ef1b5f4a65..6f38a2e6991 100644 --- a/hw/vfio/trace-events +++ b/hw/vfio/trace-events @@ -100,6 +100,7 @@ vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to liobn fd %d" vfio_listener_region_add_iommu(uint64_t start, uint64_t end) "region_add [iommu] 0x%"PRIx64" - 0x%"PRIx64 vfio_listener_region_add_ram(uint64_t iova_start, uint64_t iova_end, void *vaddr) "region_add [ram] 0x%"PRIx64" - 0x%"PRIx64" [%p]" +vfio_known_safe_misalignment(const char *name, uint64_t iova, uint64_t offset_within_region, uint64_t page_size) "Region \"%s\" iova=0x%"PRIx64" offset_within_region=0x%"PRIx64" qemu_real_host_page_mask=0x%"PRIxPTR ": cannot be mapped for DMA" vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t size, uint64_t page_size) "Region \"%s\" 0x%"PRIx64" size=0x%"PRIx64" is not aligned to 0x%"PRIx64" and cannot be mapped for DMA" vfio_listener_region_del_skip(uint64_t start, uint64_t end) "SKIPPING region_del 0x%"PRIx64" - 0x%"PRIx64 vfio_listener_region_del(uint64_t start, uint64_t end) "region_del 0x%"PRIx64" - 0x%"PRIx64 -- 2.26.3