Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[Jan Beulich]

>>> On 02.12.15 at 18:14,  wrote:
> On 02/12/15 16:46, Tim Deegan wrote:
>> At 16:30 + on 02 Dec (1449073841), George Dunlap wrote:
>>> On 02/12/15 16:23, Tim Deegan wrote:
 At 07:25 + on 02 Dec (1449041100), Tian, Kevin wrote:
>> From: David Vrabel [mailto:david.vra...@citrix.com]
>> Sent: Saturday, November 14, 2015 2:50 AM
>>
>> If a page is freed without translations being invalidated, and the page 
>> is
>> subsequently allocated to another domain, a guest with a cached
>> translation will still be able to access the page.
>>
>> Currently translations are invalidated before releasing the page ref, but
>> while still holding the mm locks.  To allow translations to be 
>> invalidated
>> without holding the mm locks, we need to keep a reference to the page
>> for a bit longer in some cases.
>>
>> [ This seems difficult to a) verify as correct; and b) difficult to get
>> correct in the future.  A better suggestion would be useful.  Perhaps
>> using something like pg->tlbflush_needed mechanism that already exists
>> for pages from PV guests? ]
>
> Per-page flag looks clean in general, but not an expert here. Tim might
> have a better idea.

 I think you can probably use the tlbflush_timestamp stuff as-is for
 EPT flushes -- the existing TLB shootdowns already drop all EPT
 translations.
>>>
>>> Are you saying that if you do a TLB shootdown you don't need to do an
>>> invept command?
>> 
>> Yes, I think so.  flush_area_local() -> hvm_flush_guest_tlbs() ->
>> hvm_asid_flush_core() should DTRT.
> 
> I don't think so. Guest-physical mappings are only tagged with the EP4TA
> and not the VPID, thus changing the VPID does nothing.

Indeed. And in fact they tell us that using VPID together with EPT is
kind of pointless when different guests use different EPTPs.

> Thus we would need to extend the current mechanism to determine that a
> invept is also needed.

Agreed.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[George Dunlap]

On Thu, Dec 3, 2015 at 10:22 AM, Jan Beulich  wrote:
 On 02.12.15 at 18:14,  wrote:
>> On 02/12/15 16:46, Tim Deegan wrote:
>>> At 16:30 + on 02 Dec (1449073841), George Dunlap wrote:
 On 02/12/15 16:23, Tim Deegan wrote:
> At 07:25 + on 02 Dec (1449041100), Tian, Kevin wrote:
>>> From: David Vrabel [mailto:david.vra...@citrix.com]
>>> Sent: Saturday, November 14, 2015 2:50 AM
>>>
>>> If a page is freed without translations being invalidated, and the page 
>>> is
>>> subsequently allocated to another domain, a guest with a cached
>>> translation will still be able to access the page.
>>>
>>> Currently translations are invalidated before releasing the page ref, 
>>> but
>>> while still holding the mm locks.  To allow translations to be 
>>> invalidated
>>> without holding the mm locks, we need to keep a reference to the page
>>> for a bit longer in some cases.
>>>
>>> [ This seems difficult to a) verify as correct; and b) difficult to get
>>> correct in the future.  A better suggestion would be useful.  Perhaps
>>> using something like pg->tlbflush_needed mechanism that already exists
>>> for pages from PV guests? ]
>>
>> Per-page flag looks clean in general, but not an expert here. Tim might
>> have a better idea.
>
> I think you can probably use the tlbflush_timestamp stuff as-is for
> EPT flushes -- the existing TLB shootdowns already drop all EPT
> translations.

 Are you saying that if you do a TLB shootdown you don't need to do an
 invept command?
>>>
>>> Yes, I think so.  flush_area_local() -> hvm_flush_guest_tlbs() ->
>>> hvm_asid_flush_core() should DTRT.
>>
>> I don't think so. Guest-physical mappings are only tagged with the EP4TA
>> and not the VPID, thus changing the VPID does nothing.
>
> Indeed. And in fact they tell us that using VPID together with EPT is
> kind of pointless when different guests use different EPTPs.

Except that of course, that they allow us to isolate the flushing of
combined mappings from between guests...

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[Tim Deegan]

