Xen devel - Mar 2013 - [PATCH 0/4] xen/arm: guest SMP support

Hi all,
this small patch series implement guest SMP support for ARM, using the
ARM PSCI interface for secondary cpu bringup.

Stefano Stabellini (4):
      xen/arm: basic PSCI support, implement cpu_on
      xen/arm: support for guest SGI
      xen/arm: support vcpu_op hypercalls
      xen: move VCPUOP_register_vcpu_info to common code

 xen/arch/arm/domain.c        |   66 ++++++++++++++++++++++++
 xen/arch/arm/mm.c            |   10 ++++
 xen/arch/arm/traps.c         |   42 ++++++++++++++++
 xen/arch/arm/vgic.c          |   53 ++++++++++++++++++--
 xen/arch/x86/domain.c        |  113 ------------------------------------------
 xen/common/domain.c          |  110 ++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/domain.h |    4 ++
 xen/include/asm-arm/gic.h    |    3 +
 xen/include/asm-x86/domain.h |    3 -
 xen/include/xen/domain.h     |    3 +
 xen/include/xen/sched.h      |    3 +
 11 files changed, 290 insertions(+), 120 deletions(-)

Cheers,

Stefano

Implement support for ARM Power State Coordination Interface, PSCI in
short. The current implementation is based on HVC and only supports the
cpu_on call.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: will.deacon@arm.com
CC: marc.zyngier@arm.com
---
 xen/arch/arm/domain.c        |   66 ++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/traps.c         |   41 ++++++++++++++++++++++++++
 xen/include/asm-arm/domain.h |    4 ++
 3 files changed, 111 insertions(+), 0 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2506369..6542233 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -584,6 +584,72 @@ void arch_dump_vcpu_info(struct vcpu *v)
 {
 }
 
+int do_psci_cpu_on(uint32_t vcpuid, uint32_t entry_point)
+{
+    struct vcpu *v;
+    struct domain *d = current->domain;
+    struct vcpu_guest_context *ctxt;
+    int rc;
+
+    if ( (vcpuid < 0) || (vcpuid >= MAX_VIRT_CPUS) )
+        return -EINVAL;
+
+    if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
+        return -ENOENT;
+
+    if ( v->is_initialised )
+        return -EEXIST;
+    if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
+        return -ENOMEM;
+
+    ctxt->user_regs.pc = entry_point;
+    /* Linux boot protocol. See linux.Documentation/arm/Booting. */
+    ctxt->user_regs.r0 = 0; /* SBZ */
+    /* Machine ID: We use DTB therefore no machine id */
+    ctxt->user_regs.r1 = 0xffffffff;
+    /* ATAGS/DTB: We currently require that the guest kernel to be
+     * using CONFIG_ARM_APPENDED_DTB. Ensure that r2 does not look
+     * like a valid pointer to a set of ATAGS or a DTB.
+     */
+    ctxt->user_regs.r2 = 0xffffffff;
+    ctxt->sctlr = SCTLR_BASE;
+    ctxt->ttbr0 = 0;
+    ctxt->ttbr1 = 0;
+    ctxt->ttbcr = 0; /* Defined Reset Value */
+    ctxt->user_regs.cpsr =
PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC;
+    ctxt->flags = VGCF_online;
+
+    domain_lock(d);
+    rc = arch_set_info_guest(v, ctxt);
+    free_vcpu_guest_context(ctxt);
+
+    if ( rc < 0 )
+    {
+        domain_unlock(d);
+        return rc;
+    }
+
+    clear_bit(_VPF_down, &v->pause_flags);
+    vcpu_wake(v);
+
+    domain_unlock(d);
+
+    return 0;
+}
+
+int do_psci_cpu_off(uint32_t power_state)
+{
+    return -ENOSYS;
+}
+int do_psci_cpu_suspend(uint32_t power_state, uint32_t entry_point)
+{
+    return -ENOSYS;
+}
+int do_psci_migrate(uint32_t vcpuid)
+{
+    return -ENOSYS;
+}
+
 void vcpu_mark_events_pending(struct vcpu *v)
 {
     int already_pending = test_and_set_bit(
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index b313e7a..1deb92c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -498,6 +498,31 @@ static arm_hypercall_t arm_hypercall_table[] = {
     HYPERCALL(grant_table_op, 3),
 };
 
+#define __PSCI_cpu_suspend 0
+#define __PSCI_cpu_off     1
+#define __PSCI_cpu_on      2
+#define __PSCI_migrate     3
+
+typedef int (*arm_psci_fn_t)(uint32_t, uint32_t);
+
+typedef struct {
+    arm_psci_fn_t fn;
+    int nr_args;
+} arm_psci_t;
+
+#define PSCI(_name, _nr_args)                                  \
+    [ __PSCI_ ## _name ] =  {                                  \
+        .fn = (arm_psci_fn_t) &do_psci_ ## _name,              \
+        .nr_args = _nr_args,                                   \
+    }
+
+static arm_psci_t arm_psci_table[] = {
+    PSCI(cpu_suspend, 2),
+    PSCI(cpu_off, 1),
+    PSCI(cpu_on, 2),
+    PSCI(migrate, 1),
+};
+
 static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 {
     uint32_t reg, *r;
@@ -525,6 +550,20 @@ static void do_debug_trap(struct cpu_user_regs *regs,
unsigned int code)
     }
 }
 
+static void do_trap_psci(struct cpu_user_regs *regs)
+{
+    arm_psci_fn_t psci_call = NULL;
+
+    if ( regs->r0 >= ARRAY_SIZE(arm_psci_table) )
+    {
+        regs->r0 = -ENOSYS;
+        return;
+    }
+
+    psci_call = arm_psci_table[regs->r0].fn;
+    regs->r0 = psci_call(regs->r1, regs->r2);
+}
+
 static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
 {
     arm_hypercall_fn_t call = NULL;
@@ -777,6 +816,8 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs
*regs)
     case HSR_EC_HVC:
         if ( (hsr.iss & 0xff00) == 0xff00 )
             return do_debug_trap(regs, hsr.iss & 0x00ff);
+        if ( !hsr.iss )
+            return do_trap_psci(regs);
         do_trap_hypercall(regs, hsr.iss);
         break;
     case HSR_EC_DATA_ABORT_GUEST:
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 4623940..60298dd 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -170,6 +170,10 @@ struct arch_vcpu
 void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
 
+int do_psci_cpu_on(uint32_t vcpuid, uint32_t entry_point);
+int do_psci_cpu_off(uint32_t power_state);
+int do_psci_cpu_suspend(uint32_t power_state, uint32_t entry_point);
+int do_psci_migrate(uint32_t vcpuid);
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
-- 
1.7.2.5

Trap writes to GICD_SGIR, parse the requests, inject SGIs into the right
guest vcpu.

Add a useful debug printk to vgic_vcpu_inject_irq.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/vgic.c       |   53 +++++++++++++++++++++++++++++++++++++++++---
 xen/include/asm-arm/gic.h |    3 ++
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8495384..26279f9 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -370,6 +370,7 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int
n)
 
 static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 {
+    struct domain *d = v->domain;
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
     uint32_t *r = select_user_reg(regs, dabt.reg);
@@ -498,10 +499,51 @@ static int vgic_distr_mmio_write(struct vcpu *v,
mmio_info_t *info)
         goto write_ignore;
 
     case GICD_SGIR:
-        if ( dabt.size != 2 ) goto bad_width;
-        printk("vGICD: unhandled write %#"PRIx32" to
ICFGR%d\n",
-               *r, gicd_reg - GICD_ICFGR);
-        return 0;
+        {
+            cpumask_t vcpu_mask;
+            int virtual_irq;
+            int filter;
+            int vcpuid;
+            struct vcpu *vt;
+            int i;
+
+            if ( dabt.size != 2 ) goto bad_width;
+
+            filter = (*r & GICD_SGI_TARGET_LIST_MASK);
+            virtual_irq = (*r & GICD_SGI_INTID_MASK);
+
+            cpumask_clear(&vcpu_mask);
+            switch ( filter )
+            {
+                case GICD_SGI_TARGET_LIST:
+                    cpumask_bits(&vcpu_mask)[0] = (*r &
GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT;
+                    break;
+                case GICD_SGI_TARGET_OTHERS:
+                    for ( i = 0; i < d->max_vcpus; i++ )
+                    {
+                        if ( i != current->vcpu_id && d->vcpu[i]
!= NULL )
+                            cpumask_set_cpu(i, &vcpu_mask);
+                    }
+                case GICD_SGI_TARGET_SELF:
+                    cpumask_of(current->vcpu_id);
+                    break;
+                default:
+                    printk("vGICD: unhandled GICD_SGIR write %x with wrong
TargetListFilter field\n", *r);
+                    return 0;
+            }
+
+            for_each_cpu( vcpuid, &vcpu_mask )
+            {
+                if ( vcpuid >= d->max_vcpus || (vt = d->vcpu[vcpuid])
== NULL ||
+                        virtual_irq >= 16 )
+                {
+                    printk("vGICD: GICD_SGIR write %x, wrong
CPUTargetList\n", *r);
+                    return 0;
+                }
+                vgic_vcpu_inject_irq(vt, virtual_irq, 1);
+            }
+            return 1;
+        }
 
     case GICD_CPENDSGIR ... GICD_CPENDSGIRN:
         if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
@@ -600,6 +642,9 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq,
int virtual)
     /* the irq is enabled */
     if ( rank->ienable & (1 << (irq % 32)) )
         gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
+    else
+        printk("%s: trying to inject irq %d to d%d:v%d, but it is not
enabled\n",
+                __func__, irq, v->domain->domain_id, v->vcpu_id);
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
     list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 907c495..ab45b41 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -51,12 +51,15 @@
 #define GICD_SPENDSGIRN (0xF2C/4)
 #define GICD_ICPIDR2    (0xFE8/4)
 
+#define GICD_SGI_TARGET_LIST_SHIFT   (24)
+#define GICD_SGI_TARGET_LIST_MASK    (0x3UL <<
GICD_SGI_TARGET_LIST_SHIFT)
 #define GICD_SGI_TARGET_LIST   (0UL<<24)
 #define GICD_SGI_TARGET_OTHERS (1UL<<24)
 #define GICD_SGI_TARGET_SELF   (2UL<<24)
 #define GICD_SGI_TARGET_SHIFT  (16)
 #define GICD_SGI_TARGET_MASK   (0xFFUL<<GICD_SGI_TARGET_SHIFT)
 #define GICD_SGI_GROUP1        (1UL<<15)
+#define GICD_SGI_INTID_MASK    (0xFUL)
 
 #define GICC_CTLR       (0x0000/4)
 #define GICC_PMR        (0x0004/4)
-- 
1.7.2.5

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/traps.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 1deb92c..4ac4af3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -496,6 +496,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
     HYPERCALL(sysctl, 2),
     HYPERCALL(hvm_op, 2),
     HYPERCALL(grant_table_op, 3),
+    HYPERCALL(vcpu_op, 3),
 };
 
 #define __PSCI_cpu_suspend 0
-- 
1.7.2.5

Move the implementation of VCPUOP_register_vcpu_info from x86 specific
to commmon code.

Move vcpu_info_mfn from an arch specific vcpu sub-field to the common
vcpu struct.
Move the initialization of vcpu_info_mfn to common code.

Move unmap_vcpu_info and the call to unmap_vcpu_info at domain
destruction time to common code.

Implement map_domain_page_global and unmap_domain_page_global on ARM.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/mm.c            |   10 ++++
 xen/arch/x86/domain.c        |  113 ------------------------------------------
 xen/common/domain.c          |  110 ++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/domain.h |    3 -
 xen/include/xen/domain.h     |    3 +
 xen/include/xen/sched.h      |    3 +
 6 files changed, 126 insertions(+), 116 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 972fa42..a875af5 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -125,6 +125,16 @@ void clear_fixmap(unsigned map)
     flush_xen_data_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
 }
 
+void *map_domain_page_global(unsigned long mfn)
+{
+    return map_domain_page(mfn);
+}
+    
+void unmap_domain_page_global(const void *va)
+{
+    unmap_domain_page(va);
+}
+
 /* Map a page of domheap memory */
 void *map_domain_page(unsigned long mfn)
 {
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7a07c06..fe0b4ef 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -416,8 +416,6 @@ int vcpu_initialise(struct vcpu *v)
         goto done;
     }
 
-    v->arch.pv_vcpu.vcpu_info_mfn = INVALID_MFN;
-
     spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
 
     if ( !is_idle_domain(d) )
@@ -983,99 +981,6 @@ void arch_vcpu_reset(struct vcpu *v)
     }
 }
 
-/* 
- * Unmap the vcpu info page if the guest decided to place it somewhere
- * else.  This is only used from arch_domain_destroy, so there''s no
- * need to do anything clever.
- */
-static void
-unmap_vcpu_info(struct vcpu *v)
-{
-    unsigned long mfn;
-
-    if ( v->arch.pv_vcpu.vcpu_info_mfn == INVALID_MFN )
-        return;
-
-    mfn = v->arch.pv_vcpu.vcpu_info_mfn;
-    unmap_domain_page_global(v->vcpu_info);
-
-    v->vcpu_info = &dummy_vcpu_info;
-    v->arch.pv_vcpu.vcpu_info_mfn = INVALID_MFN;
-
-    put_page_and_type(mfn_to_page(mfn));
-}
-
-/* 
- * Map a guest page in and point the vcpu_info pointer at it.  This
- * makes sure that the vcpu_info is always pointing at a valid piece
- * of memory, and it sets a pending event to make sure that a pending
- * event doesn''t get missed.
- */
-static int
-map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
-{
-    struct domain *d = v->domain;
-    void *mapping;
-    vcpu_info_t *new_info;
-    struct page_info *page;
-    int i;
-
-    if ( offset > (PAGE_SIZE - sizeof(vcpu_info_t)) )
-        return -EINVAL;
-
-    if ( v->arch.pv_vcpu.vcpu_info_mfn != INVALID_MFN )
-        return -EINVAL;
-
-    /* Run this command on yourself or on other offline VCPUS. */
-    if ( (v != current) && !test_bit(_VPF_down, &v->pause_flags)
)
-        return -EINVAL;
-
-    page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
-    if ( !page )
-        return -EINVAL;
-
-    if ( !get_page_type(page, PGT_writable_page) )
-    {
-        put_page(page);
-        return -EINVAL;
-    }
-
-    mapping = __map_domain_page_global(page);
-    if ( mapping == NULL )
-    {
-        put_page_and_type(page);
-        return -ENOMEM;
-    }
-
-    new_info = (vcpu_info_t *)(mapping + offset);
-
-    if ( v->vcpu_info == &dummy_vcpu_info )
-    {
-        memset(new_info, 0, sizeof(*new_info));
-        __vcpu_info(v, new_info, evtchn_upcall_mask) = 1;
-    }
-    else
-    {
-        memcpy(new_info, v->vcpu_info, sizeof(*new_info));
-    }
-
-    v->vcpu_info = new_info;
-    v->arch.pv_vcpu.vcpu_info_mfn = page_to_mfn(page);
-
-    /* Set new vcpu_info pointer /before/ setting pending flags. */
-    wmb();
-
-    /*
-     * Mark everything as being pending just to make sure nothing gets
-     * lost.  The domain will get a spurious event, but it can cope.
-     */
-    vcpu_info(v, evtchn_upcall_pending) = 1;
-    for ( i = 0; i < BITS_PER_EVTCHN_WORD(d); i++ )
-        set_bit(i, &vcpu_info(v, evtchn_pending_sel));
-
-    return 0;
-}
-
 long
 arch_do_vcpu_op(
     int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
@@ -1112,22 +1017,6 @@ arch_do_vcpu_op(
         break;
     }
 
-    case VCPUOP_register_vcpu_info:
-    {
-        struct domain *d = v->domain;
-        struct vcpu_register_vcpu_info info;
-
-        rc = -EFAULT;
-        if ( copy_from_guest(&info, arg, 1) )
-            break;
-
-        domain_lock(d);
-        rc = map_vcpu_info(v, info.mfn, info.offset);
-        domain_unlock(d);
-
-        break;
-    }
-
     /*
      * XXX Disable for 4.0.0: __update_vcpu_system_time() writes to the given
      * virtual address even when running in another domain''s address
space.
@@ -2044,8 +1933,6 @@ int domain_relinquish_resources(struct domain *d)
                  * mappings.
                  */
                 destroy_gdt(v);
-
-                unmap_vcpu_info(v);
             }
 
             if ( d->arch.pv_domain.pirq_eoi_map != NULL )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 07f62b3..e4e145b 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -141,6 +141,7 @@ struct vcpu *alloc_vcpu(
         v->vcpu_info = ((vcpu_id < XEN_LEGACY_MAX_VCPUS)
                         ? (vcpu_info_t *)&shared_info(d,
vcpu_info[vcpu_id])
                         : &dummy_vcpu_info);
+        v->vcpu_info_mfn = INVALID_MFN;
         init_waitqueue_vcpu(v);
     }
 
