Re: [PATCH 3/8] spapr/xive: Query the characteristics of a source in KVM

[Cédric Le Goater]

On 8/20/20 3:33 AM, David Gibson wrote:
> On Wed, Aug 19, 2020 at 03:08:38PM +0200, Cédric Le Goater wrote:
>> When running a guest with a kernel IRQ chip enabled, the XIVE
>> characteristics of the interrupts are advertised to the guest in the
>> H_INT_GET_SOURCE_INFO hcall. These characteristics depend on the
>> underlying HW interrupts but today, QEMU simply advertises its own
>> without checking what the host supports. It is not a problem for the
>> moment, but POWER10 will (re)add support for StoreEOI and we need a
>> way to in sync with the host.
>>
>> The KVM_DEV_XIVE_GRP_SOURCE_INFO command lets QEMU query the XIVE
>> characteristics of the underlying HW interrupts and override any
>> previous setting done by QEMU. This allows the fallback mode, when the
>> XIVE device is emulated by QEMU, to use its own custom settings on the
>> sources but makes sure that we don't let a guest run with features
>> incompatible with KVM.
>>
>> It only applies to the StoreEOI feature for the moment.
> 
> Urgh.  This means that the source characteristics can change across a
> migration, that's kind of a problem.

yes.

The official plan is : 

  * P9 compat : no StoreEOI 
  * P10 : StoreEOI 

and the obvious solution I started with was to change the global source 
flags at CAS time. 

But, we have experimental P9 firmwares in which StoreEOI is activated
and migration of a P9 compat guest from a P10 host to another P10 host 
should work. I am looking for a smarter solution.

Forbidding StoreEOI on P9 compat guests is ok, since this is already 
the default, but an option to change it would be nice to have. 

As for P10, all HW sources and IC sources now have StoreEOI but we
might want to disable it for some reason. This is already in place 
at the firmware and PowerNV level, QEMU is the next layer to address.

Thanks,

C. 




> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/spapr_xive.h |  2 ++
>>  hw/intc/spapr_xive.c        | 20 ++++++++++++++++++++
>>  hw/intc/spapr_xive_kvm.c    | 26 ++++++++++++++++++++++++++
>>  3 files changed, 48 insertions(+)
>>
>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
>> index 1dddcbcb9cdd..3f325723ea74 100644
>> --- a/include/hw/ppc/spapr_xive.h
>> +++ b/include/hw/ppc/spapr_xive.h
>> @@ -84,6 +84,8 @@ void kvmppc_xive_disconnect(SpaprInterruptController 
>> *intc);
>>  void kvmppc_xive_reset(SpaprXive *xive, Error **errp);
>>  int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS 
>> *eas,
>>                                    Error **errp);
>> +int kvmppc_xive_get_source_info(SpaprXive *xive, uint32_t lisn, uint64_t 
>> *flags,
>> +                                 Error **errp);
>>  void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp);
>>  uint64_t kvmppc_xive_esb_rw(XiveSource *xsrc, int srcno, uint32_t offset,
>>                              uint64_t data, bool write);
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index 1fa09f287ac0..943b9958a68b 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -932,6 +932,26 @@ static target_ulong h_int_get_source_info(PowerPCCPU 
>> *cpu,
>>          args[0] |= SPAPR_XIVE_SRC_STORE_EOI;
>>      }
>>  
>> +    if (kvm_irqchip_in_kernel()) {
>> +        Error *local_err = NULL;
>> +        uint64_t flags = 0;
>> +
>> +        kvmppc_xive_get_source_info(xive, lisn, &flags, &local_err);
>> +        if (local_err) {
>> +            error_report_err(local_err);
>> +            return H_HARDWARE;
>> +        }
>> +
>> +        /*
>> +         * Override QEMU settings with KVM values
>> +         */
>> +        if (flags & XIVE_SRC_STORE_EOI) {
>> +            args[0] |= SPAPR_XIVE_SRC_STORE_EOI;
>> +        } else {
>> +            args[0] &= ~SPAPR_XIVE_SRC_STORE_EOI;
>> +        }
>> +    }
>> +
>>      /*
>>       * Force the use of the H_INT_ESB hcall in case of an LSI
>>       * interrupt. This is necessary under KVM to re-trigger the
>> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
>> index e8667ce5f621..90f4509e6959 100644
>> --- a/hw/intc/spapr_xive_kvm.c
>> +++ b/hw/intc/spapr_xive_kvm.c
>> @@ -217,6 +217,32 @@ int kvmppc_xive_set_source_config(SpaprXive *xive, 
>> uint32_t lisn, XiveEAS *eas,
>>                               &kvm_src, true, errp);
>>  }
>>  
>> +int kvmppc_xive_get_source_info(SpaprXive *xive, uint32_t lisn, uint64_t 
>> *flags,
>> +                                 Error **errp)
>> +{
>> +    struct kvm_ppc_xive_src kvm_src = { 0 };
>> +    int ret;
>> +
>> +    /*
>> +     * Check that KVM supports the new attribute to query source
>> +     * characteristics.
>> +     */
>> +    if (!kvm_device_check_attr(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_INFO, 0)) {
>> +        return 0;
>> +    }
>> +
>> +    ret = kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_INFO, lisn,
>> +                            &kvm_src, false, errp);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    if (kvm_src.flags & KVM_XIVE_SOURCE_FLAG_STORE_EOI) {
>> +        *flags |= XIVE_SRC_STORE_EOI;
>> +    }
>> +    return 0;
>> +}
>> +
>>  void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp)
>>  {
>>      kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_SYNC, lisn,
>