From: Like Xu <like.xu@linux.intel.com> To: Igor Mammedov <imammedo@redhat.com> Cc: qemu-trivial@nongnu.org, qemu-devel@nongnu.org, "Daniel P. Berrangé" <berrange@redhat.com>, "Eduardo Habkost" <ehabkost@redhat.com>, "Paolo Bonzini" <pbonzini@redhat.com>, like.xu@intel.com Subject: Re: [Qemu-devel] [PATCH 3/9] cpu/topology: add uncommon arch support for smp machine properties Date: Tue, 16 Apr 2019 16:47:55 +0800 [thread overview] Message-ID: <4d0ce2df-b396-80a0-a3b9-f2513bd2289d@linux.intel.com> (raw) In-Reply-To: <20190408145424.45af3db8@redhat.com> On 2019/4/8 20:54, Igor Mammedov wrote: > On Fri, 29 Mar 2019 16:48:39 +0800 > Like Xu <like.xu@linux.intel.com> wrote: > > here should be a commit message explaining what patch does > in more detail. > > >> Signed-off-by: Like Xu <like.xu@linux.intel.com> > > Generic note, try not call qdev_get_machine() every time > you replace smp_cpus or other variables. It's often possible > to pass MachineState via call chain with trivial fixups. Hi Igor, I have some doubts on this comments after some attempts. I'm not sure if this idea could apply to all qdev_get_machine() usages in tree or just for this smp-touch-only patch. It takes a lot of efforts on hooks overrides when we undo calls to qdev_get_machine() with modification of incoming parameters. The implementation of qdev_get_machine() couldn't be simpler more and it doesn't seem to bring much overhead compared with parameter stack. > >> --- >> hw/alpha/dp264.c | 1 + >> hw/hppa/machine.c | 4 ++++ >> hw/mips/boston.c | 1 + >> hw/mips/mips_malta.c | 9 +++++++++ >> hw/sparc/sun4m.c | 2 ++ >> hw/sparc64/sun4u.c | 2 ++ >> hw/xtensa/sim.c | 1 + >> hw/xtensa/xtfpga.c | 1 + >> 8 files changed, 21 insertions(+) >> >> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c >> index 0347eb8..ee5d432 100644 >> --- a/hw/alpha/dp264.c >> +++ b/hw/alpha/dp264.c >> @@ -63,6 +63,7 @@ static void clipper_init(MachineState *machine) >> char *palcode_filename; >> uint64_t palcode_entry, palcode_low, palcode_high; >> uint64_t kernel_entry, kernel_low, kernel_high; >> + unsigned int smp_cpus = machine->topo.smp_cpus; >> >> /* Create up to 4 cpus. */ >> memset(cpus, 0, sizeof(cpus)); >> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c >> index d1b1d3c..f652891 100644 >> --- a/hw/hppa/machine.c >> +++ b/hw/hppa/machine.c >> @@ -16,6 +16,7 @@ >> #include "hw/ide.h" >> #include "hw/timer/i8254.h" >> #include "hw/char/serial.h" >> +#include "hw/boards.h" >> #include "hppa_sys.h" >> #include "qemu/units.h" >> #include "qapi/error.h" >> @@ -72,6 +73,7 @@ static void machine_hppa_init(MachineState *machine) >> MemoryRegion *ram_region; >> MemoryRegion *cpu_region; >> long i; >> + unsigned int smp_cpus = machine->topo.smp_cpus; > I'd prefer to replace smp_cpus with machine->topo.smp_cpus > directly at places it's used, as it makes affected sites > more visible in the patch. > And use local smp_cpus only in places where using machine->topo.smp_cpus > makes core less readable. > (but it's just personal preference so I don't insist on it) > >> >> ram_size = machine->ram_size; >> >> @@ -242,7 +244,9 @@ static void machine_hppa_init(MachineState *machine) >> >> static void hppa_machine_reset(void) >> { >> + MachineState *ms = MACHINE(qdev_get_machine()); >> int i; >> + unsigned int smp_cpus = ms->topo.smp_cpus; > > ***) > It would be better to pass MachineState as argument to > hppa_machine_reset(), a patch to so should go before this one. > > Quick look shows only 3 overrides (hppa, pc, pnv) and one caller, > so I'd rather fix it than calling qdev_get_machine() unnecessarily > >> >> qemu_devices_reset(); >> >> diff --git a/hw/mips/boston.c b/hw/mips/boston.c >> index e5bab3c..7752c10 100644 >> --- a/hw/mips/boston.c >> +++ b/hw/mips/boston.c >> @@ -434,6 +434,7 @@ static void boston_mach_init(MachineState *machine) >> DriveInfo *hd[6]; >> Chardev *chr; >> int fw_size, fit_err; >> + unsigned int smp_cpus = machine->topo.smp_cpus; >> bool is_64b; >> >> if ((machine->ram_size % GiB) || >> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c >> index 439665a..d595375 100644 >> --- a/hw/mips/mips_malta.c >> +++ b/hw/mips/mips_malta.c >> @@ -1095,6 +1095,8 @@ static int64_t load_kernel (void) >> >> static void malta_mips_config(MIPSCPU *cpu) >> { >> + MachineState *ms = MACHINE(qdev_get_machine()); >> + unsigned int smp_cpus = ms->topo.smp_cpus; >> CPUMIPSState *env = &cpu->env; >> CPUState *cs = CPU(cpu); > this one also called from reset, so the same [***] applies here too. > >> >> @@ -1127,9 +1129,11 @@ static void main_cpu_reset(void *opaque) >> static void create_cpu_without_cps(const char *cpu_type, >> qemu_irq *cbus_irq, qemu_irq *i8259_irq) >> { >> + MachineState *ms = MACHINE(qdev_get_machine()); > caller has an access to MachineState so pass it down call chain all the way > >> CPUMIPSState *env; >> MIPSCPU *cpu; >> int i; >> + unsigned int smp_cpus = ms->topo.smp_cpus; >> >> for (i = 0; i < smp_cpus; i++) { >> cpu = MIPS_CPU(cpu_create(cpu_type)); >> @@ -1149,7 +1153,9 @@ static void create_cpu_without_cps(const char *cpu_type, >> static void create_cps(MaltaState *s, const char *cpu_type, >> qemu_irq *cbus_irq, qemu_irq *i8259_irq) >> { >> + MachineState *ms = MACHINE(qdev_get_machine()); > ditto > >> Error *err = NULL; >> + unsigned int smp_cpus = ms->topo.smp_cpus; >> >> s->cps = MIPS_CPS(object_new(TYPE_MIPS_CPS)); >> qdev_set_parent_bus(DEVICE(s->cps), sysbus_get_default()); >> @@ -1171,6 +1177,9 @@ static void create_cps(MaltaState *s, const char *cpu_type, >> static void mips_create_cpu(MaltaState *s, const char *cpu_type, >> qemu_irq *cbus_irq, qemu_irq *i8259_irq) >> { >> + MachineState *ms = MACHINE(qdev_get_machine()); > ditto > >> + unsigned int smp_cpus = ms->topo.smp_cpus; >> + >> if ((smp_cpus > 1) && cpu_supports_cps_smp(cpu_type)) { >> create_cps(s, cpu_type, cbus_irq, i8259_irq); >> } else { >> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c >> index ca1e382..2321cfb 100644 >> --- a/hw/sparc/sun4m.c >> +++ b/hw/sparc/sun4m.c >> @@ -853,6 +853,8 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, >> unsigned int num_vsimms; >> DeviceState *dev; >> SysBusDevice *s; >> + unsigned int smp_cpus = machine->topo.smp_cpus; >> + unsigned int max_cpus = machine->topo.max_cpus; >> >> /* init CPUs */ >> for(i = 0; i < smp_cpus; i++) { >> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c >> index 399f2d7..d089c4d 100644 >> --- a/hw/sparc64/sun4u.c >> +++ b/hw/sparc64/sun4u.c >> @@ -546,6 +546,8 @@ static void sun4uv_init(MemoryRegion *address_space_mem, >> NICInfo *nd; >> MACAddr macaddr; >> bool onboard_nic; >> + unsigned int smp_cpus = machine->topo.smp_cpus; >> + unsigned int max_cpus = machine->topo.max_cpus; >> >> /* init CPUs */ >> cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr); >> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c >> index 12c7437..cc0396b 100644 >> --- a/hw/xtensa/sim.c >> +++ b/hw/xtensa/sim.c >> @@ -59,6 +59,7 @@ static void xtensa_sim_init(MachineState *machine) >> ram_addr_t ram_size = machine->ram_size; >> const char *kernel_filename = machine->kernel_filename; >> int n; >> + unsigned int smp_cpus = machine->topo.smp_cpus; >> >> for (n = 0; n < smp_cpus; n++) { >> cpu = XTENSA_CPU(cpu_create(machine->cpu_type)); >> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c >> index e05ef75..f71a15e 100644 >> --- a/hw/xtensa/xtfpga.c >> +++ b/hw/xtensa/xtfpga.c >> @@ -238,6 +238,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine) >> const unsigned system_io_size = 224 * MiB; >> uint32_t freq = 10000000; >> int n; >> + unsigned int smp_cpus = machine->topo.smp_cpus; >> >> if (smp_cpus > 1) { >> mx_pic = xtensa_mx_pic_init(31); > >
WARNING: multiple messages have this Message-ID (diff)
From: Like Xu <like.xu@linux.intel.com> To: Igor Mammedov <imammedo@redhat.com> Cc: Eduardo Habkost <ehabkost@redhat.com>, qemu-trivial@nongnu.org, qemu-devel@nongnu.org, like.xu@intel.com, Paolo Bonzini <pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH 3/9] cpu/topology: add uncommon arch support for smp machine properties Date: Tue, 16 Apr 2019 16:47:55 +0800 [thread overview] Message-ID: <4d0ce2df-b396-80a0-a3b9-f2513bd2289d@linux.intel.com> (raw) Message-ID: <20190416084755.zIPmefarcJxLfgaDNoIdUtz58mkqUfTPuiXf97QHuAk@z> (raw) In-Reply-To: <20190408145424.45af3db8@redhat.com> On 2019/4/8 20:54, Igor Mammedov wrote: > On Fri, 29 Mar 2019 16:48:39 +0800 > Like Xu <like.xu@linux.intel.com> wrote: > > here should be a commit message explaining what patch does > in more detail. > > >> Signed-off-by: Like Xu <like.xu@linux.intel.com> > > Generic note, try not call qdev_get_machine() every time > you replace smp_cpus or other variables. It's often possible > to pass MachineState via call chain with trivial fixups. Hi Igor, I have some doubts on this comments after some attempts. I'm not sure if this idea could apply to all qdev_get_machine() usages in tree or just for this smp-touch-only patch. It takes a lot of efforts on hooks overrides when we undo calls to qdev_get_machine() with modification of incoming parameters. The implementation of qdev_get_machine() couldn't be simpler more and it doesn't seem to bring much overhead compared with parameter stack. > >> --- >> hw/alpha/dp264.c | 1 + >> hw/hppa/machine.c | 4 ++++ >> hw/mips/boston.c | 1 + >> hw/mips/mips_malta.c | 9 +++++++++ >> hw/sparc/sun4m.c | 2 ++ >> hw/sparc64/sun4u.c | 2 ++ >> hw/xtensa/sim.c | 1 + >> hw/xtensa/xtfpga.c | 1 + >> 8 files changed, 21 insertions(+) >> >> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c >> index 0347eb8..ee5d432 100644 >> --- a/hw/alpha/dp264.c >> +++ b/hw/alpha/dp264.c >> @@ -63,6 +63,7 @@ static void clipper_init(MachineState *machine) >> char *palcode_filename; >> uint64_t palcode_entry, palcode_low, palcode_high; >> uint64_t kernel_entry, kernel_low, kernel_high; >> + unsigned int smp_cpus = machine->topo.smp_cpus; >> >> /* Create up to 4 cpus. */ >> memset(cpus, 0, sizeof(cpus)); >> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c >> index d1b1d3c..f652891 100644 >> --- a/hw/hppa/machine.c >> +++ b/hw/hppa/machine.c >> @@ -16,6 +16,7 @@ >> #include "hw/ide.h" >> #include "hw/timer/i8254.h" >> #include "hw/char/serial.h" >> +#include "hw/boards.h" >> #include "hppa_sys.h" >> #include "qemu/units.h" >> #include "qapi/error.h" >> @@ -72,6 +73,7 @@ static void machine_hppa_init(MachineState *machine) >> MemoryRegion *ram_region; >> MemoryRegion *cpu_region; >> long i; >> + unsigned int smp_cpus = machine->topo.smp_cpus; > I'd prefer to replace smp_cpus with machine->topo.smp_cpus > directly at places it's used, as it makes affected sites > more visible in the patch. > And use local smp_cpus only in places where using machine->topo.smp_cpus > makes core less readable. > (but it's just personal preference so I don't insist on it) > >> >> ram_size = machine->ram_size; >> >> @@ -242,7 +244,9 @@ static void machine_hppa_init(MachineState *machine) >> >> static void hppa_machine_reset(void) >> { >> + MachineState *ms = MACHINE(qdev_get_machine()); >> int i; >> + unsigned int smp_cpus = ms->topo.smp_cpus; > > ***) > It would be better to pass MachineState as argument to > hppa_machine_reset(), a patch to so should go before this one. > > Quick look shows only 3 overrides (hppa, pc, pnv) and one caller, > so I'd rather fix it than calling qdev_get_machine() unnecessarily > >> >> qemu_devices_reset(); >> >> diff --git a/hw/mips/boston.c b/hw/mips/boston.c >> index e5bab3c..7752c10 100644 >> --- a/hw/mips/boston.c >> +++ b/hw/mips/boston.c >> @@ -434,6 +434,7 @@ static void boston_mach_init(MachineState *machine) >> DriveInfo *hd[6]; >> Chardev *chr; >> int fw_size, fit_err; >> + unsigned int smp_cpus = machine->topo.smp_cpus; >> bool is_64b; >> >> if ((machine->ram_size % GiB) || >> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c >> index 439665a..d595375 100644 >> --- a/hw/mips/mips_malta.c >> +++ b/hw/mips/mips_malta.c >> @@ -1095,6 +1095,8 @@ static int64_t load_kernel (void) >> >> static void malta_mips_config(MIPSCPU *cpu) >> { >> + MachineState *ms = MACHINE(qdev_get_machine()); >> + unsigned int smp_cpus = ms->topo.smp_cpus; >> CPUMIPSState *env = &cpu->env; >> CPUState *cs = CPU(cpu); > this one also called from reset, so the same [***] applies here too. > >> >> @@ -1127,9 +1129,11 @@ static void main_cpu_reset(void *opaque) >> static void create_cpu_without_cps(const char *cpu_type, >> qemu_irq *cbus_irq, qemu_irq *i8259_irq) >> { >> + MachineState *ms = MACHINE(qdev_get_machine()); > caller has an access to MachineState so pass it down call chain all the way > >> CPUMIPSState *env; >> MIPSCPU *cpu; >> int i; >> + unsigned int smp_cpus = ms->topo.smp_cpus; >> >> for (i = 0; i < smp_cpus; i++) { >> cpu = MIPS_CPU(cpu_create(cpu_type)); >> @@ -1149,7 +1153,9 @@ static void create_cpu_without_cps(const char *cpu_type, >> static void create_cps(MaltaState *s, const char *cpu_type, >> qemu_irq *cbus_irq, qemu_irq *i8259_irq) >> { >> + MachineState *ms = MACHINE(qdev_get_machine()); > ditto > >> Error *err = NULL; >> + unsigned int smp_cpus = ms->topo.smp_cpus; >> >> s->cps = MIPS_CPS(object_new(TYPE_MIPS_CPS)); >> qdev_set_parent_bus(DEVICE(s->cps), sysbus_get_default()); >> @@ -1171,6 +1177,9 @@ static void create_cps(MaltaState *s, const char *cpu_type, >> static void mips_create_cpu(MaltaState *s, const char *cpu_type, >> qemu_irq *cbus_irq, qemu_irq *i8259_irq) >> { >> + MachineState *ms = MACHINE(qdev_get_machine()); > ditto > >> + unsigned int smp_cpus = ms->topo.smp_cpus; >> + >> if ((smp_cpus > 1) && cpu_supports_cps_smp(cpu_type)) { >> create_cps(s, cpu_type, cbus_irq, i8259_irq); >> } else { >> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c >> index ca1e382..2321cfb 100644 >> --- a/hw/sparc/sun4m.c >> +++ b/hw/sparc/sun4m.c >> @@ -853,6 +853,8 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, >> unsigned int num_vsimms; >> DeviceState *dev; >> SysBusDevice *s; >> + unsigned int smp_cpus = machine->topo.smp_cpus; >> + unsigned int max_cpus = machine->topo.max_cpus; >> >> /* init CPUs */ >> for(i = 0; i < smp_cpus; i++) { >> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c >> index 399f2d7..d089c4d 100644 >> --- a/hw/sparc64/sun4u.c >> +++ b/hw/sparc64/sun4u.c >> @@ -546,6 +546,8 @@ static void sun4uv_init(MemoryRegion *address_space_mem, >> NICInfo *nd; >> MACAddr macaddr; >> bool onboard_nic; >> + unsigned int smp_cpus = machine->topo.smp_cpus; >> + unsigned int max_cpus = machine->topo.max_cpus; >> >> /* init CPUs */ >> cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr); >> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c >> index 12c7437..cc0396b 100644 >> --- a/hw/xtensa/sim.c >> +++ b/hw/xtensa/sim.c >> @@ -59,6 +59,7 @@ static void xtensa_sim_init(MachineState *machine) >> ram_addr_t ram_size = machine->ram_size; >> const char *kernel_filename = machine->kernel_filename; >> int n; >> + unsigned int smp_cpus = machine->topo.smp_cpus; >> >> for (n = 0; n < smp_cpus; n++) { >> cpu = XTENSA_CPU(cpu_create(machine->cpu_type)); >> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c >> index e05ef75..f71a15e 100644 >> --- a/hw/xtensa/xtfpga.c >> +++ b/hw/xtensa/xtfpga.c >> @@ -238,6 +238,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine) >> const unsigned system_io_size = 224 * MiB; >> uint32_t freq = 10000000; >> int n; >> + unsigned int smp_cpus = machine->topo.smp_cpus; >> >> if (smp_cpus > 1) { >> mx_pic = xtensa_mx_pic_init(31); > >
next prev parent reply other threads:[~2019-04-16 8:48 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <1553849325-44201-1-git-send-email-like.xu@linux.intel.com> [not found] ` <20190329112152.0c7ad147@redhat.com> 2019-04-04 3:26 ` [Qemu-devel] [PATCH 0/9] refactor cpu topo into machine properties Like Xu 2019-04-08 13:26 ` Igor Mammedov 2019-04-08 13:26 ` Igor Mammedov 2019-04-08 14:38 ` Like Xu 2019-04-08 14:38 ` Like Xu [not found] ` <1553849325-44201-2-git-send-email-like.xu@linux.intel.com> 2019-04-04 11:37 ` [Qemu-devel] [PATCH 1/9] cpu/topology: add struct CpuTopology to MachineState Igor Mammedov [not found] ` <1553849325-44201-3-git-send-email-like.xu@linux.intel.com> 2019-04-04 14:25 ` [Qemu-devel] [PATCH 2/9] cpu/topology: add general support for machine properties Igor Mammedov 2019-04-04 16:21 ` Dr. David Alan Gilbert 2019-04-30 7:30 ` Like Xu 2019-04-30 7:30 ` Like Xu 2019-05-02 15:09 ` Igor Mammedov 2019-05-02 15:09 ` Igor Mammedov 2019-05-03 1:08 ` Eduardo Habkost 2019-05-03 1:01 ` Eduardo Habkost 2019-05-03 1:01 ` Eduardo Habkost [not found] ` <1553849325-44201-4-git-send-email-like.xu@linux.intel.com> 2019-04-08 12:54 ` [Qemu-devel] [PATCH 3/9] cpu/topology: add uncommon arch support for smp " Igor Mammedov 2019-04-08 12:54 ` Igor Mammedov 2019-04-16 8:47 ` Like Xu [this message] 2019-04-16 8:47 ` Like Xu 2019-04-16 12:00 ` Igor Mammedov 2019-04-16 12:00 ` Igor Mammedov [not found] ` <1553849325-44201-5-git-send-email-like.xu@linux.intel.com> [not found] ` <87h8bmuj2d.fsf@zen.linaroharston> [not found] ` <2546bf3e-2009-5a76-bc63-0ad73d333a78@linux.intel.com> 2019-04-01 23:38 ` [Qemu-devel] [PATCH 4/9] cpu/topology: add ARM " Eduardo Habkost 2019-04-02 2:35 ` Like Xu 2019-04-02 4:45 ` Peter Maydell 2019-04-02 5:20 ` Like Xu 2019-04-02 5:27 ` Peter Maydell 2019-04-08 13:11 ` Igor Mammedov 2019-04-08 13:11 ` Igor Mammedov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4d0ce2df-b396-80a0-a3b9-f2513bd2289d@linux.intel.com \ --to=like.xu@linux.intel.com \ --cc=berrange@redhat.com \ --cc=ehabkost@redhat.com \ --cc=imammedo@redhat.com \ --cc=like.xu@intel.com \ --cc=pbonzini@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=qemu-trivial@nongnu.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.