Re: [Qemu-devel] [PATCH v4 8/8] xics_kvm: Add cpu_destroy method to XICS

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Fri, Jun 05, 2015 at 06:09:38PM +1000, Alexey Kardashevskiy wrote:
> On 06/05/2015 02:25 PM, Bharata B Rao wrote:
> >XICS is setup for each CPU during initialization. Provide a routine
> >to undo the same when CPU is unplugged. Also ensure xics reset doesn't set
> >irq for CPUs that are already unplugged.
> >
> >This allows reboot of a VM that has undergone CPU hotplug and unplug
> >to work correctly.
> >
> >Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >---
> >  hw/intc/xics.c        | 14 ++++++++++++++
> >  hw/intc/xics_kvm.c    | 15 +++++++++++++--
> >  include/hw/ppc/xics.h |  2 ++
> >  3 files changed, 29 insertions(+), 2 deletions(-)
> >
> >diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> >index 924b1ae..3f87f82 100644
> >--- a/hw/intc/xics.c
> >+++ b/hw/intc/xics.c
> >@@ -44,6 +44,20 @@ static int get_cpu_index_by_dt_id(int cpu_dt_id)
> >      return -1;
> >  }
> >
> >+void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu)
> 
> 
> xics_cpu_destroy() is not used by anything, may be push it later with the
> stuff which needs it?

Yeah it is not used in this patchset, will leave to David/agraf
to see if they want this dropped from this series.

> 
> 
> >+{
> >+    CPUState *cs = CPU(cpu);
> >+    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
> >+    ICPState *ss = &icp->ss[cs->cpu_index];
> >+
> >+    assert(cs->cpu_index < icp->nr_servers);
> >+
> >+    ss->output = NULL;
> >+    if (info->cpu_destroy) {
> >+        info->cpu_destroy(icp, cpu);
> >+    }
> >+}
> >+
> >  void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> >  {
> >      CPUState *cs = CPU(cpu);
> >diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> >index d58729c..e35f319 100644
> >--- a/hw/intc/xics_kvm.c
> >+++ b/hw/intc/xics_kvm.c
> >@@ -109,8 +109,10 @@ static void icp_kvm_reset(DeviceState *dev)
> >      icp->pending_priority = 0xff;
> >      icp->mfrr = 0xff;
> >
> >-    /* Make all outputs are deasserted */
> >-    qemu_set_irq(icp->output, 0);
> >+    /* Make all outputs are deasserted only if the CPU thread is in use */
> >+    if (icp->output) {
> >+        qemu_set_irq(icp->output, 0);
> >+    }
> 
> 
> This feels unrelated to what the patch claims that it does. Or
> xics_cpu_destroy() somehow indirectly calls icp_kvm_reset()?

When a VM is rebooted, device_reset of icp device will result in
icp_kvm_reset() which will do qemu_set_irq(). This shouldn't happen for
a CPU that is already removed. So above check ensures that we
don't try to desssert irq for a removed CPU.

Regards,
Bharata.