Re: [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB

["Michael S. Tsirkin" <mst@redhat.com>]

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.auger@redhat.com>
> >>>>>>> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> >>>>>> Reviewed-by: Stefan Berger <stefanb@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 */