| Message ID | 1569507036-15314-1-git-send-email-mjrosato@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390: PCI: fix IOMMU region init | expand |
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);
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
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 >
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.
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); >
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.
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);
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(-)