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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>
> 
> Just one nit I think.
> 
>> --- 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 could be talked into setting just page_count to zero here.

Jan