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

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

On 05/30/2013 04:11 PM, Ian Campbell wrote:

> On Thu, 2013-05-30 at 15:38 +0100, Julien Grall wrote:
>> diff --git a/xen/arch/arm/arm32/vfp.c b/xen/arch/arm/arm32/vfp.c
>> new file mode 100644
>> index 0000000..16f635a
>> --- /dev/null
>> +++ b/xen/arch/arm/arm32/vfp.c
>> @@ -0,0 +1,71 @@
>> +#include <xen/sched.h>
>> +#include <asm/processor.h>
>> +#include <asm/vfp.h>
>> +
>> +void vfp_save_state(struct vcpu *v)
>> +{
> [...]
>> +}
>> +
>> +void vfp_restore_state(struct vcpu *v)
>> +{
> [...]
> 
> Without having read the documentation this seem plausible enough.
> 
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/include/asm-arm/arm64/vfp.h b/xen/include/asm-arm/arm64/vfp.h
>> new file mode 100644
>> index 0000000..34cd202
>> --- /dev/null
>> +++ b/xen/include/asm-arm/arm64/vfp.h
>> @@ -0,0 +1,16 @@
>> +#ifndef _ARM_ARM64_VFP_H
>> +#define _ARM_ARM32_VFP_H
> 
> Mismatch.
> 
>> +
>> +struct vfp_state
>> +{
>> +};
>> +
>> +#endif /* _ARM_ARM32_VFP_H */
> 
> And again.
> 
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
>> index f08d59a..d99ccfd 100644
>> --- a/xen/include/asm-arm/cpregs.h
>> +++ b/xen/include/asm-arm/cpregs.h
>> @@ -60,6 +60,12 @@
>>   * arguments, which are cp,opc1,crn,crm,opc2.
>>   */
>>  
> 
> + /* Coprocessor 10 */
> 
> Please.
> 
>> +#define FPSCR           p10,7,c1,c0,0   /* Floating-Point Status and Control Register */
>> +#define MVFR0           p10,7,c7,c0,0   /* Media and VFP Feature Register 0 */
>> +#define FPEXC           p10,7,c8,c0,0   /* Floating-Point Exception Control Register */
>> +#define FPINST          p10,7,c9,c0,0   /* Floating-Point Instruction Register */
>> +#define FPINST2         p10,7,c10,c0,0  /* Floating-point Instruction Register 2 */
>> +
>>  /* Coprocessor 14 */
>>  
>>  /* CP14 CR0: */
>> @@ -106,6 +112,7 @@
>>  #define NSACR           p15,0,c1,c1,2   /* Non-Secure Access Control Register */
>>  #define HSCTLR          p15,4,c1,c0,0   /* Hyp. System Control Register */
>>  #define HCR             p15,4,c1,c1,0   /* Hyp. Configuration Register */
>> +#define HCPTR           p15,4,c1,c1,2   /* Hyp. Coprocessor Trap Register */
>>  
>>  /* CP15 CR2: Translation Table Base and Control Registers */
>>  #define TTBCR           p15,0,c2,c0,2   /* Translatation Table Base Control Register */
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index cb251cc..6b52b5e 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -1,11 +1,13 @@
>>  #ifndef __ASM_DOMAIN_H__
>>  #define __ASM_DOMAIN_H__
>>  
>> +#include <asm/atomic.h>
> 
> What is this used for?

Lost line. I will remove it.

> 
>>  #include <xen/config.h>
>>  #include <xen/cache.h>
>>  #include <xen/sched.h>
>>  #include <asm/page.h>
>>  #include <asm/p2m.h>
>> +#include <asm/vfp.h>
>>  #include <public/hvm/params.h>
>>  
>>  /* Represents state corresponding to a block of 32 interrupts */
>> diff --git a/xen/include/asm-arm/vfp.h b/xen/include/asm-arm/vfp.h
>> index b800816..6ba3cd1 100644
>> --- a/xen/include/asm-arm/vfp.h
>> +++ b/xen/include/asm-arm/vfp.h
>> @@ -2,7 +2,14 @@
>>  #define __ARM_VFP_H_
>>  
>>  #include <xen/types.h>
>> +#include <xen/sched.h>
> 
>> +#if defined(CONFIG_ARM_32)
>> +# include <asm/arm32/vfp.h>
>> +#elif defined(CONFIG_ARM_64)
>> +# include <asm/arm64/vfp.h>
> 
> Did you mean to include a #else here?


Yes. It was lost during the creation of the patch. I will pay more
attention in the next patch series.

> 
>> +# error "Unknown ARM variant"
>> +#endif
>>  
>>  #ifdef CONFIG_ARM_32
> 
> I suppose the content of this #if now belongs in the subarch header.

This part is removed in the next patch. I was thinking to invert the 2
patch as the second patch doesn't need the first one (except for the diff).

> 
>>  
>> @@ -32,6 +39,9 @@ static inline void enable_vfp(void)
>>  }
>>  #endif
>>  
>> +void vfp_save_state(struct vcpu *v);
>> +void vfp_restore_state(struct vcpu *v);
>> +
>>  #endif
>>  /*
>>   * Local variables:
> 
>