[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignmen
From: |
Eric Auger |
Subject: |
Re: [PATCH for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning |
Date: |
Thu, 17 Mar 2022 15:34:53 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 |
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