The functions sme_enable() and sme_encrypt_kernel() are only called by
the head code which runs in identity virtual address. Therefore, it's
better to mark them as __head as well.
Signed-off-by: Hou Wenlong <[email protected]>
---
arch/x86/include/asm/mem_encrypt.h | 8 ++++----
arch/x86/mm/mem_encrypt_identity.c | 27 ++++++++++++++-------------
2 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
index 359ada486fa9..48469e22a75e 100644
--- a/arch/x86/include/asm/mem_encrypt.h
+++ b/arch/x86/include/asm/mem_encrypt.h
@@ -46,8 +46,8 @@ void __init sme_unmap_bootdata(char *real_mode_data);
void __init sme_early_init(void);
-void __init sme_encrypt_kernel(struct boot_params *bp);
-void __init sme_enable(struct boot_params *bp);
+void sme_encrypt_kernel(struct boot_params *bp);
+void sme_enable(struct boot_params *bp);
int __init early_set_memory_decrypted(unsigned long vaddr, unsigned long size);
int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size);
@@ -75,8 +75,8 @@ static inline void __init sme_unmap_bootdata(char *real_mode_data) { }
static inline void __init sme_early_init(void) { }
-static inline void __init sme_encrypt_kernel(struct boot_params *bp) { }
-static inline void __init sme_enable(struct boot_params *bp) { }
+static inline void sme_encrypt_kernel(struct boot_params *bp) { }
+static inline void sme_enable(struct boot_params *bp) { }
static inline void sev_es_init_vc_handling(void) { }
diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index d73aeb16417f..72aeb0f3dec6 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -46,6 +46,7 @@
#include <asm/cmdline.h>
#include <asm/coco.h>
#include <asm/sev.h>
+#include <asm/init.h>
#include "mm_internal.h"
@@ -99,7 +100,7 @@ static char sme_cmdline_arg[] __initdata = "mem_encrypt";
static char sme_cmdline_on[] __initdata = "on";
static char sme_cmdline_off[] __initdata = "off";
-static void __init sme_clear_pgd(struct sme_populate_pgd_data *ppd)
+static void __head sme_clear_pgd(struct sme_populate_pgd_data *ppd)
{
unsigned long pgd_start, pgd_end, pgd_size;
pgd_t *pgd_p;
@@ -114,7 +115,7 @@ static void __init sme_clear_pgd(struct sme_populate_pgd_data *ppd)
memset(pgd_p, 0, pgd_size);
}
-static pud_t __init *sme_prepare_pgd(struct sme_populate_pgd_data *ppd)
+static pud_t __head *sme_prepare_pgd(struct sme_populate_pgd_data *ppd)
{
pgd_t *pgd;
p4d_t *p4d;
@@ -151,7 +152,7 @@ static pud_t __init *sme_prepare_pgd(struct sme_populate_pgd_data *ppd)
return pud;
}
-static void __init sme_populate_pgd_large(struct sme_populate_pgd_data *ppd)
+static void __head sme_populate_pgd_large(struct sme_populate_pgd_data *ppd)
{
pud_t *pud;
pmd_t *pmd;
@@ -167,7 +168,7 @@ static void __init sme_populate_pgd_large(struct sme_populate_pgd_data *ppd)
set_pmd(pmd, __pmd(ppd->paddr | ppd->pmd_flags));
}
-static void __init sme_populate_pgd(struct sme_populate_pgd_data *ppd)
+static void __head sme_populate_pgd(struct sme_populate_pgd_data *ppd)
{
pud_t *pud;
pmd_t *pmd;
@@ -193,7 +194,7 @@ static void __init sme_populate_pgd(struct sme_populate_pgd_data *ppd)
set_pte(pte, __pte(ppd->paddr | ppd->pte_flags));
}
-static void __init __sme_map_range_pmd(struct sme_populate_pgd_data *ppd)
+static void __head __sme_map_range_pmd(struct sme_populate_pgd_data *ppd)
{
while (ppd->vaddr < ppd->vaddr_end) {
sme_populate_pgd_large(ppd);
@@ -203,7 +204,7 @@ static void __init __sme_map_range_pmd(struct sme_populate_pgd_data *ppd)
}
}
-static void __init __sme_map_range_pte(struct sme_populate_pgd_data *ppd)
+static void __head __sme_map_range_pte(struct sme_populate_pgd_data *ppd)
{
while (ppd->vaddr < ppd->vaddr_end) {
sme_populate_pgd(ppd);
@@ -213,7 +214,7 @@ static void __init __sme_map_range_pte(struct sme_populate_pgd_data *ppd)
}
}
-static void __init __sme_map_range(struct sme_populate_pgd_data *ppd,
+static void __head __sme_map_range(struct sme_populate_pgd_data *ppd,
pmdval_t pmd_flags, pteval_t pte_flags)
{
unsigned long vaddr_end;
@@ -237,22 +238,22 @@ static void __init __sme_map_range(struct sme_populate_pgd_data *ppd,
__sme_map_range_pte(ppd);
}
-static void __init sme_map_range_encrypted(struct sme_populate_pgd_data *ppd)
+static void __head sme_map_range_encrypted(struct sme_populate_pgd_data *ppd)
{
__sme_map_range(ppd, PMD_FLAGS_ENC, PTE_FLAGS_ENC);
}
-static void __init sme_map_range_decrypted(struct sme_populate_pgd_data *ppd)
+static void __head sme_map_range_decrypted(struct sme_populate_pgd_data *ppd)
{
__sme_map_range(ppd, PMD_FLAGS_DEC, PTE_FLAGS_DEC);
}
-static void __init sme_map_range_decrypted_wp(struct sme_populate_pgd_data *ppd)
+static void __head sme_map_range_decrypted_wp(struct sme_populate_pgd_data *ppd)
{
__sme_map_range(ppd, PMD_FLAGS_DEC_WP, PTE_FLAGS_DEC_WP);
}
-static unsigned long __init sme_pgtable_calc(unsigned long len)
+static unsigned long __head sme_pgtable_calc(unsigned long len)
{
unsigned long entries = 0, tables = 0;
@@ -289,7 +290,7 @@ static unsigned long __init sme_pgtable_calc(unsigned long len)
return entries + tables;
}
-void __init sme_encrypt_kernel(struct boot_params *bp)
+void __head sme_encrypt_kernel(struct boot_params *bp)
{
unsigned long workarea_start, workarea_end, workarea_len;
unsigned long execute_start, execute_end, execute_len;
@@ -502,7 +503,7 @@ void __init sme_encrypt_kernel(struct boot_params *bp)
native_write_cr3(__native_read_cr3());
}
-void __init sme_enable(struct boot_params *bp)
+void __head sme_enable(struct boot_params *bp)
{
const char *cmdline_ptr, *cmdline_arg, *cmdline_on, *cmdline_off;
unsigned int eax, ebx, ecx, edx;
--
2.31.1
* Hou Wenlong <[email protected]> wrote:
> The functions sme_enable() and sme_encrypt_kernel() are only called by
> the head code which runs in identity virtual address. Therefore, it's
> better to mark them as __head as well.
>
> Signed-off-by: Hou Wenlong <[email protected]>
> ---
> arch/x86/include/asm/mem_encrypt.h | 8 ++++----
> arch/x86/mm/mem_encrypt_identity.c | 27 ++++++++++++++-------------
> 2 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
> index 359ada486fa9..48469e22a75e 100644
> --- a/arch/x86/include/asm/mem_encrypt.h
> +++ b/arch/x86/include/asm/mem_encrypt.h
> @@ -46,8 +46,8 @@ void __init sme_unmap_bootdata(char *real_mode_data);
>
> void __init sme_early_init(void);
>
> -void __init sme_encrypt_kernel(struct boot_params *bp);
> -void __init sme_enable(struct boot_params *bp);
> +void sme_encrypt_kernel(struct boot_params *bp);
> +void sme_enable(struct boot_params *bp);
>
> int __init early_set_memory_decrypted(unsigned long vaddr, unsigned long size);
> int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size);
> @@ -75,8 +75,8 @@ static inline void __init sme_unmap_bootdata(char *real_mode_data) { }
>
> static inline void __init sme_early_init(void) { }
>
> -static inline void __init sme_encrypt_kernel(struct boot_params *bp) { }
> -static inline void __init sme_enable(struct boot_params *bp) { }
> +static inline void sme_encrypt_kernel(struct boot_params *bp) { }
> +static inline void sme_enable(struct boot_params *bp) { }
So I think we should preserve the previous convention of marking functions
__init in the header-declaration and at the definition site as well, and do
the same with __head as well?
Thanks,
Ingo
On Tue, Oct 17 2023 at 14:52, Ingo Molnar wrote:
> * Hou Wenlong <[email protected]> wrote:
>> -static inline void __init sme_encrypt_kernel(struct boot_params *bp) { }
>> -static inline void __init sme_enable(struct boot_params *bp) { }
>> +static inline void sme_encrypt_kernel(struct boot_params *bp) { }
>> +static inline void sme_enable(struct boot_params *bp) { }
>
> So I think we should preserve the previous convention of marking functions
> __init in the header-declaration and at the definition site as well, and do
> the same with __head as well?
I'm not convinced about the value of prototype annotations, but have no
strong preference either.
Thanks,
tglx
On Tue, Oct 17, 2023 at 08:52:46PM +0800, Ingo Molnar wrote:
>
> * Hou Wenlong <[email protected]> wrote:
>
> > The functions sme_enable() and sme_encrypt_kernel() are only called by
> > the head code which runs in identity virtual address. Therefore, it's
> > better to mark them as __head as well.
> >
> > Signed-off-by: Hou Wenlong <[email protected]>
> > ---
> > arch/x86/include/asm/mem_encrypt.h | 8 ++++----
> > arch/x86/mm/mem_encrypt_identity.c | 27 ++++++++++++++-------------
> > 2 files changed, 18 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
> > index 359ada486fa9..48469e22a75e 100644
> > --- a/arch/x86/include/asm/mem_encrypt.h
> > +++ b/arch/x86/include/asm/mem_encrypt.h
> > @@ -46,8 +46,8 @@ void __init sme_unmap_bootdata(char *real_mode_data);
> >
> > void __init sme_early_init(void);
> >
> > -void __init sme_encrypt_kernel(struct boot_params *bp);
> > -void __init sme_enable(struct boot_params *bp);
> > +void sme_encrypt_kernel(struct boot_params *bp);
> > +void sme_enable(struct boot_params *bp);
> >
> > int __init early_set_memory_decrypted(unsigned long vaddr, unsigned long size);
> > int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size);
> > @@ -75,8 +75,8 @@ static inline void __init sme_unmap_bootdata(char *real_mode_data) { }
> >
> > static inline void __init sme_early_init(void) { }
> >
> > -static inline void __init sme_encrypt_kernel(struct boot_params *bp) { }
> > -static inline void __init sme_enable(struct boot_params *bp) { }
> > +static inline void sme_encrypt_kernel(struct boot_params *bp) { }
> > +static inline void sme_enable(struct boot_params *bp) { }
>
> So I think we should preserve the previous convention of marking functions
> __init in the header-declaration and at the definition site as well, and do
> the same with __head as well?
>
Hi Ingo,
I tried to include <asm/init.h> into <asm/mem_encrypt.h> and mark the
function declaration as __head, but it resulted in a build failure. This
is because <asm/init.h> is not self-contained; the type "pgd_t" is
defined in <asm/pgtable_types.h>, which includes <asm/mem_encrypt.h>,
leading to mutual inclusion of header files. To avoid the issue of
complicated header file inclusion, I removed the annotation from the
function declaration.
Actually, initially, I noticed that the __init definition is in
<linux/init.h>, so I first placed the __head definition in
<linux/init.h> as well. However, this conflicted with the local variable
in the "list_next_or_null_rcu" macro in <linux/rculist.h>. Then I
realized that __head was only used in x86, so I made the decision to put
it in the architecture-specific header. Considering simplicity, I chose
to put the definition in <asm/init.h>. I also attempted to put the
definition in other headers such as <asm/boot.h> and
<asm/bootparam_utils.h>, and included them in <asm/mem_encrypt.h>, but
the build still failed.
Thanks!
> Thanks,
>
> Ingo
* Hou Wenlong <[email protected]> wrote:
> On Tue, Oct 17, 2023 at 08:52:46PM +0800, Ingo Molnar wrote:
> >
> > * Hou Wenlong <[email protected]> wrote:
> >
> > > The functions sme_enable() and sme_encrypt_kernel() are only called by
> > > the head code which runs in identity virtual address. Therefore, it's
> > > better to mark them as __head as well.
> > >
> > > Signed-off-by: Hou Wenlong <[email protected]>
> > > ---
> > > arch/x86/include/asm/mem_encrypt.h | 8 ++++----
> > > arch/x86/mm/mem_encrypt_identity.c | 27 ++++++++++++++-------------
> > > 2 files changed, 18 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
> > > index 359ada486fa9..48469e22a75e 100644
> > > --- a/arch/x86/include/asm/mem_encrypt.h
> > > +++ b/arch/x86/include/asm/mem_encrypt.h
> > > @@ -46,8 +46,8 @@ void __init sme_unmap_bootdata(char *real_mode_data);
> > >
> > > void __init sme_early_init(void);
> > >
> > > -void __init sme_encrypt_kernel(struct boot_params *bp);
> > > -void __init sme_enable(struct boot_params *bp);
> > > +void sme_encrypt_kernel(struct boot_params *bp);
> > > +void sme_enable(struct boot_params *bp);
> > >
> > > int __init early_set_memory_decrypted(unsigned long vaddr, unsigned long size);
> > > int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size);
> > > @@ -75,8 +75,8 @@ static inline void __init sme_unmap_bootdata(char *real_mode_data) { }
> > >
> > > static inline void __init sme_early_init(void) { }
> > >
> > > -static inline void __init sme_encrypt_kernel(struct boot_params *bp) { }
> > > -static inline void __init sme_enable(struct boot_params *bp) { }
> > > +static inline void sme_encrypt_kernel(struct boot_params *bp) { }
> > > +static inline void sme_enable(struct boot_params *bp) { }
> >
> > So I think we should preserve the previous convention of marking functions
> > __init in the header-declaration and at the definition site as well, and do
> > the same with __head as well?
> >
> Hi Ingo,
>
> I tried to include <asm/init.h> into <asm/mem_encrypt.h> and mark the
> function declaration as __head, but it resulted in a build failure. This
> is because <asm/init.h> is not self-contained; the type "pgd_t" is
> defined in <asm/pgtable_types.h>, which includes <asm/mem_encrypt.h>,
> leading to mutual inclusion of header files. To avoid the issue of
> complicated header file inclusion, I removed the annotation from the
> function declaration.
The right solution at that point is to make <asm/init.h> self-contained...
> Actually, initially, I noticed that the __init definition is in
> <linux/init.h>, so I first placed the __head definition in
> <linux/init.h> as well. However, this conflicted with the local variable
> in the "list_next_or_null_rcu" macro in <linux/rculist.h>. Then I
> realized that __head was only used in x86, so I made the decision to put
> it in the architecture-specific header. Considering simplicity, I chose
> to put the definition in <asm/init.h>. I also attempted to put the
> definition in other headers such as <asm/boot.h> and
> <asm/bootparam_utils.h>, and included them in <asm/mem_encrypt.h>, but
> the build still failed.
When exporting a localized definition you should consider namespace
collisions - the name '__head' is way too generic, no wonder it caused
problems elsewhere.
I'd suggest naming it __init_head or so, but still keep it in a x86-only
header.
I presume keeping it all in the separate section and widening its usage has a
specific purpose? Please outline that in the changelog as well.
Ie. instead of mechanical patches that try to follow existing patterns
cargo-cult style, this area of x86 code requires well-argued, well thought
out patches that show background knowledge of the area.
Thanks,
Ingo
* Thomas Gleixner <[email protected]> wrote:
> On Tue, Oct 17 2023 at 14:52, Ingo Molnar wrote:
> > * Hou Wenlong <[email protected]> wrote:
> >> -static inline void __init sme_encrypt_kernel(struct boot_params *bp) { }
> >> -static inline void __init sme_enable(struct boot_params *bp) { }
> >> +static inline void sme_encrypt_kernel(struct boot_params *bp) { }
> >> +static inline void sme_enable(struct boot_params *bp) { }
> >
> > So I think we should preserve the previous convention of marking functions
> > __init in the header-declaration and at the definition site as well, and do
> > the same with __head as well?
>
> I'm not convinced about the value of prototype annotations, but have no
> strong preference either.
So it has some minor documentation purpose: when someone looks up a
function via the header only (I do that frequently), __init-alike
annotations really show the intended boot-only limitations of the API.
But that's a really minor Nth order benefit, I have no strong preference
either.
Thanks,
Ingo
On Wed, Oct 18, 2023 at 06:20:15PM +0800, Ingo Molnar wrote:
>
> * Hou Wenlong <[email protected]> wrote:
>
> > On Tue, Oct 17, 2023 at 08:52:46PM +0800, Ingo Molnar wrote:
> > >
> > > * Hou Wenlong <[email protected]> wrote:
> > >
> > > > The functions sme_enable() and sme_encrypt_kernel() are only called by
> > > > the head code which runs in identity virtual address. Therefore, it's
> > > > better to mark them as __head as well.
> > > >
> > > > Signed-off-by: Hou Wenlong <[email protected]>
> > > > ---
> > > > arch/x86/include/asm/mem_encrypt.h | 8 ++++----
> > > > arch/x86/mm/mem_encrypt_identity.c | 27 ++++++++++++++-------------
> > > > 2 files changed, 18 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
> > > > index 359ada486fa9..48469e22a75e 100644
> > > > --- a/arch/x86/include/asm/mem_encrypt.h
> > > > +++ b/arch/x86/include/asm/mem_encrypt.h
> > > > @@ -46,8 +46,8 @@ void __init sme_unmap_bootdata(char *real_mode_data);
> > > >
> > > > void __init sme_early_init(void);
> > > >
> > > > -void __init sme_encrypt_kernel(struct boot_params *bp);
> > > > -void __init sme_enable(struct boot_params *bp);
> > > > +void sme_encrypt_kernel(struct boot_params *bp);
> > > > +void sme_enable(struct boot_params *bp);
> > > >
> > > > int __init early_set_memory_decrypted(unsigned long vaddr, unsigned long size);
> > > > int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size);
> > > > @@ -75,8 +75,8 @@ static inline void __init sme_unmap_bootdata(char *real_mode_data) { }
> > > >
> > > > static inline void __init sme_early_init(void) { }
> > > >
> > > > -static inline void __init sme_encrypt_kernel(struct boot_params *bp) { }
> > > > -static inline void __init sme_enable(struct boot_params *bp) { }
> > > > +static inline void sme_encrypt_kernel(struct boot_params *bp) { }
> > > > +static inline void sme_enable(struct boot_params *bp) { }
> > >
> > > So I think we should preserve the previous convention of marking functions
> > > __init in the header-declaration and at the definition site as well, and do
> > > the same with __head as well?
> > >
> > Hi Ingo,
> >
> > I tried to include <asm/init.h> into <asm/mem_encrypt.h> and mark the
> > function declaration as __head, but it resulted in a build failure. This
> > is because <asm/init.h> is not self-contained; the type "pgd_t" is
> > defined in <asm/pgtable_types.h>, which includes <asm/mem_encrypt.h>,
> > leading to mutual inclusion of header files. To avoid the issue of
> > complicated header file inclusion, I removed the annotation from the
> > function declaration.
>
> The right solution at that point is to make <asm/init.h> self-contained...
>
The "pgd_t" is a typedef declaration in <asm/pgtable_types.h>, so it
cannot be forward declared. Therefore, I had to include
<asm/pgtable_types.h> into <asm/init.h> to make it self-contained.
However, <asm/pgtable_types.h> includes <asm/mem_encrypt.h>. If I
include <asm/init.h> into <asm/mem_encrypt.h> to mark functions as
__head in the header-declaration, it would result in mutual inclusion of
header files. It appears that <asm/mem_encrypt.h> is a base header that
is included in multiple headers, so adding one more header to it would
complicate things. In reality, if it is acceptable, I could move the
__head definition into <asm/mem_encrypt.h>.
> > Actually, initially, I noticed that the __init definition is in
> > <linux/init.h>, so I first placed the __head definition in
> > <linux/init.h> as well. However, this conflicted with the local variable
> > in the "list_next_or_null_rcu" macro in <linux/rculist.h>. Then I
> > realized that __head was only used in x86, so I made the decision to put
> > it in the architecture-specific header. Considering simplicity, I chose
> > to put the definition in <asm/init.h>. I also attempted to put the
> > definition in other headers such as <asm/boot.h> and
> > <asm/bootparam_utils.h>, and included them in <asm/mem_encrypt.h>, but
> > the build still failed.
>
> When exporting a localized definition you should consider namespace
> collisions - the name '__head' is way too generic, no wonder it caused
> problems elsewhere.
>
> I'd suggest naming it __init_head or so, but still keep it in a x86-only
> header.
>
> I presume keeping it all in the separate section and widening its usage has a
> specific purpose? Please outline that in the changelog as well.
>
Based on my understanding, the __head section contains the early boot
code that runs at a low identity address instead of the compile-time
address. Therefore, it must use RIP-relative addressing to access
memory. This makes the __head section special. However, when it comes to
C source code, the compiler may generate absolute addressing, which can
result in boot failure. That's why the fixup_pointer() function is
introduced in head64.c. So maybe we could consider validating the memory
access instructions in this section using objtool to ensure that the
generated instructions are PC-relative. Then we should mark all the
early boot code as __head.
Thanks!
> Ie. instead of mechanical patches that try to follow existing patterns
> cargo-cult style, this area of x86 code requires well-argued, well thought
> out patches that show background knowledge of the area.
>
> Thanks,
>
> Ingo