diff mbox series

[XEN,v8,05/22] xen/arm: ffa: add flags for FFA_PARTITION_INFO_GET

Message ID 20230413071424.3273490-6-jens.wiklander@linaro.org (mailing list archive)
State New, archived
Headers show
Series Xen FF-A mediator | expand

Commit Message

Jens Wiklander April 13, 2023, 7:14 a.m. UTC
Defines flags used for the function FFA_PARTITION_INFO_GET.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Comments

Henry Wang April 13, 2023, 10:28 a.m. UTC | #1
Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 05/22] xen/arm: ffa: add flags for
> FFA_PARTITION_INFO_GET
> 
> Defines flags used for the function FFA_PARTITION_INFO_GET.

Nit: Similarly as my comment for patch #4, I would suggest that in
commit message you can mention the documentation number and
the chapter of FFA_PARTITION_INFO_GET. Something like:
"According to DEN0077A version 1.1 REL0, section 13.8, defines
flags used for the function FFA_PARTITION_INFO_GET"

> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  xen/arch/arm/tee/ffa.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index ba0942e76993..72e7d0575de5 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -57,6 +57,40 @@
>  #define FFA_MY_VERSION
> MAKE_FFA_VERSION(FFA_MY_VERSION_MAJOR, \
>                                                   FFA_MY_VERSION_MINOR)
> 
> +/*
> + * Flags to determine partition properties in FFA_PARTITION_INFO_GET
> return
> + * message:
> + * BIT(0): Supports receipt of direct requests
> + * BIT(1): Can send direct requests
> + * BIT(2): Can send and receive indirect messages
> + * BIT(3): Supports receipt of notifications
> + * BIT(4-5): Partition ID is a PE endpoint ID
> + * BIT(6): Partition must be informed about each VM that is created by
> + *         the Hypervisor
> + * BIT(7): Partition must be informed about each VM that is destroyed by
> + *         the Hypervisor
> + * BIT(8): Partition runs in the AArch64 execution state else AArch32
> + *         execution state
> + */
> +#define FFA_PART_PROP_DIRECT_REQ_RECV   BIT(0, U)
> +#define FFA_PART_PROP_DIRECT_REQ_SEND   BIT(1, U)
> +#define FFA_PART_PROP_INDIRECT_MSGS     BIT(2, U)
> +#define FFA_PART_PROP_RECV_NOTIF        BIT(3, U)
> +#define FFA_PART_PROP_IS_MASK           (3U << 4)

I am a bit confused here, here (3U<<4) is "IS_MASK" but...

> +#define FFA_PART_PROP_IS_PE_ID          (0U << 4)
> +#define FFA_PART_PROP_IS_SEPID_INDEP    (1U << 4)
> +#define FFA_PART_PROP_IS_SEPID_DEP      (2U << 4)
> +#define FFA_PART_PROP_IS_AUX_ID         (3U << 4)

...here the same value is used for "IS_AUX_ID". According to
the spec that I referred to, bit[5:4] has the following encoding:
b'11: Partition ID is an auxiliary ID. Hence I guess the above
"IS_MASK" should be removed?

I confirm the values of other fields are consistent with the spec.

Kind regards,
Henry
Jens Wiklander April 13, 2023, 1:45 p.m. UTC | #2
On Thu, Apr 13, 2023 at 12:28 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > Subject: [XEN PATCH v8 05/22] xen/arm: ffa: add flags for
> > FFA_PARTITION_INFO_GET
> >
> > Defines flags used for the function FFA_PARTITION_INFO_GET.
>
> Nit: Similarly as my comment for patch #4, I would suggest that in
> commit message you can mention the documentation number and
> the chapter of FFA_PARTITION_INFO_GET. Something like:
> "According to DEN0077A version 1.1 REL0, section 13.8, defines
> flags used for the function FFA_PARTITION_INFO_GET"

I'll add that.

