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

[Krystian Hebel <krystian.hebel@xxxxxxxxx>]

On 14.05.2025 16:55, 'Andrew Cooper' via trenchboot-devel wrote:

> On 13/05/2025 6:05 pm, Sergii Dmytruk wrote:
> > diff --git a/xen/arch/x86/include/asm/intel-txt.h b/xen/arch/x86/include/asm/intel-txt.h
> > new file mode 100644
> > index 0000000000..07be43f557
> > --- /dev/null
> > +++ b/xen/arch/x86/include/asm/intel-txt.h
> > @@ -0,0 +1,277 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> >       
> 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)
>     
    Not an official source and I don't know if it would be OK to use it
    here, but my go-to source is https://kib.kiev.ua/x86docs/Intel/TXT/,
    it even has previous versions, which may be helpful in understanding
    some of deprecated (e.g. related to TPM 1.2) intricacies of
    implementation.
> > +{
> > +    volatile uint64_t *reg = _txt(TXT_PRIV_CONFIG_REGS_BASE + reg_no);
> > +    return *reg;
> > +}
> > +
> > +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.
>     
    According to TXT spec, it is required only for TXT.CMD.CLOSE-PRIVATE
    and TXT.CMD.UNLOCK-MEM-CONFIG, and it probably could be changed to
    any
    other serializing instruction. IIRC, those registers are written to
    only in txt_reset(), so maybe adding a barrier there would suffice.
> > +/*
> > + * Functions to extract data from the Intel TXT Heap Memory. The layout
> > + * of the heap is as follows:
> > + *  +------------------------------------+
> > + *  | Size of Bios Data table (uint64_t) |
> > + *  +------------------------------------+
> > + *  | Bios Data table                    |
> > + *  +------------------------------------+
> > + *  | Size of OS MLE table (uint64_t)    |
> > + *  +------------------------------------+
> > + *  | OS MLE table                       |
> > + *  +--------------------------------    +
> > + *  | Size of OS SINIT table (uint64_t)  |
> > + *  +------------------------------------+
> > + *  | OS SINIT table                     |
> > + *  +------------------------------------+
> > + *  | Size of SINIT MLE table (uint64_t) |
> > + *  +------------------------------------+
> > + *  | SINIT MLE table                    |
> > + *  +------------------------------------+
> > + *
> > + *  NOTE: the table size fields include the 8 byte size field itself.
> > + */
> > +static inline uint64_t txt_bios_data_size(void *heap)
> > +{
> > +    return *((uint64_t *)heap) - sizeof(uint64_t);
> > +}
> >       
> This is quite horrible, but I presume this is as specified by TXT?
>     
    Yes.


> 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.
>     
    Yes, those are all of variable sizes. TXT specification notes that
    all
    of the sizes must be a multiple of 8 bytes, but we've seen platforms
    where BiosData (filled by BIOS ACM, so beyond our control) isn't
    aligned, which makes following fields also unaligned. Because of
    that,
    we treated it as if there was no such requirement.
-- 
Krystian Hebel
Firmware Engineer
https://3mdeb.com | @3mdeb_com