Re: [Xen-devel] [PATCH v2 2/2] x86/xpti: don't map stack guard pages

2018-03-05 Thread Jan Beulich 
>>> On 02.03.18 at 18:33,  wrote:
> On 02/03/18 14:35, Jan Beulich wrote:
>> Other than for the main mappings, don't even do this in release builds,
>> as there are no huge page shattering concerns here.
>>
>> Signed-off-by: Jan Beulich 
> 
> Acked-by: Andrew Cooper , although I think
> somewhere (even if its only the commit message) might want to identify
> that this is safe to the AMD triple fault issue, because even if someone
> enabled XPTI, it only takes effect for PV guests, rather than HVM.  Also,

I've added

"Note that since we don't run on the restructed page tables while HVM
 guests execute, the non-present mappings won't trigger the triple fault
 issue AMD SVM is susceptible to with our current placement of STGI vs
 TR loading."

>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -5576,6 +5576,14 @@ void memguard_unguard_stack(void *p)
>> STACK_SIZE - PRIMARY_STACK_SIZE - IST_MAX * 
>> PAGE_SIZE);
>>  }
>>  
>> +bool memguard_is_stack_guard_page(unsigned long addr)
>> +{
>> +addr &= STACK_SIZE - 1;
>> +
>> +return addr >= IST_MAX * PAGE_SIZE &&
>> +   addr < STACK_SIZE - PRIMARY_STACK_SIZE;
>> +}
> 
> This probably would be better as a static inline, rather than a call
> into a separate translation unit, at which point a clever compiler might
> be able to split the loop in two (and may actually have an easier time
> doing so if the logic was expressed in terms of get_stack_page()).

I've consciously decided against an inline function here, because I
wanted the new function to live next to the existing ones, in order
to reduce the risk of someone not changing all three in lock step. I
don't think performance is a primary concern for AP bringup code.

As to using get_stack_page() - that would be an option, but to be
honest I prefer the current way of expressing things because it
better pairs with how the other two functions are written.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/xpti: don't map stack guard pages

2018-03-02 Thread Andrew Cooper 
On 02/03/18 14:35, Jan Beulich wrote:
> Other than for the main mappings, don't even do this in release builds,
> as there are no huge page shattering concerns here.
>
> Signed-off-by: Jan Beulich 

Acked-by: Andrew Cooper , although I think
somewhere (even if its only the commit message) might want to identify
that this is safe to the AMD triple fault issue, because even if someone
enabled XPTI, it only takes effect for PV guests, rather than HVM.  Also,

> ---
> v2: New.
>
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -799,7 +799,8 @@ static int setup_cpu_root_pgt(unsigned i
>  
>  /* Install direct map page table entries for stack, IDT, and TSS. */
>  for ( off = rc = 0; !rc && off < STACK_SIZE; off += PAGE_SIZE )
> -rc = clone_mapping(__va(__pa(stack_base[cpu])) + off, rpt);
> +if ( !memguard_is_stack_guard_page(off) )
> +rc = clone_mapping(__va(__pa(stack_base[cpu])) + off, rpt);
>  
>  if ( !rc )
>  rc = clone_mapping(idt_tables[cpu], rpt);
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5576,6 +5576,14 @@ void memguard_unguard_stack(void *p)
> STACK_SIZE - PRIMARY_STACK_SIZE - IST_MAX * 
> PAGE_SIZE);
>  }
>  
> +bool memguard_is_stack_guard_page(unsigned long addr)
> +{
> +addr &= STACK_SIZE - 1;
> +
> +return addr >= IST_MAX * PAGE_SIZE &&
> +   addr < STACK_SIZE - PRIMARY_STACK_SIZE;
> +}

This probably would be better as a static inline, rather than a call
into a separate translation unit, at which point a clever compiler might
be able to split the loop in two (and may actually have an easier time
doing so if the logic was expressed in terms of get_stack_page()).

~Andrew

> +
>  void arch_dump_shared_mem_info(void)
>  {
>  printk("Shared frames %u -- Saved frames %u\n",
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -519,6 +519,7 @@ void memguard_unguard_range(void *p, uns
>  
>  void memguard_guard_stack(void *p);
>  void memguard_unguard_stack(void *p);
> +bool __attribute_const__ memguard_is_stack_guard_page(unsigned long addr);
>  
>  struct mmio_ro_emulate_ctxt {
>  unsigned long cr2;
>
>
>


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/xpti: don't map stack guard pages

2018-03-02 Thread Jan Beulich 
>>> On 02.03.18 at 17:41,  wrote:
> On 02/03/18 17:10, Jan Beulich wrote:
> On 02.03.18 at 16:43,  wrote:
>>> On 02/03/18 15:35, Jan Beulich wrote:
 --- a/xen/arch/x86/mm.c
 +++ b/xen/arch/x86/mm.c
 @@ -5576,6 +5576,14 @@ void memguard_unguard_stack(void *p)
 STACK_SIZE - PRIMARY_STACK_SIZE - IST_MAX * 
 PAGE_SIZE);
  }
  
 +bool memguard_is_stack_guard_page(unsigned long addr)
 +{
 +addr &= STACK_SIZE - 1;
 +
 +return addr >= IST_MAX * PAGE_SIZE &&
 +   addr < STACK_SIZE - PRIMARY_STACK_SIZE;
 +}
 +
>>>
>>> What about making use of memguard_is_stack_guard_page() in
>>> memguard_[un]guard_stack() ?
>> 
>> I was considering this as a follow-up step.
>> 
>>> This would at once ensure the other unused
>>> pages won't be accessed accidentally somewhere.
>> 
>> I don't understand this part, though.
> 
> Today memguard_guard_stack() touches only one of the unused pages, not
> all of them.

That was earlier today, but not anymore at the time I sent this
patch.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/xpti: don't map stack guard pages

2018-03-02 Thread Juergen Gross 
On 02/03/18 17:10, Jan Beulich wrote:
 On 02.03.18 at 16:43,  wrote:
>> On 02/03/18 15:35, Jan Beulich wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -5576,6 +5576,14 @@ void memguard_unguard_stack(void *p)
>>> STACK_SIZE - PRIMARY_STACK_SIZE - IST_MAX * 
>>> PAGE_SIZE);
>>>  }
>>>  
>>> +bool memguard_is_stack_guard_page(unsigned long addr)
>>> +{
>>> +addr &= STACK_SIZE - 1;
>>> +
>>> +return addr >= IST_MAX * PAGE_SIZE &&
>>> +   addr < STACK_SIZE - PRIMARY_STACK_SIZE;
>>> +}
>>> +
>>
>> What about making use of memguard_is_stack_guard_page() in
>> memguard_[un]guard_stack() ?
> 
> I was considering this as a follow-up step.
> 
>> This would at once ensure the other unused
>> pages won't be accessed accidentally somewhere.
> 
> I don't understand this part, though.