@@ -493,6 +494,7 @@ int rcu_lock_live_remote_domain_by_id(domid_t dom, struct
domain **d)
 int domain_kill(struct domain *d)
 {
     int rc = 0;
+    struct vcpu *v;
 
     if ( d == current->domain )
         return -EINVAL;
@@ -516,6 +518,8 @@ int domain_kill(struct domain *d)
             BUG_ON(rc != -EAGAIN);
             break;
         }
+        for_each_vcpu ( d, v )
+            unmap_vcpu_info(v);
         d->is_dying = DOMDYING_dead;
         /* Mem event cleanup has to go here because the rings 
          * have to be put before we call put_domain. */
@@ -841,6 +845,96 @@ void vcpu_reset(struct vcpu *v)
     vcpu_unpause(v);
 }
 
+/* 
+ * Map a guest page in and point the vcpu_info pointer at it.  This
+ * makes sure that the vcpu_info is always pointing at a valid piece
+ * of memory, and it sets a pending event to make sure that a pending
+ * event doesn''t get missed.
+ */
+int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
+{
+    struct domain *d = v->domain;
+    void *mapping;
+    vcpu_info_t *new_info;
+    struct page_info *page;
+    int i;
+
+    if ( offset > (PAGE_SIZE - sizeof(vcpu_info_t)) )
+        return -EINVAL;
+
+    if ( v->vcpu_info_mfn != INVALID_MFN )
+        return -EINVAL;
+
+    /* Run this command on yourself or on other offline VCPUS. */
+    if ( (v != current) && !test_bit(_VPF_down, &v->pause_flags)
)
+        return -EINVAL;
+
+    page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
+    if ( !page )
+        return -EINVAL;
+
+    if ( !get_page_type(page, PGT_writable_page) )
+    {
+        put_page(page);
+        return -EINVAL;
+    }
+
+    mapping = __map_domain_page_global(page);
+    if ( mapping == NULL )
+    {
+        put_page_and_type(page);
+        return -ENOMEM;
+    }
+
+    new_info = (vcpu_info_t *)(mapping + offset);
+
+    if ( v->vcpu_info == &dummy_vcpu_info )
+    {
+        memset(new_info, 0, sizeof(*new_info));
+        __vcpu_info(v, new_info, evtchn_upcall_mask) = 1;
+    }
+    else
+    {
+        memcpy(new_info, v->vcpu_info, sizeof(*new_info));
+    }
+
+    v->vcpu_info = new_info;
+    v->vcpu_info_mfn = page_to_mfn(page);
+
+    /* Set new vcpu_info pointer /before/ setting pending flags. */
+    wmb();
+
+    /*
+     * Mark everything as being pending just to make sure nothing gets
+     * lost.  The domain will get a spurious event, but it can cope.
+     */
+    vcpu_info(v, evtchn_upcall_pending) = 1;
+    for ( i = 0; i < BITS_PER_EVTCHN_WORD(d); i++ )
+        set_bit(i, &vcpu_info(v, evtchn_pending_sel));
+
+    return 0;
+}
+
+/* 
+ * Unmap the vcpu info page if the guest decided to place it somewhere
+ * else.  This is only used from arch_domain_destroy, so there''s no
+ * need to do anything clever.
+ */
+void unmap_vcpu_info(struct vcpu *v)
+{
+    unsigned long mfn;
+
+    if ( v->vcpu_info_mfn == INVALID_MFN )
+        return;
+
+    mfn = v->vcpu_info_mfn;
+    unmap_domain_page_global(v->vcpu_info);
+
+    v->vcpu_info = &dummy_vcpu_info;
+    v->vcpu_info_mfn = INVALID_MFN;
+
+    put_page_and_type(mfn_to_page(mfn));
+}
 
 long do_vcpu_op(int cmd, int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
@@ -960,6 +1054,22 @@ long do_vcpu_op(int cmd, int vcpuid,
XEN_GUEST_HANDLE_PARAM(void) arg)
 
         break;
 
+    case VCPUOP_register_vcpu_info:
+    {
+        struct domain *d = v->domain;
+        struct vcpu_register_vcpu_info info;
+
+        rc = -EFAULT;
+        if ( copy_from_guest(&info, arg, 1) )
+            break;
+
+        domain_lock(d);
+        rc = map_vcpu_info(v, info.mfn, info.offset);
+        domain_unlock(d);
+
+        break;
+    }
+
 #ifdef VCPU_TRAP_NMI
     case VCPUOP_send_nmi:
         if ( !guest_handle_is_null(arg) )
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 1b2a770..fc4336e 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -369,9 +369,6 @@ struct pv_vcpu
     /* Current LDT details. */
     unsigned long shadow_ldt_mapcnt;
     spinlock_t shadow_ldt_lock;
-
-    /* Guest-specified relocation of vcpu_info. */
-    unsigned long vcpu_info_mfn;
 };
 
 struct arch_vcpu
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index d4ac50f..d0263a9 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -52,6 +52,9 @@ void free_pirq_struct(void *);
 int  vcpu_initialise(struct vcpu *v);
 void vcpu_destroy(struct vcpu *v);
 
+int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset);
+void unmap_vcpu_info(struct vcpu *v);
+
 int arch_domain_create(struct domain *d, unsigned int domcr_flags);
 
 void arch_domain_destroy(struct domain *d);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index ba0f2f8..687c2df 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -187,6 +187,9 @@ struct vcpu
 
     struct waitqueue_vcpu *waitqueue_vcpu;
 
+    /* Guest-specified relocation of vcpu_info. */
+    unsigned long vcpu_info_mfn;
+
     struct arch_vcpu arch;
 };
 
-- 
1.7.2.5

On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini
wrote:
> Implement support for ARM Power State Coordination Interface, PSCI in
> short. The current implementation is based on HVC and only supports the
> cpu_on call.
Doesn''t the PSCI interface require the use of SMC not HVC?
I thought I heard Charles say at connect that there was now a PSCI v2,
and I suspect I''m looking at the v1 document (which mentions HVC only
in
passing). Which interface did you implement?

Anyhow, we can trap SMCs to the hypervisor by setting the right control
register bits. We should do this anyway -- no good can come of a guest
making a call direct to the monitor!
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: will.deacon@arm.com
> CC: marc.zyngier@arm.com
> ---
>  xen/arch/arm/domain.c        |   66
++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/traps.c         |   41 ++++++++++++++++++++++++++
>  xen/include/asm-arm/domain.h |    4 ++
>  3 files changed, 111 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 2506369..6542233 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
Perhaps this stuff and the obvious stuff from traps.c should be in a new
psci.c instead?
> @@ -584,6 +584,72 @@ void arch_dump_vcpu_info(struct vcpu *v)
>  {
>  }
>  
> +int do_psci_cpu_on(uint32_t vcpuid, uint32_t entry_point)
> +{
> +    struct vcpu *v;
> +    struct domain *d = current->domain;
> +    struct vcpu_guest_context *ctxt;
> +    int rc;
> +
> +    if ( (vcpuid < 0) || (vcpuid >= MAX_VIRT_CPUS) )
> +        return -EINVAL;
> +
> +    if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL
)
> +        return -ENOENT;
> +
> +    if ( v->is_initialised )
> +        return -EEXIST;
> +    if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
> +        return -ENOMEM;
Are the PSCI error codes really the same as the Xen errno values?
> +
> +    ctxt->user_regs.pc = entry_point;
> +    /* Linux boot protocol. See linux.Documentation/arm/Booting. */
> +    ctxt->user_regs.r0 = 0; /* SBZ */
> +    /* Machine ID: We use DTB therefore no machine id */
> +    ctxt->user_regs.r1 = 0xffffffff;
> +    /* ATAGS/DTB: We currently require that the guest kernel to be
> +     * using CONFIG_ARM_APPENDED_DTB. Ensure that r2 does not look
> +     * like a valid pointer to a set of ATAGS or a DTB.
> +     */
> +    ctxt->user_regs.r2 = 0xffffffff;
Is this protocol relevant to secondary CPUs? I''d have expected not and
that r[0-15] are all undefined on startup?
> +        if ( !hsr.iss )
> +            return do_trap_psci(regs);
Ugh, using ISS==0 for this interface is a bit unfortunate, who
arbitrates this namespace?

