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

[Andrew Jones]

On Tue, Jul 18, 2017 at 01:44:34PM +0100, Peter Maydell wrote:
> 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 */
>            }
>        }

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