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

[Gavin Shan <gshan@redhat.com>]

Hi Sail,

On 8/22/24 8:58 PM, Salil Mehta wrote:
>>   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
> 

Right, my point was actually vCPU objects are too heavy as the place holders. It was
reason why I had the concern: why those hotpluggable vCPU objects can't be avoided
during the bootup time.

>   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.
> 

No, it's not what I was suggesting. What I suggested is to avoid creating those hotpluggable
vCPU objects (place holders) during the bootup time. However, all vCPU file descriptors (KVM
objects) still need to be created and initialized before GICv3 is initialized. It's one of
the constrains. So we need to create and populate all vCPU file descriptors through
ioctl(vm_fd, CREATE_VCPU) and ioctl(vcpu_fd, INIT_VCPU) before GICv3 object is created and
realized. As I explained in the previous reply, the hotpluggable vCPU objects (place holders)
haven't to be created in order to initialize and populate the vCPU file descriptors for those
hotpluggable vCPUs. I think the parameters used to create and initialize vCPU-0's file descriptor
can be reused by all other vCPUs, because we don't support heterogeneous vCPUs.

What I suggested is something like below: the point is to avoid instantiating those hotpluggable
vCPUs, but their vCPU file descriptor (KVM object) are still created and initialized.

     static void machvirt_init(MachineState *machine)
     {

         /*
          * Instantiate and realize vCPU-0, record the parameter passed to
          * ioctl(vcpu-fd, VCPU_INIT, &init), or a better place to remember the parameter.
          * The point is the parameter can be shared by all vCPUs.
          */

         /*
          * Create vCPU descriptors for all other vCPUs (including hotpluggable vCPUs).
          * The remembered parameter is reused and passed to ioctl(vcpu-fd, VCPU_INIT, &init).
          */

         /* Instanaite and realize other cold-booted vCPUs */

         /* Instantiate and realize GICv3 */

     }

> 
> 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.
> 

It seems my suggestion wasn't fully understood. I was suggesting to avoid instantiating
those hotpluggable vCPU objects (place holders) during QEMU startup time. All vCPU file
descriptors (the vCPU's corresponding objects) still need to be in place before GICv3
object is initiated and realized.

> 
>>   >   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.
> 

Thanks for the linker to the slides. Yeah, it's nice to reduce the bootup time
and memory footprint if possible. vCPU hotplug feature may help to improve the
performance, but all other paths might also impact the performance. In summary,
it's a comprehensive goal to reduce the memory footprint and bootup time, and
other components (paths) need optimization to achieve this goal.

> 
>>   >>   (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.
> 

It's what has been implemented in this series. My concern remains: why those
vCPU hotpluggable objects can't be avoided? Again, their corresponding vCPU
file descritpors (KVM vCPU objects) still have to be in place before GICv3
is instantiated and realized.

> 
>>   >>   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 😊
> 

Ok :-)

> 
>>   > 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.
> 

Again, my suggestion wasn't completely understood. I was suggesting to avoid instantiating
those hotpluggable objects, but their vCPU file descriptors still need to be in place
before GICv3's instantiation and realization.

Yes, I was also concerned that too much code changes would be needed if my suggestion is
accepted. It will definitely delay the feature's upstreaming process. It's why I said the
topic (to avoid the hotpluggable objects) are just for discussion now. We can do it (as
separate optimization) after your current implementation is merged.

> 
>>   > 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.
> 

umm, As I explained from the beginning, all KVM vCPU file descriptors are still
in place before GICv3 is instantiated and realized. With those KVM vCPU file
descriptor, we shouldn't have problem except more code changes are needed, or
I miss something? :)

> 
> 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.
> 

Remember you just had the architecture agnostic series merged. The vCPU file
descriptor can be parked and picked up at a later time based on the vCPU index.
This KVM vCPU create in batch mode can be well supported. What you need to is
record the parameters passed to ioctl(vm_fd, CREATE_VCPU, index) and ioctl(vcpu_fd,
INIT_CPU) for vCPU-0 in hw/arm/virt.c and create all other vCPU file descriptors
based on the recorded parameters.

Thanks,
Gavin