| Message ID | 1487313115-9510-3-git-send-email-vijay.kilari@gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi, On 17/02/2017 07:31, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > > To Save and Restore ICC_SRE_EL1 register introduce vmstate > subsection and load only if non-zero. > Also initialize icc_sre_el1 with to 0x7 in pre_load > function. > > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > --- > hw/intc/arm_gicv3_common.c | 32 ++++++++++++++++++++++++++++++++ > include/hw/intc/arm_gicv3_common.h | 1 + > 2 files changed, 33 insertions(+) > > diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c > index 16b9b0f..e62480e 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -70,6 +70,34 @@ static const VMStateDescription vmstate_gicv3_cpu_virt = { > } > }; > > +static int icc_sre_el1_reg_pre_load(void *opaque) > +{ > + GICv3CPUState *cs = opaque; > + > + /* By default enable SRE and disable IRQ & FIQ bypass. */ > + cs->icc_sre_el1 = 0x7; > + return 0; > +} > + > +static bool icc_sre_el1_reg_needed(void *opaque) > +{ > + GICv3CPUState *cs = opaque; > + > + return cs->icc_sre_el1 != 0; Don't you tell that the reset value is always != 0. In that case the subsection will be always sent, right? If we don't want to send it for TCG, shouldn't we compare against 0xf (TCG reset value you mentioned in a previous email) Thanks Eric > +} > + > +const VMStateDescription vmstate_gicv3_cpu_sre_el1 = { > + .name = "arm_gicv3_cpu/sre_el1", > + .version_id = 1, > + .minimum_version_id = 1, > + .pre_load = icc_sre_el1_reg_pre_load, > + .needed = icc_sre_el1_reg_needed, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(icc_sre_el1, GICv3CPUState), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_gicv3_cpu = { > .name = "arm_gicv3_cpu", > .version_id = 1, > @@ -100,6 +128,10 @@ static const VMStateDescription vmstate_gicv3_cpu = { > .subsections = (const VMStateDescription * []) { > &vmstate_gicv3_cpu_virt, > NULL > + }, > + .subsections = (const VMStateDescription * []) { > + &vmstate_gicv3_cpu_sre_el1, > + NULL > } > }; > > diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h > index 4156051..bccdfe1 100644 > --- a/include/hw/intc/arm_gicv3_common.h > +++ b/include/hw/intc/arm_gicv3_common.h > @@ -172,6 +172,7 @@ struct GICv3CPUState { > uint8_t gicr_ipriorityr[GIC_INTERNAL]; > > /* CPU interface */ > + uint64_t icc_sre_el1; > uint64_t icc_ctlr_el1[2]; > uint64_t icc_pmr_el1; > uint64_t icc_bpr[3]; >
On 17 February 2017 at 06:31, <vijay.kilari@gmail.com> wrote: > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > > To Save and Restore ICC_SRE_EL1 register introduce vmstate > subsection and load only if non-zero. > Also initialize icc_sre_el1 with to 0x7 in pre_load > function. > > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > --- > hw/intc/arm_gicv3_common.c | 32 ++++++++++++++++++++++++++++++++ > include/hw/intc/arm_gicv3_common.h | 1 + > 2 files changed, 33 insertions(+) > > diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c > index 16b9b0f..e62480e 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -70,6 +70,34 @@ static const VMStateDescription vmstate_gicv3_cpu_virt = { > } > }; > > +static int icc_sre_el1_reg_pre_load(void *opaque) > +{ > + GICv3CPUState *cs = opaque; > + > + /* By default enable SRE and disable IRQ & FIQ bypass. */ > + cs->icc_sre_el1 = 0x7; Why do we need the pre_load function? I would have expected that reset would have given us these defaults already. > + return 0; > +} > + > +static bool icc_sre_el1_reg_needed(void *opaque) > +{ > + GICv3CPUState *cs = opaque; > + > + return cs->icc_sre_el1 != 0; I expected this to say "we need to transfer the value if it isn't 0x7" (since the current situation of migration is "we assume that the value is 0x7"). Something should probably fail inbound migration for TCG if the value isn't 0x7, for that matter. Is there a situation where KVM might allow a value other than 0x7? > +} > + > +const VMStateDescription vmstate_gicv3_cpu_sre_el1 = { > + .name = "arm_gicv3_cpu/sre_el1", > + .version_id = 1, > + .minimum_version_id = 1, > + .pre_load = icc_sre_el1_reg_pre_load, > + .needed = icc_sre_el1_reg_needed, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(icc_sre_el1, GICv3CPUState), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_gicv3_cpu = { > .name = "arm_gicv3_cpu", > .version_id = 1, > @@ -100,6 +128,10 @@ static const VMStateDescription vmstate_gicv3_cpu = { > .subsections = (const VMStateDescription * []) { > &vmstate_gicv3_cpu_virt, > NULL > + }, > + .subsections = (const VMStateDescription * []) { > + &vmstate_gicv3_cpu_sre_el1, > + NULL > } > }; > > diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h > index 4156051..bccdfe1 100644 > --- a/include/hw/intc/arm_gicv3_common.h > +++ b/include/hw/intc/arm_gicv3_common.h > @@ -172,6 +172,7 @@ struct GICv3CPUState { > uint8_t gicr_ipriorityr[GIC_INTERNAL]; > > /* CPU interface */ > + uint64_t icc_sre_el1; > uint64_t icc_ctlr_el1[2]; > uint64_t icc_pmr_el1; > uint64_t icc_bpr[3]; > -- > 1.9.1 thanks -- PMM
Hi Peter, On Fri, Feb 17, 2017 at 7:25 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 17 February 2017 at 06:31, <vijay.kilari@gmail.com> wrote: >> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> >> >> To Save and Restore ICC_SRE_EL1 register introduce vmstate >> subsection and load only if non-zero. >> Also initialize icc_sre_el1 with to 0x7 in pre_load >> function. >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com> >> --- >> hw/intc/arm_gicv3_common.c | 32 ++++++++++++++++++++++++++++++++ >> include/hw/intc/arm_gicv3_common.h | 1 + >> 2 files changed, 33 insertions(+) >> >> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c >> index 16b9b0f..e62480e 100644 >> --- a/hw/intc/arm_gicv3_common.c >> +++ b/hw/intc/arm_gicv3_common.c >> @@ -70,6 +70,34 @@ static const VMStateDescription vmstate_gicv3_cpu_virt = { >> } >> }; >> >> +static int icc_sre_el1_reg_pre_load(void *opaque) >> +{ >> + GICv3CPUState *cs = opaque; >> + >> + /* By default enable SRE and disable IRQ & FIQ bypass. */ >> + cs->icc_sre_el1 = 0x7; > > Why do we need the pre_load function? I would have > expected that reset would have given us these defaults > already. > >> + return 0; >> +} >> + >> +static bool icc_sre_el1_reg_needed(void *opaque) >> +{ >> + GICv3CPUState *cs = opaque; >> + >> + return cs->icc_sre_el1 != 0; > > I expected this to say "we need to transfer the value if > it isn't 0x7" (since the current situation of migration > is "we assume that the value is 0x7"). > > Something should probably fail inbound migration for TCG > if the value isn't 0x7, for that matter. > > Is there a situation where KVM might allow a value other > than 0x7? In KVM, the SRE_EL1 value is 0x1. During save, value read from KVM is 0x1 though we reset to 0x7. > >> +} >> + >> +const VMStateDescription vmstate_gicv3_cpu_sre_el1 = { >> + .name = "arm_gicv3_cpu/sre_el1", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .pre_load = icc_sre_el1_reg_pre_load, >> + .needed = icc_sre_el1_reg_needed, >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT64(icc_sre_el1, GICv3CPUState), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> static const VMStateDescription vmstate_gicv3_cpu = { >> .name = "arm_gicv3_cpu", >> .version_id = 1, >> @@ -100,6 +128,10 @@ static const VMStateDescription vmstate_gicv3_cpu = { >> .subsections = (const VMStateDescription * []) { >> &vmstate_gicv3_cpu_virt, >> NULL >> + }, >> + .subsections = (const VMStateDescription * []) { >> + &vmstate_gicv3_cpu_sre_el1, >> + NULL >> } >> }; >> >> diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h >> index 4156051..bccdfe1 100644 >> --- a/include/hw/intc/arm_gicv3_common.h >> +++ b/include/hw/intc/arm_gicv3_common.h >> @@ -172,6 +172,7 @@ struct GICv3CPUState { >> uint8_t gicr_ipriorityr[GIC_INTERNAL]; >> >> /* CPU interface */ >> + uint64_t icc_sre_el1; >> uint64_t icc_ctlr_el1[2]; >> uint64_t icc_pmr_el1; >> uint64_t icc_bpr[3]; >> -- >> 1.9.1 > > thanks > -- PMM
On 20 February 2017 at 06:21, Vijay Kilari <vijay.kilari@gmail.com> wrote: > Hi Peter, > > On Fri, Feb 17, 2017 at 7:25 PM, Peter Maydell <peter.maydell@linaro.org> wrote: [on the guest-visible ICC_SRE_EL1 value] >> Is there a situation where KVM might allow a value other >> than 0x7? > > In KVM, the SRE_EL1 value is 0x1. During save, value > read from KVM is 0x1 though we reset to 0x7. 0x1 meanss "System Register Interface enabled, IRQ bypass enabled, FIQ bypass enabled". This seems rather a weird setting, because it means "the GICv3 CPU interface functionality is disabled and the GICv3 should signal interrupts via legacy IRQ and FIQ". Does KVM really support IRQ/FIQ bypass and does Linux really leave it enabled rather than turning it off by writing the value to 1? My expectation was that the KVM GICv3 emulation would make these bits RAO/WI like the TCG implementation. Is there maybe a bug in the kernel side where it doesn't implement bypass but has made these bits be RAZ/WI rather than RAO/WI ? thanks -- PMM
Hi Christoffer, On Mon, Feb 20, 2017 at 3:21 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 20 February 2017 at 06:21, Vijay Kilari <vijay.kilari@gmail.com> wrote: >> Hi Peter, >> >> On Fri, Feb 17, 2017 at 7:25 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > [on the guest-visible ICC_SRE_EL1 value] >>> Is there a situation where KVM might allow a value other >>> than 0x7? >> >> In KVM, the SRE_EL1 value is 0x1. During save, value >> read from KVM is 0x1 though we reset to 0x7. > > 0x1 meanss "System Register Interface enabled, IRQ > bypass enabled, FIQ bypass enabled". This seems > rather a weird setting, because it means "the GICv3 > CPU interface functionality is disabled and the GICv3 > should signal interrupts via legacy IRQ and FIQ". > Does KVM really support IRQ/FIQ bypass and does Linux > really leave it enabled rather than turning it off > by writing the value to 1? > > My expectation was that the KVM GICv3 emulation would > make these bits RAO/WI like the TCG implementation. > Is there maybe a bug in the kernel side where it > doesn't implement bypass but has made these bits be > RAZ/WI rather than RAO/WI ? Do you have any inputs on this?
On 22 February 2017 at 11:56, Vijay Kilari <vijay.kilari@gmail.com> wrote: > On Mon, Feb 20, 2017 at 3:21 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> My expectation was that the KVM GICv3 emulation would >> make these bits RAO/WI like the TCG implementation. >> Is there maybe a bug in the kernel side where it >> doesn't implement bypass but has made these bits be >> RAZ/WI rather than RAO/WI ? > > Do you have any inputs on this? I talked to Marc Z who agreed this is a KVM bug -- the kernel should have these bits be RAO/WI like TCG. I think Marc was going to write a patch... thanks -- PMM
On 22/02/17 12:05, Peter Maydell wrote: > On 22 February 2017 at 11:56, Vijay Kilari <vijay.kilari@gmail.com> wrote: >> On Mon, Feb 20, 2017 at 3:21 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> My expectation was that the KVM GICv3 emulation would >>> make these bits RAO/WI like the TCG implementation. >>> Is there maybe a bug in the kernel side where it >>> doesn't implement bypass but has made these bits be >>> RAZ/WI rather than RAO/WI ? >> >> Do you have any inputs on this? > > I talked to Marc Z who agreed this is a KVM bug -- the kernel > should have these bits be RAO/WI like TCG. I think Marc > was going to write a patch... I'll post that in a minute. Thanks, M.
On 22 February 2017 at 12:05, Peter Maydell <peter.maydell@linaro.org> wrote: > I talked to Marc Z who agreed this is a KVM bug -- the kernel > should have these bits be RAO/WI like TCG. I think Marc > was going to write a patch... ...so given that, what we want on the QEMU side is: * in a migration preload function: /* if the sre_el1 subsection is not transferred this * means SRE_EL1 is 0x7 (which might not be the same as * our reset value). */ cs->icc_sre_el1 = 0x7; * the reg_needed function should be return cs->icc_sre_el1 != 0x7; and the rest of this patch is OK I think. thanks -- PMM
diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c index 16b9b0f..e62480e 100644 --- a/hw/intc/arm_gicv3_common.c +++ b/hw/intc/arm_gicv3_common.c @@ -70,6 +70,34 @@ static const VMStateDescription vmstate_gicv3_cpu_virt = { } }; +static int icc_sre_el1_reg_pre_load(void *opaque) +{ + GICv3CPUState *cs = opaque; + + /* By default enable SRE and disable IRQ & FIQ bypass. */ + cs->icc_sre_el1 = 0x7; + return 0; +} + +static bool icc_sre_el1_reg_needed(void *opaque) +{ + GICv3CPUState *cs = opaque; + + return cs->icc_sre_el1 != 0; +} + +const VMStateDescription vmstate_gicv3_cpu_sre_el1 = { + .name = "arm_gicv3_cpu/sre_el1", + .version_id = 1, + .minimum_version_id = 1, + .pre_load = icc_sre_el1_reg_pre_load, + .needed = icc_sre_el1_reg_needed, + .fields = (VMStateField[]) { + VMSTATE_UINT64(icc_sre_el1, GICv3CPUState), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_gicv3_cpu = { .name = "arm_gicv3_cpu", .version_id = 1, @@ -100,6 +128,10 @@ static const VMStateDescription vmstate_gicv3_cpu = { .subsections = (const VMStateDescription * []) { &vmstate_gicv3_cpu_virt, NULL + }, + .subsections = (const VMStateDescription * []) { + &vmstate_gicv3_cpu_sre_el1, + NULL } }; diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h index 4156051..bccdfe1 100644 --- a/include/hw/intc/arm_gicv3_common.h +++ b/include/hw/intc/arm_gicv3_common.h @@ -172,6 +172,7 @@ struct GICv3CPUState { uint8_t gicr_ipriorityr[GIC_INTERNAL]; /* CPU interface */ + uint64_t icc_sre_el1; uint64_t icc_ctlr_el1[2]; uint64_t icc_pmr_el1; uint64_t icc_bpr[3];