* Re: [PATCH-for-7.2] vhost-vdpa: skip TPM CRB memory section
2022-11-22 14:53 [PATCH] vhost-vdpa: skip TPM CRB memory section marcandre.lureau
@ 2022-11-22 15:18 ` Philippe Mathieu-Daudé
2022-11-22 16:05 ` [PATCH] " Eric Auger
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-11-22 15:18 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: eperezma, eric.auger, Michael S. Tsirkin
On 22/11/22 15:53, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> 851d6d1a0f ("vfio/common: remove spurious tpm-crb-cmd misalignment
> warning") removed the warning on vfio_listener_region_add() path.
>
> An error is reported for vhost-vdpa case:
> qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
>
> Skip the CRB device.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=2141965
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/virtio/vhost-vdpa.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 7468e44b87..9d7206e4b8 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -19,6 +19,7 @@
> #include "hw/virtio/virtio-net.h"
> #include "hw/virtio/vhost-shadow-virtqueue.h"
> #include "hw/virtio/vhost-vdpa.h"
> +#include "sysemu/tpm.h"
> #include "exec/address-spaces.h"
> #include "migration/blocker.h"
> #include "qemu/cutils.h"
> @@ -46,6 +47,11 @@ static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
> {
> Int128 llend;
>
> + if (TPM_IS_CRB(section->mr->owner)) {
> + /* The CRB command buffer has its base address unaligned. */
> + return true;
> + }
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] vhost-vdpa: skip TPM CRB memory section
2022-11-22 14:53 [PATCH] vhost-vdpa: skip TPM CRB memory section marcandre.lureau
2022-11-22 15:18 ` [PATCH-for-7.2] " Philippe Mathieu-Daudé
@ 2022-11-22 16:05 ` Eric Auger
2022-11-22 16:49 ` Eugenio Perez Martin
2022-11-22 20:32 ` Michael S. Tsirkin
3 siblings, 0 replies; 13+ messages in thread
From: Eric Auger @ 2022-11-22 16:05 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: eperezma, Michael S. Tsirkin
Hi Marc-André,
On 11/22/22 15:53, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> 851d6d1a0f ("vfio/common: remove spurious tpm-crb-cmd misalignment
> warning") removed the warning on vfio_listener_region_add() path.
>
> An error is reported for vhost-vdpa case:
> qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
>
> Skip the CRB device.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=2141965
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
> ---
> hw/virtio/vhost-vdpa.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 7468e44b87..9d7206e4b8 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -19,6 +19,7 @@
> #include "hw/virtio/virtio-net.h"
> #include "hw/virtio/vhost-shadow-virtqueue.h"
> #include "hw/virtio/vhost-vdpa.h"
> +#include "sysemu/tpm.h"
> #include "exec/address-spaces.h"
> #include "migration/blocker.h"
> #include "qemu/cutils.h"
> @@ -46,6 +47,11 @@ static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
> {
> Int128 llend;
>
> + if (TPM_IS_CRB(section->mr->owner)) {
> + /* The CRB command buffer has its base address unaligned. */
> + return true;
> + }
> +
> if ((!memory_region_is_ram(section->mr) &&
> !memory_region_is_iommu(section->mr)) ||
> memory_region_is_protected(section->mr) ||
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] vhost-vdpa: skip TPM CRB memory section
2022-11-22 14:53 [PATCH] vhost-vdpa: skip TPM CRB memory section marcandre.lureau
2022-11-22 15:18 ` [PATCH-for-7.2] " Philippe Mathieu-Daudé
2022-11-22 16:05 ` [PATCH] " Eric Auger
@ 2022-11-22 16:49 ` Eugenio Perez Martin
2022-11-23 4:25 ` Jason Wang
2022-11-22 20:32 ` Michael S. Tsirkin
3 siblings, 1 reply; 13+ messages in thread
From: Eugenio Perez Martin @ 2022-11-22 16:49 UTC (permalink / raw)
To: marcandre.lureau; +Cc: qemu-devel, eric.auger, Michael S. Tsirkin, Jason Wang
On Tue, Nov 22, 2022 at 3:53 PM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> 851d6d1a0f ("vfio/common: remove spurious tpm-crb-cmd misalignment
> warning") removed the warning on vfio_listener_region_add() path.
>
> An error is reported for vhost-vdpa case:
> qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
>
> Skip the CRB device.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=2141965
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Eugenio Pérez <eperezma@redhat.com>
And CCing Jason too.
> ---
> hw/virtio/vhost-vdpa.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 7468e44b87..9d7206e4b8 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -19,6 +19,7 @@
> #include "hw/virtio/virtio-net.h"
> #include "hw/virtio/vhost-shadow-virtqueue.h"
> #include "hw/virtio/vhost-vdpa.h"
> +#include "sysemu/tpm.h"
> #include "exec/address-spaces.h"
> #include "migration/blocker.h"
> #include "qemu/cutils.h"
> @@ -46,6 +47,11 @@ static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
> {
> Int128 llend;
>
> + if (TPM_IS_CRB(section->mr->owner)) {
> + /* The CRB command buffer has its base address unaligned. */
> + return true;
> + }
> +
> if ((!memory_region_is_ram(section->mr) &&
> !memory_region_is_iommu(section->mr)) ||
> memory_region_is_protected(section->mr) ||
> --
> 2.38.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] vhost-vdpa: skip TPM CRB memory section
2022-11-22 16:49 ` Eugenio Perez Martin
@ 2022-11-23 4:25 ` Jason Wang
0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-11-23 4:25 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: marcandre.lureau, qemu-devel, eric.auger, Michael S. Tsirkin,
Cindy Lu
On Wed, Nov 23, 2022 at 12:50 AM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Tue, Nov 22, 2022 at 3:53 PM <marcandre.lureau@redhat.com> wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > 851d6d1a0f ("vfio/common: remove spurious tpm-crb-cmd misalignment
> > warning") removed the warning on vfio_listener_region_add() path.
> >
> > An error is reported for vhost-vdpa case:
> > qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
> >
> > Skip the CRB device.
> >
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=2141965
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>
> And CCing Jason too.
Adding Cindy.
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
>
> > ---
> > hw/virtio/vhost-vdpa.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 7468e44b87..9d7206e4b8 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -19,6 +19,7 @@
> > #include "hw/virtio/virtio-net.h"
> > #include "hw/virtio/vhost-shadow-virtqueue.h"
> > #include "hw/virtio/vhost-vdpa.h"
> > +#include "sysemu/tpm.h"
> > #include "exec/address-spaces.h"
> > #include "migration/blocker.h"
> > #include "qemu/cutils.h"
> > @@ -46,6 +47,11 @@ static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
> > {
> > Int128 llend;
> >
> > + if (TPM_IS_CRB(section->mr->owner)) {
> > + /* The CRB command buffer has its base address unaligned. */
> > + return true;
> > + }
> > +
> > if ((!memory_region_is_ram(section->mr) &&
> > !memory_region_is_iommu(section->mr)) ||
> > memory_region_is_protected(section->mr) ||
> > --
> > 2.38.1
> >
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vhost-vdpa: skip TPM CRB memory section
2022-11-22 14:53 [PATCH] vhost-vdpa: skip TPM CRB memory section marcandre.lureau
` (2 preceding siblings ...)
2022-11-22 16:49 ` Eugenio Perez Martin
@ 2022-11-22 20:32 ` Michael S. Tsirkin
2022-11-23 7:20 ` Marc-André Lureau
3 siblings, 1 reply; 13+ messages in thread
From: Michael S. Tsirkin @ 2022-11-22 20:32 UTC (permalink / raw)
To: marcandre.lureau; +Cc: qemu-devel, eperezma, eric.auger
On Tue, Nov 22, 2022 at 06:53:49PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> 851d6d1a0f ("vfio/common: remove spurious tpm-crb-cmd misalignment
> warning") removed the warning on vfio_listener_region_add() path.
>
> An error is reported for vhost-vdpa case:
> qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
>
> Skip the CRB device.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=2141965
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/virtio/vhost-vdpa.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 7468e44b87..9d7206e4b8 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -19,6 +19,7 @@
> #include "hw/virtio/virtio-net.h"
> #include "hw/virtio/vhost-shadow-virtqueue.h"
> #include "hw/virtio/vhost-vdpa.h"
> +#include "sysemu/tpm.h"
> #include "exec/address-spaces.h"
> #include "migration/blocker.h"
> #include "qemu/cutils.h"
> @@ -46,6 +47,11 @@ static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
> {
> Int128 llend;
>
> + if (TPM_IS_CRB(section->mr->owner)) {
> + /* The CRB command buffer has its base address unaligned. */
> + return true;
> + }
> +
Quite a hack. We can't really keep adding dependency on random devices
to vhost. And would you add hacks like this to listeners?
Pls figure out what's special about this buffer.
Also if this section is unaligned then doesn't it break up
other aligned sections?
> if ((!memory_region_is_ram(section->mr) &&
> !memory_region_is_iommu(section->mr)) ||
> memory_region_is_protected(section->mr) ||
> --
> 2.38.1
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] vhost-vdpa: skip TPM CRB memory section
2022-11-22 20:32 ` Michael S. Tsirkin
@ 2022-11-23 7:20 ` Marc-André Lureau
2022-11-23 7:23 ` Michael S. Tsirkin
2022-11-23 7:31 ` Eugenio Perez Martin
0 siblings, 2 replies; 13+ messages in thread
From: Marc-André Lureau @ 2022-11-23 7:20 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, eperezma, eric.auger, Peter Maydell
[-- Attachment #1: Type: text/plain, Size: 2599 bytes --]
Hi
On Wed, Nov 23, 2022 at 12:32 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Nov 22, 2022 at 06:53:49PM +0400, marcandre.lureau@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > 851d6d1a0f ("vfio/common: remove spurious tpm-crb-cmd misalignment
> > warning") removed the warning on vfio_listener_region_add() path.
> >
> > An error is reported for vhost-vdpa case:
> > qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
> >
> > Skip the CRB device.
> >
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=2141965
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > hw/virtio/vhost-vdpa.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 7468e44b87..9d7206e4b8 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -19,6 +19,7 @@
> > #include "hw/virtio/virtio-net.h"
> > #include "hw/virtio/vhost-shadow-virtqueue.h"
> > #include "hw/virtio/vhost-vdpa.h"
> > +#include "sysemu/tpm.h"
> > #include "exec/address-spaces.h"
> > #include "migration/blocker.h"
> > #include "qemu/cutils.h"
> > @@ -46,6 +47,11 @@ static bool
> vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
> > {
> > Int128 llend;
> >
> > + if (TPM_IS_CRB(section->mr->owner)) {
> > + /* The CRB command buffer has its base address unaligned. */
> > + return true;
> > + }
> > +
>
> Quite a hack. We can't really keep adding dependency on random devices
>
Agree it's not great. but it's not strictly a dependency. At least you can
still build with !CONFIG_TPM.
to vhost. And would you add hacks like this to listeners?
> Pls figure out what's special about this buffer.
> Also if this section is unaligned then doesn't it break up
> other aligned sections?
>
See the original discussion:
https://patchew.org/QEMU/20220208133842.112017-1-eric.auger@redhat.com/20220208133842.112017-2-eric.auger@redhat.com/
It is not clear whether aligning the tpm-crb-cmd region would work
(overlapping tpm-crb-mmio).
Peter Maydell said: "There's nothing that guarantees alignment for memory
regions at all, whether they're RAM, IO or anything else.".
Maybe vfio/vhost should simply skip those odd regions silently.
>
> > if ((!memory_region_is_ram(section->mr) &&
> > !memory_region_is_iommu(section->mr)) ||
> > memory_region_is_protected(section->mr) ||
> > --
> > 2.38.1
>
>
[-- Attachment #2: Type: text/html, Size: 4190 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] vhost-vdpa: skip TPM CRB memory section
2022-11-23 7:20 ` Marc-André Lureau
@ 2022-11-23 7:23 ` Michael S. Tsirkin
2022-11-23 11:25 ` Philippe Mathieu-Daudé
2022-11-23 7:31 ` Eugenio Perez Martin
1 sibling, 1 reply; 13+ messages in thread
From: Michael S. Tsirkin @ 2022-11-23 7:23 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel, eperezma, eric.auger, Peter Maydell
On Wed, Nov 23, 2022 at 11:20:41AM +0400, Marc-André Lureau wrote:
> Hi
>
> On Wed, Nov 23, 2022 at 12:32 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Nov 22, 2022 at 06:53:49PM +0400, marcandre.lureau@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > 851d6d1a0f ("vfio/common: remove spurious tpm-crb-cmd misalignment
> > warning") removed the warning on vfio_listener_region_add() path.
> >
> > An error is reported for vhost-vdpa case:
> > qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
> >
> > Skip the CRB device.
> >
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=2141965
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > hw/virtio/vhost-vdpa.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 7468e44b87..9d7206e4b8 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -19,6 +19,7 @@
> > #include "hw/virtio/virtio-net.h"
> > #include "hw/virtio/vhost-shadow-virtqueue.h"
> > #include "hw/virtio/vhost-vdpa.h"
> > +#include "sysemu/tpm.h"
> > #include "exec/address-spaces.h"
> > #include "migration/blocker.h"
> > #include "qemu/cutils.h"
> > @@ -46,6 +47,11 @@ static bool vhost_vdpa_listener_skipped_section
> (MemoryRegionSection *section,
> > {
> > Int128 llend;
> >
> > + if (TPM_IS_CRB(section->mr->owner)) {
> > + /* The CRB command buffer has its base address unaligned. */
> > + return true;
> > + }
> > +
>
> Quite a hack. We can't really keep adding dependency on random devices
>
>
> Agree it's not great. but it's not strictly a dependency. At least you can
> still build with !CONFIG_TPM.
but what does it have to do with tpm?
>
> to vhost. And would you add hacks like this to listeners?
> Pls figure out what's special about this buffer.
> Also if this section is unaligned then doesn't it break up
> other aligned sections?
>
>
> See the original discussion:
> https://patchew.org/QEMU/20220208133842.112017-1-eric.auger@redhat.com/
> 20220208133842.112017-2-eric.auger@redhat.com/
>
> It is not clear whether aligning the tpm-crb-cmd region would work (overlapping
> tpm-crb-mmio).
>
> Peter Maydell said: "There's nothing that guarantees alignment for memory
> regions at all, whether they're RAM, IO or anything else.".
>
> Maybe vfio/vhost should simply skip those odd regions silently.
How do we detect them? Are these regions not DMA-able on real hardware?
>
>
>
>
> > if ((!memory_region_is_ram(section->mr) &&
> > !memory_region_is_iommu(section->mr)) ||
> > memory_region_is_protected(section->mr) ||
> > --
> > 2.38.1
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] vhost-vdpa: skip TPM CRB memory section
2022-11-23 7:23 ` Michael S. Tsirkin
@ 2022-11-23 11:25 ` Philippe Mathieu-Daudé
2022-11-23 12:20 ` Michael S. Tsirkin
2022-11-23 13:30 ` Michael S. Tsirkin
0 siblings, 2 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-11-23 11:25 UTC (permalink / raw)
To: Michael S. Tsirkin, Marc-André Lureau
Cc: qemu-devel, eperezma, eric.auger, Peter Maydell
On 23/11/22 08:23, Michael S. Tsirkin wrote:
> On Wed, Nov 23, 2022 at 11:20:41AM +0400, Marc-André Lureau wrote:
>> Hi
>>
>> On Wed, Nov 23, 2022 at 12:32 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Tue, Nov 22, 2022 at 06:53:49PM +0400, marcandre.lureau@redhat.com
>> wrote:
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > 851d6d1a0f ("vfio/common: remove spurious tpm-crb-cmd misalignment
>> > warning") removed the warning on vfio_listener_region_add() path.
>> >
>> > An error is reported for vhost-vdpa case:
>> > qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
>> >
>> > Skip the CRB device.
>> >
>> > Fixes:
>> > https://bugzilla.redhat.com/show_bug.cgi?id=2141965
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> > hw/virtio/vhost-vdpa.c | 6 ++++++
>> > 1 file changed, 6 insertions(+)
>> >
>> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> > index 7468e44b87..9d7206e4b8 100644
>> > --- a/hw/virtio/vhost-vdpa.c
>> > +++ b/hw/virtio/vhost-vdpa.c
>> > @@ -19,6 +19,7 @@
>> > #include "hw/virtio/virtio-net.h"
>> > #include "hw/virtio/vhost-shadow-virtqueue.h"
>> > #include "hw/virtio/vhost-vdpa.h"
>> > +#include "sysemu/tpm.h"
>> > #include "exec/address-spaces.h"
>> > #include "migration/blocker.h"
>> > #include "qemu/cutils.h"
>> > @@ -46,6 +47,11 @@ static bool vhost_vdpa_listener_skipped_section
>> (MemoryRegionSection *section,
>> > {
>> > Int128 llend;
>> >
>> > + if (TPM_IS_CRB(section->mr->owner)) {
>> > + /* The CRB command buffer has its base address unaligned. */
>> > + return true;
>> > + }
>> > +
>>
>> Quite a hack. We can't really keep adding dependency on random devices
>>
>>
>> Agree it's not great. but it's not strictly a dependency. At least you can
>> still build with !CONFIG_TPM.
>
> but what does it have to do with tpm?
>
>
>>
>> to vhost. And would you add hacks like this to listeners?
>> Pls figure out what's special about this buffer.
>> Also if this section is unaligned then doesn't it break up
>> other aligned sections?
>>
>>
>> See the original discussion:
>> https://patchew.org/QEMU/20220208133842.112017-1-eric.auger@redhat.com/
>> 20220208133842.112017-2-eric.auger@redhat.com/
>>
>> It is not clear whether aligning the tpm-crb-cmd region would work (overlapping
>> tpm-crb-mmio).
>>
>> Peter Maydell said: "There's nothing that guarantees alignment for memory
>> regions at all, whether they're RAM, IO or anything else.".
>>
>> Maybe vfio/vhost should simply skip those odd regions silently.
>
>
> How do we detect them? Are these regions not DMA-able on real hardware?
Are you suggesting expanding the Memory API?
- Add 'base_address_unaligned' to MemoryRegion structure (default to
false)
- Add memory_region_set_..._unaligned()
Then have TYPE_TPM_CRB use this API?
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] vhost-vdpa: skip TPM CRB memory section
2022-11-23 11:25 ` Philippe Mathieu-Daudé
@ 2022-11-23 12:20 ` Michael S. Tsirkin
2022-11-23 13:30 ` Michael S. Tsirkin
1 sibling, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2022-11-23 12:20 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Marc-André Lureau, qemu-devel, eperezma, eric.auger,
Peter Maydell
On Wed, Nov 23, 2022 at 12:25:58PM +0100, Philippe Mathieu-Daudé wrote:
> On 23/11/22 08:23, Michael S. Tsirkin wrote:
> > On Wed, Nov 23, 2022 at 11:20:41AM +0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Wed, Nov 23, 2022 at 12:32 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Nov 22, 2022 at 06:53:49PM +0400, marcandre.lureau@redhat.com
> > > wrote:
> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > >
> > > > 851d6d1a0f ("vfio/common: remove spurious tpm-crb-cmd misalignment
> > > > warning") removed the warning on vfio_listener_region_add() path.
> > > >
> > > > An error is reported for vhost-vdpa case:
> > > > qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
> > > >
> > > > Skip the CRB device.
> > > >
> > > > Fixes:
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=2141965
> > > >
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > > hw/virtio/vhost-vdpa.c | 6 ++++++
> > > > 1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > index 7468e44b87..9d7206e4b8 100644
> > > > --- a/hw/virtio/vhost-vdpa.c
> > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > @@ -19,6 +19,7 @@
> > > > #include "hw/virtio/virtio-net.h"
> > > > #include "hw/virtio/vhost-shadow-virtqueue.h"
> > > > #include "hw/virtio/vhost-vdpa.h"
> > > > +#include "sysemu/tpm.h"
> > > > #include "exec/address-spaces.h"
> > > > #include "migration/blocker.h"
> > > > #include "qemu/cutils.h"
> > > > @@ -46,6 +47,11 @@ static bool vhost_vdpa_listener_skipped_section
> > > (MemoryRegionSection *section,
> > > > {
> > > > Int128 llend;
> > > >
> > > > + if (TPM_IS_CRB(section->mr->owner)) {
> > > > + /* The CRB command buffer has its base address unaligned. */
> > > > + return true;
> > > > + }
> > > > +
> > >
> > > Quite a hack. We can't really keep adding dependency on random devices
> > >
> > >
> > > Agree it's not great. but it's not strictly a dependency. At least you can
> > > still build with !CONFIG_TPM.
> >
> > but what does it have to do with tpm?
> >
> >
> > >
> > > to vhost. And would you add hacks like this to listeners?
> > > Pls figure out what's special about this buffer.
> > > Also if this section is unaligned then doesn't it break up
> > > other aligned sections?
> > >
> > >
> > > See the original discussion:
> > > https://patchew.org/QEMU/20220208133842.112017-1-eric.auger@redhat.com/
> > > 20220208133842.112017-2-eric.auger@redhat.com/
> > >
> > > It is not clear whether aligning the tpm-crb-cmd region would work (overlapping
> > > tpm-crb-mmio).
> > >
> > > Peter Maydell said: "There's nothing that guarantees alignment for memory
> > > regions at all, whether they're RAM, IO or anything else.".
> > >
> > > Maybe vfio/vhost should simply skip those odd regions silently.
> >
> >
> > How do we detect them? Are these regions not DMA-able on real hardware?
>
> Are you suggesting expanding the Memory API?
>
> - Add 'base_address_unaligned' to MemoryRegion structure (default to
> false)
>
> - Add memory_region_set_..._unaligned()
>
> Then have TYPE_TPM_CRB use this API?
It's more of a "no_dma" isn't it? And I think we used to have
something like this, and later this morphed to device ram?
--
MST
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] vhost-vdpa: skip TPM CRB memory section
2022-11-23 11:25 ` Philippe Mathieu-Daudé
2022-11-23 12:20 ` Michael S. Tsirkin
@ 2022-11-23 13:30 ` Michael S. Tsirkin
1 sibling, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2022-11-23 13:30 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Marc-André Lureau, qemu-devel, eperezma, eric.auger,
Peter Maydell
On Wed, Nov 23, 2022 at 12:25:58PM +0100, Philippe Mathieu-Daudé wrote:
> On 23/11/22 08:23, Michael S. Tsirkin wrote:
> > On Wed, Nov 23, 2022 at 11:20:41AM +0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Wed, Nov 23, 2022 at 12:32 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Nov 22, 2022 at 06:53:49PM +0400, marcandre.lureau@redhat.com
> > > wrote:
> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > >
> > > > 851d6d1a0f ("vfio/common: remove spurious tpm-crb-cmd misalignment
> > > > warning") removed the warning on vfio_listener_region_add() path.
> > > >
> > > > An error is reported for vhost-vdpa case:
> > > > qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
> > > >
> > > > Skip the CRB device.
> > > >
> > > > Fixes:
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=2141965
> > > >
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > > hw/virtio/vhost-vdpa.c | 6 ++++++
> > > > 1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > index 7468e44b87..9d7206e4b8 100644
> > > > --- a/hw/virtio/vhost-vdpa.c
> > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > @@ -19,6 +19,7 @@
> > > > #include "hw/virtio/virtio-net.h"
> > > > #include "hw/virtio/vhost-shadow-virtqueue.h"
> > > > #include "hw/virtio/vhost-vdpa.h"
> > > > +#include "sysemu/tpm.h"
> > > > #include "exec/address-spaces.h"
> > > > #include "migration/blocker.h"
> > > > #include "qemu/cutils.h"
> > > > @@ -46,6 +47,11 @@ static bool vhost_vdpa_listener_skipped_section
> > > (MemoryRegionSection *section,
> > > > {
> > > > Int128 llend;
> > > >
> > > > + if (TPM_IS_CRB(section->mr->owner)) {
> > > > + /* The CRB command buffer has its base address unaligned. */
> > > > + return true;
> > > > + }
> > > > +
> > >
> > > Quite a hack. We can't really keep adding dependency on random devices
> > >
> > >
> > > Agree it's not great. but it's not strictly a dependency. At least you can
> > > still build with !CONFIG_TPM.
> >
> > but what does it have to do with tpm?
> >
> >
> > >
> > > to vhost. And would you add hacks like this to listeners?
> > > Pls figure out what's special about this buffer.
> > > Also if this section is unaligned then doesn't it break up
> > > other aligned sections?
> > >
> > >
> > > See the original discussion:
> > > https://patchew.org/QEMU/20220208133842.112017-1-eric.auger@redhat.com/
> > > 20220208133842.112017-2-eric.auger@redhat.com/
> > >
> > > It is not clear whether aligning the tpm-crb-cmd region would work (overlapping
> > > tpm-crb-mmio).
> > >
> > > Peter Maydell said: "There's nothing that guarantees alignment for memory
> > > regions at all, whether they're RAM, IO or anything else.".
> > >
> > > Maybe vfio/vhost should simply skip those odd regions silently.
> >
> >
> > How do we detect them? Are these regions not DMA-able on real hardware?
>
> Are you suggesting expanding the Memory API?
>
> - Add 'base_address_unaligned' to MemoryRegion structure (default to
> false)
>
> - Add memory_region_set_..._unaligned()
>
> Then have TYPE_TPM_CRB use this API?
Fundamentally since we for some reason don't seem to want to use
existing dma ram api, then yes. But let's have it say what exactly
it means, the fact it's not aligned is not the reason we don't need
to DMA there.
--
MST
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vhost-vdpa: skip TPM CRB memory section
2022-11-23 7:20 ` Marc-André Lureau
2022-11-23 7:23 ` Michael S. Tsirkin
@ 2022-11-23 7:31 ` Eugenio Perez Martin
2022-11-23 9:37 ` Michael S. Tsirkin
1 sibling, 1 reply; 13+ messages in thread
From: Eugenio Perez Martin @ 2022-11-23 7:31 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Michael S. Tsirkin, qemu-devel, eric.auger, Peter Maydell
On Wed, Nov 23, 2022 at 8:20 AM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> On Wed, Nov 23, 2022 at 12:32 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Tue, Nov 22, 2022 at 06:53:49PM +0400, marcandre.lureau@redhat.com wrote:
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > 851d6d1a0f ("vfio/common: remove spurious tpm-crb-cmd misalignment
>> > warning") removed the warning on vfio_listener_region_add() path.
>> >
>> > An error is reported for vhost-vdpa case:
>> > qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
>> >
>> > Skip the CRB device.
>> >
>> > Fixes:
>> > https://bugzilla.redhat.com/show_bug.cgi?id=2141965
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> > hw/virtio/vhost-vdpa.c | 6 ++++++
>> > 1 file changed, 6 insertions(+)
>> >
>> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> > index 7468e44b87..9d7206e4b8 100644
>> > --- a/hw/virtio/vhost-vdpa.c
>> > +++ b/hw/virtio/vhost-vdpa.c
>> > @@ -19,6 +19,7 @@
>> > #include "hw/virtio/virtio-net.h"
>> > #include "hw/virtio/vhost-shadow-virtqueue.h"
>> > #include "hw/virtio/vhost-vdpa.h"
>> > +#include "sysemu/tpm.h"
>> > #include "exec/address-spaces.h"
>> > #include "migration/blocker.h"
>> > #include "qemu/cutils.h"
>> > @@ -46,6 +47,11 @@ static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
>> > {
>> > Int128 llend;
>> >
>> > + if (TPM_IS_CRB(section->mr->owner)) {
>> > + /* The CRB command buffer has its base address unaligned. */
>> > + return true;
>> > + }
>> > +
>>
>> Quite a hack. We can't really keep adding dependency on random devices
>
>
> Agree it's not great. but it's not strictly a dependency. At least you can still build with !CONFIG_TPM.
>
>> to vhost. And would you add hacks like this to listeners?
>> Pls figure out what's special about this buffer.
>> Also if this section is unaligned then doesn't it break up
>> other aligned sections?
>
>
> See the original discussion:
> https://patchew.org/QEMU/20220208133842.112017-1-eric.auger@redhat.com/20220208133842.112017-2-eric.auger@redhat.com/
>
Based on that, I'd tune the comment to something like:
QEMU represents the CRB cmd/response buffer as a standard RAM region
to the listeners, but real HW would not be able to access them.
Besides, these regions may not be properly page aligned.
Is that more accurate?
Thanks!
> It is not clear whether aligning the tpm-crb-cmd region would work (overlapping tpm-crb-mmio).
>
> Peter Maydell said: "There's nothing that guarantees alignment for memory regions at all, whether they're RAM, IO or anything else.".
>
> Maybe vfio/vhost should simply skip those odd regions silently.
>
>
>>
>>
>> > if ((!memory_region_is_ram(section->mr) &&
>> > !memory_region_is_iommu(section->mr)) ||
>> > memory_region_is_protected(section->mr) ||
>> > --
>> > 2.38.1
>>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] vhost-vdpa: skip TPM CRB memory section
2022-11-23 7:31 ` Eugenio Perez Martin
@ 2022-11-23 9:37 ` Michael S. Tsirkin
0 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2022-11-23 9:37 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: Marc-André Lureau, qemu-devel, eric.auger, Peter Maydell
On Wed, Nov 23, 2022 at 08:31:52AM +0100, Eugenio Perez Martin wrote:
> On Wed, Nov 23, 2022 at 8:20 AM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Hi
> >
> > On Wed, Nov 23, 2022 at 12:32 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>
> >> On Tue, Nov 22, 2022 at 06:53:49PM +0400, marcandre.lureau@redhat.com wrote:
> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >
> >> > 851d6d1a0f ("vfio/common: remove spurious tpm-crb-cmd misalignment
> >> > warning") removed the warning on vfio_listener_region_add() path.
> >> >
> >> > An error is reported for vhost-vdpa case:
> >> > qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
> >> >
> >> > Skip the CRB device.
> >> >
> >> > Fixes:
> >> > https://bugzilla.redhat.com/show_bug.cgi?id=2141965
> >> >
> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> > ---
> >> > hw/virtio/vhost-vdpa.c | 6 ++++++
> >> > 1 file changed, 6 insertions(+)
> >> >
> >> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >> > index 7468e44b87..9d7206e4b8 100644
> >> > --- a/hw/virtio/vhost-vdpa.c
> >> > +++ b/hw/virtio/vhost-vdpa.c
> >> > @@ -19,6 +19,7 @@
> >> > #include "hw/virtio/virtio-net.h"
> >> > #include "hw/virtio/vhost-shadow-virtqueue.h"
> >> > #include "hw/virtio/vhost-vdpa.h"
> >> > +#include "sysemu/tpm.h"
> >> > #include "exec/address-spaces.h"
> >> > #include "migration/blocker.h"
> >> > #include "qemu/cutils.h"
> >> > @@ -46,6 +47,11 @@ static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
> >> > {
> >> > Int128 llend;
> >> >
> >> > + if (TPM_IS_CRB(section->mr->owner)) {
> >> > + /* The CRB command buffer has its base address unaligned. */
> >> > + return true;
> >> > + }
> >> > +
> >>
> >> Quite a hack. We can't really keep adding dependency on random devices
> >
> >
> > Agree it's not great. but it's not strictly a dependency. At least you can still build with !CONFIG_TPM.
> >
> >> to vhost. And would you add hacks like this to listeners?
> >> Pls figure out what's special about this buffer.
> >> Also if this section is unaligned then doesn't it break up
> >> other aligned sections?
> >
> >
> > See the original discussion:
> > https://patchew.org/QEMU/20220208133842.112017-1-eric.auger@redhat.com/20220208133842.112017-2-eric.auger@redhat.com/
> >
>
> Based on that, I'd tune the comment to something like:
> QEMU represents the CRB cmd/response buffer as a standard RAM region
> to the listeners, but real HW would not be able to access them.
But e.g. virtio can access it now with no issues, no?
Is that a problem? Given it's TPM ...
> Besides, these regions may not be properly page aligned.
>
> Is that more accurate?
>
> Thanks!
>
> > It is not clear whether aligning the tpm-crb-cmd region would work (overlapping tpm-crb-mmio).
> >
> > Peter Maydell said: "There's nothing that guarantees alignment for memory regions at all, whether they're RAM, IO or anything else.".
> >
> > Maybe vfio/vhost should simply skip those odd regions silently.
> >
> >
> >>
> >>
> >> > if ((!memory_region_is_ram(section->mr) &&
> >> > !memory_region_is_iommu(section->mr)) ||
> >> > memory_region_is_protected(section->mr) ||
> >> > --
> >> > 2.38.1
> >>
^ permalink raw reply [flat|nested] 13+ messages in thread