Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

> The method I found to work is getting the maximum_gpfn from the guest
> and then calling populate_physmap with ++max_gpfn. The only problem
> then is that I don't see a way to "unpopulate" the page from the
> domain and free the corresponding mfn while the domain is running. Is
> that currently possible to do?

Never mind, evidently XENMEM_remove_from_physmap seems to be the
answer, it just lacks a libxc wrapper so I didn't notice it.

Cheers,
Tamas

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

On Mon, Jun 19, 2017 at 9:34 AM, Julien Grall  wrote:
>
>
> On 19/06/17 15:57, Tamas K Lengyel wrote:
>>
>> On Mon, Jun 19, 2017 at 8:52 AM, Julien Grall 
>> wrote:
>>>
>>>
>>>
>>> On 19/06/17 15:39, Tamas K Lengyel wrote:


 On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall 
 wrote:
>
>
> Hi,
>
>
> On 19/06/17 09:15, Jan Beulich wrote:
>
>
>
> On 18.06.17 at 21:19,  wrote:
>>>
>>>
>>>
>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper
>>> 
>>> wrote:



 On 04/04/17 14:14, Jan Beulich wrote:
>
>
>
> We shouldn't hand MFN info back from increase-reservation for
> translated domains, just like we don't for populate-physmap and
> memory-exchange. For full symmetry also check for a NULL guest
> handle
> in populate_physmap() (but note this makes no sense in
> memory_exchange(), as there the array is also an input).
>
> Signed-off-by: Jan Beulich 




 Reviewed-by: Andrew Cooper 
>>>
>>>
>>>
>>>
>>> Unfortunately I just had time to do testing with this change and I
>>> have to report that introduces a critical regression for my tools.
>>> With this change in-place performing increase_reservation on a target
>>> domain no longer reports the guest frame number for external tools,
>>> thus completely breaking advanced use-cases that require this
>>> information to be able to do altp2m gfn remapping. This is a critical
>>> step in being able to introduce shadow-pages that are used to hide
>>> breakpoints and other memory modifications from the guest.
>>
>>
>>
>>
>> While I can see your point, I'm afraid that's not how the
>> interface was meant to be used. The mere fact that
>> populate-physmap and memory-exchange didn't return the
>> MFN(s) suggests to me that you already need to have a way
>> to deal with having to find out another way. Or are you
>> suggesting you rely on guests not using these interfaces?
>>
>> As to a solution, I could possibly see us relax the change to
>> return the MFN(s) when the current and subject domains differ,
>> or even check paging mode of the caller domain instead of the
>> subject one (which would mean PVH Dom0 still wouldn't get to
>> see them). But if we do, imo we should do this consistently for
>> all three operations, rather than just for increase-reservation.
>>
>>> If at all possible, I would like to request this change not to be
>>> part
>>> of the 4.9 release.
>>
>>
>>
>>
>> Hmm, it's been there for all of the RCs, so I'm not really happy
>> to consider the option of reverting at this point in time. But
>> Julien will have the final say anyway.
>
>
>
>
> I am a bit confuse with the description of the problem. I understood
> "guest
> frame number" as GFN. But AFAICT, this hypercall was returning MFN even
> for
> HVM guests. So how this change is breaking altp2m remapping?



 For HVM guests this hypercall returns a GFN that can subsequently be
 populated into the guest physmap:

 xc_domain_increase_reservation_exact(xch, domid, 1, 0, 0, _gfn);
 xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, _gfn);
>>>
>>>
>>>
>>> I am sorry, I can't see how this can return a GFN for the HVM. Looking at
>>> the implementation of increase_reservation in Xen:
>>>
>>> mfn = page_to_mfn(page);
>>> if ( unlikely(__copy_to_guest_offset(a->extent_list, i, , 1)) )
>>>   goto out;
>>>
>>> This is an MFN and not a GFN. Except the strict check before, the code
>>> has
>>> not change for a while.
>>>
>>> AFAICT, the purpose of increase_reservation is not to allocate a new GFN,
>>> it
>>> will just allocate the host memory for it. At least on ARM we have
>>> nothing
>>> to say "this GFN region is free". I would be surprised that such things
>>> exists on x86.
>>>
>>
>> It returns memory that can be mapped into the guest physmap
>> subsequently. So I have been referring to it as a GFN that is not
>> mapped into the physmap - similar to the magic ring pages when they
>> are in use.
>
>
> Reading the implementation, roughly:
>
> * increase_reservation will only allocate host memory and return the
> corresponding MFN
> * populate_physmap will allocate host memory and map to a specific address
>
> So by calling both, you will effectively allocate twice memory and never be
> able to free the memory allocated by increase_reservation until the guest is
> destroyed. This will *never* allocate the corresponding GFN and I think is
> just working by luck in your case.

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Julien Grall]

On 19/06/17 15:57, Tamas K Lengyel wrote:

On Mon, Jun 19, 2017 at 8:52 AM, Julien Grall  wrote:



On 19/06/17 15:39, Tamas K Lengyel wrote:


On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall 
wrote:


Hi,


On 19/06/17 09:15, Jan Beulich wrote:



On 18.06.17 at 21:19,  wrote:



On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper

wrote:



On 04/04/17 14:14, Jan Beulich wrote:



We shouldn't hand MFN info back from increase-reservation for
translated domains, just like we don't for populate-physmap and
memory-exchange. For full symmetry also check for a NULL guest handle
in populate_physmap() (but note this makes no sense in
memory_exchange(), as there the array is also an input).

Signed-off-by: Jan Beulich 




Reviewed-by: Andrew Cooper 




Unfortunately I just had time to do testing with this change and I
have to report that introduces a critical regression for my tools.
With this change in-place performing increase_reservation on a target
domain no longer reports the guest frame number for external tools,
thus completely breaking advanced use-cases that require this
information to be able to do altp2m gfn remapping. This is a critical
step in being able to introduce shadow-pages that are used to hide
breakpoints and other memory modifications from the guest.




While I can see your point, I'm afraid that's not how the
interface was meant to be used. The mere fact that
populate-physmap and memory-exchange didn't return the
MFN(s) suggests to me that you already need to have a way
to deal with having to find out another way. Or are you
suggesting you rely on guests not using these interfaces?

As to a solution, I could possibly see us relax the change to
return the MFN(s) when the current and subject domains differ,
or even check paging mode of the caller domain instead of the
subject one (which would mean PVH Dom0 still wouldn't get to
see them). But if we do, imo we should do this consistently for
all three operations, rather than just for increase-reservation.


If at all possible, I would like to request this change not to be part
of the 4.9 release.




Hmm, it's been there for all of the RCs, so I'm not really happy
to consider the option of reverting at this point in time. But
Julien will have the final say anyway.




I am a bit confuse with the description of the problem. I understood
"guest
frame number" as GFN. But AFAICT, this hypercall was returning MFN even
for
HVM guests. So how this change is breaking altp2m remapping?



For HVM guests this hypercall returns a GFN that can subsequently be
populated into the guest physmap:

xc_domain_increase_reservation_exact(xch, domid, 1, 0, 0, _gfn);
xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, _gfn);



I am sorry, I can't see how this can return a GFN for the HVM. Looking at
the implementation of increase_reservation in Xen:

mfn = page_to_mfn(page);
if ( unlikely(__copy_to_guest_offset(a->extent_list, i, , 1)) )
  goto out;

This is an MFN and not a GFN. Except the strict check before, the code has
not change for a while.

AFAICT, the purpose of increase_reservation is not to allocate a new GFN, it
will just allocate the host memory for it. At least on ARM we have nothing
to say "this GFN region is free". I would be surprised that such things
exists on x86.



It returns memory that can be mapped into the guest physmap
subsequently. So I have been referring to it as a GFN that is not
mapped into the physmap - similar to the magic ring pages when they
are in use.


Reading the implementation, roughly:

* increase_reservation will only allocate host memory and return the 
corresponding MFN

