[PATCH 0/3] x86: shim building adjustments

[PATCH 0/3] x86: shim building adjustments

[Jan Beulich]

The first change is simply addressing a build issue noticed by
Andrew. The build breakage goes beyond this specific combination
though - PV_SHIM_EXCLUSIVE plus HVM is similarly an issue. This
is what the last patch tries to take care of, in a shape already
on irc noticed to be controversial. I'm submitting the change
nevertheless because for the moment there looks to be a majority
in favor of going this route. One argument not voiced there yet:
What good does it do to allow a user to enable HVM when then on
the resulting hypervisor they still can't run HVM guests (for
the hypervisor still being a dedicated PV shim one). On top of
this, the alternative approach is likely going to get ugly.

1: fix build with PV_SHIM_EXCLUSIVE and SHADOW_PAGING
2: adjust Kconfig defaults
3: don't permit HVM and PV_SHIM_EXCLUSIVE at the same time

Jan

[PATCH 1/3] x86/shim: fix build with PV_SHIM_EXCLUSIVE and SHADOW_PAGING

[Jan Beulich]

While there's little point in enabling both, the combination ought to at
least build correctly. Drop the direct PV_SHIM_EXCLUSIVE conditionals
and instead zap PG_log_dirty to zero under the right conditions, and key
other #ifdef-s off of that.

While there also expand on ded576ce07e9 ("x86/shadow: dirty VRAM
tracking is needed for HVM only"): There was yet another is_hvm_domain()
missing, and code touching the struct fields needs to be guarded by
suitable #ifdef-s as well. While there also guard shadow-mode-only
fields accordingly.

Fixes: 8b5b49ceb3d9 ("x86: don't include domctl and alike in shim-exclusive builds")
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -47,7 +47,7 @@
 /* Per-CPU variable for enforcing the lock ordering */
 DEFINE_PER_CPU(int, mm_lock_level);
 
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#if PG_log_dirty
 
 /************************************************/
 /*              LOG DIRTY SUPPORT               */
@@ -630,7 +630,7 @@ void paging_log_dirty_init(struct domain
     d->arch.paging.log_dirty.ops = ops;
 }
 
-#endif /* CONFIG_PV_SHIM_EXCLUSIVE */
+#endif /* PG_log_dirty */
 
 /************************************************/
 /*           CODE FOR PAGING SUPPORT            */
@@ -671,7 +671,7 @@ void paging_vcpu_init(struct vcpu *v)
         shadow_vcpu_init(v);
 }
 
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#if PG_log_dirty
 int paging_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
                   XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl,
                   bool_t resuming)
@@ -792,7 +792,7 @@ long paging_domctl_continuation(XEN_GUES
 
     return ret;
 }
-#endif /* CONFIG_PV_SHIM_EXCLUSIVE */
+#endif /* PG_log_dirty */
 
 /* Call when destroying a domain */
 int paging_teardown(struct domain *d)
@@ -808,7 +808,7 @@ int paging_teardown(struct domain *d)
     if ( preempted )
         return -ERESTART;
 
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#if PG_log_dirty
     /* clean up log dirty resources. */
     rc = paging_free_log_dirty_bitmap(d, 0);
     if ( rc == -ERESTART )
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2869,12 +2869,14 @@ void shadow_teardown(struct domain *d, b
      * calls now that we've torn down the bitmap */
     d->arch.paging.mode &= ~PG_log_dirty;
 
-    if ( d->arch.hvm.dirty_vram )
+#ifdef CONFIG_HVM
+    if ( is_hvm_domain(d) && d->arch.hvm.dirty_vram )
     {
         xfree(d->arch.hvm.dirty_vram->sl1ma);
         xfree(d->arch.hvm.dirty_vram->dirty_bitmap);
         XFREE(d->arch.hvm.dirty_vram);
     }
+#endif
 
 out:
     paging_unlock(d);
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -618,6 +618,7 @@ _sh_propagate(struct vcpu *v,
         }
     }
 
+#ifdef CONFIG_HVM
     if ( unlikely(level == 1) && is_hvm_domain(d) )
     {
         struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
@@ -632,6 +633,7 @@ _sh_propagate(struct vcpu *v,
                 sflags &= ~_PAGE_RW;
         }
     }
+#endif
 
     /* Read-only memory */
     if ( p2m_is_readonly(p2mt) )
