Re: [Xen-devel] [PATCH v2 0/2] AMD/IOMMU: re-work mode updating

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/11/2019 11:33, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 15 November 2019 09:29
>> To: Durrant, Paul <pdurrant@xxxxxxxxxx>
>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper
>> <andrew.cooper3@xxxxxxxxxx>; Sander Eikelenboom <linux@xxxxxxxxxxxxxx>;
>> Juergen Gross <jgross@xxxxxxxx>
>> Subject: Re: [Xen-devel] [PATCH v2 0/2] AMD/IOMMU: re-work mode updating
>>
>> On 14.11.2019 18:29,  Durrant, Paul  wrote:
>>>> -----Original Message-----
>>>> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of
>> Jan
>>>> Beulich
>>>> Sent: 14 November 2019 16:42
>>>> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
>>>> Cc: Juergen Gross <jgross@xxxxxxxx>; Sander Eikelenboom
>>>> <linux@xxxxxxxxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> Subject: [Xen-devel] [PATCH v2 0/2] AMD/IOMMU: re-work mode updating
>>>>
>>>> update_paging_mode() in the AMD IOMMU code expects to be invoked with
>>>> the PCI devices lock held. The check occurring only when the mode
>>>> actually needs updating, the violation of this rule by the majority
>>>> of callers did go unnoticed until per-domain IOMMU setup was changed
>>>> to do away with on-demand creation of IOMMU page tables.
>>> Wouldn't it be safer to just get rid of update_paging_mode() and start
>>> with a reasonable number of levels?
>> Andrew did basically ask the same, but I continue to be unconvinced:
>> We can't pick a "reasonable" level, we have to pick the maximum a
>> guest may end up using.

4, and it is a reasonable number.

>> Yet why would we want to have all guests pay
>> the price of at least one unnecessary page walk level?

To a first approximation, I don't care.  The PTE will be cached on first
read, and in the common case will have no need to be invalidated.  I
doubt there would be any visible effect.

>>  I don't mean
>> to say I'm entirely opposed, but trading code simplicity for
>> performance is almost never an easy or obvious decision.
> I think in this case, versus the hoops your patches have to jump through just 
> to save (possibly) a level of IOMMU page walk, the simplicity argument is 
> quite compelling... particularly at this stage in the release cycle.

I agree.  The walk length should not ever have been variable.

The cost of the added complexity in Xen far outweighs any perceived
benefit.  Furthermore, you're adding an invasive and confusing core api
change to common code to work around a bug in a piece of functionality
which shouldn't have ever existed.

The safe option for 4.13 is a one-liner defaulting to 4 levels.

> The fact that we don't know, at start of day, what the max gfn of the guest 
> is going to be is also something that really ought to be fixed too... but 
> that is another debate.

We need it for migration safety decisions, so is on my list of things
needing to include into domaincreate.

At a future point where this information is available, we could optimise
4 down to 3 in most common cases.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel