Re: [PATCH v5 04/10] hw/intc: GICv3 ITS Command processing

[shashi . mallela]

On Tue, 2021-07-06 at 11:27 +0200, Eric Auger wrote:
> Hi,
> 
> On 7/5/21 4:07 PM, Peter Maydell wrote:
> > On Wed, 30 Jun 2021 at 16:32, Shashi Mallela <
> > shashi.mallela@linaro.org> wrote:
> > > Added ITS command queue handling for MAPTI,MAPI commands,handled
> > > ITS
> > > translation which triggers an LPI via INT command as well as
> > > write
> > > to GITS_TRANSLATER register,defined enum to differentiate between
> > > ITS
> > > command interrupt trigger and GITS_TRANSLATER based interrupt
> > > trigger.
> > > Each of these commands make use of other functionalities
> > > implemented to
> > > get device table entry,collection table entry or interrupt
> > > translation
> > > table entry required for their processing.
> > > 
> > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> > > ---
> > >  hw/intc/arm_gicv3_its.c            | 361
> > > ++++++++++++++++++++++++++++-
> > >  hw/intc/gicv3_internal.h           |  26 +++
> > >  include/hw/intc/arm_gicv3_common.h |   2 +
> > >  3 files changed, 388 insertions(+), 1 deletion(-)
> > > +/*
> > > + * This function handles the processing of following commands
> > > based on
> > > + * the ItsCmdType parameter passed:-
> > > + * 1. trigerring of lpi interrupt translation via ITS INT
> > > command
> > > + * 2. trigerring of lpi interrupt translation via
> > > gits_translater register
> > > + * 3. handling of ITS CLEAR command
> > > + * 4. handling of ITS DISCARD command
> > > + */
> > 
> > "triggering"
> > 
> > >  #define DEVID_SHIFT                  32
> > >  #define DEVID_MASK                MAKE_64BIT_MASK(32, 32)
> > > @@ -347,6 +368,11 @@ FIELD(MAPC, RDBASE, 16, 32)
> > >   * vPEID = 16 bits
> > >   */
> > >  #define ITS_ITT_ENTRY_SIZE            0xC
> > > +#define ITE_ENTRY_INTTYPE_SHIFT        1
> > > +#define ITE_ENTRY_INTID_SHIFT          2
> > > +#define ITE_ENTRY_INTID_MASK         ((1ULL << 24) - 1)
> > > +#define ITE_ENTRY_INTSP_SHIFT          26
> > > +#define ITE_ENTRY_ICID_MASK          ((1ULL << 16) - 1)
> > 
> > This is still using a MASK value that's at the bottom of the
> > integer, not in its shifted location.
> There are other locations, pointed out by former comments, where this
> kind of unusual masking scheme is used but well...
Have taken care of masking scheme as desired in all relevant sections
in v6 patch
> 
> Thanks
> 
> Eric
> 
> > Otherwise
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > 
> > thanks
> > -- PMM
> >