diff mbox series

[2/2] tpm_crb: mark memory as protected

Message ID 20230620195054.23929-3-lvivier@redhat.com
State New
Headers show
Series vhost-vdpa: skip TPM CRB memory section | expand

Commit Message

Laurent Vivier June 20, 2023, 7:50 p.m. UTC 
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(-)

Comments

David Hildenbrand June 21, 2023, 9:13 a.m. UTC | #1 
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?
Stefan Berger June 21, 2023, 12:29 p.m. UTC | #2 
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(),
Laurent Vivier June 22, 2023, 12:59 p.m. UTC | #3 
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
David Hildenbrand June 22, 2023, 1:05 p.m. UTC | #4 
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
David Hildenbrand June 22, 2023, 1:05 p.m. UTC | #5 
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>
Peter Maydell June 22, 2023, 1:12 p.m. UTC | #6 
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
Laurent Vivier June 22, 2023, 1:39 p.m. UTC | #7 
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
Peter Maydell June 22, 2023, 1:53 p.m. UTC | #8 
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
Jason Wang July 4, 2023, 3:07 a.m. UTC | #9 
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
>
Laurent Vivier July 4, 2023, 6:45 a.m. UTC | #10 
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 mbox series

Patch

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(),