RE: [PATCH v3 9/9] iommu/arm-smmu-v3: Use S2FWB for NESTED domains

Tian, Kevin
2024-10-28T02:25:19Z

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, October 25, 2024 10:01 PM
>
> On Thu, Oct 24, 2024 at 07:54:10AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Thursday, October 10, 2024 12:23 AM
> > >
> > > Force Write Back (FWB) changes how the S2 IOPTE's MemAttr field
> > > works. When S2FWB is supported and enabled the IOPTE will force
> cachable
> > > access to IOMMU_CACHE memory when nesting with a S1 and deny
> cachable
> > > access otherwise.
> >
> > didn't get the last part "deny cacheable access otherwise"
>
> Force Write Back (FWB) changes how the S2 IOPTE's MemAttr field
> works. When S2FWB is supported and enabled the IOPTE will force cachable
> access to IOMMU_CACHE memory when nesting with a S1 and deny
> cachable
> access when !IOMMU_CACHE.
>
> ?

Good. I read the original words as if cacheable access is denied
when S2FWB is not supported or disabled.

>
> > > @@ -169,7 +169,8 @@ arm_smmu_domain_alloc_nesting(struct device
> *dev,
> > > u32 flags,
> > > * Must support some way to prevent the VM from bypassing the
> > > cache
> > > * because VFIO currently does not do any cache maintenance.
> > > */
> > > - if (!arm_smmu_master_canwbs(master))
> > > + if (!arm_smmu_master_canwbs(master) &&
> > > + !(master->smmu->features & ARM_SMMU_FEAT_S2FWB))
> > > return ERR_PTR(-EOPNOTSUPP);
> >
> > Probably can clarify the difference between CANWBS and S2FWB here
> > by copying some words from the previous commit message. especially
> > about the part of PCI nosnoop.
>
> /*
> * Must support some way to prevent the VM from bypassing the
> cache
> * because VFIO currently does not do any cache maintenance.
> canwbs
> * indicates the device is fully coherent and no cache maintenance is
> * ever required, even for PCI No-Snoop. S2FWB means the S1 can't
> make
> * things non-coherent using the memattr, but No-Snoop behavior is
> not
> * effected.
> */
> if (!arm_smmu_master_canwbs(master) &&
> !(master->smmu->features & ARM_SMMU_FEAT_S2FWB))
> return ERR_PTR(-EOPNOTSUPP);
>
> ?
>

Looks good!