1. Patchew
  2. QEMU
  3. View series

[PATCH] s390: PCI: fix IOMMU region init

Matthew Rosato posted 1 patch 4 years, 7 months ago
Test docker-mingw@fedora passed
Test checkpatch passed
Test docker-quick@centos7 passed
Test asan passed
Test docker-clang@ubuntu failed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1569507036-15314-1-git-send-email-mjrosato@linux.ibm.com
Maintainers: Richard Henderson <rth@twiddle.net>, Christian Borntraeger <borntraeger@de.ibm.com>, "Collin L. Walling" <walling@linux.ibm.com>, Cornelia Huck <cohuck@redhat.com>, David Hildenbrand <david@redhat.com>, Halil Pasic <pasic@linux.ibm.com>
hw/s390x/s390-pci-bus.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
[PATCH] s390: PCI: fix IOMMU region init
Posted by Matthew Rosato 4 years, 7 months ago 
The fix in dbe9cf606c shrinks the IOMMU memory region to a size
that seems reasonable on the surface, however is actually too
small as it is based against a 0-mapped address space.  This
causes breakage with small guests as they can overrun the IOMMU window.

Let's go back to the prior method of initializing iommu for now.

Fixes: dbe9cf606c ("s390x/pci: Set the iommu region size mpcifc request")
Reported-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reported-by: Stefan Zimmerman <stzi@linux.ibm.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 hw/s390x/s390-pci-bus.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 963a41c..2d2f4a7 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -695,10 +695,15 @@ static const MemoryRegionOps s390_msi_ctrl_ops = {
 
 void s390_pci_iommu_enable(S390PCIIOMMU *iommu)
 {
+    /*
+     * The iommu region is initialized against a 0-mapped address space,
+     * so the smallest IOMMU region we can define runs from 0 to the end
+     * of the PCI address space.
+     */
     char *name = g_strdup_printf("iommu-s390-%04x", iommu->pbdev->uid);
     memory_region_init_iommu(&iommu->iommu_mr, sizeof(iommu->iommu_mr),
                              TYPE_S390_IOMMU_MEMORY_REGION, OBJECT(&iommu->mr),
-                             name, iommu->pal - iommu->pba + 1);
+                             name, iommu->pal + 1);
     iommu->enabled = true;
     memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr));
     g_free(name);
-- 
1.8.3.1
Re: [PATCH] s390: PCI: fix IOMMU region init
Posted by Pierre Morel 4 years, 7 months ago 
Yes, it is the right thing to do.

We will see if we one of these day can fix the address space size and 
get rid of the access to the lower memory.

The iommu region translation callback protect us from setting a 
translation outside of pba-pal, so that we should be safe.

reviewed-by: Pierre Morel <pmorel@linux.ibm.com>


On 9/26/19 4:10 PM, Matthew Rosato wrote:
> The fix in dbe9cf606c shrinks the IOMMU memory region to a size
> that seems reasonable on the surface, however is actually too
> small as it is based against a 0-mapped address space.  This
> causes breakage with small guests as they can overrun the IOMMU window.
>
> Let's go back to the prior method of initializing iommu for now.
>
> Fixes: dbe9cf606c ("s390x/pci: Set the iommu region size mpcifc request")
> Reported-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> Reported-by: Stefan Zimmerman <stzi@linux.ibm.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   hw/s390x/s390-pci-bus.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 963a41c..2d2f4a7 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -695,10 +695,15 @@ static const MemoryRegionOps s390_msi_ctrl_ops = {
>   
>   void s390_pci_iommu_enable(S390PCIIOMMU *iommu)
>   {
> +    /*
> +     * The iommu region is initialized against a 0-mapped address space,
> +     * so the smallest IOMMU region we can define runs from 0 to the end
> +     * of the PCI address space.
> +     */
>       char *name = g_strdup_printf("iommu-s390-%04x", iommu->pbdev->uid);
>       memory_region_init_iommu(&iommu->iommu_mr, sizeof(iommu->iommu_mr),
>                                TYPE_S390_IOMMU_MEMORY_REGION, OBJECT(&iommu->mr),
> -                             name, iommu->pal - iommu->pba + 1);
> +                             name, iommu->pal + 1);
>       iommu->enabled = true;
>       memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr));
>       g_free(name);