Today memguard_guard_stack() touches only one of the unused pages, not
all of them.


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/xpti: don't map stack guard pages

2018-03-02 Thread Jan Beulich 
>>> On 02.03.18 at 16:43,  wrote:
> On 02/03/18 15:35, Jan Beulich wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -5576,6 +5576,14 @@ void memguard_unguard_stack(void *p)
>> STACK_SIZE - PRIMARY_STACK_SIZE - IST_MAX * 
>> PAGE_SIZE);
>>  }
>>  
>> +bool memguard_is_stack_guard_page(unsigned long addr)
>> +{
>> +addr &= STACK_SIZE - 1;
>> +
>> +return addr >= IST_MAX * PAGE_SIZE &&
>> +   addr < STACK_SIZE - PRIMARY_STACK_SIZE;
>> +}
>> +
> 
> What about making use of memguard_is_stack_guard_page() in
> memguard_[un]guard_stack() ?

I was considering this as a follow-up step.

> This would at once ensure the other unused
> pages won't be accessed accidentally somewhere.

I don't understand this part, though.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/xpti: don't map stack guard pages

2018-03-02 Thread Juergen Gross 
On 02/03/18 15:35, Jan Beulich wrote:
> Other than for the main mappings, don't even do this in release builds,
> as there are no huge page shattering concerns here.
> 
> Signed-off-by: Jan Beulich 
> ---
> v2: New.
> 
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -799,7 +799,8 @@ static int setup_cpu_root_pgt(unsigned i
>  
>  /* Install direct map page table entries for stack, IDT, and TSS. */
>  for ( off = rc = 0; !rc && off < STACK_SIZE; off += PAGE_SIZE )
> -rc = clone_mapping(__va(__pa(stack_base[cpu])) + off, rpt);
> +if ( !memguard_is_stack_guard_page(off) )
> +rc = clone_mapping(__va(__pa(stack_base[cpu])) + off, rpt);
>  
>  if ( !rc )
>  rc = clone_mapping(idt_tables[cpu], rpt);
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5576,6 +5576,14 @@ void memguard_unguard_stack(void *p)
> STACK_SIZE - PRIMARY_STACK_SIZE - IST_MAX * 
> PAGE_SIZE);
>  }
>  
> +bool memguard_is_stack_guard_page(unsigned long addr)
> +{
> +addr &= STACK_SIZE - 1;
> +
> +return addr >= IST_MAX * PAGE_SIZE &&
> +   addr < STACK_SIZE - PRIMARY_STACK_SIZE;
> +}
> +

What about making use of memguard_is_stack_guard_page() in
memguard_[un]guard_stack() ? This would at once ensure the other unused
pages won't be accessed accidentally somewhere.


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 2/2] x86/xpti: don't map stack guard pages

2018-03-02 Thread Jan Beulich 
Other than for the main mappings, don't even do this in release builds,
as there are no huge page shattering concerns here.

Signed-off-by: Jan Beulich 
---
v2: New.

--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -799,7 +799,8 @@ static int setup_cpu_root_pgt(unsigned i
 
 /* Install direct map page table entries for stack, IDT, and TSS. */
 for ( off = rc = 0; !rc && off < STACK_SIZE; off += PAGE_SIZE )
-rc = clone_mapping(__va(__pa(stack_base[cpu])) + off, rpt);
+if ( !memguard_is_stack_guard_page(off) )
+rc = clone_mapping(__va(__pa(stack_base[cpu])) + off, rpt);
 
 if ( !rc )
 rc = clone_mapping(idt_tables[cpu], rpt);
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5576,6 +5576,14 @@ void memguard_unguard_stack(void *p)
STACK_SIZE - PRIMARY_STACK_SIZE - IST_MAX * 
PAGE_SIZE);
 }
 
+bool memguard_is_stack_guard_page(unsigned long addr)
+{
+addr &= STACK_SIZE - 1;
+
+return addr >= IST_MAX * PAGE_SIZE &&
+   addr < STACK_SIZE - PRIMARY_STACK_SIZE;
+}
+
 void arch_dump_shared_mem_info(void)
 {
 printk("Shared frames %u -- Saved frames %u\n",
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -519,6 +519,7 @@ void memguard_unguard_range(void *p, uns
 
 void memguard_guard_stack(void *p);
 void memguard_unguard_stack(void *p);
+bool __attribute_const__ memguard_is_stack_guard_page(unsigned long addr);
 
 struct mmio_ro_emulate_ctxt {
 unsigned long cr2;




___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel