Re: [PATCH 1/2][4.15?] x86/shadow: suppress "fast fault path" optimization when running virtualized

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/03/2021 14:29, Jan Beulich wrote:
> On 08.03.2021 14:59, Andrew Cooper wrote:
>> On 08/03/2021 13:51, Jan Beulich wrote:
>>> On 08.03.2021 14:47, Andrew Cooper wrote:
>>>> On 08/03/2021 09:25, Tim Deegan wrote:
>>>>> At 16:37 +0100 on 05 Mar (1614962224), Jan Beulich wrote:
>>>>>> We can't make correctness of our own behavior dependent upon a
>>>>>> hypervisor underneath us correctly telling us the true physical address
>>>>>> with hardware uses. Without knowing this, we can't be certain reserved
>>>>>> bit faults can actually be observed. Therefore, besides evaluating the
>>>>>> number of address bits when deciding whether to use the optimization,
>>>>>> also check whether we're running virtualized ourselves.
>>>>>>
>>>>>> Requested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> Acked-by: Tim Deegan <tim@xxxxxxx>
>>>>>
>>>>> I would consider this to be a bug in the underlying hypervisor, but I
>>>>> agree than in practice it won't be safe to rely on it being correct.
>>>> I'd argue against this being a hypervisor bug.  If anything, it is a
>>>> weakness in how x86 virtualisation works.
>>>>
>>>> For booting on a single host, then yes - vMAXPHYSADDR really ought to be
>>>> the same as MAXPHYSADDR, and is what happens in the common case.
>>>>
>>>> For booting in a heterogeneous pool, the only safe value is the min of
>>>> MAXPHYSADDR across the resource pool.  Anything higher, and the VM will
>>>> malfunction (get #PF[rsvd] for apparently-legal PTEs) on the smallest
>>>> pool member(s).
>>> Except that min isn't safe either - the guest may then expect reserved
>>> bit faults where none surface.
>> Such a guest is buggy, and in clear violation of the rules set out in
>> the architecture.  All reserved behaviour is subject to change in the
>> future.
>>
>> Any software (Xen included) deliberately choosing to depend on the
>> specifics of reserved behaviour, get to keep all resulting pieces.
> While I could understand what you're saying when considering our
> prior unconditional relying on getting reserved bit faults, are you
> suggesting the recently adjusted behavior is still "in clear
> violation of the rules set out in the architecture"? And hence are
> you suggesting we should have outright dropped that optimization?

Strictly speaking, we shouldn't use reserved bits at all.  That is the
most compatible option available.

Given that we have chosen to use reserved bits, it is our responsibility
for ensuring that they are safe to use.  That is why we elect not to use
them when virtualised (for this bug), or on native when we think they
won't work (the previous change for IceLake).

>From that point of view, I think we're fine.  We're (knowingly)
operating outside of the rules, and now taking appropriate precautions
to cover the corner cases we are aware of.

The behaviour of already-shipped CPUs, while not architecturally
guarenteed, is known and can reasonably be depended upon.

~Andrew