[RFC,V2,02/37] cpus-common: Add common CPU utility for possible vCPUs

Commit Message

Salil Mehta Sept. 26, 2023, 10:04 a.m. UTC

Adds various utility functions which might be required to fetch or check the
state of the possible vCPUs. This also introduces concept of *disabled* vCPUs,
which are part of the *possible* vCPUs but are not part of the *present* vCPU.
This state shall be used during machine init time to check the presence of
vcpus.

Co-developed-by: Salil Mehta <[e-mail]>
Signed-off-by: Salil Mehta <[e-mail]>
Co-developed-by: Keqian Zhu <[e-mail]>
Signed-off-by: Keqian Zhu <[e-mail]>
Signed-off-by: Salil Mehta <[e-mail]>
---
 cpus-common.c         | 31 +++++++++++++++++++++++++
 include/hw/core/cpu.h | 53 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

Comments

Gavin Shan Sept. 27, 2023, 3:54 a.m. UTC | #1

Hi Salil,

On 9/26/23 20:04, Salil Mehta wrote:
> Adds various utility functions which might be required to fetch or check the
> state of the possible vCPUs. This also introduces concept of *disabled* vCPUs,
> which are part of the *possible* vCPUs but are not part of the *present* vCPU.
> This state shall be used during machine init time to check the presence of
> vcpus.
   ^^^^^

   vCPUs

> 
> Co-developed-by: Salil Mehta <[e-mail]>
> Signed-off-by: Salil Mehta <[e-mail]>
> Co-developed-by: Keqian Zhu <[e-mail]>
> Signed-off-by: Keqian Zhu <[e-mail]>
> Signed-off-by: Salil Mehta <[e-mail]>
> ---
>   cpus-common.c         | 31 +++++++++++++++++++++++++
>   include/hw/core/cpu.h | 53 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 84 insertions(+)
> 
> diff --git a/cpus-common.c b/cpus-common.c
> index 45c745ecf6..24c04199a1 100644
> --- a/cpus-common.c
> +++ b/cpus-common.c
> @@ -24,6 +24,7 @@
>   #include "sysemu/cpus.h"
>   #include "qemu/lockable.h"
>   #include "trace/trace-root.h"
> +#include "hw/boards.h"
>   
>   QemuMutex qemu_cpu_list_lock;
>   static QemuCond exclusive_cond;
> @@ -107,6 +108,36 @@ void cpu_list_remove(CPUState *cpu)
>       cpu_list_generation_id++;
>   }
>   
> +CPUState *qemu_get_possible_cpu(int index)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    const CPUArchIdList *possible_cpus = ms->possible_cpus;
> +
> +    assert((index >= 0) && (index < possible_cpus->len));
> +
> +    return CPU(possible_cpus->cpus[index].cpu);
> +}
> +
> +bool qemu_present_cpu(CPUState *cpu)
> +{
> +    return cpu;
> +}
> +
> +bool qemu_enabled_cpu(CPUState *cpu)
> +{
> +    return cpu && !cpu->disabled;
> +}
> +

I do think it's a good idea to have wrappers to check for CPU's states since
these CPU states play important role in this series to support vCPU hotplug.
However, it would be nice to move them around into header file (include/hw/boards.h)
because all the checks are originated from ms->possible_cpus->cpus[]. It sounds
functions to a machine (board) instead of global scope. Besides, it would be
nice to have same input (index) for all functions. How about something like
below in include/hw/boards.h?

static inline  bool machine_has_possible_cpu(int index)
{
     MachineState *ms = MACHINE(qdev_get_machine());

     if (!ms || !ms->possible_cpus || index < 0 || index >= ms->possible_cus->len) {
         return false;
     }

     return true;
}

static inline bool machine_has_present_cpu(int index)
{
     MachineState *ms = MACHINE(qdev_get_machine());

     if (!machine_is_possible_cpu(index) ||
         !ms->possible_cpus->cpus[index].cpu) {
         return false;
     }

     return true;
}

