| Message ID | 1479904764-15532-5-git-send-email-vijay.kilari@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 23 November 2016 at 12:39, <vijay.kilari@gmail.com> wrote: > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > > Reset CPU interface registers of GICv3 when CPU is reset. > For this, object interface is used, which is called from > arm_cpu_reset function. > > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com> This approach doesn't handle the SMP case correctly -- when a CPU is reset then the CPU interface for that CPU (and only that CPU) should be reset. Your code will reset every CPU interface every time any CPU is reset. I think it would be better to use the same approach that the arm_gicv3_cpuif.c code uses to arrange for cpu i/f registers to be reset, perhaps by moving the appropriate parts of that code into the common source file. Having the reset state depend implicitly on the kernel's internal state (as you have here for the ICC_CTLR_EL1 state) is something I'm a bit unsure about -- what goes wrong if you don't do that? thanks -- PMM
On Mon, Nov 28, 2016 at 6:31 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 23 November 2016 at 12:39, <vijay.kilari@gmail.com> wrote: >> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> >> >> Reset CPU interface registers of GICv3 when CPU is reset. >> For this, object interface is used, which is called from >> arm_cpu_reset function. >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > > This approach doesn't handle the SMP case correctly -- > when a CPU is reset then the CPU interface for that CPU > (and only that CPU) should be reset. Your code will > reset every CPU interface every time any CPU is reset. arm_cpu_reset is not called when particular cpu is reset?. Is it called for all cpus?. OR object_child_foreach_recursive() is calling to reset cpu interfaces of all cpus?. > > I think it would be better to use the same approach that > the arm_gicv3_cpuif.c code uses to arrange for cpu i/f > registers to be reset, perhaps by moving the appropriate > parts of that code into the common source file. > > Having the reset state depend implicitly on the kernel's > internal state (as you have here for the ICC_CTLR_EL1 > state) is something I'm a bit unsure about -- what goes > wrong if you don't do that? During VM boots kvm_arm_gicv3_reset() writes all the GIC registers with reset value. kernel does not allow writing ICC_CTLR_EL1 with zeros because it validates against hw supported values. Similarly SRE_EL1. > > thanks > -- PMM
On 28 November 2016 at 16:01, Vijay Kilari <vijay.kilari@gmail.com> wrote: > On Mon, Nov 28, 2016 at 6:31 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 23 November 2016 at 12:39, <vijay.kilari@gmail.com> wrote: >>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> >>> >>> Reset CPU interface registers of GICv3 when CPU is reset. >>> For this, object interface is used, which is called from >>> arm_cpu_reset function. >>> >>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com> >> >> This approach doesn't handle the SMP case correctly -- >> when a CPU is reset then the CPU interface for that CPU >> (and only that CPU) should be reset. Your code will >> reset every CPU interface every time any CPU is reset. > > arm_cpu_reset is not called when particular cpu is reset?. > Is it called for all cpus?. It's called to reset a particular CPU (so it will be called once for each CPU). > OR object_child_foreach_recursive() is calling to reset cpu > interfaces of > all cpus?. It does "look through the whole graph of objects in the simulation and call the function on anything in the graph that implements the interface". I've just seen that your code is doing "ignore the call if the CPU that triggered this isn't the one we care about", though -- I missed that the first time reading the code. Still I would prefer it if we did this with the same mechanism for both TCG and KVM. A generic mechanism for "let the CPU reset trigger reset of many other devices in the system" isn't widely useful because real hardware doesn't have that kind of action-at-a-distance behaviour. thanks -- PMM
On Mon, Nov 28, 2016 at 10:05 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 28 November 2016 at 16:01, Vijay Kilari <vijay.kilari@gmail.com> wrote: >> On Mon, Nov 28, 2016 at 6:31 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 23 November 2016 at 12:39, <vijay.kilari@gmail.com> wrote: >>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> >>>> >>>> Reset CPU interface registers of GICv3 when CPU is reset. >>>> For this, object interface is used, which is called from >>>> arm_cpu_reset function. >>>> >>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com> >>> >>> This approach doesn't handle the SMP case correctly -- >>> when a CPU is reset then the CPU interface for that CPU >>> (and only that CPU) should be reset. Your code will >>> reset every CPU interface every time any CPU is reset. >> >> arm_cpu_reset is not called when particular cpu is reset?. >> Is it called for all cpus?. > > It's called to reset a particular CPU (so it will be called > once for each CPU). > >> OR object_child_foreach_recursive() is calling to reset cpu >> interfaces of >> all cpus?. > > It does "look through the whole graph of objects in the > simulation and call the function on anything in the > graph that implements the interface". I've just seen that > your code is doing "ignore the call if the CPU that > triggered this isn't the one we care about", though -- > I missed that the first time reading the code. > > Still I would prefer it if we did this with the same > mechanism for both TCG and KVM. A generic mechanism for > "let the CPU reset trigger reset of many other devices in the > system" isn't widely useful because real hardware doesn't > have that kind of action-at-a-distance behaviour. To make direct call from arm_cpu_reset() to reset CPUIF, I could not find a way to get GICv3CPUState from CPUARMState or ARMCPU struct. Any idea how to get GICv3CPUState? In hw/intc/arm_gicv3_cpuif.c implementation, el_hook function is registered to fetch GICv3CPUState from CPUARMState struct, but it is for TCG > > thanks > -- PMM
On 30 November 2016 at 16:23, Vijay Kilari <vijay.kilari@gmail.com> wrote: > On Mon, Nov 28, 2016 at 10:05 PM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> Still I would prefer it if we did this with the same >> mechanism for both TCG and KVM. A generic mechanism for >> "let the CPU reset trigger reset of many other devices in the >> system" isn't widely useful because real hardware doesn't >> have that kind of action-at-a-distance behaviour. > > To make direct call from arm_cpu_reset() to reset CPUIF, > I could not find a way to get GICv3CPUState from CPUARMState or > ARMCPU struct. You don't want to directly call from arm_cpu_reset(). Coprocessor regs registered via cpregs can have reset functions, which get called automatically. This is what the TCG gicv3 code already does to reset the CPU i/f, the relevant code just needs to be arranged so it's used for KVM too. > Any idea how to get GICv3CPUState? > > In hw/intc/arm_gicv3_cpuif.c implementation, > el_hook function is registered to fetch GICv3CPUState > from CPUARMState struct, but it is for TCG Yes, you don't need the el hook. thanks -- PMM
On Wed, Nov 30, 2016 at 10:29 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 November 2016 at 16:23, Vijay Kilari <vijay.kilari@gmail.com> wrote: >> On Mon, Nov 28, 2016 at 10:05 PM, Peter Maydell >> <peter.maydell@linaro.org> wrote: >>> Still I would prefer it if we did this with the same >>> mechanism for both TCG and KVM. A generic mechanism for >>> "let the CPU reset trigger reset of many other devices in the >>> system" isn't widely useful because real hardware doesn't >>> have that kind of action-at-a-distance behaviour. >> >> To make direct call from arm_cpu_reset() to reset CPUIF, >> I could not find a way to get GICv3CPUState from CPUARMState or >> ARMCPU struct. > > You don't want to directly call from arm_cpu_reset(). > Coprocessor regs registered via cpregs can have > reset functions, which get called automatically. > This is what the TCG gicv3 code already does to reset > the CPU i/f, the relevant code just needs to be > arranged so it's used for KVM too. Yes, the reset functions of cpregs get CPUARMState as parameter and still we cannot fetch GICv3CPUState from it. The TCG code in arm_gicv3_cpuif.c is rely on el_hook to get GICv3CPUState. > >> Any idea how to get GICv3CPUState? >> >> In hw/intc/arm_gicv3_cpuif.c implementation, >> el_hook function is registered to fetch GICv3CPUState >> from CPUARMState struct, but it is for TCG > > Yes, you don't need the el hook. Without this is there a way to get GICv3CPUState for KVM? I am not familiar with this code. > > thanks > -- PMM
Hi Peter, On Thu, Dec 1, 2016 at 3:40 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote: > On Wed, Nov 30, 2016 at 10:29 PM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> On 30 November 2016 at 16:23, Vijay Kilari <vijay.kilari@gmail.com> wrote: >>> On Mon, Nov 28, 2016 at 10:05 PM, Peter Maydell >>> <peter.maydell@linaro.org> wrote: >>>> Still I would prefer it if we did this with the same >>>> mechanism for both TCG and KVM. A generic mechanism for >>>> "let the CPU reset trigger reset of many other devices in the >>>> system" isn't widely useful because real hardware doesn't >>>> have that kind of action-at-a-distance behaviour. >>> >>> To make direct call from arm_cpu_reset() to reset CPUIF, >>> I could not find a way to get GICv3CPUState from CPUARMState or >>> ARMCPU struct. >> >> You don't want to directly call from arm_cpu_reset(). >> Coprocessor regs registered via cpregs can have >> reset functions, which get called automatically. >> This is what the TCG gicv3 code already does to reset >> the CPU i/f, the relevant code just needs to be >> arranged so it's used for KVM too. > > Yes, the reset functions of cpregs get CPUARMState as parameter > and still we cannot fetch GICv3CPUState from it. I propose to add new variable to CPUARMState to store GICV3CPUState to able to access when cpregs reset is called. Is it ok? > > The TCG code in arm_gicv3_cpuif.c is rely on el_hook to get > GICv3CPUState. >> >>> Any idea how to get GICv3CPUState? >>> >>> In hw/intc/arm_gicv3_cpuif.c implementation, >>> el_hook function is registered to fetch GICv3CPUState >>> from CPUARMState struct, but it is for TCG >> >> Yes, you don't need the el hook. > > Without this is there a way to get GICv3CPUState for KVM? > I am not familiar with this code. > >> >> thanks >> -- PMM
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c index 77af32d..267c2d6 100644 --- a/hw/intc/arm_gicv3_kvm.c +++ b/hw/intc/arm_gicv3_kvm.c @@ -29,6 +29,7 @@ #include "gicv3_internal.h" #include "vgic_common.h" #include "migration/migration.h" +#include "hw/arm/linux-boot-if.h" #ifdef DEBUG_GICV3_KVM #define DPRINTF(fmt, ...) \ @@ -604,6 +605,36 @@ static void kvm_arm_gicv3_get(GICv3State *s) } } +static void arm_gicv3_reset_cpuif(ARMDeviceResetIf *obj, + unsigned int cpu_num) +{ + GICv3CPUState *c; + GICv3State *s = ARM_GICV3_COMMON(obj); + + if (!s && !s->cpu) { + return; + } + + c = &s->cpu[cpu_num]; + if (!c) { + return; + } + + /* Initialize to actual HW supported configuration */ + kvm_gicc_access(s, ICC_CTLR_EL1, cpu_num, + &c->icc_ctlr_el1[GICV3_NS], false); + + c->icc_ctlr_el1[GICV3_S] = c->icc_ctlr_el1[GICV3_NS]; + c->icc_pmr_el1 = 0; + c->icc_bpr[GICV3_G0] = GIC_MIN_BPR; + c->icc_bpr[GICV3_G1] = GIC_MIN_BPR; + c->icc_bpr[GICV3_G1NS] = GIC_MIN_BPR; + + c->icc_sre_el1 = 0x7; + memset(c->icc_apr, 0, sizeof(c->icc_apr)); + memset(c->icc_igrpen, 0, sizeof(c->icc_igrpen)); +} + static void kvm_arm_gicv3_reset(DeviceState *dev) { GICv3State *s = ARM_GICV3_COMMON(dev); @@ -688,6 +719,7 @@ static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data) DeviceClass *dc = DEVICE_CLASS(klass); ARMGICv3CommonClass *agcc = ARM_GICV3_COMMON_CLASS(klass); KVMARMGICv3Class *kgc = KVM_ARM_GICV3_CLASS(klass); + ARMDeviceResetIfClass *adrifc = ARM_DEVICE_RESET_IF_CLASS(klass); agcc->pre_save = kvm_arm_gicv3_get; agcc->post_load = kvm_arm_gicv3_put; @@ -695,6 +727,7 @@ static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data) kgc->parent_reset = dc->reset; dc->realize = kvm_arm_gicv3_realize; dc->reset = kvm_arm_gicv3_reset; + adrifc->arm_device_reset = arm_gicv3_reset_cpuif; } static const TypeInfo kvm_arm_gicv3_info = { @@ -703,6 +736,10 @@ static const TypeInfo kvm_arm_gicv3_info = { .instance_size = sizeof(GICv3State), .class_init = kvm_arm_gicv3_class_init, .class_size = sizeof(KVMARMGICv3Class), + .interfaces = (InterfaceInfo []) { + { TYPE_ARM_DEVICE_RESET_IF }, + { }, + }, }; static void kvm_arm_gicv3_register_types(void) diff --git a/include/hw/arm/linux-boot-if.h b/include/hw/arm/linux-boot-if.h index aba4479..4a8affd 100644 --- a/include/hw/arm/linux-boot-if.h +++ b/include/hw/arm/linux-boot-if.h @@ -40,4 +40,32 @@ typedef struct ARMLinuxBootIfClass { void (*arm_linux_init)(ARMLinuxBootIf *obj, bool secure_boot); } ARMLinuxBootIfClass; +#define TYPE_ARM_DEVICE_RESET_IF "arm-device-reset-if" +#define ARM_DEVICE_RESET_IF_CLASS(klass) \ + OBJECT_CLASS_CHECK(ARMDeviceResetIfClass, (klass), TYPE_ARM_DEVICE_RESET_IF) +#define ARM_DEVICE_RESET_IF_GET_CLASS(obj) \ + OBJECT_GET_CLASS(ARMDeviceResetIfClass, (obj), TYPE_ARM_DEVICE_RESET_IF) +#define ARM_DEVICE_RESET_IF(obj) \ + INTERFACE_CHECK(ARMDeviceResetIf, (obj), TYPE_ARM_DEVICE_RESET_IF) + +typedef struct ARMDeviceResetIf { + /*< private >*/ + Object parent_obj; +} ARMDeviceResetIf; + +typedef struct ARMDeviceResetIfClass { + /*< private >*/ + InterfaceClass parent_class; + + /*< public >*/ + /** arm_device_reset: Reset the device when cpu is reset is + * called. Some device registers like GICv3 cpu interface registers + * required to be reset when CPU is reset instead of GICv3 device + * reset. This callback is called when arm_cpu_reset is called. + * + * @obj: the object implementing this interface + * @cpu_num: CPU number being reset + */ + void (*arm_device_reset)(ARMDeviceResetIf *obj, unsigned int cpu_num); +} ARMDeviceResetIfClass; #endif diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 99f0dbe..44806be 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -30,6 +30,7 @@ #include "hw/loader.h" #endif #include "hw/arm/arm.h" +#include "hw/arm/linux-boot-if.h" #include "sysemu/sysemu.h" #include "sysemu/kvm.h" #include "kvm_arm.h" @@ -113,6 +114,21 @@ static void cp_reg_check_reset(gpointer key, gpointer value, gpointer opaque) assert(oldvalue == newvalue); } +static int do_arm_device_reset(Object *obj, void *opaque) +{ + if (object_dynamic_cast(obj, TYPE_ARM_DEVICE_RESET_IF)) { + ARMDeviceResetIf *adrif = ARM_DEVICE_RESET_IF(obj); + ARMDeviceResetIfClass *adrifc = ARM_DEVICE_RESET_IF_GET_CLASS(obj); + CPUState *cpu = opaque; + + if (adrifc->arm_device_reset) { + adrifc->arm_device_reset(adrif, cpu->cpu_index); + } + } + return 0; +} + + /* CPUClass::reset() */ static void arm_cpu_reset(CPUState *s) { @@ -228,6 +244,8 @@ static void arm_cpu_reset(CPUState *s) &env->vfp.standard_fp_status); tlb_flush(s, 1); + object_child_foreach_recursive(object_get_root(), + do_arm_device_reset, s); #ifndef CONFIG_USER_ONLY if (kvm_enabled()) { kvm_arm_reset_vcpu(cpu); @@ -1595,6 +1613,19 @@ static void cpu_register(const ARMCPUInfo *info) g_free((void *)type_info.name); } +static const TypeInfo arm_device_reset_if_info = { + .name = TYPE_ARM_DEVICE_RESET_IF, + .parent = TYPE_INTERFACE, + .class_size = sizeof(ARMDeviceResetIfClass), +}; + +static void arm_device_reset_register_types(void) +{ + type_register_static(&arm_device_reset_if_info); +} + +type_init(arm_device_reset_register_types) + static const TypeInfo arm_cpu_type_info = { .name = TYPE_ARM_CPU, .parent = TYPE_CPU,