[Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Salil Mehta]

Hello,
I could see below within function fdt_add_pmu_nodes() part of
hw/arm/virt.c during virt machine initialization time:

Observation:
In below function, support of PMU feature is being checked for
each vcpu and if the PMU is found part of the features then PMU
is initialized with in the host/KVM. But if there is even one
vcpu which is found to not support the PMU then loop is exited
and PMU is not initialized for the rest of the vcpus as well.

static void fdt_add_pmu_nodes(const VirtMachineState *vms)
{
    CPUState *cpu;
    ARMCPU *armcpu;
    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;

    CPU_FOREACH(cpu) {
        armcpu = ARM_CPU(cpu);
        if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
            return; ------> Here, loop exits & function returns
        }
        if (kvm_enabled()) {
            if (kvm_irqchip_in_kernel()) {
                kvm_arm_pmu_set_irq(cpu, PPI(VIRTUAL_PMU_IRQ));
            }
            kvm_arm_pmu_init(cpu);
        }
    }

    if (vms->gic_version == VIRT_GIC_VERSION_2) {
        irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
                             GIC_FDT_IRQ_PPI_CPU_WIDTH,
                             (1 << vms->smp_cpus) - 1);
    }

    armcpu = ARM_CPU(qemu_get_cpu(0));
    qemu_fdt_add_subnode(vms->fdt, "/pmu");
    if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
        const char compat[] = "arm,armv8-pmuv3";
        qemu_fdt_setprop(vms->fdt, "/pmu", "compatible",
                         compat, sizeof(compat));
        qemu_fdt_setprop_cells(vms->fdt, "/pmu", "interrupts",
                               GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_PMU_IRQ, irqflags);
    }
}

Questions:
Q1. Not sure what is the logic of the premature exit and not
    continuing with further checks and initialization of other
    VCPU PMUs?
Q2. Does it even makes sense to have PMUs initialized for some
    vcpus and not for others unless we have heterogeneous system? 
Q3. Also, there is a per virt machine knob of vcc->no_pmu.
    This is something which user could specify at the init time
    and perhaps only once but we don't use it for ARM. Perhaps
    should have been used even before entering this function
    to enable or disable the support as per user config?
Q4. This function  fdt_* looks to be wrongly named. The info
    being initialized here shall be used even when ACPI is
    being used. Initialization part and FDT info looked
    mixed up here if I am right?


Best regards
Salil

Re: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Andrew Jones]

On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:
> Hello,
> I could see below within function fdt_add_pmu_nodes() part of
> hw/arm/virt.c during virt machine initialization time:
> 
> Observation:
> In below function, support of PMU feature is being checked for
> each vcpu and if the PMU is found part of the features then PMU
> is initialized with in the host/KVM. But if there is even one
> vcpu which is found to not support the PMU then loop is exited
> and PMU is not initialized for the rest of the vcpus as well.
> 
> Questions:
> Q1. Not sure what is the logic of the premature exit and not
>     continuing with further checks and initialization of other
>     VCPU PMUs?

KVM requires all VCPUs to have a PMU if one does. If the ARM ARM
says it's possible to have PMUs for only some CPUs, then, for TCG,
the restriction could be relaxed. I expect it will take more than
just removing the check for things to work though.

> Q2. Does it even makes sense to have PMUs initialized for some
>     vcpus and not for others unless we have heterogeneous system? 

I don't know, but it doesn't sound like a configuration I'd like
to see.

> Q3. Also, there is a per virt machine knob of vcc->no_pmu.
>     This is something which user could specify at the init time
>     and perhaps only once but we don't use it for ARM. Perhaps
>     should have been used even before entering this function
>     to enable or disable the support as per user config?

It's purpose is to keep users from doing 'pmu=on' on 2.6 machine
types. On 2.7 and later machine types if you don't want a PMU
you should use 'pmu=off'.

> Q4. This function  fdt_* looks to be wrongly named. The info
>     being initialized here shall be used even when ACPI is
>     being used. Initialization part and FDT info looked
>     mixed up here if I am right?

Agreed. The function has the wrong name. mach-virt has many functions that
mix the initialization and fdt building together, but those functions are
named something like create_foo(). Patches welcome.

Thanks,
drew

Re: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Auger Eric]

Hi Drew,

On 6/3/20 11:37 AM, Andrew Jones wrote:
> On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:
>> Hello,
>> I could see below within function fdt_add_pmu_nodes() part of
>> hw/arm/virt.c during virt machine initialization time:
>>
>> Observation:
>> In below function, support of PMU feature is being checked for
>> each vcpu and if the PMU is found part of the features then PMU
>> is initialized with in the host/KVM. But if there is even one
>> vcpu which is found to not support the PMU then loop is exited
>> and PMU is not initialized for the rest of the vcpus as well.
>>
>> Questions:
>> Q1. Not sure what is the logic of the premature exit and not
>>     continuing with further checks and initialization of other
>>     VCPU PMUs?
> 
> KVM requires all VCPUs to have a PMU if one does.

