On Fri, May 06 2022, Eric Auger <eric.au...@redhat.com> wrote: > 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)
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.