Hi Henry, On Thu, Apr 13, 2023 at 3:53 PM Henry Wang <henry.w...@arm.com> wrote: > > Hi Jens, > > > -----Original Message----- > > From: Jens Wiklander <jens.wiklan...@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.w...@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
