Re: [PATCH] x86/PV32: restore PAE-extended-CR3 logic

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.04.2023 22:40, Andrew Cooper wrote:
> On 04/04/2023 3:21 pm, Jan Beulich wrote:
>> On 04.04.2023 15:08, Andrew Cooper wrote:
>>> On 15/02/2023 2:54 pm, Jan Beulich wrote:
>>>> While the PAE-extended-CR3 VM assist is a 32-bit only concept, it still
>>>> applies to guests also when run on a 64-bit hypervisor:
>>> Is this really true?  Even when looking at Xen 4.2, 32bit guests are
>>> required to pass a full 4k page, not a 32b quad.
>> The full-page vs 32b-quad aspect is orthogonal. This VM-assist is solely
>> about where that data structure is, not what size it is.
>>
>>> Which makes complete sense.  It was a hard requirement of 32bit non-PAE
>>> guests, so it was a natural restriction to maintain into 32bit PAE guests.
>>>
>>> This is *only* a 32-on-64 issue, because this is the only case a 32bit
>>> guest could in principle use an L3 placed above the 4G boundary.
>> Not exactly. 32-bit Xen maintained a 4-entry "shadow" array below 4G
>> that it would copy (massage) the guest entries into upon CR3 reload
>> (just look for struct pae_l3_cache in the old sources). So above-4G
>> page table base was possible there as well.
> 
> Oh eww, so while Xen never gained an optimisation to permit only a 32b
> quad in place of a full 4k L3 table, it did support having the full
> tables higher.
> 
> (This code is especially hard to follow with #ifdefary in the common
> mm.c when there are perfectly good x86_{32,64}/mm.c's to use for
> differing function implementations...)

Except that the #ifdef-ary was wrong, and should have been suitable if()
instead. Those having been #ifdef was why the respective code got
removed in the course of purging 32-bit Xen logic.

>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>> @@ -1520,6 +1520,23 @@ static int promote_l3_table(struct page_
>>>>      unsigned int   partial_flags = page->partial_flags;
>>>>      l3_pgentry_t   l3e = l3e_empty();
>>>>  
>>>> +    /*
>>>> +     * PAE pgdirs above 4GB are unacceptable if a 32-bit guest does not
>>>> +     * understand the weird 'extended cr3' format for dealing with 
>>>> high-order
>>>> +     * address bits. We cut some slack for control tools (before vcpu0 is
>>>> +     * initialised).
>>>> +     */
>>>> +    if ( is_pv_32bit_domain(d) &&
>>>> +         unlikely(!VM_ASSIST(d, pae_extended_cr3)) &&
>>>> +         mfn_x(l3mfn) >= 0x100000 &&
>>>> +         d->vcpu[0] && d->vcpu[0]->is_initialised )
>>>> +    {
>>>> +        gdprintk(XENLOG_WARNING,
>>>> +                 "PAE pgd must be below 4GB (%#lx >= 0x100000)",
>>>> +                 mfn_x(l3mfn));
>>>> +        return -ERANGE;
>>>> +    }
>>> Having dug through source history, I see this is largely the form that
>>> it used to be.
>>>
>>> But I'm unconvinced by the "cut control tools some slack".  I'm quite
>>> tired of different bits of Xen taking on unnecessary complexity because
>>> people are unwilling to fix the problem at the correct layer.
>> But anything tools do before having created the first vCPU would not
>> have had any means to engage the VM-assist. I.e. ...
>>
>>> A toolstack which has non-pae_extended_cr3 guest on its hand will know
>>> this before any pagetables get allocated.
>> ... this knowledge buys it nothing: It would need to move the table
>> to below 4G irrespective of knowing that the guest can deal with
>> bigger addresses, just to get past this check.
> 
> This just goes from bad to worse.  It is mad that the VMASSIST flags
> can't be set ahead of a vcpu initialise hypercall.
> 
> But.
> 
> The code in xg_dom_x86.c unconditionally moves the L3 below the 4G
> boundary, so the thing actually pinned as an L3 will always pass the check.

Where do you see this being dome unconditionally? I only see this inside
a check of dom->parms->pae being XEN_PAE_YES.

> Which is just as well because it too blindly applies the extended-cr3
> transform momentarily after conditionally setting
> VMASST_TYPE_pae_extended_cr3...

Doing this "blindly" is (kind of) fine, isn't it? The transformation is
an identity one when extended-CR3 isn't in use.

> So a 32bit PV guests will pass the check irrespective of their
> pae_extended_cr3 setting.

As per above - I question this: dom->parms->pae isn't a simple boolean
(see enum xen_pae_type in libelf.h, and as can also be seen from the
condition around the enabling of the VM assist).

>>> For this check specifically, I'd suggest prohibiting non-32p guests from
>>> setting pae_extended_cr3 in the first place (I see no limit currently),
>>> and then simplifying the check to just
>>>
>>> if ( unlikely(!VM_ASSIST(d, pae_extended_cr3)) &&
>>>      mfn_x(l3mfn) >= PFN_DOWN(GB(4)) )
>> Dropping the is_pv_32bit_domain() check isn't possible because we can't,
>> all of the sudden, fail 64-bit guests' requests to enable this VM-
>> assist (no matter that we know that it is of no use to them).
> 
> I'm not so sure about this.  This VMASSIST cannot credibly be set at
> runtime,

Why not? A kernel may statically say XEN_PAE_YES, resulting in its
initial L3 to be relocated to below 4G, and then - before any further
page table creation - enable the assist.

> and making a restriction here is not usefully different from
> prior patches of yours that relax checks in Xen that still break on
> older builds.

How's this not "usefully different"? I replace potentially silent
misbehavior by the failing of a hypercall (which ought to be noticed)
plus a (debug build only) log message.

> But as I know you're going to argue with that position, I'll at least
> note that ignoring a 64bit guest's request to set that bit would be less
> bad than the current behaviour.

We already ignore this bit (as in: it has no effect). Yet just like we
can't fail the request all of the sudden, we also can't zap the bit
from the supplied mask, as kernels may legitimately check that what
they read is what they set. (That said - I'm unaware of such checking
anywhere.)

Jan

>> Dropping
>> the control-tools part of the condition is at least problematic as well,
>> as per above. Albeit I'll admit I didn't check whether nowadays vCPU 0
>> is initialized before page tables are built. But I think it's more
>> sensible the other way around: CR3 setting (in the hypervisor) is less
>> involved when the page was already validated as an L3 one.
> 
> All of this is before the guest starts running, so it doesn't matter.
> 
> The most efficient way (from Xen's point of view) is to pin the L1s,
> then L2s, then L3s and then set vCR3, because this is the only order
> where we don't have to do do recursive type acquisition.
> 
> But, the most efficient way for the toolstack to do this is the opposite
> way around, because making Xen do recursive type acquisition is faster
> than other ways, and turns all subsequent hypercalls into almost no-ops.
> 
> I doubt there is a relevant difference between these two approaches.
> 
> 
> And it doesn't matter either.  The check won't ever trip from domain
> creation (see above), nor from migration (we set vcpu context before
> pinning the pagetables, and a non pae_extended_cr3 will have exploded on
> the source side).
> 
> So there really are no toolstack codepaths that can trip the check. 
> Future improvements that might trip the check can come with a less
> broken hypercall as a prerequisite.
> 
> ~Andrew