At 07:25 + on 02 Dec (1449041100), Tian, Kevin wrote:
> > From: David Vrabel [mailto:david.vra...@citrix.com]
> > Sent: Saturday, November 14, 2015 2:50 AM
> > 
> > If a page is freed without translations being invalidated, and the page is
> > subsequently allocated to another domain, a guest with a cached
> > translation will still be able to access the page.
> > 
> > Currently translations are invalidated before releasing the page ref, but
> > while still holding the mm locks.  To allow translations to be invalidated
> > without holding the mm locks, we need to keep a reference to the page
> > for a bit longer in some cases.
> > 
> > [ This seems difficult to a) verify as correct; and b) difficult to get
> > correct in the future.  A better suggestion would be useful.  Perhaps
> > using something like pg->tlbflush_needed mechanism that already exists
> > for pages from PV guests? ]
> 
> Per-page flag looks clean in general, but not an expert here. Tim might
> have a better idea.

I think you can probably use the tlbflush_timestamp stuff as-is for
EPT flushes -- the existing TLB shootdowns already drop all EPT
translations.

Holding the ref until the flush finishes is a common enough idiom but
I can see that having to track lists of them does make it a little clunky. 

Cheers,

Tim.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[George Dunlap]

On 02/12/15 16:23, Tim Deegan wrote:
> At 07:25 + on 02 Dec (1449041100), Tian, Kevin wrote:
>>> From: David Vrabel [mailto:david.vra...@citrix.com]
>>> Sent: Saturday, November 14, 2015 2:50 AM
>>>
>>> If a page is freed without translations being invalidated, and the page is
>>> subsequently allocated to another domain, a guest with a cached
>>> translation will still be able to access the page.
>>>
>>> Currently translations are invalidated before releasing the page ref, but
>>> while still holding the mm locks.  To allow translations to be invalidated
>>> without holding the mm locks, we need to keep a reference to the page
>>> for a bit longer in some cases.
>>>
>>> [ This seems difficult to a) verify as correct; and b) difficult to get
>>> correct in the future.  A better suggestion would be useful.  Perhaps
>>> using something like pg->tlbflush_needed mechanism that already exists
>>> for pages from PV guests? ]
>>
>> Per-page flag looks clean in general, but not an expert here. Tim might
>> have a better idea.
> 
> I think you can probably use the tlbflush_timestamp stuff as-is for
> EPT flushes -- the existing TLB shootdowns already drop all EPT
> translations.

Are you saying that if you do a TLB shootdown you don't need to do an
invept command?  That doesn't sound right.

 -George


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[David Vrabel]

On 02/12/15 16:23, Tim Deegan wrote:
> At 07:25 + on 02 Dec (1449041100), Tian, Kevin wrote:
>>> From: David Vrabel [mailto:david.vra...@citrix.com]
>>> Sent: Saturday, November 14, 2015 2:50 AM
>>>
>>> If a page is freed without translations being invalidated, and the page is
>>> subsequently allocated to another domain, a guest with a cached
>>> translation will still be able to access the page.
>>>
>>> Currently translations are invalidated before releasing the page ref, but
>>> while still holding the mm locks.  To allow translations to be invalidated
>>> without holding the mm locks, we need to keep a reference to the page
>>> for a bit longer in some cases.
>>>
>>> [ This seems difficult to a) verify as correct; and b) difficult to get
>>> correct in the future.  A better suggestion would be useful.  Perhaps
>>> using something like pg->tlbflush_needed mechanism that already exists
>>> for pages from PV guests? ]
>>
>> Per-page flag looks clean in general, but not an expert here. Tim might
>> have a better idea.
> 
> I think you can probably use the tlbflush_timestamp stuff as-is for
> EPT flushes -- the existing TLB shootdowns already drop all EPT
> translations.

Regular TLB invalidate instructions (e.g., INVLPG) only invalidate
linear and combined mappings.  guest-physical mappings are not affected
and require an INVEPT.

Section 2.3.3.2 of the Intel SDM vol 3 gives a useful summary.

David

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[George Dunlap]

