Re: [Xen-devel] Fixes for low memory allocation machinery in early boot code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.09.16 at 10:23, <daniel.kiper@xxxxxxxxxx> wrote:
> Starting from the beginning it looks that there are "soft" limits enforced
> in BIOS early boot code looking for usable low memory region. Hight limit
> is set at 640 KiB and low at 256 KiB. This means that if a value from a given
> source which describes low memory region (i.e. EBDA base segment, base memory
> size, multiboot protocol) is out of bounds then we try to get new value from
> next one (I mean source). However, at the end there are no checks that assure
> us that we got what we expected. So, I think that at first we should add 
> "hard"
> checks here. This means that if we get value out of earlier mentioned bounds
> then we should print relevant message on serial console and halt the system.

I disagree. I think that the best effort approach (what you name "soft"
checks) are quite okay in the legacy BIOS case: Even if the BIOS has
screwed things up, we can still _try_ to come up nevertheless.

This is quite different for (imo) much more sophisticated EFI
environments: We know they are screwed too, but we can't as
easily ignore them using certain pieces of memory.

> Additionally, my investigation has shown that there are no bound checks in
> low memory allocation machinery for trampoline (by the way, in BIOS path we
> allocate 64 KiB for trampoline but in EFI code we properly calculate its size;
> so, I think we should do the same calculation in BIOS path), stack and boot 
> data
> taken from multiboot protocol. Hence, relevant fixes should be added here too.

Would be nice, yes, but we need to weigh the need to presumably do
at least some of this in assembly code (for now at least) against the
potential gain.

> Moreover I think that at least allocation machinery with additional checks
> described in last paragraph can be used on EFI platforms when Xen is booted
> via multiboot2 protocol. However, then high limit should be defined as 1 MiB.
> Though I think that low limit, 256 KiB, should stay as is.

Why 1Mb? The 640k limit derives from hardware aspects, but doesn't
depend on the software environment.

> So, I think that we should prepare following patches:
>   - allocate properly calculated amount of memory for trampoline,
>   - define high/low limit as a constants and use them,
>   - add bounds checks for chosen low memory region, and bounds
>     checks in allocation machinery for trampoline and stack,
>   - add bounds checks to allocator in reloc.c.
> 
> I have a feeling that this fixes are not very critical, however, nice to have.

Indeed. I'd just like to avoid that new code (read: your mb2 series)
gets introduced with similar issues. Just like the original EFI code at
least tries to properly allocate the trampoline space.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel