Patchwork [RFC,qom-cpu,v2,1/8] apic: remove apic_no from apic_init_common()

login
register
mail settings
Submitter chenfan
Date Sept. 10, 2013, 9:43 a.m.
Message ID <c0ad457b3fe6bc7df266c66c2e7f4d7778e5dbae.1378796990.git.chen.fan.fnst@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/273805/
State New
Headers show

Comments

chenfan - Sept. 10, 2013, 9:43 a.m.
the 'apic_no' is increased by one when initialize/create a vCPU each time,
which causes APICCommonState s->idx always is increased.
but if we want to re-add a vCPU after removing a vCPU, we need to re-use the
vacant s->idx which the corresponding vCPU had been removed. 
so we could use the unique cpu apic_id instead of the progressive s->idx.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/intc/apic_common.c | 4 +---
 target-i386/cpu.c     | 1 +
 2 files changed, 2 insertions(+), 3 deletions(-)
Igor Mammedov - Sept. 10, 2013, 12:09 p.m.
On Tue, 10 Sep 2013 17:43:41 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> the 'apic_no' is increased by one when initialize/create a vCPU each time,
> which causes APICCommonState s->idx always is increased.
> but if we want to re-add a vCPU after removing a vCPU, we need to re-use the
> vacant s->idx which the corresponding vCPU had been removed. 
> so we could use the unique cpu apic_id instead of the progressive s->idx.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/intc/apic_common.c | 4 +---
>  target-i386/cpu.c     | 1 +
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index a0beb10..5568621 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -289,13 +289,11 @@ static int apic_init_common(ICCDevice *dev)
>      APICCommonState *s = APIC_COMMON(dev);
>      APICCommonClass *info;
>      static DeviceState *vapic;
> -    static int apic_no;
>      static bool mmio_registered;
>  
> -    if (apic_no >= MAX_APICS) {
> +    if (s->idx >= MAX_APICS) {
>          return -1;
>      }
> -    s->idx = apic_no++;
>  
>      info = APIC_COMMON_GET_CLASS(s);
>      info->init(s);
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 42c5de0..2b99683 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2322,6 +2322,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>      /* TODO: convert to link<> */
>      apic = APIC_COMMON(env->apic_state);
>      apic->cpu = cpu;
> +    apic->idx = env->cpuid_apic_id;
earlier here we set:
 qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
so apic->idx = env->cpuid_apic_id is redundant.

it would be better to search by apic->id and preferably replace O(MAX_APIC) scans with
a faster approach since for TCG iqr delivery might be a hot path, dropping MAX_APIC
altogether and using dynamic present APICs list.

>  }
>  
>  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
Andreas Färber - Sept. 10, 2013, 12:16 p.m.
Am 10.09.2013 14:09, schrieb Igor Mammedov:
> On Tue, 10 Sep 2013 17:43:41 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> 
>> the 'apic_no' is increased by one when initialize/create a vCPU each time,
>> which causes APICCommonState s->idx always is increased.
>> but if we want to re-add a vCPU after removing a vCPU, we need to re-use the
>> vacant s->idx which the corresponding vCPU had been removed. 
>> so we could use the unique cpu apic_id instead of the progressive s->idx.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>  hw/intc/apic_common.c | 4 +---
>>  target-i386/cpu.c     | 1 +
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>> index a0beb10..5568621 100644
>> --- a/hw/intc/apic_common.c
>> +++ b/hw/intc/apic_common.c
>> @@ -289,13 +289,11 @@ static int apic_init_common(ICCDevice *dev)
>>      APICCommonState *s = APIC_COMMON(dev);
>>      APICCommonClass *info;
>>      static DeviceState *vapic;
>> -    static int apic_no;
>>      static bool mmio_registered;
>>  
>> -    if (apic_no >= MAX_APICS) {
>> +    if (s->idx >= MAX_APICS) {
>>          return -1;
>>      }
>> -    s->idx = apic_no++;
>>  
>>      info = APIC_COMMON_GET_CLASS(s);
>>      info->init(s);
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 42c5de0..2b99683 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -2322,6 +2322,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>>      /* TODO: convert to link<> */
>>      apic = APIC_COMMON(env->apic_state);
>>      apic->cpu = cpu;
>> +    apic->idx = env->cpuid_apic_id;
> earlier here we set:
>  qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
> so apic->idx = env->cpuid_apic_id is redundant.
> 
> it would be better to search by apic->id and preferably replace O(MAX_APIC) scans with
> a faster approach since for TCG iqr delivery might be a hot path, dropping MAX_APIC
> altogether and using dynamic present APICs list.

Independent of that, the recent removal of X86_CPU() cast from
x86_env_get_cpu() should allow us to finally tackle the TODO above,
moving apic_state field from CPUX86State to X86CPU.

Andreas

> 
>>  }
>>  
>>  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>
chenfan - Sept. 11, 2013, 5:37 a.m.
On Tue, 2013-09-10 at 14:16 +0200, Andreas Färber wrote:
> Am 10.09.2013 14:09, schrieb Igor Mammedov:
> > On Tue, 10 Sep 2013 17:43:41 +0800
> > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > 
> >> the 'apic_no' is increased by one when initialize/create a vCPU each time,
> >> which causes APICCommonState s->idx always is increased.
> >> but if we want to re-add a vCPU after removing a vCPU, we need to re-use the
> >> vacant s->idx which the corresponding vCPU had been removed. 
> >> so we could use the unique cpu apic_id instead of the progressive s->idx.
> >>
> >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >> ---
> >>  hw/intc/apic_common.c | 4 +---
> >>  target-i386/cpu.c     | 1 +
> >>  2 files changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> >> index a0beb10..5568621 100644
> >> --- a/hw/intc/apic_common.c
> >> +++ b/hw/intc/apic_common.c
> >> @@ -289,13 +289,11 @@ static int apic_init_common(ICCDevice *dev)
> >>      APICCommonState *s = APIC_COMMON(dev);
> >>      APICCommonClass *info;
> >>      static DeviceState *vapic;
> >> -    static int apic_no;
> >>      static bool mmio_registered;
> >>  
> >> -    if (apic_no >= MAX_APICS) {
> >> +    if (s->idx >= MAX_APICS) {
> >>          return -1;
> >>      }
> >> -    s->idx = apic_no++;
> >>  
> >>      info = APIC_COMMON_GET_CLASS(s);
> >>      info->init(s);
> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> >> index 42c5de0..2b99683 100644
> >> --- a/target-i386/cpu.c
> >> +++ b/target-i386/cpu.c
> >> @@ -2322,6 +2322,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> >>      /* TODO: convert to link<> */
> >>      apic = APIC_COMMON(env->apic_state);
> >>      apic->cpu = cpu;
> >> +    apic->idx = env->cpuid_apic_id;
> > earlier here we set:
> >  qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
> > so apic->idx = env->cpuid_apic_id is redundant.
> > 
> > it would be better to search by apic->id and preferably replace O(MAX_APIC) scans with
> > a faster approach since for TCG iqr delivery might be a hot path, dropping MAX_APIC
> > altogether and using dynamic present APICs list.
> 
> Independent of that, the recent removal of X86_CPU() cast from
> x86_env_get_cpu() should allow us to finally tackle the TODO above,
> moving apic_state field from CPUX86State to X86CPU.
> 
I will send V3 patches later including modification pointed out above. 

Thanks

> Andreas
> 
> > 
> >>  }
> >>  
> >>  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
> > 
> 
>

Patch

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index a0beb10..5568621 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -289,13 +289,11 @@  static int apic_init_common(ICCDevice *dev)
     APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info;
     static DeviceState *vapic;
-    static int apic_no;
     static bool mmio_registered;
 
-    if (apic_no >= MAX_APICS) {
+    if (s->idx >= MAX_APICS) {
         return -1;
     }
-    s->idx = apic_no++;
 
     info = APIC_COMMON_GET_CLASS(s);
     info->init(s);
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 42c5de0..2b99683 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2322,6 +2322,7 @@  static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
     /* TODO: convert to link<> */
     apic = APIC_COMMON(env->apic_state);
     apic->cpu = cpu;
+    apic->idx = env->cpuid_apic_id;
 }
 
 static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)