Re: [XenPPC] [PATCH] Enable SMP and IPIs

[Jimi Xenidis <jimix@xxxxxxxxxxxxxx>]

On Oct 24, 2006, at 12:22 AM, Amos Waterland wrote:

> This patch enables SMP and IPIs.  It works reliably on JS20 and JS21
> blades.  It is not quite ready for submission, but I would greatly
> appreciate any comments.
>
> Note that the flurry of IPIs generated by domain creation seems to  
> be a
> waste of time.  Xen on x86 doesn't actually to do anything in response
> to them, so I have made Xen on PPC properly deliver the event check  
> but
> then do nothing as well.  Comments?
>
> The important thing this patch is missing is the ability to invoke
> functions on a remote CPU, and I have left it out because I am not yet
> happy with the convention, which is to grab a lock, put the target
> address in a single global shared variable, invoke a memory  
> barrier, and
> send the IPI.  I am hoping to avoid a lock around that operation by
> making a per-IPI-destination-CPU structure with lock, address, and ack
> fields.  Comments?
It seems Linux-PPC does a similar thing, I'd be happy with borrowing  
that code:
  arch/powerpc/kernel/smp.c <global> 214 int smp_call_function ()

I will always encourage, "coming up with something better" but lets  
nail it first.
[snip]

> diff -r 3dfeb3e4a03f xen/arch/powerpc/mpic_init.c
> --- a/xen/arch/powerpc/mpic_init.c      Fri Oct 13 11:00:32 2006 -0400
> +++ b/xen/arch/powerpc/mpic_init.c      Mon Oct 23 15:32:55 2006 -0400
> @@ -358,6 +358,43 @@ static struct hw_interrupt_type *share_m
>
>  #endif
>
> +static inline struct mpic * mpic_from_ipi(unsigned int ipi)
> +{
> +       return container_of(irq_desc[ipi].handler, struct mpic, hc_ipi);
> +}
> +
> +static unsigned int mpic_startup_ipi(unsigned int irq)
> +{
> +    printk("%s\n", __func__);
> +    struct mpic *pic = mpic_from_ipi(irq);
> +    pic->hc_ipi.enable(irq);
> +    return 0;
> +}
at least in our code now, mpic pointer is static to this whole file  
so mpic_from_ipi() is unnecessary.  We don't have any multi-mpic  
machines yet and looking at upstream Linux, they have solved this  
problem another way.
> +
> +int request_irq(unsigned int irq,
> +               irqreturn_t (*handler)(int, void *, struct cpu_user_regs *),
> +               unsigned long irqflags, const char * devname, void *dev_id)
> +{
> +    int retval;
> +    struct irqaction *action;
> +
> +    printk("%s: %d: %p: %s: %p\n", __func__, irq, handler,  
> devname, dev_id);
> +
> +    action = xmalloc_bytes(sizeof(struct irqaction));
plan xmalloc() takes a type and does the sizeof() for you, so:
  action = xmalloc(struct irqaction);


> +    if (!action)
> +       BUG();
I know this is RFC, and you know I'm a bug fan of BUG(), but  
eventually I'd expect:
  BUG_ON(!action)
  if (!action)
        return -ENOMEM

> +
> +    action->handler = (void (*)(int, void *, struct cpu_user_regs  
> *))handler;
can you cast this with a local variable and comment that Xen's irq  
action is older than Linux's.
> +    action->name = devname;
> +    action->dev_id = dev_id;
> +
> +    retval = setup_irq(irq, action);
> +    if (retval)
> +       BUG();
As the BUG above:
  BUG_ON(retval);
  if (reval)
    xfree(action);

> +
> +    return retval;
> +}
> +
>  struct hw_interrupt_type *xen_mpic_init(struct hw_interrupt_type  
> *xen_irq)
>  {
>      unsigned int isu_size;
> @@ -397,6 +434,11 @@ struct hw_interrupt_type *xen_mpic_init(
>      hit = share_mpic(&mpic->hc_irq, xen_irq);
>
>      printk("%s: success\n", __func__);
> +
> +    mpic->hc_ipi.ack = xen_irq->ack;
> +    mpic->hc_ipi.startup = mpic_startup_ipi;
> +    mpic_request_ipis();
> +
>      return hit;
>  }
[snip]
> diff -r 3dfeb3e4a03f xen/arch/powerpc/smp.c
> --- a/xen/arch/powerpc/smp.c    Fri Oct 13 11:00:32 2006 -0400
> +++ b/xen/arch/powerpc/smp.c    Mon Oct 23 23:15:45 2006 -0400
> @@ -22,6 +22,8 @@
>  #include <xen/smp.h>
>  #include <asm/flushtlb.h>
>  #include <asm/debugger.h>
> +#include <asm/mpic.h>
> +#include <asm/mach-default/irq_vectors.h>
>
>  int smp_num_siblings = 1;
>  int smp_num_cpus = 1;
> @@ -46,11 +48,30 @@ void __flush_tlb_mask(cpumask_t mask, un
>          unimplemented();
>  }
>
> +static void send_IPI_mask(cpumask_t mask, int vector)
> +{
> +    unsigned long cpus;
> +
> +    switch(vector) {
> +    case CALL_FUNCTION_VECTOR:
> +    case EVENT_CHECK_VECTOR:
> +    case INVALIDATE_TLB_VECTOR:
I'm pretty sure we should not be supporting the INVALIDATE_TLB_VECTOR  
vector and will probably just do a broadcast TLB instead, do I'd like  
to see this BUG().
> +       break;
> +    default:
> +       BUG();
> +    }
> +
> +    BUG_ON(sizeof(mask.bits) != sizeof(unsigned long));
> +    cpus = mask.bits[0];
> +
> +    mpic_send_ipi(vector, cpus);
I'd like to keep all knowledge of mpic in external.c and mpic_init.c,  
so let make a new function smp_send_ipi() in external.c and move the  
bounds checking there, sinec mpic_send_ipi() takes an unsigned int  
for cpus, you should make sure that the bounds check on that rather  
than unsigned long.
> +}
> +
>  void smp_send_event_check_mask(cpumask_t mask)
>  {
>      cpu_clear(smp_processor_id(), mask);
>      if (!cpus_empty(mask))
> -        unimplemented();
> +        send_IPI_mask(mask, EVENT_CHECK_VECTOR);
>  }
>
>
> @@ -78,3 +99,39 @@ int on_selected_cpus(
>      unimplemented();
>      return 0;
>  }
> +
> +void smp_call_function_interrupt(struct cpu_user_regs *regs)
> +{
> +    BUG();
arch/powerpc/kernel/smp.c smp_call_function_interrupt 285 void  
smp_call_function_interrupt(void )
> +    return;
> +}
> +
> +void smp_event_check_interrupt(void)
> +{
> +    /* Like x86 we don't actually do anything with this IPI.  */
> +    return;
> +}
> +
> +void smp_invalidate_interrupt(void)
> +{
> +    BUG();
> +    return;
> +}
> +
> +void smp_message_recv(int msg, struct cpu_user_regs *regs)
> +{
> +    switch(msg) {
> +    case CALL_FUNCTION_VECTOR:
> +       smp_call_function_interrupt(regs);
> +       break;
> +    case EVENT_CHECK_VECTOR:
> +        smp_event_check_interrupt();
> +       break;
> +    case INVALIDATE_TLB_VECTOR:
> +        smp_invalidate_interrupt();
> +       break;
> +    default:
> +       BUG();
> +       break;
> +    }
> +}
> diff -r 3dfeb3e4a03f xen/include/asm-powerpc/mach-default/ 
> irq_vectors.h
> --- a/xen/include/asm-powerpc/mach-default/irq_vectors.h	Fri Oct 13  
> 11:00:32 2006 -0400
> +++ b/xen/include/asm-powerpc/mach-default/irq_vectors.h	Mon Oct 23  
> 21:00:34 2006 -0400
> @@ -37,8 +37,6 @@
>  #define FAST_TRAP -1 /* 0x80 */
>  #define FIRST_SYSTEM_VECTOR    -1
>
> -#if 0
> -
Please drop the entire contents of #if 0 here and only add the 3 you  
need below.
>  /*
>   * Vectors 0-16 in some cases are used for ISA interrupts.
>   */
> @@ -54,9 +52,12 @@
>   */
>  #define SPURIOUS_APIC_VECTOR   0xff
>  #define ERROR_APIC_VECTOR      0xfe
> -#define INVALIDATE_TLB_VECTOR  0xfd
> -#define EVENT_CHECK_VECTOR     0xfc
> -#define CALL_FUNCTION_VECTOR   0xfb
> +
> +#define CALL_FUNCTION_VECTOR   0x0
> +#define EVENT_CHECK_VECTOR     0x1
> +#define INVALIDATE_TLB_VECTOR  0x3
Drop the INVALIDATE_TLB_VECTOR

> +
> +#if 0
>
>  #define THERMAL_APIC_VECTOR    0xf0
>  /*

_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel