Re: [PATCH] vhost-vdpa: skip TPM CRB memory section

[Michael S. Tsirkin]

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
> >>