Re: [Qemu-ppc] [PATCH 11/13] spapr: check for the activation of the KVM IRQ device

[David Gibson]

On Tue, Feb 12, 2019 at 08:12:28AM +0100, Cédric Le Goater wrote:
> On 2/12/19 2:01 AM, David Gibson wrote:
> > On Mon, Jan 07, 2019 at 07:39:44PM +0100, Cédric Le Goater wrote:
> >> The activation of the KVM IRQ device depends on the interrupt mode
> >> chosen at CAS time by the machine and some methods used at reset or by
> >> the migration need to be protected.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  hw/intc/spapr_xive_kvm.c | 28 ++++++++++++++++++++++++++++
> >>  hw/intc/xics_kvm.c       | 25 ++++++++++++++++++++++++-
> >>  2 files changed, 52 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> >> index 93ea8e71047a..d35814c1992e 100644
> >> --- a/hw/intc/spapr_xive_kvm.c
> >> +++ b/hw/intc/spapr_xive_kvm.c
> >> @@ -95,9 +95,15 @@ static void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, 
> >> Error **errp)
> >>  
> >>  void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
> >>  {
> >> +    sPAPRXive *xive = SPAPR_MACHINE(qdev_get_machine())->xive;
> >>      uint64_t state[4] = { 0 };
> >>      int ret;
> >>  
> >> +    /* The KVM XIVE device is not in use */
> >> +    if (xive->fd == -1) {
> >> +        return;
> >> +    }
> >> +
> >>      ret = kvm_get_one_reg(tctx->cs, KVM_REG_PPC_NVT_STATE, state);
> >>      if (ret != 0) {
> >>          error_setg_errno(errp, errno,
> >> @@ -151,6 +157,11 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error 
> >> **errp)
> >>      unsigned long vcpu_id;
> >>      int ret;
> >>  
> >> +    /* The KVM XIVE device is not in use */
> >> +    if (xive->fd == -1) {
> >> +        return;
> >> +    }
> >> +
> >>      /* Check if CPU was hot unplugged and replugged. */
> >>      if (kvm_cpu_is_enabled(tctx->cs)) {
> >>          return;
> >> @@ -234,9 +245,13 @@ static void kvmppc_xive_source_get_state(XiveSource 
> >> *xsrc)
> >>  void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val)
> >>  {
> >>      XiveSource *xsrc = opaque;
> >> +    sPAPRXive *xive = SPAPR_XIVE(xsrc->xive);
> >>      struct kvm_irq_level args;
> >>      int rc;
> >>  
> >> +    /* The KVM XIVE device should be in use */
> >> +    assert(xive->fd != -1);
> >> +
> >>      args.irq = srcno;
> >>      if (!xive_source_irq_is_lsi(xsrc, srcno)) {
> >>          if (!val) {
> >> @@ -580,6 +595,11 @@ int kvmppc_xive_pre_save(sPAPRXive *xive)
> >>      Error *local_err = NULL;
> >>      CPUState *cs;
> >>  
> >> +    /* The KVM XIVE device is not in use */
> >> +    if (xive->fd == -1) {
> >> +        return 0;
> >> +    }
> >> +
> >>      /* Grab the EAT */
> >>      kvmppc_xive_get_eas_state(xive, &local_err);
> >>      if (local_err) {
> >> @@ -612,6 +632,9 @@ int kvmppc_xive_post_load(sPAPRXive *xive, int 
> >> version_id)
> >>      Error *local_err = NULL;
> >>      CPUState *cs;
> >>  
> >> +    /* The KVM XIVE device should be in use */
> >> +    assert(xive->fd != -1);
> > 
> > I'm guessing this is an assert() because the handler shouldn't be
> > registered when we're not in KVM mode.  But wouldn't that also be true
> > of the pre_save hook, which errors out rather than asserting?
> 
> The handlers are not symetric.
> 
> The pre_save is registered in the vmstate of the sPAPRXive model and the 
> post_load is handled at the machine level after all XIVE state have been
> transferred.

Ah, ok.  Some comments on site explaining why that's so would be useful.

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