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

[Markus Armbruster <armbru@redhat.com>]

Shameer Kolothum Thodi <skolothumtho@nvidia.com> writes:

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>

[...]

>> Nathan Chen <nathanc@nvidia.com> writes:
>> 
>> > 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.
>> >
>> > 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");
>> 
>> Why is this warning useful?
>> 
>> Does @auto's meaning depend on whether the device is cold- or
>> hot-plugged?
>
> If SMMUv3 accel=on and properties are set to "auto", we require
> at least one cold-plugged vfio-pci device to retrieve the associated
> host SMMUv3 information and finalise the QEMU SMMUv3 properties.
>
> In this series, "auto_mode" is set if any accel property is specified as
> "auto". The properties are then finalised using the first cold-plugged
> device and "auto_finalised" is set.
>
> Since we later fail the guest boot(see below) in the reset phase if
> auto_mode is set but auto_finalised is still false, the above hotplug
> case should not really happen. In that case the VM would not boot.

Sounds complicated.

> So likely we can get rid of the hotplug check above. Need to do
> a bit more testing to confirm we cover all corner cases.
>
>> 
>> > +        return;
>> > +    }
>> > +
>> > +    accel->auto_finalised = true;
>> > +}

[...]

>> > @@ -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.");
>> > +        exit(1);
>> 
>> How can we get here?
>
> This is reached from the SMMUv3 reset handler when auto mode is
> specified but no cold-plugged device has been attached to finalise the
> accelerated SMMUv3 properties.
>
> If no such device is present, "auto_finalised" remains false and we hit
> this path during reset.
>
> Cédric mentioned that we should not exit during the reset phase. If that
> is the case, we likely need another way to handle the scenario where
> auto mode is specified but no cold-plugged device is present.

What exactly does the "auto" feature buy us?  Is it worth the
complexity?