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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/18/2017 10:39 AM, Jan Beulich wrote:
>>>> 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.

Actually, coming back to this -- if we disable linear pagetables, how
can it ever be the case that "PGT_type_equal(x,
ptpg->u.inuse.type_info)" evaluates to true?  The ASSERT()s should never
be executed.

OTOH, if we know the code will be completely unused (and that the
compiler won't know), it's probably a better idea to just block it out
entirely anyway (and maybe add a BUG_ON() the types being equal).

 -George

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