diff mbox series

[v2] s390x/pci: Set the iommu region size mpcifc request

Message ID 1547125207-16907-2-git-send-email-pmorel@linux.ibm.com
State New
Headers show
Series [v2] s390x/pci: Set the iommu region size mpcifc request | expand

Commit Message

Pierre Morel Jan. 10, 2019, 1 p.m. UTC
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(-)

Comments

Cornelia Huck Jan. 15, 2019, 1:59 p.m. UTC | #1
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?
Collin Walling Jan. 15, 2019, 3:35 p.m. UTC | #2
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>
Cornelia Huck Jan. 15, 2019, 3:47 p.m. UTC | #3
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.
Pierre Morel Jan. 15, 2019, 5:35 p.m. UTC | #4
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. :)
Halil Pasic Jan. 16, 2019, 12:40 p.m. UTC | #5
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>
> 
>
Pierre Morel Jan. 16, 2019, 2:16 p.m. UTC | #6
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>
>>
>>
>
Cornelia Huck Jan. 16, 2019, 2:34 p.m. UTC | #7
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>
> >>
> >>  
> >   
> 
>
Halil Pasic Jan. 16, 2019, 2:50 p.m. UTC | #8
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
Pierre Morel Jan. 16, 2019, 3:44 p.m. UTC | #9
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
>
Cornelia Huck Jan. 16, 2019, 4:41 p.m. UTC | #10
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.
Halil Pasic Jan. 17, 2019, 3:13 p.m. UTC | #11
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 mbox series

Patch

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);