Re: [Qemu-devel] [PATCH V7 0/2] Add option to configure guest vPMU

[Andrew Jones]

On Tue, Oct 25, 2016 at 01:50:01PM -0500, Wei Huang wrote:
> 
> 
> On 10/25/2016 12:56 PM, Andrew Jones wrote:
> <snip>
> >>
> >> Compared with V7, my proposed solution above isn't so different as we
> >> thought. Details below. In V7,
> >>
> >> * has_pmu=off by default. It is turned on in target-arm/cpu.c (see
> >> below). Apparently it is turned on only under KVM mode; UNDER TCG, IT
> >> REMAINS OFF.
> >>
> >>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU) && kvm_enabled()) {
> >>         qdev_property_add_static(DEVICE(obj), &arm_cpu_has_pmu_property,
> >>                                  &error_abort);
> >>     }
> >> * We then remove ARM_FEATURE_PMU if has_pmu=off. The rest of code will
> >> know if PMU is on/off using arm_feature(&cpu->env, ARM_FEATURE_PMU).
> >>
> >> With this implementation, V7 can
> >> * disable PMU under TCG mode
> >> * print out an error when "pmu=on|off" property is specified under TCG.
> >>
> >> Drew didn't like it, he wanted a warning when "pmu=on", and didn't want
> >> it when "pmu=off". This is fair. To do that, we need to add
> >> arm_cpu_has_pmu_property for both KVM and TCG. But, in this case,
> >> printing a warning message only for "pmu=on" isn't possible without
> >> tri-state, because we can't tell if pmu is on by default OR turned on by
> >> users using ",pmu=on".
> >>
> >> To solve this problem, in my proposed solution above, we still add
> >> arm_cpu_has_pmu_property under TCG, but turned it off (see below).
> >>
> >>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
> >>         qdev_property_add_static(DEVICE(obj),
> >>             &arm_cpu_has_pmu_property, &error_abort);
> >>         if (!kvm_enabled())
> >>             object_property_set_bool(obj, false, "pmu", NULL);
> >>     }
> >>
> >> Because has_pmu=off now, we remove ARM_FEATURE_PMU under TCG. THIS IS
> >> SAME AS V7. If pmu=on is detected later, we know end-users turn it on
> >> intentionally and can flag a warning message. With this approach, we can:
> >> * disable PMU under TCG mode
> >> * print a warning when ",pmu=on" is specified; it remains silent under
> >> other cases.
> >>
> >> I think removing tri-state and printing out a warning only for ",pmu=on"
> >> contradict each other. We need some trick to solve this problem. The
> >> proposed solution didn't change the behavior underfoot as it might sound
> >> like. If you have other suggestion, I want to know the details.
> >>
> > 
> > I'm a bit lost as to which proposal is which now, but what I didn't like
> > was QEMU failing to run with TCG when the same command line worked for
> > KVM. If the property doesn't exist when using TCG then QEMU fails when a
> > command line including ,pmu=on/off is used.
> > 
> > Peter says he's not worried about the PMU not working for TCG right now,
> > and thus isn't worried about warning that it doesn't work. I'm not sure
> > if he was proposing to keep your v7 - fail for tcg when the property was
> > used, or if he was proposing to just not worry about the property for tcg.
> > I believe the later case would be the proposed change to v7, but without
> > any warning.
> 
> I think Peter means the later case, which requires a fix to v7. It will
> keep the same command line for both TCG and KVM; but TCG will ignore
> silently without a warning when ",pmu=on".
> 
> > 
> > Anyway, whatever you guys work out is fine by me. I can live with changing
> > the command line between KVM and TCG runs. Eventually I won't have to when
> > PMU support comes to TCG. I'm also fine with ,pmu=on silently not actually
> > providing a PMU when run under TCG. It's like Peter says, many things
> 
> I prefer the second 2nd one. If you are OK, I will send in a new spin (V8).

Works for me.


> 
> > under TCG don't work, but none of them warn about it. Only, in this case,
> > I feel a warning for ,pmu=on would be better, because the feature would be
> > getting asked for explicitly.
> > 
> > Thanks,
> > drew
> > 
>