From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH 1/2] xen/arm32: implement VFP context switch Date: Thu, 30 May 2013 16:25:39 +0100 Message-ID: <51A76F73.1090308@linaro.org> References: <1369924733-18701-1-git-send-email-julien.grall@linaro.org> <1369924733-18701-2-git-send-email-julien.grall@linaro.org> <1369926675.18727.17.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1369926675.18727.17.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Stefano.Stabellini@eu.citrix.com, tim@xen.org, patches@linaro.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.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 >> +#include >> +#include >> + >> +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 > > What is this used for? Lost line. I will remove it. > >> #include >> #include >> #include >> #include >> #include >> +#include >> #include >> >> /* 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 >> +#include > >> +#if defined(CONFIG_ARM_32) >> +# include >> +#elif defined(CONFIG_ARM_64) >> +# include > > 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: > >