From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60259) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkCqH-0001Ac-Mo for qemu-devel@nongnu.org; Wed, 05 Jun 2013 08:29:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UkCqC-0007fk-BV for qemu-devel@nongnu.org; Wed, 05 Jun 2013 08:29:29 -0400 Received: from cantor2.suse.de ([195.135.220.15]:60176 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkCqB-0007fa-Vy for qemu-devel@nongnu.org; Wed, 05 Jun 2013 08:29:24 -0400 Message-ID: <51AF2F1E.3090401@suse.de> Date: Wed, 05 Jun 2013 14:29:18 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1369926481-20720-1-git-send-email-afaerber@suse.de> <1369926481-20720-5-git-send-email-afaerber@suse.de> <20130531093345.173dfc99@redhat.com> In-Reply-To: <20130531093345.173dfc99@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH qom-cpu v3 4/9] cpu: Turn cpu_paging_enabled() into a CPUState hook List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: pbonzini@redhat.com, qiaonuohan@cn.fujitsu.com, qemu-devel@nongnu.org Am 31.05.2013 15:33, schrieb Luiz Capitulino: > On Thu, 30 May 2013 17:07:56 +0200 > Andreas F=C3=A4rber wrote: >=20 >> Signed-off-by: Andreas F=C3=A4rber >=20 > Nitpick alarm on. Very welcome :) >> --- >> include/qom/cpu.h | 10 ++++++++++ >> include/sysemu/memory_mapping.h | 1 - >> memory_mapping-stub.c | 6 ------ >> memory_mapping.c | 2 +- >> qom/cpu.c | 13 +++++++++++++ >> target-i386/arch_memory_mapping.c | 6 +----- >> target-i386/cpu.c | 11 +++++++++-- >> 7 files changed, 34 insertions(+), 15 deletions(-) >> >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h >> index 7cd9442..cf5fec2 100644 >> --- a/include/qom/cpu.h >> +++ b/include/qom/cpu.h >> @@ -48,6 +48,7 @@ typedef struct CPUState CPUState; >> * @reset: Callback to reset the #CPUState to its initial state. >> * @do_interrupt: Callback for interrupt handling. >> * @get_arch_id: Callback for getting architecture-dependent CPU ID. >> + * @get_paging_enabled: Callback for inquiring whether paging is enab= led. >> * @vmsd: State description for migration. >> * >> * Represents a CPU family or model. >> @@ -62,6 +63,7 @@ typedef struct CPUClass { >> void (*reset)(CPUState *cpu); >> void (*do_interrupt)(CPUState *cpu); >> int64_t (*get_arch_id)(CPUState *cpu); >> + bool (*get_paging_enabled)(CPUState *cpu); >=20 > Argument could be const? I haven't seen any other such example in QOM, but don't see why not, changed [1]. [...] >> diff --git a/memory_mapping-stub.c b/memory_mapping-stub.c >> index 24d5d67..6c0dfeb 100644 >> --- a/memory_mapping-stub.c >> +++ b/memory_mapping-stub.c >> @@ -25,9 +25,3 @@ int cpu_get_memory_mapping(MemoryMappingList *list, >> { >> return -1; >> } >> - >> -bool cpu_paging_enabled(CPUArchState *env) >> -{ >> - return true; >> -} >> - [...] >> diff --git a/qom/cpu.c b/qom/cpu.c >> index 04aefbb..ea7e676 100644 >> --- a/qom/cpu.c >> +++ b/qom/cpu.c >> @@ -50,6 +50,18 @@ bool cpu_exists(int64_t id) >> return data.found; >> } >> =20 >> +bool cpu_paging_enabled(CPUState *cpu) >> +{ >> + CPUClass *cc =3D CPU_GET_CLASS(cpu); >> + >> + return cc->get_paging_enabled(cpu); >> +} >> + >> +static bool cpu_common_get_paging_enabled(CPUState *cpu) >> +{ >> + return true; >> +} >=20 > Not sure if this is important, but I wonder if we want to do this >=20 > I mean, for all cases where you want to know if paging is enabled, what > will happen if this default method says "yes, it's enabled" but it > actually isn't? As you can see, this is a direct conversation of today's stub into a CPUClass callback. If we want to change the default, which I believe I have advocated elsewhere, we should do so in a follow-up patch. [...] >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> index 1a501d9..7364e3b 100644 >> --- a/target-i386/cpu.c >> +++ b/target-i386/cpu.c [...] >> @@ -2519,6 +2526,8 @@ static void x86_cpu_common_class_init(ObjectClas= s *oc, void *data) >> cc->reset =3D x86_cpu_reset; >> =20 >> cc->do_interrupt =3D x86_cpu_do_interrupt; >> + cc->get_arch_id =3D x86_cpu_get_arch_id; >=20 > Unrelated change? >=20 >> + cc->get_paging_enabled =3D x86_cpu_get_paging_enabled; >> #ifndef CONFIG_USER_ONLY >> cc->write_elf64_note =3D x86_cpu_write_elf64_note; >> cc->write_elf64_qemunote =3D x86_cpu_write_elf64_qemunote; >> @@ -2526,8 +2535,6 @@ static void x86_cpu_common_class_init(ObjectClas= s *oc, void *data) >> cc->write_elf32_qemunote =3D x86_cpu_write_elf32_qemunote; >> #endif >> cpu_class_set_vmsd(cc, &vmstate_x86_cpu); >> - >> - cc->get_arch_id =3D x86_cpu_get_arch_id; As maintainer of target-i386/cpu.c I took the liberty of grouping the get_* callbacks together - there is no reason to separate this one out, and one of the following patches adds a get_memory_mapping field that needs to be assigned inside !CONFIG_USER_ONLY, thus get_paging_enabled before the #ifndef. And I think moving one line in its own patch would be overkill, even by my standards. ;) But I should mention it in the commit message then. Andreas >> } >> =20 >> static const TypeInfo x86_cpu_type_info =3D { [1] Diff: diff --git a/include/qom/cpu.h b/include/qom/cpu.h index cf5fec2..1f70240 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -63,7 +63,7 @@ typedef struct CPUClass { void (*reset)(CPUState *cpu); void (*do_interrupt)(CPUState *cpu); int64_t (*get_arch_id)(CPUState *cpu); - bool (*get_paging_enabled)(CPUState *cpu); + bool (*get_paging_enabled)(const CPUState *cpu); const struct VMStateDescription *vmsd; int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu, @@ -145,7 +145,7 @@ struct CPUState { * * Returns: %true if paging is enabled, %false otherwise. */ -bool cpu_paging_enabled(CPUState *cpu); +bool cpu_paging_enabled(const CPUState *cpu); /** * cpu_write_elf64_note: diff --git a/qom/cpu.c b/qom/cpu.c index ea7e676..9f6da0f 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -50,14 +50,14 @@ bool cpu_exists(int64_t id) return data.found; } -bool cpu_paging_enabled(CPUState *cpu) +bool cpu_paging_enabled(const CPUState *cpu) { CPUClass *cc =3D CPU_GET_CLASS(cpu); return cc->get_paging_enabled(cpu); } -static bool cpu_common_get_paging_enabled(CPUState *cpu) +static bool cpu_common_get_paging_enabled(const CPUState *cpu) { return true; } diff --git a/target-i386/cpu.c b/target-i386/cpu.c index c717ce7..f6fa7fa 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2505,7 +2505,7 @@ static int64_t x86_cpu_get_arch_id(CPUState *cs) return env->cpuid_apic_id; } -static bool x86_cpu_get_paging_enabled(CPUState *cs) +static bool x86_cpu_get_paging_enabled(const CPUState *cs) { X86CPU *cpu =3D X86_CPU(cs); --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg