Re: [Qemu-devel] [PATCH V2 2/2] arm: virt: add PMU property to machvirt machine type

[Andrew Jones]

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 <address@hidden>
> > ---
> >  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