On 18 July 2017 at 13:38, Andrew Jones <drjo...@redhat.com> wrote:
> On Mon, Jul 10, 2017 at 04:27:35PM +0100, Peter Maydell wrote:
>> On 1 July 2017 at 17:47, Andrew Jones <drjo...@redhat.com> 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
