Re: [RFC PATCH 2/8] hw/arm/smmuv3-accel: Introduce _AUTO support for ATS

[Markus Armbruster <armbru@redhat.com>]

Eric Auger <eric.auger@redhat.com> writes:

> On 3/11/26 6:08 PM, Nathan Chen wrote:
>>
>>
>> On 3/11/2026 8:31 AM, Eric Auger wrote:
>>> On 3/10/26 8:05 AM, Markus Armbruster wrote:
>>>> Nathan Chen<nathanc@nvidia.com> writes:
>>>>
>>>>> From: Nathan Chen<nathanc@nvidia.com>
>>>>>
>>>>> Allow accelerated SMMUv3 Address Translation Services support property
>>>>> to be derived from host IOMMU capabilities. Derive host values using
>>>>> IOMMU_GET_HW_INFO, retrieving ATS capability from IDR0.
>>>>>
>>>>> Set the default value of ATS to auto. The default for ATS support used
>>>>> to be set to off, but we change it to match what the host IOMMU
>>>>> properties report.
>>>>>
>>>>> Add a "ats-enabled" read-only property for smmuv3 to address an
>>>>> expected bool for the "ats" property in iort_smmuv3_devices().
>>>>>
>>>>> Signed-off-by: Nathan Chen<nathanc@nvidia.com>

[...]

>>>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>>>> index 068108e49b..197ba7c77b 100644
>>>>> --- a/hw/arm/smmuv3.c
>>>>> +++ b/hw/arm/smmuv3.c

[...]

>>>>> @@ -2128,7 +2134,7 @@ static const Property smmuv3_properties[] = {
>>>>>      DEFINE_PROP_UINT64("msi-gpa", SMMUv3State, msi_gpa, 0),
>>>>>      /* RIL can be turned off for accel cases */
>>>>>      DEFINE_PROP_BOOL("ril", SMMUv3State, ril, true),
>>>>> -    DEFINE_PROP_BOOL("ats", SMMUv3State, ats, false),
>>>>> +    DEFINE_PROP_ON_OFF_AUTO("ats", SMMUv3State, ats, ON_OFF_AUTO_AUTO),
>>>>
>>>> Is property "ats" accessible via QMP or JSON command line?  If yes,
>>>> this
>>>> is an incompatible change: JSON values false and true no longer work.
>>> So what do you recommend to extend the property values. I guess we had
>>> some existing scenarios happening in the past. What is the best way to
>>> proceed?
>>
>> Is it a requirement to ensure boolean values are still supported when
>> switching to OnOffAuto? I found that the x86-iommu intremap property
>> had a change from DEFINE_PROP_BOOL to DEFINE_PROP_ON_OFF_AUTO [0].

We promise to follow a certain process when changing external
interfaces: docs/about/deprecated.rst.

This is why I inquired about external access.  No external access, no
compatibility worries.

I specifically asked about JSON, because going from bool to OnOffAuto is
mostly compatible in key=value syntax: values "on" and "off" keep
working.

Mostly compatible, because qapi_bool_parse() also recognizes "yes",
"true", "y", "no", "false", and "n" for convenience, and these stop
working.  We have a knack for inventing convenience features that later
bite us.

JSON breaks entirely, of course: all values stop working.

Even with external access, compatibility worries start only after we
shipped the interface in a release.  Did we?

If the interface was released, we should keep our promise to follow the
process.  From time to time, that's so onerous and we're so certain that
nobody actually cares, we bend the rules.  This should not be done
lightly.

>> Should I add a custom property setter that accepts both boolean and
>> on/off/auto for backward compatibility, or is following the intremap
>> precedent acceptable? I'm happy to implement either approach.
>>
>> [0]
>> https://github.com/qemu/qemu/commit/a924b3d8df55a395891fd5ed341d0deb135d9aa6 
>
> Maybe we also need to emphasize that libvirt integration is still under
> work (and waiting for that conversion to happen at qemu level). So do we
> really care about keeping the compat wrt QMP and JSON. 

I can explain our promise and the process to you, but I can't make the
decision to bend the rules for you.