On Tue, Jul 18, 2017 at 01:44:34PM +0100, Peter Maydell wrote:
> 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 */
> }
> }
With the top version I was setting things up for a one-liner
change in the next patch, as only the first condition needs
to have s/kvm_enabled/kvm_irqchip_in_kernel/ done to it.
>
> (and I'm not sure "silently return" is really the right error
> handling, but that is what we do currently, so whatever.)
I'll give the error handling a bit of thought before respinning.
Thanks,
drew
