| Message ID | 20170410132716.31610-5-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/04/17 14:27, Wei Liu wrote: > We want to have a single entry point to initialise hvm guest. 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> Two observations... > --- > xen/arch/x86/domain.c | 11 ++--------- > xen/arch/x86/hvm/hvm.c | 25 +++++++++++++++++-------- > xen/include/asm-x86/hvm/hvm.h | 2 +- > 3 files changed, 20 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index ddebff6187..af060d8239 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -663,7 +656,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)) != 0 ) If we are pushing these values down (which I agree is a good idea), please can we push xen_arch_domainconfig down as well. > goto fail; > } > else > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index f50d15ff50..7fc49bb03d 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -615,10 +615,17 @@ int hvm_domain_initialise(struct domain *d) > > hvm_init_cacheattr_region_list(d); > > - rc = paging_enable(d, PG_refcounts|PG_translate|PG_external); > + d->arch.hvm_domain.hap_enabled = > + hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap); We really should fail with -EOPNOTSUPP. The toolstack already knows whether HAP is available, and nothing good can come from Xen falling silently back to shadow behind the toolstacks back. Probably worth being in a solo patch though. ~Andrew
>>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote: > We want to have a single entry point to initialise hvm guest. 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> > --- > xen/arch/x86/domain.c | 11 ++--------- > xen/arch/x86/hvm/hvm.c | 25 +++++++++++++++++-------- > xen/include/asm-x86/hvm/hvm.h | 2 +- > 3 files changed, 20 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index ddebff6187..af060d8239 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -587,14 +587,7 @@ 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); I'm not really certain it is a good idea to move this last one, as I can't immediately spot uses of it from the hvm/ subtree. > - } > - else if ( is_idle_domain(d) ) > + if ( is_idle_domain(d) ) > rc = 0; > else This now needs to be "else if ( !is_hvm_domain(d) )" afaict. > @@ -615,10 +615,17 @@ int hvm_domain_initialise(struct domain *d) > > hvm_init_cacheattr_region_list(d); > > - rc = paging_enable(d, PG_refcounts|PG_translate|PG_external); > + 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 != 0 ) > goto fail0; > > + rc = paging_enable(d, PG_refcounts|PG_translate|PG_external); > + if ( rc != 0 ) > + goto fail1; Is the order of these required to be that way? The various failN labels would seem to not require re-numbering if you swapped the last two actions. Jan
On Mon, Apr 24, 2017 at 04:10:14AM -0600, Jan Beulich wrote: > >>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote: > > We want to have a single entry point to initialise hvm guest. 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> > > --- > > xen/arch/x86/domain.c | 11 ++--------- > > xen/arch/x86/hvm/hvm.c | 25 +++++++++++++++++-------- > > xen/include/asm-x86/hvm/hvm.h | 2 +- > > 3 files changed, 20 insertions(+), 18 deletions(-) > > > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > > index ddebff6187..af060d8239 100644 > > --- a/xen/arch/x86/domain.c > > +++ b/xen/arch/x86/domain.c > > @@ -587,14 +587,7 @@ 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); > > I'm not really certain it is a good idea to move this last one, as I can't > immediately spot uses of it from the hvm/ subtree. > I think the sole purpose of this call is to create perdomain_l3_pg (when nr is 0), which is then referenced in monitor table creation. Moving this doesn't seem to cause problem. > > - } > > - else if ( is_idle_domain(d) ) > > + if ( is_idle_domain(d) ) > > rc = 0; > > else > > This now needs to be "else if ( !is_hvm_domain(d) )" afaict. > I see what you mean. > > @@ -615,10 +615,17 @@ int hvm_domain_initialise(struct domain *d) > > > > hvm_init_cacheattr_region_list(d); > > > > - rc = paging_enable(d, PG_refcounts|PG_translate|PG_external); > > + 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 != 0 ) > > goto fail0; > > > > + rc = paging_enable(d, PG_refcounts|PG_translate|PG_external); > > + if ( rc != 0 ) > > + goto fail1; > > Is the order of these required to be that way? The various failN > labels would seem to not require re-numbering if you swapped > the last two actions. > The mapping needs to be created before updating paging mode. I don't think the order is important in this function. Wei. > Jan >
On Mon, Apr 10, 2017 at 04:19:12PM +0100, Andrew Cooper wrote: > On 10/04/17 14:27, Wei Liu wrote: > > We want to have a single entry point to initialise hvm guest. 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> > > Two observations... > > > --- > > xen/arch/x86/domain.c | 11 ++--------- > > xen/arch/x86/hvm/hvm.c | 25 +++++++++++++++++-------- > > xen/include/asm-x86/hvm/hvm.h | 2 +- > > 3 files changed, 20 insertions(+), 18 deletions(-) > > > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > > index ddebff6187..af060d8239 100644 > > --- a/xen/arch/x86/domain.c > > +++ b/xen/arch/x86/domain.c > > @@ -663,7 +656,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)) != 0 ) > > If we are pushing these values down (which I agree is a good idea), > please can we push xen_arch_domainconfig down as well. > Fine by me. > > goto fail; > > } > > else > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > > index f50d15ff50..7fc49bb03d 100644 > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -615,10 +615,17 @@ int hvm_domain_initialise(struct domain *d) > > > > hvm_init_cacheattr_region_list(d); > > > > - rc = paging_enable(d, PG_refcounts|PG_translate|PG_external); > > + d->arch.hvm_domain.hap_enabled = > > + hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap); > > We really should fail with -EOPNOTSUPP. > > The toolstack already knows whether HAP is available, and nothing good > can come from Xen falling silently back to shadow behind the toolstacks > back. > > Probably worth being in a solo patch though. > A solo series to be precise. Toolstack needs some fixes to make this work. Wei. > ~Andrew
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index ddebff6187..af060d8239 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -587,14 +587,7 @@ 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) ) + if ( is_idle_domain(d) ) rc = 0; else { @@ -663,7 +656,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)) != 0 ) goto fail; } else diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index f50d15ff50..7fc49bb03d 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -595,7 +595,7 @@ static int hvm_print_line( return X86EMUL_OKAY; } -int hvm_domain_initialise(struct domain *d) +int hvm_domain_initialise(struct domain *d, unsigned int domcr_flags) { unsigned int nr_gsis; int rc; @@ -615,10 +615,17 @@ int hvm_domain_initialise(struct domain *d) hvm_init_cacheattr_region_list(d); - rc = paging_enable(d, PG_refcounts|PG_translate|PG_external); + 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 != 0 ) goto fail0; + rc = paging_enable(d, PG_refcounts|PG_translate|PG_external); + if ( rc != 0 ) + goto fail1; + nr_gsis = is_hardware_domain(d) ? nr_irqs_gsi : NR_HVM_DOMU_IRQS; d->arch.hvm_domain.pl_time = xzalloc(struct pl_time); d->arch.hvm_domain.params = xzalloc_array(uint64_t, HVM_NR_PARAMS); @@ -629,7 +636,7 @@ int hvm_domain_initialise(struct domain *d) rc = -ENOMEM; if ( !d->arch.hvm_domain.pl_time || !d->arch.hvm_domain.irq || !d->arch.hvm_domain.params || !d->arch.hvm_domain.io_handler ) - goto fail1; + goto fail2; /* Set the number of GSIs */ hvm_domain_irq(d)->nr_gsis = nr_gsis; @@ -647,7 +654,7 @@ int hvm_domain_initialise(struct domain *d) if ( d->arch.hvm_domain.io_bitmap == NULL ) { rc = -ENOMEM; - goto fail1; + goto fail2; } memset(d->arch.hvm_domain.io_bitmap, ~0, HVM_IOBITMAP_SIZE); } @@ -666,7 +673,7 @@ int hvm_domain_initialise(struct domain *d) rc = vioapic_init(d); if ( rc != 0 ) - goto fail1; + goto fail2; stdvga_init(d); @@ -679,21 +686,23 @@ int hvm_domain_initialise(struct domain *d) rc = hvm_funcs.domain_initialise(d); if ( rc != 0 ) - goto fail2; + goto fail3; return 0; - fail2: + fail3: rtc_deinit(d); stdvga_deinit(d); vioapic_deinit(d); - fail1: + fail2: if ( is_hardware_domain(d) ) xfree(d->arch.hvm_domain.io_bitmap); xfree(d->arch.hvm_domain.io_handler); xfree(d->arch.hvm_domain.params); xfree(d->arch.hvm_domain.pl_time); xfree(d->arch.hvm_domain.irq); + fail1: + destroy_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0); fail0: hvm_destroy_cacheattr_region_list(d); return rc; diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index 7a85b2e3b5..d3fef9ca52 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -236,7 +236,7 @@ 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 int domcr_flags); void hvm_domain_relinquish_resources(struct domain *d); void hvm_domain_destroy(struct domain *d); void hvm_domain_soft_reset(struct domain *d);
We want to have a single entry point to initialise hvm guest. 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> --- xen/arch/x86/domain.c | 11 ++--------- xen/arch/x86/hvm/hvm.c | 25 +++++++++++++++++-------- xen/include/asm-x86/hvm/hvm.h | 2 +- 3 files changed, 20 insertions(+), 18 deletions(-)