[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH qom-cpu v3 4/9] cpu: Turn cpu_paging_enabled() i
From: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [PATCH qom-cpu v3 4/9] cpu: Turn cpu_paging_enabled() into a CPUState hook |
Date: |
Wed, 05 Jun 2013 14:29:18 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130510 Thunderbird/17.0.6 |
Am 31.05.2013 15:33, schrieb Luiz Capitulino:
> On Thu, 30 May 2013 17:07:56 +0200
> Andreas Färber <address@hidden> wrote:
>
>> Signed-off-by: Andreas Färber <address@hidden>
>
> 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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH qom-cpu v3 4/9] cpu: Turn cpu_paging_enabled() into a CPUState hook,
Andreas Färber <=