RE: [PATCH RFC V3 17/29] arm/virt: Release objects for *disabled* possible vCPUs after init

[Salil Mehta]

Hi Gavin,

>  From: Gavin Shan <gshan@redhat.com>
>  Sent: Wednesday, August 21, 2024 7:25 AM
>  To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
>  qemu-arm@nongnu.org; mst@redhat.com
>  
>  Hi Salil,
>  
>  On 8/21/24 2:40 AM, Salil Mehta wrote:
>  >
>  > I don’t understand this clearly. Are  you suggesting to reuse only
>  > single vCPU object to initialize all KVM vCPUs not yet plugged? If
>  > yes, then I'm not sure what do we gain here by adding this complexity?
>  > It does not consume time or resources because we are not realizing any
>  > of these vCPU object in any case.
>  >
>  
>  First of all, it seems we have different names and terms for those cold-
>  booted vCPUs and hotpluggable vCPUs. For example, vCPU-0 and vCPU-1
>  are cold-booted vCPUs while
>  vCPU-2 and vCPU-3 are hotpluggable vCPUs when we have '-smp
>  maxcpus=4,cpus=2'. Lets stick to convention and terms for easier discussion.
>  
>  The idea is avoid instantiating hotpluggable vCPUs in virtmach_init() and
>  released in the same function for those hotpluggable vCPUs. As I can
>  understand, those hotpluggable vCPU instances are serving for two
>  purposes: (1) Relax the constraint that all vCPU's (kvm) file descriptor have
>  to be created and populated; 


We are devising *workarounds* in Qemu for the ARM CPU architectural constraints
in KVM and in Guest Kernel,  *not relaxing* them. We are not allowed to meddle 
with
the constraints. That is the whole point.

Not having to respect those constraints led to rejection of the earlier 
attempts to
upstream Virtual CPU Hotplug for ARM.


(2) Help to instantiate and realize
>  GICv3 object.
>  
>  For (1), I don't think we have to instantiate those hotpluggable vCPUs at 
> all.
>  In the above example where we have command line '-smp
>  maxcpus=4,cpus=2', it's unnecessary to instantiate
>  vCPU-3 and vCPU-4 to create and populate their KVM file descriptors.


We cannot defer create vCPU in KVM after GIC has been initialized in KVM.
It needs to know every vCPU that will ever exists right at the time it is 
getting
Initialized. This is an ARM CPU Architectural constraint. 


 A
>  vCPU's KVM file descriptor is create and populated by the following ioctls
>  and function calls. When the first vCPU (vCPU-0) is realized, the property
>  corresponding to "&init" is fixed for all vCPUs. It means all vCPUs have same
>  properties except the "vcpu_index".
>  
>     ioctl(vm-fd,   KVM_CREATE_VCPU,   vcpu_index);
>     ioctl(vcpu-fd, KVM_ARM_VCPU_INIT, &init);
>     kvm_park_vcpu(cs);
>  
>  A vCPU's properties are determined by two sources and both are global. It
>  means all vCPUs should have same properties: (a) Feature registers
>  returned from the host. The function
>  kvm_arm_get_host_cpu_features() is called for once, meaning this source
>  is same to all vCPUs;


Sure, but what are you trying to save here?


>  (b) The parameters provided by user through '-cpu host,sve=off' are
>  translated to global properties and applied to all vCPUs when they're
>  instantiated.


Sure. Same is the case with PMU and other per-vCPU parameters.
We do not support heterogenous computing and therefore we do not
have per-vCPU control of these features as of now.


>  
>         (a)                                            (b)
>  
>     aarch64_host_initfn                          qemu_init
>     kvm_arm_set_cpu_features_from_host           parse_cpu_option
>       kvm_arm_get_host_cpu_features              cpu_common_parse_features
>                                                  qdev_prop_register_global
>                                                    :
>                                                  device_post_init
>                                                  qdev_prop_set_globals


Sure, I understand the code flow but what are you trying to suggest here?


>  For (2), I'm still looking into the GICv3 code for better understanding. 


Oh, I thought you said you've finished your reviews 😊

Please take your time. For your reference, you might want to check:

KVMForum 2023:
https://kvm-forum.qemu.org/2023/Challenges_Revisited_in_Supporting_Virt_CPU_Hotplug_-__ii0iNb3.pdf
https://kvm-forum.qemu.org/2023/KVM-forum-cpu-hotplug_7OJ1YyJ.pdf

KVMForum 2020:
https://kvm-forum.qemu.org/2020/Oct%2029_Challenges%20in%20Supporting%20Virtual%20CPU%20Hotplug%20in%20SoC%20Based%20Systems%20like%20ARM64_Salil%20Mehta.pdf


Until
>  now, I don't see we need the instantiated hotpluggable vCPUs either.


I think, I've already answered this above it is because of ARM Architectural 
constraint.


 For
>  example, the redistributor regions can be exposed based on 'maxcpus'
>  instead of 'cpus'. 

You mean during the review of the code you found that we are not doing it?


The IRQ connection and teardown can be dynamically
>  done by connecting the board with GICv3 through callbacks in
>  ARMGICv3CommonClass.
>  The connection between GICv3CPUState and CPUARMState also can be
>  done dynamically.

Are you suggesting this after reviewing the code or you have to review it yet? 😉


Thanks
Salil.


>  
>  Thanks,
>  Gavin
>