| Message ID | 1377288624-7418-5-git-send-email-christoffer.dall@linaro.org |
|---|---|
| State | New |
| Headers | show |
"arm_gic:" is sufficient, "hw/arm_gic:" if you see the need. Am 23.08.2013 22:10, schrieb Christoffer Dall: > Add a binary_point field to the gic emulation structure and support > setting/getting this register now when we have it. We don't actually > support interrupt grouping yet, oh well. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > hw/intc/arm_gic.c | 5 ++--- > hw/intc/arm_gic_common.c | 1 + > hw/intc/gic_internal.h | 3 +++ > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 4da534f..cb2004d 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -568,8 +568,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset) > case 0x04: /* Priority mask */ > return s->priority_mask[cpu]; > case 0x08: /* Binary Point */ > - /* ??? Not implemented. */ > - return 0; > + return s->binary_point[0][cpu]; > case 0x0c: /* Acknowledge */ > value = gic_acknowledge_irq(s, cpu); > value |= (GIC_SGI_SRC(value, cpu) & 0x7) << 10; > @@ -596,7 +595,7 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value) > s->priority_mask[cpu] = (value & 0xff); > break; > case 0x08: /* Binary Point */ > - /* ??? Not implemented. */ > + s->binary_point[0][cpu] = (value & 0x7); > break; > case 0x10: /* End Of Interrupt */ > return gic_complete_irq(s, cpu, value & 0x3ff); > diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c > index 7a1c9e5..a50ded2 100644 > --- a/hw/intc/arm_gic_common.c > +++ b/hw/intc/arm_gic_common.c > @@ -76,6 +76,7 @@ static const VMStateDescription vmstate_gic = { > VMSTATE_UINT16_ARRAY(running_irq, GICState, NCPU), > VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU), > VMSTATE_UINT16_ARRAY(current_pending, GICState, NCPU), > + VMSTATE_UINT8_2DARRAY(binary_point, GICState, 2, NCPU), > VMSTATE_END_OF_LIST() > } > }; You can't just add VMState fields without bumping the version_id and marking the field as starting in that version. That breaks loadvm/migration format compatibility. > diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h > index 6f04885..424a41f 100644 > --- a/hw/intc/gic_internal.h > +++ b/hw/intc/gic_internal.h > @@ -98,6 +98,9 @@ typedef struct GICState { > uint16_t running_priority[NCPU]; > uint16_t current_pending[NCPU]; > > + /* these registers are mainly used for save/restore of KVM state */ > + uint8_t binary_point[2][NCPU]; /* [0]: group 0, [1]: group 1 */ binary_point[1] is never used, but changing the VMState field from length 1 to length 2 is probably more difficult than having a few extra bytes here. Regards, Andreas > + > uint32_t num_cpu; > > MemoryRegion iomem; /* Distributor */
On 23 August 2013 21:10, Christoffer Dall <christoffer.dall@linaro.org> wrote: > Add a binary_point field to the gic emulation structure and support > setting/getting this register now when we have it. We don't actually > support interrupt grouping yet, oh well. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > hw/intc/arm_gic.c | 5 ++--- > hw/intc/arm_gic_common.c | 1 + > hw/intc/gic_internal.h | 3 +++ > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 4da534f..cb2004d 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -568,8 +568,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset) > case 0x04: /* Priority mask */ > return s->priority_mask[cpu]; > case 0x08: /* Binary Point */ > - /* ??? Not implemented. */ > - return 0; > + return s->binary_point[0][cpu]; > case 0x0c: /* Acknowledge */ > value = gic_acknowledge_irq(s, cpu); > value |= (GIC_SGI_SRC(value, cpu) & 0x7) << 10; > @@ -596,7 +595,7 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value) > s->priority_mask[cpu] = (value & 0xff); > break; > case 0x08: /* Binary Point */ > - /* ??? Not implemented. */ > + s->binary_point[0][cpu] = (value & 0x7); > break; > case 0x10: /* End Of Interrupt */ > return gic_complete_irq(s, cpu, value & 0x3ff); > diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c > index 7a1c9e5..a50ded2 100644 > --- a/hw/intc/arm_gic_common.c > +++ b/hw/intc/arm_gic_common.c > @@ -76,6 +76,7 @@ static const VMStateDescription vmstate_gic = { > VMSTATE_UINT16_ARRAY(running_irq, GICState, NCPU), > VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU), > VMSTATE_UINT16_ARRAY(current_pending, GICState, NCPU), > + VMSTATE_UINT8_2DARRAY(binary_point, GICState, 2, NCPU), > VMSTATE_END_OF_LIST() > } > }; > diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h > index 6f04885..424a41f 100644 > --- a/hw/intc/gic_internal.h > +++ b/hw/intc/gic_internal.h > @@ -98,6 +98,9 @@ typedef struct GICState { > uint16_t running_priority[NCPU]; > uint16_t current_pending[NCPU]; > > + /* these registers are mainly used for save/restore of KVM state */ > + uint8_t binary_point[2][NCPU]; /* [0]: group 0, [1]: group 1 */ This looks a bit fishy. I think fundamentally we need to be clear about what kind of GIC we (and KVM) are exposing to the guest. I think that that is supposed to be "a v2 GIC without the Security Extensions", yes? The other possible answer would be "a v2 GIC with the security extensions, but you're stuck in the NS world". The distinction is important, because it affects what state the guest can see. For v2-GIC-no-TZ, there are two registers' worth of state per CPU, accessible as GICC_BPR and GICC_ABPR. For v2-GIC-TZ, a guest stuck in the NS world only gets to see one register worth of state, ie the NS banked version of GICC_BPR (and GICC_ABPR is an alias of it). There is another register worth of state in the S banked GICC_BPR, but it's totally inaccessible to the guest. The TCG QEMU GIC model is currently adopting the "GIC without Security Extensions" model, which implies that we should be implementing GIC_ABPR too. What model does KVM's in-kernel vGIC use? (ie what state does it put into binary_point[0] and [1] on save/load)? thanks -- PMM
On Fri, Sep 06, 2013 at 03:41:04PM +0100, Peter Maydell wrote: > On 23 August 2013 21:10, Christoffer Dall <christoffer.dall@linaro.org> wrote: > > Add a binary_point field to the gic emulation structure and support > > setting/getting this register now when we have it. We don't actually > > support interrupt grouping yet, oh well. > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > hw/intc/arm_gic.c | 5 ++--- > > hw/intc/arm_gic_common.c | 1 + > > hw/intc/gic_internal.h | 3 +++ > > 3 files changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > > index 4da534f..cb2004d 100644 > > --- a/hw/intc/arm_gic.c > > +++ b/hw/intc/arm_gic.c > > @@ -568,8 +568,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset) > > case 0x04: /* Priority mask */ > > return s->priority_mask[cpu]; > > case 0x08: /* Binary Point */ > > - /* ??? Not implemented. */ > > - return 0; > > + return s->binary_point[0][cpu]; > > case 0x0c: /* Acknowledge */ > > value = gic_acknowledge_irq(s, cpu); > > value |= (GIC_SGI_SRC(value, cpu) & 0x7) << 10; > > @@ -596,7 +595,7 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value) > > s->priority_mask[cpu] = (value & 0xff); > > break; > > case 0x08: /* Binary Point */ > > - /* ??? Not implemented. */ > > + s->binary_point[0][cpu] = (value & 0x7); > > break; > > case 0x10: /* End Of Interrupt */ > > return gic_complete_irq(s, cpu, value & 0x3ff); > > diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c > > index 7a1c9e5..a50ded2 100644 > > --- a/hw/intc/arm_gic_common.c > > +++ b/hw/intc/arm_gic_common.c > > @@ -76,6 +76,7 @@ static const VMStateDescription vmstate_gic = { > > VMSTATE_UINT16_ARRAY(running_irq, GICState, NCPU), > > VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU), > > VMSTATE_UINT16_ARRAY(current_pending, GICState, NCPU), > > + VMSTATE_UINT8_2DARRAY(binary_point, GICState, 2, NCPU), > > VMSTATE_END_OF_LIST() > > } > > }; > > diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h > > index 6f04885..424a41f 100644 > > --- a/hw/intc/gic_internal.h > > +++ b/hw/intc/gic_internal.h > > @@ -98,6 +98,9 @@ typedef struct GICState { > > uint16_t running_priority[NCPU]; > > uint16_t current_pending[NCPU]; > > > > + /* these registers are mainly used for save/restore of KVM state */ > > + uint8_t binary_point[2][NCPU]; /* [0]: group 0, [1]: group 1 */ > > This looks a bit fishy. I think fundamentally we need > to be clear about what kind of GIC we (and KVM) are > exposing to the guest. I think that that is supposed > to be "a v2 GIC without the Security Extensions", yes? yes, at least that's what we export through the GICD_TYPER. > The other possible answer would be "a v2 GIC with the > security extensions, but you're stuck in the NS world". > The distinction is important, because it affects what > state the guest can see. For v2-GIC-no-TZ, there are > two registers' worth of state per CPU, accessible as > GICC_BPR and GICC_ABPR. For v2-GIC-TZ, a guest stuck in > the NS world only gets to see one register worth of > state, ie the NS banked version of GICC_BPR (and > GICC_ABPR is an alias of it). There is another register > worth of state in the S banked GICC_BPR, but it's > totally inaccessible to the guest. I'm a little uneasy about what the GICV_ABPR actually lets the guest see, but as far as I can tell it is consistent with the v2-GIC-no-TZ because group 1 interrupts can be configured to inject virtual FIQs to the guest (not that we implement distributor support for that yet) so the GICC_ABPR accesses from the non-secure (and only) state will control group 1 interrupts if the GICC_CTLR.CBPR==0 and will just be ignored if GICC_CTLR.CBPR==1. (Reading this part of the spec is not a particularly pleasent experience, especially the wording of the GICC_ABPR is funky, and it actually says that reading the GICC_ABPR reads the NS copy of GICC_IAR, which I think is a typo?) > > The TCG QEMU GIC model is currently adopting the > "GIC without Security Extensions" model, which implies > that we should be implementing GIC_ABPR too. What > model does KVM's in-kernel vGIC use? (ie what state > does it put into binary_point[0] and [1] on save/load)? > We put whatever the guest writes into the GICV_BPR and GICV_ABPR, but the in-kernel distributor does not care about priorities at all and considers all interrupts to be group 0 interrupts, which just happens to work for Linux. So yes, we should implement the GIC_ABPR, but there's no need for it yet. But, if I don't add these fields the guest will (by reading the GICC_[A]BPR registers) be able to tell it was migrated, but it will not influence anything on the distributor level. Therefore, by adding these fields we support the kernel's limited model fully without adding a bunch of code that we can't really test in real life anyhow, and it doesn't prevent us from adding full grouping support later on. -Christoffer
On 14 September 2013 02:52, Christoffer Dall <christoffer.dall@linaro.org> wrote: > On Fri, Sep 06, 2013 at 03:41:04PM +0100, Peter Maydell wrote: >> The TCG QEMU GIC model is currently adopting the >> "GIC without Security Extensions" model, which implies >> that we should be implementing GIC_ABPR too. What >> model does KVM's in-kernel vGIC use? (ie what state >> does it put into binary_point[0] and [1] on save/load)? >> > We put whatever the guest writes into the GICV_BPR and GICV_ABPR, but > the in-kernel distributor does not care about priorities at all and > considers all interrupts to be group 0 interrupts, which just happens to > work for Linux. > > So yes, we should implement the GIC_ABPR, but there's no need for it > yet. But, if I don't add these fields the guest will (by reading the > GICC_[A]BPR registers) be able to tell it was migrated, but it will not > influence anything on the distributor level. Therefore, by adding these > fields we support the kernel's limited model fully without adding a > bunch of code that we can't really test in real life anyhow, and it > doesn't prevent us from adding full grouping support later on. I agree we should have the fields for KVM's benefit. I think we should also add simple reads-as-written code in the TCG side so they're both accessible. State that the TCG implementation can't access is a bit weird. We should probably also call the fields bpr[NCPU] and abpr[NCPU] or something, so they're a little more self-documenting about what they represent. -- PMM
On Sat, Sep 14, 2013 at 10:46:37AM +0100, Peter Maydell wrote: > On 14 September 2013 02:52, Christoffer Dall > <christoffer.dall@linaro.org> wrote: > > On Fri, Sep 06, 2013 at 03:41:04PM +0100, Peter Maydell wrote: > >> The TCG QEMU GIC model is currently adopting the > >> "GIC without Security Extensions" model, which implies > >> that we should be implementing GIC_ABPR too. What > >> model does KVM's in-kernel vGIC use? (ie what state > >> does it put into binary_point[0] and [1] on save/load)? > >> > > We put whatever the guest writes into the GICV_BPR and GICV_ABPR, but > > the in-kernel distributor does not care about priorities at all and > > considers all interrupts to be group 0 interrupts, which just happens to > > work for Linux. > > > > So yes, we should implement the GIC_ABPR, but there's no need for it > > yet. But, if I don't add these fields the guest will (by reading the > > GICC_[A]BPR registers) be able to tell it was migrated, but it will not > > influence anything on the distributor level. Therefore, by adding these > > fields we support the kernel's limited model fully without adding a > > bunch of code that we can't really test in real life anyhow, and it > > doesn't prevent us from adding full grouping support later on. > > I agree we should have the fields for KVM's benefit. I think we > should also add simple reads-as-written code in the TCG > side so they're both accessible. State that the TCG implementation > can't access is a bit weird. We should probably also call > the fields bpr[NCPU] and abpr[NCPU] or something, so they're > a little more self-documenting about what they represent. > I already added the accessors for the TCG side? I will rename to bpr and abpr. -Christoffer
On Thu, Sep 19, 2013 at 08:48:35PM +0100, Christoffer Dall wrote: > On Sat, Sep 14, 2013 at 10:46:37AM +0100, Peter Maydell wrote: > > On 14 September 2013 02:52, Christoffer Dall > > <christoffer.dall@linaro.org> wrote: > > > On Fri, Sep 06, 2013 at 03:41:04PM +0100, Peter Maydell wrote: > > >> The TCG QEMU GIC model is currently adopting the > > >> "GIC without Security Extensions" model, which implies > > >> that we should be implementing GIC_ABPR too. What > > >> model does KVM's in-kernel vGIC use? (ie what state > > >> does it put into binary_point[0] and [1] on save/load)? > > >> > > > We put whatever the guest writes into the GICV_BPR and GICV_ABPR, but > > > the in-kernel distributor does not care about priorities at all and > > > considers all interrupts to be group 0 interrupts, which just happens to > > > work for Linux. > > > > > > So yes, we should implement the GIC_ABPR, but there's no need for it > > > yet. But, if I don't add these fields the guest will (by reading the > > > GICC_[A]BPR registers) be able to tell it was migrated, but it will not > > > influence anything on the distributor level. Therefore, by adding these > > > fields we support the kernel's limited model fully without adding a > > > bunch of code that we can't really test in real life anyhow, and it > > > doesn't prevent us from adding full grouping support later on. > > > > I agree we should have the fields for KVM's benefit. I think we > > should also add simple reads-as-written code in the TCG > > side so they're both accessible. State that the TCG implementation > > can't access is a bit weird. We should probably also call > > the fields bpr[NCPU] and abpr[NCPU] or something, so they're > > a little more self-documenting about what they represent. > > > I already added the accessors for the TCG side? oh, accessor for ABPR, got it. > > I will rename to bpr and abpr. > > -Christoffer
diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index 4da534f..cb2004d 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -568,8 +568,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset) case 0x04: /* Priority mask */ return s->priority_mask[cpu]; case 0x08: /* Binary Point */ - /* ??? Not implemented. */ - return 0; + return s->binary_point[0][cpu]; case 0x0c: /* Acknowledge */ value = gic_acknowledge_irq(s, cpu); value |= (GIC_SGI_SRC(value, cpu) & 0x7) << 10; @@ -596,7 +595,7 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value) s->priority_mask[cpu] = (value & 0xff); break; case 0x08: /* Binary Point */ - /* ??? Not implemented. */ + s->binary_point[0][cpu] = (value & 0x7); break; case 0x10: /* End Of Interrupt */ return gic_complete_irq(s, cpu, value & 0x3ff); diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c index 7a1c9e5..a50ded2 100644 --- a/hw/intc/arm_gic_common.c +++ b/hw/intc/arm_gic_common.c @@ -76,6 +76,7 @@ static const VMStateDescription vmstate_gic = { VMSTATE_UINT16_ARRAY(running_irq, GICState, NCPU), VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU), VMSTATE_UINT16_ARRAY(current_pending, GICState, NCPU), + VMSTATE_UINT8_2DARRAY(binary_point, GICState, 2, NCPU), VMSTATE_END_OF_LIST() } }; diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h index 6f04885..424a41f 100644 --- a/hw/intc/gic_internal.h +++ b/hw/intc/gic_internal.h @@ -98,6 +98,9 @@ typedef struct GICState { uint16_t running_priority[NCPU]; uint16_t current_pending[NCPU]; + /* these registers are mainly used for save/restore of KVM state */ + uint8_t binary_point[2][NCPU]; /* [0]: group 0, [1]: group 1 */ + uint32_t num_cpu; MemoryRegion iomem; /* Distributor */
Add a binary_point field to the gic emulation structure and support setting/getting this register now when we have it. We don't actually support interrupt grouping yet, oh well. Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> --- hw/intc/arm_gic.c | 5 ++--- hw/intc/arm_gic_common.c | 1 + hw/intc/gic_internal.h | 3 +++ 3 files changed, 6 insertions(+), 3 deletions(-)