Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.12.2022 10:50, Demi Marie Obenour wrote:
> On Thu, Dec 22, 2022 at 10:35:08AM +0100, Jan Beulich wrote:
>> On 20.12.2022 02:07, Demi Marie Obenour wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -6352,6 +6352,11 @@ unsigned long get_upper_mfn_bound(void)
>>>      return min(max_mfn, 1UL << (paddr_bits - PAGE_SHIFT)) - 1;
>>>  }
>>>  
>>> +
>>> +/*
>>> + * A bunch of static assertions to check that the XEN_MSR_PAT is valid
>>> + * and consistent with the _PAGE_* macros, and that _PAGE_WB is zero.
>>> + */
>>>  static void __init __maybe_unused build_assertions(void)
>>>  {
>>>      /*
>>> @@ -6361,6 +6366,72 @@ static void __init __maybe_unused 
>>> build_assertions(void)
>>>       * using different PATs will not work.
>>>       */
>>>      BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL);
>>> +
>>> +    /*
>>> +     * _PAGE_WB must be zero for several reasons, not least because Linux
>>> +     * assumes it.
>>> +     */
>>> +    BUILD_BUG_ON(_PAGE_WB);
>>
>> Strictly speaking this is a requirement only for PV guests. We may not
>> want to go as far as putting "#ifdef CONFIG_PV" around it, but at least
>> the code comment (and then also the part of the description referring
>> to this) will imo want to say so.
> 
> Does Xen itself depend on this?

With the wording in the description I was going from the assumption that
you have audited code and found that we properly use _PAGE_* constants
everywhere.

>>> +} while (0)
>>> +
>>> +    /*
>>> +     * If one of these trips, the corresponding _PAGE_* macro is 
>>> inconsistent
>>> +     * with XEN_MSR_PAT.  This would cause Xen to use incorrect 
>>> cacheability
>>> +     * flags, with results that are undefined and probably harmful.
>>
>> Why "undefined"? They may be wrong / broken, but the result is still well-
>> defined afaict.
> 
> “undefined” is meant as “one has violated a core assumption that
> higher-level stuff depends on, so things can go arbitrarily wrong,
> including e.g. corrupting memory or data”.  Is this accurate?  Should I
> drop the dependent clause, or do you have a suggestion for something
> better?

s/undefined/unknown/ ?

Jan