* [PATCH 0/2] memory: XSA-212 follow-up @ 2017-04-04 12:53 Jan Beulich 2017-04-04 13:13 ` [PATCH 1/2] memory: exit early from memory_exchange() upon write-back error Jan Beulich ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Jan Beulich @ 2017-04-04 12:53 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] memory: exit early from memory_exchange() upon write-back error 2017-04-04 12:53 [PATCH 0/2] memory: XSA-212 follow-up Jan Beulich @ 2017-04-04 13:13 ` Jan Beulich 2017-04-04 18:45 ` Andrew Cooper 2017-04-04 13:14 ` [PATCH 2/2] memory: don't hand MFN info to translated guests Jan Beulich 2017-04-05 13:12 ` [PATCH 0/2] memory: XSA-212 follow-up Julien Grall 2 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2017-04-04 13:13 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, Jann Horn, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall [-- 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] memory: exit early from memory_exchange() upon write-back error 2017-04-04 13:13 ` [PATCH 1/2] memory: exit early from memory_exchange() upon write-back error Jan Beulich @ 2017-04-04 18:45 ` Andrew Cooper 2017-04-05 6:58 ` Jan Beulich 2017-04-05 7:00 ` Jan Beulich 0 siblings, 2 replies; 22+ messages in thread From: Andrew Cooper @ 2017-04-04 18:45 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Stefano Stabellini, Wei Liu, Jann Horn, George Dunlap, Tim Deegan, Ian Jackson, Julien Grall 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] memory: exit early from memory_exchange() upon write-back error 2017-04-04 18:45 ` Andrew Cooper @ 2017-04-05 6:58 ` Jan Beulich 2017-04-05 7:00 ` Jan Beulich 1 sibling, 0 replies; 22+ messages in thread From: Jan Beulich @ 2017-04-05 6:58 UTC (permalink / raw) To: Andrew Cooper Cc: Stefano Stabellini, Wei Liu, Jann Horn, GeorgeDunlap, Tim Deegan, Ian Jackson, Julien Grall, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] memory: exit early from memory_exchange() upon write-back error 2017-04-04 18:45 ` Andrew Cooper 2017-04-05 6:58 ` Jan Beulich @ 2017-04-05 7:00 ` Jan Beulich 1 sibling, 0 replies; 22+ messages in thread From: Jan Beulich @ 2017-04-05 7:00 UTC (permalink / raw) To: Andrew Cooper Cc: Stefano Stabellini, Wei Liu, Jann Horn, GeorgeDunlap, Tim Deegan, Ian Jackson, Julien Grall, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-04-04 12:53 [PATCH 0/2] memory: XSA-212 follow-up Jan Beulich 2017-04-04 13:13 ` [PATCH 1/2] memory: exit early from memory_exchange() upon write-back error Jan Beulich @ 2017-04-04 13:14 ` Jan Beulich 2017-04-04 19:04 ` Andrew Cooper 2017-04-05 13:12 ` [PATCH 0/2] memory: XSA-212 follow-up Julien Grall 2 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2017-04-04 13:14 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall [-- 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-04-04 13:14 ` [PATCH 2/2] memory: don't hand MFN info to translated guests Jan Beulich @ 2017-04-04 19:04 ` Andrew Cooper 2017-06-18 19:19 ` Tamas K Lengyel 0 siblings, 1 reply; 22+ messages in thread From: Andrew Cooper @ 2017-04-04 19:04 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan, Ian Jackson, Julien Grall 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-04-04 19:04 ` Andrew Cooper @ 2017-06-18 19:19 ` Tamas K Lengyel 2017-06-19 8:15 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: Tamas K Lengyel @ 2017-06-18 19:19 UTC (permalink / raw) To: Andrew Cooper, Julien Grall, Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan, Ian Jackson, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-18 19:19 ` Tamas K Lengyel @ 2017-06-19 8:15 ` Jan Beulich 2017-06-19 9:09 ` Julien Grall 2017-06-19 9:11 ` George Dunlap 0 siblings, 2 replies; 22+ messages in thread From: Jan Beulich @ 2017-06-19 8:15 UTC (permalink / raw) To: Tamas K Lengyel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-19 8:15 ` Jan Beulich @ 2017-06-19 9:09 ` Julien Grall 2017-06-19 14:39 ` Tamas K Lengyel 2017-06-19 9:11 ` George Dunlap 1 sibling, 1 reply; 22+ messages in thread From: Julien Grall @ 2017-06-19 9:09 UTC (permalink / raw) To: Jan Beulich, Tamas K Lengyel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-19 9:09 ` Julien Grall @ 2017-06-19 14:39 ` Tamas K Lengyel 2017-06-19 14:52 ` Julien Grall 0 siblings, 1 reply; 22+ messages in thread From: Tamas K Lengyel @ 2017-06-19 14:39 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-19 14:39 ` Tamas K Lengyel @ 2017-06-19 14:52 ` Julien Grall 2017-06-19 14:57 ` Tamas K Lengyel 0 siblings, 1 reply; 22+ messages in thread From: Julien Grall @ 2017-06-19 14:52 UTC (permalink / raw) To: Tamas K Lengyel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-19 14:52 ` Julien Grall @ 2017-06-19 14:57 ` Tamas K Lengyel 2017-06-19 15:34 ` Julien Grall 0 siblings, 1 reply; 22+ messages in thread From: Tamas K Lengyel @ 2017-06-19 14:57 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-19 14:57 ` Tamas K Lengyel @ 2017-06-19 15:34 ` Julien Grall 2017-06-19 16:38 ` Tamas K Lengyel 0 siblings, 1 reply; 22+ messages in thread From: Julien Grall @ 2017-06-19 15:34 UTC (permalink / raw) To: Tamas K Lengyel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-19 15:34 ` Julien Grall @ 2017-06-19 16:38 ` Tamas K Lengyel 2017-06-19 16:57 ` Tamas K Lengyel 0 siblings, 1 reply; 22+ messages in thread From: Tamas K Lengyel @ 2017-06-19 16:38 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-19 16:38 ` Tamas K Lengyel @ 2017-06-19 16:57 ` Tamas K Lengyel 0 siblings, 0 replies; 22+ messages in thread From: Tamas K Lengyel @ 2017-06-19 16:57 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-19 8:15 ` Jan Beulich 2017-06-19 9:09 ` Julien Grall @ 2017-06-19 9:11 ` George Dunlap 2017-06-19 10:52 ` Jan Beulich 2017-06-19 14:48 ` Tamas K Lengyel 1 sibling, 2 replies; 22+ messages in thread From: George Dunlap @ 2017-06-19 9:11 UTC (permalink / raw) To: Jan Beulich, Tamas K Lengyel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-19 9:11 ` George Dunlap @ 2017-06-19 10:52 ` Jan Beulich 2017-06-19 14:48 ` Tamas K Lengyel 1 sibling, 0 replies; 22+ messages in thread From: Jan Beulich @ 2017-06-19 10:52 UTC (permalink / raw) To: George Dunlap Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-19 9:11 ` George Dunlap 2017-06-19 10:52 ` Jan Beulich @ 2017-06-19 14:48 ` Tamas K Lengyel 2017-06-19 14:54 ` George Dunlap 1 sibling, 1 reply; 22+ messages in thread From: Tamas K Lengyel @ 2017-06-19 14:48 UTC (permalink / raw) To: George Dunlap Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-19 14:48 ` Tamas K Lengyel @ 2017-06-19 14:54 ` George Dunlap 2017-06-19 14:56 ` Tamas K Lengyel 0 siblings, 1 reply; 22+ messages in thread From: George Dunlap @ 2017-06-19 14:54 UTC (permalink / raw) To: Tamas K Lengyel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] memory: don't hand MFN info to translated guests 2017-06-19 14:54 ` George Dunlap @ 2017-06-19 14:56 ` Tamas K Lengyel 0 siblings, 0 replies; 22+ messages in thread From: Tamas K Lengyel @ 2017-06-19 14:56 UTC (permalink / raw) To: George Dunlap Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] memory: XSA-212 follow-up 2017-04-04 12:53 [PATCH 0/2] memory: XSA-212 follow-up Jan Beulich 2017-04-04 13:13 ` [PATCH 1/2] memory: exit early from memory_exchange() upon write-back error Jan Beulich 2017-04-04 13:14 ` [PATCH 2/2] memory: don't hand MFN info to translated guests Jan Beulich @ 2017-04-05 13:12 ` Julien Grall 2 siblings, 0 replies; 22+ messages in thread From: Julien Grall @ 2017-04-05 13:12 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-06-19 16:58 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-04 12:53 [PATCH 0/2] memory: XSA-212 follow-up Jan Beulich 2017-04-04 13:13 ` [PATCH 1/2] memory: exit early from memory_exchange() upon write-back error Jan Beulich 2017-04-04 18:45 ` Andrew Cooper 2017-04-05 6:58 ` Jan Beulich 2017-04-05 7:00 ` Jan Beulich 2017-04-04 13:14 ` [PATCH 2/2] memory: don't hand MFN info to translated guests Jan Beulich 2017-04-04 19:04 ` Andrew Cooper 2017-06-18 19:19 ` Tamas K Lengyel 2017-06-19 8:15 ` Jan Beulich 2017-06-19 9:09 ` Julien Grall 2017-06-19 14:39 ` Tamas K Lengyel 2017-06-19 14:52 ` Julien Grall 2017-06-19 14:57 ` Tamas K Lengyel 2017-06-19 15:34 ` Julien Grall 2017-06-19 16:38 ` Tamas K Lengyel 2017-06-19 16:57 ` Tamas K Lengyel 2017-06-19 9:11 ` George Dunlap 2017-06-19 10:52 ` Jan Beulich 2017-06-19 14:48 ` Tamas K Lengyel 2017-06-19 14:54 ` George Dunlap 2017-06-19 14:56 ` Tamas K Lengyel 2017-04-05 13:12 ` [PATCH 0/2] memory: XSA-212 follow-up Julien Grall
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.