Hi Alex,
On 4/28/22 22:14, Alex Williamson wrote:
> On Thu, 28 Apr 2022 15:49:45 +0200
> Eric Auger <eric.au...@redhat.com> 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 <eric.au...@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>> Acked-by: Stefan Berger <stef...@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <coh...@redhat.com>
>>
>> ---
>>
>> v3 -> v4:
>> - rebase on top of qemu_real_host_page_size() and
>> qemu_real_host_page_size(). Print the size and make the message
>> consistent
>> - Added Stefan's A-b and Connie R-b (despite the changes)
>> ---
>> 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 2b1f78fdfa..f6b9bb6d71 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;
>> + }
> It looks like this test is going to need to be wrapped in #ifdef
> CONFIG_TPM:
sorry for the delay. Your message fell though the cracks :-(
if I put an '#ifdef CONFIG_TPM' I need to inverse the logic because by
default the function shall return false.
solution #1
#ifdef CONFIG_TPM
if (TPM_IS_CRB(mr->owner)) {
/* 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;
}
#endif
return false;
This looks weird to me.
+ if (!object_dynamic_cast(mr->owner, TYPE_TPM_CRB)) {
+ return false;
+ }
solution #2
replace !object_dynamic_cast(mr->owner, TYPE_TPM_CRB) by
!object_dynamic_cast(mr->owner, "tpm-crb")
and add a comment saying that we don't use TYPE_TPM_CRB on purpose
solution #3
Move #define TPM_IS_CRB(chr) and related defined out of
#ifdef CONFIG_TPM hoping it does not have other side effects
Thoughts?
Eric
>
> https://gitlab.com/alex.williamson/qemu/-/jobs/2391952412
>
> Thanks,
>
> Alex
>
>> +
>> + /* 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_size=0x%"PRIxPTR,
>> + __func__, memory_region_name(section->mr),
>> + section->offset_within_address_space,
>> + section->offset_within_region,
>> + qemu_real_host_page_size());
>> + }
>> return;
>> }
>>
>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>> index 0ef1b5f4a6..582882db91 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, uintptr_t page_size) "Region \"%s\" iova=0x%"PRIx64"
>> offset_within_region=0x%"PRIx64" qemu_real_host_page_size=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
