[Qemu-devel] [PATCH 1/2] ppc/xics: Fix stale irq->status bits after get

[Qemu-devel] [PATCH 1/2] ppc/xics: Fix stale irq->status bits after get

[Sam Bobroff]

ics_get_kvm_state() "or"s set bits into irq->status but does not mask
out clear bits.

Correct this by initializing the IRQ status to zero before adding bits
to it.

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
 hw/intc/xics_kvm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 42e0e0ef84..03c1fc77cb 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -213,6 +213,7 @@ static void ics_get_kvm_state(ICSState *ics)
             irq->priority = irq->saved_priority;
         }
 
+        irq->status = 0;
         if (state & KVM_XICS_PENDING) {
             if (state & KVM_XICS_LEVEL_SENSITIVE) {
                 irq->status |= XICS_STATUS_ASSERTED;
-- 
2.12.1.382.gc0f9c7058

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

[Sam Bobroff]

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>
---
 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
-- 
2.12.1.382.gc0f9c7058

Re: [Qemu-devel] [PATCH 1/2] ppc/xics: Fix stale irq->status bits after get

[David Gibson]

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

On Thu, Apr 27, 2017 at 04:31:53PM +1000, Sam Bobroff wrote:
> ics_get_kvm_state() "or"s set bits into irq->status but does not mask
> out clear bits.
> 
> Correct this by initializing the IRQ status to zero before adding bits
> to it.
> 
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

This definitely looks like a bugfix, applied to ppc-for-2.10.

> ---
>  hw/intc/xics_kvm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 42e0e0ef84..03c1fc77cb 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -213,6 +213,7 @@ static void ics_get_kvm_state(ICSState *ics)
>              irq->priority = irq->saved_priority;
>          }
>  
> +        irq->status = 0;
>          if (state & KVM_XICS_PENDING) {
>              if (state & KVM_XICS_LEVEL_SENSITIVE) {
>                  irq->status |= XICS_STATUS_ASSERTED;

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

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

[David Gibson]

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