Re: [Xen-devel] [PATCH 4/6] x86/boot: Clean up l?_bootmap[] construction

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2020 19:00, Andrew Cooper wrote:
> On 07/01/2020 16:16, Jan Beulich wrote:
>> On 06.01.2020 16:54, Andrew Cooper wrote:
>>>      for ( i = 0; i < 8; ++i )
>>>      {
>>>          unsigned int slot = (xen_phys_start >> L2_PAGETABLE_SHIFT) + i;
>>>          paddr_t addr = slot << L2_PAGETABLE_SHIFT;
>>>  
>>>          l2_identmap[slot] = l2e_from_paddr(addr, 
>>> PAGE_HYPERVISOR|_PAGE_PSE);
>>> -        slot &= L2_PAGETABLE_ENTRIES - 1;
>>>          l2_bootmap[slot] = l2e_from_paddr(addr, 
>>> __PAGE_HYPERVISOR|_PAGE_PSE);
>>>      }
>>> -    /* Initialise L3 boot-map page directory entries. */
>>> -    l3_bootmap[l3_table_offset(xen_phys_start)] =
>>> -        l3e_from_paddr((UINTN)l2_bootmap, __PAGE_HYPERVISOR);
>>> -    l3_bootmap[l3_table_offset(xen_phys_start + (8 << L2_PAGETABLE_SHIFT) 
>>> - 1)] =
>>> -        l3e_from_paddr((UINTN)l2_bootmap, __PAGE_HYPERVISOR);
>>> +
>>> +    /* Initialize L3 boot-map page directory entries. */
>>> +    for ( i = 0; i < 4; ++i )
>>> +        l3_bootmap[i] = l3e_from_paddr((UINTN)l2_bootmap + i * PAGE_SIZE,
>>> +                                       __PAGE_HYPERVISOR);
>> The idea behind the original code was to be immune to the number
>> of pages l2_bootmap[] covers, as long as it's at least one (which
>> it'll always be, I would say). The minimum requirement to any
>> change to this I have is that the build must break if the size
>> assumption here is violated. I.e. there may not be a literal 4 as
>> the upper loop bound here, or there would need to be a
>> BUILD_BUG_ON() right next to it. But I'd really prefer if the
>> code was left as is (perhaps with a comment added), unless you
>> can point out actual issues with it (which I can't see in the
>> description), or you can otherwise justify the change with better
>> than "the EFI side is further complicated by spraying non-identity
>> aliases into the mix."
> 
> Given that what you describe here is totally undocumented, and AFAICT,
> totally undescribed even in commit messages, it has cost me probably a
> weeks worth of time to reverse to the point at which I was confident
> that I knew all of what it was attempting to do.

This is not meant as an excuse (I really should have done better back
then), but you could have asked.

> The purpose of this was to make the handling of l?_bootmap[] as
> consistent as possible between the various environments.  The pagetables
> themselves are common, and should be used consistently.

I don't think I can wholeheartedly agree here: l?_bootmap[] are
throw-away page tables (living in .init), and with the non-EFI and
EFI boot paths being so different anyway, them using the available
tables differently is not a big issue imo. This heavy difference of
other aspects was also why back then I decided to be as defensive
towards l2_bootmap[] size changes as possible in code which doesn't
really need it to be multiple pages.

As said - I'm going to try to not stand in the way of you re-
arranging this, but
- the new code should not break silently when (in particular)
  l2_bootmap[] changes
- the description should be more explicit about the motivation of
  the change (which includes distinguishing between intentional
  mappings and ones simply appearing as a side effect, without
  getting in the way)

Jan

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