| Message ID | 1473833343-31531-3-git-send-email-wei@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Sep 14, 2016 at 02:09:03AM -0400, Wei Huang wrote: > CPU vPMU is now turned off by default, but it was ON in virt-2.7 > machine type. To solve this problem, this patch adds a PMU option > in machine state, which is used to control CPU's vPMU status. This > PMU option is not exposed to command line and is turned on in > virt-2.7 machine type to make sure it is backward compatible. > > Signed-off-by: Wei Huang <wei@redhat.com> > --- > hw/arm/virt.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index a781ad0..83cfea7 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -91,6 +91,7 @@ typedef struct { > bool secure; > bool highmem; > int32_t gic_version; > + bool pmu; > } VirtMachineState; The compat bool doesn't need to be in machine state, so it should be in the class. See 'disallow_affinity_adjustment' in VirtMachineClass for an example of a adding a compat bool for a different reason. Also, I'd name it something like pmu_default_on. > > #define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt") > @@ -1317,6 +1318,11 @@ static void machvirt_init(MachineState *machine) > } > } > > + if (vms->pmu) { > + /* Note: the property name is "pmu", not "has_pmu" */ The comment is unnecessary. Please remove. > + object_property_set_bool(cpuobj, true, "pmu", NULL); > + } > + > if (object_property_find(cpuobj, "reset-cbar", NULL)) { > object_property_set_int(cpuobj, vbi->memmap[VIRT_CPUPERIPHS].base, > "reset-cbar", &error_abort); > @@ -1510,6 +1516,8 @@ static void virt_2_7_instance_init(Object *obj) > object_property_set_description(obj, "gic-version", > "Set GIC version. " > "Valid values are 2, 3 and host", NULL); > + /* Default PMU is on for 2.7 */ This comment is unnecessary. Please remove. > + vms->pmu = true; > } > > static void virt_machine_2_7_options(MachineClass *mc) > @@ -1522,7 +1530,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(2, 7) > > static void virt_2_6_instance_init(Object *obj) > { > + VirtMachineState *vms = VIRT_MACHINE(obj); > + > virt_2_7_instance_init(obj); > + /* Default PMU is off for 2.6 */ > + vms->pmu = false; You shouldn't need to touch 2.6 code. The compat bool is false by default (BSS init), which is the correct value for every machine type, except the 2.7 machine type. > } > > static void virt_machine_2_6_options(MachineClass *mc) > -- > 1.8.3.1 > > Thanks, drew
On Wed, Sep 14, 2016 at 10:37:19AM +0200, Andrew Jones wrote: > On Wed, Sep 14, 2016 at 02:09:03AM -0400, Wei Huang wrote: > > CPU vPMU is now turned off by default, but it was ON in virt-2.7 > > machine type. To solve this problem, this patch adds a PMU option > > in machine state, which is used to control CPU's vPMU status. This > > PMU option is not exposed to command line and is turned on in > > virt-2.7 machine type to make sure it is backward compatible. > > > > Signed-off-by: Wei Huang <wei@redhat.com> > > --- > > hw/arm/virt.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index a781ad0..83cfea7 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -91,6 +91,7 @@ typedef struct { > > bool secure; > > bool highmem; > > int32_t gic_version; > > + bool pmu; > > } VirtMachineState; > > The compat bool doesn't need to be in machine state, so it should > be in the class. See 'disallow_affinity_adjustment' in VirtMachineClass > for an example of a adding a compat bool for a different reason. Also, > I'd name it something like pmu_default_on. > > > > > #define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt") > > @@ -1317,6 +1318,11 @@ static void machvirt_init(MachineState *machine) > > } > > } > > > > + if (vms->pmu) { > > + /* Note: the property name is "pmu", not "has_pmu" */ > > The comment is unnecessary. Please remove. > > > + object_property_set_bool(cpuobj, true, "pmu", NULL); > > + } > > + > > if (object_property_find(cpuobj, "reset-cbar", NULL)) { > > object_property_set_int(cpuobj, vbi->memmap[VIRT_CPUPERIPHS].base, > > "reset-cbar", &error_abort); > > @@ -1510,6 +1516,8 @@ static void virt_2_7_instance_init(Object *obj) > > object_property_set_description(obj, "gic-version", > > "Set GIC version. " > > "Valid values are 2, 3 and host", NULL); > > + /* Default PMU is on for 2.7 */ > > This comment is unnecessary. Please remove. > > > + vms->pmu = true; > > } > > > > static void virt_machine_2_7_options(MachineClass *mc) > > @@ -1522,7 +1530,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(2, 7) > > > > static void virt_2_6_instance_init(Object *obj) > > { > > + VirtMachineState *vms = VIRT_MACHINE(obj); > > + > > virt_2_7_instance_init(obj); > > + /* Default PMU is off for 2.6 */ > > + vms->pmu = false; > > You shouldn't need to touch 2.6 code. The compat bool is false > by default (BSS init), which is the correct value for every > machine type, except the 2.7 machine type. Eh, scratch this last comment. Only 2.8 and later don't have to touch this bool. I momentarily forgot about our chaining. 2.6 has to reset 2.7's selection of true back to false, as it modifies 2.7's choices. It works that way with class state too, so 2.6 will still need the false setting after that change. In this case, a comment is warranted; one that says something like "PMU support was first introduced in 2.7, ensure it is disabled by default for 2.6 and down." Thanks, drew > > > } > > > > static void virt_machine_2_6_options(MachineClass *mc) > > -- > > 1.8.3.1 > > > > > > Thanks, > drew
On 09/14/2016 03:37 AM, Andrew Jones wrote: > On Wed, Sep 14, 2016 at 02:09:03AM -0400, Wei Huang wrote: >> CPU vPMU is now turned off by default, but it was ON in virt-2.7 >> machine type. To solve this problem, this patch adds a PMU option >> in machine state, which is used to control CPU's vPMU status. This >> PMU option is not exposed to command line and is turned on in >> virt-2.7 machine type to make sure it is backward compatible. >> >> Signed-off-by: Wei Huang <wei@redhat.com> >> --- >> hw/arm/virt.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index a781ad0..83cfea7 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -91,6 +91,7 @@ typedef struct { >> bool secure; >> bool highmem; >> int32_t gic_version; >> + bool pmu; >> } VirtMachineState; > > The compat bool doesn't need to be in machine state, so it should > be in the class. See 'disallow_affinity_adjustment' in VirtMachineClass > for an example of a adding a compat bool for a different reason. Also, > I'd name it something like pmu_default_on. This should be an easy fix. > >> >> #define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt") >> @@ -1317,6 +1318,11 @@ static void machvirt_init(MachineState *machine) >> } >> } >> >> + if (vms->pmu) { >> + /* Note: the property name is "pmu", not "has_pmu" */ > > The comment is unnecessary. Please remove. > I might still say something here. :-) The reason is the confusion of "pmu" as a command line property and "has_pmu" as a property of ARMCPU. Most other properties use the same naming, so they are OK. But PMU, due to naming difference, did cause trouble for me and took a while to debug. >> + object_property_set_bool(cpuobj, true, "pmu", NULL); >> + } >> + >> if (object_property_find(cpuobj, "reset-cbar", NULL)) { >> object_property_set_int(cpuobj, vbi->memmap[VIRT_CPUPERIPHS].base, >> "reset-cbar", &error_abort); >> @@ -1510,6 +1516,8 @@ static void virt_2_7_instance_init(Object *obj) >> object_property_set_description(obj, "gic-version", >> "Set GIC version. " >> "Valid values are 2, 3 and host", NULL); >> + /* Default PMU is on for 2.7 */ > > This comment is unnecessary. Please remove. OK > >> + vms->pmu = true; >> } >> >> static void virt_machine_2_7_options(MachineClass *mc) >> @@ -1522,7 +1530,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(2, 7) >> >> static void virt_2_6_instance_init(Object *obj) >> { >> + VirtMachineState *vms = VIRT_MACHINE(obj); >> + >> virt_2_7_instance_init(obj); >> + /* Default PMU is off for 2.6 */ >> + vms->pmu = false; > > You shouldn't need to touch 2.6 code. The compat bool is false > by default (BSS init), which is the correct value for every > machine type, except the 2.7 machine type. I will fix the comments, using the next email from you. > >> } >> >> static void virt_machine_2_6_options(MachineClass *mc) >> -- >> 1.8.3.1 >> >> > > Thanks, > drew >
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a781ad0..83cfea7 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -91,6 +91,7 @@ typedef struct { bool secure; bool highmem; int32_t gic_version; + bool pmu; } VirtMachineState; #define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt") @@ -1317,6 +1318,11 @@ static void machvirt_init(MachineState *machine) } } + if (vms->pmu) { + /* Note: the property name is "pmu", not "has_pmu" */ + object_property_set_bool(cpuobj, true, "pmu", NULL); + } + if (object_property_find(cpuobj, "reset-cbar", NULL)) { object_property_set_int(cpuobj, vbi->memmap[VIRT_CPUPERIPHS].base, "reset-cbar", &error_abort); @@ -1510,6 +1516,8 @@ static void virt_2_7_instance_init(Object *obj) object_property_set_description(obj, "gic-version", "Set GIC version. " "Valid values are 2, 3 and host", NULL); + /* Default PMU is on for 2.7 */ + vms->pmu = true; } static void virt_machine_2_7_options(MachineClass *mc) @@ -1522,7 +1530,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(2, 7) static void virt_2_6_instance_init(Object *obj) { + VirtMachineState *vms = VIRT_MACHINE(obj); + virt_2_7_instance_init(obj); + /* Default PMU is off for 2.6 */ + vms->pmu = false; } static void virt_machine_2_6_options(MachineClass *mc)
CPU vPMU is now turned off by default, but it was ON in virt-2.7 machine type. To solve this problem, this patch adds a PMU option in machine state, which is used to control CPU's vPMU status. This PMU option is not exposed to command line and is turned on in virt-2.7 machine type to make sure it is backward compatible. Signed-off-by: Wei Huang <wei@redhat.com> --- hw/arm/virt.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)