[PATCH for-4.6 0/4] Find automatically a PPI for the DOM0 even channel IRQ

[PATCH for-4.6 0/4] Find automatically a PPI for the DOM0 even channel IRQ

[Julien Grall]

Hello,

This patch series replaces the per-platform hardcoded event channel interrupt
to a generic solution. It will make the port to a new platform more easier and
may avoid to introduce per-platform code with the new upcoming ACPI support.

This could be done by keeping track of vIRQ (emulated and assigned) used by
a domain.

Parth: I provided a branch on my personal repo [1]. It's based on the latest
upstream branch. You can use vgic_allocate_virq(d, 0) to allocate the event
channel PPI.

Sincerely yours,

[1] git://xenbits.xen.org/people/julieng/xen-unstable.git branch find-evtchn

Julien Grall (4):
  xen/arm: vgic: Rename nr_lines into nr_spis
  xen/arm: vgic: Keep track of vIRQ used by a domain
  xen/arm: vgic: notice if the vIRQ is not allocated when the guest
    enable it
  xen/arm: Find automatically a PPI for the DOM0 event channel interrupt

 xen/arch/arm/domain.c                | 13 +++--
 xen/arch/arm/domain_build.c          | 16 ++++++
 xen/arch/arm/gic-v2.c                |  2 -
 xen/arch/arm/gic-v3.c                |  2 +-
 xen/arch/arm/platform.c              |  7 ---
 xen/arch/arm/platforms/xgene-storm.c |  5 +-
 xen/arch/arm/vgic-v2.c               |  2 +-
 xen/arch/arm/vgic-v3.c               |  2 +-
 xen/arch/arm/vgic.c                  | 95 ++++++++++++++++++++++++++++++++----
 xen/arch/arm/vtimer.c                | 15 ++++++
 xen/include/asm-arm/domain.h         |  3 +-
 xen/include/asm-arm/platform.h       |  4 --
 xen/include/asm-arm/vgic.h           | 17 ++++++-
 13 files changed, 152 insertions(+), 31 deletions(-)

-- 
2.1.3

[PATCH for-4.6 1/4] xen/arm: vgic: Rename nr_lines into nr_spis

[Julien Grall]

The field nr_lines in the arch_domain vgic structure contains the number of
SPIs for the emulated GIC. Using the nr_lines make confusion with the GIC
code, where it means the number of IRQs. This can lead to coding error.

Also introduce vgic_nr_lines to get the number of IRQ handled by the emulated
GIC.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---
    It was part of the platform device passthrough series: https://patches.linaro.org/34661/

    Stefano: I've kept your ack from the previous version. Let me know
    if there is any issue.

    Changes in v3:
        - Add acked-by from Stefano.
        - Update the patch to also modify GICv3 code which has been
        pushed recently

    Changes in v2:
        - Patch added.
---
 xen/arch/arm/gic-v2.c        |  2 --
 xen/arch/arm/gic-v3.c        |  2 +-
 xen/arch/arm/vgic-v2.c       |  2 +-
 xen/arch/arm/vgic-v3.c       |  2 +-
 xen/arch/arm/vgic.c          | 15 ++++++---------
 xen/include/asm-arm/domain.h |  2 +-
 xen/include/asm-arm/vgic.h   |  4 +++-
 7 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index faad1ff..31fb81a 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -432,8 +432,6 @@ static int gicv2v_setup(struct domain *d)
         d->arch.vgic.cbase = GUEST_GICC_BASE;
     }
 
-    d->arch.vgic.nr_lines = 0;
-
     /*
      * Map the gic virtual cpu interface in the gic cpu interface
      * region of the guest.
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 076aa62..ec48fc1 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -922,7 +922,7 @@ static int gicv_v3_init(struct domain *d)
         d->arch.vgic.rbase_size[0] = GUEST_GICV3_GICR0_SIZE;
     }
 
-    d->arch.vgic.nr_lines = 0;
+    d->arch.vgic.nr_spis = 0;
 
     return 0;
 }
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 1369f78..039e19a 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -54,7 +54,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         /* No secure world support for guests. */
         vgic_lock(v);
         *r = ( (v->domain->max_vcpus << 5) & GICD_TYPE_CPUS )
-            |( ((v->domain->arch.vgic.nr_lines / 32)) & GICD_TYPE_LINES );
+            |( ((v->domain->arch.vgic.nr_spis / 32)) & GICD_TYPE_LINES );
         vgic_unlock(v);
         return 1;
     case GICD_IIDR:
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index ff99e50..2785c10 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -668,7 +668,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         if ( dabt.size != DABT_WORD ) goto bad_width;
         /* No secure world support for guests. */
         *r = (((v->domain->max_vcpus << 5) & GICD_TYPE_CPUS ) |
-              ((v->domain->arch.vgic.nr_lines / 32) & GICD_TYPE_LINES));
+              ((v->domain->arch.vgic.nr_spis / 32) & GICD_TYPE_LINES));
         return 1;
     case GICD_STATUSR:
         /*
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 97061ce..75cb7ff 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -66,13 +66,10 @@ int domain_vgic_init(struct domain *d)
 
     d->arch.vgic.ctlr = 0;
 
-    /* Currently nr_lines in vgic and gic doesn't have the same meanings
-     * Here nr_lines = number of SPIs
-     */
     if ( is_hardware_domain(d) )
-        d->arch.vgic.nr_lines = gic_number_lines() - 32;
+        d->arch.vgic.nr_spis = gic_number_lines() - 32;
     else
-        d->arch.vgic.nr_lines = 0; /* We don't need SPIs for the guest */
+        d->arch.vgic.nr_spis = 0; /* We don't need SPIs for the guest */
 
     switch ( gic_hw_version() )
     {
@@ -96,11 +93,11 @@ int domain_vgic_init(struct domain *d)
         return -ENOMEM;
 
     d->arch.vgic.pending_irqs =
-        xzalloc_array(struct pending_irq, d->arch.vgic.nr_lines);
+        xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
     if ( d->arch.vgic.pending_irqs == NULL )
         return -ENOMEM;
 
-    for (i=0; i<d->arch.vgic.nr_lines; i++)
+    for (i=0; i<d->arch.vgic.nr_spis; i++)
     {
         INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].inflight);
         INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
@@ -218,7 +215,7 @@ void arch_move_irqs(struct vcpu *v)
     struct vcpu *v_target;
     int i;
 
-    for ( i = 32; i < (d->arch.vgic.nr_lines + 32); i++ )
+    for ( i = 32; i < vgic_num_irqs(d); i++ )
     {
         v_target = vgic_get_target_vcpu(v, i);
         p = irq_to_pending(v_target, i);
@@ -344,7 +341,7 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
 struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
 {
     struct pending_irq *n;
-    /* Pending irqs allocation strategy: the first vgic.nr_lines irqs
+    /* Pending irqs allocation strategy: the first vgic.nr_spis irqs
      * are used for SPIs; the rests are used for per cpu irqs */
     if ( irq < 32 )
         n = &v->arch.vgic.pending_irqs[irq];
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 787e93c..8b7dd85 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -89,7 +89,7 @@ struct arch_domain
          */
         spinlock_t lock;
         int ctlr;
-        int nr_lines; /* Number of SPIs */
+        int nr_spis; /* Number of SPIs */
         struct vgic_irq_rank *shared_irqs;
         /*
          * SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 5160f17..74d5a4e 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -113,7 +113,7 @@ struct vgic_ops {
 };
 
 /* Number of ranks of interrupt registers for a domain */
-#define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_lines+31)/32)
+#define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_spis+31)/32)
 
 #define vgic_lock(v)   spin_lock_irq(&(v)->domain->arch.vgic.lock)
 #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
@@ -175,6 +175,8 @@ enum gic_sgi_mode;
  */
 #define REG_RANK_INDEX(b, n, s) ((((n) >> s) & ((b)-1)) % 32)
 
+#define vgic_num_irqs(d)        ((d)->arch.vgic.nr_spis + 32)
+
 extern int domain_vgic_init(struct domain *d);
 extern void domain_vgic_free(struct domain *d);
 extern int vcpu_vgic_init(struct vcpu *v);
-- 
2.1.3

Re: [PATCH for-4.6 1/4] xen/arm: vgic: Rename nr_lines into nr_spis

[Ian Campbell]

On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
> The field nr_lines in the arch_domain vgic structure contains the number of
> SPIs for the emulated GIC. Using the nr_lines make confusion with the GIC
> code, where it means the number of IRQs. This can lead to coding error.
> 
> Also introduce vgic_nr_lines to get the number of IRQ handled by the emulated
> GIC.

>From the code you seem to have called it vgic_nr_irqs, which I prefer.

> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index faad1ff..31fb81a 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -432,8 +432,6 @@ static int gicv2v_setup(struct domain *d)
>          d->arch.vgic.cbase = GUEST_GICC_BASE;
>      }
>  
> -    d->arch.vgic.nr_lines = 0;

Not replaced with an assignment to nr_spis, as you do in the v3 case?

We should be consistent, which also makes me wonder if nr_lines^Wspis
should be the responsibility of the common vgic code to setup.

> -
>      /*
>       * Map the gic virtual cpu interface in the gic cpu interface
>       * region of the guest.
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 076aa62..ec48fc1 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -922,7 +922,7 @@ static int gicv_v3_init(struct domain *d)
>          d->arch.vgic.rbase_size[0] = GUEST_GICV3_GICR0_SIZE;
>      }
>  
> -    d->arch.vgic.nr_lines = 0;
> +    d->arch.vgic.nr_spis = 0;
>  
>      return 0;
>  }

Ian.

Re: [PATCH for-4.6 1/4] xen/arm: vgic: Rename nr_lines into nr_spis

[Julien Grall]

Hi Ian,

On 13/01/15 15:38, Ian Campbell wrote:
> On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
>> The field nr_lines in the arch_domain vgic structure contains the number of
>> SPIs for the emulated GIC. Using the nr_lines make confusion with the GIC
>> code, where it means the number of IRQs. This can lead to coding error.
>>
>> Also introduce vgic_nr_lines to get the number of IRQ handled by the emulated
>> GIC.
> 
> From the code you seem to have called it vgic_nr_irqs, which I prefer.

It's an error in the commit message. I will fix it in the next version.


>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index faad1ff..31fb81a 100644
>> --- a/xen/arch/arm/gic-v2.c
>> +++ b/xen/arch/arm/gic-v2.c
>> @@ -432,8 +432,6 @@ static int gicv2v_setup(struct domain *d)
>>          d->arch.vgic.cbase = GUEST_GICC_BASE;
>>      }
>>  
>> -    d->arch.vgic.nr_lines = 0;
> 
> Not replaced with an assignment to nr_spis, as you do in the v3 case?
> 
> We should be consistent, which also makes me wonder if nr_lines^Wspis
> should be the responsibility of the common vgic code to setup.

Actually it's already in common vgic code (see domain_vgic_init).

The initialization in gicv2v_setup was unnecessary, so does in
gic_v3_init. I just forgot to drop it.

Shall I create a separate patch for the 2 lines drop? Or add a comment
in the commit message?

Regards,

-- 
Julien Grall

Re: [PATCH for-4.6 1/4] xen/arm: vgic: Rename nr_lines into nr_spis

[Ian Campbell]

On Tue, 2015-01-13 at 15:52 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 13/01/15 15:38, Ian Campbell wrote:
> > On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
> >> The field nr_lines in the arch_domain vgic structure contains the number of
> >> SPIs for the emulated GIC. Using the nr_lines make confusion with the GIC
> >> code, where it means the number of IRQs. This can lead to coding error.
> >>
> >> Also introduce vgic_nr_lines to get the number of IRQ handled by the emulated
> >> GIC.
> > 
> > From the code you seem to have called it vgic_nr_irqs, which I prefer.
> 
> It's an error in the commit message. I will fix it in the next version.
> 
> 
> >> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> >> index faad1ff..31fb81a 100644
> >> --- a/xen/arch/arm/gic-v2.c
> >> +++ b/xen/arch/arm/gic-v2.c
> >> @@ -432,8 +432,6 @@ static int gicv2v_setup(struct domain *d)
> >>          d->arch.vgic.cbase = GUEST_GICC_BASE;
> >>      }
> >>  
> >> -    d->arch.vgic.nr_lines = 0;
> > 
> > Not replaced with an assignment to nr_spis, as you do in the v3 case?
> > 
> > We should be consistent, which also makes me wonder if nr_lines^Wspis
> > should be the responsibility of the common vgic code to setup.
> 
> Actually it's already in common vgic code (see domain_vgic_init).
> 
> The initialization in gicv2v_setup was unnecessary, so does in
> gic_v3_init. I just forgot to drop it.
> 
> Shall I create a separate patch for the 2 lines drop? Or add a comment
> in the commit message?

Either works for me.

Ian.

[PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Julien Grall]

While it's easy to know which hardware IRQ is assigned to a domain, there
is no way to know which IRQ is emulated by Xen for a specific domain.

Introduce a bitmap to keep track of every vIRQ used by a domain. This
will be used later to find free vIRQ for interrupt device assignment and
emulated interrupt.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain_build.c          |  6 +++
 xen/arch/arm/platforms/xgene-storm.c |  4 ++
 xen/arch/arm/vgic.c                  | 76 ++++++++++++++++++++++++++++++++++++
 xen/arch/arm/vtimer.c                | 15 +++++++
 xen/include/asm-arm/domain.h         |  1 +
 xen/include/asm-arm/vgic.h           | 13 ++++++
 6 files changed, 115 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index de180d8..c238c8f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -968,6 +968,12 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
         irq = res;
 
         DPRINT("irq %u = %u\n", i, irq);
+        /*
+         * Checking the return of vgic_reserve_virq is not
+         * necessary. It should not fail except when we try to map
+         * twice the IRQ. This can happen if the IRQ is shared
+         */
+        vgic_reserve_virq(d, irq);
         res = route_irq_to_guest(d, irq, dt_node_name(dev));
         if ( res )
         {
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 0b3492d..416d42c 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -71,6 +71,10 @@ static int map_one_spi(struct domain *d, const char *what,
 
     printk("Additional IRQ %u (%s)\n", irq, what);
 
+    if ( !vgic_reserve_virq(d, irq) )
+        printk("Failed to reserve the vIRQ %u on dom%d\n",
+               irq, d->domain_id);
+
     ret = route_irq_to_guest(d, irq, what);
     if ( ret )
         printk("Failed to route %s to dom%d\n", what, d->domain_id);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 75cb7ff..dbfc259 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -87,6 +87,8 @@ int domain_vgic_init(struct domain *d)
         return -ENODEV;
     }
 
+    spin_lock_init(&d->arch.vgic.lock);
+
     d->arch.vgic.shared_irqs =
         xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
     if ( d->arch.vgic.shared_irqs == NULL )
@@ -107,6 +109,15 @@ int domain_vgic_init(struct domain *d)
 
     d->arch.vgic.handler->domain_init(d);
 
+    d->arch.vgic.allocated_irqs =
+        xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
+    if ( !d->arch.vgic.allocated_irqs )
+        return -ENOMEM;
+
+    /* vIRQ0-15 (SGIs) are reserved */
+    for (i = 0; i <= 15; i++)
+        set_bit(i, d->arch.vgic.allocated_irqs);
+
     return 0;
 }
 
@@ -119,6 +130,7 @@ void domain_vgic_free(struct domain *d)
 {
     xfree(d->arch.vgic.shared_irqs);
     xfree(d->arch.vgic.pending_irqs);
+    xfree(d->arch.vgic.allocated_irqs);
 }
 
 int vcpu_vgic_init(struct vcpu *v)
@@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
     return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr);
 }
 
+bool_t vgic_reserve_virq(struct domain *d, unsigned int virq)
+{
+    bool_t reserved;
+
+    if ( virq >= vgic_num_irqs(d) )
+        return 0;
+
+    spin_lock(&d->arch.vgic.lock);
+    reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
+    spin_unlock(&d->arch.vgic.lock);
+
+    return reserved;
+}
+
+int vgic_allocate_virq(struct domain *d, bool_t spi)
+{
+    int ret = -1;
+    unsigned int virq;
+
+    spin_lock(&d->arch.vgic.lock);
+    if ( !spi )
+    {
+        virq = find_first_zero_bit(d->arch.vgic.allocated_irqs, 32);
+        if ( virq >= 32 )
+            goto unlock;
+    }
+    else
+    {
+        virq = find_next_zero_bit(d->arch.vgic.allocated_irqs,
+                                  32, vgic_num_irqs(d));
+        if ( virq >= vgic_num_irqs(d) )
+            goto unlock;
+    }
+
+    set_bit(virq, d->arch.vgic.allocated_irqs);
+    ret = virq;
+
+unlock:
+    spin_unlock(&d->arch.vgic.lock);
+
+    return ret;
+}
+
+void vgic_free_virq(struct domain *d, unsigned int virq)
+{
+    unsigned int spi;
+
+    if ( is_hardware_domain(d) )
+        return;
+
+    if ( virq < 32 && virq >= vgic_num_irqs(d) )
+        return;
+
+    spi = virq - 32;
+
+    /* Taking the vGIC domain lock is not necessary. We don't care if
+     * the bit is cleared a bit later. What only matters is bit to 1.
+     *
+     * With this solution vgic_allocate may fail to find an vIRQ if the
+     * allocated_irqs is fully. But we don't care.
+     */
+    clear_bit(spi, d->arch.vgic.allocated_irqs);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 2e95ceb..de660bb 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d)
 {
     d->arch.phys_timer_base.offset = NOW();
     d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
+
+    /* At this stage vgic_reserve_virq can't fail */
+    if ( is_hardware_domain(d) )
+    {
+        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)));
+        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
+        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
+    }
+    else
+    {
+        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
+        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
+        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
+    }
+
     return 0;
 }
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 8b7dd85..d302fc9 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -90,6 +90,7 @@ struct arch_domain
         spinlock_t lock;
         int ctlr;
         int nr_spis; /* Number of SPIs */
