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

[Stefan Berger <stefanb@linux.ibm.com>]

On 5/6/22 03: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.
> 

Solution #4 looks good to me...