From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 89A3BC48BDF for ; Thu, 10 Jun 2021 23:40:23 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id ED02661403 for ; Thu, 10 Jun 2021 23:40:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org ED02661403 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:37710 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lrUHe-0005Tq-2A for qemu-devel@archiver.kernel.org; Thu, 10 Jun 2021 19:40:22 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:55220) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lrUGb-0004Yb-Bw for qemu-devel@nongnu.org; Thu, 10 Jun 2021 19:39:17 -0400 Received: from mail-qk1-x72d.google.com ([2607:f8b0:4864:20::72d]:44652) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lrUGV-0005uL-5p for qemu-devel@nongnu.org; Thu, 10 Jun 2021 19:39:17 -0400 Received: by mail-qk1-x72d.google.com with SMTP id c18so14128941qkc.11 for ; Thu, 10 Jun 2021 16:39:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:message-id:in-reply-to:references:subject :mime-version; bh=il4mGfa3ZlzXY9xugGQYUPPxm0UGOL9o7zhNdgGs9sI=; b=Czsn7kXycYNIYsUx0ZrP12pwWHOToG2SK+q6UPuLW+fqArWeZpHvEDrBD4goNc+mXU CO2Vf/DwLz9XIbdFnVAG1Vq7tIUWN/4QPQO7klLwwBA3L5gDmM980LNuwhXWKgNO4CY4 LS3KotxFl3LG8c1A24JOxLL1vLhouYEZmSKA0NZx7+0jWDRlBLHpGhn0zvbThdypRDQa OB3cjLCgwiH0pEZ/soT9dG3GXzrMvwOnqeOVa2vAcbCtzhsm4aXEbYxvt3G6wNKhNZyG sgAEkTeZNCJffI/dUHCwYhJlBPP3Or4du3fe49a0o7cDiuFNUeBWWoJr/HY2KhgQ9wuq Mpkw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:message-id:in-reply-to :references:subject:mime-version; bh=il4mGfa3ZlzXY9xugGQYUPPxm0UGOL9o7zhNdgGs9sI=; b=e/mn89u2TeXnjECIjpM2WBVvqyf3FMp/8hLI2B2BEFpiS1J3+TVxX5Ec3mSWL5/FRU Y+LMEJ6kiY8tqbgTYjsC5mcUdzJyc55rQYhR6XptjilQDy3kOls4iUyD3YA9gJoJNrk1 qpDae4l480jS6CRGJWewc4YxI7GZhdlcl9hc1KEs4De8WFIiSfASaG+R5ZXhemP8ag1V BBJ7TWJ4JiewepTnqsnRUuvAOtLbC0t77HbRbaUc6OcWqv6iuFeMmuZr2x7QoVAJCUu2 Rr2jJMQvry77q9vOwb+E672/XZ0PNzNx1SoRevonofF4ypPt3TqiXwcqVrqLZP/EuCHF 8aaQ== X-Gm-Message-State: AOAM532QpK/LE8zLSLs/W6f7Ncagb+DOyeszGXcVcYHDhOunh5jCF6dH NUglImnmmbNUwoz5V7M4YpdR37Y9cFpePA== X-Google-Smtp-Source: ABdhPJyeQl4na2nNT15NwXBQRJ7YQqmnd1BPQNbXwEDcofMPeqn3H84gLVZ8YQsMhtjTdm0OdfdXHw== X-Received: by 2002:a37:a74b:: with SMTP id q72mr1167101qke.59.1623368349828; Thu, 10 Jun 2021 16:39:09 -0700 (PDT) Received: from localhost.localdomain (bras-base-stsvon1503w-grc-21-142-114-142-78.dsl.bell.ca. [142.114.142.78]) by smtp.gmail.com with ESMTPSA id e12sm3355432qtj.48.2021.06.10.16.39.09 for (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Thu, 10 Jun 2021 16:39:09 -0700 (PDT) Date: Thu, 10 Jun 2021 19:39:05 -0400 From: Shashi Mallela To: Peter Maydell Message-ID: <551DAA51-CB17-423D-896F-AF0443A5E7AE@getmailspring.com> In-Reply-To: References: Subject: Re: [PATCH v4 6/8] hw/intc: GICv3 redistributor ITS processing X-Mailer: Mailspring MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="60c2a299_6f88f3ab_1571" Received-SPF: pass client-ip=2607:f8b0:4864:20::72d; envelope-from=shashi.mallela@linaro.org; helo=mail-qk1-x72d.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Leif Lindholm , QEMU Developers , qemu-arm , Radoslaw Biernacki Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --60c2a299_6f88f3ab_1571 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline Have addressed all comments except the ones with responses(inline) below:- On Jun 8 2021, at 9:57 am, Peter Maydell wrote: > On Wed, 2 Jun 2021 at 19:00, Shashi Mallela wrote: > > > > Implemented lpi processing at redistributor to get lpi config info > > from lpi configuration table,determine priority,set pending state in > > lpi pending table and forward the lpi to cpuif.Added logic to invoke > > redistributor lpi processing with translated LPI which set/clear LPI > > from ITS device as part of ITS INT,CLEAR,DISCARD command and > > GITS_TRANSLATER processing. > > > > Signed-off-by: Shashi Mallela > > --- > > hw/intc/arm_gicv3.c | 9 ++ > > hw/intc/arm_gicv3_common.c | 1 + > > hw/intc/arm_gicv3_cpuif.c | 7 +- > > hw/intc/arm_gicv3_its.c | 14 ++- > > hw/intc/arm_gicv3_redist.c | 145 +++++++++++++++++++++++++++++ > > hw/intc/gicv3_internal.h | 10 ++ > > include/hw/intc/arm_gicv3_common.h | 10 ++ > > 7 files changed, 190 insertions(+), 6 deletions(-) > > The code for finding/updating the best pending LPI looks a lot > better in this version -- thanks for working through that. > > An important thing which I hadn't realized previously: > the hpplpi information counts as information cached from the > LPI configuration tables (because it is based on the priority > and enable-bit information from those tables). That means that when > the guest sends the ITS INV or INVALL command we need to throw it > away and recalculate by calling gicv3_redist_update_lpi(). > (The idea here is that the guest can validly raise the priority > of an interrupt by the sequence "write to table; INVALL; SYNC", > and we need to correctly figure out that that might mean that > that LPI is now the interrupt we should be taking.) > > Agreed,will be implementing the INV/INVALL command processing in addition to existing ITS commands > diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c > index d63f8af604..4d19190b9c 100644 > --- a/hw/intc/arm_gicv3.c > +++ b/hw/intc/arm_gicv3.c > @@ -165,6 +165,15 @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs) > cs->hppi.grp = gicv3_irq_group(cs->gic, cs, cs->hppi.irq); > } > > + if (cs->gic->lpi_enable && cs->lpivalid) { You don't need a separate lpivalid flag -- you can use hpplpi.prio == 0xff as your "no pending LPI" indication. This is how the existing cs->hppi works. (irqbetter() will always return false if passed an 0xff priority, so you don't need to special case check anything here.) > + if (irqbetter(cs, cs->hpplpi.irq, cs->hpplpi.prio)) { > + cs->hppi.irq = cs->hpplpi.irq; > + cs->hppi.prio = cs->hpplpi.prio; > + cs->hppi.grp = cs->hpplpi.grp; > + seenbetter = true; > + } > + } > + > /* If the best interrupt we just found would preempt whatever > * was the previous best interrupt before this update, then > * we know it's definitely the best one now. > diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c > index 53dea2a775..223db16fec 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -435,6 +435,7 @@ static void arm_gicv3_common_reset(DeviceState *dev) > memset(cs->gicr_ipriorityr, 0, sizeof(cs->gicr_ipriorityr)); > > cs->hppi.prio = 0xff; > + cs->hpplpi.prio = 0xff; > > /* State in the CPU interface must *not* be reset here, because it > * is part of the CPU's reset domain, not the GIC device's. > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > index 81f94c7f4a..5be3efaa3f 100644 > --- a/hw/intc/arm_gicv3_cpuif.c > +++ b/hw/intc/arm_gicv3_cpuif.c > @@ -898,10 +898,12 @@ static void icc_activate_irq(GICv3CPUState *cs, int irq) > cs->gicr_iactiver0 = deposit32(cs->gicr_iactiver0, irq, 1, 1); > cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0); > gicv3_redist_update(cs); > - } else { > + } else if (irq < GICV3_LPI_INTID_START) { > gicv3_gicd_active_set(cs->gic, irq); > gicv3_gicd_pending_clear(cs->gic, irq); > gicv3_update(cs->gic, irq, 1); > + } else { > + gicv3_redist_lpi_pending(cs, irq, 0); > } > } > > @@ -1317,7 +1319,8 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri, > trace_gicv3_icc_eoir_write(is_eoir0 ? 0 : 1, > gicv3_redist_affid(cs), value); > > - if (irq >= cs->gic->num_irq) { > + if ((irq >= cs->gic->num_irq) && (!(cs->gic->lpi_enable && > + (irq >= GICV3_LPI_INTID_START)))) { Please put the line break after the first &&, not the second. That means that you avoid linebreaking in the middle of a () expression. Also you don't need the () on the outside of the !. > /* This handles two cases: > * 1. If software writes the ID of a spurious interrupt [ie 1020-1023] > * to the GICC_EOIR, the GIC ignores that write. > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > index 0a978cf55b..e0fbd4041f 100644 > --- a/hw/intc/arm_gicv3_its.c > +++ b/hw/intc/arm_gicv3_its.c > @@ -211,6 +211,7 @@ static MemTxResult process_int(GICv3ITSState *s, uint64_t value, > bool ite_valid = false; > uint64_t cte = 0; > bool cte_valid = false; > + uint64_t rdbase; > uint64_t itel = 0; > uint32_t iteh = 0; > > @@ -267,10 +268,15 @@ static MemTxResult process_int(GICv3ITSState *s, uint64_t value, > * command in the queue > */ > } else { > - /* > - * Current implementation only supports rdbase == procnum > - * Hence rdbase physical address is ignored > - */ > + rdbase = (cte >> 1U) & RDBASE_PROCNUM_MASK; > + assert(rdbase <= s->gicv3->num_cpu); We just read cte from guest memory. We mustn't allow guests to trigger assert()s in QEMU, so if the value is out of range then we need to handle it by treating the command as invalid, not by crashing. Also, your bounds-check is off by one; it should be "<", not "<=". > + > + if ((cmd == CLEAR) || (cmd == DISCARD)) { > + gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 0); > + } else { > + gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 1); > + } > + > if (cmd == DISCARD) { > /* remove mapping from interrupt translation table */ > res = update_ite(s, eventid, dte, itel, iteh); > diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c > index fb9a4ee3cc..bfc6e4e9b9 100644 > --- a/hw/intc/arm_gicv3_redist.c > +++ b/hw/intc/arm_gicv3_redist.c > @@ -255,6 +255,11 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset, > if (cs->gicr_typer & GICR_TYPER_PLPIS) { > if (value & GICR_CTLR_ENABLE_LPIS) { > cs->gicr_ctlr |= GICR_CTLR_ENABLE_LPIS; > + /* Check for any pending interr in pending table */ > + cs->lpivalid = false; > + cs->hpplpi.prio = 0xff; > + gicv3_redist_update_lpi(cs); > + gicv3_redist_update(cs); > } else { > cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS; > } > @@ -534,6 +539,146 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data, > return r; > } > > +void gicv3_redist_update_lpi(GICv3CPUState *cs) > +{ > + /* > + * This function scans the LPI pending table and for each pending > + * LPI, reads the corresponding entry from LPI configuration table > + * to extract the priority info and determine if the LPI priority > + * is lower than the current high priority interrupt.If yes, update Missing space after ".". > + * high priority pending interrupt to that of LPI. > + */ > + AddressSpace *as = &cs->gic->dma_as; > + uint64_t lpict_baddr, lpipt_baddr; > + uint32_t pendt_size = 0; > + uint8_t lpite; > + uint8_t prio, pend; > + int i; > + uint64_t idbits; You should set hpplpi.prio = 0xff; here, so you don't need to do it at every callsite. That is, what you're really doing in this function is "recalculate the hpplpi information from scratch". > + > + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS), > + GICD_TYPER_IDBITS); > + > + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser || This is the set of missing brackets that clang complains about: it should be "!(cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS)" because ! has higher priority than &. > + !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD)) { > + return; > + } > + > + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK; > + > + lpipt_baddr = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK; > + > + /* Determine the highest priority pending interrupt among LPIs */ > + pendt_size = (1ULL << (idbits + 1)); > + > + for (i = 0; i < pendt_size / 8; i++) { > + address_space_read(as, lpipt_baddr + > + (((GICV3_LPI_INTID_START + i) / 8) * sizeof(pend)), > + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend)); > + > + if ((1 << ((GICV3_LPI_INTID_START + i) % 8)) & pend) { Better written as "if (the pend bit is not set) continue;" > + address_space_read(as, lpict_baddr + (i * sizeof(lpite)), > + MEMTXATTRS_UNSPECIFIED, &lpite, sizeof(lpite)); > + > + if (!(lpite & LPI_CTE_ENABLED)) { > + continue; > + } > + > + if (cs->gic->gicd_ctlr & GICD_CTLR_DS) { > + prio = lpite & LPI_PRIORITY_MASK; > + } else { > + prio = lpite & LPI_SPRIORITY_MASK; This isn't the right calculation. When reading a priority value when GICD_CTLR.DS is zero, you need to shift it right by one and set bit 7: prio = ((lpite & LPI_PRIORITY_MASK) >> 1) & 0x80; > + } > + > + if (prio <= cs->hpplpi.prio) { > + cs->hpplpi.irq = GICV3_LPI_INTID_START + i; > + cs->hpplpi.prio = prio; > + /* LPIs are always non-secure Grp1 interrupts */ > + cs->hpplpi.grp = GICV3_G1NS; > + cs->lpivalid = true; > + } > + } > + } > +} > + > +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level) > +{ > + AddressSpace *as = &cs->gic->dma_as; > + uint64_t lpipt_baddr; > + bool ispend = false; > + uint8_t pend; > + > + /* > + * get the bit value corresponding to this irq in the > + * lpi pending table > + */ > + lpipt_baddr = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK; > + > + address_space_read(as, lpipt_baddr + ((irq / 8) * sizeof(pend)), > + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend)); > + ispend = ((pend >> (irq % 8)) & 0x1); > + > + if (ispend) { > + if (!level) { > + /* > + * clear the pending bit and update the lpi pending table > + */ > + pend &= ~(1 << (irq % 8)); > + > + address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)), > + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend)); > + } > + } else { > + if (level) { > + /* > + * if pending bit is not already set for this irq,turn-on the > + * pending bit and update the lpi pending table > + */ > + pend |= (1 << (irq % 8)); > + > + address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)), > + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend)); > + } > + } You can simplify this code a bit: address_space_read(as, lpipt_baddr + ((irq / 8) * sizeof(pend)), MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend)); ispend = extract32(pend, irq % 8, 1); if (ispend == level) { return; } pend = deposit32(pend, irq % 8, 1, level ? 1 : 0); address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)), MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend)); > + cs->lpivalid = false; > + cs->hpplpi.prio = 0xff; > + gicv3_redist_update_lpi(cs); You can avoid doing a full update a lot of the time: * if this LPI is worse than the current value in hpplpi (where by "worse" I mean lower-priority by the same kind of comparison irqbetter() does) then we haven't changed the best-available pending LPI, so we don't need to do an update * if we set the pending bit to 1 and the LPI is enabled and the priority of this LPI is better than the current hpplpi, then we know this LPI is now the best, so we can just set hpplpi.prio and .irq without doing a full rescan * if we didn't actually change the value of the pending bit, we don't need to do an update (you get this for free if you take the simplification suggestion I make above, which does an early-return in the "no change" case) > Accepted the code simplification,but by not calling gicv3_redist_update_lpi(cs) here ,the scenario of an activated LPI fails because this LPI's priority (which could be lower than current hpplpi) is never checked/updated and gicv3_redist_update_noirqset() fails to present a valid active high priority LPI(if applicable) to the cpu,since it is always checking against a stale hpplpi info. Have confirmed this with the kvm-unit-tests as well,wherein the LPIs are never processed and test cases fail. > +} > + > +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level) > +{ > + AddressSpace *as = &cs->gic->dma_as; > + uint64_t lpict_baddr; > + uint8_t lpite; > + uint64_t idbits; > + > + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS), > + GICD_TYPER_IDBITS); > + > + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser || This is the other set of missing brackets that clang complains about. > + !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD) || > + (irq > (1ULL << (idbits + 1)))) { > + return; > + } > + > + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK; > + > + /* get the lpi config table entry corresponding to this irq */ > + address_space_read(as, lpict_baddr + ((irq - GICV3_LPI_INTID_START) * > + sizeof(lpite)), MEMTXATTRS_UNSPECIFIED, > + &lpite, sizeof(lpite)); > + > + /* check if this irq is enabled before proceeding further */ > + if (!(lpite & LPI_CTE_ENABLED)) { > + return; > + } I don't think you need to make this check -- you can just set/clear the pending status of the LPI. If the LPI is not enabled then it will be ignored by gicv3_redist_update_lpi(). This is how non-LPI interrupts work and I think that LPIs behave the same way. (But it's a big spec, so I might have missed something -- if I'm wrong, please say so.) > + > + /* set/clear the pending bit for this irq */ > + gicv3_redist_lpi_pending(cs, irq, level); > + > + gicv3_redist_update(cs); > +} > + > void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level) > { > /* Update redistributor state for a change in an external PPI input line */ > diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h > index 91dbe01176..bcbccba573 100644 > --- a/hw/intc/gicv3_internal.h > +++ b/hw/intc/gicv3_internal.h > @@ -308,6 +308,13 @@ FIELD(GITS_TYPER, CIL, 36, 1) > > #define L1TABLE_ENTRY_SIZE 8 > > +#define LPI_CTE_ENABLE_OFFSET 0 > +#define LPI_CTE_ENABLED VALID_MASK > +#define LPI_CTE_PRIORITY_OFFSET 2 > +#define LPI_CTE_PRIORITY_MASK ((1U << 6) - 1) > +#define LPI_PRIORITY_MASK 0xfc > +#define LPI_SPRIORITY_MASK 0x7e > + > #define GITS_CMDQ_ENTRY_SIZE 32 > #define NUM_BYTES_IN_DW 8 > > @@ -452,6 +459,9 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data, > unsigned size, MemTxAttrs attrs); > void gicv3_dist_set_irq(GICv3State *s, int irq, int level); > void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level); > +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level); > +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level); > +void gicv3_redist_update_lpi(GICv3CPUState *cs); > void gicv3_redist_send_sgi(GICv3CPUState *cs, int grp, int irq, bool ns); > void gicv3_init_cpuif(GICv3State *s); > > diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h > index c1348cc60a..5d839da9c9 100644 > --- a/include/hw/intc/arm_gicv3_common.h > +++ b/include/hw/intc/arm_gicv3_common.h > @@ -204,6 +204,16 @@ struct GICv3CPUState { > * real state above; it doesn't need to be migrated. > */ > PendingIrq hppi; > + > + /* > + * Current highest priority pending lpi for this CPU. > + * This is cached information that can be recalculated from the > + * real state above; it doesn't need to be migrated. This comment is true for hppi, but not for hpplpi. For hpplpi it is "cached information that can be recalculated from the LPI tables in guest memory". This means that we need either to: (1) call gicv3_redist_update_lpi() in an appropriate post-load function so that the field gets re-calculated on the destination end of a migration (2) migrate the hpplpi fields Option 1 is what we do for hppi: arm_gicv3_post_load() calls gicv3_full_update_noirqset(), which does a full recalculation of the GIC state. Calling gicv3_redist_update_lpi() in arm_gicv3_post_load() before it calls gicv3_full_update_noirqset() is probably the best thing. > + */ > + PendingIrq hpplpi; > + > + bool lpivalid; /* current highest priority lpi validity status */ > + > /* This is temporary working state, to avoid a malloc in gicv3_update() */ > bool seenbetter; > }; > -- > 2.27.0 > thanks -- PMM --60c2a299_6f88f3ab_1571 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline
Have addressed all comments except the ones with responses(inline) b= elow:-

On Jun 8 2021, at 9:57 am, Peter Maydel= l <peter.maydell=40linaro.org> wrote:
On= Wed, 2 Jun 2021 at 19:00, Shashi Mallela <shashi.mallela=40linaro.org= > wrote:
>
> Implemented lpi processing at r= edistributor to get lpi config info
> from lpi configuration= table,determine priority,set pending state in
> lpi pending= table and forward the lpi to cpuif.Added logic to invoke
> = redistributor lpi processing with translated LPI which set/clear LPI
> from ITS device as part of ITS INT,CLEAR,DISCARD command and
> GITS=5FTRANSLATER processing.
>
>= Signed-off-by: Shashi Mallela <shashi.mallela=40linaro.org>
<= div>> ---
> hw/intc/arm=5Fgicv3.c =7C 9 ++
>= hw/intc/arm=5Fgicv3=5Fcommon.c =7C 1 +
> hw/intc/arm=5Fgicv= 3=5Fcpuif.c =7C 7 +-
> hw/intc/arm=5Fgicv3=5Fits.c =7C 14 ++= -
> hw/intc/arm=5Fgicv3=5Fredist.c =7C 145 +++++++++++++++++= ++++++++++++
> hw/intc/gicv3=5Finternal.h =7C 10 ++
> include/hw/intc/arm=5Fgicv3=5Fcommon.h =7C 10 ++
> 7 = files changed, 190 insertions(+), 6 deletions(-)

The code f= or finding/updating the best pending LPI looks a lot
better in = this version -- thanks for working through that.

An importa= nt thing which I hadn't realized previously:
the hpplpi informa= tion counts as information cached from the
LPI configuration ta= bles (because it is based on the priority
and enable-bit inform= ation from those tables). That means that when
the guest sends = the ITS INV or INVALL command we need to throw it
away and reca= lculate by calling gicv3=5Fredist=5Fupdate=5Flpi().
(The idea h= ere is that the guest can validly raise the priority
of an inte= rrupt by the sequence =22write to table; INVALL; SYNC=22,
and w= e need to correctly figure out that that might mean that
that L= PI is now the interrupt we should be taking.)
 > Agreed,will be implementing the INV/INVALL command proc= essing in addition to existing ITS commands

> diff --git= a/hw/intc/arm=5Fgicv3.c b/hw/intc/arm=5Fgicv3.c
> index d63= f8af604..4d19190b9c 100644
> --- a/hw/intc/arm=5Fgicv3.c
> +++ b/hw/intc/arm=5Fgicv3.c
> =40=40 -165,6 +165= ,15 =40=40 static void gicv3=5Fredist=5Fupdate=5Fnoirqset(GICv3CPUState *= cs)
> cs->hppi.grp =3D gicv3=5Firq=5Fgroup(cs->gic, cs= , cs->hppi.irq);
> =7D
>
> + if= (cs->gic->lpi=5Fenable && cs->lpivalid) =7B

You don't need a separate lpivalid flag -- you can use
hpplp= i.prio =3D=3D 0xff as your =22no pending LPI=22 indication.
Thi= s is how the existing cs->hppi works.
(irqbetter() will alwa= ys return false if passed an 0xff priority,
so you don't need t= o special case check anything here.)

> + if (irqbetter(c= s, cs->hpplpi.irq, cs->hpplpi.prio)) =7B
> + cs->hp= pi.irq =3D cs->hpplpi.irq;
> + cs->hppi.prio =3D cs-&g= t;hpplpi.prio;
> + cs->hppi.grp =3D cs->hpplpi.grp;
> + seenbetter =3D true;
> + =7D
> += =7D
> +
> /* If the best interrupt we just fou= nd would preempt whatever
> * was the previous best interrup= t before this update, then
> * we know it's definitely the b= est one now.
> diff --git a/hw/intc/arm=5Fgicv3=5Fcommon.c b= /hw/intc/arm=5Fgicv3=5Fcommon.c
> index 53dea2a775..223db16f= ec 100644
> --- a/hw/intc/arm=5Fgicv3=5Fcommon.c
&= gt; +++ b/hw/intc/arm=5Fgicv3=5Fcommon.c
> =40=40 -435,6 +43= 5,7 =40=40 static void arm=5Fgicv3=5Fcommon=5Freset(DeviceState *dev)
> memset(cs->gicr=5Fipriorityr, 0, sizeof(cs->gicr=5Fipri= orityr));
>
> cs->hppi.prio =3D 0xff;
<= div>> + cs->hpplpi.prio =3D 0xff;
>
> /* = State in the CPU interface must *not* be reset here, because it
> * is part of the CPU's reset domain, not the GIC device's.
> diff --git a/hw/intc/arm=5Fgicv3=5Fcpuif.c b/hw/intc/arm=5Fgicv3=5F= cpuif.c
> index 81f94c7f4a..5be3efaa3f 100644
>= --- a/hw/intc/arm=5Fgicv3=5Fcpuif.c
> +++ b/hw/intc/arm=5Fg= icv3=5Fcpuif.c
> =40=40 -898,10 +898,12 =40=40 static void i= cc=5Factivate=5Firq(GICv3CPUState *cs, int irq)
> cs->gic= r=5Fiactiver0 =3D deposit32(cs->gicr=5Fiactiver0, irq, 1, 1);
> cs->gicr=5Fipendr0 =3D deposit32(cs->gicr=5Fipendr0, irq, 1,= 0);
> gicv3=5Fredist=5Fupdate(cs);
> - =7D els= e =7B
> + =7D else if (irq < GICV3=5FLPI=5FINTID=5FSTART)= =7B
> gicv3=5Fgicd=5Factive=5Fset(cs->gic, irq);
> gicv3=5Fgicd=5Fpending=5Fclear(cs->gic, irq);
> g= icv3=5Fupdate(cs->gic, irq, 1);
> + =7D else =7B
> + gicv3=5Fredist=5Flpi=5Fpending(cs, irq, 0);
> =7D
> =7D
>
> =40=40 -1317,7 +1319,8 =40= =40 static void icc=5Feoir=5Fwrite(CPUARMState *env, const ARMCPRegInfo *= ri,
> trace=5Fgicv3=5Ficc=5Feoir=5Fwrite(is=5Feoir0 =3F 0 : = 1,
> gicv3=5Fredist=5Faffid(cs), value);
>
> - if (irq >=3D cs->gic->num=5Firq) =7B
>= + if ((irq >=3D cs->gic->num=5Firq) && (=21(cs->gic-= >lpi=5Fenable &&
> + (irq >=3D GICV3=5FLPI=5FI= NTID=5FSTART)))) =7B

Please put the line break after the fi= rst &&, not the second. That means
that you avoid lineb= reaking in the middle of a () expression.
Also you don't need t= he () on the outside of the =21.

> /* This handles two c= ases:
> * 1. If software writes the ID of a spurious interru= pt =5Bie 1020-1023=5D
> * to the GICC=5FEOIR, the GIC ignore= s that write.
> diff --git a/hw/intc/arm=5Fgicv3=5Fits.c b/h= w/intc/arm=5Fgicv3=5Fits.c
> index 0a978cf55b..e0fbd4041f 10= 0644
> --- a/hw/intc/arm=5Fgicv3=5Fits.c
> +++ = b/hw/intc/arm=5Fgicv3=5Fits.c
> =40=40 -211,6 +211,7 =40=40 = static MemTxResult process=5Fint(GICv3ITSState *s, uint64=5Ft value,
> bool ite=5Fvalid =3D false;
> uint64=5Ft cte =3D = 0;
> bool cte=5Fvalid =3D false;
> + uint64=5Ft= rdbase;
> uint64=5Ft itel =3D 0;
> uint32=5Ft = iteh =3D 0;
>
> =40=40 -267,10 +268,15 =40=40 s= tatic MemTxResult process=5Fint(GICv3ITSState *s, uint64=5Ft value,
=
> * command in the queue
> */
> =7D els= e =7B
> - /*
> - * Current implementation only = supports rdbase =3D=3D procnum
> - * Hence rdbase physical a= ddress is ignored
> - */
> + rdbase =3D (cte &g= t;> 1U) & RDBASE=5FPROCNUM=5FMASK;
> + assert(rdbase = <=3D s->gicv3->num=5Fcpu);

We just read cte from g= uest memory. We mustn't allow guests to
trigger assert()s in QE= MU, so if the value is out of range then
we need to handle it b= y treating the command as invalid, not by crashing.

Also, y= our bounds-check is off by one; it should be =22<=22, not =22<=3D=22= .

> +
> + if ((cmd =3D=3D CLEAR) =7C=7C (cm= d =3D=3D DISCARD)) =7B
> + gicv3=5Fredist=5Fprocess=5Flpi(&a= mp;s->gicv3->cpu=5Brdbase=5D, pIntid, 0);
> + =7D else= =7B
> + gicv3=5Fredist=5Fprocess=5Flpi(&s->gicv3->= ;cpu=5Brdbase=5D, pIntid, 1);
> + =7D
> +
=
> if (cmd =3D=3D DISCARD) =7B
> /* remove mapping fr= om interrupt translation table */
> res =3D update=5Fite(s, = eventid, dte, itel, iteh);
> diff --git a/hw/intc/arm=5Fgicv= 3=5Fredist.c b/hw/intc/arm=5Fgicv3=5Fredist.c
> index fb9a4e= e3cc..bfc6e4e9b9 100644
> --- a/hw/intc/arm=5Fgicv3=5Fredist= .c
> +++ b/hw/intc/arm=5Fgicv3=5Fredist.c
> =40= =40 -255,6 +255,11 =40=40 static MemTxResult gicr=5Fwritel(GICv3CPUState = *cs, hwaddr offset,
> if (cs->gicr=5Ftyper & GICR=5FT= YPER=5FPLPIS) =7B
> if (value & GICR=5FCTLR=5FENABLE=5FL= PIS) =7B
> cs->gicr=5Fctlr =7C=3D GICR=5FCTLR=5FENABLE=5F= LPIS;
> + /* Check for any pending interr in pending table *= /
> + cs->lpivalid =3D false;
> + cs->hpp= lpi.prio =3D 0xff;
> + gicv3=5Fredist=5Fupdate=5Flpi(cs);
> + gicv3=5Fredist=5Fupdate(cs);
> =7D else =7B
> cs->gicr=5Fctlr &=3D =7EGICR=5FCTLR=5FENABLE=5FLPIS;=
> =7D
> =40=40 -534,6 +539,146 =40=40 MemTxRes= ult gicv3=5Fredist=5Fwrite(void *opaque, hwaddr offset, uint64=5Ft data,<= /div>
> return r;
> =7D
>
>= +void gicv3=5Fredist=5Fupdate=5Flpi(GICv3CPUState *cs)
> +=7B=
> + /*
> + * This function scans the LPI pendi= ng table and for each pending
> + * LPI, reads the correspon= ding entry from LPI configuration table
> + * to extract the= priority info and determine if the LPI priority
> + * is lo= wer than the current high priority interrupt.If yes, update

Missing space after =22.=22.

> + * high priority pendin= g interrupt to that of LPI.
> + */
> + AddressS= pace *as =3D &cs->gic->dma=5Fas;
> + uint64=5Ft lp= ict=5Fbaddr, lpipt=5Fbaddr;
> + uint32=5Ft pendt=5Fsize =3D = 0;
> + uint8=5Ft lpite;
> + uint8=5Ft prio, pen= d;
> + int i;
> + uint64=5Ft idbits;

<= div>You should set hpplpi.prio =3D 0xff; here, so you don't need to do
it at every callsite.

That is, what you're really d= oing in this function is =22recalculate the
hpplpi information = from scratch=22.

> +
> + idbits =3D MIN(=46= IELD=5FEX64(cs->gicr=5Fpropbaser, GICR=5FPROPBASER, IDBITS),
> + GICD=5FTYPER=5FIDBITS);
> +
> + if ((=21= cs->gicr=5Fctlr & GICR=5FCTLR=5FENABLE=5FLPIS) =7C=7C =21cs->gi= cr=5Fpropbaser =7C=7C

This is the set of missing brackets t= hat clang complains about: it should
be =22=21(cs->gicr=5Fct= lr & GICR=5FCTLR=5FENABLE=5FLPIS)=22 because =21 has higher priority<= /div>
than &.

> + =21cs->gicr=5Fpendbaser =7C= =7C (idbits < GICR=5FPROPBASER=5FIDBITS=5FTHRESHOLD)) =7B
&g= t; + return;
> + =7D
> +
> + lpict= =5Fbaddr =3D cs->gicr=5Fpropbaser & R=5FGICR=5FPROPBASER=5FPHYADDR= =5FMASK;
> +
> + lpipt=5Fbaddr =3D cs->gicr=5F= pendbaser & R=5FGICR=5FPENDBASER=5FPHYADDR=5FMASK;
> +
> + /* Determine the highest priority pending interrupt among= LPIs */
> + pendt=5Fsize =3D (1ULL << (idbits + 1));<= /div>
> +
> + for (i =3D 0; i < pendt=5Fsize / 8; = i++) =7B
> + address=5Fspace=5Fread(as, lpipt=5Fbaddr +
> + (((GICV3=5FLPI=5FINTID=5FSTART + i) / 8) * sizeof(pend)),
> + MEMTXATTRS=5FUNSPECI=46IED, &pend, sizeof(pend));
> +
> + if ((1 << ((GICV3=5FLPI=5FINTID=5FSTA= RT + i) % 8)) & pend) =7B

Better written as =22if (the = pend bit is not set) continue;=22

> + address=5Fspace=5F= read(as, lpict=5Fbaddr + (i * sizeof(lpite)),
> + MEMTXATTRS= =5FUNSPECI=46IED, &lpite, sizeof(lpite));
> +
= > + if (=21(lpite & LPI=5FCTE=5FENABLED)) =7B
> + con= tinue;
> + =7D
> +
> + if (cs->= gic->gicd=5Fctlr & GICD=5FCTLR=5FDS) =7B
> + prio =3D= lpite & LPI=5FPRIORITY=5FMASK;
> + =7D else =7B
> + prio =3D lpite & LPI=5FSPRIORITY=5FMASK;

This= isn't the right calculation. When reading a priority value
whe= n GICD=5FCTLR.DS is zero, you need to shift it right by one
and= set bit 7:
prio =3D ((lpite & LPI=5FPRIORITY=5FMASK) >&= gt; 1) & 0x80;

> + =7D
> +
&g= t; + if (prio <=3D cs->hpplpi.prio) =7B
> + cs->hpp= lpi.irq =3D GICV3=5FLPI=5FINTID=5FSTART + i;
> + cs->hppl= pi.prio =3D prio;
> + /* LPIs are always non-secure Grp1 int= errupts */
> + cs->hpplpi.grp =3D GICV3=5FG1NS;
> + cs->lpivalid =3D true;
> + =7D
> + =7D=
> + =7D
> +=7D
> +
>= +void gicv3=5Fredist=5Flpi=5Fpending(GICv3CPUState *cs, int irq, int lev= el)
> +=7B
> + AddressSpace *as =3D &cs->= ;gic->dma=5Fas;
> + uint64=5Ft lpipt=5Fbaddr;
&= gt; + bool ispend =3D false;
> + uint8=5Ft pend;
&= gt; +
> + /*
> + * get the bit value correspond= ing to this irq in the
> + * lpi pending table
>= ; + */
> + lpipt=5Fbaddr =3D cs->gicr=5Fpendbaser & R= =5FGICR=5FPENDBASER=5FPHYADDR=5FMASK;
> +
> + a= ddress=5Fspace=5Fread(as, lpipt=5Fbaddr + ((irq / 8) * sizeof(pend)),
> + MEMTXATTRS=5FUNSPECI=46IED, &pend, sizeof(pend));
=
> + ispend =3D ((pend >> (irq % 8)) & 0x1);
&= gt; +
> + if (ispend) =7B
> + if (=21level) =7B=
> + /*
> + * clear the pending bit and update = the lpi pending table
> + */
> + pend &=3D = =7E(1 << (irq % 8));
> +
> + address=5Fsp= ace=5Fwrite(as, lpipt=5Fbaddr + ((irq / 8) * sizeof(pend)),
>= ; + MEMTXATTRS=5FUNSPECI=46IED, &pend, sizeof(pend));
> = + =7D
> + =7D else =7B
> + if (level) =7B
=
> + /*
> + * if pending bit is not already set for t= his irq,turn-on the
> + * pending bit and update the lpi pen= ding table
> + */
> + pend =7C=3D (1 << (= irq % 8));
> +
> + address=5Fspace=5Fwrite(as, = lpipt=5Fbaddr + ((irq / 8) * sizeof(pend)),
> + MEMTXATTRS=5F= UNSPECI=46IED, &pend, sizeof(pend));
> + =7D
&= gt; + =7D

You can simplify this code a bit:

a= ddress=5Fspace=5Fread(as, lpipt=5Fbaddr + ((irq / 8) * sizeof(pend)),
MEMTXATTRS=5FUNSPECI=46IED, &pend, sizeof(pend));
is= pend =3D extract32(pend, irq % 8, 1);
if (ispend =3D=3D level) = =7B
return;
=7D
pend =3D deposit32(pend, ir= q % 8, 1, level =3F 1 : 0);
address=5Fspace=5Fwrite(as, lpipt=5F= baddr + ((irq / 8) * sizeof(pend)),
MEMTXATTRS=5FUNSPECI=46IED,= &pend, sizeof(pend));


> + cs->lpivalid =3D f= alse;
> + cs->hpplpi.prio =3D 0xff;
> + gicv= 3=5Fredist=5Fupdate=5Flpi(cs);

You can avoid doing a full u= pdate a lot of the time:
* if this LPI is worse than the curren= t value in hpplpi
(where by =22worse=22 I mean lower-priority b= y the same kind of
comparison irqbetter() does) then we haven't= changed the best-available
pending LPI, so we don't need to do= an update
* if we set the pending bit to 1 and the LPI is enab= led and the priority
of this LPI is better than the current hpp= lpi, then we know this LPI
is now the best, so we can just set = hpplpi.prio and .irq without
doing a full rescan
* if= we didn't actually change the value of the pending bit, we
don= 't need to do an update (you get this for free if you take the
= simplification suggestion I make above, which does an early-return
<= div>in the =22no change=22 case)

> Accepted the code sim= plification,but by not calling gicv3=5Fredist=5Fupdate=5Flpi(cs) here ,th= e scenario of an activated LPI fails because
this LPI's priorit= y (which could be lower than current hpplpi) is never checked/updated and= gicv3=5Fredist=5Fupdate=5Fnoirqset() fails to present a valid active hig= h priority LPI(if applicable) to the cpu,since it is always checking agai= nst a stale hpplpi info.
Have confirmed this with the kvm-unit-= tests as well,wherein the LPIs are never processed and test cases fail.
> +=7D
> +
> +void gicv3=5Fredi= st=5Fprocess=5Flpi(GICv3CPUState *cs, int irq, int level)
> = +=7B
> + AddressSpace *as =3D &cs->gic->dma=5Fas;<= /div>
> + uint64=5Ft lpict=5Fbaddr;
> + uint8=5Ft lpi= te;
> + uint64=5Ft idbits;
> +
> += idbits =3D MIN(=46IELD=5FEX64(cs->gicr=5Fpropbaser, GICR=5FPROPBASER,= IDBITS),
> + GICD=5FTYPER=5FIDBITS);
> +
=
> + if ((=21cs->gicr=5Fctlr & GICR=5FCTLR=5FENABLE=5FLPIS)= =7C=7C =21cs->gicr=5Fpropbaser =7C=7C

This is the other= set of missing brackets that clang complains about.

> += =21cs->gicr=5Fpendbaser =7C=7C (idbits < GICR=5FPROPBASER=5FIDBITS= =5FTHRESHOLD) =7C=7C
> + (irq > (1ULL << (idbits + = 1)))) =7B
> + return;
> + =7D
> +<= /div>
> + lpict=5Fbaddr =3D cs->gicr=5Fpropbaser & R=5FGICR= =5FPROPBASER=5FPHYADDR=5FMASK;
> +
> + /* get t= he lpi config table entry corresponding to this irq */
> + a= ddress=5Fspace=5Fread(as, lpict=5Fbaddr + ((irq - GICV3=5FLPI=5FINTID=5FS= TART) *
> + sizeof(lpite)), MEMTXATTRS=5FUNSPECI=46IED,
> + &lpite, sizeof(lpite));
> +
> = + /* check if this irq is enabled before proceeding further */
= > + if (=21(lpite & LPI=5FCTE=5FENABLED)) =7B
> + ret= urn;
> + =7D

I don't think you need to make th= is check -- you can just set/clear
the pending status of the LP= I. If the LPI is not enabled then it will
be ignored by gicv3=5F= redist=5Fupdate=5Flpi(). This is how non-LPI interrupts
work an= d I think that LPIs behave the same way. (But it's a big spec,
= so I might have missed something -- if I'm wrong, please say so.)
> +
> + /* set/clear the pending bit for this irq = */
> + gicv3=5Fredist=5Flpi=5Fpending(cs, irq, level);
=
> +
> + gicv3=5Fredist=5Fupdate(cs);
> = +=7D
> +
> void gicv3=5Fredist=5Fset=5Firq(GICv= 3CPUState *cs, int irq, int level)
> =7B
> /* U= pdate redistributor state for a change in an external PPI input line */
> diff --git a/hw/intc/gicv3=5Finternal.h b/hw/intc/gicv3=5Fi= nternal.h
> index 91dbe01176..bcbccba573 100644
&g= t; --- a/hw/intc/gicv3=5Finternal.h
> +++ b/hw/intc/gicv3=5F= internal.h
> =40=40 -308,6 +308,13 =40=40 =46IELD(GITS=5FTYP= ER, CIL, 36, 1)
>
> =23define L1TABLE=5FENTRY=5F= SIZE 8
>
> +=23define LPI=5FCTE=5FENABLE=5FO=46= =46SET 0
> +=23define LPI=5FCTE=5FENABLED VALID=5FMASK
=
> +=23define LPI=5FCTE=5FPRIORITY=5FO=46=46SET 2
> += =23define LPI=5FCTE=5FPRIORITY=5FMASK ((1U << 6) - 1)
>= ; +=23define LPI=5FPRIORITY=5FMASK 0xfc
> +=23define LPI=5FS= PRIORITY=5FMASK 0x7e
> +
> =23define GITS=5FCMD= Q=5FENTRY=5FSIZE 32
> =23define NUM=5FBYTES=5FIN=5FDW 8
>
> =40=40 -452,6 +459,9 =40=40 MemTxResult gicv3=5F= redist=5Fwrite(void *opaque, hwaddr offset, uint64=5Ft data,
&g= t; unsigned size, MemTxAttrs attrs);
> void gicv3=5Fdist=5Fs= et=5Firq(GICv3State *s, int irq, int level);
> void gicv3=5F= redist=5Fset=5Firq(GICv3CPUState *cs, int irq, int level);
>= +void gicv3=5Fredist=5Fprocess=5Flpi(GICv3CPUState *cs, int irq, int lev= el);
> +void gicv3=5Fredist=5Flpi=5Fpending(GICv3CPUState *c= s, int irq, int level);
> +void gicv3=5Fredist=5Fupdate=5Flp= i(GICv3CPUState *cs);
> void gicv3=5Fredist=5Fsend=5Fsgi(GIC= v3CPUState *cs, int grp, int irq, bool ns);
> void gicv3=5Fi= nit=5Fcpuif(GICv3State *s);
>
> diff --git a/in= clude/hw/intc/arm=5Fgicv3=5Fcommon.h b/include/hw/intc/arm=5Fgicv3=5Fcomm= on.h
> index c1348cc60a..5d839da9c9 100644
> --= - a/include/hw/intc/arm=5Fgicv3=5Fcommon.h
> +++ b/include/h= w/intc/arm=5Fgicv3=5Fcommon.h
> =40=40 -204,6 +204,16 =40=40= struct GICv3CPUState =7B
> * real state above; it doesn't n= eed to be migrated.
> */
> PendingIrq hppi;
> +
> + /*
> + * Current highest prio= rity pending lpi for this CPU.
> + * This is cached informat= ion that can be recalculated from the
> + * real state above= ; it doesn't need to be migrated.

This comment is true for = hppi, but not for hpplpi. =46or hpplpi
it is =22cached informat= ion that can be recalculated from the LPI
tables in guest memor= y=22.

This means that we need either to:
(1) call= gicv3=5Fredist=5Fupdate=5Flpi() in an appropriate post-load function
so that the field gets re-calculated on the destination end of a m= igration
(2) migrate the hpplpi fields

Option 1 i= s what we do for hppi: arm=5Fgicv3=5Fpost=5Fload() calls
gicv3=5F= full=5Fupdate=5Fnoirqset(), which does a full recalculation of the
<= div>GIC state. Calling gicv3=5Fredist=5Fupdate=5Flpi() in arm=5Fgicv3=5Fp= ost=5Fload()
before it calls gicv3=5Ffull=5Fupdate=5Fnoirqset()= is probably the best thing.

> + */
> + Pen= dingIrq hpplpi;
> +
> + bool lpivalid; /* curre= nt highest priority lpi validity status */
> +
>= ; /* This is temporary working state, to avoid a malloc in gicv3=5Fupdate= () */
> bool seenbetter;
> =7D;
> = --
> 2.27.0
>

thanks
--= PMM
3D=22Sent --60c2a299_6f88f3ab_1571--