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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Henry,

On Thu, Apr 13, 2023 at 3:53 PM Henry Wang <Henry.Wang@xxxxxxx> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > From: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> > 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@xxxxxxx>

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