mbox series

[RFC,0/7] accel: Add per-accelerator vCPUs queue

Message ID 20250106200258.37008-1-philmd@linaro.org
Headers show
Series accel: Add per-accelerator vCPUs queue | expand

Message

Philippe Mathieu-Daudé Jan. 6, 2025, 8:02 p.m. UTC 
Hi,

Currently we register all vCPUs to the global 'cpus_queue' queue,
however we can not discriminate per accelerator or per target
architecture (which might happen in a soon future).

This series tries to add an accelerator discriminator, so
accelerator specific code can iterate on its own vCPUs. This
is required to run a pair of HW + SW accelerators like the
(HVF, TCG) or (KVM, TCG) combinations. Otherwise, i.e. the
HVF core code could iterate on TCG vCPUs...
To keep it simple and not refactor heavily the code base,
we introduce the CPU_FOREACH_TCG/HVF/KVM() macros, only
defined for each accelerator.

This is just a RFC to get some thoughts whether this is
heading in the correct direction or not ;)

Regards,

Phil.

Philippe Mathieu-Daudé (7):
  cpus: Restrict CPU_FOREACH_SAFE() to user emulation
  cpus: Introduce AccelOpsClass::get_cpus_queue()
  accel/tcg: Implement tcg_get_cpus_queue()
  accel/tcg: Use CPU_FOREACH_TCG()
  accel/hw: Implement hw_accel_get_cpus_queue()
  accel/hvf: Use CPU_FOREACH_HVF()
  accel/kvm: Use CPU_FOREACH_KVM()

 accel/tcg/tcg-accel-ops.h         | 10 ++++++++++
 include/hw/core/cpu.h             | 11 +++++++++++
 include/system/accel-ops.h        |  6 ++++++
 include/system/hvf_int.h          |  4 ++++
 include/system/hw_accel.h         |  9 +++++++++
 include/system/kvm_int.h          |  3 +++
 accel/accel-system.c              |  8 ++++++++
 accel/hvf/hvf-accel-ops.c         |  9 +++++----
 accel/kvm/kvm-accel-ops.c         |  1 +
 accel/kvm/kvm-all.c               | 14 +++++++-------
 accel/tcg/cputlb.c                |  7 ++++---
 accel/tcg/monitor.c               |  3 ++-
 accel/tcg/tb-maint.c              |  7 ++++---
 accel/tcg/tcg-accel-ops-rr.c      | 10 +++++-----
 accel/tcg/tcg-accel-ops.c         | 16 ++++++++++++----
 accel/tcg/user-exec-stub.c        |  5 +++++
 accel/xen/xen-all.c               |  1 +
 cpu-common.c                      | 10 ++++++++++
 hw/i386/kvm/clock.c               |  3 ++-
 hw/intc/spapr_xive_kvm.c          |  5 +++--
 hw/intc/xics_kvm.c                |  5 +++--
 system/cpus.c                     |  5 +++++
 target/arm/hvf/hvf.c              |  4 ++--
 target/i386/kvm/kvm.c             |  4 ++--
 target/i386/kvm/xen-emu.c         |  2 +-
 target/i386/nvmm/nvmm-accel-ops.c |  1 +
 target/i386/whpx/whpx-accel-ops.c |  1 +
 target/s390x/kvm/kvm.c            |  2 +-
 target/s390x/kvm/stsi-topology.c  |  3 ++-
 29 files changed, 130 insertions(+), 39 deletions(-)

Comments

Daniel Henrique Barboza Jan. 6, 2025, 8:19 p.m. UTC | #1 
Perhaps add in the commit msg something like "it's only being used in
bsd-user and linux-user code"

On 1/6/25 5:02 PM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   include/hw/core/cpu.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index c3ca0babcb3..48d90f50a71 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -594,8 +594,11 @@ extern CPUTailQ cpus_queue;
>   #define first_cpu        QTAILQ_FIRST_RCU(&cpus_queue)
>   #define CPU_NEXT(cpu)    QTAILQ_NEXT_RCU(cpu, node)
>   #define CPU_FOREACH(cpu) QTAILQ_FOREACH_RCU(cpu, &cpus_queue, node)
> +
> +#if defined(CONFIG_USER_ONLY)
>   #define CPU_FOREACH_SAFE(cpu, next_cpu) \
>       QTAILQ_FOREACH_SAFE_RCU(cpu, &cpus_queue, node, next_cpu)
> +#endif
>   
>   extern __thread CPUState *current_cpu;
>
Daniel Henrique Barboza Jan. 6, 2025, 8:33 p.m. UTC | #2 
On 1/6/25 5:02 PM, Philippe Mathieu-Daudé wrote:
> Only iterate over HVF vCPUs when running HVF specific code.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/system/hvf_int.h  | 4 ++++
>   accel/hvf/hvf-accel-ops.c | 9 +++++----
>   target/arm/hvf/hvf.c      | 4 ++--
>   3 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/include/system/hvf_int.h b/include/system/hvf_int.h
> index 42ae18433f0..3cf64faabd1 100644
> --- a/include/system/hvf_int.h
> +++ b/include/system/hvf_int.h
> @@ -11,6 +11,8 @@
>   #ifndef HVF_INT_H
>   #define HVF_INT_H
>   
> +#include "system/hw_accel.h"
> +
>   #ifdef __aarch64__
>   #include <Hypervisor/Hypervisor.h>
>   typedef hv_vcpu_t hvf_vcpuid;
> @@ -74,4 +76,6 @@ int hvf_put_registers(CPUState *);
>   int hvf_get_registers(CPUState *);
>   void hvf_kick_vcpu_thread(CPUState *cpu);
>   
> +#define CPU_FOREACH_HVF(cpu) CPU_FOREACH_HWACCEL(cpu)


Cosmetic comment: given that this is HVF specific code and we only support one hw
accelerator at a time, I'd skip this alias and use CPU_FOREACH_HWACCEL(cpu) directly.
It would make it easier when grepping to see where and how the macro is being used.
Same thing in the next patch with CPU_FOREACH_KVM().


LGTM otherwise. Thanks,

Daniel


> +
>   #endif
> diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
> index 945ba720513..bbbe2f8d45b 100644
> --- a/accel/hvf/hvf-accel-ops.c
> +++ b/accel/hvf/hvf-accel-ops.c
> @@ -504,7 +504,7 @@ static int hvf_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
>           }
>       }
>   
> -    CPU_FOREACH(cpu) {
> +    CPU_FOREACH_HVF(cpu) {
>           err = hvf_update_guest_debug(cpu);
>           if (err) {
>               return err;
> @@ -543,7 +543,7 @@ static int hvf_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
>           }
>       }
>   
> -    CPU_FOREACH(cpu) {
> +    CPU_FOREACH_HVF(cpu) {
>           err = hvf_update_guest_debug(cpu);
>           if (err) {
>               return err;
> @@ -560,7 +560,7 @@ static void hvf_remove_all_breakpoints(CPUState *cpu)
>       QTAILQ_FOREACH_SAFE(bp, &hvf_state->hvf_sw_breakpoints, entry, next) {
>           if (hvf_arch_remove_sw_breakpoint(cpu, bp) != 0) {
>               /* Try harder to find a CPU that currently sees the breakpoint. */
> -            CPU_FOREACH(tmpcpu)
> +            CPU_FOREACH_HVF(tmpcpu)
>               {
>                   if (hvf_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) {
>                       break;
> @@ -572,7 +572,7 @@ static void hvf_remove_all_breakpoints(CPUState *cpu)
>       }
>       hvf_arch_remove_all_hw_breakpoints();
>   
> -    CPU_FOREACH(cpu) {
> +    CPU_FOREACH_HVF(cpu) {
>           hvf_update_guest_debug(cpu);
>       }
>   }
> @@ -581,6 +581,7 @@ static void hvf_accel_ops_class_init(ObjectClass *oc, void *data)
>   {
>       AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
>   
> +    ops->get_cpus_queue = hw_accel_get_cpus_queue;
>       ops->create_vcpu_thread = hvf_start_vcpu_thread;
>       ops->kick_vcpu_thread = hvf_kick_vcpu_thread;
>   
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 0afd96018e0..13400ff0d5f 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -2269,10 +2269,10 @@ static void hvf_arch_set_traps(void)
>   
>       /* Check whether guest debugging is enabled for at least one vCPU; if it
>        * is, enable exiting the guest on all vCPUs */
> -    CPU_FOREACH(cpu) {
> +    CPU_FOREACH_HVF(cpu) {
>           should_enable_traps |= cpu->accel->guest_debug_enabled;
>       }
> -    CPU_FOREACH(cpu) {
> +    CPU_FOREACH_HVF(cpu) {
>           /* Set whether debug exceptions exit the guest */
>           r = hv_vcpu_set_trap_debug_exceptions(cpu->accel->fd,
>                                                 should_enable_traps);
Philippe Mathieu-Daudé Jan. 6, 2025, 9:08 p.m. UTC | #3 
On 6/1/25 21:33, Daniel Henrique Barboza wrote:
> 
> 
> On 1/6/25 5:02 PM, Philippe Mathieu-Daudé wrote:
>> Only iterate over HVF vCPUs when running HVF specific code.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/system/hvf_int.h  | 4 ++++
>>   accel/hvf/hvf-accel-ops.c | 9 +++++----
>>   target/arm/hvf/hvf.c      | 4 ++--
>>   3 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/system/hvf_int.h b/include/system/hvf_int.h
>> index 42ae18433f0..3cf64faabd1 100644
>> --- a/include/system/hvf_int.h
>> +++ b/include/system/hvf_int.h
>> @@ -11,6 +11,8 @@
>>   #ifndef HVF_INT_H
>>   #define HVF_INT_H
>> +#include "system/hw_accel.h"
>> +
>>   #ifdef __aarch64__
>>   #include <Hypervisor/Hypervisor.h>
>>   typedef hv_vcpu_t hvf_vcpuid;
>> @@ -74,4 +76,6 @@ int hvf_put_registers(CPUState *);
>>   int hvf_get_registers(CPUState *);
>>   void hvf_kick_vcpu_thread(CPUState *cpu);
>> +#define CPU_FOREACH_HVF(cpu) CPU_FOREACH_HWACCEL(cpu)
> 
> 
> Cosmetic comment: given that this is HVF specific code and we only 
> support one hw
> accelerator at a time, I'd skip this alias and use 
> CPU_FOREACH_HWACCEL(cpu) directly.
> It would make it easier when grepping to see where and how the macro is 
> being used.

I find it more useful to grep for a particular accelerator, or for
all of them:

$ git grep CPU_FOREACH_
accel/hvf/hvf-accel-ops.c:507:    CPU_FOREACH_HVF(cpu) {
accel/hvf/hvf-accel-ops.c:546:    CPU_FOREACH_HVF(cpu) {
accel/kvm/kvm-all.c:875:        CPU_FOREACH_KVM(cpu) {
accel/kvm/kvm-all.c:938:    CPU_FOREACH_KVM(cpu) {
accel/tcg/cputlb.c:372:    CPU_FOREACH_TCG(cpu) {
accel/tcg/cputlb.c:650:        CPU_FOREACH_TCG(dst_cpu) {
BALATON Zoltan Jan. 7, 2025, 1:09 p.m. UTC | #4 
On Mon, 6 Jan 2025, Philippe Mathieu-Daudé wrote:
> On 6/1/25 21:33, Daniel Henrique Barboza wrote:
>> 
>> 
>> On 1/6/25 5:02 PM, Philippe Mathieu-Daudé wrote:
>>> Only iterate over HVF vCPUs when running HVF specific code.
>>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   include/system/hvf_int.h  | 4 ++++
>>>   accel/hvf/hvf-accel-ops.c | 9 +++++----
>>>   target/arm/hvf/hvf.c      | 4 ++--
>>>   3 files changed, 11 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/include/system/hvf_int.h b/include/system/hvf_int.h
>>> index 42ae18433f0..3cf64faabd1 100644
>>> --- a/include/system/hvf_int.h
>>> +++ b/include/system/hvf_int.h
>>> @@ -11,6 +11,8 @@
>>>   #ifndef HVF_INT_H
>>>   #define HVF_INT_H
>>> +#include "system/hw_accel.h"
>>> +
>>>   #ifdef __aarch64__
>>>   #include <Hypervisor/Hypervisor.h>
>>>   typedef hv_vcpu_t hvf_vcpuid;
>>> @@ -74,4 +76,6 @@ int hvf_put_registers(CPUState *);
>>>   int hvf_get_registers(CPUState *);
>>>   void hvf_kick_vcpu_thread(CPUState *cpu);
>>> +#define CPU_FOREACH_HVF(cpu) CPU_FOREACH_HWACCEL(cpu)
>> 
>> 
>> Cosmetic comment: given that this is HVF specific code and we only support 
>> one hw
>> accelerator at a time, I'd skip this alias and use CPU_FOREACH_HWACCEL(cpu) 
>> directly.
>> It would make it easier when grepping to see where and how the macro is 
>> being used.
>
> I find it more useful to grep for a particular accelerator, or for
> all of them:
>
> $ git grep CPU_FOREACH_
> accel/hvf/hvf-accel-ops.c:507:    CPU_FOREACH_HVF(cpu) {
> accel/hvf/hvf-accel-ops.c:546:    CPU_FOREACH_HVF(cpu) {
> accel/kvm/kvm-all.c:875:        CPU_FOREACH_KVM(cpu) {
> accel/kvm/kvm-all.c:938:    CPU_FOREACH_KVM(cpu) {
> accel/tcg/cputlb.c:372:    CPU_FOREACH_TCG(cpu) {
> accel/tcg/cputlb.c:650:        CPU_FOREACH_TCG(dst_cpu) {

But then you need to define a new macro for every new accelerator. Maybe 
it's simpler to have CPU_FOREACH take the queue as a parameter so no 
separate macro is needed for each accel (and they cannot get inconsistent 
by changing only one of them in the future).

Regards,
BALATON Zoltan