[Xen-devel] [XEN PATCH v3 00/11] xen: arm: context switch vtimer PPI state

[Xen-devel] [XEN PATCH v3 00/11] xen: arm: context switch vtimer PPI state

[Stewart Hildebrand]

 This is an update to Ian Campbell's work to route timer PPIs to guests
[1].

I attempted to address most of the feedback on v2 of the series. There
are a couple of comments I was unsure about - instances of this are
noted in the individual patches.

Highlights in v3:
  * Rebase
  * Tested on QEMU with GICv3
  * Tested on Xilinx Zynq UltraScale+ with GICv2

While I build-tested with CONFIG_NEW_VGIC=y, I only did a quick runtime
test with the new vGIC and I encountered an ASSERT failure:

  Assertion 'irq->hwintid >= VGIC_NR_PRIVATE_IRQS' failed at vgic-mmio.c:96

Because of this, and because there is still some feedback outstanding
from v2, portions of this series may be considered RFC-ish (especially
the last patch "context switch vtimer PPI state").

[1] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg00921.html


Ian Campbell (7):
  xen: arm: fix indentation of struct vtimer
  xen: arm: fix typo in the description of struct pending_irq->desc
  xen: arm: Refactor route_irq_to_guest
  xen: arm: add interfaces to save/restore the state of a PPI.
  xen: arm: gic: supporting routing a PPI to the current vcpu.
  xen: arm: context switch vtimer PPI state.
  HACK: Force virt timer to PPI0 (IRQ16)

Stewart Hildebrand (4):
  xen: arm: remove is_assignable_irq
  Add NR_SGIS and NR_PPIS definitions to irq.h
  xen: arm: vgic: allow delivery of PPIs to guests
  xen: arm: vgic: don't fail if IRQ is already connected

 xen/arch/arm/gic-v2.c            |  69 +++++++++++
 xen/arch/arm/gic-v3.c            |  69 +++++++++++
 xen/arch/arm/gic-vgic.c          |  33 +++--
 xen/arch/arm/gic.c               |  79 ++++++++++++
 xen/arch/arm/irq.c               | 202 ++++++++++++++++++++++---------
 xen/arch/arm/time.c              |  26 +---
 xen/arch/arm/vgic.c              |   6 +-
 xen/arch/arm/vgic/vgic.c         |   4 +
 xen/arch/arm/vtimer.c            |  45 ++++++-
 xen/include/asm-arm/domain.h     |  22 +++-
 xen/include/asm-arm/gic.h        |  24 ++++
 xen/include/asm-arm/irq.h        |   9 +-
 xen/include/asm-arm/perfc_defn.h |   1 -
 xen/include/asm-arm/vgic.h       |   2 +-
 xen/include/public/arch-arm.h    |   2 +-
 15 files changed, 483 insertions(+), 110 deletions(-)

-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH v3 01/11] xen: arm: fix indentation of struct vtimer

[Stewart Hildebrand]

From: Ian Campbell <ian.campbell@citrix.com>

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com> [1]
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> [2]

[1] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg00985.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2015-12/msg02646.html

---
v3:
  * Rebase (no conflicts)
  * Add Reviewed-by and Acked-by from a few years ago
---
 xen/include/asm-arm/domain.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 86ebdd2bcf..f3f3fb7d7f 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -35,11 +35,11 @@ enum domain_type {
 #define is_domain_direct_mapped(d) ((d) == hardware_domain)
 
 struct vtimer {
-        struct vcpu *v;
-        int irq;
-        struct timer timer;
-        uint32_t ctl;
-        uint64_t cval;
+    struct vcpu *v;
+    int irq;
+    struct timer timer;
+    uint32_t ctl;
+    uint64_t cval;
 };
 
 struct arch_domain
-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 01/11] xen: arm: fix indentation of struct vtimer

[Julien Grall]

Hi,

On 15/11/2019 20:01, Stewart Hildebrand wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Reviewed-by: Julien Grall <julien.grall@citrix.com> [1]
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> [2]

Regardless the complexity of the patch, this was reviewed nearly 4 years 
ago and therefore review tags may be stall even if the reviewers are the 
same.

Indeed, the code base has changed quite a lot and some of the patches 
may require modifications to fit the direction of the project.

Anyway, for this patch:

Acked-by: Julien Grall <julien.grall@xen.org>

I will commit it in my branch for-next/4.14 and will merge it once 4.13 
has been released.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 01/11] xen: arm: fix indentation of struct vtimer

[Julien Grall]

On 23/11/2019 18:40, Julien Grall wrote:
> Hi,
> 
> On 15/11/2019 20:01, Stewart Hildebrand wrote:
>> From: Ian Campbell <ian.campbell@citrix.com>
>>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> Reviewed-by: Julien Grall <julien.grall@citrix.com> [1]
>> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> [2]
> 
> Regardless the complexity of the patch, this was reviewed nearly 4 years 
> ago and therefore review tags may be stall even if the reviewers are the 
> same.
> 
> Indeed, the code base has changed quite a lot and some of the patches 
> may require modifications to fit the direction of the project.
> 
> Anyway, for this patch:
> 
> Acked-by: Julien Grall <julien.grall@xen.org>

Sorry, it should be julien@xen.org. Still getting use of my new e-mail.

> 
> I will commit it in my branch for-next/4.14 and will merge it once 4.13 
> has been released.
> 
> Cheers,
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH v3 02/11] xen: arm: fix typo in the description of struct pending_irq->desc

[Stewart Hildebrand]

From: Ian Campbell <ian.campbell@citrix.com>

s/it/if/ makes more sense.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com> [1]
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> [2]

[1] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg00986.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2015-12/msg02645.html

---
v3:
  * Rebase (no conflicts)
  * Add Reviewed-by and Acked-by from a few years ago
---
 xen/include/asm-arm/vgic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 447d24ea59..ce1e3c4bbd 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -77,7 +77,7 @@ struct pending_irq
 #define GIC_IRQ_GUEST_MIGRATING   4
 #define GIC_IRQ_GUEST_PRISTINE_LPI  5
     unsigned long status;
-    struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
+    struct irq_desc *desc; /* only set if the irq corresponds to a physical irq */
     unsigned int irq;
 #define GIC_INVALID_LR         (uint8_t)~0
     uint8_t lr;
-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 02/11] xen: arm: fix typo in the description of struct pending_irq->desc

[Julien Grall]

Hi,

On 15/11/2019 20:01, Stewart Hildebrand wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> s/it/if/ makes more sense.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Reviewed-by: Julien Grall <julien.grall@citrix.com> [1]
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> [2]

Similar remark to the previous patch.

Acked-by: Julien Grall <julien@xen.org>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH v3 03/11] xen: arm: Refactor route_irq_to_guest

[Stewart Hildebrand]

From: Ian Campbell <ian.campbell@citrix.com>

Split out the bit which allocates the struct irqaction and calls
__setup_irq into a new function (setup_guest_irq). I'm going to want
to call this a second time in a subsequent patch.

Note that the action is now allocated and initialised with the desc
lock held (since it is taken by the caller). I don't think this is an
issue (and avoiding this would make things more complex)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
---
v2: New patch (maybe, it's been a while...)

v3: Rebase + trivial fixups

---
Note: I have not given much thought regarding Julien's comment in [1]

[1] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01041.html
---
 xen/arch/arm/irq.c | 108 +++++++++++++++++++++++++++------------------
 1 file changed, 64 insertions(+), 44 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 3877657a52..9cc0a54867 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -407,61 +407,25 @@ bool irq_type_set_by_domain(const struct domain *d)
     return (d == hardware_domain);
 }
 
-/*
- * Route an IRQ to a specific guest.
- * For now only SPIs are assignable to the guest.
- */
-int route_irq_to_guest(struct domain *d, unsigned int virq,
-                       unsigned int irq, const char * devname)
+static int setup_guest_irq(struct irq_desc *desc, unsigned int virq,
+                           unsigned int irqflags,
+                           struct irq_guest *info, const char *devname)
 {
+    const unsigned irq = desc->irq;
     struct irqaction *action;
-    struct irq_guest *info;
-    struct irq_desc *desc;
-    unsigned long flags;
-    int retval = 0;
-
-    if ( virq >= vgic_num_irqs(d) )
-    {
-        printk(XENLOG_G_ERR
-               "the vIRQ number %u is too high for domain %u (max = %u)\n",
-               irq, d->domain_id, vgic_num_irqs(d));
-        return -EINVAL;
-    }
-
-    /* Only routing to virtual SPIs is supported */
-    if ( virq < NR_LOCAL_IRQS )
-    {
-        printk(XENLOG_G_ERR "IRQ can only be routed to an SPI\n");
-        return -EINVAL;
-    }
+    int retval;
+    struct domain *d = info->d;
 
-    if ( !is_assignable_irq(irq) )
-    {
-        printk(XENLOG_G_ERR "the IRQ%u is not routable\n", irq);
-        return -EINVAL;
-    }
-    desc = irq_to_desc(irq);
+    ASSERT(spin_is_locked(&desc->lock));
 
     action = xmalloc(struct irqaction);
     if ( !action )
         return -ENOMEM;
 
-    info = xmalloc(struct irq_guest);
-    if ( !info )
-    {
-        xfree(action);
-        return -ENOMEM;
-    }
-
-    info->d = d;
-    info->virq = virq;
-
     action->dev_id = info;
     action->name = devname;
     action->free_on_release = 1;
 
-    spin_lock_irqsave(&desc->lock, flags);
-
     if ( !irq_type_set_by_domain(d) && desc->arch.type == IRQ_TYPE_INVALID )
     {
         printk(XENLOG_G_ERR "IRQ %u has not been configured\n", irq);
@@ -496,6 +460,8 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
                        d->domain_id, irq, irq_get_guest_info(desc)->virq);
                 retval = -EBUSY;
             }
+            else
+                retval = 0;
         }
         else
         {
@@ -509,6 +475,61 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
     if ( retval )
         goto out;
 
+    return 0;
+
+out:
+    xfree(action);
+    return retval;
+}
+
+/*
+ * Route an IRQ to a specific guest.
+ * For now only SPIs are assignable to the guest.
+ */
+int route_irq_to_guest(struct domain *d, unsigned int virq,
+                       unsigned int irq, const char * devname)
+{
+    struct irq_guest *info;
+    struct irq_desc *desc;
+    unsigned long flags;
+    int retval;
+
+    if ( virq >= vgic_num_irqs(d) )
+    {
+        printk(XENLOG_G_ERR
+               "the vIRQ number %u is too high for domain %u (max = %u)\n",
+               irq, d->domain_id, vgic_num_irqs(d));
+        return -EINVAL;
+    }
+
+    /* Only routing to virtual SPIs is supported */
+    if ( virq < NR_LOCAL_IRQS )
+    {
+        printk(XENLOG_G_ERR "IRQ can only be routed to an SPI\n");
+        return -EINVAL;
+    }
+
+    if ( !is_assignable_irq(irq) )
+    {
+        printk(XENLOG_G_ERR "the IRQ%u is not routable\n", irq);
+        return -EINVAL;
+    }
+
+    desc = irq_to_desc(irq);
+
+    info = xmalloc(struct irq_guest);
+    if ( !info )
+        return -ENOMEM;
+
+    info->d = d;
+    info->virq = virq;
+
+    spin_lock_irqsave(&desc->lock, flags);
+
+    retval = setup_guest_irq(desc, virq, flags, info, devname);
+    if ( retval )
+        goto out;
+
     retval = gic_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ);
 
     spin_unlock_irqrestore(&desc->lock, flags);
@@ -523,7 +544,6 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
 
 out:
     spin_unlock_irqrestore(&desc->lock, flags);
-    xfree(action);
 free_info:
     xfree(info);
 
-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 03/11] xen: arm: Refactor route_irq_to_guest

[Julien Grall]

Hi,

On 15/11/2019 20:01, Stewart Hildebrand wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> Split out the bit which allocates the struct irqaction and calls
> __setup_irq into a new function (setup_guest_irq). I'm going to want
> to call this a second time in a subsequent patch.
> 
> Note that the action is now allocated and initialised with the desc
> lock held (since it is taken by the caller). I don't think this is an
> issue (and avoiding this would make things more complex)
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> ---
> v2: New patch (maybe, it's been a while...)
> 
> v3: Rebase + trivial fixups
> 
> ---
> Note: I have not given much thought regarding Julien's comment in [1]
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01041.html

I would rather avoid to do memory allocation with interrupts disabled.

You may need to introduce a new function that will allocate/setup the 
action, but I think this is worth the trouble. Note that the new 
function could likely be re-used in request_irq() as well.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH v3 04/11] xen: arm: remove is_assignable_irq

[Stewart Hildebrand]

It only had 1 caller.

Reverse the condition for readability.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>

---
v3: new patch
---
 xen/arch/arm/irq.c        | 9 ++-------
 xen/include/asm-arm/irq.h | 2 --
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 9cc0a54867..c80782026f 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -390,12 +390,6 @@ err:
     return rc;
 }
 
-bool is_assignable_irq(unsigned int irq)
-{
-    /* For now, we can only route SPIs to the guest */
-    return (irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines());
-}
-
 /*
  * Only the hardware domain is allowed to set the configure the
  * interrupt type for now.
@@ -509,7 +503,8 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
         return -EINVAL;
     }
 
-    if ( !is_assignable_irq(irq) )
+    /* For now, we can only route SPIs to the guest */
+    if ( (irq < NR_LOCAL_IRQS) || (irq >= gic_number_lines()) )
     {
         printk(XENLOG_G_ERR "the IRQ%u is not routable\n", irq);
         return -EINVAL;
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index e45d574598..e14001b5c6 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -70,8 +70,6 @@ static inline bool is_lpi(unsigned int irq)
 
 #define domain_pirq_to_irq(d, pirq) (pirq)
 
-bool is_assignable_irq(unsigned int irq);
-
 void init_IRQ(void);
 void init_secondary_IRQ(void);
 
-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 04/11] xen: arm: remove is_assignable_irq

[Julien Grall]

Hi,

On 15/11/2019 20:01, Stewart Hildebrand wrote:
> It only had 1 caller.
If this is the only reason, then I would prefer to keep it as it makes 
easier to reason. So are you removing it because the function 
is_assignable_irq() and route_irq_to_guest() are not going to be re-used 
for PPIs?

If so, we should rename the function route_irq_to_guest() to clarify 
this can only be used on SPIs.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH v3 05/11] xen: arm: add interfaces to save/restore the state of a PPI.

[Stewart Hildebrand]

From: Ian Campbell <ian.campbell@citrix.com>

Make use of the GICD I[SC]ACTIVER registers to save and
restore the active state of the interrupt.

For edge triggered interrupts we also need to context switch the
pending bit via I[SC]PENDR. Note that for level triggered interrupts
SPENDR sets a latch which is only cleared by ICPENDR (and not by h/w
state changes), therefore we do not want to context switch the pending
state for level PPIs -- instead we rely on the context switch of the
peripheral to restore the correct level.

Unused as yet, will be used by the vtimer code shortly.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>

---
v3: Address feedback from v2 [1]:
  * Add a comment to explain that PPI are always below 31.
  * Use uint32_t for pendingr, activer, enabler
  * Fixup register names in gic-v3.c
  * Add newlines for clarity
  * Make gicv3_irq_enable/disable declarations static
  * Use readl_relaxed (not readl_gicd) in gic-v3.c
  * Add note to comment explaining devices using PPI being quiet during
        save/restore. Suggested by Julien.
  * Test on QEMU's model of GICv3

Note: I have not given any thought to the comments in [2] regarding
disabling IRQ or enable/disable state.

[1] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01049.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01051.html
---
 xen/arch/arm/gic-v2.c        | 69 ++++++++++++++++++++++++++++++++++++
 xen/arch/arm/gic-v3.c        | 69 ++++++++++++++++++++++++++++++++++++
 xen/arch/arm/gic.c           | 54 ++++++++++++++++++++++++++++
 xen/arch/arm/irq.c           |  7 ++++
 xen/include/asm-arm/domain.h | 11 ++++++
 xen/include/asm-arm/gic.h    | 22 ++++++++++++
 xen/include/asm-arm/irq.h    |  2 ++
 7 files changed, 234 insertions(+)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 256988c665..13f106cb61 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -123,6 +123,9 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
 /* Maximum cpu interface per GIC */
 #define NR_GIC_CPU_IF 8
 