+        unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
         struct vgic_irq_rank *shared_irqs;
         /*
          * SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 74d5a4e..9e167fa 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -199,6 +199,19 @@ extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
                        enum gic_sgi_mode irqmode, int virq,
                        unsigned long vcpu_mask);
 extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
+
+/* Reserve a specific guest vIRQ */
+extern bool_t vgic_reserve_virq(struct domain *d, unsigned int virq);
+
+/*
+ * Allocate a guest VIRQ
+ *  - spi == 0 => allocate a PPI. It will be the same on every vCPU
+ *  - spi == 0 => allocate an SGI
+ */
+extern int vgic_allocate_virq(struct domain *d, bool_t spi);
+
+extern void vgic_free_virq(struct domain *d, unsigned int irq);
+
 #endif /* __ASM_ARM_VGIC_H__ */
 
 /*
-- 
2.1.3

Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Stefano Stabellini]

On Fri, 12 Dec 2014, Julien Grall wrote:
> While it's easy to know which hardware IRQ is assigned to a domain, there
> is no way to know which IRQ is emulated by Xen for a specific domain.
> 
> Introduce a bitmap to keep track of every vIRQ used by a domain. This
> will be used later to find free vIRQ for interrupt device assignment and
> emulated interrupt.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain_build.c          |  6 +++
>  xen/arch/arm/platforms/xgene-storm.c |  4 ++
>  xen/arch/arm/vgic.c                  | 76 ++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/vtimer.c                | 15 +++++++
>  xen/include/asm-arm/domain.h         |  1 +
>  xen/include/asm-arm/vgic.h           | 13 ++++++
>  6 files changed, 115 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index de180d8..c238c8f 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -968,6 +968,12 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
>          irq = res;
>  
>          DPRINT("irq %u = %u\n", i, irq);
> +        /*
> +         * Checking the return of vgic_reserve_virq is not
> +         * necessary. It should not fail except when we try to map
> +         * twice the IRQ. This can happen if the IRQ is shared
> +         */
> +        vgic_reserve_virq(d, irq);
>          res = route_irq_to_guest(d, irq, dt_node_name(dev));
>          if ( res )
>          {
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index 0b3492d..416d42c 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -71,6 +71,10 @@ static int map_one_spi(struct domain *d, const char *what,
>  
>      printk("Additional IRQ %u (%s)\n", irq, what);
>  
> +    if ( !vgic_reserve_virq(d, irq) )
> +        printk("Failed to reserve the vIRQ %u on dom%d\n",
> +               irq, d->domain_id);
> +
>      ret = route_irq_to_guest(d, irq, what);
>      if ( ret )
>          printk("Failed to route %s to dom%d\n", what, d->domain_id);
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 75cb7ff..dbfc259 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -87,6 +87,8 @@ int domain_vgic_init(struct domain *d)
>          return -ENODEV;
>      }
>  
> +    spin_lock_init(&d->arch.vgic.lock);

you should probably explain in the commit message the reason why you are
making changes to the vgic lock


>      d->arch.vgic.shared_irqs =
>          xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
>      if ( d->arch.vgic.shared_irqs == NULL )
> @@ -107,6 +109,15 @@ int domain_vgic_init(struct domain *d)
>  
>      d->arch.vgic.handler->domain_init(d);
>  
> +    d->arch.vgic.allocated_irqs =
> +        xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
> +    if ( !d->arch.vgic.allocated_irqs )
> +        return -ENOMEM;
> +
> +    /* vIRQ0-15 (SGIs) are reserved */
> +    for (i = 0; i <= 15; i++)
> +        set_bit(i, d->arch.vgic.allocated_irqs);
> +
>      return 0;
>  }
>  
> @@ -119,6 +130,7 @@ void domain_vgic_free(struct domain *d)
>  {
>      xfree(d->arch.vgic.shared_irqs);
>      xfree(d->arch.vgic.pending_irqs);
> +    xfree(d->arch.vgic.allocated_irqs);
>  }
>  
>  int vcpu_vgic_init(struct vcpu *v)
> @@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
>      return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr);
>  }
>  
> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq)
> +{
> +    bool_t reserved;
> +
> +    if ( virq >= vgic_num_irqs(d) )
> +        return 0;
> +
> +    spin_lock(&d->arch.vgic.lock);
> +    reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
> +    spin_unlock(&d->arch.vgic.lock);

test_and_set_bit is atomic, why do you need to take the lock?


> +    return reserved;
> +}
> +
> +int vgic_allocate_virq(struct domain *d, bool_t spi)
> +{
> +    int ret = -1;
> +    unsigned int virq;
> +
> +    spin_lock(&d->arch.vgic.lock);
> +    if ( !spi )
> +    {
> +        virq = find_first_zero_bit(d->arch.vgic.allocated_irqs, 32);
> +        if ( virq >= 32 )
> +            goto unlock;
> +    }
> +    else
> +    {
> +        virq = find_next_zero_bit(d->arch.vgic.allocated_irqs,
> +                                  32, vgic_num_irqs(d));
> +        if ( virq >= vgic_num_irqs(d) )
> +            goto unlock;
> +    }
> +
> +    set_bit(virq, d->arch.vgic.allocated_irqs);
> +    ret = virq;
> +
> +unlock:
> +    spin_unlock(&d->arch.vgic.lock);

you might be able to write this function without taking the lock too, by
using test_and_set_bit and retries:

retry:
    virq = find_first_zero_bit;
    if (test_and_set_bit(virq))
        goto retry;



> +    return ret;
> +}
> +
> +void vgic_free_virq(struct domain *d, unsigned int virq)
> +{
> +    unsigned int spi;
> +
> +    if ( is_hardware_domain(d) )
> +        return;
> +
> +    if ( virq < 32 && virq >= vgic_num_irqs(d) )
> +        return;
> +
> +    spi = virq - 32;
> +
> +    /* Taking the vGIC domain lock is not necessary. We don't care if
> +     * the bit is cleared a bit later. What only matters is bit to 1.
> +     *
> +     * With this solution vgic_allocate may fail to find an vIRQ if the
> +     * allocated_irqs is fully. But we don't care.
> +     */
> +    clear_bit(spi, d->arch.vgic.allocated_irqs);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 2e95ceb..de660bb 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d)
>  {
>      d->arch.phys_timer_base.offset = NOW();
>      d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
> +
> +    /* At this stage vgic_reserve_virq can't fail */
> +    if ( is_hardware_domain(d) )
> +    {
> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)));
> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
> +    }
> +    else
> +    {
> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 8b7dd85..d302fc9 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -90,6 +90,7 @@ struct arch_domain
>          spinlock_t lock;
>          int ctlr;
>          int nr_spis; /* Number of SPIs */
> +        unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
>          struct vgic_irq_rank *shared_irqs;
>          /*
>           * SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 74d5a4e..9e167fa 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -199,6 +199,19 @@ extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
>                         enum gic_sgi_mode irqmode, int virq,
>                         unsigned long vcpu_mask);
>  extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
> +
> +/* Reserve a specific guest vIRQ */
> +extern bool_t vgic_reserve_virq(struct domain *d, unsigned int virq);
> +
> +/*
> + * Allocate a guest VIRQ
> + *  - spi == 0 => allocate a PPI. It will be the same on every vCPU
> + *  - spi == 0 => allocate an SGI
> + */
> +extern int vgic_allocate_virq(struct domain *d, bool_t spi);
> +
> +extern void vgic_free_virq(struct domain *d, unsigned int irq);
> +
>  #endif /* __ASM_ARM_VGIC_H__ */
>  
>  /*
> -- 
> 2.1.3
> 

Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Julien Grall]

Hi Stefano,

On 15/12/14 15:32, Stefano Stabellini wrote:
> On Fri, 12 Dec 2014, Julien Grall wrote:
>> +    spin_lock_init(&d->arch.vgic.lock);
> 
> you should probably explain in the commit message the reason why you are
> making changes to the vgic lock

Actually the domain vgic lock was never used. Only the per-vcpu vgic
lock was in used.

So I don't make any change to the vgic lock. If I don't use it, I will
add a patch to drop it.

>> @@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
>>      return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr);
>>  }
>>  
>> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq)
>> +{
>> +    bool_t reserved;
>> +
>> +    if ( virq >= vgic_num_irqs(d) )
>> +        return 0;
>> +
>> +    spin_lock(&d->arch.vgic.lock);
>> +    reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
>> +    spin_unlock(&d->arch.vgic.lock);
> 
> test_and_set_bit is atomic, why do you need to take the lock?

To avoid race condition with vgic_allocate_virq.

Anyway, I will dropped it with your suggestion to implement
vgic_allocate_virq without lock.

[..]

>> +int vgic_allocate_virq(struct domain *d, bool_t spi)
>> +{
>> +    int ret = -1;
>> +    unsigned int virq;
>> +
>> +    spin_lock(&d->arch.vgic.lock);
>> +    if ( !spi )
>> +    {
>> +        virq = find_first_zero_bit(d->arch.vgic.allocated_irqs, 32);
>> +        if ( virq >= 32 )
>> +            goto unlock;
>> +    }
>> +    else
>> +    {
>> +        virq = find_next_zero_bit(d->arch.vgic.allocated_irqs,
>> +                                  32, vgic_num_irqs(d));
>> +        if ( virq >= vgic_num_irqs(d) )
>> +            goto unlock;
>> +    }
>> +
>> +    set_bit(virq, d->arch.vgic.allocated_irqs);
>> +    ret = virq;
>> +
>> +unlock:
>> +    spin_unlock(&d->arch.vgic.lock);
> 
> you might be able to write this function without taking the lock too, by
> using test_and_set_bit and retries:
> 
> retry:
>     virq = find_first_zero_bit;
>     if (test_and_set_bit(virq))
>         goto retry;

I will give a look to it. I will also to limit the number of retry
(maybe to the number of vIRQ) for safety.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Julien Grall]

On 15/12/14 16:07, Julien Grall wrote:
> On 15/12/14 15:32, Stefano Stabellini wrote:
>> On Fri, 12 Dec 2014, Julien Grall wrote:
>>> +    spin_lock_init(&d->arch.vgic.lock);
>>
>> you should probably explain in the commit message the reason why you are
>> making changes to the vgic lock
> 
> Actually the domain vgic lock was never used. Only the per-vcpu vgic
> lock was in used.
> 
> So I don't make any change to the vgic lock. If I don't use it, I will
> add a patch to drop it.
> 
>>> @@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
>>>      return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr);
>>>  }
>>>  
>>> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq)
>>> +{
>>> +    bool_t reserved;
>>> +
>>> +    if ( virq >= vgic_num_irqs(d) )
>>> +        return 0;
>>> +
>>> +    spin_lock(&d->arch.vgic.lock);
>>> +    reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
>>> +    spin_unlock(&d->arch.vgic.lock);
>>
>> test_and_set_bit is atomic, why do you need to take the lock?
> 
> To avoid race condition with vgic_allocate_virq.
> 
> Anyway, I will dropped it with your suggestion to implement
> vgic_allocate_virq without lock.

Hmmm ... I was wrong on this one. The domain vgic lock is used in the
macro vgic_lock.

But it has never been initialized. I will send a separate patch for
correctly initialize it.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Ian Campbell]

On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
> While it's easy to know which hardware IRQ is assigned to a domain, there
> is no way to know which IRQ is emulated by Xen for a specific domain.

It seems you are tracking all valid interrupts, including hardware ones,
not just those for emulated devices? Perhaps rather than emulated you
meant "allocated to the guest" or "routed" or something?

> Introduce a bitmap to keep track of every vIRQ used by a domain. This
> will be used later to find free vIRQ for interrupt device assignment and
> emulated interrupt.

Actually, don't you implement the alloc/free of vIRQs here too?

Is there a usecase for tracking SPIs in this way, or would tracking PPIs
only be sufficient?

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain_build.c          |  6 +++
>  xen/arch/arm/platforms/xgene-storm.c |  4 ++
>  xen/arch/arm/vgic.c                  | 76 ++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/vtimer.c                | 15 +++++++
>  xen/include/asm-arm/domain.h         |  1 +
>  xen/include/asm-arm/vgic.h           | 13 ++++++
>  6 files changed, 115 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index de180d8..c238c8f 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -968,6 +968,12 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
>          irq = res;
>  
>          DPRINT("irq %u = %u\n", i, irq);
> +        /*
> +         * Checking the return of vgic_reserve_virq is not
> +         * necessary. It should not fail except when we try to map
> +         * twice the IRQ. This can happen if the IRQ is shared

Return and handle EBUSY to distinguish other errors?

("try to map the IRQ twice")

> +         */
> +        vgic_reserve_virq(d, irq);
>          res = route_irq_to_guest(d, irq, dt_node_name(dev));
>          if ( res )
>          {
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index 0b3492d..416d42c 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -71,6 +71,10 @@ static int map_one_spi(struct domain *d, const char *what,
>  
>      printk("Additional IRQ %u (%s)\n", irq, what);
>  
> +    if ( !vgic_reserve_virq(d, irq) )
> +        printk("Failed to reserve the vIRQ %u on dom%d\n",

Drop "the".

> +               irq, d->domain_id);
> +
>      ret = route_irq_to_guest(d, irq, what);
>      if ( ret )
>          printk("Failed to route %s to dom%d\n", what, d->domain_id);
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 75cb7ff..dbfc259 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -87,6 +87,8 @@ int domain_vgic_init(struct domain *d)
>          return -ENODEV;
>      }
>  
> +    spin_lock_init(&d->arch.vgic.lock);
> +
>      d->arch.vgic.shared_irqs =
>          xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
>      if ( d->arch.vgic.shared_irqs == NULL )
> @@ -107,6 +109,15 @@ int domain_vgic_init(struct domain *d)
>  
>      d->arch.vgic.handler->domain_init(d);
>  
> +    d->arch.vgic.allocated_irqs =
> +        xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));