I fail to find where this is enforced? Do you know the place?

 If the ARM ARM
> says it's possible to have PMUs for only some CPUs, then, for TCG,
> the restriction could be relaxed. I expect it will take more than
> just removing the check for things to work though.>
>> Q2. Does it even makes sense to have PMUs initialized for some
>>     vcpus and not for others unless we have heterogeneous system? 
> 
> I don't know, but it doesn't sound like a configuration I'd like
> to see.
> 
>> Q3. Also, there is a per virt machine knob of vcc->no_pmu.
>>     This is something which user could specify at the init time
>>     and perhaps only once but we don't use it for ARM. Perhaps
>>     should have been used even before entering this function
>>     to enable or disable the support as per user config?
> 
> It's purpose is to keep users from doing 'pmu=on' on 2.6 machine
> types. On 2.7 and later machine types if you don't want a PMU
> you should use 'pmu=off'.

extra note:
the cpu pmu property sets the feature at vcpu level. This is what is
retrieved when (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) gets called.

See the cpu option setter: arm_set_pmu in target/arm/cpu.c

> 
>> Q4. This function  fdt_* looks to be wrongly named. The info
>>     being initialized here shall be used even when ACPI is
>>     being used. Initialization part and FDT info looked
>>     mixed up here if I am right?
> 
> Agreed. The function has the wrong name. mach-virt has many functions that
> mix the initialization and fdt building together, but those functions are
> named something like create_foo(). Patches welcome.
agreed

Thanks

Eric
> 
> Thanks,
> drew
> 
> 

Re: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Andrew Jones]

On Wed, Jun 03, 2020 at 11:59:23AM +0200, Auger Eric wrote:
> Hi Drew,
> 
> On 6/3/20 11:37 AM, Andrew Jones wrote:
> > On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:
> >> Hello,
> >> I could see below within function fdt_add_pmu_nodes() part of
> >> hw/arm/virt.c during virt machine initialization time:
> >>
> >> Observation:
> >> In below function, support of PMU feature is being checked for
> >> each vcpu and if the PMU is found part of the features then PMU
> >> is initialized with in the host/KVM. But if there is even one
> >> vcpu which is found to not support the PMU then loop is exited
> >> and PMU is not initialized for the rest of the vcpus as well.
> >>
> >> Questions:
> >> Q1. Not sure what is the logic of the premature exit and not
> >>     continuing with further checks and initialization of other
> >>     VCPU PMUs?
> > 
> > KVM requires all VCPUs to have a PMU if one does.
> 
> I fail to find where this is enforced? Do you know the place?

kvm_vcpu_set_target(), called from KVM_ARM_VCPU_INIT. It ensures
all VCPUs have identical features selected. The PMU requires a
VCPU feature (KVM_ARM_VCPU_PMU_V3) to be set.

Thanks,
drew

Re: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Auger Eric]

Hi Drew,
On 6/3/20 12:21 PM, Andrew Jones wrote:
> On Wed, Jun 03, 2020 at 11:59:23AM +0200, Auger Eric wrote:
>> Hi Drew,
>>
>> On 6/3/20 11:37 AM, Andrew Jones wrote:
>>> On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:
>>>> Hello,
>>>> I could see below within function fdt_add_pmu_nodes() part of
>>>> hw/arm/virt.c during virt machine initialization time:
>>>>
>>>> Observation:
>>>> In below function, support of PMU feature is being checked for
>>>> each vcpu and if the PMU is found part of the features then PMU
>>>> is initialized with in the host/KVM. But if there is even one
>>>> vcpu which is found to not support the PMU then loop is exited
>>>> and PMU is not initialized for the rest of the vcpus as well.
>>>>
>>>> Questions:
>>>> Q1. Not sure what is the logic of the premature exit and not
>>>>     continuing with further checks and initialization of other
>>>>     VCPU PMUs?
>>>
>>> KVM requires all VCPUs to have a PMU if one does.
>>
>> I fail to find where this is enforced? Do you know the place?
> 
> kvm_vcpu_set_target(), called from KVM_ARM_VCPU_INIT. It ensures
> all VCPUs have identical features selected. The PMU requires a
> VCPU feature (KVM_ARM_VCPU_PMU_V3) to be set.
OK thanks!

Eric
> 
> Thanks,
> drew
> 

RE: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Salil Mehta]

Hi Andrew,
Many thanks for the reply.