@@ -1050,6 +1052,7 @@ static inline void shadow_vram_get_l1e(s
                                        mfn_t sl1mfn,
                                        struct domain *d)
 {
+#ifdef CONFIG_HVM
     mfn_t mfn = shadow_l1e_get_mfn(new_sl1e);
     int flags = shadow_l1e_get_flags(new_sl1e);
     unsigned long gfn;
@@ -1074,6 +1077,7 @@ static inline void shadow_vram_get_l1e(s
             dirty_vram->sl1ma[i] = mfn_to_maddr(sl1mfn)
                 | ((unsigned long)sl1e & ~PAGE_MASK);
     }
+#endif
 }
 
 static inline void shadow_vram_put_l1e(shadow_l1e_t old_sl1e,
@@ -1081,6 +1085,7 @@ static inline void shadow_vram_put_l1e(s
                                        mfn_t sl1mfn,
                                        struct domain *d)
 {
+#ifdef CONFIG_HVM
     mfn_t mfn = shadow_l1e_get_mfn(old_sl1e);
     int flags = shadow_l1e_get_flags(old_sl1e);
     unsigned long gfn;
@@ -1140,6 +1145,7 @@ static inline void shadow_vram_put_l1e(s
             dirty_vram->last_dirty = NOW();
         }
     }
+#endif
 }
 
 static int shadow_set_l1e(struct domain *d,
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -67,8 +67,12 @@
 #define PG_translate   0
 #define PG_external    0
 #endif
+#if defined(CONFIG_HVM) || !defined(CONFIG_PV_SHIM_EXCLUSIVE)
 /* Enable log dirty mode */
 #define PG_log_dirty   (XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY << PG_mode_shift)
+#else
+#define PG_log_dirty   0
+#endif
 
 /* All paging modes. */
 #define PG_MASK (PG_refcounts | PG_log_dirty | PG_translate | PG_external)
@@ -154,7 +158,7 @@ struct paging_mode {
 /*****************************************************************************
  * Log dirty code */
 
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#if PG_log_dirty
 
 /* get the dirty bitmap for a specific range of pfns */
 void paging_log_dirty_range(struct domain *d,
@@ -195,23 +199,28 @@ int paging_mfn_is_dirty(struct domain *d
 #define L4_LOGDIRTY_IDX(pfn) ((pfn_x(pfn) >> (PAGE_SHIFT + 3 + PAGETABLE_ORDER * 2)) & \
                               (LOGDIRTY_NODE_ENTRIES-1))
 
+#ifdef CONFIG_HVM
 /* VRAM dirty tracking support */
 struct sh_dirty_vram {
     unsigned long begin_pfn;
     unsigned long end_pfn;
+#ifdef CONFIG_SHADOW_PAGING
     paddr_t *sl1ma;
     uint8_t *dirty_bitmap;
     s_time_t last_dirty;
+#endif
 };
+#endif
 
-#else /* !CONFIG_PV_SHIM_EXCLUSIVE */
+#else /* !PG_log_dirty */
 
 static inline void paging_log_dirty_init(struct domain *d,
                                          const struct log_dirty_ops *ops) {}
 static inline void paging_mark_dirty(struct domain *d, mfn_t gmfn) {}
 static inline void paging_mark_pfn_dirty(struct domain *d, pfn_t pfn) {}
+static inline bool paging_mfn_is_dirty(struct domain *d, mfn_t gmfn) { return false; }
 
-#endif /* CONFIG_PV_SHIM_EXCLUSIVE */
+#endif /* PG_log_dirty */
 
 /*****************************************************************************
  * Entry points into the paging-assistance code */

Re: [PATCH 1/3] x86/shim: fix build with PV_SHIM_EXCLUSIVE and SHADOW_PAGING

[Roger Pau Monné]

On Mon, Sep 14, 2020 at 02:38:49PM +0200, Jan Beulich wrote:
> While there's little point in enabling both, the combination ought to at
> least build correctly. Drop the direct PV_SHIM_EXCLUSIVE conditionals
> and instead zap PG_log_dirty to zero under the right conditions, and key
> other #ifdef-s off of that.
> 
> While there also expand on ded576ce07e9 ("x86/shadow: dirty VRAM
> tracking is needed for HVM only"): There was yet another is_hvm_domain()
> missing, and code touching the struct fields needs to be guarded by
> suitable #ifdef-s as well. While there also guard shadow-mode-only
> fields accordingly.
> 
> Fixes: 8b5b49ceb3d9 ("x86: don't include domctl and alike in shim-exclusive builds")
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

We seem to be growing more and more ifdefs which is not ideal IMO, we
should rather aim to remove them by splitting code into separate
compilation units. There doesn't seem to be much option to split
stuff in this case, so be it.

Thanks, Roger.

Re: [PATCH 1/3] x86/shim: fix build with PV_SHIM_EXCLUSIVE and SHADOW_PAGING

[Jan Beulich]

On 14.09.2020 18:44, Roger Pau Monné wrote:
> On Mon, Sep 14, 2020 at 02:38:49PM +0200, Jan Beulich wrote:
>> While there's little point in enabling both, the combination ought to at
>> least build correctly. Drop the direct PV_SHIM_EXCLUSIVE conditionals
>> and instead zap PG_log_dirty to zero under the right conditions, and key
>> other #ifdef-s off of that.
>>
>> While there also expand on ded576ce07e9 ("x86/shadow: dirty VRAM
>> tracking is needed for HVM only"): There was yet another is_hvm_domain()
>> missing, and code touching the struct fields needs to be guarded by
>> suitable #ifdef-s as well. While there also guard shadow-mode-only
>> fields accordingly.
>>
>> Fixes: 8b5b49ceb3d9 ("x86: don't include domctl and alike in shim-exclusive builds")
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> We seem to be growing more and more ifdefs which is not ideal IMO, we
> should rather aim to remove them by splitting code into separate
> compilation units. There doesn't seem to be much option to split
> stuff in this case, so be it.

Right - I was indeed wondering whether maybe shadow_vram_{get,put}_l1e()
could be moved into hvm.c, but I didn't want to right away take the time
to check, as it seemed more important to get the build issue addressed.

Beyond the #ifdef-s added there the majority of the changes here are
merely adjustments to existing ones. (I'd except the new PG_log_dirty
conditional #define-s from the ones wanting better alternatives, as we
have been doing the same already for other PG_* constants.)

Jan

[PATCH 2/3] x86/shim: adjust Kconfig defaults

[Jan Beulich]

Just like HVM, defaulting SHADOW_PAGING and TBOOT to Yes in shim-
exclusive mode makes no sense, as the respective code is dead there.

Also adjust the shim default config file: It needs to specifiy values
only for settings where a non-default value is wanted.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -116,9 +116,9 @@ config XEN_SHSTK
 	  compatiblity can be provided via the PV Shim mechanism.
 
 config SHADOW_PAGING
-        bool "Shadow Paging"
-        default y
-        ---help---
+	bool "Shadow Paging"
+	default y if !PV_SHIM_EXCLUSIVE
+	---help---
 
           Shadow paging is a software alternative to hardware paging support
           (Intel EPT, AMD NPT).
@@ -165,8 +165,8 @@ config HVM_FEP
 	  If unsure, say N.
 
 config TBOOT
-	def_bool y
-	prompt "Xen tboot support" if EXPERT
+	bool "Xen tboot support" if EXPERT
+	default y if !PV_SHIM_EXCLUSIVE
 	select CRYPTO
 	---help---
 	  Allows support for Trusted Boot using the Intel(R) Trusted Execution
--- a/xen/arch/x86/configs/pvshim_defconfig
+++ b/xen/arch/x86/configs/pvshim_defconfig
@@ -8,12 +8,9 @@ CONFIG_NR_CPUS=32
 CONFIG_EXPERT=y
 CONFIG_SCHED_NULL=y
 # Disable features not used by the PV shim
-# CONFIG_HVM is not set
 # CONFIG_XEN_SHSTK is not set
 # CONFIG_HYPFS is not set
-# CONFIG_SHADOW_PAGING is not set
 # CONFIG_BIGMEM is not set
-# CONFIG_TBOOT is not set
 # CONFIG_KEXEC is not set
 # CONFIG_XENOPROF is not set
 # CONFIG_XSM is not set

Re: [PATCH 2/3] x86/shim: adjust Kconfig defaults

[Roger Pau Monné]

On Mon, Sep 14, 2020 at 02:39:08PM +0200, Jan Beulich wrote:
> Just like HVM, defaulting SHADOW_PAGING and TBOOT to Yes in shim-
> exclusive mode makes no sense, as the respective code is dead there.
> 
> Also adjust the shim default config file: It needs to specifiy values
> only for settings where a non-default value is wanted.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> 
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -116,9 +116,9 @@ config XEN_SHSTK
>  	  compatiblity can be provided via the PV Shim mechanism.
>  
>  config SHADOW_PAGING
> -        bool "Shadow Paging"
> -        default y
> -        ---help---
> +	bool "Shadow Paging"
> +	default y if !PV_SHIM_EXCLUSIVE

I think you could also do: default !PV_SHIM_EXCLUSIVE?

I'm fine with using the current form.

Thanks, Roger.

Re: [PATCH 2/3] x86/shim: adjust Kconfig defaults

[Jan Beulich]

On 15.09.2020 12:49, Roger Pau Monné wrote:
> On Mon, Sep 14, 2020 at 02:39:08PM +0200, Jan Beulich wrote:
>> Just like HVM, defaulting SHADOW_PAGING and TBOOT to Yes in shim-
>> exclusive mode makes no sense, as the respective code is dead there.
>>
>> Also adjust the shim default config file: It needs to specifiy values
>> only for settings where a non-default value is wanted.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -116,9 +116,9 @@ config XEN_SHSTK
>>  	  compatiblity can be provided via the PV Shim mechanism.
>>  
>>  config SHADOW_PAGING
>> -        bool "Shadow Paging"
>> -        default y
>> -        ---help---
>> +	bool "Shadow Paging"
>> +	default y if !PV_SHIM_EXCLUSIVE
> 
> I think you could also do: default !PV_SHIM_EXCLUSIVE?

Oh, yes, in this case I can indeed. I've mechanically used the form
wanted when the prompt isn't unconditionally visible. Will switch.

Jan

[PATCH 3/3] x86/shim: don't permit HVM and PV_SHIM_EXCLUSIVE at the same time

[Jan Beulich]

This combination doesn't really make sense (and there likely are more).
The alternative here would be some presumably intrusive #ifdef-ary to
get this combination to actually build again.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -23,7 +23,7 @@ config X86
 	select HAS_PDX
 	select HAS_SCHED_GRANULARITY
 	select HAS_UBSAN
-	select HAS_VPCI if !PV_SHIM_EXCLUSIVE && HVM
+	select HAS_VPCI if HVM
 	select NEEDS_LIBELF
 	select NUMA
 
@@ -90,8 +90,8 @@ config PV_LINEAR_PT
          If unsure, say Y.
 
 config HVM
-	def_bool !PV_SHIM_EXCLUSIVE
-	prompt "HVM support"
+	bool "HVM support"
+	depends on !PV_SHIM_EXCLUSIVE
 	---help---
 	  Interfaces to support HVM domains.  HVM domains require hardware
 	  virtualisation extensions (e.g. Intel VT-x, AMD SVM), but can boot

Re: [PATCH 3/3] x86/shim: don't permit HVM and PV_SHIM_EXCLUSIVE at the same time

[Roger Pau Monné]

On Mon, Sep 14, 2020 at 02:39:33PM +0200, Jan Beulich wrote:
> This combination doesn't really make sense (and there likely are more).
> The alternative here would be some presumably intrusive #ifdef-ary to
> get this combination to actually build again.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I'm not sure I see a way to cater for Andrew requests while allowing
PV_SHIM_EXCLUSIVE to gate options in order to be able to remove
code.

Maybe PV_SHIM_EXCLUSIVE should be moved to the top level Kconfig
screen on x86 and behave like a list, so that you select a 'Normal'
hypervisor build or a 'PV shim exclusive' build, and that could
completely change the contents of the menus?

If that doesn't seem suitable I think the proposed patch is a good
option unless someone has a better approach.

Thanks, Roger.

Re: [PATCH 3/3] x86/shim: don't permit HVM and PV_SHIM_EXCLUSIVE at the same time

[Jan Beulich]

On 15.09.2020 13:02, Roger Pau Monné wrote:
> On Mon, Sep 14, 2020 at 02:39:33PM +0200, Jan Beulich wrote:
>> This combination doesn't really make sense (and there likely are more).
>> The alternative here would be some presumably intrusive #ifdef-ary to
>> get this combination to actually build again.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I'm not sure I see a way to cater for Andrew requests while allowing
> PV_SHIM_EXCLUSIVE to gate options in order to be able to remove
> code.

Yes, I'm specifically hoping for his input, in particular in light
of the additional argument given in the cover letter (HVM not
actually usable in this mode).

> Maybe PV_SHIM_EXCLUSIVE should be moved to the top level Kconfig
> screen on x86 and behave like a list, so that you select a 'Normal'
> hypervisor build or a 'PV shim exclusive' build, and that could
> completely change the contents of the menus?

This might be another option, yes. Let's see what Andrew thinks.

And btw - the patch needs an update anyway, as I've lost the
defaulting to Y of this option.

Jan