(this was why I asked if tracking SPIs was needed...)

> +    if ( !d->arch.vgic.allocated_irqs )
> +        return -ENOMEM;
> +
> +    /* vIRQ0-15 (SGIs) are reserved */
> +    for (i = 0; i <= 15; i++)
> +        set_bit(i, d->arch.vgic.allocated_irqs);
> +
>      return 0;
>  }
>  
> @@ -119,6 +130,7 @@ void domain_vgic_free(struct domain *d)
>  {
>      xfree(d->arch.vgic.shared_irqs);
>      xfree(d->arch.vgic.pending_irqs);
> +    xfree(d->arch.vgic.allocated_irqs);
>  }
>  
>  int vcpu_vgic_init(struct vcpu *v)
> @@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
>      return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr);
>  }
>  
> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq)
> +{
> +    bool_t reserved;
> +
> +    if ( virq >= vgic_num_irqs(d) )
> +        return 0;

EINVAL?

> +    spin_lock(&d->arch.vgic.lock);
> +    reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
> +    spin_unlock(&d->arch.vgic.lock);
> +
> +    return reserved;
> +}
> +
> +int vgic_allocate_virq(struct domain *d, bool_t spi)
> +{
> +    int ret = -1;
> +    unsigned int virq;
> +
> +    spin_lock(&d->arch.vgic.lock);
> +    if ( !spi )
> +    {
> +        virq = find_first_zero_bit(d->arch.vgic.allocated_irqs, 32);

I think you could use find_next_zero_bit here to start the search at bit
16 and stop at bit 31. Having done so, it might be nicer to if (spi) to
select min and max IRQs and have the bit manipulation all be common.


> +void vgic_free_virq(struct domain *d, unsigned int virq)

It only frees spis, but the alloc version can do SPI or PPI. Is that on
purpose?

> +{
> +    unsigned int spi;
> +
> +    if ( is_hardware_domain(d) )
> +        return;
> +
> +    if ( virq < 32 && virq >= vgic_num_irqs(d) )
> +        return;
> +
> +    spi = virq - 32;
> +
> +    /* Taking the vGIC domain lock is not necessary. We don't care if
> +     * the bit is cleared a bit later. What only matters is bit to 1.

I don't grok the last sentence here.

> +     *
> +     * With this solution vgic_allocate may fail to find an vIRQ if the
> +     * allocated_irqs is fully. But we don't care.

are some words missing after fully?

> +     */
> +    clear_bit(spi, d->arch.vgic.allocated_irqs);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 2e95ceb..de660bb 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d)
>  {
>      d->arch.phys_timer_base.offset = NOW();
>      d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
> +
> +    /* At this stage vgic_reserve_virq can't fail */
> +    if ( is_hardware_domain(d) )
> +    {
> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)));
> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
> +    }
> +    else
> +    {
> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));

Although BUG_ON is not conditional on $debug I think we still should
avoid side effects in the condition.

> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 74d5a4e..9e167fa 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -199,6 +199,19 @@ extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
>                         enum gic_sgi_mode irqmode, int virq,
>                         unsigned long vcpu_mask);
>  extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
> +
> +/* Reserve a specific guest vIRQ */
> +extern bool_t vgic_reserve_virq(struct domain *d, unsigned int virq);
> +
> +/*
> + * Allocate a guest VIRQ
> + *  - spi == 0 => allocate a PPI. It will be the same on every vCPU
> + *  - spi == 0 => allocate an SGI

s/== 0/== 1/ and s/SGI/SPI/ in the last line.

> + */
> +extern int vgic_allocate_virq(struct domain *d, bool_t spi);
> +
> +extern void vgic_free_virq(struct domain *d, unsigned int irq);
> +
>  #endif /* __ASM_ARM_VGIC_H__ */
>  
>  /*

Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Julien Grall]

Hi Ian,

On 13/01/15 15:51, Ian Campbell wrote:
> On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
>> While it's easy to know which hardware IRQ is assigned to a domain, there
>> is no way to know which IRQ is emulated by Xen for a specific domain.
> 
> It seems you are tracking all valid interrupts, including hardware ones,
> not just those for emulated devices? Perhaps rather than emulated you
> meant "allocated to the guest" or "routed" or something?
> 
>> Introduce a bitmap to keep track of every vIRQ used by a domain. This
>> will be used later to find free vIRQ for interrupt device assignment and
>> emulated interrupt.
> 
> Actually, don't you implement the alloc/free of vIRQs here too?

Yes.

> Is there a usecase for tracking SPIs in this way, or would tracking PPIs
> only be sufficient?

We need to track everything for interrupt assignment to a guest/dom0. So
if the guest ask for a free vIRQ we can give it directly.

>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/domain_build.c          |  6 +++
>>  xen/arch/arm/platforms/xgene-storm.c |  4 ++
>>  xen/arch/arm/vgic.c                  | 76 ++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/vtimer.c                | 15 +++++++
>>  xen/include/asm-arm/domain.h         |  1 +
>>  xen/include/asm-arm/vgic.h           | 13 ++++++
>>  6 files changed, 115 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index de180d8..c238c8f 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -968,6 +968,12 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
>>          irq = res;
>>  
>>          DPRINT("irq %u = %u\n", i, irq);
>> +        /*
>> +         * Checking the return of vgic_reserve_virq is not
>> +         * necessary. It should not fail except when we try to map
>> +         * twice the IRQ. This can happen if the IRQ is shared
> 
> Return and handle EBUSY to distinguish other errors?

vgic_reserve_virq can fail for 2 reasons:
	- The IRQ is too high to handle by the vGIC => Unlikely as DOM0 use the
same IRQ number as the hardware.
	- The vIRQ is already reserved.

The former will be catch just after with route_irq_to_guest. So I don't
think it's worth to change the return from a bool to an int and return
-EBUSY.

> ("try to map the IRQ twice")
> 
>> +         */
>> +        vgic_reserve_virq(d, irq);
>>          res = route_irq_to_guest(d, irq, dt_node_name(dev));
>>          if ( res )
>>          {
>> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
>> index 0b3492d..416d42c 100644
>> --- a/xen/arch/arm/platforms/xgene-storm.c
>> +++ b/xen/arch/arm/platforms/xgene-storm.c
>> @@ -71,6 +71,10 @@ static int map_one_spi(struct domain *d, const char *what,
>>  
>>      printk("Additional IRQ %u (%s)\n", irq, what);
>>  
>> +    if ( !vgic_reserve_virq(d, irq) )
>> +        printk("Failed to reserve the vIRQ %u on dom%d\n",
> 
> Drop "the".

Ok.

>> +               irq, d->domain_id);
>> +
>>      ret = route_irq_to_guest(d, irq, what);
>>      if ( ret )
>>          printk("Failed to route %s to dom%d\n", what, d->domain_id);
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 75cb7ff..dbfc259 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -87,6 +87,8 @@ int domain_vgic_init(struct domain *d)
>>          return -ENODEV;
>>      }
>>  
>> +    spin_lock_init(&d->arch.vgic.lock);
>> +
>>      d->arch.vgic.shared_irqs =
>>          xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
>>      if ( d->arch.vgic.shared_irqs == NULL )
>> @@ -107,6 +109,15 @@ int domain_vgic_init(struct domain *d)
>>  
>>      d->arch.vgic.handler->domain_init(d);
>>  
>> +    d->arch.vgic.allocated_irqs =
>> +        xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
> 
> (this was why I asked if tracking SPIs was needed...)

To complete my answer above:
	- dom0: vgic_num_irqs() = number of hardware IRQS
	- guest: vgic_num_irqs() = 32.

So we don't waste memory.

> 
>> +    if ( !d->arch.vgic.allocated_irqs )
>> +        return -ENOMEM;
>> +
>> +    /* vIRQ0-15 (SGIs) are reserved */
>> +    for (i = 0; i <= 15; i++)
>> +        set_bit(i, d->arch.vgic.allocated_irqs);
>> +
>>      return 0;
>>  }
>>  
>> @@ -119,6 +130,7 @@ void domain_vgic_free(struct domain *d)
>>  {
>>      xfree(d->arch.vgic.shared_irqs);
>>      xfree(d->arch.vgic.pending_irqs);
>> +    xfree(d->arch.vgic.allocated_irqs);
>>  }
>>  
>>  int vcpu_vgic_init(struct vcpu *v)
>> @@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
>>      return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr);
>>  }
>>  
>> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq)
>> +{
>> +    bool_t reserved;
>> +
>> +    if ( virq >= vgic_num_irqs(d) )
>> +        return 0;
> 
> EINVAL?

vgic_reserve_irq returns a boolean:
	0 => not reserved
	1 => reserved

I don't see why we should return an int in this case, as the caller
should know how to use it.

>> +    spin_lock(&d->arch.vgic.lock);
>> +    reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
>> +    spin_unlock(&d->arch.vgic.lock);
>> +
>> +    return reserved;
>> +}
>> +
>> +int vgic_allocate_virq(struct domain *d, bool_t spi)
>> +{
>> +    int ret = -1;
>> +    unsigned int virq;
>> +
>> +    spin_lock(&d->arch.vgic.lock);
>> +    if ( !spi )
>> +    {
>> +        virq = find_first_zero_bit(d->arch.vgic.allocated_irqs, 32);
> 
> I think you could use find_next_zero_bit here to start the search at bit
> 16 and stop at bit 31. Having done so, it might be nicer to if (spi) to
> select min and max IRQs and have the bit manipulation all be common.

I will give a look for the next version.

> 
>> +void vgic_free_virq(struct domain *d, unsigned int virq)
> 
> It only frees spis, but the alloc version can do SPI or PPI. Is that on
> purpose?

I forgot to update vgic_free_virq when I made the support for PPIs.

>> +{
>> +    unsigned int spi;
>> +
>> +    if ( is_hardware_domain(d) )
>> +        return;
>> +
>> +    if ( virq < 32 && virq >= vgic_num_irqs(d) )
>> +        return;
>> +
>> +    spi = virq - 32;
>> +
>> +    /* Taking the vGIC domain lock is not necessary. We don't care if
>> +     * the bit is cleared a bit later. What only matters is bit to 1.
> 
> I don't grok the last sentence here.
> 
>> +     *
>> +     * With this solution vgic_allocate may fail to find an vIRQ if the
>> +     * allocated_irqs is fully. But we don't care.
> 
> are some words missing after fully?

This will be dropped in the next version. So forget this part :).

>> +     */
>> +    clear_bit(spi, d->arch.vgic.allocated_irqs);
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
>> index 2e95ceb..de660bb 100644
>> + */
>> +extern int vgic_allocate_virq(struct domain *d, bool_t spi);
>> +
>> +extern void vgic_free_virq(struct domain *d, unsigned int irq);
>> +
>>  #endif /* __ASM_ARM_VGIC_H__ */
>>  
>>  /*
> 
> 

>> --- a/xen/arch/arm/vtimer.c
>> +++ b/xen/arch/arm/vtimer.c
>> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d)
>>  {
>>      d->arch.phys_timer_base.offset = NOW();
>>      d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
>> +
>> +    /* At this stage vgic_reserve_virq can't fail */
>> +    if ( is_hardware_domain(d) )
>> +    {
>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)));
>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
>> +    }
>> +    else
>> +    {
>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
> 
> Although BUG_ON is not conditional on $debug I think we still should
> avoid side effects in the condition.

I know, but this should never fail as it called during on domain
construction. If so we may have some other issue later if we decide to
assign PPI to a guest.

I would prefer to keep the BUG_ON here.

> 
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index 74d5a4e..9e167fa 100644
>> --- a/xen/include/asm-arm/vgic.h
>> +++ b/xen/include/asm-arm/vgic.h
>> @@ -199,6 +199,19 @@ extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
>>                         enum gic_sgi_mode irqmode, int virq,
>>                         unsigned long vcpu_mask);
>>  extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
>> +
>> +/* Reserve a specific guest vIRQ */
>> +extern bool_t vgic_reserve_virq(struct domain *d, unsigned int virq);
>> +
>> +/*
>> + * Allocate a guest VIRQ
>> + *  - spi == 0 => allocate a PPI. It will be the same on every vCPU
>> + *  - spi == 0 => allocate an SGI
> 
> s/== 0/== 1/ and s/SGI/SPI/ in the last line.

I will fix it.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Ian Campbell]

On Tue, 2015-01-13 at 16:27 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 13/01/15 15:51, Ian Campbell wrote:
> > On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
> >> While it's easy to know which hardware IRQ is assigned to a domain, there
> >> is no way to know which IRQ is emulated by Xen for a specific domain.
> > 
> > It seems you are tracking all valid interrupts, including hardware ones,
> > not just those for emulated devices? Perhaps rather than emulated you
> > meant "allocated to the guest" or "routed" or something?
> > 
> >> Introduce a bitmap to keep track of every vIRQ used by a domain. This
> >> will be used later to find free vIRQ for interrupt device assignment and
> >> emulated interrupt.
> > 
> > Actually, don't you implement the alloc/free of vIRQs here too?
> 
> Yes.
> 
> > Is there a usecase for tracking SPIs in this way, or would tracking PPIs
> > only be sufficient?
> 
> We need to track everything for interrupt assignment to a guest/dom0. So
> if the guest ask for a free vIRQ we can give it directly.

Makes sense.

In that case you 0/4 mail doesn't fully describe the use case for the
series, since it talks about the dom0 PPI only.
> > 
> >> +    if ( !d->arch.vgic.allocated_irqs )
> >> +        return -ENOMEM;
> >> +
> >> +    /* vIRQ0-15 (SGIs) are reserved */
> >> +    for (i = 0; i <= 15; i++)
> >> +        set_bit(i, d->arch.vgic.allocated_irqs);
> >> +
> >>      return 0;
> >>  }
> >>  
> >> @@ -119,6 +130,7 @@ void domain_vgic_free(struct domain *d)
> >>  {
> >>      xfree(d->arch.vgic.shared_irqs);
> >>      xfree(d->arch.vgic.pending_irqs);
> >> +    xfree(d->arch.vgic.allocated_irqs);
> >>  }
> >>  
> >>  int vcpu_vgic_init(struct vcpu *v)
> >> @@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
> >>      return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr);
> >>  }
> >>  
> >> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq)
> >> +{
> >> +    bool_t reserved;
> >> +
> >> +    if ( virq >= vgic_num_irqs(d) )
> >> +        return 0;
> > 
> > EINVAL?
> 
> vgic_reserve_irq returns a boolean:

Please use true/false then.

In Xen we have xen/stdbool.h which differs from normal stdboot.h. I'm
not sure what the rules are for use.

> 	0 => not reserved
> 	1 => reserved
> 
> I don't see why we should return an int in this case, as the caller
> should know how to use it.

It's slightly more conventional to return error codes, but I guess I
don't mind much.

> >> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d)
> >>  {
> >>      d->arch.phys_timer_base.offset = NOW();
> >>      d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
> >> +
> >> +    /* At this stage vgic_reserve_virq can't fail */
> >> +    if ( is_hardware_domain(d) )
> >> +    {
> >> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)));
> >> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
> >> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
> >> +    }
> >> +    else
> >> +    {
> >> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
> >> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
> >> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
> > 
> > Although BUG_ON is not conditional on $debug I think we still should
> > avoid side effects in the condition.
> 
> I know, but this should never fail as it called during on domain
> construction. If so we may have some other issue later if we decide to
> assign PPI to a guest.
> 
> I would prefer to keep the BUG_ON here

I'm not objecting the the BUG_ON itself but to the fact that the
condition has a side effect. Please use:
        if (!do_something())
        	BUG()
instead to avoid this.

Ian.

Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Julien Grall]

(CC Jan)

Hi Ian,

On 13/01/15 16:46, Ian Campbell wrote:
>> vgic_reserve_irq returns a boolean:
> 
> Please use true/false then.
> 
> In Xen we have xen/stdbool.h which differs from normal stdboot.h. I'm
> not sure what the rules are for use.

Jan please correct me if I'm wrong, xen/stdbool.h has been introduced
for the ELF code and should not be used anywhere else.

true/false is defined in xen/stdbool.h together with Bool not bool_t.

>> 	0 => not reserved
>> 	1 => reserved
>>
>> I don't see why we should return an int in this case, as the caller
>> should know how to use it.
> 
> It's slightly more conventional to return error codes, but I guess I
> don't mind much.

Agree, but in this particular case we don't have to know the error code.
So it's pointless to return it.

>>>> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d)
>>>>  {
>>>>      d->arch.phys_timer_base.offset = NOW();
>>>>      d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
>>>> +
>>>> +    /* At this stage vgic_reserve_virq can't fail */
>>>> +    if ( is_hardware_domain(d) )
>>>> +    {
>>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)));
>>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
>>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
>>>> +    }
>>>> +    else
>>>> +    {
>>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
>>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
>>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
>>>
>>> Although BUG_ON is not conditional on $debug I think we still should
>>> avoid side effects in the condition.
>>
>> I know, but this should never fail as it called during on domain
>> construction. If so we may have some other issue later if we decide to
>> assign PPI to a guest.
>>
>> I would prefer to keep the BUG_ON here
> 
> I'm not objecting the the BUG_ON itself but to the fact that the
> condition has a side effect. Please use:
>         if (!do_something())
>         	BUG()
> instead to avoid this.

We have other place in the code where BUG_ON as a side-effect.

IHMO, if (!do_something()) BUG() <=> BUG_ON.

On the latter you know directly why it's failing, on the former you have
to look at the code.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Ian Campbell]

On Tue, 2015-01-13 at 16:57 +0000, Julien Grall wrote:
> (CC Jan)

I think you forget, I added him.

