diff mbox

[1/2] x86: limit page type width

Message ID 592E897D020000780015DF7E@prv-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Beulich May 31, 2017, 7:14 a.m. UTC 
There's no reason to burn 4 bits on page type when we only have 7 types
(plus "none") at present. This requires changing one use of
PGT_shared_page, which so far assumed that the type is both a power of
2 and the only type with the high bit set.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
x86: limit page type width

There's no reason to burn 4 bits on page type when we only have 7 types
(plus "none") at present. This requires changing one use of
PGT_shared_page, which so far assumed that the type is both a power of
2 and the only type with the high bit set.

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

--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -452,7 +452,7 @@ static int audit(void)
         }
 
         /* Check if the MFN has correct type, owner and handle. */ 
-        if ( !(pg->u.inuse.type_info & PGT_shared_page) )
+        if ( (pg->u.inuse.type_info & PGT_type_mask) != PGT_shared_page )
         {
            MEM_SHARING_DEBUG("mfn %lx in audit list, but not PGT_shared_page (%lx)!\n",
                               mfn_x(mfn), pg->u.inuse.type_info & PGT_type_mask);
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -176,16 +176,19 @@ struct page_info
 #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
 
  /* The following page types are MUTUALLY EXCLUSIVE. */
-#define PGT_none          PG_mask(0, 4)  /* no special uses of this page   */
-#define PGT_l1_page_table PG_mask(1, 4)  /* using as an L1 page table?     */
-#define PGT_l2_page_table PG_mask(2, 4)  /* using as an L2 page table?     */
-#define PGT_l3_page_table PG_mask(3, 4)  /* using as an L3 page table?     */
-#define PGT_l4_page_table PG_mask(4, 4)  /* using as an L4 page table?     */
-#define PGT_seg_desc_page PG_mask(5, 4)  /* using this page in a GDT/LDT?  */
-#define PGT_writable_page PG_mask(7, 4)  /* has writable mappings?         */
-#define PGT_shared_page   PG_mask(8, 4)  /* CoW sharable page              */
-#define PGT_type_mask     PG_mask(15, 4) /* Bits 28-31 or 60-63.           */
+#define PGT_none          PG_mask(0, 3)  /* no special uses of this page   */
+#define PGT_l1_page_table PG_mask(1, 3)  /* using as an L1 page table?     */
+#define PGT_l2_page_table PG_mask(2, 3)  /* using as an L2 page table?     */
+#define PGT_l3_page_table PG_mask(3, 3)  /* using as an L3 page table?     */
+#define PGT_l4_page_table PG_mask(4, 3)  /* using as an L4 page table?     */
+#define PGT_seg_desc_page PG_mask(5, 3)  /* using this page in a GDT/LDT?  */
+#define PGT_shared_page   PG_mask(6, 3)  /* CoW sharable page              */
+#define PGT_writable_page PG_mask(7, 3)  /* has writable mappings?         */
+#define PGT_type_mask     PG_mask(7, 3)  /* Bits 61-63.                    */
 
+ /* Page is locked? */
+#define _PGT_locked       PG_shift(4)
+#define PGT_locked        PG_mask(1, 4)
  /* Owning guest has pinned this page to its current type? */
 #define _PGT_pinned       PG_shift(5)
 #define PGT_pinned        PG_mask(1, 5)
@@ -198,12 +201,9 @@ struct page_info
 /* 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)
- /* Page is locked? */
-#define _PGT_locked       PG_shift(9)
-#define PGT_locked        PG_mask(1, 9)
 
  /* Count of uses of this frame as its current type. */
-#define PGT_count_width   PG_shift(9)
+#define PGT_count_width   PG_shift(8)
 #define PGT_count_mask    ((1UL<<PGT_count_width)-1)
 
  /* Cleared when the owning guest 'frees' this page. */

Comments

Andrew Cooper May 31, 2017, 10:01 a.m. UTC | #1 
On 31/05/17 08:14, Jan Beulich wrote:
> There's no reason to burn 4 bits on page type when we only have 7 types
> (plus "none") at present. This requires changing one use of
> PGT_shared_page, which so far assumed that the type is both a power of
> 2 and the only type with the high bit set.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Any particular reason why you reverse the order of shared and writeable?

Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Jan Beulich May 31, 2017, 10:09 a.m. UTC | #2 
>>> On 31.05.17 at 12:01, <andrew.cooper3@citrix.com> wrote:
> On 31/05/17 08:14, Jan Beulich wrote:
>> There's no reason to burn 4 bits on page type when we only have 7 types
>> (plus "none") at present. This requires changing one use of
>> PGT_shared_page, which so far assumed that the type is both a power of
>> 2 and the only type with the high bit set.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Any particular reason why you reverse the order of shared and writeable?

I've become very used to know that type 7 is "writable", and since
there was a gap at 6 it seemed reasonable to put "shared" there
instead of shifting both down by one.

> Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

Jan
Jan Beulich June 6, 2017, 8:26 a.m. UTC | #3 
>>> On 31.05.17 at 09:14, <JBeulich@suse.com> wrote:
> There's no reason to burn 4 bits on page type when we only have 7 types
> (plus "none") at present. This requires changing one use of
> PGT_shared_page, which so far assumed that the type is both a power of
> 2 and the only type with the high bit set.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -452,7 +452,7 @@ static int audit(void)
>          }
>  
>          /* Check if the MFN has correct type, owner and handle. */ 
> -        if ( !(pg->u.inuse.type_info & PGT_shared_page) )
> +        if ( (pg->u.inuse.type_info & PGT_type_mask) != PGT_shared_page )
>          {
>             MEM_SHARING_DEBUG("mfn %lx in audit list, but not PGT_shared_page (%lx)!\n",
>                                mfn_x(mfn), pg->u.inuse.type_info & PGT_type_mask);

Tamas,

I've noticed only now that I did forget to Cc you on the change
above (fixing a latent bug, which would otherwise become an
actual one with the other adjustments done here).

Jan
diff mbox

Patch

--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -452,7 +452,7 @@  static int audit(void)
         }
 
         /* Check if the MFN has correct type, owner and handle. */ 
