Re: [Qemu-ppc] [PATCH v3 04/15] spapr/xive: add state synchronization with KVM

[David Gibson]

On Fri, Mar 22, 2019 at 08:53:21AM +0100, Cédric Le Goater wrote:
> On 3/22/19 2:00 AM, David Gibson wrote:
> > On Thu, Mar 21, 2019 at 03:49:03PM +0100, Cédric Le Goater wrote:
> >> This extends the KVM XIVE device backend with 'synchronize_state'
> >> methods used to retrieve the state from KVM. The HW state of the
> >> sources, the KVM device and the thread interrupt contexts are
> >> collected for the monitor usage and also migration.
> >>
> >> These get operations rely on their KVM counterpart in the host kernel
> >> which acts as a proxy for OPAL, the host firmware. The set operations
> >> will be added for migration support later.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>
> >>  Changes since v2:
> >>
> >>  - removed the capture of the OS CAM line value from KVM
> >>  - added xive_end_is_valid() check
> >>
> >>  include/hw/ppc/spapr_xive.h |  8 ++++
> >>  include/hw/ppc/xive.h       |  1 +
> >>  hw/intc/spapr_xive.c        | 17 +++++---
> >>  hw/intc/spapr_xive_kvm.c    | 87 +++++++++++++++++++++++++++++++++++++
> >>  hw/intc/xive.c              | 10 +++++
> >>  5 files changed, 116 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> >> index 03685910e76b..7e49badd8c9a 100644
> >> --- a/include/hw/ppc/spapr_xive.h
> >> +++ b/include/hw/ppc/spapr_xive.h
> >> @@ -44,6 +44,13 @@ typedef struct SpaprXive {
> >>      void          *tm_mmap;
> >>  } SpaprXive;
> >>  
> >> +/*
> >> + * The sPAPR machine has a unique XIVE IC device. Assign a fixed value
> >> + * to the controller block id value. It can nevertheless be changed
> >> + * for testing purpose.
> >> + */
> >> +#define SPAPR_XIVE_BLOCK_ID 0x0
> >> +
> >>  bool spapr_xive_irq_claim(SpaprXive *xive, uint32_t lisn, bool lsi);
> >>  bool spapr_xive_irq_free(SpaprXive *xive, uint32_t lisn);
> >>  void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon);
> >> @@ -74,5 +81,6 @@ void kvmppc_xive_set_queue_config(SpaprXive *xive, 
> >> uint8_t end_blk,
> >>  void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
> >>                                   uint32_t end_idx, XiveEND *end,
> >>                                   Error **errp);
> >> +void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp);
> >>  
> >>  #endif /* PPC_SPAPR_XIVE_H */
> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >> index dd115da30ebc..78c919c4a590 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -435,5 +435,6 @@ void kvmppc_xive_source_reset_one(XiveSource *xsrc, 
> >> int srcno, Error **errp);
> >>  void kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp);
> >>  void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val);
> >>  void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp);
> >> +void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp);
> >>  
> >>  #endif /* PPC_XIVE_H */
> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> >> index cde2fd7c8997..4d07140f1078 100644
> >> --- a/hw/intc/spapr_xive.c
> >> +++ b/hw/intc/spapr_xive.c
> >> @@ -40,13 +40,6 @@
> >>  
> >>  #define SPAPR_XIVE_NVT_BASE 0x400
> >>  
> >> -/*
> >> - * The sPAPR machine has a unique XIVE IC device. Assign a fixed value
> >> - * to the controller block id value. It can nevertheless be changed
> >> - * for testing purpose.
> >> - */
> >> -#define SPAPR_XIVE_BLOCK_ID 0x0
> >> -
> >>  /*
> >>   * sPAPR NVT and END indexing helpers
> >>   */
> >> @@ -156,6 +149,16 @@ void spapr_xive_pic_print_info(SpaprXive *xive, 
> >> Monitor *mon)
> >>      XiveSource *xsrc = &xive->source;
> >>      int i;
> >>  
> >> +    if (kvm_irqchip_in_kernel()) {
> >> +        Error *local_err = NULL;
> >> +
> >> +        kvmppc_xive_synchronize_state(xive, &local_err);
> >> +        if (local_err) {
> >> +            error_report_err(local_err);
> >> +            return;
> >> +        }
> >> +    }
> >> +
> >>      monitor_printf(mon, "  LSIN         PQ    EISN     CPU/PRIO EQ\n");
> >>  
> >>      for (i = 0; i < xive->nr_irqs; i++) {
> >> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> >> index 2714f8e4702e..2e46661cb3ad 100644
> >> --- a/hw/intc/spapr_xive_kvm.c
> >> +++ b/hw/intc/spapr_xive_kvm.c
> >> @@ -60,6 +60,51 @@ static void kvm_cpu_enable(CPUState *cs)
> >>  /*
> >>   * XIVE Thread Interrupt Management context (KVM)
> >>   */
> >> +static void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
> >> +{
> >> +    uint64_t state[2] = { 0 };
> >> +    int ret;
> >> +
> >> +    ret = kvm_get_one_reg(tctx->cs, KVM_REG_PPC_VP_STATE, state);
> >> +    if (ret != 0) {
> >> +        error_setg_errno(errp, errno,
> >> +                         "XIVE: could not capture KVM state of CPU %ld",
> >> +                         kvm_arch_vcpu_id(tctx->cs));
> >> +        return;
> >> +    }
> >> +
> >> +    /* word0 and word1 of the OS ring. */
> >> +    *((uint64_t *) &tctx->regs[TM_QW1_OS]) = state[0];
> >> +}
> >> +
> >> +typedef struct {
> >> +    XiveTCTX *tctx;
> >> +    Error *err;
> >> +} XiveCpuGetState;
> >> +
> >> +static void kvmppc_xive_cpu_do_synchronize_state(CPUState *cpu,
> >> +                                                 run_on_cpu_data arg)
> >> +{
> >> +    XiveCpuGetState *s = arg.host_ptr;
> >> +
> >> +    kvmppc_xive_cpu_get_state(s->tctx, &s->err);
> >> +}
> >> +
> >> +void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp)
> >> +{
> >> +    XiveCpuGetState s = {
> >> +        .tctx = tctx,
> >> +        .err = NULL,
> >> +    };
> >> +
> >> +    run_on_cpu(tctx->cs, kvmppc_xive_cpu_do_synchronize_state,
> >> +               RUN_ON_CPU_HOST_PTR(&s));
> > 
> > I think I brought this up earlier, but I'm still confused.
> > 
> > Retreiving information with GET_ONE_REG doesn't usually need dancing
> > around to run on a particular thread.  Why is it necessary here?
> 
> The thread can be scheduled, so we need to kick it to get the thread
> interrupt context registers. This is very much like XICS getting the 
> ICP state.
> 
> May be there is something I am not getting from your question ? I am
> not an expert of the VCPU scheduling area either.

Ah, right I see.  We need to make sure the vcpu is not running so we
don't get stale information.  Ok, I think I'll remember that for the
next review round, should be ok as it is.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature