| Message ID | 1472202582-32549-1-git-send-email-s.munaut@whatever-company.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26/08/16 10:09, Sylvain Munaut wrote: > If we have an multiboot value and the value we got from the BDA > seems too small, use the safe one > > Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com> > --- > I need this when using linux-as-a-bootloader (i.e. kexec into Xen) because > the BDA is just zero at that point (not entirely sure why tbh). Does Linux reclam and reuse the BDA by this point in its boot? > > This is the simplest patch I could come up with and that shouldn't change > anything for system currently booting. But if the multiboot infos are > present, I'm not sure why not use that preferentially since the values > from the BDA / EBDA could just be random garbage while the multiboot header > has at least some minimal validation (checksum + magic). If we have mutliboot memory information available (should be all of the time), we should use that in preference to peeking at the BDA/EBDA > > An error message if no sane value ( i.e. > 64k at min ) can be found at > all could be printed too. Took me some time to trace this down and some > serial output would have been welcome :) All of this code is rather hairy. Unfortunately, there is no good logging method available at this point. ~Andrew > > Comments welcome wrt to what people think is best and I can re-spin this. > > xen/arch/x86/boot/head.S | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > index 85770e8..d79fcc5 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -108,6 +108,8 @@ __start: > shl $10-4,%edx > cmp %eax,%edx /* compare with BDA value */ > cmovb %edx,%eax /* and use the smaller */ > + cmp $0x1000,%eax /* or if the BDA value is too small */ > + cmovb %edx,%eax /* (and probably not valid) */ > > 2: /* Reserve 64kb for the trampoline */ > sub $0x1000,%eax
>>> On 26.08.16 at 11:09, <s.munaut@whatever-company.com> wrote: > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -108,6 +108,8 @@ __start: > shl $10-4,%edx > cmp %eax,%edx /* compare with BDA value */ > cmovb %edx,%eax /* and use the smaller */ > + cmp $0x1000,%eax /* or if the BDA value is too small */ > + cmovb %edx,%eax /* (and probably not valid) */ Considering there is a bounds check of the EBDA values a few lines up from here (against 0x4000) I don't think I see how this code can help, assuming the given explanation is applicable. In any event is bounding by 0x1000 likely not enough, as placing the trampoline at address zero is unlikely to be a good idea. Jan
Hi, On Fri, Aug 26, 2016 at 3:06 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 26.08.16 at 11:09, <s.munaut@whatever-company.com> wrote: >> --- a/xen/arch/x86/boot/head.S >> +++ b/xen/arch/x86/boot/head.S >> @@ -108,6 +108,8 @@ __start: >> shl $10-4,%edx >> cmp %eax,%edx /* compare with BDA value */ >> cmovb %edx,%eax /* and use the smaller */ >> + cmp $0x1000,%eax /* or if the BDA value is too small */ >> + cmovb %edx,%eax /* (and probably not valid) */ > > Considering there is a bounds check of the EBDA values a few > lines up from here (against 0x4000) I don't think I see how this > code can help, assuming the given explanation is applicable. Because in my case, the EBDA value is rejected, it then falls back to reading the "base memory size" from the BDA, but that also reads as 0x00. Then because 0 is smaller than the value from multiboot, it actually takes that. Then decrements it by 0x1000 (which doesn't go well) and try to place the trampoline there ... > In any event is bounding by 0x1000 likely not enough, as placing > the trampoline at address zero is unlikely to be a good idea. I just took that value since that was the lower bound used for the multiboot value. What would be reasonable as a lower bound for validity check ? Cheers, Sylvain
>>> On 26.08.16 at 15:10, <s.munaut@whatever-company.com> wrote: > Hi, > > On Fri, Aug 26, 2016 at 3:06 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 26.08.16 at 11:09, <s.munaut@whatever-company.com> wrote: >>> --- a/xen/arch/x86/boot/head.S >>> +++ b/xen/arch/x86/boot/head.S >>> @@ -108,6 +108,8 @@ __start: >>> shl $10-4,%edx >>> cmp %eax,%edx /* compare with BDA value */ >>> cmovb %edx,%eax /* and use the smaller */ >>> + cmp $0x1000,%eax /* or if the BDA value is too small */ >>> + cmovb %edx,%eax /* (and probably not valid) */ >> >> Considering there is a bounds check of the EBDA values a few >> lines up from here (against 0x4000) I don't think I see how this >> code can help, assuming the given explanation is applicable. > > Because in my case, the EBDA value is rejected, it then falls back to > reading the "base memory size" from the BDA, but that also reads as > 0x00. Ah, but that is a basic boot environment violation. Any sane OS should be permitted to refuse to boot without any low memory. > Then because 0 is smaller than the value from multiboot, it actually > takes that. Then decrements it by 0x1000 (which doesn't go well) and > try to place the trampoline there ... And it would certainly make sense to try to handle this underflow case. >> In any event is bounding by 0x1000 likely not enough, as placing >> the trampoline at address zero is unlikely to be a good idea. > > I just took that value since that was the lower bound used for the > multiboot value. > > What would be reasonable as a lower bound for validity check ? At the very least we shouldn't overlap with the BDA (starting at 0040:0000 and iirc covering up to 256 bytes, which is why DOS never used any memory below 0050:0000). Jan
Hi, > At the very least we shouldn't overlap with the BDA (starting at > 0040:0000 and iirc covering up to 256 bytes, which is why DOS > never used any memory below 0050:0000). Mmm, I misread the assembly the low limit applied to the multi boot value was 0x4000 and not 0x1000 ... Would this logic be acceptable : high_limit = 0xa0000 low_limit = 0x40000 if (MBI_MEMLIMITS available and >= low_limit) -> use that - 64k if (low_limit <= EBDA location < high_limit) -> use that - 64k if (BDA memory size >= low_limit) -> use that - 64k if all failed: print_err("No low memory available") (in pseudo code, didn't really want to code it in assembly if it's not acceptable :p) If that looks ok, I can write and test a patch implementing that. Cheers, Sylvain Munaut
>>> On 26.08.16 at 16:21, <s.munaut@whatever-company.com> wrote: > Hi, > > >> At the very least we shouldn't overlap with the BDA (starting at >> 0040:0000 and iirc covering up to 256 bytes, which is why DOS >> never used any memory below 0050:0000). > > Mmm, I misread the assembly the low limit applied to the multi boot > value was 0x4000 and not 0x1000 ... > > Would this logic be acceptable : > > high_limit = 0xa0000 > low_limit = 0x40000 > > if (MBI_MEMLIMITS available and >= low_limit) > -> use that - 64k > > if (low_limit <= EBDA location < high_limit) > -> use that - 64k > > if (BDA memory size >= low_limit) > -> use that - 64k > > if all failed: > print_err("No low memory available") > > (in pseudo code, didn't really want to code it in assembly if it's not > acceptable :p) > > > If that looks ok, I can write and test a patch implementing that. I'm not sure. I'd like to see the current logic altered as little as possible, and what you suggest above is more than that minimum. Ideally you'd just cap things at the lower end, if they turn out too small. But then again - all bets are off anyway when all three values are unreasonably low. So another question: Can you detect whether we get booted from Linux (i.e. do they provide any kind of identification anywhere)? Jan
> I'm not sure. I'd like to see the current logic altered as little as > possible, and what you suggest above is more than that minimum. Then, that would be more like the very first patch I posted but just change the 0x1000 low limit to 0x4000. > So another question: Can you > detect whether we get booted from Linux (i.e. do they provide > any kind of identification anywhere)? The multiboot info struct has the boot loader name optional field and kexec does fill it with "kexec x.y.z". Cheers, Sylvain
>>> On 26.08.16 at 16:53, <s.munaut@whatever-company.com> wrote: >> I'm not sure. I'd like to see the current logic altered as little as >> possible, and what you suggest above is more than that minimum. > > Then, that would be more like the very first patch I posted but just > change the 0x1000 low limit to 0x4000. I thought I had made clear that I think this then ought to be 0x1050. >> So another question: Can you >> detect whether we get booted from Linux (i.e. do they provide >> any kind of identification anywhere)? > > The multiboot info struct has the boot loader name optional field and > kexec does fill it with "kexec x.y.z". Perhaps better to simply ignore the two BDA values in that special case then? Andrew, what do you think? Jan
On Fri, Aug 26, 2016 at 5:10 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 26.08.16 at 16:53, <s.munaut@whatever-company.com> wrote: >>> I'm not sure. I'd like to see the current logic altered as little as >>> possible, and what you suggest above is more than that minimum. >> >> Then, that would be more like the very first patch I posted but just >> change the 0x1000 low limit to 0x4000. > > I thought I had made clear that I think this then ought to be 0x1050. The current code validates EBDA location and the value from the MBI against 0x4000 as the lower limit (I was wrong previously when saying it was 0x1000), so I thought I wouldn't introduce yet another magic constant limit and just use the same value. But if you'd rather go with 0x1050, I'll use that. Cheers, Sylvain
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 85770e8..d79fcc5 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -108,6 +108,8 @@ __start: shl $10-4,%edx cmp %eax,%edx /* compare with BDA value */ cmovb %edx,%eax /* and use the smaller */ + cmp $0x1000,%eax /* or if the BDA value is too small */ + cmovb %edx,%eax /* (and probably not valid) */ 2: /* Reserve 64kb for the trampoline */ sub $0x1000,%eax
If we have an multiboot value and the value we got from the BDA seems too small, use the safe one Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com> --- I need this when using linux-as-a-bootloader (i.e. kexec into Xen) because the BDA is just zero at that point (not entirely sure why tbh). This is the simplest patch I could come up with and that shouldn't change anything for system currently booting. But if the multiboot infos are present, I'm not sure why not use that preferentially since the values from the BDA / EBDA could just be random garbage while the multiboot header has at least some minimal validation (checksum + magic). An error message if no sane value ( i.e. > 64k at min ) can be found at all could be printed too. Took me some time to trace this down and some serial output would have been welcome :) Comments welcome wrt to what people think is best and I can re-spin this. xen/arch/x86/boot/head.S | 2 ++ 1 file changed, 2 insertions(+)