Re: [Xen-devel] [PATCH 28/45] xen: arm: arm64 trap handling.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-02-14 at 14:24 +0000, Tim Deegan wrote:
> At 15:56 +0000 on 23 Jan (1358956594), Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
> 
> > diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
> > index 815f305..be41f43 100644
> > --- a/xen/arch/arm/arm64/Makefile
> > +++ b/xen/arch/arm/arm64/Makefile
> > @@ -1,5 +1,7 @@
> >  subdir-y += lib
> >  
> > +obj-y += entry.o
> >  obj-y += mode_switch.o
> >  
> > +obj-y += traps.o
> >  obj-y += domain.o
> 
> Maybe in alphabetical order?

I suppose so ;-)
> 
> > +/* base-2 logarithm */
> > +#define __L2(_x)  (((_x) & 0x00000002) ?   1 : 0)
> > +#define __L4(_x)  (((_x) & 0x0000000c) ? ( 2 + __L2( (_x)>> 2)) : __L2( 
> > _x))
> > +#define __L8(_x)  (((_x) & 0x000000f0) ? ( 4 + __L4( (_x)>> 4)) : __L4( 
> > _x))
> > +#define __L16(_x) (((_x) & 0x0000ff00) ? ( 8 + __L8( (_x)>> 8)) : __L8( 
> > _x))
> > +#define LOG_2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : 
> > __L16(_x))
> 
> This is now replicated in three places.  Maybe it should live in, say,
> xen/bitops.h?

ACK.

> 
> > +hyp_sync:
> > +        entry   hyp=1
> > +        msr     daifclr, #2
> > +        adr     lr, return_to_hypervisor
> > +        mov     x0, sp
> > +        b       do_trap_hypervisor
> 
> This pattern (call fnX with arg0 == sp and lr == fnY) is repeated
> quite a few times.  Could we have another tidying macro for it?

I'm half considering doing away with the preload lr+b and just using bl
instead and putting the tail stuff in a macro like the entry stuff.

But if we do stick with this way then sure.

> 
> > +ENTRY(return_to_new_vcpu)
> > +ENTRY(return_to_guest)
> > +ENTRY(return_to_hypervisor)
> > +        ldp     x21, x22, [sp, #UREGS_PC]       // load ELR, SPSR
> > +
> > +        pop     x0, x1
> > +        pop     x2, x3
> > +        pop     x4, x5
> > +        pop     x6, x7
> > +        pop     x8, x9
> > +
> > +        /* XXX handle return to guest tasks, soft irqs etc */
> > +        
> > +        msr     elr_el2, x21                    // set up the return data
> > +        msr     spsr_el2, x22
> 
> Done here becasue it's roughly half-way between the load and the
> overwrite below?  Should we be using x28/x29 for this to give ourselves
> even more pipeline space?

I can't for the life of me recall why I did this here instead of
somewhere else... Lets pretend I was thinking about pipelines, sure -;)

> 
> > +        pop     x10, x11
> > +        pop     x12, x13
> > +        pop     x14, x15
> > +        pop     x16, x17
> > +        pop     x18, x19
> > +        pop     x20, x21
> > +        pop     x22, x23
> > +        pop     x24, x25
> > +        pop     x26, x27
> > +        pop     x28, x29
> > +
> > +        ldr     lr, [sp], #(UREGS_SPSR_el1 - UREGS_SP)
> > +        eret
> > +
> 
> > --- a/xen/arch/arm/io.h
> > +++ b/xen/arch/arm/io.h
> > @@ -26,7 +26,7 @@
> >  typedef struct
> >  {
> >      struct hsr_dabt dabt;
> > -    uint32_t gva;
> > +    vaddr_t gva;
> >      paddr_t gpa;
> >  } mmio_info_t;
> >  
> 
> This seems like it belongs in the earlier vaddr patch.

ACK.

> 
> > --- a/xen/include/asm-arm/processor.h
> > +++ b/xen/include/asm-arm/processor.h
> > @@ -238,7 +238,7 @@ union hsr {
> >  #endif
> >  
> >  #ifndef __ASSEMBLY__
> > -extern uint32_t hyp_traps_vector[8];
> > +extern uint32_t hyp_traps_vector;
> 
> hyp_traps_vector[], and avoid adding '&' to the users?

Could do, there's actually 8 words at each entry point, so the type is a
bit of a complete fiction anyway

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel