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

[Eugenio Perez Martin]

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