* populate_physmap will allocate host memory and map to a specific address

So by calling both, you will effectively allocate twice memory and never 
be able to free the memory allocated by increase_reservation until the 
guest is destroyed. This will *never* allocate the corresponding GFN and 
I think is just working by luck in your case.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

On Mon, Jun 19, 2017 at 8:52 AM, Julien Grall  wrote:
>
>
> On 19/06/17 15:39, Tamas K Lengyel wrote:
>>
>> On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall 
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 19/06/17 09:15, Jan Beulich wrote:
>>>
>>>
>>> On 18.06.17 at 21:19,  wrote:
>
>
> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper
> 
> wrote:
>>
>>
>> On 04/04/17 14:14, Jan Beulich wrote:
>>>
>>>
>>> We shouldn't hand MFN info back from increase-reservation for
>>> translated domains, just like we don't for populate-physmap and
>>> memory-exchange. For full symmetry also check for a NULL guest handle
>>> in populate_physmap() (but note this makes no sense in
>>> memory_exchange(), as there the array is also an input).
>>>
>>> Signed-off-by: Jan Beulich 
>>
>>
>>
>> Reviewed-by: Andrew Cooper 
>
>
>
> Unfortunately I just had time to do testing with this change and I
> have to report that introduces a critical regression for my tools.
> With this change in-place performing increase_reservation on a target
> domain no longer reports the guest frame number for external tools,
> thus completely breaking advanced use-cases that require this
> information to be able to do altp2m gfn remapping. This is a critical
> step in being able to introduce shadow-pages that are used to hide
> breakpoints and other memory modifications from the guest.



 While I can see your point, I'm afraid that's not how the
 interface was meant to be used. The mere fact that
 populate-physmap and memory-exchange didn't return the
 MFN(s) suggests to me that you already need to have a way
 to deal with having to find out another way. Or are you
 suggesting you rely on guests not using these interfaces?

 As to a solution, I could possibly see us relax the change to
 return the MFN(s) when the current and subject domains differ,
 or even check paging mode of the caller domain instead of the
 subject one (which would mean PVH Dom0 still wouldn't get to
 see them). But if we do, imo we should do this consistently for
 all three operations, rather than just for increase-reservation.

> If at all possible, I would like to request this change not to be part
> of the 4.9 release.



 Hmm, it's been there for all of the RCs, so I'm not really happy
 to consider the option of reverting at this point in time. But
 Julien will have the final say anyway.
>>>
>>>
>>>
>>> I am a bit confuse with the description of the problem. I understood
>>> "guest
>>> frame number" as GFN. But AFAICT, this hypercall was returning MFN even
>>> for
>>> HVM guests. So how this change is breaking altp2m remapping?
>>
>>
>> For HVM guests this hypercall returns a GFN that can subsequently be
>> populated into the guest physmap:
>>
>> xc_domain_increase_reservation_exact(xch, domid, 1, 0, 0, _gfn);
>> xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, _gfn);
>
>
> I am sorry, I can't see how this can return a GFN for the HVM. Looking at
> the implementation of increase_reservation in Xen:
>
> mfn = page_to_mfn(page);
> if ( unlikely(__copy_to_guest_offset(a->extent_list, i, , 1)) )
>   goto out;
>
> This is an MFN and not a GFN. Except the strict check before, the code has
> not change for a while.
>
> AFAICT, the purpose of increase_reservation is not to allocate a new GFN, it
> will just allocate the host memory for it. At least on ARM we have nothing
> to say "this GFN region is free". I would be surprised that such things
> exists on x86.
>

It returns memory that can be mapped into the guest physmap
subsequently. So I have been referring to it as a GFN that is not
mapped into the physmap - similar to the magic ring pages when they
are in use.

Tamas

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

On Mon, Jun 19, 2017 at 8:54 AM, George Dunlap  wrote:
> On 19/06/17 15:48, Tamas K Lengyel wrote:
>> On Mon, Jun 19, 2017 at 3:11 AM, George Dunlap  
>> wrote:
>>> On 19/06/17 09:15, Jan Beulich wrote:
>>> On 18.06.17 at 21:19,  wrote:
> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper 
> wrote:
>> On 04/04/17 14:14, Jan Beulich wrote:
>>> We shouldn't hand MFN info back from increase-reservation for
>>> translated domains, just like we don't for populate-physmap and
>>> memory-exchange. For full symmetry also check for a NULL guest handle
>>> in populate_physmap() (but note this makes no sense in
>>> memory_exchange(), as there the array is also an input).
>>>
>>> Signed-off-by: Jan Beulich 
>>
>> Reviewed-by: Andrew Cooper 
>
> Unfortunately I just had time to do testing with this change and I
> have to report that introduces a critical regression for my tools.
> With this change in-place performing increase_reservation on a target
> domain no longer reports the guest frame number for external tools,
> thus completely breaking advanced use-cases that require this
> information to be able to do altp2m gfn remapping. This is a critical
> step in being able to introduce shadow-pages that are used to hide
> breakpoints and other memory modifications from the guest.

 While I can see your point, I'm afraid that's not how the
 interface was meant to be used.
>>>
>>> Well the first question to ask is, is that hypercall part of the stable
>>> interface?  If so, then the standard should be, "Don't break people who
>>> call it unless there is really no other way around it."  Sure, it was a
>>> mistake whoever introduced that, but if Tamas is building on a "stable"
>>> interface he should be able to rely on that interface being maintained,
>>> at least until we can find a suitable replacement.
>>>
>>>  -George
>>>
>>
>> Of course if a suitable replacement can be made that gets me the
>> information I need that would work too. At the moment I'm not aware of
>> any other hypercall I could use for this purpose.
>
> So actually -- it sounds like both Jan and I misunderstood the
> situation.  The header file clearly says:
>
>  * XENMEM_increase_reservation:
>  *   OUT: MFN (*not* GMFN) bases of extents that were allocated
>
> Are you saying that for HVM guests, that statement is false?
>

Well, it would certainly appear so as I  have been using it to add
memory to a guest and then map it into the guest physmap as a new gfn.
I've been using it like that since Xen 4.6 without any problems.

Tamas

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[George Dunlap]

On 19/06/17 15:48, Tamas K Lengyel wrote:
> On Mon, Jun 19, 2017 at 3:11 AM, George Dunlap  
> wrote:
>> On 19/06/17 09:15, Jan Beulich wrote:
>> On 18.06.17 at 21:19,  wrote:
 On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper 
 wrote:
> On 04/04/17 14:14, Jan Beulich wrote:
>> We shouldn't hand MFN info back from increase-reservation for
>> translated domains, just like we don't for populate-physmap and
>> memory-exchange. For full symmetry also check for a NULL guest handle
>> in populate_physmap() (but note this makes no sense in
>> memory_exchange(), as there the array is also an input).
>>
>> Signed-off-by: Jan Beulich 
>
> Reviewed-by: Andrew Cooper 

 Unfortunately I just had time to do testing with this change and I
 have to report that introduces a critical regression for my tools.
 With this change in-place performing increase_reservation on a target
 domain no longer reports the guest frame number for external tools,
 thus completely breaking advanced use-cases that require this
 information to be able to do altp2m gfn remapping. This is a critical
 step in being able to introduce shadow-pages that are used to hide
 breakpoints and other memory modifications from the guest.
>>>
>>> While I can see your point, I'm afraid that's not how the
>>> interface was meant to be used.
>>
>> Well the first question to ask is, is that hypercall part of the stable
>> interface?  If so, then the standard should be, "Don't break people who
>> call it unless there is really no other way around it."  Sure, it was a
>> mistake whoever introduced that, but if Tamas is building on a "stable"
>> interface he should be able to rely on that interface being maintained,
>> at least until we can find a suitable replacement.
>>
>>  -George
>>
> 
> Of course if a suitable replacement can be made that gets me the
> information I need that would work too. At the moment I'm not aware of
> any other hypercall I could use for this purpose.

