* [PATCH v5 0/2] vfio/common: remove spurious tpm-crb-cmd misalignment warning @ 2022-05-06 13:25 Eric Auger 2022-05-06 13:25 ` [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB Eric Auger ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Eric Auger @ 2022-05-06 13:25 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, alex.williamson Cc: stefanb, cohuck, f4bug The CRB command buffer currently is a RAM MemoryRegion and given its base address alignment, it causes an error report on vfio_listener_region_add(). This region could have been a RAM device region, easing the detection of such safe situation but this option was not well received. So let's add a helper function that uses the memory region owner type to detect the situation is safe wrt the assignment. Other device types can be checked here if such kind of problem occurs again. As TPM devices can be compiled out we need to introduce a stub for TPM_IS_CRB. Best Regards Eric This series can be found at: https://github.com/eauger/qemu/tree/tpm-crb-vfio-v5 History: v4 -> v5: - Add sysemu: tpm: Add a stub function for TPM_IS_CRB to fix compilation error if CONFIG_TPM is unset Eric Auger (2): sysemu: tpm: Add a stub function for TPM_IS_CRB vfio/common: remove spurious tpm-crb-cmd misalignment warning hw/vfio/common.c | 27 ++++++++++++++++++++++++++- hw/vfio/trace-events | 1 + include/sysemu/tpm.h | 6 ++++++ 3 files changed, 33 insertions(+), 1 deletion(-) -- 2.35.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB 2022-05-06 13:25 [PATCH v5 0/2] vfio/common: remove spurious tpm-crb-cmd misalignment warning Eric Auger @ 2022-05-06 13:25 ` Eric Auger 2022-05-06 13:47 ` Stefan Berger 2022-05-06 13:25 ` [PATCH v5 2/2] vfio/common: remove spurious tpm-crb-cmd misalignment warning Eric Auger 2022-11-23 6:34 ` [PATCH v5 0/2] " Michael S. Tsirkin 2 siblings, 1 reply; 13+ messages in thread From: Eric Auger @ 2022-05-06 13:25 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, alex.williamson Cc: stefanb, cohuck, f4bug 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(+) 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 */ -- 2.35.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB 2022-05-06 13:25 ` [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB Eric Auger @ 2022-05-06 13:47 ` Stefan Berger 2022-11-23 6:36 ` Michael S. Tsirkin 0 siblings, 1 reply; 13+ messages in thread From: Stefan Berger @ 2022-05-06 13:47 UTC (permalink / raw) To: Eric Auger, eric.auger.pro, qemu-devel, alex.williamson Cc: stefanb, cohuck, f4bug 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 */ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB 2022-05-06 13:47 ` Stefan Berger @ 2022-11-23 6:36 ` Michael S. Tsirkin 2022-11-23 8:18 ` Eric Auger 0 siblings, 1 reply; 13+ messages in thread From: Michael S. Tsirkin @ 2022-11-23 6:36 UTC (permalink / raw) To: Stefan Berger Cc: Eric Auger, eric.auger.pro, qemu-devel, alex.williamson, stefanb, cohuck, f4bug 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 */ > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB 2022-11-23 6:36 ` Michael S. Tsirkin @ 2022-11-23 8:18 ` Eric Auger 2022-11-23 9:30 ` Michael S. Tsirkin 0 siblings, 1 reply; 13+ messages in thread From: Eric Auger @ 2022-11-23 8:18 UTC (permalink / raw) To: Michael S. Tsirkin, Stefan Berger Cc: eric.auger.pro, qemu-devel, alex.williamson, stefanb, cohuck, f4bug 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 */ >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB 2022-11-23 8:18 ` Eric Auger @ 2022-11-23 9:30 ` Michael S. Tsirkin 2022-11-23 11:10 ` Eric Auger 0 siblings, 1 reply; 13+ messages in thread From: Michael S. Tsirkin @ 2022-11-23 9:30 UTC (permalink / raw) To: Eric Auger Cc: Stefan Berger, eric.auger.pro, qemu-devel, alex.williamson, stefanb, cohuck, f4bug 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 */ > >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB 2022-11-23 9:30 ` Michael S. Tsirkin @ 2022-11-23 11:10 ` Eric Auger 2022-11-23 11:24 ` Michael S. Tsirkin 0 siblings, 1 reply; 13+ messages in thread From: Eric Auger @ 2022-11-23 11:10 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Stefan Berger, eric.auger.pro, qemu-devel, alex.williamson, stefanb, cohuck, f4bug 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 */ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB 2022-11-23 11:10 ` Eric Auger @ 2022-11-23 11:24 ` Michael S. Tsirkin 2022-11-23 13:01 ` Eric Auger 0 siblings, 1 reply; 13+ messages in thread From: Michael S. Tsirkin @ 2022-11-23 11:24 UTC (permalink / raw) To: Eric Auger Cc: Stefan Berger, eric.auger.pro, qemu-devel, alex.williamson, stefanb, cohuck, f4bug 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 */ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB 2022-11-23 11:24 ` Michael S. Tsirkin @ 2022-11-23 13:01 ` Eric Auger 2022-11-23 13:49 ` Michael S. Tsirkin 0 siblings, 1 reply; 13+ messages in thread From: Eric Auger @ 2022-11-23 13:01 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Stefan Berger, eric.auger.pro, qemu-devel, alex.williamson, stefanb, cohuck, f4bug 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 */ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB 2022-11-23 13:01 ` Eric Auger @ 2022-11-23 13:49 ` Michael S. Tsirkin 0 siblings, 0 replies; 13+ messages in thread From: Michael S. Tsirkin @ 2022-11-23 13:49 UTC (permalink / raw) To: Eric Auger Cc: Stefan Berger, eric.auger.pro, qemu-devel, alex.williamson, stefanb, cohuck, f4bug 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 */ ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5 2/2] vfio/common: remove spurious tpm-crb-cmd misalignment warning 2022-05-06 13:25 [PATCH v5 0/2] vfio/common: remove spurious tpm-crb-cmd misalignment warning Eric Auger 2022-05-06 13:25 ` [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB Eric Auger @ 2022-05-06 13:25 ` Eric Auger 2022-11-23 6:34 ` [PATCH v5 0/2] " Michael S. Tsirkin 2 siblings, 0 replies; 13+ messages in thread From: Eric Auger @ 2022-05-06 13:25 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, alex.williamson Cc: stefanb, cohuck, f4bug The CRB command buffer currently is a RAM MemoryRegion and given its base address alignment, it causes an error report on vfio_listener_region_add(). This region could have been a RAM device region, easing the detection of such safe situation but this option was not well received. So let's add a helper function that uses the memory region owner type to detect the situation is safe wrt the assignment. Other device types can be checked here if such kind of problem occurs again. Signed-off-by: Eric Auger <eric.auger@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Acked-by: Stefan Berger <stefanb@linux.ibm.com> Reviewed-by: Cornelia Huck <cohuck@redhat.com> --- hw/vfio/common.c | 27 ++++++++++++++++++++++++++- hw/vfio/trace-events | 1 + 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 2b1f78fdfae..f6b9bb6d718 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -40,6 +40,7 @@ #include "trace.h" #include "qapi/error.h" #include "migration/migration.h" +#include "sysemu/tpm.h" VFIOGroupList vfio_group_list = QLIST_HEAD_INITIALIZER(vfio_group_list); @@ -861,6 +862,22 @@ static void vfio_unregister_ram_discard_listener(VFIOContainer *container, g_free(vrdl); } +static bool vfio_known_safe_misalignment(MemoryRegionSection *section) +{ + MemoryRegion *mr = section->mr; + + if (!TPM_IS_CRB(mr->owner)) { + return false; + } + + /* 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; +} + static void vfio_listener_region_add(MemoryListener *listener, MemoryRegionSection *section) { @@ -884,7 +901,15 @@ static void vfio_listener_region_add(MemoryListener *listener, if (unlikely((section->offset_within_address_space & ~qemu_real_host_page_mask()) != (section->offset_within_region & ~qemu_real_host_page_mask()))) { - error_report("%s received unaligned region", __func__); + if (!vfio_known_safe_misalignment(section)) { + error_report("%s received unaligned region %s iova=0x%"PRIx64 + " offset_within_region=0x%"PRIx64 + " qemu_real_host_page_size=0x%"PRIxPTR, + __func__, memory_region_name(section->mr), + section->offset_within_address_space, + section->offset_within_region, + qemu_real_host_page_size()); + } return; } diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index 0ef1b5f4a65..582882db91c 100644 --- a/hw/vfio/trace-events +++ b/hw/vfio/trace-events @@ -100,6 +100,7 @@ vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to liobn fd %d" vfio_listener_region_add_iommu(uint64_t start, uint64_t end) "region_add [iommu] 0x%"PRIx64" - 0x%"PRIx64 vfio_listener_region_add_ram(uint64_t iova_start, uint64_t iova_end, void *vaddr) "region_add [ram] 0x%"PRIx64" - 0x%"PRIx64" [%p]" +vfio_known_safe_misalignment(const char *name, uint64_t iova, uint64_t offset_within_region, uintptr_t page_size) "Region \"%s\" iova=0x%"PRIx64" offset_within_region=0x%"PRIx64" qemu_real_host_page_size=0x%"PRIxPTR ": cannot be mapped for DMA" vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t size, uint64_t page_size) "Region \"%s\" 0x%"PRIx64" size=0x%"PRIx64" is not aligned to 0x%"PRIx64" and cannot be mapped for DMA" vfio_listener_region_del_skip(uint64_t start, uint64_t end) "SKIPPING region_del 0x%"PRIx64" - 0x%"PRIx64 vfio_listener_region_del(uint64_t start, uint64_t end) "region_del 0x%"PRIx64" - 0x%"PRIx64 -- 2.35.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v5 0/2] vfio/common: remove spurious tpm-crb-cmd misalignment warning 2022-05-06 13:25 [PATCH v5 0/2] vfio/common: remove spurious tpm-crb-cmd misalignment warning Eric Auger 2022-05-06 13:25 ` [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB Eric Auger 2022-05-06 13:25 ` [PATCH v5 2/2] vfio/common: remove spurious tpm-crb-cmd misalignment warning Eric Auger @ 2022-11-23 6:34 ` Michael S. Tsirkin 2022-11-23 8:16 ` Eric Auger 2 siblings, 1 reply; 13+ messages in thread From: Michael S. Tsirkin @ 2022-11-23 6:34 UTC (permalink / raw) To: Eric Auger Cc: eric.auger.pro, qemu-devel, alex.williamson, stefanb, cohuck, f4bug On Fri, May 06, 2022 at 03:25:08PM +0200, Eric Auger wrote: > The CRB command buffer currently is a RAM MemoryRegion and given > its base address alignment, it causes an error report on > vfio_listener_region_add(). This region could have been a RAM device > region, easing the detection of such safe situation but this option > was not well received. Eric could you point me at this discussion please? We are now asked to proliferate stuff like this into vdpa as well, this just doesn't scale. I'd like to see whether we can make it a RAM device region after all - was a patch like that posted? > So let's add a helper function that uses the > memory region owner type to detect the situation is safe wrt > the assignment. Other device types can be checked here if such kind > of problem occurs again. > > As TPM devices can be compiled out we need to introduce a stub > for TPM_IS_CRB. > > Best Regards > > Eric > > This series can be found at: > https://github.com/eauger/qemu/tree/tpm-crb-vfio-v5 > > History: > > v4 -> v5: > - Add sysemu: tpm: Add a stub function for TPM_IS_CRB to fix > compilation error if CONFIG_TPM is unset > > Eric Auger (2): > sysemu: tpm: Add a stub function for TPM_IS_CRB > vfio/common: remove spurious tpm-crb-cmd misalignment warning > > hw/vfio/common.c | 27 ++++++++++++++++++++++++++- > hw/vfio/trace-events | 1 + > include/sysemu/tpm.h | 6 ++++++ > 3 files changed, 33 insertions(+), 1 deletion(-) > > -- > 2.35.1 > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 0/2] vfio/common: remove spurious tpm-crb-cmd misalignment warning 2022-11-23 6:34 ` [PATCH v5 0/2] " Michael S. Tsirkin @ 2022-11-23 8:16 ` Eric Auger 0 siblings, 0 replies; 13+ messages in thread From: Eric Auger @ 2022-11-23 8:16 UTC (permalink / raw) To: Michael S. Tsirkin Cc: eric.auger.pro, qemu-devel, alex.williamson, stefanb, cohuck, f4bug Hi Michael, On 11/23/22 07:34, Michael S. Tsirkin wrote: > On Fri, May 06, 2022 at 03:25:08PM +0200, Eric Auger wrote: >> The CRB command buffer currently is a RAM MemoryRegion and given >> its base address alignment, it causes an error report on >> vfio_listener_region_add(). This region could have been a RAM device >> region, easing the detection of such safe situation but this option >> was not well received. > Eric could you point me at this discussion please? > We are now asked to proliferate stuff like this into vdpa > as well, this just doesn't scale. I'd like to see whether we > can make it a RAM device region after all - was a patch > like that posted? The bulk of the discussion happened in https://lore.kernel.org/all/20220208133842.112017-1-eric.auger@redhat.com/#r See exchanges with Peter who was against turning the CRB cmd/response buffer into a RAM device region at that time, hence the current workaround. You will see there also discussions about the buffer size in https://lore.kernel.org/all/eae7e6e6-2f56-c263-f1d2-19104201c8ec@redhat.com/ Thanks Eric > >> So let's add a helper function that uses the >> memory region owner type to detect the situation is safe wrt >> the assignment. Other device types can be checked here if such kind >> of problem occurs again. >> >> As TPM devices can be compiled out we need to introduce a stub >> for TPM_IS_CRB. >> >> Best Regards >> >> Eric >> >> This series can be found at: >> https://github.com/eauger/qemu/tree/tpm-crb-vfio-v5 >> >> History: >> >> v4 -> v5: >> - Add sysemu: tpm: Add a stub function for TPM_IS_CRB to fix >> compilation error if CONFIG_TPM is unset >> >> Eric Auger (2): >> sysemu: tpm: Add a stub function for TPM_IS_CRB >> vfio/common: remove spurious tpm-crb-cmd misalignment warning >> >> hw/vfio/common.c | 27 ++++++++++++++++++++++++++- >> hw/vfio/trace-events | 1 + >> include/sysemu/tpm.h | 6 ++++++ >> 3 files changed, 33 insertions(+), 1 deletion(-) >> >> -- >> 2.35.1 >> >> >> ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-11-23 13:51 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-06 13:25 [PATCH v5 0/2] vfio/common: remove spurious tpm-crb-cmd misalignment warning Eric Auger 2022-05-06 13:25 ` [PATCH v5 1/2] sysemu: tpm: Add a stub function for TPM_IS_CRB Eric Auger 2022-05-06 13:47 ` Stefan Berger 2022-11-23 6:36 ` Michael S. Tsirkin 2022-11-23 8:18 ` Eric Auger 2022-11-23 9:30 ` Michael S. Tsirkin 2022-11-23 11:10 ` Eric Auger 2022-11-23 11:24 ` Michael S. Tsirkin 2022-11-23 13:01 ` Eric Auger 2022-11-23 13:49 ` Michael S. Tsirkin 2022-05-06 13:25 ` [PATCH v5 2/2] vfio/common: remove spurious tpm-crb-cmd misalignment warning Eric Auger 2022-11-23 6:34 ` [PATCH v5 0/2] " Michael S. Tsirkin 2022-11-23 8:16 ` Eric Auger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.