[PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[Andrew Cooper]

Switch its return type to bool to match its use, and simplify the ARM
implementation slightly.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c         |  6 ++----
 xen/arch/x86/mm.c         | 10 +++++-----
 xen/arch/x86/mm/p2m.c     |  2 +-
 xen/common/grant_table.c  | 12 ++++++------
 xen/include/asm-arm/p2m.h |  2 +-
 xen/include/asm-x86/mm.h  |  2 +-
 6 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 596283f..fbeed0e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1349,11 +1349,9 @@ int replace_grant_host_mapping(unsigned long addr, unsigned long mfn,
     return GNTST_okay;
 }
 
-int is_iomem_page(unsigned long mfn)
+bool is_iomem_page(mfn_t mfn)
 {
-    if ( !mfn_valid(mfn) )
-        return 1;
-    return 0;
+    return !mfn_valid(mfn_x(mfn));
 }
 
 void clear_and_clean_page(struct page_info *page)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f35e311..f87c08f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -789,15 +789,15 @@ get_##level##_linear_pagetable(                                             \
 }
 
 
-int is_iomem_page(unsigned long mfn)
+bool is_iomem_page(mfn_t mfn)
 {
     struct page_info *page;
 
-    if ( !mfn_valid(mfn) )
-        return 1;
+    if ( !mfn_valid(mfn_x(mfn)) )
+        return true;
 
     /* Caller must know that it is an iomem page, or a reference is held. */
-    page = mfn_to_page(mfn);
+    page = mfn_to_page(mfn_x(mfn));
     ASSERT((page->count_info & PGC_count_mask) != 0);
 
     return (page_get_owner(page) == dom_io);
@@ -1209,7 +1209,7 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
     struct domain    *pg_owner;
     struct vcpu      *v;
 
-    if ( !(l1e_get_flags(l1e) & _PAGE_PRESENT) || is_iomem_page(pfn) )
+    if ( !(l1e_get_flags(l1e) & _PAGE_PRESENT) || is_iomem_page(_mfn(pfn)) )
         return;
 
     page = mfn_to_page(pfn);
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 73d93ee..6548e9f 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1240,7 +1240,7 @@ int p2m_mem_paging_nominate(struct domain *d, unsigned long gfn)
         goto out;
 
     /* Check for io memory page */
-    if ( is_iomem_page(mfn_x(mfn)) )
+    if ( is_iomem_page(mfn) )
         goto out;
 
     /* Check page count and type */
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index a425a9e..1b7d236 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1259,7 +1259,7 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
 
     if ( op->flags & GNTMAP_device_map ) 
     {
-        if ( !is_iomem_page(act->frame) )
+        if ( !is_iomem_page(_mfn(act->frame)) )
         {
             if ( op->flags & GNTMAP_readonly )
                 put_page(pg);
@@ -1279,7 +1279,7 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
             goto act_release_out;
         }
 
-        if ( !is_iomem_page(op->frame) ) 
+        if ( !is_iomem_page(_mfn(op->frame)) )
         {
             if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
                 put_page_type(pg);
@@ -3314,7 +3314,7 @@ gnttab_release_mappings(
             {
                 BUG_ON(!(act->pin & GNTPIN_devr_mask));
                 act->pin -= GNTPIN_devr_inc;
-                if ( !is_iomem_page(act->frame) )
+                if ( !is_iomem_page(_mfn(act->frame)) )
                     put_page(pg);
             }
 
@@ -3323,7 +3323,7 @@ gnttab_release_mappings(
                 BUG_ON(!(act->pin & GNTPIN_hstr_mask));
                 act->pin -= GNTPIN_hstr_inc;
                 if ( gnttab_release_host_mappings(d) &&
-                     !is_iomem_page(act->frame) )
+                     !is_iomem_page(_mfn(act->frame)) )
                     put_page(pg);
             }
         }
@@ -3333,7 +3333,7 @@ gnttab_release_mappings(
             {
                 BUG_ON(!(act->pin & GNTPIN_devw_mask));
                 act->pin -= GNTPIN_devw_inc;
-                if ( !is_iomem_page(act->frame) )
+                if ( !is_iomem_page(_mfn(act->frame)) )
                     put_page_and_type(pg);
             }
 
@@ -3342,7 +3342,7 @@ gnttab_release_mappings(
                 BUG_ON(!(act->pin & GNTPIN_hstw_mask));
                 act->pin -= GNTPIN_hstw_inc;
                 if ( gnttab_release_host_mappings(d) &&
-                     !is_iomem_page(act->frame) )
+                     !is_iomem_page(_mfn(act->frame)) )
                 {
                     if ( gnttab_host_mapping_get_page_type(map, d, rd) )
                         put_page_type(pg);
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 5ddad34..0905a3f 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -314,7 +314,7 @@ static inline struct page_info *get_page_from_gfn(
 }
 
 int get_page_type(struct page_info *page, unsigned long type);
-int is_iomem_page(unsigned long mfn);
+bool is_iomem_page(mfn_t mfn);
 static inline int get_page_and_type(struct page_info *page,
                                     struct domain *domain,
                                     unsigned long type)
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 93a073d..f0efacb 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -326,7 +326,7 @@ void init_guest_l4_table(l4_pgentry_t[], const struct domain *,
 bool_t fill_ro_mpt(unsigned long mfn);
 void zap_ro_mpt(unsigned long mfn);
 
-int is_iomem_page(unsigned long mfn);
+bool is_iomem_page(mfn_t mfn);
 
 void clear_superpage_mark(struct page_info *page);
 
-- 
2.1.4


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

Re: [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[Julien Grall]

Hi Andrew,

On 06/02/17 13:55, Andrew Cooper wrote:
> Switch its return type to bool to match its use, and simplify the ARM
> implementation slightly.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Regards,

-- 
Julien Grall

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

Re: [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[George Dunlap]

On 06/02/17 13:55, Andrew Cooper wrote:
> Switch its return type to bool to match its use, and simplify the ARM
> implementation slightly.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/mm.c         |  6 ++----
>  xen/arch/x86/mm.c         | 10 +++++-----
>  xen/arch/x86/mm/p2m.c     |  2 +-
>  xen/common/grant_table.c  | 12 ++++++------
>  xen/include/asm-arm/p2m.h |  2 +-
>  xen/include/asm-x86/mm.h  |  2 +-
>  6 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 596283f..fbeed0e 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1349,11 +1349,9 @@ int replace_grant_host_mapping(unsigned long addr, unsigned long mfn,
>      return GNTST_okay;
>  }
>  
> -int is_iomem_page(unsigned long mfn)
> +bool is_iomem_page(mfn_t mfn)
>  {
> -    if ( !mfn_valid(mfn) )
> -        return 1;
> -    return 0;
> +    return !mfn_valid(mfn_x(mfn));
>  }
>  
>  void clear_and_clean_page(struct page_info *page)
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index f35e311..f87c08f 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -789,15 +789,15 @@ get_##level##_linear_pagetable(                                             \
>  }
>  
>  
> -int is_iomem_page(unsigned long mfn)
> +bool is_iomem_page(mfn_t mfn)
>  {
>      struct page_info *page;
>  
> -    if ( !mfn_valid(mfn) )
> -        return 1;
> +    if ( !mfn_valid(mfn_x(mfn)) )
> +        return true;
>  
>      /* Caller must know that it is an iomem page, or a reference is held. */
> -    page = mfn_to_page(mfn);
> +    page = mfn_to_page(mfn_x(mfn));
>      ASSERT((page->count_info & PGC_count_mask) != 0);
>  
>      return (page_get_owner(page) == dom_io);
> @@ -1209,7 +1209,7 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
>      struct domain    *pg_owner;
>      struct vcpu      *v;
>  
> -    if ( !(l1e_get_flags(l1e) & _PAGE_PRESENT) || is_iomem_page(pfn) )
> +    if ( !(l1e_get_flags(l1e) & _PAGE_PRESENT) || is_iomem_page(_mfn(pfn)) )
>          return;
>  
>      page = mfn_to_page(pfn);
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 73d93ee..6548e9f 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1240,7 +1240,7 @@ int p2m_mem_paging_nominate(struct domain *d, unsigned long gfn)
>          goto out;
>  
>      /* Check for io memory page */
> -    if ( is_iomem_page(mfn_x(mfn)) )
> +    if ( is_iomem_page(mfn) )
>          goto out;
>  
>      /* Check page count and type */
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index a425a9e..1b7d236 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1259,7 +1259,7 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
>  
>      if ( op->flags & GNTMAP_device_map ) 
>      {
> -        if ( !is_iomem_page(act->frame) )
> +        if ( !is_iomem_page(_mfn(act->frame)) )
>          {
>              if ( op->flags & GNTMAP_readonly )
>                  put_page(pg);
> @@ -1279,7 +1279,7 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
>              goto act_release_out;
>          }
>  
> -        if ( !is_iomem_page(op->frame) ) 
> +        if ( !is_iomem_page(_mfn(op->frame)) )
>          {
>              if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
>                  put_page_type(pg);
> @@ -3314,7 +3314,7 @@ gnttab_release_mappings(
>              {
>                  BUG_ON(!(act->pin & GNTPIN_devr_mask));
>                  act->pin -= GNTPIN_devr_inc;
> -                if ( !is_iomem_page(act->frame) )
> +                if ( !is_iomem_page(_mfn(act->frame)) )
>                      put_page(pg);
>              }
>  
> @@ -3323,7 +3323,7 @@ gnttab_release_mappings(
>                  BUG_ON(!(act->pin & GNTPIN_hstr_mask));
>                  act->pin -= GNTPIN_hstr_inc;
>                  if ( gnttab_release_host_mappings(d) &&
> -                     !is_iomem_page(act->frame) )
> +                     !is_iomem_page(_mfn(act->frame)) )
>                      put_page(pg);
>              }
>          }
> @@ -3333,7 +3333,7 @@ gnttab_release_mappings(
>              {
>                  BUG_ON(!(act->pin & GNTPIN_devw_mask));
>                  act->pin -= GNTPIN_devw_inc;
> -                if ( !is_iomem_page(act->frame) )
> +                if ( !is_iomem_page(_mfn(act->frame)) )
>                      put_page_and_type(pg);
>              }
>  
> @@ -3342,7 +3342,7 @@ gnttab_release_mappings(
>                  BUG_ON(!(act->pin & GNTPIN_hstw_mask));
>                  act->pin -= GNTPIN_hstw_inc;
>                  if ( gnttab_release_host_mappings(d) &&
> -                     !is_iomem_page(act->frame) )
> +                     !is_iomem_page(_mfn(act->frame)) )
>                  {
>                      if ( gnttab_host_mapping_get_page_type(map, d, rd) )
>                          put_page_type(pg);
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 5ddad34..0905a3f 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -314,7 +314,7 @@ static inline struct page_info *get_page_from_gfn(
>  }
>  
>  int get_page_type(struct page_info *page, unsigned long type);
> -int is_iomem_page(unsigned long mfn);
> +bool is_iomem_page(mfn_t mfn);
>  static inline int get_page_and_type(struct page_info *page,
>                                      struct domain *domain,
>                                      unsigned long type)
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index 93a073d..f0efacb 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -326,7 +326,7 @@ void init_guest_l4_table(l4_pgentry_t[], const struct domain *,
>  bool_t fill_ro_mpt(unsigned long mfn);
>  void zap_ro_mpt(unsigned long mfn);
>  
> -int is_iomem_page(unsigned long mfn);
> +bool is_iomem_page(mfn_t mfn);
>  
>  void clear_superpage_mark(struct page_info *page);
>  
> 


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

Re: [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[Jan Beulich]

>>> On 06.02.17 at 14:55, <andrew.cooper3@citrix.com> wrote:
> Switch its return type to bool to match its use, and simplify the ARM
> implementation slightly.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

And perhaps that's what should be used in epte_get_entry_emt()
instead of !mfn_valid() in David's patch. David, would you be okay
with your patch changed to that effect upon commit?

Jan


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

Re: [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1490 bytes --]

On Mon, 2017-02-06 at 07:26 -0700, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 06.02.17 at 14:55, <andrew.cooper3@citrix.com> wrote:
> > Switch its return type to bool to match its use, and simplify the
> > ARM
> > implementation slightly.
> > 
> > No functional change.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> And perhaps that's what should be used in epte_get_entry_emt()
> instead of !mfn_valid() in David's patch. David, would you be okay
> with your patch changed to that effect upon commit?

I don't think that works, at least not literally
s/!mfn_valid()/is_iomem_page()/

In my patch, mfn_valid() is checked *after* we've processed the
'direct_mmio' case that all MMIO should hit. In a sane world I think
it's *only* actually catching INVALID_MFN, and probably should never
match on any other value of mfn.

I don't quite understand why we pass 'direct_mmio' in as a separate
argument. Perhaps there's scope for doing a sanity check that
'direct_mmio == is_iomem_page(mfn)' — because when would that *not* be
true?

But sanity checks are of dubious utility because it must be noted that
we have no way to return failure for order==0 anyway; that 'return -1'
idiom is dangerous in epte_get_entry_emt(), and that's why I gave it an
extra comment.

So if we can use is_iomem_page() do we ditch the direct_mmio argument
to the function entirely?

-- 
dwmw2

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[Jan Beulich]

>>> On 06.02.17 at 15:48, <dwmw2@infradead.org> wrote:
> On Mon, 2017-02-06 at 07:26 -0700, Jan Beulich wrote:
>> > 
>> > > 
>> > > > 
>> > > > On 06.02.17 at 14:55, <andrew.cooper3@citrix.com> wrote:
>> > Switch its return type to bool to match its use, and simplify the
>> > ARM
>> > implementation slightly.
>> > 
>> > No functional change.
>> > 
>> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> 
>> And perhaps that's what should be used in epte_get_entry_emt()
>> instead of !mfn_valid() in David's patch. David, would you be okay
>> with your patch changed to that effect upon commit?
> 
> I don't think that works, at least not literally
> s/!mfn_valid()/is_iomem_page()/
> 
> In my patch, mfn_valid() is checked *after* we've processed the
> 'direct_mmio' case that all MMIO should hit. In a sane world I think
> it's *only* actually catching INVALID_MFN, and probably should never
> match on any other value of mfn.
> 
> I don't quite understand why we pass 'direct_mmio' in as a separate
> argument. Perhaps there's scope for doing a sanity check that
> 'direct_mmio == is_iomem_page(mfn)' — because when would that *not* be
> true?

Likely never. The reasons for why this was done the way it is
escape me. Adding VMX maintainers once again.

> But sanity checks are of dubious utility because it must be noted that
> we have no way to return failure for order==0 anyway; that 'return -1'
> idiom is dangerous in epte_get_entry_emt(), and that's why I gave it an
> extra comment.
> 
> So if we can use is_iomem_page() do we ditch the direct_mmio argument
> to the function entirely?

I guess we could, yes. I didn't carefully check though yet what the
caller(s) derive(s) it from.

Jan

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

Re: [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, February 06, 2017 11:38 PM
> 
> >>> On 06.02.17 at 15:48, <dwmw2@infradead.org> wrote:
> > On Mon, 2017-02-06 at 07:26 -0700, Jan Beulich wrote:
> >> >
> >> > >
> >> > > >
> >> > > > On 06.02.17 at 14:55, <andrew.cooper3@citrix.com> wrote:
> >> > Switch its return type to bool to match its use, and simplify the
> >> > ARM
> >> > implementation slightly.
> >> >
> >> > No functional change.
> >> >
> >> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> And perhaps that's what should be used in epte_get_entry_emt()
> >> instead of !mfn_valid() in David's patch. David, would you be okay
> >> with your patch changed to that effect upon commit?
> >
> > I don't think that works, at least not literally
> > s/!mfn_valid()/is_iomem_page()/
> >
> > In my patch, mfn_valid() is checked *after* we've processed the
> > 'direct_mmio' case that all MMIO should hit. In a sane world I think
> > it's *only* actually catching INVALID_MFN, and probably should never
> > match on any other value of mfn.
> >
> > I don't quite understand why we pass 'direct_mmio' in as a separate
> > argument. Perhaps there's scope for doing a sanity check that
> > 'direct_mmio == is_iomem_page(mfn)' — because when would that *not* be
> > true?
> 
> Likely never. The reasons for why this was done the way it is
> escape me. Adding VMX maintainers once again.

I'm not the original author of that code. Here is what I found from the code:

There are two places caring direct_mmio: resolve_misconfig and ept_
set_entry. It's decided upon p2m type:

      int emt = epte_get_entry_emt(p2m->domain, gfn, _mfn(e.mfn),
                                    level * EPT_TABLE_ORDER, &ipat,
                                    e.sa_p2mt == p2m_mmio_direct);

I'm not sure whether p2m_mmio_direct always makes is_iomem_page
true. If true then yes we may not require this parameter at all.

> 
> > But sanity checks are of dubious utility because it must be noted that
> > we have no way to return failure for order==0 anyway; that 'return -1'
> > idiom is dangerous in epte_get_entry_emt(), and that's why I gave it an
> > extra comment.
> >
> > So if we can use is_iomem_page() do we ditch the direct_mmio argument
> > to the function entirely?
> 
> I guess we could, yes. I didn't carefully check though yet what the
> caller(s) derive(s) it from.
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[Jan Beulich]

>>> On 08.02.17 at 08:31, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, February 06, 2017 11:38 PM
>> 
>> >>> On 06.02.17 at 15:48, <dwmw2@infradead.org> wrote:
>> > On Mon, 2017-02-06 at 07:26 -0700, Jan Beulich wrote:
>> >> >
>> >> > >
>> >> > > >
>> >> > > > On 06.02.17 at 14:55, <andrew.cooper3@citrix.com> wrote:
>> >> > Switch its return type to bool to match its use, and simplify the
>> >> > ARM
>> >> > implementation slightly.
>> >> >
>> >> > No functional change.
>> >> >
>> >> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> >>
>> >> And perhaps that's what should be used in epte_get_entry_emt()
>> >> instead of !mfn_valid() in David's patch. David, would you be okay
>> >> with your patch changed to that effect upon commit?
>> >
>> > I don't think that works, at least not literally
>> > s/!mfn_valid()/is_iomem_page()/
>> >
>> > In my patch, mfn_valid() is checked *after* we've processed the
>> > 'direct_mmio' case that all MMIO should hit. In a sane world I think
>> > it's *only* actually catching INVALID_MFN, and probably should never
>> > match on any other value of mfn.
>> >
>> > I don't quite understand why we pass 'direct_mmio' in as a separate
>> > argument. Perhaps there's scope for doing a sanity check that
>> > 'direct_mmio == is_iomem_page(mfn)' — because when would that *not* be
>> > true?
>> 
>> Likely never. The reasons for why this was done the way it is
>> escape me. Adding VMX maintainers once again.
> 
> I'm not the original author of that code. Here is what I found from the 
> code:
> 
> There are two places caring direct_mmio: resolve_misconfig and ept_
> set_entry. It's decided upon p2m type:
> 
>       int emt = epte_get_entry_emt(p2m->domain, gfn, _mfn(e.mfn),
>                                     level * EPT_TABLE_ORDER, &ipat,
>                                     e.sa_p2mt == p2m_mmio_direct);
> 
> I'm not sure whether p2m_mmio_direct always makes is_iomem_page
> true. If true then yes we may not require this parameter at all.

From an abstract perspective the two should always correlate. We
may want to take an intermediate step though and first only add
checking, and perhaps a release later remove the extra parameter
if the check never triggered for anyone.

Jan

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

Re: [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[Chao Gao]

On Wed, Feb 08, 2017 at 10:05:38AM -0700, Jan Beulich wrote:
>>>> On 08.02.17 at 08:31, <kevin.tian@intel.com> wrote:
>>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: Monday, February 06, 2017 11:38 PM
>>> 
>>> >>> On 06.02.17 at 15:48, <dwmw2@infradead.org> wrote:
>>> > On Mon, 2017-02-06 at 07:26 -0700, Jan Beulich wrote:
>>> >> >
>>> >> > >
>>> >> > > >
>>> >> > > > On 06.02.17 at 14:55, <andrew.cooper3@citrix.com> wrote:
>>> >> > Switch its return type to bool to match its use, and simplify the
>>> >> > ARM
>>> >> > implementation slightly.
>>> >> >
>>> >> > No functional change.
>>> >> >
>>> >> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> >>
>>> >> And perhaps that's what should be used in epte_get_entry_emt()
>>> >> instead of !mfn_valid() in David's patch. David, would you be okay
>>> >> with your patch changed to that effect upon commit?
>>> >
>>> > I don't think that works, at least not literally
>>> > s/!mfn_valid()/is_iomem_page()/
>>> >
>>> > In my patch, mfn_valid() is checked *after* we've processed the
>>> > 'direct_mmio' case that all MMIO should hit. In a sane world I think
>>> > it's *only* actually catching INVALID_MFN, and probably should never
>>> > match on any other value of mfn.
>>> >
>>> > I don't quite understand why we pass 'direct_mmio' in as a separate
>>> > argument. Perhaps there's scope for doing a sanity check that
>>> > 'direct_mmio == is_iomem_page(mfn)' — because when would that *not* be
>>> > true?
>>> 
>>> Likely never. The reasons for why this was done the way it is
>>> escape me. Adding VMX maintainers once again.
>> 
>> I'm not the original author of that code. Here is what I found from the 
>> code:
>> 
>> There are two places caring direct_mmio: resolve_misconfig and ept_
>> set_entry. It's decided upon p2m type:
>> 
>>       int emt = epte_get_entry_emt(p2m->domain, gfn, _mfn(e.mfn),
>>                                     level * EPT_TABLE_ORDER, &ipat,
>>                                     e.sa_p2mt == p2m_mmio_direct);
>> 
>> I'm not sure whether p2m_mmio_direct always makes is_iomem_page
>> true. If true then yes we may not require this parameter at all.
>
>From an abstract perspective the two should always correlate. We
>may want to take an intermediate step though and first only add
>checking, and perhaps a release later remove the extra parameter
>if the check never triggered for anyone.
>

I add one assertion to the original code, like below.

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 86c71be..3d9386a 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -784,6 +784,8 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, 
 
     if ( direct_mmio )
     {
+        ASSERT(direct_mmio == is_iomem_page(mfn)); 
+
         if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order )
             return MTRR_TYPE_UNCACHABLE;
         if ( order )

But when I try to create a hvm domain, it failed. logs are below.

(XEN) Assertion 'direct_mmio == is_iomem_page(mfn)' failed at mtrr.c:787
(XEN) ----[ Xen-4.9-unstable  x86_64  debug=y   Not tainted ]----
(XEN) CPU:    44
(XEN) RIP:    e008:[<ffff82d0804090bc>] epte_get_entry_emt+0xbc/0x24e [EPTE_EMT_v2]
(XEN) RFLAGS: 0000000000010297   CONTEXT: hypervisor (d0v39)
(XEN) rax: ffff832077d7e000   rbx: 00000000010107ba   rcx: ffff830000000000
(XEN) rdx: 0000000002077d7e   rsi: 00000000010107ba   rdi: 00000000010107ba
(XEN) rbp: ffff832076857a68   rsp: ffff832076857a18   r8:  ffff832076857af7
(XEN) r9:  0000000000000001   r10: 0000000000000000   r11: 0000000000000000
(XEN) r12: ffff832077d7e000   r13: 0000000000000000   r14: 0000000000000000
(XEN) r15: ffff832076857af7   cr0: 0000000080050033   cr4: 00000000001526e0
(XEN) cr3: 0000000899980000   cr2: 00007f35eb86d8d1
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen code around <ffff82d0804090bc> (epte_get_entry_emt+0xbc/0x24e [EPTE_EMT_v2]):
(XEN)  37 4d d7 ff 3c 01 74 02 <0f> 0b 49 33 9c 24 40 06 00 00 44 89 e9 48 d3 eb
(XEN) Xen stack trace from rsp=ffff832076857a18:
(XEN)    ffff832076857b00 00000000000fee00 00000000010107ba 0000000100000000
(XEN)    ffff832076857a68 ffff832077da8b90 0000000000000000 ffff832076857b00
(XEN)    ffff8200403c8000 0000000000000000 ffff832076857b38 ffff82d0802101b5
(XEN)    ffff832000000005 0000000000000206 0000000000000000 ffff832076857aa8
(XEN)    ffff82d08013452b ffff831000000000 0000000000000001 0000000700000206
(XEN)    ffff832077d7e000 00000000000fee00 00000000010107ba 0000000000000005
(XEN)    ffff832077da8b90 0000000000000000 8000000000000000 00ff832077da8b90
(XEN)    0000000000000000 ffff8200403c8000 0000000000000001 00000000010107ba
(XEN)    0000000000000000 0000000000000001 00000000000fee00 ffff832077da8b90
(XEN)    ffff832076857b98 ffff82d080204d62 00000000ffffffff 0000000776857b68
(XEN)    ffff832077d7e000 0000000000000005 ffff832076857b98 ffff832077d7e000
(XEN)    00000000010107ba ffff832077da8b90 ffffffffffffffff 0000000000000007
(XEN)    ffff832076857c18 ffff82d080205408 0000000000000000 0000002800000021
(XEN)    0000000776857bc8 00000000010107ba 00000000000fee00 0000000000000005
(XEN)    000000128013452b 0000000000000004 ffff832076857c38 0000000000000000
(XEN)    00000000010107ba ffff832077d7e000 00000000000fee00 0000000000000007
(XEN)    ffff832076857c58 ffff82d080206fa5 ffff832076857c58 ffff832077d7e000
(XEN)    ffff82e02020f740 00000000010107ba 0000000000000001 0000000000000000
(XEN)    ffff832076857c88 ffff82d0801f8ff9 ffff832077d7e000 ffff832077d7e000
(XEN)    0000000000000000 0000000000000001 ffff832076857ca8 ffff82d0801d2a28
(XEN) Xen call trace:
(XEN)    [<ffff82d0804090bc>] epte_get_entry_emt+0xbc/0x24e [EPTE_EMT_v2]
(XEN)    [<ffff82d0802101b5>] p2m-ept.c#ept_set_entry+0x2f3/0x798
(XEN)    [<ffff82d080204d62>] p2m_set_entry+0xc9/0x10a
(XEN)    [<ffff82d080205408>] p2m.c#set_typed_p2m_entry+0x447/0x65f
(XEN)    [<ffff82d080206fa5>] set_mmio_p2m_entry+0x63/0x72
(XEN)    [<ffff82d0801f8ff9>] vmx.c#vmx_domain_initialise+0xb6/0xcd
(XEN)    [<ffff82d0801d2a28>] hvm_domain_initialise+0x2da/0x368
(XEN)    [<ffff82d08016b567>] arch_domain_create+0x4b0/0x68e
(XEN)    [<ffff82d08010520c>] domain_create+0x3ee/0x5a7
(XEN)    [<ffff82d080103690>] do_domctl+0x52e/0x1bae
(XEN)    [<ffff82d08017123f>] pv_hypercall+0x1e7/0x3fb
(XEN)    [<ffff82d080247026>] entry.o#test_all_events+0/0x30
(XEN) 
(XEN) 
(XEN) ****************************************
(XEN) Panic on CPU 44:
(XEN) Assertion 'direct_mmio == is_iomem_page(mfn)' failed at mtrr.c:787
(XEN) ****************************************

Thanks
Chao

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

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

Re: [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[Jan Beulich]

>>> On 09.02.17 at 09:27, <chao.gao@intel.com> wrote:
> On Wed, Feb 08, 2017 at 10:05:38AM -0700, Jan Beulich wrote:
>>>>> On 08.02.17 at 08:31, <kevin.tian@intel.com> wrote:
>>>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>>>> Sent: Monday, February 06, 2017 11:38 PM
>>>> 
>>>> >>> On 06.02.17 at 15:48, <dwmw2@infradead.org> wrote:
>>>> > On Mon, 2017-02-06 at 07:26 -0700, Jan Beulich wrote:
>>>> >> >
>>>> >> > >
>>>> >> > > >
>>>> >> > > > On 06.02.17 at 14:55, <andrew.cooper3@citrix.com> wrote:
>>>> >> > Switch its return type to bool to match its use, and simplify the
>>>> >> > ARM
>>>> >> > implementation slightly.
>>>> >> >
>>>> >> > No functional change.
>>>> >> >
>>>> >> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>> >>
>>>> >> And perhaps that's what should be used in epte_get_entry_emt()
>>>> >> instead of !mfn_valid() in David's patch. David, would you be okay
>>>> >> with your patch changed to that effect upon commit?
>>>> >
>>>> > I don't think that works, at least not literally
>>>> > s/!mfn_valid()/is_iomem_page()/
>>>> >
>>>> > In my patch, mfn_valid() is checked *after* we've processed the
>>>> > 'direct_mmio' case that all MMIO should hit. In a sane world I think
>>>> > it's *only* actually catching INVALID_MFN, and probably should never
>>>> > match on any other value of mfn.
>>>> >
>>>> > I don't quite understand why we pass 'direct_mmio' in as a separate
>>>> > argument. Perhaps there's scope for doing a sanity check that
>>>> > 'direct_mmio == is_iomem_page(mfn)' — because when would that *not* be
>>>> > true?
>>>> 
>>>> Likely never. The reasons for why this was done the way it is
>>>> escape me. Adding VMX maintainers once again.
>>> 
>>> I'm not the original author of that code. Here is what I found from the 
>>> code:
>>> 
>>> There are two places caring direct_mmio: resolve_misconfig and ept_
>>> set_entry. It's decided upon p2m type:
>>> 
>>>       int emt = epte_get_entry_emt(p2m->domain, gfn, _mfn(e.mfn),
>>>                                     level * EPT_TABLE_ORDER, &ipat,
>>>                                     e.sa_p2mt == p2m_mmio_direct);
>>> 
>>> I'm not sure whether p2m_mmio_direct always makes is_iomem_page
>>> true. If true then yes we may not require this parameter at all.
>>
>>From an abstract perspective the two should always correlate. We
>>may want to take an intermediate step though and first only add
>>checking, and perhaps a release later remove the extra parameter
>>if the check never triggered for anyone.
>>
> 
> I add one assertion to the original code, like below.
> 
> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> index 86c71be..3d9386a 100644
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -784,6 +784,8 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, 
>  
>      if ( direct_mmio )
>      {
> +        ASSERT(direct_mmio == is_iomem_page(mfn)); 
> +
>          if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order )
>              return MTRR_TYPE_UNCACHABLE;
>          if ( order )
> 
> But when I try to create a hvm domain, it failed. logs are below.

Considering the context of the change above, I'm not surprised:
You've very likely hit the APIC access MFN.

Jan

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

Re: [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[Chao Gao]

On Thu, Feb 09, 2017 at 01:56:14AM -0700, Jan Beulich wrote:
>>>> On 09.02.17 at 09:27, <chao.gao@intel.com> wrote:
>> On Wed, Feb 08, 2017 at 10:05:38AM -0700, Jan Beulich wrote:
>>>>>> On 08.02.17 at 08:31, <kevin.tian@intel.com> wrote:
>>>>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>> Sent: Monday, February 06, 2017 11:38 PM
>>>>> 
>>>>> >>> On 06.02.17 at 15:48, <dwmw2@infradead.org> wrote:
>>>>> > On Mon, 2017-02-06 at 07:26 -0700, Jan Beulich wrote:
>>>>> >> >
>>>>> >> > >
>>>>> >> > > >
>>>>> >> > > > On 06.02.17 at 14:55, <andrew.cooper3@citrix.com> wrote:
>>>>> >> > Switch its return type to bool to match its use, and simplify the
>>>>> >> > ARM
>>>>> >> > implementation slightly.
>>>>> >> >
>>>>> >> > No functional change.
>>>>> >> >
>>>>> >> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>> >>
>>>>> >> And perhaps that's what should be used in epte_get_entry_emt()
>>>>> >> instead of !mfn_valid() in David's patch. David, would you be okay
>>>>> >> with your patch changed to that effect upon commit?
>>>>> >
>>>>> > I don't think that works, at least not literally
>>>>> > s/!mfn_valid()/is_iomem_page()/
>>>>> >
>>>>> > In my patch, mfn_valid() is checked *after* we've processed the
>>>>> > 'direct_mmio' case that all MMIO should hit. In a sane world I think
>>>>> > it's *only* actually catching INVALID_MFN, and probably should never
>>>>> > match on any other value of mfn.
>>>>> >
>>>>> > I don't quite understand why we pass 'direct_mmio' in as a separate
>>>>> > argument. Perhaps there's scope for doing a sanity check that
>>>>> > 'direct_mmio == is_iomem_page(mfn)' — because when would that *not* be
>>>>> > true?
>>>>> 
>>>>> Likely never. The reasons for why this was done the way it is
>>>>> escape me. Adding VMX maintainers once again.
>>>> 
>>>> I'm not the original author of that code. Here is what I found from the 
>>>> code:
>>>> 
>>>> There are two places caring direct_mmio: resolve_misconfig and ept_
>>>> set_entry. It's decided upon p2m type:
>>>> 
>>>>       int emt = epte_get_entry_emt(p2m->domain, gfn, _mfn(e.mfn),
>>>>                                     level * EPT_TABLE_ORDER, &ipat,
>>>>                                     e.sa_p2mt == p2m_mmio_direct);
>>>> 
>>>> I'm not sure whether p2m_mmio_direct always makes is_iomem_page
>>>> true. If true then yes we may not require this parameter at all.
>>>
>>>From an abstract perspective the two should always correlate. We
>>>may want to take an intermediate step though and first only add
>>>checking, and perhaps a release later remove the extra parameter
>>>if the check never triggered for anyone.
>>>
>> 
>> I add one assertion to the original code, like below.
>> 
>> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
>> index 86c71be..3d9386a 100644
>> --- a/xen/arch/x86/hvm/mtrr.c
>> +++ b/xen/arch/x86/hvm/mtrr.c
>> @@ -784,6 +784,8 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, 
>>  
>>      if ( direct_mmio )
>>      {
>> +        ASSERT(direct_mmio == is_iomem_page(mfn)); 
>> +
>>          if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order )
>>              return MTRR_TYPE_UNCACHABLE;
>>          if ( order )
>> 
>> But when I try to create a hvm domain, it failed. logs are below.
>
>Considering the context of the change above, I'm not surprised:
>You've very likely hit the APIC access MFN.
>

Jan is right. When the assertion fails, the value of gfn is 0xfee00.

Do you mean that is_iomem_page() is equal to direct_mmio except some
corner cases such as APIC access MFN and INVALID_MFN( any others? ) ?

Thanks
Chao

>Jan

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

Re: [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 495 bytes --]

On Thu, 2017-02-09 at 11:54 +0800, Chao Gao wrote:
> 
> Jan is right. When the assertion fails, the value of gfn is 0xfee00.
> 
> Do you mean that is_iomem_page() is equal to direct_mmio except some
> corner cases such as APIC access MFN and INVALID_MFN( any others? ) ?

That was the hypothesis, and it seems reasonable.

But I also note you're only testing that hypothesis in the
direct_mmio==1 case. You don't test the hypothesis that if
direct_mmio==0, then so is is_iomem_page().

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 961 bytes --]

On Thu, 2017-02-09 at 01:56 -0700, Jan Beulich wrote:
> 
> > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> > index 86c71be..3d9386a 100644
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -784,6 +784,8 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, 
> >  
> >      if ( direct_mmio )
> >      {
> > +        ASSERT(direct_mmio == is_iomem_page(mfn)); 
> > +
> >          if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order )
> >              return MTRR_TYPE_UNCACHABLE;
> >          if ( order )
> > 
> > But when I try to create a hvm domain, it failed. logs are below.
> 
> Considering the context of the change above, I'm not surprised:
> You've very likely hit the APIC access MFN.

You'll probably hit it for INVALID_MFN too on an unmap. And *do* make
it print the offending address when it triggers. :)

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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