[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry
From: |
Andrew Jones |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry as enabled unconditionally |
Date: |
Fri, 29 Jan 2016 17:07:31 +0100 |
User-agent: |
Mutt/1.5.23.1 (2014-03-12) |
On Fri, Jan 29, 2016 at 11:44:24PM +0800, Shannon Zhao wrote:
>
>
> On 2016/1/29 23:26, Andrew Jones wrote:
> >On Fri, Jan 29, 2016 at 10:59:32PM +0800, Shannon Zhao wrote:
> >>>
> >>>
> >>>On 2016/1/29 22:24, Igor Mammedov wrote:
> >>>> >in current impl. condition
> >>>> >
> >>>> >build_madt() {
> >>>> > ...
> >>>> > if (test_bit(i, cpuinfo->found_cpus))
> >>>> >
> >>>> >is always true since loop handles only present CPUs
> >>>> >in range [0..smp_cpus).
> >>>> >But to fill usless cpuinfo->found_cpus we do unnecessary
> >>>> >scan over QOM tree to find the same CPUs.
> >>>> >So mark GICC as present always and drop not needed
> >>>> >code that fills cpuinfo->found_cpus.
> >>>> >
> >>>> >Signed-off-by: Igor Mammedov<address@hidden>
> >>>> >---
> >>>> >It's just simple cleanup but I'm trying to generalize
> >>>> >a bit CPU related ACPI tables and as part of it get rid
> >>>> >of found_cpus bitmap and if possible cpu_index usage
> >>>> >in ACPI parts of code.
> >>>> >---
> >>>> > hw/arm/virt-acpi-build.c | 26 +++-----------------------
> >>>> > 1 file changed, 3 insertions(+), 23 deletions(-)
> >>>> >
> >>>> >diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >>>> >index 87fbe7c..3ed39fc 100644
> >>>> >--- a/hw/arm/virt-acpi-build.c
> >>>> >+++ b/hw/arm/virt-acpi-build.c
> >>>> >@@ -46,20 +46,6 @@
> >>>> > #define ARM_SPI_BASE 32
> >>>> > #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> >>>> >
> >>>> >-typedef struct VirtAcpiCpuInfo {
> >>>> >- DECLARE_BITMAP(found_cpus, VIRT_ACPI_CPU_ID_LIMIT);
> >>>> >-} VirtAcpiCpuInfo;
> >>>> >-
> >>>> >-static void virt_acpi_get_cpu_info(VirtAcpiCpuInfo *cpuinfo)
> >>>> >-{
> >>>> >- CPUState *cpu;
> >>>> >-
> >>>> >- memset(cpuinfo->found_cpus, 0, sizeof cpuinfo->found_cpus);
> >>>> >- CPU_FOREACH(cpu) {
> >>>> >- set_bit(cpu->cpu_index, cpuinfo->found_cpus);
> >>>> >- }
> >>>> >-}
> >>>> >-
> >>>> > static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> >>>> > {
> >>>> > uint16_t i;
> >>>> >@@ -458,8 +444,7 @@ build_gtdt(GArray *table_data, GArray *linker)
> >>>> >
> >>>> > /* MADT */
> >>>> > static void
> >>>> >-build_madt(GArray *table_data, GArray *linker, VirtGuestInfo
> >>>> >*guest_info,
> >>>> >- VirtAcpiCpuInfo *cpuinfo)
> >>>> >+build_madt(GArray *table_data, GArray *linker, VirtGuestInfo
> >>>> >*guest_info)
> >>>> > {
> >>>> > int madt_start = table_data->len;
> >>>> > const MemMapEntry *memmap = guest_info->memmap;
> >>>> >@@ -489,9 +474,7 @@ build_madt(GArray *table_data, GArray *linker,
> >>>> >VirtGuestInfo *guest_info,
> >>>> > gicc->cpu_interface_number = i;
> >>>> > gicc->arm_mpidr = armcpu->mp_affinity;
> >>>> > gicc->uid = i;
> >>>> >- if (test_bit(i, cpuinfo->found_cpus)) {
> >>>> >- gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> >>>> >- }
> >>>> >+ gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> >>>> > }
> >>>Ah, yes, it uses smp_cpus not max_cpus. But we still needs to support
> >>>max_cpus usage even though it doesn't support vcpu hotplug currently. So we
> >>>may need to introduce guest_info->max_cpus and use it here.
> >We should leave that for when the hotplug patches come, and we should
> >probably leave the hotplug patches until we see what Igor plans for
> >sharing more ACPI code between x86 and ARM.
> >
> Even if ignoring the vcpu hotplug, we still need to support max_cpus and
> smp_cpus usage like -smp 1,maxcpus=4.
OK, without hotplug, max > smp doesn't gain anything, max < smp results
in an error, and therefore the only useful case is max == smp.
>
> >>>And below check in virt.c is not right while it should compare the global
> >>>max_cpus with the max_cpus GIC supports.
> >>>
> >>> if (smp_cpus > max_cpus) {
> >>> error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
> >>> "supported by machine 'mach-virt' (%d)",
> >>> smp_cpus, max_cpus);
> >>> exit(1);
> >>> }
> >max_cpus is getting set to the number the gic supports just above this
> >check. So max_cpus == gic_supported_cpus already, and this check is just
> >confirming the number of cpus the user has selected is OK.
> No, the global max_cpus (which is defined in vl.c and exported in
> sysemu/sysemu.h) is not the local variable max_cpus.
I now see what you mean though. If we don't want something like
-smp 1,maxcpus=9 to erroneously succeed on a gicv2 machine, then we
should be checking the global max_cpus here. I agree it should be
fixed, because, even though it changes nothing atm, we don't want to
allow invalid command lines.
Will you send the patch?
Thanks,
drew