>
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  xen/arch/arm/tee/ffa.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index ba0942e76993..72e7d0575de5 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -57,6 +57,40 @@
> >  #define FFA_MY_VERSION
> > MAKE_FFA_VERSION(FFA_MY_VERSION_MAJOR, \
> >                                                   FFA_MY_VERSION_MINOR)
> >
> > +/*
> > + * Flags to determine partition properties in FFA_PARTITION_INFO_GET
> > return
> > + * message:
> > + * BIT(0): Supports receipt of direct requests
> > + * BIT(1): Can send direct requests
> > + * BIT(2): Can send and receive indirect messages
> > + * BIT(3): Supports receipt of notifications
> > + * BIT(4-5): Partition ID is a PE endpoint ID
> > + * BIT(6): Partition must be informed about each VM that is created by
> > + *         the Hypervisor
> > + * BIT(7): Partition must be informed about each VM that is destroyed by
> > + *         the Hypervisor
> > + * BIT(8): Partition runs in the AArch64 execution state else AArch32
> > + *         execution state
> > + */
> > +#define FFA_PART_PROP_DIRECT_REQ_RECV   BIT(0, U)
> > +#define FFA_PART_PROP_DIRECT_REQ_SEND   BIT(1, U)
> > +#define FFA_PART_PROP_INDIRECT_MSGS     BIT(2, U)
> > +#define FFA_PART_PROP_RECV_NOTIF        BIT(3, U)
> > +#define FFA_PART_PROP_IS_MASK           (3U << 4)
>
> I am a bit confused here, here (3U<<4) is "IS_MASK" but...
>
> > +#define FFA_PART_PROP_IS_PE_ID          (0U << 4)
> > +#define FFA_PART_PROP_IS_SEPID_INDEP    (1U << 4)
> > +#define FFA_PART_PROP_IS_SEPID_DEP      (2U << 4)
> > +#define FFA_PART_PROP_IS_AUX_ID         (3U << 4)
>
> ...here the same value is used for "IS_AUX_ID". According to
> the spec that I referred to, bit[5:4] has the following encoding:
> b'11: Partition ID is an auxiliary ID. Hence I guess the above
> "IS_MASK" should be removed?

FFA_PART_PROP_IS_MASK is supposed to be used when extracting the bits
to compare with one of the other  FFA_PART_PROP_IS_* defines. For
example:
if ((props & FFA_PART_PROP_IS_MASK) == FFA_PART_PROP_IS_PE_ID)

using
if ((props & FFA_PART_PROP_IS_AUX_ID) == FFA_PART_PROP_IS_PE_ID)

doesn't seem right.

>
> I confirm the values of other fields are consistent with the spec.

Thanks,
Jens

>
> Kind regards,
> Henry
Henry Wang April 13, 2023, 1:53 p.m. UTC | #3
Hi Jens,

> -----Original Message-----
> From: Jens Wiklander <jens.wiklander@linaro.org>
> Subject: Re: [XEN PATCH v8 05/22] xen/arm: ffa: add flags for
> FFA_PARTITION_INFO_GET
> > > +#define FFA_PART_PROP_DIRECT_REQ_RECV   BIT(0, U)
> > > +#define FFA_PART_PROP_DIRECT_REQ_SEND   BIT(1, U)
> > > +#define FFA_PART_PROP_INDIRECT_MSGS     BIT(2, U)
> > > +#define FFA_PART_PROP_RECV_NOTIF        BIT(3, U)
> > > +#define FFA_PART_PROP_IS_MASK           (3U << 4)
> >
> > I am a bit confused here, here (3U<<4) is "IS_MASK" but...
> >
> > > +#define FFA_PART_PROP_IS_PE_ID          (0U << 4)
> > > +#define FFA_PART_PROP_IS_SEPID_INDEP    (1U << 4)
> > > +#define FFA_PART_PROP_IS_SEPID_DEP      (2U << 4)
> > > +#define FFA_PART_PROP_IS_AUX_ID         (3U << 4)
> >
> > ...here the same value is used for "IS_AUX_ID". According to
> > the spec that I referred to, bit[5:4] has the following encoding:
> > b'11: Partition ID is an auxiliary ID. Hence I guess the above
> > "IS_MASK" should be removed?
> 
> FFA_PART_PROP_IS_MASK is supposed to be used when extracting the bits
> to compare with one of the other  FFA_PART_PROP_IS_* defines. For
> example:
> if ((props & FFA_PART_PROP_IS_MASK) == FFA_PART_PROP_IS_PE_ID)

Ohh I now understand, the naming does not mean it "is a mask" but actually
means "this is a mask for FFA_PART_PROP_IS_". That makes a lot of sense.

To avoid this kind of ambiguity, do you think changing the name to something
like "FFA_PART_PROP_IS_TYPE_MASK" makes sense here? Note that this
is just my suggestion, you can decide to change or not, I am asking just
because I downloaded the whole series and found that currently
FFA_PART_PROP_IS_MASK is not used anywhere, so before it is used everywhere
in the code, it might be good to use a more clear name.

> 
> using
> if ((props & FFA_PART_PROP_IS_AUX_ID) == FFA_PART_PROP_IS_PE_ID)
> 
> doesn't seem right.

Indeed. Please see my above reply.

Personally after the above clarification, I am good with the patch, so:

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

> 
> >
> > I confirm the values of other fields are consistent with the spec.
> 
> Thanks,
> Jens
> 
> >
> > Kind regards,
> > Henry
Jens Wiklander April 14, 2023, 9:24 a.m. UTC | #4
Hi Henry,