static inline bool machine_has_enabled_cpu(int index)
{
     MachineState *ms = MACHINE(qdev_get_machine());
     CPUState *cs;

     if (!machine_is_present_cpu(index)) {
         return false;
     }

     cs = CPU(ms->possible_cpus->cpus[index].cpu);
     return !cs->disabled
}

> +uint64_t qemu_get_cpu_archid(int cpu_index)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    const CPUArchIdList *possible_cpus = ms->possible_cpus;
> +
> +    assert((cpu_index >= 0) && (cpu_index < possible_cpus->len));
> +
> +    return possible_cpus->cpus[cpu_index].arch_id;
> +}
> +

I think it's unnecessary to keep it since it's called for once by
hw/arm/virt-acpi-build.c::build_madt. The architectural ID can be
directly fetched from possible_cpus->cpus[i].arch_id. It's fine
to drop this function and fold the logic to the following patch.

[PATCH RFC V2 21/37] hw/arm: MADT Tbl change to size the guest with possible vCPUs


>   CPUState *qemu_get_cpu(int index)
>   {
>       CPUState *cpu;
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index fdcbe87352..e5af79950c 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -413,6 +413,17 @@ struct CPUState {
>       SavedIOTLB saved_iotlb;
>   #endif
>   
> +    /*
> +     * Some architectures do not allow *presence* of vCPUs to be changed
> +     * after guest has booted using information specified by VMM/firmware
> +     * via ACPI MADT at the boot time. Thus to enable vCPU hotplug on these
> +     * architectures possible vCPU can have CPUState object in 'disabled'
> +     * state or can also not have CPUState object at all. This is possible
> +     * when vCPU Hotplug is supported and vCPUs are 'yet-to-be-plugged' in
> +     * the QOM or have been hot-unplugged.
> +     * By default every CPUState is enabled as of now across all archs.
> +     */
> +    bool disabled;
>       /* TODO Move common fields from CPUArchState here. */
>       int cpu_index;
>       int cluster_index;

I guess the comments can be simplified a bit. How about something like below?

     /*
      * In order to support vCPU hotplug on architectures like aarch64,
      * the vCPU states fall into possible, present or enabled. This field
      * is added to distinguish present and enabled vCPUs. By default, all
      * vCPUs are present and enabled.
      */

> @@ -770,6 +781,48 @@ static inline bool cpu_in_exclusive_context(const CPUState *cpu)
>    */
>   CPUState *qemu_get_cpu(int index);
>   
> +/**
> + * qemu_get_possible_cpu:
> + * @index: The CPUState@cpu_index value of the CPU to obtain.
> + *         Input index MUST be in range [0, Max Possible CPUs)
> + *
> + * If CPUState object exists,then it gets a CPU matching
> + * @index in the possible CPU array.
> + *
> + * Returns: The possible CPU or %NULL if CPU does not exist.
> + */
> +CPUState *qemu_get_possible_cpu(int index);
> +
> +/**
> + * qemu_present_cpu:
> + * @cpu: The vCPU to check
> + *
> + * Checks if the vCPU is amongst the present possible vcpus.
> + *
> + * Returns: True if it is present possible vCPU else false
> + */
> +bool qemu_present_cpu(CPUState *cpu);
> +
> +/**
> + * qemu_enabled_cpu:
> + * @cpu: The vCPU to check
> + *
> + * Checks if the vCPU is enabled.
> + *
> + * Returns: True if it is 'enabled' else false
> + */
> +bool qemu_enabled_cpu(CPUState *cpu);
> +
> +/**
> + * qemu_get_cpu_archid:
> + * @cpu_index: possible vCPU for which arch-id needs to be retreived
> + *
> + * Fetches the vCPU arch-id from the present possible vCPUs.
> + *
> + * Returns: arch-id of the possible vCPU
> + */
> +uint64_t qemu_get_cpu_archid(int cpu_index);
> +

All these descriptive stuff isn't needed after the functions are moved to
include/hw/boards.h, and qemu_get_cpu_archid() is dropped.

>   /**
>    * cpu_exists:
>    * @id: Guest-exposed CPU ID to lookup.

Thanks,
Gavin

Salil Mehta Oct. 2, 2023, 10:21 a.m. UTC | #2

Hi Gavin,

> From: Gavin Shan <[e-mail]>
> Sent: Wednesday, September 27, 2023 4:54 AM
> To: Salil Mehta <[e-mail]>; [e-mail]; qemu-
> [e-mail]
> Cc: [e-mail]; [e-mail]; Jonathan Cameron
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> zhukeqian <[e-mail]>; wangxiongfeng (C)
> <[e-mail]>; wangyanan (Y) <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail]
> Subject: Re: [PATCH RFC V2 02/37] cpus-common: Add common CPU utility for
> possible vCPUs
> 
> Hi Salil,
> 
> On 9/26/23 20:04, Salil Mehta wrote:
> > Adds various utility functions which might be required to fetch or check
> the
> > state of the possible vCPUs. This also introduces concept of *disabled*
> vCPUs,
> > which are part of the *possible* vCPUs but are not part of the *present*
> vCPU.
> > This state shall be used during machine init time to check the presence
> of
> > vcpus.
>    ^^^^^
> 
>    vCPUs


Yes. Thanks.


> > Co-developed-by: Salil Mehta <[e-mail]>
> > Signed-off-by: Salil Mehta <[e-mail]>
> > Co-developed-by: Keqian Zhu <[e-mail]>
> > Signed-off-by: Keqian Zhu <[e-mail]>
> > Signed-off-by: Salil Mehta <[e-mail]>
> > ---
> >   cpus-common.c         | 31 +++++++++++++++++++++++++
> >   include/hw/core/cpu.h | 53 +++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 84 insertions(+)
> >
> > diff --git a/cpus-common.c b/cpus-common.c
> > index 45c745ecf6..24c04199a1 100644
> > --- a/cpus-common.c
> > +++ b/cpus-common.c
> > @@ -24,6 +24,7 @@
> >   #include "sysemu/cpus.h"
> >   #include "qemu/lockable.h"
> >   #include "trace/trace-root.h"
> > +#include "hw/boards.h"
> >
> >   QemuMutex qemu_cpu_list_lock;
> >   static QemuCond exclusive_cond;
> > @@ -107,6 +108,36 @@ void cpu_list_remove(CPUState *cpu)
> >       cpu_list_generation_id++;
> >   }
> >
> > +CPUState *qemu_get_possible_cpu(int index)
> > +{
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> > +    const CPUArchIdList *possible_cpus = ms->possible_cpus;
> > +
> > +    assert((index >= 0) && (index < possible_cpus->len));
> > +
> > +    return CPU(possible_cpus->cpus[index].cpu);
> > +}
> > +
> > +bool qemu_present_cpu(CPUState *cpu)
> > +{
> > +    return cpu;
> > +}
> > +
> > +bool qemu_enabled_cpu(CPUState *cpu)
> > +{
> > +    return cpu && !cpu->disabled;
> > +}
> > +
> 
> I do think it's a good idea to have wrappers to check for CPU's states since
> these CPU states play important role in this series to support vCPU hotplug.
> However, it would be nice to move them around into header file (include/hw/boards.h)
> because all the checks are originated from ms->possible_cpus->cpus[]. It sounds
> functions to a machine (board) instead of global scope. Besides, it would be
> nice to have same input (index) for all functions. How about something like
> below in include/hw/boards.h?

These are operations related to CPUState and hence cpus-common.c seem to be
more appropriate to me. You can see similar functions like qemu_get_cpu()
already exists in the same file.

Yes, some operation do make use of the possible list which is maintained at
board level but eventually what we are returning is the CPUState. 

I am open to move some of above to board level not all like present,
enabled checks should exist in this file only. I would prefer to keep
all of them in this file.


> 
> static inline  bool machine_has_possible_cpu(int index)
> {
>      MachineState *ms = MACHINE(qdev_get_machine());
> 
>      if (!ms || !ms->possible_cpus || index < 0 || index >= ms-
> >possible_cus->len) {
>          return false;
>      }
> 
>      return true;
> }
> 
> static inline bool machine_has_present_cpu(int index)
> {
>      MachineState *ms = MACHINE(qdev_get_machine());
> 
>      if (!machine_is_possible_cpu(index) ||
>          !ms->possible_cpus->cpus[index].cpu) {
>          return false;
>      }
> 
>      return true;
> }
> 
> static inline bool machine_has_enabled_cpu(int index)
> {
>      MachineState *ms = MACHINE(qdev_get_machine());
>      CPUState *cs;
> 
>      if (!machine_is_present_cpu(index)) {
>          return false;
>      }
> 
>      cs = CPU(ms->possible_cpus->cpus[index].cpu);
>      return !cs->disabled
> }
> 
> > +uint64_t qemu_get_cpu_archid(int cpu_index)
> > +{
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> > +    const CPUArchIdList *possible_cpus = ms->possible_cpus;
> > +
> > +    assert((cpu_index >= 0) && (cpu_index < possible_cpus->len));
> > +
> > +    return possible_cpus->cpus[cpu_index].arch_id;
> > +}
> > +
> 
> I think it's unnecessary to keep it since it's called for once by
> hw/arm/virt-acpi-build.c::build_madt. The architectural ID can be
> directly fetched from possible_cpus->cpus[i].arch_id. It's fine
> to drop this function and fold the logic to the following patch.

It is a very useful accessor API. I can see this code is being
replicated everywhere which also means many time its related
variables are repeatedly defined. 

Maybe this is being used once now. But this can be used across
architectures later.

> 
> [PATCH RFC V2 21/37] hw/arm: MADT Tbl change to size the guest with
> possible vCPUs
> 
> 
> >   CPUState *qemu_get_cpu(int index)
> >   {
> >       CPUState *cpu;
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index fdcbe87352..e5af79950c 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -413,6 +413,17 @@ struct CPUState {
> >       SavedIOTLB saved_iotlb;
> >   #endif
> >
> > +    /*
> > +     * Some architectures do not allow *presence* of vCPUs to be changed
> > +     * after guest has booted using information specified by VMM/firmware
> > +     * via ACPI MADT at the boot time. Thus to enable vCPU hotplug on these
> > +     * architectures possible vCPU can have CPUState object in 'disabled'
> > +     * state or can also not have CPUState object at all. This is possible
> > +     * when vCPU Hotplug is supported and vCPUs are 'yet-to-be-plugged' in
> > +     * the QOM or have been hot-unplugged.
> > +     * By default every CPUState is enabled as of now across all archs.
> > +     */
> > +    bool disabled;
> >       /* TODO Move common fields from CPUArchState here. */
> >       int cpu_index;
> >       int cluster_index;
> 
> I guess the comments can be simplified a bit. How about something like
> below?
>      /*
>       * In order to support vCPU hotplug on architectures like aarch64,
>       * the vCPU states fall into possible, present or enabled. This field
>       * is added to distinguish present and enabled vCPUs. By default, all
>       * vCPUs are present and enabled.
>       */

I can definitely try to simplify it but above is not properly conveying the
reason why we require the disabled state.


> 
> > @@ -770,6 +781,48 @@ static inline bool cpu_in_exclusive_context(const
> CPUState *cpu)
> >    */
> >   CPUState *qemu_get_cpu(int index);
> >
> > +/**
> > + * qemu_get_possible_cpu:
> > + * @index: The CPUState@cpu_index value of the CPU to obtain.
> > + *         Input index MUST be in range [0, Max Possible CPUs)
> > + *
> > + * If CPUState object exists,then it gets a CPU matching
> > + * @index in the possible CPU array.
> > + *
> > + * Returns: The possible CPU or %NULL if CPU does not exist.
> > + */
> > +CPUState *qemu_get_possible_cpu(int index);
> > +
> > +/**
> > + * qemu_present_cpu:
> > + * @cpu: The vCPU to check
> > + *
> > + * Checks if the vCPU is amongst the present possible vcpus.
> > + *
> > + * Returns: True if it is present possible vCPU else false
> > + */
> > +bool qemu_present_cpu(CPUState *cpu);
> > +
> > +/**
> > + * qemu_enabled_cpu:
> > + * @cpu: The vCPU to check
> > + *
> > + * Checks if the vCPU is enabled.
> > + *
> > + * Returns: True if it is 'enabled' else false
> > + */
> > +bool qemu_enabled_cpu(CPUState *cpu);
> > +
> > +/**
> > + * qemu_get_cpu_archid:
> > + * @cpu_index: possible vCPU for which arch-id needs to be retreived
> > + *
> > + * Fetches the vCPU arch-id from the present possible vCPUs.
> > + *
> > + * Returns: arch-id of the possible vCPU
> > + */
> > +uint64_t qemu_get_cpu_archid(int cpu_index);
> > +

Gavin Shan Oct. 3, 2023, 5:34 a.m. UTC | #3

Hi Salil,

On 10/2/23 20:21, Salil Mehta wrote:
>> From: Gavin Shan <[e-mail]>
>> Sent: Wednesday, September 27, 2023 4:54 AM
>> To: Salil Mehta <[e-mail]>; [e-mail]; qemu-
>> [e-mail]
>> Cc: [e-mail]; [e-mail]; Jonathan Cameron
>> <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> zhukeqian <[e-mail]>; wangxiongfeng (C)
>> <[e-mail]>; wangyanan (Y) <[e-mail]>;
>> [e-mail]; [e-mail]; [e-mail]
>> Subject: Re: [PATCH RFC V2 02/37] cpus-common: Add common CPU utility for
>> possible vCPUs
>>
>> Hi Salil,
>>
>> On 9/26/23 20:04, Salil Mehta wrote:
>>> Adds various utility functions which might be required to fetch or check
>> the
>>> state of the possible vCPUs. This also introduces concept of *disabled*
>> vCPUs,
>>> which are part of the *possible* vCPUs but are not part of the *present*
>> vCPU.
>>> This state shall be used during machine init time to check the presence
>> of
>>> vcpus.
>>     ^^^^^
>>
>>     vCPUs
> 
> 
> Yes. Thanks.
> 
> 
>>> Co-developed-by: Salil Mehta <[e-mail]>
>>> Signed-off-by: Salil Mehta <[e-mail]>
>>> Co-developed-by: Keqian Zhu <[e-mail]>
>>> Signed-off-by: Keqian Zhu <[e-mail]>
>>> Signed-off-by: Salil Mehta <[e-mail]>
>>> ---
>>>    cpus-common.c         | 31 +++++++++++++++++++++++++
>>>    include/hw/core/cpu.h | 53 +++++++++++++++++++++++++++++++++++++++++++
>>>    2 files changed, 84 insertions(+)
>>>
>>> diff --git a/cpus-common.c b/cpus-common.c
>>> index 45c745ecf6..24c04199a1 100644
>>> --- a/cpus-common.c
>>> +++ b/cpus-common.c
>>> @@ -24,6 +24,7 @@
>>>    #include "sysemu/cpus.h"
>>>    #include "qemu/lockable.h"
>>>    #include "trace/trace-root.h"
>>> +#include "hw/boards.h"
>>>
>>>    QemuMutex qemu_cpu_list_lock;
>>>    static QemuCond exclusive_cond;
>>> @@ -107,6 +108,36 @@ void cpu_list_remove(CPUState *cpu)
>>>        cpu_list_generation_id++;
>>>    }
>>>
>>> +CPUState *qemu_get_possible_cpu(int index)
>>> +{
>>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>> +    const CPUArchIdList *possible_cpus = ms->possible_cpus;
>>> +
>>> +    assert((index >= 0) && (index < possible_cpus->len));
>>> +
>>> +    return CPU(possible_cpus->cpus[index].cpu);
>>> +}
>>> +
>>> +bool qemu_present_cpu(CPUState *cpu)
>>> +{
>>> +    return cpu;
>>> +}
>>> +
>>> +bool qemu_enabled_cpu(CPUState *cpu)
>>> +{
>>> +    return cpu && !cpu->disabled;
>>> +}
>>> +
>>
>> I do think it's a good idea to have wrappers to check for CPU's states since
>> these CPU states play important role in this series to support vCPU hotplug.
>> However, it would be nice to move them around into header file (include/hw/boards.h)
>> because all the checks are originated from ms->possible_cpus->cpus[]. It sounds
>> functions to a machine (board) instead of global scope. Besides, it would be
>> nice to have same input (index) for all functions. How about something like
>> below in include/hw/boards.h?
> 
> These are operations related to CPUState and hence cpus-common.c seem to be
> more appropriate to me. You can see similar functions like qemu_get_cpu()
> already exists in the same file.
> 
> Yes, some operation do make use of the possible list which is maintained at
> board level but eventually what we are returning is the CPUState.
> 
> I am open to move some of above to board level not all like present,
> enabled checks should exist in this file only. I would prefer to keep
> all of them in this file.
> 

There are two lists (arrays): ms->possible_cpus->cpus[] and cpus-common.c::cpus.
The former one is a board's property and the later is a global property. In our
implementation, the vCPU state depends on ms->possible_cpus->cpus[], for example:

- The possible vCPU is determined by checking its index falls in the range of
   [0, ms->possible_cpus->len]
- The present vCPU is determined by checking ms->possible_cpus->cpus[index].cpu

However, other two states have to be determined by checking CPUState

- CPUState::acpi_persistent, for always-present vCPUs
- CPUState::disabled, for enabled vCPU

As suggested in other replies, we may manage the vCPU states from board level
due the fact: the vCPU state changes on Creation, hot-add or hot-remove, which
are all driven by a board. Besides, the hotplug handler is managed by board.
Lastly, scatting the information in different places (ms->possible_cpus->cpus[]
and CPUState), which helps to determine vCPU states, seems not a good idea.

In order to maintain all the information in board level, 'struct CPUArchId'
need some adaption like below. With it, the vCPU states can be determined
from ms->possible_cpus.

#define CPU_ARCH_ID_FLAG_ALWAYS_PRESENT		(1UL << 0)
#define CPU_ARCH_ID_FLAG_ENABLED		(1UL << 1)
typedef struct CPUArchId {
     unsigned long flags
        :
};

> 
>>
>> static inline  bool machine_has_possible_cpu(int index)
>> {
>>       MachineState *ms = MACHINE(qdev_get_machine());
>>
>>       if (!ms || !ms->possible_cpus || index < 0 || index >= ms-
>>> possible_cus->len) {
>>           return false;
>>       }
>>
>>       return true;
>> }
>>
>> static inline bool machine_has_present_cpu(int index)
>> {
>>       MachineState *ms = MACHINE(qdev_get_machine());
>>
>>       if (!machine_is_possible_cpu(index) ||
>>           !ms->possible_cpus->cpus[index].cpu) {
>>           return false;
>>       }
>>
>>       return true;
>> }
>>
>> static inline bool machine_has_enabled_cpu(int index)
>> {
>>       MachineState *ms = MACHINE(qdev_get_machine());
>>       CPUState *cs;
>>
>>       if (!machine_is_present_cpu(index)) {
>>           return false;
>>       }
>>
>>       cs = CPU(ms->possible_cpus->cpus[index].cpu);
>>       return !cs->disabled
>> }
>>
>>> +uint64_t qemu_get_cpu_archid(int cpu_index)
>>> +{
>>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>> +    const CPUArchIdList *possible_cpus = ms->possible_cpus;
>>> +
>>> +    assert((cpu_index >= 0) && (cpu_index < possible_cpus->len));
>>> +
>>> +    return possible_cpus->cpus[cpu_index].arch_id;
>>> +}
>>> +
>>
>> I think it's unnecessary to keep it since it's called for once by
>> hw/arm/virt-acpi-build.c::build_madt. The architectural ID can be
>> directly fetched from possible_cpus->cpus[i].arch_id. It's fine
>> to drop this function and fold the logic to the following patch.
> 
> It is a very useful accessor API. I can see this code is being
> replicated everywhere which also means many time its related
> variables are repeatedly defined.
> 
> Maybe this is being used once now. But this can be used across
> architectures later.
> 

Ok, then please make it inline at least.

>>
>> [PATCH RFC V2 21/37] hw/arm: MADT Tbl change to size the guest with
>> possible vCPUs
>>
>>
>>>    CPUState *qemu_get_cpu(int index)
>>>    {
>>>        CPUState *cpu;
>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>> index fdcbe87352..e5af79950c 100644
>>> --- a/include/hw/core/cpu.h
>>> +++ b/include/hw/core/cpu.h
>>> @@ -413,6 +413,17 @@ struct CPUState {
>>>        SavedIOTLB saved_iotlb;
>>>    #endif
>>>
>>> +    /*
>>> +     * Some architectures do not allow *presence* of vCPUs to be changed
>>> +     * after guest has booted using information specified by VMM/firmware
>>> +     * via ACPI MADT at the boot time. Thus to enable vCPU hotplug on these
>>> +     * architectures possible vCPU can have CPUState object in 'disabled'
>>> +     * state or can also not have CPUState object at all. This is possible
>>> +     * when vCPU Hotplug is supported and vCPUs are 'yet-to-be-plugged' in
>>> +     * the QOM or have been hot-unplugged.
>>> +     * By default every CPUState is enabled as of now across all archs.
>>> +     */
>>> +    bool disabled;
>>>        /* TODO Move common fields from CPUArchState here. */
>>>        int cpu_index;
>>>        int cluster_index;
>>
>> I guess the comments can be simplified a bit. How about something like
>> below?
>>       /*
>>        * In order to support vCPU hotplug on architectures like aarch64,
>>        * the vCPU states fall into possible, present or enabled. This field
>>        * is added to distinguish present and enabled vCPUs. By default, all
>>        * vCPUs are present and enabled.
>>        */
> 
> I can definitely try to simplify it but above is not properly conveying the
> reason why we require the disabled state.
> 

Ok, I think the association between this field and MDAT still need to be
mentioned.
  
>>
>>> @@ -770,6 +781,48 @@ static inline bool cpu_in_exclusive_context(const
>> CPUState *cpu)
>>>     */
>>>    CPUState *qemu_get_cpu(int index);
>>>
>>> +/**
>>> + * qemu_get_possible_cpu:
>>> + * @index: The CPUState@cpu_index value of the CPU to obtain.
>>> + *         Input index MUST be in range [0, Max Possible CPUs)
>>> + *
>>> + * If CPUState object exists,then it gets a CPU matching
>>> + * @index in the possible CPU array.
>>> + *
>>> + * Returns: The possible CPU or %NULL if CPU does not exist.
>>> + */
>>> +CPUState *qemu_get_possible_cpu(int index);
>>> +
>>> +/**
>>> + * qemu_present_cpu:
>>> + * @cpu: The vCPU to check
>>> + *
>>> + * Checks if the vCPU is amongst the present possible vcpus.
>>> + *
>>> + * Returns: True if it is present possible vCPU else false
>>> + */
>>> +bool qemu_present_cpu(CPUState *cpu);
>>> +
>>> +/**
>>> + * qemu_enabled_cpu:
>>> + * @cpu: The vCPU to check
>>> + *
>>> + * Checks if the vCPU is enabled.
>>> + *
>>> + * Returns: True if it is 'enabled' else false
>>> + */
>>> +bool qemu_enabled_cpu(CPUState *cpu);
>>> +
>>> +/**
>>> + * qemu_get_cpu_archid:
>>> + * @cpu_index: possible vCPU for which arch-id needs to be retreived
>>> + *
>>> + * Fetches the vCPU arch-id from the present possible vCPUs.
>>> + *
>>> + * Returns: arch-id of the possible vCPU
>>> + */
>>> +uint64_t qemu_get_cpu_archid(int cpu_index);
>>> +

Thanks,
Gavin

Patch

diff --git a/cpus-common.c b/cpus-common.c
index 45c745ecf6..24c04199a1 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -24,6 +24,7 @@ 
 #include "sysemu/cpus.h"
 #include "qemu/lockable.h"
 #include "trace/trace-root.h"
+#include "hw/boards.h"
 
 QemuMutex qemu_cpu_list_lock;
 static QemuCond exclusive_cond;
@@ -107,6 +108,36 @@  void cpu_list_remove(CPUState *cpu)
     cpu_list_generation_id++;
 }
 
+CPUState *qemu_get_possible_cpu(int index)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    const CPUArchIdList *possible_cpus = ms->possible_cpus;
+
+    assert((index >= 0) && (index < possible_cpus->len));
+
+    return CPU(possible_cpus->cpus[index].cpu);
+}
+
+bool qemu_present_cpu(CPUState *cpu)
+{
+    return cpu;
+}
+
+bool qemu_enabled_cpu(CPUState *cpu)
+{
+    return cpu && !cpu->disabled;
+}
+
+uint64_t qemu_get_cpu_archid(int cpu_index)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    const CPUArchIdList *possible_cpus = ms->possible_cpus;
+
+    assert((cpu_index >= 0) && (cpu_index < possible_cpus->len));
+
+    return possible_cpus->cpus[cpu_index].arch_id;
+}
+
 CPUState *qemu_get_cpu(int index)
 {
     CPUState *cpu;
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index fdcbe87352..e5af79950c 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -413,6 +413,17 @@  struct CPUState {
     SavedIOTLB saved_iotlb;
 #endif
 
+    /*
+     * Some architectures do not allow *presence* of vCPUs to be changed
+     * after guest has booted using information specified by VMM/firmware
+     * via ACPI MADT at the boot time. Thus to enable vCPU hotplug on these
+     * architectures possible vCPU can have CPUState object in 'disabled'
+     * state or can also not have CPUState object at all. This is possible
+     * when vCPU Hotplug is supported and vCPUs are 'yet-to-be-plugged' in
+     * the QOM or have been hot-unplugged.
+     * By default every CPUState is enabled as of now across all archs.
+     */
+    bool disabled;
     /* TODO Move common fields from CPUArchState here. */
     int cpu_index;
     int cluster_index;
@@ -770,6 +781,48 @@  static inline bool cpu_in_exclusive_context(const CPUState *cpu)
  */
 CPUState *qemu_get_cpu(int index);
 
+/**
+ * qemu_get_possible_cpu:
+ * @index: The CPUState@cpu_index value of the CPU to obtain.
+ *         Input index MUST be in range [0, Max Possible CPUs)
+ *
+ * If CPUState object exists,then it gets a CPU matching
+ * @index in the possible CPU array.
+ *
+ * Returns: The possible CPU or %NULL if CPU does not exist.
+ */
+CPUState *qemu_get_possible_cpu(int index);
+
+/**
+ * qemu_present_cpu:
+ * @cpu: The vCPU to check
+ *
+ * Checks if the vCPU is amongst the present possible vcpus.
+ *
+ * Returns: True if it is present possible vCPU else false
+ */
+bool qemu_present_cpu(CPUState *cpu);
+
+/**
+ * qemu_enabled_cpu:
+ * @cpu: The vCPU to check
+ *
+ * Checks if the vCPU is enabled.
+ *
+ * Returns: True if it is 'enabled' else false
+ */
+bool qemu_enabled_cpu(CPUState *cpu);
+
+/**
+ * qemu_get_cpu_archid:
+ * @cpu_index: possible vCPU for which arch-id needs to be retreived
+ *
+ * Fetches the vCPU arch-id from the present possible vCPUs.
+ *
+ * Returns: arch-id of the possible vCPU
+ */
+uint64_t qemu_get_cpu_archid(int cpu_index);
+
 /**
  * cpu_exists:
  * @id: Guest-exposed CPU ID to lookup.