> >>>> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d)
> >>>>  {
> >>>>      d->arch.phys_timer_base.offset = NOW();
> >>>>      d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
> >>>> +
> >>>> +    /* At this stage vgic_reserve_virq can't fail */
> >>>> +    if ( is_hardware_domain(d) )
> >>>> +    {
> >>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)));
> >>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
> >>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
> >>>> +    }
> >>>> +    else
> >>>> +    {
> >>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
> >>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
> >>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
> >>>
> >>> Although BUG_ON is not conditional on $debug I think we still should
> >>> avoid side effects in the condition.
> >>
> >> I know, but this should never fail as it called during on domain
> >> construction. If so we may have some other issue later if we decide to
> >> assign PPI to a guest.
> >>
> >> I would prefer to keep the BUG_ON here
> > 
> > I'm not objecting the the BUG_ON itself but to the fact that the
> > condition has a side effect. Please use:
> >         if (!do_something())
> >         	BUG()
> > instead to avoid this.
> 
> We have other place in the code where BUG_ON as a side-effect.

If we do then it is a tiny minority of places, and they are IMHO wrong.
I spotted one in the 600+ results of grepping for BUG_ON.

> IHMO, if (!do_something()) BUG() <=> BUG_ON.

No, BUG_ON() is a variant of ASSERT(), with the distinction being that
the former is not only included when debug=y. It is as wrong to have a
side-effect in the BUG_ON as it is to have one in an ASSERT.

> On the latter you know directly why it's failing, on the former you have
> to look at the code.

If it's important/possible to fail then a log message would be
appropriate.

Ian.

Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Julien Grall]

On 13/01/15 17:18, Ian Campbell wrote:
> On Tue, 2015-01-13 at 16:57 +0000, Julien Grall wrote:
>> (CC Jan)
> 
> I think you forget, I added him.
> 
>>>>>> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d)
>>>>>>  {
>>>>>>      d->arch.phys_timer_base.offset = NOW();
>>>>>>      d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
>>>>>> +
>>>>>> +    /* At this stage vgic_reserve_virq can't fail */
>>>>>> +    if ( is_hardware_domain(d) )
>>>>>> +    {
>>>>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)));
>>>>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
>>>>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
>>>>>> +    }
>>>>>> +    else
>>>>>> +    {
>>>>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
>>>>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
>>>>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
>>>>>
>>>>> Although BUG_ON is not conditional on $debug I think we still should
>>>>> avoid side effects in the condition.
>>>>
>>>> I know, but this should never fail as it called during on domain
>>>> construction. If so we may have some other issue later if we decide to
>>>> assign PPI to a guest.
>>>>
>>>> I would prefer to keep the BUG_ON here
>>>
>>> I'm not objecting the the BUG_ON itself but to the fact that the
>>> condition has a side effect. Please use:
>>>         if (!do_something())
>>>         	BUG()
>>> instead to avoid this.
>>
>> We have other place in the code where BUG_ON as a side-effect.
> 
> If we do then it is a tiny minority of places, and they are IMHO wrong.
> I spotted one in the 600+ results of grepping for BUG_ON.

I spotted more. Anyway, I will move to a if (!do_smth()) BUG() form.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Julien Grall]

On 13/01/15 16:57, Julien Grall wrote:
> (CC Jan)

Forgot to really CC Jan for the bool stuff.

> Hi Ian,
> 
> On 13/01/15 16:46, Ian Campbell wrote:
>>> vgic_reserve_irq returns a boolean:
>>
>> Please use true/false then.
>>
>> In Xen we have xen/stdbool.h which differs from normal stdboot.h. I'm
>> not sure what the rules are for use.
> 
> Jan please correct me if I'm wrong, xen/stdbool.h has been introduced
> for the ELF code and should not be used anywhere else.
> 
> true/false is defined in xen/stdbool.h together with Bool not bool_t.
> 
>>> 	0 => not reserved
>>> 	1 => reserved
>>>
>>> I don't see why we should return an int in this case, as the caller
>>> should know how to use it.
>>
>> It's slightly more conventional to return error codes, but I guess I
>> don't mind much.
> 
> Agree, but in this particular case we don't have to know the error code.
> So it's pointless to return it.
> 
>>>>> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d)
>>>>>  {
>>>>>      d->arch.phys_timer_base.offset = NOW();
>>>>>      d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
>>>>> +
>>>>> +    /* At this stage vgic_reserve_virq can't fail */
>>>>> +    if ( is_hardware_domain(d) )
>>>>> +    {
>>>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)));
>>>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
>>>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
>>>>> +    }
>>>>> +    else
>>>>> +    {
>>>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
>>>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
>>>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
>>>>
>>>> Although BUG_ON is not conditional on $debug I think we still should
>>>> avoid side effects in the condition.
>>>
>>> I know, but this should never fail as it called during on domain
>>> construction. If so we may have some other issue later if we decide to
>>> assign PPI to a guest.
>>>
>>> I would prefer to keep the BUG_ON here
>>
>> I'm not objecting the the BUG_ON itself but to the fact that the
>> condition has a side effect. Please use:
>>         if (!do_something())
>>         	BUG()
>> instead to avoid this.
> 
> We have other place in the code where BUG_ON as a side-effect.
> 
> IHMO, if (!do_something()) BUG() <=> BUG_ON.
> 
> On the latter you know directly why it's failing, on the former you have
> to look at the code.
> 
> Regards,
> 


-- 
Julien Grall

Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Julien Grall]

On 13/01/15 16:46, Ian Campbell wrote:
>> We need to track everything for interrupt assignment to a guest/dom0. So
>> if the guest ask for a free vIRQ we can give it directly.
> 
> Makes sense.
> 
> In that case you 0/4 mail doesn't fully describe the use case for the
> series, since it talks about the dom0 PPI only.

Sorry I skipped this comment by inadvertence. My cover letter was
explaining the current use case, I didn't think to explain the future
use case. I will update the cover letter.

Regards,

-- 
Julien Grall

[PATCH for-4.6 3/4] xen/arm: vgic: notice if the vIRQ is not allocated when the guest enable it

[Julien Grall]

This help for guest interrupts debugging. If the vIRQ is not allocate,
this means that nothing is wired to it.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/vgic.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index dbfc259..719cb9f 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -282,6 +282,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
             gic_raise_guest_irq(v_target, irq, p->priority);
         spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
+
+        if ( !test_bit(irq, d->arch.vgic.allocated_irqs) )
+            gdprintk(XENLOG_DEBUG, "vIRQ %u is not allocated\n", irq);
+
         if ( p->desc != NULL )
         {
             irq_set_affinity(p->desc, cpumask_of(v_target->processor));
-- 
2.1.3

Re: [PATCH for-4.6 3/4] xen/arm: vgic: notice if the vIRQ is not allocated when the guest enable it

[Ian Campbell]

On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
> This help for guest interrupts debugging. If the vIRQ is not allocate,
> this means that nothing is wired to it.

Should we short circuit the rest of the enable operation for this IRQ
then? i.e. implement such writes as ignored, e.g. not reflect it in
reads of ISENABLER etc.

What (if anything) does the GIC spec have to say on the subject?

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/vgic.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index dbfc259..719cb9f 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -282,6 +282,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>          if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>              gic_raise_guest_irq(v_target, irq, p->priority);
>          spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> +
> +        if ( !test_bit(irq, d->arch.vgic.allocated_irqs) )
> +            gdprintk(XENLOG_DEBUG, "vIRQ %u is not allocated\n", irq);

Should this not be first after the irq= line inside this loop?

> +
>          if ( p->desc != NULL )
>          {
>              irq_set_affinity(p->desc, cpumask_of(v_target->processor));

Re: [PATCH for-4.6 3/4] xen/arm: vgic: notice if the vIRQ is not allocated when the guest enable it

[Julien Grall]

Hi Ian,

On 13/01/15 15:55, Ian Campbell wrote:
> On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
>> This help for guest interrupts debugging. If the vIRQ is not allocate,
>> this means that nothing is wired to it.
> 
> Should we short circuit the rest of the enable operation for this IRQ
> then? i.e. implement such writes as ignored, e.g. not reflect it in
> reads of ISENABLER etc.
> 
> What (if anything) does the GIC spec have to say on the subject?

"A register bit corresponding to an unimplemented interrupt is RAZ/WI."

The goal of this print was mostly for debugging physical IRQ routed to a
guest.

I could extend to ignore write to any register that should be RAZ/WI for
this specific interrupt.
But, I will have to think about possible race condition with the
hypercall to route a physical IRQ to the guest (see [1] and [2]).

The vIRQ is reserved before the physical IRQ is effectively routed. So
a guest may enable the vIRQ before this time lapse. Though, the patch
[2] protected for a such case.

Not sure if we should take care of a such case.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.6 3/4] xen/arm: vgic: notice if the vIRQ is not allocated when the guest enable it

[Ian Campbell]

On Tue, 2015-01-13 at 20:33 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 13/01/15 15:55, Ian Campbell wrote:
> > On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
> >> This help for guest interrupts debugging. If the vIRQ is not allocate,
> >> this means that nothing is wired to it.
> > 
> > Should we short circuit the rest of the enable operation for this IRQ
> > then? i.e. implement such writes as ignored, e.g. not reflect it in
> > reads of ISENABLER etc.
> > 
> > What (if anything) does the GIC spec have to say on the subject?
> 
> "A register bit corresponding to an unimplemented interrupt is RAZ/WI."
> 
> The goal of this print was mostly for debugging physical IRQ routed to a
> guest.
> 
> I could extend to ignore write to any register that should be RAZ/WI for
> this specific interrupt.

Since those are the defined semantics I think that is the best thing to
do.

> But, I will have to think about possible race condition with the
> hypercall to route a physical IRQ to the guest (see [1] and [2]).
> 
> The vIRQ is reserved before the physical IRQ is effectively routed. So
> a guest may enable the vIRQ before this time lapse. Though, the patch
> [2] protected for a such case.
> 
> Not sure if we should take care of a such case.

I don't think so, that routing hypercall ought to be happening strictly
before any hotplug event visible to the guest causes it to think there
is a device (vacuously true for coldplug too), so the guest should have
no expectation of being able to do anything with the irq in question.

Ian.

Re: [PATCH for-4.6 3/4] xen/arm: vgic: notice if the vIRQ is not allocated when the guest enable it

[Julien Grall]

On 14/01/15 12:28, Ian Campbell wrote:
> On Tue, 2015-01-13 at 20:33 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 13/01/15 15:55, Ian Campbell wrote:
>>> On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
>>>> This help for guest interrupts debugging. If the vIRQ is not allocate,
>>>> this means that nothing is wired to it.
>>>
>>> Should we short circuit the rest of the enable operation for this IRQ
>>> then? i.e. implement such writes as ignored, e.g. not reflect it in
>>> reads of ISENABLER etc.
>>>
>>> What (if anything) does the GIC spec have to say on the subject?
>>
>> "A register bit corresponding to an unimplemented interrupt is RAZ/WI."
>>
>> The goal of this print was mostly for debugging physical IRQ routed to a
>> guest.
>>
>> I could extend to ignore write to any register that should be RAZ/WI for
>> this specific interrupt.
> 
> Since those are the defined semantics I think that is the best thing to
> do.

Ok. I will look at it to see how we can implement it.

>> But, I will have to think about possible race condition with the
>> hypercall to route a physical IRQ to the guest (see [1] and [2]).
>>
>> The vIRQ is reserved before the physical IRQ is effectively routed. So
>> a guest may enable the vIRQ before this time lapse. Though, the patch
>> [2] protected for a such case.
>>
>> Not sure if we should take care of a such case.
> 
> I don't think so, that routing hypercall ought to be happening strictly
> before any hotplug event visible to the guest causes it to think there
> is a device (vacuously true for coldplug too), so the guest should have
> no expectation of being able to do anything with the irq in question.

Good, one less headache :)

