| Message ID | 20210630153156.9421-5-shashi.mallela@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | GICv3 LPI and ITS feature implementation | expand |
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. Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
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> > --- > +static MemTxResult process_mapti(GICv3ITSState *s, uint64_t value, > + uint32_t offset, bool ignore_pInt) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint32_t devid, eventid; > + uint32_t pIntid = 0; > + uint32_t max_eventid, max_Intid; > + bool dte_valid; > + MemTxResult res = MEMTX_OK; > + uint16_t icid = 0; > + uint64_t dte = 0; > + IteEntry ite; > + uint32_t int_spurious = INTID_SPURIOUS; > + uint64_t idbits; > + > + devid = ((value & DEVID_MASK) >> DEVID_SHIFT); > + offset += NUM_BYTES_IN_DW; > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + eventid = (value & EVENTID_MASK); > + > + if (!ignore_pInt) { > + pIntid = ((value & pINTID_MASK) >> pINTID_SHIFT); > + } > + > + offset += NUM_BYTES_IN_DW; > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + icid = value & ICID_MASK; > + > + dte = get_dte(s, devid, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + dte_valid = dte & TABLE_ENTRY_VALID_MASK; > + > + max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1)); > + > + if (!ignore_pInt) { > + idbits = MIN(FIELD_EX64(s->gicv3->cpu->gicr_propbaser, GICR_PROPBASER, > + IDBITS), GICD_TYPER_IDBITS); I missed this the first time around, but I don't think this is right. Different CPUs could have different GICR_PROPBASER values, so checking against just one of them is wrong. The pseudocode only tests LPIOutOfRange() which is documented as testing "larger than GICD_TYPER.IDbits or not in the LPI range and not 1023". So I don't think we should be looking at the GICR_PROPBASER field here. More generally, "s->gicv3->cpu->something" is usually going to be wrong, because it is implicitly looking at CPU 0; often either there should be something else telling is which CPU to use (as in &s->gicv3->cpu[rdbase] where the CTE told us which redistributor), or we might need to operate on all CPUs/redistributors. The only exception is where we can guarantee that all the CPUs are the same (eg when looking at GICR_TYPER.PLPIS.) thanks -- PMM
On Mon, 2021-07-05 at 15:54 +0100, 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> > > --- > > +static MemTxResult process_mapti(GICv3ITSState *s, uint64_t value, > > + uint32_t offset, bool > > ignore_pInt) > > +{ > > + AddressSpace *as = &s->gicv3->dma_as; > > + uint32_t devid, eventid; > > + uint32_t pIntid = 0; > > + uint32_t max_eventid, max_Intid; > > + bool dte_valid; > > + MemTxResult res = MEMTX_OK; > > + uint16_t icid = 0; > > + uint64_t dte = 0; > > + IteEntry ite; > > + uint32_t int_spurious = INTID_SPURIOUS; > > + uint64_t idbits; > > + > > + devid = ((value & DEVID_MASK) >> DEVID_SHIFT); > > + offset += NUM_BYTES_IN_DW; > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > + MEMTXATTRS_UNSPECIFIED, &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + > > + eventid = (value & EVENTID_MASK); > > + > > + if (!ignore_pInt) { > > + pIntid = ((value & pINTID_MASK) >> pINTID_SHIFT); > > + } > > + > > + offset += NUM_BYTES_IN_DW; > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > + MEMTXATTRS_UNSPECIFIED, &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + > > + icid = value & ICID_MASK; > > + > > + dte = get_dte(s, devid, &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + dte_valid = dte & TABLE_ENTRY_VALID_MASK; > > + > > + max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1)); > > + > > + if (!ignore_pInt) { > > + idbits = MIN(FIELD_EX64(s->gicv3->cpu->gicr_propbaser, > > GICR_PROPBASER, > > + IDBITS), GICD_TYPER_IDBITS); > > I missed this the first time around, but I don't think this is right. > Different CPUs could have different GICR_PROPBASER values, so > checking > against just one of them is wrong. The pseudocode only tests > LPIOutOfRange() > which is documented as testing "larger than GICD_TYPER.IDbits or not > in > the LPI range and not 1023". So I don't think we should be looking > at the GICR_PROPBASER field here. > > More generally, "s->gicv3->cpu->something" is usually going to be > wrong, because it is implicitly looking at CPU 0; often either there > should be something else telling is which CPU to use (as in > &s->gicv3->cpu[rdbase] where the CTE told us which redistributor), > or we might need to operate on all CPUs/redistributors. The only > exception is where we can guarantee that all the CPUs are the same > (eg when looking at GICR_TYPER.PLPIS.) In that case,the validation of IDBITS(in case of ITS enabled) could be done during the write of gicr_propbaser register value itself(in arm_gicv3_redist.c) and the its command processing code here can just extract the idbits for its use. > > thanks > -- PMM
On Mon, 2021-07-05 at 20:47 -0400, shashi.mallela@linaro.org wrote: > On Mon, 2021-07-05 at 15:54 +0100, 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> > > > --- > > > +static MemTxResult process_mapti(GICv3ITSState *s, uint64_t > > > value, > > > + uint32_t offset, bool > > > ignore_pInt) > > > +{ > > > + AddressSpace *as = &s->gicv3->dma_as; > > > + uint32_t devid, eventid; > > > + uint32_t pIntid = 0; > > > + uint32_t max_eventid, max_Intid; > > > + bool dte_valid; > > > + MemTxResult res = MEMTX_OK; > > > + uint16_t icid = 0; > > > + uint64_t dte = 0; > > > + IteEntry ite; > > > + uint32_t int_spurious = INTID_SPURIOUS; > > > + uint64_t idbits; > > > + > > > + devid = ((value & DEVID_MASK) >> DEVID_SHIFT); > > > + offset += NUM_BYTES_IN_DW; > > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > > + MEMTXATTRS_UNSPECIFIED, &res); > > > + > > > + if (res != MEMTX_OK) { > > > + return res; > > > + } > > > + > > > + eventid = (value & EVENTID_MASK); > > > + > > > + if (!ignore_pInt) { > > > + pIntid = ((value & pINTID_MASK) >> pINTID_SHIFT); > > > + } > > > + > > > + offset += NUM_BYTES_IN_DW; > > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > > + MEMTXATTRS_UNSPECIFIED, &res); > > > + > > > + if (res != MEMTX_OK) { > > > + return res; > > > + } > > > + > > > + icid = value & ICID_MASK; > > > + > > > + dte = get_dte(s, devid, &res); > > > + > > > + if (res != MEMTX_OK) { > > > + return res; > > > + } > > > + dte_valid = dte & TABLE_ENTRY_VALID_MASK; > > > + > > > + max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1)); > > > + > > > + if (!ignore_pInt) { > > > + idbits = MIN(FIELD_EX64(s->gicv3->cpu->gicr_propbaser, > > > GICR_PROPBASER, > > > + IDBITS), GICD_TYPER_IDBITS); > > > > I missed this the first time around, but I don't think this is > > right. > > Different CPUs could have different GICR_PROPBASER values, so > > checking > > against just one of them is wrong. The pseudocode only tests > > LPIOutOfRange() > > which is documented as testing "larger than GICD_TYPER.IDbits or > > not > > in > > the LPI range and not 1023". So I don't think we should be looking > > at the GICR_PROPBASER field here. > > > > More generally, "s->gicv3->cpu->something" is usually going to be > > wrong, because it is implicitly looking at CPU 0; often either > > there > > should be something else telling is which CPU to use (as in > > &s->gicv3->cpu[rdbase] where the CTE told us which redistributor), > > or we might need to operate on all CPUs/redistributors. The only > > exception is where we can guarantee that all the CPUs are the same > > (eg when looking at GICR_TYPER.PLPIS.) > In that case,the validation of IDBITS(in case of ITS enabled) could > be > done during the write of gicr_propbaser register value itself(in > arm_gicv3_redist.c) and the its command processing code here can just > extract the idbits for its use. > > thanks > > -- PMM Hi Peter Please ignore my last comment. To address this scenario,i think the feasible option would be to call get_cte() to get the rdbase corresponding to icid value passed to mapti command.Since each icid is mapped to a rdbase(by virtue of calling MAPC command),if the collection table has a valid mapping for this icid we continue processing this MAPTI command using &s->gicv3->cpu[rdbase] applicable propbaser value to validate idbits, else return without further processing. Thanks Shashi
On Tue, 6 Jul 2021 at 04:25, <shashi.mallela@linaro.org> wrote: > > On Mon, 2021-07-05 at 20:47 -0400, shashi.mallela@linaro.org wrote: > > On Mon, 2021-07-05 at 15:54 +0100, Peter Maydell wrote: > > > I missed this the first time around, but I don't think this is > > > right. > > > Different CPUs could have different GICR_PROPBASER values, so > > > checking > > > against just one of them is wrong. The pseudocode only tests > > > LPIOutOfRange() > > > which is documented as testing "larger than GICD_TYPER.IDbits or > > > not > > > in > > > the LPI range and not 1023". So I don't think we should be looking > > > at the GICR_PROPBASER field here. > > > > > > More generally, "s->gicv3->cpu->something" is usually going to be > > > wrong, because it is implicitly looking at CPU 0; often either > > > there > > > should be something else telling is which CPU to use (as in > > > &s->gicv3->cpu[rdbase] where the CTE told us which redistributor), > > > or we might need to operate on all CPUs/redistributors. The only > > > exception is where we can guarantee that all the CPUs are the same > > > (eg when looking at GICR_TYPER.PLPIS.) > Please ignore my last comment. > > To address this scenario,i think the feasible option would be to call > get_cte() to get the rdbase corresponding to icid value passed to mapti > command.Since each icid is mapped to a rdbase(by virtue of calling MAPC > command),if the collection table has a valid mapping for this icid we > continue processing this MAPTI command using &s->gicv3->cpu[rdbase] > applicable propbaser value to validate idbits, else return without > further processing. But the pseudocode for MAPTI does not say anywhere that we should be checking the pIntID against any CPU's GICR_PROPBASER field. It is checked only by the checks in LPIOutOfRange(), which tests: * is it larger than permitted by GICD_TYPER.IDbits * is it not in the LPI range and not 1023 Checking whether the intID is too big and would cause us to index off the end of the redistributor's configuration table should be done later, only when the ITS actually sends the interrupt to a particular redistributor, I think. (You can't rely on the guest having done the MAPC before the MAPTI; and in any case the guest could choose to do a MAPC to a different redistributor after it's done the MAPTI.) thanks -- PMM
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... Thanks Eric > > Otherwise > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > thanks > -- PMM >
Hi Shashi, Peter, On 7/6/21 5:25 AM, shashi.mallela@linaro.org wrote: > On Mon, 2021-07-05 at 20:47 -0400, shashi.mallela@linaro.org wrote: >> On Mon, 2021-07-05 at 15:54 +0100, 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> >>>> --- >>>> +static MemTxResult process_mapti(GICv3ITSState *s, uint64_t >>>> value, >>>> + uint32_t offset, bool >>>> ignore_pInt) >>>> +{ >>>> + AddressSpace *as = &s->gicv3->dma_as; >>>> + uint32_t devid, eventid; >>>> + uint32_t pIntid = 0; >>>> + uint32_t max_eventid, max_Intid; >>>> + bool dte_valid; >>>> + MemTxResult res = MEMTX_OK; >>>> + uint16_t icid = 0; >>>> + uint64_t dte = 0; >>>> + IteEntry ite; >>>> + uint32_t int_spurious = INTID_SPURIOUS; >>>> + uint64_t idbits; >>>> + >>>> + devid = ((value & DEVID_MASK) >> DEVID_SHIFT); >>>> + offset += NUM_BYTES_IN_DW; >>>> + value = address_space_ldq_le(as, s->cq.base_addr + offset, >>>> + MEMTXATTRS_UNSPECIFIED, &res); >>>> + >>>> + if (res != MEMTX_OK) { >>>> + return res; >>>> + } >>>> + >>>> + eventid = (value & EVENTID_MASK); >>>> + >>>> + if (!ignore_pInt) { >>>> + pIntid = ((value & pINTID_MASK) >> pINTID_SHIFT); >>>> + } >>>> + >>>> + offset += NUM_BYTES_IN_DW; >>>> + value = address_space_ldq_le(as, s->cq.base_addr + offset, >>>> + MEMTXATTRS_UNSPECIFIED, &res); >>>> + >>>> + if (res != MEMTX_OK) { >>>> + return res; >>>> + } >>>> + >>>> + icid = value & ICID_MASK; >>>> + >>>> + dte = get_dte(s, devid, &res); >>>> + >>>> + if (res != MEMTX_OK) { >>>> + return res; >>>> + } >>>> + dte_valid = dte & TABLE_ENTRY_VALID_MASK; >>>> + >>>> + max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1)); >>>> + >>>> + if (!ignore_pInt) { >>>> + idbits = MIN(FIELD_EX64(s->gicv3->cpu->gicr_propbaser, >>>> GICR_PROPBASER, >>>> + IDBITS), GICD_TYPER_IDBITS); >>> >>> I missed this the first time around, but I don't think this is >>> right. >>> Different CPUs could have different GICR_PROPBASER values, so >>> checking >>> against just one of them is wrong. "5.1.1 LPI configuration tables" says " It is IMPLEMENTATION DEFINED whether GICR_PROPBASER can be set to different values on different Redistributors. GICR_TYPER.CommonLPIAff indicates which Redistributors must have GICR_PROPBASER set to the same value whenever GICR_CTLR.EnableLPIs == 1. " So we can choose to set CommonLPIAff to 0 if we do not need to emulate everything. This is what KVM does Thanks Eric The pseudocode only tests >>> LPIOutOfRange() >>> which is documented as testing "larger than GICD_TYPER.IDbits or >>> not >>> in >>> the LPI range and not 1023". So I don't think we should be looking >>> at the GICR_PROPBASER field here. >>> >>> More generally, "s->gicv3->cpu->something" is usually going to be >>> wrong, because it is implicitly looking at CPU 0; often either >>> there >>> should be something else telling is which CPU to use (as in >>> &s->gicv3->cpu[rdbase] where the CTE told us which redistributor), >>> or we might need to operate on all CPUs/redistributors. The only >>> exception is where we can guarantee that all the CPUs are the same >>> (eg when looking at GICR_TYPER.PLPIS.) >> In that case,the validation of IDBITS(in case of ITS enabled) could >> be >> done during the write of gicr_propbaser register value itself(in >> arm_gicv3_redist.c) and the its command processing code here can just >> extract the idbits for its use. >>> thanks >>> -- PMM > Hi Peter > > Please ignore my last comment. > > To address this scenario,i think the feasible option would be to call > get_cte() to get the rdbase corresponding to icid value passed to mapti > command.Since each icid is mapped to a rdbase(by virtue of calling MAPC > command),if the collection table has a valid mapping for this icid we > continue processing this MAPTI command using &s->gicv3->cpu[rdbase] > applicable propbaser value to validate idbits, else return without > further processing. > > Thanks > Shashi > >
Hi Shashi, On 6/30/21 5:31 PM, Shashi Mallela 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(-) > > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > index 5919d8d7b1..adaee72c1f 100644 > --- a/hw/intc/arm_gicv3_its.c > +++ b/hw/intc/arm_gicv3_its.c > @@ -28,6 +28,22 @@ struct GICv3ITSClass { > void (*parent_reset)(DeviceState *dev); > }; > > +/* > + * This is an internal enum used to distinguish between LPI triggered > + * via command queue and LPI triggered via gits_translater write. > + */ > +typedef enum ItsCmdType { > + NONE = 0, /* internal indication for GITS_TRANSLATER write */ > + CLEAR = 1, > + DISCARD = 2, > + INT = 3, > +} ItsCmdType; > + > +typedef struct { > + uint32_t iteh; > + uint64_t itel; > +} IteEntry; > + > static uint64_t baser_base_addr(uint64_t value, uint32_t page_sz) > { > uint64_t result = 0; > @@ -49,6 +65,330 @@ static uint64_t baser_base_addr(uint64_t value, uint32_t page_sz) > return result; > } > > +static bool get_cte(GICv3ITSState *s, uint16_t icid, uint64_t *cte, > + MemTxResult *res) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint64_t l2t_addr; > + uint64_t value; > + bool valid_l2t; > + uint32_t l2t_id; > + uint32_t max_l2_entries; > + > + if (s->ct.indirect) { > + l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE); > + > + value = address_space_ldq_le(as, > + s->ct.base_addr + > + (l2t_id * L1TABLE_ENTRY_SIZE), > + MEMTXATTRS_UNSPECIFIED, res); > + > + if (*res == MEMTX_OK) { > + valid_l2t = (value & L2_TABLE_VALID_MASK) != 0; > + > + if (valid_l2t) { > + max_l2_entries = s->ct.page_sz / s->ct.entry_sz; > + > + l2t_addr = value & ((1ULL << 51) - 1); > + > + *cte = address_space_ldq_le(as, l2t_addr + > + ((icid % max_l2_entries) * GITS_CTE_SIZE), > + MEMTXATTRS_UNSPECIFIED, res); > + } > + } > + } else { > + /* Flat level table */ > + *cte = address_space_ldq_le(as, s->ct.base_addr + > + (icid * GITS_CTE_SIZE), > + MEMTXATTRS_UNSPECIFIED, res); > + } > + > + return (*cte & TABLE_ENTRY_VALID_MASK) != 0; > +} > + > +static MemTxResult update_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte, > + IteEntry ite) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint64_t itt_addr; > + MemTxResult res = MEMTX_OK; > + > + itt_addr = (dte >> 6ULL) & ITTADDR_MASK; usual mask scheme > + itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */ > + > + address_space_stq_le(as, itt_addr + (eventid * sizeof(uint64_t)), > + ite.itel, MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res == MEMTX_OK) { > + address_space_stl_le(as, itt_addr + ((eventid + sizeof(uint64_t)) * > + sizeof(uint32_t)), ite.iteh, > + MEMTXATTRS_UNSPECIFIED, &res); > + } > + return res; > +} > + > +static bool get_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte, > + uint16_t *icid, uint32_t *pIntid, MemTxResult *res) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint64_t itt_addr; > + bool status = false; > + IteEntry ite; > + > + itt_addr = (dte >> 6ULL) & ITTADDR_MASK; usual mask scheme > + itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */ > + > + memset(&ite, 0 , sizeof(ite)); nit you could have initialized ite directly = {}. > + ite.itel = address_space_ldq_le(as, itt_addr + > + (eventid * sizeof(uint64_t)), > + MEMTXATTRS_UNSPECIFIED, res); > + > + if (*res == MEMTX_OK) { > + ite.iteh = address_space_ldl_le(as, itt_addr + ((eventid + > + sizeof(uint64_t)) * sizeof(uint32_t)), > + MEMTXATTRS_UNSPECIFIED, res); > + > + if (*res == MEMTX_OK) { > + if (ite.itel & TABLE_ENTRY_VALID_MASK) { > + if ((ite.itel >> ITE_ENTRY_INTTYPE_SHIFT) & > + GITS_TYPE_PHYSICAL) { > + *pIntid = (ite.itel >> ITE_ENTRY_INTID_SHIFT) & > + ITE_ENTRY_INTID_MASK; > + *icid = ite.iteh & ITE_ENTRY_ICID_MASK; > + status = true; > + } > + } > + } > + } > + return status; > +} > + > +static uint64_t get_dte(GICv3ITSState *s, uint32_t devid, MemTxResult *res) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint64_t l2t_addr; > + uint64_t value; > + bool valid_l2t; > + uint32_t l2t_id; > + uint32_t max_l2_entries; > + > + if (s->dt.indirect) { > + l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE); > + > + value = address_space_ldq_le(as, > + s->dt.base_addr + > + (l2t_id * L1TABLE_ENTRY_SIZE), > + MEMTXATTRS_UNSPECIFIED, res); > + > + if (*res == MEMTX_OK) { > + valid_l2t = (value & L2_TABLE_VALID_MASK) != 0; > + > + if (valid_l2t) { > + max_l2_entries = s->dt.page_sz / s->dt.entry_sz; > + > + l2t_addr = value & ((1ULL << 51) - 1); > + > + value = address_space_ldq_le(as, l2t_addr + > + ((devid % max_l2_entries) * GITS_DTE_SIZE), > + MEMTXATTRS_UNSPECIFIED, res); > + } > + } > + } else { > + /* Flat level table */ > + value = address_space_ldq_le(as, s->dt.base_addr + > + (devid * GITS_DTE_SIZE), > + MEMTXATTRS_UNSPECIFIED, res); > + } > + > + return value; > +} > + > +/* > + * 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 > + */ > +static MemTxResult process_its_cmd(GICv3ITSState *s, uint64_t value, > + uint32_t offset, ItsCmdType cmd) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint32_t devid, eventid; > + MemTxResult res = MEMTX_OK; > + bool dte_valid; > + uint64_t dte = 0; > + uint32_t max_eventid; > + uint16_t icid = 0; > + uint32_t pIntid = 0; > + bool ite_valid = false; > + uint64_t cte = 0; > + bool cte_valid = false; > + IteEntry ite; > + > + if (cmd == NONE) { > + devid = offset; > + } else {> + devid = ((value & DEVID_MASK) >> DEVID_SHIFT); > + > + offset += NUM_BYTES_IN_DW; > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + } > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + eventid = (value & EVENTID_MASK); > + > + dte = get_dte(s, devid, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + dte_valid = dte & TABLE_ENTRY_VALID_MASK; > + > + if (dte_valid) { > + max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1)); > + > + ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + if (ite_valid) { > + cte_valid = get_cte(s, icid, &cte, &res); > + } > + > + if (res != MEMTX_OK) { > + return res; > + } > + } > + > + if ((devid > s->dt.maxids.max_devids) || !dte_valid || !ite_valid || > + !cte_valid || (eventid > max_eventid)) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: invalid command attributes " > + "devid %d or eventid %d or invalid dte %d or" > + "invalid cte %d or invalid ite %d\n", > + __func__, devid, eventid, dte_valid, cte_valid, > + ite_valid); > + /* > + * in this implementation, in case of error > + * we ignore this command and move onto the next > + * command in the queue > + */ > + } else { > + /* > + * Current implementation only supports rdbase == procnum > + * Hence rdbase physical address is ignored > + */ > + if (cmd == DISCARD) { > + memset(&ite, 0 , sizeof(ite)); > + /* remove mapping from interrupt translation table */ > + res = update_ite(s, eventid, dte, ite); > + } > + } > + > + return res; > +} > + > +static MemTxResult process_mapti(GICv3ITSState *s, uint64_t value, > + uint32_t offset, bool ignore_pInt) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint32_t devid, eventid; > + uint32_t pIntid = 0; > + uint32_t max_eventid, max_Intid; > + bool dte_valid; > + MemTxResult res = MEMTX_OK; > + uint16_t icid = 0; > + uint64_t dte = 0; > + IteEntry ite; > + uint32_t int_spurious = INTID_SPURIOUS; > + uint64_t idbits; > + > + devid = ((value & DEVID_MASK) >> DEVID_SHIFT); > + offset += NUM_BYTES_IN_DW; > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + eventid = (value & EVENTID_MASK); > + > + if (!ignore_pInt) { > + pIntid = ((value & pINTID_MASK) >> pINTID_SHIFT); > + } > + > + offset += NUM_BYTES_IN_DW; > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + icid = value & ICID_MASK; > + > + dte = get_dte(s, devid, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + dte_valid = dte & TABLE_ENTRY_VALID_MASK; > + > + max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1)); > + > + if (!ignore_pInt) { > + idbits = MIN(FIELD_EX64(s->gicv3->cpu->gicr_propbaser, GICR_PROPBASER, > + IDBITS), GICD_TYPER_IDBITS); > + > + if (idbits < GICR_PROPBASER_IDBITS_THRESHOLD) { > + return res; > + } > + max_Intid = (1ULL << (idbits + 1)); > + } > + > + if ((devid > s->dt.maxids.max_devids) || (icid > s->ct.maxids.max_collids) > + || !dte_valid || (eventid > max_eventid) || > + (!ignore_pInt && ((pIntid < GICV3_LPI_INTID_START) || > + (pIntid > max_Intid)))) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: invalid command attributes " > + "devid %d or icid %d or eventid %d or pIntid %d or" > + "unmapped dte %d\n", __func__, devid, icid, eventid, > + pIntid, dte_valid); > + /* > + * in this implementation, in case of error > + * we ignore this command and move onto the next > + * command in the queue > + */ > + } else { > + memset(&ite, 0 , sizeof(ite)); > + /* add ite entry to interrupt translation table */ > + ite.itel = (dte_valid & TABLE_ENTRY_VALID_MASK) | > + (GITS_TYPE_PHYSICAL << ITE_ENTRY_INTTYPE_SHIFT); > + > + if (ignore_pInt) { > + ite.itel |= (eventid << ITE_ENTRY_INTID_SHIFT); > + } else { > + ite.itel |= (pIntid << ITE_ENTRY_INTID_SHIFT); > + } > + ite.itel |= (int_spurious << ITE_ENTRY_INTSP_SHIFT); > + ite.iteh |= icid; > + > + res = update_ite(s, eventid, dte, ite); > + } > + > + return res; > +} > + > static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, bool valid, > uint64_t rdbase) > { > @@ -127,7 +467,8 @@ static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset) > > icid = value & ICID_MASK; > > - rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK; > + rdbase = (value & R_MAPC_RDBASE_MASK) >> R_MAPC_RDBASE_SHIFT; > + rdbase &= RDBASE_PROCNUM_MASK; > > valid = (value & CMD_FIELD_VALID_MASK); > > @@ -301,8 +642,10 @@ static void process_cmdq(GICv3ITSState *s) > > switch (cmd) { > case GITS_CMD_INT: > + res = process_its_cmd(s, data, cq_offset, INT); > break; > case GITS_CMD_CLEAR: > + res = process_its_cmd(s, data, cq_offset, CLEAR); > break; > case GITS_CMD_SYNC: > /* > @@ -319,10 +662,13 @@ static void process_cmdq(GICv3ITSState *s) > res = process_mapc(s, cq_offset); > break; > case GITS_CMD_MAPTI: > + res = process_mapti(s, data, cq_offset, false); > break; > case GITS_CMD_MAPI: > + res = process_mapti(s, data, cq_offset, true); > break; > case GITS_CMD_DISCARD: > + res = process_its_cmd(s, data, cq_offset, DISCARD); > break; > case GITS_CMD_INV: > case GITS_CMD_INVALL: > @@ -484,7 +830,20 @@ static MemTxResult gicv3_its_translation_write(void *opaque, hwaddr offset, > uint64_t data, unsigned size, > MemTxAttrs attrs) > { > + GICv3ITSState *s = (GICv3ITSState *)opaque; > MemTxResult result = MEMTX_OK; > + uint32_t devid = 0; > + > + switch (offset) { > + case GITS_TRANSLATER: > + if (s->ctlr & ITS_CTLR_ENABLED) { > + devid = attrs.requester_id; > + result = process_its_cmd(s, data, devid, NONE); > + } > + break; > + default: > + break; > + } > > return result; > } > diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h > index a27b1e4d19..f7675a5adc 100644 > --- a/hw/intc/gicv3_internal.h > +++ b/hw/intc/gicv3_internal.h > @@ -123,6 +123,20 @@ > #define GICR_TYPER_COMMONLPIAFF (0x3 << 24) > #define GICR_TYPER_AFFINITYVALUE (0xFFFFFFFFULL << 32) > > +FIELD(GICR_PROPBASER, IDBITS, 0, 5) > +FIELD(GICR_PROPBASER, INNERCACHE, 7, 3) > +FIELD(GICR_PROPBASER, SHAREABILITY, 10, 2) > +FIELD(GICR_PROPBASER, PHYADDR, 12, 40) > +FIELD(GICR_PROPBASER, OUTERCACHE, 56, 3) > + > +#define GICR_PROPBASER_IDBITS_THRESHOLD 0xd > + > +FIELD(GICR_PENDBASER, INNERCACHE, 7, 3) > +FIELD(GICR_PENDBASER, SHAREABILITY, 10, 2) > +FIELD(GICR_PENDBASER, PHYADDR, 16, 36) > +FIELD(GICR_PENDBASER, OUTERCACHE, 56, 3) > +FIELD(GICR_PENDBASER, PTZ, 62, 1) > + > #define GICR_WAKER_ProcessorSleep (1U << 1) > #define GICR_WAKER_ChildrenAsleep (1U << 2) > > @@ -322,6 +336,13 @@ FIELD(MAPC, RDBASE, 16, 32) > #define ITTADDR_MASK ((1ULL << ITTADDR_LENGTH) - 1) > #define SIZE_MASK 0x1f > > +/* MAPI command fields */ > +#define EVENTID_MASK ((1ULL << 32) - 1) > + > +/* MAPTI command fields */ > +#define pINTID_SHIFT 32 > +#define pINTID_MASK MAKE_64BIT_MASK(32, 32) > + > #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) > > /* 16 bits EventId */ > #define ITS_IDBITS GICD_TYPER_IDBITS > diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h > index 1fd5cedbbd..0715b0bc2a 100644 > --- a/include/hw/intc/arm_gicv3_common.h > +++ b/include/hw/intc/arm_gicv3_common.h > @@ -36,6 +36,8 @@ > #define GICV3_MAXIRQ 1020 > #define GICV3_MAXSPI (GICV3_MAXIRQ - GIC_INTERNAL) > > +#define GICV3_LPI_INTID_START 8192 > + > #define GICV3_REDIST_SIZE 0x20000 > > /* Number of SGI target-list bits */ > Besides Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric
On Tue, 6 Jul 2021 at 11:04, Eric Auger <eauger@redhat.com> wrote: > > Hi Shashi, Peter, > > On 7/6/21 5:25 AM, shashi.mallela@linaro.org wrote: > > On Mon, 2021-07-05 at 20:47 -0400, shashi.mallela@linaro.org wrote: > >> On Mon, 2021-07-05 at 15:54 +0100, Peter Maydell wrote: > >>> I missed this the first time around, but I don't think this is > >>> right. > >>> Different CPUs could have different GICR_PROPBASER values, so > >>> checking > >>> against just one of them is wrong. > > "5.1.1 LPI configuration tables" says > > " > It is IMPLEMENTATION DEFINED whether GICR_PROPBASER can be set to > different values on different > Redistributors. GICR_TYPER.CommonLPIAff indicates which Redistributors > must have GICR_PROPBASER set > to the same value whenever GICR_CTLR.EnableLPIs == 1. > " > > So we can choose to set CommonLPIAff to 0 if we do not need to emulate > everything. This is what KVM does We could choose to do that, but as it happens we don't. And as far as I can tell from the spec we should not be looking at GICR_PROPBASER at all here anyway. thanks -- PMM
On Tue, 2021-07-06 at 10:19 +0100, Peter Maydell wrote: > On Tue, 6 Jul 2021 at 04:25, <shashi.mallela@linaro.org> wrote: > > On Mon, 2021-07-05 at 20:47 -0400, shashi.mallela@linaro.org wrote: > > > On Mon, 2021-07-05 at 15:54 +0100, Peter Maydell wrote: > > > > I missed this the first time around, but I don't think this is > > > > right. > > > > Different CPUs could have different GICR_PROPBASER values, so > > > > checking > > > > against just one of them is wrong. The pseudocode only tests > > > > LPIOutOfRange() > > > > which is documented as testing "larger than GICD_TYPER.IDbits > > > > or > > > > not > > > > in > > > > the LPI range and not 1023". So I don't think we should be > > > > looking > > > > at the GICR_PROPBASER field here. > > > > > > > > More generally, "s->gicv3->cpu->something" is usually going to > > > > be > > > > wrong, because it is implicitly looking at CPU 0; often either > > > > there > > > > should be something else telling is which CPU to use (as in > > > > &s->gicv3->cpu[rdbase] where the CTE told us which > > > > redistributor), > > > > or we might need to operate on all CPUs/redistributors. The > > > > only > > > > exception is where we can guarantee that all the CPUs are the > > > > same > > > > (eg when looking at GICR_TYPER.PLPIS.) > > Please ignore my last comment. > > > > To address this scenario,i think the feasible option would be to > > call > > get_cte() to get the rdbase corresponding to icid value passed to > > mapti > > command.Since each icid is mapped to a rdbase(by virtue of calling > > MAPC > > command),if the collection table has a valid mapping for this icid > > we > > continue processing this MAPTI command using &s->gicv3->cpu[rdbase] > > applicable propbaser value to validate idbits, else return without > > further processing. > > But the pseudocode for MAPTI does not say anywhere that we should > be checking the pIntID against any CPU's GICR_PROPBASER field. > It is checked only by the checks in LPIOutOfRange(), which tests: > * is it larger than permitted by GICD_TYPER.IDbits > * is it not in the LPI range and not 1023 > > Checking whether the intID is too big and would cause us to index > off the end of the redistributor's configuration table should be done > later, only when the ITS actually sends the interrupt to a particular > redistributor, I think. > > (You can't rely on the guest having done the MAPC before the MAPTI; > and in any case the guest could choose to do a MAPC to a different > redistributor after it's done the MAPTI.) > > thanks > -- PMM We already have the "intID too big check" in place within the redistributor processing when ITS sends the interrupt trigger. "the LPI range and not 1023" is also handled in this function,but for validating "is it larger than permitted by GICD_TYPER.IDbits",the source of GICD_TYPER.IDbits is GICR_PROPBASER because we pick up min of GICR_PROPBASER.IDbits and GICD_TYPER.IDBits. If we are to not use gicr_propbaser,then are we good to just accept the intID value here since we are validating the same during interrupt processing?
On Tue, 6 Jul 2021 at 13:46, <shashi.mallela@linaro.org> wrote: > > On Tue, 2021-07-06 at 10:19 +0100, Peter Maydell wrote: > > On Tue, 6 Jul 2021 at 04:25, <shashi.mallela@linaro.org> wrote: > > > > But the pseudocode for MAPTI does not say anywhere that we should > > be checking the pIntID against any CPU's GICR_PROPBASER field. > > It is checked only by the checks in LPIOutOfRange(), which tests: > > * is it larger than permitted by GICD_TYPER.IDbits > > * is it not in the LPI range and not 1023 > > > > Checking whether the intID is too big and would cause us to index > > off the end of the redistributor's configuration table should be done > > later, only when the ITS actually sends the interrupt to a particular > > redistributor, I think. > > > > (You can't rely on the guest having done the MAPC before the MAPTI; > > and in any case the guest could choose to do a MAPC to a different > > redistributor after it's done the MAPTI.) > We already have the "intID too big check" in place within the > redistributor processing when ITS sends the interrupt trigger. > "the LPI range and not 1023" is also handled in this function,but for > validating "is it larger than permitted by GICD_TYPER.IDbits",the > source of GICD_TYPER.IDbits is GICR_PROPBASER because we pick up min of > GICR_PROPBASER.IDbits and GICD_TYPER.IDBits. > > If we are to not use gicr_propbaser,then are we good to just accept the > intID value here since we are validating the same during interrupt > processing? You should check the things the pseudocode says you should check. When processing MAPTI, that's GICD_TYPER.IDbits. GICR_PROPBASER.IDbits is not the same thing because the guest can set it to a smaller value. thanks -- PMM
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 > >
On Tue, 2021-07-06 at 14:27 +0100, Peter Maydell wrote: > On Tue, 6 Jul 2021 at 13:46, <shashi.mallela@linaro.org> wrote: > > On Tue, 2021-07-06 at 10:19 +0100, Peter Maydell wrote: > > > On Tue, 6 Jul 2021 at 04:25, <shashi.mallela@linaro.org> wrote: > > > > > > But the pseudocode for MAPTI does not say anywhere that we should > > > be checking the pIntID against any CPU's GICR_PROPBASER field. > > > It is checked only by the checks in LPIOutOfRange(), which tests: > > > * is it larger than permitted by GICD_TYPER.IDbits > > > * is it not in the LPI range and not 1023 > > > > > > Checking whether the intID is too big and would cause us to index > > > off the end of the redistributor's configuration table should be > > > done > > > later, only when the ITS actually sends the interrupt to a > > > particular > > > redistributor, I think. > > > > > > (You can't rely on the guest having done the MAPC before the > > > MAPTI; > > > and in any case the guest could choose to do a MAPC to a > > > different > > > redistributor after it's done the MAPTI.) > > We already have the "intID too big check" in place within the > > redistributor processing when ITS sends the interrupt trigger. > > "the LPI range and not 1023" is also handled in this function,but > > for > > validating "is it larger than permitted by GICD_TYPER.IDbits",the > > source of GICD_TYPER.IDbits is GICR_PROPBASER because we pick up > > min of > > GICR_PROPBASER.IDbits and GICD_TYPER.IDBits. > > > > If we are to not use gicr_propbaser,then are we good to just accept > > the > > intID value here since we are validating the same during interrupt > > processing? > > You should check the things the pseudocode says you should check. > When processing MAPTI, that's GICD_TYPER.IDbits. > GICR_PROPBASER.IDbits > is not the same thing because the guest can set it to a smaller > value. Have made changes in code to check "intID too big" case using GICD_TYPER.IDbits instead of GICR_PROPBASER.IDbits > thanks > -- PMM
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index 5919d8d7b1..adaee72c1f 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -28,6 +28,22 @@ struct GICv3ITSClass { void (*parent_reset)(DeviceState *dev); }; +/* + * This is an internal enum used to distinguish between LPI triggered + * via command queue and LPI triggered via gits_translater write. + */ +typedef enum ItsCmdType { + NONE = 0, /* internal indication for GITS_TRANSLATER write */ + CLEAR = 1, + DISCARD = 2, + INT = 3, +} ItsCmdType; + +typedef struct { + uint32_t iteh; + uint64_t itel; +} IteEntry; + static uint64_t baser_base_addr(uint64_t value, uint32_t page_sz) { uint64_t result = 0; @@ -49,6 +65,330 @@ static uint64_t baser_base_addr(uint64_t value, uint32_t page_sz) return result; } +static bool get_cte(GICv3ITSState *s, uint16_t icid, uint64_t *cte, + MemTxResult *res) +{ + AddressSpace *as = &s->gicv3->dma_as; + uint64_t l2t_addr; + uint64_t value; + bool valid_l2t; + uint32_t l2t_id; + uint32_t max_l2_entries; + + if (s->ct.indirect) { + l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE); + + value = address_space_ldq_le(as, + s->ct.base_addr + + (l2t_id * L1TABLE_ENTRY_SIZE), + MEMTXATTRS_UNSPECIFIED, res); + + if (*res == MEMTX_OK) { + valid_l2t = (value & L2_TABLE_VALID_MASK) != 0; + + if (valid_l2t) { + max_l2_entries = s->ct.page_sz / s->ct.entry_sz; + + l2t_addr = value & ((1ULL << 51) - 1); + + *cte = address_space_ldq_le(as, l2t_addr + + ((icid % max_l2_entries) * GITS_CTE_SIZE), + MEMTXATTRS_UNSPECIFIED, res); + } + } + } else { + /* Flat level table */ + *cte = address_space_ldq_le(as, s->ct.base_addr + + (icid * GITS_CTE_SIZE), + MEMTXATTRS_UNSPECIFIED, res); + } + + return (*cte & TABLE_ENTRY_VALID_MASK) != 0; +} + +static MemTxResult update_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte, + IteEntry ite) +{ + AddressSpace *as = &s->gicv3->dma_as; + uint64_t itt_addr; + MemTxResult res = MEMTX_OK; + + itt_addr = (dte >> 6ULL) & ITTADDR_MASK; + itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */ + + address_space_stq_le(as, itt_addr + (eventid * sizeof(uint64_t)), + ite.itel, MEMTXATTRS_UNSPECIFIED, &res); + + if (res == MEMTX_OK) { + address_space_stl_le(as, itt_addr + ((eventid + sizeof(uint64_t)) * + sizeof(uint32_t)), ite.iteh, + MEMTXATTRS_UNSPECIFIED, &res); + } + return res; +} + +static bool get_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte, + uint16_t *icid, uint32_t *pIntid, MemTxResult *res) +{ + AddressSpace *as = &s->gicv3->dma_as; + uint64_t itt_addr; + bool status = false; + IteEntry ite; + + itt_addr = (dte >> 6ULL) & ITTADDR_MASK; + itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */ + + memset(&ite, 0 , sizeof(ite)); + ite.itel = address_space_ldq_le(as, itt_addr + + (eventid * sizeof(uint64_t)), + MEMTXATTRS_UNSPECIFIED, res); + + if (*res == MEMTX_OK) { + ite.iteh = address_space_ldl_le(as, itt_addr + ((eventid + + sizeof(uint64_t)) * sizeof(uint32_t)), + MEMTXATTRS_UNSPECIFIED, res); + + if (*res == MEMTX_OK) { + if (ite.itel & TABLE_ENTRY_VALID_MASK) { + if ((ite.itel >> ITE_ENTRY_INTTYPE_SHIFT) & + GITS_TYPE_PHYSICAL) { + *pIntid = (ite.itel >> ITE_ENTRY_INTID_SHIFT) & + ITE_ENTRY_INTID_MASK; + *icid = ite.iteh & ITE_ENTRY_ICID_MASK; + status = true; + } + } + } + } + return status; +} + +static uint64_t get_dte(GICv3ITSState *s, uint32_t devid, MemTxResult *res) +{ + AddressSpace *as = &s->gicv3->dma_as; + uint64_t l2t_addr; + uint64_t value; + bool valid_l2t; + uint32_t l2t_id; + uint32_t max_l2_entries; + + if (s->dt.indirect) { + l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE); + + value = address_space_ldq_le(as, + s->dt.base_addr + + (l2t_id * L1TABLE_ENTRY_SIZE), + MEMTXATTRS_UNSPECIFIED, res); + + if (*res == MEMTX_OK) { + valid_l2t = (value & L2_TABLE_VALID_MASK) != 0; + + if (valid_l2t) { + max_l2_entries = s->dt.page_sz / s->dt.entry_sz; + + l2t_addr = value & ((1ULL << 51) - 1); + + value = address_space_ldq_le(as, l2t_addr + + ((devid % max_l2_entries) * GITS_DTE_SIZE), + MEMTXATTRS_UNSPECIFIED, res); + } + } + } else { + /* Flat level table */ + value = address_space_ldq_le(as, s->dt.base_addr + + (devid * GITS_DTE_SIZE), + MEMTXATTRS_UNSPECIFIED, res); + } + + return value; +} + +/* + * 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 + */ +static MemTxResult process_its_cmd(GICv3ITSState *s, uint64_t value, + uint32_t offset, ItsCmdType cmd) +{ + AddressSpace *as = &s->gicv3->dma_as; + uint32_t devid, eventid; + MemTxResult res = MEMTX_OK; + bool dte_valid; + uint64_t dte = 0; + uint32_t max_eventid; + uint16_t icid = 0; + uint32_t pIntid = 0; + bool ite_valid = false; + uint64_t cte = 0; + bool cte_valid = false; + IteEntry ite; + + if (cmd == NONE) { + devid = offset; + } else { + devid = ((value & DEVID_MASK) >> DEVID_SHIFT); + + offset += NUM_BYTES_IN_DW; + value = address_space_ldq_le(as, s->cq.base_addr + offset, + MEMTXATTRS_UNSPECIFIED, &res); + } + + if (res != MEMTX_OK) { + return res; + } + + eventid = (value & EVENTID_MASK); + + dte = get_dte(s, devid, &res); + + if (res != MEMTX_OK) { + return res; + } + dte_valid = dte & TABLE_ENTRY_VALID_MASK; + + if (dte_valid) { + max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1)); + + ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res); + + if (res != MEMTX_OK) { + return res; + } + + if (ite_valid) { + cte_valid = get_cte(s, icid, &cte, &res); + } + + if (res != MEMTX_OK) { + return res; + } + } + + if ((devid > s->dt.maxids.max_devids) || !dte_valid || !ite_valid || + !cte_valid || (eventid > max_eventid)) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: invalid command attributes " + "devid %d or eventid %d or invalid dte %d or" + "invalid cte %d or invalid ite %d\n", + __func__, devid, eventid, dte_valid, cte_valid, + ite_valid); + /* + * in this implementation, in case of error + * we ignore this command and move onto the next + * command in the queue + */ + } else { + /* + * Current implementation only supports rdbase == procnum + * Hence rdbase physical address is ignored + */ + if (cmd == DISCARD) { + memset(&ite, 0 , sizeof(ite)); + /* remove mapping from interrupt translation table */ + res = update_ite(s, eventid, dte, ite); + } + } + + return res; +} + +static MemTxResult process_mapti(GICv3ITSState *s, uint64_t value, + uint32_t offset, bool ignore_pInt) +{ + AddressSpace *as = &s->gicv3->dma_as; + uint32_t devid, eventid; + uint32_t pIntid = 0; + uint32_t max_eventid, max_Intid; + bool dte_valid; + MemTxResult res = MEMTX_OK; + uint16_t icid = 0; + uint64_t dte = 0; + IteEntry ite; + uint32_t int_spurious = INTID_SPURIOUS; + uint64_t idbits; + + devid = ((value & DEVID_MASK) >> DEVID_SHIFT); + offset += NUM_BYTES_IN_DW; + value = address_space_ldq_le(as, s->cq.base_addr + offset, + MEMTXATTRS_UNSPECIFIED, &res); + + if (res != MEMTX_OK) { + return res; + } + + eventid = (value & EVENTID_MASK); + + if (!ignore_pInt) { + pIntid = ((value & pINTID_MASK) >> pINTID_SHIFT); + } + + offset += NUM_BYTES_IN_DW; + value = address_space_ldq_le(as, s->cq.base_addr + offset, + MEMTXATTRS_UNSPECIFIED, &res); + + if (res != MEMTX_OK) { + return res; + } + + icid = value & ICID_MASK; + + dte = get_dte(s, devid, &res); + + if (res != MEMTX_OK) { + return res; + } + dte_valid = dte & TABLE_ENTRY_VALID_MASK; + + max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1)); + + if (!ignore_pInt) { + idbits = MIN(FIELD_EX64(s->gicv3->cpu->gicr_propbaser, GICR_PROPBASER, + IDBITS), GICD_TYPER_IDBITS); + + if (idbits < GICR_PROPBASER_IDBITS_THRESHOLD) { + return res; + } + max_Intid = (1ULL << (idbits + 1)); + } + + if ((devid > s->dt.maxids.max_devids) || (icid > s->ct.maxids.max_collids) + || !dte_valid || (eventid > max_eventid) || + (!ignore_pInt && ((pIntid < GICV3_LPI_INTID_START) || + (pIntid > max_Intid)))) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: invalid command attributes " + "devid %d or icid %d or eventid %d or pIntid %d or" + "unmapped dte %d\n", __func__, devid, icid, eventid, + pIntid, dte_valid); + /* + * in this implementation, in case of error + * we ignore this command and move onto the next + * command in the queue + */ + } else { + memset(&ite, 0 , sizeof(ite)); + /* add ite entry to interrupt translation table */ + ite.itel = (dte_valid & TABLE_ENTRY_VALID_MASK) | + (GITS_TYPE_PHYSICAL << ITE_ENTRY_INTTYPE_SHIFT); + + if (ignore_pInt) { + ite.itel |= (eventid << ITE_ENTRY_INTID_SHIFT); + } else { + ite.itel |= (pIntid << ITE_ENTRY_INTID_SHIFT); + } + ite.itel |= (int_spurious << ITE_ENTRY_INTSP_SHIFT); + ite.iteh |= icid; + + res = update_ite(s, eventid, dte, ite); + } + + return res; +} + static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, bool valid, uint64_t rdbase) { @@ -127,7 +467,8 @@ static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset) icid = value & ICID_MASK; - rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK; + rdbase = (value & R_MAPC_RDBASE_MASK) >> R_MAPC_RDBASE_SHIFT; + rdbase &= RDBASE_PROCNUM_MASK; valid = (value & CMD_FIELD_VALID_MASK); @@ -301,8 +642,10 @@ static void process_cmdq(GICv3ITSState *s) switch (cmd) { case GITS_CMD_INT: + res = process_its_cmd(s, data, cq_offset, INT); break; case GITS_CMD_CLEAR: + res = process_its_cmd(s, data, cq_offset, CLEAR); break; case GITS_CMD_SYNC: /* @@ -319,10 +662,13 @@ static void process_cmdq(GICv3ITSState *s) res = process_mapc(s, cq_offset); break; case GITS_CMD_MAPTI: + res = process_mapti(s, data, cq_offset, false); break; case GITS_CMD_MAPI: + res = process_mapti(s, data, cq_offset, true); break; case GITS_CMD_DISCARD: + res = process_its_cmd(s, data, cq_offset, DISCARD); break; case GITS_CMD_INV: case GITS_CMD_INVALL: @@ -484,7 +830,20 @@ static MemTxResult gicv3_its_translation_write(void *opaque, hwaddr offset, uint64_t data, unsigned size, MemTxAttrs attrs) { + GICv3ITSState *s = (GICv3ITSState *)opaque; MemTxResult result = MEMTX_OK; + uint32_t devid = 0; + + switch (offset) { + case GITS_TRANSLATER: + if (s->ctlr & ITS_CTLR_ENABLED) { + devid = attrs.requester_id; + result = process_its_cmd(s, data, devid, NONE); + } + break; + default: + break; + } return result; } diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h index a27b1e4d19..f7675a5adc 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -123,6 +123,20 @@ #define GICR_TYPER_COMMONLPIAFF (0x3 << 24) #define GICR_TYPER_AFFINITYVALUE (0xFFFFFFFFULL << 32) +FIELD(GICR_PROPBASER, IDBITS, 0, 5) +FIELD(GICR_PROPBASER, INNERCACHE, 7, 3) +FIELD(GICR_PROPBASER, SHAREABILITY, 10, 2) +FIELD(GICR_PROPBASER, PHYADDR, 12, 40) +FIELD(GICR_PROPBASER, OUTERCACHE, 56, 3) + +#define GICR_PROPBASER_IDBITS_THRESHOLD 0xd + +FIELD(GICR_PENDBASER, INNERCACHE, 7, 3) +FIELD(GICR_PENDBASER, SHAREABILITY, 10, 2) +FIELD(GICR_PENDBASER, PHYADDR, 16, 36) +FIELD(GICR_PENDBASER, OUTERCACHE, 56, 3) +FIELD(GICR_PENDBASER, PTZ, 62, 1) + #define GICR_WAKER_ProcessorSleep (1U << 1) #define GICR_WAKER_ChildrenAsleep (1U << 2) @@ -322,6 +336,13 @@ FIELD(MAPC, RDBASE, 16, 32) #define ITTADDR_MASK ((1ULL << ITTADDR_LENGTH) - 1) #define SIZE_MASK 0x1f +/* MAPI command fields */ +#define EVENTID_MASK ((1ULL << 32) - 1) + +/* MAPTI command fields */ +#define pINTID_SHIFT 32 +#define pINTID_MASK MAKE_64BIT_MASK(32, 32) + #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) /* 16 bits EventId */ #define ITS_IDBITS GICD_TYPER_IDBITS diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h index 1fd5cedbbd..0715b0bc2a 100644 --- a/include/hw/intc/arm_gicv3_common.h +++ b/include/hw/intc/arm_gicv3_common.h @@ -36,6 +36,8 @@ #define GICV3_MAXIRQ 1020 #define GICV3_MAXSPI (GICV3_MAXIRQ - GIC_INTERNAL) +#define GICV3_LPI_INTID_START 8192 + #define GICV3_REDIST_SIZE 0x20000 /* Number of SGI target-list bits */
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(-)