Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On 18 October 2016 at 18:57, Andrew Joneswrote: > On Tue, Oct 18, 2016 at 06:07:49PM +0100, Peter Maydell wrote: >> Why do you want to un-property mp_affinity? Eventually it would >> be nice for the machine model to be able to use it to set up >> a specific NUMA configuration. > > I thought about that, but I think we'll want to specify machine > properties; nr_sockets, nr_cores, nr_threads and use the -device > command line for the cpu to specify which socket, which core, > which thread it is. This would be consistent with other architectures > and easily map to the MPIDR & cpu topology hardware descriptions. I was thinking more about "modelling board X, which we know always has 2xA53 and 4xA57 with these MPIDRs". We actually have a concrete instance in the tree at the moment: the raspberry pi 2. Specifically hw/arm/bcm2836.c sets the mp_affinity for each cpu to 0xF00 | n (where n is the CPUID). Currently it's doing that by reaching in and messing with the mp_affinity field directly, but really it ought to be doing it by setting a property on the CPU, and what it wants isn't somethnig that can be expressed with a simple nr_sockets/nr_cores/etc scheme. > Anyway, atm, I don't know of any reason to have the property user- > settable, so it seems safest to keep it hidden until we decide. I agree that it doesn't make sense to let the user mess with it, but it should be available for the board code to read and write. thanks -- PMM
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On Tue, Oct 18, 2016 at 06:07:49PM +0100, Peter Maydell wrote: > On 18 October 2016 at 17:22, Andrew Joneswrote: > > I'll note that it's > > accepted to reach into machine state through current_machine from > > the machine's devices. > > That doesn't sound like a great idea to me -- where do we > do it? A quick grep for uses of current_machine in hw/ shows only > hw/ppc/spapr_rtas.c, which is a bit of a weirdo thing because > it's the implementation of a bunch of hypercalls, not an actual > device. > > We have lots of code currently in devices that looks at bits > of overall state that it should ideally not (but which it can > because we have a lot of global variables). In an ideal world > we'd clean these up so that the devices were initialized with > suitable properties instead to tell them about this global state. > > >> > I considered that, but if we un-property ARMCPU->mp_affinity, then where > >> > can it be initialized to "MP_AFFINITY_INVALID", which would be ff00? > >> > >> Property defaults are normally initialized on instance_init. > > > > OK, so after this series goes through we can un-property mp_affinity and > > add an mp_affinity initialization back to arm_cpu_initfn, but one that > > only sets the initial invalid value. > > Note that mp_affinity is a bit awkward because for KVM we can't > get the right values out of the kernel until quite late (when > kvm_init_vcpu() is called). > > Why do you want to un-property mp_affinity? Eventually it would > be nice for the machine model to be able to use it to set up > a specific NUMA configuration. I thought about that, but I think we'll want to specify machine properties; nr_sockets, nr_cores, nr_threads and use the -device command line for the cpu to specify which socket, which core, which thread it is. This would be consistent with other architectures and easily map to the MPIDR & cpu topology hardware descriptions. Anyway, atm, I don't know of any reason to have the property user- settable, so it seems safest to keep it hidden until we decide. Thanks, drew
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On 18 October 2016 at 17:22, Andrew Joneswrote: > I'll note that it's > accepted to reach into machine state through current_machine from > the machine's devices. That doesn't sound like a great idea to me -- where do we do it? A quick grep for uses of current_machine in hw/ shows only hw/ppc/spapr_rtas.c, which is a bit of a weirdo thing because it's the implementation of a bunch of hypercalls, not an actual device. We have lots of code currently in devices that looks at bits of overall state that it should ideally not (but which it can because we have a lot of global variables). In an ideal world we'd clean these up so that the devices were initialized with suitable properties instead to tell them about this global state. >> > I considered that, but if we un-property ARMCPU->mp_affinity, then where >> > can it be initialized to "MP_AFFINITY_INVALID", which would be ff00? >> >> Property defaults are normally initialized on instance_init. > > OK, so after this series goes through we can un-property mp_affinity and > add an mp_affinity initialization back to arm_cpu_initfn, but one that > only sets the initial invalid value. Note that mp_affinity is a bit awkward because for KVM we can't get the right values out of the kernel until quite late (when kvm_init_vcpu() is called). Why do you want to un-property mp_affinity? Eventually it would be nice for the machine model to be able to use it to set up a specific NUMA configuration. thanks -- PMM
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On 18/10/2016 18:22, Andrew Jones wrote: > On Tue, Oct 18, 2016 at 01:22:07PM -0200, Eduardo Habkost wrote: >> On Tue, Oct 18, 2016 at 04:22:47PM +0200, Andrew Jones wrote: ... >>> OK, to proceed with this patch, since mp-affinity *is* currently a >>> property, we can just change its default value to ff00. Then, >>> when moving the calculation to realize we just need to ensure the >>> current value is ff00 before overwriting it. >> >> Sounds good to me. > > Good. Laurent, to be clear, MP_AFFINITY_INVALID doesn't currently exist, > and it should be defined as ~ARM64_AFFINITY_MASK OK, thank you for all your comments. I'm going to update my series with this change. Thanks, Laurent
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On Tue, Oct 18, 2016 at 01:22:07PM -0200, Eduardo Habkost wrote: > On Tue, Oct 18, 2016 at 04:22:47PM +0200, Andrew Jones wrote: > > On Tue, Oct 18, 2016 at 11:18:29AM -0200, Eduardo Habkost wrote: > > > On Tue, Oct 18, 2016 at 03:00:07PM +0200, Andrew Jones wrote: > > > > On Mon, Oct 17, 2016 at 05:20:22PM -0200, Eduardo Habkost wrote: > > > > > On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote: > > > > > > Modify all CPUs to call it from XXX_cpu_realizefn() function. > > > > > > > > > > > > Remove all the cannot_destroy_with_object_finalize_yet as > > > > > > unsafe references have been moved to cpu_exec_realizefn(). > > > > > > (tested with QOM command provided by commit 4c315c27) > > > > > > > > > > > > for arm: > > > > > > > > > > > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn() > > > > > > to arm_cpu_realizefn() as setting of cpu_index is now done > > > > > > in cpu_exec_realizefn(). > > > > > > > > > > > > Signed-off-by: Laurent Vivier> > > > > [...] > > > > > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > > > > > > index 1b9540e..364a45d 100644 > > > > > > --- a/target-arm/cpu.c > > > > > > +++ b/target-arm/cpu.c > > > > > > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj) > > > > > > CPUState *cs = CPU(obj); > > > > > > ARMCPU *cpu = ARM_CPU(obj); > > > > > > static bool inited; > > > > > > -uint32_t Aff1, Aff0; > > > > > > > > > > > > cs->env_ptr = >env; > > > > > > -cpu_exec_init(cs, _abort); > > > > > > cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, > > > > > > g_free, g_free); > > > > > > > > > > > > -/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM > > > > > > will override it. > > > > > > - * We don't support setting cluster ID ([16..23]) (known as > > > > > > Aff2 > > > > > > - * in later ARM ARM versions), or any of the higher affinity > > > > > > level fields, > > > > > > - * so these bits always RAZ. > > > > > > - */ > > > > > > -Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > > > > > > -Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > > > > > > -cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > > > > > > - > > > > > > #ifndef CONFIG_USER_ONLY > > > > > > /* Our inbound IRQ and FIQ lines */ > > > > > > if (kvm_enabled()) { > > > > > [...] > > > > > > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState > > > > > > *dev, Error **errp) > > > > > > set_feature(env, ARM_FEATURE_THUMB_DSP); > > > > > > } > > > > > > > > > > > > +/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM > > > > > > will override it. > > > > > > + * We don't support setting cluster ID ([16..23]) (known as > > > > > > Aff2 > > > > > > + * in later ARM ARM versions), or any of the higher affinity > > > > > > level fields, > > > > > > + * so these bits always RAZ. > > > > > > + */ > > > > > > +Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > > > > > > +Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > > > > > > +cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > > > > > > + > > > > > > > > > > This will override any value set in the "mp-affinity" property, > > > > > The mp-affinity property can be set by the user in the > > > > > command-line, and it is also set by machvirt_init() in > > > > > hw/arm/virt.c. > > > > > > > > I'm glad you noticed this Eduardo. We'd indeed lose changes made to > > > > the MPIDRs for mach-virt and Raspberry Pi. > > > > > > > > > > > > > > Considering that each CPU is supposed to have a different value, > > > > > I doubt there are existing use cases for mp-affinity being set > > > > > directly by the user. > > > > > > > > Probably not. It was turned into a property in order for the gicv3 > > > > model to access it. I don't think that's necessary, so we can just > > > > un-property it, accessing it directly from the structure instead. > > > > > > > > > > > > > > I suggest having a "cluster-size" property, instead of > > > > > "mp-affinity". This way the mp_affinity field can be calculated > > > > > on realize, based on the configured cluster-size. > > > > > > > > This won't work for Raspberry Pi's MPIDR adjustment. > > > > > > Where's that code? > > > > hw/arm/bcm2836.c:109 > > Thanks! > > > > > > > > > > Anyway, IMO realize should only set values when it knows > > > > nothing has been set. > > > > > > Realize could also initialize some (private) fields using other > > > (public) fields as input. Instead of making machine code > > > calculate mp_affinity, machine code could just provide input for > > > realize to calculate the right values. > > > > > > But maybe my cluster-size suggestion won't work anyway because of > > > other cases where mp_affinity is set (I didn't check all code > > > that sets/gets mp_affinity). > > > > Anyway, cluster-size is machine state, not cpu state. > > Right:
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On Tue, Oct 18, 2016 at 04:22:47PM +0200, Andrew Jones wrote: > On Tue, Oct 18, 2016 at 11:18:29AM -0200, Eduardo Habkost wrote: > > On Tue, Oct 18, 2016 at 03:00:07PM +0200, Andrew Jones wrote: > > > On Mon, Oct 17, 2016 at 05:20:22PM -0200, Eduardo Habkost wrote: > > > > On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote: > > > > > Modify all CPUs to call it from XXX_cpu_realizefn() function. > > > > > > > > > > Remove all the cannot_destroy_with_object_finalize_yet as > > > > > unsafe references have been moved to cpu_exec_realizefn(). > > > > > (tested with QOM command provided by commit 4c315c27) > > > > > > > > > > for arm: > > > > > > > > > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn() > > > > > to arm_cpu_realizefn() as setting of cpu_index is now done > > > > > in cpu_exec_realizefn(). > > > > > > > > > > Signed-off-by: Laurent Vivier> > > > [...] > > > > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > > > > > index 1b9540e..364a45d 100644 > > > > > --- a/target-arm/cpu.c > > > > > +++ b/target-arm/cpu.c > > > > > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj) > > > > > CPUState *cs = CPU(obj); > > > > > ARMCPU *cpu = ARM_CPU(obj); > > > > > static bool inited; > > > > > -uint32_t Aff1, Aff0; > > > > > > > > > > cs->env_ptr = >env; > > > > > -cpu_exec_init(cs, _abort); > > > > > cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, > > > > > g_free, g_free); > > > > > > > > > > -/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will > > > > > override it. > > > > > - * We don't support setting cluster ID ([16..23]) (known as Aff2 > > > > > - * in later ARM ARM versions), or any of the higher affinity > > > > > level fields, > > > > > - * so these bits always RAZ. > > > > > - */ > > > > > -Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > > > > > -Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > > > > > -cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > > > > > - > > > > > #ifndef CONFIG_USER_ONLY > > > > > /* Our inbound IRQ and FIQ lines */ > > > > > if (kvm_enabled()) { > > > > [...] > > > > > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, > > > > > Error **errp) > > > > > set_feature(env, ARM_FEATURE_THUMB_DSP); > > > > > } > > > > > > > > > > +/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will > > > > > override it. > > > > > + * We don't support setting cluster ID ([16..23]) (known as Aff2 > > > > > + * in later ARM ARM versions), or any of the higher affinity > > > > > level fields, > > > > > + * so these bits always RAZ. > > > > > + */ > > > > > +Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > > > > > +Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > > > > > +cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > > > > > + > > > > > > > > This will override any value set in the "mp-affinity" property, > > > > The mp-affinity property can be set by the user in the > > > > command-line, and it is also set by machvirt_init() in > > > > hw/arm/virt.c. > > > > > > I'm glad you noticed this Eduardo. We'd indeed lose changes made to > > > the MPIDRs for mach-virt and Raspberry Pi. > > > > > > > > > > > Considering that each CPU is supposed to have a different value, > > > > I doubt there are existing use cases for mp-affinity being set > > > > directly by the user. > > > > > > Probably not. It was turned into a property in order for the gicv3 > > > model to access it. I don't think that's necessary, so we can just > > > un-property it, accessing it directly from the structure instead. > > > > > > > > > > > I suggest having a "cluster-size" property, instead of > > > > "mp-affinity". This way the mp_affinity field can be calculated > > > > on realize, based on the configured cluster-size. > > > > > > This won't work for Raspberry Pi's MPIDR adjustment. > > > > Where's that code? > > hw/arm/bcm2836.c:109 Thanks! > > > > > > Anyway, IMO realize should only set values when it knows > > > nothing has been set. > > > > Realize could also initialize some (private) fields using other > > (public) fields as input. Instead of making machine code > > calculate mp_affinity, machine code could just provide input for > > realize to calculate the right values. > > > > But maybe my cluster-size suggestion won't work anyway because of > > other cases where mp_affinity is set (I didn't check all code > > that sets/gets mp_affinity). > > Anyway, cluster-size is machine state, not cpu state. Right: if in this case the realize function can query current_machine for input instead of requiring a new field/property to be used as input, that's even better. But sometimes we can't easily avoid adding fields/properties that aren't device state, to be used as input to realize (e.g. nr_cores/nr_threads). (This is just
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On Tue, Oct 18, 2016 at 11:18:29AM -0200, Eduardo Habkost wrote: > On Tue, Oct 18, 2016 at 03:00:07PM +0200, Andrew Jones wrote: > > On Mon, Oct 17, 2016 at 05:20:22PM -0200, Eduardo Habkost wrote: > > > On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote: > > > > Modify all CPUs to call it from XXX_cpu_realizefn() function. > > > > > > > > Remove all the cannot_destroy_with_object_finalize_yet as > > > > unsafe references have been moved to cpu_exec_realizefn(). > > > > (tested with QOM command provided by commit 4c315c27) > > > > > > > > for arm: > > > > > > > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn() > > > > to arm_cpu_realizefn() as setting of cpu_index is now done > > > > in cpu_exec_realizefn(). > > > > > > > > Signed-off-by: Laurent Vivier> > > [...] > > > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > > > > index 1b9540e..364a45d 100644 > > > > --- a/target-arm/cpu.c > > > > +++ b/target-arm/cpu.c > > > > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj) > > > > CPUState *cs = CPU(obj); > > > > ARMCPU *cpu = ARM_CPU(obj); > > > > static bool inited; > > > > -uint32_t Aff1, Aff0; > > > > > > > > cs->env_ptr = >env; > > > > -cpu_exec_init(cs, _abort); > > > > cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, > > > > g_free, g_free); > > > > > > > > -/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will > > > > override it. > > > > - * We don't support setting cluster ID ([16..23]) (known as Aff2 > > > > - * in later ARM ARM versions), or any of the higher affinity level > > > > fields, > > > > - * so these bits always RAZ. > > > > - */ > > > > -Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > > > > -Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > > > > -cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > > > > - > > > > #ifndef CONFIG_USER_ONLY > > > > /* Our inbound IRQ and FIQ lines */ > > > > if (kvm_enabled()) { > > > [...] > > > > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, > > > > Error **errp) > > > > set_feature(env, ARM_FEATURE_THUMB_DSP); > > > > } > > > > > > > > +/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will > > > > override it. > > > > + * We don't support setting cluster ID ([16..23]) (known as Aff2 > > > > + * in later ARM ARM versions), or any of the higher affinity level > > > > fields, > > > > + * so these bits always RAZ. > > > > + */ > > > > +Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > > > > +Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > > > > +cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > > > > + > > > > > > This will override any value set in the "mp-affinity" property, > > > The mp-affinity property can be set by the user in the > > > command-line, and it is also set by machvirt_init() in > > > hw/arm/virt.c. > > > > I'm glad you noticed this Eduardo. We'd indeed lose changes made to > > the MPIDRs for mach-virt and Raspberry Pi. > > > > > > > > Considering that each CPU is supposed to have a different value, > > > I doubt there are existing use cases for mp-affinity being set > > > directly by the user. > > > > Probably not. It was turned into a property in order for the gicv3 > > model to access it. I don't think that's necessary, so we can just > > un-property it, accessing it directly from the structure instead. > > > > > > > > I suggest having a "cluster-size" property, instead of > > > "mp-affinity". This way the mp_affinity field can be calculated > > > on realize, based on the configured cluster-size. > > > > This won't work for Raspberry Pi's MPIDR adjustment. > > Where's that code? hw/arm/bcm2836.c:109 > > > Anyway, IMO realize should only set values when it knows > > nothing has been set. > > Realize could also initialize some (private) fields using other > (public) fields as input. Instead of making machine code > calculate mp_affinity, machine code could just provide input for > realize to calculate the right values. > > But maybe my cluster-size suggestion won't work anyway because of > other cases where mp_affinity is set (I didn't check all code > that sets/gets mp_affinity). Anyway, cluster-size is machine state, not cpu state. > > > If values have been set, it should only sanity check them. In this > > case it's safe to simply check for uniqueness and zeros; > > > > 1) The MPIDRs are all zero. As they're not all unique that's not > > valid. It's pretty safe to assume they just weren't set in this > > case though, so we can set the default values without complaining. > > If there was only one cpu, then MPIDR=0 is valid, but that's the > > default anyway. > > Implementing this check in arm_cpu_realizefn() would be > difficult, as the CPU realize method don't have access to the > other CPUs. But maybe we can
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On Tue, Oct 18, 2016 at 03:00:07PM +0200, Andrew Jones wrote: > On Mon, Oct 17, 2016 at 05:20:22PM -0200, Eduardo Habkost wrote: > > On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote: > > > Modify all CPUs to call it from XXX_cpu_realizefn() function. > > > > > > Remove all the cannot_destroy_with_object_finalize_yet as > > > unsafe references have been moved to cpu_exec_realizefn(). > > > (tested with QOM command provided by commit 4c315c27) > > > > > > for arm: > > > > > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn() > > > to arm_cpu_realizefn() as setting of cpu_index is now done > > > in cpu_exec_realizefn(). > > > > > > Signed-off-by: Laurent Vivier> > [...] > > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > > > index 1b9540e..364a45d 100644 > > > --- a/target-arm/cpu.c > > > +++ b/target-arm/cpu.c > > > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj) > > > CPUState *cs = CPU(obj); > > > ARMCPU *cpu = ARM_CPU(obj); > > > static bool inited; > > > -uint32_t Aff1, Aff0; > > > > > > cs->env_ptr = >env; > > > -cpu_exec_init(cs, _abort); > > > cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, > > > g_free, g_free); > > > > > > -/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will > > > override it. > > > - * We don't support setting cluster ID ([16..23]) (known as Aff2 > > > - * in later ARM ARM versions), or any of the higher affinity level > > > fields, > > > - * so these bits always RAZ. > > > - */ > > > -Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > > > -Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > > > -cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > > > - > > > #ifndef CONFIG_USER_ONLY > > > /* Our inbound IRQ and FIQ lines */ > > > if (kvm_enabled()) { > > [...] > > > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, > > > Error **errp) > > > set_feature(env, ARM_FEATURE_THUMB_DSP); > > > } > > > > > > +/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will > > > override it. > > > + * We don't support setting cluster ID ([16..23]) (known as Aff2 > > > + * in later ARM ARM versions), or any of the higher affinity level > > > fields, > > > + * so these bits always RAZ. > > > + */ > > > +Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > > > +Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > > > +cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > > > + > > > > This will override any value set in the "mp-affinity" property, > > The mp-affinity property can be set by the user in the > > command-line, and it is also set by machvirt_init() in > > hw/arm/virt.c. > > I'm glad you noticed this Eduardo. We'd indeed lose changes made to > the MPIDRs for mach-virt and Raspberry Pi. > > > > > Considering that each CPU is supposed to have a different value, > > I doubt there are existing use cases for mp-affinity being set > > directly by the user. > > Probably not. It was turned into a property in order for the gicv3 > model to access it. I don't think that's necessary, so we can just > un-property it, accessing it directly from the structure instead. > > > > > I suggest having a "cluster-size" property, instead of > > "mp-affinity". This way the mp_affinity field can be calculated > > on realize, based on the configured cluster-size. > > This won't work for Raspberry Pi's MPIDR adjustment. Where's that code? > Anyway, IMO realize should only set values when it knows > nothing has been set. Realize could also initialize some (private) fields using other (public) fields as input. Instead of making machine code calculate mp_affinity, machine code could just provide input for realize to calculate the right values. But maybe my cluster-size suggestion won't work anyway because of other cases where mp_affinity is set (I didn't check all code that sets/gets mp_affinity). > If values have been set, it should only sanity check them. In this > case it's safe to simply check for uniqueness and zeros; > > 1) The MPIDRs are all zero. As they're not all unique that's not > valid. It's pretty safe to assume they just weren't set in this > case though, so we can set the default values without complaining. > If there was only one cpu, then MPIDR=0 is valid, but that's the > default anyway. Implementing this check in arm_cpu_realizefn() would be difficult, as the CPU realize method don't have access to the other CPUs. But maybe we can use some other default value to indicate that the field was never set? UINT64_MAX? > 2) The MPIDRs are all unique, or there's a single cpu and its MPIDR > is not zero. In this case we shouldn't touch them. > 3) The MPIDRs are not all zero and not all unique. This is a failed > sanity check and should abort. A sanity check would be useful, but I don't know
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On Mon, Oct 17, 2016 at 05:20:22PM -0200, Eduardo Habkost wrote: > On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote: > > Modify all CPUs to call it from XXX_cpu_realizefn() function. > > > > Remove all the cannot_destroy_with_object_finalize_yet as > > unsafe references have been moved to cpu_exec_realizefn(). > > (tested with QOM command provided by commit 4c315c27) > > > > for arm: > > > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn() > > to arm_cpu_realizefn() as setting of cpu_index is now done > > in cpu_exec_realizefn(). > > > > Signed-off-by: Laurent Vivier> [...] > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > > index 1b9540e..364a45d 100644 > > --- a/target-arm/cpu.c > > +++ b/target-arm/cpu.c > > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj) > > CPUState *cs = CPU(obj); > > ARMCPU *cpu = ARM_CPU(obj); > > static bool inited; > > -uint32_t Aff1, Aff0; > > > > cs->env_ptr = >env; > > -cpu_exec_init(cs, _abort); > > cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, > > g_free, g_free); > > > > -/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will > > override it. > > - * We don't support setting cluster ID ([16..23]) (known as Aff2 > > - * in later ARM ARM versions), or any of the higher affinity level > > fields, > > - * so these bits always RAZ. > > - */ > > -Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > > -Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > > -cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > > - > > #ifndef CONFIG_USER_ONLY > > /* Our inbound IRQ and FIQ lines */ > > if (kvm_enabled()) { > [...] > > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error > > **errp) > > set_feature(env, ARM_FEATURE_THUMB_DSP); > > } > > > > +/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will > > override it. > > + * We don't support setting cluster ID ([16..23]) (known as Aff2 > > + * in later ARM ARM versions), or any of the higher affinity level > > fields, > > + * so these bits always RAZ. > > + */ > > +Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > > +Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > > +cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > > + > > This will override any value set in the "mp-affinity" property, > The mp-affinity property can be set by the user in the > command-line, and it is also set by machvirt_init() in > hw/arm/virt.c. I'm glad you noticed this Eduardo. We'd indeed lose changes made to the MPIDRs for mach-virt and Raspberry Pi. > > Considering that each CPU is supposed to have a different value, > I doubt there are existing use cases for mp-affinity being set > directly by the user. Probably not. It was turned into a property in order for the gicv3 model to access it. I don't think that's necessary, so we can just un-property it, accessing it directly from the structure instead. > > I suggest having a "cluster-size" property, instead of > "mp-affinity". This way the mp_affinity field can be calculated > on realize, based on the configured cluster-size. This won't work for Raspberry Pi's MPIDR adjustment. Anyway, IMO realize should only set values when it knows nothing has been set. If values have been set, it should only sanity check them. In this case it's safe to simply check for uniqueness and zeros; 1) The MPIDRs are all zero. As they're not all unique that's not valid. It's pretty safe to assume they just weren't set in this case though, so we can set the default values without complaining. If there was only one cpu, then MPIDR=0 is valid, but that's the default anyway. 2) The MPIDRs are all unique, or there's a single cpu and its MPIDR is not zero. In this case we shouldn't touch them. 3) The MPIDRs are not all zero and not all unique. This is a failed sanity check and should abort. Thanks, drew > > > if (cpu->reset_hivecs) { > > cpu->reset_sctlr |= (1 << 13); > > } > [...] > > -- > Eduardo >
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On Mon, 17 Oct 2016 17:20:22 -0200 Eduardo Habkostwrote: > On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote: > > Modify all CPUs to call it from XXX_cpu_realizefn() function. > > > > Remove all the cannot_destroy_with_object_finalize_yet as > > unsafe references have been moved to cpu_exec_realizefn(). > > (tested with QOM command provided by commit 4c315c27) > > > > for arm: > > > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn() > > to arm_cpu_realizefn() as setting of cpu_index is now done > > in cpu_exec_realizefn(). > > > > Signed-off-by: Laurent Vivier > [...] > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > > index 1b9540e..364a45d 100644 > > --- a/target-arm/cpu.c > > +++ b/target-arm/cpu.c > > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj) > > CPUState *cs = CPU(obj); > > ARMCPU *cpu = ARM_CPU(obj); > > static bool inited; > > -uint32_t Aff1, Aff0; > > > > cs->env_ptr = >env; > > -cpu_exec_init(cs, _abort); > > cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, > > g_free, g_free); > > > > -/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will > > override it. > > - * We don't support setting cluster ID ([16..23]) (known as Aff2 > > - * in later ARM ARM versions), or any of the higher affinity level > > fields, > > - * so these bits always RAZ. > > - */ > > -Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > > -Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > > -cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > > - > > #ifndef CONFIG_USER_ONLY > > /* Our inbound IRQ and FIQ lines */ > > if (kvm_enabled()) { > [...] > > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error > > **errp) > > set_feature(env, ARM_FEATURE_THUMB_DSP); > > } > > > > +/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will > > override it. > > + * We don't support setting cluster ID ([16..23]) (known as Aff2 > > + * in later ARM ARM versions), or any of the higher affinity level > > fields, > > + * so these bits always RAZ. > > + */ > > +Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > > +Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > > +cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > > + > > This will override any value set in the "mp-affinity" property, > The mp-affinity property can be set by the user in the > command-line, and it is also set by machvirt_init() in > hw/arm/virt.c. > > Considering that each CPU is supposed to have a different value, > I doubt there are existing use cases for mp-affinity being set > directly by the user. > > I suggest having a "cluster-size" property, instead of > "mp-affinity". This way the mp_affinity field can be calculated > on realize, based on the configured cluster-size. CCing Drew as he knows more about MPIDR stuff more. > > > if (cpu->reset_hivecs) { > > cpu->reset_sctlr |= (1 << 13); > > } > [...] >
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote: > Modify all CPUs to call it from XXX_cpu_realizefn() function. > > Remove all the cannot_destroy_with_object_finalize_yet as > unsafe references have been moved to cpu_exec_realizefn(). > (tested with QOM command provided by commit 4c315c27) > > for arm: > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn() > to arm_cpu_realizefn() as setting of cpu_index is now done > in cpu_exec_realizefn(). > > Signed-off-by: Laurent Vivier[...] > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 1b9540e..364a45d 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj) > CPUState *cs = CPU(obj); > ARMCPU *cpu = ARM_CPU(obj); > static bool inited; > -uint32_t Aff1, Aff0; > > cs->env_ptr = >env; > -cpu_exec_init(cs, _abort); > cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, > g_free, g_free); > > -/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override > it. > - * We don't support setting cluster ID ([16..23]) (known as Aff2 > - * in later ARM ARM versions), or any of the higher affinity level > fields, > - * so these bits always RAZ. > - */ > -Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > -Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > -cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > - > #ifndef CONFIG_USER_ONLY > /* Our inbound IRQ and FIQ lines */ > if (kvm_enabled()) { [...] > @@ -631,6 +628,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error > **errp) > set_feature(env, ARM_FEATURE_THUMB_DSP); > } > > +/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override > it. > + * We don't support setting cluster ID ([16..23]) (known as Aff2 > + * in later ARM ARM versions), or any of the higher affinity level > fields, > + * so these bits always RAZ. > + */ > +Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; > +Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; > +cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; > + This will override any value set in the "mp-affinity" property, The mp-affinity property can be set by the user in the command-line, and it is also set by machvirt_init() in hw/arm/virt.c. Considering that each CPU is supposed to have a different value, I doubt there are existing use cases for mp-affinity being set directly by the user. I suggest having a "cluster-size" property, instead of "mp-affinity". This way the mp_affinity field can be calculated on realize, based on the configured cluster-size. > if (cpu->reset_hivecs) { > cpu->reset_sctlr |= (1 << 13); > } [...] -- Eduardo
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote: > Modify all CPUs to call it from XXX_cpu_realizefn() function. > > Remove all the cannot_destroy_with_object_finalize_yet as > unsafe references have been moved to cpu_exec_realizefn(). > (tested with QOM command provided by commit 4c315c27) > > for arm: > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn() > to arm_cpu_realizefn() as setting of cpu_index is now done > in cpu_exec_realizefn(). > > Signed-off-by: Laurent VivierSubject line is misleading: it looks like a trivial function rename, but there are functional changes in multiple architectures. Do you mind if subject is changed to "move cpu_exec_init() calls to realize functions" when committing? That's the main purpose of the patch, the rename is just a detail. I liked to have the "move cpu_exec_realize() to realize function" part split per architecture in v1/v2. But the rename would touch all architectures anyway, and making this as a single patch would help us avoid leaving some architecture behind. > --- > exec.c | 2 +- > include/exec/exec-all.h | 1 - > include/qom/cpu.h | 1 + > target-alpha/cpu.c | 15 +++ > target-arm/cpu.c| 39 +-- > target-cris/cpu.c | 15 +++ > target-i386/cpu.c | 11 +-- > target-lm32/cpu.c | 15 +++ > target-m68k/cpu.c | 15 +++ > target-microblaze/cpu.c | 14 +++--- > target-mips/cpu.c | 15 +++ > target-moxie/cpu.c | 15 +++ > target-openrisc/cpu.c | 15 +++ > target-ppc/translate_init.c | 2 +- > target-s390x/cpu.c | 8 +--- > target-sh4/cpu.c| 15 +++ > target-sparc/cpu.c | 18 +- > target-tilegx/cpu.c | 15 +++ > target-tricore/cpu.c| 15 +++ > target-unicore32/cpu.c | 18 +- > target-xtensa/cpu.c | 15 +++ > 21 files changed, 128 insertions(+), 151 deletions(-) > > diff --git a/exec.c b/exec.c > index d1e57c4..203eb52 100644 > --- a/exec.c > +++ b/exec.c > @@ -634,7 +634,7 @@ void cpu_exec_initfn(CPUState *cpu) > #endif > } > > -void cpu_exec_init(CPUState *cpu, Error **errp) > +void cpu_exec_realizefn(CPUState *cpu, Error **errp) > { > CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu); > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 336a57c..9797d55 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -57,7 +57,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu, >uint32_t flags, >int cflags); > > -void cpu_exec_init(CPUState *cpu, Error **errp); > void QEMU_NORETURN cpu_loop_exit(CPUState *cpu); > void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc); > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index d7648a9..5520c6c 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -947,6 +947,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int > asidx); > void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...) > GCC_FMT_ATTR(2, 3); > void cpu_exec_initfn(CPUState *cpu); > +void cpu_exec_realizefn(CPUState *cpu, Error **errp); > void cpu_exec_exit(CPUState *cpu); > > #ifdef CONFIG_SOFTMMU > diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c > index 6d01d7f..30d77ce 100644 > --- a/target-alpha/cpu.c > +++ b/target-alpha/cpu.c > @@ -59,6 +59,13 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error > **errp) > { > CPUState *cs = CPU(dev); > AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev); > +Error *local_err = NULL; > + > +cpu_exec_realizefn(cs, _err); > +if (local_err != NULL) { > +error_propagate(errp, local_err); > +return; > +} > > qemu_init_vcpu(cs); > > @@ -266,7 +273,6 @@ static void alpha_cpu_initfn(Object *obj) > CPUAlphaState *env = >env; > > cs->env_ptr = env; > -cpu_exec_init(cs, _abort); > tlb_flush(cs, 1); > > alpha_translate_init(); > @@ -309,13 +315,6 @@ static void alpha_cpu_class_init(ObjectClass *oc, void > *data) > cc->disas_set_info = alpha_cpu_disas_set_info; > > cc->gdb_num_core_regs = 67; > - > -/* > - * Reason: alpha_cpu_initfn() calls cpu_exec_init(), which saves > - * the object in cpus -> dangling pointer after final > - * object_unref(). > - */ > -dc->cannot_destroy_with_object_finalize_yet = true; > } > > static const TypeInfo alpha_cpu_type_info = { > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 1b9540e..364a45d 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj) > CPUState *cs =
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On 17/10/2016 16:03, Eduardo Habkost wrote: > On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote: >> Modify all CPUs to call it from XXX_cpu_realizefn() function. >> >> Remove all the cannot_destroy_with_object_finalize_yet as >> unsafe references have been moved to cpu_exec_realizefn(). >> (tested with QOM command provided by commit 4c315c27) >> >> for arm: >> >> Setting of cpu->mp_affinity is moved from arm_cpu_initfn() >> to arm_cpu_realizefn() as setting of cpu_index is now done >> in cpu_exec_realizefn(). >> >> Signed-off-by: Laurent Vivier> > Subject line is misleading: it looks like a trivial function > rename, but there are functional changes in multiple > architectures. > > Do you mind if subject is changed to "move cpu_exec_init() calls > to realize functions" when committing? That's the main purpose of > the patch, the rename is just a detail. I agree. Thanks, Laurent > I liked to have the "move cpu_exec_realize() to realize function" > part split per architecture in v1/v2. But the rename would touch > all architectures anyway, and making this as a single patch would > help us avoid leaving some architecture behind. > > >> --- >> exec.c | 2 +- >> include/exec/exec-all.h | 1 - >> include/qom/cpu.h | 1 + >> target-alpha/cpu.c | 15 +++ >> target-arm/cpu.c| 39 +-- >> target-cris/cpu.c | 15 +++ >> target-i386/cpu.c | 11 +-- >> target-lm32/cpu.c | 15 +++ >> target-m68k/cpu.c | 15 +++ >> target-microblaze/cpu.c | 14 +++--- >> target-mips/cpu.c | 15 +++ >> target-moxie/cpu.c | 15 +++ >> target-openrisc/cpu.c | 15 +++ >> target-ppc/translate_init.c | 2 +- >> target-s390x/cpu.c | 8 +--- >> target-sh4/cpu.c| 15 +++ >> target-sparc/cpu.c | 18 +- >> target-tilegx/cpu.c | 15 +++ >> target-tricore/cpu.c| 15 +++ >> target-unicore32/cpu.c | 18 +- >> target-xtensa/cpu.c | 15 +++ >> 21 files changed, 128 insertions(+), 151 deletions(-) >> >> diff --git a/exec.c b/exec.c >> index d1e57c4..203eb52 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -634,7 +634,7 @@ void cpu_exec_initfn(CPUState *cpu) >> #endif >> } >> >> -void cpu_exec_init(CPUState *cpu, Error **errp) >> +void cpu_exec_realizefn(CPUState *cpu, Error **errp) >> { >> CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu); >> >> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h >> index 336a57c..9797d55 100644 >> --- a/include/exec/exec-all.h >> +++ b/include/exec/exec-all.h >> @@ -57,7 +57,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu, >>uint32_t flags, >>int cflags); >> >> -void cpu_exec_init(CPUState *cpu, Error **errp); >> void QEMU_NORETURN cpu_loop_exit(CPUState *cpu); >> void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc); >> >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h >> index d7648a9..5520c6c 100644 >> --- a/include/qom/cpu.h >> +++ b/include/qom/cpu.h >> @@ -947,6 +947,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int >> asidx); >> void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...) >> GCC_FMT_ATTR(2, 3); >> void cpu_exec_initfn(CPUState *cpu); >> +void cpu_exec_realizefn(CPUState *cpu, Error **errp); >> void cpu_exec_exit(CPUState *cpu); >> >> #ifdef CONFIG_SOFTMMU >> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c >> index 6d01d7f..30d77ce 100644 >> --- a/target-alpha/cpu.c >> +++ b/target-alpha/cpu.c >> @@ -59,6 +59,13 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error >> **errp) >> { >> CPUState *cs = CPU(dev); >> AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev); >> +Error *local_err = NULL; >> + >> +cpu_exec_realizefn(cs, _err); >> +if (local_err != NULL) { >> +error_propagate(errp, local_err); >> +return; >> +} >> >> qemu_init_vcpu(cs); >> >> @@ -266,7 +273,6 @@ static void alpha_cpu_initfn(Object *obj) >> CPUAlphaState *env = >env; >> >> cs->env_ptr = env; >> -cpu_exec_init(cs, _abort); >> tlb_flush(cs, 1); >> >> alpha_translate_init(); >> @@ -309,13 +315,6 @@ static void alpha_cpu_class_init(ObjectClass *oc, void >> *data) >> cc->disas_set_info = alpha_cpu_disas_set_info; >> >> cc->gdb_num_core_regs = 67; >> - >> -/* >> - * Reason: alpha_cpu_initfn() calls cpu_exec_init(), which saves >> - * the object in cpus -> dangling pointer after final >> - * object_unref(). >> - */ >> -dc->cannot_destroy_with_object_finalize_yet = true; >> } >> >> static const TypeInfo alpha_cpu_type_info = { >> diff
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On Sat, 15 Oct 2016 00:52:48 +0200 Laurent Vivierwrote: > Modify all CPUs to call it from XXX_cpu_realizefn() function. > > Remove all the cannot_destroy_with_object_finalize_yet as > unsafe references have been moved to cpu_exec_realizefn(). > (tested with QOM command provided by commit 4c315c27) > > for arm: > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn() > to arm_cpu_realizefn() as setting of cpu_index is now done > in cpu_exec_realizefn(). > > Signed-off-by: Laurent Vivier For target-i386 part: Reviewed-by: Igor Mammedov > --- > exec.c | 2 +- > include/exec/exec-all.h | 1 - > include/qom/cpu.h | 1 + > target-alpha/cpu.c | 15 +++ > target-arm/cpu.c| 39 +-- > target-cris/cpu.c | 15 +++ > target-i386/cpu.c | 11 +-- > target-lm32/cpu.c | 15 +++ > target-m68k/cpu.c | 15 +++ > target-microblaze/cpu.c | 14 +++--- > target-mips/cpu.c | 15 +++ > target-moxie/cpu.c | 15 +++ > target-openrisc/cpu.c | 15 +++ > target-ppc/translate_init.c | 2 +- > target-s390x/cpu.c | 8 +--- > target-sh4/cpu.c| 15 +++ > target-sparc/cpu.c | 18 +- > target-tilegx/cpu.c | 15 +++ > target-tricore/cpu.c| 15 +++ > target-unicore32/cpu.c | 18 +- > target-xtensa/cpu.c | 15 +++ > 21 files changed, 128 insertions(+), 151 deletions(-) > > diff --git a/exec.c b/exec.c > index d1e57c4..203eb52 100644 > --- a/exec.c > +++ b/exec.c > @@ -634,7 +634,7 @@ void cpu_exec_initfn(CPUState *cpu) > #endif > } > > -void cpu_exec_init(CPUState *cpu, Error **errp) > +void cpu_exec_realizefn(CPUState *cpu, Error **errp) > { > CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu); > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 336a57c..9797d55 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -57,7 +57,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu, >uint32_t flags, >int cflags); > > -void cpu_exec_init(CPUState *cpu, Error **errp); > void QEMU_NORETURN cpu_loop_exit(CPUState *cpu); > void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc); > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index d7648a9..5520c6c 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -947,6 +947,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int > asidx); > void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...) > GCC_FMT_ATTR(2, 3); > void cpu_exec_initfn(CPUState *cpu); > +void cpu_exec_realizefn(CPUState *cpu, Error **errp); > void cpu_exec_exit(CPUState *cpu); > > #ifdef CONFIG_SOFTMMU > diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c > index 6d01d7f..30d77ce 100644 > --- a/target-alpha/cpu.c > +++ b/target-alpha/cpu.c > @@ -59,6 +59,13 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error > **errp) > { > CPUState *cs = CPU(dev); > AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev); > +Error *local_err = NULL; > + > +cpu_exec_realizefn(cs, _err); > +if (local_err != NULL) { > +error_propagate(errp, local_err); > +return; > +} > > qemu_init_vcpu(cs); > > @@ -266,7 +273,6 @@ static void alpha_cpu_initfn(Object *obj) > CPUAlphaState *env = >env; > > cs->env_ptr = env; > -cpu_exec_init(cs, _abort); > tlb_flush(cs, 1); > > alpha_translate_init(); > @@ -309,13 +315,6 @@ static void alpha_cpu_class_init(ObjectClass *oc, void > *data) > cc->disas_set_info = alpha_cpu_disas_set_info; > > cc->gdb_num_core_regs = 67; > - > -/* > - * Reason: alpha_cpu_initfn() calls cpu_exec_init(), which saves > - * the object in cpus -> dangling pointer after final > - * object_unref(). > - */ > -dc->cannot_destroy_with_object_finalize_yet = true; > } > > static const TypeInfo alpha_cpu_type_info = { > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 1b9540e..364a45d 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj) > CPUState *cs = CPU(obj); > ARMCPU *cpu = ARM_CPU(obj); > static bool inited; > -uint32_t Aff1, Aff0; > > cs->env_ptr = >env; > -cpu_exec_init(cs, _abort); > cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, > g_free, g_free); > > -/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override > it. > - * We don't support setting cluster ID ([16..23]) (known as Aff2 > -
Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
On Sat, Oct 15, 2016 at 12:52:48AM +0200, Laurent Vivier wrote: > Modify all CPUs to call it from XXX_cpu_realizefn() function. > > Remove all the cannot_destroy_with_object_finalize_yet as > unsafe references have been moved to cpu_exec_realizefn(). > (tested with QOM command provided by commit 4c315c27) > > for arm: > > Setting of cpu->mp_affinity is moved from arm_cpu_initfn() > to arm_cpu_realizefn() as setting of cpu_index is now done > in cpu_exec_realizefn(). > > Signed-off-by: Laurent VivierReviewed-by: David Gibson > --- > exec.c | 2 +- > include/exec/exec-all.h | 1 - > include/qom/cpu.h | 1 + > target-alpha/cpu.c | 15 +++ > target-arm/cpu.c| 39 +-- > target-cris/cpu.c | 15 +++ > target-i386/cpu.c | 11 +-- > target-lm32/cpu.c | 15 +++ > target-m68k/cpu.c | 15 +++ > target-microblaze/cpu.c | 14 +++--- > target-mips/cpu.c | 15 +++ > target-moxie/cpu.c | 15 +++ > target-openrisc/cpu.c | 15 +++ > target-ppc/translate_init.c | 2 +- > target-s390x/cpu.c | 8 +--- > target-sh4/cpu.c| 15 +++ > target-sparc/cpu.c | 18 +- > target-tilegx/cpu.c | 15 +++ > target-tricore/cpu.c| 15 +++ > target-unicore32/cpu.c | 18 +- > target-xtensa/cpu.c | 15 +++ > 21 files changed, 128 insertions(+), 151 deletions(-) > > diff --git a/exec.c b/exec.c > index d1e57c4..203eb52 100644 > --- a/exec.c > +++ b/exec.c > @@ -634,7 +634,7 @@ void cpu_exec_initfn(CPUState *cpu) > #endif > } > > -void cpu_exec_init(CPUState *cpu, Error **errp) > +void cpu_exec_realizefn(CPUState *cpu, Error **errp) > { > CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu); > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 336a57c..9797d55 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -57,7 +57,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu, >uint32_t flags, >int cflags); > > -void cpu_exec_init(CPUState *cpu, Error **errp); > void QEMU_NORETURN cpu_loop_exit(CPUState *cpu); > void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc); > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index d7648a9..5520c6c 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -947,6 +947,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int > asidx); > void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...) > GCC_FMT_ATTR(2, 3); > void cpu_exec_initfn(CPUState *cpu); > +void cpu_exec_realizefn(CPUState *cpu, Error **errp); > void cpu_exec_exit(CPUState *cpu); > > #ifdef CONFIG_SOFTMMU > diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c > index 6d01d7f..30d77ce 100644 > --- a/target-alpha/cpu.c > +++ b/target-alpha/cpu.c > @@ -59,6 +59,13 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error > **errp) > { > CPUState *cs = CPU(dev); > AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev); > +Error *local_err = NULL; > + > +cpu_exec_realizefn(cs, _err); > +if (local_err != NULL) { > +error_propagate(errp, local_err); > +return; > +} > > qemu_init_vcpu(cs); > > @@ -266,7 +273,6 @@ static void alpha_cpu_initfn(Object *obj) > CPUAlphaState *env = >env; > > cs->env_ptr = env; > -cpu_exec_init(cs, _abort); > tlb_flush(cs, 1); > > alpha_translate_init(); > @@ -309,13 +315,6 @@ static void alpha_cpu_class_init(ObjectClass *oc, void > *data) > cc->disas_set_info = alpha_cpu_disas_set_info; > > cc->gdb_num_core_regs = 67; > - > -/* > - * Reason: alpha_cpu_initfn() calls cpu_exec_init(), which saves > - * the object in cpus -> dangling pointer after final > - * object_unref(). > - */ > -dc->cannot_destroy_with_object_finalize_yet = true; > } > > static const TypeInfo alpha_cpu_type_info = { > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 1b9540e..364a45d 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj) > CPUState *cs = CPU(obj); > ARMCPU *cpu = ARM_CPU(obj); > static bool inited; > -uint32_t Aff1, Aff0; > > cs->env_ptr = >env; > -cpu_exec_init(cs, _abort); > cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, > g_free, g_free); > > -/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override > it. > - * We don't support setting cluster ID ([16..23]) (known as Aff2 > - * in later ARM ARM versions),
[Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()
Modify all CPUs to call it from XXX_cpu_realizefn() function. Remove all the cannot_destroy_with_object_finalize_yet as unsafe references have been moved to cpu_exec_realizefn(). (tested with QOM command provided by commit 4c315c27) for arm: Setting of cpu->mp_affinity is moved from arm_cpu_initfn() to arm_cpu_realizefn() as setting of cpu_index is now done in cpu_exec_realizefn(). Signed-off-by: Laurent Vivier--- exec.c | 2 +- include/exec/exec-all.h | 1 - include/qom/cpu.h | 1 + target-alpha/cpu.c | 15 +++ target-arm/cpu.c| 39 +-- target-cris/cpu.c | 15 +++ target-i386/cpu.c | 11 +-- target-lm32/cpu.c | 15 +++ target-m68k/cpu.c | 15 +++ target-microblaze/cpu.c | 14 +++--- target-mips/cpu.c | 15 +++ target-moxie/cpu.c | 15 +++ target-openrisc/cpu.c | 15 +++ target-ppc/translate_init.c | 2 +- target-s390x/cpu.c | 8 +--- target-sh4/cpu.c| 15 +++ target-sparc/cpu.c | 18 +- target-tilegx/cpu.c | 15 +++ target-tricore/cpu.c| 15 +++ target-unicore32/cpu.c | 18 +- target-xtensa/cpu.c | 15 +++ 21 files changed, 128 insertions(+), 151 deletions(-) diff --git a/exec.c b/exec.c index d1e57c4..203eb52 100644 --- a/exec.c +++ b/exec.c @@ -634,7 +634,7 @@ void cpu_exec_initfn(CPUState *cpu) #endif } -void cpu_exec_init(CPUState *cpu, Error **errp) +void cpu_exec_realizefn(CPUState *cpu, Error **errp) { CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu); diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 336a57c..9797d55 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -57,7 +57,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu, uint32_t flags, int cflags); -void cpu_exec_init(CPUState *cpu, Error **errp); void QEMU_NORETURN cpu_loop_exit(CPUState *cpu); void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc); diff --git a/include/qom/cpu.h b/include/qom/cpu.h index d7648a9..5520c6c 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -947,6 +947,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx); void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...) GCC_FMT_ATTR(2, 3); void cpu_exec_initfn(CPUState *cpu); +void cpu_exec_realizefn(CPUState *cpu, Error **errp); void cpu_exec_exit(CPUState *cpu); #ifdef CONFIG_SOFTMMU diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c index 6d01d7f..30d77ce 100644 --- a/target-alpha/cpu.c +++ b/target-alpha/cpu.c @@ -59,6 +59,13 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp) { CPUState *cs = CPU(dev); AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev); +Error *local_err = NULL; + +cpu_exec_realizefn(cs, _err); +if (local_err != NULL) { +error_propagate(errp, local_err); +return; +} qemu_init_vcpu(cs); @@ -266,7 +273,6 @@ static void alpha_cpu_initfn(Object *obj) CPUAlphaState *env = >env; cs->env_ptr = env; -cpu_exec_init(cs, _abort); tlb_flush(cs, 1); alpha_translate_init(); @@ -309,13 +315,6 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data) cc->disas_set_info = alpha_cpu_disas_set_info; cc->gdb_num_core_regs = 67; - -/* - * Reason: alpha_cpu_initfn() calls cpu_exec_init(), which saves - * the object in cpus -> dangling pointer after final - * object_unref(). - */ -dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo alpha_cpu_type_info = { diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 1b9540e..364a45d 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -441,22 +441,11 @@ static void arm_cpu_initfn(Object *obj) CPUState *cs = CPU(obj); ARMCPU *cpu = ARM_CPU(obj); static bool inited; -uint32_t Aff1, Aff0; cs->env_ptr = >env; -cpu_exec_init(cs, _abort); cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free); -/* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it. - * We don't support setting cluster ID ([16..23]) (known as Aff2 - * in later ARM ARM versions), or any of the higher affinity level fields, - * so these bits always RAZ. - */ -Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER; -Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER; -cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0; - #ifndef CONFIG_USER_ONLY /* Our inbound IRQ and FIQ lines */ if (kvm_enabled()) { @@ -576,6 +565,14 @@ static void