From: Julien Grall <julien.grall@arm.com> To: Jan Beulich <JBeulich@suse.com> Cc: Juergen Gross <jgross@suse.com>, Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wei.liu2@citrix.com>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, George Dunlap <George.Dunlap@eu.citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>, xen-devel <xen-devel@lists.xenproject.org> Subject: Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests Date: Mon, 8 Apr 2019 12:47:12 +0100 [thread overview] Message-ID: <eca8d08b-41da-5e88-3303-a0b14adfe0be@arm.com> (raw) In-Reply-To: <5CA3898C0200007800223F9D@prv1-mh.provo.novell.com> Hi Jan, On 4/2/19 5:10 PM, Jan Beulich wrote: >>>> On 02.04.19 at 12:26, <julien.grall@arm.com> wrote: >> On 05/03/2019 13:28, Jan Beulich wrote: >>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously >>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having >>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video >>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused >>> XENMEM_remove_from_physmap"]) make clear that this operation is intended >>> for use on HVM (i.e. translated) guests only. Restrict it at least as >>> much, because for PV guests documentation (in the public header) does >>> not even match the implementation: It talks about GPFN as input, but >>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands >>> back the value passed in). >>> >>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up >>> directly into top level hypercall handling, and clarify things in the >>> public header accordingly. >>> >>> Take the liberty and also replace a pointless use of "current" with a >>> more efficient use of an existing local variable (or function parameter >>> to be precise). >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> TBD: It could be further restricted, disallowing its use by a HVM guest >>> on itself. >> >> By HVM guest, do you refer to any auto-translated guest? > > Yes - sorry for using an x86 term. > >> The interface XENME_remove_from_physmap is used by Arm to remove foreign >> mappings from its p2m. There are potentially other space with similar case >> (e.g grant-table...). > > Oh, I see - this option goes away then. > >>> TBD: Is using P2M_ALLOC here really appropriate? It means e.g. >>> pointlessly populating a PoD slot just to unpopulate it again right >>> away, with the page then free floating, i.e. no longer available >>> for use to replace another PoD slot, and (afaict) no longer >>> accessible by the guest in any way. >>> TBD: Is using guest_physmap_remove_page() here really appropriate? It >>> means that e.g. MMIO pages wouldn't be removed. Going through >>> guest_remove_page() (while skipping the de-allocation step) would >>> seem more appropriate to me, which would address the P2M_ALLOC >>> aspect above as well. >> >> How is that an issue? Does XENMEM_add_to_physmap allows you to map MMIO >> pages? > > Well, there's XENMAPSPACE_dev_mmio which xatp handles. But > perhaps the MMIO example is more confusing than helpful. The > question really just is whether guest_remove_page() shouldn't > be used here instead of guest_physmap_remove_page() de-allocation step aside, I am not really convinced you can reuse guest_remove_page() here. On x86, the function will not work on certain p2m types. Is it what we really want? > > But of course - first of all I'd like to get acks (or feedback what to > change) on the actual patch here. The further points would all, if > anything, result in independent patches. Make sense. I will have a look at the patch. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
WARNING: multiple messages have this Message-ID (diff)
From: Julien Grall <julien.grall@arm.com> To: Jan Beulich <JBeulich@suse.com> Cc: Juergen Gross <jgross@suse.com>, Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wei.liu2@citrix.com>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, George Dunlap <George.Dunlap@eu.citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>, xen-devel <xen-devel@lists.xenproject.org> Subject: Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests Date: Mon, 8 Apr 2019 12:47:12 +0100 [thread overview] Message-ID: <eca8d08b-41da-5e88-3303-a0b14adfe0be@arm.com> (raw) Message-ID: <20190408114712.v4fN_1KY_IjaS_KRbnFhXcT4QGbLeT1yrKq-5fr4Cuo@z> (raw) In-Reply-To: <5CA3898C0200007800223F9D@prv1-mh.provo.novell.com> Hi Jan, On 4/2/19 5:10 PM, Jan Beulich wrote: >>>> On 02.04.19 at 12:26, <julien.grall@arm.com> wrote: >> On 05/03/2019 13:28, Jan Beulich wrote: >>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously >>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having >>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video >>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused >>> XENMEM_remove_from_physmap"]) make clear that this operation is intended >>> for use on HVM (i.e. translated) guests only. Restrict it at least as >>> much, because for PV guests documentation (in the public header) does >>> not even match the implementation: It talks about GPFN as input, but >>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands >>> back the value passed in). >>> >>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up >>> directly into top level hypercall handling, and clarify things in the >>> public header accordingly. >>> >>> Take the liberty and also replace a pointless use of "current" with a >>> more efficient use of an existing local variable (or function parameter >>> to be precise). >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> TBD: It could be further restricted, disallowing its use by a HVM guest >>> on itself. >> >> By HVM guest, do you refer to any auto-translated guest? > > Yes - sorry for using an x86 term. > >> The interface XENME_remove_from_physmap is used by Arm to remove foreign >> mappings from its p2m. There are potentially other space with similar case >> (e.g grant-table...). > > Oh, I see - this option goes away then. > >>> TBD: Is using P2M_ALLOC here really appropriate? It means e.g. >>> pointlessly populating a PoD slot just to unpopulate it again right >>> away, with the page then free floating, i.e. no longer available >>> for use to replace another PoD slot, and (afaict) no longer >>> accessible by the guest in any way. >>> TBD: Is using guest_physmap_remove_page() here really appropriate? It >>> means that e.g. MMIO pages wouldn't be removed. Going through >>> guest_remove_page() (while skipping the de-allocation step) would >>> seem more appropriate to me, which would address the P2M_ALLOC >>> aspect above as well. >> >> How is that an issue? Does XENMEM_add_to_physmap allows you to map MMIO >> pages? > > Well, there's XENMAPSPACE_dev_mmio which xatp handles. But > perhaps the MMIO example is more confusing than helpful. The > question really just is whether guest_remove_page() shouldn't > be used here instead of guest_physmap_remove_page() de-allocation step aside, I am not really convinced you can reuse guest_remove_page() here. On x86, the function will not work on certain p2m types. Is it what we really want? > > But of course - first of all I'd like to get acks (or feedback what to > change) on the actual patch here. The further points would all, if > anything, result in independent patches. Make sense. I will have a look at the patch. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-04-08 11:47 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-03-05 13:21 [PATCH 0/3] today's XSAs assorted 4.12 candidate follow-up Jan Beulich 2019-03-05 13:25 ` [PATCH 1/3] x86/mm: fix #GP(0) in switch_cr3_cr4() Jan Beulich 2019-03-05 13:58 ` Andrew Cooper 2019-03-05 13:26 ` [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages Jan Beulich 2019-05-13 13:44 ` George Dunlap 2019-05-13 13:44 ` [Xen-devel] " George Dunlap 2019-05-13 13:59 ` Jan Beulich 2019-05-13 13:59 ` [Xen-devel] " Jan Beulich 2019-05-14 11:17 ` Jan Beulich 2019-05-14 11:17 ` [Xen-devel] " Jan Beulich [not found] ` <5C7E78B0020000780021BB1E@suse.com> 2019-03-05 13:28 ` [PATCH 1/3] x86/mm: fix #GP(0) in switch_cr3_cr4() Juergen Gross 2019-03-05 13:28 ` [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests Jan Beulich 2019-04-02 10:26 ` Julien Grall 2019-04-02 16:10 ` Jan Beulich 2019-04-08 11:47 ` Julien Grall [this message] 2019-04-08 11:47 ` [Xen-devel] " Julien Grall 2019-04-08 14:29 ` Jan Beulich 2019-04-08 14:29 ` [Xen-devel] " Jan Beulich 2019-04-09 9:50 ` Julien Grall 2019-04-09 9:50 ` [Xen-devel] " Julien Grall 2019-04-09 12:21 ` Jan Beulich 2019-04-09 12:21 ` [Xen-devel] " Jan Beulich 2019-04-14 16:33 ` Julien Grall 2019-04-14 16:33 ` [Xen-devel] " Julien Grall 2019-04-25 10:36 ` Jan Beulich 2019-04-25 10:36 ` [Xen-devel] " Jan Beulich 2019-04-08 11:58 ` Julien Grall 2019-04-08 11:58 ` [Xen-devel] " Julien Grall 2019-04-08 14:02 ` Jan Beulich 2019-04-08 14:02 ` [Xen-devel] " Jan Beulich 2019-04-08 16:10 ` Julien Grall 2019-04-08 16:10 ` [Xen-devel] " Julien Grall 2019-05-13 8:06 ` Ping: " Jan Beulich 2019-05-13 8:06 ` [Xen-devel] " Jan Beulich 2019-05-13 14:35 ` George Dunlap 2019-05-13 14:35 ` [Xen-devel] " George Dunlap 2019-05-13 15:13 ` Jan Beulich 2019-05-13 15:13 ` [Xen-devel] " Jan Beulich [not found] ` <5C7E78F6020000780021BB21@suse.com> 2019-03-05 13:50 ` [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages Juergen Gross 2019-03-05 15:21 ` Jan Beulich [not found] ` <5C7E798E020000780021BB43@suse.com> 2019-03-05 13:53 ` [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests Juergen Gross 2019-03-05 15:22 ` Jan Beulich
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=eca8d08b-41da-5e88-3303-a0b14adfe0be@arm.com \ --to=julien.grall@arm.com \ --cc=George.Dunlap@eu.citrix.com \ --cc=Ian.Jackson@eu.citrix.com \ --cc=JBeulich@suse.com \ --cc=andrew.cooper3@citrix.com \ --cc=jgross@suse.com \ --cc=konrad.wilk@oracle.com \ --cc=sstabellini@kernel.org \ --cc=tim@xen.org \ --cc=wei.liu2@citrix.com \ --cc=xen-devel@lists.xenproject.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.