* [PATCH for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning
@ 2022-03-16 20:29 Eric Auger
2022-03-16 23:08 ` Alex Williamson
0 siblings, 1 reply; 6+ messages in thread
From: Eric Auger @ 2022-03-16 20:29 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, alex.williamson
Cc: cohuck, peter.maydell, david, stefanb
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 name to recognize the region and detect the situation
is safe wrt assignment. Other regions can be listed here if such kind
of problem occurs again.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
hw/vfio/common.c | 26 +++++++++++++++++++++++++-
hw/vfio/trace-events | 1 +
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 080046e3f51..b58a38f5c57 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -861,6 +861,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 (strcmp(memory_region_name(mr), "tpm-crb-cmd") != 0) {
+ 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 +900,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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning
2022-03-16 20:29 [PATCH for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning Eric Auger
@ 2022-03-16 23:08 ` Alex Williamson
2022-03-17 13:57 ` Eric Auger
0 siblings, 1 reply; 6+ messages in thread
From: Alex Williamson @ 2022-03-16 23:08 UTC (permalink / raw)
To: Eric Auger
Cc: peter.maydell, stefanb, cohuck, qemu-devel, eric.auger.pro, david
On Wed, 16 Mar 2022 21:29:51 +0100
Eric Auger <eric.auger@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 name to recognize the region and detect the situation
> is safe wrt assignment. Other regions can be listed here if such kind
> of problem occurs again.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
> hw/vfio/common.c | 26 +++++++++++++++++++++++++-
> hw/vfio/trace-events | 1 +
> 2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 080046e3f51..b58a38f5c57 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -861,6 +861,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 (strcmp(memory_region_name(mr), "tpm-crb-cmd") != 0) {
> + return false;
> + }
Hi Eric,
I was thinking more along the lines that we could use
memory_region_owner() to get the owning Object, then on
that we could maybe use INTERFACE_CHECK to look for TYPE_MEMORY_DEVICE,
then consider anything else optional. (a) could something like that
work and (b) do all required mappings currently expose that interface?
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 +900,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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning
2022-03-16 23:08 ` Alex Williamson
@ 2022-03-17 13:57 ` Eric Auger
2022-03-17 14:23 ` Alex Williamson
0 siblings, 1 reply; 6+ messages in thread
From: Eric Auger @ 2022-03-17 13:57 UTC (permalink / raw)
To: Alex Williamson
Cc: peter.maydell, stefanb, cohuck, qemu-devel, eric.auger.pro, david
Hi Alex,
On 3/17/22 12:08 AM, Alex Williamson wrote:
> On Wed, 16 Mar 2022 21:29:51 +0100
> Eric Auger <eric.auger@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 name to recognize the region and detect the situation
>> is safe wrt assignment. Other regions can be listed here if such kind
>> of problem occurs again.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>> hw/vfio/common.c | 26 +++++++++++++++++++++++++-
>> hw/vfio/trace-events | 1 +
>> 2 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 080046e3f51..b58a38f5c57 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -861,6 +861,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 (strcmp(memory_region_name(mr), "tpm-crb-cmd") != 0) {
>> + return false;
>> + }
> Hi Eric,
>
> I was thinking more along the lines that we could use
> memory_region_owner() to get the owning Object, then on
> that we could maybe use INTERFACE_CHECK to look for TYPE_MEMORY_DEVICE,
> then consider anything else optional. (a) could something like that
> work and (b) do all required mappings currently expose that interface?
> Thanks,
If I understand correctly you just want to error_report() misalignement
of MR sections belonging to
TYPE_MEMORY_DEVICE devices and silence the rest? Is that a correct understanding? I thought you wanted to be much more protective and ignore misalignments on a case by case basis hence the white listing of this single tpm-crb-cmd region.
Thanks
Eric
>
> 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 +900,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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning
2022-03-17 13:57 ` Eric Auger
@ 2022-03-17 14:23 ` Alex Williamson
2022-03-17 14:34 ` Eric Auger
0 siblings, 1 reply; 6+ messages in thread
From: Alex Williamson @ 2022-03-17 14:23 UTC (permalink / raw)
To: Eric Auger
Cc: peter.maydell, stefanb, cohuck, qemu-devel, eric.auger.pro, david
On Thu, 17 Mar 2022 14:57:30 +0100
Eric Auger <eric.auger@redhat.com> wrote:
> Hi Alex,
>
> On 3/17/22 12:08 AM, Alex Williamson wrote:
> > On Wed, 16 Mar 2022 21:29:51 +0100
> > Eric Auger <eric.auger@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 name to recognize the region and detect the situation
> >> is safe wrt assignment. Other regions can be listed here if such kind
> >> of problem occurs again.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> ---
> >> hw/vfio/common.c | 26 +++++++++++++++++++++++++-
> >> hw/vfio/trace-events | 1 +
> >> 2 files changed, 26 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >> index 080046e3f51..b58a38f5c57 100644
> >> --- a/hw/vfio/common.c
> >> +++ b/hw/vfio/common.c
> >> @@ -861,6 +861,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 (strcmp(memory_region_name(mr), "tpm-crb-cmd") != 0) {
> >> + return false;
> >> + }
> > Hi Eric,
> >
> > I was thinking more along the lines that we could use
> > memory_region_owner() to get the owning Object, then on
> > that we could maybe use INTERFACE_CHECK to look for TYPE_MEMORY_DEVICE,
> > then consider anything else optional. (a) could something like that
> > work and (b) do all required mappings currently expose that interface?
> > Thanks,
> If I understand correctly you just want to error_report() misalignement
> of MR sections belonging to
>
> TYPE_MEMORY_DEVICE devices and silence the rest? Is that a correct
> understanding? I thought you wanted to be much more protective and
> ignore misalignments on a case by case basis hence the white listing
> of this single tpm-crb-cmd region.
Ah right, so I'm just slipping back into what we currently do, fail for
memory and warn on devices, which would be a generally reasonable long
term plan except people file bugs about those warnings. Crud.
I guess I don't have a better idea than creating essentially an
exception list like this. Do you think it's better to do the strcmp
for the specific memory region or would it maybe be sufficient to test
the owner object is TYPE_TPM_CRB? 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 +900,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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning
2022-03-17 14:23 ` Alex Williamson
@ 2022-03-17 14:34 ` Eric Auger
2022-03-17 18:37 ` Alex Williamson
0 siblings, 1 reply; 6+ messages in thread
From: Eric Auger @ 2022-03-17 14:34 UTC (permalink / raw)
To: Alex Williamson
Cc: peter.maydell, stefanb, cohuck, qemu-devel, eric.auger.pro, david
Hi Alex,
On 3/17/22 3:23 PM, Alex Williamson wrote:
> On Thu, 17 Mar 2022 14:57:30 +0100
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> Hi Alex,
>>
>> On 3/17/22 12:08 AM, Alex Williamson wrote:
>>> On Wed, 16 Mar 2022 21:29:51 +0100
>>> Eric Auger <eric.auger@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 name to recognize the region and detect the situation
>>>> is safe wrt assignment. Other regions can be listed here if such kind
>>>> of problem occurs again.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>> hw/vfio/common.c | 26 +++++++++++++++++++++++++-
>>>> hw/vfio/trace-events | 1 +
>>>> 2 files changed, 26 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>> index 080046e3f51..b58a38f5c57 100644
>>>> --- a/hw/vfio/common.c
>>>> +++ b/hw/vfio/common.c
>>>> @@ -861,6 +861,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 (strcmp(memory_region_name(mr), "tpm-crb-cmd") != 0) {
>>>> + return false;
>>>> + }
>>> Hi Eric,
>>>
>>> I was thinking more along the lines that we could use
>>> memory_region_owner() to get the owning Object, then on
>>> that we could maybe use INTERFACE_CHECK to look for TYPE_MEMORY_DEVICE,
>>> then consider anything else optional. (a) could something like that
>>> work and (b) do all required mappings currently expose that interface?
>>> Thanks,
>> If I understand correctly you just want to error_report() misalignement
>> of MR sections belonging to
>>
>> TYPE_MEMORY_DEVICE devices and silence the rest? Is that a correct
>> understanding? I thought you wanted to be much more protective and
>> ignore misalignments on a case by case basis hence the white listing
>> of this single tpm-crb-cmd region.
> Ah right, so I'm just slipping back into what we currently do, fail for
> memory and warn on devices, which would be a generally reasonable long
> term plan except people file bugs about those warnings. Crud.
>
> I guess I don't have a better idea than creating essentially an
> exception list like this. Do you think it's better to do the strcmp
> for the specific memory region or would it maybe be sufficient to test
> the owner object is TYPE_TPM_CRB? Thanks,
I asked myself the question and eventually chose to be more conservative
with the granularity of the MR. Sometimes objects own several MRs and I
wondered if some misalignments could be considered as safe while others
unsafe, within the same object. Nevertheless I don't have a strong
opinion and will respin according to your preferencee.
Thanks
Eric
>
> 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 +900,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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning
2022-03-17 14:34 ` Eric Auger
@ 2022-03-17 18:37 ` Alex Williamson
0 siblings, 0 replies; 6+ messages in thread
From: Alex Williamson @ 2022-03-17 18:37 UTC (permalink / raw)
To: Eric Auger
Cc: peter.maydell, stefanb, cohuck, qemu-devel, eric.auger.pro, david
On Thu, 17 Mar 2022 15:34:53 +0100
Eric Auger <eric.auger@redhat.com> wrote:
> Hi Alex,
>
> On 3/17/22 3:23 PM, Alex Williamson wrote:
> > On Thu, 17 Mar 2022 14:57:30 +0100
> > Eric Auger <eric.auger@redhat.com> wrote:
> >
> >> Hi Alex,
> >>
> >> On 3/17/22 12:08 AM, Alex Williamson wrote:
> >>> On Wed, 16 Mar 2022 21:29:51 +0100
> >>> Eric Auger <eric.auger@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 name to recognize the region and detect the situation
> >>>> is safe wrt assignment. Other regions can be listed here if such kind
> >>>> of problem occurs again.
> >>>>
> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>>> ---
> >>>> hw/vfio/common.c | 26 +++++++++++++++++++++++++-
> >>>> hw/vfio/trace-events | 1 +
> >>>> 2 files changed, 26 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >>>> index 080046e3f51..b58a38f5c57 100644
> >>>> --- a/hw/vfio/common.c
> >>>> +++ b/hw/vfio/common.c
> >>>> @@ -861,6 +861,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 (strcmp(memory_region_name(mr), "tpm-crb-cmd") != 0) {
> >>>> + return false;
> >>>> + }
> >>> Hi Eric,
> >>>
> >>> I was thinking more along the lines that we could use
> >>> memory_region_owner() to get the owning Object, then on
> >>> that we could maybe use INTERFACE_CHECK to look for TYPE_MEMORY_DEVICE,
> >>> then consider anything else optional. (a) could something like that
> >>> work and (b) do all required mappings currently expose that interface?
> >>> Thanks,
> >> If I understand correctly you just want to error_report() misalignement
> >> of MR sections belonging to
> >>
> >> TYPE_MEMORY_DEVICE devices and silence the rest? Is that a correct
> >> understanding? I thought you wanted to be much more protective and
> >> ignore misalignments on a case by case basis hence the white listing
> >> of this single tpm-crb-cmd region.
> > Ah right, so I'm just slipping back into what we currently do, fail for
> > memory and warn on devices, which would be a generally reasonable long
> > term plan except people file bugs about those warnings. Crud.
> >
> > I guess I don't have a better idea than creating essentially an
> > exception list like this. Do you think it's better to do the strcmp
> > for the specific memory region or would it maybe be sufficient to test
> > the owner object is TYPE_TPM_CRB? Thanks,
> I asked myself the question and eventually chose to be more conservative
> with the granularity of the MR. Sometimes objects own several MRs and I
> wondered if some misalignments could be considered as safe while others
> unsafe, within the same object. Nevertheless I don't have a strong
> opinion and will respin according to your preferencee.
Hi Eric,
As we discussed offline, I think the benefits of being able to test the
type, ie. TYPE_TPM_CRB, might outweigh the flexibility of having per mr
granularity. The strcmp seems like a maintenance red flag since that's
subject to change, though maybe migration support forces it to be more
static than it would otherwise appear. In any case, it's probably not
worth warning about any DMA mapping failures for mr's backed by a tpm
device. 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 +900,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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-17 18:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-16 20:29 [PATCH for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning Eric Auger
2022-03-16 23:08 ` Alex Williamson
2022-03-17 13:57 ` Eric Auger
2022-03-17 14:23 ` Alex Williamson
2022-03-17 14:34 ` Eric Auger
2022-03-17 18:37 ` Alex Williamson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).