Re: [PATCH v3 01/22] x86/include/asm/intel-txt.h: constants and accessors for TXT registers and heap

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2025 17:57, Sergii Dmytruk wrote:
> On Wed, Jul 02, 2025 at 04:29:18PM +0200, Jan Beulich wrote:
>> Btw, a brief rev log would be nice here. I saw you have something in the
>> cover letter, but having to look in two places isn't very helpful.
> 
> I don't really know how to effectively maintain 23 logs at the same time
> given that changing one patch has cascading effects on the rest.  I'd
> suggest using `git diff-range` instead, commands for which I can include
> in cover letters for convenience.

Well, no, doing this per patch is possible and relevant. For cascading
effects their mentioning in a revlog can be pretty brief.

>>> +#define _txt(x) __va(x)
>>> +#endif
>>
>> Without any uses the correctness of the above is hard to judge.
> 
> The _txt() macro is used right below:

Well, the comment was meant a little indirectly: The correctness in
particular wrt the __EARLY_SLAUNCH__ distinction.

>>> +/*
>>> + * Always use private space as some of registers are either read-only or 
>>> not
>>> + * present in public space.
>>> + */
>>> +static inline uint64_t txt_read(unsigned int reg_no)
>>> +{
>>> +    volatile uint64_t *reg = _txt(TXT_PRIV_CONFIG_REGS_BASE + reg_no);
>>> +    return *reg;
>>> +}
>>> +
>>> +static inline void txt_write(unsigned int reg_no, uint64_t val)
>>> +{
>>> +    volatile uint64_t *reg = _txt(TXT_PRIV_CONFIG_REGS_BASE + reg_no);
>>> +    *reg = val;
>>> +}
> 
>>> +     * This serves as TXT register barrier after writing to
>>> +     * TXTCR_CMD_UNLOCK_MEM_CONFIG. Must be done to ensure that any future
>>> +     * chipset operations see the write.
>>> +     */
>>> +    (void)txt_read(TXTCR_ESTS);
>>
>> I don't think the cast is needed.
> 
> It's not needed, but I think that explicitly discarding unused return
> value is a generally good practice even when there is a comment.

In the context of Misra there has been discussion about doing so. But in our
present code base you will find such as the exception, not the rule.

>>> +    txt_write(TXTCR_CMD_RESET, 1);
>>> +    unreachable();
>>
>> What guarantees the write to take immediate effect? That is, shouldn't there
>> be e.g. an infinite loop here, just in case?
> 
> I'll return infinite loop from v2.  Tried adding `halt()` as Ross
> suggests, but including <asm/system.h> doesn't work in the early code
> (something about compat headers and missing expansion of things like
> __DeFiNe__).

Yeah, untangling that may be a little involved. Open-coding halt() is an
option, as long as you clearly indicate it as such (for e.g. grep to still
find that instance).

Jan