* [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus'
@ 2020-12-15 17:48 Andrew Jones
2020-12-15 18:20 ` David Edmondson
2020-12-16 9:01 ` Ying Fang
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Jones @ 2020-12-15 17:48 UTC (permalink / raw)
To: qemu-devel, qemu-arm; +Cc: fangying1, peter.maydell, salil.mehta
virt machine's 'smp_cpus' and machine->smp.cpus must always have the
same value. And, anywhere we have virt machine state we have machine
state. So let's remove the redundancy. Also, to make it easier to see
that machine->smp is the true source for "smp_cpus" and "max_cpus",
avoid passing them in function parameters, preferring instead to get
them from the state.
No functional change intended.
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
hw/arm/virt-acpi-build.c | 9 +++++----
hw/arm/virt.c | 24 +++++++++++-------------
include/hw/arm/virt.h | 3 +--
3 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 711cf2069fe8..9d9ee2405345 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -59,11 +59,12 @@
#define ACPI_BUILD_TABLE_SIZE 0x20000
-static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
+static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
{
+ MachineState *ms = MACHINE(vms);
uint16_t i;
- for (i = 0; i < smp_cpus; i++) {
+ for (i = 0; i < ms->smp.cpus; i++) {
Aml *dev = aml_device("C%.03X", i);
aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
aml_append(dev, aml_name_decl("_UID", aml_int(i)));
@@ -484,7 +485,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base);
gicd->version = vms->gic_version;
- for (i = 0; i < vms->smp_cpus; i++) {
+ for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
sizeof(*gicc));
ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
@@ -603,7 +604,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
* the RTC ACPI device at all when using UEFI.
*/
scope = aml_scope("\\_SB");
- acpi_dsdt_add_cpus(scope, vms->smp_cpus);
+ acpi_dsdt_add_cpus(scope, vms);
acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
(irqmap[VIRT_UART] + ARM_SPI_BASE));
if (vmc->acpi_expose_flash) {
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 556592012ee0..534d306f3104 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -323,7 +323,7 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
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);
+ (1 << MACHINE(vms)->smp.cpus) - 1);
}
qemu_fdt_add_subnode(vms->fdt, "/timer");
@@ -347,9 +347,9 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
static void fdt_add_cpu_nodes(const VirtMachineState *vms)
{
- int cpu;
- int addr_cells = 1;
const MachineState *ms = MACHINE(vms);
+ int smp_cpus = ms->smp.cpus, cpu;
+ int addr_cells = 1;
/*
* From Documentation/devicetree/bindings/arm/cpus.txt
@@ -364,7 +364,7 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
* The simplest way to go is to examine affinity IDs of all our CPUs. If
* at least one of them has Aff3 populated, we set #address-cells to 2.
*/
- for (cpu = 0; cpu < vms->smp_cpus; cpu++) {
+ for (cpu = 0; cpu < smp_cpus; cpu++) {
ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
if (armcpu->mp_affinity & ARM_AFF3_MASK) {
@@ -377,7 +377,7 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
qemu_fdt_setprop_cell(vms->fdt, "/cpus", "#address-cells", addr_cells);
qemu_fdt_setprop_cell(vms->fdt, "/cpus", "#size-cells", 0x0);
- for (cpu = vms->smp_cpus - 1; cpu >= 0; cpu--) {
+ for (cpu = smp_cpus - 1; cpu >= 0; cpu--) {
char *nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
CPUState *cs = CPU(armcpu);
@@ -387,8 +387,7 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
armcpu->dtb_compatible);
- if (vms->psci_conduit != QEMU_PSCI_CONDUIT_DISABLED
- && vms->smp_cpus > 1) {
+ if (vms->psci_conduit != QEMU_PSCI_CONDUIT_DISABLED && smp_cpus > 1) {
qemu_fdt_setprop_string(vms->fdt, nodename,
"enable-method", "psci");
}
@@ -534,7 +533,7 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
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);
+ (1 << MACHINE(vms)->smp.cpus) - 1);
}
qemu_fdt_add_subnode(vms->fdt, "/pmu");
@@ -1673,9 +1672,9 @@ static void finalize_gic_version(VirtMachineState *vms)
* virt_cpu_post_init() must be called after the CPUs have
* been realized and the GIC has been created.
*/
-static void virt_cpu_post_init(VirtMachineState *vms, int max_cpus,
- MemoryRegion *sysmem)
+static void virt_cpu_post_init(VirtMachineState *vms, MemoryRegion *sysmem)
{
+ int max_cpus = MACHINE(vms)->smp.max_cpus;
bool aarch64, pmu, steal_time;
CPUState *cpu;
@@ -1828,8 +1827,6 @@ static void machvirt_init(MachineState *machine)
exit(1);
}
- vms->smp_cpus = smp_cpus;
-
if (vms->virt && kvm_enabled()) {
error_report("mach-virt: KVM does not support providing "
"Virtualization extensions to the guest CPU");
@@ -1845,6 +1842,7 @@ static void machvirt_init(MachineState *machine)
create_fdt(vms);
possible_cpus = mc->possible_cpu_arch_ids(machine);
+ assert(possible_cpus->len == max_cpus);
for (n = 0; n < possible_cpus->len; n++) {
Object *cpuobj;
CPUState *cs;
@@ -1965,7 +1963,7 @@ static void machvirt_init(MachineState *machine)
create_gic(vms);
- virt_cpu_post_init(vms, possible_cpus->len, sysmem);
+ virt_cpu_post_init(vms, sysmem);
fdt_add_pmu_nodes(vms);
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index abf54fab4981..e4a2d216420f 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -151,7 +151,6 @@ struct VirtMachineState {
MemMapEntry *memmap;
char *pciehb_nodename;
const int *irqmap;
- int smp_cpus;
void *fdt;
int fdt_size;
uint32_t clock_phandle;
@@ -182,7 +181,7 @@ static inline int virt_gicv3_redist_region_count(VirtMachineState *vms)
assert(vms->gic_version == VIRT_GIC_VERSION_3);
- return vms->smp_cpus > redist0_capacity ? 2 : 1;
+ return MACHINE(vms)->smp.cpus > redist0_capacity ? 2 : 1;
}
#endif /* QEMU_ARM_VIRT_H */
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus'
2020-12-15 17:48 [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus' Andrew Jones
@ 2020-12-15 18:20 ` David Edmondson
2020-12-16 6:43 ` Andrew Jones
2020-12-16 9:01 ` Ying Fang
1 sibling, 1 reply; 6+ messages in thread
From: David Edmondson @ 2020-12-15 18:20 UTC (permalink / raw)
To: Andrew Jones, qemu-devel, qemu-arm; +Cc: fangying1, peter.maydell, salil.mehta
On Tuesday, 2020-12-15 at 18:48:15 +01, Andrew Jones wrote:
> virt machine's 'smp_cpus' and machine->smp.cpus must always have the
> same value. And, anywhere we have virt machine state we have machine
> state. So let's remove the redundancy. Also, to make it easier to see
> that machine->smp is the true source for "smp_cpus" and "max_cpus",
> avoid passing them in function parameters, preferring instead to get
> them from the state.
>
> No functional change intended.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
Minor question below...
Reviewed-by: David Edmondson <david.edmondson@oracle.com>
> ---
> hw/arm/virt-acpi-build.c | 9 +++++----
> hw/arm/virt.c | 24 +++++++++++-------------
> include/hw/arm/virt.h | 3 +--
> 3 files changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 711cf2069fe8..9d9ee2405345 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -59,11 +59,12 @@
>
> #define ACPI_BUILD_TABLE_SIZE 0x20000
>
> -static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> +static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
> {
> + MachineState *ms = MACHINE(vms);
> uint16_t i;
>
> - for (i = 0; i < smp_cpus; i++) {
> + for (i = 0; i < ms->smp.cpus; i++) {
> Aml *dev = aml_device("C%.03X", i);
> aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
> aml_append(dev, aml_name_decl("_UID", aml_int(i)));
> @@ -484,7 +485,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base);
> gicd->version = vms->gic_version;
>
> - for (i = 0; i < vms->smp_cpus; i++) {
> + for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
> AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
> sizeof(*gicc));
> ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
> @@ -603,7 +604,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> * the RTC ACPI device at all when using UEFI.
> */
> scope = aml_scope("\\_SB");
> - acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> + acpi_dsdt_add_cpus(scope, vms);
> acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> (irqmap[VIRT_UART] + ARM_SPI_BASE));
> if (vmc->acpi_expose_flash) {
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 556592012ee0..534d306f3104 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -323,7 +323,7 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
> 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);
> + (1 << MACHINE(vms)->smp.cpus) - 1);
> }
>
> qemu_fdt_add_subnode(vms->fdt, "/timer");
> @@ -347,9 +347,9 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
>
> static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> {
> - int cpu;
> - int addr_cells = 1;
> const MachineState *ms = MACHINE(vms);
> + int smp_cpus = ms->smp.cpus, cpu;
Is it house-style to have initialised and un-initialised local variables
declared on the same line?
> + int addr_cells = 1;
>
> /*
> * From Documentation/devicetree/bindings/arm/cpus.txt
> @@ -364,7 +364,7 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> * The simplest way to go is to examine affinity IDs of all our CPUs. If
> * at least one of them has Aff3 populated, we set #address-cells to 2.
> */
> - for (cpu = 0; cpu < vms->smp_cpus; cpu++) {
> + for (cpu = 0; cpu < smp_cpus; cpu++) {
> ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
>
> if (armcpu->mp_affinity & ARM_AFF3_MASK) {
> @@ -377,7 +377,7 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> qemu_fdt_setprop_cell(vms->fdt, "/cpus", "#address-cells", addr_cells);
> qemu_fdt_setprop_cell(vms->fdt, "/cpus", "#size-cells", 0x0);
>
> - for (cpu = vms->smp_cpus - 1; cpu >= 0; cpu--) {
> + for (cpu = smp_cpus - 1; cpu >= 0; cpu--) {
> char *nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
> ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
> CPUState *cs = CPU(armcpu);
> @@ -387,8 +387,7 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
> armcpu->dtb_compatible);
>
> - if (vms->psci_conduit != QEMU_PSCI_CONDUIT_DISABLED
> - && vms->smp_cpus > 1) {
> + if (vms->psci_conduit != QEMU_PSCI_CONDUIT_DISABLED && smp_cpus > 1) {
> qemu_fdt_setprop_string(vms->fdt, nodename,
> "enable-method", "psci");
> }
> @@ -534,7 +533,7 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
> 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);
> + (1 << MACHINE(vms)->smp.cpus) - 1);
> }
>
> qemu_fdt_add_subnode(vms->fdt, "/pmu");
> @@ -1673,9 +1672,9 @@ static void finalize_gic_version(VirtMachineState *vms)
> * virt_cpu_post_init() must be called after the CPUs have
> * been realized and the GIC has been created.
> */
> -static void virt_cpu_post_init(VirtMachineState *vms, int max_cpus,
> - MemoryRegion *sysmem)
> +static void virt_cpu_post_init(VirtMachineState *vms, MemoryRegion *sysmem)
> {
> + int max_cpus = MACHINE(vms)->smp.max_cpus;
> bool aarch64, pmu, steal_time;
> CPUState *cpu;
>
> @@ -1828,8 +1827,6 @@ static void machvirt_init(MachineState *machine)
> exit(1);
> }
>
> - vms->smp_cpus = smp_cpus;
> -
> if (vms->virt && kvm_enabled()) {
> error_report("mach-virt: KVM does not support providing "
> "Virtualization extensions to the guest CPU");
> @@ -1845,6 +1842,7 @@ static void machvirt_init(MachineState *machine)
> create_fdt(vms);
>
> possible_cpus = mc->possible_cpu_arch_ids(machine);
> + assert(possible_cpus->len == max_cpus);
> for (n = 0; n < possible_cpus->len; n++) {
> Object *cpuobj;
> CPUState *cs;
> @@ -1965,7 +1963,7 @@ static void machvirt_init(MachineState *machine)
>
> create_gic(vms);
>
> - virt_cpu_post_init(vms, possible_cpus->len, sysmem);
> + virt_cpu_post_init(vms, sysmem);
>
> fdt_add_pmu_nodes(vms);
>
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index abf54fab4981..e4a2d216420f 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -151,7 +151,6 @@ struct VirtMachineState {
> MemMapEntry *memmap;
> char *pciehb_nodename;
> const int *irqmap;
> - int smp_cpus;
> void *fdt;
> int fdt_size;
> uint32_t clock_phandle;
> @@ -182,7 +181,7 @@ static inline int virt_gicv3_redist_region_count(VirtMachineState *vms)
>
> assert(vms->gic_version == VIRT_GIC_VERSION_3);
>
> - return vms->smp_cpus > redist0_capacity ? 2 : 1;
> + return MACHINE(vms)->smp.cpus > redist0_capacity ? 2 : 1;
> }
>
> #endif /* QEMU_ARM_VIRT_H */
> --
> 2.26.2
dme.
--
Why does it have to be like this? I can never tell.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus'
2020-12-15 18:20 ` David Edmondson
@ 2020-12-16 6:43 ` Andrew Jones
2020-12-16 8:38 ` David Edmondson
2021-01-07 12:16 ` Peter Maydell
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Jones @ 2020-12-16 6:43 UTC (permalink / raw)
To: David Edmondson
Cc: fangying1, peter.maydell, salil.mehta, qemu-arm, qemu-devel
On Tue, Dec 15, 2020 at 06:20:48PM +0000, David Edmondson wrote:
> On Tuesday, 2020-12-15 at 18:48:15 +01, Andrew Jones wrote:
>
> > virt machine's 'smp_cpus' and machine->smp.cpus must always have the
> > same value. And, anywhere we have virt machine state we have machine
> > state. So let's remove the redundancy. Also, to make it easier to see
> > that machine->smp is the true source for "smp_cpus" and "max_cpus",
> > avoid passing them in function parameters, preferring instead to get
> > them from the state.
> >
> > No functional change intended.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
>
> Minor question below...
>
> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
Thanks
> > static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> > {
> > - int cpu;
> > - int addr_cells = 1;
> > const MachineState *ms = MACHINE(vms);
> > + int smp_cpus = ms->smp.cpus, cpu;
>
> Is it house-style to have initialised and un-initialised local variables
> declared on the same line?
>
checkpatch.pl doesn't complain and a grep of qemu shows hundreds of other
examples. That said, I only see one other example in hw/arm/virt.c, so if
we'd rather avoid it, I'll repost.
Thanks,
drew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus'
2020-12-16 6:43 ` Andrew Jones
@ 2020-12-16 8:38 ` David Edmondson
2021-01-07 12:16 ` Peter Maydell
1 sibling, 0 replies; 6+ messages in thread
From: David Edmondson @ 2020-12-16 8:38 UTC (permalink / raw)
To: Andrew Jones; +Cc: fangying1, peter.maydell, salil.mehta, qemu-arm, qemu-devel
On Wednesday, 2020-12-16 at 07:43:53 +01, Andrew Jones wrote:
> On Tue, Dec 15, 2020 at 06:20:48PM +0000, David Edmondson wrote:
>> On Tuesday, 2020-12-15 at 18:48:15 +01, Andrew Jones wrote:
>>
>> > static void fdt_add_cpu_nodes(const VirtMachineState *vms)
>> > {
>> > - int cpu;
>> > - int addr_cells = 1;
>> > const MachineState *ms = MACHINE(vms);
>> > + int smp_cpus = ms->smp.cpus, cpu;
>>
>> Is it house-style to have initialised and un-initialised local variables
>> declared on the same line?
>>
>
> checkpatch.pl doesn't complain and a grep of qemu shows hundreds of other
> examples. That said, I only see one other example in hw/arm/virt.c, so if
> we'd rather avoid it, I'll repost.
Not at all, I was just curious.
dme.
--
Driving at 90 down those country lanes, singing to "Tiny Dancer".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus'
2020-12-15 17:48 [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus' Andrew Jones
2020-12-15 18:20 ` David Edmondson
@ 2020-12-16 9:01 ` Ying Fang
1 sibling, 0 replies; 6+ messages in thread
From: Ying Fang @ 2020-12-16 9:01 UTC (permalink / raw)
To: Andrew Jones, qemu-devel, qemu-arm; +Cc: peter.maydell, salil.mehta
On 12/16/2020 1:48 AM, Andrew Jones wrote:
> virt machine's 'smp_cpus' and machine->smp.cpus must always have the
> same value. And, anywhere we have virt machine state we have machine
> state. So let's remove the redundancy. Also, to make it easier to see
> that machine->smp is the true source for "smp_cpus" and "max_cpus",
> avoid passing them in function parameters, preferring instead to get
> them from the state.
>
> No functional change intended.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Ying Fang <fangying1@huawei.com>
> ---
> hw/arm/virt-acpi-build.c | 9 +++++----
> hw/arm/virt.c | 24 +++++++++++-------------
> include/hw/arm/virt.h | 3 +--
> 3 files changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 711cf2069fe8..9d9ee2405345 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -59,11 +59,12 @@
>
> #define ACPI_BUILD_TABLE_SIZE 0x20000
>
> -static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> +static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
> {
> + MachineState *ms = MACHINE(vms);
> uint16_t i;
>
> - for (i = 0; i < smp_cpus; i++) {
> + for (i = 0; i < ms->smp.cpus; i++) {
> Aml *dev = aml_device("C%.03X", i);
> aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
> aml_append(dev, aml_name_decl("_UID", aml_int(i)));
> @@ -484,7 +485,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base);
> gicd->version = vms->gic_version;
>
> - for (i = 0; i < vms->smp_cpus; i++) {
> + for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
> AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
> sizeof(*gicc));
> ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
> @@ -603,7 +604,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> * the RTC ACPI device at all when using UEFI.
> */
> scope = aml_scope("\\_SB");
> - acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> + acpi_dsdt_add_cpus(scope, vms);
> acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> (irqmap[VIRT_UART] + ARM_SPI_BASE));
> if (vmc->acpi_expose_flash) {
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 556592012ee0..534d306f3104 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -323,7 +323,7 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
> 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);
> + (1 << MACHINE(vms)->smp.cpus) - 1);
> }
>
> qemu_fdt_add_subnode(vms->fdt, "/timer");
> @@ -347,9 +347,9 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
>
> static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> {
> - int cpu;
> - int addr_cells = 1;
> const MachineState *ms = MACHINE(vms);
> + int smp_cpus = ms->smp.cpus, cpu;
> + int addr_cells = 1;
>
> /*
> * From Documentation/devicetree/bindings/arm/cpus.txt
> @@ -364,7 +364,7 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> * The simplest way to go is to examine affinity IDs of all our CPUs. If
> * at least one of them has Aff3 populated, we set #address-cells to 2.
> */
> - for (cpu = 0; cpu < vms->smp_cpus; cpu++) {
> + for (cpu = 0; cpu < smp_cpus; cpu++) {
> ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
>
> if (armcpu->mp_affinity & ARM_AFF3_MASK) {
> @@ -377,7 +377,7 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> qemu_fdt_setprop_cell(vms->fdt, "/cpus", "#address-cells", addr_cells);
> qemu_fdt_setprop_cell(vms->fdt, "/cpus", "#size-cells", 0x0);
>
> - for (cpu = vms->smp_cpus - 1; cpu >= 0; cpu--) {
> + for (cpu = smp_cpus - 1; cpu >= 0; cpu--) {
> char *nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
> ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
> CPUState *cs = CPU(armcpu);
> @@ -387,8 +387,7 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
> armcpu->dtb_compatible);
>
> - if (vms->psci_conduit != QEMU_PSCI_CONDUIT_DISABLED
> - && vms->smp_cpus > 1) {
> + if (vms->psci_conduit != QEMU_PSCI_CONDUIT_DISABLED && smp_cpus > 1) {
> qemu_fdt_setprop_string(vms->fdt, nodename,
> "enable-method", "psci");
> }
> @@ -534,7 +533,7 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
> 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);
> + (1 << MACHINE(vms)->smp.cpus) - 1);
> }
>
> qemu_fdt_add_subnode(vms->fdt, "/pmu");
> @@ -1673,9 +1672,9 @@ static void finalize_gic_version(VirtMachineState *vms)
> * virt_cpu_post_init() must be called after the CPUs have
> * been realized and the GIC has been created.
> */
> -static void virt_cpu_post_init(VirtMachineState *vms, int max_cpus,
> - MemoryRegion *sysmem)
> +static void virt_cpu_post_init(VirtMachineState *vms, MemoryRegion *sysmem)
> {
> + int max_cpus = MACHINE(vms)->smp.max_cpus;
> bool aarch64, pmu, steal_time;
> CPUState *cpu;
>
> @@ -1828,8 +1827,6 @@ static void machvirt_init(MachineState *machine)
> exit(1);
> }
>
> - vms->smp_cpus = smp_cpus;
> -
> if (vms->virt && kvm_enabled()) {
> error_report("mach-virt: KVM does not support providing "
> "Virtualization extensions to the guest CPU");
> @@ -1845,6 +1842,7 @@ static void machvirt_init(MachineState *machine)
> create_fdt(vms);
>
> possible_cpus = mc->possible_cpu_arch_ids(machine);
> + assert(possible_cpus->len == max_cpus);
> for (n = 0; n < possible_cpus->len; n++) {
> Object *cpuobj;
> CPUState *cs;
> @@ -1965,7 +1963,7 @@ static void machvirt_init(MachineState *machine)
>
> create_gic(vms);
>
> - virt_cpu_post_init(vms, possible_cpus->len, sysmem);
> + virt_cpu_post_init(vms, sysmem);
>
> fdt_add_pmu_nodes(vms);
>
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index abf54fab4981..e4a2d216420f 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -151,7 +151,6 @@ struct VirtMachineState {
> MemMapEntry *memmap;
> char *pciehb_nodename;
> const int *irqmap;
> - int smp_cpus;
> void *fdt;
> int fdt_size;
> uint32_t clock_phandle;
> @@ -182,7 +181,7 @@ static inline int virt_gicv3_redist_region_count(VirtMachineState *vms)
>
> assert(vms->gic_version == VIRT_GIC_VERSION_3);
>
> - return vms->smp_cpus > redist0_capacity ? 2 : 1;
> + return MACHINE(vms)->smp.cpus > redist0_capacity ? 2 : 1;
> }
>
> #endif /* QEMU_ARM_VIRT_H */
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus'
2020-12-16 6:43 ` Andrew Jones
2020-12-16 8:38 ` David Edmondson
@ 2021-01-07 12:16 ` Peter Maydell
1 sibling, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2021-01-07 12:16 UTC (permalink / raw)
To: Andrew Jones
Cc: David Edmondson, Ying Fang, salil.mehta, qemu-arm, QEMU Developers
On Wed, 16 Dec 2020 at 06:44, Andrew Jones <drjones@redhat.com> wrote:
>
> On Tue, Dec 15, 2020 at 06:20:48PM +0000, David Edmondson wrote:
> > On Tuesday, 2020-12-15 at 18:48:15 +01, Andrew Jones wrote:
> >
> > > virt machine's 'smp_cpus' and machine->smp.cpus must always have the
> > > same value. And, anywhere we have virt machine state we have machine
> > > state. So let's remove the redundancy. Also, to make it easier to see
> > > that machine->smp is the true source for "smp_cpus" and "max_cpus",
> > > avoid passing them in function parameters, preferring instead to get
> > > them from the state.
> > > static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> > > {
> > > - int cpu;
> > > - int addr_cells = 1;
> > > const MachineState *ms = MACHINE(vms);
> > > + int smp_cpus = ms->smp.cpus, cpu;
> >
> > Is it house-style to have initialised and un-initialised local variables
> > declared on the same line?
> >
>
> checkpatch.pl doesn't complain and a grep of qemu shows hundreds of other
> examples. That said, I only see one other example in hw/arm/virt.c, so if
> we'd rather avoid it, I'll repost.
I think this is one of those things where the style guide doesn't
say anything, so it comes down to individual developer preference.
Personally I find declaring an uninitialized local on the same
line and after an initialized local is a bit confusing to read
so I've tweaked the patch, but it's not a big deal either way.
Applied to target-arm.next, thanks.
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-07 12:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-15 17:48 [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus' Andrew Jones
2020-12-15 18:20 ` David Edmondson
2020-12-16 6:43 ` Andrew Jones
2020-12-16 8:38 ` David Edmondson
2021-01-07 12:16 ` Peter Maydell
2020-12-16 9:01 ` Ying Fang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.