| Message ID | 20170905113716.3960-2-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 05, 2017 at 12:37:05PM +0100, Paul Durrant wrote: > In the case where a PV domain is mapping guest resources then it needs make > the HYPERVISOR_mmu_update call using DOMID_SELF, rather than the guest > domid, so that the passed in gmfn values are correctly treated as mfns > rather than gfns present in the guest p2m. > > This patch removes a check which currently disallows mapping of a page when > the owner of the page tables matches the domain passed to > HYPERVISOR_mmu_update, but that domain is not the real owner of the page. > The check was introduced by patch d3c6a215ca9 ("x86: Clean up > get_page_from_l1e() to correctly distinguish between owner-of-pte and > owner-of-data-page in all cases") but it's not clear why it was needed. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > --- > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > --- > xen/arch/x86/mm.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index c94f1e5406..bd8aeac59e 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -1024,12 +1024,15 @@ get_page_from_l1e( > (real_pg_owner != dom_cow) ) ) > { > /* > - * Let privileged domains transfer the right to map their target > - * domain's pages. This is used to allow stub-domain pvfb export to > - * dom0, until pvfb supports granted mappings. At that time this > - * minor hack can go away. > + * If the real page owner is not the domain specified in the > + * hypercall then establish that the specified domain has > + * mapping privilege over the page owner. > + * This is used to allow stub-domain pvfb export to dom0. It is > + * also used to allow a privileged PV domain to map mfns using > + * DOMID_SELF, which is needed for mapping guest resources such > + * grant table frames. > */ > - if ( (real_pg_owner == NULL) || (pg_owner == l1e_owner) || > + if ( (real_pg_owner == NULL) || I still can't quite figure out if it is safe to remove the check. Looking at the rest of the series, you already have the foreign domid to hand when you call the get_resource hypercall. What is wrong with using that directly? Why do you need DOMID_SELF in the first place? > xsm_priv_mapping(XSM_TARGET, pg_owner, real_pg_owner) ) > { > gdprintk(XENLOG_WARNING, > -- > 2.11.0 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
> -----Original Message----- > From: Wei Liu [mailto:wei.liu2@citrix.com] > Sent: 07 September 2017 12:02 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Jan Beulich <jbeulich@suse.com>; Wei Liu > <wei.liu2@citrix.com> > Subject: Re: [Xen-devel] [PATCH v4 01/12] x86/mm: allow a privileged PV > domain to map guest mfns > > On Tue, Sep 05, 2017 at 12:37:05PM +0100, Paul Durrant wrote: > > In the case where a PV domain is mapping guest resources then it needs > make > > the HYPERVISOR_mmu_update call using DOMID_SELF, rather than the > guest > > domid, so that the passed in gmfn values are correctly treated as mfns > > rather than gfns present in the guest p2m. > > > > This patch removes a check which currently disallows mapping of a page > when > > the owner of the page tables matches the domain passed to > > HYPERVISOR_mmu_update, but that domain is not the real owner of the > page. > > The check was introduced by patch d3c6a215ca9 ("x86: Clean up > > get_page_from_l1e() to correctly distinguish between owner-of-pte and > > owner-of-data-page in all cases") but it's not clear why it was needed. > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > --- > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > --- > > xen/arch/x86/mm.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > > index c94f1e5406..bd8aeac59e 100644 > > --- a/xen/arch/x86/mm.c > > +++ b/xen/arch/x86/mm.c > > @@ -1024,12 +1024,15 @@ get_page_from_l1e( > > (real_pg_owner != dom_cow) ) ) > > { > > /* > > - * Let privileged domains transfer the right to map their target > > - * domain's pages. This is used to allow stub-domain pvfb export to > > - * dom0, until pvfb supports granted mappings. At that time this > > - * minor hack can go away. > > + * If the real page owner is not the domain specified in the > > + * hypercall then establish that the specified domain has > > + * mapping privilege over the page owner. > > + * This is used to allow stub-domain pvfb export to dom0. It is > > + * also used to allow a privileged PV domain to map mfns using > > + * DOMID_SELF, which is needed for mapping guest resources such > > + * grant table frames. > > */ > > - if ( (real_pg_owner == NULL) || (pg_owner == l1e_owner) || > > + if ( (real_pg_owner == NULL) || > > I still can't quite figure out if it is safe to remove the check. > > Looking at the rest of the series, you already have the foreign domid to > hand when you call the get_resource hypercall. What is wrong with using > that directly? Why do you need DOMID_SELF in the first place? Because the page is not in the foreign domain's p2m... that's kind of the entire point of the resource mapping API! Paul > > > xsm_priv_mapping(XSM_TARGET, pg_owner, real_pg_owner) ) > > { > > gdprintk(XENLOG_WARNING, > > -- > > 2.11.0 > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > https://lists.xen.org/xen-devel
On Thu, Sep 07, 2017 at 12:05:08PM +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Wei Liu [mailto:wei.liu2@citrix.com] > > Sent: 07 September 2017 12:02 > > To: Paul Durrant <Paul.Durrant@citrix.com> > > Cc: xen-devel@lists.xenproject.org; Andrew Cooper > > <Andrew.Cooper3@citrix.com>; Jan Beulich <jbeulich@suse.com>; Wei Liu > > <wei.liu2@citrix.com> > > Subject: Re: [Xen-devel] [PATCH v4 01/12] x86/mm: allow a privileged PV > > domain to map guest mfns > > > > On Tue, Sep 05, 2017 at 12:37:05PM +0100, Paul Durrant wrote: > > > In the case where a PV domain is mapping guest resources then it needs > > make > > > the HYPERVISOR_mmu_update call using DOMID_SELF, rather than the > > guest > > > domid, so that the passed in gmfn values are correctly treated as mfns > > > rather than gfns present in the guest p2m. > > > > > > This patch removes a check which currently disallows mapping of a page > > when > > > the owner of the page tables matches the domain passed to > > > HYPERVISOR_mmu_update, but that domain is not the real owner of the > > page. > > > The check was introduced by patch d3c6a215ca9 ("x86: Clean up > > > get_page_from_l1e() to correctly distinguish between owner-of-pte and > > > owner-of-data-page in all cases") but it's not clear why it was needed. > > > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > > --- > > > Cc: Jan Beulich <jbeulich@suse.com> > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > > --- > > > xen/arch/x86/mm.c | 13 ++++++++----- > > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > > > index c94f1e5406..bd8aeac59e 100644 > > > --- a/xen/arch/x86/mm.c > > > +++ b/xen/arch/x86/mm.c > > > @@ -1024,12 +1024,15 @@ get_page_from_l1e( > > > (real_pg_owner != dom_cow) ) ) > > > { > > > /* > > > - * Let privileged domains transfer the right to map their target > > > - * domain's pages. This is used to allow stub-domain pvfb export to > > > - * dom0, until pvfb supports granted mappings. At that time this > > > - * minor hack can go away. > > > + * If the real page owner is not the domain specified in the > > > + * hypercall then establish that the specified domain has > > > + * mapping privilege over the page owner. > > > + * This is used to allow stub-domain pvfb export to dom0. It is > > > + * also used to allow a privileged PV domain to map mfns using > > > + * DOMID_SELF, which is needed for mapping guest resources such > > > + * grant table frames. > > > */ > > > - if ( (real_pg_owner == NULL) || (pg_owner == l1e_owner) || > > > + if ( (real_pg_owner == NULL) || > > > > I still can't quite figure out if it is safe to remove the check. > > > > Looking at the rest of the series, you already have the foreign domid to > > hand when you call the get_resource hypercall. What is wrong with using > > that directly? Why do you need DOMID_SELF in the first place? > > Because the page is not in the foreign domain's p2m... that's kind of the entire point of the resource mapping API! Oh, yes, I'm utterly confused. Sorry. :-/
On 07/09/17 12:02, Wei Liu wrote: > On Tue, Sep 05, 2017 at 12:37:05PM +0100, Paul Durrant wrote: >> In the case where a PV domain is mapping guest resources then it needs make >> the HYPERVISOR_mmu_update call using DOMID_SELF, rather than the guest >> domid, so that the passed in gmfn values are correctly treated as mfns >> rather than gfns present in the guest p2m. >> >> This patch removes a check which currently disallows mapping of a page when >> the owner of the page tables matches the domain passed to >> HYPERVISOR_mmu_update, but that domain is not the real owner of the page. >> The check was introduced by patch d3c6a215ca9 ("x86: Clean up >> get_page_from_l1e() to correctly distinguish between owner-of-pte and >> owner-of-data-page in all cases") but it's not clear why it was needed. >> >> Signed-off-by: Paul Durrant <paul.durrant@citrix.com> >> --- >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> xen/arch/x86/mm.c | 13 ++++++++----- >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c >> index c94f1e5406..bd8aeac59e 100644 >> --- a/xen/arch/x86/mm.c >> +++ b/xen/arch/x86/mm.c >> @@ -1024,12 +1024,15 @@ get_page_from_l1e( >> (real_pg_owner != dom_cow) ) ) >> { >> /* >> - * Let privileged domains transfer the right to map their target >> - * domain's pages. This is used to allow stub-domain pvfb export to >> - * dom0, until pvfb supports granted mappings. At that time this >> - * minor hack can go away. >> + * If the real page owner is not the domain specified in the >> + * hypercall then establish that the specified domain has >> + * mapping privilege over the page owner. >> + * This is used to allow stub-domain pvfb export to dom0. It is >> + * also used to allow a privileged PV domain to map mfns using >> + * DOMID_SELF, which is needed for mapping guest resources such >> + * grant table frames. >> */ >> - if ( (real_pg_owner == NULL) || (pg_owner == l1e_owner) || >> + if ( (real_pg_owner == NULL) || > I still can't quite figure out if it is safe to remove the check. > > Looking at the rest of the series, you already have the foreign domid to > hand when you call the get_resource hypercall. What is wrong with using > that directly? Why do you need DOMID_SELF in the first place? The problem is that when passing a foreign domid, the GFN offered by dom0 gets interpreted using the foreign domU's paging mode. In the mapping case (for PV guests), Xen has given us a MFN which logically belongs to domU, which we need to map as an MFN, rather than having Xen think "Ah - for domU, I need to do a GFN translation for you". Passing DOMID_SELF causes Xen to treat the mapping with our own paging mode, not the paging mode of the page owner. ~Andrew
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index c94f1e5406..bd8aeac59e 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -1024,12 +1024,15 @@ get_page_from_l1e( (real_pg_owner != dom_cow) ) ) { /* - * Let privileged domains transfer the right to map their target - * domain's pages. This is used to allow stub-domain pvfb export to - * dom0, until pvfb supports granted mappings. At that time this - * minor hack can go away. + * If the real page owner is not the domain specified in the + * hypercall then establish that the specified domain has + * mapping privilege over the page owner. + * This is used to allow stub-domain pvfb export to dom0. It is + * also used to allow a privileged PV domain to map mfns using + * DOMID_SELF, which is needed for mapping guest resources such + * grant table frames. */ - if ( (real_pg_owner == NULL) || (pg_owner == l1e_owner) || + if ( (real_pg_owner == NULL) || xsm_priv_mapping(XSM_TARGET, pg_owner, real_pg_owner) ) { gdprintk(XENLOG_WARNING,
In the case where a PV domain is mapping guest resources then it needs make the HYPERVISOR_mmu_update call using DOMID_SELF, rather than the guest domid, so that the passed in gmfn values are correctly treated as mfns rather than gfns present in the guest p2m. This patch removes a check which currently disallows mapping of a page when the owner of the page tables matches the domain passed to HYPERVISOR_mmu_update, but that domain is not the real owner of the page. The check was introduced by patch d3c6a215ca9 ("x86: Clean up get_page_from_l1e() to correctly distinguish between owner-of-pte and owner-of-data-page in all cases") but it's not clear why it was needed. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/mm.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)