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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2026 18:18, Andrew Cooper wrote:
> On 06/05/2026 2:55 pm, 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
> 
> Sorry, one more.  "We don't know which registers might".
> 
>> 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 attempt to forcefully disable the IOMMU ahead of enabling it.  Fold
>> map_iommu_mmio_region() into it's 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>
>> ---
>> Changes since v1:
>>  - Zero the control register after calling disable_iommu().
>>  - Print a warning message if the IOMMU is handed enabled to Xen from
>>    firmware.
>>  - Fix commit log grammar issues.
>> ---
>>  xen/drivers/passthrough/amd/iommu_init.c | 31 +++++++++++++-----------
>>  1 file changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/amd/iommu_init.c 
>> b/xen/drivers/passthrough/amd/iommu_init.c
>> index 76ae78e5ea53..ffc041211fb5 100644
>> --- 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;
>> -}
>> -
>>  static void __init unmap_iommu_mmio_region(struct amd_iommu *iommu)
>>  {
>>      if ( iommu->mmio_base )
>> @@ -1367,11 +1355,14 @@ static int __init amd_iommu_prepare_one(struct 
>> amd_iommu *iommu)
>>  {
>>      int rc = alloc_ivrs_mappings(iommu->sbdf.seg);
>>  
>> -    if ( !rc )
>> -        rc = map_iommu_mmio_region(iommu);
>>      if ( rc )
>>          return rc;
>>  
>> +    iommu->mmio_base = ioremap(iommu->mmio_base_phys,
>> +                               IOMMU_MMIO_REGION_LENGTH);
>> +    if ( !iommu->mmio_base )
>> +        return -ENOMEM;
>> +
>>      get_iommu_features(iommu);
>>  
>>      /*
>> @@ -1381,6 +1372,18 @@ static int __init amd_iommu_prepare_one(struct 
>> amd_iommu *iommu)
>>      if ( amd_iommu_max_paging_mode < amd_iommu_min_paging_mode )
>>          return -ERANGE;
>>  
>> +    /* Read current control register and forcefully disable the IOMMU. */
>> +    iommu->ctrl.raw = readq(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
>> +    if ( iommu->ctrl.iommu_en )
>> +        printk(XENLOG_WARNING
>> +               "AMD-Vi: IOMMU %pp enabled by firmware (%016lx)\n",
>> +               &iommu->sbdf, iommu->ctrl.raw);
>> +    disable_iommu(iommu, true);
>> +
>> +    /* With the IOMMU disabled zero the control register. */
>> +    iommu->ctrl.raw = 0;
>> +    writeq(0, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
>> +
>>      return 0;
>>  }
>>  
> 
> I don't think calling disable_iommu() is a good thing here.
> 
> It's just a cascade of clearing one/few bits in ctrl at a time, but is
> is added unconditionally so that's 5 UC stores writing 0's to the same
> register in the common case.
> 
> I think this logic wants to be:
> 
> @@ -1381,6 +1372,18 @@ static int __init amd_iommu_prepare_one(struct 
> amd_iommu *iommu)
>      if ( amd_iommu_max_paging_mode < amd_iommu_min_paging_mode )
>          return -ERANGE;
>  
> +    /* Check if the IOMMU is active, and disable. */
> +    iommu->ctrl.raw = readq(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
> +    if ( iommu->ctrl.iommu_en )
> +    {
> +        printk(XENLOG_WARNING
> +               "AMD-Vi: IOMMU %pp enabled by firmware (ctrl %016lx)\n",
> +               &iommu->sbdf, iommu->ctrl.raw);
> +
> +        iommu->ctrl.raw = 0;
> +        writeq(0, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
> +    }
> +
>      return 0;
>  }
> 
>  
> 
> which has the advantage that it's closer to the current behaviour, and
> therefore arguably a safer backport.
> 
> The only thing that disable_iommu() does which isn't editing the control
> register is playing with the PCI MSI enable bit, but that really doesn't
> matter when we clear ctrl.int_cap_xt_en.  (And in fact, that path is
> buggy because it clears MSI enable without inhibiting interrupt
> generation, which architecturally will turn into legacy line interrupt
> to deal with.)

Except there's no line interrupt associated with this?

Jan