On Wed, Nov 23, 2022 at 02:01:32PM +0100, Eric Auger wrote: > > > On 11/23/22 12:24, Michael S. Tsirkin wrote: > > On Wed, Nov 23, 2022 at 12:10:09PM +0100, Eric Auger wrote: > >> > >> On 11/23/22 10:30, Michael S. Tsirkin wrote: > >>> On Wed, Nov 23, 2022 at 09:18:39AM +0100, Eric Auger wrote: > >>>> Hi, > >>>> > >>>> On 11/23/22 07:36, Michael S. Tsirkin wrote: > >>>>> On Fri, May 06, 2022 at 09:47:52AM -0400, Stefan Berger wrote: > >>>>>> On 5/6/22 09:25, Eric Auger wrote: > >>>>>>> In a subsequent patch, VFIO will need to recognize if > >>>>>>> a memory region owner is a TPM CRB device. Hence VFIO > >>>>>>> needs to use TPM_IS_CRB() even if CONFIG_TPM is unset. So > >>>>>>> let's add a stub function. > >>>>>>> > >>>>>>> Signed-off-by: Eric Auger <eric.au...@redhat.com> > >>>>>>> Suggested-by: Cornelia Huck <coh...@redhat.com> > >>>>>> Reviewed-by: Stefan Berger <stef...@linnux.ibm.com> > >>>>> ... and now in 7.2 vdpa needs a dependency on tpm too, what a hack :( > >>>>> And what exactly is it about TPM CRB that everyone needs to > >>>>> know about it and skip it? The API does not tell ... > >>>> An excerpt of one reply I made at that time: > >>>> > >>>> The spec (CG PC Client Platform TPM Profile (PTP) > >>>> Specification Family “2.0” Level 00 Revision 01.03 v22, page 100) > >>>> says that the command/response data "may be defined as large as 3968", > >>>> which is (0x1000 - 0x80), 0x80 being the size of the control struct. > >>>> so the size of the region logically is less than a 4kB page, hence our > >>>> trouble. > >>>> > >>>> We learnt in the past Windows driver has some stronger expectation wrt > >>>> memory mapping. I don't know if those latter would work if we were to > >>>> enlarge the window by some tricks. > >>>> > >>>> https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf > >>>> says > >>>> > >>>> " > >>>> Including the control structure, the three memory areas comprise the > >>>> entirety of the CRB. There are no constraints on how those three memory > >>>> areas are provided. They can all be in system RAM, or all be in device > >>>> memory, or any combination. > >>>> > >>>> Thanks > >>>> > >>>> Eric > >>> So we put it in system RAM then? But why isn't DMA there allowed? > >> I don't think there is any need and since it violates the alignment > >> check in VFIO we discard the region from DMA mapped ones. > >> > >> Thanks > >> > >> Eric > > If that's all then we could just check alignment - > > why are we bothering with a tpm specific hack? > I think Alex prefered to avoid silently skipping the DMA mapping of a > region (a possible scenario may be invalid P2P DMA access?). Except if > we know this region can be safely ignored, which is the case for the TPM > CRB, hence this whitelist. > > Eric
As a vdpa maintainer I might know (more like trust) TPM can be safely ignored right now, but for sure I won't know if that ever changes nor will I remember why down the road. Nor will TPM maintainers remember to go poke at vdpa if this changes. > > > > > > >>>>>>> --- > >>>>>>> include/sysemu/tpm.h | 6 ++++++ > >>>>>>> 1 file changed, 6 insertions(+) > >>>>>>> > >>>>>>> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h > >>>>>>> index 68b2206463c..fb40e30ff60 100644 > >>>>>>> --- a/include/sysemu/tpm.h > >>>>>>> +++ b/include/sysemu/tpm.h > >>>>>>> @@ -80,6 +80,12 @@ static inline TPMVersion tpm_get_version(TPMIf *ti) > >>>>>>> #define tpm_init() (0) > >>>>>>> #define tpm_cleanup() > >>>>>>> > >>>>>>> +/* needed for an alignment check in non-tpm code */ > >>>>>>> +static inline Object *TPM_IS_CRB(Object *obj) > >>>>>>> +{ > >>>>>>> + return NULL; > >>>>>>> +} > >>>>>>> + > >>>>>>> #endif /* CONFIG_TPM */ > >>>>>>> > >>>>>>> #endif /* QEMU_TPM_H */