So actually -- it sounds like both Jan and I misunderstood the
situation.  The header file clearly says:

 * XENMEM_increase_reservation:
 *   OUT: MFN (*not* GMFN) bases of extents that were allocated

Are you saying that for HVM guests, that statement is false?

 -George

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Julien Grall]

On 19/06/17 15:39, Tamas K Lengyel wrote:

On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall  wrote:

Hi,


On 19/06/17 09:15, Jan Beulich wrote:


On 18.06.17 at 21:19,  wrote:


On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper 
wrote:


On 04/04/17 14:14, Jan Beulich wrote:


We shouldn't hand MFN info back from increase-reservation for
translated domains, just like we don't for populate-physmap and
memory-exchange. For full symmetry also check for a NULL guest handle
in populate_physmap() (but note this makes no sense in
memory_exchange(), as there the array is also an input).

Signed-off-by: Jan Beulich 



Reviewed-by: Andrew Cooper 



Unfortunately I just had time to do testing with this change and I
have to report that introduces a critical regression for my tools.
With this change in-place performing increase_reservation on a target
domain no longer reports the guest frame number for external tools,
thus completely breaking advanced use-cases that require this
information to be able to do altp2m gfn remapping. This is a critical
step in being able to introduce shadow-pages that are used to hide
breakpoints and other memory modifications from the guest.



While I can see your point, I'm afraid that's not how the
interface was meant to be used. The mere fact that
populate-physmap and memory-exchange didn't return the
MFN(s) suggests to me that you already need to have a way
to deal with having to find out another way. Or are you
suggesting you rely on guests not using these interfaces?

As to a solution, I could possibly see us relax the change to
return the MFN(s) when the current and subject domains differ,
or even check paging mode of the caller domain instead of the
subject one (which would mean PVH Dom0 still wouldn't get to
see them). But if we do, imo we should do this consistently for
all three operations, rather than just for increase-reservation.


If at all possible, I would like to request this change not to be part
of the 4.9 release.



Hmm, it's been there for all of the RCs, so I'm not really happy
to consider the option of reverting at this point in time. But
Julien will have the final say anyway.



I am a bit confuse with the description of the problem. I understood "guest
frame number" as GFN. But AFAICT, this hypercall was returning MFN even for
HVM guests. So how this change is breaking altp2m remapping?


For HVM guests this hypercall returns a GFN that can subsequently be
populated into the guest physmap:

xc_domain_increase_reservation_exact(xch, domid, 1, 0, 0, _gfn);
xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, _gfn);


I am sorry, I can't see how this can return a GFN for the HVM. Looking 
at the implementation of increase_reservation in Xen:


mfn = page_to_mfn(page);
if ( unlikely(__copy_to_guest_offset(a->extent_list, i, , 1)) )
  goto out;

This is an MFN and not a GFN. Except the strict check before, the code 
has not change for a while.


AFAICT, the purpose of increase_reservation is not to allocate a new 
GFN, it will just allocate the host memory for it. At least on ARM we 
have nothing to say "this GFN region is free". I would be surprised that 
such things exists on x86.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

On Mon, Jun 19, 2017 at 3:11 AM, George Dunlap  wrote:
> On 19/06/17 09:15, Jan Beulich wrote:
> On 18.06.17 at 21:19,  wrote:
>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper 
>>> wrote:
 On 04/04/17 14:14, Jan Beulich wrote:
> We shouldn't hand MFN info back from increase-reservation for
> translated domains, just like we don't for populate-physmap and
> memory-exchange. For full symmetry also check for a NULL guest handle
> in populate_physmap() (but note this makes no sense in
> memory_exchange(), as there the array is also an input).
>
> Signed-off-by: Jan Beulich 

 Reviewed-by: Andrew Cooper 