> From: Andrew Jones [mailto:drjones@redhat.com]
> Sent: Wednesday, June 3, 2020 10:38 AM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; Peter Maydell
> <peter.maydell@linaro.org>; Igor Mammedov <imammedo@redhat.com>;
> mst@redhat.com
> Subject: Re: [Question] Regarding PMU initialization within the QEMU for ARM
> VCPUs
> 
> On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:
> > Hello,
> > I could see below within function fdt_add_pmu_nodes() part of
> > hw/arm/virt.c during virt machine initialization time:
> >
> > Observation:
> > In below function, support of PMU feature is being checked for
> > each vcpu and if the PMU is found part of the features then PMU
> > is initialized with in the host/KVM. But if there is even one
> > vcpu which is found to not support the PMU then loop is exited
> > and PMU is not initialized for the rest of the vcpus as well.
> >
> > Questions:
> > Q1. Not sure what is the logic of the premature exit and not
> >     continuing with further checks and initialization of other
> >     VCPU PMUs?
> 
> KVM requires all VCPUs to have a PMU if one does. If the ARM ARM
> says it's possible to have PMUs for only some CPUs, then, for TCG,
> the restriction could be relaxed. I expect it will take more than
> just removing the check for things to work though.

Got it. Many thanks for this info.

During virt machine init we take cpu type from (-cpu <cpu-type>)
option and it should apply evenly to all of the vcpus. Therefore,
I can assume all of the processors to be identical for now. This
combined with the KVM restriction you mentioned above means for
PMU we could only have Enable-for-All OR Enable-for-none config
for all of the vcpus being booted even though we at different
places do have per-vcpu specific check like below available

/* MADT */
static void
build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
{
[...]

    for (i = 0; i < vms->smp_cpus; i++) {
        AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
                                                           sizeof(*gicc));
        [...]

        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {---> This check
            gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
        }
 [...]
}

Do per-vcpu feature check for PMU even makes sense till we allow
heterogeneous support of processors or relax the PMU enablement
on the per-vcpu basis within the KVM?



> 
> > Q2. Does it even makes sense to have PMUs initialized for some
> >     vcpus and not for others unless we have heterogeneous system?
> 
> I don't know, but it doesn't sound like a configuration I'd like
> to see.


sure. but in the existing code we do prematurely exit after we
discover first vcpu amongst the possible vcpus not supporting
PMU feature. This looks abnormal as well?


> 
> > Q3. Also, there is a per virt machine knob of vcc->no_pmu.
> >     This is something which user could specify at the init time
> >     and perhaps only once but we don't use it for ARM. Perhaps
> >     should have been used even before entering this function
> >     to enable or disable the support as per user config?
> 
> It's purpose is to keep users from doing 'pmu=on' on 2.6 machine
> types. On 2.7 and later machine types if you don't want a PMU
> you should use 'pmu=off'.

sure. so by default on latest machines PMU is on. 

> 
> > Q4. This function  fdt_* looks to be wrongly named. The info
> >     being initialized here shall be used even when ACPI is
> >     being used. Initialization part and FDT info looked
> >     mixed up here if I am right?
> 
> Agreed. The function has the wrong name. mach-virt has many functions that
> mix the initialization and fdt building together, but those functions are
> named something like create_foo(). Patches welcome.


Will do. I have created one already. Will float soon.


> 
> Thanks,
> drew

RE: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Salil Mehta]

Hi Eric,

> From: Qemu-arm [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> On Behalf Of Auger Eric
> Sent: Wednesday, June 3, 2020 10:59 AM
> To: Andrew Jones <drjones@redhat.com>; Salil Mehta <salil.mehta@huawei.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>; Igor Mammedov
> <imammedo@redhat.com>; qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> mst@redhat.com
> Subject: Re: [Question] Regarding PMU initialization within the QEMU for ARM
> VCPUs
> 
> Hi Drew,
> 
> On 6/3/20 11:37 AM, Andrew Jones wrote:
> > On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:
> >> Hello,
> >> I could see below within function fdt_add_pmu_nodes() part of
> >> hw/arm/virt.c during virt machine initialization time:
> >>
> >> Observation:
> >> In below function, support of PMU feature is being checked for
> >> each vcpu and if the PMU is found part of the features then PMU
> >> is initialized with in the host/KVM. But if there is even one
> >> vcpu which is found to not support the PMU then loop is exited
> >> and PMU is not initialized for the rest of the vcpus as well.
> >>
> >> Questions:
> >> Q1. Not sure what is the logic of the premature exit and not
> >>     continuing with further checks and initialization of other
> >>     VCPU PMUs?
> >
> > KVM requires all VCPUs to have a PMU if one does.
> 
> I fail to find where this is enforced? Do you know the place?
> 
>  If the ARM ARM
> > says it's possible to have PMUs for only some CPUs, then, for TCG,
> > the restriction could be relaxed. I expect it will take more than
> > just removing the check for things to work though.>
> >> Q2. Does it even makes sense to have PMUs initialized for some
> >>     vcpus and not for others unless we have heterogeneous system?
> >
> > I don't know, but it doesn't sound like a configuration I'd like
> > to see.
> >
> >> Q3. Also, there is a per virt machine knob of vcc->no_pmu.
> >>     This is something which user could specify at the init time
> >>     and perhaps only once but we don't use it for ARM. Perhaps
> >>     should have been used even before entering this function
> >>     to enable or disable the support as per user config?
> >
> > It's purpose is to keep users from doing 'pmu=on' on 2.6 machine
> > types. On 2.7 and later machine types if you don't want a PMU
> > you should use 'pmu=off'.
> 
> extra note:
> the cpu pmu property sets the feature at vcpu level. This is what is
> retrieved when (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) gets called.
> 
> See the cpu option setter: arm_set_pmu in target/arm/cpu.c


Indeed. It is being set on per-vcpu level but actually all of
the vcpus will either have ON or OFF PMU feature - at least for
now. Not sure if we ever want to change this in future?

Thanks
Salil.

Re: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Andrew Jones]

