Shameer Kolothum Thodi <[email protected]> writes: >> -----Original Message----- >> From: Markus Armbruster <[email protected]>
[...] >> Nathan Chen <[email protected]> writes: >> >> > From: Nathan Chen <[email protected]> >> > >> > 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 <[email protected]> >> > Signed-off-by: Nathan Chen <[email protected]> >> > --- >> > 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?
