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.
Thanks,
--
Shannon
