Re: [PATCH v2 14/18] IOMMU: fold flush-all hook into "flush one"

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Dec 14, 2021 at 10:06:37AM +0100, Jan Beulich wrote:
> On 13.12.2021 16:04, Roger Pau Monné wrote:
> > On Fri, Sep 24, 2021 at 11:53:59AM +0200, Jan Beulich wrote:
> >> Having a separate flush-all hook has always been puzzling me some. We
> >> will want to be able to force a full flush via accumulated flush flags
> >> from the map/unmap functions. Introduce a respective new flag and fold
> >> all flush handling to use the single remaining hook.
> >>
> >> Note that because of the respective comments in SMMU and IPMMU-VMSA
> >> code, I've folded the two prior hook functions into one. For SMMU-v3,
> >> which lacks a comment towards incapable hardware, I've left both
> >> functions in place on the assumption that selective and full flushes
> >> will eventually want separating.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> >> --- a/xen/drivers/passthrough/vtd/iommu.c
> >> +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> @@ -731,18 +731,21 @@ static int __must_check iommu_flush_iotl
> >>                                                  unsigned long page_count,
> >>                                                  unsigned int flush_flags)
> >>  {
> >> -    ASSERT(page_count && !dfn_eq(dfn, INVALID_DFN));
> >> -    ASSERT(flush_flags);
> >> +    if ( flush_flags & IOMMU_FLUSHF_all )
> >> +    {
> >> +        dfn = INVALID_DFN;
> >> +        page_count = 0;
> > 
> > Don't we expect callers to already pass an invalid dfn and a 0 page
> > count when doing a full flush?
> 
> I didn't want to introduce such a requirement. The two arguments should
> imo be don't-cares with IOMMU_FLUSHF_all, such that callers handing on
> (or accumulating) flags don't need to apply extra care.
> 
> > In the equivalent AMD code you didn't set those for the FLUSHF_all
> > case.
> 
> There's no similar dependency there in AMD code. For VT-d,
> iommu_flush_iotlb() needs at least one of the two set this way to
> actually do a full-address-space flush. (Which, as an aside, I've
> recently learned is supposedly wrong when cap_isoch() returns true. But
> that's an orthogonal issue, albeit it may be possible to deal with at
> the same time as, down the road, limiting the too aggressive flushing
> done by subsequent patches using this new flag.)

I see. AMD flush helper gets the flags as a parameter (because
the flush all function is a wrapper around the flush pages one), so
there's no need to signal a full flush using the other parameters.

> I could be talked into setting just page_count to zero here.

No, I think it's fine.

Thanks, Roger.