diff mbox

[v3] x86: clarify shadow paging Dom0 support

Message ID 20170327133401.3573-1-wei.liu2@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wei Liu March 27, 2017, 1:34 p.m. UTC
Classic PV shadow paging Dom0 has been broken for years, and can't
possibly be configured after 4045953.

PVH shadow paging Dom0 should still be possible.

Change the code and documentation to clarify that.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3:
1. don't panic

v2:
1. simplify need_paging
2. move panic to construct_dom0

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>
Cc: Roger Pau Monné <roger.pau@citrix.com>
---
 docs/misc/xen-command-line.markdown |  8 ++------
 xen/arch/x86/dom0_build.c           | 11 ++++++++---
 xen/arch/x86/pv/dom0_build.c        |  5 -----
 3 files changed, 10 insertions(+), 14 deletions(-)

Comments

Wei Liu March 27, 2017, 1:36 p.m. UTC | #1
On Mon, Mar 27, 2017 at 02:34:01PM +0100, Wei Liu wrote:
>  
> +    if ( opt_dom0_shadow && !dom0_pvh )
> +    {
> +        opt_dom0_shadow = false;
> +        printk("Shadow Dom0 requires PVH. Option ignored.");

There should be a '\n' at the end.

Wei.
Jan Beulich March 27, 2017, 1:51 p.m. UTC | #2
>>> On 27.03.17 at 15:36, <wei.liu2@citrix.com> wrote:
> On Mon, Mar 27, 2017 at 02:34:01PM +0100, Wei Liu wrote:
>>  
>> +    if ( opt_dom0_shadow && !dom0_pvh )
>> +    {
>> +        opt_dom0_shadow = false;
>> +        printk("Shadow Dom0 requires PVH. Option ignored.");
> 
> There should be a '\n' at the end.

With additionally XENLOG_WARNING added
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan
Andrew Cooper March 27, 2017, 1:58 p.m. UTC | #3
On 27/03/17 14:34, Wei Liu wrote:
> Classic PV shadow paging Dom0 has been broken for years, and can't
> possibly be configured after 4045953.
>
> PVH shadow paging Dom0 should still be possible.
>
> Change the code and documentation to clarify that.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> v3:
> 1. don't panic
>
> v2:
> 1. simplify need_paging
> 2. move panic to construct_dom0
>
> 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>
> Cc: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  docs/misc/xen-command-line.markdown |  8 ++------
>  xen/arch/x86/dom0_build.c           | 11 ++++++++---
>  xen/arch/x86/pv/dom0_build.c        |  5 -----
>  3 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index bdbdb8a53b..9eb85d68b5 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -652,11 +652,6 @@ restrictions set up here. Note that the values to be specified here are
>  ACPI PXM ones, not Xen internal node numbers. `relaxed` sets up vCPU
>  affinities to prefer but be not limited to the specified node(s).
>  
> -### dom0\_shadow
> -> `= <boolean>`
> -
> -This option is deprecated, please use `dom0=shadow` instead.
> -
>  ### dom0\_vcpus\_pin
>  > `= <boolean>`
>  
> @@ -679,7 +674,8 @@ Flag that makes a dom0 boot in PVHv2 mode.
>  
>  > Default: `false`
>  
> -Flag that makes a dom0 use shadow paging.
> +Flag that makes a dom0 use shadow paging. Only works when "pvh" is
> +enabled.
>  
>  ### dtuart (ARM)
>  > `= path [:options]`
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 20221b5e32..8597d43b11 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -174,7 +174,6 @@ struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
>  
>  #ifdef CONFIG_SHADOW_PAGING
>  bool __initdata opt_dom0_shadow;
> -boolean_param("dom0_shadow", opt_dom0_shadow);
>  #endif
>  bool __initdata dom0_pvh;
>  
> @@ -252,8 +251,8 @@ unsigned long __init dom0_compute_nr_pages(
>              avail -= max_pdx >> s;
>      }
>  
> -    need_paging = is_hvm_domain(d) ? !iommu_hap_pt_share || !paging_mode_hap(d)
> -                                   : opt_dom0_shadow;
> +    need_paging = is_hvm_domain(d) &&
> +        (!iommu_hap_pt_share || !paging_mode_hap(d));
>      for ( ; ; need_paging = 0 )
>      {
>          nr_pages = dom0_nrpages;
> @@ -456,6 +455,12 @@ int __init construct_dom0(struct domain *d, const module_t *image,
>  
>      process_pending_softirqs();
>  
> +    if ( opt_dom0_shadow && !dom0_pvh )
> +    {
> +        opt_dom0_shadow = false;
> +        printk("Shadow Dom0 requires PVH. Option ignored.");
> +    }

This needs to be inside a #ifdef CONFIG_SHADOW_PAGING to avoid breaking
the build.

With the other corrections as well, Reviewed-by: Andrew Cooper
<andrew.cooper@citrix.com>
Wei Liu March 27, 2017, 2:15 p.m. UTC | #4
On Mon, Mar 27, 2017 at 02:58:15PM +0100, Andrew Cooper wrote:
> 
> This needs to be inside a #ifdef CONFIG_SHADOW_PAGING to avoid breaking
> the build.
> 

I just found out there is a similar issue in x86/setup.c and I sent
a separate patch to fix that.

Wei.
Jan Beulich March 27, 2017, 2:24 p.m. UTC | #5
>>> On 27.03.17 at 16:15, <wei.liu2@citrix.com> wrote:
> On Mon, Mar 27, 2017 at 02:58:15PM +0100, Andrew Cooper wrote:
>> 
>> This needs to be inside a #ifdef CONFIG_SHADOW_PAGING to avoid breaking
>> the build.
>> 
> 
> I just found out there is a similar issue in x86/setup.c and I sent
> a separate patch to fix that.

The situation there is different: What you can't have without
CONFIG_SHADOW_PAGING is the identifier used as an lvalue.

Jan
diff mbox

Patch

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index bdbdb8a53b..9eb85d68b5 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -652,11 +652,6 @@  restrictions set up here. Note that the values to be specified here are
 ACPI PXM ones, not Xen internal node numbers. `relaxed` sets up vCPU
 affinities to prefer but be not limited to the specified node(s).
 
-### dom0\_shadow
-> `= <boolean>`
-
-This option is deprecated, please use `dom0=shadow` instead.
-
 ### dom0\_vcpus\_pin
 > `= <boolean>`
 
@@ -679,7 +674,8 @@  Flag that makes a dom0 boot in PVHv2 mode.
 
 > Default: `false`
 
-Flag that makes a dom0 use shadow paging.
+Flag that makes a dom0 use shadow paging. Only works when "pvh" is
+enabled.
 
 ### dtuart (ARM)
 > `= path [:options]`
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 20221b5e32..8597d43b11 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -174,7 +174,6 @@  struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
 
 #ifdef CONFIG_SHADOW_PAGING
 bool __initdata opt_dom0_shadow;
-boolean_param("dom0_shadow", opt_dom0_shadow);
 #endif
 bool __initdata dom0_pvh;
 
@@ -252,8 +251,8 @@  unsigned long __init dom0_compute_nr_pages(
             avail -= max_pdx >> s;
     }
 
-    need_paging = is_hvm_domain(d) ? !iommu_hap_pt_share || !paging_mode_hap(d)
-                                   : opt_dom0_shadow;
+    need_paging = is_hvm_domain(d) &&
+        (!iommu_hap_pt_share || !paging_mode_hap(d));
     for ( ; ; need_paging = 0 )
     {
         nr_pages = dom0_nrpages;
@@ -456,6 +455,12 @@  int __init construct_dom0(struct domain *d, const module_t *image,
 
     process_pending_softirqs();
 
+    if ( opt_dom0_shadow && !dom0_pvh )
+    {
+        opt_dom0_shadow = false;
+        printk("Shadow Dom0 requires PVH. Option ignored.");
+    }
+
     return (is_hvm_domain(d) ? dom0_construct_pvh : dom0_construct_pv)
            (d, image, image_headroom, initrd,bootstrap_map, cmdline);
 }
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 65cd2c5019..18c19a256f 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -870,11 +870,6 @@  int __init dom0_construct_pv(struct domain *d,
     regs->rsi = vstartinfo_start;
     regs->eflags = X86_EFLAGS_IF;
 
-#ifdef CONFIG_SHADOW_PAGING
-    if ( opt_dom0_shadow && paging_enable(d, PG_SH_enable) == 0 )
-        paging_update_paging_modes(v);
-#endif
-
     if ( test_bit(XENFEAT_supervisor_mode_kernel, parms.f_required) )
         panic("Dom0 requires supervisor-mode execution");