Re: [PATCH v3] iommu/amd-vi: do not zero IOMMU MMIO region

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2026 18:51, Roger Pau Monne wrote:
> Attempting to memset the whole IOMMU MMIO region to zero is dangerous to
> say the least.  We don't know what registers might be there, nor which
> values might be safe for those registers.  On a forthcoming platform doing
> the zeroing of the MMIO region does put the IOMMU in a broken state, which
> is not recoverable by the IOMMU initialization procedure in Xen.
> 
> Instead just zero the control register, which mimics the current behavior
> with regards to how the control register is handled, and ensures the IOMU
> setup is done with the unit disabled.  This approach will need revisiting
> in order to support Preboot DMA Protection.
> 
> Fold map_iommu_mmio_region() into its only caller, as the function body is
> just an ioremap() call after the removal of the memset().
> 
> Fixes: 0700c962ac2d ("Add AMD IOMMU support into hypervisor")
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

While you got Andrew's R-b, I don't view that as enough to commit it. My
prior concern towards ...

> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -42,18 +42,6 @@ static bool iommu_has_ht_flag(struct amd_iommu *iommu, u8 
> mask)
>      return iommu->ht_flags & mask;
>  }
>  
> -static int __init map_iommu_mmio_region(struct amd_iommu *iommu)
> -{
> -    iommu->mmio_base = ioremap(iommu->mmio_base_phys,
> -                               IOMMU_MMIO_REGION_LENGTH);
> -    if ( !iommu->mmio_base )
> -        return -ENOMEM;
> -
> -    memset(iommu->mmio_base, 0, IOMMU_MMIO_REGION_LENGTH);
> -
> -    return 0;
> -}

... this part of the change wasn't addressed, neither verbally nor by an
adjustment to the description of what was committed. As previously stated,
blindly memset()-ing the entire area may not be the best of all options,
but the downsides of not doing this need to somehow be addressed. As
indicated, once they run out of bits in the main control register, they
likely will add a 2nd one. That'll then also need clearing, yet we have
no code to do so anymore.

Jan