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

From: Eric Auger <eric.auger@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Stefan Berger <stefanb@linux.ibm.com>,
	eric.auger.pro@gmail.com, qemu-devel@nongnu.org,
	alex.williamson@redhat.com, stefanb@linux.vnet.ibm.com,
	cohuck@redhat.com, f4bug@amsat.org
Subject: Re: [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB
Date: Wed, 23 Nov 2022 14:01:32 +0100	[thread overview]
Message-ID: <2ba28eec-d205-d4de-2c48-6ff476ed5633@redhat.com> (raw)
In-Reply-To: <20221123062202-mutt-send-email-mst@kernel.org>


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


>
>
>>>>>>> ---
>>>>>>>   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 */