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

[Shannon Zhao]

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.

Thanks,
-- 
Shannon