Hi Salil,
On 8/23/24 11:17 PM, Salil Mehta wrote:
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.
Sure, to list down again. For the reasons I've already explained :
1. KVM MUST have details about all the vCPUs and its features finalized before
the VGIC
initialization. This is ARM CPU Architecture constraint. This is immutable
requirement!
2. QOM vCPUs has got representational changes of the KVM vCPU like vcpu-id,
features
list etc. These MUST be finalized even before QOM begins its own GICv3
initialization
which will end up in initialization of KVM VGIC. QOM vCPUs MUST be handed
over to
the GICV3 QOM in fully initialized state. The same reason applies here as
well.
Till this point we are architecture compliant. The only place where we are
not ARM
Architecture compliant is where in QOM, we dissociate the vCPU state with
GICV3
CPU State during unplug action or for the yet-to-be-plugged vCPUs. Later
are
released as part of virt_cpu_post_init() in our current design.
Thanks for your time to evaluate and reply. Sorry that I'm not convinced.
You're explaining
what we already have in current design and implementation. It doesn't mean the
current design
and implementation is 100% perfect and flawless.
In current design and implementation, all (QOM) vCPU objects have to be
instantiated, even
though those hotpluggable (QOM) vCPU objects aren't realized at bootup time.
After that, those
(QOM) hotpluggable vCPU objects are finalized and destroyed so that they can be
hot added
afterwards. This sounds like we create (QOM) vCPU objects, remove those (QOM)
vCPU objects
at booting time so that they can be hot-added at running time. The duplicate
work is obvious
there. This scheme and design looks just-for-working, not in an optimized way
to me.
I'm giving up the efforts to convince you. Lets see if other reviewers will
have same concern
or not. Another possibility would be to have current implementation (with all
fixes) merged
upstream firstly, and then seek optimizations after that. That time, the
suggestion can be
re-evaluated.
> 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 */
}
No. For the reasons I've mentioned above, we MUST provide fully initialize the
QOM
vCPU objects before initialization of QOM GICV3 kicks in. This ensures that
nothing
breaks during initialization process of the QOM GICV3. Therefore, the
optimization
steps mentioned above are unnecessary and could cause problems in future.
Additionally, the evolution of the GICV3 QOM can be independent of the ARM Virt
Machine as it can be used with other Machines as well so we MUST treat it as a
black
box which needs QOM vCPU objects as inputs during its initialization.
Your explanation is not completely correct to me. It's what we had in current
design. It
doesn't means the design should have to be like this. The (KVM) vCPU file
descriptor must
be in place before QOM GICv3 object is instantiated and realized, but the (QOM)
vCPU objects
don't have to exist before that. However, we may need a lot of code changes to
tame GICv3Common
and GICv3KVM so that they can be vCPU hot-add/remove friendly and then to avoid
the pre-created
(QOM) vCPU objects.
It can be something to be re-evaluated in the future, as I said above. Frankly,
It's pointless
to take our time on the discussions without reaching to any conclusions. From
my side, I've tried
my best to provide my comments and explain my thoughts. Appreciated for your
patience, time on
evaluation, and replies :)
Thanks,
Gavin