(nit: you are logically testing for 0 not False, so I think == 0 would
be nicer than !, or even a switch statement might be good)
>          do_trap_hypercall(regs, hsr.iss);
>          break;
>      case HSR_EC_DATA_ABORT_GUEST:

On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini
wrote:
> Trap writes to GICD_SGIR, parse the requests, inject SGIs into the right
> guest vcpu.
> 
> Add a useful debug printk to vgic_vcpu_inject_irq.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/vgic.c       |   53
+++++++++++++++++++++++++++++++++++++++++---
>  xen/include/asm-arm/gic.h |    3 ++
>  2 files changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 8495384..26279f9 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -370,6 +370,7 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t
r, int n)
>  
>  static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>  {
> +    struct domain *d = v->domain;
>      struct hsr_dabt dabt = info->dabt;
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
>      uint32_t *r = select_user_reg(regs, dabt.reg);
> @@ -498,10 +499,51 @@ static int vgic_distr_mmio_write(struct vcpu *v,
mmio_info_t *info)
>          goto write_ignore;
>  
>      case GICD_SGIR:
> -        if ( dabt.size != 2 ) goto bad_width;
> -        printk("vGICD: unhandled write %#"PRIx32" to
ICFGR%d\n",
> -               *r, gicd_reg - GICD_ICFGR);
> -        return 0;
> +        {
> +            cpumask_t vcpu_mask;
> +            int virtual_irq;
> +            int filter;
> +            int vcpuid;
> +            struct vcpu *vt;
> +            int i;
> +
> +            if ( dabt.size != 2 ) goto bad_width;
> +
> +            filter = (*r & GICD_SGI_TARGET_LIST_MASK);
> +            virtual_irq = (*r & GICD_SGI_INTID_MASK);
> +
> +            cpumask_clear(&vcpu_mask);
> +            switch ( filter )
> +            {
> +                case GICD_SGI_TARGET_LIST:
> +                    cpumask_bits(&vcpu_mask)[0] = (*r &
GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT;
Is this the best available way to set a cpumask in Xen?
> +                    break;
> +                case GICD_SGI_TARGET_OTHERS:
> +                    for ( i = 0; i < d->max_vcpus; i++ )
> +                    {
> +                        if ( i != current->vcpu_id &&
d->vcpu[i] != NULL )
> +                            cpumask_set_cpu(i, &vcpu_mask);
Can this be done with cpumask_any/all and then clearing the current cpu?
> +                    }
> +                case GICD_SGI_TARGET_SELF:
> +                    cpumask_of(current->vcpu_id);
> +                    break;
> +                default:
> +                    printk("vGICD: unhandled GICD_SGIR write %x with
wrong TargetListFilter field\n", *r);
> +                    return 0;
> +            }
> +
> +            for_each_cpu( vcpuid, &vcpu_mask )
> +            {
> +                if ( vcpuid >= d->max_vcpus || (vt =
d->vcpu[vcpuid]) == NULL ||
> +                        virtual_irq >= 16 )
> +                {
> +                    printk("vGICD: GICD_SGIR write %x, wrong
CPUTargetList\n", *r);
> +                    return 0;
> +                }
> +                vgic_vcpu_inject_irq(vt, virtual_irq, 1);
> +            }
> +            return 1;
> +        }
>  
>      case GICD_CPENDSGIR ... GICD_CPENDSGIRN:
>          if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
> @@ -600,6 +642,9 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int
irq, int virtual)
>      /* the irq is enabled */
>      if ( rank->ienable & (1 << (irq % 32)) )
>          gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
> +    else
> +        printk("%s: trying to inject irq %d to d%d:v%d, but it is not
enabled\n",
> +                __func__, irq, v->domain->domain_id, v->vcpu_id);
>  
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>      list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs,
inflight )
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 907c495..ab45b41 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -51,12 +51,15 @@
>  #define GICD_SPENDSGIRN (0xF2C/4)
>  #define GICD_ICPIDR2    (0xFE8/4)
>  
> +#define GICD_SGI_TARGET_LIST_SHIFT   (24)
> +#define GICD_SGI_TARGET_LIST_MASK    (0x3UL <<
GICD_SGI_TARGET_LIST_SHIFT)
>  #define GICD_SGI_TARGET_LIST   (0UL<<24)
>  #define GICD_SGI_TARGET_OTHERS (1UL<<24)
>  #define GICD_SGI_TARGET_SELF   (2UL<<24)
>  #define GICD_SGI_TARGET_SHIFT  (16)
>  #define GICD_SGI_TARGET_MASK   (0xFFUL<<GICD_SGI_TARGET_SHIFT)
>  #define GICD_SGI_GROUP1        (1UL<<15)
> +#define GICD_SGI_INTID_MASK    (0xFUL)
>  
>  #define GICC_CTLR       (0x0000/4)
>  #define GICC_PMR        (0x0004/4)

On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:

It''s possible that we might want to restrict the available operations?
e.g. remove VPCUOP_initialize/up if we are doing this via PSCI instead?

My concern is that there have been security bugs in VCPUOP_initialize on
x86 in the past and if we don''t have to expose that possibility on ARM
lets not.

Ian.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/traps.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 1deb92c..4ac4af3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -496,6 +496,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
>      HYPERCALL(sysctl, 2),
>      HYPERCALL(hvm_op, 2),
>      HYPERCALL(grant_table_op, 3),
> +    HYPERCALL(vcpu_op, 3),
>  };
>  
>  #define __PSCI_cpu_suspend 0

On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini
wrote:
> Move the implementation of VCPUOP_register_vcpu_info from x86 specific
> to commmon code.
> 
> Move vcpu_info_mfn from an arch specific vcpu sub-field to the common
> vcpu struct.
> Move the initialization of vcpu_info_mfn to common code.
> 
> Move unmap_vcpu_info and the call to unmap_vcpu_info at domain
> destruction time to common code.
> 
> Implement map_domain_page_global and unmap_domain_page_global on ARM.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/mm.c            |   10 ++++
>  xen/arch/x86/domain.c        |  113
------------------------------------------
>  xen/common/domain.c          |  110
++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/domain.h |    3 -
>  xen/include/xen/domain.h     |    3 +
>  xen/include/xen/sched.h      |    3 +
>  6 files changed, 126 insertions(+), 116 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 972fa42..a875af5 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -125,6 +125,16 @@ void clear_fixmap(unsigned map)
>      flush_xen_data_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
>  }
>  
> +void *map_domain_page_global(unsigned long mfn)
> +{
> +    return map_domain_page(mfn);
This isn''t a global mapping, is it? IIRC map_domain_page is a vcpu
local
mapping.

I''ve presumed that most of the rest of this patch is just motion.

Given the common & x86 impact you should CC Keir + Jan too...

Ian.

On 09/04/13 14:57, Ian Campbell wrote:
> On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:
>> Implement support for ARM Power State Coordination Interface, PSCI in
>> short. The current implementation is based on HVC and only supports the
>> cpu_on call.
> 
> Doesn''t the PSCI interface require the use of SMC not HVC?
You can use both, and KVM uses HVC.
> I thought I heard Charles say at connect that there was now a PSCI v2,
> and I suspect I''m looking at the v1 document (which mentions HVC
only in
> passing). Which interface did you implement?
> 
> Anyhow, we can trap SMCs to the hypervisor by setting the right control
> register bits. We should do this anyway -- no good can come of a guest
> making a call direct to the monitor!
Trapping guest access to Secure mode is always a good idea! ;-)

Unfortunately, there''s a catch on ARMv8. If the CPU doesn''t
implement
secure mode, then SMC will UNDEF at the current exception level (not
trapping to EL2). Which means that for ARMv8, you basically have to
mandate HVC for PSCI at the HYP level...

	M.
-- 
Jazz is not dead. It just smells funny...

