| Message ID | 1547125207-16907-2-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] s390x/pci: Set the iommu region size mpcifc request | expand |
On Thu, 10 Jan 2019 14:00:07 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > The size of the accessible iommu memory region in the guest > is given to the IOMMU by the guest through the mpcifc request > specifying the PCI Base Address and the PCI Address Limit. > > Let set the size of the IOMMU region to: s/Let/Let's/ > (PCI Address Limit) - (PCI Base Address) + 1. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > hw/s390x/s390-pci-bus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > index 69e0671..e97696a 100644 > --- a/hw/s390x/s390-pci-bus.c > +++ b/hw/s390x/s390-pci-bus.c > @@ -660,7 +660,7 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu) > 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 + 1); > + name, iommu->pal - iommu->pba + 1); > iommu->enabled = true; > memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr)); > g_free(name); Looks good to me. Collin, can I get an ack from you so I can queue it?
On 1/10/19 8:00 AM, Pierre Morel wrote: > The size of the accessible iommu memory region in the guest > is given to the IOMMU by the guest through the mpcifc request > specifying the PCI Base Address and the PCI Address Limit. > > Let set the size of the IOMMU region to: > (PCI Address Limit) - (PCI Base Address) + 1. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > hw/s390x/s390-pci-bus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > index 69e0671..e97696a 100644 > --- a/hw/s390x/s390-pci-bus.c > +++ b/hw/s390x/s390-pci-bus.c > @@ -660,7 +660,7 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu) > 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 + 1); > + name, iommu->pal - iommu->pba + 1); > iommu->enabled = true; > memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr)); > g_free(name); > Acked-by: Collin Walling <walling@linux.ibm.com>
On Thu, 10 Jan 2019 14:00:07 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > The size of the accessible iommu memory region in the guest > is given to the IOMMU by the guest through the mpcifc request > specifying the PCI Base Address and the PCI Address Limit. > > Let set the size of the IOMMU region to: > (PCI Address Limit) - (PCI Base Address) + 1. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > hw/s390x/s390-pci-bus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied.
On 15/01/2019 16:47, Cornelia Huck wrote: > On Thu, 10 Jan 2019 14:00:07 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> The size of the accessible iommu memory region in the guest >> is given to the IOMMU by the guest through the mpcifc request >> specifying the PCI Base Address and the PCI Address Limit. >> >> Let set the size of the IOMMU region to: >> (PCI Address Limit) - (PCI Base Address) + 1. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> hw/s390x/s390-pci-bus.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Thanks, applied. > Thanks. :)
On Tue, 15 Jan 2019 10:35:42 -0500 Collin Walling <walling@linux.ibm.com> wrote: > On 1/10/19 8:00 AM, Pierre Morel wrote: > > The size of the accessible iommu memory region in the guest > > is given to the IOMMU by the guest through the mpcifc request > > specifying the PCI Base Address and the PCI Address Limit. > > > > Let set the size of the IOMMU region to: > > (PCI Address Limit) - (PCI Base Address) + 1. > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > > --- > > hw/s390x/s390-pci-bus.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > > index 69e0671..e97696a 100644 > > --- a/hw/s390x/s390-pci-bus.c > > +++ b/hw/s390x/s390-pci-bus.c > > @@ -660,7 +660,7 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu) > > 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 + 1); > > + name, iommu->pal - iommu->pba + 1); From the the look of this, I would say we basically used the address denoting the end of the region as the size of the region. This smells like a bug to me, but the commit message and the title ain't clear about this, and there is no fixes tag. Because of the latter I did some digging and came to commit f7c40aa "s390x/pci: fix failures of dma map/unmap" (Yi Min Zhao, 2016-06-19) which basically did the inverse of this commit! My initial motivation was to check if this is stable material. But now I'm very confused. I'm admittedly zPCI incompetent. Could some of the people that understand what is going on help me feel better about this patch? Regards, Halil > > iommu->enabled = true; > > memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr)); > > g_free(name); > > > > Acked-by: Collin Walling <walling@linux.ibm.com> > >
On 16/01/2019 13:40, Halil Pasic wrote: > On Tue, 15 Jan 2019 10:35:42 -0500 > Collin Walling <walling@linux.ibm.com> wrote: > >> On 1/10/19 8:00 AM, Pierre Morel wrote: >>> The size of the accessible iommu memory region in the guest >>> is given to the IOMMU by the guest through the mpcifc request >>> specifying the PCI Base Address and the PCI Address Limit. >>> >>> Let set the size of the IOMMU region to: >>> (PCI Address Limit) - (PCI Base Address) + 1. >>> >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>> --- >>> hw/s390x/s390-pci-bus.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >>> index 69e0671..e97696a 100644 >>> --- a/hw/s390x/s390-pci-bus.c >>> +++ b/hw/s390x/s390-pci-bus.c >>> @@ -660,7 +660,7 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu) >>> 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 + 1); >>> + name, iommu->pal - iommu->pba + 1); > > From the the look of this, I would say we basically used the address > denoting the end of the region as the size of the region. This smells > like a bug to me, but the commit message and the title ain't clear about > this, and there is no fixes tag. Because of the latter I did some digging > and came to commit f7c40aa "s390x/pci: fix failures of dma > map/unmap" (Yi Min Zhao, 2016-06-19) which basically did the inverse of > this commit! > > My initial motivation was to check if this is stable material. But now > I'm very confused. I'm admittedly zPCI incompetent. Could some of the > people that understand what is going on help me feel better about this > patch? > > Regards, > Halil The patch you speak about corrected the problem described in its comment by setting the offset address of the subregion to 0, making sure VFIO_PCI works for Z but introduced a bug we did not see at that time by making the subregion too large. This patch correct the bug, I can add a reference to this with: fixing: commit f7c40aa1e7feb50bc4d4bc171fa811bdd9a93e51 Regards, Pierre > > > >>> iommu->enabled = true; >>> memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr)); >>> g_free(name); >>> >> >> Acked-by: Collin Walling <walling@linux.ibm.com> >> >> >
On Wed, 16 Jan 2019 15:16:44 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 16/01/2019 13:40, Halil Pasic wrote: > > On Tue, 15 Jan 2019 10:35:42 -0500 > > Collin Walling <walling@linux.ibm.com> wrote: > > > >> On 1/10/19 8:00 AM, Pierre Morel wrote: > >>> The size of the accessible iommu memory region in the guest > >>> is given to the IOMMU by the guest through the mpcifc request > >>> specifying the PCI Base Address and the PCI Address Limit. > >>> > >>> Let set the size of the IOMMU region to: > >>> (PCI Address Limit) - (PCI Base Address) + 1. > >>> > >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > >>> --- > >>> hw/s390x/s390-pci-bus.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > >>> index 69e0671..e97696a 100644 > >>> --- a/hw/s390x/s390-pci-bus.c > >>> +++ b/hw/s390x/s390-pci-bus.c > >>> @@ -660,7 +660,7 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu) > >>> 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 + 1); > >>> + name, iommu->pal - iommu->pba + 1); > > > > From the the look of this, I would say we basically used the address > > denoting the end of the region as the size of the region. This smells > > like a bug to me, but the commit message and the title ain't clear about > > this, and there is no fixes tag. Because of the latter I did some digging > > and came to commit f7c40aa "s390x/pci: fix failures of dma > > map/unmap" (Yi Min Zhao, 2016-06-19) which basically did the inverse of > > this commit! > > > > My initial motivation was to check if this is stable material. But now > > I'm very confused. I'm admittedly zPCI incompetent. Could some of the > > people that understand what is going on help me feel better about this > > patch? > > > > Regards, > > Halil > > > The patch you speak about corrected the problem described in its comment > by setting the offset address of the subregion to 0, making sure > VFIO_PCI works for Z but introduced a bug we did not see at that time by > making the subregion too large. > > This patch correct the bug, I can add a reference to this with: > fixing: commit f7c40aa1e7feb50bc4d4bc171fa811bdd9a93e51 The patch is already queued, but I can add Fixes: f7c40aa1e7 ("s390x/pci: fix failures of dma map/unmap") > > Regards, > Pierre > > > > > > > > >>> iommu->enabled = true; > >>> memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr)); > >>> g_free(name); > >>> > >> > >> Acked-by: Collin Walling <walling@linux.ibm.com> > >> > >> > > > >
On Wed, 16 Jan 2019 15:16:44 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 16/01/2019 13:40, Halil Pasic wrote: > > On Tue, 15 Jan 2019 10:35:42 -0500 > > Collin Walling <walling@linux.ibm.com> wrote: > > > >> On 1/10/19 8:00 AM, Pierre Morel wrote: > >>> The size of the accessible iommu memory region in the guest > >>> is given to the IOMMU by the guest through the mpcifc request > >>> specifying the PCI Base Address and the PCI Address Limit. > >>> > >>> Let set the size of the IOMMU region to: > >>> (PCI Address Limit) - (PCI Base Address) + 1. > >>> > >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > >>> --- > >>> hw/s390x/s390-pci-bus.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > >>> index 69e0671..e97696a 100644 > >>> --- a/hw/s390x/s390-pci-bus.c > >>> +++ b/hw/s390x/s390-pci-bus.c > >>> @@ -660,7 +660,7 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu) > >>> 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 + 1); > >>> + name, iommu->pal - iommu->pba + 1); > > > > From the the look of this, I would say we basically used the address > > denoting the end of the region as the size of the region. This smells > > like a bug to me, but the commit message and the title ain't clear about > > this, and there is no fixes tag. Because of the latter I did some digging > > and came to commit f7c40aa "s390x/pci: fix failures of dma > > map/unmap" (Yi Min Zhao, 2016-06-19) which basically did the inverse of > > this commit! > > > > My initial motivation was to check if this is stable material. But now > > I'm very confused. I'm admittedly zPCI incompetent. Could some of the > > people that understand what is going on help me feel better about this > > patch? > > > > Regards, > > Halil > > > The patch you speak about corrected the problem described in its comment > by setting the offset address of the subregion to 0, making sure > VFIO_PCI works for Z but introduced a bug we did not see at that time by > making the subregion too large. > > This patch correct the bug, I can add a reference to this with: > fixing: commit f7c40aa1e7feb50bc4d4bc171fa811bdd9a93e51 > @Connie, will you add the Fixes tag? Do we need a cc stable (since broken since 2016-06-19)? @Pierre: So you say it's a bug. What can go wrong because of this? For example if we interpret pal as a size, I guess we could end up with the memory region not fitting the guest memory, or? I'm still pretty much in the dark about the implications of this bug. Regards, Halil
On 16/01/2019 15:50, Halil Pasic wrote: > On Wed, 16 Jan 2019 15:16:44 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 16/01/2019 13:40, Halil Pasic wrote: >>> On Tue, 15 Jan 2019 10:35:42 -0500 >>> Collin Walling <walling@linux.ibm.com> wrote: >>> >>>> On 1/10/19 8:00 AM, Pierre Morel wrote: >>>>> The size of the accessible iommu memory region in the guest >>>>> is given to the IOMMU by the guest through the mpcifc request >>>>> specifying the PCI Base Address and the PCI Address Limit. >>>>> >>>>> Let set the size of the IOMMU region to: >>>>> (PCI Address Limit) - (PCI Base Address) + 1. >>>>> >>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>>> --- >>>>> hw/s390x/s390-pci-bus.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >>>>> index 69e0671..e97696a 100644 >>>>> --- a/hw/s390x/s390-pci-bus.c >>>>> +++ b/hw/s390x/s390-pci-bus.c >>>>> @@ -660,7 +660,7 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu) >>>>> 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 + 1); >>>>> + name, iommu->pal - iommu->pba + 1); >>> >>> From the the look of this, I would say we basically used the address >>> denoting the end of the region as the size of the region. This smells >>> like a bug to me, but the commit message and the title ain't clear about >>> this, and there is no fixes tag. Because of the latter I did some digging >>> and came to commit f7c40aa "s390x/pci: fix failures of dma >>> map/unmap" (Yi Min Zhao, 2016-06-19) which basically did the inverse of >>> this commit! >>> >>> My initial motivation was to check if this is stable material. But now >>> I'm very confused. I'm admittedly zPCI incompetent. Could some of the >>> people that understand what is going on help me feel better about this >>> patch? >>> >>> Regards, >>> Halil >> >> >> The patch you speak about corrected the problem described in its comment >> by setting the offset address of the subregion to 0, making sure >> VFIO_PCI works for Z but introduced a bug we did not see at that time by >> making the subregion too large. >> >> This patch correct the bug, I can add a reference to this with: >> fixing: commit f7c40aa1e7feb50bc4d4bc171fa811bdd9a93e51 >> > > @Connie, will you add the Fixes tag? Do we need a cc stable (since > broken since 2016-06-19)? > > @Pierre: So you say it's a bug. > What can go wrong because of this? > For example if we interpret pal as a size, I guess we could end up with > the memory region not fitting the guest memory, The memory region will be too large compared with what the guest required. > or? I'm still pretty > much in the dark about the implications of this bug. > > Regards, > Halil >
On Wed, 16 Jan 2019 16:44:09 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 16/01/2019 15:50, Halil Pasic wrote: > > @Connie, will you add the Fixes tag? Do we need a cc stable (since > > broken since 2016-06-19)? > > > > @Pierre: So you say it's a bug. > > What can go wrong because of this? > > For example if we interpret pal as a size, I guess we could end up with > > the memory region not fitting the guest memory, > > The memory region will be too large compared with what the guest required. Honestly, this does not look like QEMU stable material to me.
On Wed, 16 Jan 2019 17:41:30 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Wed, 16 Jan 2019 16:44:09 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > > > On 16/01/2019 15:50, Halil Pasic wrote: > > > > @Connie, will you add the Fixes tag? Do we need a cc stable (since > > > broken since 2016-06-19)? > > > > > > @Pierre: So you say it's a bug. > > > What can go wrong because of this? > > > For example if we interpret pal as a size, I guess we could end up with > > > the memory region not fitting the guest memory, > > > > The memory region will be too large compared with what the guest required. > > Honestly, this does not look like QEMU stable material to me. > Based on Pierre's offline explanation, from which I understood, the worst thing that can happen is, that a buggy guest can render it's pci function unusable, I have to agree: not stable material. My problem is my non-existent understanding of zPCI. My intuition was this should be much nastier than that. I would have appreciated a better explanation on why this is not a problem in practice though -- best as a part of the commit message. Regards, Halil
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c index 69e0671..e97696a 100644 --- a/hw/s390x/s390-pci-bus.c +++ b/hw/s390x/s390-pci-bus.c @@ -660,7 +660,7 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu) 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 + 1); + name, iommu->pal - iommu->pba + 1); iommu->enabled = true; memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr)); g_free(name);
The size of the accessible iommu memory region in the guest is given to the IOMMU by the guest through the mpcifc request specifying the PCI Base Address and the PCI Address Limit. Let set the size of the IOMMU region to: (PCI Address Limit) - (PCI Base Address) + 1. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- hw/s390x/s390-pci-bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)