Re: [PATCH v3 03/22] x86/boot: add MLE header and Secure Launch entry point

[Jan Beulich <jbeulich@xxxxxxxx>]

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