On Tue, 2013-04-09 at 15:23 +0100, Marc Zyngier wrote:
> On 09/04/13 14:57, Ian Campbell wrote:
> > On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:
> >> Implement support for ARM Power State Coordination Interface, PSCI
in
> >> short. The current implementation is based on HVC and only
supports the
> >> cpu_on call.
> > 
> > Doesn''t the PSCI interface require the use of SMC not HVC?
> 
> You can use both, and KVM uses HVC.
It makes good sense for Xen to follow suit then I think.
> > I thought I heard Charles say at connect that there was now a PSCI v2,
> > and I suspect I''m looking at the v1 document (which mentions
HVC only in
> > passing). Which interface did you implement?
> > 
> > Anyhow, we can trap SMCs to the hypervisor by setting the right
control
> > register bits. We should do this anyway -- no good can come of a guest
> > making a call direct to the monitor!
> 
> Trapping guest access to Secure mode is always a good idea! ;-)
:-)
> Unfortunately, there''s a catch on ARMv8. If the CPU
doesn''t implement
> secure mode, then SMC will UNDEF at the current exception level (not
> trapping to EL2). Which means that for ARMv8, you basically have to
> mandate HVC for PSCI at the HYP level...
That pretty much seals it then!

Do I infer that on v7 SMC w/o security extensions will trap?

Ian.

On 10/04/13 11:13, Ian Campbell wrote:
> On Tue, 2013-04-09 at 15:23 +0100, Marc Zyngier wrote:
>> On 09/04/13 14:57, Ian Campbell wrote:
>>> On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:
>>>> Implement support for ARM Power State Coordination Interface,
PSCI in
>>>> short. The current implementation is based on HVC and only
supports the
>>>> cpu_on call.
>>>
>>> Doesn''t the PSCI interface require the use of SMC not HVC?
>>
>> You can use both, and KVM uses HVC.
> 
> It makes good sense for Xen to follow suit then I think.
> 
>>> I thought I heard Charles say at connect that there was now a PSCI
v2,
>>> and I suspect I''m looking at the v1 document (which
mentions HVC only in
>>> passing). Which interface did you implement?
>>>
>>> Anyhow, we can trap SMCs to the hypervisor by setting the right
control
>>> register bits. We should do this anyway -- no good can come of a
guest
>>> making a call direct to the monitor!
>>
>> Trapping guest access to Secure mode is always a good idea! ;-)
> 
> :-)
> 
>> Unfortunately, there''s a catch on ARMv8. If the CPU
doesn''t implement
>> secure mode, then SMC will UNDEF at the current exception level (not
>> trapping to EL2). Which means that for ARMv8, you basically have to
>> mandate HVC for PSCI at the HYP level...
> 
> That pretty much seals it then!
> 
> Do I infer that on v7 SMC w/o security extensions will trap?
ARMv7 mandates security extensions if you have virtualization
extensions, so this is a moot point... ;-)

	M.
-- 
Jazz is not dead. It just smells funny...

On Wed, 2013-04-10 at 11:41 +0100, Marc Zyngier wrote:
> On 10/04/13 11:13, Ian Campbell wrote:
> > On Tue, 2013-04-09 at 15:23 +0100, Marc Zyngier wrote:
> >> On 09/04/13 14:57, Ian Campbell wrote:
> >>> On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:
> >>>> Implement support for ARM Power State Coordination
Interface, PSCI in
> >>>> short. The current implementation is based on HVC and only
supports the
> >>>> cpu_on call.
> >>>
> >>> Doesn''t the PSCI interface require the use of SMC not
HVC?
> >>
> >> You can use both, and KVM uses HVC.
> > 
> > It makes good sense for Xen to follow suit then I think.
> > 
> >>> I thought I heard Charles say at connect that there was now a
PSCI v2,
> >>> and I suspect I''m looking at the v1 document (which
mentions HVC only in
> >>> passing). Which interface did you implement?
> >>>
> >>> Anyhow, we can trap SMCs to the hypervisor by setting the
right control
> >>> register bits. We should do this anyway -- no good can come of
a guest
> >>> making a call direct to the monitor!
> >>
> >> Trapping guest access to Secure mode is always a good idea! ;-)
> > 
> > :-)
> > 
> >> Unfortunately, there''s a catch on ARMv8. If the CPU
doesn''t implement
> >> secure mode, then SMC will UNDEF at the current exception level
(not
> >> trapping to EL2). Which means that for ARMv8, you basically have
to
> >> mandate HVC for PSCI at the HYP level...
> > 
> > That pretty much seals it then!
> > 
> > Do I infer that on v7 SMC w/o security extensions will trap?
> 
> ARMv7 mandates security extensions if you have virtualization
> extensions, so this is a moot point... ;-)
I hadn''t noticed (or perhaps, remembered) that, thanks!

Ian.

On Wed, 2013-04-10 at 11:13 +0100, Ian Campbell wrote:
> On Tue, 2013-04-09 at 15:23 +0100, Marc Zyngier wrote:
> > On 09/04/13 14:57, Ian Campbell wrote:
> > > On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:
> > >> Implement support for ARM Power State Coordination Interface,
PSCI in
> > >> short. The current implementation is based on HVC and only
supports the
> > >> cpu_on call.
> > > 
> > > Doesn''t the PSCI interface require the use of SMC not
HVC?
> > 
> > You can use both, and KVM uses HVC.
> 
> It makes good sense for Xen to follow suit then I think.
Stefano, this means that Xen might need to rewrite the dom0 dtb to
s/smc/hvc/ I think?

Ian.

Adding Charles and the l-a-k list which I hadn''t noticed
wasn''t CC''d
before. Are there other lists which would be useful to include?

On Tue, 2013-04-09 at 14:57 +0100, Ian Campbell wrote:
> > +        if ( !hsr.iss )
> > +            return do_trap_psci(regs);
> 
> Ugh, using ISS==0 for this interface is a bit unfortunate, who
> arbitrates this namespace?
Xen chose to use a non-0 immediate for its hypercalls so we are lucky
that we don''t have to play tricks to distinguish PSCI calls from
hypercalls but it seems to me that either there needs to be some
semi-formal registry for smv/hvc immediates to avoid clashes or that
interfaces need to pick a random non-zero number to try and minimise the
chances of a clash.

Or maybe it gets pushed down a layer and the registry is of r0/x0
function ids and the immediate argument is generally ignored?

But in general we need some way for interfaces provided by hypervisors
and secure mode monitors to safely co-exist, I think, both for the case
where the hypervisor is implementing a shim for a secure mode interface
(i.e. implementing PSCI with hvc) and for the case where there are
multiple interfaces at each layer (e.g. hypervisors sometimes want to
implement foreign hypervisor interfaces and I can easily imagine other
firmware level interfaces than PSCI coming into existence in the
future).

In the specific case of PSCI I couldn''t actually find the calling
convention (i.e. the specific registers to use). I suspect I''m just
looking at an old version of the spec...

Ian.

On Thu, Apr 11, 2013 at 02:32:28PM +0100, Ian Campbell
wrote:
> Adding Charles and the l-a-k list which I hadn''t noticed
wasn''t CC''d
> before. Are there other lists which would be useful to include?
> 
> On Tue, 2013-04-09 at 14:57 +0100, Ian Campbell wrote:
> > > +        if ( !hsr.iss )
> > > +            return do_trap_psci(regs);
Have you seen this work?

Reading the specs, I thought that the SMC immediate is not available in
the HSR in ARMv7, or when the caller is AArch32.  ARMv7 specifies these
bits as UNK/SBZP, and the A15 TRM doesn''t seem seem to specify any
extra information.

Otherwise, you would need to go examine the trapped instruction in
guest memory.  This is even less easy than it sounds.
> > Ugh, using ISS==0 for this interface is a bit unfortunate, who
> > arbitrates this namespace?
Nobody, officially.  ARM is working to promulgate some standards in
this area, but there is plenty of pre-existing firmware and software
out there with non-standardised interfaces and it''s hard to predict
adoption timescales.  I believe nothing is published relating to this
just yet.
> Xen chose to use a non-0 immediate for its hypercalls so we are lucky
> that we don''t have to play tricks to distinguish PSCI calls from
> hypercalls but it seems to me that either there needs to be some
> semi-formal registry for smv/hvc immediates to avoid clashes or that
> interfaces need to pick a random non-zero number to try and minimise the
> chances of a clash.
> 
> Or maybe it gets pushed down a layer and the registry is of r0/x0
> function ids and the immediate argument is generally ignored?
> 
> But in general we need some way for interfaces provided by hypervisors
> and secure mode monitors to safely co-exist, I think, both for the case
> where the hypervisor is implementing a shim for a secure mode interface
> (i.e. implementing PSCI with hvc) and for the case where there are
> multiple interfaces at each layer (e.g. hypervisors sometimes want to
> implement foreign hypervisor interfaces and I can easily imagine other
> firmware level interfaces than PSCI coming into existence in the
> future).
For PSCI and similar firmware functionality, we envisage multiplexing
all interfaces based on a function number in r0, with arguments and
results constrained to r0-r7.  For PSCI, we specify SMC #0/HVC #0 as the
call mechanism.

