Hi Connie, On 5/6/22 09:34, Cornelia Huck wrote: > On Fri, May 06 2022, Eric Auger <eric.au...@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.au...@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