-- 
Pierre Morel
IBM Lab Boeblingen
Re: [PATCH] s390: PCI: fix IOMMU region init
Posted by Peter Maydell 4 years, 7 months ago 
On Thu, 26 Sep 2019 at 15:12, Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>
> The fix in dbe9cf606c shrinks the IOMMU memory region to a size
> that seems reasonable on the surface, however is actually too
> small as it is based against a 0-mapped address space.  This
> causes breakage with small guests as they can overrun the IOMMU window.
>
> Let's go back to the prior method of initializing iommu for now.
>
> Fixes: dbe9cf606c ("s390x/pci: Set the iommu region size mpcifc request")
> Reported-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> Reported-by: Stefan Zimmerman <stzi@linux.ibm.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>

So in commit f0a399dbae6a2d0e2 (Nov 2015) we used "pal - pba + 1".
In commit f7c40aa1e7feb50bc4 (June 2016) we switched to "pal + 1".
In commit dbe9cf606c (Jan 2019) we went back to "pal - pba + 1"
Now we're on "pal + 1" again...

Are we really sure that this is correct and that we're not
just going to keep looping around between these two formations
forever? :-)

thanks
-- PMM
Re: [PATCH] s390: PCI: fix IOMMU region init
Posted by Christian Borntraeger 4 years, 7 months ago 

On 26.09.19 16:34, Peter Maydell wrote:
> On Thu, 26 Sep 2019 at 15:12, Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>>
>> The fix in dbe9cf606c shrinks the IOMMU memory region to a size
>> that seems reasonable on the surface, however is actually too
>> small as it is based against a 0-mapped address space.  This
>> causes breakage with small guests as they can overrun the IOMMU window.
>>
>> Let's go back to the prior method of initializing iommu for now.
>>
>> Fixes: dbe9cf606c ("s390x/pci: Set the iommu region size mpcifc request")
>> Reported-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> Reported-by: Stefan Zimmerman <stzi@linux.ibm.com>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> So in commit f0a399dbae6a2d0e2 (Nov 2015) we used "pal - pba + 1".
> In commit f7c40aa1e7feb50bc4 (June 2016) we switched to "pal + 1".
> In commit dbe9cf606c (Jan 2019) we went back to "pal - pba + 1"
> Now we're on "pal + 1" again...
> 
> Are we really sure that this is correct and that we're not
> just going to keep looping around between these two formations
> forever? :-)

As Matt and Pierre outlined this is indeed the variant that works
reliably. I will add 
Cc: qemu-stable@nongnu.org

and apply.
Re: [PATCH] s390: PCI: fix IOMMU region init
Posted by Matthew Rosato 4 years, 7 months ago 
On 9/26/19 10:34 AM, Peter Maydell wrote:
> On Thu, 26 Sep 2019 at 15:12, Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>>
>> The fix in dbe9cf606c shrinks the IOMMU memory region to a size
>> that seems reasonable on the surface, however is actually too
>> small as it is based against a 0-mapped address space.  This
>> causes breakage with small guests as they can overrun the IOMMU window.
>>
>> Let's go back to the prior method of initializing iommu for now.
>>
>> Fixes: dbe9cf606c ("s390x/pci: Set the iommu region size mpcifc request")
>> Reported-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> Reported-by: Stefan Zimmerman <stzi@linux.ibm.com>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> So in commit f0a399dbae6a2d0e2 (Nov 2015) we used "pal - pba + 1".
> In commit f7c40aa1e7feb50bc4 (June 2016) we switched to "pal + 1".
> In commit dbe9cf606c (Jan 2019) we went back to "pal - pba + 1"
> Now we're on "pal + 1" again...
> 
> Are we really sure that this is correct and that we're not
> just going to keep looping around between these two formations
> forever? :-)
> 