On 02/12/15 16:46, Tim Deegan wrote:
> At 16:30 + on 02 Dec (1449073841), George Dunlap wrote:
>> On 02/12/15 16:23, Tim Deegan wrote:
>>> At 07:25 + on 02 Dec (1449041100), Tian, Kevin wrote:
> From: David Vrabel [mailto:david.vra...@citrix.com]
> Sent: Saturday, November 14, 2015 2:50 AM
>
> If a page is freed without translations being invalidated, and the page is
> subsequently allocated to another domain, a guest with a cached
> translation will still be able to access the page.
>
> Currently translations are invalidated before releasing the page ref, but
> while still holding the mm locks.  To allow translations to be invalidated
> without holding the mm locks, we need to keep a reference to the page
> for a bit longer in some cases.
>
> [ This seems difficult to a) verify as correct; and b) difficult to get
> correct in the future.  A better suggestion would be useful.  Perhaps
> using something like pg->tlbflush_needed mechanism that already exists
> for pages from PV guests? ]

 Per-page flag looks clean in general, but not an expert here. Tim might
 have a better idea.
>>>
>>> I think you can probably use the tlbflush_timestamp stuff as-is for
>>> EPT flushes -- the existing TLB shootdowns already drop all EPT
>>> translations.
>>
>> Are you saying that if you do a TLB shootdown you don't need to do an
>> invept command?
> 
> Yes, I think so.  flush_area_local() -> hvm_flush_guest_tlbs() ->
> hvm_asid_flush_core() should DTRT.

Looks like that ends up causing vpid_sync_all(), which executes invvpid.
 I presume that's different than invept.  But perhaps we could extend
the basic functionality to call invept when we need it.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[Tim Deegan]

At 16:30 + on 02 Dec (1449073841), George Dunlap wrote:
> On 02/12/15 16:23, Tim Deegan wrote:
> > At 07:25 + on 02 Dec (1449041100), Tian, Kevin wrote:
> >>> From: David Vrabel [mailto:david.vra...@citrix.com]
> >>> Sent: Saturday, November 14, 2015 2:50 AM
> >>>
> >>> If a page is freed without translations being invalidated, and the page is
> >>> subsequently allocated to another domain, a guest with a cached
> >>> translation will still be able to access the page.
> >>>
> >>> Currently translations are invalidated before releasing the page ref, but
> >>> while still holding the mm locks.  To allow translations to be invalidated
> >>> without holding the mm locks, we need to keep a reference to the page
> >>> for a bit longer in some cases.
> >>>
> >>> [ This seems difficult to a) verify as correct; and b) difficult to get
> >>> correct in the future.  A better suggestion would be useful.  Perhaps
> >>> using something like pg->tlbflush_needed mechanism that already exists
> >>> for pages from PV guests? ]
> >>
> >> Per-page flag looks clean in general, but not an expert here. Tim might
> >> have a better idea.
> > 
> > I think you can probably use the tlbflush_timestamp stuff as-is for
> > EPT flushes -- the existing TLB shootdowns already drop all EPT
> > translations.
> 
> Are you saying that if you do a TLB shootdown you don't need to do an
> invept command?

Yes, I think so.  flush_area_local() -> hvm_flush_guest_tlbs() ->
hvm_asid_flush_core() should DTRT.

Tim.


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[David Vrabel]

On 02/12/15 16:46, Tim Deegan wrote:
> At 16:30 + on 02 Dec (1449073841), George Dunlap wrote:
>> On 02/12/15 16:23, Tim Deegan wrote:
>>> At 07:25 + on 02 Dec (1449041100), Tian, Kevin wrote:
> From: David Vrabel [mailto:david.vra...@citrix.com]
> Sent: Saturday, November 14, 2015 2:50 AM
>
> If a page is freed without translations being invalidated, and the page is
> subsequently allocated to another domain, a guest with a cached
> translation will still be able to access the page.
>
> Currently translations are invalidated before releasing the page ref, but
> while still holding the mm locks.  To allow translations to be invalidated
> without holding the mm locks, we need to keep a reference to the page
> for a bit longer in some cases.
>
> [ This seems difficult to a) verify as correct; and b) difficult to get
> correct in the future.  A better suggestion would be useful.  Perhaps
> using something like pg->tlbflush_needed mechanism that already exists
> for pages from PV guests? ]

 Per-page flag looks clean in general, but not an expert here. Tim might
 have a better idea.
>>>
>>> I think you can probably use the tlbflush_timestamp stuff as-is for
>>> EPT flushes -- the existing TLB shootdowns already drop all EPT
>>> translations.
>>
>> Are you saying that if you do a TLB shootdown you don't need to do an
>> invept command?
> 
> Yes, I think so.  flush_area_local() -> hvm_flush_guest_tlbs() ->
> hvm_asid_flush_core() should DTRT.