>>>
>>> Unfortunately I just had time to do testing with this change and I
>>> have to report that introduces a critical regression for my tools.
>>> With this change in-place performing increase_reservation on a target
>>> domain no longer reports the guest frame number for external tools,
>>> thus completely breaking advanced use-cases that require this
>>> information to be able to do altp2m gfn remapping. This is a critical
>>> step in being able to introduce shadow-pages that are used to hide
>>> breakpoints and other memory modifications from the guest.
>>
>> While I can see your point, I'm afraid that's not how the
>> interface was meant to be used.
>
> Well the first question to ask is, is that hypercall part of the stable
> interface?  If so, then the standard should be, "Don't break people who
> call it unless there is really no other way around it."  Sure, it was a
> mistake whoever introduced that, but if Tamas is building on a "stable"
> interface he should be able to rely on that interface being maintained,
> at least until we can find a suitable replacement.
>
>  -George
>

Of course if a suitable replacement can be made that gets me the
information I need that would work too. At the moment I'm not aware of
any other hypercall I could use for this purpose.

Tamas

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall  wrote:
> Hi,
>
>
> On 19/06/17 09:15, Jan Beulich wrote:
>
> On 18.06.17 at 21:19,  wrote:
>>>
>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper 
>>> wrote:

 On 04/04/17 14:14, Jan Beulich wrote:
>
> We shouldn't hand MFN info back from increase-reservation for
> translated domains, just like we don't for populate-physmap and
> memory-exchange. For full symmetry also check for a NULL guest handle
> in populate_physmap() (but note this makes no sense in
> memory_exchange(), as there the array is also an input).
>
> Signed-off-by: Jan Beulich 


 Reviewed-by: Andrew Cooper 
>>>
>>>
>>> Unfortunately I just had time to do testing with this change and I
>>> have to report that introduces a critical regression for my tools.
>>> With this change in-place performing increase_reservation on a target
>>> domain no longer reports the guest frame number for external tools,
>>> thus completely breaking advanced use-cases that require this
>>> information to be able to do altp2m gfn remapping. This is a critical
>>> step in being able to introduce shadow-pages that are used to hide
>>> breakpoints and other memory modifications from the guest.
>>
>>
>> While I can see your point, I'm afraid that's not how the
>> interface was meant to be used. The mere fact that
>> populate-physmap and memory-exchange didn't return the
>> MFN(s) suggests to me that you already need to have a way
>> to deal with having to find out another way. Or are you
>> suggesting you rely on guests not using these interfaces?
>>
>> As to a solution, I could possibly see us relax the change to
>> return the MFN(s) when the current and subject domains differ,
>> or even check paging mode of the caller domain instead of the
>> subject one (which would mean PVH Dom0 still wouldn't get to
>> see them). But if we do, imo we should do this consistently for
>> all three operations, rather than just for increase-reservation.
>>
>>> If at all possible, I would like to request this change not to be part
>>> of the 4.9 release.
>>
>>
>> Hmm, it's been there for all of the RCs, so I'm not really happy
>> to consider the option of reverting at this point in time. But
>> Julien will have the final say anyway.
>
>
> I am a bit confuse with the description of the problem. I understood "guest
> frame number" as GFN. But AFAICT, this hypercall was returning MFN even for
> HVM guests. So how this change is breaking altp2m remapping?

For HVM guests this hypercall returns a GFN that can subsequently be
populated into the guest physmap:

xc_domain_increase_reservation_exact(xch, domid, 1, 0, 0, _gfn);
xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, _gfn);

...
Copy page contents from old_gfn to new_gfn and inject breakpoints,
make other memory modifications
...

xc_altp2m_change_gfn(xch, domid, altp2m_id, old_gfn, new_gfn);

Without being able to introduce a new gfn into the HVM guest's
physmap, we are unable to create a shadow page. It doesn't break
altp2m remapping itself, it breaks a per-requisite step in introducing
the page to remap to.

Tamas

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Jan Beulich]

>>> On 19.06.17 at 11:11,  wrote:
> On 19/06/17 09:15, Jan Beulich wrote:
> On 18.06.17 at 21:19,  wrote:
>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper  
>>> wrote:
 On 04/04/17 14:14, Jan Beulich wrote:
> We shouldn't hand MFN info back from increase-reservation for
> translated domains, just like we don't for populate-physmap and
> memory-exchange. For full symmetry also check for a NULL guest handle
> in populate_physmap() (but note this makes no sense in
> memory_exchange(), as there the array is also an input).
>
> Signed-off-by: Jan Beulich 

 Reviewed-by: Andrew Cooper 
>>>
>>> Unfortunately I just had time to do testing with this change and I
>>> have to report that introduces a critical regression for my tools.
>>> With this change in-place performing increase_reservation on a target
>>> domain no longer reports the guest frame number for external tools,
>>> thus completely breaking advanced use-cases that require this
>>> information to be able to do altp2m gfn remapping. This is a critical
>>> step in being able to introduce shadow-pages that are used to hide
>>> breakpoints and other memory modifications from the guest.
>> 
>> While I can see your point, I'm afraid that's not how the
>> interface was meant to be used. 
> 
> Well the first question to ask is, is that hypercall part of the stable
> interface?  If so, then the standard should be, "Don't break people who
> call it unless there is really no other way around it."  Sure, it was a
> mistake whoever introduced that, but if Tamas is building on a "stable"
> interface he should be able to rely on that interface being maintained,
> at least until we can find a suitable replacement.

Tool stack use of interfaces has never really been considered
stable, i.e. the interfaces here are "stable" for a domain to use
on itself, but fall in the same group as tool-stack only interfaces
when using them on a foreign domain. At least that's the way
I view it.

Jan


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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[George Dunlap]

On 19/06/17 09:15, Jan Beulich wrote:
 On 18.06.17 at 21:19,  wrote:
>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper  
>> wrote:
>>> On 04/04/17 14:14, Jan Beulich wrote:
 We shouldn't hand MFN info back from increase-reservation for
 translated domains, just like we don't for populate-physmap and
 memory-exchange. For full symmetry also check for a NULL guest handle
 in populate_physmap() (but note this makes no sense in
 memory_exchange(), as there the array is also an input).

 Signed-off-by: Jan Beulich 
>>>
>>> Reviewed-by: Andrew Cooper 
>>
>> Unfortunately I just had time to do testing with this change and I
>> have to report that introduces a critical regression for my tools.
>> With this change in-place performing increase_reservation on a target
>> domain no longer reports the guest frame number for external tools,
>> thus completely breaking advanced use-cases that require this
>> information to be able to do altp2m gfn remapping. This is a critical
>> step in being able to introduce shadow-pages that are used to hide
>> breakpoints and other memory modifications from the guest.
> 
> While I can see your point, I'm afraid that's not how the
> interface was meant to be used. 

Well the first question to ask is, is that hypercall part of the stable
interface?  If so, then the standard should be, "Don't break people who
call it unless there is really no other way around it."  Sure, it was a
mistake whoever introduced that, but if Tamas is building on a "stable"
interface he should be able to rely on that interface being maintained,
at least until we can find a suitable replacement.

 -George


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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Julien Grall]

Hi,

On 19/06/17 09:15, Jan Beulich wrote:

On 18.06.17 at 21:19,  wrote:

On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper 
wrote:

On 04/04/17 14:14, Jan Beulich wrote:

We shouldn't hand MFN info back from increase-reservation for
translated domains, just like we don't for populate-physmap and
memory-exchange. For full symmetry also check for a NULL guest handle
in populate_physmap() (but note this makes no sense in
memory_exchange(), as there the array is also an input).

Signed-off-by: Jan Beulich 


Reviewed-by: Andrew Cooper 


Unfortunately I just had time to do testing with this change and I
have to report that introduces a critical regression for my tools.
With this change in-place performing increase_reservation on a target
domain no longer reports the guest frame number for external tools,
thus completely breaking advanced use-cases that require this
information to be able to do altp2m gfn remapping. This is a critical
step in being able to introduce shadow-pages that are used to hide
breakpoints and other memory modifications from the guest.


While I can see your point, I'm afraid that's not how the
interface was meant to be used. The mere fact that
populate-physmap and memory-exchange didn't return the
MFN(s) suggests to me that you already need to have a way
to deal with having to find out another way. Or are you
suggesting you rely on guests not using these interfaces?

