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

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

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.

> > +#include <asm/page.h>   // __va()
>
> Nit: No C++ style comments, please.

Sure.

> > +#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:

> > +/*
> > + * 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.

> > +    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__).

> > +static inline uint64_t txt_bios_data_size(const void *heap)
> > +{
> > +    return *(const uint64_t *)heap - sizeof(uint64_t);
>
> Like you already do here, ...
>
> > +}
> > +
> > +static inline void *txt_bios_data_start(const void *heap)
> > +{
> > +    return (void *)heap + sizeof(uint64_t);
>
> ... please don't cast away const-ness. I'm pretty sure I said before that
> Misra objects to us doing so.

Mind that you had left some comments on v2 after I sent v3.  v4 will
have this section rewritten using loops and constants, which resolves
issues with constness and readability.

> > @@ -409,7 +393,7 @@ int __init tboot_protect_mem_regions(void)
> >
> >      /* TXT Private Space */
> >      rc = e820_change_range_type(&e820, TXT_PRIV_CONFIG_REGS_BASE,
> > -                 TXT_PRIV_CONFIG_REGS_BASE + NR_TXT_CONFIG_PAGES * 
> > PAGE_SIZE,
> > +                 TXT_PRIV_CONFIG_REGS_BASE + NR_TXT_CONFIG_SIZE,
>
> Was this perhaps meant to be TXT_CONFIG_SPACE_SIZE?
>
> Jan

Right, thanks, building without tboot didn't catch this.

Regards