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 <shannon.z...@linaro.org>
> >>>>> > >> >
> >>>>> > >> > Add a virtual PMU device for virt machine while use PPI 7 for PMU
> >>>>> > >> > overflow interrupt number.
> >>>>> > >> >
> >>>>> > >> > Signed-off-by: Shannon Zhao <shannon.z...@linaro.org>
> >>>>> > >> > ---
> >>>>> > >> > 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
