Re: [Qemu-arm] [Qemu-devel] [PATCH v3 2/3] hw/arm/virt: Add PMU node for virt machine

[Andrew Jones]

On Mon, Apr 25, 2016 at 09:54:50PM +0800, Shannon Zhao wrote:
> On 2016年04月25日 19:52, Andrew Jones wrote:
> > On Mon, Apr 25, 2016 at 07:37:13PM +0800, Shannon Zhao wrote:
> >> > 
> >> > 
> >> > On 2016/4/25 17:22, Andrew Jones wrote:
> >>> > > On Mon, Apr 25, 2016 at 03:11:45PM +0800, Shannon Zhao wrote:
> >>>>> > >> > From: Shannon Zhao <address@hidden>
> >>>>> > >> > 
> >>>>> > >> > Add a virtual PMU device for virt machine while use PPI 7 for PMU
> >>>>> > >> > overflow interrupt number.
> >>>>> > >> > 
> >>>>> > >> > Signed-off-by: Shannon Zhao <address@hidden>
> >>>>> > >> > ---
> >>>>> > >> >  hw/arm/virt.c         | 34 ++++++++++++++++++++++++++++++++++
> >>>>> > >> >  include/hw/arm/virt.h |  4 ++++
> >>>>> > >> >  include/sysemu/kvm.h  |  1 +
> >>>>> > >> >  stubs/kvm.c           |  5 +++++
> >>>>> > >> >  target-arm/kvm64.c    | 39 
> >>>>> > >> > +++++++++++++++++++++++++++++++++++++++
> >>>>> > >> >  5 files changed, 83 insertions(+)
> >>>>> > >> > 
> >>>>> > >> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >>>>> > >> > index 56d35c7..c3632d8 100644
> >>>>> > >> > --- a/hw/arm/virt.c
> >>>>> > >> > +++ b/hw/arm/virt.c
> >>>>> > >> > @@ -428,6 +428,38 @@ static void fdt_add_gic_node(VirtBoardInfo 
> >>>>> > >> > *vbi, int type)
> >>>>> > >> >      qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", 
> >>>>> > >> > vbi->gic_phandle);
> >>>>> > >> >  }
> >>>>> > >> >  
> >>>>> > >> > +static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, int 
> >>>>> > >> > gictype)
> >>>>> > >> > +{
> >>>>> > >> > +    CPUState *cpu;
> >>>>> > >> > +    ARMCPU *armcpu;
> >>>>> > >> > +    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
> >>>>> > >> > +
> >>>>> > >> > +    CPU_FOREACH(cpu) {
> >>>>> > >> > +        armcpu = ARM_CPU(cpu);
> >>>>> > >> > +        if (!armcpu->has_pmu) {
> >>>>> > >> > +            return;
> >>>>> > >> > +        }
> >>>>> > >> > +
> >>>>> > >> > +        kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ));
> >>> > > I think we need to return a failure code from kvm_arm_pmu_create, and
> >>> > > then do
> >>> > > 
> >>> > >            if (!kvm_arm_pmu_create(...)) {
> >>> > >                return;
> >>> > >            }
> >>> > > 
> >>> > > Otherwise we create a /pmu node for the guest that won't work.
> >>> > > 
> >> > Ok, will update.
> >> > 
> >>>>> > >> > +    }
> >>>>> > >> > +
> >>>>> > >> > +    if (gictype == 2) {
> >>>>> > >> > +        irqflags = deposit32(irqflags, 
> >>>>> > >> > GIC_FDT_IRQ_PPI_CPU_START,
> >>>>> > >> > +                             GIC_FDT_IRQ_PPI_CPU_WIDTH,
> >>>>> > >> > +                             (1 << vbi->smp_cpus) - 1);
> >>> > > So, if we're using gicv3, then we're level triggered and these cpu
> >>> > > mask bits aren't defined by the arm,gic-v3.txt bindings spec. Good.
> >>> > > 
> >>> > > If we're using gicv2, then we're edge triggered and this mask is
> >>> > > defined. Assuming the GIC implementation requires using edge
> >>> > > triggered interrupts (as the comment in fdt_add_timer_nodes says),
> >>> > > then OK.
> >>> > > 
> >> > IIRC the comments in fdt_add_timer_nodes are not correct now since KVM
> >> > makes PPI level triggered.
> >> > 
> >> >     /* Note that on A15 h/w these interrupts are level-triggered,
> >> >      * but for the GIC implementation provided by both QEMU and KVM
> >> >      * they are edge-triggered.
> >> >      */
> > OK, in that case the comment should be updated to say something like
> > "used to", but I suspect it's too late for the timer to switch to
> > level at this point anyway, unless KVM can be probed to determine if
> > level is OK.
> > 
> > However, for PMU, we know that if the PMU feature exists, then level is
> > OK, so if we want a level triggered interrupt for PMU, then we should
> > change the giv2 irqflags assignment to an '|=' of the cpu mask.
> Since for gicv2, the maximum of vbi->smp_cpus is 8, so it will not
> overflow and the result of deposit32 is right, I think.

Yes. I was actually referring to the level flag, thinking it was getting
overwritten with zero, but now I see in deposit32's description that
"Bits of @value outside the bit field are not modified." So this is
fine.

Thanks,
drew