+static void gicv2_irq_enable(struct irq_desc *desc);
+static void gicv2_irq_disable(struct irq_desc *desc);
+
 static inline void writeb_gicd(uint8_t val, unsigned int offset)
 {
     writeb_relaxed(val, gicv2.map_dbase + offset);
@@ -191,6 +194,38 @@ static void gicv2_save_state(struct vcpu *v)
     writel_gich(0, GICH_HCR);
 }
 
+static void gicv2_save_and_mask_hwppi(struct irq_desc *desc,
+                                      struct hwppi_state *s)
+{
+    const uint32_t mask = (1u << desc->irq); /* PPIs are IRQ# 16-31 */
+    const uint32_t pendingr = readl_gicd(GICD_ISPENDR);
+    const uint32_t activer = readl_gicd(GICD_ISACTIVER);
+    const uint32_t enabler = readl_gicd(GICD_ISENABLER);
+    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
+
+    s->active = !!(activer & mask);
+    s->enabled = !!(enabler & mask);
+    s->pending = !!(pendingr & mask);
+
+    /* Write a 1 to IC...R to clear the corresponding bit of state */
+    if ( s->active )
+        writel_gicd(mask, GICD_ICACTIVER);
+
+    /*
+     * For an edge interrupt clear the pending state, for a level interrupt
+     * this clears the latch there is no need since saving the peripheral state
+     * (and/or restoring the next VCPU) will cause the correct action.
+     */
+    if ( is_edge && s->pending )
+        writel_gicd(mask, GICD_ICPENDR);
+
+    if ( s->enabled )
+        gicv2_irq_disable(desc);
+
+    ASSERT(!(readl_gicd(GICD_ISACTIVER) & mask));
+    ASSERT(!(readl_gicd(GICD_ISENABLER) & mask));
+}
+
 static void gicv2_restore_state(const struct vcpu *v)
 {
     int i;
@@ -203,6 +238,38 @@ static void gicv2_restore_state(const struct vcpu *v)
     writel_gich(GICH_HCR_EN, GICH_HCR);
 }
 
+static void gicv2_restore_hwppi(struct irq_desc *desc,
+                                const struct hwppi_state *s)
+{
+    const uint32_t mask = (1u << desc->irq); /* PPIs are IRQ# 16-31 */
+    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
+
+    /*
+     * The IRQ must always have been set inactive and masked etc by
+     * the saving of the previous state via save_and_mask_hwppi.
+     */
+    ASSERT(!(readl_gicd(GICD_ISACTIVER) & mask));
+    ASSERT(!(readl_gicd(GICD_ISENABLER) & mask));
+
+    if ( s->active )
+        writel_gicd(mask, GICD_ICACTIVER);
+
+    /*
+     * Restore pending state for edge triggered interrupts only. For
+     * level triggered interrupts the level will be restored as
+     * necessary by restoring the state of the relevant peripheral.
+     *
+     * For a level triggered interrupt ISPENDR acts as a *latch* which
+     * is only cleared by ICPENDR (i.e. the input level is no longer
+     * relevant). We certainly do not want that here.
+     */
+    if ( is_edge && s->pending )
+        writel_gicd(mask, GICD_ISPENDR);
+
+    if ( s->enabled )
+        gicv2_irq_enable(desc);
+}
+
 static void gicv2_dump_state(const struct vcpu *v)
 {
     int i;
@@ -1335,7 +1402,9 @@ const static struct gic_hw_operations gicv2_ops = {
     .init                = gicv2_init,
     .secondary_init      = gicv2_secondary_cpu_init,
     .save_state          = gicv2_save_state,
+    .save_and_mask_hwppi = gicv2_save_and_mask_hwppi,
     .restore_state       = gicv2_restore_state,
+    .restore_hwppi       = gicv2_restore_hwppi,
     .dump_state          = gicv2_dump_state,
     .gic_host_irq_type   = &gicv2_host_irq_type,
     .gic_guest_irq_type  = &gicv2_guest_irq_type,
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 0f6cbf6224..be5ea61ab5 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -63,6 +63,9 @@ static DEFINE_PER_CPU(void __iomem*, rbase);
 #define GICD_RDIST_BASE        (this_cpu(rbase))
 #define GICD_RDIST_SGI_BASE    (GICD_RDIST_BASE + SZ_64K)
 
+static void gicv3_irq_enable(struct irq_desc *desc);
+static void gicv3_irq_disable(struct irq_desc *desc);
+
 /*
  * Saves all 16(Max) LR registers. Though number of LRs implemented
  * is implementation specific.
@@ -375,6 +378,38 @@ static void gicv3_save_state(struct vcpu *v)
     v->arch.gic.v3.sre_el1 = READ_SYSREG32(ICC_SRE_EL1);
 }
 
+static void gicv3_save_and_mask_hwppi(struct irq_desc *desc,
+                                      struct hwppi_state *s)
+{
+    const uint32_t mask = (1u << desc->irq); /* PPIs are IRQ# 16-31 */
+    const uint32_t pendingr = readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISPENDR0);
+    const uint32_t activer = readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISACTIVER0);
+    const uint32_t enabler = readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISENABLER0);
+    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
+
+    s->active = !!(activer & mask);
+    s->enabled = !!(enabler & mask);
+    s->pending = !!(pendingr & mask);
+
+    /* Write a 1 to IC...R to clear the corresponding bit of state */
+    if ( s->active )
+        writel_relaxed(mask, GICD_RDIST_SGI_BASE + GICR_ICACTIVER0);
+
+    /*
+     * For an edge interrupt clear the pending state, for a level interrupt
+     * this clears the latch there is no need since saving the peripheral state
+     * (and/or restoring the next VCPU) will cause the correct action.
+     */
+    if ( is_edge && s->pending )
+        writel_relaxed(mask, GICD_RDIST_SGI_BASE + GICR_ICPENDR0);
+
+    if ( s->enabled )
+        gicv3_irq_disable(desc);
+
+    ASSERT(!(readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISACTIVER0) & mask));
+    ASSERT(!(readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISENABLER0) & mask));
+}
+
 static void gicv3_restore_state(const struct vcpu *v)
 {
     uint32_t val;
@@ -410,6 +445,38 @@ static void gicv3_restore_state(const struct vcpu *v)
     dsb(sy);
 }
 
+static void gicv3_restore_hwppi(struct irq_desc *desc,
+                                const struct hwppi_state *s)
+{
+    const uint32_t mask = (1u << desc->irq); /* PPIs are IRQ# 16-31 */
+    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
+
+    /*
+     * The IRQ must always have been set inactive and masked etc by
+     * the saving of the previous state via save_and_mask_hwppi.
+     */
+    ASSERT(!(readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISACTIVER0) & mask));
+    ASSERT(!(readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISENABLER0) & mask));
+
+    if ( s->active )
+        writel_relaxed(mask, GICD_RDIST_SGI_BASE + GICR_ICACTIVER0);
+
+    /*
+     * Restore pending state for edge triggered interrupts only. For
+     * level triggered interrupts the level will be restored as
+     * necessary by restoring the state of the relevant peripheral.
+     *
+     * For a level triggered interrupt ISPENDR acts as a *latch* which
+     * is only cleared by ICPENDR (i.e. the input level is no longer
+     * relevant). We certainly do not want that here.
+     */
+    if ( is_edge && s->pending )
+        writel_relaxed(mask, GICD_RDIST_SGI_BASE + GICR_ISPENDR0);
+
+    if ( s->enabled )
+        gicv3_irq_enable(desc);
+}
+
 static void gicv3_dump_state(const struct vcpu *v)
 {
     int i;
@@ -1835,7 +1902,9 @@ static const struct gic_hw_operations gicv3_ops = {
     .info                = &gicv3_info,
     .init                = gicv3_init,
     .save_state          = gicv3_save_state,
+    .save_and_mask_hwppi = gicv3_save_and_mask_hwppi,
     .restore_state       = gicv3_restore_state,
+    .restore_hwppi       = gicv3_restore_hwppi,
     .dump_state          = gicv3_dump_state,
     .gic_host_irq_type   = &gicv3_host_irq_type,
     .gic_guest_irq_type  = &gicv3_guest_irq_type,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 113655a789..75921724dd 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -64,6 +64,17 @@ unsigned int gic_number_lines(void)
     return gic_hw_ops->info->nr_lines;
 }
 
+void gic_hwppi_state_init(struct hwppi_state *s, unsigned irq)
+{
+    memset(s, 0, sizeof(*s));
+    s->irq = irq;
+}
+
+void gic_hwppi_set_pending(struct hwppi_state *s)
+{
+    s->pending = true;
+}
+
 void gic_save_state(struct vcpu *v)
 {
     ASSERT(!local_irq_is_enabled());
@@ -78,6 +89,25 @@ void gic_save_state(struct vcpu *v)
     isb();
 }
 
+void gic_save_and_mask_hwppi(struct vcpu *v, const unsigned virq,
+                             struct hwppi_state *s)
+{
+    struct pending_irq *p = irq_to_pending(v, virq);
+    struct irq_desc *desc = p->desc;
+
+    spin_lock(&desc->lock);
+
+    ASSERT(virq >= 16 && virq < 32);
+    ASSERT(desc->irq >= 16 && desc->irq < 32);
+    ASSERT(!is_idle_vcpu(v));
+
+    s->inprogress = test_and_clear_bit(_IRQ_INPROGRESS, &desc->status);
+
+    gic_hw_ops->save_and_mask_hwppi(desc, s);
+
+    spin_unlock(&desc->lock);
+}
+
 void gic_restore_state(struct vcpu *v)
 {
     ASSERT(!local_irq_is_enabled());
@@ -89,6 +119,30 @@ void gic_restore_state(struct vcpu *v)
     isb();
 }
 
+void gic_restore_hwppi(struct vcpu *v,
+                       const unsigned virq,
+                       const struct hwppi_state *s)
+{
+    struct pending_irq *p = irq_to_pending(v, virq);
+    struct irq_desc *desc = irq_to_desc(s->irq);
+
+    spin_lock(&desc->lock);
+
+    ASSERT(virq >= 16 && virq < 32);
+    ASSERT(!is_idle_vcpu(v));
+
+    p->desc = desc; /* Migrate to new physical processor */
+
+    irq_set_virq(desc, virq);
+
+    gic_hw_ops->restore_hwppi(desc, s);
+
+    if ( s->inprogress )
+        set_bit(_IRQ_INPROGRESS, &desc->status);
+
+    spin_unlock(&desc->lock);
+}
+
 /* desc->irq needs to be disabled before calling this function */
 void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
 {
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index c80782026f..1a8e599c2e 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -150,6 +150,13 @@ static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc)
     return desc->action->dev_id;
 }
 