I don't think so. Guest-physical mappings are only tagged with the EP4TA
and not the VPID, thus changing the VPID does nothing.

Thus we would need to extend the current mechanism to determine that a
invept is also needed.

David

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[George Dunlap]

On 02/12/15 16:45, David Vrabel wrote:
> On 02/12/15 16:23, Tim Deegan wrote:
>> At 07:25 + on 02 Dec (1449041100), Tian, Kevin wrote:
 From: David Vrabel [mailto:david.vra...@citrix.com]
 Sent: Saturday, November 14, 2015 2:50 AM

 If a page is freed without translations being invalidated, and the page is
 subsequently allocated to another domain, a guest with a cached
 translation will still be able to access the page.

 Currently translations are invalidated before releasing the page ref, but
 while still holding the mm locks.  To allow translations to be invalidated
 without holding the mm locks, we need to keep a reference to the page
 for a bit longer in some cases.

 [ This seems difficult to a) verify as correct; and b) difficult to get
 correct in the future.  A better suggestion would be useful.  Perhaps
 using something like pg->tlbflush_needed mechanism that already exists
 for pages from PV guests? ]
>>>
>>> Per-page flag looks clean in general, but not an expert here. Tim might
>>> have a better idea.
>>
>> I think you can probably use the tlbflush_timestamp stuff as-is for
>> EPT flushes -- the existing TLB shootdowns already drop all EPT
>> translations.
> 
> Regular TLB invalidate instructions (e.g., INVLPG) only invalidate
> linear and combined mappings.  guest-physical mappings are not affected
> and require an INVEPT.
> 
> Section 2.3.3.2 of the Intel SDM vol 3 gives a useful summary.

That's 28.3.3.2, in case anyone else wants to look for it...

 -George


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[Jan Beulich]

>>> On 02.12.15 at 17:30,  wrote:
> On 02/12/15 16:23, Tim Deegan wrote:
>> At 07:25 + on 02 Dec (1449041100), Tian, Kevin wrote:
 From: David Vrabel [mailto:david.vra...@citrix.com]
 Sent: Saturday, November 14, 2015 2:50 AM

 If a page is freed without translations being invalidated, and the page is
 subsequently allocated to another domain, a guest with a cached
 translation will still be able to access the page.

 Currently translations are invalidated before releasing the page ref, but
 while still holding the mm locks.  To allow translations to be invalidated
 without holding the mm locks, we need to keep a reference to the page
 for a bit longer in some cases.

 [ This seems difficult to a) verify as correct; and b) difficult to get
 correct in the future.  A better suggestion would be useful.  Perhaps
 using something like pg->tlbflush_needed mechanism that already exists
 for pages from PV guests? ]
>>>
>>> Per-page flag looks clean in general, but not an expert here. Tim might
>>> have a better idea.
>> 
>> I think you can probably use the tlbflush_timestamp stuff as-is for
>> EPT flushes -- the existing TLB shootdowns already drop all EPT
>> translations.
> 
> Are you saying that if you do a TLB shootdown you don't need to do an
> invept command?  That doesn't sound right.

No. flush_area_local() (i.e. the actual worker function of TLB
shootdown) calls hvm_flush_guest_tlbs() (in turn leading to
hvm_asid_flush_core()).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[Tian, Kevin]

> From: David Vrabel [mailto:david.vra...@citrix.com]
> Sent: Saturday, November 14, 2015 2:50 AM
> 
> If a page is freed without translations being invalidated, and the page is
> subsequently allocated to another domain, a guest with a cached
> translation will still be able to access the page.
> 
> Currently translations are invalidated before releasing the page ref, but
> while still holding the mm locks.  To allow translations to be invalidated
> without holding the mm locks, we need to keep a reference to the page
> for a bit longer in some cases.
> 
> [ This seems difficult to a) verify as correct; and b) difficult to get
> correct in the future.  A better suggestion would be useful.  Perhaps
> using something like pg->tlbflush_needed mechanism that already exists
> for pages from PV guests? ]

Per-page flag looks clean in general, but not an expert here. Tim might
have a better idea.

