Re: [PATCH v3 04/22] x86/boot/slaunch-early: implement early initialization

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

On Thu, Jul 03, 2025 at 12:50:39PM +0200, Jan Beulich wrote:
> As indicated in reply to patch 3 - imo all code additions here want to be
> under some CONFIG_xyz. I repeat this here, but I don't think I'll repeat it
> any further.

I'll add one.  In case this is problematic for some reason I want to
mention that in the new version I don't add #ifdef where there are
if-statements because I did:

    #ifdef CONFIG_SLAUNCH
    ...
    #else
    static const bool slaunch_active = false;
    #endif

and that's enough for the compiler to discard
`if (slaunch_active ...) { ... }`.

> > +        /*
> > +         * Prepare space for output parameter of slaunch_early_init(), 
> > which is
> > +         * a structure of two uint32_t fields.
> > +         */
> > +        sub     $8, %esp
>
> At the very least a textual reference to the struct type is needed here,
> to be able to find it. Better would be to have the size calculated into
> asm-offsets.h, to use a proper symbolic name here.

Will do both of those things so it's easier to understand behaviour of
POPs.

> > +        push    %esp                             /* pointer to output 
> > structure */
> > +        lea     sym_offs(__2M_rwdata_end), %ecx  /* end of target image */
> > +        lea     sym_offs(_start), %edx           /* target base address */
>
> Why LEA when this can be expressed with (shorter) MOV?

I'll change to MOVs for consistency.  The reason is probably that these
are addresses and that's what LEA is for.

> > +        /* Move outputs of slaunch_early_init() from stack into registers. 
> > */
> > +        pop     %eax  /* physical MBI address */
> > +        pop     %edx  /* physical SLRT address */
> > +
> > +        /* Save physical address of SLRT for C code. */
> > +        mov     %edx, sym_esi(slaunch_slrt)
>
> Why go through %edx?
>
> > +        /* Store MBI address in EBX where MB2 code expects it. */
> > +        mov     %eax, %ebx
>
> Why go through %eax?

I think I just wanted to fully unpack the structure before processing
its fields, but there is real need for that, so I'll combine it.

> > +struct early_init_results
> > +{
> > +    uint32_t mbi_pa;
> > +    uint32_t slrt_pa;
> > +} __packed;
>
> Why __packed?

Just a bullet-proof form of documenting a requirement.

> > +void asmlinkage slaunch_early_init(uint32_t load_base_addr,
>
> __init ?

This is early code to which no such sections apply, as far as I can
tell.

> > +    slrt = (const struct slr_table *)(uintptr_t)os_mle->slrt;
>
> I think the cast to uintptr_t wants omitting here. This is 32-bit code, and
> hence the conversion to a pointer ought to go fine without. Or else you're
> silently discarding bits in the earlier assignment to ->slrt_pa.

`os_mle->slrt` is 64-bit and compiler dislikes implicit narrowing for
pointer casts, so can't just drop the cast.  I'll use `result->slrt_pa`
(32-bit) to get rid of the cast and will add a check that the address is
in fact 32-bit.

The values of pointers are generally below 4 GiB, so no harm is done.
The address fields are 64-bit probably for the extensibility and because
they are mostly consumed by 64-bit code.

> > +    intel_info = (const struct slr_entry_intel_info *)
> > +        slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_INTEL_INFO);
> > +    if ( intel_info == NULL || intel_info->hdr.size != sizeof(*intel_info) 
> > )
> > +        return;
>
> This size check is best effort only, isn't it? Or else how do you
> know ->hdr.size is actually within bounds?

It's just a sanity check the structure is not of completely wrong format.

> Further in txt_init() you use less-than checks; why more relaxed there
> and more strict here?

Those are different kinds of checks: here the code checks that an entry
in SLRT size matches Xen's structure in size, while txt_init() verifies
that a section of TXT heap is large enough to fit the data we expect to
find there.

> > --- a/xen/arch/x86/include/asm/intel-txt.h
> > +++ b/xen/arch/x86/include/asm/intel-txt.h
> > @@ -292,6 +292,22 @@ static inline void *txt_sinit_mle_data_start(const 
> > void *heap)
> >             sizeof(uint64_t);
> >  }
> >
> > +static inline void *txt_init(void)
>
> __init ?

Then it won't work in an early code.

> > +/*
> > + * Retrieves pointer to SLRT.  Checks table's validity and maps it as 
> > necessary.
> > + */
> > +struct slr_table *slaunch_get_slrt(void);
>
> There's no definition of this here, nor a use. Why is this living in this
> patch? Misra objects to declarations without definitions, and you want to
> be prepared that such a large series may go in piece by piece. Hence there
> may not be new Misra violations at any patch boundary.

The reason is that a comment mentions this function.  I'll change the
comment to not do that until the function is introduced.

> > +#include <xen/types.h>
>
> Looks like all you need here is xen/stdint.h?

Right, <xen/types.h> will be necessary for NULL in patch #6.

> > +#include <asm/slaunch.h>
>
> We try to move to there being blanks lines between groups of #include-s,
> e.g. all xen/ ones separated from all asm/ ones.

Will add a blank line.

> > +/*
> > + * These variables are assigned to by the code near Xen's entry point.
> > + *
> > + * slaunch_active is not __initdata to allow checking for an active Secure
> > + * Launch boot.
> > + */
> > +bool slaunch_active;
>
> Not using __initdata is quite plausible, but why not __ro_after_init?

I haven't tried it and likely didn't even see it (it's in a separate
header), will try changing.

> > +uint32_t __initdata slaunch_slrt; /* physical address */
> > +
> > +/* Using slaunch_active in head.S assumes it's a single byte in size, so 
> > enforce
> > + * this assumption. */
>
> Please follow comment style as per ./CODING_STYLE.
>
> Jan

Will adjust.

Regards