Re: [XEN PATCH v7 17/20] xen/arm: ffa: add ABI structs for sharing memory

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Fri, Mar 3, 2023 at 3:20 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > Adds the ABI structs used by function FFA_MEM_SHARE and friends for
> > sharing memory.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
>
> All the structures are coherent with the spec.

Thanks for double-checking.

>
> Just one small question after but independent if you choose or not to change 
> the names:
> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>
> Cheers
> Bertrand
>
> > ---
> > xen/arch/arm/tee/ffa.c | 74 ++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 74 insertions(+)
> >
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index bfd378f7fcd7..94c90b252454 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -197,6 +197,11 @@
> > #define FFA_MSG_SEND                    0x8400006EU
> > #define FFA_MSG_POLL                    0x8400006AU
> >
> > +/*
> > + * Structs below ending with _1_0 are defined in FF-A-1.0-REL and
> > + * struct ending with _1_1 are defined in FF-A-1.1-REL0.
> > + */
> > +
> > /* Partition information descriptor */
> > struct ffa_partition_info_1_0 {
> >     uint16_t id;
> > @@ -211,6 +216,75 @@ struct ffa_partition_info_1_1 {
> >     uint8_t uuid[16];
> > };
> >
> > +/* Constituent memory region descriptor */
> > +struct ffa_address_range {
> > +    uint64_t address;
> > +    uint32_t page_count;
> > +    uint32_t reserved;
> > +};
> > +
> > +/* Composite memory region descriptor */
> > +struct ffa_mem_region {
> > +    uint32_t total_page_count;
> > +    uint32_t address_range_count;
> > +    uint64_t reserved;
> > +    struct ffa_address_range address_range_array[];
> > +};
> > +
> > +/* Memory access permissions descriptor */
> > +struct ffa_mem_access_perm {
> > +    uint16_t endpoint_id;
> > +    uint8_t perm;
> > +    uint8_t flags;
> > +};
> > +
> > +/* Endpoint memory access descriptor */
> > +struct ffa_mem_access {
> > +    struct ffa_mem_access_perm access_perm;
> > +    uint32_t region_offs;
> > +    uint64_t reserved;
> > +};
> > +
> > +/* Lend, donate or share memory transaction descriptor */
> > +struct ffa_mem_transaction_1_0 {
> > +    uint16_t sender_id;
> > +    uint8_t mem_reg_attr;
> > +    uint8_t reserved0;
> > +    uint32_t flags;
> > +    uint64_t global_handle;
>
> Why global ? spec is just saying handle.
>
> > +    uint64_t tag;
> > +    uint32_t reserved1;
> > +    uint32_t mem_access_count;
> > +    struct ffa_mem_access mem_access_array[];
> > +};
> > +
> > +struct ffa_mem_transaction_1_1 {
> > +    uint16_t sender_id;
> > +    uint16_t mem_reg_attr;
> > +    uint32_t flags;
> > +    uint64_t global_handle;
>
> Same here

I'll change it.

Cheers,
Jens

>
> > +    uint64_t tag;
> > +    uint32_t mem_access_size;
> > +    uint32_t mem_access_count;
> > +    uint32_t mem_access_offs;
> > +    uint8_t reserved[12];
> > +};
> > +
> > +/* Endpoint RX/TX descriptor */
> > +struct ffa_endpoint_rxtx_descriptor_1_0 {
> > +    uint16_t sender_id;
> > +    uint16_t reserved;
> > +    uint32_t rx_range_count;
> > +    uint32_t tx_range_count;
> > +};
> > +
> > +struct ffa_endpoint_rxtx_descriptor_1_1 {
> > +    uint16_t sender_id;
> > +    uint16_t reserved;
> > +    uint32_t rx_region_offs;
> > +    uint32_t tx_region_offs;
> > +};
> > +
> > struct ffa_ctx {
> >     void *rx;
> >     const void *tx;
> > --
> > 2.34.1
> >
>