RE: [PATCH v2 1/4] accel/kvm: Extract common KVM vCPU {creation, parking} code

[Salil Mehta]

>  From: Harsh Prateek Bora <harshpb@linux.ibm.com>
>  Sent: Thursday, May 16, 2024 2:07 PM
>  
>  Hi Salil,
>  
>  On 5/16/24 17:42, Salil Mehta wrote:
>  > Hi Harsh,
>  >
>  >>   From: Harsh Prateek Bora <harshpb@linux.ibm.com>
>  >>   Sent: Thursday, May 16, 2024 11:15 AM
>  >>
>  >>   Hi Salil,
>  >>
>  >>   Thanks for your email.
>  >>   Your patch 1/8 is included here based on review comments on my  previous
>  >>   patch from one of the maintainers in the community and therefore I  had
>  >>   kept you in CC to be aware of the desire of having this independent 
> patch to
>  >>   get merged earlier even if your other patches in the series may go 
> through
>  >>   further reviews.
>  >
>  > I really don’t know which discussion are  you pointing at? Please
>  > understand you are fixing a bug and we are pushing a feature which has got 
> large series.
>  > It will break the patch-set  which is about t be merged.
>  >
>  > There will be significant overhead of testing on us for the work we
>  > have been carrying forward for large time. This will be disruptive. Please 
> dont!
>  >
>  
>  I was referring to the review discussion on my prev patch here:
>  D191D2JFAR7L.2EH4S445M4TGK@gmail.com/">https://lore.kernel.org/qemu-devel/D191D2JFAR7L.2EH4S445M4TGK@gmail.com/


Sure, I'm, not sure what this means. 


>  Although your patch was included with this series only to facilitate review 
> of
>  the additional patches depending on just one of your patch.


Generally you rebase your patch-set over the other and clearly state on the 
cover
letter that this patch-set is dependent upon such and such patch-set. Just 
imagine
if everyone starts to unilaterally pick up patches from each other's patch-set 
it will
create a chaos not only for the feature owners but also for the maintainers.


>  
>  I am not sure what is appearing disruptive here. It is a common practive in
>  the community that maintainer(s) can pick individual patches from the
>  series if it has been vetted by siginificant number of reviewers.


Don’t you think this patch-set is asking for acceptance for a patch already 
part of another patch-set which is about to be accepted and is a bigger feature?
Will it cause maintenance overhead at the last moment? Yes, of course!


>  However, in this case, since you have mentioned to post next version soon,
>  you need not worry about it as that would be the preferred version for both
>  of the series.


Yes, but please understand we are working for the benefit of overall community.
Please cooperate here.

