Re: [PATCH] s390: PCI: fix IOMMU region init

[Pierre Morel]

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 <address@hidden>


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 <address@hidden>
> Reported-by: Stefan Zimmerman <address@hidden>
> Signed-off-by: Matthew Rosato <address@hidden>
> ---
>   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);

--
Pierre Morel
IBM Lab Boeblingen