Re: [PATCH v3 06/22] xen/arch/x86: reserve TXT memory during Slaunch

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

On Thu, Jul 10, 2025 at 03:00:07PM +0200, Jan Beulich wrote:
> On 30.05.2025 15:17, Sergii Dmytruk wrote:
> > --- a/xen/arch/x86/include/asm/mm.h
> > +++ b/xen/arch/x86/include/asm/mm.h
> > @@ -106,6 +106,9 @@
> >  #define _PGC_need_scrub   _PGC_allocated
> >  #define PGC_need_scrub    PGC_allocated
> >
> > +/* How much of the directmap is prebuilt at compile time. */
> > +#define PREBUILT_MAP_LIMIT (1 << L2_PAGETABLE_SHIFT)
>
> Better 1U or even 1UL?

Will change to 1UL.  L2_PAGETABLE_SHIFT is 21, so all variants are
essentially the same.

>From another email:
> Oh, also - I don't think mm.h is a good place for this. Please
> consider putting into setup.h.

Sure, mm.h just had a more suggestive name.

> > +/*
> > + * evt_log is assigned a physical address and the caller must map it to
> > + * virtual, if needed.
>
> In which case you want to use paddr_t, not void *.

Will change.

> > + */
> > +static inline void find_evt_log(const struct slr_table *slrt, void 
> > **evt_log,
> > +                                uint32_t *evt_log_size)
> > +{
> > +    const struct slr_entry_log_info *log_info;
> > +
> > +    log_info = (const struct slr_entry_log_info *)
> > +        slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_LOG_INFO);
>
> In situations like this please use the less type-unsafe container_of().
> (Apparently applies also to at least one earlier patch.)

Will update all places.

> > +int slaunch_map_l2(unsigned long paddr, unsigned long size);
>
> While largely benign on x86-64, maybe better paddr_t and size_t. And then ...

OK.

> > +static uint64_t __initdata txt_heap_base, txt_heap_size;
>
> ... why suddenly uint64_t here (and then elsewhere below)?

These have 64 bits allocated for them in TXT register space.
The spec only talks about bits 31:0 (unless its a typo), so I'll add a
comment about that and use `uint32_t`.

> > +/* Mark RAM region as RESERVED if it isn't marked that way already. */
> > +static int __init mark_ram_as(struct e820map *map, uint64_t start,
> > +                              uint64_t end, uint32_t type)
> > +{
> > ...
> > +
> > +    /* E820_ACPI or E820_NVS are really unexpected, but others are fine. */
> > +    if ( map->map[i].type == E820_RESERVED ||
> > +         map->map[i].type == E820_UNUSABLE )
>
> Are you sure about permitting UNUSABLE here?

Not really, I'll drop it.

> > +        from_type = map->map[i].type;
> > +
> > +    return e820_change_range_type(map, start, end, from_type, type);
>
> Even if this function, for historic reasons, also returns int/0/1, please make
> new code with boolean results return bool/false/true.

OK.

> > +void __init txt_reserve_mem_regions(void)
> > +{
> > +    int rc;
> > +    uint64_t sinit_base, sinit_size;
> > +
> > +    /* TXT Heap */
> > +    BUG_ON(txt_heap_base == 0);
> > +    printk("SLAUNCH: reserving TXT heap (%#lx - %#lx)\n", txt_heap_base,
> > +           txt_heap_base + txt_heap_size);
>
> Please log ranges in a way that makes it unambiguous whether they're exclusive
> or inclusive (especially at the upper end).

I'll use start:end notation which I think suggests inclusive bounds.

> > +    /* TXT Private Space */
> > +    rc = mark_ram_as(&e820_raw, TXT_PRIV_CONFIG_REGS_BASE,
> > +                     TXT_PRIV_CONFIG_REGS_BASE + TXT_CONFIG_SPACE_SIZE,
> > +                     E820_UNUSABLE);
>
> Why UNUSABLE? Then, if all callers used RESERVED, this wouldn't need to be
> a function arguments anymore, and you also wouldn't need to change RESERVED
> ranges.

I think it was suggested during some review as a way to prevent an OS
from using a range (reserved ones can still get used), but I'm not sure
it even works, so will revert to always reserve memory.

> > - * Launch boot.
> > + * Launch boot at any point.
>
> This comment adjustment should probably move to where the comment is being
> introduced.

Thanks, I probably forgot to do it.

> > +struct slr_table *__init slaunch_get_slrt(void)
> > +{
> > +    static struct slr_table *slrt;
>
> __initdata?

Right, will add.

> > +    if (slrt == NULL) {
>
> Nit: Style.

Will fix.

> > +            panic("SLRT has invalid magic value: %#08x!\n", slrt->magic);
>
> While %#x is indeed the prefered form to use, in particular when padding 
> that's
> not normally helpful, as the 0x prefix is included in the character count. And
> the value zero also ends up odd in that case, I think.

I constantly forget about prefix being included.  Won't specify width
here, it's not particularly useful in these cases.

> > +int __init slaunch_map_l2(unsigned long paddr, unsigned long size)
> > +{
> > +    unsigned long aligned_paddr = paddr & ~((1ULL << L2_PAGETABLE_SHIFT) - 
> > 1);
> > +    unsigned long pages = ((paddr + size) - aligned_paddr);
> > +    pages = ROUNDUP(pages, 1ULL << L2_PAGETABLE_SHIFT) >> PAGE_SHIFT;
>
> Nit: Blank line please between declaration(s) and statement(s).

OK.

> > +    if ( aligned_paddr + pages * PAGE_SIZE <= PREBUILT_MAP_LIMIT )
> > +        return 0;
> > +
> > +    if ( aligned_paddr < PREBUILT_MAP_LIMIT )
> > +    {
> > +        pages -= (PREBUILT_MAP_LIMIT - aligned_paddr) >> PAGE_SHIFT;
> > +        aligned_paddr = PREBUILT_MAP_LIMIT;
> > +    }
> > +
> > +    return map_pages_to_xen((uintptr_t)__va(aligned_paddr),
> > +                            maddr_to_mfn(aligned_paddr),
> > +                            pages, PAGE_HYPERVISOR);
> > +}
>
> What is being mapped here is (silently?) assumed to be below 4Gb? The
> function could anyway do with a brief comment saying what it's intended
> to do, and what assumptions it makes.
>
> It further looks as if you may be doing the same mapping multiple times,
> as you don't record what was already mapped.
>
> Jan

There is a large comment in slaunch.h which explains that we don't care
about unmapping because memory pages are being rebuilt after this
function is used.  No need to track what's being mapped for the same
reason.

DRTM data is below 4 GiB, will add BUG_ON() calls and update a comment
to state that.

Regards