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

[Salil Mehta]

Hi Gavin,

>  From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org <qemu-
>  arm-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Gavin Shan
>  Sent: Wednesday, August 21, 2024 2:33 PM
>  To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
>  qemu-arm@nongnu.org; mst@redhat.com
>  
>  Hi Salil,
>  
>  On 8/21/24 8:23 PM, Salil Mehta wrote:
>  >>
>  >>   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.
>  >
>  
>  I meant to 'overcome' the constraints by 'relax'. My apologies if there're 
> any
>  caused confusions.


Ok. No issues. It was important for me to clarify though.


 Previously, you had attempt to create all vCPU objects
>  and reuse them when vCPU hot added.

Yes, at QOM level. But that approach did not realize the 
unplugged/yet-to-be-plugged
vCPUs. We were just using QOM vCPU objects as the place holders

 In current implementation, the
>  hotpluggable vCPUs are instantiated and released pretty soon. I was
>  bringing the third possibility, to avoid instantiating those hotpluggable 
> vCPU
>  objects, for discussion. 


Are you suggesting not calling KVM_ARM_VCPU_INIT IOCTL as all for the vCPUs
which are part of possible list but not yet plugged?

If yes, we cannot do that as KVM vCPUs should be fully initialized even before
VGIC is initialized inside the KVM. This is a constraint. I've explained this in
detail in the cover letter of this patch-set and in the slides I have shared
earlier.


In this series, the life cycle of those hotpluggable
>  vCPU objects are really short. Again, I didn't say we must avoid 
> instantiating
>  those vCPU objects, I brought the topic ONLY for discussion.

Sure, I appreciate that. For the details of the reasons please follow below:

1. Cover Letter of this patch-set (Constraints are explained there)
2. KVMForum Slides of 2020 and 2023


>  > (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.
>  >
>  
>  It will be appreciated if more details other than 'an ARM CPU architectural 
> constraint'
>  can be provided. I don't understand this constrain very well at least.


We cannot do that as we MUST present KVM vCPUs to the VGIC fully initialized,
even before it starts its initialization. Initialization of the vCPUs also 
initializes
the system registers for the corresponding KVM vCPU.  

For example, MPIDR_EL1 must be initialized at VCPU INIT time. We cannot
avoid this. MPIDR value is used by VGIC during its initialization. This MUST be
present for all of the possible KVM vCPUs right from start during vgic_init()

Please check the cover letter of this patch-set, I explained these there and the
KVMForum slides.  Please review and comment there and let me know what is
not clear from the text.


>  >   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?
>  >
>  
>  As mentioned above, the life cycle of those hotpluggable vCPU objects are
>  really short. They still consume time and memory to instantiate them. If I'm
>  correct, one of the primary goal for vCPU hotplug feature is to save system
>  boot-up time, correct?


Correct. We targeted vCPU hotplug for Kata-containers for on-demand resource
allocation and saving the resources. Kata-containers can work with different 
types
of VMM like Qemu and microVMs like Firecracker. AFAIK, Usecase of microVM is
slightly different than the normal containers. They are short lived (say around
15 min) and require ultrafast boot-up times (say less than 125 ms) - these 
figures
are from Amazon who invented the concept of microVM in the earlier decade.

With the current patches, we have only partially achieved what we had started
i.e. Kata/Qemu but we also want to target Kata/microVM. In our case, we want
that microVM to be Qemu based fro ARM. I think x86 already has reduced lots
of legacy stuff and created a microVM in Qemu. I'm not sure how it compares
against the true microVM like Firecracker. It will be a good target to reduce
memory foot print of ARM Qemu Virt Machine. or think or creating a new one
just like x86. Using the vCPU Hotplug feature we were drastically able to reduce
the boot up times of Qemu. Please check the calibrated performance figures in
KVmForum  2023 slide 19G (Page 26) [1]

[1]  
https://kvm-forum.qemu.org/2023/Challenges_Revisited_in_Supporting_Virt_CPU_Hotplug_-__ii0iNb3.pdf

Last year, I had prototyped a microVM for ARM, Michael Tsirkin suggested that if
the performance number of the ARM Virt machine can match the x86 microVM then
we might not require an explicit microVM code for ARM. Hence, my current efforts
are to reduce the memory foot print of existing Virt machine. But I do have a 
rough
prototype of microVM as well. We can debate about that later in a different 
discussion.


>  >>   (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?
>  >
>  
>  I tried to explain that vCPU object isn't needed to create and populate
>  vCPU's file descriptors, as highlight in (1). The information used to create 
> the
>  cold-booted
>  vCPU-0 can be reused because all vCPUs have same properties and feature
>  set.


It does not matter. We use those QOM vCPU object states to initializes Qemu 
GICv3 model with max possible vCPUs and then release the QOM vCPU objects.
which are yet-to-be-plugged.


>  >>   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_Vir
>  > t_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%2
>  0ARM64_
>  > Salil%20Mehta.pdf
>  >
>  
>  hmm, 'finished my review' has been misread frankly. By that, I meant I
>  finished my tests and provided all the comments I had. Some of them are
>  questions and discussions, which I still need to follow up.


Sure. No worries. Even if you miss, you will have more chance to comment on
upcoming RFC V4 😊


>  > 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?
>  >
>  
>  It's all about the discussion to the possibility to avoid instantiating
>  hotpluggable vCPU objects.


As mentioned above, with the current KVM code you cannot. But if we
really want to then perhaps we would need to change KVM.

I might be wrong but AFAICS I don’t see a reason why we cannot have
something like *batch* KVM vCPU create and  initialize instead of current
sequential KVM operations. This will avoid multiple calls to the KVM Host
and can improve Qemu init time further. But this will require a separate
discussion in the LKML including all the KVM folks.

This has potential to delay the vCPU hotplug feature acceptance and I'm
really not in favor of that. We have already stretched it a lot because of
the standards change acceptance earlier.


>  > 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? 😉
>  >
>  
>  I was actually trying to ask for your input and feedback. I was hoping your
>  input to clear my puzzles: why vCPU objects must be in place to create
>  GICv3 object?
>  Is it possible to create the GICv3 object without those vCPU objects? 


No. VGIC initializes IRQs to target KVM vCPUs, it would expect same KVM vCPU 
MPIDR
or MP-AFFINITY configured when KVM vCPUs were initialized at the first place
otherwise the VGIC initialization will not happen correctly. Hence, the 
sequence.

The sequence of these initialization is generally strictly controlled by 
specification
which is closely tied up with hardware including powering up initializations. 
You will need to honor the expectations of the KVM VGIC init which in turn are
ARM CPU Architecture specification compliant. It is not just a loosely written 
code.


What
>  kinds of efforts we need to avoid instantiating those hotpluggable vCPU
>  objects.


I mentioned one of the ways above. Introduce *batch* KVM vCPU create &
initialize. But it will have to undergo greater scrutiny because we are touching
a common part which might affect many stake holders.  But this is a discussion
we can do later as part of microVM for ARM. 


Thanks
Salil.

>  The best way perhaps is to find the answer from the code by myself ;-)
>  
>  Thanks,
>  Gavin
>