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

[David Gibson]

On Mon, Mar 11, 2019 at 09:41:12PM +0100, Cédric Le Goater wrote:
> On 2/26/19 1:01 AM, David Gibson wrote:
> > On Fri, Feb 22, 2019 at 02:13:13PM +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>
> >> ---
> >>  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    | 89 +++++++++++++++++++++++++++++++++++++
> >>  hw/intc/xive.c              | 10 +++++
> >>  5 files changed, 118 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> >> index 749c6cbc2c56..ebd65e7fe36b 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 061d43fea24d..f3766fd881a2 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -431,5 +431,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 3db24391e31c..9f07567f4d78 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
> >>   */
> >> @@ -153,6 +146,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 6b50451b4f85..4b1ffb9835f9 100644
> >> --- a/hw/intc/spapr_xive_kvm.c
> >> +++ b/hw/intc/spapr_xive_kvm.c
> >> @@ -60,6 +60,57 @@ 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[4] = { 0 };
> >> +    int ret;
> >> +
> >> +    ret = kvm_get_one_reg(tctx->cs, KVM_REG_PPC_NVT_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];
> >> +
> >> +    /*
> >> +     * KVM also returns word2 containing the OS CAM line which is
> >> +     * interesting to print out in the QEMU monitor.
> >> +     */
> >> +    *((uint64_t *) &tctx->regs[TM_QW1_OS + TM_WORD2]) = state[1];
> > 
> > As mentioned elsewhere, it is interesting for debugging, but doesn't
> > seem to really match the guest visible CAM state, 
> 
> The guest is not allowed to see these registers in the TIMA OS page 

Ah.. right.  IIRC, roughly speaking the first doubleword in each ring
belongs to that level, but the second doubleword belongs to the right
above (since that's what manages the scheduling of what's in this
ring).

Makes it a big bogus to carry that as migrated state then.

> and we are not violating the XIVE architecture. That is where the 
> CAM value belong in HW. The exact same place. I was even thinking 
> to propagate the POOL value which could be useful for nested.
> 
> > so I'm not convinced it's a good idea to put it into the regs[] 
> > structure.
> 
> I understand it is problematic in case of a KVM->QEMU migration 
> because we need to force a XiveTCTX reset to update the registers 
> with the QEMU CAM line which has been overridden with the KVM CAM 
> line. 
> 
> Another solution could be to add a 'nvt_base' property to SpaprXive 
> and a KVM control to get its value (xive->vp_base in the KVM XIVE 
> device). It would be migrated and used by the QEMU XIVE device 
> after migration. 
>  
> >> +}
> >> +
> >> +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));
> > 
> > Why does this need a run_on_cpu() ?  The KVM call which is getting the
> > actual info takes a cpu parameter.
> 
> Don't we need to kick the vCPU ?

Um.. you tell me?  If it's not safe to call on a vcpu other than one
owned by the calling thread, I'm not sure if the cpu parameter makes
sense.

-- 
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