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

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

On Wed, May 14, 2025 at 03:55:51PM +0100, Andrew Cooper wrote:
> Please have at least a one-liner introduction to what TXT is.  Is there
> a stable URL for the TXT spec?  (I can't spot an obvious one, googling
> around)

In addition to a short definition I'll add:
 * 
https://www.intel.com/content/www/us/en/support/articles/000025873/processors.html
 * unversioned link to Software Development Guide
 * link to that unofficial archive (marked as such) mentioned by Krystian

> > +#ifndef ASM__X86__INTEL_TXT_H
> > +#define ASM__X86__INTEL_TXT_H
>
> I realise this is what the documentation currently says, but we're just
> in the process of finalising some changes.  Please make this
> X86_INTEL_TXT_H.

Will fix.

> Commit message needs to note that you're swapping NR_TXT_CONFIG_PAGES
> for NR_TXT_CONFIG_SIZE, hence the change in logic in
> tboot_protect_mem_regions().

Will clarify that.

> > +#ifndef __ASSEMBLY__
> > +
> > +/* Need to differentiate between pre- and post paging enabled. */
> > +#ifdef __EARLY_SLAUNCH__
> > +#include <xen/macros.h>
> > +#define _txt(x) _p(x)
> > +#else
> > +#include <xen/types.h>
> > +#include <asm/page.h>   // __va()
> > +#define _txt(x) __va(x)
> > +#endif
>
> I have to admit that I'm rather suspicious of this, but I'm going to
> have to wait to see later patches before making a suggestion.  (I highly
> suspect we'll want a fixmap entry).

Leaving it as is for now.

> > +/*
> > + * Always use private space as some of registers are either read-only or 
> > not
> > + * present in public space.
> > + */
> > +static inline uint64_t read_txt_reg(int reg_no)
>
> unsigned int reg

OK, for both read and write functions.

> I'd be tempted to name it simply txt_read().  I don't think the extra
> "_reg" is a helpful suffix, and it makes the APIs consistent with
> txt_reset().  But I expect others may have opinions here.

Will be renamed to txt_read() and txt_write(), seems sensible to me.

> > +static inline void write_txt_reg(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 */
> > +    (void)read_txt_reg(TXTCR_ESTS);
>
> What is this barrier for?
>
> It's not for anything in the CPU side of things, because UC accesses are
> strictly ordered.
>
> I presume it's for LPC bus ordering then, but making every write be a
> write/read pair seems very expensive.

The barrier will be moved to txt_reset() as suggested by Krystian in his
reply.

> > +static inline void txt_reset(uint32_t error)
> > +{
> > +    write_txt_reg(TXTCR_ERRORCODE, error);
> > +    write_txt_reg(TXTCR_CMD_NO_SECRETS, 1);
> > +    write_txt_reg(TXTCR_CMD_UNLOCK_MEM_CONFIG, 1);
> > +    write_txt_reg(TXTCR_CMD_RESET, 1);
> > +    while (1);
>
> for ( ;; )
>     cpu_relax();
>
> please.
>
> Will the write to CMD_RESET try to initiate a platform reset, or will we
> just hang here forever?

Will mark the function as `noreturn` and use `unreachable()` instead.

The write sends a platform into a warm reboot which retains the error
code for later examination.

> > +/*
> > + * Functions to extract data from the Intel TXT Heap Memory. The layout
> > + * of the heap is as follows:
>
> This is quite horrible, but I presume this is as specified by TXT?
>
> To confirm, it's a tightly packed data structure where each of the 4
> blobs is variable size?  Are there any alignment requirements on the
> table sizes?  I suspect not, given the __packed attributes.

Will improve the comment to state this explicitly, including that no
particular alignment is assumed and why (as in Krystian's reply).

> > diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
> > index d5db60d335..8a573d8c79 100644
> > --- a/xen/arch/x86/tboot.c
> > +++ b/xen/arch/x86/tboot.c
> > @@ -15,6 +15,7 @@
> >  #include <asm/tboot.h>
> >  #include <asm/setup.h>
> >  #include <asm/trampoline.h>
> > +#include <asm/intel-txt.h>
> >
> >  #include <crypto/vmac.h>
>
> I'll send a prep patch to fix tboot.c, but we're trying to keep includes
> sorted (for sanity of backports).
>
> ~Andrew

I've seen commits sorting includes and trying to do the same, but files
which don't have includes sorted yet often don't have any good place for
a new entry and I refrain from sorting them at the same time as I add a
new line.

Regards