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

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

On Wed, Jul 02, 2025 at 04:36:27PM +0200, Jan Beulich wrote:
> On 30.05.2025 15:17, Sergii Dmytruk wrote:
> > The file provides constants, structures and several helper functions for
> > parsing SLRT.
> >
> > The data described by the structures is passed to Xen by a bootloader
> > which initiated DRTM.
> >
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Ross Philipson <ross.philipson@xxxxxxxxxx>
> > Signed-off-by: Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>
> > ---
> >  xen/include/xen/slr-table.h | 276 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 276 insertions(+)
> >  create mode 100644 xen/include/xen/slr-table.h
>
> Btw, please don't forget to Cc maintainers of code you're changing / adding.

What do you mean?  I'm running add_maintainers.pl on the patches.

> > +/*
> > + * Prototype of a function pointed to by slr_entry_dl_info::dl_handler.
> > + */
> > +typedef void (*dl_handler_func)(struct slr_bl_context *bl_context);
>
> I keep wondering why this ...
>
> > +/*
> > + * DRTM Dynamic Launch Configuration
> > + */
> > +struct slr_entry_dl_info
> > +{
> > +    struct slr_entry_hdr hdr;
> > +    uint64_t dce_size;
> > +    uint64_t dce_base;
> > +    uint64_t dlme_size;
> > +    uint64_t dlme_base;
> > +    uint64_t dlme_entry;
> > +    struct slr_bl_context bl_context;
> > +    uint64_t dl_handler;
>
> ... then isn't used right here, instead requiring a cast somewhere 
> (presumably,
> as code using this isn't visible in this patch).

As was mentioned earlier: because size of a pointer between Xen and a
bootloader doesn't necessarily match.  What you're proposing can break
under certain conditions.

> > +} __packed;
>
> I similarly keep wondering why there are all these packed attributes here, 
> when
> (afaics) all of the structures are defined in a way that any padding is 
> explicit
> anyway.

As before: it won't hurt, it's future-proof, just in case and to let
reader of the code know the structure must have no padding.  If you want
them gone, I can do that, but I think it will make the code harder to
maintain.

> > +static inline const struct slr_entry_hdr *
> > +slr_next_entry(const struct slr_table *table, const struct slr_entry_hdr 
> > *curr)
> > +{
> > +    const struct slr_entry_hdr *next = (void *)curr + curr->size;
> > +
> > +    if ( (void *)next + sizeof(*next) > slr_end_of_entries(table) )
> > +        return NULL;
> > +    if ( next->tag == SLR_ENTRY_END )
> > +        return NULL;
> > +    if ( (void *)next + next->size > slr_end_of_entries(table) )
> > +        return NULL;
> > +
> > +    return next;
> > +}
> > +
> > +static inline const struct slr_entry_hdr *
> > +slr_next_entry_by_tag(const struct slr_table *table,
> > +                      const struct slr_entry_hdr *entry,
> > +                      uint16_t tag)
> > +{
> > +    if ( !entry ) /* Start from the beginning */
> > +        entry = (void *)table + sizeof(*table);
> > +
> > +    for ( ; ; )
> > +    {
> > +        if ( entry->tag == tag )
> > +            return entry;
> > +
> > +        entry = slr_next_entry(table, entry);
> > +        if ( !entry )
> > +            return NULL;
> > +    }
> > +
> > +    return NULL;
> > +}
>
> For both of the functions, again: Please don't cast away const-ness.
>
> Jan

You had commented on v2 after v3 was already sent.

Regards