Re: [Xen-devel] [PATCH] x86/pv: Fix construction of 32bit dom0's

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/02/2019 13:56, Juergen Gross wrote:
> On 07/02/2019 14:45, Jan Beulich wrote:
>>
>>>>> @@ -606,23 +598,14 @@ int __init dom0_construct_pv(struct domain *d,
>>>>>      {
>>>>>          maddr_to_page(mpt_alloc)->u.inuse.type_info = PGT_l4_page_table;
>>>>>          l4start = l4tab = __va(mpt_alloc); mpt_alloc += PAGE_SIZE;
>>>>> +        clear_page(l4tab);
>>>>> +        init_xen_l4_slots(l4tab, _mfn(virt_to_mfn(l4start)),
>>>>> +                          d, INVALID_MFN, true);
>>>>> +        v->arch.guest_table = pagetable_from_paddr(__pa(l4start));
>>>>>      }
>>>>>      else
>>>>> -    {
>>>>> -        page = alloc_domheap_page(d, MEMF_no_owner | MEMF_no_scrub);
>>>>> -        if ( !page )
>>>>> -            panic("Not enough RAM for domain 0 PML4\n");
>>>>> -        page->u.inuse.type_info = PGT_l4_page_table|PGT_validated|1;
>>>>> -        l4start = l4tab = page_to_virt(page);
>>>>> -        maddr_to_page(mpt_alloc)->u.inuse.type_info = PGT_l3_page_table;
>>>>> -        l3start = __va(mpt_alloc); mpt_alloc += PAGE_SIZE;
>>>> This one is lost without replacement, but is needed. Commit
>>>> 7a9d764630 ("x86/32-on-64: adjust Dom0 initial page table layout")
>>>> specifically introduced it to make sure the guest-perceived top level
>>>> page table is allocated first (and hence marks the beginning of the
>>>> boot page tables, so Dom0 can later put all of them into general use).
>>> I did call this out specifically in the commit message.  I had no idea
>>> about that commit when editing the code, but I still don't understand
>>> why it is important that the guests top level needs to be first.
>> The start info field "pt_base" is specified to point at the root table.
>> If the root table isn't first, it's harder for the kernel to know where
>> the counting of "nr_pt_frames" actually starts (see Linux'es
>> xen_find_pt_base(), which tells me that nowadays they do that
>> extra scanning, but iirc this hadn't been there from the beginning).
> Before I introduced xen_find_pt_base() 32-bit pv domains just assumed
> there could be 2 page tables located before PGD.
>
> There is an exhaustive comment in Xen's include/public/xen.h in this
> regard.

Oh - so there is.  That is rather more helpful at explaining what is
going on, but there really needs to be a comment in the code.

>
>> Furthermore your change even violates the specification, as
>> "pt_base" no longer points at the root table; you'd have to undo
> This is of course a major problem.
>
> pt_base is similar to "where cr3 is supposed to point at".

Obviously this is a bug needing fixing.

~Andrew

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