Re: [PATCH 05/22] x86/srat: vmap the pages for acpi_slit

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Hongyan Xia" <hongyxia@amazon.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	xen-devel@lists.xenproject.org,
	"Julien Grall" <jgrall@amazon.com>
Subject: Re: [PATCH 05/22] x86/srat: vmap the pages for acpi_slit
Date: Fri, 23 Dec 2022 11:31:05 +0000	[thread overview]
Message-ID: <965e3faa-472d-9a79-83ca-fef57cda81c5@xen.org> (raw)
In-Reply-To: <ca02a313-0fa2-8041-3e8f-d467c3e99fb6@suse.com>

Hi Jan,

On 20/12/2022 15:30, Jan Beulich wrote:
> On 16.12.2022 12:48, Julien Grall wrote:
>> From: Hongyan Xia <hongyxia@amazon.com>
>>
>> This avoids the assumption that boot pages are in the direct map.
>>
>> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> However, ...
> 
>> --- a/xen/arch/x86/srat.c
>> +++ b/xen/arch/x86/srat.c
>> @@ -139,7 +139,8 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
>>   		return;
>>   	}
>>   	mfn = alloc_boot_pages(PFN_UP(slit->header.length), 1);
>> -	acpi_slit = mfn_to_virt(mfn_x(mfn));
>> +	acpi_slit = vmap_contig_pages(mfn, PFN_UP(slit->header.length));
> 
> ... with the increased use of vmap space the VA range used will need
> growing. And that's perhaps better done ahead of time than late.

I will have a look to increase the vmap().

> 
>> +	BUG_ON(!acpi_slit);
> 
> Similarly relevant for the earlier patch: It would be nice if boot
> failure for optional things like NUMA data could be avoided.

If you can't map (or allocate the memory), then you are probably in a 
very bad situation because both should really not fail at boot.

So I think this is correct to crash early because the admin will be able 
to look what went wrong. Otherwise, it may be missed in the noise.

>  But I
> understand this is somewhat orthogonal to this series (the more that
> alloc_boot_pages() itself is also affected). Yet not entirely so,
> since previously there was no mapping failure possible here.

See above. I don't see the problem of adding a potential mapping failure 
here and before.

Cheers,

-- 
Julien Grall