| Message ID | 20170918153126.3058-2-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ping? > -----Original Message----- > From: Paul Durrant [mailto:paul.durrant@citrix.com] > Sent: 18 September 2017 16:31 > To: xen-devel@lists.xenproject.org > Cc: Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich > <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com> > Subject: [PATCH v7 01/12] x86/mm: allow a privileged PV domain to map > guest mfns > > 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> I believe this is now the only patch in the series without a R-b or A-b. Paul > --- > 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 2e5b15e7a2..cb0189efae 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, > -- > 2.11.0
>>> On 19.09.17 at 14:51, <Paul.Durrant@citrix.com> wrote: > Ping? Your patch series hasn't been forgotten, but I can't currently predict when I would be able to look at it (together with the well over 100 other patches sitting in the queue). I can only promise: As soon as other, often higher priority, work permits me to get there (unless Andrew manages to get there earlier). Jan
>>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> 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. Since things are presently working fine, I think the description is not really accurate. You only require the new behavior if you don't know the GFN of the page you want to map, and that it has to be DOMID_SELF that should be passed also doesn't appear to derive from anything else. To properly judge about the need for this patch it would help if it was briefly explained why being able to map by GFN is no longer sufficient, and to re-word the DOMID_SELF part. The other aspect I don't understand is why this is needed for PV Dom0, but not for PVH. The answer here can't be "because PVH Dom0 isn't supported yet", because it eventually will be, and then there will still be the problem of PVH supposedly having no notion of MFNs (be their own or foreign guest ones). The answer also can't be "since it would use XENMAPSPACE_gmfn_foreign", as that's acting in terms of GFN too. > 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. I think the goal here simply was to not permit anything that doesn't really need permitting. Furthermore the check being "introduced" there was, afaict, replacing the earlier d != curr->domain. > --- 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. How do grant table frames come into the picture here? So far I had assumed only ioreq server pages are in need of this. > */ > - 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, I'm concerned of the effect of the change on the code paths which you're not really interested in: alloc_l1_table(), ptwr_emulated_update(), and shadow_get_page_from_l1e() all explicitly pass both domains identical, and are now suddenly able to do things they weren't supposed to do. A similar concern applies to __do_update_va_mapping() calling mod_l1_table(). I therefore wonder whether the solution to your problem wouldn't rather be MMU_FOREIGN_PT_UPDATE (name subject to improvement suggestions). This at the same time would address my concern regarding the misleading DOMID_SELF passing when really a foreign domain's page is meant. Jan
On 25/09/17 14:02, Jan Beulich wrote: >>>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> 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. > Since things are presently working fine, I think the description is not > really accurate. You only require the new behavior if you don't know > the GFN of the page you want to map, and that it has to be > DOMID_SELF that should be passed also doesn't appear to derive > from anything else. To properly judge about the need for this patch > it would help if it was briefly explained why being able to map by GFN > is no longer sufficient, and to re-word the DOMID_SELF part. I think there is still confusion as to the purpose here. For security and scalability reasons, we explicitly want to be able to create frames which are not part of a guests p2m. We still need to map these frames however. The frames are referred to in an abstract way by a space id/offset. To create mappings of these frames, PV guests pass an array which Xen fills in with MFNs, while HVM guests pass an array of GFNs which have their mappings updated. The problem is trying to map an MFN belonging to a target domain, because Xen interprets the frame under the targets paging mode. Passing DOMID_SELF here is Pauls way of getting Xen to interpret the frame under current's paging mode. Alternative suggestions welcome. ~Andrew
>>> On 25.09.17 at 15:29, <andrew.cooper3@citrix.com> wrote: > On 25/09/17 14:02, Jan Beulich wrote: >>>>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> 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. >> Since things are presently working fine, I think the description is not >> really accurate. You only require the new behavior if you don't know >> the GFN of the page you want to map, and that it has to be >> DOMID_SELF that should be passed also doesn't appear to derive >> from anything else. To properly judge about the need for this patch >> it would help if it was briefly explained why being able to map by GFN >> is no longer sufficient, and to re-word the DOMID_SELF part. > > I think there is still confusion as to the purpose here. > > For security and scalability reasons, we explicitly want to be able to > create frames which are not part of a guests p2m. We still need to map > these frames however. > > The frames are referred to in an abstract way by a space id/offset. To > create mappings of these frames, PV guests pass an array which Xen fills > in with MFNs, while HVM guests pass an array of GFNs which have their > mappings updated. In the course of reviewing patch 2 I've gained some more understanding of the intentions. Still it would have been helpful to have an abstract understanding already before even looking at patch 1, i.e. presented in the overview mail. > The problem is trying to map an MFN belonging to a target domain, > because Xen interprets the frame under the targets paging mode. Passing > DOMID_SELF here is Pauls way of getting Xen to interpret the frame under > current's paging mode. > > Alternative suggestions welcome. I've given one in the earlier reply. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 25 September 2017 14:03 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH v7 01/12] x86/mm: allow a privileged PV domain to map > guest mfns > > >>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> 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. > > Since things are presently working fine, I think the description is not > really accurate. You only require the new behavior if you don't know > the GFN of the page you want to map, and that it has to be > DOMID_SELF that should be passed also doesn't appear to derive > from anything else. To properly judge about the need for this patch > it would help if it was briefly explained why being able to map by GFN > is no longer sufficient, and to re-word the DOMID_SELF part. Ok, I can expand the explanation. > > The other aspect I don't understand is why this is needed for PV > Dom0, but not for PVH. The answer here can't be "because PVH > Dom0 isn't supported yet", because it eventually will be, and then > there will still be the problem of PVH supposedly having no notion > of MFNs (be their own or foreign guest ones). The answer also > can't be "since it would use XENMAPSPACE_gmfn_foreign", as > that's acting in terms of GFN too. The hypercall is PV-only. For a PVH/HVM tools domain things are handled by doing an add-to-physmap to gfns specified by the tools domain. I have tested both PV and HVM clients of my new memory op. > > > 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. > > I think the goal here simply was to not permit anything that doesn't > really need permitting. Furthermore the check being "introduced" > there was, afaict, replacing the earlier d != curr->domain. I'm not entirely sure why that check was there though. > > > --- 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. > > How do grant table frames come into the picture here? So far > I had assumed only ioreq server pages are in need of this. > Grant frames required less re-work in other places so I started with them. Nothing to prevent the series from being re-ordered now that it's complete. > > */ > > - 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, > > I'm concerned of the effect of the change on the code paths > which you're not really interested in: alloc_l1_table(), > ptwr_emulated_update(), and shadow_get_page_from_l1e() all > explicitly pass both domains identical, and are now suddenly able > to do things they weren't supposed to do. A similar concern > applies to __do_update_va_mapping() calling mod_l1_table(). > > I therefore wonder whether the solution to your problem > wouldn't rather be MMU_FOREIGN_PT_UPDATE (name subject > to improvement suggestions). This at the same time would > address my concern regarding the misleading DOMID_SELF > passing when really a foreign domain's page is meant. Ok. I'm not familiar with MMU_FOREIGN_PT_UPDATE so I'd need to investigate. I just need a mechanism for a privileged PV guest to map pages belonging to another guest that don't appear in that guests P2M. As I said above, it's much simpler if the tools domain is PVH or HVM. Paul > > Jan
>>> On 25.09.17 at 16:42, <Paul.Durrant@citrix.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 25 September 2017 14:03 >> >>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> wrote: >> The other aspect I don't understand is why this is needed for PV >> Dom0, but not for PVH. The answer here can't be "because PVH >> Dom0 isn't supported yet", because it eventually will be, and then >> there will still be the problem of PVH supposedly having no notion >> of MFNs (be their own or foreign guest ones). The answer also >> can't be "since it would use XENMAPSPACE_gmfn_foreign", as >> that's acting in terms of GFN too. > > The hypercall is PV-only. For a PVH/HVM tools domain things are handled by > doing an add-to-physmap to gfns specified by the tools domain. I have tested > both PV and HVM clients of my new memory op. And how is this add-to-physmap any better superpage shattering wise than the old mechansim? >> > - 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, >> >> I'm concerned of the effect of the change on the code paths >> which you're not really interested in: alloc_l1_table(), >> ptwr_emulated_update(), and shadow_get_page_from_l1e() all >> explicitly pass both domains identical, and are now suddenly able >> to do things they weren't supposed to do. A similar concern >> applies to __do_update_va_mapping() calling mod_l1_table(). >> >> I therefore wonder whether the solution to your problem >> wouldn't rather be MMU_FOREIGN_PT_UPDATE (name subject >> to improvement suggestions). This at the same time would >> address my concern regarding the misleading DOMID_SELF >> passing when really a foreign domain's page is meant. > > Ok. I'm not familiar with MMU_FOREIGN_PT_UPDATE so I'd need to investigate. > I just need a mechanism for a privileged PV guest to map pages belonging to > another guest that don't appear in that guests P2M. As I said above, it's > much simpler if the tools domain is PVH or HVM. Hmm, looks like I wasn't able to express things such that it becomes clear the MMU_FOREIGN_PT_UPDATE is the proposed (sub-optimal) name for a new sub-op. Jan
> -----Original Message----- > From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Jan > Beulich > Sent: 25 September 2017 15:50 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > devel@lists.xenproject.org > Subject: Re: [Xen-devel] [PATCH v7 01/12] x86/mm: allow a privileged PV > domain to map guest mfns > > >>> On 25.09.17 at 16:42, <Paul.Durrant@citrix.com> wrote: > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: 25 September 2017 14:03 > >> >>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> wrote: > >> The other aspect I don't understand is why this is needed for PV > >> Dom0, but not for PVH. The answer here can't be "because PVH > >> Dom0 isn't supported yet", because it eventually will be, and then > >> there will still be the problem of PVH supposedly having no notion > >> of MFNs (be their own or foreign guest ones). The answer also > >> can't be "since it would use XENMAPSPACE_gmfn_foreign", as > >> that's acting in terms of GFN too. > > > > The hypercall is PV-only. For a PVH/HVM tools domain things are handled > by > > doing an add-to-physmap to gfns specified by the tools domain. I have > tested > > both PV and HVM clients of my new memory op. > > And how is this add-to-physmap any better superpage shattering > wise than the old mechansim? Because the calling domain can make an intelligent choice about what gfns to use? > > >> > - 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, > >> > >> I'm concerned of the effect of the change on the code paths > >> which you're not really interested in: alloc_l1_table(), > >> ptwr_emulated_update(), and shadow_get_page_from_l1e() all > >> explicitly pass both domains identical, and are now suddenly able > >> to do things they weren't supposed to do. A similar concern > >> applies to __do_update_va_mapping() calling mod_l1_table(). > >> > >> I therefore wonder whether the solution to your problem > >> wouldn't rather be MMU_FOREIGN_PT_UPDATE (name subject > >> to improvement suggestions). This at the same time would > >> address my concern regarding the misleading DOMID_SELF > >> passing when really a foreign domain's page is meant. > > > > Ok. I'm not familiar with MMU_FOREIGN_PT_UPDATE so I'd need to > investigate. > > I just need a mechanism for a privileged PV guest to map pages belonging > to > > another guest that don't appear in that guests P2M. As I said above, it's > > much simpler if the tools domain is PVH or HVM. > > Hmm, looks like I wasn't able to express things such that it > becomes clear the MMU_FOREIGN_PT_UPDATE is the proposed > (sub-optimal) name for a new sub-op. > Ok. I see what you mean now. Paul > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 25.09.17 at 16:56, <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Jan >> Beulich >> Sent: 25 September 2017 15:50 >> To: Paul Durrant <Paul.Durrant@citrix.com> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- >> devel@lists.xenproject.org >> Subject: Re: [Xen-devel] [PATCH v7 01/12] x86/mm: allow a privileged PV >> domain to map guest mfns >> >> >>> On 25.09.17 at 16:42, <Paul.Durrant@citrix.com> wrote: >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> Sent: 25 September 2017 14:03 >> >> >>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> wrote: >> >> The other aspect I don't understand is why this is needed for PV >> >> Dom0, but not for PVH. The answer here can't be "because PVH >> >> Dom0 isn't supported yet", because it eventually will be, and then >> >> there will still be the problem of PVH supposedly having no notion >> >> of MFNs (be their own or foreign guest ones). The answer also >> >> can't be "since it would use XENMAPSPACE_gmfn_foreign", as >> >> that's acting in terms of GFN too. >> > >> > The hypercall is PV-only. For a PVH/HVM tools domain things are handled >> by >> > doing an add-to-physmap to gfns specified by the tools domain. I have >> tested >> > both PV and HVM clients of my new memory op. >> >> And how is this add-to-physmap any better superpage shattering >> wise than the old mechansim? > > Because the calling domain can make an intelligent choice about what gfns to > use? And why was an intelligent choice not possible with the old mechanism? The calling domain is the tool stack one in both cases, isn't it? Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 25 September 2017 16:31 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > devel@lists.xenproject.org > Subject: RE: [Xen-devel] [PATCH v7 01/12] x86/mm: allow a privileged PV > domain to map guest mfns > > >>> On 25.09.17 at 16:56, <Paul.Durrant@citrix.com> wrote: > >> -----Original Message----- > >> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of > Jan > >> Beulich > >> Sent: 25 September 2017 15:50 > >> To: Paul Durrant <Paul.Durrant@citrix.com> > >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > >> devel@lists.xenproject.org > >> Subject: Re: [Xen-devel] [PATCH v7 01/12] x86/mm: allow a privileged PV > >> domain to map guest mfns > >> > >> >>> On 25.09.17 at 16:42, <Paul.Durrant@citrix.com> wrote: > >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> Sent: 25 September 2017 14:03 > >> >> >>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> wrote: > >> >> The other aspect I don't understand is why this is needed for PV > >> >> Dom0, but not for PVH. The answer here can't be "because PVH > >> >> Dom0 isn't supported yet", because it eventually will be, and then > >> >> there will still be the problem of PVH supposedly having no notion > >> >> of MFNs (be their own or foreign guest ones). The answer also > >> >> can't be "since it would use XENMAPSPACE_gmfn_foreign", as > >> >> that's acting in terms of GFN too. > >> > > >> > The hypercall is PV-only. For a PVH/HVM tools domain things are > handled > >> by > >> > doing an add-to-physmap to gfns specified by the tools domain. I have > >> tested > >> > both PV and HVM clients of my new memory op. > >> > >> And how is this add-to-physmap any better superpage shattering > >> wise than the old mechansim? > > > > Because the calling domain can make an intelligent choice about what gfns > to > > use? > > And why was an intelligent choice not possible with the old > mechanism? The calling domain is the tool stack one in both > cases, isn't it? True, I suppose. Paul > > Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 25 September 2017 14:03 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH v7 01/12] x86/mm: allow a privileged PV domain to map > guest mfns > > >>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> 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. > > Since things are presently working fine, I think the description is not > really accurate. You only require the new behavior if you don't know > the GFN of the page you want to map, and that it has to be > DOMID_SELF that should be passed also doesn't appear to derive > from anything else. To properly judge about the need for this patch > it would help if it was briefly explained why being able to map by GFN > is no longer sufficient, and to re-word the DOMID_SELF part. > > The other aspect I don't understand is why this is needed for PV > Dom0, but not for PVH. The answer here can't be "because PVH > Dom0 isn't supported yet", because it eventually will be, and then > there will still be the problem of PVH supposedly having no notion > of MFNs (be their own or foreign guest ones). The answer also > can't be "since it would use XENMAPSPACE_gmfn_foreign", as > that's acting in terms of GFN too. > > > 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. > > I think the goal here simply was to not permit anything that doesn't > really need permitting. Furthermore the check being "introduced" > there was, afaict, replacing the earlier d != curr->domain. > > > --- 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. > > How do grant table frames come into the picture here? So far > I had assumed only ioreq server pages are in need of this. > > > */ > > - 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, > > I'm concerned of the effect of the change on the code paths > which you're not really interested in: alloc_l1_table(), > ptwr_emulated_update(), and shadow_get_page_from_l1e() all > explicitly pass both domains identical, and are now suddenly able > to do things they weren't supposed to do. A similar concern > applies to __do_update_va_mapping() calling mod_l1_table(). > > I therefore wonder whether the solution to your problem > wouldn't rather be MMU_FOREIGN_PT_UPDATE (name subject > to improvement suggestions). This at the same time would > address my concern regarding the misleading DOMID_SELF > passing when really a foreign domain's page is meant. Looking at this I wonder whether a cleaner solution would be to introduce a new domid: DOMID_ANY. This meaning of this would be along the same sort of lines as DOMID_XEN or DOMID_IO and would be used in mmu_update to mean 'any page over which the caller has privilege'. Does that sound reasonable? Paul > > Jan
>>> On 27.09.17 at 13:18, <Paul.Durrant@citrix.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 25 September 2017 14:03 >> >>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> wrote: >> > - 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, >> >> I'm concerned of the effect of the change on the code paths >> which you're not really interested in: alloc_l1_table(), >> ptwr_emulated_update(), and shadow_get_page_from_l1e() all >> explicitly pass both domains identical, and are now suddenly able >> to do things they weren't supposed to do. A similar concern >> applies to __do_update_va_mapping() calling mod_l1_table(). >> >> I therefore wonder whether the solution to your problem >> wouldn't rather be MMU_FOREIGN_PT_UPDATE (name subject >> to improvement suggestions). This at the same time would >> address my concern regarding the misleading DOMID_SELF >> passing when really a foreign domain's page is meant. > > Looking at this I wonder whether a cleaner solution would be to introduce a > new domid: DOMID_ANY. This meaning of this would be along the same sort of > lines as DOMID_XEN or DOMID_IO and would be used in mmu_update to mean 'any > page over which the caller has privilege'. Does that sound reasonable? Not really, no. Even if the caller has privilege over multiple domains, it should still specify which one it means. Otherwise we may end up with a page transferring ownership behind its back, and it doing something to one domain which was meant to be done to another. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 27 September 2017 13:47 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > devel@lists.xenproject.org > Subject: RE: [PATCH v7 01/12] x86/mm: allow a privileged PV domain to map > guest mfns > > >>> On 27.09.17 at 13:18, <Paul.Durrant@citrix.com> wrote: > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: 25 September 2017 14:03 > >> >>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> wrote: > >> > - 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, > >> > >> I'm concerned of the effect of the change on the code paths > >> which you're not really interested in: alloc_l1_table(), > >> ptwr_emulated_update(), and shadow_get_page_from_l1e() all > >> explicitly pass both domains identical, and are now suddenly able > >> to do things they weren't supposed to do. A similar concern > >> applies to __do_update_va_mapping() calling mod_l1_table(). > >> > >> I therefore wonder whether the solution to your problem > >> wouldn't rather be MMU_FOREIGN_PT_UPDATE (name subject > >> to improvement suggestions). This at the same time would > >> address my concern regarding the misleading DOMID_SELF > >> passing when really a foreign domain's page is meant. > > > > Looking at this I wonder whether a cleaner solution would be to introduce a > > new domid: DOMID_ANY. This meaning of this would be along the same > sort of > > lines as DOMID_XEN or DOMID_IO and would be used in mmu_update to > mean 'any > > page over which the caller has privilege'. Does that sound reasonable? > > Not really, no. Even if the caller has privilege over multiple domains, > it should still specify which one it means. Otherwise we may end up > with a page transferring ownership behind its back, and it doing > something to one domain which was meant to be done to another. > Ok, I'll claim the final cmd value then. Paul > Jan
>>> On 27.09.17 at 14:49, <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 27 September 2017 13:47 >> To: Paul Durrant <Paul.Durrant@citrix.com> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- >> devel@lists.xenproject.org >> Subject: RE: [PATCH v7 01/12] x86/mm: allow a privileged PV domain to map >> guest mfns >> >> >>> On 27.09.17 at 13:18, <Paul.Durrant@citrix.com> wrote: >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> Sent: 25 September 2017 14:03 >> >> >>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> wrote: >> >> > - 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, >> >> >> >> I'm concerned of the effect of the change on the code paths >> >> which you're not really interested in: alloc_l1_table(), >> >> ptwr_emulated_update(), and shadow_get_page_from_l1e() all >> >> explicitly pass both domains identical, and are now suddenly able >> >> to do things they weren't supposed to do. A similar concern >> >> applies to __do_update_va_mapping() calling mod_l1_table(). >> >> >> >> I therefore wonder whether the solution to your problem >> >> wouldn't rather be MMU_FOREIGN_PT_UPDATE (name subject >> >> to improvement suggestions). This at the same time would >> >> address my concern regarding the misleading DOMID_SELF >> >> passing when really a foreign domain's page is meant. >> > >> > Looking at this I wonder whether a cleaner solution would be to introduce a >> > new domid: DOMID_ANY. This meaning of this would be along the same >> sort of >> > lines as DOMID_XEN or DOMID_IO and would be used in mmu_update to >> mean 'any >> > page over which the caller has privilege'. Does that sound reasonable? >> >> Not really, no. Even if the caller has privilege over multiple domains, >> it should still specify which one it means. Otherwise we may end up >> with a page transferring ownership behind its back, and it doing >> something to one domain which was meant to be done to another. >> > > Ok, I'll claim the final cmd value then. Final? We've got 5 left (for a total of 3 bits) afaict. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 27 September 2017 14:31 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > devel@lists.xenproject.org > Subject: RE: [PATCH v7 01/12] x86/mm: allow a privileged PV domain to map > guest mfns > > >>> On 27.09.17 at 14:49, <Paul.Durrant@citrix.com> wrote: > >> -----Original Message----- > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: 27 September 2017 13:47 > >> To: Paul Durrant <Paul.Durrant@citrix.com> > >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > >> devel@lists.xenproject.org > >> Subject: RE: [PATCH v7 01/12] x86/mm: allow a privileged PV domain to > map > >> guest mfns > >> > >> >>> On 27.09.17 at 13:18, <Paul.Durrant@citrix.com> wrote: > >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> Sent: 25 September 2017 14:03 > >> >> >>> On 18.09.17 at 17:31, <paul.durrant@citrix.com> wrote: > >> >> > - 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, > >> >> > >> >> I'm concerned of the effect of the change on the code paths > >> >> which you're not really interested in: alloc_l1_table(), > >> >> ptwr_emulated_update(), and shadow_get_page_from_l1e() all > >> >> explicitly pass both domains identical, and are now suddenly able > >> >> to do things they weren't supposed to do. A similar concern > >> >> applies to __do_update_va_mapping() calling mod_l1_table(). > >> >> > >> >> I therefore wonder whether the solution to your problem > >> >> wouldn't rather be MMU_FOREIGN_PT_UPDATE (name subject > >> >> to improvement suggestions). This at the same time would > >> >> address my concern regarding the misleading DOMID_SELF > >> >> passing when really a foreign domain's page is meant. > >> > > >> > Looking at this I wonder whether a cleaner solution would be to > introduce a > >> > new domid: DOMID_ANY. This meaning of this would be along the same > >> sort of > >> > lines as DOMID_XEN or DOMID_IO and would be used in mmu_update > to > >> mean 'any > >> > page over which the caller has privilege'. Does that sound reasonable? > >> > >> Not really, no. Even if the caller has privilege over multiple domains, > >> it should still specify which one it means. Otherwise we may end up > >> with a page transferring ownership behind its back, and it doing > >> something to one domain which was meant to be done to another. > >> > > > > Ok, I'll claim the final cmd value then. > > Final? We've got 5 left (for a total of 3 bits) afaict. Really? Maybe I misread... looks like only 2 bits to me. Paul > > Jan
>>> On 27.09.17 at 16:22, <Paul.Durrant@citrix.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 27 September 2017 14:31 >> >>> On 27.09.17 at 14:49, <Paul.Durrant@citrix.com> wrote: >> > Ok, I'll claim the final cmd value then. >> >> Final? We've got 5 left (for a total of 3 bits) afaict. > > Really? Maybe I misread... looks like only 2 bits to me. Maybe you and I looked in different places. I'm deriving this from cmd = req.ptr & (sizeof(l1_pgentry_t)-1); switch ( cmd ) in do_mmu_update(). Only 32-bit non-PAE guests would have been limited to 2 bits. Jan
> -----Original Message----- > From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Jan > Beulich > Sent: 27 September 2017 15:42 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > devel@lists.xenproject.org > Subject: Re: [Xen-devel] [PATCH v7 01/12] x86/mm: allow a privileged PV > domain to map guest mfns > > >>> On 27.09.17 at 16:22, <Paul.Durrant@citrix.com> wrote: > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: 27 September 2017 14:31 > >> >>> On 27.09.17 at 14:49, <Paul.Durrant@citrix.com> wrote: > >> > Ok, I'll claim the final cmd value then. > >> > >> Final? We've got 5 left (for a total of 3 bits) afaict. > > > > Really? Maybe I misread... looks like only 2 bits to me. > > Maybe you and I looked in different places. I'm deriving this from > > cmd = req.ptr & (sizeof(l1_pgentry_t)-1); > > switch ( cmd ) > > in do_mmu_update(). Only 32-bit non-PAE guests would have been > limited to 2 bits. Ah, ok. I was going off what it says in the header, where the comments state that [0:1] are command bits and [:2] are address bits, but for 64-bit or PAE guests then it makes sense that bit 2 is up for grabs. Anyway I can use 3, which still fits in bits [0:1]. Paul > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 2e5b15e7a2..cb0189efae 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(-)