HI Peter,
> From: Peter Maydell <peter.maydell@xxxxxxxxxx>
> Sent: Thursday, July 25, 2024 11:36 AM
> To: Michael S. Tsirkin <mst@xxxxxxxxxx>
>
> On Tue, 23 Jul 2024 at 11:58, Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
> >
> > From: Salil Mehta <salil.mehta@xxxxxxxxxx>
> >
> > KVM vCPU creation is done once during the vCPU realization when Qemu
> > vCPU thread is spawned. This is common to all the architectures as of now.
> >
> > Hot-unplug of vCPU results in destruction of the vCPU object in QOM
> > but the corresponding KVM vCPU object in the Host KVM is not destroyed
> > as KVM doesn't support vCPU removal. Therefore, its representative KVM
> > vCPU object/context in Qemu is parked.
> >
> > Refactor architecture common logic so that some APIs could be reused
> > by vCPU Hotplug code of some architectures likes ARM, Loongson etc.
> > Update new/old APIs with trace events. New APIs
> > qemu_{create,park,unpark}_vcpu() can be externally called. No functional
> change is intended here.
>
> Hi; Coverity points out an issue with this code (CID 1558552):
>
> > +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id) {
> > + struct KVMParkedVcpu *cpu;
> > + int kvm_fd = -ENOENT;
> > +
> > + QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> > + if (cpu->vcpu_id == vcpu_id) {
> > + QLIST_REMOVE(cpu, node);
> > + kvm_fd = cpu->kvm_fd;
> > + g_free(cpu);
> > + }
> > + }
>
> If you are going to remove an entry from a list as you iterate over it, you
> can't use QLIST_FOREACH(), because QLIST_FOREACH will look at the next
> pointer of the iteration variable at the end of the loop when it wants to
> advance to the next node. In this case we've already freed 'cpu', so it would
> be reading freed memory.
>
> Should we break out of the loop when we find the entry?
Thanks for identifying this. Yes, a break is missing. Should I send a fix for this
now or you can incorporate it?
Best regards
Salil
>
> If we do need to continue iteration after removing the list node, you need
> to use QLIST_FOREACH_SAFE() to do the list iteration.
>
> > -static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id) -{
> > - struct KVMParkedVcpu *cpu;
> > -
> > - QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> > - if (cpu->vcpu_id == vcpu_id) {
> > - int kvm_fd;
> > -
> > - QLIST_REMOVE(cpu, node);
> > - kvm_fd = cpu->kvm_fd;
> > - g_free(cpu);
> > - return kvm_fd;
>
> In this old piece of code we were OK using QLIST_FOREACH because we
> returned immediately we took the node off the list and didn't continue the
> iteration.
Agreed.
>
> > - }
> > - }
> > -
> > - return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> > -}
>
> thanks
> -- PMM
