Re: [PATCH v5 09/10] arm_ffa: introduce FF-A MM communication

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Simon,

[...]

> > >
> > > >   * always begin with efi_mm_communicate_header.
> > > >   */
> > > > -struct __packed efi_mm_communicate_header {
> > > > +struct efi_mm_communicate_header {
> > > >     efi_guid_t header_guid;
> > > >     size_t     message_len;
> > > >     u8         data[];
> > > > @@ -145,7 +150,7 @@ struct smm_variable_communicate_header {
> > > >   * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE.
> > > >   *
> > > >   */
> > > > -struct smm_variable_access {
> > > > +struct __packed smm_variable_access {
> > >
> > > You are randomly adding and deleting __packed cwin both structs. But you can't do that.
> > > Those structs are defined in StandAloneMM.  This is the reason each struct
> > > description has the corresponding EDK2 definition.
> >
> > Thanks for the comment.
> >
> > However, we are not setting randomly the __packed keyword. There is a good reason for that.
> > It has been explained before in this reply [1]. Basically it's because of data padding issues
> > breaking the communication between u-boot and secure world (Optee).
> >
> > When upgrading Optee to v3.18, no issues anymore.
> >
> > The __packed changes have been dropped in patchset v6.
> >
> > [1]: https://lore.kernel.org/all/20220926105620.GA22382@e121910.cambridge.arm.com/
>
> That is the Linux mailing list. I cannot see any reason to add
> __packed to this struct as it is already set up that way.
>
> Also efi_mm_communicate_header is really long. I suggest efi_mm_hdr or
> efi_mm_comms_hdr
>
> Why are you using SMM on ARM? Isn't that an Intel thing?

This is due to the fact that we run StandAloneMM, which is a specific
EDK2 'package' to handle the variables in Arm secure world. There's a
really long explanation on how that works here [0] in case you are
interested.  The naming is a bit unfortunate, but since we have to
read it and reason about the EDK2 API in future versions, I suggest
keeping it as close to the original as possible.  Alternatively we
could just add a comment with the EDK2 equivalent, but in any case,
the change you are request is out of scope for this patchset.


[0] https://www.linaro.org/blog/protected-uefi-variables-with-u-boot/

Regards
/Ilias
>
> [..]
>
> Regards,
> SImon