| Message ID | 20220506132510.1847942-2-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio/common: remove spurious tpm-crb-cmd misalignment warning | expand |
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> > --- > 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 */
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 ... > > --- > > 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 */ > >
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 > >>> --- >>> 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 */ >>
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? > > > >>> --- > >>> 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 */ > >>
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 > >>>>> --- >>>>> 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 */
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? > > > >>>>> --- > >>>>> 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 */
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 */
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 */
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 */
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> --- include/sysemu/tpm.h | 6 ++++++ 1 file changed, 6 insertions(+)