Re: [Xen-devel] [PATCH] x86/mm: Make PV linear pagetables optional

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.10.17 at 19:10, <george.dunlap@xxxxxxxxxx> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1422,6 +1422,22 @@ The following resources are available:
>      CDP, one COS will corespond two CBMs other than one with CAT, due to the
>      sum of CBMs is fixed, that means actual `cos_max` in use will 
> automatically
>      reduce to half when CDP is enabled.
> +     
> +### pv-linear-pt
> +> `= <boolean>`
> +
> +> Default: `false`

This looks to be wrong now.

> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -97,6 +97,27 @@ config TBOOT
>         Technology (TXT)
>  
>         If unsure, say Y.
> +
> +config PV_LINEAR_PT
> +       bool "Support for PV linear pagetables"
> +       depends on PV

For this to look reasonable in a hierarchical menu, it should follow
PV (with - if there were any - only other options also depending on
PV in between) rather than being added at a random place.

> +       default y
> +       ---help---
> +         Linear pagetables (also called "recursive pagetables") refers
> +      to the practice of a guest operating system having pagetable

The two lines above should match in how they're being indented.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -587,6 +587,12 @@ static void put_data_page(
>          put_page(page);
>  }
>  
> +#ifdef CONFIG_PV_LINEAR_PT
> +static void zero_linear_entries(struct page_info *pg)

When framing multiple functions, I think it is better to have a blank
line between #ifdef and first piece of code (as well as around the
#else and prior to the #endif), and I think the #else and #endif
would also benefit from having /* PV_LINEAR_PT */ or some such
added on their lines.

> @@ -719,6 +735,20 @@ get_##level##_linear_pagetable(                          
>                    \
>                                                                              \
>      return 1;                                                               \
>  }
> +#define LPT_ASSERT ASSERT
> +#else
> +#define define_get_linear_pagetable(level)                              \
> +static int                                                              \
> +get_##level##_linear_pagetable(                                         \
> +        level##_pgentry_t pde, unsigned long pde_pfn, struct domain *d) \
> +{                                                                       \
> +        return 0;                                                       \
> +}
> +#define zero_linear_entries(pg)
> +#define dec_linear_uses(pg)
> +#define dec_linear_entries(pg)

Would perhaps be better if these evaluated their arguments.

> +#define LPT_ASSERT(x)
> +#endif
>  
>  
>  bool is_iomem_page(mfn_t mfn)

Could you arrange for the double blank lines to go away here with
the blank line additions asked for above?

> @@ -2330,8 +2360,8 @@ static int _put_page_type(struct page_info *page, bool 
> preemptible,
>                   * necessary anymore for a dying domain.
>                   */
>                  ASSERT(page_get_owner(page)->is_dying);
> -                ASSERT(page->linear_pt_count < 0);
> -                ASSERT(ptpg->linear_pt_count > 0);
> +                LPT_ASSERT(page->linear_pt_count < 0);
> +                LPT_ASSERT(ptpg->linear_pt_count > 0);

Other than Andrew has suggested, with these I don't think
LPT_ASSERT() can go away, unless you played tricks and forced
the function's ptpg to be NULL regardless of caller, or unless you
put the entire if() into an #ifdef.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel