[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Andrew Jones
Subject: Re: [Qemu-arm] [Qemu-devel] [PATCH V7 0/2] Add option to configure guest vPMU
Date: Tue, 25 Oct 2016 19:56:15 +0200
User-agent: Mutt/ (2016-04-01)

On Tue, Oct 25, 2016 at 11:55:05AM -0500, Wei Huang wrote:
> On 10/25/2016 10:26 AM, Peter Maydell wrote:
> > On 25 October 2016 at 08:33, Andrew Jones <address@hidden> wrote:
> >> On Mon, Oct 24, 2016 at 11:39:59PM -0500, Wei Huang wrote:
> >>>>> V6->V7:
> >>>>>   * change has_pmu variable type from OnOffAuto to Boolean
> >>>>>   * only add "pmu" property to CPU under kvm mode, default ON
> >>>>
> >>>> Hmm, if we don't allow the property with TCG then switching a guest from
> >>>> KVM to TCG will require more than just an accelerator switch. That's a
> >>>> bit annoying and I think we'd have to teach it to libvirt too. I'd prefer
> >>>>
> >>>>  -M virt-2.8,accel=tcg -cpu cortex-a57         NO     NO
> >>>>  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=off NO     NO
> >>>>  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=on  NO     "Warning: PMU not
> >>>> yet supported with TCG" (or something)
> >>>
> >>> I am fine with this request. But note that, if we enforce
> >>> pmu-default=ON, we can't tell "-cpu cortex-a57" apart from
> >>> "cortex-a57,pmu=on", implying that we have to print the warning msg for
> >>> the case of "cortex-a57" as well (this was why we switch to tri-state
> >>> before). To solve this problem, we have to switch from pmu-default=ON to
> >>> pmu-default=OFF under TCG mode, something like:
> >>>
> >>>     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);
> >>>     }
> >>>
> >>> Then we do:
> >>>     if (cpu->has_pmu && !kvm_enabled()) {
> >>>         cpu->has_pmu = false;
> >>>         if (!pmu_warned && !qtest_enabled()) {
> >>>             error_report("warning: pmu not supported under TCG");
> >>>             pmu_warned = true;
> >>>         }
> >>>     }
> >>>
> >>> This will work. Are you and Peter OK with this solution?
> >>>
> >>
> >> Yes, I prefer it to not being able to easily switch a command line from
> >> KVM to TCG. Granted, right now we mostly need to run with -cpu host for
> >> KVM, which also needs to be switched for TCG. However that will hopefully
> >> change with Shannon's work.
> > 
> > If the TCG default is different from the KVM default then you
> > will have problems with switching it anyway, because it will
> > silently change behaviour underfoot. I think we should avoid that...
> 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
>     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.

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
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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]