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 1:27 PM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  
>  On Thu, 25 Jul 2024 at 13:05, Salil Mehta <salil.mehta@huawei.com> wrote:
>  >
>  > 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?
>  
>  The code is already in upstream git, so please send a patch to fix the bug.


Sure, done. Please have a look,

20240725145132.99355-1-salil.mehta@huawei.com/">https://lore.kernel.org/qemu-devel/20240725145132.99355-1-salil.mehta@huawei.com/


Best regards
Salil.

>  
>  thanks
>  -- PMM