Regards,

-- 
Julien Grall

Re: [PATCH for-4.6 3/4] xen/arm: vgic: notice if the vIRQ is not allocated when the guest enable it

[Julien Grall]

Hi Ian,

On 14/01/15 12:42, Julien Grall wrote:
> On 14/01/15 12:28, Ian Campbell wrote:
>> On Tue, 2015-01-13 at 20:33 +0000, Julien Grall wrote:
>>> On 13/01/15 15:55, Ian Campbell wrote:
>>>> On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
>>>>> This help for guest interrupts debugging. If the vIRQ is not allocate,
>>>>> this means that nothing is wired to it.
>>>>
>>>> Should we short circuit the rest of the enable operation for this IRQ
>>>> then? i.e. implement such writes as ignored, e.g. not reflect it in
>>>> reads of ISENABLER etc.
>>>>
>>>> What (if anything) does the GIC spec have to say on the subject?
>>>
>>> "A register bit corresponding to an unimplemented interrupt is RAZ/WI."
>>>
>>> The goal of this print was mostly for debugging physical IRQ routed to a
>>> guest.
>>>
>>> I could extend to ignore write to any register that should be RAZ/WI for
>>> this specific interrupt.
>>
>> Since those are the defined semantics I think that is the best thing to
>> do.
> 
> Ok. I will look at it to see how we can implement it.

So I looked to the code. It may need some rework to effectively
implement most of registers bits RAZ/WI when the interrupt doesn't exist.

As this series is required for the ACPI series, I suggest to defer this
work in a follow-up series.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.6 3/4] xen/arm: vgic: notice if the vIRQ is not allocated when the guest enable it

[Ian Campbell]

On Thu, 2015-01-15 at 13:27 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 14/01/15 12:42, Julien Grall wrote:
> > On 14/01/15 12:28, Ian Campbell wrote:
> >> On Tue, 2015-01-13 at 20:33 +0000, Julien Grall wrote:
> >>> On 13/01/15 15:55, Ian Campbell wrote:
> >>>> On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
> >>>>> This help for guest interrupts debugging. If the vIRQ is not allocate,
> >>>>> this means that nothing is wired to it.
> >>>>
> >>>> Should we short circuit the rest of the enable operation for this IRQ
> >>>> then? i.e. implement such writes as ignored, e.g. not reflect it in
> >>>> reads of ISENABLER etc.
> >>>>
> >>>> What (if anything) does the GIC spec have to say on the subject?
> >>>
> >>> "A register bit corresponding to an unimplemented interrupt is RAZ/WI."
> >>>
> >>> The goal of this print was mostly for debugging physical IRQ routed to a
> >>> guest.
> >>>
> >>> I could extend to ignore write to any register that should be RAZ/WI for
> >>> this specific interrupt.
> >>
> >> Since those are the defined semantics I think that is the best thing to
> >> do.
> > 
> > Ok. I will look at it to see how we can implement it.
> 
> So I looked to the code. It may need some rework to effectively
> implement most of registers bits RAZ/WI when the interrupt doesn't exist.
> 
> As this series is required for the ACPI series, I suggest to defer this
> work in a follow-up series.

OK.

[PATCH for-4.6 4/4] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt

[Julien Grall]

Use the new vgic interface to know which virtual PPI is free and use it
for the event channel code.

At the DOM0 creation time, Xen still don't know which vIRQ will be free.
All the vIRQ will be reserved when we parse the device tree. So allocate
when the hypervisor node is created.

It's safe to defer the allocation because no vIRQ can be injected as
long as the vCPU is not online.

Also correct the check in arch_domain_create to use is_hardware_domain.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain.c                | 13 ++++++++++---
 xen/arch/arm/domain_build.c          | 10 ++++++++++
 xen/arch/arm/platform.c              |  7 -------
 xen/arch/arm/platforms/xgene-storm.c |  1 -
 xen/include/asm-arm/platform.h       |  4 ----
 5 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 7221bc8..7d14377 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -543,10 +543,17 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
     if ( (rc = domain_vtimer_init(d)) != 0 )
         goto fail;
 
