Re: [Qemu-devel] [PATCH 2/2] ppc/xics: preserve P and Q bits for KVM IRQs

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3115 bytes --]

On Thu, Apr 27, 2017 at 04:32:03PM +1000, Sam Bobroff wrote:
> Kernel commit 17d48610ae0f ("KVM: PPC: Book 3S: XICS: Implement ICS
> P/Q states") added new bits to the state used by KVM IRQs. Currently,
> QEMU does not preserve these bits, so migrating (or otherwise saving
> and restoring) the guest state causes the P and Q bits to be cleared.
> 
> Clearing the P bit has no effect, because the kernel will set it based
> on other data, but the loss of a set Q bit will cause a lost
> interrupt.
> 
> This patch preserves the P and Q bits, correcting the problem.
> 
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

So, I've applied this to ppc-for-2.10, because AFAICT it makes some
situations better, and can't make any worse.

However, it bothers me that this is effectively adding two bits to the
TCG XICS state, but isn't putting any TCG logic to handle them
correctly.  I'm rather hoping corresponding TCG logic is coming.

I don't believe TCG<->KVM migration works at present, but I don't
think there's a theoretical reason it couldn't, and I'd certainly
prefer not to introduce reasons for it not to work.

> ---
>  hw/intc/xics_kvm.c    | 12 ++++++++++++
>  include/hw/ppc/xics.h |  2 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 03c1fc77cb..dd93531ae3 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -229,6 +229,12 @@ static void ics_get_kvm_state(ICSState *ics)
>                      | XICS_STATUS_REJECTED;
>              }
>          }
> +        if (state & KVM_XICS_PRESENTED) {
> +                irq->status |= XICS_STATUS_PRESENTED;
> +        }
> +        if (state & KVM_XICS_QUEUED) {
> +                irq->status |= XICS_STATUS_QUEUED;
> +        }
>      }
>  }
>  
> @@ -266,6 +272,12 @@ static int ics_set_kvm_state(ICSState *ics, int version_id)
>                  state |= KVM_XICS_PENDING;
>              }
>          }
> +        if (irq->status & XICS_STATUS_PRESENTED) {
> +                state |= KVM_XICS_PRESENTED;
> +        }
> +        if (irq->status & XICS_STATUS_QUEUED) {
> +                state |= KVM_XICS_QUEUED;
> +        }
>  
>          ret = ioctl(kernel_xics_fd, KVM_SET_DEVICE_ATTR, &attr);
>          if (ret != 0) {
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 9a5e715fe5..36c79eb45c 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -131,6 +131,8 @@ struct ICSIRQState {
>  #define XICS_STATUS_SENT               0x2
>  #define XICS_STATUS_REJECTED           0x4
>  #define XICS_STATUS_MASKED_PENDING     0x8
> +#define XICS_STATUS_PRESENTED          0x10
> +#define XICS_STATUS_QUEUED             0x20
>      uint8_t status;
>  /* (flags & XICS_FLAGS_IRQ_MASK) == 0 means the interrupt is not allocated */
>  #define XICS_FLAGS_IRQ_LSI             0x1

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]