-        if ( !(pg->u.inuse.type_info & PGT_shared_page) )
+        if ( (pg->u.inuse.type_info & PGT_type_mask) != PGT_shared_page )
         {
            MEM_SHARING_DEBUG("mfn %lx in audit list, but not PGT_shared_page (%lx)!\n",
                               mfn_x(mfn), pg->u.inuse.type_info & PGT_type_mask);
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -176,16 +176,19 @@  struct page_info
 #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
 
  /* The following page types are MUTUALLY EXCLUSIVE. */
-#define PGT_none          PG_mask(0, 4)  /* no special uses of this page   */
-#define PGT_l1_page_table PG_mask(1, 4)  /* using as an L1 page table?     */
-#define PGT_l2_page_table PG_mask(2, 4)  /* using as an L2 page table?     */
-#define PGT_l3_page_table PG_mask(3, 4)  /* using as an L3 page table?     */
-#define PGT_l4_page_table PG_mask(4, 4)  /* using as an L4 page table?     */
-#define PGT_seg_desc_page PG_mask(5, 4)  /* using this page in a GDT/LDT?  */
-#define PGT_writable_page PG_mask(7, 4)  /* has writable mappings?         */
-#define PGT_shared_page   PG_mask(8, 4)  /* CoW sharable page              */
-#define PGT_type_mask     PG_mask(15, 4) /* Bits 28-31 or 60-63.           */
+#define PGT_none          PG_mask(0, 3)  /* no special uses of this page   */
+#define PGT_l1_page_table PG_mask(1, 3)  /* using as an L1 page table?     */
+#define PGT_l2_page_table PG_mask(2, 3)  /* using as an L2 page table?     */
+#define PGT_l3_page_table PG_mask(3, 3)  /* using as an L3 page table?     */
+#define PGT_l4_page_table PG_mask(4, 3)  /* using as an L4 page table?     */
+#define PGT_seg_desc_page PG_mask(5, 3)  /* using this page in a GDT/LDT?  */
+#define PGT_shared_page   PG_mask(6, 3)  /* CoW sharable page              */
+#define PGT_writable_page PG_mask(7, 3)  /* has writable mappings?         */
+#define PGT_type_mask     PG_mask(7, 3)  /* Bits 61-63.                    */
 
+ /* Page is locked? */
+#define _PGT_locked       PG_shift(4)
+#define PGT_locked        PG_mask(1, 4)
  /* Owning guest has pinned this page to its current type? */
 #define _PGT_pinned       PG_shift(5)
 #define PGT_pinned        PG_mask(1, 5)
@@ -198,12 +201,9 @@  struct page_info
 /* 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)
- /* Page is locked? */
-#define _PGT_locked       PG_shift(9)
-#define PGT_locked        PG_mask(1, 9)
 
  /* Count of uses of this frame as its current type. */
-#define PGT_count_width   PG_shift(9)
+#define PGT_count_width   PG_shift(8)
 #define PGT_count_mask    ((1UL<<PGT_count_width)-1)
 
  /* Cleared when the owning guest 'frees' this page. */