Re: [Xen-devel] [PATCH v3 2/2] xen/arm32: implement VFP context switch

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-06-12 at 16:14 +0100, Ian Campbell wrote:
> On Wed, 2013-06-05 at 14:06 +0100, Julien Grall wrote:
> > On 06/03/2013 04:14 PM, Ian Campbell wrote:
> > 
> > > On Mon, 2013-06-03 at 15:57 +0100, Julien Grall wrote:
> > >> On 06/03/2013 03:38 PM, Ian Campbell wrote:
> > >>
> > >>> On Mon, 2013-06-03 at 15:32 +0100, Julien Grall wrote:
> > >>>> On 06/03/2013 03:15 PM, Ian Campbell wrote:
> > >>>>
> > >>>>> On Mon, 2013-06-03 at 15:00 +0100, Julien Grall wrote:
> > >>>>>> Add support for VFP context switch on arm32 and a dummy support for 
> > >>>>>> arm64
> > >>>>>>
> > >>>>>> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> > >>>>>>
> > >>>>>> Changes in v3:
> > >>>>>>     - Add vfp_init to check if the processor supports VFP 3
> > >>>>>>     - Add clobber memory
> > >>>>>>     - Remove tmps
> > >>>>>>     - s/COFNIG_ARM64/CONFIG_ARM64/ in include/asm/arm.h
> > >>>>>>
> > >>>>>> Changes in v2:
> > >>>>>>     - Fix all the small errors (type, lost headers...)
> > >>>>>>     - Add some comments
> > >>>>>> ---
> > >>>>>>  xen/arch/arm/arm32/Makefile     |    1 +
> > >>>>>>  xen/arch/arm/arm32/vfp.c        |   99 
> > >>>>>> +++++++++++++++++++++++++++++++++++++++
> > >>>>>>  xen/arch/arm/arm64/Makefile     |    1 +
> > >>>>>>  xen/arch/arm/arm64/vfp.c        |   13 +++++
> > >>>>>>  xen/arch/arm/domain.c           |    7 ++-
> > >>>>>>  xen/include/asm-arm/arm32/vfp.h |   41 ++++++++++++++++
> > >>>>>>  xen/include/asm-arm/arm64/vfp.h |   16 +++++++
> > >>>>>>  xen/include/asm-arm/cpregs.h    |    9 ++++
> > >>>>>>  xen/include/asm-arm/domain.h    |    4 ++
> > >>>>>>  xen/include/asm-arm/vfp.h       |   25 ++++++++++
> > >>>>>>  10 files changed, 214 insertions(+), 2 deletions(-)
> > >>>>>>  create mode 100644 xen/arch/arm/arm32/vfp.c
> > >>>>>>  create mode 100644 xen/arch/arm/arm64/vfp.c
> > >>>>>>  create mode 100644 xen/include/asm-arm/arm32/vfp.h
> > >>>>>>  create mode 100644 xen/include/asm-arm/arm64/vfp.h
> > >>>>>>  create mode 100644 xen/include/asm-arm/vfp.h
> > >>>>>>
> > >>>>>> diff --git a/xen/arch/arm/arm32/Makefile 
> > >>>>>> b/xen/arch/arm/arm32/Makefile
> > >>>>>> index aaf277a..b903803 100644
> > >>>>>> --- a/xen/arch/arm/arm32/Makefile
> > >>>>>> +++ b/xen/arch/arm/arm32/Makefile
> > >>>>>> @@ -6,5 +6,6 @@ obj-y += proc-ca15.o
> > >>>>>>  
> > >>>>>>  obj-y += traps.o
> > >>>>>>  obj-y += domain.o
> > >>>>>> +obj-y += vfp.o
> > >>>>>>  
> > >>>>>>  obj-$(EARLY_PRINTK) += debug.o
> > >>>>>> diff --git a/xen/arch/arm/arm32/vfp.c b/xen/arch/arm/arm32/vfp.c
> > >>>>>> new file mode 100644
> > >>>>>> index 0000000..2ece43d
> > >>>>>> --- /dev/null
> > >>>>>> +++ b/xen/arch/arm/arm32/vfp.c
> > >>>>>> @@ -0,0 +1,99 @@
> > >>>>>> +#include <xen/sched.h>
> > >>>>>> +#include <xen/init.h>
> > >>>>>> +#include <asm/processor.h>
> > >>>>>> +#include <asm/vfp.h>
> > >>>>>> +
> > >>>>>> +void vfp_save_state(struct vcpu *v)
> > >>>>>> +{
> > >>>>>> +    v->arch.vfp.fpexc = READ_CP32(FPEXC);
> > >>>>>> +
> > >>>>>> +    WRITE_CP32(v->arch.vfp.fpexc | FPEXC_EN, FPEXC);
> > >>>>>> +
> > >>>>>> +    v->arch.vfp.fpscr = READ_CP32(FPSCR);
> > >>>>>> +
> > >>>>>> +    if ( v->arch.vfp.fpexc & FPEXC_EX ) /* Check for 
> > >>>>>> sub-architecture */
> > >>>>>> +    {
> > >>>>>> +        v->arch.vfp.fpinst = READ_CP32(FPINST);
> > >>>>>> +
> > >>>>>> +        if ( v->arch.vfp.fpexc & FPEXC_FP2V )
> > >>>>>> +            v->arch.vfp.fpinst2 = READ_CP32(FPINST2);
> > >>>>>> +        /* Disable FPEXC_EX */
> > >>>>>> +        WRITE_CP32((v->arch.vfp.fpexc | FPEXC_EN) & ~FPEXC_EX, 
> > >>>>>> FPEXC);
> > >>>>>> +    }
> > >>>>>> +
> > >>>>>> +    /* Save {d0-d15} */
> > >>>>>> +    asm volatile("stc p11, cr0, [%0], #32*4"
> > >>>>>> +                 : : "r" (v->arch.vfp.fpregs1)
> > >>>>>> +                 : "memory");
> > >>>>>
> > >>>>> http://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html suggests 
> > >>>>> that
> > >>>>> "=m" (v->arch.vfp.fpregs1) or "=Q" (...) as output constraints might 
> > >>>>> do
> > >>>>> the job without clobbering the whole of memory.
> > >>>>
> > >>>> I'm not sure to fully understand the concept behind "=m". Does it mean
> > >>>> that gcc will clobber all the memory range addressed by fpregs?
> > >>>
> > >>> I'm not totally confident in this stuff myself....
> > >>>
> > >>> Apparently the "=" modified means[0] "this operand is write-only for
> > >>> this instruction: the previous value is discarded and replaced by output
> > >>> data." In the case of a memory constraint you'd want to hope that
> > >>> "discarded and replaced" would be equivalent to clobbering the address,
> > >>> at least in cases where the compiler knows the size of the thing, as it
> > >>> does here.
> > >>>
> > >>> Ian.
> > >>>
> > >>> [0] http://gcc.gnu.org/onlinedocs/gcc-4.8.1/gcc/Modifiers.html#Modifiers
> > >>>
> > >>
> > >>
> > >> Shall I use this contrainst for the stc instructions and send a new
> > >> patch series?
> > > 
> > > If you buy my reasoning and if it works then I guess so.
> > 
> > 
> > Hi,
> > 
> > So I gave a try and it doesn't work.
> 
> That's a shame, oh well we'll have to use the big clobber then -- not
> the end of the world.
> 
> > The modifiers =m replaces %0 by [rn,imm] which is wrong. So it's not
> > possible to specify the amount of data which the instruction need to
> > read (the #32*4) because pre-index and post-index aren't allowed in a
> > same instruction.

I had a chat with Tim and he pointed out that this is exactly what the
=Q output constraint is for. According to
http://gcc.gnu.org/onlinedocs/gcc-4.8.1/gcc/Machine-Constraints.html#Machine-Constraints
it is:

        A memory reference where the exact address is in a single
        register

In other words it outlaws the [rn,imm] construct.

Does that work?

(that doc goes on to say:
         (`âmâ' is preferable for asm statements) 
lets ignore that ;-))

Ian.


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