As to a solution, I could possibly see us relax the change to
return the MFN(s) when the current and subject domains differ,
or even check paging mode of the caller domain instead of the
subject one (which would mean PVH Dom0 still wouldn't get to
see them). But if we do, imo we should do this consistently for
all three operations, rather than just for increase-reservation.


If at all possible, I would like to request this change not to be part
of the 4.9 release.


Hmm, it's been there for all of the RCs, so I'm not really happy
to consider the option of reverting at this point in time. But
Julien will have the final say anyway.


I am a bit confuse with the description of the problem. I understood 
"guest frame number" as GFN. But AFAICT, this hypercall was returning 
MFN even for HVM guests. So how this change is breaking altp2m remapping?


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Jan Beulich]

>>> On 18.06.17 at 21:19,  wrote:
> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper  
> wrote:
>> On 04/04/17 14:14, Jan Beulich wrote:
>>> We shouldn't hand MFN info back from increase-reservation for
>>> translated domains, just like we don't for populate-physmap and
>>> memory-exchange. For full symmetry also check for a NULL guest handle
>>> in populate_physmap() (but note this makes no sense in
>>> memory_exchange(), as there the array is also an input).
>>>
>>> Signed-off-by: Jan Beulich 
>>
>> Reviewed-by: Andrew Cooper 
> 
> Unfortunately I just had time to do testing with this change and I
> have to report that introduces a critical regression for my tools.
> With this change in-place performing increase_reservation on a target
> domain no longer reports the guest frame number for external tools,
> thus completely breaking advanced use-cases that require this
> information to be able to do altp2m gfn remapping. This is a critical
> step in being able to introduce shadow-pages that are used to hide
> breakpoints and other memory modifications from the guest.

While I can see your point, I'm afraid that's not how the
interface was meant to be used. The mere fact that
populate-physmap and memory-exchange didn't return the
MFN(s) suggests to me that you already need to have a way
to deal with having to find out another way. Or are you
suggesting you rely on guests not using these interfaces?

As to a solution, I could possibly see us relax the change to
return the MFN(s) when the current and subject domains differ,
or even check paging mode of the caller domain instead of the
subject one (which would mean PVH Dom0 still wouldn't get to
see them). But if we do, imo we should do this consistently for
all three operations, rather than just for increase-reservation.

> If at all possible, I would like to request this change not to be part
> of the 4.9 release.

Hmm, it's been there for all of the RCs, so I'm not really happy
to consider the option of reverting at this point in time. But
Julien will have the final say anyway.

Jan


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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper  wrote:
> On 04/04/17 14:14, Jan Beulich wrote:
>> We shouldn't hand MFN info back from increase-reservation for
>> translated domains, just like we don't for populate-physmap and
>> memory-exchange. For full symmetry also check for a NULL guest handle
>> in populate_physmap() (but note this makes no sense in
>> memory_exchange(), as there the array is also an input).
>>
>> Signed-off-by: Jan Beulich 
>
> Reviewed-by: Andrew Cooper 

Unfortunately I just had time to do testing with this change and I
have to report that introduces a critical regression for my tools.
With this change in-place performing increase_reservation on a target
domain no longer reports the guest frame number for external tools,
thus completely breaking advanced use-cases that require this
information to be able to do altp2m gfn remapping. This is a critical
step in being able to introduce shadow-pages that are used to hide
breakpoints and other memory modifications from the guest.

If at all possible, I would like to request this change not to be part
of the 4.9 release.

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Andrew Cooper]

On 04/04/17 14:14, Jan Beulich wrote:
> We shouldn't hand MFN info back from increase-reservation for
> translated domains, just like we don't for populate-physmap and
> memory-exchange. For full symmetry also check for a NULL guest handle
> in populate_physmap() (but note this makes no sense in
> memory_exchange(), as there the array is also an input).
>
> Signed-off-by: Jan Beulich 

Reviewed-by: Andrew Cooper 

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