On 06.08.2025 10:11, Jan Beulich wrote:
> On 05.08.2025 17:27, Roger Pau Monné wrote:
>> On Tue, Aug 05, 2025 at 02:38:38PM +0200, Jan Beulich wrote:
>>> On 05.08.2025 11:52, Roger Pau Monne wrote:
>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>> @@ -275,7 +275,7 @@ static void __init assign_io_page(struct page_info
>>>> *page)
>>>>
>>>> void __init arch_init_memory(void)
>>>> {
>>>> - unsigned long i, pfn, rstart_pfn, rend_pfn, iostart_pfn, ioend_pfn;
>>>> + unsigned long i, pfn, rstart_pfn, rend_pfn, iostart_pfn, ioend_pfn,
>>>> pdx;
>>>>
>>>> /*
>>>> * Basic guest-accessible flags:
>>>> @@ -328,9 +328,20 @@ void __init arch_init_memory(void)
>>>> destroy_xen_mappings((unsigned long)mfn_to_virt(iostart_pfn),
>>>> (unsigned long)mfn_to_virt(ioend_pfn));
>>>>
>>>> - /* Mark as I/O up to next RAM region. */
>>>> - for ( ; pfn < rstart_pfn; pfn++ )
>>>> + /*
>>>> + * Mark as I/O up to next RAM region. Iterate over the PDX space
>>>> to
>>>> + * skip holes which would always fail the mfn_valid() check.
>>>> + *
>>>> + * pfn_to_pdx() requires a valid (iow: RAM) PFN to convert to PDX,
>>>> + * hence provide pfn - 1, which is the tailing PFN from the last
>>>> RAM
>>>> + * range, or pdx 0 if the input pfn is 0.
>>>> + */
>>>> + for ( pdx = pfn ? pfn_to_pdx(pfn - 1) + 1 : 0;
>>>> + pdx < pfn_to_pdx(rstart_pfn);
>>>> + pdx++ )
>>>> {
>>>> + pfn = pdx_to_pfn(pdx);
>>>> +
>>>> if ( !mfn_valid(_mfn(pfn)) )
>>>> continue;
>>>>
>>>
>>> As much as I would have liked to ack this, I fear there's another caveat
>>> here:
>>> At the top of the loop we check not only for RAM, but also for UNUSABLE. The
>>> latter, like RAM, shouldn't be marked I/O, but we also can't use PFN <-> PDX
>>> transformations on any such page.
>>
>> Right you are. I'm not sure however why we do this - won't we want
>> the mappings of UNUSABLE regions also be removed from the Xen
>> page-tables? (but not marked as IO)
>
> Yes, I think this is a flaw in current code. Perhaps it was (wrongly) assumed
> that no UNUSABLE regions would ever exist this low in a memory map? Imo we
> want
> to deal with this in two steps - first sort the UNUSABLE issue, then improve
> the dealing with what is passed to assign_io_page().
>
> While there we may also want to find a way to tie together the 16Mb boundary
> checks - the 16UL isn't properly connected to the BOOTSTRAP_MAP_BASE
> definition
> in setup.c. Yet then: Am I overlooking something, or is the 16Mb boundary not
> really special anymore? I.e. could e.g. BOOTSTRAP_MAP_BASE perhaps be moved
> (at
> least in principle), either almost arbitrarily up (within the low 4Gb), or
> down
> as much as to the 2Mb boundary? The relevant aspect here would be that the
> comment saying "the statically-initialised 1-16MB mapping area" looks to be
> stale, as of 7cd7f2f5e116 ("x86/boot: Remove the preconstructed low 16M
> superpage mappings"). If there are excess mappings to worry about, those may
> nowadays well live above the 16Mb boundary (because of it being 2Mb mappings
> that head.S inserts into l2_directmap[]).
Hmm, extending this to beyond 16M collides with mappings done by
acpi_dmar_init(),
erst_init(), and acpi_hest_init(). Luckily efi_init_memory() runs only
afterwards.
While I guess I could limit this to the space 2M mappings were done for in
head.S,
theoretically there could still be a collision afterwards. So I think we need to
either somehow exclude such mappings (might end up fragile), or stop (ab)using
the directmap there. Thoughts?
Jan
