[11/13] spapr: check for the activation of the KVM IRQ device

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 <clg@kaod.org>
---
 hw/intc/spapr_xive_kvm.c | 28 ++++++++++++++++++++++++++++
 hw/intc/xics_kvm.c       | 25 ++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

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 <clg@kaod.org>
> ---
>  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?

>      /* Restore the ENDT first. The targetting depends on it. */
>      CPU_FOREACH(cs) {
>          kvmppc_xive_set_eq_state(xive, cs, &local_err);
> @@ -649,6 +672,11 @@ void kvmppc_xive_synchronize_state(sPAPRXive *xive, Error **errp)
>      CPUState *cs;
>      Error *local_err = NULL;
>  
> +    /* The KVM XIVE device is not in use */
> +    if (xive->fd == -1) {
> +        return;
> +    }
> +
>      /*
>       * When the VM is stopped, the sources are masked and the previous
>       * state is saved in anticipation of a migration. We should not
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index da6a00bc88cc..651bbfdf6966 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -68,6 +68,11 @@ static void icp_get_kvm_state(ICPState *icp)
>      uint64_t state;
>      int ret;
>  
> +    /* The KVM XICS device is not in use */
> +    if (kernel_xics_fd == -1) {
> +        return;
> +    }
> +
>      /* ICP for this CPU thread is not in use, exiting */
>      if (!icp->cs) {
>          return;
> @@ -104,6 +109,11 @@ static int icp_set_kvm_state(ICPState *icp, int version_id)
>      uint64_t state;
>      int ret;
>  
> +    /* The KVM XICS device is not in use */
> +    if (kernel_xics_fd == -1) {
> +        return 0;
> +    }
> +
>      /* ICP for this CPU thread is not in use, exiting */
>      if (!icp->cs) {
>          return 0;
> @@ -140,8 +150,8 @@ static void icp_kvm_connect(ICPState *icp, Error **errp)
>      unsigned long vcpu_id;
>      int ret;
>  
> +    /* The KVM XICS device is not in use */
>      if (kernel_xics_fd == -1) {
> -        abort();
>          return;
>      }
>  
> @@ -220,6 +230,11 @@ static void ics_get_kvm_state(ICSState *ics)
>      uint64_t state;
>      int i;
>  
> +    /* The KVM XICS device is not in use */
> +    if (kernel_xics_fd == -1) {
> +        return;
> +    }
> +
>      for (i = 0; i < ics->nr_irqs; i++) {
>          ICSIRQState *irq = &ics->irqs[i];
>  
> @@ -279,6 +294,11 @@ static int ics_set_kvm_state(ICSState *ics, int version_id)
>      int i;
>      Error *local_err = NULL;
>  
> +    /* The KVM XICS device is not in use */
> +    if (kernel_xics_fd == -1) {
> +        return 0;
> +    }
> +
>      for (i = 0; i < ics->nr_irqs; i++) {
>          ICSIRQState *irq = &ics->irqs[i];
>          int ret;
> @@ -325,6 +345,9 @@ void ics_kvm_set_irq(void *opaque, int srcno, int val)
>      struct kvm_irq_level args;
>      int rc;
>  
> +    /* The KVM XICS device should be in use */
> +    assert(kernel_xics_fd != -1);
> +
>      args.irq = srcno + ics->offset;
>      if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_MSI) {
>          if (!val) {

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 <clg@kaod.org>
>> ---
>>  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.

C.

> 
>>      /* Restore the ENDT first. The targetting depends on it. */
>>      CPU_FOREACH(cs) {
>>          kvmppc_xive_set_eq_state(xive, cs, &local_err);
>> @@ -649,6 +672,11 @@ void kvmppc_xive_synchronize_state(sPAPRXive *xive, Error **errp)
>>      CPUState *cs;
>>      Error *local_err = NULL;
>>  
>> +    /* The KVM XIVE device is not in use */
>> +    if (xive->fd == -1) {
>> +        return;
>> +    }
>> +
>>      /*
>>       * When the VM is stopped, the sources are masked and the previous
>>       * state is saved in anticipation of a migration. We should not
>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>> index da6a00bc88cc..651bbfdf6966 100644
>> --- a/hw/intc/xics_kvm.c
>> +++ b/hw/intc/xics_kvm.c
>> @@ -68,6 +68,11 @@ static void icp_get_kvm_state(ICPState *icp)
>>      uint64_t state;
>>      int ret;
>>  
>> +    /* The KVM XICS device is not in use */
>> +    if (kernel_xics_fd == -1) {
>> +        return;
>> +    }
>> +
>>      /* ICP for this CPU thread is not in use, exiting */
>>      if (!icp->cs) {
>>          return;
>> @@ -104,6 +109,11 @@ static int icp_set_kvm_state(ICPState *icp, int version_id)
>>      uint64_t state;
>>      int ret;
>>  
>> +    /* The KVM XICS device is not in use */
>> +    if (kernel_xics_fd == -1) {
>> +        return 0;
>> +    }
>> +
>>      /* ICP for this CPU thread is not in use, exiting */
>>      if (!icp->cs) {
>>          return 0;
>> @@ -140,8 +150,8 @@ static void icp_kvm_connect(ICPState *icp, Error **errp)
>>      unsigned long vcpu_id;
>>      int ret;
>>  
>> +    /* The KVM XICS device is not in use */
>>      if (kernel_xics_fd == -1) {
>> -        abort();
>>          return;
>>      }
>>  
>> @@ -220,6 +230,11 @@ static void ics_get_kvm_state(ICSState *ics)
>>      uint64_t state;
>>      int i;
>>  
>> +    /* The KVM XICS device is not in use */
>> +    if (kernel_xics_fd == -1) {
>> +        return;
>> +    }
>> +
>>      for (i = 0; i < ics->nr_irqs; i++) {
>>          ICSIRQState *irq = &ics->irqs[i];
>>  
>> @@ -279,6 +294,11 @@ static int ics_set_kvm_state(ICSState *ics, int version_id)
>>      int i;
>>      Error *local_err = NULL;
>>  
>> +    /* The KVM XICS device is not in use */
>> +    if (kernel_xics_fd == -1) {
>> +        return 0;
>> +    }
>> +
>>      for (i = 0; i < ics->nr_irqs; i++) {
>>          ICSIRQState *irq = &ics->irqs[i];
>>          int ret;
>> @@ -325,6 +345,9 @@ void ics_kvm_set_irq(void *opaque, int srcno, int val)
>>      struct kvm_irq_level args;
>>      int rc;
>>  
>> +    /* The KVM XICS device should be in use */
>> +    assert(kernel_xics_fd != -1);
>> +
>>      args.irq = srcno + ics->offset;
>>      if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_MSI) {
>>          if (!val) {
>

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 <clg@kaod.org>
> >> ---
> >>  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.