> 
> Signed-off-by: David Vrabel 
> ---
>  xen/arch/x86/mm/p2m.c | 9 +++--
>  xen/common/memory.c   | 2 +-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index ed0bbd7..e2c82b1 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2758,6 +2758,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long 
> fgfn,
>  p2m_type_t p2mt, p2mt_prev;
>  unsigned long prev_mfn, mfn;
>  struct page_info *page;
> +struct page_info *prev_page = NULL;
>  int rc;
>  struct domain *fdom;
> 
> @@ -2805,6 +2806,9 @@ int p2m_add_foreign(struct domain *tdom, unsigned long 
> fgfn,
>  prev_mfn = mfn_x(get_gfn(tdom, gpfn, _prev));
>  if ( mfn_valid(_mfn(prev_mfn)) )
>  {
> +prev_page = mfn_to_page(_mfn(prev_mfn));
> +get_page(prev_page, tdom);
> +
>  if ( is_xen_heap_mfn(prev_mfn) )
>  /* Xen heap frames are simply unhooked from this phys slot */
>  guest_physmap_remove_page(tdom, gpfn, prev_mfn, 0);
> @@ -2823,14 +2827,15 @@ int p2m_add_foreign(struct domain *tdom, unsigned long
> fgfn,
>   "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n",
>   gpfn, mfn, fgfn, tdom->domain_id, fdom->domain_id);
> 
> -put_page(page);
> -
>  /*
>   * This put_gfn for the above get_gfn for prev_mfn.  We must do this
>   * after set_foreign_p2m_entry so another cpu doesn't populate the gpfn
>   * before us.
>   */
>  put_gfn(tdom, gpfn);
> +if ( prev_page )
> +put_page(prev_page);
> +put_page(page);
> 
>  out:
>  if ( fdom )
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index a3bffb7..571c754 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -272,8 +272,8 @@ int guest_remove_page(struct domain *d, unsigned long 
> gmfn)
> 
>  guest_physmap_remove_page(d, gmfn, mfn, 0);
> 
> -put_page(page);
>  put_gfn(d, gmfn);
> +put_page(page);
> 
>  return 1;
>  }
> --
> 2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[Jan Beulich]

>>> On 16.11.15 at 13:02,  wrote:
> On 16/11/15 11:52, Jan Beulich wrote:
> On 13.11.15 at 19:49,  wrote:
>>> @@ -2805,6 +2806,9 @@ int p2m_add_foreign(struct domain *tdom, unsigned 
>>> long 
> fgfn,
>>>  prev_mfn = mfn_x(get_gfn(tdom, gpfn, _prev));
>>>  if ( mfn_valid(_mfn(prev_mfn)) )
>>>  {
>>> +prev_page = mfn_to_page(_mfn(prev_mfn));
>>> +get_page(prev_page, tdom);
>> 
>> If you're absolutely sure that this can never fail, then still at the very
>> least this imo should be documented by a respective ASSERT() (or
>> ASSERT_UNREACHABLE()).
> 
> What are you suggesting may fail?

get_page()

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[David Vrabel]

On 16/11/15 11:52, Jan Beulich wrote:
 On 13.11.15 at 19:49,  wrote:
>> @@ -2805,6 +2806,9 @@ int p2m_add_foreign(struct domain *tdom, unsigned long 
>> fgfn,
>>  prev_mfn = mfn_x(get_gfn(tdom, gpfn, _prev));
>>  if ( mfn_valid(_mfn(prev_mfn)) )
>>  {
>> +prev_page = mfn_to_page(_mfn(prev_mfn));
>> +get_page(prev_page, tdom);
> 
> If you're absolutely sure that this can never fail, then still at the very
> least this imo should be documented by a respective ASSERT() (or
> ASSERT_UNREACHABLE()).

What are you suggesting may fail?

David

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 2/3] mm: don't free pages until mm locks are released

[Jan Beulich]

>>> On 13.11.15 at 19:49,  wrote:
> @@ -2805,6 +2806,9 @@ int p2m_add_foreign(struct domain *tdom, unsigned long 
> fgfn,
>  prev_mfn = mfn_x(get_gfn(tdom, gpfn, _prev));
>  if ( mfn_valid(_mfn(prev_mfn)) )
>  {
> +prev_page = mfn_to_page(_mfn(prev_mfn));
> +get_page(prev_page, tdom);

If you're absolutely sure that this can never fail, then still at the very
least this imo should be documented by a respective ASSERT() (or
ASSERT_UNREACHABLE()).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel