Re: [PATCH v2 3/4] spapr/xive: Allocate IPIs independently from the other sources

[Cédric Le Goater]

On 8/20/20 3:45 PM, Cédric Le Goater wrote:
> The vCPU IPIs are now allocated in kvmppc_xive_cpu_connect() when the
> vCPU connects to the KVM device and not when all the sources are reset
> in kvmppc_xive_source_reset()

This patch is introducing a regression when vsmt is in used.

  https://bugs.launchpad.net/qemu/+bug/1900241

when the OS boots, H_INT_SET_SOURCE_CONFIG fails with EINVAL, which 
should mean that the IPI is not created at the host/KVM level.

> This requires extra care for hotplug vCPUs and VM restore.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/intc/spapr_xive_kvm.c | 47 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 4ea687c93ecd..f838c208b559 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -146,6 +146,15 @@ int kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, 
> Error **errp)
>      return s.ret;
>  }
>  
> +static int kvmppc_xive_reset_ipi(SpaprXive *xive, CPUState *cs, Error **errp)
> +{
> +    unsigned long ipi = kvm_arch_vcpu_id(cs);

( I am wondering if this is the correct id to use ? )

> +    uint64_t state = 0;
> +
> +    return kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE, ipi,
> +                              &state, true, errp);
> +}
> +
>  int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
>  {
>      ERRP_GUARD();
> @@ -175,6 +184,12 @@ int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
>          return ret;
>      }
>  
> +    /* Create/reset the vCPU IPI */
> +    ret = kvmppc_xive_reset_ipi(xive, tctx->cs, errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
>      kvm_cpu_enable(tctx->cs);
>      return 0;
>  }
> @@ -260,6 +275,12 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int 
> srcno, Error **errp)
>  
>      assert(xive->fd != -1);
>  
> +    /*
> +     * The vCPU IPIs are now allocated in kvmppc_xive_cpu_connect()
> +     * and not with all sources in kvmppc_xive_source_reset()
> +     */
> +    assert(srcno >= SPAPR_XIRQ_BASE);
> +
>      if (xive_source_irq_is_lsi(xsrc, srcno)) {
>          state |= KVM_XIVE_LEVEL_SENSITIVE;
>          if (xsrc->status[srcno] & XIVE_STATUS_ASSERTED) {
> @@ -271,12 +292,28 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int 
> srcno, Error **errp)
>                               true, errp);
>  }
>  
> +/*
> + * To be valid, a source must have been claimed by the machine (valid
> + * entry in the EAS table) and if it is a vCPU IPI, the vCPU should
> + * have been enabled, which means the IPI has been allocated in
> + * kvmppc_xive_cpu_connect().
> + */
> +static bool xive_source_is_valid(SpaprXive *xive, int i)
> +{
> +    return xive_eas_is_valid(&xive->eat[i]) &&
> +        (i >= SPAPR_XIRQ_BASE || kvm_cpu_is_enabled(i));
> +}
> +
>  static int kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp)
>  {
>      SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
>      int i;
>  
> -    for (i = 0; i < xsrc->nr_irqs; i++) {
> +    /*
> +     * Skip the vCPU IPIs. These are created/reset when the vCPUs are
> +     * connected in kvmppc_xive_cpu_connect()
> +     */
> +    for (i = SPAPR_XIRQ_BASE; i < xsrc->nr_irqs; i++) {

This change skips the range [ 0x0 ... 0x1000 ] and relies on the presenter
to create the vCPU IPIs at the KVM level. But spapr_irq_init() could have 
claimed more in : 

        /* Enable the CPU IPIs */
        for (i = 0; i < nr_servers; ++i) {
            SpaprInterruptControllerClass *sicc
                = SPAPR_INTC_GET_CLASS(spapr->xive);

            if (sicc->claim_irq(SPAPR_INTC(spapr->xive), SPAPR_IRQ_IPI + i,
                                false, errp) < 0) {
                return;
            }
        }

I think this is what is happening with vsmt. However, I don't know how to
fix it :/

Thanks,

C.

>          int ret;
>  
>          if (!xive_eas_is_valid(&xive->eat[i])) {
> @@ -370,7 +407,7 @@ static void kvmppc_xive_source_get_state(XiveSource *xsrc)
>      for (i = 0; i < xsrc->nr_irqs; i++) {
>          uint8_t pq;
>  
> -        if (!xive_eas_is_valid(&xive->eat[i])) {
> +        if (!xive_source_is_valid(xive, i)) {
>              continue;
>          }
>  
> @@ -553,7 +590,7 @@ static void kvmppc_xive_change_state_handler(void 
> *opaque, int running,
>              uint8_t pq;
>              uint8_t old_pq;
>  
> -            if (!xive_eas_is_valid(&xive->eat[i])) {
> +            if (!xive_source_is_valid(xive, i)) {
>                  continue;
>              }
>  
> @@ -581,7 +618,7 @@ static void kvmppc_xive_change_state_handler(void 
> *opaque, int running,
>      for (i = 0; i < xsrc->nr_irqs; i++) {
>          uint8_t pq;
>  
> -        if (!xive_eas_is_valid(&xive->eat[i])) {
> +        if (!xive_source_is_valid(xive, i)) {
>              continue;
>          }
>  
> @@ -696,7 +733,7 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
>  
>      /* Restore the EAT */
>      for (i = 0; i < xive->nr_irqs; i++) {
> -        if (!xive_eas_is_valid(&xive->eat[i])) {
> +        if (!xive_source_is_valid(xive, i)) {
>              continue;
>          }
>  
>