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

[Pavel Hrdina <phrdina@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7036 bytes --]

On Wed, Mar 11, 2026 at 06:16:09PM +0100, Eric Auger wrote:
> 
> 
> 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>
> >>>> ---
> >>>>   hw/arm/smmuv3-accel.c    | 25 +++++++++++++++++++++++--
> >>>>   hw/arm/smmuv3.c          | 12 ++++++++++--
> >>>>   hw/arm/virt-acpi-build.c |  2 +-
> >>>>   include/hw/arm/smmuv3.h  |  2 +-
> >>>>   4 files changed, 35 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> >>>> index 617629bacd..8fec335557 100644
> >>>> --- a/hw/arm/smmuv3-accel.c
> >>>> +++ b/hw/arm/smmuv3-accel.c
> >>>> @@ -52,6 +52,12 @@ static void
> >>>> smmuv3_accel_auto_finalise(SMMUv3State *s, PCIDevice *pdev,
> >>>>           return;
> >>>>       }
> >>>>   +    /* Update ATS if auto from info */
> >>>> +    if (s->ats == ON_OFF_AUTO_AUTO) {
> >>>> +        s->idr[0] = FIELD_DP32(s->idr[0], IDR0, ATS,
> >>>> +                               FIELD_EX32(info->idr[0], IDR0, ATS));
> >>>> +    }
> >>>> +
> >>>>       accel->auto_finalised = true;
> >>>>   }
> >>>>   @@ -124,6 +130,13 @@ smmuv3_accel_check_hw_compatible(SMMUv3State
> >>>> *s,
> >>>>                      smmuv3_oas_bits(FIELD_EX32(s->idr[5], IDR5,
> >>>> OAS)));
> >>>>           return false;
> >>>>       }
> >>>> +    /* Check ATS value opted is compatible with Host SMMUv3 */
> >>>> +    if (FIELD_EX32(info->idr[0], IDR0, ATS) <
> >>>> +                FIELD_EX32(s->idr[0], IDR0, ATS)) {
> >>>> +        error_setg(errp, "Host SMMUv3 doesn't support Address
> >>>> Translation"
> >>>> +                   " Services");
> >>>> +        return false;
> >>>> +    }
> >>>>         /* QEMU SMMUv3 supports GRAN4K/GRAN16K/GRAN64K translation
> >>>> granules */
> >>>>       if (FIELD_EX32(info->idr[5], IDR5, GRAN4K) !=
> >>>> @@ -844,8 +857,12 @@ void smmuv3_accel_idr_override(SMMUv3State *s)
> >>>>       /* By default QEMU SMMUv3 has RIL. Update IDR3 if user has
> >>>> disabled it */
> >>>>       s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, s->ril);
> >>>>   -    /* QEMU SMMUv3 has no ATS. Advertise ATS if opt-in by
> >>>> property */
> >>>> -    s->idr[0] = FIELD_DP32(s->idr[0], IDR0, ATS, s->ats);
> >>>> +    /* Only override ATS if user explicitly set ON or OFF */
> >>>> +    if (s->ats == ON_OFF_AUTO_ON) {
> >>>> +        s->idr[0] = FIELD_DP32(s->idr[0], IDR0, ATS, 1);
> >>>> +    } else if (s->ats == ON_OFF_AUTO_OFF) {
> >>>> +        s->idr[0] = FIELD_DP32(s->idr[0], IDR0, ATS, 0);
> >>>> +    }
> >>>>         /* Advertise 48-bit OAS in IDR5 when requested (default is
> >>>> 44 bits). */
> >>>>       if (s->oas == SMMU_OAS_48BIT) {
> >>>> @@ -923,4 +940,8 @@ void smmuv3_accel_init(SMMUv3State *s)
> >>>>       s->s_accel = g_new0(SMMUv3AccelState, 1);
> >>>>       bs->iommu_ops = &smmuv3_accel_ops;
> >>>>       smmuv3_accel_as_init(s);
> >>>> +
> >>>> +    if (s->ats == ON_OFF_AUTO_AUTO) {
> >>>> +        s->s_accel->auto_mode = true;
> >>>> +    }
> >>>>   }
> >>>> 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
> >>>> @@ -317,6 +317,12 @@ static void smmuv3_init_id_regs(SMMUv3State *s)
> >>>>       smmuv3_accel_idr_override(s);
> >>>>   }
> >>>>   +static bool get_ats_enabled(Object *obj, Error **errp)
> >>>> +{
> >>>> +    SMMUv3State *s = ARM_SMMUV3(obj);
> >>>> +    return FIELD_EX32(s->idr[0], IDR0, ATS);
> >>>> +}
> >>>> +
> >>>>   static void smmuv3_reset(SMMUv3State *s)
> >>>>   {
> >>>>       s->cmdq.base = deposit64(s->cmdq.base, 0, 5, SMMU_CMDQS);
> >>>> @@ -1971,7 +1977,7 @@ static bool
> >>>> smmu_validate_property(SMMUv3State *s, Error **errp)
> >>>>               error_setg(errp, "ril can only be disabled if
> >>>> accel=on");
> >>>>               return false;
> >>>>           }
> >>>> -        if (s->ats) {
> >>>> +        if (s->ats == ON_OFF_AUTO_ON) {
> >>>>               error_setg(errp, "ats can only be enabled if accel=on");
> >>>>               return false;
> >>>>           }
> >>>> @@ -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].
> >
> > 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. 
> Thanks

I think it should be safe and to change the type of the property, it's
not part of any QEMU release and not even implemented in libvirt.

Pavel

> 
> Eric
> >
> > Thanks,
> > Nathan
> >
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]