[PATCH] s390: PCI: fix IOMMU region init

[PATCH] s390: PCI: fix IOMMU region init

[Matthew Rosato]

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

[Pierre Morel]

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

[Peter Maydell]

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

[Matthew Rosato]

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

[Christian Borntraeger]

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

[Christian Borntraeger]

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

[Christian Borntraeger]

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.