On Wed, Jun 03, 2020 at 11:45:22AM +0000, Salil Mehta wrote:
> Hi Andrew,
> Many thanks for the reply.
> 
> > From: Andrew Jones [mailto:drjones@redhat.com]
> > Sent: Wednesday, June 3, 2020 10:38 AM
> > To: Salil Mehta <salil.mehta@huawei.com>
> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; Peter Maydell
> > <peter.maydell@linaro.org>; Igor Mammedov <imammedo@redhat.com>;
> > mst@redhat.com
> > Subject: Re: [Question] Regarding PMU initialization within the QEMU for ARM
> > VCPUs
> > 
> > On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:
> > > Hello,
> > > I could see below within function fdt_add_pmu_nodes() part of
> > > hw/arm/virt.c during virt machine initialization time:
> > >
> > > Observation:
> > > In below function, support of PMU feature is being checked for
> > > each vcpu and if the PMU is found part of the features then PMU
> > > is initialized with in the host/KVM. But if there is even one
> > > vcpu which is found to not support the PMU then loop is exited
> > > and PMU is not initialized for the rest of the vcpus as well.
> > >
> > > Questions:
> > > Q1. Not sure what is the logic of the premature exit and not
> > >     continuing with further checks and initialization of other
> > >     VCPU PMUs?
> > 
> > KVM requires all VCPUs to have a PMU if one does. If the ARM ARM
> > says it's possible to have PMUs for only some CPUs, then, for TCG,
> > the restriction could be relaxed. I expect it will take more than
> > just removing the check for things to work though.
> 
> Got it. Many thanks for this info.
> 
> During virt machine init we take cpu type from (-cpu <cpu-type>)
> option and it should apply evenly to all of the vcpus. Therefore,
> I can assume all of the processors to be identical for now. This
> combined with the KVM restriction you mentioned above means for
> PMU we could only have Enable-for-All OR Enable-for-none config
> for all of the vcpus being booted even though we at different
> places do have per-vcpu specific check like below available
> 
> /* MADT */
> static void
> build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> {
> [...]
> 
>     for (i = 0; i < vms->smp_cpus; i++) {
>         AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
>                                                            sizeof(*gicc));
>         [...]
> 
>         if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {---> This check
>             gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>         }
>  [...]
> }
> 
> Do per-vcpu feature check for PMU even makes sense till we allow
> heterogeneous support of processors or relax the PMU enablement
> on the per-vcpu basis within the KVM?

It may not be necessary now or ever to test more than one CPU for the
PMU feature, but without a good reason to change it to a machine
property then I'd prefer we always to the N-1 pointless checks. The
feature is a CPU feature, not a machine feature, so, IMO, it should
remain something configured and tested at the CPU level, not the machine
level.

> 
> 
> 
> > 
> > > Q2. Does it even makes sense to have PMUs initialized for some
> > >     vcpus and not for others unless we have heterogeneous system?
> > 
> > I don't know, but it doesn't sound like a configuration I'd like
> > to see.
> 
> 
> sure. but in the existing code we do prematurely exit after we
> discover first vcpu amongst the possible vcpus not supporting
> PMU feature. This looks abnormal as well?

Are you trying to configure heterogeneous mach-virt machines? Or machines
that only provide PMUs to some CPUs? If not, then I'm not sure why this
would be a problem. Indeed it's likely a pointless check and, instead of
silently returning, it should output a warning or even assert. Otherwise,
I don't see a problem with it, since we want to be sure we're dealing with
the type of configuration we expect, i.e. one where each CPU has a PMU if
any CPU has a PMU.

Thanks,
drew

RE: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Salil Mehta]

Hi Andrew,