On Thu, Apr 13, 2023 at 3:53 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > From: Jens Wiklander <jens.wiklander@linaro.org>
> > Subject: Re: [XEN PATCH v8 05/22] xen/arm: ffa: add flags for
> > FFA_PARTITION_INFO_GET
> > > > +#define FFA_PART_PROP_DIRECT_REQ_RECV   BIT(0, U)
> > > > +#define FFA_PART_PROP_DIRECT_REQ_SEND   BIT(1, U)
> > > > +#define FFA_PART_PROP_INDIRECT_MSGS     BIT(2, U)
> > > > +#define FFA_PART_PROP_RECV_NOTIF        BIT(3, U)
> > > > +#define FFA_PART_PROP_IS_MASK           (3U << 4)
> > >
> > > I am a bit confused here, here (3U<<4) is "IS_MASK" but...
> > >
> > > > +#define FFA_PART_PROP_IS_PE_ID          (0U << 4)
> > > > +#define FFA_PART_PROP_IS_SEPID_INDEP    (1U << 4)
> > > > +#define FFA_PART_PROP_IS_SEPID_DEP      (2U << 4)
> > > > +#define FFA_PART_PROP_IS_AUX_ID         (3U << 4)
> > >
> > > ...here the same value is used for "IS_AUX_ID". According to
> > > the spec that I referred to, bit[5:4] has the following encoding:
> > > b'11: Partition ID is an auxiliary ID. Hence I guess the above
> > > "IS_MASK" should be removed?
> >
> > FFA_PART_PROP_IS_MASK is supposed to be used when extracting the bits
> > to compare with one of the other  FFA_PART_PROP_IS_* defines. For
> > example:
> > if ((props & FFA_PART_PROP_IS_MASK) == FFA_PART_PROP_IS_PE_ID)
>
> Ohh I now understand, the naming does not mean it "is a mask" but actually
> means "this is a mask for FFA_PART_PROP_IS_". That makes a lot of sense.
>
> To avoid this kind of ambiguity, do you think changing the name to something
> like "FFA_PART_PROP_IS_TYPE_MASK" makes sense here? Note that this
> is just my suggestion, you can decide to change or not, I am asking just
> because I downloaded the whole series and found that currently
> FFA_PART_PROP_IS_MASK is not used anywhere, so before it is used everywhere
> in the code, it might be good to use a more clear name.
>
> >
> > using
> > if ((props & FFA_PART_PROP_IS_AUX_ID) == FFA_PART_PROP_IS_PE_ID)
> >
> > doesn't seem right.
>
> Indeed. Please see my above reply.
>
> Personally after the above clarification, I am good with the patch, so:
>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

I'll update it as you suggest.

Thanks,
Jens

>
> Kind regards,
> Henry
>
> >
> > >
> > > I confirm the values of other fields are consistent with the spec.
> >
> > Thanks,
> > Jens
> >
> > >
> > > Kind regards,
> > > Henry
diff mbox series

Patch

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index ba0942e76993..72e7d0575de5 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -57,6 +57,40 @@ 
 #define FFA_MY_VERSION          MAKE_FFA_VERSION(FFA_MY_VERSION_MAJOR, \
                                                  FFA_MY_VERSION_MINOR)
 
+/*
+ * Flags to determine partition properties in FFA_PARTITION_INFO_GET return
+ * message:
+ * BIT(0): Supports receipt of direct requests
+ * BIT(1): Can send direct requests
+ * BIT(2): Can send and receive indirect messages
+ * BIT(3): Supports receipt of notifications
+ * BIT(4-5): Partition ID is a PE endpoint ID
+ * BIT(6): Partition must be informed about each VM that is created by
+ *         the Hypervisor
+ * BIT(7): Partition must be informed about each VM that is destroyed by
+ *         the Hypervisor
+ * BIT(8): Partition runs in the AArch64 execution state else AArch32
+ *         execution state
+ */
+#define FFA_PART_PROP_DIRECT_REQ_RECV   BIT(0, U)
+#define FFA_PART_PROP_DIRECT_REQ_SEND   BIT(1, U)
+#define FFA_PART_PROP_INDIRECT_MSGS     BIT(2, U)
+#define FFA_PART_PROP_RECV_NOTIF        BIT(3, U)
+#define FFA_PART_PROP_IS_MASK           (3U << 4)
+#define FFA_PART_PROP_IS_PE_ID          (0U << 4)
+#define FFA_PART_PROP_IS_SEPID_INDEP    (1U << 4)
+#define FFA_PART_PROP_IS_SEPID_DEP      (2U << 4)
+#define FFA_PART_PROP_IS_AUX_ID         (3U << 4)
+#define FFA_PART_PROP_NOTIF_CREATED     BIT(6, U)
+#define FFA_PART_PROP_NOTIF_DESTROYED   BIT(7, U)
+#define FFA_PART_PROP_AARCH64_STATE     BIT(8, U)
+
+/*
+ * Flag used as parameter to FFA_PARTITION_INFO_GET to return partition
+ * count only.
+ */
+#define FFA_PARTITION_INFO_GET_COUNT_FLAG BIT(0, U)
+
 /* Function IDs */
 #define FFA_ERROR                       0x84000060U
 #define FFA_SUCCESS_32                  0x84000061U