For now, there is no global management of the r0 namespace, which
is why for PSCI we don''t make static assumptions about the numbers
on the client side.  Instead, the function number for each PSCI function
is passed in the DT.  This means that the firmware or hypervisor can
allocate the numbers as appropriate to avoid clashes.

In general, this is likely to be the best approach for Linux until a
standard probing interface exists.

In theory we could define a Xen-specific call mechanism for PSCI, but
that should be avoided unless it is absolutely unavoidable.  It sounds
like the different SMC comment field means that we don''t need to
worry for Xen/PSCI coexistence specifically, assuming that the hypervisor
can read it reliably.
> In the specific case of PSCI I couldn''t actually find the calling
> convention (i.e. the specific registers to use). I suspect I''m
just
> looking at an old version of the spec...
This is indeed vague in the existing draft.  An update should appear
soon -- Charles can comment on the current status.

Cheers
---Dave

On Thu, 11 Apr 2013, Ian Campbell wrote:
> On Wed, 2013-04-10 at 11:13 +0100, Ian Campbell wrote:
> > On Tue, 2013-04-09 at 15:23 +0100, Marc Zyngier wrote:
> > > On 09/04/13 14:57, Ian Campbell wrote:
> > > > On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:
> > > >> Implement support for ARM Power State Coordination
Interface, PSCI in
> > > >> short. The current implementation is based on HVC and
only supports the
> > > >> cpu_on call.
> > > > 
> > > > Doesn''t the PSCI interface require the use of SMC
not HVC?
> > > 
> > > You can use both, and KVM uses HVC.
> > 
> > It makes good sense for Xen to follow suit then I think.
> 
> Stefano, this means that Xen might need to rewrite the dom0 dtb to
> s/smc/hvc/ I think?
We do need to provide a dtb with the right psci method, see this patch
for a DomU example:

http://marc.info/?l=linux-arm-kernel&m=136430894810691&w=2

On Fri, 2013-04-12 at 00:42 +0100, Stefano Stabellini
wrote:
> On Thu, 11 Apr 2013, Ian Campbell wrote:
> > On Wed, 2013-04-10 at 11:13 +0100, Ian Campbell wrote:
> > > On Tue, 2013-04-09 at 15:23 +0100, Marc Zyngier wrote:
> > > > On 09/04/13 14:57, Ian Campbell wrote:
> > > > > On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini
wrote:
> > > > >> Implement support for ARM Power State Coordination
Interface, PSCI in
> > > > >> short. The current implementation is based on HVC
and only supports the
> > > > >> cpu_on call.
> > > > > 
> > > > > Doesn''t the PSCI interface require the use of
SMC not HVC?
> > > > 
> > > > You can use both, and KVM uses HVC.
> > > 
> > > It makes good sense for Xen to follow suit then I think.
> > 
> > Stefano, this means that Xen might need to rewrite the dom0 dtb to
> > s/smc/hvc/ I think?
> 
> We do need to provide a dtb with the right psci method, see this patch
> for a DomU example:
> 
> http://marc.info/?l=linux-arm-kernel&m=136430894810691&w=2
Right, I was talking about dom0 though which may already have a PSCI
entry for the physical hardware (which Xen ultimately needs to use), so
we need to rewrite it.

Ian.

On Thu, 2013-04-11 at 18:10 +0100, Dave Martin wrote:
> On Thu, Apr 11, 2013 at 02:32:28PM +0100, Ian Campbell wrote:
> > Adding Charles and the l-a-k list which I hadn''t noticed
wasn''t CC''d
> > before. Are there other lists which would be useful to include?
> > 
> > On Tue, 2013-04-09 at 14:57 +0100, Ian Campbell wrote:
> > > > +        if ( !hsr.iss )
> > > > +            return do_trap_psci(regs);
> 
> Have you seen this work?
I presume Stefano has.
> Reading the specs, I thought that the SMC immediate is not available in
> the HSR in ARMv7, or when the caller is AArch32.  ARMv7 specifies these
> bits as UNK/SBZP, and the A15 TRM doesn''t seem seem to specify any
> extra information.
I trimmed the context so you probably don''t know but this is handling
PSCI via an HVC call not SMC, Linux lets you specify which to use in the
DTB.

I didn''t know that about SMC though -- think my eye confused it with
SVC when I scanned the docs. Kind of a lame limitation...
> Otherwise, you would need to go examine the trapped instruction in
> guest memory.  This is even less easy than it sounds.
Right, luckily we avoid it by using hvc.
> > > Ugh, using ISS==0 for this interface is a bit unfortunate, who
> > > arbitrates this namespace?
> 
> Nobody, officially.  ARM is working to promulgate some standards in
> this area, but there is plenty of pre-existing firmware and software
> out there with non-standardised interfaces and it''s hard to
predict
> adoption timescales.  I believe nothing is published relating to this
> just yet.
> 
> > Xen chose to use a non-0 immediate for its hypercalls so we are lucky
> > that we don''t have to play tricks to distinguish PSCI calls
from
> > hypercalls but it seems to me that either there needs to be some
> > semi-formal registry for smv/hvc immediates to avoid clashes or that
> > interfaces need to pick a random non-zero number to try and minimise
the
> > chances of a clash.
> > 
> > Or maybe it gets pushed down a layer and the registry is of r0/x0
> > function ids and the immediate argument is generally ignored?
> > 
> > But in general we need some way for interfaces provided by hypervisors
> > and secure mode monitors to safely co-exist, I think, both for the
case
> > where the hypervisor is implementing a shim for a secure mode
interface
> > (i.e. implementing PSCI with hvc) and for the case where there are
> > multiple interfaces at each layer (e.g. hypervisors sometimes want to
> > implement foreign hypervisor interfaces and I can easily imagine other
> > firmware level interfaces than PSCI coming into existence in the
> > future).
> 
> For PSCI and similar firmware functionality, we envisage multiplexing
> all interfaces based on a function number in r0, with arguments and
> results constrained to r0-r7.  For PSCI, we specify SMC #0/HVC #0 as the
> call mechanism.
> 
> For now, there is no global management of the r0 namespace, which
> is why for PSCI we don''t make static assumptions about the numbers
> on the client side.  Instead, the function number for each PSCI function
> is passed in the DT.  This means that the firmware or hypervisor can
> allocate the numbers as appropriate to avoid clashes.
> 
> In general, this is likely to be the best approach for Linux until a
> standard probing interface exists.
Yes, this is as good a scheme as we can hope for right now I think.
 
> In theory we could define a Xen-specific call mechanism for PSCI, but
> that should be avoided unless it is absolutely unavoidable.  It sounds
> like the different SMC comment field means that we don''t need to
> worry for Xen/PSCI coexistence specifically, assuming that the hypervisor
> can read it reliably.
I think we''ve got the Xen side in hand, I was more bringing it up as a
general issue.
> > In the specific case of PSCI I couldn''t actually find the
calling
> > convention (i.e. the specific registers to use). I suspect
I''m just
> > looking at an old version of the spec...
> 
> This is indeed vague in the existing draft.  An update should appear
> soon -- Charles can comment on the current status.
> 
> Cheers
> ---Dave