> From: Andrew Jones [mailto:drjones@redhat.com]
> Sent: Wednesday, June 3, 2020 1:16 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>; mst@redhat.com;
> qemu-devel@nongnu.org; eric.auger@redhat.com; qemu-arm@nongnu.org; Igor
> Mammedov <imammedo@redhat.com>
> Subject: Re: [Question] Regarding PMU initialization within the QEMU for ARM
> VCPUs
> 
> On Wed, Jun 03, 2020 at 11:45:22AM +0000, Salil Mehta wrote:
> > Hi Andrew,
> > Many thanks for the reply.
> >
> > > From: Andrew Jones [mailto:drjones@redhat.com]
> > > Sent: Wednesday, June 3, 2020 10:38 AM
> > > To: Salil Mehta <salil.mehta@huawei.com>
> > > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; Peter Maydell
> > > <peter.maydell@linaro.org>; Igor Mammedov <imammedo@redhat.com>;
> > > mst@redhat.com
> > > Subject: Re: [Question] Regarding PMU initialization within the QEMU for ARM
> > > VCPUs
> > >
> > > On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:
> > > > Hello,
> > > > I could see below within function fdt_add_pmu_nodes() part of
> > > > hw/arm/virt.c during virt machine initialization time:
> > > >
> > > > Observation:
> > > > In below function, support of PMU feature is being checked for
> > > > each vcpu and if the PMU is found part of the features then PMU
> > > > is initialized with in the host/KVM. But if there is even one
> > > > vcpu which is found to not support the PMU then loop is exited
> > > > and PMU is not initialized for the rest of the vcpus as well.
> > > >
> > > > Questions:
> > > > Q1. Not sure what is the logic of the premature exit and not
> > > >     continuing with further checks and initialization of other
> > > >     VCPU PMUs?
> > >
> > > KVM requires all VCPUs to have a PMU if one does. If the ARM ARM
> > > says it's possible to have PMUs for only some CPUs, then, for TCG,
> > > the restriction could be relaxed. I expect it will take more than
> > > just removing the check for things to work though.
> >
> > Got it. Many thanks for this info.
> >
> > During virt machine init we take cpu type from (-cpu <cpu-type>)
> > option and it should apply evenly to all of the vcpus. Therefore,
> > I can assume all of the processors to be identical for now. This
> > combined with the KVM restriction you mentioned above means for
> > PMU we could only have Enable-for-All OR Enable-for-none config
> > for all of the vcpus being booted even though we at different
> > places do have per-vcpu specific check like below available
> >
> > /* MADT */
> > static void
> > build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > {
> > [...]
> >
> >     for (i = 0; i < vms->smp_cpus; i++) {
> >         AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
> >                                                            sizeof(*gicc));
> >         [...]
> >
> >         if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {---> This check
> >             gicc->performance_interrupt =
> cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
> >         }
> >  [...]
> > }
> >
> > Do per-vcpu feature check for PMU even makes sense till we allow
> > heterogeneous support of processors or relax the PMU enablement
> > on the per-vcpu basis within the KVM?
> 
> It may not be necessary now or ever to test more than one CPU for the
> PMU feature, but without a good reason to change it to a machine
> property then I'd prefer we always to the N-1 pointless checks. The
> feature is a CPU feature, not a machine feature, so, IMO, it should
> remain something configured and tested at the CPU level, not the machine
> level.


I do understand this and all looks logical what you have said.

Yes, there is a reason for this (but not sure if that is
convincing enough) but having an added flag at per-machine level
will help in supporting ARM VCPU Hotplug feature I am dealing with.

Just a summary to give you a context of vcpu hotplug design:

As per the design, at the time of machvirt_init() we pre-create
the ARMCPU objects along with the corresponding KVM vcpus at the
host. KVM vcpu initialized context(struct kvm_parked_vcpus) for the
disabled vcpus is parked at per VM list kvm_parked_vcpus. 

We create the ARMCPU objects(but these are not *realized* in qdev
sense) to facilitate the GIC initialization (pre-sized with possible
vcpus) with minimum change. After Initialization of the machine is
complete we release the ARMCPU Objects for the disabled vcpus(which
shall be re-created at the time when vcpu is hot plugged and
at that time we re-attach this new ARMCPU object with already
parked KVM VCPU context). 

So we have few options related to ARMCPU object release:
1. The ARMCPU objects for the disabled vcpus are released in
   context to the virt_machine_done() notifier. 
2. Defer release till a new vcpu object is hot plugged.
3. Never release and keep on reusing them and release once
   at VM exit.

Each of the above approaches come with  their own pros and cons.
(And I have prototyped each)

1st case looks more cleaner but the only problem we are facing
is after ARMCPUs are released and later during UEFI takes over,
it could again call the QEMU virt_acpi_build_update() to update
the ACPI tables(which UEFI has patched) which further ends up in
build_dsdt()->build_madt() path and leads to a problem since
disabled ARMCPU object are not available as they were released
earlier in context of virt_machine_done(). 

The problem happens in MADT which  needs per-vcpu PMU feature
check  to decide whether to enable it in the GICC MDAT entry.

