| Message ID | 1386963461-6520-2-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 2013-12-13 at 19:37 +0000, Julien Grall wrote: > From: Mukesh Rathor <mukesh.rathor@oracle.com> > > In this patch, a new type p2m_map_foreign is introduced for pages > that toolstack on PVH dom0 maps from foreign domains that its creating > or supporting during it's run time. > > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Keir Fraser <keir@xen.org> > Cc: George Dunlap <george.dunlap@eu.citrix.com> > > --- > Changes in v4: > - Use the patch #4 from dom0 pvh v6 series. We need it for ARM Is this really the right patch? I'd have thought it would need to include some generic/!x86 bits but: > --- > xen/arch/x86/mm/p2m-ept.c | 1 + > xen/arch/x86/mm/p2m-pt.c | 1 + > xen/arch/x86/mm/p2m.c | 28 ++++++++++++++++++++-------- > xen/include/asm-x86/p2m.h | 4 ++++ > 4 files changed, 26 insertions(+), 8 deletions(-) Don't you want patch #6 from the PVH series instead? Ian.
On 12/13/2013 11:19 PM, Ian Campbell wrote: > On Fri, 2013-12-13 at 19:37 +0000, Julien Grall wrote: >> From: Mukesh Rathor <mukesh.rathor@oracle.com> >> >> In this patch, a new type p2m_map_foreign is introduced for pages >> that toolstack on PVH dom0 maps from foreign domains that its creating >> or supporting during it's run time. >> >> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Keir Fraser <keir@xen.org> >> Cc: George Dunlap <george.dunlap@eu.citrix.com> >> >> --- >> Changes in v4: >> - Use the patch #4 from dom0 pvh v6 series. We need it for ARM > > Is this really the right patch? I'd have thought it would need to > include some generic/!x86 bits but: This patch introduce p2m_map_foreign type. I think it's safe to push it without the other patches. Moreover, it will avoid dummy #define p2m_is_foreign in my patch #8. > >> --- >> xen/arch/x86/mm/p2m-ept.c | 1 + >> xen/arch/x86/mm/p2m-pt.c | 1 + >> xen/arch/x86/mm/p2m.c | 28 ++++++++++++++++++++-------- >> xen/include/asm-x86/p2m.h | 4 ++++ >> 4 files changed, 26 insertions(+), 8 deletions(-) > > Don't you want patch #6 from the PVH series instead? I made a mistake in my cover letter. I was talking about patch #6 not #7. I have taken the common code in merge into my patch #8.
On Sat, 2013-12-14 at 00:00 +0000, Julien Grall wrote: > > On 12/13/2013 11:19 PM, Ian Campbell wrote: > > On Fri, 2013-12-13 at 19:37 +0000, Julien Grall wrote: > >> From: Mukesh Rathor <mukesh.rathor@oracle.com> > >> > >> In this patch, a new type p2m_map_foreign is introduced for pages > >> that toolstack on PVH dom0 maps from foreign domains that its creating > >> or supporting during it's run time. > >> > >> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com> > >> Cc: Jan Beulich <jbeulich@suse.com> > >> Cc: Keir Fraser <keir@xen.org> > >> Cc: George Dunlap <george.dunlap@eu.citrix.com> > >> > >> --- > >> Changes in v4: > >> - Use the patch #4 from dom0 pvh v6 series. We need it for ARM > > > > Is this really the right patch? I'd have thought it would need to > > include some generic/!x86 bits but: > > This patch introduce p2m_map_foreign type. I think it's safe to push it > without the other patches. Is it really that simple? I suppose there is nothing to create a foreign mapping on x86 yet so this is harmless? I'm mostly concerned because the big remaining concern about this stuff on x86 relates to the reference counting of foreign p2m entries. I'll defer to the x86 chaps about whether that conversation is directly related to or impacts this patch or not. > Moreover, it will avoid dummy #define > p2m_is_foreign in my patch #8. > > > > >> --- > >> xen/arch/x86/mm/p2m-ept.c | 1 + > >> xen/arch/x86/mm/p2m-pt.c | 1 + > >> xen/arch/x86/mm/p2m.c | 28 ++++++++++++++++++++-------- > >> xen/include/asm-x86/p2m.h | 4 ++++ > >> 4 files changed, 26 insertions(+), 8 deletions(-) > > > > Don't you want patch #6 from the PVH series instead? > > I made a mistake in my cover letter. I was talking about patch #6 not > #7. I have taken the common code in merge into my patch #8. OK.
At 10:45 +0000 on 16 Dec (1387187114), Ian Campbell wrote: > On Sat, 2013-12-14 at 00:00 +0000, Julien Grall wrote: > > > > On 12/13/2013 11:19 PM, Ian Campbell wrote: > > > On Fri, 2013-12-13 at 19:37 +0000, Julien Grall wrote: > > >> From: Mukesh Rathor <mukesh.rathor@oracle.com> > > >> > > >> In this patch, a new type p2m_map_foreign is introduced for pages > > >> that toolstack on PVH dom0 maps from foreign domains that its creating > > >> or supporting during it's run time. > > >> > > >> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com> > > >> Cc: Jan Beulich <jbeulich@suse.com> > > >> Cc: Keir Fraser <keir@xen.org> > > >> Cc: George Dunlap <george.dunlap@eu.citrix.com> > > >> > > >> --- > > >> Changes in v4: > > >> - Use the patch #4 from dom0 pvh v6 series. We need it for ARM > > > > > > Is this really the right patch? I'd have thought it would need to > > > include some generic/!x86 bits but: > > > > This patch introduce p2m_map_foreign type. I think it's safe to push it > > without the other patches. > > Is it really that simple? I suppose there is nothing to create a foreign > mapping on x86 yet so this is harmless? Presumably you're about to add some common code that creates foreign mappings -- otherwise there would be no need to take the x86 patch. In that case, it's not really safe to take only part of the x86 implementation. It'd be better to stub out any x86 interfaces with explicit errors, to be removed when x86 supports this properly. Tim.
On 12/16/2013 10:45 AM, Ian Campbell wrote: > On Sat, 2013-12-14 at 00:00 +0000, Julien Grall wrote: >> On 12/13/2013 11:19 PM, Ian Campbell wrote: >>> On Fri, 2013-12-13 at 19:37 +0000, Julien Grall wrote: >>>> From: Mukesh Rathor <mukesh.rathor@oracle.com> >>>> >>>> In this patch, a new type p2m_map_foreign is introduced for pages >>>> that toolstack on PVH dom0 maps from foreign domains that its creating >>>> or supporting during it's run time. >>>> >>>> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com> >>>> Cc: Jan Beulich <jbeulich@suse.com> >>>> Cc: Keir Fraser <keir@xen.org> >>>> Cc: George Dunlap <george.dunlap@eu.citrix.com> >>>> >>>> --- >>>> Changes in v4: >>>> - Use the patch #4 from dom0 pvh v6 series. We need it for ARM >>> Is this really the right patch? I'd have thought it would need to >>> include some generic/!x86 bits but: >> This patch introduce p2m_map_foreign type. I think it's safe to push it >> without the other patches. > Is it really that simple? I suppose there is nothing to create a foreign > mapping on x86 yet so this is harmless? > > I'm mostly concerned because the big remaining concern about this stuff > on x86 relates to the reference counting of foreign p2m entries. I'll > defer to the x86 chaps about whether that conversation is directly > related to or impacts this patch or not. Do you have the reference counting for the new p2m type sorted out sufficiently? -George
On 12/16/2013 10:59 AM, Tim Deegan wrote: > At 10:45 +0000 on 16 Dec (1387187114), Ian Campbell wrote: >> On Sat, 2013-12-14 at 00:00 +0000, Julien Grall wrote: >>> >>> On 12/13/2013 11:19 PM, Ian Campbell wrote: >>>> On Fri, 2013-12-13 at 19:37 +0000, Julien Grall wrote: >>>>> From: Mukesh Rathor <mukesh.rathor@oracle.com> >>>>> >>>>> In this patch, a new type p2m_map_foreign is introduced for pages >>>>> that toolstack on PVH dom0 maps from foreign domains that its creating >>>>> or supporting during it's run time. >>>>> >>>>> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com> >>>>> Cc: Jan Beulich <jbeulich@suse.com> >>>>> Cc: Keir Fraser <keir@xen.org> >>>>> Cc: George Dunlap <george.dunlap@eu.citrix.com> >>>>> >>>>> --- >>>>> Changes in v4: >>>>> - Use the patch #4 from dom0 pvh v6 series. We need it for ARM >>>> >>>> Is this really the right patch? I'd have thought it would need to >>>> include some generic/!x86 bits but: >>> >>> This patch introduce p2m_map_foreign type. I think it's safe to push it >>> without the other patches. >> >> Is it really that simple? I suppose there is nothing to create a foreign >> mapping on x86 yet so this is harmless? > > Presumably you're about to add some common code that creates foreign > mappings -- otherwise there would be no need to take the x86 patch. > In that case, it's not really safe to take only part of the x86 > implementation. It'd be better to stub out any x86 interfaces with > explicit errors, to be removed when x86 supports this properly. This patch is no longer necessary. I have reworked my patch #8 to remove modification in common code.
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 92d9e2d..08d1d72 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -75,6 +75,7 @@ static void ept_p2m_type_to_flags(ept_entry_t *entry, p2m_type_t type, p2m_acces entry->w = 0; break; case p2m_grant_map_rw: + case p2m_map_foreign: entry->r = entry->w = 1; entry->x = 0; break; diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index a1d5650..09b60ce 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -89,6 +89,7 @@ static unsigned long p2m_type_to_flags(p2m_type_t t, mfn_t mfn) case p2m_ram_rw: return flags | P2M_BASE_FLAGS | _PAGE_RW; case p2m_grant_map_rw: + case p2m_map_foreign: return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT; case p2m_mmio_direct: if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)) ) diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 8f380ed..0659ef1 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -525,7 +525,7 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn, for ( i = 0; i < (1UL << page_order); i++ ) { mfn_return = p2m->get_entry(p2m, gfn + i, &t, &a, 0, NULL); - if ( !p2m_is_grant(t) && !p2m_is_shared(t) ) + if ( !p2m_is_grant(t) && !p2m_is_shared(t) && !p2m_is_foreign(t) ) set_gpfn_from_mfn(mfn+i, INVALID_M2P_ENTRY); ASSERT( !p2m_is_valid(t) || mfn + i == mfn_x(mfn_return) ); } @@ -756,10 +756,9 @@ void p2m_change_type_range(struct domain *d, p2m_unlock(p2m); } - - -int -set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn) +static int +set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn, + p2m_type_t gfn_p2mt) { int rc = 0; p2m_access_t a; @@ -784,16 +783,29 @@ set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn) set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY); } - P2M_DEBUG("set mmio %lx %lx\n", gfn, mfn_x(mfn)); - rc = set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_mmio_direct, p2m->default_access); + P2M_DEBUG("set %d %lx %lx\n", gfn_p2mt, gfn, mfn_x(mfn)); + rc = set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, gfn_p2mt, + p2m->default_access); gfn_unlock(p2m, gfn, 0); if ( 0 == rc ) gdprintk(XENLOG_ERR, - "set_mmio_p2m_entry: set_p2m_entry failed! mfn=%08lx\n", + "%s: set_p2m_entry failed! mfn=%08lx\n", __func__, mfn_x(get_gfn_query_unlocked(p2m->domain, gfn, &ot))); return rc; } +/* Returns: True for success. */ +int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn) +{ + return set_typed_p2m_entry(d, gfn, mfn, p2m_map_foreign); +} + +int +set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn) +{ + return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct); +} + int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn) { diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index f4e7253..d5d6391 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -70,6 +70,7 @@ typedef enum { p2m_ram_paging_in = 11, /* Memory that is being paged in */ p2m_ram_shared = 12, /* Shared or sharable memory */ p2m_ram_broken = 13, /* Broken page, access cause domain crash */ + p2m_map_foreign = 14, /* ram pages from foreign domain */ } p2m_type_t; /* @@ -180,6 +181,7 @@ typedef unsigned int p2m_query_t; #define p2m_is_sharable(_t) (p2m_to_mask(_t) & P2M_SHARABLE_TYPES) #define p2m_is_shared(_t) (p2m_to_mask(_t) & P2M_SHARED_TYPES) #define p2m_is_broken(_t) (p2m_to_mask(_t) & P2M_BROKEN_TYPES) +#define p2m_is_foreign(_t) (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign)) /* Per-p2m-table state */ struct p2m_domain { @@ -510,6 +512,8 @@ p2m_type_t p2m_change_type(struct domain *d, unsigned long gfn, int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn); int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn); +/* Set foreign mfn in the current guest's p2m table. */ +int set_foreign_p2m_entry(struct domain *domp, unsigned long gfn, mfn_t mfn); /* * Populate-on-demand