Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1660282 > unrolled thread

Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against  just the physical address in cr3 Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-06-08 00:10 +0200
    Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against  just the physical address in cr3 Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-06-08 23:00 +0200
      Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against  just the physical address in cr3 Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-06-08 23:20 +0200
        Re: [Xen-devel] [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use  __va() against just the physical address in cr3 Andrew Cooper <andrew.cooper3@citrix.com> - 2017-06-09 00:10 +0200
          Re: [Xen-devel] [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use  __va() against just the physical address in cr3 Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-06-09 20:50 +0200
            Re: [Xen-devel] [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use  __va() against just the physical address in cr3 Andrew Cooper <andrew.cooper3@citrix.com> - 2017-06-09 21:00 +0200
            Re: [Xen-devel] [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use  __va() against just the physical address in cr3 Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-06-09 21:50 +0200

#1660282 — 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 <thomas.lendacky@amd.com>
> ---
>  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());
>  
>  	/* Program wakeup mask (using dword access to CS5536_PM1_EN) */
>  	outl(wakeup_mask << 16, acpi_base + CS5536_PM1_STS);
> diff --git a/arch/x86/power/hibernate_64.c b/arch/x86/power/hibernate_64.c
> index a6e21fe..0a7650d 100644
> --- a/arch/x86/power/hibernate_64.c
> +++ b/arch/x86/power/hibernate_64.c
> @@ -150,7 +150,7 @@ static int relocate_restore_code(void)
>  	memcpy((void *)relocated_restore_code, &core_restore_code, PAGE_SIZE);
>  
>  	/* Make the page containing the relocated code executable */
> -	pgd = (pgd_t *)__va(read_cr3()) + pgd_index(relocated_restore_code);
> +	pgd = (pgd_t *)__va(read_cr3_pa()) + pgd_index(relocated_restore_code);
>  	p4d = p4d_offset(pgd, relocated_restore_code);
>  	if (p4d_large(*p4d)) {
>  		set_p4d(p4d, __p4d(p4d_val(*p4d) & ~_PAGE_NX));
> 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.)

Given that page tables for Xen PV guests are controlled by the
hypervisor I don't think this change (although harmless) is necessary.
What may be needed is making sure X86_FEATURE_SME is not set for PV guests.

-boris

[toc] | [next] | [standalone]

#1661685

>
>> 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
0x80000007 is passed to the guest unchanged. And so is MSR_K8_SYSCFG.

-boris

[toc] | [prev] | [next] | [standalone]

#1661697

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
>> 0x80000007 is passed to the guest unchanged. And so is MSR_K8_SYSCFG.
>
> The SME feature is in leaf 0x8000001f, 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

[toc] | [prev] | [next] | [standalone]

#1661734 — Re: [Xen-devel] [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3

On 08/06/2017 22:17, Boris Ostrovsky wrote:
> 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
>>> 0x80000007 is passed to the guest unchanged. And so is MSR_K8_SYSCFG.
>> The SME feature is in leaf 0x8000001f, 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().

AMD processors still don't support CPUID Faulting (or at least, I
couldn't find any reference to it in the latest docs), so we cannot
actually hide SME from a guest which goes looking at native CPUID. 
Furthermore, I'm not aware of any CPUID masking support covering that leaf.

However, if Linux is using the paravirtual cpuid hook, things are
slightly better.

On Xen 4.9 and later, no guests will see the feature.  On earlier
versions of Xen (before I fixed the logic), plain domUs will not see the
feature, while dom0 will.

For safely, I'd recommend unilaterally clobbering the feature as Boris
suggested.  There is no way SME will be supportable on a per-PV guest
basis, although (as far as I am aware) Xen as a whole would be able to
encompass itself and all of its PV guests inside one single SME instance.

~Andrew

[toc] | [prev] | [next] | [standalone]

#1662683 — Re: [Xen-devel] [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3

On 06/09/2017 02:36 PM, Tom Lendacky wrote:
> On 6/8/2017 5:01 PM, Andrew Cooper wrote:
>> On 08/06/2017 22:17, Boris Ostrovsky wrote:
>>> 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
>>>>> 0x80000007 is passed to the guest unchanged. And so is MSR_K8_SYSCFG.
>>>> The SME feature is in leaf 0x8000001f, 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().
>>
>> AMD processors still don't support CPUID Faulting (or at least, I
>> couldn't find any reference to it in the latest docs), so we cannot
>> actually hide SME from a guest which goes looking at native CPUID.
>> Furthermore, I'm not aware of any CPUID masking support covering that
>> leaf.
>>
>> However, if Linux is using the paravirtual cpuid hook, things are
>> slightly better.
>>
>> On Xen 4.9 and later, no guests will see the feature.  On earlier
>> versions of Xen (before I fixed the logic), plain domUs will not see the
>> feature, while dom0 will.
>>
>> For safely, I'd recommend unilaterally clobbering the feature as Boris
>> suggested.  There is no way SME will be supportable on a per-PV guest
>
> That may be too late. Early boot support in head_64.S will make calls to
> check for the feature (through CPUID and MSR), set the sme_me_mask and
> encrypt the kernel in place. Is there another way to approach this?


PV guests don't go through Linux x86 early boot code. They start at
xen_start_kernel() (well, xen-head.S:startup_xen(), really) and  merge
with baremetal path at x86_64_start_reservations() (for 64-bit).


-boris

>
>> basis, although (as far as I am aware) Xen as a whole would be able to
>> encompass itself and all of its PV guests inside one single SME
>> instance.
>
> Yes, that is correct.
>
> Thanks,
> Tom
>
>>
>> ~Andrew
>>

[toc] | [prev] | [next] | [standalone]

#1662691 — Re: [Xen-devel] [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3

On 09/06/17 19:43, Boris Ostrovsky wrote:
> On 06/09/2017 02:36 PM, Tom Lendacky wrote:
>>> basis, although (as far as I am aware) Xen as a whole would be able to
>>> encompass itself and all of its PV guests inside one single SME
>>> instance.
>> Yes, that is correct.

Thinking more about this, it would only be possible if all the PV guests
were SME-aware and understood not to choke when it finds a frame with a
high address bit set.

I expect the only viable way to implement this (should we wish) is to
have PV guests explicitly signal support (probably via an ELF note),
after which it needs to know about the existence of SME, the meaning of
the encrypted bit in PTEs, and to defer all configuration responsibility
to Xen.

~Andrew

[toc] | [prev] | [next] | [standalone]

#1662711 — Re: [Xen-devel] [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3

>>
>> PV guests don't go through Linux x86 early boot code. They start at
>> xen_start_kernel() (well, xen-head.S:startup_xen(), really) and  merge
>> with baremetal path at x86_64_start_reservations() (for 64-bit).
>>
>
> Ok, I don't think anything needs to be done then. The sme_me_mask is set
> in sme_enable() which is only called from head_64.S. If the sme_me_mask
> isn't set then SME won't be active. The feature will just report the
> capability of the processor, but that doesn't mean it is active. If you
> still want the feature to be clobbered we can do that, though.

I'd prefer to explicitly clear to avoid any ambiguity.

-boris

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web