Perhaps the problem is that maybe we are going against some
design expectations and we should not be releasing the ARMCPU
at all as firmware is dependent on it. And maybe UEFI and QEMU
have some sort of coupled design which does not helps.



> > > > Q2. Does it even makes sense to have PMUs initialized for some
> > > >     vcpus and not for others unless we have heterogeneous system?
> > >
> > > I don't know, but it doesn't sound like a configuration I'd like
> > > to see.
> >
> >
> > sure. but in the existing code we do prematurely exit after we
> > discover first vcpu amongst the possible vcpus not supporting
> > PMU feature. This looks abnormal as well?
> 
> Are you trying to configure heterogeneous mach-virt machines? Or machines
> that only provide PMUs to some CPUs?

No, I am not rather assuming identical/homogenous processing.

 If not, then I'm not sure why this
> would be a problem. Indeed it's likely a pointless check and, instead of
> silently returning, it should output a warning or even assert. Otherwise,
> I don't see a problem with it, since we want to be sure we're dealing with
> the type of configuration we expect, i.e. one where each CPU has a PMU if
> any CPU has a PMU.

It is not exactly a problem and as you said a rather pointless check
present. But as explained I was trying to check if we could have
a per-machine flag to devise a workaround for the PMU for now and have
the design(above 3 approaches discussed) of the vcpu hotplug discussed
as part of the patches which I have almost prepared.

(Maybe I should float the ARM VCPU Hotplug patches and let this
 discussion be held over there?)

Many thanks,
Salil.

Re: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Andrew Jones]

On Wed, Jun 03, 2020 at 01:48:10PM +0000, Salil Mehta wrote:
> (Maybe I should float the ARM VCPU Hotplug patches and let this
>  discussion be held over there?)
>

Yes, I think that would be best. Keep in mind that the 'pmu' CPU property
is just one CPU property that we require all CPUs to have, if any have it.
'aarch64' and 'sve' are two other examples. And, likely any CPU feature
that comes down the line that we want to use with KVM will fit that
pattern. I think the hotplug patch series will need to handle those
features in some way other than to push them all into machine properties.

Thanks,
drew

RE: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Salil Mehta]

> From: Qemu-arm [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> On Behalf Of Andrew Jones
> Sent: Wednesday, June 3, 2020 3:37 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>; mst@redhat.com;
> qemu-devel@nongnu.org; eric.auger@redhat.com; qemu-arm@nongnu.org; Igor
> Mammedov <imammedo@redhat.com>
> Subject: Re: [Question] Regarding PMU initialization within the QEMU for ARM
> VCPUs
> 
> On Wed, Jun 03, 2020 at 01:48:10PM +0000, Salil Mehta wrote:
> > (Maybe I should float the ARM VCPU Hotplug patches and let this
> >  discussion be held over there?)
> >
> 
> Yes, I think that would be best. Keep in mind that the 'pmu' CPU property
> is just one CPU property that we require all CPUs to have, if any have it.
> 'aarch64' and 'sve' are two other examples. And, likely any CPU feature
> that comes down the line that we want to use with KVM will fit that
> pattern. I think the hotplug patch series will need to handle those
> features in some way other than to push them all into machine properties.

Sure, I do have realization about that. I have for now used a per virt
machine flag for PMU to do a workaround of the problem I discussed.
Maybe once I float the patches further comments on the ways to improve
the design would be very helpful. So for now I will keep the flag and
use this approach and invite everyone for open discussion about this
and we could evolve the design as we discuss there.


Many thanks
Salil.

Re: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Igor Mammedov]

On Wed, 3 Jun 2020 12:21:16 +0200
Andrew Jones <drjones@redhat.com> wrote:

> On Wed, Jun 03, 2020 at 11:59:23AM +0200, Auger Eric wrote:
> > Hi Drew,
> > 
> > On 6/3/20 11:37 AM, Andrew Jones wrote:  
> > > On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:  
> > >> Hello,
> > >> I could see below within function fdt_add_pmu_nodes() part of
> > >> hw/arm/virt.c during virt machine initialization time:
> > >>
> > >> Observation:
> > >> In below function, support of PMU feature is being checked for
> > >> each vcpu and if the PMU is found part of the features then PMU
> > >> is initialized with in the host/KVM. But if there is even one
> > >> vcpu which is found to not support the PMU then loop is exited
> > >> and PMU is not initialized for the rest of the vcpus as well.
> > >>
> > >> Questions:
> > >> Q1. Not sure what is the logic of the premature exit and not
> > >>     continuing with further checks and initialization of other
> > >>     VCPU PMUs?  
> > > 
> > > KVM requires all VCPUs to have a PMU if one does.  
> > 
> > I fail to find where this is enforced? Do you know the place?  
> 
> kvm_vcpu_set_target(), called from KVM_ARM_VCPU_INIT. It ensures
> all VCPUs have identical features selected. The PMU requires a
> VCPU feature (KVM_ARM_VCPU_PMU_V3) to be set.