+void irq_set_virq(struct irq_desc *desc, unsigned virq)
+{
+    struct irq_guest *info = irq_get_guest_info(desc);
+    ASSERT(test_bit(_IRQ_PER_CPU, &desc->status));
+    info->virq = virq;
+}
+
 static inline struct domain *irq_get_domain(struct irq_desc *desc)
 {
     return irq_get_guest_info(desc)->d;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index f3f3fb7d7f..c3f4cd5069 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -34,6 +34,17 @@ enum domain_type {
 /* The hardware domain has always its memory direct mapped. */
 #define is_domain_direct_mapped(d) ((d) == hardware_domain)
 
+struct hwppi_state {
+    /* h/w state */
+    unsigned irq;
+    unsigned long enabled:1;
+    unsigned long pending:1;
+    unsigned long active:1;
+
+    /* Xen s/w state */
+    unsigned long inprogress:1;
+};
+
 struct vtimer {
     struct vcpu *v;
     int irq;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 793d324b33..1164e0c7a6 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -275,6 +275,26 @@ extern int gicv_setup(struct domain *d);
 extern void gic_save_state(struct vcpu *v);
 extern void gic_restore_state(struct vcpu *v);
 
+/*
+ * Save/restore the state of a single PPI which must be routed to
+ * <current-vcpu> (that is, is defined to be injected to the current
+ * vcpu).
+ *
+ * We expect the device which use this PPI to be quiet while we
+ * save/restore.
+ *
+ * For instance we want to disable the timer before saving the state.
+ * Otherwise we will mess up the state.
+ */
+struct hwppi_state;
+extern void gic_hwppi_state_init(struct hwppi_state *s, unsigned irq);
+extern void gic_hwppi_set_pending(struct hwppi_state *s);
+extern void gic_save_and_mask_hwppi(struct vcpu *v, unsigned irq,
+                                    struct hwppi_state *s);
+extern void gic_restore_hwppi(struct vcpu *v,
+                              const unsigned virq,
+                              const struct hwppi_state *s);
+
 /* SGI (AKA IPIs) */
 enum gic_sgi {
     GIC_SGI_EVENT_CHECK = 0,
@@ -325,8 +345,10 @@ struct gic_hw_operations {
     int (*init)(void);
     /* Save GIC registers */
     void (*save_state)(struct vcpu *);
+    void (*save_and_mask_hwppi)(struct irq_desc *desc, struct hwppi_state *s);
     /* Restore GIC registers */
     void (*restore_state)(const struct vcpu *);
+    void (*restore_hwppi)(struct irq_desc *desc, const struct hwppi_state *s);
     /* Dump GIC LR register information */
     void (*dump_state)(const struct vcpu *);
 
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index e14001b5c6..3b37a21c06 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -96,6 +96,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
  */
 bool irq_type_set_by_domain(const struct domain *d);
 
+void irq_set_virq(struct irq_desc *desc, unsigned virq);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:
-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 05/11] xen: arm: add interfaces to save/restore the state of a PPI.

[Stewart Hildebrand]

On Friday, November 15, 2019 3:11 PM, Stewart Hildebrand write:

[...]

>diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>index 113655a789..75921724dd 100644
>--- a/xen/arch/arm/gic.c
>+++ b/xen/arch/arm/gic.c

[...]

>@@ -78,6 +89,25 @@ void gic_save_state(struct vcpu *v)
>     isb();
> }
>
>+void gic_save_and_mask_hwppi(struct vcpu *v, const unsigned virq,
>+                             struct hwppi_state *s)
>+{
>+    struct pending_irq *p = irq_to_pending(v, virq);
>+    struct irq_desc *desc = p->desc;

I intended to replace this with a call to vgic_get_hw_irq_desc, but I
accidentally rolled the change into a later patch instead of this one.

>+
>+    spin_lock(&desc->lock);
>+
>+    ASSERT(virq >= 16 && virq < 32);
>+    ASSERT(desc->irq >= 16 && desc->irq < 32);
>+    ASSERT(!is_idle_vcpu(v));
>+
>+    s->inprogress = test_and_clear_bit(_IRQ_INPROGRESS, &desc->status);
>+
>+    gic_hw_ops->save_and_mask_hwppi(desc, s);
>+
>+    spin_unlock(&desc->lock);
>+}
>+
> void gic_restore_state(struct vcpu *v)
> {
>     ASSERT(!local_irq_is_enabled());
>@@ -89,6 +119,30 @@ void gic_restore_state(struct vcpu *v)
>     isb();
> }
>
>+void gic_restore_hwppi(struct vcpu *v,
>+                       const unsigned virq,
>+                       const struct hwppi_state *s)
>+{
>+    struct pending_irq *p = irq_to_pending(v, virq);
>+    struct irq_desc *desc = irq_to_desc(s->irq);
>+
>+    spin_lock(&desc->lock);
>+
>+    ASSERT(virq >= 16 && virq < 32);
>+    ASSERT(!is_idle_vcpu(v));
>+
>+    p->desc = desc; /* Migrate to new physical processor */

I intended to replace this with a call to vgic_connect_hw_irq. Same story as above.

>+
>+    irq_set_virq(desc, virq);
>+
>+    gic_hw_ops->restore_hwppi(desc, s);
>+
>+    if ( s->inprogress )
>+        set_bit(_IRQ_INPROGRESS, &desc->status);
>+
>+    spin_unlock(&desc->lock);
>+}
>+
> /* desc->irq needs to be disabled before calling this function */
> void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
> {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 05/11] xen: arm: add interfaces to save/restore the state of a PPI.

[Julien Grall]

Hi,

On 15/11/2019 20:10, Stewart Hildebrand wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> Make use of the GICD I[SC]ACTIVER registers to save and
> restore the active state of the interrupt.
> 
> For edge triggered interrupts we also need to context switch the
> pending bit via I[SC]PENDR. Note that for level triggered interrupts
> SPENDR sets a latch which is only cleared by ICPENDR (and not by h/w
> state changes), therefore we do not want to context switch the pending
> state for level PPIs -- instead we rely on the context switch of the
> peripheral to restore the correct level.
> 
> Unused as yet, will be used by the vtimer code shortly.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> 
> ---
> v3: Address feedback from v2 [1]:
>    * Add a comment to explain that PPI are always below 31.
>    * Use uint32_t for pendingr, activer, enabler
>    * Fixup register names in gic-v3.c
>    * Add newlines for clarity
>    * Make gicv3_irq_enable/disable declarations static
>    * Use readl_relaxed (not readl_gicd) in gic-v3.c
>    * Add note to comment explaining devices using PPI being quiet during
>          save/restore. Suggested by Julien.
>    * Test on QEMU's model of GICv3
> 
> Note: I have not given any thought to the comments in [2] regarding
> disabling IRQ or enable/disable state.

I will try to answer this below.

> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01049.html
> [2] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01051.html
> ---
>   xen/arch/arm/gic-v2.c        | 69 ++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/gic-v3.c        | 69 ++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/gic.c           | 54 ++++++++++++++++++++++++++++
>   xen/arch/arm/irq.c           |  7 ++++
>   xen/include/asm-arm/domain.h | 11 ++++++
>   xen/include/asm-arm/gic.h    | 22 ++++++++++++
>   xen/include/asm-arm/irq.h    |  2 ++
>   7 files changed, 234 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 256988c665..13f106cb61 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -123,6 +123,9 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
>   /* Maximum cpu interface per GIC */
>   #define NR_GIC_CPU_IF 8
>   
> +static void gicv2_irq_enable(struct irq_desc *desc);
> +static void gicv2_irq_disable(struct irq_desc *desc);
> +
>   static inline void writeb_gicd(uint8_t val, unsigned int offset)
>   {
>       writeb_relaxed(val, gicv2.map_dbase + offset);
> @@ -191,6 +194,38 @@ static void gicv2_save_state(struct vcpu *v)
>       writel_gich(0, GICH_HCR);
>   }
>   
> +static void gicv2_save_and_mask_hwppi(struct irq_desc *desc,
> +                                      struct hwppi_state *s)

I would prefer if the two functions are implemented one after each other 
rather than having gic_restore_state() between them. But could we move 
them in place where a forward declaration for gicv2_irq_{enable, 
disable} is not necessary?

While I understand the goal of this function is to be as generic as 
possible, GICv2 interface access is slow and therefore a cost will occur 
for every access. There are also some unwritten rule that interrupt will 
be masked/not active/not pending when the vCPU is created. This rely on 
the vGIC state to be the same. If not, then we are going to be in a 
broken state.

So I would rather prefer if we try to use the vGIC state as much as 
possible and even avoid touching the hardware GIC.

> +{
> +    const uint32_t mask = (1u << desc->irq); /* PPIs are IRQ# 16-31 */

Please use BIT(...).

> +    const uint32_t pendingr = readl_gicd(GICD_ISPENDR);

This is only necessary for edge interrupt.

> +    const uint32_t activer = readl_gicd(GICD_ISACTIVER);
> +    const uint32_t enabler = readl_gicd(GICD_ISENABLER);

If the device is quiescent as suggested below, then masking/unmasking 
the interrupt should not be necessary. This would save a few extra cycle 
here.

> +    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
> +
> +    s->active = !!(activer & mask);
> +    s->enabled = !!(enabler & mask);
> +    s->pending = !!(pendingr & mask);
> +
> +    /* Write a 1 to IC...R to clear the corresponding bit of state */
> +    if ( s->active )
> +        writel_gicd(mask, GICD_ICACTIVER);
> +
> +    /*
> +     * For an edge interrupt clear the pending state, for a level interrupt
> +     * this clears the latch there is no need since saving the peripheral state
> +     * (and/or restoring the next VCPU) will cause the correct action.
> +     */
> +    if ( is_edge && s->pending )
> +        writel_gicd(mask, GICD_ICPENDR);
> +
> +    if ( s->enabled )
> +        gicv2_irq_disable(desc);
> +
> +    ASSERT(!(readl_gicd(GICD_ISACTIVER) & mask));
> +    ASSERT(!(readl_gicd(GICD_ISENABLER) & mask));
> +}
> +
>   static void gicv2_restore_state(const struct vcpu *v)
>   {
>       int i;
> @@ -203,6 +238,38 @@ static void gicv2_restore_state(const struct vcpu *v)
>       writel_gich(GICH_HCR_EN, GICH_HCR);
>   }
>   
> +static void gicv2_restore_hwppi(struct irq_desc *desc,
> +                                const struct hwppi_state *s)
> +{
> +    const uint32_t mask = (1u << desc->irq); /* PPIs are IRQ# 16-31 */
> +    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
> +
> +    /*
> +     * The IRQ must always have been set inactive and masked etc by
> +     * the saving of the previous state via save_and_mask_hwppi.
> +     */
> +    ASSERT(!(readl_gicd(GICD_ISACTIVER) & mask));
> +    ASSERT(!(readl_gicd(GICD_ISENABLER) & mask));
> +
> +    if ( s->active )
> +        writel_gicd(mask, GICD_ICACTIVER);

At least on GICv3, the interrupt may have been deactivated while we were 
unscheduled. So you would restore the wrong active state here.

> +
> +    /*
> +     * Restore pending state for edge triggered interrupts only. For
> +     * level triggered interrupts the level will be restored as
> +     * necessary by restoring the state of the relevant peripheral.
> +     *
> +     * For a level triggered interrupt ISPENDR acts as a *latch* which
> +     * is only cleared by ICPENDR (i.e. the input level is no longer
> +     * relevant). We certainly do not want that here.
> +     */
> +    if ( is_edge && s->pending )
> +        writel_gicd(mask, GICD_ISPENDR);
> +
> +    if ( s->enabled )
> +        gicv2_irq_enable(desc);
> +}
> +
>   static void gicv2_dump_state(const struct vcpu *v)
>   {
>       int i;
> @@ -1335,7 +1402,9 @@ const static struct gic_hw_operations gicv2_ops = {
>       .init                = gicv2_init,
>       .secondary_init      = gicv2_secondary_cpu_init,
>       .save_state          = gicv2_save_state,
> +    .save_and_mask_hwppi = gicv2_save_and_mask_hwppi,
>       .restore_state       = gicv2_restore_state,
> +    .restore_hwppi       = gicv2_restore_hwppi,
>       .dump_state          = gicv2_dump_state,
>       .gic_host_irq_type   = &gicv2_host_irq_type,
>       .gic_guest_irq_type  = &gicv2_guest_irq_type,
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0f6cbf6224..be5ea61ab5 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c

My remarks about GICv2 are valid for GICv3 as well.

> @@ -63,6 +63,9 @@ static DEFINE_PER_CPU(void __iomem*, rbase);
>   #define GICD_RDIST_BASE        (this_cpu(rbase))
>   #define GICD_RDIST_SGI_BASE    (GICD_RDIST_BASE + SZ_64K)
>   
> +static void gicv3_irq_enable(struct irq_desc *desc);
> +static void gicv3_irq_disable(struct irq_desc *desc);
> +
>   /*
>    * Saves all 16(Max) LR registers. Though number of LRs implemented
>    * is implementation specific.
> @@ -375,6 +378,38 @@ static void gicv3_save_state(struct vcpu *v)
>       v->arch.gic.v3.sre_el1 = READ_SYSREG32(ICC_SRE_EL1);
>   }
>   
> +static void gicv3_save_and_mask_hwppi(struct irq_desc *desc,
> +                                      struct hwppi_state *s)
> +{
> +    const uint32_t mask = (1u << desc->irq); /* PPIs are IRQ# 16-31 */
> +    const uint32_t pendingr = readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISPENDR0);
> +    const uint32_t activer = readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISACTIVER0);
> +    const uint32_t enabler = readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISENABLER0);
> +    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
> +
> +    s->active = !!(activer & mask);
> +    s->enabled = !!(enabler & mask);
> +    s->pending = !!(pendingr & mask);
> +
> +    /* Write a 1 to IC...R to clear the corresponding bit of state */
> +    if ( s->active )
> +        writel_relaxed(mask, GICD_RDIST_SGI_BASE + GICR_ICACTIVER0);
> +
> +    /*
> +     * For an edge interrupt clear the pending state, for a level interrupt
> +     * this clears the latch there is no need since saving the peripheral state
> +     * (and/or restoring the next VCPU) will cause the correct action.
> +     */
> +    if ( is_edge && s->pending )
> +        writel_relaxed(mask, GICD_RDIST_SGI_BASE + GICR_ICPENDR0);
> +
> +    if ( s->enabled )
> +        gicv3_irq_disable(desc);
> +
> +    ASSERT(!(readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISACTIVER0) & mask));
> +    ASSERT(!(readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISENABLER0) & mask));
> +}
> +
>   static void gicv3_restore_state(const struct vcpu *v)
>   {
>       uint32_t val;
> @@ -410,6 +445,38 @@ static void gicv3_restore_state(const struct vcpu *v)
>       dsb(sy);
>   }
>   
> +static void gicv3_restore_hwppi(struct irq_desc *desc,
> +                                const struct hwppi_state *s)
> +{
> +    const uint32_t mask = (1u << desc->irq); /* PPIs are IRQ# 16-31 */
> +    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
> +
> +    /*
> +     * The IRQ must always have been set inactive and masked etc by
> +     * the saving of the previous state via save_and_mask_hwppi.
> +     */
> +    ASSERT(!(readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISACTIVER0) & mask));
> +    ASSERT(!(readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISENABLER0) & mask));
> +
> +    if ( s->active )
> +        writel_relaxed(mask, GICD_RDIST_SGI_BASE + GICR_ICACTIVER0);
> +
> +    /*
> +     * Restore pending state for edge triggered interrupts only. For
> +     * level triggered interrupts the level will be restored as
> +     * necessary by restoring the state of the relevant peripheral.
> +     *
> +     * For a level triggered interrupt ISPENDR acts as a *latch* which
> +     * is only cleared by ICPENDR (i.e. the input level is no longer
> +     * relevant). We certainly do not want that here.
> +     */
> +    if ( is_edge && s->pending )
> +        writel_relaxed(mask, GICD_RDIST_SGI_BASE + GICR_ISPENDR0);
> +
> +    if ( s->enabled )
> +        gicv3_irq_enable(desc);
> +}
> +
>   static void gicv3_dump_state(const struct vcpu *v)
>   {
>       int i;
> @@ -1835,7 +1902,9 @@ static const struct gic_hw_operations gicv3_ops = {
>       .info                = &gicv3_info,
>       .init                = gicv3_init,
>       .save_state          = gicv3_save_state,
> +    .save_and_mask_hwppi = gicv3_save_and_mask_hwppi,
>       .restore_state       = gicv3_restore_state,
> +    .restore_hwppi       = gicv3_restore_hwppi,
>       .dump_state          = gicv3_dump_state,
>       .gic_host_irq_type   = &gicv3_host_irq_type,
>       .gic_guest_irq_type  = &gicv3_guest_irq_type,
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 113655a789..75921724dd 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -64,6 +64,17 @@ unsigned int gic_number_lines(void)
>       return gic_hw_ops->info->nr_lines;
>   }
>   
> +void gic_hwppi_state_init(struct hwppi_state *s, unsigned irq)
> +{
> +    memset(s, 0, sizeof(*s));

See above about the unwritten rule here.

> +    s->irq = irq;
> +}
> +
> +void gic_hwppi_set_pending(struct hwppi_state *s)
> +{
> +    s->pending = true;
> +}

I think you want to explain why you need this and not using 
vgic_inject_irq(). I assume this is because setting pending will lead 
the HW to generate the interrupt and therefore deal as the device 
generated it.

However, I am not entirely sure that we really need this (I will comment 
on this in patch #11).

> +
>   void gic_save_state(struct vcpu *v)
>   {
>       ASSERT(!local_irq_is_enabled());
> @@ -78,6 +89,25 @@ void gic_save_state(struct vcpu *v)
>       isb();
>   }
>   
> +void gic_save_and_mask_hwppi(struct vcpu *v, const unsigned virq,
> +                             struct hwppi_state *s)
> +{
> +    struct pending_irq *p = irq_to_pending(v, virq);
> +    struct irq_desc *desc = p->desc;
> +
> +    spin_lock(&desc->lock);
> +
> +    ASSERT(virq >= 16 && virq < 32);
> +    ASSERT(desc->irq >= 16 && desc->irq < 32);
> +    ASSERT(!is_idle_vcpu(v));
> +
> +    s->inprogress = test_and_clear_bit(_IRQ_INPROGRESS, &desc->status);
> +
> +    gic_hw_ops->save_and_mask_hwppi(desc, s);
> +
> +    spin_unlock(&desc->lock);
> +}
> +
>   void gic_restore_state(struct vcpu *v)
>   {
>       ASSERT(!local_irq_is_enabled());
> @@ -89,6 +119,30 @@ void gic_restore_state(struct vcpu *v)
>       isb();
>   }
>   
> +void gic_restore_hwppi(struct vcpu *v,
> +                       const unsigned virq,
> +                       const struct hwppi_state *s)
> +{
> +    struct pending_irq *p = irq_to_pending(v, virq);
> +    struct irq_desc *desc = irq_to_desc(s->irq);
> +
> +    spin_lock(&desc->lock);
> +
> +    ASSERT(virq >= 16 && virq < 32);
> +    ASSERT(!is_idle_vcpu(v));
> +
> +    p->desc = desc; /* Migrate to new physical processor */
> +
> +    irq_set_virq(desc, virq);
> +
> +    gic_hw_ops->restore_hwppi(desc, s);
> +
> +    if ( s->inprogress )
> +        set_bit(_IRQ_INPROGRESS, &desc->status);
> +
> +    spin_unlock(&desc->lock);
> +}
> +
>   /* desc->irq needs to be disabled before calling this function */
>   void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
>   {
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index c80782026f..1a8e599c2e 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -150,6 +150,13 @@ static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc)
>       return desc->action->dev_id;
>   }
>   
> +void irq_set_virq(struct irq_desc *desc, unsigned virq)
> +{
> +    struct irq_guest *info = irq_get_guest_info(desc);
> +    ASSERT(test_bit(_IRQ_PER_CPU, &desc->status));
> +    info->virq = virq;
> +}
> +
>   static inline struct domain *irq_get_domain(struct irq_desc *desc)
>   {
>       return irq_get_guest_info(desc)->d;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index f3f3fb7d7f..c3f4cd5069 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -34,6 +34,17 @@ enum domain_type {
>   /* The hardware domain has always its memory direct mapped. */
>   #define is_domain_direct_mapped(d) ((d) == hardware_domain)
>   
> +struct hwppi_state {
> +    /* h/w state */
> +    unsigned irq;
> +    unsigned long enabled:1;
> +    unsigned long pending:1;
> +    unsigned long active:1;
> +
> +    /* Xen s/w state */
> +    unsigned long inprogress:1;

It would be best if we use bool :1 for all the fields.

> +};
> +
>   struct vtimer {
>       struct vcpu *v;
>       int irq;
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 793d324b33..1164e0c7a6 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -275,6 +275,26 @@ extern int gicv_setup(struct domain *d);
>   extern void gic_save_state(struct vcpu *v);
>   extern void gic_restore_state(struct vcpu *v);
>   
> +/*
> + * Save/restore the state of a single PPI which must be routed to
> + * <current-vcpu> (that is, is defined to be injected to the current
> + * vcpu).
> + *
> + * We expect the device which use this PPI to be quiet while we
> + * save/restore.
> + *
> + * For instance we want to disable the timer before saving the state.
> + * Otherwise we will mess up the state.
> + */
> +struct hwppi_state;
> +extern void gic_hwppi_state_init(struct hwppi_state *s, unsigned irq);
> +extern void gic_hwppi_set_pending(struct hwppi_state *s);
> +extern void gic_save_and_mask_hwppi(struct vcpu *v, unsigned irq,
> +                                    struct hwppi_state *s);
> +extern void gic_restore_hwppi(struct vcpu *v,
> +                              const unsigned virq,
> +                              const struct hwppi_state *s);
> +
>   /* SGI (AKA IPIs) */
>   enum gic_sgi {
>       GIC_SGI_EVENT_CHECK = 0,
> @@ -325,8 +345,10 @@ struct gic_hw_operations {
>       int (*init)(void);
>       /* Save GIC registers */
>       void (*save_state)(struct vcpu *);
> +    void (*save_and_mask_hwppi)(struct irq_desc *desc, struct hwppi_state *s);

Please at least give a brief description of the callback as we did for 
the other one.

>       /* Restore GIC registers */
>       void (*restore_state)(const struct vcpu *);
> +    void (*restore_hwppi)(struct irq_desc *desc, const struct hwppi_state *s);

Ditto.

>       /* Dump GIC LR register information */
>       void (*dump_state)(const struct vcpu *);
>   
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index e14001b5c6..3b37a21c06 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -96,6 +96,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
>    */
>   bool irq_type_set_by_domain(const struct domain *d);
>   
> +void irq_set_virq(struct irq_desc *desc, unsigned virq);

Please document the function.

> +
>   #endif /* _ASM_HW_IRQ_H */
>   /*
>    * Local variables:
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 05/11] xen: arm: add interfaces to save/restore the state of a PPI.

[Stefano Stabellini]

On Fri, 15 Nov 2019, Stewart Hildebrand wrote:
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index f3f3fb7d7f..c3f4cd5069 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -34,6 +34,17 @@ enum domain_type {
>  /* The hardware domain has always its memory direct mapped. */
>  #define is_domain_direct_mapped(d) ((d) == hardware_domain)
>  
> +struct hwppi_state {
> +    /* h/w state */
> +    unsigned irq;

It doesn't look like we need to save the irq number again here.


> +    unsigned long enabled:1;
> +    unsigned long pending:1;
> +    unsigned long active:1;
> +
> +    /* Xen s/w state */
> +    unsigned long inprogress:1;
> +};
> +
>  struct vtimer {
>      struct vcpu *v;
>      int irq;
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 793d324b33..1164e0c7a6 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -275,6 +275,26 @@ extern int gicv_setup(struct domain *d);
>  extern void gic_save_state(struct vcpu *v);
>  extern void gic_restore_state(struct vcpu *v);
>  
> +/*
> + * Save/restore the state of a single PPI which must be routed to
> + * <current-vcpu> (that is, is defined to be injected to the current
> + * vcpu).
> + *
> + * We expect the device which use this PPI to be quiet while we
> + * save/restore.
> + *
> + * For instance we want to disable the timer before saving the state.
> + * Otherwise we will mess up the state.
> + */
> +struct hwppi_state;

It is a bit awkward to have to do this "redefine" struct hwppi_state
here. I know that the Xen headers file don't always include correctly,
but could we move the full definition of struct hwppi_state here?
domain.h is already including gic.h, so it should work?


> +extern void gic_hwppi_state_init(struct hwppi_state *s, unsigned irq);
> +extern void gic_hwppi_set_pending(struct hwppi_state *s);
> +extern void gic_save_and_mask_hwppi(struct vcpu *v, unsigned irq,
> +                                    struct hwppi_state *s);
> +extern void gic_restore_hwppi(struct vcpu *v,
> +                              const unsigned virq,
> +                              const struct hwppi_state *s);
> +
>  /* SGI (AKA IPIs) */
>  enum gic_sgi {
>      GIC_SGI_EVENT_CHECK = 0,
> @@ -325,8 +345,10 @@ struct gic_hw_operations {
>      int (*init)(void);
>      /* Save GIC registers */
>      void (*save_state)(struct vcpu *);
> +    void (*save_and_mask_hwppi)(struct irq_desc *desc, struct hwppi_state *s);
>      /* Restore GIC registers */
>      void (*restore_state)(const struct vcpu *);
> +    void (*restore_hwppi)(struct irq_desc *desc, const struct hwppi_state *s);
>      /* Dump GIC LR register information */
>      void (*dump_state)(const struct vcpu *);
>  
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index e14001b5c6..3b37a21c06 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -96,6 +96,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
>   */
>  bool irq_type_set_by_domain(const struct domain *d);
>  
> +void irq_set_virq(struct irq_desc *desc, unsigned virq);
> +
>  #endif /* _ASM_HW_IRQ_H */
>  /*
>   * Local variables:

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH v3 06/11] Add NR_SGIS and NR_PPIS definitions to irq.h

[Stewart Hildebrand]

These will be used in a follow-up patch.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
---
v3: new patch
---
 xen/include/asm-arm/irq.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 3b37a21c06..367fe6269c 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -33,7 +33,9 @@ struct arch_irq_desc {
     unsigned int type;
 };
 
-#define NR_LOCAL_IRQS	32
+#define NR_SGIS         16
+#define NR_PPIS         16
+#define NR_LOCAL_IRQS   (NR_SGIS + NR_PPIS)
 
 /*
  * This only covers the interrupts that Xen cares about, so SGIs, PPIs and
-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 06/11] Add NR_SGIS and NR_PPIS definitions to irq.h

[Julien Grall]

Hi,

On 15/11/2019 20:10, Stewart Hildebrand wrote:
> These will be used in a follow-up patch.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> ---
> v3: new patch
> ---
>   xen/include/asm-arm/irq.h | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 3b37a21c06..367fe6269c 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -33,7 +33,9 @@ struct arch_irq_desc {
>       unsigned int type;
>   };
>   
> -#define NR_LOCAL_IRQS	32
> +#define NR_SGIS         16
> +#define NR_PPIS         16
> +#define NR_LOCAL_IRQS   (NR_SGIS + NR_PPIS)

We have already NR_GIC_SGI (see include/asm-arm/gic.h) and VGIC_NR_SGIS 
(see include/asm-arm/new_vgic.h).

So I would rather not want to define the same value (with the same 
meaning) a third time.

Note that I am ok if the two existing one are dropped in favor of NR_SGIS.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Stewart Hildebrand]

Allow vgic_get_hw_irq_desc to be called with a vcpu argument.

Use vcpu argument in vgic_connect_hw_irq.

vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
ASSERTs.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>

---
v3: new patch

---
Note: I have only modified the old vgic to allow delivery of PPIs.
---
 xen/arch/arm/gic-vgic.c | 24 ++++++++++++++++--------
 xen/arch/arm/vgic.c     |  6 +++---
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 98c021f1a8..2c66a8fa92 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -418,7 +418,7 @@ struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
 {
     struct pending_irq *p;
 
-    ASSERT(!v && virq >= 32);
+    ASSERT((!v && (virq >= 32)) || (!d && v && (virq >= 16) && (virq < 32)));
 
     if ( !v )
         v = d->vcpu[0];
@@ -434,15 +434,23 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
                         struct irq_desc *desc, bool connect)
 {
     unsigned long flags;
-    /*
-     * Use vcpu0 to retrieve the pending_irq struct. Given that we only
-     * route SPIs to guests, it doesn't make any difference.
-     */
-    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
-    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
-    struct pending_irq *p = irq_to_pending(v_target, virq);
+    struct vcpu *v_target;
+    struct vgic_irq_rank *rank;
+    struct pending_irq *p;
     int ret = 0;
 
+    if (v)
+        v_target = v;
+    else
+        /* Use vcpu0 to retrieve the pending_irq struct. */
+        v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
+
+    rank = vgic_rank_irq(v_target, virq);
+    p = irq_to_pending(v_target, virq);
+
+    ASSERT(virq >= NR_SGIS);
+    ASSERT(p->irq >= NR_SGIS);
+
     /* "desc" is optional when we disconnect an IRQ. */
     ASSERT(!connect || desc);
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 82f524a35c..c3933c2687 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -410,10 +410,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
             irq_set_affinity(p->desc, cpumask_of(v_target->processor));
             spin_lock_irqsave(&p->desc->lock, flags);
             /*
-             * The irq cannot be a PPI, we only support delivery of SPIs
-             * to guests.
+             * The irq cannot be a SGI, we only support delivery of SPIs
+             * and PPIs to guests.
              */
-            ASSERT(irq >= 32);
+            ASSERT(irq >= NR_SGIS);
             if ( irq_type_set_by_domain(d) )
                 gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, i));
             p->desc->handler->enable(p->desc);
-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Julien Grall]

Hi,

On 15/11/2019 20:10, Stewart Hildebrand wrote:
> Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
> 
> Use vcpu argument in vgic_connect_hw_irq.
> 
> vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
> ASSERTs.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> 
> ---
> v3: new patch
> 
> ---
> Note: I have only modified the old vgic to allow delivery of PPIs.

The new vGIC should also be modified to support delivery of PPIs.

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 82f524a35c..c3933c2687 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -410,10 +410,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>               irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>               spin_lock_irqsave(&p->desc->lock, flags);
>               /*
> -             * The irq cannot be a PPI, we only support delivery of SPIs
> -             * to guests.
> +             * The irq cannot be a SGI, we only support delivery of SPIs
> +             * and PPIs to guests.
>                */
> -            ASSERT(irq >= 32);
> +            ASSERT(irq >= NR_SGIS);

We usually put ASSERT() in place we know that code wouldn't be able to 
work correctly if there ASSERT were hit. In this particular case:

>               if ( irq_type_set_by_domain(d) )
>                   gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, i));

1) We don't want to allow any domain (including Dom0) to modify the 
interrupt type (i.e. level/edge) for PPIs as this is shared. You will 
also most likely need to modify the counterpart in setup_guest_irq().

>               p->desc->handler->enable(p->desc);

2) On GICv3, the re-distributor of vCPU A is accessible by vCPU B. So 
vCPU B could enable the SGI for vCPU A. But this would be called on the 
wrong pCPU leading to inconsistency between the hardware state of the 
internal vGIC state.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Julien Grall]

(+ Andre)

On 23/11/2019 20:35, Julien Grall wrote:
> Hi,
> 
> On 15/11/2019 20:10, Stewart Hildebrand wrote:
>> Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
>>
>> Use vcpu argument in vgic_connect_hw_irq.
>>
>> vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
>> ASSERTs.
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
>>
>> ---
>> v3: new patch
>>
>> ---
>> Note: I have only modified the old vgic to allow delivery of PPIs.
> 
> The new vGIC should also be modified to support delivery of PPIs.
> 
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 82f524a35c..c3933c2687 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -410,10 +410,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t 
>> r, int n)
>>               irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>>               spin_lock_irqsave(&p->desc->lock, flags);
>>               /*
>> -             * The irq cannot be a PPI, we only support delivery of SPIs
>> -             * to guests.
>> +             * The irq cannot be a SGI, we only support delivery of SPIs
>> +             * and PPIs to guests.
>>                */
>> -            ASSERT(irq >= 32);
>> +            ASSERT(irq >= NR_SGIS);
> 
> We usually put ASSERT() in place we know that code wouldn't be able to 
> work correctly if there ASSERT were hit. In this particular case:
> 
>>               if ( irq_type_set_by_domain(d) )
>>                   gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, i));
> 
> 1) We don't want to allow any domain (including Dom0) to modify the 
> interrupt type (i.e. level/edge) for PPIs as this is shared. You will 
> also most likely need to modify the counterpart in setup_guest_irq().
> 
>>               p->desc->handler->enable(p->desc);
> 
> 2) On GICv3, the re-distributor of vCPU A is accessible by vCPU B. So 
> vCPU B could enable the SGI for vCPU A. But this would be called on the 
> wrong pCPU leading to inconsistency between the hardware state of the 
> internal vGIC state.