Yes :) -- Pierre's RB comment sums it up pretty well, until we change
the way the address space is mapped it is not safe to use pal - pba + 1.
 This was noted in f7c40aa1e and then erroneously missed in dbe9cf606c.
 With this, small guests break immediately (PCI base is higher than the
IOMMU region can handle).  Larger guests don't break immediately but can
break later if their PCI space usage pushes high enough (their IOMMU
region can handle pba, but somewhere < pal).

The comment block added was to help assist in keeping further hands off
of this call until such a time where the address space mapping is changed.

> thanks
> -- PMM
>
Re: [PATCH] s390: PCI: fix IOMMU region init
Posted by Christian Borntraeger 4 years, 7 months ago 
On 26.09.19 16:10, Matthew Rosato wrote:
> The fix in dbe9cf606c shrinks the IOMMU memory region to a size
> that seems reasonable on the surface, however is actually too
> small as it is based against a 0-mapped address space.  This
> causes breakage with small guests as they can overrun the IOMMU window.
> 
> Let's go back to the prior method of initializing iommu for now.
> 
> Fixes: dbe9cf606c ("s390x/pci: Set the iommu region size mpcifc request")
> Reported-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> Reported-by: Stefan Zimmerman <stzi@linux.ibm.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>

Matt can you also send a patch adding you as the PCI maintainer now
that you have taken over from Collin?



> ---
>  hw/s390x/s390-pci-bus.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 963a41c..2d2f4a7 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -695,10 +695,15 @@ static const MemoryRegionOps s390_msi_ctrl_ops = {
>  
>  void s390_pci_iommu_enable(S390PCIIOMMU *iommu)
>  {
> +    /*
> +     * The iommu region is initialized against a 0-mapped address space,
> +     * so the smallest IOMMU region we can define runs from 0 to the end
> +     * of the PCI address space.
> +     */
>      char *name = g_strdup_printf("iommu-s390-%04x", iommu->pbdev->uid);
>      memory_region_init_iommu(&iommu->iommu_mr, sizeof(iommu->iommu_mr),
>                               TYPE_S390_IOMMU_MEMORY_REGION, OBJECT(&iommu->mr),
> -                             name, iommu->pal - iommu->pba + 1);
> +                             name, iommu->pal + 1);
>      iommu->enabled = true;
>      memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr));
>      g_free(name);
>
Re: [PATCH] s390: PCI: fix IOMMU region init
Posted by Christian Borntraeger 4 years, 7 months ago 

On 26.09.19 16:10, Matthew Rosato wrote:
> The fix in dbe9cf606c shrinks the IOMMU memory region to a size
> that seems reasonable on the surface, however is actually too
> small as it is based against a 0-mapped address space.  This
> causes breakage with small guests as they can overrun the IOMMU window.
> 
> Let's go back to the prior method of initializing iommu for now.
> 
> Fixes: dbe9cf606c ("s390x/pci: Set the iommu region size mpcifc request")
> Reported-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> Reported-by: Stefan Zimmerman <stzi@linux.ibm.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>  hw/s390x/s390-pci-bus.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 963a41c..2d2f4a7 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -695,10 +695,15 @@ static const MemoryRegionOps s390_msi_ctrl_ops = {
>  
>  void s390_pci_iommu_enable(S390PCIIOMMU *iommu)
>  {
> +    /*
> +     * The iommu region is initialized against a 0-mapped address space,
> +     * so the smallest IOMMU region we can define runs from 0 to the end
> +     * of the PCI address space.
> +     */
>      char *name = g_strdup_printf("iommu-s390-%04x", iommu->pbdev->uid);
>      memory_region_init_iommu(&iommu->iommu_mr, sizeof(iommu->iommu_mr),
>                               TYPE_S390_IOMMU_MEMORY_REGION, OBJECT(&iommu->mr),
> -                             name, iommu->pal - iommu->pba + 1);
> +                             name, iommu->pal + 1);
>      iommu->enabled = true;
>      memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr));
>      g_free(name);
> 
#

Thanks applied.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.