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