Messages in this thread | | | Subject | Re: [PATCH] x86/mm: Rewrite sme_populate_pgd() in a more sensible way | From | Tom Lendacky <> | Date | Fri, 8 Dec 2017 08:43:55 -0600 |
| |
On 12/4/2017 12:50 PM, Kirill A. Shutemov wrote: > On Mon, Dec 04, 2017 at 12:33:01PM -0600, Tom Lendacky wrote: >> On 12/4/2017 10:34 AM, Kirill A. Shutemov wrote: >>> On Mon, Dec 04, 2017 at 04:00:26PM +0000, Tom Lendacky wrote: >>>> On 12/4/2017 8:57 AM, Kirill A. Shutemov wrote: >>>>> On Mon, Dec 04, 2017 at 08:19:11AM -0600, Tom Lendacky wrote: >>>>>> On 12/4/2017 5:23 AM, Kirill A. Shutemov wrote: >>>>>>> sme_populate_pgd() open-codes a lot of things that are not needed to be >>>>>>> open-coded. >>>>>>> >>>>>>> Let's rewrite it in a more stream-lined way. >>>>>>> >>>>>>> This would also buy us boot-time switching between support between >>>>>>> paging modes, when rest of the pieces will be upstream. >>>>>> >>>>>> Hi Kirill, >>>>>> >>>>>> Unfortunately, some of these can't be changed. The use of p4d_offset(), >>>>>> pud_offset(), etc., use non-identity mapped virtual addresses which cause >>>>>> failures at this point of the boot process. >>>>> >>>>> Wat? Virtual address is virtual address. p?d_offset() doesn't care about >>>>> what mapping you're using. >>>> >>>> Yes it does. For example, pmd_offset() issues a pud_page_addr() call, >>>> which does a __va() returning a non-identity mapped address (0xffff88...). >>>> Only identity mapped virtual addresses have been setup at this point, so >>>> the use of that virtual address panics the kernel. >>> >>> Stupid me. You are right. >>> >>> What about something like this: >>> >>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c >>> index d9a9e9fc75dd..65e0d68f863f 100644 >>> --- a/arch/x86/mm/mem_encrypt.c >>> +++ b/arch/x86/mm/mem_encrypt.c >>> @@ -12,6 +12,23 @@ >>> #define DISABLE_BRANCH_PROFILING >>> +/* >>> + * Since we're dealing with identity mappings, physical and virtual >>> + * addresses are the same, so override these defines which are ultimately >>> + * used by the headers in misc.h. >>> + */ >>> +#define __pa(x) ((unsigned long)(x)) >>> +#define __va(x) ((void *)((unsigned long)(x))) >> >> No, you can't do this. There are routines in this file that are called >> after the kernel has switched to its standard virtual address map where >> this definition of __va() will likely cause a failure. > > Let's than split it up into separate compilation unit. > >>> +/* >>> + * Special hack: we have to be careful, because no indirections are >>> + * allowed here, and paravirt_ops is a kind of one. As it will only run in >>> + * baremetal anyway, we just keep it from happening. (This list needs to >>> + * be extended when new paravirt and debugging variants are added.) >>> + */ >>> +#undef CONFIG_PARAVIRT >>> +#undef CONFIG_PARAVIRT_SPINLOCKS >> >> I'd really, really like to avoid doing something like this. > > Any other proposals? > > Current code is way too hairy and hard to modify.
I'd like to hear some other opinions, but if there are none, I don't see an issue with splitting this up, I guess. I just dislike doing some of these hacks, but if it makes things cleaner and simpler to understand, then I guess I can't really object.
Thanks, Tom
>
| |