perhaps add a check at cpu_preplug time to ensure that it's so before cpu is create
instead of much later FDT initialization?

> 
> Thanks,
> drew

Re: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Igor Mammedov]

On Wed, 3 Jun 2020 11:37:45 +0200
Andrew Jones <drjones@redhat.com> wrote:

> On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:
> > Hello,
> > I could see below within function fdt_add_pmu_nodes() part of
> > hw/arm/virt.c during virt machine initialization time:
...
> 
> > Q4. This function  fdt_* looks to be wrongly named. The info
> >     being initialized here shall be used even when ACPI is
> >     being used. Initialization part and FDT info looked
> >     mixed up here if I am right?  
> 
> Agreed. The function has the wrong name. mach-virt has many functions that
> mix the initialization and fdt building together, but those functions are
> named something like create_foo(). Patches welcome.
that was where I gave up on cpu hotplug arm/virt the last time.

Ideally we should split out from create_foo() all firmware generation code
(fdt) and move it to virt_machine_done time + make sure that it could be
regenerated at reset time so guest would get updated FDT on reset.

> 
> Thanks,
> drew

RE: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Salil Mehta]

> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: Friday, June 5, 2020 4:31 PM
> To: Andrew Jones <drjones@redhat.com>
> Cc: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
> qemu-arm@nongnu.org; Peter Maydell <peter.maydell@linaro.org>; mst@redhat.com
> Subject: Re: [Question] Regarding PMU initialization within the QEMU for ARM
> VCPUs
> 
> On Wed, 3 Jun 2020 11:37:45 +0200
> Andrew Jones <drjones@redhat.com> wrote:
> 
> > On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:
> > > Hello,
> > > I could see below within function fdt_add_pmu_nodes() part of
> > > hw/arm/virt.c during virt machine initialization time:
> ...
> >
> > > Q4. This function  fdt_* looks to be wrongly named. The info
> > >     being initialized here shall be used even when ACPI is
> > >     being used. Initialization part and FDT info looked
> > >     mixed up here if I am right?
> >
> > Agreed. The function has the wrong name. mach-virt has many functions that
> > mix the initialization and fdt building together, but those functions are
> > named something like create_foo(). Patches welcome.
> that was where I gave up on cpu hotplug arm/virt the last time.

Were you releasing the ARM objects as well? Or are you referring to some
other problem?

> Ideally we should split out from create_foo() all firmware generation code
> (fdt) and move it to virt_machine_done time + make sure that it could be
> regenerated at reset time so guest would get updated FDT on reset.

Agreed, just like ACPI part. That would be more cleaner. 

Thanks
Salil.

Re: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Igor Mammedov]

On Fri, 5 Jun 2020 16:38:37 +0000
Salil Mehta <salil.mehta@huawei.com> wrote:

> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: Friday, June 5, 2020 4:31 PM
> > To: Andrew Jones <drjones@redhat.com>
> > Cc: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
> > qemu-arm@nongnu.org; Peter Maydell <peter.maydell@linaro.org>; mst@redhat.com
> > Subject: Re: [Question] Regarding PMU initialization within the QEMU for ARM
> > VCPUs
> > 
> > On Wed, 3 Jun 2020 11:37:45 +0200
> > Andrew Jones <drjones@redhat.com> wrote:
> >   
> > > On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:  
> > > > Hello,
> > > > I could see below within function fdt_add_pmu_nodes() part of
> > > > hw/arm/virt.c during virt machine initialization time:  
> > ...  
> > >  
> > > > Q4. This function  fdt_* looks to be wrongly named. The info
> > > >     being initialized here shall be used even when ACPI is
> > > >     being used. Initialization part and FDT info looked
> > > >     mixed up here if I am right?  
> > >
> > > Agreed. The function has the wrong name. mach-virt has many functions that
> > > mix the initialization and fdt building together, but those functions are
> > > named something like create_foo(). Patches welcome.  
> > that was where I gave up on cpu hotplug arm/virt the last time.  
> 
> Were you releasing the ARM objects as well? Or are you referring to some
> other problem?
I was talking about mix of FDT and device creation code.

> 
> > Ideally we should split out from create_foo() all firmware generation code
> > (fdt) and move it to virt_machine_done time + make sure that it could be
> > regenerated at reset time so guest would get updated FDT on reset.  
> 
> Agreed, just like ACPI part. That would be more cleaner. 
> 
> Thanks
> Salil.
> 
> 

RE: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Salil Mehta]

> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: Monday, June 8, 2020 1:00 PM
> 
> On Fri, 5 Jun 2020 16:38:37 +0000
> Salil Mehta <salil.mehta@huawei.com> wrote:
> 
> > > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > > Sent: Friday, June 5, 2020 4:31 PM
> > > To: Andrew Jones <drjones@redhat.com>
> > > Cc: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
> > > qemu-arm@nongnu.org; Peter Maydell <peter.maydell@linaro.org>; mst@redhat.com
> > > Subject: Re: [Question] Regarding PMU initialization within the QEMU for ARM
> > > VCPUs
> > >
> > > On Wed, 3 Jun 2020 11:37:45 +0200
> > > Andrew Jones <drjones@redhat.com> wrote:
> > >
> > > > On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:
> > > > > Hello,
> > > > > I could see below within function fdt_add_pmu_nodes() part of
> > > > > hw/arm/virt.c during virt machine initialization time:
> > > ...
> > > >
> > > > > Q4. This function  fdt_* looks to be wrongly named. The info
> > > > >     being initialized here shall be used even when ACPI is
> > > > >     being used. Initialization part and FDT info looked
> > > > >     mixed up here if I am right?
> > > >
> > > > Agreed. The function has the wrong name. mach-virt has many functions that
> > > > mix the initialization and fdt building together, but those functions are
> > > > named something like create_foo(). Patches welcome.
> > > that was where I gave up on cpu hotplug arm/virt the last time.
> >
> > Were you releasing the ARM objects as well? Or are you referring to some
> > other problem?
> I was talking about mix of FDT and device creation code.

Ok. I have worked around that for now. Maybe you would like to review
it in the cpuhp patches which I should be able to float this week to the
community.

Thanks
Salil.

> > > Ideally we should split out from create_foo() all firmware generation code
> > > (fdt) and move it to virt_machine_done time + make sure that it could be
> > > regenerated at reset time so guest would get updated FDT on reset.
> >
> > Agreed, just like ACPI part. That would be more cleaner.
> >
> > Thanks
> > Salil.

RE: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Salil Mehta]

Hello,
Any comments on this would be really helpful & much appreciated.


Thanks in anticipation!

Best regards
Salil
> -----Original Message-----
> From: Salil Mehta
> Sent: Monday, June 1, 2020 4:00 PM
> To: qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Cc: Peter Maydell <peter.maydell@linaro.org>; mst@redhat.com; Igor Mammedov
> <imammedo@redhat.com>
> Subject: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs
> 
> Hello,
> I could see below within function fdt_add_pmu_nodes() part of
> hw/arm/virt.c during virt machine initialization time:
> 
> Observation:
> In below function, support of PMU feature is being checked for
> each vcpu and if the PMU is found part of the features then PMU
> is initialized with in the host/KVM. But if there is even one
> vcpu which is found to not support the PMU then loop is exited
> and PMU is not initialized for the rest of the vcpus as well.
> 
> static void fdt_add_pmu_nodes(const VirtMachineState *vms)
> {
>     CPUState *cpu;
>     ARMCPU *armcpu;
>     uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
> 
>     CPU_FOREACH(cpu) {
>         armcpu = ARM_CPU(cpu);
>         if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>             return; ------> Here, loop exits & function returns
>         }
>         if (kvm_enabled()) {
>             if (kvm_irqchip_in_kernel()) {
>                 kvm_arm_pmu_set_irq(cpu, PPI(VIRTUAL_PMU_IRQ));
>             }
>             kvm_arm_pmu_init(cpu);
>         }
>     }
> 
>     if (vms->gic_version == VIRT_GIC_VERSION_2) {
>         irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
>                              GIC_FDT_IRQ_PPI_CPU_WIDTH,
>                              (1 << vms->smp_cpus) - 1);
>     }
> 
>     armcpu = ARM_CPU(qemu_get_cpu(0));
>     qemu_fdt_add_subnode(vms->fdt, "/pmu");
>     if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
>         const char compat[] = "arm,armv8-pmuv3";
>         qemu_fdt_setprop(vms->fdt, "/pmu", "compatible",
>                          compat, sizeof(compat));
>         qemu_fdt_setprop_cells(vms->fdt, "/pmu", "interrupts",
>                                GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_PMU_IRQ, irqflags);
>     }
> }
> 
> Questions:
> Q1. Not sure what is the logic of the premature exit and not
>     continuing with further checks and initialization of other
>     VCPU PMUs?
> Q2. Does it even makes sense to have PMUs initialized for some
>     vcpus and not for others unless we have heterogeneous system?
> Q3. Also, there is a per virt machine knob of vcc->no_pmu.
>     This is something which user could specify at the init time
>     and perhaps only once but we don't use it for ARM. Perhaps
>     should have been used even before entering this function
>     to enable or disable the support as per user config?
> Q4. This function  fdt_* looks to be wrongly named. The info
>     being initialized here shall be used even when ACPI is
>     being used. Initialization part and FDT info looked
>     mixed up here if I am right?
> 
> 
> Best regards
> Salil
>