I thought a bit more of the issue over the week-end. The current vGIC is 
fairly messy. I can see two solutions on how to solve this:
     1) Send an IPI to the pCPU where the vCPU A is running and 
disable/enable the interrupt. The other side would need to the vCPU was 
actually running to avoid disabling the PPI for the wrong pCPU
     2) Keep the HW interrupt always enabled

We propagated the enable/disable because of some messy part in the vGIC:
     - vgic_inject_irq() will not queue any pending interrupt if the 
vCPU is offline. While interrupt cannot be delivered, we still need to 
keep them pending as they will never occur again otherwise. This is 
because they are active on the host side and the guest has no way to 
deactivate them.
     - Our implementation of PSCI CPU will remove all pending interrupts 
(see vgic_clear_pending_irqs()). I am not entirely sure the implication 
here because of the previous.

There are a probably more. Aside the issues with it, I don't really see 
good advantage to propagate the interrupt state as the interrupts (PPIs, 
SPIs) have active state. So they can only be received once until the 
guest actually handles it.

So my preference would still be 2) because this makes the code simpler, 
avoid IPI and other potential locking trouble.

On a side note, there are more issues with enable/disable on the current 
vGIC as a pending interrupt already in the LR will not get dropped...

All of this is quite nasty. The sooner the new vGIC is finished the 
sooner we can kill the current one.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Stefano Stabellini]

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

On Mon, 25 Nov 2019, Julien Grall wrote:
> (+ Andre)
> 
> On 23/11/2019 20:35, Julien Grall wrote:
> > Hi,
> > 
> > On 15/11/2019 20:10, Stewart Hildebrand wrote:
> > > Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
> > > 
> > > Use vcpu argument in vgic_connect_hw_irq.
> > > 
> > > vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
> > > ASSERTs.
> > > 
> > > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> > > 
> > > ---
> > > v3: new patch
> > > 
> > > ---
> > > Note: I have only modified the old vgic to allow delivery of PPIs.
> > 
> > The new vGIC should also be modified to support delivery of PPIs.
> > 
> > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > index 82f524a35c..c3933c2687 100644
> > > --- a/xen/arch/arm/vgic.c
> > > +++ b/xen/arch/arm/vgic.c
> > > @@ -410,10 +410,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r,
> > > int n)
> > >               irq_set_affinity(p->desc, cpumask_of(v_target->processor));
> > >               spin_lock_irqsave(&p->desc->lock, flags);
> > >               /*
> > > -             * The irq cannot be a PPI, we only support delivery of SPIs
> > > -             * to guests.
> > > +             * The irq cannot be a SGI, we only support delivery of SPIs
> > > +             * and PPIs to guests.
> > >                */
> > > -            ASSERT(irq >= 32);
> > > +            ASSERT(irq >= NR_SGIS);
> > 
> > We usually put ASSERT() in place we know that code wouldn't be able to work
> > correctly if there ASSERT were hit. In this particular case:
> > 
> > >               if ( irq_type_set_by_domain(d) )
> > >                   gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, i));
> > 
> > 1) We don't want to allow any domain (including Dom0) to modify the
> > interrupt type (i.e. level/edge) for PPIs as this is shared. You will also
> > most likely need to modify the counterpart in setup_guest_irq().
> > 
> > >               p->desc->handler->enable(p->desc);
> > 
> > 2) On GICv3, the re-distributor of vCPU A is accessible by vCPU B. So vCPU B
> > could enable the SGI for vCPU A. But this would be called on the wrong pCPU
> > leading to inconsistency between the hardware state of the internal vGIC
> > state.

Is it actually meant to work from a GIC specification perspective? It
sounds "wrong" somehow to me, but I went through the spec and it doesn't
say explicitly that cpuB couldn't enable a SGI/PPI of cpuA. I am still
a bit shocked by this revelation.

[I haven't had a chance to think carefully about what you wrote below
yet. I'll follow-up.]



> I thought a bit more of the issue over the week-end. The current vGIC is
> fairly messy. I can see two solutions on how to solve this:
>     1) Send an IPI to the pCPU where the vCPU A is running and disable/enable
> the interrupt. The other side would need to the vCPU was actually running to
> avoid disabling the PPI for the wrong pCPU
>     2) Keep the HW interrupt always enabled
> 
> We propagated the enable/disable because of some messy part in the vGIC:
>     - vgic_inject_irq() will not queue any pending interrupt if the vCPU is
> offline. While interrupt cannot be delivered, we still need to keep them
> pending as they will never occur again otherwise. This is because they are
> active on the host side and the guest has no way to deactivate them.
>     - Our implementation of PSCI CPU will remove all pending interrupts (see
> vgic_clear_pending_irqs()). I am not entirely sure the implication here
> because of the previous.
> 
> There are a probably more. Aside the issues with it, I don't really see good
> advantage to propagate the interrupt state as the interrupts (PPIs, SPIs) have
> active state. So they can only be received once until the guest actually
> handles it.
> 
> So my preference would still be 2) because this makes the code simpler, avoid
> IPI and other potential locking trouble.
> 
> On a side note, there are more issues with enable/disable on the current vGIC
> as a pending interrupt already in the LR will not get dropped...
> 
> All of this is quite nasty. The sooner the new vGIC is finished the sooner we
> can kill the current one.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Julien Grall]