>  
>  >
>  >>
>  >>   I am hoping to see your v9 soon and thereafter maintainer(s) may
>  choose to
>  >>   pick the latest independent patch if needs to be merged earlier.
>  >
>  >
>  > I don’t think you are understanding what problem it is causing. For
>  > your small bug fix you are causing significant delays at our end.
>  >
>  
>  I hope I clarfied above that including your patch here doesnt delay anything.
>  Hoping to see your v9 soon!
>  
>  Thanks
>  Harsh
>  >
>  > Thanks
>  > Salil.
>  >>
>  >>   Thanks for your work and let's be hopeful it gets merged soon.
>  >>
>  >>   regards,
>  >>   Harsh
>  >>
>  >>   On 5/16/24 14:00, Salil Mehta wrote:
>  >>   > Hi Harsh,
>  >>   >
>  >>   > Thanks for your interest in the patch-set but taking away patches like
>  >>   > this from other series without any discussion can disrupt others work
>  >>   > and its acceptance on time. This is because we will have to put lot of
>  >>   > effort in rebasing bigger series and then testing overhead comes
>  along
>  >>   > with it.
>  >>   >
>  >>   > The patch-set (from where this  patch has been taken) is part of even
>  >>   > bigger series and there have been many people and companies toiling
>  to
>  >>   > fix the bugs collectively in that series and for years.
>  >>   >
>  >>   > I'm about float the V9 version of the Arch agnostic series which this
>  >>   > patch is part of and you can rebase your patch-set from there. I'm
>  >>   > hopeful that it will get accepted in this cycle.
>  >>   >
>  >>   >
>  >>   > Many thanks
>  >>   > Salil.
>  >>   >
>  >>   >>   From: Harsh Prateek Bora <harshpb@linux.ibm.com>
>  >>   >>   Sent: Thursday, May 16, 2024 6:32 AM
>  >>   >>
>  >>   >>   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 instead of DPRINTF. No functional
>  >>   change is
>  >>   >>   intended here.
>  >>   >>
>  >>   >>   Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  >>   >>   Reviewed-by: Gavin Shan <gshan@redhat.com>
>  >>   >>   Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>  >>   >>   Reviewed-by: Jonathan Cameron
>  <Jonathan.Cameron@huawei.com>
>  >>   >>   Tested-by: Xianglai Li <lixianglai@loongson.cn>
>  >>   >>   Tested-by: Miguel Luis <miguel.luis@oracle.com>
>  >>   >>   Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
>  >>   >>   [harshpb: fixed rebase failures in include/sysemu/kvm.h]
>  >>   >>   Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>  >>   >>   ---
>  >>   >>    include/sysemu/kvm.h   | 15 ++++++++++
>  >>   >>    accel/kvm/kvm-all.c    | 64
>  ++++++++++++++++++++++++++++++++---
>  >>   -----
>  >>   >>   --
>  >>   >>    accel/kvm/trace-events |  5 +++-
>  >>   >>    3 files changed, 68 insertions(+), 16 deletions(-)
>  >>   >>
>  >>   >>   diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index
>  >>   >>   eaf801bc93..fa3ec74442 100644
>  >>   >>   --- a/include/sysemu/kvm.h
>  >>   >>   +++ b/include/sysemu/kvm.h
>  >>   >>   @@ -434,6 +434,21 @@ void kvm_set_sigmask_len(KVMState *s,
>  >>   unsigned
>  >>   >>   int sigmask_len);
>  >>   >>
>  >>   >>    int kvm_physical_memory_addr_from_host(KVMState *s, void
>  >>   >>   *ram_addr,
>  >>   >>                                           hwaddr *phys_addr);
>  >>   >>   +/**
>  >>   >>   + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM
>  >>   vCPU
>  >>   >>   + * @cpu: QOM CPUState object for which KVM vCPU has to be
>  >>   >>   fetched/created.
>  >>   >>   + *
>  >>   >>   + * @returns: 0 when success, errno (<0) when failed.
>  >>   >>   + */
>  >>   >>   +int kvm_create_vcpu(CPUState *cpu);
>  >>   >>   +
>  >>   >>   +/**
>  >>   >>   + * kvm_park_vcpu - Park QEMU KVM vCPU context
>  >>   >>   + * @cpu: QOM CPUState object for which QEMU KVM vCPU
>  context
>  >>   has to
>  >>   >>   be parked.
>  >>   >>   + *
>  >>   >>   + * @returns: none
>  >>   >>   + */
>  >>   >>   +void kvm_park_vcpu(CPUState *cpu);
>  >>   >>
>  >>   >>    #endif /* COMPILING_PER_TARGET */
>  >>   >>
>  >>   >>   diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
>  >>   >>   d7281b93f3..30d42847de 100644
>  >>   >>   --- a/accel/kvm/kvm-all.c
>  >>   >>   +++ b/accel/kvm/kvm-all.c
>  >>   >>   @@ -128,6 +128,7 @@ static QemuMutex kml_slots_lock;  #define
>  >>   >>   kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>  >>   >>
>  >>   >>    static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>  >>   >>   +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>  >>   >>
>  >>   >>    static inline void kvm_resample_fd_remove(int gsi)  { @@ -340,14
>  >>   +341,53
>  >>   >>   @@ err:
>  >>   >>        return ret;
>  >>   >>    }
>  >>   >>
>  >>   >>   +void kvm_park_vcpu(CPUState *cpu)
>  >>   >>   +{
>  >>   >>   +    struct KVMParkedVcpu *vcpu;
>  >>   >>   +
>  >>   >>   +    trace_kvm_park_vcpu(cpu->cpu_index,
>  kvm_arch_vcpu_id(cpu));
>  >>   >>   +
>  >>   >>   +    vcpu = g_malloc0(sizeof(*vcpu));
>  >>   >>   +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>  >>   >>   +    vcpu->kvm_fd = cpu->kvm_fd;
>  >>   >>   +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu,
>  >>   node); }
>  >>   >>   +
>  >>   >>   +int kvm_create_vcpu(CPUState *cpu)
>  >>   >>   +{
>  >>   >>   +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>  >>   >>   +    KVMState *s = kvm_state;
>  >>   >>   +    int kvm_fd;
>  >>   >>   +
>  >>   >>   +    trace_kvm_create_vcpu(cpu->cpu_index,
>  kvm_arch_vcpu_id(cpu));
>  >>   >>   +
>  >>   >>   +    /* check if the KVM vCPU already exist but is parked */
>  >>   >>   +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
>  >>   >>   +    if (kvm_fd < 0) {
>  >>   >>   +        /* vCPU not parked: create a new KVM vCPU */
>  >>   >>   +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>  >>   >>   +        if (kvm_fd < 0) {
>  >>   >>   +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU
>  %lu",
>  >>   >>   vcpu_id);
>  >>   >>   +            return kvm_fd;
>  >>   >>   +        }
>  >>   >>   +    }
>  >>   >>   +
>  >>   >>   +    cpu->kvm_fd = kvm_fd;
>  >>   >>   +    cpu->kvm_state = s;
>  >>   >>   +    cpu->vcpu_dirty = true;
>  >>   >>   +    cpu->dirty_pages = 0;
>  >>   >>   +    cpu->throttle_us_per_full = 0;
>  >>   >>   +
>  >>   >>   +    return 0;
>  >>   >>   +}
>  >>   >>   +
>  >>   >>    static int do_kvm_destroy_vcpu(CPUState *cpu)  {
>  >>   >>        KVMState *s = kvm_state;
>  >>   >>        long mmap_size;
>  >>   >>   -    struct KVMParkedVcpu *vcpu = NULL;
>  >>   >>        int ret = 0;
>  >>   >>
>  >>   >>   -    trace_kvm_destroy_vcpu();
>  >>   >>   +    trace_kvm_destroy_vcpu(cpu->cpu_index,
>  >>   kvm_arch_vcpu_id(cpu));
>  >>   >>
>  >>   >>        ret = kvm_arch_destroy_vcpu(cpu);
>  >>   >>        if (ret < 0) {
>  >>   >>   @@ -373,10 +413,7 @@ static int do_kvm_destroy_vcpu(CPUState
>  >>   *cpu)
>  >>   >>            }
>  >>   >>        }
>  >>   >>
>  >>   >>   -    vcpu = g_malloc0(sizeof(*vcpu));
>  >>   >>   -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>  >>   >>   -    vcpu->kvm_fd = cpu->kvm_fd;
>  >>   >>   -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu,
>  >>   node);
>  >>   >>   +    kvm_park_vcpu(cpu);
>  >>   >>    err:
>  >>   >>        return ret;
>  >>   >>    }
>  >>   >>   @@ -397,6 +434,8 @@ static int kvm_get_vcpu(KVMState *s,
>  unsigned
>  >>   long
>  >>   >>   vcpu_id)
>  >>   >>            if (cpu->vcpu_id == vcpu_id) {
>  >>   >>                int kvm_fd;
>  >>   >>
>  >>   >>   +            trace_kvm_get_vcpu(vcpu_id);
>  >>   >>   +
>  >>   >>                QLIST_REMOVE(cpu, node);
>  >>   >>                kvm_fd = cpu->kvm_fd;
>  >>   >>                g_free(cpu);
>  >>   >>   @@ -404,7 +443,7 @@ static int kvm_get_vcpu(KVMState *s,
>  unsigned
>  >>   long
>  >>   >>   vcpu_id)
>  >>   >>            }
>  >>   >>        }
>  >>   >>
>  >>   >>   -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>  >>   >>   +    return -ENOENT;
>  >>   >>    }
>  >>   >>
>  >>   >>    int kvm_init_vcpu(CPUState *cpu, Error **errp) @@ -415,19
>  +454,14
>  >>   @@
>  >>   >>   int kvm_init_vcpu(CPUState *cpu, Error **errp)
>  >>   >>
>  >>   >>        trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  >>   >>
>  >>   >>   -    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>  >>   >>   +    ret = kvm_create_vcpu(cpu);
>  >>   >>        if (ret < 0) {
>  >>   >>   -        error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu
>  failed
>  >>   >>   (%lu)",
>  >>   >>   +        error_setg_errno(errp, -ret,
>  >>   >>   +                         "kvm_init_vcpu: kvm_create_vcpu failed 
> (%lu)",
>  >>   >>                             kvm_arch_vcpu_id(cpu));
>  >>   >>            goto err;
>  >>   >>        }
>  >>   >>
>  >>   >>   -    cpu->kvm_fd = ret;
>  >>   >>   -    cpu->kvm_state = s;
>  >>   >>   -    cpu->vcpu_dirty = true;
>  >>   >>   -    cpu->dirty_pages = 0;
>  >>   >>   -    cpu->throttle_us_per_full = 0;
>  >>   >>   -
>  >>   >>        mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>  >>   >>        if (mmap_size < 0) {
>  >>   >>            ret = mmap_size;
>  >>   >>   diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events index
>  >>   >>   681ccb667d..75c1724e78 100644
>  >>   >>   --- a/accel/kvm/trace-events
>  >>   >>   +++ b/accel/kvm/trace-events
>  >>   >>   @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg)
>  "dev fd
>  >>   %d,
>  >>   >>   type 0x%x, arg %p"
>  >>   >>    kvm_failed_reg_get(uint64_t id, const char *msg) "Warning:
>  Unable to
>  >>   >>   retrieve ONEREG %" PRIu64 " from KVM: %s"
>  >>   >>    kvm_failed_reg_set(uint64_t id, const char *msg) "Warning:
>  Unable to
>  >>   set
>  >>   >>   ONEREG %" PRIu64 " to KVM: %s"
>  >>   >>    kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index:
>  %d
>  >>   id:
>  >>   >>   %lu"
>  >>   >>   +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id)
>  "index:
>  >>   %d
>  >>   >>   id: %lu"
>  >>   >>   +kvm_get_vcpu(unsigned long arch_cpu_id) "id: %lu"
>  >>   >>   +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id)
>  "index:
>  >>   %d
>  >>   >>   id: %lu"
>  >>   >>   +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id)
>  "index: %d
>  >>   id:
>  >>   >>   %lu"
>  >>   >>    kvm_irqchip_commit_routes(void) ""
>  >>   >>    kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev
>  %s
>  >>   >>   vector %d virq %d"
>  >>   >>    kvm_irqchip_update_msi_route(int virq) "Updating MSI route
>  >>   virq=%d"
>  >>   >>   @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
>  >>   >>    kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64"
>  >>   pages
>  >>   >>   (took %"PRIi64" us)"
>  >>   >>    kvm_dirty_ring_reaper_kick(const char *reason) "%s"
>  >>   >>    kvm_dirty_ring_flush(int finished) "%d"
>  >>   >>   -kvm_destroy_vcpu(void) ""
>  >>   >>    kvm_failed_get_vcpu_mmap_size(void) ""
>  >>   >>    kvm_cpu_exec(void) ""
>  >>   >>    kvm_interrupt_exit_request(void) ""
>  >>   >>   --
>  >>   >>   2.39.3
>  >>   >