Re: [Xen-devel] [PATCH v3 5/7] xen/x86: disable global pages for domains with XPTI active

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.03.18 at 09:29, <jgross@xxxxxxxx> wrote:
> On 23/03/18 09:14, Jan Beulich wrote:
>>>>> On 23.03.18 at 08:58, <jgross@xxxxxxxx> wrote:
>>> On 23/03/18 08:46, Jan Beulich wrote:
>>>>>>> On 22.03.18 at 19:18, <jgross@xxxxxxxx> wrote:
>>>>> On 22/03/18 17:30, Jan Beulich wrote:
>>>>>>>>> On 21.03.18 at 13:51, <jgross@xxxxxxxx> wrote:
>>>>>>> --- a/xen/arch/x86/mm.c
>>>>>>> +++ b/xen/arch/x86/mm.c
>>>>>>> @@ -508,18 +508,23 @@ void make_cr3(struct vcpu *v, mfn_t mfn)
>>>>>>>  void write_ptbase(struct vcpu *v)
>>>>>>>  {
>>>>>>>      struct cpu_info *cpu_info = get_cpu_info();
>>>>>>> +    unsigned long new_cr4;
>>>>>>> +
>>>>>>> +    new_cr4 = (is_pv_vcpu(v) && !is_idle_vcpu(v))
>>>>>>> +              ? pv_guest_cr4_to_real_cr4(v) : mmu_cr4_features;
>>>>>>
>>>>>> I'm not overly happy to see any new uses of mmu_cr4_features.
>>>>>> This should really only be used for priming certain values imo,
>>>>>> which isn't the case here (otoh pv_guest_cr4_to_real_cr4() does
>>>>>> so too, and perhaps better wouldn't). Hence I wonder whether
>>>>>> this shouldn't be read_cr4() | X86_CR4_PGE, not the least
>>>>>> because we've just got rid of the blanket reversion to
>>>>>> mmu_cr4_features in VMX code.
>>>>>
>>>>> I do understand that using mmu_cr4_features isn't the best way to set
>>>>> cr4. But I think it is a good idea to have a default value which should
>>>>> normally be used instead of only switching various bits on and off.
>>>>>
>>>>> In case cr4 is loaded with a strange value in some corner case that
>>>>> value might be used from then on instead of being repaired by loading a
>>>>> dedicated value at certain points in time, e.g. when doing a context
>>>>> switch.
>>>>
>>>> But that would make it even more difficult to notice and debug a
>>>> possible problem. The more we play with CR4 bits, the more
>>>> important it is that we keep an accurate record of what is currently
>>>> loaded into it, and that we have a clear understanding of what we
>>>> mean to be loaded into the register at any given point in time.
>>>
>>> What about adding an appropriate ASSERT() for that case?
>>>
>>> So I would use read_cr4() | X86_CR4_PGE and ASSERT() that cr4 matches
>>> the default value.
>> 
>> That's an option; later we may want to sprinkle around a few more
>> such assertions.
> 
> Okay, I'll go that route then. Do you want me to use a new default_cr4
> variable for doing the assertion (and as initial cr4 value for secondary
> cpus), or are you fine with using mmu_cr4_features for now?

I'd prefer if we could get away without yet another variable holding
some variant of possible CR4 values. As a follow-up we'll then want
to switch pv_guest_cr4_to_real_cr4() away from using
mmu_cr4_features as well (except for a possible assertion to put
there).

And btw, please consider re-organizing your change to
pv_guest_cr4_to_real_cr4() to match what we do for X86_CR4_TSD,
rather than first ORing in X86_CR4_PGE in order to then
(conditionally) mask it back out.

Jan


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