Hi,

On 26/11/2019 01:20, Stefano Stabellini wrote:
> On Mon, 25 Nov 2019, Julien Grall wrote:
>> (+ Andre)
>>
>> On 23/11/2019 20:35, Julien Grall wrote:
>>> Hi,
>>>
>>> On 15/11/2019 20:10, Stewart Hildebrand wrote:
>>>> Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
>>>>
>>>> Use vcpu argument in vgic_connect_hw_irq.
>>>>
>>>> vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
>>>> ASSERTs.
>>>>
>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
>>>>
>>>> ---
>>>> v3: new patch
>>>>
>>>> ---
>>>> Note: I have only modified the old vgic to allow delivery of PPIs.
>>>
>>> The new vGIC should also be modified to support delivery of PPIs.
>>>
>>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>>>> index 82f524a35c..c3933c2687 100644
>>>> --- a/xen/arch/arm/vgic.c
>>>> +++ b/xen/arch/arm/vgic.c
>>>> @@ -410,10 +410,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r,
>>>> int n)
>>>>                irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>>>>                spin_lock_irqsave(&p->desc->lock, flags);
>>>>                /*
>>>> -             * The irq cannot be a PPI, we only support delivery of SPIs
>>>> -             * to guests.
>>>> +             * The irq cannot be a SGI, we only support delivery of SPIs
>>>> +             * and PPIs to guests.
>>>>                 */
>>>> -            ASSERT(irq >= 32);
>>>> +            ASSERT(irq >= NR_SGIS);
>>>
>>> We usually put ASSERT() in place we know that code wouldn't be able to work
>>> correctly if there ASSERT were hit. In this particular case:
>>>
>>>>                if ( irq_type_set_by_domain(d) )
>>>>                    gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, i));
>>>
>>> 1) We don't want to allow any domain (including Dom0) to modify the
>>> interrupt type (i.e. level/edge) for PPIs as this is shared. You will also
>>> most likely need to modify the counterpart in setup_guest_irq().
>>>
>>>>                p->desc->handler->enable(p->desc);
>>>
>>> 2) On GICv3, the re-distributor of vCPU A is accessible by vCPU B. So vCPU B
>>> could enable the SGI for vCPU A. But this would be called on the wrong pCPU
>>> leading to inconsistency between the hardware state of the internal vGIC
>>> state.
> 
> Is it actually meant to work from a GIC specification perspective? It
> sounds "wrong" somehow to me, but I went through the spec and it doesn't
> say explicitly that cpuB couldn't enable a SGI/PPI of cpuA. I am still
> a bit shocked by this revelation.

To be honest, I can see reason to allow this but this is a different 
subject.

In this case the re-distributor is per-CPU and can accessible by any 
CPU. For instance, Linux will access it to find the re-distributor 
associated to a given CPU at boot.

FWIW, the vGIC implementation in KVM handles it the same way.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Stefano Stabellini]

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

On Mon, 25 Nov 2019, Julien Grall wrote:
> On 23/11/2019 20:35, Julien Grall wrote:
> > Hi,
> > 
> > On 15/11/2019 20:10, Stewart Hildebrand wrote:
> > > Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
> > > 
> > > Use vcpu argument in vgic_connect_hw_irq.
> > > 
> > > vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
> > > ASSERTs.
> > > 
> > > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> > > 
> > > ---
> > > v3: new patch
> > > 
> > > ---
> > > Note: I have only modified the old vgic to allow delivery of PPIs.
> > 
> > The new vGIC should also be modified to support delivery of PPIs.
> > 
> > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > index 82f524a35c..c3933c2687 100644
> > > --- a/xen/arch/arm/vgic.c
> > > +++ b/xen/arch/arm/vgic.c
> > > @@ -410,10 +410,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r,
> > > int n)
> > >               irq_set_affinity(p->desc, cpumask_of(v_target->processor));
> > >               spin_lock_irqsave(&p->desc->lock, flags);
> > >               /*
> > > -             * The irq cannot be a PPI, we only support delivery of SPIs
> > > -             * to guests.
> > > +             * The irq cannot be a SGI, we only support delivery of SPIs
> > > +             * and PPIs to guests.
> > >                */
> > > -            ASSERT(irq >= 32);
> > > +            ASSERT(irq >= NR_SGIS);
> > 
> > We usually put ASSERT() in place we know that code wouldn't be able to work
> > correctly if there ASSERT were hit. In this particular case:
> > 
> > >               if ( irq_type_set_by_domain(d) )
> > >                   gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, i));
> > 
> > 1) We don't want to allow any domain (including Dom0) to modify the
> > interrupt type (i.e. level/edge) for PPIs as this is shared. You will also
> > most likely need to modify the counterpart in setup_guest_irq().
> > 
> > >               p->desc->handler->enable(p->desc);
> > 
> > 2) On GICv3, the re-distributor of vCPU A is accessible by vCPU B. So vCPU B
> > could enable the SGI for vCPU A. But this would be called on the wrong pCPU
> > leading to inconsistency between the hardware state of the internal vGIC
> > state.
> 
> I thought a bit more of the issue over the week-end. The current vGIC is
> fairly messy. I can see two solutions on how to solve this:
>     1) Send an IPI to the pCPU where the vCPU A is running and disable/enable
> the interrupt. The other side would need to the vCPU was actually running to
> avoid disabling the PPI for the wrong pCPU
>     2) Keep the HW interrupt always enabled
> 
> We propagated the enable/disable because of some messy part in the vGIC:
>     - vgic_inject_irq() will not queue any pending interrupt if the vCPU is
> offline. While interrupt cannot be delivered, we still need to keep them
> pending as they will never occur again otherwise. This is because they are
> active on the host side and the guest has no way to deactivate them.
>     - Our implementation of PSCI CPU will remove all pending interrupts (see
> vgic_clear_pending_irqs()). I am not entirely sure the implication here
> because of the previous.
> 
> There are a probably more. Aside the issues with it, I don't really see good
> advantage to propagate the interrupt state as the interrupts (PPIs, SPIs) have
> active state. So they can only be received once until the guest actually
> handles it.
> 
> So my preference would still be 2) because this makes the code simpler, avoid
> IPI and other potential locking trouble.

Yes, I think that is a good suggestion. I take that you mean that in
vgic_disable_irqs for PPIs we would only clear GIC_IRQ_GUEST_ENABLED
then return basically, right?

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Julien Grall]

On 26/11/2019 22:36, Stefano Stabellini wrote:
> On Mon, 25 Nov 2019, Julien Grall wrote:
>> On 23/11/2019 20:35, Julien Grall wrote:
>>> Hi,
>>>
>>> On 15/11/2019 20:10, Stewart Hildebrand wrote:
>>>> Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
>>>>
>>>> Use vcpu argument in vgic_connect_hw_irq.
>>>>
>>>> vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
>>>> ASSERTs.
>>>>
>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
>>>>
>>>> ---
>>>> v3: new patch
>>>>
>>>> ---
>>>> Note: I have only modified the old vgic to allow delivery of PPIs.
>>>
>>> The new vGIC should also be modified to support delivery of PPIs.
>>>
>>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>>>> index 82f524a35c..c3933c2687 100644
>>>> --- a/xen/arch/arm/vgic.c
>>>> +++ b/xen/arch/arm/vgic.c
>>>> @@ -410,10 +410,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r,
>>>> int n)
>>>>                irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>>>>                spin_lock_irqsave(&p->desc->lock, flags);
>>>>                /*
>>>> -             * The irq cannot be a PPI, we only support delivery of SPIs
>>>> -             * to guests.
>>>> +             * The irq cannot be a SGI, we only support delivery of SPIs
>>>> +             * and PPIs to guests.
>>>>                 */
>>>> -            ASSERT(irq >= 32);
>>>> +            ASSERT(irq >= NR_SGIS);
>>>
>>> We usually put ASSERT() in place we know that code wouldn't be able to work
>>> correctly if there ASSERT were hit. In this particular case:
>>>
>>>>                if ( irq_type_set_by_domain(d) )
>>>>                    gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, i));
>>>
>>> 1) We don't want to allow any domain (including Dom0) to modify the
>>> interrupt type (i.e. level/edge) for PPIs as this is shared. You will also
>>> most likely need to modify the counterpart in setup_guest_irq().
>>>
>>>>                p->desc->handler->enable(p->desc);
>>>
>>> 2) On GICv3, the re-distributor of vCPU A is accessible by vCPU B. So vCPU B
>>> could enable the SGI for vCPU A. But this would be called on the wrong pCPU
>>> leading to inconsistency between the hardware state of the internal vGIC
>>> state.
>>
>> I thought a bit more of the issue over the week-end. The current vGIC is
>> fairly messy. I can see two solutions on how to solve this:
>>      1) Send an IPI to the pCPU where the vCPU A is running and disable/enable
>> the interrupt. The other side would need to the vCPU was actually running to
>> avoid disabling the PPI for the wrong pCPU
>>      2) Keep the HW interrupt always enabled
>>
>> We propagated the enable/disable because of some messy part in the vGIC:
>>      - vgic_inject_irq() will not queue any pending interrupt if the vCPU is
>> offline. While interrupt cannot be delivered, we still need to keep them
>> pending as they will never occur again otherwise. This is because they are
>> active on the host side and the guest has no way to deactivate them.
>>      - Our implementation of PSCI CPU will remove all pending interrupts (see
>> vgic_clear_pending_irqs()). I am not entirely sure the implication here
>> because of the previous.
>>
>> There are a probably more. Aside the issues with it, I don't really see good
>> advantage to propagate the interrupt state as the interrupts (PPIs, SPIs) have
>> active state. So they can only be received once until the guest actually
>> handles it.
>>
>> So my preference would still be 2) because this makes the code simpler, avoid
>> IPI and other potential locking trouble.
> 
> Yes, I think that is a good suggestion. I take that you mean that in
> vgic_disable_irqs for PPIs we would only clear GIC_IRQ_GUEST_ENABLED
> then return basically, right?
Not really, I am only suggesting to remove the part

if ( desc != NULL )
   ...

But this change alone is not enough. It would require some modification 
in the rest of the vGIC (see my previous e-mail) and likely some 
investigation to understand the implication of keeping the interrupt 
enabled from the HW (I am a bit worry we may have backed this assumption 
into other part of the vGIC :().

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Stefano Stabellini]

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

On Tue, 26 Nov 2019, Julien Grall wrote:
> On 26/11/2019 22:36, Stefano Stabellini wrote:
> > On Mon, 25 Nov 2019, Julien Grall wrote:
> > > On 23/11/2019 20:35, Julien Grall wrote:
> > > > Hi,
> > > > 
> > > > On 15/11/2019 20:10, Stewart Hildebrand wrote:
> > > > > Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
> > > > > 
> > > > > Use vcpu argument in vgic_connect_hw_irq.
> > > > > 
> > > > > vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce
> > > > > with
> > > > > ASSERTs.
> > > > > 
> > > > > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> > > > > 
> > > > > ---
> > > > > v3: new patch
> > > > > 
> > > > > ---
> > > > > Note: I have only modified the old vgic to allow delivery of PPIs.
> > > > 
> > > > The new vGIC should also be modified to support delivery of PPIs.
> > > > 
> > > > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > > > index 82f524a35c..c3933c2687 100644
> > > > > --- a/xen/arch/arm/vgic.c
> > > > > +++ b/xen/arch/arm/vgic.c
> > > > > @@ -410,10 +410,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t
> > > > > r,
> > > > > int n)
> > > > >                irq_set_affinity(p->desc,
> > > > > cpumask_of(v_target->processor));
> > > > >                spin_lock_irqsave(&p->desc->lock, flags);
> > > > >                /*
> > > > > -             * The irq cannot be a PPI, we only support delivery of
> > > > > SPIs
> > > > > -             * to guests.
> > > > > +             * The irq cannot be a SGI, we only support delivery of
> > > > > SPIs
> > > > > +             * and PPIs to guests.
> > > > >                 */
> > > > > -            ASSERT(irq >= 32);
> > > > > +            ASSERT(irq >= NR_SGIS);
> > > > 
> > > > We usually put ASSERT() in place we know that code wouldn't be able to
> > > > work
> > > > correctly if there ASSERT were hit. In this particular case:
> > > > 
> > > > >                if ( irq_type_set_by_domain(d) )
> > > > >                    gic_set_irq_type(p->desc, vgic_get_virq_type(v, n,
> > > > > i));
> > > > 
> > > > 1) We don't want to allow any domain (including Dom0) to modify the
> > > > interrupt type (i.e. level/edge) for PPIs as this is shared. You will
> > > > also
> > > > most likely need to modify the counterpart in setup_guest_irq().
> > > > 
> > > > >                p->desc->handler->enable(p->desc);
> > > > 
> > > > 2) On GICv3, the re-distributor of vCPU A is accessible by vCPU B. So
> > > > vCPU B
> > > > could enable the SGI for vCPU A. But this would be called on the wrong
> > > > pCPU
> > > > leading to inconsistency between the hardware state of the internal vGIC
> > > > state.
> > > 
> > > I thought a bit more of the issue over the week-end. The current vGIC is
> > > fairly messy. I can see two solutions on how to solve this:
> > >      1) Send an IPI to the pCPU where the vCPU A is running and
> > > disable/enable
> > > the interrupt. The other side would need to the vCPU was actually running
> > > to
> > > avoid disabling the PPI for the wrong pCPU
> > >      2) Keep the HW interrupt always enabled
> > > 
> > > We propagated the enable/disable because of some messy part in the vGIC:
> > >      - vgic_inject_irq() will not queue any pending interrupt if the vCPU
> > > is
> > > offline. While interrupt cannot be delivered, we still need to keep them
> > > pending as they will never occur again otherwise. This is because they are
> > > active on the host side and the guest has no way to deactivate them.
> > >      - Our implementation of PSCI CPU will remove all pending interrupts
> > > (see
> > > vgic_clear_pending_irqs()). I am not entirely sure the implication here
> > > because of the previous.
> > > 
> > > There are a probably more. Aside the issues with it, I don't really see
> > > good
> > > advantage to propagate the interrupt state as the interrupts (PPIs, SPIs)
> > > have
> > > active state. So they can only be received once until the guest actually
> > > handles it.
> > > 
> > > So my preference would still be 2) because this makes the code simpler,
> > > avoid
> > > IPI and other potential locking trouble.
> > 
> > Yes, I think that is a good suggestion. I take that you mean that in
> > vgic_disable_irqs for PPIs we would only clear GIC_IRQ_GUEST_ENABLED
> > then return basically, right?
> Not really, I am only suggesting to remove the part
> 
> if ( desc != NULL )
>   ...

I think we are saying the same thing


