Message ID | 20230620195054.23929-3-lvivier@redhat.com |
---|---|
State | New |
Headers | show |
Series | vhost-vdpa: skip TPM CRB memory section | expand |
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?
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.maydell@linaro.org > cc: marcandre.lureau@redhat.com > cc: eric.auger@redhat.com > cc: mst@redhat.com > cc: jasowang@redhat.com > Signed-off-by: Laurent Vivier <lvivier@redhat.com> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > --- > 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(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s, > "tpm-crb-mmio", sizeof(s->regs)); > - memory_region_init_ram(&s->cmdmem, OBJECT(s), > + memory_region_init_ram_protected(&s->cmdmem, OBJECT(s), > "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp); > > memory_region_add_subregion(get_system_memory(),
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
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
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.maydell@linaro.org > cc: marcandre.lureau@redhat.com > cc: eric.auger@redhat.com > cc: mst@redhat.com > cc: jasowang@redhat.com > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > 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(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s, > "tpm-crb-mmio", sizeof(s->regs)); > - memory_region_init_ram(&s->cmdmem, OBJECT(s), > + memory_region_init_ram_protected(&s->cmdmem, OBJECT(s), > "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp); > > memory_region_add_subregion(get_system_memory(), Reviewed-by: David Hildenbrand <david@redhat.com>
On Tue, 20 Jun 2023 at 20:51, Laurent Vivier <lvivier@redhat.com> 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
On 6/22/23 15:12, Peter Maydell wrote: > On Tue, 20 Jun 2023 at 20:51, Laurent Vivier <lvivier@redhat.com> 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
On Thu, 22 Jun 2023 at 14:39, Laurent Vivier <lvivier@redhat.com> wrote: > > On 6/22/23 15:12, Peter Maydell wrote: > > On Tue, 20 Jun 2023 at 20:51, Laurent Vivier <lvivier@redhat.com> 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
On Thu, Jun 22, 2023 at 9:39 PM Laurent Vivier <lvivier@redhat.com> wrote: > > On 6/22/23 15:12, Peter Maydell wrote: > > On Tue, 20 Jun 2023 at 20:51, Laurent Vivier <lvivier@redhat.com> 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 >
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 <lvivier@redhat.com> wrote: >> >> On 6/22/23 15:12, Peter Maydell wrote: >>> On Tue, 20 Jun 2023 at 20:51, Laurent Vivier <lvivier@redhat.com> 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 >> >
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(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s, "tpm-crb-mmio", sizeof(s->regs)); - memory_region_init_ram(&s->cmdmem, OBJECT(s), + memory_region_init_ram_protected(&s->cmdmem, OBJECT(s), "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp); memory_region_add_subregion(get_system_memory(),
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.maydell@linaro.org cc: marcandre.lureau@redhat.com cc: eric.auger@redhat.com cc: mst@redhat.com cc: jasowang@redhat.com Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- hw/tpm/tpm_crb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)