[PATCH 0/2] memory: XSA-212 follow-up

[PATCH 0/2] memory: XSA-212 follow-up

[Jan Beulich]

1: exit early from memory_exchange() upon write-back error
2: don't hand MFN info to translated guests

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I realize it's past last posting date, but I couldn't possibly post these
earlier, due to their connection with XSA-212.


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

[PATCH 1/2] memory: exit early from memory_exchange() upon write-back error

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

There's no point in continuing if in the end we'll return -EFAULT
anyway. It also seems wrong to report a chunk for which at least one
write-back failed as successfully exchanged (albeit the indication of
an error is also not fully correct, as the exchange happened in that
case at least partially - retrieving the GFN to assign the memory to
and/or handing back the information on the replacement memory didn't
work). In any case limiting the amount of damage done to the guest
can't be all that bad an idea.

Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm additionally surprised we don't require input GFNs to be order
aligned for both IN- and OUT-chunks (similarly for populate-physmap
and decrease-reservation).

--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -639,6 +639,9 @@ static long memory_exchange(XEN_GUEST_HA
             }
         }
         BUG_ON( !(d->is_dying) && (j != (1UL << out_chunk_order)) );
+
+        if ( rc )
+            goto fail;
     }
 
     exch.nr_exchanged = exch.in.nr_extents;




[-- Attachment #2: memory-exchange-write-fail.patch --]
[-- Type: text/plain, Size: 1170 bytes --]

memory: exit early from memory_exchange() upon write-back error

There's no point in continuing if in the end we'll return -EFAULT
anyway. It also seems wrong to report a chunk for which at least one
write-back failed as successfully exchanged (albeit the indication of
an error is also not fully correct, as the exchange happened in that
case at least partially - retrieving the GFN to assign the memory to
and/or handing back the information on the replacement memory didn't
work). In any case limiting the amount of damage done to the guest
can't be all that bad an idea.

Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm additionally surprised we don't require input GFNs to be order
aligned for both IN- and OUT-chunks (similarly for populate-physmap
and decrease-reservation).

--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -639,6 +639,9 @@ static long memory_exchange(XEN_GUEST_HA
             }
         }
         BUG_ON( !(d->is_dying) && (j != (1UL << out_chunk_order)) );
+
+        if ( rc )
+            goto fail;
     }
 
     exch.nr_exchanged = exch.in.nr_extents;

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]

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 <jbeulich@suse.com>

--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -122,7 +122,8 @@ static void increase_reservation(struct
         }
 
         /* Inform the domain of the new page's machine address. */ 
-        if ( !guest_handle_is_null(a->extent_list) )
+        if ( !paging_mode_translate(d) &&
+             !guest_handle_is_null(a->extent_list) )
         {
             mfn = page_to_mfn(page);
             if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn, 1)) )
@@ -238,7 +239,8 @@ static void populate_physmap(struct memo
 
             guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), a->extent_order);
 
-            if ( !paging_mode_translate(d) )
+            if ( !paging_mode_translate(d) &&
+                 !guest_handle_is_null(a->extent_list) )
             {
                 for ( j = 0; j < (1U << a->extent_order); j++ )
                     set_gpfn_from_mfn(mfn + j, gpfn + j);




[-- Attachment #2: memory-increase-trans-no-wb.patch --]
[-- Type: text/plain, Size: 1349 bytes --]

memory: don't hand MFN info to translated guests

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 <jbeulich@suse.com>

--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -122,7 +122,8 @@ static void increase_reservation(struct
         }
 
         /* Inform the domain of the new page's machine address. */ 
-        if ( !guest_handle_is_null(a->extent_list) )
+        if ( !paging_mode_translate(d) &&
+             !guest_handle_is_null(a->extent_list) )
         {
             mfn = page_to_mfn(page);
             if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn, 1)) )
@@ -238,7 +239,8 @@ static void populate_physmap(struct memo
 
             guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), a->extent_order);
 
-            if ( !paging_mode_translate(d) )
+            if ( !paging_mode_translate(d) &&
+                 !guest_handle_is_null(a->extent_list) )
             {
                 for ( j = 0; j < (1U << a->extent_order); j++ )
                     set_gpfn_from_mfn(mfn + j, gpfn + j);

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/2] memory: exit early from memory_exchange() upon write-back error

[Andrew Cooper]

