Re: [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus'
On Wed, 16 Dec 2020 at 06:44, Andrew Jones wrote:
>
> On Tue, Dec 15, 2020 at 06:20:48PM +, 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
Re: [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus'
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
Reviewed-by: Ying Fang
---
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 0x2
-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, [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,
-
Re: [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus'
On Wednesday, 2020-12-16 at 07:43:53 +01, Andrew Jones wrote:
> On Tue, Dec 15, 2020 at 06:20:48PM +, 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".
Re: [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus'
On Tue, Dec 15, 2020 at 06:20:48PM +, 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
>
> Minor question below...
>
> Reviewed-by: David Edmondson
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
Re: [PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus'
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
Minor question below...
Reviewed-by: David Edmondson
> ---
> 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 0x2
>
> -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, [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,
>
[PATCH] hw/arm/virt: Remove virt machine state 'smp_cpus'
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
---
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 0x2
-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, [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);
}
