subject:"\[Xen\-devel\] \[PATCH RFC v2 10\/12\] x86\: allocate per\-vcpu stacks for interrupt entries"

Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries

2018-02-13 Thread Jan Beulich

>>> On 09.02.18 at 13:35,  wrote:
> On 30/01/18 16:40, Jan Beulich wrote:
> On 22.01.18 at 13:32,  wrote:
>>> @@ -37,10 +52,24 @@ struct vcpu;
>>>  
>>>  struct cpu_info {
>>>  struct cpu_user_regs guest_cpu_user_regs;
>>> -unsigned int processor_id;
>>> -struct vcpu *current_vcpu;
>>> -unsigned long per_cpu_offset;
>>> -unsigned long cr4;
>>> +union {
>>> +/* per physical cpu mapping */
>>> +struct {
>>> +struct vcpu *current_vcpu;
>>> +unsigned long per_cpu_offset;
>>> +unsigned long cr4;
>>> +};
>>> +/* per vcpu mapping (xpti) */
>>> +struct {
>>> +unsigned long pad1;
>>> +unsigned long pad2;
>>> +unsigned long stack_bottom_cpu;
>>> +};
>> 
>> In order to avoid accidental use in the wrong context as much as
>> possible, I think you want to name both structures.
> 
> I'd like to leave it as is in order to make a possible backport much
> more easier.

Well, I can see why you would want the pre-existing fields left
without structure field name, but the new (vcpu) ones? And
even the pre-existing (pcpu) ones should gain a name, just
perhaps in a patch late in the series, which then wouldn't be
backported.

Jan


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

Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries

2018-02-09 Thread Juergen Gross

On 30/01/18 16:40, Jan Beulich wrote:
 On 22.01.18 at 13:32,  wrote:
>> @@ -37,10 +52,24 @@ struct vcpu;
>>  
>>  struct cpu_info {
>>  struct cpu_user_regs guest_cpu_user_regs;
>> -unsigned int processor_id;
>> -struct vcpu *current_vcpu;
>> -unsigned long per_cpu_offset;
>> -unsigned long cr4;
>> +union {
>> +/* per physical cpu mapping */
>> +struct {
>> +struct vcpu *current_vcpu;
>> +unsigned long per_cpu_offset;
>> +unsigned long cr4;
>> +};
>> +/* per vcpu mapping (xpti) */
>> +struct {
>> +unsigned long pad1;
>> +unsigned long pad2;
>> +unsigned long stack_bottom_cpu;
>> +};
> 
> In order to avoid accidental use in the wrong context as much as
> possible, I think you want to name both structures.

I'd like to leave it as is in order to make a possible backport much
more easier.


Juergen


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

Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries

2018-01-30 Thread Juergen Gross

On 30/01/18 16:40, Jan Beulich wrote:
 On 22.01.18 at 13:32,  wrote:
>> In case of XPTI being active for a pv-domain allocate and initialize
>> per-vcpu stacks. The stacks are added to the per-domain mappings of
>> the pv-domain.
> 
> Considering the intended use of these stacks (as per the overview
> mail) I consider 32k per vCPU a non-negligible amount of extra memory
> use.

Maybe I can shrink this by putting multiple entry stacks into one page.
In the end I only need struct cpu_info and maybe some spare for each
stack.

> 
>> +static int pv_vcpu_init_xpti(struct vcpu *v)
>> +{
>> +struct domain *d = v->domain;
>> +struct page_info *pg;
>> +void *ptr;
>> +struct cpu_info *info;
>> +unsigned long stack_bottom;
>> +int rc;
>> +
>> +/* Populate page tables. */
>> +rc = create_perdomain_mapping(d, XPTI_START(v), STACK_PAGES,
>> +  NIL(l1_pgentry_t *), NULL);
>> +if ( rc )
>> +goto done;
>> +
>> +/* Map stacks. */
>> +rc = create_perdomain_mapping(d, XPTI_START(v), IST_MAX,
>> +  NULL, NIL(struct page_info *));
>> +if ( rc )
>> +goto done;
>> +
>> +ptr = alloc_xenheap_page();
>> +if ( !ptr )
>> +{
>> +rc = -ENOMEM;
>> +goto done;
>> +}
>> +clear_page(ptr);
>> +addmfn_to_perdomain_mapping(d, XPTI_START(v) + STACK_SIZE - PAGE_SIZE,
>> +_mfn(virt_to_mfn(ptr)));
> 
> This can't be create_perdomain_mapping() because of ...? If it's
> the Xen heap page you use here - that would be the next question:
> Does it need to be such, rather than a domheap one? I do see ...

I need to reference the user regs in __context_switch() before
switching to the new address space (otherwise I'd had to rework
__context_switch() which I wanted to avoid).

> 
>> +info = (struct cpu_info *)((unsigned long)ptr + PAGE_SIZE) - 1;
>> +info->flags = ON_VCPUSTACK;
>> +v->arch.pv_vcpu.stack_regs = >guest_cpu_user_regs;
> 
> ... this pointer, but without a clear picture on intended use it's
> hard to judge.

See patch 12.

> 
>> +/* Map TSS. */
>> +rc = create_perdomain_mapping(d, XPTI_TSS(v), 1, NULL, );
>> +if ( rc )
>> +goto done;
>> +info = (struct cpu_info *)(XPTI_START(v) + STACK_SIZE) - 1;
> 
> Iiuc this is a pointer one absolutely must not de-reference. A bit
> dangerous, I would say, the more that further up the same
> variable is being de-referenced.

Okay, I'll add another variable for this purpose.

> 
> Also I would assume the TSS can be mapped r/o.

Right.

> 
>> +stack_bottom = (unsigned long)>guest_cpu_user_regs.es;
>> +ptr = __map_domain_page(pg);
>> +tss_init(ptr, stack_bottom);
>> +unmap_domain_page(ptr);
>> +
>> +/* Map stub trampolines. */
>> +rc = create_perdomain_mapping(d, XPTI_TRAMPOLINE(v), 1, NULL, );
>> +if ( rc )
>> +goto done;
>> +ptr = __map_domain_page(pg);
>> +write_stub_trampoline((unsigned char *)ptr, XPTI_TRAMPOLINE(v),
> 
> I would be very surprised if you really needed the cast here.

Oh, this is a leftover from a previous version where ptr was char *.

> 
>> @@ -25,6 +25,21 @@
>>   */
>>  
>>  /*
>> + * The vcpu stacks used for XPTI are arranged similar to the physical cpu
>> + * stacks with some modifications. The main difference are the primary stack
>> + * size (only 1 page) and usage of the unused mappings for TSS and IDT.
>> + *
>> + * 7 - Primary stack (with a struct cpu_info at the top)
>> + * 6 - unused
>> + * 5 - TSS
> 
> Judging by the comment this might mean "TSS / IDT", or slots 4 or 6
> might be used for the IDT. Otoh I don't see any IDT related logic in
> pv_vcpu_init_xpti(). Please clarify this.

Oh yes. I'll remove the IDT related comments, as I think I can just map
the original IDT.

> 
>> @@ -37,10 +52,24 @@ struct vcpu;
>>  
>>  struct cpu_info {
>>  struct cpu_user_regs guest_cpu_user_regs;
>> -unsigned int processor_id;
>> -struct vcpu *current_vcpu;
>> -unsigned long per_cpu_offset;
>> -unsigned long cr4;
>> +union {
>> +/* per physical cpu mapping */
>> +struct {
>> +struct vcpu *current_vcpu;
>> +unsigned long per_cpu_offset;
>> +unsigned long cr4;
>> +};
>> +/* per vcpu mapping (xpti) */
>> +struct {
>> +unsigned long pad1;
>> +unsigned long pad2;
>> +unsigned long stack_bottom_cpu;
>> +};
> 
> In order to avoid accidental use in the wrong context as much as
> possible, I think you want to name both structures.

Okay.


Juergen

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

Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries

2018-01-30 Thread Jan Beulich

>>> On 22.01.18 at 13:32,  wrote:
> In case of XPTI being active for a pv-domain allocate and initialize
> per-vcpu stacks. The stacks are added to the per-domain mappings of
> the pv-domain.

Considering the intended use of these stacks (as per the overview
mail) I consider 32k per vCPU a non-negligible amount of extra memory
use.

> +static int pv_vcpu_init_xpti(struct vcpu *v)
> +{
> +struct domain *d = v->domain;
> +struct page_info *pg;
> +void *ptr;
> +struct cpu_info *info;
> +unsigned long stack_bottom;
> +int rc;
> +
> +/* Populate page tables. */
> +rc = create_perdomain_mapping(d, XPTI_START(v), STACK_PAGES,
> +  NIL(l1_pgentry_t *), NULL);
> +if ( rc )
> +goto done;
> +
> +/* Map stacks. */
> +rc = create_perdomain_mapping(d, XPTI_START(v), IST_MAX,
> +  NULL, NIL(struct page_info *));
> +if ( rc )
> +goto done;
> +
> +ptr = alloc_xenheap_page();
> +if ( !ptr )
> +{
> +rc = -ENOMEM;
> +goto done;
> +}
> +clear_page(ptr);
> +addmfn_to_perdomain_mapping(d, XPTI_START(v) + STACK_SIZE - PAGE_SIZE,
> +_mfn(virt_to_mfn(ptr)));

This can't be create_perdomain_mapping() because of ...? If it's
the Xen heap page you use here - that would be the next question:
Does it need to be such, rather than a domheap one? I do see ...

> +info = (struct cpu_info *)((unsigned long)ptr + PAGE_SIZE) - 1;
> +info->flags = ON_VCPUSTACK;
> +v->arch.pv_vcpu.stack_regs = >guest_cpu_user_regs;

... this pointer, but without a clear picture on intended use it's
hard to judge.

> +/* Map TSS. */
> +rc = create_perdomain_mapping(d, XPTI_TSS(v), 1, NULL, );
> +if ( rc )
> +goto done;
> +info = (struct cpu_info *)(XPTI_START(v) + STACK_SIZE) - 1;

Iiuc this is a pointer one absolutely must not de-reference. A bit
dangerous, I would say, the more that further up the same
variable is being de-referenced.

Also I would assume the TSS can be mapped r/o.

> +stack_bottom = (unsigned long)>guest_cpu_user_regs.es;
> +ptr = __map_domain_page(pg);
> +tss_init(ptr, stack_bottom);
> +unmap_domain_page(ptr);
> +
> +/* Map stub trampolines. */
> +rc = create_perdomain_mapping(d, XPTI_TRAMPOLINE(v), 1, NULL, );
> +if ( rc )
> +goto done;
> +ptr = __map_domain_page(pg);
> +write_stub_trampoline((unsigned char *)ptr, XPTI_TRAMPOLINE(v),

I would be very surprised if you really needed the cast here.

> @@ -25,6 +25,21 @@
>   */
>  
>  /*
> + * The vcpu stacks used for XPTI are arranged similar to the physical cpu
> + * stacks with some modifications. The main difference are the primary stack
> + * size (only 1 page) and usage of the unused mappings for TSS and IDT.
> + *
> + * 7 - Primary stack (with a struct cpu_info at the top)
> + * 6 - unused
> + * 5 - TSS

Judging by the comment this might mean "TSS / IDT", or slots 4 or 6
might be used for the IDT. Otoh I don't see any IDT related logic in
pv_vcpu_init_xpti(). Please clarify this.

> @@ -37,10 +52,24 @@ struct vcpu;
>  
>  struct cpu_info {
>  struct cpu_user_regs guest_cpu_user_regs;
> -unsigned int processor_id;
> -struct vcpu *current_vcpu;
> -unsigned long per_cpu_offset;
> -unsigned long cr4;
> +union {
> +/* per physical cpu mapping */
> +struct {
> +struct vcpu *current_vcpu;
> +unsigned long per_cpu_offset;
> +unsigned long cr4;
> +};
> +/* per vcpu mapping (xpti) */
> +struct {
> +unsigned long pad1;
> +unsigned long pad2;
> +unsigned long stack_bottom_cpu;
> +};

In order to avoid accidental use in the wrong context as much as
possible, I think you want to name both structures.

Jan


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

[Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries

2018-01-22 Thread Juergen Gross

In case of XPTI being active for a pv-domain allocate and initialize
per-vcpu stacks. The stacks are added to the per-domain mappings of
the pv-domain.

Signed-off-by: Juergen Gross 
---
 xen/arch/x86/pv/domain.c  | 72 +++
 xen/include/asm-x86/config.h  | 13 +++-
 xen/include/asm-x86/current.h | 39 ---
 xen/include/asm-x86/domain.h  |  3 ++
 4 files changed, 121 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 7d50f9bc19..834be96ed8 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -156,6 +156,75 @@ void pv_vcpu_destroy(struct vcpu *v)
 pv_destroy_gdt_ldt_l1tab(v);
 xfree(v->arch.pv_vcpu.trap_ctxt);
 v->arch.pv_vcpu.trap_ctxt = NULL;
+
+if ( v->domain->arch.pv_domain.xpti )
+{
+free_xenheap_page(v->arch.pv_vcpu.stack_regs);
+v->arch.pv_vcpu.stack_regs = NULL;
+destroy_perdomain_mapping(v->domain, XPTI_START(v), STACK_PAGES);
+}
+}
+
+static int pv_vcpu_init_xpti(struct vcpu *v)
+{
+struct domain *d = v->domain;
+struct page_info *pg;
+void *ptr;
+struct cpu_info *info;
+unsigned long stack_bottom;
+int rc;
+
+/* Populate page tables. */
+rc = create_perdomain_mapping(d, XPTI_START(v), STACK_PAGES,
+  NIL(l1_pgentry_t *), NULL);
+if ( rc )
+goto done;
+
+/* Map stacks. */
+rc = create_perdomain_mapping(d, XPTI_START(v), IST_MAX,
+  NULL, NIL(struct page_info *));
+if ( rc )
+goto done;
+
+ptr = alloc_xenheap_page();
+if ( !ptr )
+{
+rc = -ENOMEM;
+goto done;
+}
+clear_page(ptr);
+addmfn_to_perdomain_mapping(d, XPTI_START(v) + STACK_SIZE - PAGE_SIZE,
+_mfn(virt_to_mfn(ptr)));
+info = (struct cpu_info *)((unsigned long)ptr + PAGE_SIZE) - 1;
+info->flags = ON_VCPUSTACK;
+v->arch.pv_vcpu.stack_regs = >guest_cpu_user_regs;
+
+/* Map TSS. */
+rc = create_perdomain_mapping(d, XPTI_TSS(v), 1, NULL, );
+if ( rc )
+goto done;
+info = (struct cpu_info *)(XPTI_START(v) + STACK_SIZE) - 1;
+stack_bottom = (unsigned long)>guest_cpu_user_regs.es;
+ptr = __map_domain_page(pg);
+tss_init(ptr, stack_bottom);
+unmap_domain_page(ptr);
+
+/* Map stub trampolines. */
+rc = create_perdomain_mapping(d, XPTI_TRAMPOLINE(v), 1, NULL, );
+if ( rc )
+goto done;
+ptr = __map_domain_page(pg);
+write_stub_trampoline((unsigned char *)ptr, XPTI_TRAMPOLINE(v),
+  stack_bottom, (unsigned long)lstar_enter);
+write_stub_trampoline((unsigned char *)ptr + STUB_TRAMPOLINE_SIZE_PERVCPU,
+  XPTI_TRAMPOLINE(v) + STUB_TRAMPOLINE_SIZE_PERVCPU,
+  stack_bottom, (unsigned long)cstar_enter);
+unmap_domain_page(ptr);
+flipflags_perdomain_mapping(d, XPTI_TRAMPOLINE(v),
+_PAGE_NX | _PAGE_RW | _PAGE_DIRTY);
+
+ done:
+return rc;
 }
 
 int pv_vcpu_initialise(struct vcpu *v)
@@ -195,6 +264,9 @@ int pv_vcpu_initialise(struct vcpu *v)
 goto done;
 }
 
+if ( d->arch.pv_domain.xpti )
+rc = pv_vcpu_init_xpti(v);
+
  done:
 if ( rc )
 pv_vcpu_destroy(v);
diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
index 9ef9d03ca7..cb107255af 100644
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -66,6 +66,7 @@
 #endif
 
 #define STACK_ORDER 3
+#define STACK_PAGES (1 << STACK_ORDER)
 #define STACK_SIZE  (PAGE_SIZE << STACK_ORDER)
 
 #define TRAMPOLINE_STACK_SPACE  PAGE_SIZE
@@ -202,7 +203,7 @@ extern unsigned char boot_edid_info[128];
 /* Slot 260: per-domain mappings (including map cache). */
 #define PERDOMAIN_VIRT_START(PML4_ADDR(260))
 #define PERDOMAIN_SLOT_MBYTES   (PML4_ENTRY_BYTES >> (20 + PAGETABLE_ORDER))
-#define PERDOMAIN_SLOTS 3
+#define PERDOMAIN_SLOTS 4
 #define PERDOMAIN_VIRT_SLOT(s)  (PERDOMAIN_VIRT_START + (s) * \
  (PERDOMAIN_SLOT_MBYTES << 20))
 /* Slot 261: machine-to-phys conversion table (256GB). */
@@ -310,6 +311,16 @@ extern unsigned long xen_phys_start;
 #define ARG_XLAT_START(v)\
 (ARG_XLAT_VIRT_START + ((v)->vcpu_id << ARG_XLAT_VA_SHIFT))
 
+/* Per-vcpu XPTI pages. The fourth per-domain-mapping sub-area. */
+#define XPTI_VIRT_START  PERDOMAIN_VIRT_SLOT(3)
+#define XPTI_VA_SHIFT(PAGE_SHIFT + STACK_ORDER)
+#define XPTI_TRAMPOLINE_OFF  (IST_MAX << PAGE_SHIFT)
+#define XPTI_TSS_OFF ((IST_MAX + 2) << PAGE_SHIFT)
+#define XPTI_START(v)(XPTI_VIRT_START + \
+  ((v)->vcpu_id << XPTI_VA_SHIFT))
+#define XPTI_TRAMPOLINE(v)   (XPTI_START(v) + XPTI_TRAMPOLINE_OFF)
+#define XPTI_TSS(v)  (XPTI_START(v) + XPTI_TSS_OFF)
+

Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries

Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries

Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries

Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries

[Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries

5 matches

Site Navigation

Mail list logo

Footer information