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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2026 11:20, Roger Pau Monné wrote:
> On Wed, May 06, 2026 at 11:17:25AM +0200, Jan Beulich wrote:
>> On 06.05.2026 10:43, Roger Pau Monné wrote:
>>> On Wed, May 06, 2026 at 10:28:52AM +0200, Jan Beulich wrote:
>>>> On 06.05.2026 09:37, Roger Pau Monne wrote:
>>>>> @@ -1381,6 +1372,11 @@ 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);
>>>>> +    disable_iommu(iommu, true);
>>>>
>>>> Don't you also need to pre-fill iommu->features?
>>>
>>> Indeed, that's done just ahead of this chunk, in the
>>> get_iommu_features() call.
>>>
>>>> And with that field's use in
>>>> disable_iommu(), won't we be at risk of leaving stuff enabled which we are
>>>> entirely unaware of?
>>>
>>> Possibly, yes, that's always a risk.
>>>
>>>> Even if we fully cleared the control register (which
>>>> would eliminate the need to fetch features), down the road a 2nd control
>>>> register could appear.
>>>
>>> We do clear the control register, it's indirectly done by us setting
>>> iommu->ctrl.raw = 0 after the disable_iommu() call.
>>>
>>> I did wonder about just doing a write of 0 to the control register,
>>> but I think it's best if we try to gracefully disable the features (as
>>> done in disable_iommu()), and then reset the cached control state to
>>> 0.  Future writes to the control register will clear any bits not
>>> directly set by Xen.
>>
>> Maybe better to explicitly write out that 0 right away, even if you want
>> to keep using disable_iommu()?
> 
> Yeah, I also considered that.  So after disable_iommu() set the cached
> control state to 0 and also zero the control register right there.
> Can do in the next version, unless there are further objections.

Well, as said - I'm wary of fully dropping the memset(). It may help now,
but it could easily cause issues later.

Jan