On Tue, Mar 25, 2025 at 07:08:29PM +0100, Eric Auger wrote:
> > +static int
> > +smmuv3_accel_dev_install_nested_ste(SMMUv3AccelDevice *accel_dev,
> > + uint32_t data_type, uint32_t data_len,
> > + void *data)
> > +{
> > + SMMUViommu *viommu = accel_dev->viommu;
> > + SMMUS1Hwpt *s1_hwpt = accel_dev->s1_hwpt;
> > + HostIOMMUDeviceIOMMUFD *idev = accel_dev->idev;
> > +
> > + if (!idev || !viommu) {
> > + return -ENOENT;
> > + }
> > +
> > + if (s1_hwpt) {
> > + smmuv3_accel_dev_uninstall_nested_ste(accel_dev, false);
> why do you choose abort = false?
There is no particular reason. This is in the way where guest is
updating the STE. So we want to stage the device somewhere as its
default stage. Maybe ABORT could be a better place? I didn't give
this a deeper thought, to be honest. Good question :)
> > + ret = smmu_find_ste(sdev->smmu, sid, &ste, &event);
> > + if (ret) {
> > + /*
> > + * For a 2-level Stream Table, the level-2 table might not be ready
> > + * until the device gets inserted to the stream table. Ignore this.
> > + */
> I am confused by the above comment. Please can you describe the
> circumstances when this happens and why this should be an error
I added this since one of the early versions, and I don't recall
what was going on exactly... likely I saw smmu_find_ste() return
an error at that time when guest OS boots with Stream Table init,
yet later it installed the stage-1 STE and then smmu_find_ste()
started to return STE.
I think we can drop this comments, until we hit this again.
> > + return;
> > + }
> > +
> > + config = STE_CONFIG(&ste);
> > + if (!STE_VALID(&ste) || !STE_CFG_S1_ENABLED(config)) {
> you fully bypass the logic of smmuv3_get_config/decode_config. Couldn't
> you reuse it. Originally we used the s1ctxptr and disabled/bypassed info.
We likely can, though we don't need the CD part in decode_config
so we might need to split those functions to reuse.
Thanks
Nicolin