On 04/04/17 14:13, Jan Beulich wrote:
> There's no point in continuing if in the end we'll return -EFAULT
> anyway. It also seems wrong to report a chunk for which at least one
> write-back failed as successfully exchanged (albeit the indication of
> an error is also not fully correct, as the exchange happened in that
> case at least partially - retrieving the GFN to assign the memory to
> and/or handing back the information on the replacement memory didn't
> work). In any case limiting the amount of damage done to the guest
> can't be all that bad an idea.
>
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> I'm additionally surprised we don't require input GFNs to be order
> aligned for both IN- and OUT-chunks (similarly for populate-physmap
> and decrease-reservation).

This sounds like a bug, rather than being intentional.

>
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c

As an observation, I find it amusing that there is a comment just above
this which states

/*
 * Success! Beyond this point we cannot fail for this chunk.
 */

> @@ -639,6 +639,9 @@ static long memory_exchange(XEN_GUEST_HA
>              }
>          }
>          BUG_ON( !(d->is_dying) && (j != (1UL << out_chunk_order)) );
> +
> +        if ( rc )
> +            goto fail;
>      }
>  
>      exch.nr_exchanged = exch.in.nr_extents;
>
>
>


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

Re: [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 <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 1/2] memory: exit early from memory_exchange() upon write-back error

[Jan Beulich]

>>> On 04.04.17 at 20:45, <andrew.cooper3@citrix.com> wrote:
> On 04/04/17 14:13, Jan Beulich wrote:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
> 
> As an observation, I find it amusing that there is a comment just above
> this which states
> 
> /*
>  * Success! Beyond this point we cannot fail for this chunk.
>  */

I don't understand the "amusing" part: It's a point of no return,
hence there mustn't be any immediate exit from any of the
following processing upon encountering some kind of error. It's
just that ignoring errors went a little too far here.

Jan

>> @@ -639,6 +639,9 @@ static long memory_exchange(XEN_GUEST_HA
>>              }
>>          }
>>          BUG_ON( !(d->is_dying) && (j != (1UL << out_chunk_order)) );
>> +
>> +        if ( rc )
>> +            goto fail;
>>      }
>>  
>>      exch.nr_exchanged = exch.in.nr_extents;
>>
>>
>>




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

Re: [PATCH 1/2] memory: exit early from memory_exchange() upon write-back error

[Jan Beulich]

>>> On 04.04.17 at 20:45, <andrew.cooper3@citrix.com> wrote:
> On 04/04/17 14:13, Jan Beulich wrote:
>> I'm additionally surprised we don't require input GFNs to be order
>> aligned for both IN- and OUT-chunks (similarly for populate-physmap
>> and decrease-reservation).
> 
> This sounds like a bug, rather than being intentional.

Problem being that we can't be sure guests aren't depending on this
current behavior, or else I would have added a 3rd patch to this
series right away.

Jan


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

Re: [PATCH 0/2] memory: XSA-212 follow-up

[Julien Grall]

Hi Jan,

On 04/04/17 13:53, Jan Beulich wrote:
> 1: exit early from memory_exchange() upon write-back error
> 2: don't hand MFN info to translated guests
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I realize it's past last posting date, but I couldn't possibly post these
> earlier, due to their connection with XSA-212.

Released-acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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

