RE: [PULL v2 37/61] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Salil Mehta]

HI Peter,

>  From: Peter Maydell <peter.maydell@linaro.org>
>  Sent: Thursday, July 25, 2024 11:36 AM
>  To: Michael S. Tsirkin <mst@redhat.com>
>  
>  On Tue, 23 Jul 2024 at 11:58, Michael S. Tsirkin <mst@redhat.com> wrote:
>  >
>  > From: Salil Mehta <salil.mehta@huawei.com>
>  >
>  > 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