| Message ID | 20211211191135.1764649-10-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm gicv3 ITS: Various bug fixes and refactorings | expand |
On 12/11/21 11:11 AM, Peter Maydell wrote: > The MAPI command takes arguments DeviceID, EventID, ICID, and is > defined to be equivalent to MAPTI DeviceID, EventID, EventID, ICID. > (That is, where MAPTI takes an explicit pINTID, MAPI uses the EventID > as the pINTID.) > > We didn't quite get this right. In particular the error checks for > MAPI include "EventID does not specify a valid LPI identifier", which > is the same as MAPTI's error check for the pINTID field. QEMU's code > skips the pINTID error check entirely in the MAPI case. > > We can fix this bug and in the process simplify the code by switching > to the obvious implementation of setting pIntid = eventid early > if ignore_pInt is true. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/intc/arm_gicv3_its.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Peter Maydell <peter.maydell@linaro.org> writes: > The MAPI command takes arguments DeviceID, EventID, ICID, and is > defined to be equivalent to MAPTI DeviceID, EventID, EventID, ICID. > (That is, where MAPTI takes an explicit pINTID, MAPI uses the EventID > as the pINTID.) > > We didn't quite get this right. In particular the error checks for > MAPI include "EventID does not specify a valid LPI identifier", which > is the same as MAPTI's error check for the pINTID field. QEMU's code > skips the pINTID error check entirely in the MAPI case. > > We can fix this bug and in the process simplify the code by switching > to the obvious implementation of setting pIntid = eventid early > if ignore_pInt is true. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/intc/arm_gicv3_its.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > index 15eb72a0a15..6f21c56fba2 100644 > --- a/hw/intc/arm_gicv3_its.c > +++ b/hw/intc/arm_gicv3_its.c > @@ -354,7 +354,9 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, > > eventid = (value & EVENTID_MASK); > > - if (!ignore_pInt) { > + if (ignore_pInt) { > + pIntid = eventid; > + } else { > pIntid = ((value & pINTID_MASK) >> pINTID_SHIFT); This would be worth cleaning up with field accessors at some point. Anyway: Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index 15eb72a0a15..6f21c56fba2 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -354,7 +354,9 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, eventid = (value & EVENTID_MASK); - if (!ignore_pInt) { + if (ignore_pInt) { + pIntid = eventid; + } else { pIntid = ((value & pINTID_MASK) >> pINTID_SHIFT); } @@ -377,14 +379,12 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1)); - if (!ignore_pInt) { - max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1; - } + max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1; if ((devid > s->dt.max_ids) || (icid > s->ct.max_ids) || !dte_valid || (eventid > max_eventid) || - (!ignore_pInt && (((pIntid < GICV3_LPI_INTID_START) || - (pIntid > max_Intid)) && (pIntid != INTID_SPURIOUS)))) { + (((pIntid < GICV3_LPI_INTID_START) || (pIntid > max_Intid)) && + (pIntid != INTID_SPURIOUS))) { qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid command attributes " "devid %d or icid %d or eventid %d or pIntid %d or" @@ -400,11 +400,7 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, IteEntry ite = {}; ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, dte_valid); ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL); - if (ignore_pInt) { - ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, eventid); - } else { - ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, pIntid); - } + ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, pIntid); ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS); ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, icid);
The MAPI command takes arguments DeviceID, EventID, ICID, and is defined to be equivalent to MAPTI DeviceID, EventID, EventID, ICID. (That is, where MAPTI takes an explicit pINTID, MAPI uses the EventID as the pINTID.) We didn't quite get this right. In particular the error checks for MAPI include "EventID does not specify a valid LPI identifier", which is the same as MAPTI's error check for the pINTID field. QEMU's code skips the pINTID error check entirely in the MAPI case. We can fix this bug and in the process simplify the code by switching to the obvious implementation of setting pIntid = eventid early if ignore_pInt is true. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/intc/arm_gicv3_its.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)