Am 31.05.2013 15:33, schrieb Luiz Capitulino: > On Thu, 30 May 2013 17:07:56 +0200 > Andreas Färber <afaer...@suse.de> wrote: > >> Signed-off-by: Andreas Färber <afaer...@suse.de> > > 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 enabled. >> * @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); > > 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; >> } >> >> +bool cpu_paging_enabled(CPUState *cpu) >> +{ >> + CPUClass *cc = CPU_GET_CLASS(cpu); >> + >> + return cc->get_paging_enabled(cpu); >> +} >> + >> +static bool cpu_common_get_paging_enabled(CPUState *cpu) >> +{ >> + return true; >> +} > > Not sure if this is important, but I wonder if we want to do this > > 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(ObjectClass *oc, >> void *data) >> cc->reset = x86_cpu_reset; >> >> cc->do_interrupt = x86_cpu_do_interrupt; >> + cc->get_arch_id = x86_cpu_get_arch_id; > > Unrelated change? > >> + cc->get_paging_enabled = x86_cpu_get_paging_enabled; >> #ifndef CONFIG_USER_ONLY >> cc->write_elf64_note = x86_cpu_write_elf64_note; >> cc->write_elf64_qemunote = x86_cpu_write_elf64_qemunote; >> @@ -2526,8 +2535,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, >> void *data) >> cc->write_elf32_qemunote = x86_cpu_write_elf32_qemunote; >> #endif >> cpu_class_set_vmsd(cc, &vmstate_x86_cpu); >> - >> - cc->get_arch_id = 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 >> } >> >> static const TypeInfo x86_cpu_type_info = { [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 = 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 = X86_CPU(cs); -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg