All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size as guest wants
@ 2019-01-10 13:00 Pierre Morel
  2019-01-10 13:00 ` [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request Pierre Morel
  0 siblings, 1 reply; 13+ messages in thread
From: Pierre Morel @ 2019-01-10 13:00 UTC (permalink / raw)
  To: walling
  Cc: borntraeger, cohuck, rth, david, qemu-s390x, qemu-devel, pasic, thuth

Changed the subject (kept only in cover letter)

Changed the commit message to specify that the PAL/PBA values are
given by the guest through the mpcifc call.

Pierre Morel (1):
  s390x/pci: Set the iommu region size mpcifc request

 hw/s390x/s390-pci-bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request
  2019-01-10 13:00 [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size as guest wants Pierre Morel
@ 2019-01-10 13:00 ` Pierre Morel
  2019-01-15 13:59   ` Cornelia Huck
                     ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Pierre Morel @ 2019-01-10 13:00 UTC (permalink / raw)
  To: walling
  Cc: borntraeger, cohuck, rth, david, qemu-s390x, qemu-devel, pasic, thuth

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request
  2019-01-10 13:00 ` [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request Pierre Morel
@ 2019-01-15 13:59   ` Cornelia Huck
  2019-01-15 15:35   ` Collin Walling
  2019-01-15 15:47   ` Cornelia Huck
  2 siblings, 0 replies; 13+ messages in thread
From: Cornelia Huck @ 2019-01-15 13:59 UTC (permalink / raw)
  To: Pierre Morel, walling
  Cc: borntraeger, rth, david, qemu-s390x, qemu-devel, pasic, thuth

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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request
  2019-01-10 13:00 ` [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request Pierre Morel
  2019-01-15 13:59   ` Cornelia Huck
@ 2019-01-15 15:35   ` Collin Walling
  2019-01-16 12:40     ` Halil Pasic
  2019-01-15 15:47   ` Cornelia Huck
  2 siblings, 1 reply; 13+ messages in thread
From: Collin Walling @ 2019-01-15 15:35 UTC (permalink / raw)
  To: Pierre Morel
  Cc: thuth, david, cohuck, qemu-devel, pasic, borntraeger, qemu-s390x, rth

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>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request
  2019-01-10 13:00 ` [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request Pierre Morel
  2019-01-15 13:59   ` Cornelia Huck
  2019-01-15 15:35   ` Collin Walling
@ 2019-01-15 15:47   ` Cornelia Huck
  2019-01-15 17:35     ` Pierre Morel
  2 siblings, 1 reply; 13+ messages in thread
From: Cornelia Huck @ 2019-01-15 15:47 UTC (permalink / raw)
  To: Pierre Morel
  Cc: walling, borntraeger, rth, david, qemu-s390x, qemu-devel, pasic, thuth

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request
  2019-01-15 15:47   ` Cornelia Huck
@ 2019-01-15 17:35     ` Pierre Morel
  0 siblings, 0 replies; 13+ messages in thread
From: Pierre Morel @ 2019-01-15 17:35 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: walling, borntraeger, rth, david, qemu-s390x, qemu-devel, pasic, thuth

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. :)

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request
  2019-01-15 15:35   ` Collin Walling
@ 2019-01-16 12:40     ` Halil Pasic
  2019-01-16 14:16       ` Pierre Morel
  0 siblings, 1 reply; 13+ messages in thread
From: Halil Pasic @ 2019-01-16 12:40 UTC (permalink / raw)
  To: Collin Walling
  Cc: Pierre Morel, thuth, david, cohuck, qemu-devel, borntraeger,
	qemu-s390x, rth

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request
  2019-01-16 12:40     ` Halil Pasic
@ 2019-01-16 14:16       ` Pierre Morel
  2019-01-16 14:34         ` Cornelia Huck
  2019-01-16 14:50         ` Halil Pasic
  0 siblings, 2 replies; 13+ messages in thread
From: Pierre Morel @ 2019-01-16 14:16 UTC (permalink / raw)
  To: Halil Pasic, Collin Walling
  Cc: thuth, david, cohuck, qemu-devel, borntraeger, qemu-s390x, rth

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


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request
  2019-01-16 14:16       ` Pierre Morel
@ 2019-01-16 14:34         ` Cornelia Huck
  2019-01-16 14:50         ` Halil Pasic
  1 sibling, 0 replies; 13+ messages in thread
From: Cornelia Huck @ 2019-01-16 14:34 UTC (permalink / raw)
  To: Pierre Morel
  Cc: Halil Pasic, Collin Walling, thuth, david, qemu-devel,
	borntraeger, qemu-s390x, rth

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request
  2019-01-16 14:16       ` Pierre Morel
  2019-01-16 14:34         ` Cornelia Huck
@ 2019-01-16 14:50         ` Halil Pasic
  2019-01-16 15:44           ` Pierre Morel
  1 sibling, 1 reply; 13+ messages in thread
From: Halil Pasic @ 2019-01-16 14:50 UTC (permalink / raw)
  To: Pierre Morel
  Cc: Collin Walling, thuth, david, cohuck, qemu-devel, borntraeger,
	qemu-s390x, rth

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request
  2019-01-16 14:50         ` Halil Pasic
@ 2019-01-16 15:44           ` Pierre Morel
  2019-01-16 16:41             ` Cornelia Huck
  0 siblings, 1 reply; 13+ messages in thread
From: Pierre Morel @ 2019-01-16 15:44 UTC (permalink / raw)
  To: Halil Pasic
  Cc: Collin Walling, thuth, david, cohuck, qemu-devel, borntraeger,
	qemu-s390x, rth

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
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request
  2019-01-16 15:44           ` Pierre Morel
@ 2019-01-16 16:41             ` Cornelia Huck
  2019-01-17 15:13               ` Halil Pasic
  0 siblings, 1 reply; 13+ messages in thread
From: Cornelia Huck @ 2019-01-16 16:41 UTC (permalink / raw)
  To: Pierre Morel
  Cc: Halil Pasic, Collin Walling, thuth, david, qemu-devel,
	borntraeger, qemu-s390x, rth

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request
  2019-01-16 16:41             ` Cornelia Huck
@ 2019-01-17 15:13               ` Halil Pasic
  0 siblings, 0 replies; 13+ messages in thread
From: Halil Pasic @ 2019-01-17 15:13 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Pierre Morel, Collin Walling, david, qemu-devel, borntraeger,
	qemu-s390x, thuth, rth

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-01-17 15:14 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-10 13:00 [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size as guest wants Pierre Morel
2019-01-10 13:00 ` [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request Pierre Morel
2019-01-15 13:59   ` Cornelia Huck
2019-01-15 15:35   ` Collin Walling
2019-01-16 12:40     ` Halil Pasic
2019-01-16 14:16       ` Pierre Morel
2019-01-16 14:34         ` Cornelia Huck
2019-01-16 14:50         ` Halil Pasic
2019-01-16 15:44           ` Pierre Morel
2019-01-16 16:41             ` Cornelia Huck
2019-01-17 15:13               ` Halil Pasic
2019-01-15 15:47   ` Cornelia Huck
2019-01-15 17:35     ` Pierre Morel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.