Re: [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 <andrew.cooper3@citrix.com> 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 <jbeulich@suse.com>
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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: [PATCH 2/2] memory: don't hand MFN info to translated guests

[Jan Beulich]

>>> On 18.06.17 at 21:19, <tamas.k.lengyel@gmail.com> wrote:
> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper <andrew.cooper3@citrix.com> 
> 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 <jbeulich@suse.com>
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> 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: [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, <tamas.k.lengyel@gmail.com> wrote:
>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper <andrew.cooper3@citrix.com>
>> 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 <jbeulich@suse.com>
>>>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> 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: [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, <tamas.k.lengyel@gmail.com> wrote:
>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper <andrew.cooper3@citrix.com> 
>> 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 <jbeulich@suse.com>
>>>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> 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: [PATCH 2/2] memory: don't hand MFN info to translated guests

[Jan Beulich]

>>> On 19.06.17 at 11:11, <george.dunlap@citrix.com> wrote:
> On 19/06/17 09:15, Jan Beulich wrote:
>>>>> On 18.06.17 at 21:19, <tamas.k.lengyel@gmail.com> wrote:
>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper <andrew.cooper3@citrix.com> 
>>> 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 <jbeulich@suse.com>
>>>>
>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>
>>> 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: [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 <julien.grall@arm.com> wrote:
> Hi,
>
>
> On 19/06/17 09:15, Jan Beulich wrote:
>>>>>
>>>>> On 18.06.17 at 21:19, <tamas.k.lengyel@gmail.com> wrote:
>>>
>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper <andrew.cooper3@citrix.com>
>>> 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 <jbeulich@suse.com>
>>>>
>>>>
>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>
>>>
>>> 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, &new_gfn);
xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, &new_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: [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 <george.dunlap@citrix.com> wrote:
> On 19/06/17 09:15, Jan Beulich wrote:
>>>>> On 18.06.17 at 21:19, <tamas.k.lengyel@gmail.com> wrote:
>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper <andrew.cooper3@citrix.com>
>>> 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 <jbeulich@suse.com>
>>>>
>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>
>>> 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: [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 <julien.grall@arm.com> wrote:
>> Hi,
>>
>>
>> On 19/06/17 09:15, Jan Beulich wrote:
>>>>>>
>>>>>> On 18.06.17 at 21:19, <tamas.k.lengyel@gmail.com> wrote:
>>>>
>>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper <andrew.cooper3@citrix.com>
>>>> 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 <jbeulich@suse.com>
>>>>>
>>>>>
>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>
>>>>
>>>> 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, &new_gfn);
> xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, &new_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, &mfn, 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: [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 <george.dunlap@citrix.com> wrote:
>> On 19/06/17 09:15, Jan Beulich wrote:
>>>>>> On 18.06.17 at 21:19, <tamas.k.lengyel@gmail.com> wrote:
>>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper <andrew.cooper3@citrix.com>
>>>> 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 <jbeulich@suse.com>
>>>>>
>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>
>>>> 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: [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 <george.dunlap@citrix.com> wrote:
> On 19/06/17 15:48, Tamas K Lengyel wrote:
>> On Mon, Jun 19, 2017 at 3:11 AM, George Dunlap <george.dunlap@citrix.com> wrote:
>>> On 19/06/17 09:15, Jan Beulich wrote:
>>>>>>> On 18.06.17 at 21:19, <tamas.k.lengyel@gmail.com> wrote:
>>>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> 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 <jbeulich@suse.com>
>>>>>>
>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>
>>>>> 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: [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 <julien.grall@arm.com> wrote:
>
>
> On 19/06/17 15:39, Tamas K Lengyel wrote:
>>
>> On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 19/06/17 09:15, Jan Beulich wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 18.06.17 at 21:19, <tamas.k.lengyel@gmail.com> wrote:
>>>>>
>>>>>
>>>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper
>>>>> <andrew.cooper3@citrix.com>
>>>>> 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 <jbeulich@suse.com>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>
>>>>>
>>>>>
>>>>> 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, &new_gfn);
>> xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, &new_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, &mfn, 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: [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 <julien.grall@arm.com> wrote:
>>
>>
>> On 19/06/17 15:39, Tamas K Lengyel wrote:
>>>
>>> On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall <julien.grall@arm.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>>
>>>> On 19/06/17 09:15, Jan Beulich wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 18.06.17 at 21:19, <tamas.k.lengyel@gmail.com> wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper
>>>>>> <andrew.cooper3@citrix.com>
>>>>>> 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 <jbeulich@suse.com>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>
>>>>>>
>>>>>>
>>>>>> 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, &new_gfn);
>>> xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, &new_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, &mfn, 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: [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 <julien.grall@arm.com> wrote:
>
>
> On 19/06/17 15:57, Tamas K Lengyel wrote:
>>
>> On Mon, Jun 19, 2017 at 8:52 AM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>>
>>>
>>> On 19/06/17 15:39, Tamas K Lengyel wrote:
>>>>
>>>>
>>>> On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall <julien.grall@arm.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 19/06/17 09:15, Jan Beulich wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 18.06.17 at 21:19, <tamas.k.lengyel@gmail.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper
>>>>>>> <andrew.cooper3@citrix.com>
>>>>>>> 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 <jbeulich@suse.com>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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, &new_gfn);
>>>> xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, &new_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, &mfn, 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.

Ough, yes, you are correct.

After digging into the implementation of populate_physmap more closely
it indeed seems like it was pure luck that my use of it was working
properly. My understanding was the memory allocated by
increase_reservation will be used as a GFN in the guest. This appears
not to be so, it just returns a newly allocated MFN. When called with
populate_physmap that MFN was treated as a GFN to be mapped into the
guest and as you say, another MFN was getting allocated for it. So the
lucky part has been that the MFN returned by increase_reservation has
always been higher then the maximum GFN used by the guests. I had been
freeing the MFN that was returned via increase_reservation by calling
decrease_reservation. However, the page allocated during
populate_physmap is only freed during domain shutdown.

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?

Thanks,
Tamas

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

Re: [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