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

[Eric Auger]

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)

Eric