> But this change alone is not enough. It would require some modification in the
> rest of the vGIC (see my previous e-mail) and likely some investigation to
> understand the implication of keeping the interrupt enabled from the HW (I am
> a bit worry we may have backed this assumption into other part of the vGIC
> :().

I can see that at least save_and_mask_hwppi and restore_hwppi would need
to be modified to account for the fact that GICD_ISENABLER would say "it
is enabled" but actually GIC_IRQ_GUEST_ENABLED is unset.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Julien Grall]

Hi,

On 27/11/2019 18:48, Stefano Stabellini wrote:
>>>
>>> Yes, I think that is a good suggestion. I take that you mean that in
>>> vgic_disable_irqs for PPIs we would only clear GIC_IRQ_GUEST_ENABLED
>>> then return basically, right?
>> Not really, I am only suggesting to remove the part
>>
>> if ( desc != NULL )
>>    ...
> 
> I think we are saying the same thing

The function is doing a bit more, hence why I wasn't not sure :).

>> But this change alone is not enough. It would require some modification in the
>> rest of the vGIC (see my previous e-mail) and likely some investigation to
>> understand the implication of keeping the interrupt enabled from the HW (I am
>> a bit worry we may have backed this assumption into other part of the vGIC
>> :().
> 
> I can see that at least save_and_mask_hwppi and restore_hwppi would need
> to be modified to account for the fact that GICD_ISENABLER would say "it
> is enabled" but actually GIC_IRQ_GUEST_ENABLED is unset.
It depends how we decide to implement the two functions. We may want to 
decouple the GIC completely the GIC state from the vGIC state. For 
instance, you may still want to mask the interrupt regardless of the 
vGIC state when the vCPU is scheduled out. This would prevent a 
non-quiescent device to generate interrupt while we can't deal with them.

But as we seem to consider the device will be quiescent and also clear 
the pending bit, then I think we can completely avoid to mask/unmask the 
interrupt. This would save a couple of access to the GIC interface.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Stefano Stabellini]

On Fri, 15 Nov 2019, Stewart Hildebrand wrote:
> Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
> 
> Use vcpu argument in vgic_connect_hw_irq.
> 
> vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
> ASSERTs.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> 
> ---
> v3: new patch
> 
> ---
> Note: I have only modified the old vgic to allow delivery of PPIs.
> ---
>  xen/arch/arm/gic-vgic.c | 24 ++++++++++++++++--------
>  xen/arch/arm/vgic.c     |  6 +++---
>  2 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 98c021f1a8..2c66a8fa92 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -418,7 +418,7 @@ struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
>  {
>      struct pending_irq *p;
>  
> -    ASSERT(!v && virq >= 32);
> +    ASSERT((!v && (virq >= 32)) || (!d && v && (virq >= 16) && (virq < 32)));

I don't think !d is necessary for this to work as intended so I would
limit the ASSERT to

  ASSERT((!v && (virq >= 32)) || (v && (virq >= 16) && (virq < 32)));

the caller can always pass v->domain


>      if ( !v )
>          v = d->vcpu[0];

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Julien Grall]

[-- Attachment #1.1: Type: text/plain, Size: 1690 bytes --]

On Tue, 26 Nov 2019, 23:18 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Fri, 15 Nov 2019, Stewart Hildebrand wrote:
> > Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
> >
> > Use vcpu argument in vgic_connect_hw_irq.
> >
> > vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
> > ASSERTs.
> >
> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> >
> > ---
> > v3: new patch
> >
> > ---
> > Note: I have only modified the old vgic to allow delivery of PPIs.
> > ---
> >  xen/arch/arm/gic-vgic.c | 24 ++++++++++++++++--------
> >  xen/arch/arm/vgic.c     |  6 +++---
> >  2 files changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> > index 98c021f1a8..2c66a8fa92 100644
> > --- a/xen/arch/arm/gic-vgic.c
> > +++ b/xen/arch/arm/gic-vgic.c
> > @@ -418,7 +418,7 @@ struct irq_desc *vgic_get_hw_irq_desc(struct domain
> *d, struct vcpu *v,
> >  {
> >      struct pending_irq *p;
> >
> > -    ASSERT(!v && virq >= 32);
> > +    ASSERT((!v && (virq >= 32)) || (!d && v && (virq >= 16) && (virq <
> 32)));
>
> I don't think !d is necessary for this to work as intended so I would
> limit the ASSERT to
>
>   ASSERT((!v && (virq >= 32)) || (v && (virq >= 16) && (virq < 32)));
>
> the caller can always pass v->domain
>

But then you have the risk to run into d != v->domain. So at least with the
ASSERT you document the expectation.

Cheers,


>
> >      if ( !v )
> >          v = d->vcpu[0];
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

[-- Attachment #1.2: Type: text/html, Size: 2907 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Stefano Stabellini]

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

On Wed, 27 Nov 2019, Julien Grall wrote:
> On Tue, 26 Nov 2019, 23:18 Stefano Stabellini, <sstabellini@kernel.org> wrote:
>       On Fri, 15 Nov 2019, Stewart Hildebrand wrote:
>       > Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
>       >
>       > Use vcpu argument in vgic_connect_hw_irq.
>       >
>       > vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
>       > ASSERTs.
>       >
>       > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
>       >
>       > ---
>       > v3: new patch
>       >
>       > ---
>       > Note: I have only modified the old vgic to allow delivery of PPIs.
>       > ---
>       >  xen/arch/arm/gic-vgic.c | 24 ++++++++++++++++--------
>       >  xen/arch/arm/vgic.c     |  6 +++---
>       >  2 files changed, 19 insertions(+), 11 deletions(-)
>       >
>       > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>       > index 98c021f1a8..2c66a8fa92 100644
>       > --- a/xen/arch/arm/gic-vgic.c
>       > +++ b/xen/arch/arm/gic-vgic.c
>       > @@ -418,7 +418,7 @@ struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
>       >  {
>       >      struct pending_irq *p;
>       > 
>       > -    ASSERT(!v && virq >= 32);
>       > +    ASSERT((!v && (virq >= 32)) || (!d && v && (virq >= 16) && (virq < 32)));
> 
>       I don't think !d is necessary for this to work as intended so I would
>       limit the ASSERT to
> 
>         ASSERT((!v && (virq >= 32)) || (v && (virq >= 16) && (virq < 32)));
> 
>       the caller can always pass v->domain
> 
> But then you have the risk to run into d != v->domain. So at least with the ASSERT you document the expectation.

Yes, that was not my intention.

It makes sense in certain scenarios for v to be NULL. What I was trying
to say is that when v is not-NULL, then also d should be not-NULL for
consistency. I don't think it makes sense to pass v corresponding to
vcpu1 of domain2 and d == NULL, right?

I don't know if you want to add a (d == v->domain) check to the ASSERT
as it is pretty busy already. I am OK either way.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Julien Grall]

On 28/11/2019 01:07, Stefano Stabellini wrote:
> On Wed, 27 Nov 2019, Julien Grall wrote:
>> On Tue, 26 Nov 2019, 23:18 Stefano Stabellini, <sstabellini@kernel.org> wrote:
>>        On Fri, 15 Nov 2019, Stewart Hildebrand wrote:
>>        > Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
>>        >
>>        > Use vcpu argument in vgic_connect_hw_irq.
>>        >
>>        > vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
>>        > ASSERTs.
>>        >
>>        > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
>>        >
>>        > ---
>>        > v3: new patch
>>        >
>>        > ---
>>        > Note: I have only modified the old vgic to allow delivery of PPIs.
>>        > ---
>>        >  xen/arch/arm/gic-vgic.c | 24 ++++++++++++++++--------
>>        >  xen/arch/arm/vgic.c     |  6 +++---
>>        >  2 files changed, 19 insertions(+), 11 deletions(-)
>>        >
>>        > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>>        > index 98c021f1a8..2c66a8fa92 100644
>>        > --- a/xen/arch/arm/gic-vgic.c
>>        > +++ b/xen/arch/arm/gic-vgic.c
>>        > @@ -418,7 +418,7 @@ struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
>>        >  {
>>        >      struct pending_irq *p;
>>        >
>>        > -    ASSERT(!v && virq >= 32);
>>        > +    ASSERT((!v && (virq >= 32)) || (!d && v && (virq >= 16) && (virq < 32)));
>>
>>        I don't think !d is necessary for this to work as intended so I would
>>        limit the ASSERT to
>>
>>          ASSERT((!v && (virq >= 32)) || (v && (virq >= 16) && (virq < 32)));
>>
>>        the caller can always pass v->domain
>>
>> But then you have the risk to run into d != v->domain. So at least with the ASSERT you document the expectation.
> 
> Yes, that was not my intention.
> 
> It makes sense in certain scenarios for v to be NULL. What I was trying
> to say is that when v is not-NULL, then also d should be not-NULL for
> consistency. I don't think it makes sense to pass v corresponding to
> vcpu1 of domain2 and d == NULL, right?

While I usually like consistency, 'd' is only used to find 'v' if it is 
NULL. So I really see limited reason to impose the caller to set 'd' in 
this case.

> 
> I don't know if you want to add a (d == v->domain) check to the ASSERT
> as it is pretty busy already. I am OK either way.
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [RFC XEN PATCH v3 08/11] xen: arm: vgic: don't fail if IRQ is already connected

[Stewart Hildebrand]

There are some IRQs that happen to have multiple "interrupts = < ... >;"
properties with the same IRQ in the device tree. For example:

interrupts = <0 123 4>,
             <0 123 4>,
             <0 123 4>,
             <0 123 4>,
             <0 123 4>;

In this case it seems that we are invoking vgic_connect_hw_irq multiple
times for the same IRQ.

Rework the checks to allow booting in this scenario.

I have not seen any cases where the pre-existing p->desc is any different from
the new desc, so BUG() out if they're different for now.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>

---
v3: new patch

I tested on Xilinx Zynq UltraScale+ with the old vGIC. I have not fully
tested with CONFIG_NEW_VGIC. This hack only became necessary after
introducing the PPI series, and I'm not entirely sure what the reason
is for that.

I'm also unsure if BUG()ing out is the right thing to do in case of
desc != p->desc, or what conditions would even trigger this? Is this
function exposed to guests?
---
 xen/arch/arm/gic-vgic.c  | 9 +++++++--
 xen/arch/arm/vgic/vgic.c | 4 ++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 2c66a8fa92..5c16e66b32 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -460,9 +460,14 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
     if ( connect )
     {
         /* The VIRQ should not be already enabled by the guest */
-        if ( !p->desc &&
-             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+        if ( !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+        {
+            if (p->desc && p->desc != desc)
+            {
+                BUG();
+            }
             p->desc = desc;
+        }
         else
             ret = -EBUSY;
     }
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index f0f2ea5021..aa775f7668 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -882,6 +882,10 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
             irq->hw = true;
             irq->hwintid = desc->irq;
         }
+        else if ( irq->hw && !irq->enabled && irq->hwintid == desc->irq )
+        {
+            /* The IRQ was already connected. No action is necessary. */
+        }
         else
             ret = -EBUSY;
     }
-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC XEN PATCH v3 08/11] xen: arm: vgic: don't fail if IRQ is already connected

[Stefano Stabellini]

On Fri, 15 Nov 2019, Stewart Hildebrand wrote:
> There are some IRQs that happen to have multiple "interrupts = < ... >;"
> properties with the same IRQ in the device tree. For example:
> 
> interrupts = <0 123 4>,
>              <0 123 4>,
>              <0 123 4>,
>              <0 123 4>,
>              <0 123 4>;
> 
> In this case it seems that we are invoking vgic_connect_hw_irq multiple
> times for the same IRQ.
> 
> Rework the checks to allow booting in this scenario.
> 
> I have not seen any cases where the pre-existing p->desc is any different from
> the new desc, so BUG() out if they're different for now.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> 
> ---
> v3: new patch
> 
> I tested on Xilinx Zynq UltraScale+ with the old vGIC. I have not fully
> tested with CONFIG_NEW_VGIC. This hack only became necessary after
> introducing the PPI series, and I'm not entirely sure what the reason
> is for that.

I think the reason is actually very simple: with the previous code if
the irq was already setup and the details matched it would "goto out"
all the way out of route_irq_to_guest.

Now with the new code, it would "goto out" of setup_guest_irq returning
zero, which means that gic_route_irq_to_guest is actually going to be
called anyway, which is a mistake. I think we want to avoid that by
returning an appropriate error condition from setup_guest_irq so that we
also return early from route_irq_to_guest.


> I'm also unsure if BUG()ing out is the right thing to do in case of
> desc != p->desc, or what conditions would even trigger this? Is this
> function exposed to guests?

I think the original code printed a warning and returned an error.
That's probably still what we want.



> ---
>  xen/arch/arm/gic-vgic.c  | 9 +++++++--
>  xen/arch/arm/vgic/vgic.c | 4 ++++
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 2c66a8fa92..5c16e66b32 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -460,9 +460,14 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
>      if ( connect )
>      {
>          /* The VIRQ should not be already enabled by the guest */
> -        if ( !p->desc &&
> -             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> +        if ( !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> +        {
> +            if (p->desc && p->desc != desc)

Code style


> +            {
> +                BUG();
> +            }
>              p->desc = desc;
> +        }
>          else
>              ret = -EBUSY;
>      }
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index f0f2ea5021..aa775f7668 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -882,6 +882,10 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
>              irq->hw = true;
>              irq->hwintid = desc->irq;
>          }
> +        else if ( irq->hw && !irq->enabled && irq->hwintid == desc->irq )
> +        {
> +            /* The IRQ was already connected. No action is necessary. */
> +        }
>          else
>              ret = -EBUSY;
>      }


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC XEN PATCH v3 08/11] xen: arm: vgic: don't fail if IRQ is already connected

[Julien Grall]

Hi,

On 15/11/2019 20:10, Stewart Hildebrand wrote:
> There are some IRQs that happen to have multiple "interrupts = < ... >;"
> properties with the same IRQ in the device tree. For example:
> 
> interrupts = <0 123 4>,
>               <0 123 4>,
>               <0 123 4>,
>               <0 123 4>,
>               <0 123 4>;
> 
> In this case it seems that we are invoking vgic_connect_hw_irq multiple
> times for the same IRQ.
> 
> Rework the checks to allow booting in this scenario.
> 
> I have not seen any cases where the pre-existing p->desc is any different from
> the new desc, so BUG() out if they're different for now.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> 
> ---
> v3: new patch
> 
> I tested on Xilinx Zynq UltraScale+ with the old vGIC. I have not fully
> tested with CONFIG_NEW_VGIC. This hack only became necessary after
> introducing the PPI series, and I'm not entirely sure what the reason
> is for that.
> 
> I'm also unsure if BUG()ing out is the right thing to do in case of
> desc != p->desc, or what conditions would even trigger this? Is this
> function exposed to guests?

This can happen with PPIs as the desc is per-CPU. If you migrate the 
vCPU to another pCPU, you will likely hit the BUG() below if the guest 
disabled the interrupt.

But I don't think we should call vgic_connect_hw_irq() multiples time on 
the same IRQ. The upper layer should take care of such issue (such as 
setup_guest_irq()).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH v3 09/11] xen: arm: gic: supporting routing a PPI to the current vcpu.

[Stewart Hildebrand]

From: Ian Campbell <ian.campbell@citrix.com>

That is whichever vcpu is resident when the interrupt fires. An
interrupt is in this state when both IRQ_GUEST and IRQ_PER_CPU are set
in the descriptor status. Only PPIs can be in this mode.

This requires some peripheral specific code to make use of the
previously introduced functionality to save and restore the PPI state.
The vtimer driver will do so shortly.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>

---
v3:
  * Change calls to gic_set_irq_properties() to gic_set_irq_type() and
    gic_set_irq_priority() due to following commits:
    16580cde5a xen/arm: gic: Do not configure affinity during routing
    23e8118b8e xen/arm: gic: split set_irq_properties
  * Partially address feedback from v2 [1]:
  * Clarify a comment.
  * Switch loglevel back to XENLOG_G_ERR and bump a parameter to the
    next line to comply with line length coding style.
  * Call vgic_get_hw_irq_desc from gic_save_and_mask_hwppi
  * Call vgic_connect_hw_irq from gic_restore_hwppi

---
Note: I have not yet addressed feedback from [1] regarding
differentiating between CPU0/CPU1 in the error message.

I also have not yet given much thought to Julien's comment in [1] "Why
do you set the parameter virq to irq?"

I hope to investigate further if time allows, but if anyone has any
input I'd like to hear it.

[1] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01064.html
---
 xen/arch/arm/gic.c        | 33 ++++++++++++++--
 xen/arch/arm/irq.c        | 80 +++++++++++++++++++++++++++++++++++----
 xen/include/asm-arm/gic.h |  2 +
 xen/include/asm-arm/irq.h |  1 +
 4 files changed, 104 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 75921724dd..982afaadbd 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -92,8 +92,7 @@ void gic_save_state(struct vcpu *v)
 void gic_save_and_mask_hwppi(struct vcpu *v, const unsigned virq,
                              struct hwppi_state *s)
 {
-    struct pending_irq *p = irq_to_pending(v, virq);
-    struct irq_desc *desc = p->desc;
+    struct irq_desc *desc = vgic_get_hw_irq_desc(NULL, v, virq);
 
     spin_lock(&desc->lock);
 
@@ -123,7 +122,6 @@ void gic_restore_hwppi(struct vcpu *v,
                        const unsigned virq,
                        const struct hwppi_state *s)
 {
-    struct pending_irq *p = irq_to_pending(v, virq);
     struct irq_desc *desc = irq_to_desc(s->irq);
 
     spin_lock(&desc->lock);
@@ -131,7 +129,8 @@ void gic_restore_hwppi(struct vcpu *v,
     ASSERT(virq >= 16 && virq < 32);
     ASSERT(!is_idle_vcpu(v));
 
-    p->desc = desc; /* Migrate to new physical processor */
+    /* Migrate to new physical processor */
+    vgic_connect_hw_irq(v->domain, v, virq, desc, true);
 
     irq_set_virq(desc, virq);
 
@@ -178,6 +177,32 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
     gic_set_irq_priority(desc, priority);
 }
 
+/*
+ * Program the GIC to route an interrupt to the current guest.
+ *
+ * That is, the IRQ is delivered to whichever VCPU happens to be
+ * resident on the PCPU when the interrupt arrives.
+ *
+ * Currently the interrupt *must* be a PPI and the code responsible
+ * for the related hardware must save and restore the PPI with
+ * gic_save_and_mask_hwppi/gic_restore_hwppi.
+ */
+int gic_route_irq_to_current_guest(struct irq_desc *desc,
+                                   unsigned int priority)
+{
+    ASSERT(spin_is_locked(&desc->lock));
+    ASSERT(desc->irq >= 16 && desc->irq < 32);
+
+    desc->handler = gic_hw_ops->gic_guest_irq_type;
+    set_bit(_IRQ_GUEST, &desc->status);
+    set_bit(_IRQ_PER_CPU, &desc->status);
+
+    gic_set_irq_type(desc, desc->arch.type);
+    gic_set_irq_priority(desc, GIC_PRI_IRQ);
+
+    return 0;
+}
+
 /* Program the GIC to route an interrupt to a guest
  *   - desc.lock must be held
  */
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 1a8e599c2e..17dec64203 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -236,6 +236,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
     if ( test_bit(_IRQ_GUEST, &desc->status) )
     {
         struct irq_guest *info = irq_get_guest_info(desc);
+        struct vcpu *v;
 
         perfc_incr(guest_irqs);
         desc->handler->end(desc);
@@ -243,10 +244,15 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
         set_bit(_IRQ_INPROGRESS, &desc->status);
 
         /*
-         * The irq cannot be a PPI, we only support delivery of SPIs to
-         * guests.
+         * A PPI exposed to a guest must always be in IRQ_GUEST|IRQ_PER_CPU
+         * mode ("route to active VCPU"), so we use current.
+         *
+         * For SPI, we use NULL. In this case, vgic_inject_irq() will look up
+         * the required target for delivery to a specific guest.
          */
-        vgic_inject_irq(info->d, NULL, info->virq, true);
+        v = test_bit(_IRQ_PER_CPU, &desc->status) ? current : NULL;
+        vgic_inject_irq(info->d, v, info->virq, true);
+
         goto out_no_end;
     }
 
@@ -362,11 +368,15 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
 
     if ( test_bit(_IRQ_GUEST, &desc->status) )
     {
-        struct domain *d = irq_get_domain(desc);
+        struct irq_guest *info = irq_get_guest_info(desc);
 
         spin_unlock_irqrestore(&desc->lock, flags);
-        printk(XENLOG_ERR "ERROR: IRQ %u is already in use by the domain %u\n",
-               irq, d->domain_id);
+        if ( !test_bit(_IRQ_PER_CPU, &desc->status) )
+            printk(XENLOG_ERR "ERROR: IRQ %u is already in use by domain %u\n",
+                   irq, info->d->domain_id);
+        else
+            printk(XENLOG_ERR
+                   "ERROR: IRQ %u is already in use by <current-vcpu>\n", irq);
         return -EBUSY;
     }
 
@@ -450,8 +460,14 @@ static int setup_guest_irq(struct irq_desc *desc, unsigned int virq,
 
             if ( d != ad )
             {
-                printk(XENLOG_G_ERR "IRQ %u is already used by domain %u\n",
-                       irq, ad->domain_id);
+                if ( !test_bit(_IRQ_PER_CPU, &desc->status) )
+                    printk(XENLOG_G_ERR
+                           "ERROR: IRQ %u is already used by domain %u\n",
+                           irq, ad->domain_id);
+                else
+                    printk(XENLOG_G_ERR
+                           "ERROR: IRQ %u is already used by <current-vcpu>\n",
+                           irq);
                 retval = -EBUSY;
             }
             else if ( irq_get_guest_info(desc)->virq != virq )
@@ -552,6 +568,54 @@ free_info:
     return retval;
 }
 
+/*
+ * Route a PPI such that it is always delivered to the current vcpu on
+ * the pcpu. The driver for the peripheral must use
+ * gic_{save_and_mask,restore}_hwppi as part of the context switch.
+ */
+int route_hwppi_to_current_vcpu(unsigned int irq, const char *devname)
+{
+    struct irq_guest *info;
+    struct irq_desc *desc;
+    unsigned long flags;
+    int retval = 0;
+
+    /* Can only route PPIs to current VCPU */
+    if ( irq < 16 || irq >= 32 )
+        return -EINVAL;
+
+    desc = irq_to_desc(irq);
+
+    info = xmalloc(struct irq_guest);
+    if ( !info )
+        return -ENOMEM;
+
+    info->d = NULL; /* Routed to current vcpu, so no specific domain */
+    /* info->virq is set by gic_restore_hwppi. */
+
+    spin_lock_irqsave(&desc->lock, flags);
+
+    retval = setup_guest_irq(desc, irq, flags, info, devname);
+    if ( retval )
+    {
+        xfree(info);
+        return retval;
+    }
+
+    retval = gic_route_irq_to_current_guest(desc, GIC_PRI_IRQ);
+
+    spin_unlock_irqrestore(&desc->lock, flags);
+
+    if ( retval )
+    {
+        release_irq(desc->irq, info);
+        xfree(info);
+        return retval;
+    }
+
+    return 0;
+}
+
 int release_guest_irq(struct domain *d, unsigned int virq)
 {
     struct irq_desc *desc;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 1164e0c7a6..6a0910e13e 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -244,6 +244,8 @@ extern void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority);
 extern int gic_route_irq_to_guest(struct domain *, unsigned int virq,
                                   struct irq_desc *desc,
                                   unsigned int priority);
+int gic_route_irq_to_current_guest(struct irq_desc *desc,
+                                   unsigned int priority);
 
 /* Remove an IRQ passthrough to a guest */
 int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 367fe6269c..c51265180b 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -77,6 +77,7 @@ void init_secondary_IRQ(void);
 
 int route_irq_to_guest(struct domain *d, unsigned int virq,
                        unsigned int irq, const char *devname);
+int route_hwppi_to_current_vcpu(unsigned int irq, const char *devname);
 int release_guest_irq(struct domain *d, unsigned int irq);
 
 void arch_move_irqs(struct vcpu *v);
-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 09/11] xen: arm: gic: supporting routing a PPI to the current vcpu.

[Stewart Hildebrand]

On Friday, November 15, 2019 3:11 PM, Stewart Hildebrand wrote:
>diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>index 75921724dd..982afaadbd 100644
>--- a/xen/arch/arm/gic.c
>+++ b/xen/arch/arm/gic.c
>@@ -92,8 +92,7 @@ void gic_save_state(struct vcpu *v)
> void gic_save_and_mask_hwppi(struct vcpu *v, const unsigned virq,
>                              struct hwppi_state *s)
> {
>-    struct pending_irq *p = irq_to_pending(v, virq);
>-    struct irq_desc *desc = p->desc;
>+    struct irq_desc *desc = vgic_get_hw_irq_desc(NULL, v, virq);
>
>     spin_lock(&desc->lock);
>
>@@ -123,7 +122,6 @@ void gic_restore_hwppi(struct vcpu *v,
>                        const unsigned virq,
>                        const struct hwppi_state *s)
> {
>-    struct pending_irq *p = irq_to_pending(v, virq);
>     struct irq_desc *desc = irq_to_desc(s->irq);
>
>     spin_lock(&desc->lock);
>@@ -131,7 +129,8 @@ void gic_restore_hwppi(struct vcpu *v,
>     ASSERT(virq >= 16 && virq < 32);
>     ASSERT(!is_idle_vcpu(v));
>
>-    p->desc = desc; /* Migrate to new physical processor */
>+    /* Migrate to new physical processor */
>+    vgic_connect_hw_irq(v->domain, v, virq, desc, true);
>
>     irq_set_virq(desc, virq);
>

This snippet was intended to be rolled into [XEN PATCH v3 05/11] xen:
arm: add interfaces to save/restore the state of a PPI.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [RFC XEN PATCH v3 10/11] xen: arm: context switch vtimer PPI state.

[Stewart Hildebrand]

From: Ian Campbell <ian.campbell@citrix.com>

... instead of artificially masking the timer interrupt in the timer
state and relying on the guest to unmask (which it isn't required to
do per the h/w spec, although Linux does).

By using the newly added hwppi save/restore functionality we make use
of the GICD I[SC]ACTIVER registers to save and restore the active
state of the interrupt, which prevents the nested invocations which
the current masking works around.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
---
v2: Rebased, in particular over Julien's passthrough stuff which
    reworked a bunch of IRQ related stuff.
    Also largely rewritten since precursor patches now lay very
    different groundwork.

v3: Address feedback from v2 [1]:
  * Remove virt_timer_irqs performance counter since it is now unused.
  * Add caveat to comment about not using I*ACTIVER register.
  * HACK: don't initialize pending_irq->irq in vtimer for new vGIC to
    allows us to build with CONFIG_NEW_VGIC=y

[1] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01065.html
---

Note: Regarding Stefano's comment in [2], I did test it with the call
to gic_hwppi_set_pending removed, and I was able to boot dom0.

[2] https://lists.xenproject.org/archives/html/xen-devel/2015-12/msg02683.html
---
 xen/arch/arm/time.c              | 26 ++----------------
 xen/arch/arm/vtimer.c            | 45 +++++++++++++++++++++++++++++---
 xen/include/asm-arm/domain.h     |  1 +
 xen/include/asm-arm/perfc_defn.h |  1 -
 4 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 739bcf186c..e3a23b8e16 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -243,28 +243,6 @@ static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
     }
 }
 
-static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
-{
-    /*
-     * Edge-triggered interrupts can be used for the virtual timer. Even
-     * if the timer output signal is masked in the context switch, the
-     * GIC will keep track that of any interrupts raised while IRQS are
-     * disabled. As soon as IRQs are re-enabled, the virtual interrupt
-     * will be injected to Xen.
-     *
-     * If an IDLE vCPU was scheduled next then we should ignore the
-     * interrupt.
-     */
-    if ( unlikely(is_idle_vcpu(current)) )
-        return;
-
-    perfc_incr(virt_timer_irqs);
-
-    current->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0);
-    WRITE_SYSREG32(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL_EL0);
-    vgic_inject_irq(current->domain, current, current->arch.virt_timer.irq, true);
-}
-
 /*
  * Arch timer interrupt really ought to be level triggered, since the
  * design of the timer/comparator mechanism is based around that
@@ -304,8 +282,8 @@ void init_timer_interrupt(void)
 
     request_irq(timer_irq[TIMER_HYP_PPI], 0, timer_interrupt,
                 "hyptimer", NULL);
-    request_irq(timer_irq[TIMER_VIRT_PPI], 0, vtimer_interrupt,
-                   "virtimer", NULL);
+    route_hwppi_to_current_vcpu(timer_irq[TIMER_VIRT_PPI], "virtimer");
+
     request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], 0, timer_interrupt,
                 "phytimer", NULL);
 
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index e6aebdac9e..6e3498952d 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -55,9 +55,19 @@ static void phys_timer_expired(void *data)
 static void virt_timer_expired(void *data)
 {
     struct vtimer *t = data;
-    t->ctl |= CNTx_CTL_MASK;
-    vgic_inject_irq(t->v->domain, t->v, t->irq, true);
-    perfc_incr(vtimer_virt_inject);
+    t->ctl |= CNTx_CTL_PENDING;
+    if ( !(t->ctl & CNTx_CTL_MASK) )
+    {
+        /*
+         * An edge triggered interrupt should now be pending. Since
+         * this timer can never expire while the domain is scheduled
+         * we know that the gic_restore_hwppi in virt_timer_restore
+         * will cause the real hwppi to occur and be routed.
+         */
+        gic_hwppi_set_pending(&t->ppi_state);
+        vcpu_unblock(t->v);
+        perfc_incr(vtimer_virt_inject);
+    }
 }
 
 int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
@@ -98,9 +108,14 @@ int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
 
 int vcpu_vtimer_init(struct vcpu *v)
 {
+#ifndef CONFIG_NEW_VGIC
+    struct pending_irq *p;
+#endif
     struct vtimer *t = &v->arch.phys_timer;
     bool d0 = is_hardware_domain(v->domain);
 
+    const unsigned host_vtimer_irq_ppi = timer_get_irq(TIMER_VIRT_PPI);
+
     /*
      * Hardware domain uses the hardware interrupts, guests get the virtual
      * platform.
@@ -118,10 +133,18 @@ int vcpu_vtimer_init(struct vcpu *v)
     init_timer(&t->timer, virt_timer_expired, t, v->processor);
     t->ctl = 0;
     t->irq = d0
-        ? timer_get_irq(TIMER_VIRT_PPI)
+        ? host_vtimer_irq_ppi
         : GUEST_TIMER_VIRT_PPI;
     t->v = v;
 
+#ifndef CONFIG_NEW_VGIC
+    p = irq_to_pending(v, t->irq);
+    p->irq = t->irq;
+#endif
+
+    gic_hwppi_state_init(&v->arch.virt_timer.ppi_state,
+                         host_vtimer_irq_ppi);
+
     v->arch.vtimer_initialized = 1;
 
     return 0;
@@ -149,6 +172,16 @@ void virt_timer_save(struct vcpu *v)
         set_timer(&v->arch.virt_timer.timer, ticks_to_ns(v->arch.virt_timer.cval +
                   v->domain->arch.virt_timer_base.offset - boot_count));
     }
+
+    /*
+     * Since the vtimer irq is a PPI we don't need to worry about
+     * racing against it becoming active while we are saving the
+     * state, since that requires the guest to be reading the IAR,
+     * as long as the guest is not using I*ACTIVER register which we
+     * don't yet implement.
+     */
+    gic_save_and_mask_hwppi(v, v->arch.virt_timer.irq,
+                            &v->arch.virt_timer.ppi_state);
 }
 
 void virt_timer_restore(struct vcpu *v)
@@ -162,6 +195,10 @@ void virt_timer_restore(struct vcpu *v)
     WRITE_SYSREG64(v->domain->arch.virt_timer_base.offset, CNTVOFF_EL2);
     WRITE_SYSREG64(v->arch.virt_timer.cval, CNTV_CVAL_EL0);
     WRITE_SYSREG32(v->arch.virt_timer.ctl, CNTV_CTL_EL0);
+
+    gic_restore_hwppi(v,
+                      v->arch.virt_timer.irq,
+                      &v->arch.virt_timer.ppi_state);
 }
 
 static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool read)
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c3f4cd5069..b8fe142960 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -51,6 +51,7 @@ struct vtimer {
     struct timer timer;
     uint32_t ctl;
     uint64_t cval;
+    struct hwppi_state ppi_state;
 };
 
 struct arch_domain
diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h
index 6a83185163..198dd4eadb 100644
--- a/xen/include/asm-arm/perfc_defn.h
+++ b/xen/include/asm-arm/perfc_defn.h
@@ -70,7 +70,6 @@ PERFCOUNTER(guest_irqs,           "#GUEST-IRQS")
 
 PERFCOUNTER(hyp_timer_irqs,   "Hypervisor timer interrupts")
 PERFCOUNTER(phys_timer_irqs,  "Physical timer interrupts")
-PERFCOUNTER(virt_timer_irqs,  "Virtual timer interrupts")
 PERFCOUNTER(maintenance_irqs, "Maintenance interrupts")
 
 PERFCOUNTER(atomics_guest,    "atomics: guest access")
-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC XEN PATCH v3 10/11] xen: arm: context switch vtimer PPI state.

[Julien Grall]

(+ Andre)

Hi,

On 15/11/2019 20:14, Stewart Hildebrand wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> ... instead of artificially masking the timer interrupt in the timer
> state and relying on the guest to unmask (which it isn't required to
> do per the h/w spec, although Linux does).
> 
> By using the newly added hwppi save/restore functionality we make use
> of the GICD I[SC]ACTIVER registers to save and restore the active
> state of the interrupt, which prevents the nested invocations which
> the current masking works around.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> ---
> v2: Rebased, in particular over Julien's passthrough stuff which
>      reworked a bunch of IRQ related stuff.
>      Also largely rewritten since precursor patches now lay very
>      different groundwork.
> 
> v3: Address feedback from v2 [1]:
>    * Remove virt_timer_irqs performance counter since it is now unused.
>    * Add caveat to comment about not using I*ACTIVER register.
>    * HACK: don't initialize pending_irq->irq in vtimer for new vGIC to
>      allows us to build with CONFIG_NEW_VGIC=y
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01065.html
> ---
> 
> Note: Regarding Stefano's comment in [2], I did test it with the call
> to gic_hwppi_set_pending removed, and I was able to boot dom0.

