| Message ID | 20190926183808.11630-8-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: XSA-201 and XSA-263 fixes | expand |
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
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 >
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,
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.
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
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(+)