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
