| Message ID | 20220208133842.112017-2-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | TPM-CRB: Remove spurious error report when used with VFIO | expand |
On Tue, 8 Feb 2022 at 15:08, Eric Auger <eric.auger@redhat.com> wrote: > > Representing the CRB cmd/response buffer as a standard > RAM region causes some trouble when the device is used > with VFIO. Indeed VFIO attempts to DMA_MAP this region > as usual RAM but this latter does not have a valid page > size alignment causing such an error report: > "vfio_listener_region_add received unaligned region". > To allow VFIO to detect that failing dma mapping > this region is not an issue, let's use a ram_device > memory region type instead. This seems like VFIO's problem to me. There's nothing that guarantees alignment for memory regions at all, whether they're RAM, IO or anything else. > + s->crb_cmd_buf = qemu_memalign(qemu_real_host_page_size, > + HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); > + > memory_region_init_io(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s, > "tpm-crb-mmio", sizeof(s->regs)); > - memory_region_init_ram(&s->cmdmem, OBJECT(s), > - "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp); > + memory_region_init_ram_device_ptr(&s->cmdmem, OBJECT(s), "tpm-crb-cmd", > + CRB_CTRL_CMD_SIZE, s->crb_cmd_buf); > + vmstate_register_ram(&s->cmdmem, dev); > > memory_region_add_subregion(get_system_memory(), > TPM_CRB_ADDR_BASE, &s->mmio); > @@ -309,12 +315,25 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp) > qemu_register_reset(tpm_crb_reset, dev); > } As QEMU code goes, this seems much worse than what it replaces. To have a memory region backed by RAM and migrated in the usual way, memory_region_init_ram() is the right thing. thanks -- PMM
Hi Peter, On 2/8/22 4:17 PM, Peter Maydell wrote: > On Tue, 8 Feb 2022 at 15:08, Eric Auger <eric.auger@redhat.com> wrote: >> Representing the CRB cmd/response buffer as a standard >> RAM region causes some trouble when the device is used >> with VFIO. Indeed VFIO attempts to DMA_MAP this region >> as usual RAM but this latter does not have a valid page >> size alignment causing such an error report: >> "vfio_listener_region_add received unaligned region". >> To allow VFIO to detect that failing dma mapping >> this region is not an issue, let's use a ram_device >> memory region type instead. > This seems like VFIO's problem to me. There's nothing > that guarantees alignment for memory regions at all, > whether they're RAM, IO or anything else. VFIO dma maps all the guest RAM. I understand the cmd/response buffer is RAM but does not need to be dma mapped, all the more so it has a bad alignment. By the way the PPI region also has the ram_device type (tpm_ppi.c tpm_ppi_init). In that case, using the ram_device type allows VFIO to discriminate between critical mapping errors and non critical ones. We have no other mean atm. Thanks Eric > >> + s->crb_cmd_buf = qemu_memalign(qemu_real_host_page_size, >> + HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); >> + >> memory_region_init_io(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s, >> "tpm-crb-mmio", sizeof(s->regs)); >> - memory_region_init_ram(&s->cmdmem, OBJECT(s), >> - "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp); >> + memory_region_init_ram_device_ptr(&s->cmdmem, OBJECT(s), "tpm-crb-cmd", >> + CRB_CTRL_CMD_SIZE, s->crb_cmd_buf); >> + vmstate_register_ram(&s->cmdmem, dev); >> >> memory_region_add_subregion(get_system_memory(), >> TPM_CRB_ADDR_BASE, &s->mmio); >> @@ -309,12 +315,25 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp) >> qemu_register_reset(tpm_crb_reset, dev); >> } > As QEMU code goes, this seems much worse than what it replaces. > To have a memory region backed by RAM and migrated in the > usual way, memory_region_init_ram() is the right thing. > > thanks > -- PMM >
On Tue, 8 Feb 2022 at 15:56, Eric Auger <eric.auger@redhat.com> wrote: > > Hi Peter, > > On 2/8/22 4:17 PM, Peter Maydell wrote: > > On Tue, 8 Feb 2022 at 15:08, Eric Auger <eric.auger@redhat.com> wrote: > >> Representing the CRB cmd/response buffer as a standard > >> RAM region causes some trouble when the device is used > >> with VFIO. Indeed VFIO attempts to DMA_MAP this region > >> as usual RAM but this latter does not have a valid page > >> size alignment causing such an error report: > >> "vfio_listener_region_add received unaligned region". > >> To allow VFIO to detect that failing dma mapping > >> this region is not an issue, let's use a ram_device > >> memory region type instead. > > This seems like VFIO's problem to me. There's nothing > > that guarantees alignment for memory regions at all, > > whether they're RAM, IO or anything else. > > VFIO dma maps all the guest RAM. Well, it can if it likes, but "this is a RAM-backed MemoryRegion" doesn't imply "this is really guest actual RAM RAM", so if it's using that as its discriminator it should probably use something else. What is it actually trying to do here ? thanks -- PMM
On Tue, 8 Feb 2022 16:01:48 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On Tue, 8 Feb 2022 at 15:56, Eric Auger <eric.auger@redhat.com> wrote: > > > > Hi Peter, > > > > On 2/8/22 4:17 PM, Peter Maydell wrote: > > > On Tue, 8 Feb 2022 at 15:08, Eric Auger <eric.auger@redhat.com> wrote: > > >> Representing the CRB cmd/response buffer as a standard > > >> RAM region causes some trouble when the device is used > > >> with VFIO. Indeed VFIO attempts to DMA_MAP this region > > >> as usual RAM but this latter does not have a valid page > > >> size alignment causing such an error report: > > >> "vfio_listener_region_add received unaligned region". > > >> To allow VFIO to detect that failing dma mapping > > >> this region is not an issue, let's use a ram_device > > >> memory region type instead. > > > This seems like VFIO's problem to me. There's nothing > > > that guarantees alignment for memory regions at all, > > > whether they're RAM, IO or anything else. > > > > VFIO dma maps all the guest RAM. > > Well, it can if it likes, but "this is a RAM-backed MemoryRegion" > doesn't imply "this is really guest actual RAM RAM", so if it's > using that as its discriminator it should probably use something else. > What is it actually trying to do here ? VFIO is device agnostic, we don't understand the device programming model, we can't know how the device is programmed to perform DMA. The only way we can provide transparent assignment of arbitrary PCI devices is to install DMA mappings for everything in the device AddressSpace through the system IOMMU. If we were to get a sub-page RAM mapping through the MemoryListener and that mapping had the possibility of being a DMA target, then we have a problem, because we cannot represent that through the IOMMU. If the device were to use that address for DMA, we'd likely have data loss/corruption in the VM. AFAIK, and I thought we had some general agreement on this, declaring device memory as ram_device is the only means we have to differentiate MemoryRegion segments generated by a device from actual system RAM. For device memory, we can lean on the fact that peer-to-peer DMA is much more rare and likely involves some degree of validation by the drivers since it can be blocked on physical hardware due to various topology and chipset related issues. Therefore we can consider failures to map device memory at a lower risk than failures to map ranges we think are actual system RAM. Are there better approaches? We can't rely on the device sitting behind a vIOMMU in the guest to restrict the address space and we can't afford the performance hit for dyanmic DMA mappings through a vIOMMU either. Thanks, Alex
Hi, On 2/8/22 5:01 PM, Peter Maydell wrote: > On Tue, 8 Feb 2022 at 15:56, Eric Auger <eric.auger@redhat.com> wrote: >> Hi Peter, >> >> On 2/8/22 4:17 PM, Peter Maydell wrote: >>> On Tue, 8 Feb 2022 at 15:08, Eric Auger <eric.auger@redhat.com> wrote: >>>> Representing the CRB cmd/response buffer as a standard >>>> RAM region causes some trouble when the device is used >>>> with VFIO. Indeed VFIO attempts to DMA_MAP this region >>>> as usual RAM but this latter does not have a valid page >>>> size alignment causing such an error report: >>>> "vfio_listener_region_add received unaligned region". >>>> To allow VFIO to detect that failing dma mapping >>>> this region is not an issue, let's use a ram_device >>>> memory region type instead. >>> This seems like VFIO's problem to me. There's nothing >>> that guarantees alignment for memory regions at all, >>> whether they're RAM, IO or anything else. >> VFIO dma maps all the guest RAM. > Well, it can if it likes, but "this is a RAM-backed MemoryRegion" > doesn't imply "this is really guest actual RAM RAM", so if it's > using that as its discriminator it should probably use something else. > What is it actually trying to do here ? We avoid outputting an error msg for something that is not an issue. Besides, a little bit farther in hw/vfio/common.c (vfio_listener_region_add) memory_region_is_ram_device() already is used to avoid doing the dma_map for this type of region. Originally, according to 21e00fa55f3 ("memory: Replace skip_dump flag with "ram_device"), we had a skip_dump field in MemoryRegion which was then turned into a whole ram_device type. Doing it differently would mean that we would somehow introduce a new flag saying skip_dma_map? Originally this was mainly meant for MMIO bars but I understood from Alex that it could be sensible in that case too. Thanks Eric > > thanks > -- PMM >
* Eric Auger (eric.auger@redhat.com) wrote: > Representing the CRB cmd/response buffer as a standard > RAM region causes some trouble when the device is used > with VFIO. Indeed VFIO attempts to DMA_MAP this region > as usual RAM but this latter does not have a valid page > size alignment causing such an error report: > "vfio_listener_region_add received unaligned region". > To allow VFIO to detect that failing dma mapping > this region is not an issue, let's use a ram_device > memory region type instead. Don't the weird sizes and alignments also cause problems with the RAM migration code? Why don't you just align this up to a convenient boundary? Dave > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Tested-by: Stefan Berger <stefanb@linux.ibm.com> > Acked-by: Stefan Berger <stefanb@linux.ibm.com> > [PMD: Keep tpm_crb.c in meson's softmmu_ss] > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > v3 -> v4: > - call vmstate_unregister_ram > --- > hw/tpm/tpm_crb.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c > index 58ebd1469c3..668f969b409 100644 > --- a/hw/tpm/tpm_crb.c > +++ b/hw/tpm/tpm_crb.c > @@ -25,6 +25,7 @@ > #include "sysemu/tpm_backend.h" > #include "sysemu/tpm_util.h" > #include "sysemu/reset.h" > +#include "exec/cpu-common.h" > #include "tpm_prop.h" > #include "tpm_ppi.h" > #include "trace.h" > @@ -43,6 +44,7 @@ struct CRBState { > > bool ppi_enabled; > TPMPPI ppi; > + uint8_t *crb_cmd_buf; > }; > typedef struct CRBState CRBState; > > @@ -291,10 +293,14 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp) > return; > } > > + s->crb_cmd_buf = qemu_memalign(qemu_real_host_page_size, > + HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); > + > memory_region_init_io(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s, > "tpm-crb-mmio", sizeof(s->regs)); > - memory_region_init_ram(&s->cmdmem, OBJECT(s), > - "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp); > + memory_region_init_ram_device_ptr(&s->cmdmem, OBJECT(s), "tpm-crb-cmd", > + CRB_CTRL_CMD_SIZE, s->crb_cmd_buf); > + vmstate_register_ram(&s->cmdmem, dev); > > memory_region_add_subregion(get_system_memory(), > TPM_CRB_ADDR_BASE, &s->mmio); > @@ -309,12 +315,25 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp) > qemu_register_reset(tpm_crb_reset, dev); > } > > +static void tpm_crb_unrealize(DeviceState *dev) > +{ > + CRBState *s = CRB(dev); > + > + vmstate_unregister_ram(&s->cmdmem, dev); > + qemu_vfree(s->crb_cmd_buf); > + > + if (s->ppi_enabled) { > + qemu_vfree(s->ppi.buf); > + } > +} > + > static void tpm_crb_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > TPMIfClass *tc = TPM_IF_CLASS(klass); > > dc->realize = tpm_crb_realize; > + dc->unrealize = tpm_crb_unrealize; > device_class_set_props(dc, tpm_crb_properties); > dc->vmsd = &vmstate_tpm_crb; > dc->user_creatable = true; > -- > 2.26.3 >
On Tue, 8 Feb 2022 at 16:36, Alex Williamson <alex.williamson@redhat.com> wrote: > > On Tue, 8 Feb 2022 16:01:48 +0000 > Peter Maydell <peter.maydell@linaro.org> wrote: > > Well, it can if it likes, but "this is a RAM-backed MemoryRegion" > > doesn't imply "this is really guest actual RAM RAM", so if it's > > using that as its discriminator it should probably use something else. > > What is it actually trying to do here ? > > VFIO is device agnostic, we don't understand the device programming > model, we can't know how the device is programmed to perform DMA. The > only way we can provide transparent assignment of arbitrary PCI devices > is to install DMA mappings for everything in the device AddressSpace > through the system IOMMU. If we were to get a sub-page RAM mapping > through the MemoryListener and that mapping had the possibility of > being a DMA target, then we have a problem, because we cannot represent > that through the IOMMU. If the device were to use that address for DMA, > we'd likely have data loss/corruption in the VM. This is true whether that small MR is RAM-backed or MMIO-backed or RAM-backed and marked as being a "ram device", though, isn't it ? > AFAIK, and I thought we had some general agreement on this, declaring > device memory as ram_device is the only means we have to differentiate > MemoryRegion segments generated by a device from actual system RAM. What do you mean by "generated by a device" here? Devices within QEMU create MemoryRegions of all kinds; some of them are RAM-backed, some of them are not. memory_region_init_ram_device_ptr() is (per the documentation) for when the backing device is a real host device that vfio is wrapping to turn into a QEMU device. tpm_crb is not a real host device, though -- it's an actually emulated-by-QEMU device. > For device memory, we can lean on the fact that peer-to-peer DMA is > much more rare and likely involves some degree of validation by the > drivers since it can be blocked on physical hardware due to various > topology and chipset related issues. Therefore we can consider > failures to map device memory at a lower risk than failures to map > ranges we think are actual system RAM. Well, if it's not page aligned and at least page sized it's a pretty reasonable guess that it's not system RAM... thanks -- PMM
On 2/8/22 08:38, Eric Auger wrote: > Representing the CRB cmd/response buffer as a standard > RAM region causes some trouble when the device is used > with VFIO. Indeed VFIO attempts to DMA_MAP this region > as usual RAM but this latter does not have a valid page > size alignment causing such an error report: > "vfio_listener_region_add received unaligned region". > To allow VFIO to detect that failing dma mapping > this region is not an issue, let's use a ram_device > memory region type instead. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Tested-by: Stefan Berger <stefanb@linux.ibm.com> > Acked-by: Stefan Berger <stefanb@linux.ibm.com> > [PMD: Keep tpm_crb.c in meson's softmmu_ss] > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> v4 doesn't build for me: ../hw/tpm/tpm_crb.c: In function ?tpm_crb_realize?: ../hw/tpm/tpm_crb.c:297:33: error: implicit declaration of function ?HOST_PAGE_ALIGN? [-Werror=implicit-function-declaration] 297 | HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); | ^~~~~~~~~~~~~~~ ../hw/tpm/tpm_crb.c:297:33: error: nested extern declaration of ?HOST_PAGE_ALIGN? [-Werror=nested-externs] cc1: all warnings being treated as errors
Hi Stefan, On 2/8/22 6:16 PM, Stefan Berger wrote: > > On 2/8/22 08:38, Eric Auger wrote: >> Representing the CRB cmd/response buffer as a standard >> RAM region causes some trouble when the device is used >> with VFIO. Indeed VFIO attempts to DMA_MAP this region >> as usual RAM but this latter does not have a valid page >> size alignment causing such an error report: >> "vfio_listener_region_add received unaligned region". >> To allow VFIO to detect that failing dma mapping >> this region is not an issue, let's use a ram_device >> memory region type instead. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Tested-by: Stefan Berger <stefanb@linux.ibm.com> >> Acked-by: Stefan Berger <stefanb@linux.ibm.com> >> [PMD: Keep tpm_crb.c in meson's softmmu_ss] >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > v4 doesn't build for me: > > ../hw/tpm/tpm_crb.c: In function ?tpm_crb_realize?: > ../hw/tpm/tpm_crb.c:297:33: error: implicit declaration of function > ?HOST_PAGE_ALIGN? [-Werror=implicit-function-declaration] > 297 | HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); > | ^~~~~~~~~~~~~~~ > ../hw/tpm/tpm_crb.c:297:33: error: nested extern declaration of > ?HOST_PAGE_ALIGN? [-Werror=nested-externs] > cc1: all warnings being treated as errors Do you have b269a70810a exec/cpu: Make host pages variables / macros 'target agnostic' in your tree? Thanks Eric > > >
Hi Dave, On 2/8/22 6:03 PM, Dr. David Alan Gilbert wrote: > * Eric Auger (eric.auger@redhat.com) wrote: >> Representing the CRB cmd/response buffer as a standard >> RAM region causes some trouble when the device is used >> with VFIO. Indeed VFIO attempts to DMA_MAP this region >> as usual RAM but this latter does not have a valid page >> size alignment causing such an error report: >> "vfio_listener_region_add received unaligned region". >> To allow VFIO to detect that failing dma mapping >> this region is not an issue, let's use a ram_device >> memory region type instead. > Don't the weird sizes and alignments also cause problems with the RAM > migration code? I tested CRB migration and it seems to work properly. > Why don't you just align this up to a convenient boundary? 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. Besides 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. " So device memory looks an option too. Adding Marc-Andre in the loop Thanks Eric > > Dave > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Tested-by: Stefan Berger <stefanb@linux.ibm.com> >> Acked-by: Stefan Berger <stefanb@linux.ibm.com> >> [PMD: Keep tpm_crb.c in meson's softmmu_ss] >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >> --- >> >> v3 -> v4: >> - call vmstate_unregister_ram >> --- >> hw/tpm/tpm_crb.c | 23 +++++++++++++++++++++-- >> 1 file changed, 21 insertions(+), 2 deletions(-) >> >> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c >> index 58ebd1469c3..668f969b409 100644 >> --- a/hw/tpm/tpm_crb.c >> +++ b/hw/tpm/tpm_crb.c >> @@ -25,6 +25,7 @@ >> #include "sysemu/tpm_backend.h" >> #include "sysemu/tpm_util.h" >> #include "sysemu/reset.h" >> +#include "exec/cpu-common.h" >> #include "tpm_prop.h" >> #include "tpm_ppi.h" >> #include "trace.h" >> @@ -43,6 +44,7 @@ struct CRBState { >> >> bool ppi_enabled; >> TPMPPI ppi; >> + uint8_t *crb_cmd_buf; >> }; >> typedef struct CRBState CRBState; >> >> @@ -291,10 +293,14 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp) >> return; >> } >> >> + s->crb_cmd_buf = qemu_memalign(qemu_real_host_page_size, >> + HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); >> + >> memory_region_init_io(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s, >> "tpm-crb-mmio", sizeof(s->regs)); >> - memory_region_init_ram(&s->cmdmem, OBJECT(s), >> - "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp); >> + memory_region_init_ram_device_ptr(&s->cmdmem, OBJECT(s), "tpm-crb-cmd", >> + CRB_CTRL_CMD_SIZE, s->crb_cmd_buf); >> + vmstate_register_ram(&s->cmdmem, dev); >> >> memory_region_add_subregion(get_system_memory(), >> TPM_CRB_ADDR_BASE, &s->mmio); >> @@ -309,12 +315,25 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp) >> qemu_register_reset(tpm_crb_reset, dev); >> } >> >> +static void tpm_crb_unrealize(DeviceState *dev) >> +{ >> + CRBState *s = CRB(dev); >> + >> + vmstate_unregister_ram(&s->cmdmem, dev); >> + qemu_vfree(s->crb_cmd_buf); >> + >> + if (s->ppi_enabled) { >> + qemu_vfree(s->ppi.buf); >> + } >> +} >> + >> static void tpm_crb_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> TPMIfClass *tc = TPM_IF_CLASS(klass); >> >> dc->realize = tpm_crb_realize; >> + dc->unrealize = tpm_crb_unrealize; >> device_class_set_props(dc, tpm_crb_properties); >> dc->vmsd = &vmstate_tpm_crb; >> dc->user_creatable = true; >> -- >> 2.26.3 >>
Hi Peter, On 2/8/22 6:14 PM, Peter Maydell wrote: > On Tue, 8 Feb 2022 at 16:36, Alex Williamson <alex.williamson@redhat.com> wrote: >> On Tue, 8 Feb 2022 16:01:48 +0000 >> Peter Maydell <peter.maydell@linaro.org> wrote: >>> Well, it can if it likes, but "this is a RAM-backed MemoryRegion" >>> doesn't imply "this is really guest actual RAM RAM", so if it's >>> using that as its discriminator it should probably use something else. >>> What is it actually trying to do here ? >> VFIO is device agnostic, we don't understand the device programming >> model, we can't know how the device is programmed to perform DMA. The >> only way we can provide transparent assignment of arbitrary PCI devices >> is to install DMA mappings for everything in the device AddressSpace >> through the system IOMMU. If we were to get a sub-page RAM mapping >> through the MemoryListener and that mapping had the possibility of >> being a DMA target, then we have a problem, because we cannot represent >> that through the IOMMU. If the device were to use that address for DMA, >> we'd likely have data loss/corruption in the VM. > This is true whether that small MR is RAM-backed or MMIO-backed > or RAM-backed and marked as being a "ram device", though, > isn't it ? > >> AFAIK, and I thought we had some general agreement on this, declaring >> device memory as ram_device is the only means we have to differentiate >> MemoryRegion segments generated by a device from actual system RAM. > What do you mean by "generated by a device" here? Devices within > QEMU create MemoryRegions of all kinds; some of them are RAM-backed, > some of them are not. > > memory_region_init_ram_device_ptr() is (per the documentation) > for when the backing device is a real host device that vfio is > wrapping to turn into a QEMU device. > > tpm_crb is not a real host device, though -- it's an actually > emulated-by-QEMU device. CRB can work in passthrough mode though, although I don't know the underlying implementation. As mentionned in the other email 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. > >> For device memory, we can lean on the fact that peer-to-peer DMA is >> much more rare and likely involves some degree of validation by the >> drivers since it can be blocked on physical hardware due to various >> topology and chipset related issues. Therefore we can consider >> failures to map device memory at a lower risk than failures to map >> ranges we think are actual system RAM. > Well, if it's not page aligned and at least page sized it's > a pretty reasonable guess that it's not system RAM... Assuming that obviously makes things simpler and remove a bunch of checks & error reports in vfio :) But wouldn' we silence some actual dma-map failures we properly detect & report today? Thanks Eric > > thanks > -- PMM >
Hi Stefan, On 2/8/22 6:58 PM, Eric Auger wrote: > Hi Stefan, > > On 2/8/22 6:16 PM, Stefan Berger wrote: >> >> On 2/8/22 08:38, Eric Auger wrote: >>> Representing the CRB cmd/response buffer as a standard >>> RAM region causes some trouble when the device is used >>> with VFIO. Indeed VFIO attempts to DMA_MAP this region >>> as usual RAM but this latter does not have a valid page >>> size alignment causing such an error report: >>> "vfio_listener_region_add received unaligned region". >>> To allow VFIO to detect that failing dma mapping >>> this region is not an issue, let's use a ram_device >>> memory region type instead. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> Tested-by: Stefan Berger <stefanb@linux.ibm.com> >>> Acked-by: Stefan Berger <stefanb@linux.ibm.com> >>> [PMD: Keep tpm_crb.c in meson's softmmu_ss] >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >> >> v4 doesn't build for me: >> >> ../hw/tpm/tpm_crb.c: In function ?tpm_crb_realize?: >> ../hw/tpm/tpm_crb.c:297:33: error: implicit declaration of function >> ?HOST_PAGE_ALIGN? [-Werror=implicit-function-declaration] >> 297 | HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); >> | ^~~~~~~~~~~~~~~ >> ../hw/tpm/tpm_crb.c:297:33: error: nested extern declaration of >> ?HOST_PAGE_ALIGN? [-Werror=nested-externs] >> cc1: all warnings being treated as errors > > Do you have > b269a70810a exec/cpu: Make host pages variables / macros 'target > agnostic' in your tree? I may have missed your reply. Did you have that dependency? Were you able to compile eventually? Besides, do you have any opinion overall about the relevance of transforming the CRB ctrl cmd region into a RAM device wrt the TPM spec? Again spec 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. " (https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf) What was the rationale behind using RAM device for the PPI region? There are some spurious warnings when using CRB with VFIO and that would be nice to remove them one way or the other. Thanks Eric > > Thanks > > Eric >> >> >> >
Hi On Thu, Mar 3, 2022 at 6:41 PM Eric Auger <eauger@redhat.com> wrote: > Hi Stefan, > > On 2/8/22 6:58 PM, Eric Auger wrote: > > Hi Stefan, > > > > On 2/8/22 6:16 PM, Stefan Berger wrote: > >> > >> On 2/8/22 08:38, Eric Auger wrote: > >>> Representing the CRB cmd/response buffer as a standard > >>> RAM region causes some trouble when the device is used > >>> with VFIO. Indeed VFIO attempts to DMA_MAP this region > >>> as usual RAM but this latter does not have a valid page > >>> size alignment causing such an error report: > >>> "vfio_listener_region_add received unaligned region". > >>> To allow VFIO to detect that failing dma mapping > >>> this region is not an issue, let's use a ram_device > >>> memory region type instead. > >>> > >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> > >>> Tested-by: Stefan Berger <stefanb@linux.ibm.com> > >>> Acked-by: Stefan Berger <stefanb@linux.ibm.com> > >>> [PMD: Keep tpm_crb.c in meson's softmmu_ss] > >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >> > >> > >> v4 doesn't build for me: > >> > >> ../hw/tpm/tpm_crb.c: In function ?tpm_crb_realize?: > >> ../hw/tpm/tpm_crb.c:297:33: error: implicit declaration of function > >> ?HOST_PAGE_ALIGN? [-Werror=implicit-function-declaration] > >> 297 | HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); > >> | ^~~~~~~~~~~~~~~ > >> ../hw/tpm/tpm_crb.c:297:33: error: nested extern declaration of > >> ?HOST_PAGE_ALIGN? [-Werror=nested-externs] > >> cc1: all warnings being treated as errors > > > > Do you have > > b269a70810a exec/cpu: Make host pages variables / macros 'target > > agnostic' in your tree? > I may have missed your reply. Did you have that dependency? Were you > able to compile eventually? > > Besides, do you have any opinion overall about the relevance of > transforming the CRB ctrl cmd region into a RAM device wrt the TPM spec? > > Again spec 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. > " > ( > https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf > ) > > What was the rationale behind using RAM device for the PPI region? > Is this the rationale you are looking for? https://gitlab.com/qemu-project/qemu/-/commit/3b97c01e9ccdfbd517a0fd631838d6252dbfa692 Note: bios_linker cannot be used to allocate the PPI memory region, since the reserved memory should stay stable across reboots, and might be needed before the ACPI tables are installed. > > There are some spurious warnings when using CRB with VFIO and that would > be nice to remove them one way or the other. > > Thanks > > Eric > > > > Thanks > > > > Eric > >> > >> > >> > > > > >
Hi Marc-André, On 3/3/22 5:16 PM, Marc-André Lureau wrote: > Hi > > On Thu, Mar 3, 2022 at 6:41 PM Eric Auger <eauger@redhat.com > <mailto:eauger@redhat.com>> wrote: > > Hi Stefan, > > On 2/8/22 6:58 PM, Eric Auger wrote: > > Hi Stefan, > > > > On 2/8/22 6:16 PM, Stefan Berger wrote: > >> > >> On 2/8/22 08:38, Eric Auger wrote: > >>> Representing the CRB cmd/response buffer as a standard > >>> RAM region causes some trouble when the device is used > >>> with VFIO. Indeed VFIO attempts to DMA_MAP this region > >>> as usual RAM but this latter does not have a valid page > >>> size alignment causing such an error report: > >>> "vfio_listener_region_add received unaligned region". > >>> To allow VFIO to detect that failing dma mapping > >>> this region is not an issue, let's use a ram_device > >>> memory region type instead. > >>> > >>> Signed-off-by: Eric Auger <eric.auger@redhat.com > <mailto:eric.auger@redhat.com>> > >>> Tested-by: Stefan Berger <stefanb@linux.ibm.com > <mailto:stefanb@linux.ibm.com>> > >>> Acked-by: Stefan Berger <stefanb@linux.ibm.com > <mailto:stefanb@linux.ibm.com>> > >>> [PMD: Keep tpm_crb.c in meson's softmmu_ss] > >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org > <mailto:f4bug@amsat.org>> > >> > >> > >> v4 doesn't build for me: > >> > >> ../hw/tpm/tpm_crb.c: In function ?tpm_crb_realize?: > >> ../hw/tpm/tpm_crb.c:297:33: error: implicit declaration of function > >> ?HOST_PAGE_ALIGN? [-Werror=implicit-function-declaration] > >> 297 | HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); > >> | ^~~~~~~~~~~~~~~ > >> ../hw/tpm/tpm_crb.c:297:33: error: nested extern declaration of > >> ?HOST_PAGE_ALIGN? [-Werror=nested-externs] > >> cc1: all warnings being treated as errors > > > > Do you have > > b269a70810a exec/cpu: Make host pages variables / macros 'target > > agnostic' in your tree? > I may have missed your reply. Did you have that dependency? Were you > able to compile eventually? > > Besides, do you have any opinion overall about the relevance of > transforming the CRB ctrl cmd region into a RAM device wrt the TPM spec? > > Again spec 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. > " > (https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf > <https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf>) > > What was the rationale behind using RAM device for the PPI region? > > > Is this the rationale you are looking for? > https://gitlab.com/qemu-project/qemu/-/commit/3b97c01e9ccdfbd517a0fd631838d6252dbfa692 > <https://gitlab.com/qemu-project/qemu/-/commit/3b97c01e9ccdfbd517a0fd631838d6252dbfa692> > > Note: bios_linker cannot be used to allocate the PPI memory region, > since the reserved memory should stay stable across reboots, and might > be needed before the ACPI tables are installed. And did this mandate to use "ram_device" memory type instead of standard system RAM. As I understand the spec (statement above), the CRB areas can be implemented as system RAM or device memory. So I want to understand why using RAM device for the CRB is not a reasonable choice. By the way I understand my motivation behind that change is a bit far-fetched and aiming at fixing another issue, but well ;-) Thanks Eric > > > > There are some spurious warnings when using CRB with VFIO and that would > be nice to remove them one way or the other. > > Thanks > > Eric > > > > Thanks > > > > Eric > >> > >> > >> > > > > > > > -- > Marc-André Lureau
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c index 58ebd1469c3..668f969b409 100644 --- a/hw/tpm/tpm_crb.c +++ b/hw/tpm/tpm_crb.c @@ -25,6 +25,7 @@ #include "sysemu/tpm_backend.h" #include "sysemu/tpm_util.h" #include "sysemu/reset.h" +#include "exec/cpu-common.h" #include "tpm_prop.h" #include "tpm_ppi.h" #include "trace.h" @@ -43,6 +44,7 @@ struct CRBState { bool ppi_enabled; TPMPPI ppi; + uint8_t *crb_cmd_buf; }; typedef struct CRBState CRBState; @@ -291,10 +293,14 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp) return; } + s->crb_cmd_buf = qemu_memalign(qemu_real_host_page_size, + HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); + memory_region_init_io(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s, "tpm-crb-mmio", sizeof(s->regs)); - memory_region_init_ram(&s->cmdmem, OBJECT(s), - "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp); + memory_region_init_ram_device_ptr(&s->cmdmem, OBJECT(s), "tpm-crb-cmd", + CRB_CTRL_CMD_SIZE, s->crb_cmd_buf); + vmstate_register_ram(&s->cmdmem, dev); memory_region_add_subregion(get_system_memory(), TPM_CRB_ADDR_BASE, &s->mmio); @@ -309,12 +315,25 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp) qemu_register_reset(tpm_crb_reset, dev); } +static void tpm_crb_unrealize(DeviceState *dev) +{ + CRBState *s = CRB(dev); + + vmstate_unregister_ram(&s->cmdmem, dev); + qemu_vfree(s->crb_cmd_buf); + + if (s->ppi_enabled) { + qemu_vfree(s->ppi.buf); + } +} + static void tpm_crb_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); TPMIfClass *tc = TPM_IF_CLASS(klass); dc->realize = tpm_crb_realize; + dc->unrealize = tpm_crb_unrealize; device_class_set_props(dc, tpm_crb_properties); dc->vmsd = &vmstate_tpm_crb; dc->user_creatable = true;