Hi Peter,
> From: Peter Maydell <peter.maydell@xxxxxxxxxx>
> Sent: Thursday, July 25, 2024 1:27 PM
> To: Salil Mehta <salil.mehta@xxxxxxxxxx>
>
> On Thu, 25 Jul 2024 at 13:05, Salil Mehta <salil.mehta@xxxxxxxxxx> wrote:
> >
> > 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?
>
> The code is already in upstream git, so please send a patch to fix the bug.
Sure, done. Please have a look,
https://lore.kernel.org/qemu-devel/20240725145132.99355-1-salil.mehta@xxxxxxxxxx/
Best regards
Salil.
>
> thanks
> -- PMM
