Re: [Qemu-devel] [PATCH qom-cpu v3 4/9] cpu: Turn cpu_paging_enabled() into a CPUState hook

[Andreas Färber]

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