Re: [PATCH v4] vfio/common: remove spurious tpm-crb-cmd misalignment warning

[Cornelia Huck]

On Fri, May 06 2022, Eric Auger <eric.auger@redhat.com> wrote:

> Hi Connie,
>
> On 5/6/22 09:34, Cornelia Huck wrote:
>> On Fri, May 06 2022, Eric Auger <eric.auger@redhat.com> wrote:
>>
>>> Hi Alex,
>>>
>>> On 4/28/22 22:14, Alex Williamson wrote:
>>>> On Thu, 28 Apr 2022 15:49:45 +0200
>>>> Eric Auger <eric.auger@redhat.com> wrote:
>>>>> +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
>> solution #4
>>
>> #ifndef CONFIG_TPM
>> /* needed for an alignment check in non-tpm code */
>> static inline Object *TPM_IS_CRB(Object *obj)
>> {
>>     return NULL;
>> }
>> #endif
>>
>> I think it would be good if we could hide the configuration details in
>> the header.
>>
> Yep, I forgot to mention solution #3 also happened in include/sysemu/tpm.h.
> Connie, either we add your stub function or we move the following out of
> the #ifdef CONFIG_TPM. This should be harmless, no?
> Stefan, any preference?
>
> #define TYPE_TPM_TIS_ISA            "tpm-tis"
> #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
> #define TYPE_TPM_CRB                "tpm-crb"
> #define TYPE_TPM_SPAPR              "tpm-spapr"
>
> #define TPM_IS_TIS_ISA(chr)                         \
>     object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
> #define TPM_IS_TIS_SYSBUS(chr)                      \
>     object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
> #define TPM_IS_CRB(chr)                             \
>     object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
> #define TPM_IS_SPAPR(chr)                           \
>     object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)

If it's just those simple defines, I can't see how that could break
things (you won't have the respective objects anyway). So yes, I think
that is the best solution.