Re: [Qemu-devel] [PATCH 2/3] target/arm/kvm: split pmu init from creation

[Peter Maydell]

On 18 July 2017 at 13:38, Andrew Jones <address@hidden> wrote:
> On Mon, Jul 10, 2017 at 04:27:35PM +0100, Peter Maydell wrote:
>> On 1 July 2017 at 17:47, Andrew Jones <address@hidden> wrote:
>> > When adding a PMU with a userspace irqchip we only do the INIT
>> > stage of the device creation.
>>
>> Can you explain why? My assumption would be that either
>> (a) we don't need the kernel's PMU, in which case don't
>> create it at all, or
>> (b) we do need the kernel's PMU, so we should both create
>> and INIT it.
>>
>> If we do one but not the other then we've left a half
>> created and unusable PMU device in the kernel, haven't we?
>>
>
> I should have renamed the 'create' function to 'set_irq', then
> it would make sense, because after the split done by this patch
> 'create' only sets the irq, which can only be done with an
> in-kernel irqchip. Both irqchip types still require INIT though,
> see kernel doc Documentation/virtual/kvm/devices/vcpu.txt.

Oh, I see, I read the commit message as "we only do the
INIT stage when adding a PMU with a userspace irqchip",
which isn't the same meaning as what you actually wrote.
(perhaps "When adding a PMU with a userspace irqchip we
skip the set-irq stage of device creation" would be clearer?)

> I'll send a v2 that renames the create function. But, before I do,
> assuming the rename, do you have another comments on this or the
> next patch? We might as well batch all the changes :-)

I think they looked mostly OK. A minor nit:

+        if (kvm_enabled() &&
+            !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) {
+            return;
+        }
+        if (kvm_enabled() && !kvm_arm_pmu_init(cpu)) {
             return;
         }

might be better as
       if (kvm_enabled()) {
           if (!kvm_arm_pmu_create(...)) {
               /* error handling */
           }
           if (!kvm_arm_pmu_init(...)) {
               /* error handling */
           }
       }

(and I'm not sure "silently return" is really the right error
handling, but that is what we do currently, so whatever.)

thanks
-- PMM