Re: [Xen-devel] [PATCH v4 1/4] x86/mm: split p2m ioreq server pages special handling into helper

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 19, 2019 at 01:49:18AM -0700, Jan Beulich wrote:
> >>> On 18.02.19 at 18:27, <roger.pau@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/mm/shadow/common.c
> > +++ b/xen/arch/x86/mm/shadow/common.c
> > @@ -3188,6 +3188,9 @@ shadow_write_p2m_entry(struct domain *d, unsigned 
> > long gfn,
> >      if ( likely(d->arch.paging.shadow.total_pages != 0) )
> >           sh_unshadow_for_p2m_change(d, gfn, p, new, level);
> >  
> > +    p2m_entry_modify(p2m_get_hostp2m(d), 
> > p2m_flags_to_type(l1e_get_flags(new)),
> > +                     p2m_flags_to_type(l1e_get_flags(*p)), level);
> > +
> >      /* Update the entry with new content */
> >      safe_write_pte(p, new);
> 
> Strictly speaking you should Cc Tim for this change.

sorry, I guess I didn't rerun get_maintainer after changing the patch.

> Also at this example (a possible issue elsewhere as well) - is
> acting on the host P2M (only) really the right thing to do here?

I've wondered the same while working on this, what's even worse is
that paging_write_p2m_entry takes a p2m as it's first argument, but
then the write_p2m_entry expect a domain instead of a p2m.

I have the following patch that pushes the p2m up to the
write_p2m_entry handlers, thus removing the need to do a
p2m_get_hostp2m in the write_p2m_entry handlers.

I'm planning to add this as a pre-patch to my series.

Thanks, Roger.
---8<---
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 3d651b94c3..28fe48d158 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -709,9 +709,10 @@ static void hap_update_paging_modes(struct vcpu *v)
 }
 
 static void
-hap_write_p2m_entry(struct domain *d, unsigned long gfn, l1_pgentry_t *p,
+hap_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, l1_pgentry_t *p,
                     l1_pgentry_t new, unsigned int level)
 {
+    struct domain *d = p2m->domain;
     uint32_t old_flags;
     bool_t flush_nestedp2m = 0;
 
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index d5836eb688..e6ed3006fe 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -941,7 +941,7 @@ void paging_write_p2m_entry(struct p2m_domain *p2m, 
unsigned long gfn,
     if ( v->domain != d )
         v = d->vcpu ? d->vcpu[0] : NULL;
     if ( likely(v && paging_mode_enabled(d) && paging_get_hostmode(v) != NULL) 
)
-        paging_get_hostmode(v)->write_p2m_entry(d, gfn, p, new, level);
+        paging_get_hostmode(v)->write_p2m_entry(p2m, gfn, p, new, level);
     else
         safe_write_pte(p, new);
 }
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 07840ff727..6c67ef4996 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3177,10 +3177,12 @@ static void sh_unshadow_for_p2m_change(struct domain 
*d, unsigned long gfn,
 }
 
 void
-shadow_write_p2m_entry(struct domain *d, unsigned long gfn,
+shadow_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
                        l1_pgentry_t *p, l1_pgentry_t new,
                        unsigned int level)
 {
+    struct domain *d = p2m->domain;
+
     paging_lock(d);
 
     /* If there are any shadows, update them.  But if shadow_teardown()
diff --git a/xen/arch/x86/mm/shadow/none.c b/xen/arch/x86/mm/shadow/none.c
index 4de645a433..316002771d 100644
--- a/xen/arch/x86/mm/shadow/none.c
+++ b/xen/arch/x86/mm/shadow/none.c
@@ -60,7 +60,7 @@ static void _update_paging_modes(struct vcpu *v)
     ASSERT_UNREACHABLE();
 }
 
-static void _write_p2m_entry(struct domain *d, unsigned long gfn,
+static void _write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
                              l1_pgentry_t *p, l1_pgentry_t new,
                              unsigned int level)
 {
diff --git a/xen/arch/x86/mm/shadow/private.h b/xen/arch/x86/mm/shadow/private.h
index e8ed7ac714..0aaed1edfc 100644
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -372,7 +372,7 @@ extern int sh_remove_write_access(struct domain *d, mfn_t 
readonly_mfn,
                                   unsigned long fault_addr);
 
 /* Functions that atomically write PT/P2M entries and update state */
-void shadow_write_p2m_entry(struct domain *d, unsigned long gfn,
+void shadow_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
                             l1_pgentry_t *p, l1_pgentry_t new,
                             unsigned int level);
 
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index fdcc22844b..7ec09d7b11 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -124,7 +124,8 @@ struct paging_mode {
     void          (*update_cr3            )(struct vcpu *v, int do_locking,
                                             bool noflush);
     void          (*update_paging_modes   )(struct vcpu *v);
-    void          (*write_p2m_entry       )(struct domain *d, unsigned long 
gfn,
+    void          (*write_p2m_entry       )(struct p2m_domain *p2m,
+                                            unsigned long gfn,
                                             l1_pgentry_t *p, l1_pgentry_t new,
                                             unsigned int level);
 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel