Re: [PATCH 2/3] x86: don't maintain compat M2P when !PV32

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/08/2020 10:28, Jan Beulich wrote:
> Note that opt_pv32's declaration / #define need to be moved due to other
> header dependencies; in particular can asm-x86/mm.h not include
> asm-x86/pv/domain.h.

While I do appreciate that our headers are a complete tangle, I can't
help but feel that this is making the problem worse.

mm.h isn't a better place for opt_pv32 to live.  config.h perhaps,
seeing as its effects are wider than both the domain support itself, or
the memory management support ?

> While touching their definitions anyway, also adjust section placement
> of m2p_compat_vstart and compat_idle_pg_table_l2. Similarly, while
> putting init_xen_pae_l2_slots() inside #ifdef, also move it to a PV-only
> source file.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

So interestingly, this is done out of the order which I was expecting to
do things.  Its not a problem, but I'd like to double check that we
aren't creating future problems.

The goal of this suggestion was actually for PV-Shim, to have only the
regular or compat M2P, as they're fairly large structures and adversely
affect VM density.

This of course requires the kernel elf file to be parsed earlier during
boot, but that isn't a problem.  (It also allows for a PV/PVH dom0
usability fix, whereby the Xen command line has to match the ELF image
provided, rather than auto-selecting the default when only one option is
available.)

The other aspect would be to teach Xen to run on only the compat M2P,
which is fine for any shim smaller than 16T.  (Honestly, if it weren't
an ABI with guests, Shim ought to run exclusively on the compat M2P to
reduce the memory overhead.)

Then during boot, the Shim path would chose to construct only the
regular or compat M2P, based on bitness of the provided kernel.

> ---
> An alternative place for opt_pv32.h would seem to be asm-x86/config.h.

Oh - yes please.  I think that would be better overall.

>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -597,8 +597,10 @@ int arch_domain_create(struct domain *d,
>      }
>      d->arch.emulation_flags = emflags;
>  
> +#ifdef CONFIG_PV32
>      HYPERVISOR_COMPAT_VIRT_START(d) =
>          is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
> +#endif

Can we drop HYPERVISOR_COMPAT_VIRT_START() ?

Its use here as an lvalue in particular makes logic especually hard to
follow, but all it is actually doing is wrapping the shorter
d->arch.hv_compat_vstart

In particular, it would remove the need to conditionally stub
HYPERVISOR_COMPAT_VIRT_START() later.

> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -315,10 +318,10 @@ static void destroy_m2p_mapping(struct m
>   */
>  static int setup_compat_m2p_table(struct mem_hotadd_info *info)
>  {
> +    int err = 0;
>      unsigned long i, smap, emap, epfn = info->epfn;
>      mfn_t mfn;
>      unsigned int n;
> -    int err = 0;

Remnants of an earlier change?

> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -42,8 +42,12 @@
>  #define _PGT_validated    PG_shift(6)
>  #define PGT_validated     PG_mask(1, 6)
>   /* PAE only: is this an L2 page directory containing Xen-private mappings? 
> */
> +#ifdef CONFIG_PV32
>  #define _PGT_pae_xen_l2   PG_shift(7)
>  #define PGT_pae_xen_l2    PG_mask(1, 7)
> +#else
> +#define PGT_pae_xen_l2    0
> +#endif

Hmm - this is going to irritate Coverity and Clang some more.  I still
need to figure out an effective way to make Coverity not object to this
type of short circuiting like this.

I've looked through the users and I think that they're all safe.  I do
however wonder whether is_guest_l2_slot() can be simplified and have its
is_pv_32bit_domain() clause dropped, seeing as it is expensive with its
lfences, and the logic ought to only care about PGT_pae_xen_l2 vs
PGT_l2_page_table.

>  /* Has this page been *partially* validated for use as its current type? */
>  #define _PGT_partial      PG_shift(8)
>  #define PGT_partial       PG_mask(1, 8)
> @@ -494,15 +498,39 @@ extern paddr_t mem_hotplug;
>  #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>  #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>  
> -#define compat_machine_to_phys_mapping ((unsigned int 
> *)RDWR_COMPAT_MPT_VIRT_START)
> +#ifdef CONFIG_PV32
> +
> +extern int8_t opt_pv32;
> +
> +# define compat_machine_to_phys_mapping ((unsigned int 
> *)RDWR_COMPAT_MPT_VIRT_START)
> +
> +# define set_compat_m2p(mfn, entry) \
> +    ((void)(!opt_pv32 || \
> +            (mfn) >= (RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) 
> / 4 || \
> +            (compat_machine_to_phys_mapping[mfn] = (entry))))

I know this is extracting previous logic, but "entry" would probably be
better if it were named "val" or similar.

However, see my reply to patch 3 which I think will simplify this
substantially.

~Andrew