-    if ( d->domain_id )
+    /*
+     * The hardware domain will get a PPI later in
+     * arch/arm/domain_build.c  depending on the
+     * interrupt map of the hardware.
+     */
+    if ( !is_hardware_domain(d) )
+    {
         d->arch.evtchn_irq = GUEST_EVTCHN_PPI;
-    else
-        d->arch.evtchn_irq = platform_dom0_evtchn_ppi();
+        /* At this stage vgic_reserve_virq should never fail */
+        BUG_ON(vgic_reserve_virq(d, GUEST_EVTCHN_PPI));
+    }
 
     /*
      * Virtual UART is only used by linux early printk and decompress code.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c238c8f..8dedc60 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -625,6 +625,16 @@ static int make_hypervisor_node(struct domain *d,
         return res;
 
     /*
+     * The allocation of the event channel IRQ has been deferred until
+     * now. At this time, all PPIs use by DOM0 has been registered
+     */
+    res = vgic_allocate_virq(d, 0);
+    if ( res < 0 )
+        return -FDT_ERR_XEN(ENOSPC);
+
+    d->arch.evtchn_irq = res;
+
+    /*
      * interrupts is evtchn upcall:
      *  - Active-low level-sensitive
      *  - All cpus
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index cb4cda8..d016797 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -160,13 +160,6 @@ bool_t platform_device_is_blacklisted(const struct dt_device_node *node)
     return dt_match_node(blacklist, node);
 }
 
-unsigned int platform_dom0_evtchn_ppi(void)
-{
-    if ( platform && platform->dom0_evtchn_ppi )
-        return platform->dom0_evtchn_ppi;
-    return GUEST_EVTCHN_PPI;
-}
-
 void platform_dom0_gnttab(paddr_t *start, paddr_t *size)
 {
     if ( platform && platform->dom0_gnttab_size )
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 416d42c..b0808b8 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -232,7 +232,6 @@ PLATFORM_START(xgene_storm, "APM X-GENE STORM")
     .quirks = xgene_storm_quirks,
     .specific_mapping = xgene_storm_specific_mapping,
 
-    .dom0_evtchn_ppi = 24,
     .dom0_gnttab_start = 0x1f800000,
     .dom0_gnttab_size = 0x20000,
 PLATFORM_END
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index eefaca6..4eba37b 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -38,10 +38,6 @@ struct platform_desc {
      */
     const struct dt_device_match *blacklist_dev;
     /*
-     * The IRQ (PPI) to use to inject event channels to dom0.
-     */
-    unsigned int dom0_evtchn_ppi;
-    /*
      * The location of a region of physical address space which dom0
      * can use for grant table mappings. If size is zero defaults to
      * 0xb0000000-0xb0020000.
-- 
2.1.3

Re: [PATCH for-4.6 4/4] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt

[Julien Grall]

On 12/12/14 14:43, Julien Grall wrote:
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 7221bc8..7d14377 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -543,10 +543,17 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
>      if ( (rc = domain_vtimer_init(d)) != 0 )
>          goto fail;
>  
> -    if ( d->domain_id )
> +    /*
> +     * The hardware domain will get a PPI later in
> +     * arch/arm/domain_build.c  depending on the
> +     * interrupt map of the hardware.
> +     */
> +    if ( !is_hardware_domain(d) )
> +    {
>          d->arch.evtchn_irq = GUEST_EVTCHN_PPI;
> -    else
> -        d->arch.evtchn_irq = platform_dom0_evtchn_ppi();
> +        /* At this stage vgic_reserve_virq should never fail */
> +        BUG_ON(vgic_reserve_virq(d, GUEST_EVTCHN_PPI));

I forgot the "!" in the BUG_ON. This line should be:

BUG_ON(!..)


-- 
Julien Grall

Re: [PATCH for-4.6 4/4] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt

[Stefano Stabellini]

On Fri, 12 Dec 2014, Julien Grall wrote:
> Use the new vgic interface to know which virtual PPI is free and use it
> for the event channel code.
> 
> At the DOM0 creation time, Xen still don't know which vIRQ will be free.
> All the vIRQ will be reserved when we parse the device tree. So allocate
> when the hypervisor node is created.
> 
> It's safe to defer the allocation because no vIRQ can be injected as
> long as the vCPU is not online.
> 
> Also correct the check in arch_domain_create to use is_hardware_domain.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain.c                | 13 ++++++++++---
>  xen/arch/arm/domain_build.c          | 10 ++++++++++
>  xen/arch/arm/platform.c              |  7 -------
>  xen/arch/arm/platforms/xgene-storm.c |  1 -
>  xen/include/asm-arm/platform.h       |  4 ----
>  5 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 7221bc8..7d14377 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -543,10 +543,17 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
>      if ( (rc = domain_vtimer_init(d)) != 0 )
>          goto fail;
>  
> -    if ( d->domain_id )
> +    /*
> +     * The hardware domain will get a PPI later in
> +     * arch/arm/domain_build.c  depending on the
> +     * interrupt map of the hardware.
> +     */
> +    if ( !is_hardware_domain(d) )
> +    {
>          d->arch.evtchn_irq = GUEST_EVTCHN_PPI;
> -    else
> -        d->arch.evtchn_irq = platform_dom0_evtchn_ppi();
> +        /* At this stage vgic_reserve_virq should never fail */
> +        BUG_ON(vgic_reserve_virq(d, GUEST_EVTCHN_PPI));
> +    }

Why do we still need this, if we have another vgic_allocate_virq call in
make_hypervisor_node? Wouldn't that work for DomUs too?


>      /*
>       * Virtual UART is only used by linux early printk and decompress code.
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c238c8f..8dedc60 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -625,6 +625,16 @@ static int make_hypervisor_node(struct domain *d,
>          return res;
>  
>      /*
> +     * The allocation of the event channel IRQ has been deferred until
> +     * now. At this time, all PPIs use by DOM0 has been registered
> +     */
> +    res = vgic_allocate_virq(d, 0);
> +    if ( res < 0 )
> +        return -FDT_ERR_XEN(ENOSPC);
> +
> +    d->arch.evtchn_irq = res;
> +
> +    /*
>       * interrupts is evtchn upcall:
>       *  - Active-low level-sensitive
>       *  - All cpus
> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> index cb4cda8..d016797 100644
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -160,13 +160,6 @@ bool_t platform_device_is_blacklisted(const struct dt_device_node *node)
>      return dt_match_node(blacklist, node);
>  }
>  
> -unsigned int platform_dom0_evtchn_ppi(void)
> -{
> -    if ( platform && platform->dom0_evtchn_ppi )
> -        return platform->dom0_evtchn_ppi;
> -    return GUEST_EVTCHN_PPI;
> -}
> -
>  void platform_dom0_gnttab(paddr_t *start, paddr_t *size)
>  {
>      if ( platform && platform->dom0_gnttab_size )
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index 416d42c..b0808b8 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -232,7 +232,6 @@ PLATFORM_START(xgene_storm, "APM X-GENE STORM")
>      .quirks = xgene_storm_quirks,
>      .specific_mapping = xgene_storm_specific_mapping,
>  
> -    .dom0_evtchn_ppi = 24,
>      .dom0_gnttab_start = 0x1f800000,
>      .dom0_gnttab_size = 0x20000,
>  PLATFORM_END
> diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
> index eefaca6..4eba37b 100644
> --- a/xen/include/asm-arm/platform.h
> +++ b/xen/include/asm-arm/platform.h
> @@ -38,10 +38,6 @@ struct platform_desc {
>       */
>      const struct dt_device_match *blacklist_dev;
>      /*
> -     * The IRQ (PPI) to use to inject event channels to dom0.
> -     */
> -    unsigned int dom0_evtchn_ppi;
> -    /*
>       * The location of a region of physical address space which dom0
>       * can use for grant table mappings. If size is zero defaults to
>       * 0xb0000000-0xb0020000.
> -- 
> 2.1.3
> 

Re: [PATCH for-4.6 4/4] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt

[Julien Grall]

Hi Stefano,

On 15/12/14 15:35, Stefano Stabellini wrote:
> On Fri, 12 Dec 2014, Julien Grall wrote:
>> Use the new vgic interface to know which virtual PPI is free and use it
>> for the event channel code.
>>
>> At the DOM0 creation time, Xen still don't know which vIRQ will be free.
>> All the vIRQ will be reserved when we parse the device tree. So allocate
>> when the hypervisor node is created.
>>
>> It's safe to defer the allocation because no vIRQ can be injected as
>> long as the vCPU is not online.
>>
>> Also correct the check in arch_domain_create to use is_hardware_domain.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/domain.c                | 13 ++++++++++---
>>  xen/arch/arm/domain_build.c          | 10 ++++++++++
>>  xen/arch/arm/platform.c              |  7 -------
>>  xen/arch/arm/platforms/xgene-storm.c |  1 -
>>  xen/include/asm-arm/platform.h       |  4 ----
>>  5 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 7221bc8..7d14377 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -543,10 +543,17 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
>>      if ( (rc = domain_vtimer_init(d)) != 0 )
>>          goto fail;
>>  
>> -    if ( d->domain_id )
>> +    /*
>> +     * The hardware domain will get a PPI later in
>> +     * arch/arm/domain_build.c  depending on the
>> +     * interrupt map of the hardware.
>> +     */
>> +    if ( !is_hardware_domain(d) )
>> +    {
>>          d->arch.evtchn_irq = GUEST_EVTCHN_PPI;
>> -    else
>> -        d->arch.evtchn_irq = platform_dom0_evtchn_ppi();
>> +        /* At this stage vgic_reserve_virq should never fail */
>> +        BUG_ON(vgic_reserve_virq(d, GUEST_EVTCHN_PPI));
>> +    }
> 
> Why do we still need this, if we have another vgic_allocate_virq call in
> make_hypervisor_node? Wouldn't that work for DomUs too?

Because make_hypervisor_node is only used for DOM0. Futhermore, DOMUs
are using a specific hardcoded layout (see xen/include/public/arch-arm.h).

Regards,

-- 
Julien Grall

Re: [PATCH for-4.6 4/4] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt

[Ian Campbell]

On Mon, 2014-12-15 at 16:09 +0000, Julien Grall wrote:
> Hi Stefano,
> 
> On 15/12/14 15:35, Stefano Stabellini wrote:
> > On Fri, 12 Dec 2014, Julien Grall wrote:
> >> Use the new vgic interface to know which virtual PPI is free and use it
> >> for the event channel code.
> >>
> >> At the DOM0 creation time, Xen still don't know which vIRQ will be free.
> >> All the vIRQ will be reserved when we parse the device tree. So allocate
> >> when the hypervisor node is created.
> >>
> >> It's safe to defer the allocation because no vIRQ can be injected as
> >> long as the vCPU is not online.
> >>
> >> Also correct the check in arch_domain_create to use is_hardware_domain.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> ---
> >>  xen/arch/arm/domain.c                | 13 ++++++++++---
> >>  xen/arch/arm/domain_build.c          | 10 ++++++++++
> >>  xen/arch/arm/platform.c              |  7 -------
> >>  xen/arch/arm/platforms/xgene-storm.c |  1 -
> >>  xen/include/asm-arm/platform.h       |  4 ----
> >>  5 files changed, 20 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> >> index 7221bc8..7d14377 100644
> >> --- a/xen/arch/arm/domain.c
> >> +++ b/xen/arch/arm/domain.c
> >> @@ -543,10 +543,17 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
> >>      if ( (rc = domain_vtimer_init(d)) != 0 )
> >>          goto fail;
> >>  
> >> -    if ( d->domain_id )
> >> +    /*
> >> +     * The hardware domain will get a PPI later in
> >> +     * arch/arm/domain_build.c  depending on the
> >> +     * interrupt map of the hardware.
> >> +     */
> >> +    if ( !is_hardware_domain(d) )
> >> +    {
> >>          d->arch.evtchn_irq = GUEST_EVTCHN_PPI;
> >> -    else
> >> -        d->arch.evtchn_irq = platform_dom0_evtchn_ppi();
> >> +        /* At this stage vgic_reserve_virq should never fail */
> >> +        BUG_ON(vgic_reserve_virq(d, GUEST_EVTCHN_PPI));
> >> +    }
> > 
> > Why do we still need this, if we have another vgic_allocate_virq call in
> > make_hypervisor_node? Wouldn't that work for DomUs too?
> 
> Because make_hypervisor_node is only used for DOM0.

Yeah.

> Futhermore, DOMUs
> are using a specific hardcoded layout (see xen/include/public/arch-arm.h).

They don't actually have to be, but exposing vgic_allocate_virq to the
tools would be overkill, so hardcoding is the pragmatic choice.

We could e.g. randomise the PPI in the tools, to stop people making
assumptions. (If we were feeling mean of course...)

Ian.

Re: [PATCH for-4.6 4/4] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt

[Julien Grall]

On 13/01/15 15:58, Ian Campbell wrote:
> They don't actually have to be, but exposing vgic_allocate_virq to the
> tools would be overkill, so hardcoding is the pragmatic choice.
> 
> We could e.g. randomise the PPI in the tools, to stop people making
> assumptions. (If we were feeling mean of course...)

It would be nice to have a such think. If not randomize at each boot,
maybe per Xen version.

Though it would break mini-os on ARM as the IRQ is hardcoded in the OS.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.6 4/4] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt

[Ian Campbell]

On Wed, 2015-01-14 at 12:24 +0000, Julien Grall wrote:
> On 13/01/15 15:58, Ian Campbell wrote:
> > They don't actually have to be, but exposing vgic_allocate_virq to the
> > tools would be overkill, so hardcoding is the pragmatic choice.
> > 
> > We could e.g. randomise the PPI in the tools, to stop people making
> > assumptions. (If we were feeling mean of course...)
> 
> It would be nice to have a such think. If not randomize at each boot,
> maybe per Xen version.

Like I say, that would be quite mean of us. (I was 99% joking about this
FWIW, perhaps I needed to ring that bell a bit more though).

> Though it would break mini-os on ARM as the IRQ is hardcoded in the OS.

AIUI there is an eventual desire to have an ARM mini-os binary be
portable across Xen versions, which will require dtb parsing for this
stuff (actually, I thought the latest patches had added a bunch of such
stuff, but maybe not this specific aspect).

Ian.