On Tue, 2013-04-09 at 15:04 +0100, Ian Campbell wrote:
> On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:
> > Move the implementation of VCPUOP_register_vcpu_info from x86 specific
> > to commmon code.
> > 
> > Move vcpu_info_mfn from an arch specific vcpu sub-field to the common
> > vcpu struct.
> > Move the initialization of vcpu_info_mfn to common code.
> > 
> > Move unmap_vcpu_info and the call to unmap_vcpu_info at domain
> > destruction time to common code.
> > 
> > Implement map_domain_page_global and unmap_domain_page_global on ARM.
> > 
> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > ---
> >  xen/arch/arm/mm.c            |   10 ++++
> >  xen/arch/x86/domain.c        |  113
------------------------------------------
> >  xen/common/domain.c          |  110
++++++++++++++++++++++++++++++++++++++++
> >  xen/include/asm-x86/domain.h |    3 -
> >  xen/include/xen/domain.h     |    3 +
> >  xen/include/xen/sched.h      |    3 +
> >  6 files changed, 126 insertions(+), 116 deletions(-)
> > 
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 972fa42..a875af5 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -125,6 +125,16 @@ void clear_fixmap(unsigned map)
> >      flush_xen_data_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
> >  }
> >  
> > +void *map_domain_page_global(unsigned long mfn)
> > +{
> > +    return map_domain_page(mfn);
> 
> This isn''t a global mapping, is it? IIRC map_domain_page is a vcpu
local
> mapping.
I think I meant "... pcpu local mapping".
> I''ve presumed that most of the rest of this patch is just motion.
> 
> Given the common & x86 impact you should CC Keir + Jan too...
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Fri, Apr 12, 2013 at 12:16:18PM +0100, Ian Campbell
wrote:
> On Thu, 2013-04-11 at 18:10 +0100, Dave Martin wrote:
> > On Thu, Apr 11, 2013 at 02:32:28PM +0100, Ian Campbell wrote:
> > > Adding Charles and the l-a-k list which I hadn''t noticed
wasn''t CC''d
> > > before. Are there other lists which would be useful to include?
> > > 
> > > On Tue, 2013-04-09 at 14:57 +0100, Ian Campbell wrote:
> > > > > +        if ( !hsr.iss )
> > > > > +            return do_trap_psci(regs);
> > 
> > Have you seen this work?
> 
> I presume Stefano has.
> 
> > Reading the specs, I thought that the SMC immediate is not available
in
> > the HSR in ARMv7, or when the caller is AArch32.  ARMv7 specifies
these
> > bits as UNK/SBZP, and the A15 TRM doesn''t seem seem to
specify any
> > extra information.
> 
> I trimmed the context so you probably don''t know but this is
handling
> PSCI via an HVC call not SMC, Linux lets you specify which to use in the
> DTB.
> 
> I didn''t know that about SMC though -- think my eye confused it
with
> SVC when I scanned the docs. Kind of a lame limitation...
Ah, OK (I''m remembering previous threads now).  Yes, I think the
comment
field is guaranteed to appear in HSR.ISS for HVCs.


SMC pre-dates the existence of exception syndrome registers, so software
should in general not have been relying on the immediate for that anyway
(but what software should do and what it actually does do are of course not
always the same).

> > Otherwise, you would need to go examine the trapped instruction in
> > guest memory.  This is even less easy than it sounds.
> 
> Right, luckily we avoid it by using hvc.
> 
> > > > Ugh, using ISS==0 for this interface is a bit unfortunate,
who
> > > > arbitrates this namespace?
> > 
> > Nobody, officially.  ARM is working to promulgate some standards in
> > this area, but there is plenty of pre-existing firmware and software
> > out there with non-standardised interfaces and it''s hard to
predict
> > adoption timescales.  I believe nothing is published relating to this
> > just yet.
> > 
> > > Xen chose to use a non-0 immediate for its hypercalls so we are
lucky
> > > that we don''t have to play tricks to distinguish PSCI
calls from
> > > hypercalls but it seems to me that either there needs to be some
> > > semi-formal registry for smv/hvc immediates to avoid clashes or
that
> > > interfaces need to pick a random non-zero number to try and
minimise the
> > > chances of a clash.
> > > 
> > > Or maybe it gets pushed down a layer and the registry is of r0/x0
> > > function ids and the immediate argument is generally ignored?
> > > 
> > > But in general we need some way for interfaces provided by
hypervisors
> > > and secure mode monitors to safely co-exist, I think, both for
the case
> > > where the hypervisor is implementing a shim for a secure mode
interface
> > > (i.e. implementing PSCI with hvc) and for the case where there
are
> > > multiple interfaces at each layer (e.g. hypervisors sometimes
want to
> > > implement foreign hypervisor interfaces and I can easily imagine
other
> > > firmware level interfaces than PSCI coming into existence in the
> > > future).
> > 
> > For PSCI and similar firmware functionality, we envisage multiplexing
> > all interfaces based on a function number in r0, with arguments and
> > results constrained to r0-r7.  For PSCI, we specify SMC #0/HVC #0 as
the
> > call mechanism.
> > 
> > For now, there is no global management of the r0 namespace, which
> > is why for PSCI we don''t make static assumptions about the
numbers
> > on the client side.  Instead, the function number for each PSCI
function
> > is passed in the DT.  This means that the firmware or hypervisor can
> > allocate the numbers as appropriate to avoid clashes.
> > 
> > In general, this is likely to be the best approach for Linux until a
> > standard probing interface exists.
> 
> Yes, this is as good a scheme as we can hope for right now I think.
>  
> > In theory we could define a Xen-specific call mechanism for PSCI, but
> > that should be avoided unless it is absolutely unavoidable.  It sounds
> > like the different SMC comment field means that we don''t need
to
> > worry for Xen/PSCI coexistence specifically, assuming that the
hypervisor
> > can read it reliably.
> 
> I think we''ve got the Xen side in hand, I was more bringing it up
as a
> general issue.
OK, sounds good.

The PSCI bindings make it possible for the firmware or hypervisor to
manage the function ID namespace, so we should encourage people to
follow a similar approach for anything new, at least until there is
a better standard.
> > > In the specific case of PSCI I couldn''t actually find
the calling
> > > convention (i.e. the specific registers to use). I suspect
I''m just
> > > looking at an old version of the spec...
> > 
> > This is indeed vague in the existing draft.  An update should appear
> > soon -- Charles can comment on the current status.
 
Cheers
---Dave

On Fri, 12 Apr 2013, Ian Campbell wrote:
> On Fri, 2013-04-12 at 00:42 +0100, Stefano Stabellini wrote:
> > On Thu, 11 Apr 2013, Ian Campbell wrote:
> > > On Wed, 2013-04-10 at 11:13 +0100, Ian Campbell wrote:
> > > > On Tue, 2013-04-09 at 15:23 +0100, Marc Zyngier wrote:
> > > > > On 09/04/13 14:57, Ian Campbell wrote:
> > > > > > On Thu, 2013-03-21 at 18:42 +0000, Stefano
Stabellini wrote:
> > > > > >> Implement support for ARM Power State
Coordination Interface, PSCI in
> > > > > >> short. The current implementation is based on
HVC and only supports the
> > > > > >> cpu_on call.
> > > > > > 
> > > > > > Doesn''t the PSCI interface require the
use of SMC not HVC?
> > > > > 
> > > > > You can use both, and KVM uses HVC.
> > > > 
> > > > It makes good sense for Xen to follow suit then I think.
> > > 
> > > Stefano, this means that Xen might need to rewrite the dom0 dtb
to
> > > s/smc/hvc/ I think?
> > 
> > We do need to provide a dtb with the right psci method, see this patch
> > for a DomU example:
> > 
> > http://marc.info/?l=linux-arm-kernel&m=136430894810691&w=2
> 
> Right, I was talking about dom0 though which may already have a PSCI
> entry for the physical hardware (which Xen ultimately needs to use), so
> we need to rewrite it.
Yep, that''s right.

On Tue, 9 Apr 2013, Ian Campbell wrote:
> On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:
> 
> It''s possible that we might want to restrict the available
operations?
> e.g. remove VPCUOP_initialize/up if we are doing this via PSCI instead?
> 
> My concern is that there have been security bugs in VCPUOP_initialize on
> x86 in the past and if we don''t have to expose that possibility on
ARM
> lets not.
I think that restricting the vcpu_op available is a good idea. Smaller
the ABI, smaller the surface of attack.

However the resulting patch won''t be extremely pretty (usually all
hypercalls go straight to common code).

On Tue, 9 Apr 2013, Ian Campbell wrote:
> On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:
> > Trap writes to GICD_SGIR, parse the requests, inject SGIs into the
right
> > guest vcpu.
> > 
> > Add a useful debug printk to vgic_vcpu_inject_irq.
> > 
> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > ---
> >  xen/arch/arm/vgic.c       |   53
+++++++++++++++++++++++++++++++++++++++++---
> >  xen/include/asm-arm/gic.h |    3 ++
> >  2 files changed, 52 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 8495384..26279f9 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -370,6 +370,7 @@ static void vgic_enable_irqs(struct vcpu *v,
uint32_t r, int n)
> >  
> >  static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >  {
> > +    struct domain *d = v->domain;
> >      struct hsr_dabt dabt = info->dabt;
> >      struct cpu_user_regs *regs = guest_cpu_user_regs();
> >      uint32_t *r = select_user_reg(regs, dabt.reg);
> > @@ -498,10 +499,51 @@ static int vgic_distr_mmio_write(struct vcpu *v,
mmio_info_t *info)
> >          goto write_ignore;
> >  
> >      case GICD_SGIR:
> > -        if ( dabt.size != 2 ) goto bad_width;
> > -        printk("vGICD: unhandled write %#"PRIx32" to
ICFGR%d\n",
> > -               *r, gicd_reg - GICD_ICFGR);
> > -        return 0;
> > +        {
> > +            cpumask_t vcpu_mask;
> > +            int virtual_irq;
> > +            int filter;
> > +            int vcpuid;
> > +            struct vcpu *vt;
> > +            int i;
> > +
> > +            if ( dabt.size != 2 ) goto bad_width;
> > +
> > +            filter = (*r & GICD_SGI_TARGET_LIST_MASK);
> > +            virtual_irq = (*r & GICD_SGI_INTID_MASK);
> > +
> > +            cpumask_clear(&vcpu_mask);
> > +            switch ( filter )
> > +            {
> > +                case GICD_SGI_TARGET_LIST:
> > +                    cpumask_bits(&vcpu_mask)[0] = (*r &
GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT;
> 
> Is this the best available way to set a cpumask in Xen?
We could probably use cpumask_or to set the bits in vcpu_mask, but I
don''t think it would be a great improvement.

> > +                    break;
> > +                case GICD_SGI_TARGET_OTHERS:
> > +                    for ( i = 0; i < d->max_vcpus; i++ )
> > +                    {
> > +                        if ( i != current->vcpu_id &&
d->vcpu[i] != NULL )
> > +                            cpumask_set_cpu(i, &vcpu_mask);
> 
> Can this be done with cpumask_any/all and then clearing the current cpu?
cpumask_all doesn''t actually exist and I don''t see a way to
use
cpumask_any to improve this code.
If we were dealing with pcpus, we could use cpu_online_map and
cpumask_clear_cpu, but given that they are vcpus, I don''t think we can
do much better than this.

On Tue, 2013-04-23 at 12:50 +0100, Stefano Stabellini
wrote:
> On Tue, 9 Apr 2013, Ian Campbell wrote:
> > On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:
> > > Trap writes to GICD_SGIR, parse the requests, inject SGIs into
the right
> > > guest vcpu.
> > > 
> > > Add a useful debug printk to vgic_vcpu_inject_irq.
> > > 
> > > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > > ---
> > >  xen/arch/arm/vgic.c       |   53
+++++++++++++++++++++++++++++++++++++++++---
> > >  xen/include/asm-arm/gic.h |    3 ++
> > >  2 files changed, 52 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > index 8495384..26279f9 100644
> > > --- a/xen/arch/arm/vgic.c
> > > +++ b/xen/arch/arm/vgic.c
> > > @@ -370,6 +370,7 @@ static void vgic_enable_irqs(struct vcpu *v,
uint32_t r, int n)
> > >  
> > >  static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t
*info)
> > >  {
> > > +    struct domain *d = v->domain;
> > >      struct hsr_dabt dabt = info->dabt;
> > >      struct cpu_user_regs *regs = guest_cpu_user_regs();
> > >      uint32_t *r = select_user_reg(regs, dabt.reg);
> > > @@ -498,10 +499,51 @@ static int vgic_distr_mmio_write(struct
vcpu *v, mmio_info_t *info)
> > >          goto write_ignore;
> > >  
> > >      case GICD_SGIR:
> > > -        if ( dabt.size != 2 ) goto bad_width;
> > > -        printk("vGICD: unhandled write %#"PRIx32"
to ICFGR%d\n",
> > > -               *r, gicd_reg - GICD_ICFGR);
> > > -        return 0;
> > > +        {
> > > +            cpumask_t vcpu_mask;
> > > +            int virtual_irq;
> > > +            int filter;
> > > +            int vcpuid;
> > > +            struct vcpu *vt;
> > > +            int i;
> > > +
> > > +            if ( dabt.size != 2 ) goto bad_width;
> > > +
> > > +            filter = (*r & GICD_SGI_TARGET_LIST_MASK);
> > > +            virtual_irq = (*r & GICD_SGI_INTID_MASK);
> > > +
> > > +            cpumask_clear(&vcpu_mask);
> > > +            switch ( filter )
> > > +            {
> > > +                case GICD_SGI_TARGET_LIST:
> > > +                    cpumask_bits(&vcpu_mask)[0] = (*r &
GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT;
> > 
> > Is this the best available way to set a cpumask in Xen?
> 
> We could probably use cpumask_or to set the bits in vcpu_mask, but I
> don''t think it would be a great improvement.
It would have the advantage of not breaking the cpumask abstraction
quite so wide open though.
> > > +                    break;
> > > +                case GICD_SGI_TARGET_OTHERS:
> > > +                    for ( i = 0; i < d->max_vcpus; i++ )
> > > +                    {
> > > +                        if ( i != current->vcpu_id &&
d->vcpu[i] != NULL )
> > > +                            cpumask_set_cpu(i, &vcpu_mask);
> > 
> > Can this be done with cpumask_any/all and then clearing the current
cpu?
> 
> cpumask_all doesn''t actually exist and I don''t see a way
to use
> cpumask_any to improve this code.
> If we were dealing with pcpus, we could use cpu_online_map and
> cpumask_clear_cpu, but given that they are vcpus, I don''t think we
can
> do much better than this.
OK.

On Tue, 23 Apr 2013, Ian Campbell wrote:
> > > > +            cpumask_clear(&vcpu_mask);
> > > > +            switch ( filter )
> > > > +            {
> > > > +                case GICD_SGI_TARGET_LIST:
> > > > +                    cpumask_bits(&vcpu_mask)[0] = (*r
& GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT;
> > > 
> > > Is this the best available way to set a cpumask in Xen?
> > 
> > We could probably use cpumask_or to set the bits in vcpu_mask, but I
> > don''t think it would be a great improvement.
> 
> It would have the advantage of not breaking the cpumask abstraction
> quite so wide open though.
Nope, I was wrong, cpumask_or only takes cpumask_t as arguments.  In
fact the issue is that *r is not a cpumask_t, so the only way to make it
into a cpumask_t is to use cpumask_bits or a loop.

On Tue, 2013-04-23 at 13:08 +0100, Stefano Stabellini
wrote:
> On Tue, 23 Apr 2013, Ian Campbell wrote:
> > > > > +            cpumask_clear(&vcpu_mask);
> > > > > +            switch ( filter )
> > > > > +            {
> > > > > +                case GICD_SGI_TARGET_LIST:
> > > > > +                    cpumask_bits(&vcpu_mask)[0] =
(*r & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT;
> > > > 
> > > > Is this the best available way to set a cpumask in Xen?
> > > 
> > > We could probably use cpumask_or to set the bits in vcpu_mask,
but I
> > > don''t think it would be a great improvement.
> > 
> > It would have the advantage of not breaking the cpumask abstraction
> > quite so wide open though.
> 
> Nope, I was wrong, cpumask_or only takes cpumask_t as arguments.  In
> fact the issue is that *r is not a cpumask_t, so the only way to make it
> into a cpumask_t is to use cpumask_bits or a loop.
Would a cpumask_from_bitmap(unsigned long *bm, int nr) helper be a
generally useful thing to have?

Ian.

On Tue, 2013-04-23 at 12:32 +0100, Stefano Stabellini
wrote:
> On Tue, 9 Apr 2013, Ian Campbell wrote:
> > On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:
> > 
> > It''s possible that we might want to restrict the available
operations?
> > e.g. remove VPCUOP_initialize/up if we are doing this via PSCI
instead?
> > 
> > My concern is that there have been security bugs in VCPUOP_initialize
on
> > x86 in the past and if we don''t have to expose that
possibility on ARM
> > lets not.
> 
> I think that restricting the vcpu_op available is a good idea. Smaller
> the ABI, smaller the surface of attack.
> 
> However the resulting patch won''t be extremely pretty (usually all
> hypercalls go straight to common code).
Yes :-/

I expect this is going to be something we want to do in several places,
and also that we are going to want this capability on x86 too once PVH
takes hold and the deprecation plan for PVMMU comes into play in half a
decades time (I expect PVMMU stuff to become a compile time option for
some time before it gets fully deprecated/removed).

So perhaps it is worth trying to find an acceptable longterm solution?
We should probably involve Keir & Jan too.

For start_info and evtchn_upcall_mask I''ve added XEN_HAVE_FOO defines,
perhaps this is a path to follow e.g. with XEN_HAVE_PV_VCPUOP (or some
better name).

Ian.