Re: [PATCH 2/2] tpm_crb: mark memory as protected
Hi,
as the region is already skipped by the test of the memory region alignment, I'm going to
update my patches by only removing the error_report() as proposed by Peter.
I will replace it by a trace to help to debug.
Thanks,
Laurent
On 7/4/23 05:07, Jason Wang wrote:
On Thu, Jun 22, 2023 at 9:39 PM Laurent Vivier wrote:
On 6/22/23 15:12, Peter Maydell wrote:
On Tue, 20 Jun 2023 at 20:51, Laurent Vivier wrote:
This memory is not correctly aligned and cannot be registered
by vDPA and VFIO.
Isn't this a vDPA/VFIO problem? There's no requirement
for RAM MemoryRegions to be aligned in any way.
It's more about the limitation of the IOMMU which can't do subpage protection.
Code
that doesn't want to work with small or weirdly aligned
regions should skip them if that's the right behaviour
for that particular code IMHO.
We had already had this:
if ((!memory_region_is_ram(section->mr) &&
!memory_region_is_iommu(section->mr)) ||
memory_region_is_protected(section->mr) ||
/* vhost-vDPA doesn't allow MMIO to be mapped */
memory_region_is_ram_device(section->mr)) {
return true;
}
Marc-André proposed to modify vDPA code to skip the region but Michal disagreed:
https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03670.html
No one wants the modification, so the problem cannot be fixed.
Yes, otherwise we end up with explicit check for TPM crb in vhost code...
Thanks
Thanks,
Laurent
Re: [PATCH 2/2] tpm_crb: mark memory as protected
On Thu, Jun 22, 2023 at 9:39 PM Laurent Vivier wrote:
>
> On 6/22/23 15:12, Peter Maydell wrote:
> > On Tue, 20 Jun 2023 at 20:51, Laurent Vivier wrote:
> >>
> >> This memory is not correctly aligned and cannot be registered
> >> by vDPA and VFIO.
> >
> > Isn't this a vDPA/VFIO problem? There's no requirement
> > for RAM MemoryRegions to be aligned in any way.
It's more about the limitation of the IOMMU which can't do subpage protection.
> > Code
> > that doesn't want to work with small or weirdly aligned
> > regions should skip them if that's the right behaviour
> > for that particular code IMHO.
We had already had this:
if ((!memory_region_is_ram(section->mr) &&
!memory_region_is_iommu(section->mr)) ||
memory_region_is_protected(section->mr) ||
/* vhost-vDPA doesn't allow MMIO to be mapped */
memory_region_is_ram_device(section->mr)) {
return true;
}
> >
>
> Marc-André proposed to modify vDPA code to skip the region but Michal
> disagreed:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03670.html
>
> No one wants the modification, so the problem cannot be fixed.
>
Yes, otherwise we end up with explicit check for TPM crb in vhost code...
Thanks
> Thanks,
> Laurent
>
Re: [PATCH 2/2] tpm_crb: mark memory as protected
On Thu, 22 Jun 2023 at 14:39, Laurent Vivier wrote: > > On 6/22/23 15:12, Peter Maydell wrote: > > On Tue, 20 Jun 2023 at 20:51, Laurent Vivier wrote: > >> > >> This memory is not correctly aligned and cannot be registered > >> by vDPA and VFIO. > > > > Isn't this a vDPA/VFIO problem? There's no requirement > > for RAM MemoryRegions to be aligned in any way. Code > > that doesn't want to work with small or weirdly aligned > > regions should skip them if that's the right behaviour > > for that particular code IMHO. > > > > Marc-André proposed to modify vDPA code to skip the region but Michal > disagreed: > > https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03670.html "Special case the TPM device in the vhost-vdpa" is clearly also not the right way to solve the issue, so I agree with Michael about that. The vhost-vdpa code already seems able to correctly detect whether a region is unaligned and ignores it. If that's the right thing to do, maybe we should just remove the error_report() ? Listeners are going to see MemoryRegions which are RAM and which aren't necessarily page-aligned, so they should handle them, not complain about them. thanks -- PMM
Re: [PATCH 2/2] tpm_crb: mark memory as protected
On 6/22/23 15:12, Peter Maydell wrote: On Tue, 20 Jun 2023 at 20:51, Laurent Vivier wrote: This memory is not correctly aligned and cannot be registered by vDPA and VFIO. Isn't this a vDPA/VFIO problem? There's no requirement for RAM MemoryRegions to be aligned in any way. Code that doesn't want to work with small or weirdly aligned regions should skip them if that's the right behaviour for that particular code IMHO. Marc-André proposed to modify vDPA code to skip the region but Michal disagreed: https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03670.html No one wants the modification, so the problem cannot be fixed. Thanks, Laurent
Re: [PATCH 2/2] tpm_crb: mark memory as protected
On Tue, 20 Jun 2023 at 20:51, Laurent Vivier wrote: > > This memory is not correctly aligned and cannot be registered > by vDPA and VFIO. Isn't this a vDPA/VFIO problem? There's no requirement for RAM MemoryRegions to be aligned in any way. Code that doesn't want to work with small or weirdly aligned regions should skip them if that's the right behaviour for that particular code IMHO. > An error is reported for vhost-vdpa case: > qemu-kvm: vhost_vdpa_listener_region_add received unaligned region > > To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED. > > The RAM_PROTECTED flag has been introduced to skip memory > region that looks like RAM but is not accessible via normal > mechanims, including DMA. You can DMA to a small RAM region if you want to... thanks -- PMM
Re: [PATCH 2/2] tpm_crb: mark memory as protected
On 22.06.23 14:59, Laurent Vivier wrote: On 6/21/23 11:13, David Hildenbrand wrote: On 20.06.23 21:50, Laurent Vivier wrote: This memory is not correctly aligned and cannot be registered by vDPA and VFIO. An error is reported for vhost-vdpa case: qemu-kvm: vhost_vdpa_listener_region_add received unaligned region To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED. So, VFIO will simply skip these sections via vfio_listener_valid_section() I guess. Yes, it will report an error but it will happily continue. So regarding vDPA, we're also only concerned about removing the reported error, everything else works as expected? Yes, it has been tested and vDPA works as expected. Okay, so no Fixes: tags required -- Cheers, David / dhildenb
Re: [PATCH 2/2] tpm_crb: mark memory as protected
On 20.06.23 21:50, Laurent Vivier wrote:
This memory is not correctly aligned and cannot be registered
by vDPA and VFIO.
An error is reported for vhost-vdpa case:
qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED.
The RAM_PROTECTED flag has been introduced to skip memory
region that looks like RAM but is not accessible via normal
mechanims, including DMA.
See 56918a126a ("memory: Add RAM_PROTECTED flag to skip IOMMU mappings")
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=2141965
cc: peter.mayd...@linaro.org
cc: marcandre.lur...@redhat.com
cc: eric.au...@redhat.com
cc: m...@redhat.com
cc: jasow...@redhat.com
Signed-off-by: Laurent Vivier
---
hw/tpm/tpm_crb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index ea930da545af..0a93c488f2fa 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -296,7 +296,7 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
memory_region_init_io(>mmio, OBJECT(s), _crb_memory_ops, s,
"tpm-crb-mmio", sizeof(s->regs));
-memory_region_init_ram(>cmdmem, OBJECT(s),
+memory_region_init_ram_protected(>cmdmem, OBJECT(s),
"tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
memory_region_add_subregion(get_system_memory(),
Reviewed-by: David Hildenbrand
--
Cheers,
David / dhildenb
Re: [PATCH 2/2] tpm_crb: mark memory as protected
On 6/21/23 11:13, David Hildenbrand wrote: On 20.06.23 21:50, Laurent Vivier wrote: This memory is not correctly aligned and cannot be registered by vDPA and VFIO. An error is reported for vhost-vdpa case: qemu-kvm: vhost_vdpa_listener_region_add received unaligned region To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED. So, VFIO will simply skip these sections via vfio_listener_valid_section() I guess. Yes, it will report an error but it will happily continue. So regarding vDPA, we're also only concerned about removing the reported error, everything else works as expected? Yes, it has been tested and vDPA works as expected. Thanks, Laurent
Re: [PATCH 2/2] tpm_crb: mark memory as protected
On 6/20/23 15:50, Laurent Vivier wrote:
This memory is not correctly aligned and cannot be registered
by vDPA and VFIO.
An error is reported for vhost-vdpa case:
qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED.
The RAM_PROTECTED flag has been introduced to skip memory
region that looks like RAM but is not accessible via normal
mechanims, including DMA.
See 56918a126a ("memory: Add RAM_PROTECTED flag to skip IOMMU mappings")
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=2141965
cc: peter.mayd...@linaro.org
cc: marcandre.lur...@redhat.com
cc: eric.au...@redhat.com
cc: m...@redhat.com
cc: jasow...@redhat.com
Signed-off-by: Laurent Vivier
Reviewed-by: Stefan Berger
---
hw/tpm/tpm_crb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index ea930da545af..0a93c488f2fa 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -296,7 +296,7 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
memory_region_init_io(>mmio, OBJECT(s), _crb_memory_ops, s,
"tpm-crb-mmio", sizeof(s->regs));
-memory_region_init_ram(>cmdmem, OBJECT(s),
+memory_region_init_ram_protected(>cmdmem, OBJECT(s),
"tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
memory_region_add_subregion(get_system_memory(),
Re: [PATCH 2/2] tpm_crb: mark memory as protected
On 20.06.23 21:50, Laurent Vivier wrote: This memory is not correctly aligned and cannot be registered by vDPA and VFIO. An error is reported for vhost-vdpa case: qemu-kvm: vhost_vdpa_listener_region_add received unaligned region To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED. So, VFIO will simply skip these sections via vfio_listener_valid_section() I guess. Yes, it will report an error but it will happily continue. So regarding vDPA, we're also only concerned about removing the reported error, everything else works as expected? -- Cheers, David / dhildenb
[PATCH 2/2] tpm_crb: mark memory as protected
This memory is not correctly aligned and cannot be registered
by vDPA and VFIO.
An error is reported for vhost-vdpa case:
qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED.
The RAM_PROTECTED flag has been introduced to skip memory
region that looks like RAM but is not accessible via normal
mechanims, including DMA.
See 56918a126a ("memory: Add RAM_PROTECTED flag to skip IOMMU mappings")
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=2141965
cc: peter.mayd...@linaro.org
cc: marcandre.lur...@redhat.com
cc: eric.au...@redhat.com
cc: m...@redhat.com
cc: jasow...@redhat.com
Signed-off-by: Laurent Vivier
---
hw/tpm/tpm_crb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index ea930da545af..0a93c488f2fa 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -296,7 +296,7 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
memory_region_init_io(>mmio, OBJECT(s), _crb_memory_ops, s,
"tpm-crb-mmio", sizeof(s->regs));
-memory_region_init_ram(>cmdmem, OBJECT(s),
+memory_region_init_ram_protected(>cmdmem, OBJECT(s),
"tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
memory_region_add_subregion(get_system_memory(),
--
2.41.0
