Re: [PATCH v2 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

On Wed, May 21, 2025 at 05:45:04PM +0200, Jan Beulich wrote:
> > +/* SPDX-License-Identifier: GPL-2.0 */
>
> GPL-2.0-only is, I think, the one to use for new code.

Right.

> > +/*
> > + *  Copyright (c) 2025 Apertus Solutions, LLC
> > + *  Copyright (c) 2025 Oracle and/or its affiliates.
> > + *  Copyright (c) 2025 3mdeb Sp. z o.o
>
> I'm curious: Considering the (just) 2 S-o-b, where's the 3rd copyright
> line coming from?

I'll add "Daniel P. Smith" (already in CC), not sure why his S-o-B
wasn't there.

> > +#include <xen/types.h>
>
> Looks like xen/stdint.h would suffice?

It would for types, but there is also use of `NULL`.

> > +#define UEFI_SLR_TABLE_GUID \
> > +    { 0x877a9b2aU, 0x0385, 0x45d1, { 0xa0, 0x34, 0x9d, 0xac, 0x9c, 0x9e, 
> > 0x56, 0x5f } }
>
> I'm not sure this is a good place to put UEFI GUIDs. Considering e.g ...

It's here because the GUID is related more to SLRT than to EFI.  I can
move it if there is a more fitting place for table GUIDs.

> > +/* SLR table header values */
> > +#define SLR_TABLE_MAGIC         0x4452544d
> > +#define SLR_TABLE_REVISION      1
> > +
> > +/* Current revisions for the policy and UEFI config */
> > +#define SLR_POLICY_REVISION         1
> > +#define SLR_UEFI_CONFIG_REVISION    1
>
> ... this, is the whole concept perhaps bound to UEFI? In which casethe
> whole header may want to move to the efi/ subdir?

This isn't EFI-specific, legacy boot is supported.  Some types of
entries are there to provide EFI-specific information.

> > +/* SLR defined architectures */
> > +#define SLR_INTEL_TXT   1
> > +#define SLR_AMD_SKINIT  2
>
> These are both x86, yet the header is put in the common include dir?

It's x86-specific with the goal to add more architectures in the future.
I don't know, maybe the header should start as arch-specific and be
moved later, your call.

> > +/*
> > + * Primary SLR Table Header
> > + */
> > +struct slr_table
> > +{
> > +    uint32_t magic;
> > +    uint16_t revision;
> > +    uint16_t architecture;
> > +    uint32_t size;
> > +    uint32_t max_size;
> > +    /* entries[] */
> > +} __packed;
>
> If x86-specific, the question on the need for some of the __packed arises
> again.

The table is used to communicate data from pre-DRTM world to DRTM-world
and is produced and consumed by unrelated software components that don't
necessarily pad structures the same way by default.

> > +/*
> > + * Prototype of a function pointed to by slr_entry_dl_info::dl_handler.
> > + */
> > +typedef void (*dl_handler_func)(struct slr_bl_context *bl_context);
>
> It being an internal header, ...
> > +    uint64_t dl_handler;
>
> ... why can't this type be used here then? This would presumably avoid a
> typecast later.

It's not an internal header in my understanding of the phrase, Xen
parses what a bootloader has passed to it.  In principle, pointers could
be 32-bit here.

> > +static inline void *
> > +slr_end_of_entries(struct slr_table *table)
> > +{
> > +    return (uint8_t *)table + table->size;
>
> Considering the function's return type, why not cast to void * (or perhaps
> const void *, if the return type also can be such)?

No particular reason other than that pointer arithmetic on
pointers-to-void typically causes build issues.  Can be changed for Xen.

> > +static inline struct slr_entry_hdr *
> > +slr_next_entry(struct slr_table *table, struct slr_entry_hdr *curr)
> > +{
> > +    struct slr_entry_hdr *next = (struct slr_entry_hdr *)
> > +                                 ((uint8_t *)curr + curr->size);
> > +
> > +    if ( (void *)next >= slr_end_of_entries(table) )
> > +        return NULL;
>
> Is this sufficient as a check? With it fulfilled, ...
>
> > +    if ( next->tag == SLR_ENTRY_END )
>
> ... this member access may still be out of bounds. IOW the question is what
> level of checking is really adequate here.

SLR_ENTRY_END should really end the table, but it won't hurt to check
for out of bounds.  Thanks, will correct the checks.

> > +static inline struct slr_entry_hdr *
> > +slr_next_entry_by_tag (struct slr_table *table,
> > +                       struct slr_entry_hdr *entry,
> > +                       uint16_t tag)
> > +{
> > +    if ( !entry ) /* Start from the beginning */
> > +        entry = (struct slr_entry_hdr *)((uint8_t *)table + 
> > sizeof(*table));
>
> Extending from the earlier comment - if the inner cast was to void * here,
> the outer one could be dropped altogether.
>
> Jan

Will update.

Regards