Re: [RFC PATCH 1/8] hw/arm/smmuv3-accel: Add helper for resolving auto parameters

["Cédric Le Goater" <clg@redhat.com>]

Hello Shameer

On 3/10/26 09:40, Shameer Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: 10 March 2026 07:42
>> To: Nathan Chen <nathanc@nvidia.com>; qemu-devel@nongnu.org; qemu-
>> arm@nongnu.org
>> Cc: Yi Liu <yi.l.liu@intel.com>; Eric Auger <eric.auger@redhat.com>;
>> Zhenzhong Duan <zhenzhong.duan@intel.com>; Peter Maydell
>> <peter.maydell@linaro.org>; Shannon Zhao <shannon.zhaosl@gmail.com>;
>> Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
>> <imammedo@redhat.com>; Ani Sinha <anisinha@redhat.com>; Paolo
>> Bonzini <pbonzini@redhat.com>; Daniel P . Berrangé
>> <berrange@redhat.com>; Alex Williamson <alex@shazbot.org>; Eric Blake
>> <eblake@redhat.com>; Markus Armbruster <armbru@redhat.com>;
>> Shameer Kolothum Thodi <skolothumtho@nvidia.com>
>> Subject: Re: [RFC PATCH 1/8] hw/arm/smmuv3-accel: Add helper for resolving
>> auto parameters
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 3/9/26 20:21, Nathan Chen wrote:
>>> From: Nathan Chen <nathanc@nvidia.com>
>>>
>>> Introduce smmuv3_accel_auto_finalise() to resolve properties
>>> that are set to 'auto' for accelerated SMMUv3. This helper
>>> function allows properties such as ATS, RIL, SSIDSIZE, and OAS
>>> support to be resolved from host IOMMU values, while avoiding
>>> triggering auto-resolved values for hot-plugged devices.
>>>
>>> Auto mode requires at least one cold-plugged device to retrieve
>>> and finalise these properties, and we fail boot if that is not
>>> the case.
>>
>> IIUC, QEMU will require a minimum of one PCI device if
>> arm-smmuv3,accel=on is passed on the command line and if the
>> smmu properties are set to 'auto'.
> 
> Yes, that’s correct.
> 
>> I would try to enforce this requirement in the realize routine.
>> Can't we leverage the supports_address_space() handler ? See :
>>
>>
>>       static bool smmuv3_accel_supports_as(PCIBus *bus, void *opaque, int
>> devfn,
>>                                            Error **errp)
>>       {
>>           PCIDevice *pdev = pci_find_device(bus, pci_bus_num(bus), devfn);
>>           bool vfio_pci = false;
>>
>>           if (pdev && !smmuv3_accel_pdev_allowed(pdev, &vfio_pci)) {
>>               if (vfio_pci) {
>>                   error_setg(errp, "vfio-pci endpoint devices without an iommufd "
>>                              "backend not allowed when using arm-smmuv3,accel=on");
>>       ...
> Hmm..i doubt we can do it here as we need HostIOMMUDeviceIOMMUFD *idev
> for retrieving the IOMMU_GET_HW_INFO.
> 
> supports_address_space() is invoked very early during do_pci_register_device()
> and there are no associated idev available at that time.


yes. Have your considered adding an 'iommufd' property to smmuv3
instead ? I think this should solve a lot of potential issues
and would be more model friendly.

Sorry if that was proposed already. I have been through all the
emails.


> 
>>>
>>> Subsequent patches will make use of this helper to set the
>>> values when we convert the values to OnOffAuto. New auto_mode
>>> and auto_finalised bool members are added to SMMUv3AccelState.
>>> smmuv3_accel_init() will set auto_mode to true when 'auto' is
>>> detected for the accel SMMUv3 properties.
>>> smmuv3_accel_auto_finalise() will set auto_finalised to true
>>> after all 'auto' properties are resolved, and subsequent
>>> calls to this function will return early if auto_finalised is
>>> set to true.
>>>
>>> Suggested-by: Shameer Kolothum <skolothumtho@nvidia.com>
>>> Signed-off-by: Nathan Chen <nathanc@nvidia.com>
>>> ---
>>>    hw/arm/smmuv3-accel.c | 38 +++++++++++++++++++++++++++++++++-----
>>>    hw/arm/smmuv3-accel.h |  2 ++
>>>    2 files changed, 35 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
>>> index 17306cd04b..617629bacd 100644
>>> --- a/hw/arm/smmuv3-accel.c
>>> +++ b/hw/arm/smmuv3-accel.c
>>> @@ -35,11 +35,34 @@ static int smmuv3_oas_bits(uint32_t oas)
>>>        return map[oas];
>>>    }
>>>
>>> +static void smmuv3_accel_auto_finalise(SMMUv3State *s, PCIDevice
>> *pdev,
>>> +                                       struct iommu_hw_info_arm_smmuv3 *info) {
>>> +    SMMUv3AccelState *accel = s->s_accel;
>>> +
>>> +    /* Return if no auto for any or finalised already */
>>> +    if (!accel->auto_mode || accel->auto_finalised) {
>>> +        return;
>>> +    }
>>> +
>>> +    /* We can't update if device is hotplugged */
>>> +    if (DEVICE(pdev)->hotplugged) {
>>> +        warn_report("arm-smmuv3: 'auto' feature property detected, but host
>> "
>>> +                    "value cannot be applied for hot-plugged device; using "
>>> +                    "existing value");
>>
>> Please add an 'Error **' parameter instead.
>>
>>> +        return;
>>> +    }
>>> +
>>> +    accel->auto_finalised = true;
>>> +}
>>> +
>>>    static bool
>>>    smmuv3_accel_check_hw_compatible(SMMUv3State *s,
>>>                                     struct iommu_hw_info_arm_smmuv3 *info,
>>> +                                 PCIDevice *pdev,
>>>                                     Error **errp)
>>>    {
>>> +    smmuv3_accel_auto_finalise(s, pdev, info);
>>> +
>>>        /* QEMU SMMUv3 supports both linear and 2-level stream tables */
>>>        if (FIELD_EX32(info->idr[0], IDR0, STLEVEL) !=
>>>                    FIELD_EX32(s->idr[0], IDR0, STLEVEL)) {
>>> @@ -124,7 +147,7 @@
>> smmuv3_accel_check_hw_compatible(SMMUv3State *s,
>>>
>>>    static bool
>>>    smmuv3_accel_hw_compatible(SMMUv3State *s,
>> HostIOMMUDeviceIOMMUFD *idev,
>>> -                           Error **errp)
>>> +                           PCIDevice *pdev, Error **errp)
>>>    {
>>>        struct iommu_hw_info_arm_smmuv3 info;
>>>        uint32_t data_type;
>>> @@ -142,7 +165,7 @@ smmuv3_accel_hw_compatible(SMMUv3State *s,
>> HostIOMMUDeviceIOMMUFD *idev,
>>>            return false;
>>>        }
>>>
>>> -    if (!smmuv3_accel_check_hw_compatible(s, &info, errp)) {
>>> +    if (!smmuv3_accel_check_hw_compatible(s, &info, pdev, errp)) {
>>>            return false;
>>>        }
>>>        return true;
>>> @@ -595,6 +618,7 @@ static bool
>> smmuv3_accel_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>>>        SMMUv3State *s = ARM_SMMUV3(bs);
>>>        SMMUPciBus *sbus = smmu_get_sbus(bs, bus);
>>>        SMMUv3AccelDevice *accel_dev = smmuv3_accel_get_dev(bs, sbus,
>> bus, devfn);
>>> +    PCIDevice *pdev = pci_find_device(bus, pci_bus_num(bus), devfn);
>>>
>>>        if (!idev) {
>>>            return true;
>>> @@ -613,7 +637,7 @@ static bool
>> smmuv3_accel_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>>>         * Check the host SMMUv3 associated with the dev is compatible with
>> the
>>>         * QEMU SMMUv3 accel.
>>>         */
>>> -    if (!smmuv3_accel_hw_compatible(s, idev, errp)) {
>>> +    if (!smmuv3_accel_hw_compatible(s, idev, pdev, errp)) {
>>>            return false;
>>>        }
>>>
>>> @@ -867,8 +891,12 @@ bool
>> smmuv3_accel_attach_gbpa_hwpt(SMMUv3State *s, Error **errp)
>>>
>>>    void smmuv3_accel_reset(SMMUv3State *s)
>>>    {
>>> -     /* Attach a HWPT based on GBPA reset value */
>>> -     smmuv3_accel_attach_gbpa_hwpt(s, NULL);
>>> +    if (s->s_accel && s->s_accel->auto_mode && !s->s_accel-
>>> auto_finalised) {
>>> +        error_report("AUTO mode specified but properties not finalised.");
>>
>> This is not a very friendly message for the user.
>>
>>
>>> +        exit(1);
>>
>> QEMU should not exit in the reset phase. Can this check be done during
>> the realize stage ?
> 
> SMMUv3 realize stage happens before the cold-plugged device
> set_iommu_device(), and therefore we can't check whether the SMMUv3
> properties have been retrieved and finalised at that point.
> 
> Not sure there is any other place we can do this other than in the reset
> path.
> 
> Is avoiding exit(1) in the reset phase a strict requirement or a nice to
> have one?

reset is a runtime handler called at each reboot. One should not
exit.

We should try to verify that the conditions to run the machine
are correct before reaching the reset phase.

Thanks,

C.