Borislav Petkov <bp@xxxxxxxxx> writes: > On Thu, Sep 24, 2020 at 12:23:27PM -0500, Smita Koralahalli Channabasappa wrote: >> > Even though it's not defined in the UEFI spec, it doesn't mean a >> > structure definition cannot be created. > > Created for what? That structure better have a big fat comment above it, what > firmware generates its layout. Maybe I could've used a better choice of words - I meant to define a structure with meaningful member names to replace the *(ptr + i) accesses in the patch. The requirement for documenting the record layout doesn't change - whether using raw pointer arithmetic vs a structure definition. >> > After all, the patch is relying on some guarantee of the meaning of >> > the values and their ordering. > > AFAICT, this looks like an ad-hoc definition and the moment they change > it in some future revision, that struct of yours becomes invalid so we'd > need to add another one. If there's no spec backing the current layout, then it'll indeed be an ad-hoc definition of a structure in the kernel. But considering that it's part of firmware / OS interface for an important part of the RAS story I would hope that the code is based on a spec - having that reference included would help maintainability. Incompatible changes will indeed break the assumptions in the kernel and code will need to be updated - regardless of the choice of kernel implementation; pointer arithmetic, structure definition - ad-hoc or spec provided. Having versioning will allow running older kernels on newer hardware and vice versa - but I don't see why that is important only when using a structure based access. > >> > If the patch is relying on the definitions in the SMCA spec it is a good > > Yes, what SMCA spec is that? > >> > idea to reference it here - both for review and providing relevant >> > context for future developers. >> >> Okay, I agree the structure definition will make the code less arbitrary >> and provides relevant context compared to pointer arithmetic. I did not >> think this way. I can try this out if no objections. > > Again, this struct better have "versioning" info because the moment your > fw people change it in some future platform, this code needs touching > again. > > It probably would need touching even with the offsets if those offsets > change but at least not having it adhere to some slow-moving spec is > probably easier in case they wanna add/change fields. > > So Smita, you probably should talk to fw people about how stable that > layout at ctx_info + 1 is going to be wrt future platforms so that > we make sure we only access the correct offsets, now and on future > platforms. > > Thx.
