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

[Julien Grall <julien.grall@linaro.org>]

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@linaro.org>
>>>>>>
>>>>>> 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.
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.

If I remove #32*4, the instruction is different. With objdump on each
generate object:
- vstmia r3!, {d0,d15}
- vstr d0, [rn, imm]

--
Julien