* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
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:58 ` [Xen-devel] " Julien Grall
` (2 subsequent siblings)
3 siblings, 1 reply; 42+ messages in thread
From: Julien Grall @ 2019-04-02 10:26 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan
Hi Jan,
Sorry I was meant to answer to this earlier on.
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?
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...).
> 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?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
2019-04-02 10:26 ` Julien Grall
@ 2019-04-02 16:10 ` Jan Beulich
2019-04-08 11:47 ` [Xen-devel] " Julien Grall
0 siblings, 1 reply; 42+ messages in thread
From: Jan Beulich @ 2019-04-02 16:10 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
>>> 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().
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.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-08 11:47 ` Julien Grall
0 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2019-04-08 11:47 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-08 11:47 ` Julien Grall
0 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2019-04-08 11:47 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-08 14:29 ` Jan Beulich
0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2019-04-08 14:29 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
> 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?
Hmm, I'm confused. Afaics the only two types it refuses a request
for are p2m_invalid and p2m_mmio_dm. These represent cases
where there's no p2m entry anyway, i.e. nothing to remove. Am
I perhaps overlooking something you see?
Or are you referring to the mfn_invalid() check (which isn't x86-
specific)? This ought to be covered by the p2m_is_paging() and
p2m_mmio_direct special cases a few lines up from there. Other
cases with invalid MFNs would indeed represent an error condition
imo.
In the end it's actually quite the opposite that I'm thinking: For
the caller it shouldn't, for example, matter whether the
requested page was paged out. We wouldn't even call
guest_physmap_remove_page() in that case, while
guest_remove_page() would take care of it.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-08 14:29 ` Jan Beulich
0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2019-04-08 14:29 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
> 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?
Hmm, I'm confused. Afaics the only two types it refuses a request
for are p2m_invalid and p2m_mmio_dm. These represent cases
where there's no p2m entry anyway, i.e. nothing to remove. Am
I perhaps overlooking something you see?
Or are you referring to the mfn_invalid() check (which isn't x86-
specific)? This ought to be covered by the p2m_is_paging() and
p2m_mmio_direct special cases a few lines up from there. Other
cases with invalid MFNs would indeed represent an error condition
imo.
In the end it's actually quite the opposite that I'm thinking: For
the caller it shouldn't, for example, matter whether the
requested page was paged out. We wouldn't even call
guest_physmap_remove_page() in that case, while
guest_remove_page() would take care of it.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-09 9:50 ` Julien Grall
0 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2019-04-09 9:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
Hi,
On 08/04/2019 15:29, Jan Beulich wrote:
>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>> 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?
>
> Hmm, I'm confused. Afaics the only two types it refuses a request
> for are p2m_invalid and p2m_mmio_dm. These represent cases
> where there's no p2m entry anyway, i.e. nothing to remove. Am
> I perhaps overlooking something you see?
>
> Or are you referring to the mfn_invalid() check (which isn't x86-
> specific)? This ought to be covered by the p2m_is_paging() and
> p2m_mmio_direct special cases a few lines up from there. Other
> cases with invalid MFNs would indeed represent an error condition
> imo.
I am referring to get_page(...) which would not work for foreign pages.
>
> In the end it's actually quite the opposite that I'm thinking: For
> the caller it shouldn't, for example, matter whether the
> requested page was paged out. We wouldn't even call
> guest_physmap_remove_page() in that case, while
> guest_remove_page() would take care of it.
But those pages should never be removed via physmap, I am correct?
Anyway, guest_physmap_remove_page() would need some rework to do the correct
things for physmap. I will wait an see your patch to comment.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-09 9:50 ` Julien Grall
0 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2019-04-09 9:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
Hi,
On 08/04/2019 15:29, Jan Beulich wrote:
>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>> 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?
>
> Hmm, I'm confused. Afaics the only two types it refuses a request
> for are p2m_invalid and p2m_mmio_dm. These represent cases
> where there's no p2m entry anyway, i.e. nothing to remove. Am
> I perhaps overlooking something you see?
>
> Or are you referring to the mfn_invalid() check (which isn't x86-
> specific)? This ought to be covered by the p2m_is_paging() and
> p2m_mmio_direct special cases a few lines up from there. Other
> cases with invalid MFNs would indeed represent an error condition
> imo.
I am referring to get_page(...) which would not work for foreign pages.
>
> In the end it's actually quite the opposite that I'm thinking: For
> the caller it shouldn't, for example, matter whether the
> requested page was paged out. We wouldn't even call
> guest_physmap_remove_page() in that case, while
> guest_remove_page() would take care of it.
But those pages should never be removed via physmap, I am correct?
Anyway, guest_physmap_remove_page() would need some rework to do the correct
things for physmap. I will wait an see your patch to comment.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-09 12:21 ` Jan Beulich
0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2019-04-09 12:21 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
>>> On 09.04.19 at 11:50, <julien.grall@arm.com> wrote:
> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>>> 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?
>>
>> Hmm, I'm confused. Afaics the only two types it refuses a request
>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>> I perhaps overlooking something you see?
>>
>> Or are you referring to the mfn_invalid() check (which isn't x86-
>> specific)? This ought to be covered by the p2m_is_paging() and
>> p2m_mmio_direct special cases a few lines up from there. Other
>> cases with invalid MFNs would indeed represent an error condition
>> imo.
>
> I am referring to get_page(...) which would not work for foreign pages.
Ah, that's part of the de-allocation portion of the code, which I've
previously indicated would need to be skipped in the case here.
>> In the end it's actually quite the opposite that I'm thinking: For
>> the caller it shouldn't, for example, matter whether the
>> requested page was paged out. We wouldn't even call
>> guest_physmap_remove_page() in that case, while
>> guest_remove_page() would take care of it.
>
> But those pages should never be removed via physmap, I am correct?
The guest is unaware of paging, so as long as it's permitted to
use this hypercall, it should make no difference to it whether a
page is actually in memory at the time it issues this hypercall.
> Anyway, guest_physmap_remove_page() would need some rework to do the correct
> things for physmap.
Of course.
> I will wait an see your patch to comment.
Well, the reason for the TBD item in the patch here was precisely
to figure out whether creating such a patch would make sense,
or whether the current use of guest_physmap_remove_page() is
correct.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-09 12:21 ` Jan Beulich
0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2019-04-09 12:21 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
>>> On 09.04.19 at 11:50, <julien.grall@arm.com> wrote:
> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>>> 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?
>>
>> Hmm, I'm confused. Afaics the only two types it refuses a request
>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>> I perhaps overlooking something you see?
>>
>> Or are you referring to the mfn_invalid() check (which isn't x86-
>> specific)? This ought to be covered by the p2m_is_paging() and
>> p2m_mmio_direct special cases a few lines up from there. Other
>> cases with invalid MFNs would indeed represent an error condition
>> imo.
>
> I am referring to get_page(...) which would not work for foreign pages.
Ah, that's part of the de-allocation portion of the code, which I've
previously indicated would need to be skipped in the case here.
>> In the end it's actually quite the opposite that I'm thinking: For
>> the caller it shouldn't, for example, matter whether the
>> requested page was paged out. We wouldn't even call
>> guest_physmap_remove_page() in that case, while
>> guest_remove_page() would take care of it.
>
> But those pages should never be removed via physmap, I am correct?
The guest is unaware of paging, so as long as it's permitted to
use this hypercall, it should make no difference to it whether a
page is actually in memory at the time it issues this hypercall.
> Anyway, guest_physmap_remove_page() would need some rework to do the correct
> things for physmap.
Of course.
> I will wait an see your patch to comment.
Well, the reason for the TBD item in the patch here was precisely
to figure out whether creating such a patch would make sense,
or whether the current use of guest_physmap_remove_page() is
correct.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-14 16:33 ` Julien Grall
0 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2019-04-14 16:33 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
Hi,
On 4/9/19 1:21 PM, Jan Beulich wrote:
>>>> On 09.04.19 at 11:50, <julien.grall@arm.com> wrote:
>> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>>>> 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?
>>>
>>> Hmm, I'm confused. Afaics the only two types it refuses a request
>>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>>> I perhaps overlooking something you see?
>>>
>>> Or are you referring to the mfn_invalid() check (which isn't x86-
>>> specific)? This ought to be covered by the p2m_is_paging() and
>>> p2m_mmio_direct special cases a few lines up from there. Other
>>> cases with invalid MFNs would indeed represent an error condition
>>> imo.
>>
>> I am referring to get_page(...) which would not work for foreign pages.
>
> Ah, that's part of the de-allocation portion of the code, which I've
> previously indicated would need to be skipped in the case here.
>
>>> In the end it's actually quite the opposite that I'm thinking: For
>>> the caller it shouldn't, for example, matter whether the
>>> requested page was paged out. We wouldn't even call
>>> guest_physmap_remove_page() in that case, while
>>> guest_remove_page() would take care of it.
>>
>> But those pages should never be removed via physmap, I am correct?
>
> The guest is unaware of paging, so as long as it's permitted to
> use this hypercall, it should make no difference to it whether a
> page is actually in memory at the time it issues this hypercall.
Well, I only agree with that statement if it is possible to map page
that can be page out with one of the physmap hypercall.
If it is not possible, then I don't think we should allow the guest to
remove those pages.
One of my main concern is a guest trying to mistakenly use
XENMEM_remove_from_physmap rather XENMEM_decrease_reservation. IIUC your
point above, the former would not do de-allocation. So you would end up
losing the page forever (there are no way to get the page back for
auto-translated guest).
I realize the issue is already present (at least on Arm) today. But if
we were going to modify XENME_remove_from_physmap, then a bit more
safety check to avoid a guest shooting its own foot would be useful.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-14 16:33 ` Julien Grall
0 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2019-04-14 16:33 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
Hi,
On 4/9/19 1:21 PM, Jan Beulich wrote:
>>>> On 09.04.19 at 11:50, <julien.grall@arm.com> wrote:
>> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>>>> 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?
>>>
>>> Hmm, I'm confused. Afaics the only two types it refuses a request
>>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>>> I perhaps overlooking something you see?
>>>
>>> Or are you referring to the mfn_invalid() check (which isn't x86-
>>> specific)? This ought to be covered by the p2m_is_paging() and
>>> p2m_mmio_direct special cases a few lines up from there. Other
>>> cases with invalid MFNs would indeed represent an error condition
>>> imo.
>>
>> I am referring to get_page(...) which would not work for foreign pages.
>
> Ah, that's part of the de-allocation portion of the code, which I've
> previously indicated would need to be skipped in the case here.
>
>>> In the end it's actually quite the opposite that I'm thinking: For
>>> the caller it shouldn't, for example, matter whether the
>>> requested page was paged out. We wouldn't even call
>>> guest_physmap_remove_page() in that case, while
>>> guest_remove_page() would take care of it.
>>
>> But those pages should never be removed via physmap, I am correct?
>
> The guest is unaware of paging, so as long as it's permitted to
> use this hypercall, it should make no difference to it whether a
> page is actually in memory at the time it issues this hypercall.
Well, I only agree with that statement if it is possible to map page
that can be page out with one of the physmap hypercall.
If it is not possible, then I don't think we should allow the guest to
remove those pages.
One of my main concern is a guest trying to mistakenly use
XENMEM_remove_from_physmap rather XENMEM_decrease_reservation. IIUC your
point above, the former would not do de-allocation. So you would end up
losing the page forever (there are no way to get the page back for
auto-translated guest).
I realize the issue is already present (at least on Arm) today. But if
we were going to modify XENME_remove_from_physmap, then a bit more
safety check to avoid a guest shooting its own foot would be useful.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-25 10:36 ` Jan Beulich
0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2019-04-25 10:36 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
>>> On 14.04.19 at 18:33, <julien.grall@arm.com> wrote:
> Hi,
>
> On 4/9/19 1:21 PM, Jan Beulich wrote:
>>>>> On 09.04.19 at 11:50, <julien.grall@arm.com> wrote:
>>> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>>>>> 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?
>>>>
>>>> Hmm, I'm confused. Afaics the only two types it refuses a request
>>>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>>>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>>>> I perhaps overlooking something you see?
>>>>
>>>> Or are you referring to the mfn_invalid() check (which isn't x86-
>>>> specific)? This ought to be covered by the p2m_is_paging() and
>>>> p2m_mmio_direct special cases a few lines up from there. Other
>>>> cases with invalid MFNs would indeed represent an error condition
>>>> imo.
>>>
>>> I am referring to get_page(...) which would not work for foreign pages.
>>
>> Ah, that's part of the de-allocation portion of the code, which I've
>> previously indicated would need to be skipped in the case here.
>>
>>>> In the end it's actually quite the opposite that I'm thinking: For
>>>> the caller it shouldn't, for example, matter whether the
>>>> requested page was paged out. We wouldn't even call
>>>> guest_physmap_remove_page() in that case, while
>>>> guest_remove_page() would take care of it.
>>>
>>> But those pages should never be removed via physmap, I am correct?
>>
>> The guest is unaware of paging, so as long as it's permitted to
>> use this hypercall, it should make no difference to it whether a
>> page is actually in memory at the time it issues this hypercall.
>
> Well, I only agree with that statement if it is possible to map page
> that can be page out with one of the physmap hypercall.
>
> If it is not possible, then I don't think we should allow the guest to
> remove those pages.
From the looks of it XENMAPSPACE_gmfn mapping would simply
fail for paged-out pages. Hence on one hand I agree with you
that "add" and "remove" should act similarly. Otoh though I
think we'd widen a problem here, because to me it looks like
passing a GFN of a paged out page to add-to-physmap should
work (and transparently to the guest).
> One of my main concern is a guest trying to mistakenly use
> XENMEM_remove_from_physmap rather XENMEM_decrease_reservation. IIUC your
> point above, the former would not do de-allocation. So you would end up
> losing the page forever (there are no way to get the page back for
> auto-translated guest).
Correct - to me that's implied from the sub-function name.
Just like add-to-physmap doesn't allocate anything, remove-
from-physmap doesn't free.
> I realize the issue is already present (at least on Arm) today. But if
> we were going to modify XENME_remove_from_physmap, then a bit more
> safety check to avoid a guest shooting its own foot would be useful.
I'm not sure I see ways of properly checking for such
situations - right after any such check the information gathered
might already be stale. And I don't think we go to great lengths
to prevent guests shooting themselves in the foot by other
means.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-25 10:36 ` Jan Beulich
0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2019-04-25 10:36 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
>>> On 14.04.19 at 18:33, <julien.grall@arm.com> wrote:
> Hi,
>
> On 4/9/19 1:21 PM, Jan Beulich wrote:
>>>>> On 09.04.19 at 11:50, <julien.grall@arm.com> wrote:
>>> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>>>>> 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?
>>>>
>>>> Hmm, I'm confused. Afaics the only two types it refuses a request
>>>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>>>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>>>> I perhaps overlooking something you see?
>>>>
>>>> Or are you referring to the mfn_invalid() check (which isn't x86-
>>>> specific)? This ought to be covered by the p2m_is_paging() and
>>>> p2m_mmio_direct special cases a few lines up from there. Other
>>>> cases with invalid MFNs would indeed represent an error condition
>>>> imo.
>>>
>>> I am referring to get_page(...) which would not work for foreign pages.
>>
>> Ah, that's part of the de-allocation portion of the code, which I've
>> previously indicated would need to be skipped in the case here.
>>
>>>> In the end it's actually quite the opposite that I'm thinking: For
>>>> the caller it shouldn't, for example, matter whether the
>>>> requested page was paged out. We wouldn't even call
>>>> guest_physmap_remove_page() in that case, while
>>>> guest_remove_page() would take care of it.
>>>
>>> But those pages should never be removed via physmap, I am correct?
>>
>> The guest is unaware of paging, so as long as it's permitted to
>> use this hypercall, it should make no difference to it whether a
>> page is actually in memory at the time it issues this hypercall.
>
> Well, I only agree with that statement if it is possible to map page
> that can be page out with one of the physmap hypercall.
>
> If it is not possible, then I don't think we should allow the guest to
> remove those pages.
From the looks of it XENMAPSPACE_gmfn mapping would simply
fail for paged-out pages. Hence on one hand I agree with you
that "add" and "remove" should act similarly. Otoh though I
think we'd widen a problem here, because to me it looks like
passing a GFN of a paged out page to add-to-physmap should
work (and transparently to the guest).
> One of my main concern is a guest trying to mistakenly use
> XENMEM_remove_from_physmap rather XENMEM_decrease_reservation. IIUC your
> point above, the former would not do de-allocation. So you would end up
> losing the page forever (there are no way to get the page back for
> auto-translated guest).
Correct - to me that's implied from the sub-function name.
Just like add-to-physmap doesn't allocate anything, remove-
from-physmap doesn't free.
> I realize the issue is already present (at least on Arm) today. But if
> we were going to modify XENME_remove_from_physmap, then a bit more
> safety check to avoid a guest shooting its own foot would be useful.
I'm not sure I see ways of properly checking for such
situations - right after any such check the information gathered
might already be stale. And I don't think we go to great lengths
to prevent guests shooting themselves in the foot by other
means.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-08 11:58 ` Julien Grall
0 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2019-04-08 11:58 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan
Hi Jan,
On 3/5/19 1:28 PM, Jan Beulich wrote:
> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>
> /*
> * Unmaps the page appearing at a particular GPFN from the specified guest's
> - * pseudophysical address space.
> + * physical address space (translated guests only).
While you modify the comment, can you replace GPFN with GFN?
Other than that:
Reviewed-by: Julien Grall <julien.grall@arm.com>
Cheer,s
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-08 11:58 ` Julien Grall
0 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2019-04-08 11:58 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan
Hi Jan,
On 3/5/19 1:28 PM, Jan Beulich wrote:
> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>
> /*
> * Unmaps the page appearing at a particular GPFN from the specified guest's
> - * pseudophysical address space.
> + * physical address space (translated guests only).
While you modify the comment, can you replace GPFN with GFN?
Other than that:
Reviewed-by: Julien Grall <julien.grall@arm.com>
Cheer,s
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-08 14:02 ` Jan Beulich
0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2019-04-08 14:02 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
>>> On 08.04.19 at 13:58, <julien.grall@arm.com> wrote:
> On 3/5/19 1:28 PM, Jan Beulich wrote:
>> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>>
>> /*
>> * Unmaps the page appearing at a particular GPFN from the specified guest's
>> - * pseudophysical address space.
>> + * physical address space (translated guests only).
>
> While you modify the comment, can you replace GPFN with GFN?
I did consider this when writing the patch, but then this would
bring it out of sync with the structure field and its associated
comment. Plus the "add" side comment would then want
changing too. And public/memory.h has quite a few more uses
of GPFN / gpfn.
To be honest I'd prefer to leave this as is right here, for it to get
cleaned up in one go.
> Other than that:
>
> Reviewed-by: Julien Grall <julien.grall@arm.com>
Thanks, but as per above I'm not sure whether to actually apply
it.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-08 14:02 ` Jan Beulich
0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2019-04-08 14:02 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
>>> On 08.04.19 at 13:58, <julien.grall@arm.com> wrote:
> On 3/5/19 1:28 PM, Jan Beulich wrote:
>> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>>
>> /*
>> * Unmaps the page appearing at a particular GPFN from the specified guest's
>> - * pseudophysical address space.
>> + * physical address space (translated guests only).
>
> While you modify the comment, can you replace GPFN with GFN?
I did consider this when writing the patch, but then this would
bring it out of sync with the structure field and its associated
comment. Plus the "add" side comment would then want
changing too. And public/memory.h has quite a few more uses
of GPFN / gpfn.
To be honest I'd prefer to leave this as is right here, for it to get
cleaned up in one go.
> Other than that:
>
> Reviewed-by: Julien Grall <julien.grall@arm.com>
Thanks, but as per above I'm not sure whether to actually apply
it.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-08 16:10 ` Julien Grall
0 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2019-04-08 16:10 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
Hi Jan,
On 4/8/19 3:02 PM, Jan Beulich wrote:
>>>> On 08.04.19 at 13:58, <julien.grall@arm.com> wrote:
>> On 3/5/19 1:28 PM, Jan Beulich wrote:
>>> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>>>
>>> /*
>>> * Unmaps the page appearing at a particular GPFN from the specified guest's
>>> - * pseudophysical address space.
>>> + * physical address space (translated guests only).
>>
>> While you modify the comment, can you replace GPFN with GFN?
>
> I did consider this when writing the patch, but then this would
> bring it out of sync with the structure field and its associated
> comment. Plus the "add" side comment would then want
> changing too. And public/memory.h has quite a few more uses
> of GPFN / gpfn.
>
> To be honest I'd prefer to leave this as is right here, for it to get
> cleaned up in one go.
I am fine with that.
>
>> Other than that:
>>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>
> Thanks, but as per above I'm not sure whether to actually apply > it.
Yes it applies, for the common code (and indirectly Arm).
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-04-08 16:10 ` Julien Grall
0 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2019-04-08 16:10 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
Hi Jan,
On 4/8/19 3:02 PM, Jan Beulich wrote:
>>>> On 08.04.19 at 13:58, <julien.grall@arm.com> wrote:
>> On 3/5/19 1:28 PM, Jan Beulich wrote:
>>> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>>>
>>> /*
>>> * Unmaps the page appearing at a particular GPFN from the specified guest's
>>> - * pseudophysical address space.
>>> + * physical address space (translated guests only).
>>
>> While you modify the comment, can you replace GPFN with GFN?
>
> I did consider this when writing the patch, but then this would
> bring it out of sync with the structure field and its associated
> comment. Plus the "add" side comment would then want
> changing too. And public/memory.h has quite a few more uses
> of GPFN / gpfn.
>
> To be honest I'd prefer to leave this as is right here, for it to get
> cleaned up in one go.
I am fine with that.
>
>> Other than that:
>>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>
> Thanks, but as per above I'm not sure whether to actually apply > it.
Yes it applies, for the common code (and indirectly Arm).
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Ping: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-05-13 8:06 ` Jan Beulich
0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2019-05-13 8:06 UTC (permalink / raw)
To: Andrew Cooper, George Dunlap
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, Tim Deegan, Ian Jackson, Julien Grall,
xen-devel
>>> On 05.03.19 at 14:28, 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>
Andrew, Goerge - any chance of getting an ack for the pretty simple
x86-specific code adjustment?
Jan
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
> mfn_t mfn = INVALID_MFN;
> p2m_type_t p2mt;
>
> - if ( !paging_mode_translate(d) )
> - return -EACCES;
> -
> switch ( space )
> {
> case XENMAPSPACE_shared_info:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
> long rc = 0;
> union xen_add_to_physmap_batch_extra extra;
>
> + ASSERT(paging_mode_translate(d));
> +
> if ( xatp->space != XENMAPSPACE_gmfn_foreign )
> extra.res0 = 0;
> else
> @@ -997,12 +999,15 @@ static int get_reserved_device_memory(xe
>
> static long xatp_permission_check(struct domain *d, unsigned int space)
> {
> + if ( !paging_mode_translate(d) )
> + return -EACCES;
> +
> /*
> * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain
> * to map this kind of space to itself.
> */
> if ( (space == XENMAPSPACE_dev_mmio) &&
> - (!is_hardware_domain(current->domain) || (d != current->domain)) )
> + (!is_hardware_domain(d) || (d != current->domain)) )
> return -EACCES;
>
> return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> @@ -1386,7 +1391,9 @@ long do_memory_op(unsigned long cmd, XEN
> if ( d == NULL )
> return -ESRCH;
>
> - rc = xsm_remove_from_physmap(XSM_TARGET, curr_d, d);
> + rc = paging_mode_translate(d)
> + ? xsm_remove_from_physmap(XSM_TARGET, curr_d, d)
> + : -EACCES;
> if ( rc )
> {
> rcu_unlock_domain(d);
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -231,7 +231,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_map
>
> /*
> * Sets the GPFN at which a particular page appears in the specified
> guest's
> - * pseudophysical address space.
> + * physical address space (translated guests only).
> * arg == addr of xen_add_to_physmap_t.
> */
> #define XENMEM_add_to_physmap 7
> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>
> /*
> * Unmaps the page appearing at a particular GPFN from the specified
> guest's
> - * pseudophysical address space.
> + * physical address space (translated guests only).
> * arg == addr of xen_remove_from_physmap_t.
> */
> #define XENMEM_remove_from_physmap 15
>
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* [Xen-devel] Ping: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-05-13 8:06 ` Jan Beulich
0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2019-05-13 8:06 UTC (permalink / raw)
To: Andrew Cooper, George Dunlap
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, Tim Deegan, Ian Jackson, Julien Grall,
xen-devel
>>> On 05.03.19 at 14:28, 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>
Andrew, Goerge - any chance of getting an ack for the pretty simple
x86-specific code adjustment?
Jan
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
> mfn_t mfn = INVALID_MFN;
> p2m_type_t p2mt;
>
> - if ( !paging_mode_translate(d) )
> - return -EACCES;
> -
> switch ( space )
> {
> case XENMAPSPACE_shared_info:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
> long rc = 0;
> union xen_add_to_physmap_batch_extra extra;
>
> + ASSERT(paging_mode_translate(d));
> +
> if ( xatp->space != XENMAPSPACE_gmfn_foreign )
> extra.res0 = 0;
> else
> @@ -997,12 +999,15 @@ static int get_reserved_device_memory(xe
>
> static long xatp_permission_check(struct domain *d, unsigned int space)
> {
> + if ( !paging_mode_translate(d) )
> + return -EACCES;
> +
> /*
> * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain
> * to map this kind of space to itself.
> */
> if ( (space == XENMAPSPACE_dev_mmio) &&
> - (!is_hardware_domain(current->domain) || (d != current->domain)) )
> + (!is_hardware_domain(d) || (d != current->domain)) )
> return -EACCES;
>
> return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> @@ -1386,7 +1391,9 @@ long do_memory_op(unsigned long cmd, XEN
> if ( d == NULL )
> return -ESRCH;
>
> - rc = xsm_remove_from_physmap(XSM_TARGET, curr_d, d);
> + rc = paging_mode_translate(d)
> + ? xsm_remove_from_physmap(XSM_TARGET, curr_d, d)
> + : -EACCES;
> if ( rc )
> {
> rcu_unlock_domain(d);
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -231,7 +231,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_map
>
> /*
> * Sets the GPFN at which a particular page appears in the specified
> guest's
> - * pseudophysical address space.
> + * physical address space (translated guests only).
> * arg == addr of xen_add_to_physmap_t.
> */
> #define XENMEM_add_to_physmap 7
> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>
> /*
> * Unmaps the page appearing at a particular GPFN from the specified
> guest's
> - * pseudophysical address space.
> + * physical address space (translated guests only).
> * arg == addr of xen_remove_from_physmap_t.
> */
> #define XENMEM_remove_from_physmap 15
>
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-05-13 14:35 ` George Dunlap
0 siblings, 0 replies; 42+ messages in thread
From: George Dunlap @ 2019-05-13 14:35 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall
On 3/5/19 1:28 PM, 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>
Looks good, sorry for the delay:
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
A couple of comments:
> ---
> 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.
Looks like the P2M_ALLOC was introduced in c/s 06e7bfed206. I can't
immediately see any reason to do the allocation -- I think it just must
have been C&P without much thought given as to what was going to happen
next.
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
> mfn_t mfn = INVALID_MFN;
> p2m_type_t p2mt;
>
> - if ( !paging_mode_translate(d) )
> - return -EACCES;
> -
> switch ( space )
> {
> case XENMAPSPACE_shared_info:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
> long rc = 0;
> union xen_add_to_physmap_batch_extra extra;
>
> + ASSERT(paging_mode_translate(d));
So, just trying to think through the principles behind these two. We
know that this is never going to be called w/o first calling
xatp_permission_check(); if we assume that such a change will be tested
(i.e., that something with paging_mode_translate() will call this
hypercall before a release), then a single ASSERT() should be enough to
make sure that both functions are updated properly?
I guess that's good enough. (It's hard not to start to get paranoid
when you ask yourself these sorts of questions.)
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-05-13 14:35 ` George Dunlap
0 siblings, 0 replies; 42+ messages in thread
From: George Dunlap @ 2019-05-13 14:35 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall
On 3/5/19 1:28 PM, 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>
Looks good, sorry for the delay:
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
A couple of comments:
> ---
> 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.
Looks like the P2M_ALLOC was introduced in c/s 06e7bfed206. I can't
immediately see any reason to do the allocation -- I think it just must
have been C&P without much thought given as to what was going to happen
next.
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
> mfn_t mfn = INVALID_MFN;
> p2m_type_t p2mt;
>
> - if ( !paging_mode_translate(d) )
> - return -EACCES;
> -
> switch ( space )
> {
> case XENMAPSPACE_shared_info:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
> long rc = 0;
> union xen_add_to_physmap_batch_extra extra;
>
> + ASSERT(paging_mode_translate(d));
So, just trying to think through the principles behind these two. We
know that this is never going to be called w/o first calling
xatp_permission_check(); if we assume that such a change will be tested
(i.e., that something with paging_mode_translate() will call this
hypercall before a release), then a single ASSERT() should be enough to
make sure that both functions are updated properly?
I guess that's good enough. (It's hard not to start to get paranoid
when you ask yourself these sorts of questions.)
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-05-13 15:13 ` Jan Beulich
0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2019-05-13 15:13 UTC (permalink / raw)
To: george.dunlap
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, xen-devel
>>> On 13.05.19 at 16:35, <george.dunlap@citrix.com> wrote:
> On 3/5/19 1:28 PM, 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>
>
> Looks good, sorry for the delay:
>
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Thanks.
> A couple of comments:
>
>> ---
>> 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.
>
> Looks like the P2M_ALLOC was introduced in c/s 06e7bfed206. I can't
> immediately see any reason to do the allocation -- I think it just must
> have been C&P without much thought given as to what was going to happen
> next.
The question is: If we remove it, what is the -ENOENT condition
going to be?
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
>> mfn_t mfn = INVALID_MFN;
>> p2m_type_t p2mt;
>>
>> - if ( !paging_mode_translate(d) )
>> - return -EACCES;
>> -
>> switch ( space )
>> {
>> case XENMAPSPACE_shared_info:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
>> long rc = 0;
>> union xen_add_to_physmap_batch_extra extra;
>>
>> + ASSERT(paging_mode_translate(d));
>
> So, just trying to think through the principles behind these two. We
> know that this is never going to be called w/o first calling
> xatp_permission_check(); if we assume that such a change will be tested
> (i.e., that something with paging_mode_translate() will call this
> hypercall before a release), then a single ASSERT() should be enough to
> make sure that both functions are updated properly?
Yes, that's my expectation.
> I guess that's good enough. (It's hard not to start to get paranoid
> when you ask yourself these sorts of questions.)
Good. (Indeed.)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
@ 2019-05-13 15:13 ` Jan Beulich
0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2019-05-13 15:13 UTC (permalink / raw)
To: george.dunlap
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, xen-devel
>>> On 13.05.19 at 16:35, <george.dunlap@citrix.com> wrote:
> On 3/5/19 1:28 PM, 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>
>
> Looks good, sorry for the delay:
>
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Thanks.
> A couple of comments:
>
>> ---
>> 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.
>
> Looks like the P2M_ALLOC was introduced in c/s 06e7bfed206. I can't
> immediately see any reason to do the allocation -- I think it just must
> have been C&P without much thought given as to what was going to happen
> next.
The question is: If we remove it, what is the -ENOENT condition
going to be?
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
>> mfn_t mfn = INVALID_MFN;
>> p2m_type_t p2mt;
>>
>> - if ( !paging_mode_translate(d) )
>> - return -EACCES;
>> -
>> switch ( space )
>> {
>> case XENMAPSPACE_shared_info:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
>> long rc = 0;
>> union xen_add_to_physmap_batch_extra extra;
>>
>> + ASSERT(paging_mode_translate(d));
>
> So, just trying to think through the principles behind these two. We
> know that this is never going to be called w/o first calling
> xatp_permission_check(); if we assume that such a change will be tested
> (i.e., that something with paging_mode_translate() will call this
> hypercall before a release), then a single ASSERT() should be enough to
> make sure that both functions are updated properly?
Yes, that's my expectation.
> I guess that's good enough. (It's hard not to start to get paranoid
> when you ask yourself these sorts of questions.)
Good. (Indeed.)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread