From: Kacper Stojek <kacper.stojek@3mdeb.com>
Signed-off-by: Kacper Stojek <kacper.stojek@3mdeb.com>
Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>
Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
---
docs/hypervisor-guide/x86/how-xen-boots.rst | 5 ++
xen/arch/x86/boot/head.S | 53 +++++++++++++++++++++
2 files changed, 58 insertions(+)
diff --git a/docs/hypervisor-guide/x86/how-xen-boots.rst b/docs/hypervisor-guide/x86/how-xen-boots.rst
index 8b3229005c..050fe9c61f 100644
--- a/docs/hypervisor-guide/x86/how-xen-boots.rst
+++ b/docs/hypervisor-guide/x86/how-xen-boots.rst
@@ -55,6 +55,11 @@ If ``CONFIG_PVH_GUEST`` was selected at build time, an Elf note is included
which indicates the ability to use the PVH boot protocol, and registers
``__pvh_start`` as the entrypoint, entered in 32bit mode.
+A combination of Multiboot 2 and MLE headers is used to implement DRTM for
+legacy (BIOS) boot. The separate entry point is used mainly to differentiate
+from other kinds of boots. It moves a magic number to EAX before jumping into
+common startup code.
+
xen.gz
~~~~~~
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 77bb7a9e21..a69107bd81 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -4,6 +4,7 @@
#include <public/xen.h>
#include <asm/asm_defns.h>
#include <asm/fixmap.h>
+#include <asm/intel-txt.h>
#include <asm/page.h>
#include <asm/processor.h>
#include <asm/msr-index.h>
@@ -126,6 +127,25 @@ multiboot2_header:
.size multiboot2_header, . - multiboot2_header
.type multiboot2_header, @object
+ .balign 16
+mle_header:
+ .long 0x9082ac5a /* UUID0 */
+ .long 0x74a7476f /* UUID1 */
+ .long 0xa2555c0f /* UUID2 */
+ .long 0x42b651cb /* UUID3 */
+ .long 0x00000034 /* MLE header size */
+ .long 0x00020002 /* MLE version 2.2 */
+ .long (slaunch_stub_entry - start) /* Linear entry point of MLE (SINIT virt. address) */
+ .long 0x00000000 /* First valid page of MLE */
+ .long 0x00000000 /* Offset within binary of first byte of MLE */
+ .long (_end - start) /* Offset within binary of last byte + 1 of MLE */
+ .long 0x00000723 /* Bit vector of MLE-supported capabilities */
+ .long 0x00000000 /* Starting linear address of command line (unused) */
+ .long 0x00000000 /* Ending linear address of command line (unused) */
+
+ .size mle_header, .-mle_header
+ .type mle_header, @object
+
.section .init.rodata, "a", @progbits
.Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
@@ -332,6 +352,38 @@ cs32_switch:
/* Jump to earlier loaded address. */
jmp *%edi
+ /*
+ * Entry point for TrenchBoot Secure Launch on Intel TXT platforms.
+ *
+ * CPU is in 32b protected mode with paging disabled. On entry:
+ * - %ebx = %eip = MLE entry point,
+ * - stack pointer is undefined,
+ * - CS is flat 4GB code segment,
+ * - DS, ES, SS, FS and GS are undefined according to TXT SDG, but this
+ * would make it impossible to initialize GDTR, because GDT base must
+ * be relocated in the descriptor, which requires write access that
+ * CS doesn't provide. Instead we have to assume that DS is set by
+ * SINIT ACM as flat 4GB data segment.
+ *
+ * Additional restrictions:
+ * - some MSRs are partially cleared, among them IA32_MISC_ENABLE, so
+ * some capabilities might be reported as disabled even if they are
+ * supported by CPU
+ * - interrupts (including NMIs and SMIs) are disabled and must be
+ * enabled later
+ * - trying to enter real mode results in reset
+ * - APs must be brought up by MONITOR or GETSEC[WAKEUP], depending on
+ * which is supported by a given SINIT ACM
+ */
+slaunch_stub_entry:
+ /* Calculate the load base address. */
+ mov %ebx, %esi
+ sub $sym_offs(slaunch_stub_entry), %esi
+
+ /* Mark Secure Launch boot protocol and jump to common entry. */
+ mov $SLAUNCH_BOOTLOADER_MAGIC, %eax
+ jmp .Lset_stack
+
#ifdef CONFIG_PVH_GUEST
ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .long sym_offs(__pvh_start))
@@ -371,6 +423,7 @@ __start:
/* Restore the clobbered field. */
mov %edx, (%ebx)
+.Lset_stack:
/* Set up stack. */
lea STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp
--
2.49.0On 30.05.2025 15:17, Sergii Dmytruk wrote: > From: Kacper Stojek <kacper.stojek@3mdeb.com> > > Signed-off-by: Kacper Stojek <kacper.stojek@3mdeb.com> > Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com> > Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com> Such a change can hardly come without any description. As just one aspect, neither here nor ... > --- a/docs/hypervisor-guide/x86/how-xen-boots.rst > +++ b/docs/hypervisor-guide/x86/how-xen-boots.rst > @@ -55,6 +55,11 @@ If ``CONFIG_PVH_GUEST`` was selected at build time, an Elf note is included > which indicates the ability to use the PVH boot protocol, and registers > ``__pvh_start`` as the entrypoint, entered in 32bit mode. > > +A combination of Multiboot 2 and MLE headers is used to implement DRTM for > +legacy (BIOS) boot. The separate entry point is used mainly to differentiate ... here the MLE acronym is being deciphered. Same for DRTM here. There's also no reference anywhere as to some kind of spec (except in the cover letter, but that won't land in the tree). > +from other kinds of boots. It moves a magic number to EAX before jumping into > +common startup code. > + > > xen.gz > ~~~~~~ Any reason the single blank line is converted to a double one? Generally, in particular for patch context to be more meaningful, we'd prefer to not have double blank lines. In documentation they _sometimes_ may be warranted. > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -4,6 +4,7 @@ > #include <public/xen.h> > #include <asm/asm_defns.h> > #include <asm/fixmap.h> > +#include <asm/intel-txt.h> > #include <asm/page.h> > #include <asm/processor.h> > #include <asm/msr-index.h> > @@ -126,6 +127,25 @@ multiboot2_header: > .size multiboot2_header, . - multiboot2_header > .type multiboot2_header, @object > > + .balign 16 > +mle_header: > + .long 0x9082ac5a /* UUID0 */ > + .long 0x74a7476f /* UUID1 */ > + .long 0xa2555c0f /* UUID2 */ > + .long 0x42b651cb /* UUID3 */ > + .long 0x00000034 /* MLE header size */ Better use an expression (difference of two labels)? > + .long 0x00020002 /* MLE version 2.2 */ > + .long (slaunch_stub_entry - start) /* Linear entry point of MLE (SINIT virt. address) */ > + .long 0x00000000 /* First valid page of MLE */ > + .long 0x00000000 /* Offset within binary of first byte of MLE */ > + .long (_end - start) /* Offset within binary of last byte + 1 of MLE */ Is the data here describing xen.gz or (rather) xen.efi? In the latter case, does data past _end (in particular the .reloc section) not matter here? > + .long 0x00000723 /* Bit vector of MLE-supported capabilities */ > + .long 0x00000000 /* Starting linear address of command line (unused) */ > + .long 0x00000000 /* Ending linear address of command line (unused) */ > + > + .size mle_header, .-mle_header > + .type mle_header, @object Please use what xen/linkage.h provides now. However, the entire additions here and below likely want to go inside some #ifdef CONFIG_xyz, just like additions in subsequent patches. Which obviously would require a suitable Kconfig option to be introduced up front. > @@ -332,6 +352,38 @@ cs32_switch: > /* Jump to earlier loaded address. */ > jmp *%edi > > + /* > + * Entry point for TrenchBoot Secure Launch on Intel TXT platforms. > + * > + * CPU is in 32b protected mode with paging disabled. On entry: > + * - %ebx = %eip = MLE entry point, > + * - stack pointer is undefined, > + * - CS is flat 4GB code segment, > + * - DS, ES, SS, FS and GS are undefined according to TXT SDG, but this > + * would make it impossible to initialize GDTR, because GDT base must > + * be relocated in the descriptor, which requires write access that > + * CS doesn't provide. Instead we have to assume that DS is set by > + * SINIT ACM as flat 4GB data segment. Do you really _have to_? At least as plausibly SS might be properly set up, while DS might not be. > + * Additional restrictions: > + * - some MSRs are partially cleared, among them IA32_MISC_ENABLE, so > + * some capabilities might be reported as disabled even if they are > + * supported by CPU > + * - interrupts (including NMIs and SMIs) are disabled and must be > + * enabled later > + * - trying to enter real mode results in reset > + * - APs must be brought up by MONITOR or GETSEC[WAKEUP], depending on > + * which is supported by a given SINIT ACM I'm curious: How would MONITOR allow to bring up an AP? That's not even a memory access. > + */ > +slaunch_stub_entry: > + /* Calculate the load base address. */ > + mov %ebx, %esi > + sub $sym_offs(slaunch_stub_entry), %esi > + > + /* Mark Secure Launch boot protocol and jump to common entry. */ > + mov $SLAUNCH_BOOTLOADER_MAGIC, %eax While I understand you can't add real handling of this case just yet, wouldn't it be better to at least cover the case by checking for this magic later, and in that case enter, say, an infinite loop? You don't want to give the wrong impression of this path functioning, do you? Jan
On Thu, Jul 03, 2025 at 12:25:27PM +0200, Jan Beulich wrote: > On 30.05.2025 15:17, Sergii Dmytruk wrote: > > From: Kacper Stojek <kacper.stojek@3mdeb.com> > > > > Signed-off-by: Kacper Stojek <kacper.stojek@3mdeb.com> > > Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com> > > Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com> > > Such a change can hardly come without any description. As just one aspect, > neither here nor ... > > > --- a/docs/hypervisor-guide/x86/how-xen-boots.rst > > +++ b/docs/hypervisor-guide/x86/how-xen-boots.rst > > @@ -55,6 +55,11 @@ If ``CONFIG_PVH_GUEST`` was selected at build time, an Elf note is included > > which indicates the ability to use the PVH boot protocol, and registers > > ``__pvh_start`` as the entrypoint, entered in 32bit mode. > > > > +A combination of Multiboot 2 and MLE headers is used to implement DRTM for > > +legacy (BIOS) boot. The separate entry point is used mainly to differentiate > > ... here the MLE acronym is being deciphered. Same for DRTM here. There's > also no reference anywhere as to some kind of spec (except in the cover > letter, but that won't land in the tree). Will add more details. > > +from other kinds of boots. It moves a magic number to EAX before jumping into > > +common startup code. > > + > > > > xen.gz > > ~~~~~~ > > Any reason the single blank line is converted to a double one? Generally, in > particular for patch context to be more meaningful, we'd prefer to not have > double blank lines. In documentation they _sometimes_ may be warranted. Take a closer look, the patch just preserves double blank lines which are used consistently to separate sections within this file. > > --- a/xen/arch/x86/boot/head.S > > +++ b/xen/arch/x86/boot/head.S > > @@ -4,6 +4,7 @@ > > #include <public/xen.h> > > #include <asm/asm_defns.h> > > #include <asm/fixmap.h> > > +#include <asm/intel-txt.h> > > #include <asm/page.h> > > #include <asm/processor.h> > > #include <asm/msr-index.h> > > @@ -126,6 +127,25 @@ multiboot2_header: > > .size multiboot2_header, . - multiboot2_header > > .type multiboot2_header, @object > > > > + .balign 16 > > +mle_header: > > + .long 0x9082ac5a /* UUID0 */ > > + .long 0x74a7476f /* UUID1 */ > > + .long 0xa2555c0f /* UUID2 */ > > + .long 0x42b651cb /* UUID3 */ > > + .long 0x00000034 /* MLE header size */ > > Better use an expression (difference of two labels)? Won't hurt. > > + .long 0x00020002 /* MLE version 2.2 */ > > + .long (slaunch_stub_entry - start) /* Linear entry point of MLE (SINIT virt. address) */ > > + .long 0x00000000 /* First valid page of MLE */ > > + .long 0x00000000 /* Offset within binary of first byte of MLE */ > > + .long (_end - start) /* Offset within binary of last byte + 1 of MLE */ > > Is the data here describing xen.gz or (rather) xen.efi? In the latter case, > does data past _end (in particular the .reloc section) not matter here? Eventually, both. EFI case deals with loaded image which, I believe, should have all relocations applied at the time of measurement. > > + .long 0x00000723 /* Bit vector of MLE-supported capabilities */ > > + .long 0x00000000 /* Starting linear address of command line (unused) */ > > + .long 0x00000000 /* Ending linear address of command line (unused) */ > > + > > + .size mle_header, .-mle_header > > + .type mle_header, @object > > Please use what xen/linkage.h provides now. OK. > However, the entire additions here and below likely want to go inside some > #ifdef CONFIG_xyz, just like additions in subsequent patches. Which obviously > would require a suitable Kconfig option to be introduced up front. Will add CONFIG_SLAUNCH. > > @@ -332,6 +352,38 @@ cs32_switch: > > /* Jump to earlier loaded address. */ > > jmp *%edi > > > > + /* > > + * Entry point for TrenchBoot Secure Launch on Intel TXT platforms. > > + * > > + * CPU is in 32b protected mode with paging disabled. On entry: > > + * - %ebx = %eip = MLE entry point, > > + * - stack pointer is undefined, > > + * - CS is flat 4GB code segment, > > + * - DS, ES, SS, FS and GS are undefined according to TXT SDG, but this > > + * would make it impossible to initialize GDTR, because GDT base must > > + * be relocated in the descriptor, which requires write access that > > + * CS doesn't provide. Instead we have to assume that DS is set by > > + * SINIT ACM as flat 4GB data segment. > > Do you really _have to_? At least as plausibly SS might be properly set up, > while DS might not be. "have to" is referring to the fact that making this assumption is forced on the implementation. LGDT instruction uses DS in the code below, hence it's DS. > > + * Additional restrictions: > > + * - some MSRs are partially cleared, among them IA32_MISC_ENABLE, so > > + * some capabilities might be reported as disabled even if they are > > + * supported by CPU > > + * - interrupts (including NMIs and SMIs) are disabled and must be > > + * enabled later > > + * - trying to enter real mode results in reset > > + * - APs must be brought up by MONITOR or GETSEC[WAKEUP], depending on > > + * which is supported by a given SINIT ACM > > I'm curious: How would MONITOR allow to bring up an AP? That's not even a > memory access. See patch #15. BSP sets up TXT.MLE.JOIN and writes to an address monitored by APs, this causes APs to become part of dynamic launch by continuing execution at TXT-specific entry point. It's more of a redirection rather than waking up, just another case of bad terminology. > > + */ > > +slaunch_stub_entry: > > + /* Calculate the load base address. */ > > + mov %ebx, %esi > > + sub $sym_offs(slaunch_stub_entry), %esi > > + > > + /* Mark Secure Launch boot protocol and jump to common entry. */ > > + mov $SLAUNCH_BOOTLOADER_MAGIC, %eax > > While I understand you can't add real handling of this case just yet, wouldn't > it be better to at least cover the case by checking for this magic later, and > in that case enter, say, an infinite loop? You don't want to give the wrong > impression of this path functioning, do you? > > Jan Good point, I'll add an infinite loop. Regards
On 07.07.2025 23:54, Sergii Dmytruk wrote: > On Thu, Jul 03, 2025 at 12:25:27PM +0200, Jan Beulich wrote: >> On 30.05.2025 15:17, Sergii Dmytruk wrote: >>> From: Kacper Stojek <kacper.stojek@3mdeb.com> >>> >>> Signed-off-by: Kacper Stojek <kacper.stojek@3mdeb.com> >>> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com> >>> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com> >> >> Such a change can hardly come without any description. As just one aspect, >> neither here nor ... >> >>> --- a/docs/hypervisor-guide/x86/how-xen-boots.rst >>> +++ b/docs/hypervisor-guide/x86/how-xen-boots.rst >>> @@ -55,6 +55,11 @@ If ``CONFIG_PVH_GUEST`` was selected at build time, an Elf note is included >>> which indicates the ability to use the PVH boot protocol, and registers >>> ``__pvh_start`` as the entrypoint, entered in 32bit mode. >>> >>> +A combination of Multiboot 2 and MLE headers is used to implement DRTM for >>> +legacy (BIOS) boot. The separate entry point is used mainly to differentiate >> >> ... here the MLE acronym is being deciphered. Same for DRTM here. There's >> also no reference anywhere as to some kind of spec (except in the cover >> letter, but that won't land in the tree). > > Will add more details. > >>> +from other kinds of boots. It moves a magic number to EAX before jumping into >>> +common startup code. >>> + >>> >>> xen.gz >>> ~~~~~~ >> >> Any reason the single blank line is converted to a double one? Generally, in >> particular for patch context to be more meaningful, we'd prefer to not have >> double blank lines. In documentation they _sometimes_ may be warranted. > > Take a closer look, the patch just preserves double blank lines which > are used consistently to separate sections within this file. Oh, indeed. I'm sorry. >>> + .long 0x00020002 /* MLE version 2.2 */ >>> + .long (slaunch_stub_entry - start) /* Linear entry point of MLE (SINIT virt. address) */ >>> + .long 0x00000000 /* First valid page of MLE */ >>> + .long 0x00000000 /* Offset within binary of first byte of MLE */ >>> + .long (_end - start) /* Offset within binary of last byte + 1 of MLE */ >> >> Is the data here describing xen.gz or (rather) xen.efi? In the latter case, >> does data past _end (in particular the .reloc section) not matter here? > > Eventually, both. EFI case deals with loaded image which, I believe, > should have all relocations applied at the time of measurement. But you're aware of the need to apply relocations a 2nd time? See efi_arch_relocate_image(), which reads .reloc contents. Hence I assume that section needs to be included in any measurements. >>> @@ -332,6 +352,38 @@ cs32_switch: >>> /* Jump to earlier loaded address. */ >>> jmp *%edi >>> >>> + /* >>> + * Entry point for TrenchBoot Secure Launch on Intel TXT platforms. >>> + * >>> + * CPU is in 32b protected mode with paging disabled. On entry: >>> + * - %ebx = %eip = MLE entry point, >>> + * - stack pointer is undefined, >>> + * - CS is flat 4GB code segment, >>> + * - DS, ES, SS, FS and GS are undefined according to TXT SDG, but this >>> + * would make it impossible to initialize GDTR, because GDT base must >>> + * be relocated in the descriptor, which requires write access that >>> + * CS doesn't provide. Instead we have to assume that DS is set by >>> + * SINIT ACM as flat 4GB data segment. >> >> Do you really _have to_? At least as plausibly SS might be properly set up, >> while DS might not be. > > "have to" is referring to the fact that making this assumption is forced > on the implementation. But that's not really true. The Xen bits could be changed if needed, e.g. ... > LGDT instruction uses DS in the code below, hence it's DS. ... these could be made use SS or even CS. >>> + * Additional restrictions: >>> + * - some MSRs are partially cleared, among them IA32_MISC_ENABLE, so >>> + * some capabilities might be reported as disabled even if they are >>> + * supported by CPU >>> + * - interrupts (including NMIs and SMIs) are disabled and must be >>> + * enabled later >>> + * - trying to enter real mode results in reset >>> + * - APs must be brought up by MONITOR or GETSEC[WAKEUP], depending on >>> + * which is supported by a given SINIT ACM >> >> I'm curious: How would MONITOR allow to bring up an AP? That's not even a >> memory access. > > See patch #15. BSP sets up TXT.MLE.JOIN and writes to an address > monitored by APs, this causes APs to become part of dynamic launch by > continuing execution at TXT-specific entry point. It's more of a > redirection rather than waking up, just another case of bad terminology. Okay, (just ftaod) then my more general request: Please try to be as accurate as possible in comments (and similarly patch descriptions). "must be brought up by" is wording that I interpret to describe the action the "active" party (i.e. the BSP) needs to take. Whereas MONITOR, as you now clarify, is the action the AP needs to take (and then apparently is further required to check for false wakeups). Jan
On Tue, Jul 08, 2025 at 09:02:55AM +0200, Jan Beulich wrote: > >>> + .long 0x00020002 /* MLE version 2.2 */ > >>> + .long (slaunch_stub_entry - start) /* Linear entry point of MLE (SINIT virt. address) */ > >>> + .long 0x00000000 /* First valid page of MLE */ > >>> + .long 0x00000000 /* Offset within binary of first byte of MLE */ > >>> + .long (_end - start) /* Offset within binary of last byte + 1 of MLE */ > >> > >> Is the data here describing xen.gz or (rather) xen.efi? In the latter case, > >> does data past _end (in particular the .reloc section) not matter here? > > > > Eventually, both. EFI case deals with loaded image which, I believe, > > should have all relocations applied at the time of measurement. > > But you're aware of the need to apply relocations a 2nd time? See > efi_arch_relocate_image(), which reads .reloc contents. Hence I assume > that section needs to be included in any measurements. Checked map files and you're right, `__base_relocs_end` goes after `_end`. Will update, thanks. > >>> + * - DS, ES, SS, FS and GS are undefined according to TXT SDG, but this > >>> + * would make it impossible to initialize GDTR, because GDT base must > >>> + * be relocated in the descriptor, which requires write access that > >>> + * CS doesn't provide. Instead we have to assume that DS is set by > >>> + * SINIT ACM as flat 4GB data segment. > >> > >> Do you really _have to_? At least as plausibly SS might be properly set up, > >> while DS might not be. > > > > "have to" is referring to the fact that making this assumption is forced > > on the implementation. > > But that's not really true. The Xen bits could be changed if needed, e.g. ... > > > LGDT instruction uses DS in the code below, hence it's DS. > > ... these could be made use SS or even CS. SS can be used, but is it really any better than DS? CS can be used for LGDT but it won't work for modifying base address because code segments are read-only. Or do you mean that the comment should be made more precise? > >>> + * Additional restrictions: > >>> + * - some MSRs are partially cleared, among them IA32_MISC_ENABLE, so > >>> + * some capabilities might be reported as disabled even if they are > >>> + * supported by CPU > >>> + * - interrupts (including NMIs and SMIs) are disabled and must be > >>> + * enabled later > >>> + * - trying to enter real mode results in reset > >>> + * - APs must be brought up by MONITOR or GETSEC[WAKEUP], depending on > >>> + * which is supported by a given SINIT ACM > >> > >> I'm curious: How would MONITOR allow to bring up an AP? That's not even a > >> memory access. > > > > See patch #15. BSP sets up TXT.MLE.JOIN and writes to an address > > monitored by APs, this causes APs to become part of dynamic launch by > > continuing execution at TXT-specific entry point. It's more of a > > redirection rather than waking up, just another case of bad terminology. > > Okay, (just ftaod) then my more general request: Please try to be as accurate > as possible in comments (and similarly patch descriptions). "must be brought > up by" is wording that I interpret to describe the action the "active" party > (i.e. the BSP) needs to take. Whereas MONITOR, as you now clarify, is the > action the AP needs to take (and then apparently is further required to > check for false wakeups). > > Jan I'll try and in particular will correct this comment, but I might still miss things due to being used to them. In general when code is developed over the years by several people doing other projects in between, things just end up looking weird, so please bear with me. Regards
On 13.07.2025 19:51, Sergii Dmytruk wrote: > On Tue, Jul 08, 2025 at 09:02:55AM +0200, Jan Beulich wrote: >>>>> + * - DS, ES, SS, FS and GS are undefined according to TXT SDG, but this >>>>> + * would make it impossible to initialize GDTR, because GDT base must >>>>> + * be relocated in the descriptor, which requires write access that >>>>> + * CS doesn't provide. Instead we have to assume that DS is set by >>>>> + * SINIT ACM as flat 4GB data segment. >>>> >>>> Do you really _have to_? At least as plausibly SS might be properly set up, >>>> while DS might not be. >>> >>> "have to" is referring to the fact that making this assumption is forced >>> on the implementation. >> >> But that's not really true. The Xen bits could be changed if needed, e.g. ... >> >>> LGDT instruction uses DS in the code below, hence it's DS. >> >> ... these could be made use SS or even CS. > > SS can be used, but is it really any better than DS? CS can be used for > LGDT but it won't work for modifying base address because code segments > are read-only. Or do you mean that the comment should be made more > precise? Exactly. I was specifically referring to you saying "have to". Which is fine to say when that's actually true. >>>>> + * Additional restrictions: >>>>> + * - some MSRs are partially cleared, among them IA32_MISC_ENABLE, so >>>>> + * some capabilities might be reported as disabled even if they are >>>>> + * supported by CPU >>>>> + * - interrupts (including NMIs and SMIs) are disabled and must be >>>>> + * enabled later >>>>> + * - trying to enter real mode results in reset >>>>> + * - APs must be brought up by MONITOR or GETSEC[WAKEUP], depending on >>>>> + * which is supported by a given SINIT ACM >>>> >>>> I'm curious: How would MONITOR allow to bring up an AP? That's not even a >>>> memory access. >>> >>> See patch #15. BSP sets up TXT.MLE.JOIN and writes to an address >>> monitored by APs, this causes APs to become part of dynamic launch by >>> continuing execution at TXT-specific entry point. It's more of a >>> redirection rather than waking up, just another case of bad terminology. >> >> Okay, (just ftaod) then my more general request: Please try to be as accurate >> as possible in comments (and similarly patch descriptions). "must be brought >> up by" is wording that I interpret to describe the action the "active" party >> (i.e. the BSP) needs to take. Whereas MONITOR, as you now clarify, is the >> action the AP needs to take (and then apparently is further required to >> check for false wakeups). > > I'll try and in particular will correct this comment, but I might still > miss things due to being used to them. In general when code is > developed over the years by several people doing other projects in > between, things just end up looking weird, so please bear with me. That I can surely understand. Still my expectation is that when one takes over code, everything is being looked at carefully. Much like a non-public review. After all once you submit such work publicly, you will be the one to "defend" that code, including all of the commentary. Jan
© 2016 - 2025 Red Hat, Inc.