Re: [PATCH] iommu/arm-smmu-v3: Maintain valid access attributes for non-coherent SMMU

[Jason Gunthorpe <jgg@ziepe.ca>]

> >  /* DMA for "CMDQ MSI" which targets q->base_dma allocated by arm_smmu_init_one_queue() */
> > 
> > > @@ -1612,11 +1624,18 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
> > >  		(cd_table->cdtab_dma & STRTAB_STE_0_S1CTXPTR_MASK) |
> > >  		FIELD_PREP(STRTAB_STE_0_S1CDMAX, cd_table->s1cdmax));
> > >  
> > > +	if (smmu_coherent(smmu)) {
> > > +		val = FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) |
> > > +		      FIELD_PREP(STRTAB_STE_1_S1COR, STRTAB_STE_1_S1C_CACHE_WBRA) |
> > > +		      FIELD_PREP(STRTAB_STE_1_S1CSH, ARM_SMMU_SH_ISH);
> > > +	} else {
> > > +		val = FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_NC) |
> > > +		      FIELD_PREP(STRTAB_STE_1_S1COR, STRTAB_STE_1_S1C_CACHE_NC) |
> > > +		      FIELD_PREP(STRTAB_STE_1_S1CSH, ARM_SMMU_SH_OSH);
> > > +	}
> > 
> > This one is "CD fetch" allocated by arm_smmu_alloc_cd_ptr()
> > 
> > etc
> > 
> > And note that the above will need this hunk too:
> > 
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > @@ -432,6 +432,14 @@ size_t arm_smmu_get_viommu_size(struct device *dev,
> >             !(smmu->features & ARM_SMMU_FEAT_S2FWB))
> >                 return 0;
> >  
> > +       /*
> > +        * When running non-coherent we can't suppot S2FWB since it will also
> > +        * force a coherent CD fetch, aside from the question of what
> > +        * S2FWB/CANWBS even does with non-coherent SMMUs.
> > +        */
> > +       if (!smmu_coherent(smmu))
> > +               return 0;
> 
> I was wondering why S2FWB can affect CD fetching before reading 13.8.

Yeah, that table is super useful

> +               if (!fwb) {
> +                       val = FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_NC) |
> +                               FIELD_PREP(STRTAB_STE_1_S1COR, STRTAB_STE_1_S1C_CACHE_NC) |
> +                               FIELD_PREP(STRTAB_STE_1_S1CSH, ARM_SMMU_SH_OSH);
> +               } else {
> +                       dev_warn(&smmu->dev, "Inconsitency between COHACC & S2FWB\n");
> +                       /* FIX ME */
> +                       return;

I prefer we block it as I showed instead of printing a warning, cache
inconsistencies in virtualization can become security problems.

Though it is probably good to add a comment here about s2fwb also

Jason