When dealing with the vGIC, testing is not enough to justify the removal 
of some code. We need a worded justification of why it is (or not) 
necessary.

In this case the timer is level (despite some broken HW misconfiguring 
it), so by removing set_pending() you don't affect anything as restoring 
the timer registers will automatically mark the interrupt pending.

> 
> [2] https://lists.xenproject.org/archives/html/xen-devel/2015-12/msg02683.html
> ---
>   xen/arch/arm/time.c              | 26 ++----------------
>   xen/arch/arm/vtimer.c            | 45 +++++++++++++++++++++++++++++---
>   xen/include/asm-arm/domain.h     |  1 +
>   xen/include/asm-arm/perfc_defn.h |  1 -
>   4 files changed, 44 insertions(+), 29 deletions(-)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 739bcf186c..e3a23b8e16 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -243,28 +243,6 @@ static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
>       }
>   }
>   
> -static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
> -{
> -    /*
> -     * Edge-triggered interrupts can be used for the virtual timer. Even
> -     * if the timer output signal is masked in the context switch, the
> -     * GIC will keep track that of any interrupts raised while IRQS are
> -     * disabled. As soon as IRQs are re-enabled, the virtual interrupt
> -     * will be injected to Xen.
> -     *
> -     * If an IDLE vCPU was scheduled next then we should ignore the
> -     * interrupt.
> -     */
> -    if ( unlikely(is_idle_vcpu(current)) )
> -        return;
> -
> -    perfc_incr(virt_timer_irqs);
> -
> -    current->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0);
> -    WRITE_SYSREG32(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL_EL0);
> -    vgic_inject_irq(current->domain, current, current->arch.virt_timer.irq, true);
> -}
> -
>   /*
>    * Arch timer interrupt really ought to be level triggered, since the
>    * design of the timer/comparator mechanism is based around that
> @@ -304,8 +282,8 @@ void init_timer_interrupt(void)
>   
>       request_irq(timer_irq[TIMER_HYP_PPI], 0, timer_interrupt,
>                   "hyptimer", NULL);
> -    request_irq(timer_irq[TIMER_VIRT_PPI], 0, vtimer_interrupt,
> -                   "virtimer", NULL);
> +    route_hwppi_to_current_vcpu(timer_irq[TIMER_VIRT_PPI], "virtimer");
> +
>       request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], 0, timer_interrupt,
>                   "phytimer", NULL);
>   
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index e6aebdac9e..6e3498952d 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -55,9 +55,19 @@ static void phys_timer_expired(void *data)
>   static void virt_timer_expired(void *data)
>   {
>       struct vtimer *t = data;
> -    t->ctl |= CNTx_CTL_MASK;
> -    vgic_inject_irq(t->v->domain, t->v, t->irq, true);
> -    perfc_incr(vtimer_virt_inject);
> +    t->ctl |= CNTx_CTL_PENDING;

I don't think this is necessary. If the software timer fire, then the 
virtual timer (in HW) would have fired too. So by restoring the timer, 
then the HW should by itself set the pending bit and trigger the interrupt.

> +    if ( !(t->ctl & CNTx_CTL_MASK) )

The timer is only set if the virtual timer is enabled and not masked. So 
I think this check is unnecessary as we could never reached this code 
with the virtual timer masked.

> +    {
> +        /*
> +         * An edge triggered interrupt should now be pending. Since

This does not make sense, the timer interrupt ought to be level. So why 
are you even speaking about edge here?

> +         * this timer can never expire while the domain is scheduled
> +         * we know that the gic_restore_hwppi in virt_timer_restore
> +         * will cause the real hwppi to occur and be routed.
> +         */
> +        gic_hwppi_set_pending(&t->ppi_state);
> +        vcpu_unblock(t->v);
> +        perfc_incr(vtimer_virt_inject);
> +    }

I think the implementation of virt_timer_expired could only be:

vcpu_unlock(...);
perfc_incr(vtimer_virt_inject);

>   }
>   
>   int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
> @@ -98,9 +108,14 @@ int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
>   
>   int vcpu_vtimer_init(struct vcpu *v)
>   {
> +#ifndef CONFIG_NEW_VGIC
> +    struct pending_irq *p;
> +#endif
>       struct vtimer *t = &v->arch.phys_timer;
>       bool d0 = is_hardware_domain(v->domain);
>   
> +    const unsigned host_vtimer_irq_ppi = timer_get_irq(TIMER_VIRT_PPI);
> +
>       /*
>        * Hardware domain uses the hardware interrupts, guests get the virtual
>        * platform.
> @@ -118,10 +133,18 @@ int vcpu_vtimer_init(struct vcpu *v)
>       init_timer(&t->timer, virt_timer_expired, t, v->processor);
>       t->ctl = 0;
>       t->irq = d0
> -        ? timer_get_irq(TIMER_VIRT_PPI)
> +        ? host_vtimer_irq_ppi
>           : GUEST_TIMER_VIRT_PPI;
>       t->v = v;
>   
> +#ifndef CONFIG_NEW_VGIC
> +    p = irq_to_pending(v, t->irq);
> +    p->irq = t->irq;
> +#endif

p->irq is initialized by vcpu_vgic_init(), so why do you need to 
override it?

> +
> +    gic_hwppi_state_init(&v->arch.virt_timer.ppi_state,
> +                         host_vtimer_irq_ppi);
> +
>       v->arch.vtimer_initialized = 1;
>   
>       return 0;
> @@ -149,6 +172,16 @@ void virt_timer_save(struct vcpu *v)
>           set_timer(&v->arch.virt_timer.timer, ticks_to_ns(v->arch.virt_timer.cval +
>                     v->domain->arch.virt_timer_base.offset - boot_count));
>       }
> +
> +    /*
> +     * Since the vtimer irq is a PPI we don't need to worry about
> +     * racing against it becoming active while we are saving the
> +     * state, since that requires the guest to be reading the IAR,
> +     * as long as the guest is not using I*ACTIVER register which we
> +     * don't yet implement.

I really don't think this is the correct place to document it. If 
someone were to implement I*ACTIVER then this would likely be missed.

But, in this case, I think we should not rely in the vtimer about the 
implementation of I*ACTIVER and DTRT from the start.

> +     */
> +    gic_save_and_mask_hwppi(v, v->arch.virt_timer.irq,
> +                            &v->arch.virt_timer.ppi_state);

I am not entirely sure of the ordering here. Don't we want to restoring 
the interrupt state first and then the timer registers?

>   } >
>   void virt_timer_restore(struct vcpu *v)
> @@ -162,6 +195,10 @@ void virt_timer_restore(struct vcpu *v)
>       WRITE_SYSREG64(v->domain->arch.virt_timer_base.offset, CNTVOFF_EL2);
>       WRITE_SYSREG64(v->arch.virt_timer.cval, CNTV_CVAL_EL0);
>       WRITE_SYSREG32(v->arch.virt_timer.ctl, CNTV_CTL_EL0);
> +
> +    gic_restore_hwppi(v,
> +                      v->arch.virt_timer.irq,
> +                      &v->arch.virt_timer.ppi_state);
>   }
>   
>   static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool read)
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index c3f4cd5069..b8fe142960 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -51,6 +51,7 @@ struct vtimer {
>       struct timer timer;
>       uint32_t ctl;
>       uint64_t cval;
> +    struct hwppi_state ppi_state;
>   };
>   
>   struct arch_domain
> diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h
> index 6a83185163..198dd4eadb 100644
> --- a/xen/include/asm-arm/perfc_defn.h
> +++ b/xen/include/asm-arm/perfc_defn.h
> @@ -70,7 +70,6 @@ PERFCOUNTER(guest_irqs,           "#GUEST-IRQS")
>   
>   PERFCOUNTER(hyp_timer_irqs,   "Hypervisor timer interrupts")
>   PERFCOUNTER(phys_timer_irqs,  "Physical timer interrupts")
> -PERFCOUNTER(virt_timer_irqs,  "Virtual timer interrupts")

Please add a word in the commit message explaining why virt_timer_irqs 
is removed.

>   PERFCOUNTER(maintenance_irqs, "Maintenance interrupts")
>   
>   PERFCOUNTER(atomics_guest,    "atomics: guest access")
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC XEN PATCH v3 10/11] xen: arm: context switch vtimer PPI state.

[Stefano Stabellini]

On Mon, 25 Nov 2019, Julien Grall wrote:
> On 15/11/2019 20:14, Stewart Hildebrand wrote:
> > From: Ian Campbell <ian.campbell@citrix.com>
> > 
> > ... instead of artificially masking the timer interrupt in the timer
> > state and relying on the guest to unmask (which it isn't required to
> > do per the h/w spec, although Linux does).
> > 
> > By using the newly added hwppi save/restore functionality we make use
> > of the GICD I[SC]ACTIVER registers to save and restore the active
> > state of the interrupt, which prevents the nested invocations which
> > the current masking works around.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> > ---
> > v2: Rebased, in particular over Julien's passthrough stuff which
> >      reworked a bunch of IRQ related stuff.
> >      Also largely rewritten since precursor patches now lay very
> >      different groundwork.
> > 
> > v3: Address feedback from v2 [1]:
> >    * Remove virt_timer_irqs performance counter since it is now unused.
> >    * Add caveat to comment about not using I*ACTIVER register.
> >    * HACK: don't initialize pending_irq->irq in vtimer for new vGIC to
> >      allows us to build with CONFIG_NEW_VGIC=y
> > 
> > [1]
> > https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01065.html
> > ---
> > 
> > Note: Regarding Stefano's comment in [2], I did test it with the call
> > to gic_hwppi_set_pending removed, and I was able to boot dom0.
> 
> When dealing with the vGIC, testing is not enough to justify the removal of
> some code. We need a worded justification of why it is (or not) necessary.
> 
> In this case the timer is level (despite some broken HW misconfiguring it), so
> by removing set_pending() you don't affect anything as restoring the timer
> registers will automatically mark the interrupt pending.
> 
> > 
> > [2]
> > https://lists.xenproject.org/archives/html/xen-devel/2015-12/msg02683.html
> > ---
> >   xen/arch/arm/time.c              | 26 ++----------------
> >   xen/arch/arm/vtimer.c            | 45 +++++++++++++++++++++++++++++---
> >   xen/include/asm-arm/domain.h     |  1 +
> >   xen/include/asm-arm/perfc_defn.h |  1 -
> >   4 files changed, 44 insertions(+), 29 deletions(-)
> > 
> > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> > index 739bcf186c..e3a23b8e16 100644
> > --- a/xen/arch/arm/time.c
> > +++ b/xen/arch/arm/time.c
> > @@ -243,28 +243,6 @@ static void timer_interrupt(int irq, void *dev_id,
> > struct cpu_user_regs *regs)
> >       }
> >   }
> >   -static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs
> > *regs)
> > -{
> > -    /*
> > -     * Edge-triggered interrupts can be used for the virtual timer. Even
> > -     * if the timer output signal is masked in the context switch, the
> > -     * GIC will keep track that of any interrupts raised while IRQS are
> > -     * disabled. As soon as IRQs are re-enabled, the virtual interrupt
> > -     * will be injected to Xen.
> > -     *
> > -     * If an IDLE vCPU was scheduled next then we should ignore the
> > -     * interrupt.
> > -     */
> > -    if ( unlikely(is_idle_vcpu(current)) )
> > -        return;
> > -
> > -    perfc_incr(virt_timer_irqs);
> > -
> > -    current->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0);
> > -    WRITE_SYSREG32(current->arch.virt_timer.ctl | CNTx_CTL_MASK,
> > CNTV_CTL_EL0);
> > -    vgic_inject_irq(current->domain, current, current->arch.virt_timer.irq,
> > true);
> > -}
> > -
> >   /*
> >    * Arch timer interrupt really ought to be level triggered, since the
> >    * design of the timer/comparator mechanism is based around that
> > @@ -304,8 +282,8 @@ void init_timer_interrupt(void)
> >         request_irq(timer_irq[TIMER_HYP_PPI], 0, timer_interrupt,
> >                   "hyptimer", NULL);
> > -    request_irq(timer_irq[TIMER_VIRT_PPI], 0, vtimer_interrupt,
> > -                   "virtimer", NULL);
> > +    route_hwppi_to_current_vcpu(timer_irq[TIMER_VIRT_PPI], "virtimer");
> > +
> >       request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], 0, timer_interrupt,
> >                   "phytimer", NULL);
> >   diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> > index e6aebdac9e..6e3498952d 100644
> > --- a/xen/arch/arm/vtimer.c
> > +++ b/xen/arch/arm/vtimer.c
> > @@ -55,9 +55,19 @@ static void phys_timer_expired(void *data)
> >   static void virt_timer_expired(void *data)
> >   {
> >       struct vtimer *t = data;
> > -    t->ctl |= CNTx_CTL_MASK;
> > -    vgic_inject_irq(t->v->domain, t->v, t->irq, true);
> > -    perfc_incr(vtimer_virt_inject);
> > +    t->ctl |= CNTx_CTL_PENDING;
> 
> I don't think this is necessary. If the software timer fire, then the virtual
> timer (in HW) would have fired too. So by restoring the timer, then the HW
> should by itself set the pending bit and trigger the interrupt.
> 
> > +    if ( !(t->ctl & CNTx_CTL_MASK) )
> 
> The timer is only set if the virtual timer is enabled and not masked. So I
> think this check is unnecessary as we could never reached this code with the
> virtual timer masked.
> 
> > +    {
> > +        /*
> > +         * An edge triggered interrupt should now be pending. Since
> 
> This does not make sense, the timer interrupt ought to be level. So why are
> you even speaking about edge here?
> 
> > +         * this timer can never expire while the domain is scheduled
> > +         * we know that the gic_restore_hwppi in virt_timer_restore
> > +         * will cause the real hwppi to occur and be routed.
> > +         */
> > +        gic_hwppi_set_pending(&t->ppi_state);
> > +        vcpu_unblock(t->v);
> > +        perfc_incr(vtimer_virt_inject);
> > +    }
> 
> I think the implementation of virt_timer_expired could only be:
> 
> vcpu_unlock(...);
  ^ vcpu_unblock

Your reasoning seems sound


> perfc_incr(vtimer_virt_inject);



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [HACK XEN PATCH v3 11/11] HACK: Force virt timer to PPI0 (IRQ16)

[Stewart Hildebrand]

From: Ian Campbell <ian.campbell@citrix.com>

Just for testing so the guest vtimer ppi it isn't the same as the
physical virt timer PPI on my platform, and therefore allows to
exercise the non-1:1 bits of the code.
---
 xen/include/public/arch-arm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index c365b1b39e..e7ab984a3b 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -451,7 +451,7 @@ typedef uint64_t xen_callback_t;
 #define GUEST_MAX_VCPUS 128
 
 /* Interrupts */
-#define GUEST_TIMER_VIRT_PPI    27
+#define GUEST_TIMER_VIRT_PPI    16
 #define GUEST_TIMER_PHYS_S_PPI  29
 #define GUEST_TIMER_PHYS_NS_PPI 30
 #define GUEST_EVTCHN_PPI        31
-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel