* [Xen-devel] [PATCH v2 1/6] x86/boot: Remove gratuitous call back into low-memory code
[not found] <cover.1565362089.git.dwmw@amazon.co.uk>
@ 2019-08-09 15:01 ` David Woodhouse
2019-08-09 15:01 ` [Xen-devel] [PATCH v2 2/6] x86/boot: Only jump into low trampoline code for real-mode boot David Woodhouse
` (4 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: David Woodhouse @ 2019-08-09 15:01 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 3590 bytes --]
From: David Woodhouse <dwmw@amazon.co.uk>
We appear to have implemented a memcpy() in the low-memory trampoline
which we then call into from __start_xen(), for no adequately defined
reason.
Kill it with fire.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: Minor fixups from Andrew.
xen/arch/x86/boot/mem.S | 27 +++++----------------------
xen/arch/x86/setup.c | 10 ++++++++++
xen/include/asm-x86/e820.h | 5 ++---
3 files changed, 17 insertions(+), 25 deletions(-)
diff --git a/xen/arch/x86/boot/mem.S b/xen/arch/x86/boot/mem.S
index c6a9bd4d3b..2d61d28835 100644
--- a/xen/arch/x86/boot/mem.S
+++ b/xen/arch/x86/boot/mem.S
@@ -7,7 +7,7 @@ get_memory_map:
.Lmeme820:
xorl %ebx, %ebx # continuation counter
- movw $bootsym(e820map), %di # point into the whitelist
+ movw $bootsym(bios_e820map), %di # point into the whitelist
# so we can have the bios
# directly write into it.
@@ -22,8 +22,8 @@ get_memory_map:
cmpl $SMAP,%eax # check the return is `SMAP'
jne .Lmem88
- incw bootsym(e820nr)
- cmpw $E820_BIOS_MAX,bootsym(e820nr) # up to this many entries
+ incw bootsym(bios_e820nr)
+ cmpw $E820_BIOS_MAX,bootsym(bios_e820nr) # up to this many entries
jae .Lmem88
movw %di,%ax
@@ -66,27 +66,10 @@ get_memory_map:
ret
-/*
- * Copy E820 map obtained from BIOS to a buffer allocated by Xen.
- * Input: %rdi: target address of e820 entry array
- * %esi: maximum number of entries to copy
- * Output: %eax: number of entries copied
- */
- .code64
-ENTRY(e820map_copy)
- mov %esi, %eax
- lea e820map(%rip), %rsi
- mov e820nr(%rip), %ecx
- cmp %ecx, %eax
- cmova %ecx, %eax # number of entries to move
- imul $5, %eax, %ecx
- rep movsl # do the move
- ret
-
.align 4
-e820map:
+GLOBAL(bios_e820map)
.fill E820_BIOS_MAX*20,1,0
-e820nr:
+GLOBAL(bios_e820nr)
.long 0
GLOBAL(lowmem_kb)
.long 0
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d2011910fa..decea2e77a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -672,6 +672,16 @@ static char * __init cmdline_cook(char *p, const char *loader_name)
return p;
}
+static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int limit)
+{
+ unsigned int n = min(bootsym(bios_e820nr), limit);
+
+ if ( n )
+ memcpy(map, bootsym(bios_e820map), sizeof(*map) * n);
+
+ return n;
+}
+
void __init noreturn __start_xen(unsigned long mbi_p)
{
char *memmap_type = NULL;
diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
index ee317b17aa..52916fb75d 100644
--- a/xen/include/asm-x86/e820.h
+++ b/xen/include/asm-x86/e820.h
@@ -37,8 +37,7 @@ extern struct e820map e820_raw;
/* These symbols live in the boot trampoline. */
extern unsigned int lowmem_kb, highmem_kb;
-unsigned int e820map_copy(struct e820entry *map, unsigned int limit);
-
-#define copy_bios_e820 bootsym(e820map_copy)
+extern struct e820map bios_e820map[];
+extern unsigned int bios_e820nr;
#endif /*__E820_HEADER*/
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread* [Xen-devel] [PATCH v2 2/6] x86/boot: Only jump into low trampoline code for real-mode boot
[not found] <cover.1565362089.git.dwmw@amazon.co.uk>
2019-08-09 15:01 ` [Xen-devel] [PATCH v2 1/6] x86/boot: Remove gratuitous call back into low-memory code David Woodhouse
@ 2019-08-09 15:01 ` David Woodhouse
2019-08-12 9:10 ` Jan Beulich
2019-08-09 15:01 ` [Xen-devel] [PATCH v2 3/6] x86/boot: Split bootsym() into four types of relocations David Woodhouse
` (3 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: David Woodhouse @ 2019-08-09 15:01 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 2027 bytes --]
From: David Woodhouse <dwmw@amazon.co.uk>
If the no-real-mode flag is set, don't go there at all. This is a prelude
to not even putting it there in the first place.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
xen/arch/x86/boot/head.S | 10 ++++++++++
xen/arch/x86/boot/trampoline.S | 4 ----
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index d78bed394a..e3b42e3263 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -735,7 +735,17 @@ trampoline_setup:
/* Switch to low-memory stack which lives at the end of trampoline region. */
mov sym_fs(trampoline_phys),%edi
lea TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
+ cmpb $0, sym_fs(skip_realmode)
+ jz 1f
+ /* If no-real-mode, jump straight to trampoline_protmode_entry */
+ lea trampoline_protmode_entry-trampoline_start(%edi),%eax
+ /* EBX == 0 indicates we are the BP (Boot Processor). */
+ xor %ebx,%ebx
+ jmp 2f
+1:
+ /* Go via 16-bit code in trampoline_boot_cpu_entry */
lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
+2:
pushl $BOOT_CS32
push %eax
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 7c6a2328d2..429a088b19 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -194,9 +194,6 @@ gdt_48: .word 6*8-1
.code32
trampoline_boot_cpu_entry:
- cmpb $0,bootsym_rel(skip_realmode,5)
- jnz .Lskip_realmode
-
/* Load pseudo-real-mode segments. */
mov $BOOT_PSEUDORM_DS,%eax
mov %eax,%ds
@@ -276,7 +273,6 @@ trampoline_boot_cpu_entry:
mov %eax,%gs
mov %eax,%ss
-.Lskip_realmode:
/* EBX == 0 indicates we are the BP (Boot Processor). */
xor %ebx,%ebx
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 2/6] x86/boot: Only jump into low trampoline code for real-mode boot
2019-08-09 15:01 ` [Xen-devel] [PATCH v2 2/6] x86/boot: Only jump into low trampoline code for real-mode boot David Woodhouse
@ 2019-08-12 9:10 ` Jan Beulich
2019-08-21 14:04 ` David Woodhouse
0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-08-12 9:10 UTC (permalink / raw)
To: David Woodhouse; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
On 09.08.2019 17:01, David Woodhouse wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -735,7 +735,17 @@ trampoline_setup:
> /* Switch to low-memory stack which lives at the end of trampoline region. */
> mov sym_fs(trampoline_phys),%edi
> lea TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
> + cmpb $0, sym_fs(skip_realmode)
> + jz 1f
> + /* If no-real-mode, jump straight to trampoline_protmode_entry */
> + lea trampoline_protmode_entry-trampoline_start(%edi),%eax
> + /* EBX == 0 indicates we are the BP (Boot Processor). */
> + xor %ebx,%ebx
> + jmp 2f
> +1:
> + /* Go via 16-bit code in trampoline_boot_cpu_entry */
> lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
> +2:
> pushl $BOOT_CS32
> push %eax
May I suggest to slightly streamline this into
/* Switch to low-memory stack which lives at the end of trampoline region. */
mov sym_fs(trampoline_phys),%edi
lea TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
/* Go via 16-bit code in trampoline_boot_cpu_entry */
lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
cmpb $0,sym_fs(skip_realmode)
je 1f
/* If no-real-mode, jump straight to trampoline_protmode_entry */
lea trampoline_protmode_entry-trampoline_start(%edi),%eax
/* EBX == 0 indicates we are the BP (Boot Processor). */
xor %ebx,%ebx
1:
pushl $BOOT_CS32
push %eax
perhaps with the second slightly adapted to it now being an override
rather than an alternative path?
Additionally I think it would be nice if the clearing of %ebx wasn't
replicated here and ...
> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -194,9 +194,6 @@ gdt_48: .word 6*8-1
>
> .code32
> trampoline_boot_cpu_entry:
> - cmpb $0,bootsym_rel(skip_realmode,5)
> - jnz .Lskip_realmode
> -
> /* Load pseudo-real-mode segments. */
> mov $BOOT_PSEUDORM_DS,%eax
> mov %eax,%ds
> @@ -276,7 +273,6 @@ trampoline_boot_cpu_entry:
> mov %eax,%gs
> mov %eax,%ss
>
> -.Lskip_realmode:
> /* EBX == 0 indicates we are the BP (Boot Processor). */
> xor %ebx,%ebx
... here. Why don't you further do
.code32
trampoline_protmode_entry_bsp:
/* EBX == 0 indicates we are the BSP (Boot Strap Processor). */
xor %ebx,%ebx
trampoline_protmode_entry:
directing the BSP paths to the new label?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 2/6] x86/boot: Only jump into low trampoline code for real-mode boot
2019-08-12 9:10 ` Jan Beulich
@ 2019-08-21 14:04 ` David Woodhouse
2019-08-27 8:43 ` Jan Beulich
0 siblings, 1 reply; 23+ messages in thread
From: David Woodhouse @ 2019-08-21 14:04 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
[-- Attachment #1.1: Type: text/plain, Size: 3783 bytes --]
On Mon, 2019-08-12 at 11:10 +0200, Jan Beulich wrote:
> On 09.08.2019 17:01, David Woodhouse wrote:
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -735,7 +735,17 @@ trampoline_setup:
> > /* Switch to low-memory stack which lives at the end of trampoline region. */
> > mov sym_fs(trampoline_phys),%edi
> > lea TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
> > + cmpb $0, sym_fs(skip_realmode)
> > + jz 1f
> > + /* If no-real-mode, jump straight to trampoline_protmode_entry */
> > + lea trampoline_protmode_entry-trampoline_start(%edi),%eax
> > + /* EBX == 0 indicates we are the BP (Boot Processor). */
> > + xor %ebx,%ebx
> > + jmp 2f
> > +1:
> > + /* Go via 16-bit code in trampoline_boot_cpu_entry */
> > lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
> > +2:
> > pushl $BOOT_CS32
> > push %eax
>
> May I suggest to slightly streamline this into
>
> /* Switch to low-memory stack which lives at the end of trampoline region. */
> mov sym_fs(trampoline_phys),%edi
> lea TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
> /* Go via 16-bit code in trampoline_boot_cpu_entry */
> lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
> cmpb $0,sym_fs(skip_realmode)
> je 1f
> /* If no-real-mode, jump straight to trampoline_protmode_entry */
> lea trampoline_protmode_entry-trampoline_start(%edi),%eax
> /* EBX == 0 indicates we are the BP (Boot Processor). */
> xor %ebx,%ebx
> 1:
> pushl $BOOT_CS32
> push %eax
>
> perhaps with the second slightly adapted to it now being an override
> rather than an alternative path?
It's a *temporary* alternative path, and it's gone away by the end of
the series. Obviously we do insist on each interim commit being
buildable and working. I kind of draw the line at optimising the asm
for each intermediate step :)
> Additionally I think it would be nice if the clearing of %ebx wasn't
> replicated here and ...
>
> > --- a/xen/arch/x86/boot/trampoline.S
> > +++ b/xen/arch/x86/boot/trampoline.S
> > @@ -194,9 +194,6 @@ gdt_48: .word 6*8-1
> >
> > .code32
> > trampoline_boot_cpu_entry:
> > - cmpb $0,bootsym_rel(skip_realmode,5)
> > - jnz .Lskip_realmode
> > -
> > /* Load pseudo-real-mode segments. */
> > mov $BOOT_PSEUDORM_DS,%eax
> > mov %eax,%ds
> > @@ -276,7 +273,6 @@ trampoline_boot_cpu_entry:
> > mov %eax,%gs
> > mov %eax,%ss
> >
> > -.Lskip_realmode:
> > /* EBX == 0 indicates we are the BP (Boot Processor). */
> > xor %ebx,%ebx
>
> ... here. Why don't you further do
>
> .code32
> trampoline_protmode_entry_bsp:
> /* EBX == 0 indicates we are the BSP (Boot Strap Processor).
> */
> xor %ebx,%ebx
> trampoline_protmode_entry:
>
> directing the BSP paths to the new label?
Yeah, I kind of see your point. But that gives us one entry point which
clears %ebx... and one which doesn't, so you still have to make sure
it's not already zero for the AP startup.
If we ended up with two simple entry points that didn't care about %ebx
for trampoline_protmode_entry_bsp and trampoline_protmode_entry_ap then
that'd be nice and simple — but I don't like the inconsistency.
I think I prefer having to set %ebx explicitly in all three separate
callers, over having one entry point that requires it and another that
doesn't.
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v2 2/6] x86/boot: Only jump into low trampoline code for real-mode boot
2019-08-21 14:04 ` David Woodhouse
@ 2019-08-27 8:43 ` Jan Beulich
0 siblings, 0 replies; 23+ messages in thread
From: Jan Beulich @ 2019-08-27 8:43 UTC (permalink / raw)
To: David Woodhouse; +Cc: Andrew Cooper, xen-devel, Wei Liu, Roger Pau Monné
On 21.08.2019 16:04, David Woodhouse wrote:
> On Mon, 2019-08-12 at 11:10 +0200, Jan Beulich wrote:
>> On 09.08.2019 17:01, David Woodhouse wrote:
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -735,7 +735,17 @@ trampoline_setup:
>>> /* Switch to low-memory stack which lives at the end of trampoline region. */
>>> mov sym_fs(trampoline_phys),%edi
>>> lea TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
>>> + cmpb $0, sym_fs(skip_realmode)
>>> + jz 1f
>>> + /* If no-real-mode, jump straight to trampoline_protmode_entry */
>>> + lea trampoline_protmode_entry-trampoline_start(%edi),%eax
>>> + /* EBX == 0 indicates we are the BP (Boot Processor). */
>>> + xor %ebx,%ebx
>>> + jmp 2f
>>> +1:
>>> + /* Go via 16-bit code in trampoline_boot_cpu_entry */
>>> lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
>>> +2:
>>> pushl $BOOT_CS32
>>> push %eax
>>
>> May I suggest to slightly streamline this into
>>
>> /* Switch to low-memory stack which lives at the end of trampoline region. */
>> mov sym_fs(trampoline_phys),%edi
>> lea TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
>> /* Go via 16-bit code in trampoline_boot_cpu_entry */
>> lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
>> cmpb $0,sym_fs(skip_realmode)
>> je 1f
>> /* If no-real-mode, jump straight to trampoline_protmode_entry */
>> lea trampoline_protmode_entry-trampoline_start(%edi),%eax
>> /* EBX == 0 indicates we are the BP (Boot Processor). */
>> xor %ebx,%ebx
>> 1:
>> pushl $BOOT_CS32
>> push %eax
>>
>> perhaps with the second slightly adapted to it now being an override
>> rather than an alternative path?
>
> It's a *temporary* alternative path, and it's gone away by the end of
> the series.
Indeed I did notice it's temporary when making it to the later patches
of the series. If all parts go in at about the same time, I think I'm
okay with leaving the code as is.
>> Additionally I think it would be nice if the clearing of %ebx wasn't
>> replicated here and ...
>>
>>> --- a/xen/arch/x86/boot/trampoline.S
>>> +++ b/xen/arch/x86/boot/trampoline.S
>>> @@ -194,9 +194,6 @@ gdt_48: .word 6*8-1
>>>
>>> .code32
>>> trampoline_boot_cpu_entry:
>>> - cmpb $0,bootsym_rel(skip_realmode,5)
>>> - jnz .Lskip_realmode
>>> -
>>> /* Load pseudo-real-mode segments. */
>>> mov $BOOT_PSEUDORM_DS,%eax
>>> mov %eax,%ds
>>> @@ -276,7 +273,6 @@ trampoline_boot_cpu_entry:
>>> mov %eax,%gs
>>> mov %eax,%ss
>>>
>>> -.Lskip_realmode:
>>> /* EBX == 0 indicates we are the BP (Boot Processor). */
>>> xor %ebx,%ebx
>>
>> ... here. Why don't you further do
>>
>> .code32
>> trampoline_protmode_entry_bsp:
>> /* EBX == 0 indicates we are the BSP (Boot Strap Processor).
>> */
>> xor %ebx,%ebx
>> trampoline_protmode_entry:
>>
>> directing the BSP paths to the new label?
>
> Yeah, I kind of see your point. But that gives us one entry point which
> clears %ebx... and one which doesn't, so you still have to make sure
> it's not already zero for the AP startup.
>
> If we ended up with two simple entry points that didn't care about %ebx
> for trampoline_protmode_entry_bsp and trampoline_protmode_entry_ap then
> that'd be nice and simple — but I don't like the inconsistency.
>
> I think I prefer having to set %ebx explicitly in all three separate
> callers, over having one entry point that requires it and another that
> doesn't.
I think differently, but I'm not going to insist if Andrew agrees with
your preference.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Xen-devel] [PATCH v2 3/6] x86/boot: Split bootsym() into four types of relocations
[not found] <cover.1565362089.git.dwmw@amazon.co.uk>
2019-08-09 15:01 ` [Xen-devel] [PATCH v2 1/6] x86/boot: Remove gratuitous call back into low-memory code David Woodhouse
2019-08-09 15:01 ` [Xen-devel] [PATCH v2 2/6] x86/boot: Only jump into low trampoline code for real-mode boot David Woodhouse
@ 2019-08-09 15:01 ` David Woodhouse
2019-08-12 9:41 ` Jan Beulich
2019-08-09 15:02 ` [Xen-devel] [PATCH v2 4/6] x86/boot: Rename trampoline_{start, end} to boot_trampoline_{start, end} David Woodhouse
` (2 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: David Woodhouse @ 2019-08-09 15:01 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 19906 bytes --]
From: David Woodhouse <dwmw@amazon.co.uk>
As a first step toward using the low-memory trampoline only when necessary
for a legacy boot without no-real-mode, clean up the relocations into
three separate groups.
• bootsym() is now used only at boot time when no-real-mode isn't set.
• bootdatasym() is for variables containing information discovered by
the 16-bit boot code. This is currently accessed directly in place
in low memory by Xen at runtime, but will be copied back to its
location in high memory to avoid the pointer gymnastics (and because
a subsequent patch will stop copying the 16-bit boot code into low
memory at all when it isn't being used).
• trampsym() is for the permanent 16-bit trampoline used for AP startup
and for wake from sleep. This is not used at boot, and can be copied
into (properly allocated) low memory once the system is running.
• tramp32sym() is used both at boot and for AP startup/wakeup. During
boot it can be used in-place, running from the physical address of
the Xen image. For AP startup it can't, because at that point there
isn't a full 1:1 mapping of all memory; only the low trampoline page
is mapped.
No (intentional) functional change yet; just a "cleanup" to allow the
various parts to be treated separately in subsequent patches.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
xen/arch/x86/boot/edd.S | 16 +++----
xen/arch/x86/boot/head.S | 22 +++++++--
xen/arch/x86/boot/mem.S | 12 ++---
xen/arch/x86/boot/trampoline.S | 85 ++++++++++++++++++++++++++--------
xen/arch/x86/boot/video.S | 6 +--
xen/arch/x86/boot/wakeup.S | 12 ++---
xen/arch/x86/efi/efi-boot.h | 8 ++--
xen/arch/x86/xen.lds.S | 15 ++++--
8 files changed, 122 insertions(+), 54 deletions(-)
diff --git a/xen/arch/x86/boot/edd.S b/xen/arch/x86/boot/edd.S
index 3df712bce1..434bbbd960 100644
--- a/xen/arch/x86/boot/edd.S
+++ b/xen/arch/x86/boot/edd.S
@@ -41,7 +41,7 @@ get_edd:
# This code is sensitive to the size of the structs in edd.h
edd_start:
/* ds:si points at fn48 results. Fn41 results go immediately before. */
- movw $bootsym(boot_edd_info)+EDDEXTSIZE, %si
+ movw $bootdatasym(boot_edd_info)+EDDEXTSIZE, %si
movb $0x80, %dl # BIOS device 0x80
edd_check_ext:
@@ -56,7 +56,7 @@ edd_check_ext:
movb %dl, %ds:-8(%si) # store device number
movb %ah, %ds:-7(%si) # store version
movw %cx, %ds:-6(%si) # store extensions
- incb bootsym(boot_edd_info_nr) # note that we stored something
+ incb bootdatasym(boot_edd_info_nr) # note that we stored something
edd_get_device_params:
movw $EDDPARMSIZE, %ds:(%si) # put size
@@ -97,7 +97,7 @@ edd_legacy_done:
edd_next:
incb %dl # increment to next device
jz edd_done
- cmpb $EDD_INFO_MAX,bootsym(boot_edd_info_nr)
+ cmpb $EDD_INFO_MAX,bootdatasym(boot_edd_info_nr)
jb edd_check_ext
edd_done:
@@ -108,11 +108,11 @@ edd_done:
.Ledd_mbr_sig_start:
pushw %es
movb $0x80, %dl # from device 80
- movw $bootsym(boot_mbr_signature), %bx # store buffer ptr in bx
+ movw $bootdatasym(boot_mbr_signature), %bx # store buffer ptr in bx
.Ledd_mbr_sig_read:
pushw %bx
- movw $bootsym(boot_edd_info), %bx
- movzbw bootsym(boot_edd_info_nr), %cx
+ movw $bootdatasym(boot_edd_info), %bx
+ movzbw bootdatasym(boot_edd_info_nr), %cx
jcxz .Ledd_mbr_sig_default
.Ledd_mbr_sig_find_info:
cmpb %dl, (%bx)
@@ -151,12 +151,12 @@ edd_done:
jne .Ledd_mbr_sig_next
movb %dl, (%bx) # store BIOS drive number
movl %ecx, 4(%bx) # store signature from MBR
- incb bootsym(boot_mbr_signature_nr) # note that we stored something
+ incb bootdatasym(boot_mbr_signature_nr) # note that we stored something
addw $8, %bx # increment sig buffer ptr
.Ledd_mbr_sig_next:
incb %dl # increment to next device
jz .Ledd_mbr_sig_done
- cmpb $EDD_MBR_SIG_MAX, bootsym(boot_mbr_signature_nr)
+ cmpb $EDD_MBR_SIG_MAX, bootdatasym(boot_mbr_signature_nr)
jb .Ledd_mbr_sig_read
.Ledd_mbr_sig_done:
popw %es
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index e3b42e3263..07621d1a30 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -707,14 +707,30 @@ trampoline_setup:
cmp $sym_offs(__trampoline_rel_stop),%edi
jb 1b
- /* Patch in the trampoline segment. */
+ mov $sym_offs(__trampoline32_rel_start),%edi
+1:
+ mov %fs:(%edi),%eax
+ add %edx,%fs:(%edi,%eax)
+ add $4,%edi
+ cmp $sym_offs(__trampoline32_rel_stop),%edi
+ jb 1b
+
+ mov $sym_offs(__bootsym_rel_start),%edi
+1:
+ mov %fs:(%edi),%eax
+ add %edx,%fs:(%edi,%eax)
+ add $4,%edi
+ cmp $sym_offs(__bootsym_rel_stop),%edi
+ jb 1b
+
+ /* Patch in the boot trampoline segment. */
shr $4,%edx
- mov $sym_offs(__trampoline_seg_start),%edi
+ mov $sym_offs(__bootsym_seg_start),%edi
1:
mov %fs:(%edi),%eax
mov %dx,%fs:(%edi,%eax)
add $4,%edi
- cmp $sym_offs(__trampoline_seg_stop),%edi
+ cmp $sym_offs(__bootsym_seg_stop),%edi
jb 1b
/* Do not parse command line on EFI platform here. */
diff --git a/xen/arch/x86/boot/mem.S b/xen/arch/x86/boot/mem.S
index 2d61d28835..aa39608442 100644
--- a/xen/arch/x86/boot/mem.S
+++ b/xen/arch/x86/boot/mem.S
@@ -7,7 +7,7 @@ get_memory_map:
.Lmeme820:
xorl %ebx, %ebx # continuation counter
- movw $bootsym(bios_e820map), %di # point into the whitelist
+ movw $bootdatasym(bios_e820map), %di # point into the whitelist
# so we can have the bios
# directly write into it.
@@ -22,8 +22,8 @@ get_memory_map:
cmpl $SMAP,%eax # check the return is `SMAP'
jne .Lmem88
- incw bootsym(bios_e820nr)
- cmpw $E820_BIOS_MAX,bootsym(bios_e820nr) # up to this many entries
+ incw bootdatasym(bios_e820nr)
+ cmpw $E820_BIOS_MAX,bootdatasym(bios_e820nr) # up to this many entries
jae .Lmem88
movw %di,%ax
@@ -35,7 +35,7 @@ get_memory_map:
.Lmem88:
movb $0x88, %ah
int $0x15
- movw %ax,bootsym(highmem_kb)
+ movw %ax,bootdatasym(highmem_kb)
.Lmeme801:
stc # fix to work around buggy
@@ -58,11 +58,11 @@ get_memory_map:
shll $6,%edx # and go from 64k to 1k chunks
movzwl %cx, %ecx
addl %ecx, %edx # add in lower memory
- movl %edx,bootsym(highmem_kb) # store extended memory size
+ movl %edx,bootdatasym(highmem_kb) # store extended memory size
.Lint12:
int $0x12
- movw %ax,bootsym(lowmem_kb)
+ movw %ax,bootdatasym(lowmem_kb)
ret
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 429a088b19..95a4bef553 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -16,21 +16,62 @@
* not guaranteed to persist.
*/
-/* NB. bootsym() is only usable in real mode, or via BOOT_PSEUDORM_DS. */
+/*
+ * There are four sets of relocations:
+ *
+ * bootsym(): Boot-time code relocated to low memory and run only once.
+ * Only usable at boot, in real mode or via BOOT_PSEUDORM_DS.
+ * bootdatasym(): Boot-time BIOS-discovered data, relocated back up to Xen
+ * image after discovery.
+ * trampsym(): Trampoline code relocated into low memory for AP startup
+ * and wakeup.
+ * tramp32sym(): 32-bit trampoline code which at boot can be used directly
+ * from the Xen image in memory, but which will need to be
+ * relocated into low (well, into *mapped*) memory in order
+ * to be used for AP startup.
+ */
#undef bootsym
#define bootsym(s) ((s)-trampoline_start)
#define bootsym_rel(sym, off, opnd...) \
bootsym(sym),##opnd; \
111:; \
- .pushsection .trampoline_rel, "a"; \
+ .pushsection .bootsym_rel, "a"; \
.long 111b - (off) - .; \
.popsection
#define bootsym_segrel(sym, off) \
$0,$bootsym(sym); \
111:; \
- .pushsection .trampoline_seg, "a"; \
+ .pushsection .bootsym_seg, "a"; \
+ .long 111b - (off) - .; \
+ .popsection
+
+#define bootdatasym(s) ((s)-trampoline_start)
+#define bootdatasym_rel(sym, off, opnd...) \
+ bootdatasym(sym),##opnd; \
+111:; \
+ .pushsection .bootdatasym_rel, "a";\
+ .long 111b - (off) - .; \
+ .popsection
+
+#undef trampsym
+#define trampsym(s) ((s)-trampoline_start)
+
+#define trampsym_rel(sym, off, opnd...) \
+ trampsym(sym),##opnd; \
+111:; \
+ .pushsection .trampsym_rel, "a"; \
+ .long 111b - (off) - .; \
+ .popsection
+
+#undef tramp32sym
+#define tramp32sym(s) ((s)-trampoline_start)
+
+#define tramp32sym_rel(sym, off, opnd...) \
+ tramp32sym(sym),##opnd; \
+111:; \
+ .pushsection .tramp32sym_rel, "a"; \
.long 111b - (off) - .; \
.popsection
@@ -48,16 +89,19 @@
GLOBAL(trampoline_realmode_entry)
mov %cs,%ax
mov %ax,%ds
- movb $0xA5,bootsym(trampoline_cpu_started)
+ movb $0xA5,trampsym(trampoline_cpu_started)
cld
cli
- lidt bootsym(idt_48)
- lgdt bootsym(gdt_48)
+ lidt trampsym(idt_48)
+ lgdt trampsym(gdt_48)
mov $1,%bl # EBX != 0 indicates we are an AP
xor %ax, %ax
inc %ax
lmsw %ax # CR0.PE = 1 (enter protected mode)
- ljmpl $BOOT_CS32,$bootsym_rel(trampoline_protmode_entry,6)
+ ljmpl $BOOT_CS32,$tramp32sym_rel(trampoline_protmode_entry,6)
+
+GLOBAL(trampoline_cpu_started)
+ .byte 0
trampoline_gdt:
/* 0x0000: unused */
@@ -79,8 +123,12 @@ trampoline_gdt:
* address is computed at runtime.
*/
.quad 0x00c0920000000fff
-
- .pushsection .trampoline_rel, "a"
+ /*
+ * BOOT_PSEUDORM_CS and BOOT_PSEUDORM_DS are usable only at boot time,
+ * and their base addresses must reference the low location to which
+ * the boot-time code was loaded. Hence bootsym.
+ */
+ .pushsection .bootsym_rel, "a"
.long trampoline_gdt + BOOT_PSEUDORM_CS + 2 - .
.long trampoline_gdt + BOOT_PSEUDORM_DS + 2 - .
.popsection
@@ -94,9 +142,6 @@ GLOBAL(cpuid_ext_features)
GLOBAL(trampoline_xen_phys_start)
.long 0
-GLOBAL(trampoline_cpu_started)
- .byte 0
-
.code32
trampoline_protmode_entry:
/* Set up a few descriptors: on entry only CS is guaranteed good. */
@@ -113,12 +158,12 @@ trampoline_protmode_entry:
/* Load pagetable base register. */
mov $sym_offs(idle_pg_table),%eax
- add bootsym_rel(trampoline_xen_phys_start,4,%eax)
+ add tramp32sym_rel(trampoline_xen_phys_start,4,%eax)
mov %eax,%cr3
/* Adjust IA32_MISC_ENABLE if needed (for NX enabling below). */
- mov bootsym_rel(trampoline_misc_enable_off,4,%esi)
- mov bootsym_rel(trampoline_misc_enable_off+4,4,%edi)
+ mov tramp32sym_rel(trampoline_misc_enable_off,4,%esi)
+ mov tramp32sym_rel(trampoline_misc_enable_off+4,4,%edi)
mov %esi,%eax
or %edi,%eax
jz 1f
@@ -132,7 +177,7 @@ trampoline_protmode_entry:
1:
/* Set up EFER (Extended Feature Enable Register). */
- mov bootsym_rel(cpuid_ext_features,4,%edi)
+ mov tramp32sym_rel(cpuid_ext_features,4,%edi)
movl $MSR_EFER,%ecx
rdmsr
or $EFER_LME|EFER_SCE,%eax /* Long Mode + SYSCALL/SYSRET */
@@ -148,7 +193,7 @@ trampoline_protmode_entry:
1:
/* Now in compatibility mode. Long-jump into 64-bit mode. */
- ljmp $BOOT_CS64,$bootsym_rel(start64,6)
+ ljmp $BOOT_CS64,$tramp32sym_rel(start64,6)
.code64
start64:
@@ -183,7 +228,7 @@ start64:
idt_48: .word 0, 0, 0 # base = limit = 0
.word 0
gdt_48: .word 6*8-1
- .long bootsym_rel(trampoline_gdt,4)
+ .long tramp32sym_rel(trampoline_gdt,4)
/* The first page of trampoline is permanent, the rest boot-time only. */
/* Reuse the boot trampoline on the 1st trampoline page as stack for wakeup. */
@@ -249,7 +294,7 @@ trampoline_boot_cpu_entry:
mov $0x0200,%ax
int $0x16
- mov %al,bootsym(kbd_shift_flags)
+ mov %al,bootdatasym(kbd_shift_flags)
/* Disable irqs before returning to protected mode. */
cli
@@ -294,7 +339,7 @@ opt_edid:
.byte 0
#ifdef CONFIG_VIDEO
-GLOBAL(boot_vid_mode)
+boot_vid_mode:
.word VIDEO_80x25 /* If we don't run at all, assume basic video mode 3 at 80x25. */
vesa_size:
.word 0,0,0 /* width x depth x height */
diff --git a/xen/arch/x86/boot/video.S b/xen/arch/x86/boot/video.S
index 335a51c9b5..03907e9e9a 100644
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -45,7 +45,7 @@
#define PARAM_VESAPM_SEG 0x24
#define PARAM_VESAPM_OFF 0x26
#define PARAM_VESA_ATTRIB 0x28
-#define _param(param) bootsym(boot_vid_info)+(param)
+#define _param(param) bootdatasym(boot_vid_info)+(param)
video: xorw %ax, %ax
movw %ax, %gs # GS is zero
@@ -917,7 +917,7 @@ store_edid:
cmpw $0x004f, %ax # Call failed?
jne .Lno_edid
- movw %bx, bootsym(boot_edid_caps)
+ movw %bx, bootdatasym(boot_edid_caps)
cmpb $2, bootsym(opt_edid) # EDID forced on cmdline (edid=force)?
je .Lforce_edid
@@ -933,7 +933,7 @@ store_edid:
movw $0x01, %bx
movw $0x00, %cx
movw $0x00, %dx
- movw $bootsym(boot_edid_info), %di
+ movw $bootdatasym(boot_edid_info), %di
int $0x10
.Lno_edid:
diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
index e3cb9e033a..95bedddd0f 100644
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -53,7 +53,7 @@ ENTRY(wakeup_start)
movw $1, %ax
lmsw %ax # Turn on CR0.PE
- ljmpl $BOOT_CS32, $bootsym_rel(wakeup_32, 6)
+ ljmpl $BOOT_CS32, $trampsym_rel(wakeup_32, 6)
/* This code uses an extended set of video mode numbers. These include:
* Aliases for standard modes
@@ -118,11 +118,11 @@ wakeup_32:
mov $BOOT_DS, %eax
mov %eax, %ds
mov %eax, %ss
- mov $bootsym_rel(wakeup_stack, 4, %esp)
+ mov $trampsym_rel(wakeup_stack, 4, %esp)
# check saved magic again
mov $sym_offs(saved_magic),%eax
- add bootsym_rel(trampoline_xen_phys_start, 4, %eax)
+ add trampsym_rel(trampoline_xen_phys_start, 4, %eax)
mov (%eax), %eax
cmp $0x9abcdef0, %eax
jne bogus_saved_magic
@@ -135,12 +135,12 @@ wakeup_32:
/* Load pagetable base register */
mov $sym_offs(idle_pg_table),%eax
- add bootsym_rel(trampoline_xen_phys_start,4,%eax)
+ add trampsym_rel(trampoline_xen_phys_start,4,%eax)
mov %eax,%cr3
/* Will cpuid feature change after resume? */
/* Set up EFER (Extended Feature Enable Register). */
- mov bootsym_rel(cpuid_ext_features,4,%edi)
+ mov trampsym_rel(cpuid_ext_features,4,%edi)
test $0x20100800,%edi /* SYSCALL/SYSRET, No Execute, Long Mode? */
jz .Lskip_eferw
movl $MSR_EFER,%ecx
@@ -162,7 +162,7 @@ wakeup_32:
1:
/* Now in compatibility mode. Long-jump to 64-bit mode */
- ljmp $BOOT_CS64, $bootsym_rel(wakeup_64,6)
+ ljmp $BOOT_CS64, $trampsym_rel(wakeup_64,6)
.code64
wakeup_64:
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 7a13a30bc0..556942482e 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -99,7 +99,7 @@ static void __init efi_arch_relocate_image(unsigned long delta)
}
extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
-extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
+extern const s32 __trampoline32_rel_start[], __trampoline32_rel_stop[];
static void __init relocate_trampoline(unsigned long phys)
{
@@ -115,10 +115,10 @@ static void __init relocate_trampoline(unsigned long phys)
trampoline_ptr < __trampoline_rel_stop;
++trampoline_ptr )
*(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
- for ( trampoline_ptr = __trampoline_seg_start;
- trampoline_ptr < __trampoline_seg_stop;
+ for ( trampoline_ptr = __trampoline32_rel_start;
+ trampoline_ptr < __trampoline32_rel_stop;
++trampoline_ptr )
- *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
+ *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
}
static void __init place_string(u32 *addr, const char *s)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index a73139cd29..400dffaf23 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -237,11 +237,18 @@ SECTIONS
*(.init.data.rel.*)
. = ALIGN(4);
__trampoline_rel_start = .;
- *(.trampoline_rel)
+ *(.trampsym_rel)
__trampoline_rel_stop = .;
- __trampoline_seg_start = .;
- *(.trampoline_seg)
- __trampoline_seg_stop = .;
+ __trampoline32_rel_start = .;
+ *(.tramp32sym_rel)
+ __trampoline32_rel_stop = .;
+ __bootsym_rel_start = .;
+ *(.bootsym_rel)
+ *(.bootdatasym_rel)
+ __bootsym_rel_stop = .;
+ __bootsym_seg_start = .;
+ *(.bootsym_seg)
+ __bootsym_seg_stop = .;
/*
* struct alt_inst entries. From the header (alternative.h):
* "Alternative instructions for different CPU types or capabilities"
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 3/6] x86/boot: Split bootsym() into four types of relocations
2019-08-09 15:01 ` [Xen-devel] [PATCH v2 3/6] x86/boot: Split bootsym() into four types of relocations David Woodhouse
@ 2019-08-12 9:41 ` Jan Beulich
2019-08-19 15:24 ` David Woodhouse
0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-08-12 9:41 UTC (permalink / raw)
To: David Woodhouse; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
On 09.08.2019 17:01, David Woodhouse wrote:
> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -16,21 +16,62 @@
> * not guaranteed to persist.
> */
>
> -/* NB. bootsym() is only usable in real mode, or via BOOT_PSEUDORM_DS. */
> +/*
> + * There are four sets of relocations:
> + *
> + * bootsym(): Boot-time code relocated to low memory and run only once.
> + * Only usable at boot, in real mode or via BOOT_PSEUDORM_DS.
I'm not a native speaker, so my viewing this as ambiguous may be wrong,
but to me it reads as "Only usable at boot or in real mode or via
BOOT_PSEUDORM_DS" when aiui it ought to be "Only usable at boot AND (in
real mode OR via BOOT_PSEUDORM_DS)". In which case how about "Only usable
at boot from real mode or via BOOT_PSEUDORM_DS"?
> + * bootdatasym(): Boot-time BIOS-discovered data, relocated back up to Xen
> + * image after discovery.
> + * trampsym(): Trampoline code relocated into low memory for AP startup
> + * and wakeup.
> + * tramp32sym(): 32-bit trampoline code which at boot can be used directly
> + * from the Xen image in memory, but which will need to be
> + * relocated into low (well, into *mapped*) memory in order
> + * to be used for AP startup.
> + */
> #undef bootsym
> #define bootsym(s) ((s)-trampoline_start)
>
> #define bootsym_rel(sym, off, opnd...) \
> bootsym(sym),##opnd; \
> 111:; \
> - .pushsection .trampoline_rel, "a"; \
> + .pushsection .bootsym_rel, "a"; \
> .long 111b - (off) - .; \
> .popsection
>
> #define bootsym_segrel(sym, off) \
> $0,$bootsym(sym); \
> 111:; \
> - .pushsection .trampoline_seg, "a"; \
> + .pushsection .bootsym_seg, "a"; \
> + .long 111b - (off) - .; \
> + .popsection
> +
> +#define bootdatasym(s) ((s)-trampoline_start)
> +#define bootdatasym_rel(sym, off, opnd...) \
> + bootdatasym(sym),##opnd; \
> +111:; \
> + .pushsection .bootdatasym_rel, "a";\
> + .long 111b - (off) - .; \
> + .popsection
> +
> +#undef trampsym
> +#define trampsym(s) ((s)-trampoline_start)
> +
> +#define trampsym_rel(sym, off, opnd...) \
> + trampsym(sym),##opnd; \
> +111:; \
> + .pushsection .trampsym_rel, "a"; \
> + .long 111b - (off) - .; \
> + .popsection
> +
> +#undef tramp32sym
> +#define tramp32sym(s) ((s)-trampoline_start)
> +
> +#define tramp32sym_rel(sym, off, opnd...) \
> + tramp32sym(sym),##opnd; \
> +111:; \
> + .pushsection .tramp32sym_rel, "a"; \
> .long 111b - (off) - .; \
> .popsection
This repeats the basically same sequence of things several times.
I've not peeked ahead yet to see in how far more differences would
appear later on, but I don't really expect much of a further
change. In which case it might be nice to reduce the redundancy
here (by introducing a single "base" macro from which the four
similar ones would be derived).
Furthermore, with the intended isolation, wouldn't it be better to
limit visibility of the individual macros, such that using the
wrong one will be easier noticeable? This would be helped by there
being such a single "base" macro, as permitted to use macros could
then be, if needed, defined and undefined perhaps even multiple
times (for the time being at least).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/6] x86/boot: Split bootsym() into four types of relocations
2019-08-12 9:41 ` Jan Beulich
@ 2019-08-19 15:24 ` David Woodhouse
0 siblings, 0 replies; 23+ messages in thread
From: David Woodhouse @ 2019-08-19 15:24 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
[-- Attachment #1.1: Type: text/plain, Size: 6161 bytes --]
Apologies for delayed response; I was away last week and was frowned at
every time I so much as looked towards the laptop.
On Mon, 2019-08-12 at 11:41 +0200, Jan Beulich wrote:
> On 09.08.2019 17:01, David Woodhouse wrote:
> > --- a/xen/arch/x86/boot/trampoline.S
> > +++ b/xen/arch/x86/boot/trampoline.S
> > @@ -16,21 +16,62 @@
> > * not guaranteed to persist.
> > */
> >
> > -/* NB. bootsym() is only usable in real mode, or via BOOT_PSEUDORM_DS. */
> > +/*
> > + * There are four sets of relocations:
> > + *
> > + * bootsym(): Boot-time code relocated to low memory and run only once.
> > + * Only usable at boot, in real mode or via BOOT_PSEUDORM_DS.
>
> I'm not a native speaker, so my viewing this as ambiguous may be wrong,
> but to me it reads as "Only usable at boot or in real mode or via
> BOOT_PSEUDORM_DS" when aiui it ought to be "Only usable at boot AND (in
> real mode OR via BOOT_PSEUDORM_DS)". In which case how about "Only usable
> at boot from real mode or via BOOT_PSEUDORM_DS"?
Yes, I suppose I see that ambiguity. But ultimately I file it under the
category of "don't hack boot code while drunk".
I agree that to the reader of English (native or otherwise), that
sentence may have either meaning.
But this isn't documentation; it's code comments in an assembler file.
Anyone who is actually going to make a meaningful contribution to the
boot code, might reasonably be expected to understand that "real mode
or using the pseudo-realmode segment selector" are grouped together,
and that they must use one or the other of those. At boot time.
This is not an attempt at a two-line tutorial on all the pitfalls of
touching the boot code/data; via bootsym() or otherwise. It's just a
pointer in the right direction.
But sure, I'll have a look at fixing it — if I don't feel that what I
come up with is too clumsy.
> > + * bootdatasym(): Boot-time BIOS-discovered data, relocated back up to Xen
> > + * image after discovery.
> > + * trampsym(): Trampoline code relocated into low memory for AP startup
> > + * and wakeup.
> > + * tramp32sym(): 32-bit trampoline code which at boot can be used directly
> > + * from the Xen image in memory, but which will need to be
> > + * relocated into low (well, into *mapped*) memory in order
> > + * to be used for AP startup.
> > + */
> > #undef bootsym
> > #define bootsym(s) ((s)-trampoline_start)
> >
> > #define bootsym_rel(sym, off, opnd...) \
> > bootsym(sym),##opnd; \
> > 111:; \
> > - .pushsection .trampoline_rel, "a"; \
> > + .pushsection .bootsym_rel, "a"; \
> > .long 111b - (off) - .; \
> > .popsection
> >
> > #define bootsym_segrel(sym, off) \
> > $0,$bootsym(sym); \
> > 111:; \
> > - .pushsection .trampoline_seg, "a"; \
> > + .pushsection .bootsym_seg, "a"; \
> > + .long 111b - (off) - .; \
> > + .popsection
> > +
> > +#define bootdatasym(s) ((s)-trampoline_start)
> > +#define bootdatasym_rel(sym, off, opnd...) \
> > + bootdatasym(sym),##opnd; \
> > +111:; \
> > + .pushsection .bootdatasym_rel, "a";\
> > + .long 111b - (off) - .; \
> > + .popsection
> > +
> > +#undef trampsym
> > +#define trampsym(s) ((s)-trampoline_start)
> > +
> > +#define trampsym_rel(sym, off, opnd...) \
> > + trampsym(sym),##opnd; \
> > +111:; \
> > + .pushsection .trampsym_rel, "a"; \
> > + .long 111b - (off) - .; \
> > + .popsection
> > +
> > +#undef tramp32sym
> > +#define tramp32sym(s) ((s)-trampoline_start)
> > +
> > +#define tramp32sym_rel(sym, off, opnd...) \
> > + tramp32sym(sym),##opnd; \
> > +111:; \
> > + .pushsection .tramp32sym_rel, "a"; \
> > .long 111b - (off) - .; \
> > .popsection
>
> This repeats the basically same sequence of things several times.
> I've not peeked ahead yet to see in how far more differences would
> appear later on, but I don't really expect much of a further
> change. In which case it might be nice to reduce the redundancy
> here (by introducing a single "base" macro from which the four
> similar ones would be derived).
They end up being more different than this. It was my judgement that
attempting to create a more generic building block from which they
could all be derived would end up being less readable than just a
little bit of partial duplication. If you feel strongly otherwise, I
would welcome a follow-on patch to attempt to remedy that, if you
choose to send one.
> Furthermore, with the intended isolation, wouldn't it be better to
> limit visibility of the individual macros, such that using the
> wrong one will be easier noticeable? This would be helped by there
> being such a single "base" macro, as permitted to use macros could
> then be, if needed, defined and undefined perhaps even multiple
> times (for the time being at least).
I think I'd class that under 'don't hack boot code while drunk" too,
which is apparently the existing approach taken by this most of code
(except on the occasions when people have clearly done precisely that
☺).
The other .S files are *included* from head.S; some indirectly via
trampoline.S. Various macros are defined just once in head.S rather
than being in a header file or with mixed visibility. There is a
potential cleanup to be done there, but it is not one of the cleanups I
had elected to make because I see it as being of limited utility except
cosmetic. Again, if you feel strongly then I would welcome a follow-on
patch or series to move everything out and build each .S file as a
separate compilation unit.
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Xen-devel] [PATCH v2 4/6] x86/boot: Rename trampoline_{start, end} to boot_trampoline_{start, end}
[not found] <cover.1565362089.git.dwmw@amazon.co.uk>
` (2 preceding siblings ...)
2019-08-09 15:01 ` [Xen-devel] [PATCH v2 3/6] x86/boot: Split bootsym() into four types of relocations David Woodhouse
@ 2019-08-09 15:02 ` David Woodhouse
2019-08-12 9:55 ` Jan Beulich
2019-08-09 15:02 ` [Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image David Woodhouse
2019-08-09 15:02 ` [Xen-devel] [PATCH v2 6/6] x86/boot: Do not use trampoline for no-real-mode boot paths David Woodhouse
5 siblings, 1 reply; 23+ messages in thread
From: David Woodhouse @ 2019-08-09 15:02 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 10134 bytes --]
From: David Woodhouse <dwmw@amazon.co.uk>
In preparation for splitting the boot and permanent trampolines from
each other. Some of these will change back, but most are boot so do the
plain search/replace that way first, then a subsequent patch will extract
the permanent trampoline code.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
xen/arch/x86/boot/head.S | 12 ++++++------
xen/arch/x86/boot/trampoline.S | 10 +++++-----
xen/arch/x86/boot/video.S | 4 ++--
xen/arch/x86/efi/efi-boot.h | 4 ++--
xen/arch/x86/setup.c | 4 ++--
xen/arch/x86/tboot.c | 6 +++---
xen/arch/x86/x86_64/mm.c | 2 +-
xen/arch/x86/xen.lds.S | 6 +++---
xen/include/asm-x86/config.h | 6 +++---
9 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 07621d1a30..556dab127f 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -754,20 +754,20 @@ trampoline_setup:
cmpb $0, sym_fs(skip_realmode)
jz 1f
/* If no-real-mode, jump straight to trampoline_protmode_entry */
- lea trampoline_protmode_entry-trampoline_start(%edi),%eax
+ lea trampoline_protmode_entry-boot_trampoline_start(%edi),%eax
/* EBX == 0 indicates we are the BP (Boot Processor). */
xor %ebx,%ebx
jmp 2f
1:
/* Go via 16-bit code in trampoline_boot_cpu_entry */
- lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
+ lea trampoline_boot_cpu_entry-boot_trampoline_start(%edi),%eax
2:
pushl $BOOT_CS32
push %eax
/* Copy bootstrap trampoline to low memory, below 1MB. */
- mov $sym_offs(trampoline_start),%esi
- mov $((trampoline_end - trampoline_start) / 4),%ecx
+ mov $sym_offs(boot_trampoline_start),%esi
+ mov $((boot_trampoline_end - boot_trampoline_start) / 4),%ecx
rep movsl %fs:(%esi),%es:(%edi)
/* Jump into the relocated trampoline. */
@@ -779,8 +779,8 @@ cmdline_parse_early:
reloc:
#include "reloc.S"
-ENTRY(trampoline_start)
+ENTRY(boot_trampoline_start)
#include "trampoline.S"
-ENTRY(trampoline_end)
+ENTRY(boot_trampoline_end)
#include "x86_64.S"
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 95a4bef553..26af9c6beb 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -31,7 +31,7 @@
* to be used for AP startup.
*/
#undef bootsym
-#define bootsym(s) ((s)-trampoline_start)
+#define bootsym(s) ((s)-boot_trampoline_start)
#define bootsym_rel(sym, off, opnd...) \
bootsym(sym),##opnd; \
@@ -47,7 +47,7 @@
.long 111b - (off) - .; \
.popsection
-#define bootdatasym(s) ((s)-trampoline_start)
+#define bootdatasym(s) ((s)-boot_trampoline_start)
#define bootdatasym_rel(sym, off, opnd...) \
bootdatasym(sym),##opnd; \
111:; \
@@ -56,7 +56,7 @@
.popsection
#undef trampsym
-#define trampsym(s) ((s)-trampoline_start)
+#define trampsym(s) ((s)-boot_trampoline_start)
#define trampsym_rel(sym, off, opnd...) \
trampsym(sym),##opnd; \
@@ -66,7 +66,7 @@
.popsection
#undef tramp32sym
-#define tramp32sym(s) ((s)-trampoline_start)
+#define tramp32sym(s) ((s)-boot_trampoline_start)
#define tramp32sym_rel(sym, off, opnd...) \
tramp32sym(sym),##opnd; \
@@ -232,7 +232,7 @@ gdt_48: .word 6*8-1
/* The first page of trampoline is permanent, the rest boot-time only. */
/* Reuse the boot trampoline on the 1st trampoline page as stack for wakeup. */
- .equ wakeup_stack, trampoline_start + PAGE_SIZE
+ .equ wakeup_stack, boot_trampoline_start + PAGE_SIZE
.global wakeup_stack
/* From here on early boot only. */
diff --git a/xen/arch/x86/boot/video.S b/xen/arch/x86/boot/video.S
index 03907e9e9a..5087c6a4d5 100644
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -15,8 +15,8 @@
#include "video.h"
-/* Scratch space layout: trampoline_end to trampoline_end+0x1000. */
-#define modelist bootsym(trampoline_end) /* 2kB (256 entries) */
+/* Scratch space layout: boot_trampoline_end to boot_trampoline_end+0x1000. */
+#define modelist bootsym(boot_trampoline_end) /* 2kB (256 entries) */
#define vesa_glob_info (modelist + 0x800) /* 1kB */
#define vesa_mode_info (vesa_glob_info + 0x400) /* 1kB */
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 556942482e..fc2ea554b5 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -232,7 +232,7 @@ static void __init noreturn efi_arch_post_exit_boot(void)
u64 cr4 = XEN_MINIMAL_CR4 & ~X86_CR4_PGE, efer;
efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start);
- memcpy((void *)trampoline_phys, trampoline_start, cfg.size);
+ memcpy((void *)trampoline_phys, boot_trampoline_start, cfg.size);
/* Set system registers and transfer control. */
asm volatile("pushq $0\n\tpopfq");
@@ -566,7 +566,7 @@ static void __init efi_arch_memory_setup(void)
cfg.addr = 0x100000;
if ( efi_enabled(EFI_LOADER) )
- cfg.size = trampoline_end - trampoline_start;
+ cfg.size = boot_trampoline_end - boot_trampoline_start;
else
cfg.size = TRAMPOLINE_SPACE + TRAMPOLINE_STACK_SPACE;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index decea2e77a..06e779368c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1879,8 +1879,8 @@ int __hwdom_init xen_in_range(unsigned long mfn)
if ( !xen_regions[0].s )
{
/* S3 resume code (and other real mode trampoline code) */
- xen_regions[region_s3].s = bootsym_phys(trampoline_start);
- xen_regions[region_s3].e = bootsym_phys(trampoline_end);
+ xen_regions[region_s3].s = bootsym_phys(boot_trampoline_start);
+ xen_regions[region_s3].e = bootsym_phys(boot_trampoline_end);
/*
* This needs to remain in sync with the uses of the same symbols in
diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index f3fdee4d39..325d94d23a 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -369,9 +369,9 @@ void tboot_shutdown(uint32_t shutdown_type)
*/
g_tboot_shared->num_mac_regions = 3;
/* S3 resume code (and other real mode trampoline code) */
- g_tboot_shared->mac_regions[0].start = bootsym_phys(trampoline_start);
- g_tboot_shared->mac_regions[0].size = bootsym_phys(trampoline_end) -
- bootsym_phys(trampoline_start);
+ g_tboot_shared->mac_regions[0].start = bootsym_phys(boot_trampoline_start);
+ g_tboot_shared->mac_regions[0].size = bootsym_phys(boot_trampoline_end) -
+ bootsym_phys(boot_trampoline_start);
/* hypervisor .text + .rodata */
g_tboot_shared->mac_regions[1].start = (uint64_t)__pa(&_stext);
g_tboot_shared->mac_regions[1].size = __pa(&__2M_rodata_end) -
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 1919cae18b..149cc4f7b5 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -697,7 +697,7 @@ void __init zap_low_mappings(void)
/* Replace with mapping of the boot trampoline only. */
map_pages_to_xen(trampoline_phys, maddr_to_mfn(trampoline_phys),
- PFN_UP(trampoline_end - trampoline_start),
+ PFN_UP(boot_trampoline_end - boot_trampoline_start),
__PAGE_HYPERVISOR);
}
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 400dffaf23..6968262a60 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -377,12 +377,12 @@ ASSERT(IS_ALIGNED(cpu0_stack, STACK_SIZE), "cpu0_stack misaligned")
ASSERT(IS_ALIGNED(__init_begin, PAGE_SIZE), "__init_begin misaligned")
ASSERT(IS_ALIGNED(__init_end, PAGE_SIZE), "__init_end misaligned")
-ASSERT(IS_ALIGNED(trampoline_start, 4), "trampoline_start misaligned")
-ASSERT(IS_ALIGNED(trampoline_end, 4), "trampoline_end misaligned")
+ASSERT(IS_ALIGNED(boot_trampoline_start, 4), "boot_trampoline_start misaligned")
+ASSERT(IS_ALIGNED(boot_trampoline_end, 4), "boot_trampoline_end misaligned")
ASSERT(IS_ALIGNED(__bss_start, 8), "__bss_start misaligned")
ASSERT(IS_ALIGNED(__bss_end, 8), "__bss_end misaligned")
-ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN,
+ASSERT((boot_trampoline_end - boot_trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN,
"not enough room for trampoline and mbi data")
ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
"wakeup stack too small")
diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
index 6e4f28d934..ada8c7b4f6 100644
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -90,11 +90,11 @@
#ifndef __ASSEMBLY__
extern unsigned long trampoline_phys;
#define bootsym_phys(sym) \
- (((unsigned long)&(sym)-(unsigned long)&trampoline_start)+trampoline_phys)
+ (((unsigned long)&(sym)-(unsigned long)&boot_trampoline_start)+trampoline_phys)
#define bootsym(sym) \
(*RELOC_HIDE((typeof(&(sym)))__va(__pa(&(sym))), \
- trampoline_phys-__pa(trampoline_start)))
-extern char trampoline_start[], trampoline_end[];
+ trampoline_phys-__pa(boot_trampoline_start)))
+extern char boot_trampoline_start[], boot_trampoline_end[];
extern char trampoline_realmode_entry[];
extern unsigned int trampoline_xen_phys_start;
extern unsigned char trampoline_cpu_started;
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 4/6] x86/boot: Rename trampoline_{start, end} to boot_trampoline_{start, end}
2019-08-09 15:02 ` [Xen-devel] [PATCH v2 4/6] x86/boot: Rename trampoline_{start, end} to boot_trampoline_{start, end} David Woodhouse
@ 2019-08-12 9:55 ` Jan Beulich
2019-08-19 15:24 ` David Woodhouse
0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-08-12 9:55 UTC (permalink / raw)
To: David Woodhouse; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
On 09.08.2019 17:02, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> In preparation for splitting the boot and permanent trampolines from
> each other. Some of these will change back, but most are boot so do the
> plain search/replace that way first, then a subsequent patch will extract
> the permanent trampoline code.
To be honest I don't view it as helpful to do things in this order.
If you first re-arranged the ordering of items within the trampoline,
we'd then not end up with an intermediate state where the labels are
misleading. Is there a reason things can't sensibly be done the other
way around?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 4/6] x86/boot: Rename trampoline_{start, end} to boot_trampoline_{start, end}
2019-08-12 9:55 ` Jan Beulich
@ 2019-08-19 15:24 ` David Woodhouse
2019-08-27 8:51 ` Jan Beulich
0 siblings, 1 reply; 23+ messages in thread
From: David Woodhouse @ 2019-08-19 15:24 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
[-- Attachment #1.1: Type: text/plain, Size: 1943 bytes --]
On Mon, 2019-08-12 at 11:55 +0200, Jan Beulich wrote:
> On 09.08.2019 17:02, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> >
> > In preparation for splitting the boot and permanent trampolines from
> > each other. Some of these will change back, but most are boot so do the
> > plain search/replace that way first, then a subsequent patch will extract
> > the permanent trampoline code.
>
> To be honest I don't view it as helpful to do things in this order.
> If you first re-arranged the ordering of items within the trampoline,
> we'd then not end up with an intermediate state where the labels are
> misleading. Is there a reason things can't sensibly be done the other
> way around?
Obviously I did all this in a working tree first, swore at it a lot and
finally got it working, then attempted to split it up into separate
meaningful commits which individually made sense. There is plenty of
room for subjectivity in the choices I made in that last step.
I'm not sure I quite see why you say the labels are misleading. My
intent was to apply labels based on what each object is *used* for,
despite the fact that to start with they're all actually in the same
place. And then to actually move each different type of symbol into its
separate section/location to clean things up.
Is it just the code comments at the start of trampoline.S that you find
misleading in the interim stage? Because those *don't* purely talk
about what bootsym/bootdatasym/trampsym/tramp32sym are used for; they
do say how they are (eventually) relocated. I suppose I could rip that
code comment out of patch #3 completely and add it again in a later
commit... or just just add it again. I write code comments in an
attempt to be helpful to those who come after me (especially when
that's actually myself) but if they're going to cause problems, then
maybe they're more hassle than they're worth?
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 4/6] x86/boot: Rename trampoline_{start, end} to boot_trampoline_{start, end}
2019-08-19 15:24 ` David Woodhouse
@ 2019-08-27 8:51 ` Jan Beulich
2019-08-27 9:31 ` David Woodhouse
0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-08-27 8:51 UTC (permalink / raw)
To: David Woodhouse; +Cc: Andrew Cooper, xen-devel, Wei Liu, Roger Pau Monné
On 19.08.2019 17:24, David Woodhouse wrote:
> On Mon, 2019-08-12 at 11:55 +0200, Jan Beulich wrote:
>> On 09.08.2019 17:02, David Woodhouse wrote:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> In preparation for splitting the boot and permanent trampolines from
>>> each other. Some of these will change back, but most are boot so do the
>>> plain search/replace that way first, then a subsequent patch will extract
>>> the permanent trampoline code.
>>
>> To be honest I don't view it as helpful to do things in this order.
>> If you first re-arranged the ordering of items within the trampoline,
>> we'd then not end up with an intermediate state where the labels are
>> misleading. Is there a reason things can't sensibly be done the other
>> way around?
>
> Obviously I did all this in a working tree first, swore at it a lot and
> finally got it working, then attempted to split it up into separate
> meaningful commits which individually made sense. There is plenty of
> room for subjectivity in the choices I made in that last step.
>
> I'm not sure I quite see why you say the labels are misleading. My
> intent was to apply labels based on what each object is *used* for,
> despite the fact that to start with they're all actually in the same
> place. And then to actually move each different type of symbol into its
> separate section/location to clean things up.
>
> Is it just the code comments at the start of trampoline.S that you find
> misleading in the interim stage? Because those *don't* purely talk
> about what bootsym/bootdatasym/trampsym/tramp32sym are used for; they
> do say how they are (eventually) relocated. I suppose I could rip that
> code comment out of patch #3 completely and add it again in a later
> commit... or just just add it again. I write code comments in an
> attempt to be helpful to those who come after me (especially when
> that's actually myself) but if they're going to cause problems, then
> maybe they're more hassle than they're worth?
No, it's actually the label names: The "boot" that this patch prefixes
to them isn't correct until all post-boot (i.e. AP bringup) parts
have been moved out of the framed block of code.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 4/6] x86/boot: Rename trampoline_{start, end} to boot_trampoline_{start, end}
2019-08-27 8:51 ` Jan Beulich
@ 2019-08-27 9:31 ` David Woodhouse
0 siblings, 0 replies; 23+ messages in thread
From: David Woodhouse @ 2019-08-27 9:31 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, David Woodhouse, xen-devel, Wei Liu,
"Roger Pau Monné"
> On 19.08.2019 17:24, David Woodhouse wrote:
>> On Mon, 2019-08-12 at 11:55 +0200, Jan Beulich wrote:
>>> On 09.08.2019 17:02, David Woodhouse wrote:
>>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>>
>>>> In preparation for splitting the boot and permanent trampolines from
>>>> each other. Some of these will change back, but most are boot so do
>>>> the
>>>> plain search/replace that way first, then a subsequent patch will
>>>> extract
>>>> the permanent trampoline code.
>>>
>>> To be honest I don't view it as helpful to do things in this order.
>>> If you first re-arranged the ordering of items within the trampoline,
>>> we'd then not end up with an intermediate state where the labels are
>>> misleading. Is there a reason things can't sensibly be done the other
>>> way around?
>>
>> Obviously I did all this in a working tree first, swore at it a lot and
>> finally got it working, then attempted to split it up into separate
>> meaningful commits which individually made sense. There is plenty of
>> room for subjectivity in the choices I made in that last step.
>>
>> I'm not sure I quite see why you say the labels are misleading. My
>> intent was to apply labels based on what each object is *used* for,
>> despite the fact that to start with they're all actually in the same
>> place. And then to actually move each different type of symbol into its
>> separate section/location to clean things up.
>>
>> Is it just the code comments at the start of trampoline.S that you find
>> misleading in the interim stage? Because those *don't* purely talk
>> about what bootsym/bootdatasym/trampsym/tramp32sym are used for; they
>> do say how they are (eventually) relocated. I suppose I could rip that
>> code comment out of patch #3 completely and add it again in a later
>> commit... or just just add it again. I write code comments in an
>> attempt to be helpful to those who come after me (especially when
>> that's actually myself) but if they're going to cause problems, then
>> maybe they're more hassle than they're worth?
>
> No, it's actually the label names: The "boot" that this patch prefixes
> to them isn't correct until all post-boot (i.e. AP bringup) parts
> have been moved out of the framed block of code.
Hm, OK. AFK at this moment but I'll take another look. Basically there
wasn't a perfect way to label and move things; either way there were
glitches during the transition and my recollection was that I preferred
this one because it was purely cosmetic and only lasted for a commit or
two.
Will see if I can come up with something nicer within the amount of time
it is reasonable to spend on such a transitional issue.
--
dwmw2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image
[not found] <cover.1565362089.git.dwmw@amazon.co.uk>
` (3 preceding siblings ...)
2019-08-09 15:02 ` [Xen-devel] [PATCH v2 4/6] x86/boot: Rename trampoline_{start, end} to boot_trampoline_{start, end} David Woodhouse
@ 2019-08-09 15:02 ` David Woodhouse
2019-08-12 10:24 ` Jan Beulich
2019-08-09 15:02 ` [Xen-devel] [PATCH v2 6/6] x86/boot: Do not use trampoline for no-real-mode boot paths David Woodhouse
5 siblings, 1 reply; 23+ messages in thread
From: David Woodhouse @ 2019-08-09 15:02 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 17414 bytes --]
From: David Woodhouse <dwmw@amazon.co.uk>
Ditch the bootsym() access from C code for the variables populated by
16-bit boot code. As well as being cleaner this also paves the way for
not having the 16-bit boot code in low memory for no-real-mode or EFI
loader boots at all.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
xen/arch/x86/boot/edd.S | 2 ++
xen/arch/x86/boot/head.S | 16 +++++++++++++++
xen/arch/x86/boot/mem.S | 2 ++
xen/arch/x86/boot/trampoline.S | 33 ++++++++++++++++++++++++++++---
xen/arch/x86/boot/video.S | 30 +++++++++++++++-------------
xen/arch/x86/platform_hypercall.c | 18 ++++++++---------
xen/arch/x86/setup.c | 22 ++++++++++-----------
xen/arch/x86/xen.lds.S | 8 +++++++-
xen/include/asm-x86/edd.h | 1 -
9 files changed, 93 insertions(+), 39 deletions(-)
diff --git a/xen/arch/x86/boot/edd.S b/xen/arch/x86/boot/edd.S
index 434bbbd960..138d04c964 100644
--- a/xen/arch/x86/boot/edd.S
+++ b/xen/arch/x86/boot/edd.S
@@ -163,6 +163,7 @@ edd_done:
.Ledd_mbr_sig_skip:
ret
+ .pushsection .data.boot16, "aw", @progbits
GLOBAL(boot_edd_info_nr)
.byte 0
GLOBAL(boot_mbr_signature_nr)
@@ -171,3 +172,4 @@ GLOBAL(boot_mbr_signature)
.fill EDD_MBR_SIG_MAX*8,1,0
GLOBAL(boot_edd_info)
.fill EDD_INFO_MAX * (EDDEXTSIZE + EDDPARMSIZE), 1, 0
+ .popsection
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 556dab127f..104eb0eb3c 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -733,6 +733,17 @@ trampoline_setup:
cmp $sym_offs(__bootsym_seg_stop),%edi
jb 1b
+ /* Relocations for the boot data section. */
+ mov sym_fs(trampoline_phys),%edx
+ add $(boot_trampoline_end - boot_trampoline_start),%edx
+ mov $sym_offs(__bootdatasym_rel_start),%edi
+1:
+ mov %fs:(%edi),%eax
+ add %edx,%fs:(%edi,%eax)
+ add $4,%edi
+ cmp $sym_offs(__bootdatasym_rel_stop),%edi
+ jb 1b
+
/* Do not parse command line on EFI platform here. */
cmpb $0,sym_fs(efi_platform)
jnz 1f
@@ -770,6 +781,11 @@ trampoline_setup:
mov $((boot_trampoline_end - boot_trampoline_start) / 4),%ecx
rep movsl %fs:(%esi),%es:(%edi)
+ /* Copy boot data template to low memory. */
+ mov $sym_offs(bootdata_start),%esi
+ mov $((bootdata_end - bootdata_start + 3) / 4),%ecx
+ rep movsl %fs:(%esi),%es:(%edi)
+
/* Jump into the relocated trampoline. */
lret
diff --git a/xen/arch/x86/boot/mem.S b/xen/arch/x86/boot/mem.S
index aa39608442..86f0fa9af7 100644
--- a/xen/arch/x86/boot/mem.S
+++ b/xen/arch/x86/boot/mem.S
@@ -67,6 +67,7 @@ get_memory_map:
ret
.align 4
+ .pushsection .data.boot16, "aw", @progbits
GLOBAL(bios_e820map)
.fill E820_BIOS_MAX*20,1,0
GLOBAL(bios_e820nr)
@@ -75,3 +76,4 @@ GLOBAL(lowmem_kb)
.long 0
GLOBAL(highmem_kb)
.long 0
+ .popsection
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 26af9c6beb..b5517a44bb 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -47,11 +47,15 @@
.long 111b - (off) - .; \
.popsection
-#define bootdatasym(s) ((s)-boot_trampoline_start)
+ .pushsection .data.boot16, "aw", @progbits
+GLOBAL(bootdata_start)
+ .popsection
+
+#define bootdatasym(s) ((s)-bootdata_start+(boot_trampoline_end-boot_trampoline_start))
#define bootdatasym_rel(sym, off, opnd...) \
bootdatasym(sym),##opnd; \
111:; \
- .pushsection .bootdatasym_rel, "a";\
+ .pushsection .bootsym_rel, "a";\
.long 111b - (off) - .; \
.popsection
@@ -227,7 +231,7 @@ start64:
.word 0
idt_48: .word 0, 0, 0 # base = limit = 0
.word 0
-gdt_48: .word 6*8-1
+gdt_48: .word 7*8-1
.long tramp32sym_rel(trampoline_gdt,4)
/* The first page of trampoline is permanent, the rest boot-time only. */
@@ -318,6 +322,23 @@ trampoline_boot_cpu_entry:
mov %eax,%gs
mov %eax,%ss
+ /*
+ * Copy locally-gathered data back up into the Xen physical image
+ */
+ mov $BOOT_FS,%eax
+ mov %eax,%es
+
+ mov $sym_offs(bootdata_end),%ecx
+ mov $sym_offs(bootdata_start),%edi
+ sub %edi,%ecx
+ mov $bootdatasym_rel(bootdata_start,4,%esi)
+ rep movsb %ds:(%esi),%es:(%edi)
+
+ /*
+ * %es still points to BOOT_FS but trampoline_protmode_entry
+ * reloads it anyway.
+ */
+
/* EBX == 0 indicates we are the BP (Boot Processor). */
xor %ebx,%ebx
@@ -345,8 +366,10 @@ vesa_size:
.word 0,0,0 /* width x depth x height */
#endif
+ .pushsection .data.boot16, "aw", @progbits
GLOBAL(kbd_shift_flags)
.byte 0
+ .popsection
rm_idt: .word 256*4-1, 0, 0
@@ -355,3 +378,7 @@ rm_idt: .word 256*4-1, 0, 0
#ifdef CONFIG_VIDEO
#include "video.S"
#endif
+
+ .pushsection .data.boot16, "aw", @progbits
+GLOBAL(bootdata_end)
+ .popsection
diff --git a/xen/arch/x86/boot/video.S b/xen/arch/x86/boot/video.S
index 5087c6a4d5..4608464b77 100644
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -15,10 +15,10 @@
#include "video.h"
-/* Scratch space layout: boot_trampoline_end to boot_trampoline_end+0x1000. */
-#define modelist bootsym(boot_trampoline_end) /* 2kB (256 entries) */
-#define vesa_glob_info (modelist + 0x800) /* 1kB */
-#define vesa_mode_info (vesa_glob_info + 0x400) /* 1kB */
+/* Scratch space layout: bootdata_end to bootdata_end+0x1000. */
+#define modelist(t) bootdatasym_rel(bootdata_end,2,t) /* 2KiB (256 entries) */
+#define vesa_glob_info(t) bootdatasym_rel((bootdata_end+0x800),2,t) /* 1KiB */
+#define vesa_mode_info(t) bootdatasym_rel((bootdata_end+0xc00),2,t) /* 1KiB */
/* Retrieve Extended Display Identification Data. */
#define CONFIG_FIRMWARE_EDID
@@ -113,7 +113,7 @@ mopar2: movb %al, _param(PARAM_VIDEO_LINES)
# Fetching of VESA frame buffer parameters
mopar_gr:
- leaw vesa_mode_info, %di
+ leaw vesa_mode_info(%di)
movb $0x23, _param(PARAM_HAVE_VGA)
movw 16(%di), %ax
movw %ax, _param(PARAM_LFB_LINELENGTH)
@@ -134,7 +134,7 @@ mopar_gr:
movw %ax, _param(PARAM_VESA_ATTRIB)
# get video mem size
- leaw vesa_glob_info, %di
+ leaw vesa_glob_info(%di)
xorl %eax, %eax
movw 18(%di), %ax
movl %eax, _param(PARAM_LFB_SIZE)
@@ -226,7 +226,7 @@ an1: call prtstr
leaw bootsym(listhdr), %si # Table header
call prtstr
movb $0x30, %dl # DL holds mode number
- leaw modelist, %si
+ leaw modelist(%si)
lm1: cmpw $ASK_VGA, (%si) # End?
jz lm2
@@ -435,13 +435,13 @@ setmenu:
jmp mode_set
check_vesa:
- leaw vesa_glob_info, %di
+ leaw vesa_glob_info(%di)
movw $0x4f00, %ax
int $0x10
cmpw $0x004f, %ax
jnz setbad
- leaw vesa_mode_info, %di
+ leaw vesa_mode_info(%di)
subb $VIDEO_FIRST_VESA>>8, %bh
movw %bx, %cx # Get mode information structure
movw $0x4f01, %ax
@@ -509,7 +509,7 @@ inidx: outb %al, %dx # Read from indexed VGA register
setvesabysize:
call mode_table
- leaw modelist,%si
+ leaw modelist(%si)
1: add $8,%si
cmpw $ASK_VGA,-8(%si) # End?
je _setbad
@@ -669,7 +669,7 @@ mode_table:
orw %di, %di
jnz mtab1
- leaw modelist, %di # Store standard modes:
+ leaw modelist(%di) # Store standard modes:
movw $VIDEO_80x25,(%di) # The 80x25 mode (ALL)
movw $0x50,2(%di)
movw $0x19,4(%di)
@@ -684,7 +684,7 @@ mode_table:
movw $ASK_VGA, (%di) # End marker
movw %di, bootsym(mt_end)
-mtab1: leaw modelist, %si # SI=mode list, DI=list end
+mtab1: leaw modelist(%si) # SI=mode list, DI=list end
ret0: ret
# Modes usable on all standard VGAs
@@ -700,7 +700,7 @@ vga_modes_end:
# Detect VESA modes.
vesa_modes:
movw %di, %bp # BP=original mode table end
- leaw vesa_glob_info, %di
+ leaw vesa_glob_info(%di)
movw $0x4f00, %ax # VESA Get card info call
int $0x10
movw %di, %si
@@ -897,7 +897,7 @@ store_edid:
cmpb $1, bootsym(opt_edid) # EDID disabled on cmdline (edid=no)?
je .Lno_edid
- leaw vesa_glob_info, %di
+ leaw vesa_glob_info(%di)
movw $0x4f00, %ax
int $0x10
cmpw $0x004f, %ax
@@ -990,6 +990,7 @@ name_bann: .asciz "Video adapter: "
force_size: .word 0 # Use this size instead of the one in BIOS vars
+ .pushsection .data.boot16, "aw", @progbits
GLOBAL(boot_vid_info)
.byte 0, 0 /* orig_x, orig_y */
.byte 3 /* text mode 3 */
@@ -1001,3 +1002,4 @@ GLOBAL(boot_edid_info)
.fill 128,1,0x13
GLOBAL(boot_edid_caps)
.word 0x1313
+ .popsection
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index b19f6ec4ed..9a56bd8f84 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -318,10 +318,10 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
u16 length;
ret = -ESRCH;
- if ( op->u.firmware_info.index >= bootsym(boot_edd_info_nr) )
+ if ( op->u.firmware_info.index >= boot_edd_info_nr )
break;
- info = bootsym(boot_edd_info) + op->u.firmware_info.index;
+ info = boot_edd_info + op->u.firmware_info.index;
/* Transfer the EDD info block. */
ret = -EFAULT;
@@ -357,10 +357,10 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
const struct mbr_signature *sig;
ret = -ESRCH;
- if ( op->u.firmware_info.index >= bootsym(boot_mbr_signature_nr) )
+ if ( op->u.firmware_info.index >= boot_mbr_signature_nr )
break;
- sig = bootsym(boot_mbr_signature) + op->u.firmware_info.index;
+ sig = boot_mbr_signature + op->u.firmware_info.index;
op->u.firmware_info.u.disk_mbr_signature.device = sig->device;
op->u.firmware_info.u.disk_mbr_signature.mbr_signature =
@@ -376,13 +376,13 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
#ifdef CONFIG_VIDEO
if ( op->u.firmware_info.index != 0 )
break;
- if ( *(u32 *)bootsym(boot_edid_info) == 0x13131313 )
+ if ( *(u32 *)boot_edid_info == 0x13131313 )
break;
op->u.firmware_info.u.vbeddc_info.capabilities =
- bootsym(boot_edid_caps);
+ boot_edid_caps;
op->u.firmware_info.u.vbeddc_info.edid_transfer_time =
- bootsym(boot_edid_caps) >> 8;
+ boot_edid_caps >> 8;
ret = 0;
if ( __copy_field_to_guest(u_xenpf_op, op, u.firmware_info.
@@ -390,7 +390,7 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
__copy_field_to_guest(u_xenpf_op, op, u.firmware_info.
u.vbeddc_info.edid_transfer_time) ||
copy_to_compat(op->u.firmware_info.u.vbeddc_info.edid,
- bootsym(boot_edid_info), 128) )
+ boot_edid_info, 128) )
ret = -EFAULT;
#endif
break;
@@ -407,7 +407,7 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
if ( op->u.firmware_info.index != 0 )
break;
- op->u.firmware_info.u.kbd_shift_flags = bootsym(kbd_shift_flags);
+ op->u.firmware_info.u.kbd_shift_flags = kbd_shift_flags;
ret = 0;
if ( __copy_field_to_guest(u_xenpf_op, op,
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 06e779368c..101c9dd272 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -509,7 +509,7 @@ extern struct boot_video_info boot_vid_info;
static void __init parse_video_info(void)
{
#ifdef CONFIG_VIDEO
- struct boot_video_info *bvi = &bootsym(boot_vid_info);
+ struct boot_video_info *bvi = &boot_vid_info;
/* vga_console_info is filled directly on EFI platform. */
if ( efi_enabled(EFI_BOOT) )
@@ -674,10 +674,10 @@ static char * __init cmdline_cook(char *p, const char *loader_name)
static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int limit)
{
- unsigned int n = min(bootsym(bios_e820nr), limit);
+ unsigned int n = min(bios_e820nr, limit);
if ( n )
- memcpy(map, bootsym(bios_e820map), sizeof(*map) * n);
+ memcpy(map, bios_e820map, sizeof(*map) * n);
return n;
}
@@ -818,15 +818,15 @@ void __init noreturn __start_xen(unsigned long mbi_p)
}
/* Print VBE/DDC EDID information. */
- if ( bootsym(boot_edid_caps) != 0x1313 )
+ if ( boot_edid_caps != 0x1313 )
{
- u16 caps = bootsym(boot_edid_caps);
+ u16 caps = boot_edid_caps;
printk(" VBE/DDC methods:%s%s%s; ",
(caps & 1) ? " V1" : "",
(caps & 2) ? " V2" : "",
!(caps & 3) ? " none" : "");
printk("EDID transfer time: %d seconds\n", caps >> 8);
- if ( *(u32 *)bootsym(boot_edid_info) == 0x13131313 )
+ if ( *(u32 *)boot_edid_info == 0x13131313 )
{
printk(" EDID info not retrieved because ");
if ( !(caps & 3) )
@@ -841,9 +841,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
printk("Disc information:\n");
printk(" Found %d MBR signatures\n",
- bootsym(boot_mbr_signature_nr));
+ boot_mbr_signature_nr);
printk(" Found %d EDD information structures\n",
- bootsym(boot_edd_info_nr));
+ boot_edd_info_nr);
/* Check that we have at least one Multiboot module. */
if ( !(mbi->flags & MBI_MODULES) || (mbi->mods_count == 0) )
@@ -926,14 +926,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
bytes += map->size + 4;
}
}
- else if ( bootsym(lowmem_kb) )
+ else if ( lowmem_kb )
{
memmap_type = "Xen-e801";
e820_raw.map[0].addr = 0;
- e820_raw.map[0].size = bootsym(lowmem_kb) << 10;
+ e820_raw.map[0].size = lowmem_kb << 10;
e820_raw.map[0].type = E820_RAM;
e820_raw.map[1].addr = 0x100000;
- e820_raw.map[1].size = bootsym(highmem_kb) << 10;
+ e820_raw.map[1].size = highmem_kb << 10;
e820_raw.map[1].type = E820_RAM;
e820_raw.nr_map = 2;
}
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 6968262a60..acc1e2d593 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -244,11 +244,13 @@ SECTIONS
__trampoline32_rel_stop = .;
__bootsym_rel_start = .;
*(.bootsym_rel)
- *(.bootdatasym_rel)
__bootsym_rel_stop = .;
__bootsym_seg_start = .;
*(.bootsym_seg)
__bootsym_seg_stop = .;
+ __bootdatasym_rel_start = .;
+ *(.bootdatasym_rel)
+ __bootdatasym_rel_stop = .;
/*
* struct alt_inst entries. From the header (alternative.h):
* "Alternative instructions for different CPU types or capabilities"
@@ -291,6 +293,10 @@ SECTIONS
DECL_SECTION(.data) {
*(.data.page_aligned)
*(.data)
+ . = ALIGN(16);
+ __bootdata_start = .;
+ *(.data.boot16)
+ __bootdata_end = .;
*(.data.rel)
*(.data.rel.*)
CONSTRUCTORS
diff --git a/xen/include/asm-x86/edd.h b/xen/include/asm-x86/edd.h
index afaa23732a..a4d6b4d90e 100644
--- a/xen/include/asm-x86/edd.h
+++ b/xen/include/asm-x86/edd.h
@@ -143,7 +143,6 @@ struct __packed mbr_signature {
u32 signature;
};
-/* These all reside in the boot trampoline. Access via bootsym(). */
extern struct mbr_signature boot_mbr_signature[];
extern u8 boot_mbr_signature_nr;
extern struct edd_info boot_edd_info[];
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image
2019-08-09 15:02 ` [Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image David Woodhouse
@ 2019-08-12 10:24 ` Jan Beulich
2019-08-19 15:25 ` David Woodhouse
0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-08-12 10:24 UTC (permalink / raw)
To: David Woodhouse; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
On 09.08.2019 17:02, David Woodhouse wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -733,6 +733,17 @@ trampoline_setup:
> cmp $sym_offs(__bootsym_seg_stop),%edi
> jb 1b
>
> + /* Relocations for the boot data section. */
> + mov sym_fs(trampoline_phys),%edx
> + add $(boot_trampoline_end - boot_trampoline_start),%edx
> + mov $sym_offs(__bootdatasym_rel_start),%edi
> +1:
> + mov %fs:(%edi),%eax
> + add %edx,%fs:(%edi,%eax)
> + add $4,%edi
> + cmp $sym_offs(__bootdatasym_rel_stop),%edi
> + jb 1b
> +
> /* Do not parse command line on EFI platform here. */
> cmpb $0,sym_fs(efi_platform)
> jnz 1f
> @@ -770,6 +781,11 @@ trampoline_setup:
> mov $((boot_trampoline_end - boot_trampoline_start) / 4),%ecx
> rep movsl %fs:(%esi),%es:(%edi)
>
> + /* Copy boot data template to low memory. */
> + mov $sym_offs(bootdata_start),%esi
> + mov $((bootdata_end - bootdata_start + 3) / 4),%ecx
> + rep movsl %fs:(%esi),%es:(%edi)
The new data arrangement should be described in the commit message.
Also just like for the trampoline copying I think it would be better
if you suitable aligned bootdata_start and bootdata_end, such that
you wouldn't need to add 3 here before dividing by 4.
> @@ -227,7 +231,7 @@ start64:
> .word 0
> idt_48: .word 0, 0, 0 # base = limit = 0
> .word 0
> -gdt_48: .word 6*8-1
> +gdt_48: .word 7*8-1
> .long tramp32sym_rel(trampoline_gdt,4)
You don't grow trampoline_gdt here, so I think this change is
wrong. And if a change was needed at all (perhaps in the next
patch), then I think it would be better to replace the use of
literal numbers, using the difference of two labels instead
(the "end" lable preferably being a .L-prefixed one).
> --- a/xen/arch/x86/boot/video.S
> +++ b/xen/arch/x86/boot/video.S
> @@ -15,10 +15,10 @@
>
> #include "video.h"
>
> -/* Scratch space layout: boot_trampoline_end to boot_trampoline_end+0x1000. */
> -#define modelist bootsym(boot_trampoline_end) /* 2kB (256 entries) */
> -#define vesa_glob_info (modelist + 0x800) /* 1kB */
> -#define vesa_mode_info (vesa_glob_info + 0x400) /* 1kB */
> +/* Scratch space layout: bootdata_end to bootdata_end+0x1000. */
> +#define modelist(t) bootdatasym_rel(bootdata_end,2,t) /* 2KiB (256 entries) */
> +#define vesa_glob_info(t) bootdatasym_rel((bootdata_end+0x800),2,t) /* 1KiB */
> +#define vesa_mode_info(t) bootdatasym_rel((bootdata_end+0xc00),2,t) /* 1KiB */
>
> /* Retrieve Extended Display Identification Data. */
> #define CONFIG_FIRMWARE_EDID
> @@ -113,7 +113,7 @@ mopar2: movb %al, _param(PARAM_VIDEO_LINES)
>
> # Fetching of VESA frame buffer parameters
> mopar_gr:
> - leaw vesa_mode_info, %di
> + leaw vesa_mode_info(%di)
Just as a note, as I can't really see how to improve the situation:
The embedding of the relocation offset (2) in the macros is making
this code even more fragile, as they're now not usable anymore in
an arbitrary way (consider e.g. their use for the memory operand if
an insn which also requires an immediate). I think you want to at
least warn about this restriction in the comment above.
> @@ -291,6 +293,10 @@ SECTIONS
> DECL_SECTION(.data) {
> *(.data.page_aligned)
> *(.data)
> + . = ALIGN(16);
> + __bootdata_start = .;
> + *(.data.boot16)
> + __bootdata_end = .;
Why 16-byte alignment?
Having reached the end of the patch without seeing the C-level
bootsym() go away (and as a result noticing that you didn't remove
all uses) - could you please explain in the commit message what
the replacement (or not) criteria are?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image
2019-08-12 10:24 ` Jan Beulich
@ 2019-08-19 15:25 ` David Woodhouse
2019-08-27 8:59 ` Jan Beulich
0 siblings, 1 reply; 23+ messages in thread
From: David Woodhouse @ 2019-08-19 15:25 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
[-- Attachment #1.1: Type: text/plain, Size: 5274 bytes --]
On Mon, 2019-08-12 at 12:24 +0200, Jan Beulich wrote:
> On 09.08.2019 17:02, David Woodhouse wrote:
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -733,6 +733,17 @@ trampoline_setup:
> > cmp $sym_offs(__bootsym_seg_stop),%edi
> > jb 1b
> >
> > + /* Relocations for the boot data section. */
> > + mov sym_fs(trampoline_phys),%edx
> > + add $(boot_trampoline_end - boot_trampoline_start),%edx
> > + mov $sym_offs(__bootdatasym_rel_start),%edi
> > +1:
> > + mov %fs:(%edi),%eax
> > + add %edx,%fs:(%edi,%eax)
> > + add $4,%edi
> > + cmp $sym_offs(__bootdatasym_rel_stop),%edi
> > + jb 1b
> > +
> > /* Do not parse command line on EFI platform here. */
> > cmpb $0,sym_fs(efi_platform)
> > jnz 1f
> > @@ -770,6 +781,11 @@ trampoline_setup:
> > mov $((boot_trampoline_end - boot_trampoline_start) / 4),%ecx
> > rep movsl %fs:(%esi),%es:(%edi)
> >
> > + /* Copy boot data template to low memory. */
> > + mov $sym_offs(bootdata_start),%esi
> > + mov $((bootdata_end - bootdata_start + 3) / 4),%ecx
> > + rep movsl %fs:(%esi),%es:(%edi)
>
> The new data arrangement should be described in the commit message.
> Also just like for the trampoline copying I think it would be better
> if you suitable aligned bootdata_start and bootdata_end, such that
> you wouldn't need to add 3 here before dividing by 4.
Ack.
> > @@ -227,7 +231,7 @@ start64:
> > .word 0
> > idt_48: .word 0, 0, 0 # base = limit = 0
> > .word 0
> > -gdt_48: .word 6*8-1
> > +gdt_48: .word 7*8-1
> > .long tramp32sym_rel(trampoline_gdt,4)
>
> You don't grow trampoline_gdt here, so I think this change is
> wrong. And if a change was needed at all (perhaps in the next
> patch), then I think it would be better to replace the use of
> literal numbers, using the difference of two labels instead
> (the "end" lable preferably being a .L-prefixed one).
I don't grow it but... count it ☺.
I do start using sym_fs() here in places that it wasn't before, so the
incorrect size started to *matter* because the BOOT_FS selector wasn't
included in the limit.
I will make sure I explicitly comment on that in the commit message; no
need for a code comment to explain why the limit actually *does* match
the size of the table.
> > --- a/xen/arch/x86/boot/video.S
> > +++ b/xen/arch/x86/boot/video.S
> > @@ -15,10 +15,10 @@
> >
> > #include "video.h"
> >
> > -/* Scratch space layout: boot_trampoline_end to boot_trampoline_end+0x1000. */
> > -#define modelist bootsym(boot_trampoline_end) /* 2kB (256 entries) */
> > -#define vesa_glob_info (modelist + 0x800) /* 1kB */
> > -#define vesa_mode_info (vesa_glob_info + 0x400) /* 1kB */
> > +/* Scratch space layout: bootdata_end to bootdata_end+0x1000. */
> > +#define modelist(t) bootdatasym_rel(bootdata_end,2,t) /* 2KiB (256 entries) */
> > +#define vesa_glob_info(t) bootdatasym_rel((bootdata_end+0x800),2,t) /* 1KiB */
> > +#define vesa_mode_info(t) bootdatasym_rel((bootdata_end+0xc00),2,t) /* 1KiB */
> >
> > /* Retrieve Extended Display Identification Data. */
> > #define CONFIG_FIRMWARE_EDID
> > @@ -113,7 +113,7 @@ mopar2: movb %al, _param(PARAM_VIDEO_LINES)
> >
> > # Fetching of VESA frame buffer parameters
> > mopar_gr:
> > - leaw vesa_mode_info, %di
> > + leaw vesa_mode_info(%di)
>
> Just as a note, as I can't really see how to improve the situation:
> The embedding of the relocation offset (2) in the macros is making
> this code even more fragile, as they're now not usable anymore in
> an arbitrary way (consider e.g. their use for the memory operand if
> an insn which also requires an immediate). I think you want to at
> least warn about this restriction in the comment above.
Yeah. I file that one under "don't touch the VESA code unless you want
your brain to dribble out of your ears". Which was basically true
before I touched it too, in my defence ☺.
> > @@ -291,6 +293,10 @@ SECTIONS
> > DECL_SECTION(.data) {
> > *(.data.page_aligned)
> > *(.data)
> > + . = ALIGN(16);
> > + __bootdata_start = .;
> > + *(.data.boot16)
> > + __bootdata_end = .;
>
> Why 16-byte alignment?
Er... not sure. I think this (and the end) can be 4 as you suggest
elsewhere. Will make that change and retest.
> Having reached the end of the patch without seeing the C-level
> bootsym() go away (and as a result noticing that you didn't remove
> all uses) - could you please explain in the commit message what
> the replacement (or not) criteria are?
In the subsequent patch (6/6), bootsym() is indeed gone from C code,
and only trampsym() is left. The latter is for the permanent (not boot
time) trampoline used wakeup and for AP startup. As noted in the commit
message of that patch, the physical location of the Xen image isn't
mapped when those code paths run. So anything they need must be
relocated with them.
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image
2019-08-19 15:25 ` David Woodhouse
@ 2019-08-27 8:59 ` Jan Beulich
2019-08-27 9:19 ` David Woodhouse
0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-08-27 8:59 UTC (permalink / raw)
To: David Woodhouse; +Cc: Andrew Cooper, xen-devel, Wei Liu, Roger Pau Monné
On 19.08.2019 17:25, David Woodhouse wrote:
> On Mon, 2019-08-12 at 12:24 +0200, Jan Beulich wrote:
>> On 09.08.2019 17:02, David Woodhouse wrote:
>>> @@ -227,7 +231,7 @@ start64:
>>> .word 0
>>> idt_48: .word 0, 0, 0 # base = limit = 0
>>> .word 0
>>> -gdt_48: .word 6*8-1
>>> +gdt_48: .word 7*8-1
>>> .long tramp32sym_rel(trampoline_gdt,4)
>>
>> You don't grow trampoline_gdt here, so I think this change is
>> wrong. And if a change was needed at all (perhaps in the next
>> patch), then I think it would be better to replace the use of
>> literal numbers, using the difference of two labels instead
>> (the "end" lable preferably being a .L-prefixed one).
>
> I don't grow it but... count it ☺.
>
> I do start using sym_fs() here in places that it wasn't before, so the
> incorrect size started to *matter* because the BOOT_FS selector wasn't
> included in the limit.
Oh, I see - a (latent) bug introduced by b28044226e. Should perhaps
be a separate patch to fix this then (by, as suggested, using an
"end" label rather than hard coded numbers).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image
2019-08-27 8:59 ` Jan Beulich
@ 2019-08-27 9:19 ` David Woodhouse
0 siblings, 0 replies; 23+ messages in thread
From: David Woodhouse @ 2019-08-27 9:19 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, David Woodhouse, xen-devel, Wei Liu,
"Roger Pau Monné"
> On 19.08.2019 17:25, David Woodhouse wrote:
>> On Mon, 2019-08-12 at 12:24 +0200, Jan Beulich wrote:
>>> On 09.08.2019 17:02, David Woodhouse wrote:
>>>> @@ -227,7 +231,7 @@ start64:
>>>> .word 0
>>>> idt_48: .word 0, 0, 0 # base = limit = 0
>>>> .word 0
>>>> -gdt_48: .word 6*8-1
>>>> +gdt_48: .word 7*8-1
>>>> .long tramp32sym_rel(trampoline_gdt,4)
>>>
>>> You don't grow trampoline_gdt here, so I think this change is
>>> wrong. And if a change was needed at all (perhaps in the next
>>> patch), then I think it would be better to replace the use of
>>> literal numbers, using the difference of two labels instead
>>> (the "end" lable preferably being a .L-prefixed one).
>>
>> I don't grow it but... count it ☺.
>>
>> I do start using sym_fs() here in places that it wasn't before, so the
>> incorrect size started to *matter* because the BOOT_FS selector wasn't
>> included in the limit.
>
> Oh, I see - a (latent) bug introduced by b28044226e. Should perhaps
> be a separate patch to fix this then (by, as suggested, using an
> "end" label rather than hard coded numbers).
Indeed. Andrew already posted a patch for that, which (along with his
others) I have rebased on top of my tree at
https://xenbits.xen.org/gitweb/?p=people/dwmw2/xen.git;a=shortlog;h=refs/heads/bootcleanup-andy
--
dwmw2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Xen-devel] [PATCH v2 6/6] x86/boot: Do not use trampoline for no-real-mode boot paths
[not found] <cover.1565362089.git.dwmw@amazon.co.uk>
` (4 preceding siblings ...)
2019-08-09 15:02 ` [Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image David Woodhouse
@ 2019-08-09 15:02 ` David Woodhouse
2019-08-12 10:55 ` Jan Beulich
5 siblings, 1 reply; 23+ messages in thread
From: David Woodhouse @ 2019-08-09 15:02 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 20698 bytes --]
From: David Woodhouse <dwmw@amazon.co.uk>
Where booted from EFI or with no-real-mode, there is no need to stomp
on low memory with the 16-boot code. Instead, just go straight to
trampoline_protmode_entry() at its physical location within the Xen
image.
For now, the boot code (including the EFI loader path) still determines
what the trampoline_phys address should be. The trampoline is actually
relocated for that address and copied into low memory, from a
relocate_trampoline() call made from __start_xen().
For subsequent AP startup and wakeup, the 32-bit trampoline can't
trivially be used in-place as that region isn't mapped. So copy it
down to low memory too, having relocated it (again) to work from
there.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
xen/arch/x86/acpi/power.c | 6 +--
xen/arch/x86/boot/head.S | 67 ++++++++++++++++------------------
xen/arch/x86/boot/trampoline.S | 32 ++++++++++++----
xen/arch/x86/cpu/common.c | 2 +-
xen/arch/x86/cpu/intel.c | 2 +-
xen/arch/x86/efi/efi-boot.h | 31 ++--------------
xen/arch/x86/setup.c | 43 ++++++++++++++++++++--
xen/arch/x86/smpboot.c | 6 +--
xen/arch/x86/tboot.c | 6 +--
xen/arch/x86/x86_64/mm.c | 2 +-
xen/include/asm-x86/acpi.h | 2 +-
xen/include/asm-x86/config.h | 10 ++---
12 files changed, 118 insertions(+), 91 deletions(-)
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index aecc754fdb..d2a355429e 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -152,9 +152,9 @@ static void acpi_sleep_prepare(u32 state)
return;
if ( acpi_sinfo.vector_width == 32 )
- *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
+ *(uint32_t *)wakeup_vector_va = trampsym_phys(wakeup_start);
else
- *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
+ *(uint64_t *)wakeup_vector_va = trampsym_phys(wakeup_start);
}
static void acpi_sleep_post(u32 state) {}
@@ -388,7 +388,7 @@ static void tboot_sleep(u8 sleep_state)
g_tboot_shared->acpi_sinfo.wakeup_vector = acpi_sinfo.wakeup_vector;
g_tboot_shared->acpi_sinfo.vector_width = acpi_sinfo.vector_width;
g_tboot_shared->acpi_sinfo.kernel_s3_resume_vector =
- bootsym_phys(wakeup_start);
+ trampsym_phys(wakeup_start);
switch ( sleep_state )
{
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 104eb0eb3c..2268ec99ed 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -697,16 +697,23 @@ trampoline_setup:
lea __PAGE_HYPERVISOR+sym_esi(l1_identmap),%edi
mov %edi,sym_fs(l2_bootmap)
- /* Apply relocations to bootstrap trampoline. */
- mov sym_fs(trampoline_phys),%edx
- mov $sym_offs(__trampoline_rel_start),%edi
-1:
- mov %fs:(%edi),%eax
- add %edx,%fs:(%edi,%eax)
- add $4,%edi
- cmp $sym_offs(__trampoline_rel_stop),%edi
- jb 1b
+ /* Do not parse command line on EFI platform here. */
+ cmpb $0,sym_fs(efi_platform)
+ jnz 1f
+ /* Bail if there is no command line to parse. */
+ mov sym_fs(multiboot_ptr),%ebx
+ testl $MBI_CMDLINE,MB_flags(%ebx)
+ jz 1f
+
+ lea sym_esi(early_boot_opts),%eax
+ push %eax
+ pushl MB_cmdline(%ebx)
+ call cmdline_parse_early
+
+1:
+ /* Apply relocations to 32-bit trampoline for execution in place. */
+ lea sym_esi(perm_trampoline_start),%edx
mov $sym_offs(__trampoline32_rel_start),%edi
1:
mov %fs:(%edi),%eax
@@ -715,6 +722,21 @@ trampoline_setup:
cmp $sym_offs(__trampoline32_rel_stop),%edi
jb 1b
+ cmp $0,sym_esi(skip_realmode)
+ jz .Ldo_realmode
+
+ /* Go directly to trampoline_protmode_entry at its physical address */
+ lea trampoline_protmode_entry-__XEN_VIRT_START(%esi),%eax
+ pushl $BOOT_CS32
+ push %eax
+
+ /* EBX == 0 indicates we are the BP (Boot Processor). */
+ xor %ebx,%ebx
+ retl
+
+.Ldo_realmode:
+ /* Apply relocations to 16-bit boot code. */
+ mov sym_fs(trampoline_phys),%edx
mov $sym_offs(__bootsym_rel_start),%edi
1:
mov %fs:(%edi),%eax
@@ -744,35 +766,12 @@ trampoline_setup:
cmp $sym_offs(__bootdatasym_rel_stop),%edi
jb 1b
- /* Do not parse command line on EFI platform here. */
- cmpb $0,sym_fs(efi_platform)
- jnz 1f
-
- /* Bail if there is no command line to parse. */
- mov sym_fs(multiboot_ptr),%ebx
- testl $MBI_CMDLINE,MB_flags(%ebx)
- jz 1f
-
- lea sym_esi(early_boot_opts),%eax
- push %eax
- pushl MB_cmdline(%ebx)
- call cmdline_parse_early
-
-1:
/* Switch to low-memory stack which lives at the end of trampoline region. */
mov sym_fs(trampoline_phys),%edi
lea TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
- cmpb $0, sym_fs(skip_realmode)
- jz 1f
- /* If no-real-mode, jump straight to trampoline_protmode_entry */
- lea trampoline_protmode_entry-boot_trampoline_start(%edi),%eax
- /* EBX == 0 indicates we are the BP (Boot Processor). */
- xor %ebx,%ebx
- jmp 2f
-1:
+
/* Go via 16-bit code in trampoline_boot_cpu_entry */
lea trampoline_boot_cpu_entry-boot_trampoline_start(%edi),%eax
-2:
pushl $BOOT_CS32
push %eax
@@ -795,8 +794,6 @@ cmdline_parse_early:
reloc:
#include "reloc.S"
-ENTRY(boot_trampoline_start)
#include "trampoline.S"
-ENTRY(boot_trampoline_end)
#include "x86_64.S"
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index b5517a44bb..7b9ebb6172 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -60,7 +60,7 @@ GLOBAL(bootdata_start)
.popsection
#undef trampsym
-#define trampsym(s) ((s)-boot_trampoline_start)
+#define trampsym(s) ((s)-perm_trampoline_start)
#define trampsym_rel(sym, off, opnd...) \
trampsym(sym),##opnd; \
@@ -70,7 +70,7 @@ GLOBAL(bootdata_start)
.popsection
#undef tramp32sym
-#define tramp32sym(s) ((s)-boot_trampoline_start)
+#define tramp32sym(s) ((s)-perm_trampoline_start)
#define tramp32sym_rel(sym, off, opnd...) \
tramp32sym(sym),##opnd; \
@@ -83,6 +83,8 @@ GLOBAL(bootdata_start)
.code16
+ENTRY(perm_trampoline_start)
+
/*
* do_boot_cpu() programs the Startup-IPI to point here. Due to the SIPI
* format, the relocated entrypoint must be 4k aligned.
@@ -90,6 +92,7 @@ GLOBAL(bootdata_start)
* It is entered in Real Mode, with %cs = trampoline_realmode_entry >> 4 and
* %ip = 0.
*/
+
GLOBAL(trampoline_realmode_entry)
mov %cs,%ax
mov %ax,%ds
@@ -97,7 +100,7 @@ GLOBAL(trampoline_realmode_entry)
cld
cli
lidt trampsym(idt_48)
- lgdt trampsym(gdt_48)
+ lgdtl trampsym(gdt_48)
mov $1,%bl # EBX != 0 indicates we are an AP
xor %ax, %ax
inc %ax
@@ -236,11 +239,23 @@ gdt_48: .word 7*8-1
/* The first page of trampoline is permanent, the rest boot-time only. */
/* Reuse the boot trampoline on the 1st trampoline page as stack for wakeup. */
- .equ wakeup_stack, boot_trampoline_start + PAGE_SIZE
+ .equ wakeup_stack, perm_trampoline_start + PAGE_SIZE
.global wakeup_stack
+ENTRY(perm_trampoline_end)
+
/* From here on early boot only. */
+ENTRY(boot_trampoline_start)
+
+ .word 0
+boot16_idt:
+ .word 0, 0, 0 # base = limit = 0
+ .word 0
+boot16_gdt:
+ .word 7*8-1
+ .long tramp32sym_rel(trampoline_gdt,4)
+
.code32
trampoline_boot_cpu_entry:
/* Load pseudo-real-mode segments. */
@@ -304,8 +319,8 @@ trampoline_boot_cpu_entry:
cli
/* Reset GDT and IDT. Some BIOSes clobber GDTR. */
- lidt bootsym(idt_48)
- lgdt bootsym(gdt_48)
+ lidt bootsym(boot16_idt)
+ lgdtl bootsym(boot16_gdt)
/* Enter protected mode, and flush insn queue. */
xor %ax,%ax
@@ -343,7 +358,8 @@ trampoline_boot_cpu_entry:
xor %ebx,%ebx
/* Jump to the common bootstrap entry point. */
- jmp trampoline_protmode_entry
+ mov $tramp32sym_rel(trampoline_protmode_entry,4,%eax)
+ jmp *%eax
#include "video.h"
@@ -379,6 +395,8 @@ rm_idt: .word 256*4-1, 0, 0
#include "video.S"
#endif
+ENTRY(boot_trampoline_end)
+
.pushsection .data.boot16, "aw", @progbits
GLOBAL(bootdata_end)
.popsection
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 7478e21177..d202216739 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -389,7 +389,7 @@ static void generic_identify(struct cpuinfo_x86 *c)
&c->x86_capability[cpufeat_word(X86_FEATURE_LAHF_LM)],
&c->x86_capability[cpufeat_word(X86_FEATURE_SYSCALL)]);
if (c == &boot_cpu_data)
- bootsym(cpuid_ext_features) =
+ trampsym(cpuid_ext_features) =
c->x86_capability[cpufeat_word(X86_FEATURE_NX)];
if (c->extended_cpuid_level >= 0x80000004)
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index 5356a6ae10..1970eb1848 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -269,7 +269,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
MSR_IA32_MISC_ENABLE_XD_DISABLE);
if (disable) {
wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable & ~disable);
- bootsym(trampoline_misc_enable_off) |= disable;
+ trampsym(trampoline_misc_enable_off) |= disable;
}
if (disable & MSR_IA32_MISC_ENABLE_LIMIT_CPUID)
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index fc2ea554b5..e7418894a4 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -98,29 +98,6 @@ static void __init efi_arch_relocate_image(unsigned long delta)
}
}
-extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
-extern const s32 __trampoline32_rel_start[], __trampoline32_rel_stop[];
-
-static void __init relocate_trampoline(unsigned long phys)
-{
- const s32 *trampoline_ptr;
-
- trampoline_phys = phys;
-
- if ( !efi_enabled(EFI_LOADER) )
- return;
-
- /* Apply relocations to trampoline. */
- for ( trampoline_ptr = __trampoline_rel_start;
- trampoline_ptr < __trampoline_rel_stop;
- ++trampoline_ptr )
- *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
- for ( trampoline_ptr = __trampoline32_rel_start;
- trampoline_ptr < __trampoline32_rel_stop;
- ++trampoline_ptr )
- *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
-}
-
static void __init place_string(u32 *addr, const char *s)
{
char *alloc = NULL;
@@ -223,7 +200,7 @@ static void __init efi_arch_pre_exit_boot(void)
{
if ( !cfg.addr )
blexit(L"No memory for trampoline");
- relocate_trampoline(cfg.addr);
+ trampoline_phys = cfg.addr;
}
}
@@ -232,7 +209,6 @@ static void __init noreturn efi_arch_post_exit_boot(void)
u64 cr4 = XEN_MINIMAL_CR4 & ~X86_CR4_PGE, efer;
efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start);
- memcpy((void *)trampoline_phys, boot_trampoline_start, cfg.size);
/* Set system registers and transfer control. */
asm volatile("pushq $0\n\tpopfq");
@@ -566,14 +542,14 @@ static void __init efi_arch_memory_setup(void)
cfg.addr = 0x100000;
if ( efi_enabled(EFI_LOADER) )
- cfg.size = boot_trampoline_end - boot_trampoline_start;
+ cfg.size = perm_trampoline_end - perm_trampoline_start;
else
cfg.size = TRAMPOLINE_SPACE + TRAMPOLINE_STACK_SPACE;
status = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
PFN_UP(cfg.size), &cfg.addr);
if ( status == EFI_SUCCESS )
- relocate_trampoline(cfg.addr);
+ trampoline_phys = cfg.addr;
else
{
cfg.addr = 0;
@@ -665,7 +641,6 @@ static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
blexit(L"Xen must be loaded below 4Gb.");
if ( xen_phys_start & ((1 << L2_PAGETABLE_SHIFT) - 1) )
blexit(L"Xen must be loaded at a 2Mb boundary.");
- trampoline_xen_phys_start = xen_phys_start;
}
static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 101c9dd272..eb8bc7e33b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -682,6 +682,42 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
return n;
}
+extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
+extern const s32 __trampoline32_rel_start[], __trampoline32_rel_stop[];
+
+static void __init relocate_trampoline(unsigned long phys)
+{
+ const s32 *trampoline_ptr;
+ uint32_t tramp32_delta = 0;
+
+ /* Apply relocations to trampoline. */
+ for ( trampoline_ptr = __trampoline_rel_start;
+ trampoline_ptr < __trampoline_rel_stop;
+ ++trampoline_ptr )
+ *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
+
+ tramp32_delta = phys;
+ if (!efi_enabled(EFI_LOADER)) {
+ /*
+ * The non-EFI boot code uses the 32-bit trampoline in place
+ * so will have relocated it to the physical address of
+ * perm_trampoline_start already. Undo that as it needs to
+ * run from low memory for AP startup, because the Xen
+ * physical address range won't be mapped.
+ */
+ tramp32_delta -= trampoline_xen_phys_start;
+ tramp32_delta -= (unsigned long)(perm_trampoline_start - __XEN_VIRT_START);
+ }
+ for ( trampoline_ptr = __trampoline32_rel_start;
+ trampoline_ptr < __trampoline32_rel_stop;
+ ++trampoline_ptr )
+ *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += tramp32_delta;
+ trampoline_xen_phys_start = xen_phys_start;
+
+ memcpy(trampsym(perm_trampoline_start), perm_trampoline_start,
+ perm_trampoline_end - perm_trampoline_start);
+}
+
void __init noreturn __start_xen(unsigned long mbi_p)
{
char *memmap_type = NULL;
@@ -1076,7 +1112,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
/* Select relocation address. */
e = end - reloc_size;
xen_phys_start = e;
- bootsym(trampoline_xen_phys_start) = e;
/*
* No PTEs pointing above this address are candidates for relocation.
@@ -1509,6 +1544,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
else
end_boot_allocator();
+ relocate_trampoline(trampoline_phys);
+
system_state = SYS_STATE_boot;
/*
* No calls involving ACPI code should go between the setting of
@@ -1879,8 +1916,8 @@ int __hwdom_init xen_in_range(unsigned long mfn)
if ( !xen_regions[0].s )
{
/* S3 resume code (and other real mode trampoline code) */
- xen_regions[region_s3].s = bootsym_phys(boot_trampoline_start);
- xen_regions[region_s3].e = bootsym_phys(boot_trampoline_end);
+ xen_regions[region_s3].s = trampsym_phys(perm_trampoline_start);
+ xen_regions[region_s3].e = trampsym_phys(perm_trampoline_end);
/*
* This needs to remain in sync with the uses of the same symbols in
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 65e9ceeece..0410331a9e 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -47,7 +47,7 @@
#include <asm/tboot.h>
#include <mach_apic.h>
-#define setup_trampoline() (bootsym_phys(trampoline_realmode_entry))
+#define setup_trampoline() (trampsym_phys(trampoline_realmode_entry))
unsigned long __read_mostly trampoline_phys;
@@ -598,7 +598,7 @@ static int do_boot_cpu(int apicid, int cpu)
{
boot_error = 1;
smp_mb();
- if ( bootsym(trampoline_cpu_started) == 0xA5 )
+ if ( trampsym(trampoline_cpu_started) == 0xA5 )
/* trampoline started but...? */
printk("Stuck ??\n");
else
@@ -614,7 +614,7 @@ static int do_boot_cpu(int apicid, int cpu)
}
/* mark "stuck" area as not stuck */
- bootsym(trampoline_cpu_started) = 0;
+ trampsym(trampoline_cpu_started) = 0;
smp_mb();
return rc;
diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index 325d94d23a..daf19d346a 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -369,9 +369,9 @@ void tboot_shutdown(uint32_t shutdown_type)
*/
g_tboot_shared->num_mac_regions = 3;
/* S3 resume code (and other real mode trampoline code) */
- g_tboot_shared->mac_regions[0].start = bootsym_phys(boot_trampoline_start);
- g_tboot_shared->mac_regions[0].size = bootsym_phys(boot_trampoline_end) -
- bootsym_phys(boot_trampoline_start);
+ g_tboot_shared->mac_regions[0].start = trampsym_phys(perm_trampoline_start);
+ g_tboot_shared->mac_regions[0].size = trampsym_phys(perm_trampoline_end) -
+ trampsym_phys(perm_trampoline_start);
/* hypervisor .text + .rodata */
g_tboot_shared->mac_regions[1].start = (uint64_t)__pa(&_stext);
g_tboot_shared->mac_regions[1].size = __pa(&__2M_rodata_end) -
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 149cc4f7b5..05d476c423 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -697,7 +697,7 @@ void __init zap_low_mappings(void)
/* Replace with mapping of the boot trampoline only. */
map_pages_to_xen(trampoline_phys, maddr_to_mfn(trampoline_phys),
- PFN_UP(boot_trampoline_end - boot_trampoline_start),
+ PFN_UP(perm_trampoline_end - perm_trampoline_start),
__PAGE_HYPERVISOR);
}
diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
index 7032f3a001..8c5381aa47 100644
--- a/xen/include/asm-x86/acpi.h
+++ b/xen/include/asm-x86/acpi.h
@@ -106,7 +106,7 @@ extern int acpi_scan_nodes(u64 start, u64 end);
#define NR_NODE_MEMBLKS (MAX_NUMNODES*2)
extern struct acpi_sleep_info acpi_sinfo;
-#define acpi_video_flags bootsym(video_flags)
+#define acpi_video_flags trampsym(video_flags)
struct xenpf_enter_acpi_sleep;
extern int acpi_enter_sleep(struct xenpf_enter_acpi_sleep *sleep);
extern int acpi_enter_state(u32 state);
diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
index ada8c7b4f6..78634296b5 100644
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -89,12 +89,12 @@
#ifndef __ASSEMBLY__
extern unsigned long trampoline_phys;
-#define bootsym_phys(sym) \
- (((unsigned long)&(sym)-(unsigned long)&boot_trampoline_start)+trampoline_phys)
-#define bootsym(sym) \
+#define trampsym_phys(sym) \
+ (((unsigned long)&(sym)-(unsigned long)&perm_trampoline_start)+trampoline_phys)
+#define trampsym(sym) \
(*RELOC_HIDE((typeof(&(sym)))__va(__pa(&(sym))), \
- trampoline_phys-__pa(boot_trampoline_start)))
-extern char boot_trampoline_start[], boot_trampoline_end[];
+ trampoline_phys-__pa(perm_trampoline_start)))
+extern char perm_trampoline_start[], perm_trampoline_end[];
extern char trampoline_realmode_entry[];
extern unsigned int trampoline_xen_phys_start;
extern unsigned char trampoline_cpu_started;
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 6/6] x86/boot: Do not use trampoline for no-real-mode boot paths
2019-08-09 15:02 ` [Xen-devel] [PATCH v2 6/6] x86/boot: Do not use trampoline for no-real-mode boot paths David Woodhouse
@ 2019-08-12 10:55 ` Jan Beulich
2019-08-19 15:25 ` David Woodhouse
0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-08-12 10:55 UTC (permalink / raw)
To: David Woodhouse; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
On 09.08.2019 17:02, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> Where booted from EFI or with no-real-mode, there is no need to stomp
> on low memory with the 16-boot code. Instead, just go straight to
> trampoline_protmode_entry() at its physical location within the Xen
> image.
>
> For now, the boot code (including the EFI loader path) still determines
> what the trampoline_phys address should be. The trampoline is actually
> relocated for that address and copied into low memory, from a
> relocate_trampoline() call made from __start_xen().
I assume this talks about the real mode part of the trampoline, as
opposed to the next paragraph? Would be nice if you made this
explicit.
> For subsequent AP startup and wakeup, the 32-bit trampoline can't
> trivially be used in-place as that region isn't mapped. So copy it
> down to low memory too, having relocated it (again) to work from
> there.
trampoline_protmode_entry gets entered with CR0.PG=0, i.e. at
that point there's not even the question yet of there being a
mapping. Subsequently idle_pg_table gets loaded into CR3. I wonder
if, rather than relocating the 32-bit part of the trampoline, it
wouldn't be better to install a 1:1 mapping into idle_pg_table.
Such a mapping would need to have the G bits clear in order to
not conflict with PV guest mappings of the same linear addresses.
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -152,9 +152,9 @@ static void acpi_sleep_prepare(u32 state)
> return;
>
> if ( acpi_sinfo.vector_width == 32 )
> - *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> + *(uint32_t *)wakeup_vector_va = trampsym_phys(wakeup_start);
> else
> - *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> + *(uint64_t *)wakeup_vector_va = trampsym_phys(wakeup_start);
> }
>
> static void acpi_sleep_post(u32 state) {}
> @@ -388,7 +388,7 @@ static void tboot_sleep(u8 sleep_state)
> g_tboot_shared->acpi_sinfo.wakeup_vector = acpi_sinfo.wakeup_vector;
> g_tboot_shared->acpi_sinfo.vector_width = acpi_sinfo.vector_width;
> g_tboot_shared->acpi_sinfo.kernel_s3_resume_vector =
> - bootsym_phys(wakeup_start);
> + trampsym_phys(wakeup_start);
Shouldn't changes like these have happened earlier, when you
introduce the (logical only at that point) distinction between
trampoline pieces?
> @@ -97,7 +100,7 @@ GLOBAL(trampoline_realmode_entry)
> cld
> cli
> lidt trampsym(idt_48)
> - lgdt trampsym(gdt_48)
> + lgdtl trampsym(gdt_48)
Stray / unrelated change (and if needed, then also for lidt)?
> @@ -236,11 +239,23 @@ gdt_48: .word 7*8-1
>
> /* The first page of trampoline is permanent, the rest boot-time only. */
> /* Reuse the boot trampoline on the 1st trampoline page as stack for wakeup. */
> - .equ wakeup_stack, boot_trampoline_start + PAGE_SIZE
> + .equ wakeup_stack, perm_trampoline_start + PAGE_SIZE
> .global wakeup_stack
>
> +ENTRY(perm_trampoline_end)
> +
> /* From here on early boot only. */
>
> +ENTRY(boot_trampoline_start)
> +
> + .word 0
> +boot16_idt:
> + .word 0, 0, 0 # base = limit = 0
> + .word 0
> +boot16_gdt:
> + .word 7*8-1
> + .long tramp32sym_rel(trampoline_gdt,4)
Can we really not get away without a second copy of these?
> @@ -304,8 +319,8 @@ trampoline_boot_cpu_entry:
> cli
>
> /* Reset GDT and IDT. Some BIOSes clobber GDTR. */
> - lidt bootsym(idt_48)
> - lgdt bootsym(gdt_48)
> + lidt bootsym(boot16_idt)
> + lgdtl bootsym(boot16_gdt)
As above - either both should gain a suffix, or neither of them.
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -682,6 +682,42 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
> return n;
> }
>
> +extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
> +extern const s32 __trampoline32_rel_start[], __trampoline32_rel_stop[];
> +
> +static void __init relocate_trampoline(unsigned long phys)
> +{
> + const s32 *trampoline_ptr;
> + uint32_t tramp32_delta = 0;
> +
> + /* Apply relocations to trampoline. */
> + for ( trampoline_ptr = __trampoline_rel_start;
> + trampoline_ptr < __trampoline_rel_stop;
> + ++trampoline_ptr )
> + *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
> +
> + tramp32_delta = phys;
Any reason this can't be the initializer of the variable, or the
zero initializer above can't be dropped?
> + if (!efi_enabled(EFI_LOADER)) {
Style (missing blanks inside the parentheses, and brace to go on
its own line).
> --- a/xen/include/asm-x86/config.h
> +++ b/xen/include/asm-x86/config.h
> @@ -89,12 +89,12 @@
>
> #ifndef __ASSEMBLY__
> extern unsigned long trampoline_phys;
> -#define bootsym_phys(sym) \
> - (((unsigned long)&(sym)-(unsigned long)&boot_trampoline_start)+trampoline_phys)
> -#define bootsym(sym) \
> +#define trampsym_phys(sym) \
> + (((unsigned long)&(sym)-(unsigned long)&perm_trampoline_start)+trampoline_phys)
> +#define trampsym(sym) \
> (*RELOC_HIDE((typeof(&(sym)))__va(__pa(&(sym))), \
> - trampoline_phys-__pa(boot_trampoline_start)))
> -extern char boot_trampoline_start[], boot_trampoline_end[];
> + trampoline_phys-__pa(perm_trampoline_start)))
As you're touching these, could you please also insert the missing
blanks around the binary + and - ?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 6/6] x86/boot: Do not use trampoline for no-real-mode boot paths
2019-08-12 10:55 ` Jan Beulich
@ 2019-08-19 15:25 ` David Woodhouse
2019-08-27 9:07 ` Jan Beulich
0 siblings, 1 reply; 23+ messages in thread
From: David Woodhouse @ 2019-08-19 15:25 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
[-- Attachment #1.1: Type: text/plain, Size: 7923 bytes --]
On Mon, 2019-08-12 at 12:55 +0200, Jan Beulich wrote:
> On 09.08.2019 17:02, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> >
> > Where booted from EFI or with no-real-mode, there is no need to stomp
> > on low memory with the 16-boot code. Instead, just go straight to
> > trampoline_protmode_entry() at its physical location within the Xen
> > image.
> >
> > For now, the boot code (including the EFI loader path) still determines
> > what the trampoline_phys address should be. The trampoline is actually
> > relocated for that address and copied into low memory, from a
> > relocate_trampoline() call made from __start_xen().
>
> I assume this talks about the real mode part of the trampoline, as
> opposed to the next paragraph? Would be nice if you made this
> explicit.
This is the permanent real-mode trampoline used for AP startup and
wakeup, not the real-mode boot code (which the boot code has to have
put there for itself it it wanted it).
I will try to make the commit message clearer; thanks for pointing it
out.
> > For subsequent AP startup and wakeup, the 32-bit trampoline can't
> > trivially be used in-place as that region isn't mapped. So copy it
> > down to low memory too, having relocated it (again) to work from
> > there.
>
> trampoline_protmode_entry gets entered with CR0.PG=0, i.e. at
> that point there's not even the question yet of there being a
> mapping. Subsequently idle_pg_table gets loaded into CR3. I wonder
> if, rather than relocating the 32-bit part of the trampoline, it
> wouldn't be better to install a 1:1 mapping into idle_pg_table.
> Such a mapping would need to have the G bits clear in order to
> not conflict with PV guest mappings of the same linear addresses.
Yeah, I tried making that happen. It made me sad. This seemed to be
simpler and less fragile.
> > --- a/xen/arch/x86/acpi/power.c
> > +++ b/xen/arch/x86/acpi/power.c
> > @@ -152,9 +152,9 @@ static void acpi_sleep_prepare(u32 state)
> > return;
> >
> > if ( acpi_sinfo.vector_width == 32 )
> > - *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> > + *(uint32_t *)wakeup_vector_va = trampsym_phys(wakeup_start);
> > else
> > - *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> > + *(uint64_t *)wakeup_vector_va = trampsym_phys(wakeup_start);
> > }
> >
> > static void acpi_sleep_post(u32 state) {}
> > @@ -388,7 +388,7 @@ static void tboot_sleep(u8 sleep_state)
> > g_tboot_shared->acpi_sinfo.wakeup_vector = acpi_sinfo.wakeup_vector;
> > g_tboot_shared->acpi_sinfo.vector_width = acpi_sinfo.vector_width;
> > g_tboot_shared->acpi_sinfo.kernel_s3_resume_vector =
> > - bootsym_phys(wakeup_start);
> > + trampsym_phys(wakeup_start);
>
> Shouldn't changes like these have happened earlier, when you
> introduce the (logical only at that point) distinction between
> trampoline pieces?
That was in assembler code. This is C, which never had to be that
involved with the distinction. But now that all the dust has settled,
I'm making it consistent, using 'trampsym' for stuff in the permanent
trampoline just like the asm code does.
> > @@ -97,7 +100,7 @@ GLOBAL(trampoline_realmode_entry)
> > cld
> > cli
> > lidt trampsym(idt_48)
> > - lgdt trampsym(gdt_48)
> > + lgdtl trampsym(gdt_48)
>
> Stray / unrelated change (and if needed, then also for lidt)?
The difference between 16bit l.dt and 32-bit l.dtl is that the former
only loads 24 bits of the actual table address (trampoline_gdt in this
case).
Thus, when trampoline_gdt is being used in-place, as it is during early
boot, and *if* the Xen image is loaded higher than 16MiB, lgdt doesn't
work. That's half a day of my life I want back.
It doesn't matter for lidt because we're just loading an empty limit
and pointer there, and we don't care about bits 24-31 of a zero value.
> > @@ -236,11 +239,23 @@ gdt_48: .word 7*8-1
> >
> > /* The first page of trampoline is permanent, the rest boot-time only. */
> > /* Reuse the boot trampoline on the 1st trampoline page as stack for wakeup. */
> > - .equ wakeup_stack, boot_trampoline_start + PAGE_SIZE
> > + .equ wakeup_stack, perm_trampoline_start + PAGE_SIZE
> > .global wakeup_stack
> >
> > +ENTRY(perm_trampoline_end)
> > +
> > /* From here on early boot only. */
> >
> > +ENTRY(boot_trampoline_start)
> > +
> > + .word 0
> > +boot16_idt:
> > + .word 0, 0, 0 # base = limit = 0
> > + .word 0
> > +boot16_gdt:
> > + .word 7*8-1
> > + .long tramp32sym_rel(trampoline_gdt,4)
>
> Can we really not get away without a second copy of these?
Probably, but my judgement was that the complexity and the pain of
doing so would exceed the benefit. I'll take another look at doing so.
> > @@ -304,8 +319,8 @@ trampoline_boot_cpu_entry:
> > cli
> >
> > /* Reset GDT and IDT. Some BIOSes clobber GDTR. */
> > - lidt bootsym(idt_48)
> > - lgdt bootsym(gdt_48)
> > + lidt bootsym(boot16_idt)
> > + lgdtl bootsym(boot16_gdt)
>
> As above - either both should gain a suffix, or neither of them.
>
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -682,6 +682,42 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
> > return n;
> > }
> >
> > +extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
> > +extern const s32 __trampoline32_rel_start[], __trampoline32_rel_stop[];
> > +
> > +static void __init relocate_trampoline(unsigned long phys)
> > +{
> > + const s32 *trampoline_ptr;
> > + uint32_t tramp32_delta = 0;
> > +
> > + /* Apply relocations to trampoline. */
> > + for ( trampoline_ptr = __trampoline_rel_start;
> > + trampoline_ptr < __trampoline_rel_stop;
> > + ++trampoline_ptr )
> > + *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
> > +
> > + tramp32_delta = phys;
>
> Any reason this can't be the initializer of the variable, or the
> zero initializer above can't be dropped?
I can't think of one. I think I quite like the initial setting of
tramp32_delta=phys to live *right* above the subsequent if(something)
tramp32_delta-=something, to make it very clear what that calculation
is.
So maybe I'll just drop the pointless =0 initialiser.
> > + if (!efi_enabled(EFI_LOADER)) {
>
> Style (missing blanks inside the parentheses, and brace to go on
> its own line).
Ack. You can take the Linux hacker out of the Linux kernel but...
> > --- a/xen/include/asm-x86/config.h
> > +++ b/xen/include/asm-x86/config.h
> > @@ -89,12 +89,12 @@
> >
> > #ifndef __ASSEMBLY__
> > extern unsigned long trampoline_phys;
> > -#define bootsym_phys(sym) \
> > - (((unsigned long)&(sym)-(unsigned long)&boot_trampoline_start)+trampoline_phys)
> > -#define bootsym(sym) \
> > +#define trampsym_phys(sym) \
> > + (((unsigned long)&(sym)-(unsigned long)&perm_trampoline_start)+trampoline_phys)
> > +#define trampsym(sym) \
> > (*RELOC_HIDE((typeof(&(sym)))__va(__pa(&(sym))), \
> > - trampoline_phys-__pa(boot_trampoline_start)))
> > -extern char boot_trampoline_start[], boot_trampoline_end[];
> > + trampoline_phys-__pa(perm_trampoline_start)))
>
> As you're touching these, could you please also insert the missing
> blanks around the binary + and - ?
Will do. Thanks.
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Xen-devel] [PATCH v2 6/6] x86/boot: Do not use trampoline for no-real-mode boot paths
2019-08-19 15:25 ` David Woodhouse
@ 2019-08-27 9:07 ` Jan Beulich
2019-08-27 9:12 ` David Woodhouse
0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-08-27 9:07 UTC (permalink / raw)
To: David Woodhouse; +Cc: Andrew Cooper, xen-devel, Wei Liu, Roger Pau Monné
On 19.08.2019 17:25, David Woodhouse wrote:
> On Mon, 2019-08-12 at 12:55 +0200, Jan Beulich wrote:
>> On 09.08.2019 17:02, David Woodhouse wrote:
>>> @@ -97,7 +100,7 @@ GLOBAL(trampoline_realmode_entry)
>>> cld
>>> cli
>>> lidt trampsym(idt_48)
>>> - lgdt trampsym(gdt_48)
>>> + lgdtl trampsym(gdt_48)
>>
>> Stray / unrelated change (and if needed, then also for lidt)?
>
> The difference between 16bit l.dt and 32-bit l.dtl is that the former
> only loads 24 bits of the actual table address (trampoline_gdt in this
> case).
>
> Thus, when trampoline_gdt is being used in-place, as it is during early
> boot, and *if* the Xen image is loaded higher than 16MiB, lgdt doesn't
> work. That's half a day of my life I want back.
But isn't this an issue even independent of your series? I.e. doesn't
this want to be fixed in a separate (to be backported) patch?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v2 6/6] x86/boot: Do not use trampoline for no-real-mode boot paths
2019-08-27 9:07 ` Jan Beulich
@ 2019-08-27 9:12 ` David Woodhouse
0 siblings, 0 replies; 23+ messages in thread
From: David Woodhouse @ 2019-08-27 9:12 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, David Woodhouse, xen-devel, Wei Liu,
"Roger Pau Monné"
> On 19.08.2019 17:25, David Woodhouse wrote:
>> On Mon, 2019-08-12 at 12:55 +0200, Jan Beulich wrote:
>>> On 09.08.2019 17:02, David Woodhouse wrote:
>>>> @@ -97,7 +100,7 @@ GLOBAL(trampoline_realmode_entry)
>>>> cld
>>>> cli
>>>> lidt trampsym(idt_48)
>>>> - lgdt trampsym(gdt_48)
>>>> + lgdtl trampsym(gdt_48)
>>>
>>> Stray / unrelated change (and if needed, then also for lidt)?
>>
>> The difference between 16bit l.dt and 32-bit l.dtl is that the former
>> only loads 24 bits of the actual table address (trampoline_gdt in this
>> case).
>>
>> Thus, when trampoline_gdt is being used in-place, as it is during early
>> boot, and *if* the Xen image is loaded higher than 16MiB, lgdt doesn't
>> work. That's half a day of my life I want back.
>
> But isn't this an issue even independent of your series? I.e. doesn't
> this want to be fixed in a separate (to be backported) patch?
Before my series it isn't used in place in the Xen image; it's also
(mostly gratuitously) copied to low memory.
--
dwmw2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread