On 3/18/26 10:31 AM, Shameer Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Nathan Chen <[email protected]>
>> Sent: 17 March 2026 18:38
>> To: [email protected]; [email protected]
>> Cc: Eric Auger <[email protected]>; Peter Maydell
>> <[email protected]>; Shannon Zhao <[email protected]>;
>> Michael S . Tsirkin <[email protected]>; Igor Mammedov
>> <[email protected]>; Ani Sinha <[email protected]>; Paolo Bonzini
>> <[email protected]>; Daniel P . Berrangé <[email protected]>; Eric
>> Blake <[email protected]>; Markus Armbruster <[email protected]>;
>> Shameer Kolothum Thodi <[email protected]>; Matt Ochs
>> <[email protected]>; Nicolin Chen <[email protected]>; Nathan Chen
>> <[email protected]>
>> Subject: [PATCH v3 2/8] hw/arm/smmuv3-accel: Change ATS property to
>> OnOffAuto
>>
>> From: Nathan Chen <[email protected]>
>>
>> Change accel SMMUv3 ATS property from bool to OnOffAuto. The 'auto'
>> value is not implemented, as this commit is meant to set the property
>> to the correct type and avoid breaking JSON/QMP when the auto mode is
>> introduced. A future patch will implement resolution of the 'auto'
>> value to match the host SMMUv3 ATS support.
>>
>> Fixes: f7f5013a55a3 ("hw/arm/smmuv3-accel: Add support for ATS")
>> Signed-off-by: Nathan Chen <[email protected]>
>> ---
>> hw/arm/smmuv3-accel.c | 6 ++++--
>> hw/arm/smmuv3.c | 14 ++++++++++++--
>> hw/arm/virt-acpi-build.c | 2 +-
>> include/hw/arm/smmuv3.h | 4 +++-
>> 4 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
>> index 2bb142c47f..621ac531a5 100644
>> --- a/hw/arm/smmuv3-accel.c
>> +++ b/hw/arm/smmuv3-accel.c
>> @@ -826,8 +826,10 @@ 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 */
>> + if (s->ats == ON_OFF_AUTO_ON) {
>> + s->idr[0] = FIELD_DP32(s->idr[0], IDR0, ATS, 1);
>> + }
> Nit: I think we can keep the original comment.
>
>> /* Advertise 48-bit OAS in IDR5 when requested (default is 44 bits). */
>> if (s->oas == SMMU_OAS_48BIT) {
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 068108e49b..3dead0bcd3 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -317,6 +317,11 @@ static void smmuv3_init_id_regs(SMMUv3State *s)
>> smmuv3_accel_idr_override(s);
>> }
>>
>> +bool smmuv3_ats_enabled(SMMUv3State *s)
>> +{
>> + 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 +1976,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;
>> }
>> @@ -1993,6 +1998,11 @@ static bool
>> smmu_validate_property(SMMUv3State *s, Error **errp)
>> return false;
>> }
>>
>> + if (s->ats == ON_OFF_AUTO_AUTO) {
>> + error_setg(errp, "ats cannot be set to auto");
>> + return false;
>> + }
> Nit: Is "ats auto mode not supported" better?
>
> Also, should we do this before the if (!s->accel) check above?
> Otherwise, I think it will carry on for non-accel smmuv3 cases like,
>
> -device arm-smmuv3,primary-bus=pcie.1,accel=off,ats=auto
>
> I think this applies to other properties in this series as well.
>
>> if (s->oas != SMMU_OAS_44BIT && s->oas != SMMU_OAS_48BIT) {
>> error_setg(errp, "OAS can only be set to 44 or 48 bits");
>> return false;
>> @@ -2128,7 +2138,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_OFF),
> I think we should update the description in
> object_class_property_set_description
> to mention auto not supported.
I agree with Shameer wrt all above comments
Eric
>
> Thanks,
> Shameer
>
>> DEFINE_PROP_UINT8("oas", SMMUv3State, oas, 44),
>> DEFINE_PROP_UINT8("ssidsize", SMMUv3State, ssidsize, 0),
>> };
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 719d2f994e..591cfc993c 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -402,7 +402,7 @@ static int iort_smmuv3_devices(Object *obj, void
>> *opaque)
>>
>> bus = PCI_BUS(object_property_get_link(obj, "primary-bus",
>> &error_abort));
>> sdev.accel = object_property_get_bool(obj, "accel", &error_abort);
>> - sdev.ats = object_property_get_bool(obj, "ats", &error_abort);
>> + sdev.ats = smmuv3_ats_enabled(ARM_SMMUV3(obj));
>> pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
>> sbdev = SYS_BUS_DEVICE(obj);
>> sdev.base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
>> diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
>> index 26b2fc42fd..ce51a5b9b4 100644
>> --- a/include/hw/arm/smmuv3.h
>> +++ b/include/hw/arm/smmuv3.h
>> @@ -70,7 +70,7 @@ struct SMMUv3State {
>> uint64_t msi_gpa;
>> Error *migration_blocker;
>> bool ril;
>> - bool ats;
>> + OnOffAuto ats;
>> uint8_t oas;
>> uint8_t ssidsize;
>> };
>> @@ -91,6 +91,8 @@ struct SMMUv3Class {
>> ResettablePhases parent_phases;
>> };
>>
>> +bool smmuv3_ats_enabled(struct SMMUv3State *s);
>> +
>> #define TYPE_ARM_SMMUV3 "arm-smmuv3"
>> OBJECT_DECLARE_TYPE(SMMUv3State, SMMUv3Class, ARM_SMMUV3)
>>
>> --
>> 2.43.0
