Re: [Qemu-devel] [PATCH v2] s390x/pci: Set the iommu region size mpcifc request

[Halil Pasic]

On Tue, 15 Jan 2019 10:35:42 -0500
Collin Walling <address@hidden> 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 <address@hidden>
> > ---
> >   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 <address@hidden>
> 
>