* [PATCH for-next v2 00/10] x86: refactor x86/domain.c
@ 2017-04-25 13:52 Wei Liu
2017-04-25 13:52 ` [PATCH for-next v2 01/10] x86/mm: make free_perdomain_mappings idempotent Wei Liu
` (9 more replies)
0 siblings, 10 replies; 43+ messages in thread
From: Wei Liu @ 2017-04-25 13:52 UTC (permalink / raw)
To: Xen-devel; +Cc: George Dunlap, Andrew Cooper, Wei Liu, Jan Beulich, Tim Deegan
Can be found at
https://xenbits.xen.org/git-http/people/liuw/xen.git wip.x86-domain-v2
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Tim Deegan <tim@xen.org>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Wei Liu (10):
x86/mm: make free_perdomain_mappings idempotent
x86/domain: provide pv_{create,destroy}_gdt_ldt_l1tab and use them
x86/domain: make release_compact_l4 NULL tolerant
x86/domain: factor out pv_vcpu_destroy
x86/domain: factor out pv_vcpu_initialise
x86/domain: push some code down to hvm_domain_initialise
x86/domain: factor out pv_domain_destroy
x86/domain: factor out pv_domain_initialise
x86/domain: move PV specific code to pv/domain.c
x86/domain: move HVM specific code to hvm/domain.c
xen/arch/x86/domain.c | 508 ++--------------------------------------
xen/arch/x86/hvm/Makefile | 1 +
xen/arch/x86/hvm/domain.c | 326 ++++++++++++++++++++++++++
xen/arch/x86/hvm/hvm.c | 11 +-
xen/arch/x86/mm.c | 8 +-
xen/arch/x86/pv/Makefile | 1 +
xen/arch/x86/pv/domain.c | 232 ++++++++++++++++++
xen/include/asm-x86/hvm/hvm.h | 3 +-
xen/include/asm-x86/pv/domain.h | 30 +++
9 files changed, 633 insertions(+), 487 deletions(-)
create mode 100644 xen/arch/x86/hvm/domain.c
create mode 100644 xen/arch/x86/pv/domain.c
create mode 100644 xen/include/asm-x86/pv/domain.h
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH for-next v2 01/10] x86/mm: make free_perdomain_mappings idempotent
2017-04-25 13:52 [PATCH for-next v2 00/10] x86: refactor x86/domain.c Wei Liu
@ 2017-04-25 13:52 ` Wei Liu
2017-04-25 13:54 ` Andrew Cooper
2017-04-25 13:52 ` [PATCH for-next v2 02/10] x86/domain: provide pv_{create, destroy}_gdt_ldt_l1tab and use them Wei Liu
` (8 subsequent siblings)
9 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2017-04-25 13:52 UTC (permalink / raw)
To: Xen-devel; +Cc: George Dunlap, Tim Deegan, Wei Liu, Jan Beulich, Andrew Cooper
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Tim Deegan <tim@xen.org>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/mm.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index bd68e56dc7..cdef5c49c4 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -6503,9 +6503,14 @@ void destroy_perdomain_mapping(struct domain *d, unsigned long va,
void free_perdomain_mappings(struct domain *d)
{
- l3_pgentry_t *l3tab = __map_domain_page(d->arch.perdomain_l3_pg);
+ l3_pgentry_t *l3tab;
unsigned int i;
+ if ( !d->arch.perdomain_l3_pg )
+ return;
+
+ l3tab = __map_domain_page(d->arch.perdomain_l3_pg);
+
for ( i = 0; i < PERDOMAIN_SLOTS; ++i)
if ( l3e_get_flags(l3tab[i]) & _PAGE_PRESENT )
{
@@ -6544,6 +6549,7 @@ void free_perdomain_mappings(struct domain *d)
unmap_domain_page(l3tab);
free_domheap_page(d->arch.perdomain_l3_pg);
+ d->arch.perdomain_l3_pg = NULL;
}
#ifdef MEMORY_GUARD
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH for-next v2 02/10] x86/domain: provide pv_{create, destroy}_gdt_ldt_l1tab and use them
2017-04-25 13:52 [PATCH for-next v2 00/10] x86: refactor x86/domain.c Wei Liu
2017-04-25 13:52 ` [PATCH for-next v2 01/10] x86/mm: make free_perdomain_mappings idempotent Wei Liu
@ 2017-04-25 13:52 ` Wei Liu
2017-04-25 13:56 ` Andrew Cooper
2017-04-25 13:52 ` [PATCH for-next v2 03/10] x86/domain: make release_compact_l4 NULL tolerant Wei Liu
` (7 subsequent siblings)
9 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2017-04-25 13:52 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
This patch encapsulates the perdomain creation and destruction into
helper functions and use them where appropriate.
Since destroy_perdomain_mapping is idempotent, it is safe to call the
destruction function multiple times.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/domain.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 90e2b1f82a..1f76d034a7 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -387,6 +387,18 @@ int switch_compat(struct domain *d)
return rc;
}
+static int pv_create_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
+{
+ return create_perdomain_mapping(d, GDT_VIRT_START(v),
+ 1 << GDT_LDT_VCPU_SHIFT,
+ d->arch.pv_domain.gdt_ldt_l1tab, NULL);
+}
+
+static void pv_destroy_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
+{
+ destroy_perdomain_mapping(d, GDT_VIRT_START(v), 1 << GDT_LDT_VCPU_SHIFT);
+}
+
int vcpu_initialise(struct vcpu *v)
{
struct domain *d = v->domain;
@@ -423,9 +435,7 @@ int vcpu_initialise(struct vcpu *v)
if ( !is_idle_domain(d) )
{
- rc = create_perdomain_mapping(d, GDT_VIRT_START(v),
- 1 << GDT_LDT_VCPU_SHIFT,
- d->arch.pv_domain.gdt_ldt_l1tab, NULL);
+ rc = pv_create_gdt_ldt_l1tab(d, v);
if ( rc )
goto done;
@@ -435,6 +445,7 @@ int vcpu_initialise(struct vcpu *v)
NR_VECTORS);
if ( !v->arch.pv_vcpu.trap_ctxt )
{
+ pv_destroy_gdt_ldt_l1tab(d, v);
rc = -ENOMEM;
goto done;
}
@@ -464,7 +475,10 @@ int vcpu_initialise(struct vcpu *v)
vcpu_destroy_fpu(v);
if ( is_pv_domain(d) )
+ {
+ pv_destroy_gdt_ldt_l1tab(d, v);
xfree(v->arch.pv_vcpu.trap_ctxt);
+ }
}
else if ( !is_idle_domain(v->domain) )
vpmu_initialise(v);
@@ -491,7 +505,10 @@ void vcpu_destroy(struct vcpu *v)
if ( is_hvm_vcpu(v) )
hvm_vcpu_destroy(v);
else
+ {
+ pv_destroy_gdt_ldt_l1tab(v->domain, v);
xfree(v->arch.pv_vcpu.trap_ctxt);
+ }
}
static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH for-next v2 03/10] x86/domain: make release_compact_l4 NULL tolerant
2017-04-25 13:52 [PATCH for-next v2 00/10] x86: refactor x86/domain.c Wei Liu
2017-04-25 13:52 ` [PATCH for-next v2 01/10] x86/mm: make free_perdomain_mappings idempotent Wei Liu
2017-04-25 13:52 ` [PATCH for-next v2 02/10] x86/domain: provide pv_{create, destroy}_gdt_ldt_l1tab and use them Wei Liu
@ 2017-04-25 13:52 ` Wei Liu
2017-04-25 13:56 ` Andrew Cooper
2017-04-25 13:52 ` [PATCH for-next v2 04/10] x86/domain: factor out pv_vcpu_destroy Wei Liu
` (6 subsequent siblings)
9 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2017-04-25 13:52 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
Push the check in caller down to that function so that it becomes
idempotent.
No functional change.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/domain.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 1f76d034a7..b3d65b1f82 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -338,7 +338,8 @@ static int setup_compat_l4(struct vcpu *v)
static void release_compat_l4(struct vcpu *v)
{
- free_domheap_page(pagetable_get_page(v->arch.guest_table));
+ if ( !pagetable_is_null(v->arch.guest_table) )
+ free_domheap_page(pagetable_get_page(v->arch.guest_table));
v->arch.guest_table = pagetable_null();
v->arch.guest_table_user = pagetable_null();
}
@@ -379,9 +380,7 @@ int switch_compat(struct domain *d)
for_each_vcpu( d, v )
{
free_compat_arg_xlat(v);
-
- if ( !pagetable_is_null(v->arch.guest_table) )
- release_compat_l4(v);
+ release_compat_l4(v);
}
return rc;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH for-next v2 04/10] x86/domain: factor out pv_vcpu_destroy
2017-04-25 13:52 [PATCH for-next v2 00/10] x86: refactor x86/domain.c Wei Liu
` (2 preceding siblings ...)
2017-04-25 13:52 ` [PATCH for-next v2 03/10] x86/domain: make release_compact_l4 NULL tolerant Wei Liu
@ 2017-04-25 13:52 ` Wei Liu
2017-04-25 13:58 ` Andrew Cooper
2017-04-26 12:21 ` Jan Beulich
2017-04-25 13:52 ` [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise Wei Liu
` (5 subsequent siblings)
9 siblings, 2 replies; 43+ messages in thread
From: Wei Liu @ 2017-04-25 13:52 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
The function is made idempotent on purpose.
No functional change.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/domain.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index b3d65b1f82..cde0917f5b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -485,17 +485,24 @@ int vcpu_initialise(struct vcpu *v)
return rc;
}
-void vcpu_destroy(struct vcpu *v)
+static void pv_vcpu_destroy(struct vcpu *v)
{
- xfree(v->arch.vm_event);
- v->arch.vm_event = NULL;
-
if ( is_pv_32bit_vcpu(v) )
{
free_compat_arg_xlat(v);
release_compat_l4(v);
}
+ pv_destroy_gdt_ldt_l1tab(v->domain, v);
+ xfree(v->arch.pv_vcpu.trap_ctxt);
+ v->arch.pv_vcpu.trap_ctxt = NULL;
+}
+
+void vcpu_destroy(struct vcpu *v)
+{
+ xfree(v->arch.vm_event);
+ v->arch.vm_event = NULL;
+
vcpu_destroy_fpu(v);
if ( !is_idle_domain(v->domain) )
@@ -504,10 +511,7 @@ void vcpu_destroy(struct vcpu *v)
if ( is_hvm_vcpu(v) )
hvm_vcpu_destroy(v);
else
- {
- pv_destroy_gdt_ldt_l1tab(v->domain, v);
- xfree(v->arch.pv_vcpu.trap_ctxt);
- }
+ pv_vcpu_destroy(v);
}
static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise
2017-04-25 13:52 [PATCH for-next v2 00/10] x86: refactor x86/domain.c Wei Liu
` (3 preceding siblings ...)
2017-04-25 13:52 ` [PATCH for-next v2 04/10] x86/domain: factor out pv_vcpu_destroy Wei Liu
@ 2017-04-25 13:52 ` Wei Liu
2017-04-25 14:08 ` Andrew Cooper
2017-04-26 12:25 ` Jan Beulich
2017-04-25 13:52 ` [PATCH for-next v2 06/10] x86/domain: push some code down to hvm_domain_initialise Wei Liu
` (4 subsequent siblings)
9 siblings, 2 replies; 43+ messages in thread
From: Wei Liu @ 2017-04-25 13:52 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
Move PV specific vcpu initialisation code to said function, but leave
the only line needed by idle domain in vcpu_initialise.
Use pv_vcpu_destroy in error path to simplify code. It is safe to do so
because the destruction function accepts partially initialised vcpu
struct.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/domain.c | 99 ++++++++++++++++++++++++++-------------------------
1 file changed, 50 insertions(+), 49 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index cde0917f5b..38fc4f5d8b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -398,6 +398,50 @@ static void pv_destroy_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
destroy_perdomain_mapping(d, GDT_VIRT_START(v), 1 << GDT_LDT_VCPU_SHIFT);
}
+static void pv_vcpu_destroy(struct vcpu *v);
+static int pv_vcpu_initialise(struct vcpu *v)
+{
+ struct domain *d = v->domain;
+ int rc;
+
+ ASSERT(!is_idle_domain(d));
+
+ spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
+
+ rc = pv_create_gdt_ldt_l1tab(d, v);
+ if ( rc )
+ goto done;
+
+ BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
+ PAGE_SIZE);
+ v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
+ NR_VECTORS);
+ if ( !v->arch.pv_vcpu.trap_ctxt )
+ {
+ rc = -ENOMEM;
+ goto done;
+ }
+
+ /* PV guests by default have a 100Hz ticker. */
+ v->periodic_period = MILLISECS(10);
+
+ v->arch.pv_vcpu.ctrlreg[4] = real_cr4_to_pv_guest_cr4(mmu_cr4_features);
+
+ if ( is_pv_32bit_domain(d) )
+ {
+ if ( (rc = setup_compat_arg_xlat(v)) )
+ goto done;
+
+ if ( (rc = setup_compat_l4(v)) )
+ goto done;
+ }
+
+ done:
+ if ( rc )
+ pv_vcpu_destroy(v);
+ return rc;
+}
+
int vcpu_initialise(struct vcpu *v)
{
struct domain *d = v->domain;
@@ -424,61 +468,18 @@ int vcpu_initialise(struct vcpu *v)
spin_lock_init(&v->arch.vpmu.vpmu_lock);
if ( is_hvm_domain(d) )
- {
rc = hvm_vcpu_initialise(v);
- goto done;
- }
-
-
- spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
-
- if ( !is_idle_domain(d) )
- {
- rc = pv_create_gdt_ldt_l1tab(d, v);
- if ( rc )
- goto done;
-
- BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
- PAGE_SIZE);
- v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
- NR_VECTORS);
- if ( !v->arch.pv_vcpu.trap_ctxt )
- {
- pv_destroy_gdt_ldt_l1tab(d, v);
- rc = -ENOMEM;
- goto done;
- }
-
- /* PV guests by default have a 100Hz ticker. */
- v->periodic_period = MILLISECS(10);
- }
+ else if ( is_pv_domain(d) && !is_idle_domain(d) )
+ rc = pv_vcpu_initialise(v);
else
- v->arch.cr3 = __pa(idle_pg_table);
-
- v->arch.pv_vcpu.ctrlreg[4] = real_cr4_to_pv_guest_cr4(mmu_cr4_features);
-
- if ( is_pv_32bit_domain(d) )
{
- if ( (rc = setup_compat_arg_xlat(v)) )
- goto done;
-
- if ( (rc = setup_compat_l4(v)) )
- {
- free_compat_arg_xlat(v);
- goto done;
- }
+ /* Idle domain */
+ v->arch.cr3 = __pa(idle_pg_table);
+ rc = 0;
}
- done:
+
if ( rc )
- {
vcpu_destroy_fpu(v);
-
- if ( is_pv_domain(d) )
- {
- pv_destroy_gdt_ldt_l1tab(d, v);
- xfree(v->arch.pv_vcpu.trap_ctxt);
- }
- }
else if ( !is_idle_domain(v->domain) )
vpmu_initialise(v);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH for-next v2 06/10] x86/domain: push some code down to hvm_domain_initialise
2017-04-25 13:52 [PATCH for-next v2 00/10] x86: refactor x86/domain.c Wei Liu
` (4 preceding siblings ...)
2017-04-25 13:52 ` [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise Wei Liu
@ 2017-04-25 13:52 ` Wei Liu
2017-04-25 14:15 ` Andrew Cooper
2017-04-25 13:52 ` [PATCH for-next v2 07/10] x86/domain: factor out pv_domain_destroy Wei Liu
` (3 subsequent siblings)
9 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2017-04-25 13:52 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
We want to have a single entry point to initialise hvm guest. Now the
timing to set hap bit and create per domain mapping is deferred, but
that's not a problem.
No functional change.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v2:
1. reorder things to avoid rename labels
2. add config to hvm_domain_initialise
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/domain.c | 14 +++-----------
xen/arch/x86/hvm/hvm.c | 11 ++++++++++-
xen/include/asm-x86/hvm/hvm.h | 3 ++-
3 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 38fc4f5d8b..fd5d47ab3d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -596,16 +596,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
d->arch.emulation_flags = emflags;
}
- if ( is_hvm_domain(d) )
- {
- d->arch.hvm_domain.hap_enabled =
- hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);
-
- rc = create_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0, NULL, NULL);
- }
- else if ( is_idle_domain(d) )
- rc = 0;
- else
+ rc = 0; /* HVM and idle domain */
+ if ( is_pv_domain(d) )
{
d->arch.pv_domain.gdt_ldt_l1tab =
alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
@@ -672,7 +664,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
if ( is_hvm_domain(d) )
{
- if ( (rc = hvm_domain_initialise(d)) != 0 )
+ if ( (rc = hvm_domain_initialise(d, domcr_flags, config)) != 0 )
goto fail;
}
else
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a441955322..9e9f1dbd53 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -595,7 +595,8 @@ static int hvm_print_line(
return X86EMUL_OKAY;
}
-int hvm_domain_initialise(struct domain *d)
+int hvm_domain_initialise(struct domain *d, unsigned long domcr_flags,
+ struct xen_arch_domainconfig *config)
{
unsigned int nr_gsis;
int rc;
@@ -613,6 +614,12 @@ int hvm_domain_initialise(struct domain *d)
INIT_LIST_HEAD(&d->arch.hvm_domain.write_map.list);
INIT_LIST_HEAD(&d->arch.hvm_domain.g2m_ioport_list);
+ d->arch.hvm_domain.hap_enabled =
+ hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);
+ rc = create_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0, NULL, NULL);
+ if ( rc )
+ goto fail;
+
hvm_init_cacheattr_region_list(d);
rc = paging_enable(d, PG_refcounts|PG_translate|PG_external);
@@ -696,6 +703,8 @@ int hvm_domain_initialise(struct domain *d)
xfree(d->arch.hvm_domain.irq);
fail0:
hvm_destroy_cacheattr_region_list(d);
+ destroy_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0);
+ fail:
return rc;
}
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 7a85b2e3b5..b687e03dce 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -236,7 +236,8 @@ extern s8 hvm_port80_allowed;
extern const struct hvm_function_table *start_svm(void);
extern const struct hvm_function_table *start_vmx(void);
-int hvm_domain_initialise(struct domain *d);
+int hvm_domain_initialise(struct domain *d, unsigned long domcr_flags,
+ struct xen_arch_domainconfig *config);
void hvm_domain_relinquish_resources(struct domain *d);
void hvm_domain_destroy(struct domain *d);
void hvm_domain_soft_reset(struct domain *d);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH for-next v2 07/10] x86/domain: factor out pv_domain_destroy
2017-04-25 13:52 [PATCH for-next v2 00/10] x86: refactor x86/domain.c Wei Liu
` (5 preceding siblings ...)
2017-04-25 13:52 ` [PATCH for-next v2 06/10] x86/domain: push some code down to hvm_domain_initialise Wei Liu
@ 2017-04-25 13:52 ` Wei Liu
2017-04-25 14:18 ` Andrew Cooper
2017-04-26 12:32 ` Jan Beulich
2017-04-25 13:52 ` [PATCH for-next v2 08/10] x86/domain: factor out pv_domain_initialise Wei Liu
` (2 subsequent siblings)
9 siblings, 2 replies; 43+ messages in thread
From: Wei Liu @ 2017-04-25 13:52 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
No functional change.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v2:
1. reset fields
2. destroy perdomain mapping
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/domain.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index fd5d47ab3d..952e05366a 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -536,6 +536,16 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
return true;
}
+static void pv_domain_destroy(struct domain *d)
+{
+ destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
+ GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
+ xfree(d->arch.pv_domain.cpuidmasks);
+ d->arch.pv_domain.cpuidmasks = NULL;
+ free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
+ d->arch.pv_domain.gdt_ldt_l1tab = NULL;
+}
+
int arch_domain_create(struct domain *d, unsigned int domcr_flags,
struct xen_arch_domainconfig *config)
{
@@ -712,10 +722,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
paging_final_teardown(d);
free_perdomain_mappings(d);
if ( is_pv_domain(d) )
- {
- xfree(d->arch.pv_domain.cpuidmasks);
- free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
- }
+ pv_domain_destroy(d);
+
return rc;
}
@@ -735,10 +743,7 @@ void arch_domain_destroy(struct domain *d)
free_perdomain_mappings(d);
if ( is_pv_domain(d) )
- {
- free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
- xfree(d->arch.pv_domain.cpuidmasks);
- }
+ pv_domain_destroy(d);
free_xenheap_page(d->shared_info);
cleanup_domain_irq_mapping(d);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH for-next v2 08/10] x86/domain: factor out pv_domain_initialise
2017-04-25 13:52 [PATCH for-next v2 00/10] x86: refactor x86/domain.c Wei Liu
` (6 preceding siblings ...)
2017-04-25 13:52 ` [PATCH for-next v2 07/10] x86/domain: factor out pv_domain_destroy Wei Liu
@ 2017-04-25 13:52 ` Wei Liu
2017-04-25 14:30 ` Andrew Cooper
2017-04-25 13:52 ` [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c Wei Liu
2017-04-25 13:52 ` [PATCH for-next v2 10/10] x86/domain: move HVM specific code to hvm/domain.c Wei Liu
9 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2017-04-25 13:52 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
Lump everything PV related in arch_domain_create into
pv_domain_initialise.
Though domcr_flags and config are not necessary, the new function is
given those to match hvm counterpart.
Since it cleans up after itself there is no need to clean up in
arch_domain_create in case of failure. Remove the initialiser of rc in
arch_domain_create.
No functional change.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/domain.c | 89 +++++++++++++++++++++++++++++----------------------
1 file changed, 51 insertions(+), 38 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 952e05366a..f5e917431d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -546,11 +546,54 @@ static void pv_domain_destroy(struct domain *d)
d->arch.pv_domain.gdt_ldt_l1tab = NULL;
}
+static int pv_domain_initialise(struct domain *d, unsigned int domcr_flags,
+ struct xen_arch_domainconfig *config)
+{
+ static const struct arch_csw pv_csw = {
+ .from = paravirt_ctxt_switch_from,
+ .to = paravirt_ctxt_switch_to,
+ .tail = continue_nonidle_domain,
+ };
+ int rc = -ENOMEM;
+
+ d->arch.pv_domain.gdt_ldt_l1tab =
+ alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
+ if ( !d->arch.pv_domain.gdt_ldt_l1tab )
+ goto fail;
+ clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
+
+ if ( levelling_caps & ~LCAP_faulting )
+ {
+ d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
+ if ( !d->arch.pv_domain.cpuidmasks )
+ goto fail;
+ *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
+ }
+
+ rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
+ GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
+ NULL, NULL);
+ if ( rc )
+ goto fail;
+
+ d->arch.ctxt_switch = &pv_csw;
+
+ /* 64-bit PV guest by default. */
+ d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+
+ return 0;
+
+ fail:
+ pv_domain_destroy(d);
+
+ return rc;
+}
+
int arch_domain_create(struct domain *d, unsigned int domcr_flags,
struct xen_arch_domainconfig *config)
{
bool paging_initialised = false;
- int rc = -ENOMEM;
+ int rc;
if ( config == NULL && !is_idle_domain(d) )
return -EINVAL;
@@ -606,30 +649,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
d->arch.emulation_flags = emflags;
}
- rc = 0; /* HVM and idle domain */
- if ( is_pv_domain(d) )
- {
- d->arch.pv_domain.gdt_ldt_l1tab =
- alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
- if ( !d->arch.pv_domain.gdt_ldt_l1tab )
- goto fail;
- clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
-
- if ( levelling_caps & ~LCAP_faulting )
- {
- d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
- if ( !d->arch.pv_domain.cpuidmasks )
- goto fail;
- *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
- }
-
- rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
- GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
- NULL, NULL);
- }
- if ( rc )
- goto fail;
-
mapcache_domain_init(d);
HYPERVISOR_COMPAT_VIRT_START(d) =
@@ -677,23 +696,20 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
if ( (rc = hvm_domain_initialise(d, domcr_flags, config)) != 0 )
goto fail;
}
- else
+ else if ( is_idle_domain(d) )
{
- static const struct arch_csw pv_csw = {
- .from = paravirt_ctxt_switch_from,
- .to = paravirt_ctxt_switch_to,
- .tail = continue_nonidle_domain,
- };
static const struct arch_csw idle_csw = {
.from = paravirt_ctxt_switch_from,
.to = paravirt_ctxt_switch_to,
.tail = continue_idle_domain,
};
- d->arch.ctxt_switch = is_idle_domain(d) ? &idle_csw : &pv_csw;
-
- /* 64-bit PV guest by default. */
- d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+ d->arch.ctxt_switch = &idle_csw;
+ }
+ else
+ {
+ if ( (rc = pv_domain_initialise(d, domcr_flags, config)) != 0 )
+ goto fail;
}
/* initialize default tsc behavior in case tools don't */
@@ -721,9 +737,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
if ( paging_initialised )
paging_final_teardown(d);
free_perdomain_mappings(d);
- if ( is_pv_domain(d) )
- pv_domain_destroy(d);
-
return rc;
}
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c
2017-04-25 13:52 [PATCH for-next v2 00/10] x86: refactor x86/domain.c Wei Liu
` (7 preceding siblings ...)
2017-04-25 13:52 ` [PATCH for-next v2 08/10] x86/domain: factor out pv_domain_initialise Wei Liu
@ 2017-04-25 13:52 ` Wei Liu
2017-04-25 14:52 ` Andrew Cooper
2017-04-25 13:52 ` [PATCH for-next v2 10/10] x86/domain: move HVM specific code to hvm/domain.c Wei Liu
9 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2017-04-25 13:52 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
Move all the PV specific code along with the supporting code to
pv/domain.c.
This in turn requires exporting a few functions in header files. Create
pv/domain.h for that.
No functional change.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/domain.c | 214 ++----------------------------------
xen/arch/x86/pv/Makefile | 1 +
xen/arch/x86/pv/domain.c | 232 ++++++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/pv/domain.h | 30 ++++++
4 files changed, 270 insertions(+), 207 deletions(-)
create mode 100644 xen/arch/x86/pv/domain.c
create mode 100644 xen/include/asm-x86/pv/domain.h
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index f5e917431d..7c83fec8fc 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -63,6 +63,7 @@
#include <xen/iommu.h>
#include <compat/vcpu.h>
#include <asm/psr.h>
+#include <asm/pv/domain.h>
DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
@@ -70,9 +71,6 @@ static void default_idle(void);
void (*pm_idle) (void) __read_mostly = default_idle;
void (*dead_idle) (void) __read_mostly = default_dead_idle;
-static void paravirt_ctxt_switch_from(struct vcpu *v);
-static void paravirt_ctxt_switch_to(struct vcpu *v);
-
static void default_idle(void)
{
local_irq_disable();
@@ -145,13 +143,6 @@ static void noreturn continue_idle_domain(struct vcpu *v)
reset_stack_and_jump(idle_loop);
}
-static void noreturn continue_nonidle_domain(struct vcpu *v)
-{
- check_wakeup_from_wait();
- mark_regs_dirty(guest_cpu_user_regs());
- reset_stack_and_jump(ret_from_intr);
-}
-
void dump_pageframe_info(struct domain *d)
{
struct page_info *page;
@@ -313,135 +304,6 @@ void free_vcpu_struct(struct vcpu *v)
free_xenheap_page(v);
}
-static int setup_compat_l4(struct vcpu *v)
-{
- struct page_info *pg;
- l4_pgentry_t *l4tab;
-
- pg = alloc_domheap_page(v->domain, MEMF_no_owner);
- if ( pg == NULL )
- return -ENOMEM;
-
- /* This page needs to look like a pagetable so that it can be shadowed */
- pg->u.inuse.type_info = PGT_l4_page_table|PGT_validated|1;
-
- l4tab = __map_domain_page(pg);
- clear_page(l4tab);
- init_guest_l4_table(l4tab, v->domain, 1);
- unmap_domain_page(l4tab);
-
- v->arch.guest_table = pagetable_from_page(pg);
- v->arch.guest_table_user = v->arch.guest_table;
-
- return 0;
-}
-
-static void release_compat_l4(struct vcpu *v)
-{
- if ( !pagetable_is_null(v->arch.guest_table) )
- free_domheap_page(pagetable_get_page(v->arch.guest_table));
- v->arch.guest_table = pagetable_null();
- v->arch.guest_table_user = pagetable_null();
-}
-
-int switch_compat(struct domain *d)
-{
- struct vcpu *v;
- int rc;
-
- if ( is_hvm_domain(d) || d->tot_pages != 0 )
- return -EACCES;
- if ( is_pv_32bit_domain(d) )
- return 0;
-
- d->arch.has_32bit_shinfo = 1;
- if ( is_pv_domain(d) )
- d->arch.is_32bit_pv = 1;
-
- for_each_vcpu( d, v )
- {
- rc = setup_compat_arg_xlat(v);
- if ( !rc )
- rc = setup_compat_l4(v);
-
- if ( rc )
- goto undo_and_fail;
- }
-
- domain_set_alloc_bitsize(d);
- recalculate_cpuid_policy(d);
-
- d->arch.x87_fip_width = 4;
-
- return 0;
-
- undo_and_fail:
- d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
- for_each_vcpu( d, v )
- {
- free_compat_arg_xlat(v);
- release_compat_l4(v);
- }
-
- return rc;
-}
-
-static int pv_create_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
-{
- return create_perdomain_mapping(d, GDT_VIRT_START(v),
- 1 << GDT_LDT_VCPU_SHIFT,
- d->arch.pv_domain.gdt_ldt_l1tab, NULL);
-}
-
-static void pv_destroy_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
-{
- destroy_perdomain_mapping(d, GDT_VIRT_START(v), 1 << GDT_LDT_VCPU_SHIFT);
-}
-
-static void pv_vcpu_destroy(struct vcpu *v);
-static int pv_vcpu_initialise(struct vcpu *v)
-{
- struct domain *d = v->domain;
- int rc;
-
- ASSERT(!is_idle_domain(d));
-
- spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
-
- rc = pv_create_gdt_ldt_l1tab(d, v);
- if ( rc )
- goto done;
-
- BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
- PAGE_SIZE);
- v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
- NR_VECTORS);
- if ( !v->arch.pv_vcpu.trap_ctxt )
- {
- rc = -ENOMEM;
- goto done;
- }
-
- /* PV guests by default have a 100Hz ticker. */
- v->periodic_period = MILLISECS(10);
-
- v->arch.pv_vcpu.ctrlreg[4] = real_cr4_to_pv_guest_cr4(mmu_cr4_features);
-
- if ( is_pv_32bit_domain(d) )
- {
- if ( (rc = setup_compat_arg_xlat(v)) )
- goto done;
-
- if ( (rc = setup_compat_l4(v)) )
- goto done;
- }
-
- done:
- if ( rc )
- pv_vcpu_destroy(v);
- return rc;
-}
-
int vcpu_initialise(struct vcpu *v)
{
struct domain *d = v->domain;
@@ -486,19 +348,6 @@ int vcpu_initialise(struct vcpu *v)
return rc;
}
-static void pv_vcpu_destroy(struct vcpu *v)
-{
- if ( is_pv_32bit_vcpu(v) )
- {
- free_compat_arg_xlat(v);
- release_compat_l4(v);
- }
-
- pv_destroy_gdt_ldt_l1tab(v->domain, v);
- xfree(v->arch.pv_vcpu.trap_ctxt);
- v->arch.pv_vcpu.trap_ctxt = NULL;
-}
-
void vcpu_destroy(struct vcpu *v)
{
xfree(v->arch.vm_event);
@@ -536,59 +385,8 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
return true;
}
-static void pv_domain_destroy(struct domain *d)
-{
- destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
- GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
- xfree(d->arch.pv_domain.cpuidmasks);
- d->arch.pv_domain.cpuidmasks = NULL;
- free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
- d->arch.pv_domain.gdt_ldt_l1tab = NULL;
-}
-
-static int pv_domain_initialise(struct domain *d, unsigned int domcr_flags,
- struct xen_arch_domainconfig *config)
-{
- static const struct arch_csw pv_csw = {
- .from = paravirt_ctxt_switch_from,
- .to = paravirt_ctxt_switch_to,
- .tail = continue_nonidle_domain,
- };
- int rc = -ENOMEM;
-
- d->arch.pv_domain.gdt_ldt_l1tab =
- alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
- if ( !d->arch.pv_domain.gdt_ldt_l1tab )
- goto fail;
- clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
-
- if ( levelling_caps & ~LCAP_faulting )
- {
- d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
- if ( !d->arch.pv_domain.cpuidmasks )
- goto fail;
- *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
- }
-
- rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
- GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
- NULL, NULL);
- if ( rc )
- goto fail;
-
- d->arch.ctxt_switch = &pv_csw;
-
- /* 64-bit PV guest by default. */
- d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
-
- return 0;
-
- fail:
- pv_domain_destroy(d);
-
- return rc;
-}
-
+void paravirt_ctxt_switch_from(struct vcpu *v);
+void paravirt_ctxt_switch_to(struct vcpu *v);
int arch_domain_create(struct domain *d, unsigned int domcr_flags,
struct xen_arch_domainconfig *config)
{
@@ -1919,7 +1717,8 @@ static void save_segments(struct vcpu *v)
#define switch_kernel_stack(v) ((void)0)
-static void paravirt_ctxt_switch_from(struct vcpu *v)
+/* Needed by PV guests */
+void paravirt_ctxt_switch_from(struct vcpu *v)
{
save_segments(v);
@@ -1933,7 +1732,8 @@ static void paravirt_ctxt_switch_from(struct vcpu *v)
write_debugreg(7, 0);
}
-static void paravirt_ctxt_switch_to(struct vcpu *v)
+/* Needed by PV guests */
+void paravirt_ctxt_switch_to(struct vcpu *v)
{
unsigned long cr4;
diff --git a/xen/arch/x86/pv/Makefile b/xen/arch/x86/pv/Makefile
index ea94599438..2737824e81 100644
--- a/xen/arch/x86/pv/Makefile
+++ b/xen/arch/x86/pv/Makefile
@@ -1,2 +1,3 @@
obj-y += hypercall.o
obj-bin-y += dom0_build.init.o
+obj-y += domain.o
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
new file mode 100644
index 0000000000..562c3d03f5
--- /dev/null
+++ b/xen/arch/x86/pv/domain.c
@@ -0,0 +1,232 @@
+/******************************************************************************
+ * arch/x86/pv/domain.c
+ *
+ * PV domain handling
+ */
+
+/*
+ * Copyright (C) 1995 Linus Torvalds
+ *
+ * Pentium III FXSR, SSE support
+ * Gareth Hughes <gareth@valinux.com>, May 2000
+ */
+
+#include <xen/domain_page.h>
+#include <xen/errno.h>
+#include <xen/lib.h>
+#include <xen/sched.h>
+
+static void noreturn continue_nonidle_domain(struct vcpu *v)
+{
+ check_wakeup_from_wait();
+ mark_regs_dirty(guest_cpu_user_regs());
+ reset_stack_and_jump(ret_from_intr);
+}
+
+static int setup_compat_l4(struct vcpu *v)
+{
+ struct page_info *pg;
+ l4_pgentry_t *l4tab;
+
+ pg = alloc_domheap_page(v->domain, MEMF_no_owner);
+ if ( pg == NULL )
+ return -ENOMEM;
+
+ /* This page needs to look like a pagetable so that it can be shadowed */
+ pg->u.inuse.type_info = PGT_l4_page_table|PGT_validated|1;
+
+ l4tab = __map_domain_page(pg);
+ clear_page(l4tab);
+ init_guest_l4_table(l4tab, v->domain, 1);
+ unmap_domain_page(l4tab);
+
+ v->arch.guest_table = pagetable_from_page(pg);
+ v->arch.guest_table_user = v->arch.guest_table;
+
+ return 0;
+}
+
+static void release_compat_l4(struct vcpu *v)
+{
+ if ( !pagetable_is_null(v->arch.guest_table) )
+ free_domheap_page(pagetable_get_page(v->arch.guest_table));
+ v->arch.guest_table = pagetable_null();
+ v->arch.guest_table_user = pagetable_null();
+}
+
+int switch_compat(struct domain *d)
+{
+ struct vcpu *v;
+ int rc;
+
+ if ( is_hvm_domain(d) || d->tot_pages != 0 )
+ return -EACCES;
+ if ( is_pv_32bit_domain(d) )
+ return 0;
+
+ d->arch.has_32bit_shinfo = 1;
+ if ( is_pv_domain(d) )
+ d->arch.is_32bit_pv = 1;
+
+ for_each_vcpu( d, v )
+ {
+ rc = setup_compat_arg_xlat(v);
+ if ( !rc )
+ rc = setup_compat_l4(v);
+
+ if ( rc )
+ goto undo_and_fail;
+ }
+
+ domain_set_alloc_bitsize(d);
+ recalculate_cpuid_policy(d);
+
+ d->arch.x87_fip_width = 4;
+
+ return 0;
+
+ undo_and_fail:
+ d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+ for_each_vcpu( d, v )
+ {
+ free_compat_arg_xlat(v);
+ release_compat_l4(v);
+ }
+
+ return rc;
+}
+
+static int pv_create_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
+{
+ return create_perdomain_mapping(d, GDT_VIRT_START(v),
+ 1 << GDT_LDT_VCPU_SHIFT,
+ d->arch.pv_domain.gdt_ldt_l1tab, NULL);
+}
+
+static void pv_destroy_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
+{
+ destroy_perdomain_mapping(d, GDT_VIRT_START(v), 1 << GDT_LDT_VCPU_SHIFT);
+}
+
+void pv_vcpu_destroy(struct vcpu *v)
+{
+ if ( is_pv_32bit_vcpu(v) )
+ {
+ free_compat_arg_xlat(v);
+ release_compat_l4(v);
+ }
+
+ pv_destroy_gdt_ldt_l1tab(v->domain, v);
+ xfree(v->arch.pv_vcpu.trap_ctxt);
+ v->arch.pv_vcpu.trap_ctxt = NULL;
+}
+
+int pv_vcpu_initialise(struct vcpu *v)
+{
+ struct domain *d = v->domain;
+ int rc;
+
+ ASSERT(!is_idle_domain(d));
+
+ spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
+
+ rc = pv_create_gdt_ldt_l1tab(d, v);
+ if ( rc )
+ goto done;
+
+ BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
+ PAGE_SIZE);
+ v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
+ NR_VECTORS);
+ if ( !v->arch.pv_vcpu.trap_ctxt )
+ {
+ rc = -ENOMEM;
+ goto done;
+ }
+
+ /* PV guests by default have a 100Hz ticker. */
+ v->periodic_period = MILLISECS(10);
+
+ v->arch.pv_vcpu.ctrlreg[4] = real_cr4_to_pv_guest_cr4(mmu_cr4_features);
+
+ if ( is_pv_32bit_domain(d) )
+ {
+ if ( (rc = setup_compat_arg_xlat(v)) )
+ goto done;
+
+ if ( (rc = setup_compat_l4(v)) )
+ goto done;
+ }
+
+ done:
+ if ( rc )
+ pv_vcpu_destroy(v);
+ return rc;
+}
+
+void pv_domain_destroy(struct domain *d)
+{
+ destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
+ GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
+ xfree(d->arch.pv_domain.cpuidmasks);
+ d->arch.pv_domain.cpuidmasks = NULL;
+ free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
+ d->arch.pv_domain.gdt_ldt_l1tab = NULL;
+}
+
+
+extern void paravirt_ctxt_switch_from(struct vcpu *v);
+extern void paravirt_ctxt_switch_to(struct vcpu *v);
+
+int pv_domain_initialise(struct domain *d, unsigned int domcr_flags,
+ struct xen_arch_domainconfig *config)
+{
+ static const struct arch_csw pv_csw = {
+ .from = paravirt_ctxt_switch_from,
+ .to = paravirt_ctxt_switch_to,
+ .tail = continue_nonidle_domain,
+ };
+ int rc = -ENOMEM;
+
+ d->arch.pv_domain.gdt_ldt_l1tab =
+ alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
+ if ( !d->arch.pv_domain.gdt_ldt_l1tab )
+ goto fail;
+ clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
+
+ if ( levelling_caps & ~LCAP_faulting )
+ {
+ d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
+ if ( !d->arch.pv_domain.cpuidmasks )
+ goto fail;
+ *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
+ }
+
+ rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
+ GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
+ NULL, NULL);
+ if ( rc )
+ goto fail;
+
+ d->arch.ctxt_switch = &pv_csw;
+
+ /* 64-bit PV guest by default. */
+ d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+
+ return 0;
+
+ fail:
+ pv_domain_destroy(d);
+
+ return rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-x86/pv/domain.h b/xen/include/asm-x86/pv/domain.h
new file mode 100644
index 0000000000..6288ae24df
--- /dev/null
+++ b/xen/include/asm-x86/pv/domain.h
@@ -0,0 +1,30 @@
+/*
+ * pv/domain.h
+ *
+ * PV guest interface definitions
+ *
+ * Copyright (C) 2017 Wei Liu <wei.liu2@citrix.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __X86_PV_DOMAIN_H__
+#define __X86_PV_DOMAIN_H__
+
+void pv_vcpu_destroy(struct vcpu *v);
+int pv_vcpu_initialise(struct vcpu *v);
+void pv_domain_destroy(struct domain *d);
+int pv_domain_initialise(struct domain *d, unsigned int domcr_flags,
+ struct xen_arch_domainconfig *config);
+
+#endif /* __X86_PV_DOMAIN_H__ */
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH for-next v2 10/10] x86/domain: move HVM specific code to hvm/domain.c
2017-04-25 13:52 [PATCH for-next v2 00/10] x86: refactor x86/domain.c Wei Liu
` (8 preceding siblings ...)
2017-04-25 13:52 ` [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c Wei Liu
@ 2017-04-25 13:52 ` Wei Liu
2017-04-25 14:56 ` Andrew Cooper
9 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2017-04-25 13:52 UTC (permalink / raw)
To: Xen-devel; +Cc: Wei Liu
There is only one function arch_set_info_hvm_guest is moved. The
check_segment function is also moved since arch_set_info_hvm_guest is
the only user.
No functional change.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/domain.c | 291 -----------------------------------------
xen/arch/x86/hvm/Makefile | 1 +
xen/arch/x86/hvm/domain.c | 326 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 327 insertions(+), 291 deletions(-)
create mode 100644 xen/arch/x86/hvm/domain.c
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7c83fec8fc..3d86e5641d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1104,297 +1104,6 @@ int arch_set_info_guest(
#undef c
}
-static inline int check_segment(struct segment_register *reg,
- enum x86_segment seg)
-{
-
- if ( reg->attr.fields.pad != 0 )
- {
- gprintk(XENLOG_ERR, "Segment attribute bits 12-15 are not zero\n");
- return -EINVAL;
- }
-
- if ( reg->attr.bytes == 0 )
- {
- if ( seg != x86_seg_ds && seg != x86_seg_es )
- {
- gprintk(XENLOG_ERR, "Null selector provided for CS, SS or TR\n");
- return -EINVAL;
- }
- return 0;
- }
-
- if ( seg == x86_seg_tr )
- {
- if ( reg->attr.fields.s )
- {
- gprintk(XENLOG_ERR, "Code or data segment provided for TR\n");
- return -EINVAL;
- }
-
- if ( reg->attr.fields.type != SYS_DESC_tss_busy )
- {
- gprintk(XENLOG_ERR, "Non-32-bit-TSS segment provided for TR\n");
- return -EINVAL;
- }
- }
- else if ( !reg->attr.fields.s )
- {
- gprintk(XENLOG_ERR,
- "System segment provided for a code or data segment\n");
- return -EINVAL;
- }
-
- if ( !reg->attr.fields.p )
- {
- gprintk(XENLOG_ERR, "Non-present segment provided\n");
- return -EINVAL;
- }
-
- if ( seg == x86_seg_cs && !(reg->attr.fields.type & 0x8) )
- {
- gprintk(XENLOG_ERR, "Non-code segment provided for CS\n");
- return -EINVAL;
- }
-
- if ( seg == x86_seg_ss &&
- ((reg->attr.fields.type & 0x8) || !(reg->attr.fields.type & 0x2)) )
- {
- gprintk(XENLOG_ERR, "Non-writeable segment provided for SS\n");
- return -EINVAL;
- }
-
- if ( reg->attr.fields.s && seg != x86_seg_ss && seg != x86_seg_cs &&
- (reg->attr.fields.type & 0x8) && !(reg->attr.fields.type & 0x2) )
- {
- gprintk(XENLOG_ERR, "Non-readable segment provided for DS or ES\n");
- return -EINVAL;
- }
-
- return 0;
-}
-
-/* Called by VCPUOP_initialise for HVM guests. */
-int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
-{
- struct cpu_user_regs *uregs = &v->arch.user_regs;
- struct segment_register cs, ds, ss, es, tr;
- const char *errstr;
- int rc;
-
- if ( ctx->pad != 0 )
- return -EINVAL;
-
- switch ( ctx->mode )
- {
- default:
- return -EINVAL;
-
- case VCPU_HVM_MODE_32B:
- {
- const struct vcpu_hvm_x86_32 *regs = &ctx->cpu_regs.x86_32;
- uint32_t limit;
-
- if ( ctx->cpu_regs.x86_32.pad1 != 0 ||
- ctx->cpu_regs.x86_32.pad2[0] != 0 ||
- ctx->cpu_regs.x86_32.pad2[1] != 0 ||
- ctx->cpu_regs.x86_32.pad2[2] != 0 )
- return -EINVAL;
-
-#define SEG(s, r) ({ \
- s = (struct segment_register){ .base = (r)->s ## _base, \
- .limit = (r)->s ## _limit, \
- .attr.bytes = (r)->s ## _ar }; \
- /* Set accessed / busy bit for present segments. */ \
- if ( s.attr.fields.p ) \
- s.attr.fields.type |= (x86_seg_##s != x86_seg_tr ? 1 : 2); \
- check_segment(&s, x86_seg_ ## s); })
-
- rc = SEG(cs, regs);
- rc |= SEG(ds, regs);
- rc |= SEG(ss, regs);
- rc |= SEG(es, regs);
- rc |= SEG(tr, regs);
-#undef SEG
-
- if ( rc != 0 )
- return rc;
-
- /* Basic sanity checks. */
- limit = cs.limit;
- if ( cs.attr.fields.g )
- limit = (limit << 12) | 0xfff;
- if ( regs->eip > limit )
- {
- gprintk(XENLOG_ERR, "EIP (%#08x) outside CS limit (%#08x)\n",
- regs->eip, limit);
- return -EINVAL;
- }
-
- if ( ss.attr.fields.dpl != cs.attr.fields.dpl )
- {
- gprintk(XENLOG_ERR, "SS.DPL (%u) is different than CS.DPL (%u)\n",
- ss.attr.fields.dpl, cs.attr.fields.dpl);
- return -EINVAL;
- }
-
- if ( ds.attr.fields.p && ds.attr.fields.dpl > cs.attr.fields.dpl )
- {
- gprintk(XENLOG_ERR, "DS.DPL (%u) is greater than CS.DPL (%u)\n",
- ds.attr.fields.dpl, cs.attr.fields.dpl);
- return -EINVAL;
- }
-
- if ( es.attr.fields.p && es.attr.fields.dpl > cs.attr.fields.dpl )
- {
- gprintk(XENLOG_ERR, "ES.DPL (%u) is greater than CS.DPL (%u)\n",
- es.attr.fields.dpl, cs.attr.fields.dpl);
- return -EINVAL;
- }
-
- if ( (regs->efer & EFER_LMA) && !(regs->efer & EFER_LME) )
- {
- gprintk(XENLOG_ERR, "EFER.LMA set without EFER.LME (%#016lx)\n",
- regs->efer);
- return -EINVAL;
- }
-
- uregs->rax = regs->eax;
- uregs->rcx = regs->ecx;
- uregs->rdx = regs->edx;
- uregs->rbx = regs->ebx;
- uregs->rsp = regs->esp;
- uregs->rbp = regs->ebp;
- uregs->rsi = regs->esi;
- uregs->rdi = regs->edi;
- uregs->rip = regs->eip;
- uregs->rflags = regs->eflags;
-
- v->arch.hvm_vcpu.guest_cr[0] = regs->cr0;
- v->arch.hvm_vcpu.guest_cr[3] = regs->cr3;
- v->arch.hvm_vcpu.guest_cr[4] = regs->cr4;
- v->arch.hvm_vcpu.guest_efer = regs->efer;
- }
- break;
-
- case VCPU_HVM_MODE_64B:
- {
- const struct vcpu_hvm_x86_64 *regs = &ctx->cpu_regs.x86_64;
-
- /* Basic sanity checks. */
- if ( !is_canonical_address(regs->rip) )
- {
- gprintk(XENLOG_ERR, "RIP contains a non-canonical address (%#lx)\n",
- regs->rip);
- return -EINVAL;
- }
-
- if ( !(regs->cr0 & X86_CR0_PG) )
- {
- gprintk(XENLOG_ERR, "CR0 doesn't have paging enabled (%#016lx)\n",
- regs->cr0);
- return -EINVAL;
- }
-
- if ( !(regs->cr4 & X86_CR4_PAE) )
- {
- gprintk(XENLOG_ERR, "CR4 doesn't have PAE enabled (%#016lx)\n",
- regs->cr4);
- return -EINVAL;
- }
-
- if ( !(regs->efer & EFER_LME) )
- {
- gprintk(XENLOG_ERR, "EFER doesn't have LME enabled (%#016lx)\n",
- regs->efer);
- return -EINVAL;
- }
-
- uregs->rax = regs->rax;
- uregs->rcx = regs->rcx;
- uregs->rdx = regs->rdx;
- uregs->rbx = regs->rbx;
- uregs->rsp = regs->rsp;
- uregs->rbp = regs->rbp;
- uregs->rsi = regs->rsi;
- uregs->rdi = regs->rdi;
- uregs->rip = regs->rip;
- uregs->rflags = regs->rflags;
-
- v->arch.hvm_vcpu.guest_cr[0] = regs->cr0;
- v->arch.hvm_vcpu.guest_cr[3] = regs->cr3;
- v->arch.hvm_vcpu.guest_cr[4] = regs->cr4;
- v->arch.hvm_vcpu.guest_efer = regs->efer;
-
-#define SEG(l, a) (struct segment_register){ .limit = (l), .attr.bytes = (a) }
- cs = SEG(~0u, 0xa9b); /* 64bit code segment. */
- ds = ss = es = SEG(~0u, 0xc93);
- tr = SEG(0x67, 0x8b); /* 64bit TSS (busy). */
-#undef SEG
- }
- break;
-
- }
-
- if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )
- v->arch.hvm_vcpu.guest_efer |= EFER_LMA;
-
- if ( v->arch.hvm_vcpu.guest_cr[4] & ~hvm_cr4_guest_valid_bits(v, 0) )
- {
- gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
- v->arch.hvm_vcpu.guest_cr[4]);
- return -EINVAL;
- }
-
- errstr = hvm_efer_valid(v, v->arch.hvm_vcpu.guest_efer, -1);
- if ( errstr )
- {
- gprintk(XENLOG_ERR, "Bad EFER value (%#016lx): %s\n",
- v->arch.hvm_vcpu.guest_efer, errstr);
- return -EINVAL;
- }
-
- hvm_update_guest_cr(v, 0);
- hvm_update_guest_cr(v, 3);
- hvm_update_guest_cr(v, 4);
- hvm_update_guest_efer(v);
-
- if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) )
- {
- /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
- struct page_info *page = get_page_from_gfn(v->domain,
- v->arch.hvm_vcpu.guest_cr[3] >> PAGE_SHIFT,
- NULL, P2M_ALLOC);
- if ( !page )
- {
- gprintk(XENLOG_ERR, "Invalid CR3: %#lx\n",
- v->arch.hvm_vcpu.guest_cr[3]);
- return -EINVAL;
- }
-
- v->arch.guest_table = pagetable_from_page(page);
- }
-
- hvm_set_segment_register(v, x86_seg_cs, &cs);
- hvm_set_segment_register(v, x86_seg_ds, &ds);
- hvm_set_segment_register(v, x86_seg_ss, &ss);
- hvm_set_segment_register(v, x86_seg_es, &es);
- hvm_set_segment_register(v, x86_seg_tr, &tr);
-
- /* Sync AP's TSC with BSP's. */
- v->arch.hvm_vcpu.cache_tsc_offset =
- v->domain->vcpu[0]->arch.hvm_vcpu.cache_tsc_offset;
- hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset,
- v->domain->arch.hvm_domain.sync_tsc);
-
- paging_update_paging_modes(v);
-
- v->is_initialised = 1;
- set_bit(_VPF_down, &v->pause_flags);
-
- return 0;
-}
-
int arch_initialise_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
{
int rc;
diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 0a3d0f4f7e..4dc0773a93 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -4,6 +4,7 @@ subdir-y += vmx
obj-y += asid.o
obj-y += dm.o
obj-bin-y += dom0_build.init.o
+obj-y += domain.o
obj-y += emulate.o
obj-y += hpet.o
obj-y += hvm.o
diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
new file mode 100644
index 0000000000..dd03cffd0b
--- /dev/null
+++ b/xen/arch/x86/hvm/domain.c
@@ -0,0 +1,326 @@
+/*
+ * HVM domain specific functions.
+ *
+ * Copyright (C) 2017 Citrix Systems R&D
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/domain_page.h>
+#include <xen/errno.h>
+#include <xen/lib.h>
+#include <xen/paging.h>
+#include <xen/sched.h>
+
+#include <public/hvm/hvm_vcpu.h>
+
+static inline int check_segment(struct segment_register *reg,
+ enum x86_segment seg)
+{
+
+ if ( reg->attr.fields.pad != 0 )
+ {
+ gprintk(XENLOG_ERR, "Segment attribute bits 12-15 are not zero\n");
+ return -EINVAL;
+ }
+
+ if ( reg->attr.bytes == 0 )
+ {
+ if ( seg != x86_seg_ds && seg != x86_seg_es )
+ {
+ gprintk(XENLOG_ERR, "Null selector provided for CS, SS or TR\n");
+ return -EINVAL;
+ }
+ return 0;
+ }
+
+ if ( seg == x86_seg_tr )
+ {
+ if ( reg->attr.fields.s )
+ {
+ gprintk(XENLOG_ERR, "Code or data segment provided for TR\n");
+ return -EINVAL;
+ }
+
+ if ( reg->attr.fields.type != SYS_DESC_tss_busy )
+ {
+ gprintk(XENLOG_ERR, "Non-32-bit-TSS segment provided for TR\n");
+ return -EINVAL;
+ }
+ }
+ else if ( !reg->attr.fields.s )
+ {
+ gprintk(XENLOG_ERR,
+ "System segment provided for a code or data segment\n");
+ return -EINVAL;
+ }
+
+ if ( !reg->attr.fields.p )
+ {
+ gprintk(XENLOG_ERR, "Non-present segment provided\n");
+ return -EINVAL;
+ }
+
+ if ( seg == x86_seg_cs && !(reg->attr.fields.type & 0x8) )
+ {
+ gprintk(XENLOG_ERR, "Non-code segment provided for CS\n");
+ return -EINVAL;
+ }
+
+ if ( seg == x86_seg_ss &&
+ ((reg->attr.fields.type & 0x8) || !(reg->attr.fields.type & 0x2)) )
+ {
+ gprintk(XENLOG_ERR, "Non-writeable segment provided for SS\n");
+ return -EINVAL;
+ }
+
+ if ( reg->attr.fields.s && seg != x86_seg_ss && seg != x86_seg_cs &&
+ (reg->attr.fields.type & 0x8) && !(reg->attr.fields.type & 0x2) )
+ {
+ gprintk(XENLOG_ERR, "Non-readable segment provided for DS or ES\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/* Called by VCPUOP_initialise for HVM guests. */
+int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
+{
+ struct cpu_user_regs *uregs = &v->arch.user_regs;
+ struct segment_register cs, ds, ss, es, tr;
+ const char *errstr;
+ int rc;
+
+ if ( ctx->pad != 0 )
+ return -EINVAL;
+
+ switch ( ctx->mode )
+ {
+ default:
+ return -EINVAL;
+
+ case VCPU_HVM_MODE_32B:
+ {
+ const struct vcpu_hvm_x86_32 *regs = &ctx->cpu_regs.x86_32;
+ uint32_t limit;
+
+ if ( ctx->cpu_regs.x86_32.pad1 != 0 ||
+ ctx->cpu_regs.x86_32.pad2[0] != 0 ||
+ ctx->cpu_regs.x86_32.pad2[1] != 0 ||
+ ctx->cpu_regs.x86_32.pad2[2] != 0 )
+ return -EINVAL;
+
+#define SEG(s, r) ({ \
+ s = (struct segment_register){ .base = (r)->s ## _base, \
+ .limit = (r)->s ## _limit, \
+ .attr.bytes = (r)->s ## _ar }; \
+ /* Set accessed / busy bit for present segments. */ \
+ if ( s.attr.fields.p ) \
+ s.attr.fields.type |= (x86_seg_##s != x86_seg_tr ? 1 : 2); \
+ check_segment(&s, x86_seg_ ## s); })
+
+ rc = SEG(cs, regs);
+ rc |= SEG(ds, regs);
+ rc |= SEG(ss, regs);
+ rc |= SEG(es, regs);
+ rc |= SEG(tr, regs);
+#undef SEG
+
+ if ( rc != 0 )
+ return rc;
+
+ /* Basic sanity checks. */
+ limit = cs.limit;
+ if ( cs.attr.fields.g )
+ limit = (limit << 12) | 0xfff;
+ if ( regs->eip > limit )
+ {
+ gprintk(XENLOG_ERR, "EIP (%#08x) outside CS limit (%#08x)\n",
+ regs->eip, limit);
+ return -EINVAL;
+ }
+
+ if ( ss.attr.fields.dpl != cs.attr.fields.dpl )
+ {
+ gprintk(XENLOG_ERR, "SS.DPL (%u) is different than CS.DPL (%u)\n",
+ ss.attr.fields.dpl, cs.attr.fields.dpl);
+ return -EINVAL;
+ }
+
+ if ( ds.attr.fields.p && ds.attr.fields.dpl > cs.attr.fields.dpl )
+ {
+ gprintk(XENLOG_ERR, "DS.DPL (%u) is greater than CS.DPL (%u)\n",
+ ds.attr.fields.dpl, cs.attr.fields.dpl);
+ return -EINVAL;
+ }
+
+ if ( es.attr.fields.p && es.attr.fields.dpl > cs.attr.fields.dpl )
+ {
+ gprintk(XENLOG_ERR, "ES.DPL (%u) is greater than CS.DPL (%u)\n",
+ es.attr.fields.dpl, cs.attr.fields.dpl);
+ return -EINVAL;
+ }
+
+ if ( (regs->efer & EFER_LMA) && !(regs->efer & EFER_LME) )
+ {
+ gprintk(XENLOG_ERR, "EFER.LMA set without EFER.LME (%#016lx)\n",
+ regs->efer);
+ return -EINVAL;
+ }
+
+ uregs->rax = regs->eax;
+ uregs->rcx = regs->ecx;
+ uregs->rdx = regs->edx;
+ uregs->rbx = regs->ebx;
+ uregs->rsp = regs->esp;
+ uregs->rbp = regs->ebp;
+ uregs->rsi = regs->esi;
+ uregs->rdi = regs->edi;
+ uregs->rip = regs->eip;
+ uregs->rflags = regs->eflags;
+
+ v->arch.hvm_vcpu.guest_cr[0] = regs->cr0;
+ v->arch.hvm_vcpu.guest_cr[3] = regs->cr3;
+ v->arch.hvm_vcpu.guest_cr[4] = regs->cr4;
+ v->arch.hvm_vcpu.guest_efer = regs->efer;
+ }
+ break;
+
+ case VCPU_HVM_MODE_64B:
+ {
+ const struct vcpu_hvm_x86_64 *regs = &ctx->cpu_regs.x86_64;
+
+ /* Basic sanity checks. */
+ if ( !is_canonical_address(regs->rip) )
+ {
+ gprintk(XENLOG_ERR, "RIP contains a non-canonical address (%#lx)\n",
+ regs->rip);
+ return -EINVAL;
+ }
+
+ if ( !(regs->cr0 & X86_CR0_PG) )
+ {
+ gprintk(XENLOG_ERR, "CR0 doesn't have paging enabled (%#016lx)\n",
+ regs->cr0);
+ return -EINVAL;
+ }
+
+ if ( !(regs->cr4 & X86_CR4_PAE) )
+ {
+ gprintk(XENLOG_ERR, "CR4 doesn't have PAE enabled (%#016lx)\n",
+ regs->cr4);
+ return -EINVAL;
+ }
+
+ if ( !(regs->efer & EFER_LME) )
+ {
+ gprintk(XENLOG_ERR, "EFER doesn't have LME enabled (%#016lx)\n",
+ regs->efer);
+ return -EINVAL;
+ }
+
+ uregs->rax = regs->rax;
+ uregs->rcx = regs->rcx;
+ uregs->rdx = regs->rdx;
+ uregs->rbx = regs->rbx;
+ uregs->rsp = regs->rsp;
+ uregs->rbp = regs->rbp;
+ uregs->rsi = regs->rsi;
+ uregs->rdi = regs->rdi;
+ uregs->rip = regs->rip;
+ uregs->rflags = regs->rflags;
+
+ v->arch.hvm_vcpu.guest_cr[0] = regs->cr0;
+ v->arch.hvm_vcpu.guest_cr[3] = regs->cr3;
+ v->arch.hvm_vcpu.guest_cr[4] = regs->cr4;
+ v->arch.hvm_vcpu.guest_efer = regs->efer;
+
+#define SEG(l, a) (struct segment_register){ .limit = (l), .attr.bytes = (a) }
+ cs = SEG(~0u, 0xa9b); /* 64bit code segment. */
+ ds = ss = es = SEG(~0u, 0xc93);
+ tr = SEG(0x67, 0x8b); /* 64bit TSS (busy). */
+#undef SEG
+ }
+ break;
+
+ }
+
+ if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )
+ v->arch.hvm_vcpu.guest_efer |= EFER_LMA;
+
+ if ( v->arch.hvm_vcpu.guest_cr[4] & ~hvm_cr4_guest_valid_bits(v, 0) )
+ {
+ gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
+ v->arch.hvm_vcpu.guest_cr[4]);
+ return -EINVAL;
+ }
+
+ errstr = hvm_efer_valid(v, v->arch.hvm_vcpu.guest_efer, -1);
+ if ( errstr )
+ {
+ gprintk(XENLOG_ERR, "Bad EFER value (%#016lx): %s\n",
+ v->arch.hvm_vcpu.guest_efer, errstr);
+ return -EINVAL;
+ }
+
+ hvm_update_guest_cr(v, 0);
+ hvm_update_guest_cr(v, 3);
+ hvm_update_guest_cr(v, 4);
+ hvm_update_guest_efer(v);
+
+ if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) )
+ {
+ /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
+ struct page_info *page = get_page_from_gfn(v->domain,
+ v->arch.hvm_vcpu.guest_cr[3] >> PAGE_SHIFT,
+ NULL, P2M_ALLOC);
+ if ( !page )
+ {
+ gprintk(XENLOG_ERR, "Invalid CR3: %#lx\n",
+ v->arch.hvm_vcpu.guest_cr[3]);
+ return -EINVAL;
+ }
+
+ v->arch.guest_table = pagetable_from_page(page);
+ }
+
+ hvm_set_segment_register(v, x86_seg_cs, &cs);
+ hvm_set_segment_register(v, x86_seg_ds, &ds);
+ hvm_set_segment_register(v, x86_seg_ss, &ss);
+ hvm_set_segment_register(v, x86_seg_es, &es);
+ hvm_set_segment_register(v, x86_seg_tr, &tr);
+
+ /* Sync AP's TSC with BSP's. */
+ v->arch.hvm_vcpu.cache_tsc_offset =
+ v->domain->vcpu[0]->arch.hvm_vcpu.cache_tsc_offset;
+ hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset,
+ v->domain->arch.hvm_domain.sync_tsc);
+
+ paging_update_paging_modes(v);
+
+ v->is_initialised = 1;
+ set_bit(_VPF_down, &v->pause_flags);
+
+ return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 01/10] x86/mm: make free_perdomain_mappings idempotent
2017-04-25 13:52 ` [PATCH for-next v2 01/10] x86/mm: make free_perdomain_mappings idempotent Wei Liu
@ 2017-04-25 13:54 ` Andrew Cooper
0 siblings, 0 replies; 43+ messages in thread
From: Andrew Cooper @ 2017-04-25 13:54 UTC (permalink / raw)
To: Wei Liu, Xen-devel; +Cc: George Dunlap, Tim Deegan, Jan Beulich
On 25/04/17 14:52, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> Cc: Tim Deegan <tim@xen.org>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> xen/arch/x86/mm.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index bd68e56dc7..cdef5c49c4 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -6503,9 +6503,14 @@ void destroy_perdomain_mapping(struct domain *d, unsigned long va,
>
> void free_perdomain_mappings(struct domain *d)
> {
> - l3_pgentry_t *l3tab = __map_domain_page(d->arch.perdomain_l3_pg);
> + l3_pgentry_t *l3tab;
> unsigned int i;
>
> + if ( !d->arch.perdomain_l3_pg )
> + return;
> +
> + l3tab = __map_domain_page(d->arch.perdomain_l3_pg);
> +
> for ( i = 0; i < PERDOMAIN_SLOTS; ++i)
> if ( l3e_get_flags(l3tab[i]) & _PAGE_PRESENT )
> {
> @@ -6544,6 +6549,7 @@ void free_perdomain_mappings(struct domain *d)
>
> unmap_domain_page(l3tab);
> free_domheap_page(d->arch.perdomain_l3_pg);
> + d->arch.perdomain_l3_pg = NULL;
> }
>
> #ifdef MEMORY_GUARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 02/10] x86/domain: provide pv_{create, destroy}_gdt_ldt_l1tab and use them
2017-04-25 13:52 ` [PATCH for-next v2 02/10] x86/domain: provide pv_{create, destroy}_gdt_ldt_l1tab and use them Wei Liu
@ 2017-04-25 13:56 ` Andrew Cooper
2017-04-25 14:02 ` Wei Liu
0 siblings, 1 reply; 43+ messages in thread
From: Andrew Cooper @ 2017-04-25 13:56 UTC (permalink / raw)
To: Wei Liu, Xen-devel; +Cc: Jan Beulich
On 25/04/17 14:52, Wei Liu wrote:
> This patch encapsulates the perdomain creation and destruction into
> helper functions and use them where appropriate.
>
> Since destroy_perdomain_mapping is idempotent, it is safe to call the
> destruction function multiple times.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> xen/arch/x86/domain.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 90e2b1f82a..1f76d034a7 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -387,6 +387,18 @@ int switch_compat(struct domain *d)
> return rc;
> }
>
> +static int pv_create_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
> +{
> + return create_perdomain_mapping(d, GDT_VIRT_START(v),
> + 1 << GDT_LDT_VCPU_SHIFT,
> + d->arch.pv_domain.gdt_ldt_l1tab, NULL);
> +}
> +
> +static void pv_destroy_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
> +{
> + destroy_perdomain_mapping(d, GDT_VIRT_START(v), 1 << GDT_LDT_VCPU_SHIFT);
> +}
What is the point of passing both d and v? Isn't d always v->domain ?
Otherwise, LGTM.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 03/10] x86/domain: make release_compact_l4 NULL tolerant
2017-04-25 13:52 ` [PATCH for-next v2 03/10] x86/domain: make release_compact_l4 NULL tolerant Wei Liu
@ 2017-04-25 13:56 ` Andrew Cooper
0 siblings, 0 replies; 43+ messages in thread
From: Andrew Cooper @ 2017-04-25 13:56 UTC (permalink / raw)
To: Wei Liu, Xen-devel; +Cc: Jan Beulich
On 25/04/17 14:52, Wei Liu wrote:
> Push the check in caller down to that function so that it becomes
> idempotent.
>
> No functional change.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 04/10] x86/domain: factor out pv_vcpu_destroy
2017-04-25 13:52 ` [PATCH for-next v2 04/10] x86/domain: factor out pv_vcpu_destroy Wei Liu
@ 2017-04-25 13:58 ` Andrew Cooper
2017-04-26 12:21 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Andrew Cooper @ 2017-04-25 13:58 UTC (permalink / raw)
To: Wei Liu, Xen-devel; +Cc: Jan Beulich
On 25/04/17 14:52, Wei Liu wrote:
> The function is made idempotent on purpose.
>
> No functional change.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 02/10] x86/domain: provide pv_{create, destroy}_gdt_ldt_l1tab and use them
2017-04-25 13:56 ` Andrew Cooper
@ 2017-04-25 14:02 ` Wei Liu
0 siblings, 0 replies; 43+ messages in thread
From: Wei Liu @ 2017-04-25 14:02 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Jan Beulich
On Tue, Apr 25, 2017 at 02:56:01PM +0100, Andrew Cooper wrote:
> On 25/04/17 14:52, Wei Liu wrote:
> > This patch encapsulates the perdomain creation and destruction into
> > helper functions and use them where appropriate.
> >
> > Since destroy_perdomain_mapping is idempotent, it is safe to call the
> > destruction function multiple times.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > xen/arch/x86/domain.c | 23 ++++++++++++++++++++---
> > 1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index 90e2b1f82a..1f76d034a7 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -387,6 +387,18 @@ int switch_compat(struct domain *d)
> > return rc;
> > }
> >
> > +static int pv_create_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
> > +{
> > + return create_perdomain_mapping(d, GDT_VIRT_START(v),
> > + 1 << GDT_LDT_VCPU_SHIFT,
> > + d->arch.pv_domain.gdt_ldt_l1tab, NULL);
> > +}
> > +
> > +static void pv_destroy_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
> > +{
> > + destroy_perdomain_mapping(d, GDT_VIRT_START(v), 1 << GDT_LDT_VCPU_SHIFT);
> > +}
>
> What is the point of passing both d and v? Isn't d always v->domain ?
>
Yes you're right.
> Otherwise, LGTM.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise
2017-04-25 13:52 ` [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise Wei Liu
@ 2017-04-25 14:08 ` Andrew Cooper
2017-04-25 14:27 ` Wei Liu
2017-04-26 12:25 ` Jan Beulich
1 sibling, 1 reply; 43+ messages in thread
From: Andrew Cooper @ 2017-04-25 14:08 UTC (permalink / raw)
To: Wei Liu, Xen-devel; +Cc: Jan Beulich
On 25/04/17 14:52, Wei Liu wrote:
> Move PV specific vcpu initialisation code to said function, but leave
> the only line needed by idle domain in vcpu_initialise.
>
> Use pv_vcpu_destroy in error path to simplify code. It is safe to do so
> because the destruction function accepts partially initialised vcpu
> struct.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> xen/arch/x86/domain.c | 99 ++++++++++++++++++++++++++-------------------------
> 1 file changed, 50 insertions(+), 49 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index cde0917f5b..38fc4f5d8b 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -398,6 +398,50 @@ static void pv_destroy_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
> destroy_perdomain_mapping(d, GDT_VIRT_START(v), 1 << GDT_LDT_VCPU_SHIFT);
> }
>
> +static void pv_vcpu_destroy(struct vcpu *v);
If in the previous patch, you create pv_vcpu_destroy() earlier than
vcpu_initialise(), you wouldn't need this forward declaration here.
Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 06/10] x86/domain: push some code down to hvm_domain_initialise
2017-04-25 13:52 ` [PATCH for-next v2 06/10] x86/domain: push some code down to hvm_domain_initialise Wei Liu
@ 2017-04-25 14:15 ` Andrew Cooper
2017-04-25 14:29 ` Wei Liu
0 siblings, 1 reply; 43+ messages in thread
From: Andrew Cooper @ 2017-04-25 14:15 UTC (permalink / raw)
To: Wei Liu, Xen-devel; +Cc: Jan Beulich
On 25/04/17 14:52, Wei Liu wrote:
> We want to have a single entry point to initialise hvm guest. Now the
> timing to set hap bit and create per domain mapping is deferred, but
> that's not a problem.
This wording is a little odd to read. How about "To do this, the
setting of hap_enabled and creation of the per domain mappings is
deferred, but..." ?
>
> No functional change.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 07/10] x86/domain: factor out pv_domain_destroy
2017-04-25 13:52 ` [PATCH for-next v2 07/10] x86/domain: factor out pv_domain_destroy Wei Liu
@ 2017-04-25 14:18 ` Andrew Cooper
2017-04-26 12:32 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Andrew Cooper @ 2017-04-25 14:18 UTC (permalink / raw)
To: Wei Liu, Xen-devel; +Cc: Jan Beulich
On 25/04/17 14:52, Wei Liu wrote:
> No functional change.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> v2:
> 1. reset fields
> 2. destroy perdomain mapping
You are introducing an additional destroy_perdomain_mapping(), and
freeing of the cpuidmasks into this path. Functionally it is fine, but
it needs calling out in the commit message as to why it is safe to do.
>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> xen/arch/x86/domain.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index fd5d47ab3d..952e05366a 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -536,6 +536,16 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> return true;
> }
>
> +static void pv_domain_destroy(struct domain *d)
> +{
> + destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
> + GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
Please could we have a newline here...
> + xfree(d->arch.pv_domain.cpuidmasks);
> + d->arch.pv_domain.cpuidmasks = NULL;
And here, just to visually separate the different parts.
~Andrew
> + free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> + d->arch.pv_domain.gdt_ldt_l1tab = NULL;
> +}
> +
> int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> struct xen_arch_domainconfig *config)
> {
> @@ -712,10 +722,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> paging_final_teardown(d);
> free_perdomain_mappings(d);
> if ( is_pv_domain(d) )
> - {
> - xfree(d->arch.pv_domain.cpuidmasks);
> - free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> - }
> + pv_domain_destroy(d);
> +
> return rc;
> }
>
> @@ -735,10 +743,7 @@ void arch_domain_destroy(struct domain *d)
>
> free_perdomain_mappings(d);
> if ( is_pv_domain(d) )
> - {
> - free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> - xfree(d->arch.pv_domain.cpuidmasks);
> - }
> + pv_domain_destroy(d);
>
> free_xenheap_page(d->shared_info);
> cleanup_domain_irq_mapping(d);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise
2017-04-25 14:08 ` Andrew Cooper
@ 2017-04-25 14:27 ` Wei Liu
0 siblings, 0 replies; 43+ messages in thread
From: Wei Liu @ 2017-04-25 14:27 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Jan Beulich
On Tue, Apr 25, 2017 at 03:08:23PM +0100, Andrew Cooper wrote:
> On 25/04/17 14:52, Wei Liu wrote:
> > Move PV specific vcpu initialisation code to said function, but leave
> > the only line needed by idle domain in vcpu_initialise.
> >
> > Use pv_vcpu_destroy in error path to simplify code. It is safe to do so
> > because the destruction function accepts partially initialised vcpu
> > struct.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > xen/arch/x86/domain.c | 99 ++++++++++++++++++++++++++-------------------------
> > 1 file changed, 50 insertions(+), 49 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index cde0917f5b..38fc4f5d8b 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -398,6 +398,50 @@ static void pv_destroy_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
> > destroy_perdomain_mapping(d, GDT_VIRT_START(v), 1 << GDT_LDT_VCPU_SHIFT);
> > }
> >
> > +static void pv_vcpu_destroy(struct vcpu *v);
>
> If in the previous patch, you create pv_vcpu_destroy() earlier than
> vcpu_initialise(), you wouldn't need this forward declaration here.
>
Yeah I know. I chose to rearranged them in the patch that moved pv code
instead, because rebasing huge chunk of code is error-prone.
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 06/10] x86/domain: push some code down to hvm_domain_initialise
2017-04-25 14:15 ` Andrew Cooper
@ 2017-04-25 14:29 ` Wei Liu
0 siblings, 0 replies; 43+ messages in thread
From: Wei Liu @ 2017-04-25 14:29 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Jan Beulich
On Tue, Apr 25, 2017 at 03:15:03PM +0100, Andrew Cooper wrote:
> On 25/04/17 14:52, Wei Liu wrote:
> > We want to have a single entry point to initialise hvm guest. Now the
> > timing to set hap bit and create per domain mapping is deferred, but
> > that's not a problem.
>
> This wording is a little odd to read. How about "To do this, the
> setting of hap_enabled and creation of the per domain mappings is
> deferred, but..." ?
>
No problem.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 08/10] x86/domain: factor out pv_domain_initialise
2017-04-25 13:52 ` [PATCH for-next v2 08/10] x86/domain: factor out pv_domain_initialise Wei Liu
@ 2017-04-25 14:30 ` Andrew Cooper
0 siblings, 0 replies; 43+ messages in thread
From: Andrew Cooper @ 2017-04-25 14:30 UTC (permalink / raw)
To: Wei Liu, Xen-devel; +Cc: Jan Beulich
On 25/04/17 14:52, Wei Liu wrote:
> Lump everything PV related in arch_domain_create into
> pv_domain_initialise.
>
> Though domcr_flags and config are not necessary, the new function is
> given those to match hvm counterpart.
>
> Since it cleans up after itself there is no need to clean up in
> arch_domain_create in case of failure. Remove the initialiser of rc in
> arch_domain_create.
>
> No functional change.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c
2017-04-25 13:52 ` [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c Wei Liu
@ 2017-04-25 14:52 ` Andrew Cooper
2017-04-25 15:07 ` Wei Liu
` (3 more replies)
0 siblings, 4 replies; 43+ messages in thread
From: Andrew Cooper @ 2017-04-25 14:52 UTC (permalink / raw)
To: Wei Liu, Xen-devel; +Cc: Jan Beulich
On 25/04/17 14:52, Wei Liu wrote:
> - fail:
> - pv_domain_destroy(d);
> -
> - return rc;
> -}
> -
> +void paravirt_ctxt_switch_from(struct vcpu *v);
> +void paravirt_ctxt_switch_to(struct vcpu *v);
> int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> struct xen_arch_domainconfig *config)
> {
> @@ -1919,7 +1717,8 @@ static void save_segments(struct vcpu *v)
>
> #define switch_kernel_stack(v) ((void)0)
>
> -static void paravirt_ctxt_switch_from(struct vcpu *v)
> +/* Needed by PV guests */
> +void paravirt_ctxt_switch_from(struct vcpu *v)
> {
Could these be moved up to avoid the forward declarations above?
> diff --git a/xen/arch/x86/pv/Makefile b/xen/arch/x86/pv/Makefile
> index ea94599438..2737824e81 100644
> --- a/xen/arch/x86/pv/Makefile
> +++ b/xen/arch/x86/pv/Makefile
> @@ -1,2 +1,3 @@
> obj-y += hypercall.o
> obj-bin-y += dom0_build.init.o
> +obj-y += domain.o
> diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> new file mode 100644
> index 0000000000..562c3d03f5
> --- /dev/null
> +++ b/xen/arch/x86/pv/domain.c
> @@ -0,0 +1,232 @@
> +/******************************************************************************
> + * arch/x86/pv/domain.c
> + *
> + * PV domain handling
> + */
> +
> +/*
> + * Copyright (C) 1995 Linus Torvalds
> + *
> + * Pentium III FXSR, SSE support
> + * Gareth Hughes <gareth@valinux.com>, May 2000
> + */
> +
> +#include <xen/domain_page.h>
> +#include <xen/errno.h>
> +#include <xen/lib.h>
> +#include <xen/sched.h>
> +
> +static void noreturn continue_nonidle_domain(struct vcpu *v)
> +{
> + check_wakeup_from_wait();
> + mark_regs_dirty(guest_cpu_user_regs());
> + reset_stack_and_jump(ret_from_intr);
> +}
> +
> +static int setup_compat_l4(struct vcpu *v)
> +{
> + struct page_info *pg;
> + l4_pgentry_t *l4tab;
> +
> + pg = alloc_domheap_page(v->domain, MEMF_no_owner);
> + if ( pg == NULL )
> + return -ENOMEM;
> +
> + /* This page needs to look like a pagetable so that it can be shadowed */
.
> + pg->u.inuse.type_info = PGT_l4_page_table|PGT_validated|1;
More spaces please.
> +
> + l4tab = __map_domain_page(pg);
> + clear_page(l4tab);
I know this patch is only code motion, but I really think it would be
safer to defer the type_info update until after we have cleared the page.
> + init_guest_l4_table(l4tab, v->domain, 1);
> + unmap_domain_page(l4tab);
> +
> + v->arch.guest_table = pagetable_from_page(pg);
> + v->arch.guest_table_user = v->arch.guest_table;
> +
> + return 0;
> +}
> +
> +static void release_compat_l4(struct vcpu *v)
> +{
> + if ( !pagetable_is_null(v->arch.guest_table) )
> + free_domheap_page(pagetable_get_page(v->arch.guest_table));
> + v->arch.guest_table = pagetable_null();
> + v->arch.guest_table_user = pagetable_null();
> +}
> +
> +int switch_compat(struct domain *d)
> +{
> + struct vcpu *v;
> + int rc;
> +
> + if ( is_hvm_domain(d) || d->tot_pages != 0 )
> + return -EACCES;
> + if ( is_pv_32bit_domain(d) )
> + return 0;
> +
> + d->arch.has_32bit_shinfo = 1;
> + if ( is_pv_domain(d) )
> + d->arch.is_32bit_pv = 1;
This is_pv_domain() is redundant. I expect this was fallout from
ripping PVH out.
> +
> + for_each_vcpu( d, v )
> + {
> + rc = setup_compat_arg_xlat(v);
> + if ( !rc )
> + rc = setup_compat_l4(v);
> +
> + if ( rc )
> + goto undo_and_fail;
This is odd structuring. Care to rearrange it with two goto's, rather
than inverting the first rc check?
> + }
> +
> + domain_set_alloc_bitsize(d);
> + recalculate_cpuid_policy(d);
> +
> + d->arch.x87_fip_width = 4;
> +
> + return 0;
> +
> + undo_and_fail:
> + d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> + for_each_vcpu( d, v )
> + {
> + free_compat_arg_xlat(v);
> + release_compat_l4(v);
> + }
> +
> + return rc;
> +}
> +
> +static int pv_create_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
> +{
> + return create_perdomain_mapping(d, GDT_VIRT_START(v),
> + 1 << GDT_LDT_VCPU_SHIFT,
This should be 1u, when introduced in patch 1.
> + d->arch.pv_domain.gdt_ldt_l1tab, NULL);
> +}
> +
> +static void pv_destroy_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
> +{
> + destroy_perdomain_mapping(d, GDT_VIRT_START(v), 1 << GDT_LDT_VCPU_SHIFT);
> +}
> +
> +void pv_vcpu_destroy(struct vcpu *v)
> +{
> + if ( is_pv_32bit_vcpu(v) )
> + {
> + free_compat_arg_xlat(v);
> + release_compat_l4(v);
> + }
> +
> + pv_destroy_gdt_ldt_l1tab(v->domain, v);
> + xfree(v->arch.pv_vcpu.trap_ctxt);
> + v->arch.pv_vcpu.trap_ctxt = NULL;
> +}
> +
> +int pv_vcpu_initialise(struct vcpu *v)
> +{
> + struct domain *d = v->domain;
> + int rc;
> +
> + ASSERT(!is_idle_domain(d));
> +
> + spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
> +
> + rc = pv_create_gdt_ldt_l1tab(d, v);
> + if ( rc )
> + goto done;
> +
> + BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
> + PAGE_SIZE);
> + v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
> + NR_VECTORS);
> + if ( !v->arch.pv_vcpu.trap_ctxt )
> + {
> + rc = -ENOMEM;
> + goto done;
> + }
> +
> + /* PV guests by default have a 100Hz ticker. */
> + v->periodic_period = MILLISECS(10);
> +
> + v->arch.pv_vcpu.ctrlreg[4] = real_cr4_to_pv_guest_cr4(mmu_cr4_features);
> +
> + if ( is_pv_32bit_domain(d) )
> + {
> + if ( (rc = setup_compat_arg_xlat(v)) )
> + goto done;
> +
> + if ( (rc = setup_compat_l4(v)) )
> + goto done;
> + }
Now the code is split apart like this, this construct looks suspicious.
I suppose it probably copes with the toolstack using
XEN_DOMCTL_set_address_size and XEN_DOMCTL_max_vcpus in either order.
> +
> + done:
> + if ( rc )
> + pv_vcpu_destroy(v);
> + return rc;
> +}
> +
> +void pv_domain_destroy(struct domain *d)
> +{
> + destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
> + GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
> + xfree(d->arch.pv_domain.cpuidmasks);
> + d->arch.pv_domain.cpuidmasks = NULL;
> + free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> + d->arch.pv_domain.gdt_ldt_l1tab = NULL;
> +}
> +
> +
> +extern void paravirt_ctxt_switch_from(struct vcpu *v);
> +extern void paravirt_ctxt_switch_to(struct vcpu *v);
> +
> +int pv_domain_initialise(struct domain *d, unsigned int domcr_flags,
> + struct xen_arch_domainconfig *config)
> +{
> + static const struct arch_csw pv_csw = {
> + .from = paravirt_ctxt_switch_from,
> + .to = paravirt_ctxt_switch_to,
> + .tail = continue_nonidle_domain,
> + };
> + int rc = -ENOMEM;
> +
> + d->arch.pv_domain.gdt_ldt_l1tab =
> + alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
> + if ( !d->arch.pv_domain.gdt_ldt_l1tab )
> + goto fail;
> + clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
> +
> + if ( levelling_caps & ~LCAP_faulting )
> + {
> + d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
> + if ( !d->arch.pv_domain.cpuidmasks )
> + goto fail;
> + *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
> + }
> +
> + rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
> + GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
> + NULL, NULL);
> + if ( rc )
> + goto fail;
> +
> + d->arch.ctxt_switch = &pv_csw;
> +
> + /* 64-bit PV guest by default. */
> + d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> +
> + return 0;
> +
> + fail:
> + pv_domain_destroy(d);
> +
> + return rc;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-x86/pv/domain.h b/xen/include/asm-x86/pv/domain.h
> new file mode 100644
> index 0000000000..6288ae24df
> --- /dev/null
> +++ b/xen/include/asm-x86/pv/domain.h
> @@ -0,0 +1,30 @@
> +/*
> + * pv/domain.h
> + *
> + * PV guest interface definitions
> + *
> + * Copyright (C) 2017 Wei Liu <wei.liu2@citrix.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __X86_PV_DOMAIN_H__
> +#define __X86_PV_DOMAIN_H__
> +
#ifdef CONFIG_PV
> +void pv_vcpu_destroy(struct vcpu *v);
> +int pv_vcpu_initialise(struct vcpu *v);
> +void pv_domain_destroy(struct domain *d);
> +int pv_domain_initialise(struct domain *d, unsigned int domcr_flags,
> + struct xen_arch_domainconfig *config);
#else
static inline void pv_vcpu_destroy(struct vcpu *v) {};
static inline int pv_vcpu_initialise(struct vcpu *v) { return
-EOPNOTSUPP; };
static inline void pv_domain_destroy(struct domain *d) {};
static inline int pv_domain_initialise(struct domain *d, unsigned int
domcr_flags,
struct xen_arch_domainconfig
*config) { return -EOPNOTSUPP; }
#endif
Please can we try to make new code compatible with eventually turning
off CONFIG_PV and CONFIG_HVM.
~Andrew
> +
> +#endif /* __X86_PV_DOMAIN_H__ */
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 10/10] x86/domain: move HVM specific code to hvm/domain.c
2017-04-25 13:52 ` [PATCH for-next v2 10/10] x86/domain: move HVM specific code to hvm/domain.c Wei Liu
@ 2017-04-25 14:56 ` Andrew Cooper
0 siblings, 0 replies; 43+ messages in thread
From: Andrew Cooper @ 2017-04-25 14:56 UTC (permalink / raw)
To: Wei Liu, Xen-devel
On 25/04/17 14:52, Wei Liu wrote:
> diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> new file mode 100644
> index 0000000000..dd03cffd0b
> --- /dev/null
> +++ b/xen/arch/x86/hvm/domain.c
> @@ -0,0 +1,326 @@
> +/*
> + * HVM domain specific functions.
> + *
> + * Copyright (C) 2017 Citrix Systems R&D
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/domain_page.h>
> +#include <xen/errno.h>
> +#include <xen/lib.h>
> +#include <xen/paging.h>
> +#include <xen/sched.h>
> +
> +#include <public/hvm/hvm_vcpu.h>
> +
> +static inline int check_segment(struct segment_register *reg,
The inline can be dropped. This isn't a header file.
Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c
2017-04-25 14:52 ` Andrew Cooper
@ 2017-04-25 15:07 ` Wei Liu
2017-04-26 6:04 ` Jan Beulich
` (2 subsequent siblings)
3 siblings, 0 replies; 43+ messages in thread
From: Wei Liu @ 2017-04-25 15:07 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Jan Beulich
On Tue, Apr 25, 2017 at 03:52:06PM +0100, Andrew Cooper wrote:
> On 25/04/17 14:52, Wei Liu wrote:
> > - fail:
> > - pv_domain_destroy(d);
> > -
> > - return rc;
> > -}
> > -
> > +void paravirt_ctxt_switch_from(struct vcpu *v);
> > +void paravirt_ctxt_switch_to(struct vcpu *v);
> > int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> > struct xen_arch_domainconfig *config)
> > {
> > @@ -1919,7 +1717,8 @@ static void save_segments(struct vcpu *v)
> >
> > #define switch_kernel_stack(v) ((void)0)
> >
> > -static void paravirt_ctxt_switch_from(struct vcpu *v)
> > +/* Needed by PV guests */
> > +void paravirt_ctxt_switch_from(struct vcpu *v)
> > {
>
> Could these be moved up to avoid the forward declarations above?
>
Yes and frankly this is going to require more brain power to review, but
I'm not the one who reviews this so I don't care. ;p
> > diff --git a/xen/arch/x86/pv/Makefile b/xen/arch/x86/pv/Makefile
> > index ea94599438..2737824e81 100644
> > --- a/xen/arch/x86/pv/Makefile
> > +++ b/xen/arch/x86/pv/Makefile
> > @@ -1,2 +1,3 @@
> > obj-y += hypercall.o
> > obj-bin-y += dom0_build.init.o
> > +obj-y += domain.o
> > diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> > new file mode 100644
> > index 0000000000..562c3d03f5
> > --- /dev/null
> > +++ b/xen/arch/x86/pv/domain.c
> > @@ -0,0 +1,232 @@
[...]
> > +
> > +static int pv_create_gdt_ldt_l1tab(struct domain *d, struct vcpu *v)
> > +{
> > + return create_perdomain_mapping(d, GDT_VIRT_START(v),
> > + 1 << GDT_LDT_VCPU_SHIFT,
>
> This should be 1u, when introduced in patch 1.
>
Will fix.
As for other issues you point out, it is rather easier to review and
test if I write separate patches for all of them. Expect more patches.
> > +
>
> #ifdef CONFIG_PV
>
> > +void pv_vcpu_destroy(struct vcpu *v);
> > +int pv_vcpu_initialise(struct vcpu *v);
> > +void pv_domain_destroy(struct domain *d);
> > +int pv_domain_initialise(struct domain *d, unsigned int domcr_flags,
> > + struct xen_arch_domainconfig *config);
>
> #else
>
> static inline void pv_vcpu_destroy(struct vcpu *v) {};
> static inline int pv_vcpu_initialise(struct vcpu *v) { return
> -EOPNOTSUPP; };
> static inline void pv_domain_destroy(struct domain *d) {};
> static inline int pv_domain_initialise(struct domain *d, unsigned int
> domcr_flags,
> struct xen_arch_domainconfig
> *config) { return -EOPNOTSUPP; }
>
> #endif
>
> Please can we try to make new code compatible with eventually turning
> off CONFIG_PV and CONFIG_HVM.
>
My original plan was to do that in next stage. But I'm also ok with
doing it now.
Wei.
> ~Andrew
>
> > +
> > +#endif /* __X86_PV_DOMAIN_H__ */
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c
2017-04-25 14:52 ` Andrew Cooper
2017-04-25 15:07 ` Wei Liu
@ 2017-04-26 6:04 ` Jan Beulich
2017-04-26 12:38 ` Andrew Cooper
2017-04-26 12:39 ` Jan Beulich
2017-04-26 15:46 ` Wei Liu
3 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2017-04-26 6:04 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu
>>> On 25.04.17 at 16:52, <andrew.cooper3@citrix.com> wrote:
> On 25/04/17 14:52, Wei Liu wrote:
>> +
>> + for_each_vcpu( d, v )
>> + {
>> + rc = setup_compat_arg_xlat(v);
>> + if ( !rc )
>> + rc = setup_compat_l4(v);
>> +
>> + if ( rc )
>> + goto undo_and_fail;
>
> This is odd structuring. Care to rearrange it with two goto's, rather
> than inverting the first rc check?
I don't see anything odd about it - just like with preferably limiting
the number of return statements, I think limiting the number of
goto-s is quite desirable. What if the second if()'s body had more
than just a goto? I'd certainly prefer the code structure above in
that case over _adding_ a goto into this second if(). Further down
the same two functions are being called, pointlessly using two
goto-s. If you really wanted to get rid of the inverted first
condition, then how about if ( (rc = ...) || (rc = ...) ) ?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 04/10] x86/domain: factor out pv_vcpu_destroy
2017-04-25 13:52 ` [PATCH for-next v2 04/10] x86/domain: factor out pv_vcpu_destroy Wei Liu
2017-04-25 13:58 ` Andrew Cooper
@ 2017-04-26 12:21 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2017-04-26 12:21 UTC (permalink / raw)
To: Wei Liu; +Cc: AndrewCooper, Xen-devel
>>> On 25.04.17 at 15:52, <wei.liu2@citrix.com> wrote:
> The function is made idempotent on purpose.
It may be worth mentioning that free_compat_arg_xlat() already is,
as that's neither obvious nor the result of any earlier patch in this
series.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise
2017-04-25 13:52 ` [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise Wei Liu
2017-04-25 14:08 ` Andrew Cooper
@ 2017-04-26 12:25 ` Jan Beulich
2017-04-26 12:53 ` Wei Liu
1 sibling, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2017-04-26 12:25 UTC (permalink / raw)
To: Wei Liu; +Cc: AndrewCooper, Xen-devel
>>> On 25.04.17 at 15:52, <wei.liu2@citrix.com> wrote:
> +static int pv_vcpu_initialise(struct vcpu *v)
> +{
> + struct domain *d = v->domain;
> + int rc;
> +
> + ASSERT(!is_idle_domain(d));
> +
> + spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
> +
> + rc = pv_create_gdt_ldt_l1tab(d, v);
> + if ( rc )
> + goto done;
You may simply return here.
> @@ -424,61 +468,18 @@ int vcpu_initialise(struct vcpu *v)
> spin_lock_init(&v->arch.vpmu.vpmu_lock);
>
> if ( is_hvm_domain(d) )
> - {
> rc = hvm_vcpu_initialise(v);
> - goto done;
> - }
> -
> -
> - spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
> -
> - if ( !is_idle_domain(d) )
> - {
> - rc = pv_create_gdt_ldt_l1tab(d, v);
> - if ( rc )
> - goto done;
> -
> - BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
> - PAGE_SIZE);
> - v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
> - NR_VECTORS);
> - if ( !v->arch.pv_vcpu.trap_ctxt )
> - {
> - pv_destroy_gdt_ldt_l1tab(d, v);
> - rc = -ENOMEM;
> - goto done;
> - }
> -
> - /* PV guests by default have a 100Hz ticker. */
> - v->periodic_period = MILLISECS(10);
> - }
> + else if ( is_pv_domain(d) && !is_idle_domain(d) )
Only the right side of the && is needed, as is_pv is now (again) the
opposite of is_hvm.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 07/10] x86/domain: factor out pv_domain_destroy
2017-04-25 13:52 ` [PATCH for-next v2 07/10] x86/domain: factor out pv_domain_destroy Wei Liu
2017-04-25 14:18 ` Andrew Cooper
@ 2017-04-26 12:32 ` Jan Beulich
2017-04-26 12:56 ` Wei Liu
1 sibling, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2017-04-26 12:32 UTC (permalink / raw)
To: Wei Liu; +Cc: AndrewCooper, Xen-devel
>>> On 25.04.17 at 15:52, <wei.liu2@citrix.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -536,6 +536,16 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> return true;
> }
>
> +static void pv_domain_destroy(struct domain *d)
> +{
> + destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
> + GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
What use is this, considering both calling paths have ...
> @@ -712,10 +722,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> paging_final_teardown(d);
> free_perdomain_mappings(d);
... this and ...
> if ( is_pv_domain(d) )
> - {
> - xfree(d->arch.pv_domain.cpuidmasks);
> - free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> - }
> + pv_domain_destroy(d);
> +
> return rc;
> }
>
> @@ -735,10 +743,7 @@ void arch_domain_destroy(struct domain *d)
>
> free_perdomain_mappings(d);
... this?
Jan
> if ( is_pv_domain(d) )
> - {
> - free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> - xfree(d->arch.pv_domain.cpuidmasks);
> - }
> + pv_domain_destroy(d);
>
> free_xenheap_page(d->shared_info);
> cleanup_domain_irq_mapping(d);
> --
> 2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c
2017-04-26 6:04 ` Jan Beulich
@ 2017-04-26 12:38 ` Andrew Cooper
0 siblings, 0 replies; 43+ messages in thread
From: Andrew Cooper @ 2017-04-26 12:38 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel, Wei Liu
On 26/04/17 07:04, Jan Beulich wrote:
>>>> On 25.04.17 at 16:52, <andrew.cooper3@citrix.com> wrote:
>> On 25/04/17 14:52, Wei Liu wrote:
>>> +
>>> + for_each_vcpu( d, v )
>>> + {
>>> + rc = setup_compat_arg_xlat(v);
>>> + if ( !rc )
>>> + rc = setup_compat_l4(v);
>>> +
>>> + if ( rc )
>>> + goto undo_and_fail;
>> This is odd structuring. Care to rearrange it with two goto's, rather
>> than inverting the first rc check?
> I don't see anything odd about it - just like with preferably limiting
> the number of return statements, I think limiting the number of
> goto-s is quite desirable. What if the second if()'s body had more
> than just a goto? I'd certainly prefer the code structure above in
> that case over _adding_ a goto into this second if(). Further down
> the same two functions are being called, pointlessly using two
> goto-s. If you really wanted to get rid of the inverted first
> condition, then how about if ( (rc = ...) || (rc = ...) ) ?
For functions which have standard rc semantics, you can actually chain
them together like:
rc = setup_compat_arg_xlat(v) ?: setup_compat_l4(v) ?: ... ;
which will break at the first non-zero return in the chain, and allows
you to have a single assignment and single goto.
Whether this style is sensible to follow is a different matter, but it
certainly is compact.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c
2017-04-25 14:52 ` Andrew Cooper
2017-04-25 15:07 ` Wei Liu
2017-04-26 6:04 ` Jan Beulich
@ 2017-04-26 12:39 ` Jan Beulich
2017-04-26 12:46 ` Andrew Cooper
2017-04-26 15:46 ` Wei Liu
3 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2017-04-26 12:39 UTC (permalink / raw)
To: Andrew Cooper, Wei Liu; +Cc: Xen-devel
>>> On 25.04.17 at 16:52, <andrew.cooper3@citrix.com> wrote:
> On 25/04/17 14:52, Wei Liu wrote:
>> - fail:
>> - pv_domain_destroy(d);
>> -
>> - return rc;
>> -}
>> -
>> +void paravirt_ctxt_switch_from(struct vcpu *v);
>> +void paravirt_ctxt_switch_to(struct vcpu *v);
>> int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>> struct xen_arch_domainconfig *config)
>> {
>> @@ -1919,7 +1717,8 @@ static void save_segments(struct vcpu *v)
>>
>> #define switch_kernel_stack(v) ((void)0)
>>
>> -static void paravirt_ctxt_switch_from(struct vcpu *v)
>> +/* Needed by PV guests */
>> +void paravirt_ctxt_switch_from(struct vcpu *v)
>> {
>
> Could these be moved up to avoid the forward declarations above?
Moved up? I don't see why they're not simply being moved to
pv/domain.c and kept static there (suitably placed so that the
forward declarations don't need to be retained). As their names
say, they're very PV-specific...
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c
2017-04-26 12:39 ` Jan Beulich
@ 2017-04-26 12:46 ` Andrew Cooper
2017-04-26 13:00 ` Wei Liu
2017-04-26 13:26 ` Jan Beulich
0 siblings, 2 replies; 43+ messages in thread
From: Andrew Cooper @ 2017-04-26 12:46 UTC (permalink / raw)
To: Jan Beulich, Wei Liu; +Cc: Xen-devel
On 26/04/17 13:39, Jan Beulich wrote:
>>>> On 25.04.17 at 16:52, <andrew.cooper3@citrix.com> wrote:
>> On 25/04/17 14:52, Wei Liu wrote:
>>> - fail:
>>> - pv_domain_destroy(d);
>>> -
>>> - return rc;
>>> -}
>>> -
>>> +void paravirt_ctxt_switch_from(struct vcpu *v);
>>> +void paravirt_ctxt_switch_to(struct vcpu *v);
>>> int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>> struct xen_arch_domainconfig *config)
>>> {
>>> @@ -1919,7 +1717,8 @@ static void save_segments(struct vcpu *v)
>>>
>>> #define switch_kernel_stack(v) ((void)0)
>>>
>>> -static void paravirt_ctxt_switch_from(struct vcpu *v)
>>> +/* Needed by PV guests */
>>> +void paravirt_ctxt_switch_from(struct vcpu *v)
>>> {
>> Could these be moved up to avoid the forward declarations above?
> Moved up? I don't see why they're not simply being moved to
> pv/domain.c and kept static there (suitably placed so that the
> forward declarations don't need to be retained). As their names
> say, they're very PV-specific...
They are also used by idle_csw, whose setup remains in this file.
To sensibly compile without CONFIG_PV in the future, these either need
to remain common, or we split the idle vcpu routines away from the PV ones.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise
2017-04-26 12:25 ` Jan Beulich
@ 2017-04-26 12:53 ` Wei Liu
2017-04-26 13:27 ` Jan Beulich
0 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2017-04-26 12:53 UTC (permalink / raw)
To: Jan Beulich; +Cc: AndrewCooper, Wei Liu, Xen-devel
On Wed, Apr 26, 2017 at 06:25:32AM -0600, Jan Beulich wrote:
> > if ( is_hvm_domain(d) )
> > - {
> > rc = hvm_vcpu_initialise(v);
> > - goto done;
> > - }
> > -
> > -
> > - spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
> > -
> > - if ( !is_idle_domain(d) )
> > - {
> > - rc = pv_create_gdt_ldt_l1tab(d, v);
> > - if ( rc )
> > - goto done;
> > -
> > - BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
> > - PAGE_SIZE);
> > - v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
> > - NR_VECTORS);
> > - if ( !v->arch.pv_vcpu.trap_ctxt )
> > - {
> > - pv_destroy_gdt_ldt_l1tab(d, v);
> > - rc = -ENOMEM;
> > - goto done;
> > - }
> > -
> > - /* PV guests by default have a 100Hz ticker. */
> > - v->periodic_period = MILLISECS(10);
> > - }
> > + else if ( is_pv_domain(d) && !is_idle_domain(d) )
>
> Only the right side of the && is needed, as is_pv is now (again) the
> opposite of is_hvm.
>
The idle domain is a PV guest (see domain_create) and handled in the
following else branch.
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 07/10] x86/domain: factor out pv_domain_destroy
2017-04-26 12:32 ` Jan Beulich
@ 2017-04-26 12:56 ` Wei Liu
2017-04-26 13:29 ` Jan Beulich
0 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2017-04-26 12:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: AndrewCooper, Wei Liu, Xen-devel
On Wed, Apr 26, 2017 at 06:32:46AM -0600, Jan Beulich wrote:
> >>> On 25.04.17 at 15:52, <wei.liu2@citrix.com> wrote:
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -536,6 +536,16 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> > return true;
> > }
> >
> > +static void pv_domain_destroy(struct domain *d)
> > +{
> > + destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
> > + GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
>
> What use is this, considering both calling paths have ...
>
> > @@ -712,10 +722,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> > paging_final_teardown(d);
> > free_perdomain_mappings(d);
>
> ... this and ...
>
> > if ( is_pv_domain(d) )
> > - {
> > - xfree(d->arch.pv_domain.cpuidmasks);
> > - free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> > - }
> > + pv_domain_destroy(d);
> > +
> > return rc;
> > }
> >
> > @@ -735,10 +743,7 @@ void arch_domain_destroy(struct domain *d)
> >
> > free_perdomain_mappings(d);
>
> ... this?
>
Yes, I know. But IMO it would be far clearer we have matching allocation
and deallocation.
We still need free_perdomain_mappings to have it free the top-level page
table.
Wei.
> Jan
>
> > if ( is_pv_domain(d) )
> > - {
> > - free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> > - xfree(d->arch.pv_domain.cpuidmasks);
> > - }
> > + pv_domain_destroy(d);
> >
> > free_xenheap_page(d->shared_info);
> > cleanup_domain_irq_mapping(d);
> > --
> > 2.11.0
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c
2017-04-26 12:46 ` Andrew Cooper
@ 2017-04-26 13:00 ` Wei Liu
2017-04-26 13:26 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Wei Liu @ 2017-04-26 13:00 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Jan Beulich
On Wed, Apr 26, 2017 at 01:46:53PM +0100, Andrew Cooper wrote:
> On 26/04/17 13:39, Jan Beulich wrote:
> >>>> On 25.04.17 at 16:52, <andrew.cooper3@citrix.com> wrote:
> >> On 25/04/17 14:52, Wei Liu wrote:
> >>> - fail:
> >>> - pv_domain_destroy(d);
> >>> -
> >>> - return rc;
> >>> -}
> >>> -
> >>> +void paravirt_ctxt_switch_from(struct vcpu *v);
> >>> +void paravirt_ctxt_switch_to(struct vcpu *v);
> >>> int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> >>> struct xen_arch_domainconfig *config)
> >>> {
> >>> @@ -1919,7 +1717,8 @@ static void save_segments(struct vcpu *v)
> >>>
> >>> #define switch_kernel_stack(v) ((void)0)
> >>>
> >>> -static void paravirt_ctxt_switch_from(struct vcpu *v)
> >>> +/* Needed by PV guests */
> >>> +void paravirt_ctxt_switch_from(struct vcpu *v)
> >>> {
> >> Could these be moved up to avoid the forward declarations above?
> > Moved up? I don't see why they're not simply being moved to
> > pv/domain.c and kept static there (suitably placed so that the
> > forward declarations don't need to be retained). As their names
> > say, they're very PV-specific...
>
> They are also used by idle_csw, whose setup remains in this file.
>
> To sensibly compile without CONFIG_PV in the future, these either need
> to remain common, or we split the idle vcpu routines away from the PV ones.
>
I am inclined to split out idle domain routines in the future --
arguably it is going to be of low priority.
Wei.
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c
2017-04-26 12:46 ` Andrew Cooper
2017-04-26 13:00 ` Wei Liu
@ 2017-04-26 13:26 ` Jan Beulich
2017-04-26 13:32 ` Wei Liu
1 sibling, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2017-04-26 13:26 UTC (permalink / raw)
To: Andrew Cooper, Wei Liu; +Cc: Xen-devel
>>> On 26.04.17 at 14:46, <andrew.cooper3@citrix.com> wrote:
> On 26/04/17 13:39, Jan Beulich wrote:
>>>>> On 25.04.17 at 16:52, <andrew.cooper3@citrix.com> wrote:
>>> On 25/04/17 14:52, Wei Liu wrote:
>>>> - fail:
>>>> - pv_domain_destroy(d);
>>>> -
>>>> - return rc;
>>>> -}
>>>> -
>>>> +void paravirt_ctxt_switch_from(struct vcpu *v);
>>>> +void paravirt_ctxt_switch_to(struct vcpu *v);
>>>> int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>>> struct xen_arch_domainconfig *config)
>>>> {
>>>> @@ -1919,7 +1717,8 @@ static void save_segments(struct vcpu *v)
>>>>
>>>> #define switch_kernel_stack(v) ((void)0)
>>>>
>>>> -static void paravirt_ctxt_switch_from(struct vcpu *v)
>>>> +/* Needed by PV guests */
>>>> +void paravirt_ctxt_switch_from(struct vcpu *v)
>>>> {
>>> Could these be moved up to avoid the forward declarations above?
>> Moved up? I don't see why they're not simply being moved to
>> pv/domain.c and kept static there (suitably placed so that the
>> forward declarations don't need to be retained). As their names
>> say, they're very PV-specific...
>
> They are also used by idle_csw, whose setup remains in this file.
Oh, right. But then their declarations should move into a header,
instead of being done in two(!) source files. That'll then also
eliminate any ordering constraints.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise
2017-04-26 12:53 ` Wei Liu
@ 2017-04-26 13:27 ` Jan Beulich
2017-04-26 13:35 ` Wei Liu
0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2017-04-26 13:27 UTC (permalink / raw)
To: Wei Liu; +Cc: AndrewCooper, Xen-devel
>>> On 26.04.17 at 14:53, <wei.liu2@citrix.com> wrote:
> On Wed, Apr 26, 2017 at 06:25:32AM -0600, Jan Beulich wrote:
>> > if ( is_hvm_domain(d) )
>> > - {
>> > rc = hvm_vcpu_initialise(v);
>> > - goto done;
>> > - }
>> > -
>> > -
>> > - spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
>> > -
>> > - if ( !is_idle_domain(d) )
>> > - {
>> > - rc = pv_create_gdt_ldt_l1tab(d, v);
>> > - if ( rc )
>> > - goto done;
>> > -
>> > - BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
>> > - PAGE_SIZE);
>> > - v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
>> > - NR_VECTORS);
>> > - if ( !v->arch.pv_vcpu.trap_ctxt )
>> > - {
>> > - pv_destroy_gdt_ldt_l1tab(d, v);
>> > - rc = -ENOMEM;
>> > - goto done;
>> > - }
>> > -
>> > - /* PV guests by default have a 100Hz ticker. */
>> > - v->periodic_period = MILLISECS(10);
>> > - }
>> > + else if ( is_pv_domain(d) && !is_idle_domain(d) )
>>
>> Only the right side of the && is needed, as is_pv is now (again) the
>> opposite of is_hvm.
>>
>
> The idle domain is a PV guest (see domain_create) and handled in the
> following else branch.
Exactly, so all you need here is "else if ( !is_idle_domain(d) )".
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 07/10] x86/domain: factor out pv_domain_destroy
2017-04-26 12:56 ` Wei Liu
@ 2017-04-26 13:29 ` Jan Beulich
2017-04-26 13:36 ` Wei Liu
0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2017-04-26 13:29 UTC (permalink / raw)
To: Wei Liu; +Cc: AndrewCooper, Xen-devel
>>> On 26.04.17 at 14:56, <wei.liu2@citrix.com> wrote:
> On Wed, Apr 26, 2017 at 06:32:46AM -0600, Jan Beulich wrote:
>> >>> On 25.04.17 at 15:52, <wei.liu2@citrix.com> wrote:
>> > --- a/xen/arch/x86/domain.c
>> > +++ b/xen/arch/x86/domain.c
>> > @@ -536,6 +536,16 @@ static bool emulation_flags_ok(const struct domain *d,
> uint32_t emflags)
>> > return true;
>> > }
>> >
>> > +static void pv_domain_destroy(struct domain *d)
>> > +{
>> > + destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
>> > + GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
>>
>> What use is this, considering both calling paths have ...
>>
>> > @@ -712,10 +722,8 @@ int arch_domain_create(struct domain *d, unsigned int
> domcr_flags,
>> > paging_final_teardown(d);
>> > free_perdomain_mappings(d);
>>
>> ... this and ...
>>
>> > if ( is_pv_domain(d) )
>> > - {
>> > - xfree(d->arch.pv_domain.cpuidmasks);
>> > - free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
>> > - }
>> > + pv_domain_destroy(d);
>> > +
>> > return rc;
>> > }
>> >
>> > @@ -735,10 +743,7 @@ void arch_domain_destroy(struct domain *d)
>> >
>> > free_perdomain_mappings(d);
>>
>> ... this?
>>
>
> Yes, I know. But IMO it would be far clearer we have matching allocation
> and deallocation.
Well, in the case here it's really pointless, but if want to keep it I
don't mind as long as ...
> We still need free_perdomain_mappings to have it free the top-level page
> table.
... you then at least switch their order, so that
free_perdomain_mappings() happens last.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c
2017-04-26 13:26 ` Jan Beulich
@ 2017-04-26 13:32 ` Wei Liu
2017-04-26 14:12 ` Jan Beulich
0 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2017-04-26 13:32 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, Xen-devel
On Wed, Apr 26, 2017 at 07:26:29AM -0600, Jan Beulich wrote:
> >>> On 26.04.17 at 14:46, <andrew.cooper3@citrix.com> wrote:
> > On 26/04/17 13:39, Jan Beulich wrote:
> >>>>> On 25.04.17 at 16:52, <andrew.cooper3@citrix.com> wrote:
> >>> On 25/04/17 14:52, Wei Liu wrote:
> >>>> - fail:
> >>>> - pv_domain_destroy(d);
> >>>> -
> >>>> - return rc;
> >>>> -}
> >>>> -
> >>>> +void paravirt_ctxt_switch_from(struct vcpu *v);
> >>>> +void paravirt_ctxt_switch_to(struct vcpu *v);
> >>>> int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> >>>> struct xen_arch_domainconfig *config)
> >>>> {
> >>>> @@ -1919,7 +1717,8 @@ static void save_segments(struct vcpu *v)
> >>>>
> >>>> #define switch_kernel_stack(v) ((void)0)
> >>>>
> >>>> -static void paravirt_ctxt_switch_from(struct vcpu *v)
> >>>> +/* Needed by PV guests */
> >>>> +void paravirt_ctxt_switch_from(struct vcpu *v)
> >>>> {
> >>> Could these be moved up to avoid the forward declarations above?
> >> Moved up? I don't see why they're not simply being moved to
> >> pv/domain.c and kept static there (suitably placed so that the
> >> forward declarations don't need to be retained). As their names
> >> say, they're very PV-specific...
> >
> > They are also used by idle_csw, whose setup remains in this file.
>
> Oh, right. But then their declarations should move into a header,
> instead of being done in two(!) source files. That'll then also
> eliminate any ordering constraints.
>
That was how it was done in v1. But I opted to not do that in v2 because
I had a long term plan to split out idle domain routines. It is rather
easy to accumulate kludge in header files like that.
But in the end I don't care to argue for this. If that's how you want it
to be done, then I'm fine with it too.
Wei.
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise
2017-04-26 13:27 ` Jan Beulich
@ 2017-04-26 13:35 ` Wei Liu
0 siblings, 0 replies; 43+ messages in thread
From: Wei Liu @ 2017-04-26 13:35 UTC (permalink / raw)
To: Jan Beulich; +Cc: AndrewCooper, Wei Liu, Xen-devel
On Wed, Apr 26, 2017 at 07:27:23AM -0600, Jan Beulich wrote:
> >>> On 26.04.17 at 14:53, <wei.liu2@citrix.com> wrote:
> > On Wed, Apr 26, 2017 at 06:25:32AM -0600, Jan Beulich wrote:
> >> > if ( is_hvm_domain(d) )
> >> > - {
> >> > rc = hvm_vcpu_initialise(v);
> >> > - goto done;
> >> > - }
> >> > -
> >> > -
> >> > - spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
> >> > -
> >> > - if ( !is_idle_domain(d) )
> >> > - {
> >> > - rc = pv_create_gdt_ldt_l1tab(d, v);
> >> > - if ( rc )
> >> > - goto done;
> >> > -
> >> > - BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
> >> > - PAGE_SIZE);
> >> > - v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
> >> > - NR_VECTORS);
> >> > - if ( !v->arch.pv_vcpu.trap_ctxt )
> >> > - {
> >> > - pv_destroy_gdt_ldt_l1tab(d, v);
> >> > - rc = -ENOMEM;
> >> > - goto done;
> >> > - }
> >> > -
> >> > - /* PV guests by default have a 100Hz ticker. */
> >> > - v->periodic_period = MILLISECS(10);
> >> > - }
> >> > + else if ( is_pv_domain(d) && !is_idle_domain(d) )
> >>
> >> Only the right side of the && is needed, as is_pv is now (again) the
> >> opposite of is_hvm.
> >>
> >
> > The idle domain is a PV guest (see domain_create) and handled in the
> > following else branch.
>
> Exactly, so all you need here is "else if ( !is_idle_domain(d) )".
>
Yes you're right. I misread your comment, sorry.
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 07/10] x86/domain: factor out pv_domain_destroy
2017-04-26 13:29 ` Jan Beulich
@ 2017-04-26 13:36 ` Wei Liu
0 siblings, 0 replies; 43+ messages in thread
From: Wei Liu @ 2017-04-26 13:36 UTC (permalink / raw)
To: Jan Beulich; +Cc: AndrewCooper, Wei Liu, Xen-devel
On Wed, Apr 26, 2017 at 07:29:34AM -0600, Jan Beulich wrote:
> >>> On 26.04.17 at 14:56, <wei.liu2@citrix.com> wrote:
> > On Wed, Apr 26, 2017 at 06:32:46AM -0600, Jan Beulich wrote:
> >> >>> On 25.04.17 at 15:52, <wei.liu2@citrix.com> wrote:
> >> > --- a/xen/arch/x86/domain.c
> >> > +++ b/xen/arch/x86/domain.c
> >> > @@ -536,6 +536,16 @@ static bool emulation_flags_ok(const struct domain *d,
> > uint32_t emflags)
> >> > return true;
> >> > }
> >> >
> >> > +static void pv_domain_destroy(struct domain *d)
> >> > +{
> >> > + destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
> >> > + GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
> >>
> >> What use is this, considering both calling paths have ...
> >>
> >> > @@ -712,10 +722,8 @@ int arch_domain_create(struct domain *d, unsigned int
> > domcr_flags,
> >> > paging_final_teardown(d);
> >> > free_perdomain_mappings(d);
> >>
> >> ... this and ...
> >>
> >> > if ( is_pv_domain(d) )
> >> > - {
> >> > - xfree(d->arch.pv_domain.cpuidmasks);
> >> > - free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> >> > - }
> >> > + pv_domain_destroy(d);
> >> > +
> >> > return rc;
> >> > }
> >> >
> >> > @@ -735,10 +743,7 @@ void arch_domain_destroy(struct domain *d)
> >> >
> >> > free_perdomain_mappings(d);
> >>
> >> ... this?
> >>
> >
> > Yes, I know. But IMO it would be far clearer we have matching allocation
> > and deallocation.
>
> Well, in the case here it's really pointless, but if want to keep it I
> don't mind as long as ...
>
> > We still need free_perdomain_mappings to have it free the top-level page
> > table.
>
> ... you then at least switch their order, so that
> free_perdomain_mappings() happens last.
>
OK. This is a good point.
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c
2017-04-26 13:32 ` Wei Liu
@ 2017-04-26 14:12 ` Jan Beulich
0 siblings, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2017-04-26 14:12 UTC (permalink / raw)
To: Wei Liu; +Cc: Andrew Cooper, Xen-devel
>>> On 26.04.17 at 15:32, <wei.liu2@citrix.com> wrote:
> On Wed, Apr 26, 2017 at 07:26:29AM -0600, Jan Beulich wrote:
>> >>> On 26.04.17 at 14:46, <andrew.cooper3@citrix.com> wrote:
>> > On 26/04/17 13:39, Jan Beulich wrote:
>> >>>>> On 25.04.17 at 16:52, <andrew.cooper3@citrix.com> wrote:
>> >>> On 25/04/17 14:52, Wei Liu wrote:
>> >>>> - fail:
>> >>>> - pv_domain_destroy(d);
>> >>>> -
>> >>>> - return rc;
>> >>>> -}
>> >>>> -
>> >>>> +void paravirt_ctxt_switch_from(struct vcpu *v);
>> >>>> +void paravirt_ctxt_switch_to(struct vcpu *v);
>> >>>> int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>> >>>> struct xen_arch_domainconfig *config)
>> >>>> {
>> >>>> @@ -1919,7 +1717,8 @@ static void save_segments(struct vcpu *v)
>> >>>>
>> >>>> #define switch_kernel_stack(v) ((void)0)
>> >>>>
>> >>>> -static void paravirt_ctxt_switch_from(struct vcpu *v)
>> >>>> +/* Needed by PV guests */
>> >>>> +void paravirt_ctxt_switch_from(struct vcpu *v)
>> >>>> {
>> >>> Could these be moved up to avoid the forward declarations above?
>> >> Moved up? I don't see why they're not simply being moved to
>> >> pv/domain.c and kept static there (suitably placed so that the
>> >> forward declarations don't need to be retained). As their names
>> >> say, they're very PV-specific...
>> >
>> > They are also used by idle_csw, whose setup remains in this file.
>>
>> Oh, right. But then their declarations should move into a header,
>> instead of being done in two(!) source files. That'll then also
>> eliminate any ordering constraints.
>
> That was how it was done in v1. But I opted to not do that in v2 because
> I had a long term plan to split out idle domain routines. It is rather
> easy to accumulate kludge in header files like that.
>
> But in the end I don't care to argue for this. If that's how you want it
> to be done, then I'm fine with it too.
The fundamental thing here is: Both producer and consumer(s) of
any symbol need to see the _same_ declaration. You can't make
sure this is the case without putting the declaration in a header.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c
2017-04-25 14:52 ` Andrew Cooper
` (2 preceding siblings ...)
2017-04-26 12:39 ` Jan Beulich
@ 2017-04-26 15:46 ` Wei Liu
3 siblings, 0 replies; 43+ messages in thread
From: Wei Liu @ 2017-04-26 15:46 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Jan Beulich
On Tue, Apr 25, 2017 at 03:52:06PM +0100, Andrew Cooper wrote:
> > + if ( is_pv_32bit_domain(d) )
> > + {
> > + if ( (rc = setup_compat_arg_xlat(v)) )
> > + goto done;
> > +
> > + if ( (rc = setup_compat_l4(v)) )
> > + goto done;
> > + }
>
> Now the code is split apart like this, this construct looks suspicious.
> I suppose it probably copes with the toolstack using
> XEN_DOMCTL_set_address_size and XEN_DOMCTL_max_vcpus in either order.
I don't know. We can't change this now because some toolstack may depend
on this particular behaviour.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2017-04-26 15:46 UTC | newest]
Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-25 13:52 [PATCH for-next v2 00/10] x86: refactor x86/domain.c Wei Liu
2017-04-25 13:52 ` [PATCH for-next v2 01/10] x86/mm: make free_perdomain_mappings idempotent Wei Liu
2017-04-25 13:54 ` Andrew Cooper
2017-04-25 13:52 ` [PATCH for-next v2 02/10] x86/domain: provide pv_{create, destroy}_gdt_ldt_l1tab and use them Wei Liu
2017-04-25 13:56 ` Andrew Cooper
2017-04-25 14:02 ` Wei Liu
2017-04-25 13:52 ` [PATCH for-next v2 03/10] x86/domain: make release_compact_l4 NULL tolerant Wei Liu
2017-04-25 13:56 ` Andrew Cooper
2017-04-25 13:52 ` [PATCH for-next v2 04/10] x86/domain: factor out pv_vcpu_destroy Wei Liu
2017-04-25 13:58 ` Andrew Cooper
2017-04-26 12:21 ` Jan Beulich
2017-04-25 13:52 ` [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise Wei Liu
2017-04-25 14:08 ` Andrew Cooper
2017-04-25 14:27 ` Wei Liu
2017-04-26 12:25 ` Jan Beulich
2017-04-26 12:53 ` Wei Liu
2017-04-26 13:27 ` Jan Beulich
2017-04-26 13:35 ` Wei Liu
2017-04-25 13:52 ` [PATCH for-next v2 06/10] x86/domain: push some code down to hvm_domain_initialise Wei Liu
2017-04-25 14:15 ` Andrew Cooper
2017-04-25 14:29 ` Wei Liu
2017-04-25 13:52 ` [PATCH for-next v2 07/10] x86/domain: factor out pv_domain_destroy Wei Liu
2017-04-25 14:18 ` Andrew Cooper
2017-04-26 12:32 ` Jan Beulich
2017-04-26 12:56 ` Wei Liu
2017-04-26 13:29 ` Jan Beulich
2017-04-26 13:36 ` Wei Liu
2017-04-25 13:52 ` [PATCH for-next v2 08/10] x86/domain: factor out pv_domain_initialise Wei Liu
2017-04-25 14:30 ` Andrew Cooper
2017-04-25 13:52 ` [PATCH for-next v2 09/10] x86/domain: move PV specific code to pv/domain.c Wei Liu
2017-04-25 14:52 ` Andrew Cooper
2017-04-25 15:07 ` Wei Liu
2017-04-26 6:04 ` Jan Beulich
2017-04-26 12:38 ` Andrew Cooper
2017-04-26 12:39 ` Jan Beulich
2017-04-26 12:46 ` Andrew Cooper
2017-04-26 13:00 ` Wei Liu
2017-04-26 13:26 ` Jan Beulich
2017-04-26 13:32 ` Wei Liu
2017-04-26 14:12 ` Jan Beulich
2017-04-26 15:46 ` Wei Liu
2017-04-25 13:52 ` [PATCH for-next v2 10/10] x86/domain: move HVM specific code to hvm/domain.c Wei Liu
2017-04-25 14:56 ` Andrew Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.