[Xen-devel,RFC,for-4.13,07/10] xen/arm: Allow insn.h to be called from assembly

Message ID 20190926183808.11630-8-julien.grall@arm.com
State New
Headers show
Series
  • xen/arm: XSA-201 and XSA-263 fixes
Related show

Commit Message

Julien Grall Sept. 26, 2019, 6:38 p.m.
A follow-up patch will require to include insn.h from assembly code. So
wee need to protect any C-specific definition to avoid compilation
error when used in assembly code.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/insn.h | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Volodymyr Babchuk Sept. 27, 2019, 11:52 a.m. | #1
Julien Grall writes:

> A follow-up patch will require to include insn.h from assembly code. So
> wee need to protect any C-specific definition to avoid compilation
> error when used in assembly code.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
>  xen/include/asm-arm/insn.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
> index 19277212e1..00391f83f9 100644
> --- a/xen/include/asm-arm/insn.h
> +++ b/xen/include/asm-arm/insn.h
> @@ -1,8 +1,14 @@
>  #ifndef __ARCH_ARM_INSN
>  #define __ARCH_ARM_INSN
>  
> +#ifndef __ASSEMBLY__
> +
>  #include <xen/types.h>
>  
> +/*
> + * At the moment, arch-specific headers contain only definition for C
> + * code.
> + */
>  #if defined(CONFIG_ARM_64)
>  # include <asm/arm64/insn.h>
>  #elif defined(CONFIG_ARM_32)
> @@ -11,6 +17,8 @@
>  # error "unknown ARM variant"
>  #endif
>  
> +#endif /* __ASSEMBLY__ */
> +
>  /* On ARM32,64 instructions are always 4 bytes long. */
>  #define ARCH_PATCH_INSN_SIZE 4
Stefano Stabellini Oct. 1, 2019, 9 p.m. | #2
On Thu, 26 Sep 2019, Julien Grall wrote:
> A follow-up patch will require to include insn.h from assembly code. So
> wee need to protect any C-specific definition to avoid compilation
  ^ we                               ^ definitions

> error when used in assembly code.
  ^ errors


> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/include/asm-arm/insn.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
> index 19277212e1..00391f83f9 100644
> --- a/xen/include/asm-arm/insn.h
> +++ b/xen/include/asm-arm/insn.h
> @@ -1,8 +1,14 @@
>  #ifndef __ARCH_ARM_INSN
>  #define __ARCH_ARM_INSN
>  
> +#ifndef __ASSEMBLY__
> +
>  #include <xen/types.h>
>  
> +/*
> + * At the moment, arch-specific headers contain only definition for C
> + * code.
> + */

Please remove "At the moment, " because in-code comment should always
reflect the latest state of the codebase.


>  #if defined(CONFIG_ARM_64)
>  # include <asm/arm64/insn.h>
>  #elif defined(CONFIG_ARM_32)
> @@ -11,6 +17,8 @@
>  # error "unknown ARM variant"
>  #endif
>  
> +#endif /* __ASSEMBLY__ */
> +
>  /* On ARM32,64 instructions are always 4 bytes long. */
>  #define ARCH_PATCH_INSN_SIZE 4
>  
> -- 
> 2.11.0
>
Julien Grall Oct. 21, 2019, 4:43 p.m. | #3
Hi Stefano,

On 10/1/19 10:00 PM, Stefano Stabellini wrote:
> On Thu, 26 Sep 2019, Julien Grall wrote:
>> A follow-up patch will require to include insn.h from assembly code. So
>> wee need to protect any C-specific definition to avoid compilation
>    ^ we                               ^ definitions
> 
>> error when used in assembly code.
>    ^ errors
> 
> 
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/include/asm-arm/insn.h | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
>> index 19277212e1..00391f83f9 100644
>> --- a/xen/include/asm-arm/insn.h
>> +++ b/xen/include/asm-arm/insn.h
>> @@ -1,8 +1,14 @@
>>   #ifndef __ARCH_ARM_INSN
>>   #define __ARCH_ARM_INSN
>>   
>> +#ifndef __ASSEMBLY__
>> +
>>   #include <xen/types.h>
>>   
>> +/*
>> + * At the moment, arch-specific headers contain only definition for C
>> + * code.
>> + */
> 
> Please remove "At the moment, " because in-code comment should always
> reflect the latest state of the codebase.

Well, we do have a lot of "Currently"/"At the moment"/"For now" in the 
code. Some of my patches went in recently with such wording, so why this 
particular one is a problem?

Cheers,
Stefano Stabellini Oct. 21, 2019, 5:23 p.m. | #4
On Mon, 21 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 10/1/19 10:00 PM, Stefano Stabellini wrote:
> > On Thu, 26 Sep 2019, Julien Grall wrote:
> > > A follow-up patch will require to include insn.h from assembly code. So
> > > wee need to protect any C-specific definition to avoid compilation
> >    ^ we                               ^ definitions
> > 
> > > error when used in assembly code.
> >    ^ errors
> > 
> > 
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > ---
> > >   xen/include/asm-arm/insn.h | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
> > > index 19277212e1..00391f83f9 100644
> > > --- a/xen/include/asm-arm/insn.h
> > > +++ b/xen/include/asm-arm/insn.h
> > > @@ -1,8 +1,14 @@
> > >   #ifndef __ARCH_ARM_INSN
> > >   #define __ARCH_ARM_INSN
> > >   +#ifndef __ASSEMBLY__
> > > +
> > >   #include <xen/types.h>
> > >   +/*
> > > + * At the moment, arch-specific headers contain only definition for C
> > > + * code.
> > > + */
> > 
> > Please remove "At the moment, " because in-code comment should always
> > reflect the latest state of the codebase.
> 
> Well, we do have a lot of "Currently"/"At the moment"/"For now" in the code.
> Some of my patches went in recently with such wording, so why this particular
> one is a problem?

Yeah, I realize that a lot of our conventions are still tribal knowledge
(i.e. not written down anywhere.)

Let me explain my take on this. When one writes "At the
moment"/"Currently"/"For now" he/she implies that the code is just
temporary, or at least sub-optimal and something should be down about
it to improve. Maybe not immediately, but it would be nice to have.

I think it is OK to write something like "At the moment," to express
that meaning. That's OK.

In this specific case, the reason why I wrote that reply is that when I
read the in-code comment, it jarred for it ("jarred" as in something was
off, out of place.) And I think the reason is that the code looked OK,
and I didn't get what you think we should "improve" in the code after
this patch is applied.

Patch

diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
index 19277212e1..00391f83f9 100644
--- a/xen/include/asm-arm/insn.h
+++ b/xen/include/asm-arm/insn.h
@@ -1,8 +1,14 @@ 
 #ifndef __ARCH_ARM_INSN
 #define __ARCH_ARM_INSN
 
+#ifndef __ASSEMBLY__
+
 #include <xen/types.h>
 
+/*
+ * At the moment, arch-specific headers contain only definition for C
+ * code.
+ */
 #if defined(CONFIG_ARM_64)
 # include <asm/arm64/insn.h>
 #elif defined(CONFIG_ARM_32)
@@ -11,6 +17,8 @@ 
 # error "unknown ARM variant"
 #endif
 
+#endif /* __ASSEMBLY__ */
+
 /* On ARM32,64 instructions are always 4 bytes long. */
 #define ARCH_PATCH_INSN_SIZE 4