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

[Cédric Le Goater]

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.

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