Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3
On 6/9/2017 1:46 PM, Andy Lutomirski wrote:
On Thu, Jun 8, 2017 at 3:38 PM, Tom Lendacky wrote:
On 6/8/2017 1:05 AM, Andy Lutomirski wrote:
On Wed, Jun 7, 2017 at 12:14 PM, Tom Lendacky
wrote:
The cr3 register entry can contain the SME encryption bit that indicates
the PGD is encrypted. The encryption bit should not be used when
creating
a virtual address for the PGD table.
Create a new function, read_cr3_pa(), that will extract the physical
address from the cr3 register. This function is then used where a virtual
address of the PGD needs to be created/used from the cr3 register.
This is going to conflict with:
https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/pcid&id=555c81e5d01a62b629ec426a2f50d27e2127c1df
We're both encountering the fact that CR3 munges the page table PA
with some other stuff, and some readers want to see the actual CR3
value and other readers just want the PA. The thing I prefer about my
patch is that I get rid of read_cr3() entirely, forcing the patch to
update every single reader, making review and conflict resolution much
safer.
I'd be willing to send a patch tomorrow that just does the split into
__read_cr3() and read_cr3_pa() (I like your name better) and then we
can both base on top of it. Would that make sense?
That makes sense to me.
Draft patch:
https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/read_cr3&id=9adebbc1071f066421a27b4f6e040190f1049624
Looks good to me. I'll look at how to best mask off the encryption bit
in CR3_ADDR_MASK for SME support. I should be able to just do an
__sme_clr() against it.
Also:
+static inline unsigned long read_cr3_pa(void)
+{
+ return (read_cr3() & PHYSICAL_PAGE_MASK);
+}
Is there any guarantee that the magic encryption bit is masked out in
PHYSICAL_PAGE_MASK? The docs make it sound like it could be any bit.
(But if it's one of the low 12 bits, that would be quite confusing.)
Right now it's bit 47 and we're steering away from any of the currently
reserved bits so we should be safe.
Should the SME init code check that it's a usable bit (i.e. outside
our physical address mask and not one of the bottom twelve bits)? If
some future CPU daftly picks, say, bit 12, we'll regret it if we
enable SME.
I think I can safely say that it will never be any of the lower 12 bits,
but let me talk to some of the hardware folks and see about the other
end of the range.
Thanks,
Tom
--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3
On Thu, Jun 8, 2017 at 3:38 PM, Tom Lendacky wrote:
> On 6/8/2017 1:05 AM, Andy Lutomirski wrote:
>>
>> On Wed, Jun 7, 2017 at 12:14 PM, Tom Lendacky
>> wrote:
>>>
>>> The cr3 register entry can contain the SME encryption bit that indicates
>>> the PGD is encrypted. The encryption bit should not be used when
>>> creating
>>> a virtual address for the PGD table.
>>>
>>> Create a new function, read_cr3_pa(), that will extract the physical
>>> address from the cr3 register. This function is then used where a virtual
>>> address of the PGD needs to be created/used from the cr3 register.
>>
>>
>> This is going to conflict with:
>>
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/pcid&id=555c81e5d01a62b629ec426a2f50d27e2127c1df
>>
>> We're both encountering the fact that CR3 munges the page table PA
>> with some other stuff, and some readers want to see the actual CR3
>> value and other readers just want the PA. The thing I prefer about my
>> patch is that I get rid of read_cr3() entirely, forcing the patch to
>> update every single reader, making review and conflict resolution much
>> safer.
>>
>> I'd be willing to send a patch tomorrow that just does the split into
>> __read_cr3() and read_cr3_pa() (I like your name better) and then we
>> can both base on top of it. Would that make sense?
>
>
> That makes sense to me.
Draft patch:
https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/read_cr3&id=9adebbc1071f066421a27b4f6e040190f1049624
>
>>
>> Also:
>>
>>> +static inline unsigned long read_cr3_pa(void)
>>> +{
>>> + return (read_cr3() & PHYSICAL_PAGE_MASK);
>>> +}
>>
>>
>> Is there any guarantee that the magic encryption bit is masked out in
>> PHYSICAL_PAGE_MASK? The docs make it sound like it could be any bit.
>> (But if it's one of the low 12 bits, that would be quite confusing.)
>
>
> Right now it's bit 47 and we're steering away from any of the currently
> reserved bits so we should be safe.
Should the SME init code check that it's a usable bit (i.e. outside
our physical address mask and not one of the bottom twelve bits)? If
some future CPU daftly picks, say, bit 12, we'll regret it if we
enable SME.
--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3
On 6/8/2017 1:05 AM, Andy Lutomirski wrote:
On Wed, Jun 7, 2017 at 12:14 PM, Tom Lendacky wrote:
The cr3 register entry can contain the SME encryption bit that indicates
the PGD is encrypted. The encryption bit should not be used when creating
a virtual address for the PGD table.
Create a new function, read_cr3_pa(), that will extract the physical
address from the cr3 register. This function is then used where a virtual
address of the PGD needs to be created/used from the cr3 register.
This is going to conflict with:
https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/pcid&id=555c81e5d01a62b629ec426a2f50d27e2127c1df
We're both encountering the fact that CR3 munges the page table PA
with some other stuff, and some readers want to see the actual CR3
value and other readers just want the PA. The thing I prefer about my
patch is that I get rid of read_cr3() entirely, forcing the patch to
update every single reader, making review and conflict resolution much
safer.
I'd be willing to send a patch tomorrow that just does the split into
__read_cr3() and read_cr3_pa() (I like your name better) and then we
can both base on top of it. Would that make sense?
That makes sense to me.
Also:
+static inline unsigned long read_cr3_pa(void)
+{
+ return (read_cr3() & PHYSICAL_PAGE_MASK);
+}
Is there any guarantee that the magic encryption bit is masked out in
PHYSICAL_PAGE_MASK? The docs make it sound like it could be any bit.
(But if it's one of the low 12 bits, that would be quite confusing.)
Right now it's bit 47 and we're steering away from any of the currently
reserved bits so we should be safe.
Thanks,
Tom
--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3
On 06/08/2017 05:02 PM, Tom Lendacky wrote: > On 6/8/2017 3:51 PM, Boris Ostrovsky wrote: >> >>> What may be needed is making sure X86_FEATURE_SME is not set for PV guests. >>> >>> And that may be something that Xen will need to control through either >>> CPUID or MSR support for the PV guests. >> >> >> Only on newer versions of Xen. On earlier versions (2-3 years old) leaf >> 0x8007 is passed to the guest unchanged. And so is MSR_K8_SYSCFG. > > The SME feature is in leaf 0x801f, is that leaf passed to the guest > unchanged? Oh, I misread the patch where X86_FEATURE_SME is defined. Then all versions, including the current one, pass it unchanged. All that's needed is setup_clear_cpu_cap(X86_FEATURE_SME) in xen_init_capabilities(). -boris -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3
On 6/8/2017 3:51 PM, Boris Ostrovsky wrote: What may be needed is making sure X86_FEATURE_SME is not set for PV guests. And that may be something that Xen will need to control through either CPUID or MSR support for the PV guests. Only on newer versions of Xen. On earlier versions (2-3 years old) leaf 0x8007 is passed to the guest unchanged. And so is MSR_K8_SYSCFG. The SME feature is in leaf 0x801f, is that leaf passed to the guest unchanged? Thanks, Tom -boris -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3
> >> What may be needed is making sure X86_FEATURE_SME is not set for PV >> guests. > > And that may be something that Xen will need to control through either > CPUID or MSR support for the PV guests. Only on newer versions of Xen. On earlier versions (2-3 years old) leaf 0x8007 is passed to the guest unchanged. And so is MSR_K8_SYSCFG. -boris -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3
On 6/7/2017 5:06 PM, Boris Ostrovsky wrote:
On 06/07/2017 03:14 PM, Tom Lendacky wrote:
The cr3 register entry can contain the SME encryption bit that indicates
the PGD is encrypted. The encryption bit should not be used when creating
a virtual address for the PGD table.
Create a new function, read_cr3_pa(), that will extract the physical
address from the cr3 register. This function is then used where a virtual
address of the PGD needs to be created/used from the cr3 register.
Signed-off-by: Tom Lendacky
---
arch/x86/include/asm/special_insns.h |9 +
arch/x86/kernel/head64.c |2 +-
arch/x86/mm/fault.c | 10 +-
arch/x86/mm/ioremap.c|2 +-
arch/x86/platform/olpc/olpc-xo1-pm.c |2 +-
arch/x86/power/hibernate_64.c|2 +-
arch/x86/xen/mmu_pv.c|6 +++---
7 files changed, 21 insertions(+), 12 deletions(-)
...
diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index 1f386d7..2dc5243 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -2022,7 +2022,7 @@ static phys_addr_t __init xen_early_virt_to_phys(unsigned
long vaddr)
pmd_t pmd;
pte_t pte;
- pa = read_cr3();
+ pa = read_cr3_pa();
pgd = native_make_pgd(xen_read_phys_ulong(pa + pgd_index(vaddr) *
sizeof(pgd)));
if (!pgd_present(pgd))
@@ -2102,7 +2102,7 @@ void __init xen_relocate_p2m(void)
pt_phys = pmd_phys + PFN_PHYS(n_pmd);
p2m_pfn = PFN_DOWN(pt_phys) + n_pt;
- pgd = __va(read_cr3());
+ pgd = __va(read_cr3_pa());
new_p2m = (unsigned long *)(2 * PGDIR_SIZE);
idx_p4d = 0;
save_pud = n_pud;
@@ -2209,7 +2209,7 @@ static void __init xen_write_cr3_init(unsigned long cr3)
{
unsigned long pfn = PFN_DOWN(__pa(swapper_pg_dir));
- BUG_ON(read_cr3() != __pa(initial_page_table));
+ BUG_ON(read_cr3_pa() != __pa(initial_page_table));
BUG_ON(cr3 != __pa(swapper_pg_dir));
/*
(Please copy Xen maintainers when modifying xen-related files.)
Sorry about that, missed adding the Xen maintainers when I added this
change.
Given that page tables for Xen PV guests are controlled by the
hypervisor I don't think this change (although harmless) is necessary.
I can back this change out if the Xen maintainers think that's best.
What may be needed is making sure X86_FEATURE_SME is not set for PV guests.
And that may be something that Xen will need to control through either
CPUID or MSR support for the PV guests.
Thanks,
Tom
-boris
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3
Hi Tom,
[auto build test ERROR on linus/master]
[also build test ERROR on v4.12-rc4 next-20170607]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Tom-Lendacky/x86-Secure-Memory-Encryption-AMD/20170608-104147
config: um-x86_64_defconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=um SUBARCH=x86_64
All errors (new ones prefixed by >>):
In file included from arch/x86/include/asm/cacheflush.h:6:0,
from include/linux/highmem.h:11,
from net/core/sock.c:116:
arch/x86/include/asm/special_insns.h: In function 'native_read_cr3_pa':
>> arch/x86/include/asm/special_insns.h:239:30: error: 'PHYSICAL_PAGE_MASK'
>> undeclared (first use in this function)
return (native_read_cr3() & PHYSICAL_PAGE_MASK);
^~
arch/x86/include/asm/special_insns.h:239:30: note: each undeclared
identifier is reported only once for each function it appears in
arch/x86/include/asm/special_insns.h: In function 'read_cr3_pa':
arch/x86/include/asm/special_insns.h:244:23: error: 'PHYSICAL_PAGE_MASK'
undeclared (first use in this function)
return (read_cr3() & PHYSICAL_PAGE_MASK);
^~
vim +/PHYSICAL_PAGE_MASK +239 arch/x86/include/asm/special_insns.h
233 }
234
235 #define nop() asm volatile ("nop")
236
237 static inline unsigned long native_read_cr3_pa(void)
238 {
> 239 return (native_read_cr3() & PHYSICAL_PAGE_MASK);
240 }
241
242 static inline unsigned long read_cr3_pa(void)
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3
On Wed, Jun 7, 2017 at 12:14 PM, Tom Lendacky wrote:
> The cr3 register entry can contain the SME encryption bit that indicates
> the PGD is encrypted. The encryption bit should not be used when creating
> a virtual address for the PGD table.
>
> Create a new function, read_cr3_pa(), that will extract the physical
> address from the cr3 register. This function is then used where a virtual
> address of the PGD needs to be created/used from the cr3 register.
This is going to conflict with:
https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/pcid&id=555c81e5d01a62b629ec426a2f50d27e2127c1df
We're both encountering the fact that CR3 munges the page table PA
with some other stuff, and some readers want to see the actual CR3
value and other readers just want the PA. The thing I prefer about my
patch is that I get rid of read_cr3() entirely, forcing the patch to
update every single reader, making review and conflict resolution much
safer.
I'd be willing to send a patch tomorrow that just does the split into
__read_cr3() and read_cr3_pa() (I like your name better) and then we
can both base on top of it. Would that make sense?
Also:
> +static inline unsigned long read_cr3_pa(void)
> +{
> + return (read_cr3() & PHYSICAL_PAGE_MASK);
> +}
Is there any guarantee that the magic encryption bit is masked out in
PHYSICAL_PAGE_MASK? The docs make it sound like it could be any bit.
(But if it's one of the low 12 bits, that would be quite confusing.)
--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3
On 06/07/2017 03:14 PM, Tom Lendacky wrote:
> The cr3 register entry can contain the SME encryption bit that indicates
> the PGD is encrypted. The encryption bit should not be used when creating
> a virtual address for the PGD table.
>
> Create a new function, read_cr3_pa(), that will extract the physical
> address from the cr3 register. This function is then used where a virtual
> address of the PGD needs to be created/used from the cr3 register.
>
> Signed-off-by: Tom Lendacky
> ---
> arch/x86/include/asm/special_insns.h |9 +
> arch/x86/kernel/head64.c |2 +-
> arch/x86/mm/fault.c | 10 +-
> arch/x86/mm/ioremap.c|2 +-
> arch/x86/platform/olpc/olpc-xo1-pm.c |2 +-
> arch/x86/power/hibernate_64.c|2 +-
> arch/x86/xen/mmu_pv.c|6 +++---
> 7 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/include/asm/special_insns.h
> b/arch/x86/include/asm/special_insns.h
> index 12af3e3..d8e8ace 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -234,6 +234,15 @@ static inline void clwb(volatile void *__p)
>
> #define nop() asm volatile ("nop")
>
> +static inline unsigned long native_read_cr3_pa(void)
> +{
> + return (native_read_cr3() & PHYSICAL_PAGE_MASK);
> +}
> +
> +static inline unsigned long read_cr3_pa(void)
> +{
> + return (read_cr3() & PHYSICAL_PAGE_MASK);
> +}
>
> #endif /* __KERNEL__ */
>
> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> index 43b7002..dc03624 100644
> --- a/arch/x86/kernel/head64.c
> +++ b/arch/x86/kernel/head64.c
> @@ -55,7 +55,7 @@ int __init early_make_pgtable(unsigned long address)
> pmdval_t pmd, *pmd_p;
>
> /* Invalid address or early pgt is done ? */
> - if (physaddr >= MAXMEM || read_cr3() != __pa_nodebug(early_level4_pgt))
> + if (physaddr >= MAXMEM || read_cr3_pa() !=
> __pa_nodebug(early_level4_pgt))
> return -1;
>
> again:
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 8ad91a0..2a1fa10c 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -346,7 +346,7 @@ static noinline int vmalloc_fault(unsigned long address)
>* Do _not_ use "current" here. We might be inside
>* an interrupt in the middle of a task switch..
>*/
> - pgd_paddr = read_cr3();
> + pgd_paddr = read_cr3_pa();
> pmd_k = vmalloc_sync_one(__va(pgd_paddr), address);
> if (!pmd_k)
> return -1;
> @@ -388,7 +388,7 @@ static bool low_pfn(unsigned long pfn)
>
> static void dump_pagetable(unsigned long address)
> {
> - pgd_t *base = __va(read_cr3());
> + pgd_t *base = __va(read_cr3_pa());
> pgd_t *pgd = &base[pgd_index(address)];
> p4d_t *p4d;
> pud_t *pud;
> @@ -451,7 +451,7 @@ static noinline int vmalloc_fault(unsigned long address)
>* happen within a race in page table update. In the later
>* case just flush:
>*/
> - pgd = (pgd_t *)__va(read_cr3()) + pgd_index(address);
> + pgd = (pgd_t *)__va(read_cr3_pa()) + pgd_index(address);
> pgd_ref = pgd_offset_k(address);
> if (pgd_none(*pgd_ref))
> return -1;
> @@ -555,7 +555,7 @@ static int bad_address(void *p)
>
> static void dump_pagetable(unsigned long address)
> {
> - pgd_t *base = __va(read_cr3() & PHYSICAL_PAGE_MASK);
> + pgd_t *base = __va(read_cr3_pa());
> pgd_t *pgd = base + pgd_index(address);
> p4d_t *p4d;
> pud_t *pud;
> @@ -700,7 +700,7 @@ static int is_f00f_bug(struct pt_regs *regs, unsigned
> long address)
> pgd_t *pgd;
> pte_t *pte;
>
> - pgd = __va(read_cr3() & PHYSICAL_PAGE_MASK);
> + pgd = __va(read_cr3_pa());
> pgd += pgd_index(address);
>
> pte = lookup_address_in_pgd(pgd, address, &level);
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 2a0fa89..e6305dd 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -427,7 +427,7 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
> static inline pmd_t * __init early_ioremap_pmd(unsigned long addr)
> {
> /* Don't assume we're using swapper_pg_dir at this point */
> - pgd_t *base = __va(read_cr3());
> + pgd_t *base = __va(read_cr3_pa());
> pgd_t *pgd = &base[pgd_index(addr)];
> p4d_t *p4d = p4d_offset(pgd, addr);
> pud_t *pud = pud_offset(p4d, addr);
> diff --git a/arch/x86/platform/olpc/olpc-xo1-pm.c
> b/arch/x86/platform/olpc/olpc-xo1-pm.c
> index c5350fd..0668aaf 100644
> --- a/arch/x86/platform/olpc/olpc-xo1-pm.c
> +++ b/arch/x86/platform/olpc/olpc-xo1-pm.c
> @@ -77,7 +77,7 @@ static int xo1_power_state_enter(suspend_state_t pm_state)
>
> asmlinkage __visible int xo1_do_sleep(u8 sleep_state)
> {
> - void *pgd_addr = __va(read_cr3());
> + void *pgd_addr = __va(read_cr3_pa()
