Instead let map_vcpu_info() and it's call to get_page_from_gfn()
populate the page in the child as needed. Also remove the bogus
copy_domain_page(): should be placed before the call to map_vcpu_info(),
as the later can update the contents of the vcpu_info page.
Note that this eliminates a bug in copy_vcpu_settings(): The function did
allocate a new page regardless of the GFN already having a mapping, thus in
particular breaking the case of two vCPU-s having their info areas on the same
page.
Fixes: 41548c5472a3 ('mem_sharing: VM forking')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Only build tested.
---
Changes since v4:
- New in this version.
---
xen/arch/x86/mm/mem_sharing.c | 36 ++++++-----------------------------
1 file changed, 6 insertions(+), 30 deletions(-)
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index ae5366d4476e..5f8f1fb4d871 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1689,48 +1689,24 @@ static int copy_vcpu_settings(struct domain *cd, const struct domain *d)
unsigned int i;
struct p2m_domain *p2m = p2m_get_hostp2m(cd);
int ret = -EINVAL;
+ mfn_t vcpu_info_mfn;
for ( i = 0; i < cd->max_vcpus; i++ )
{
struct vcpu *d_vcpu = d->vcpu[i];
struct vcpu *cd_vcpu = cd->vcpu[i];
- mfn_t vcpu_info_mfn;
if ( !d_vcpu || !cd_vcpu )
continue;
- /* Copy & map in the vcpu_info page if the guest uses one */
+ /* Map in the vcpu_info page if the guest uses one */
vcpu_info_mfn = d_vcpu->vcpu_info_mfn;
if ( !mfn_eq(vcpu_info_mfn, INVALID_MFN) )
{
- mfn_t new_vcpu_info_mfn = cd_vcpu->vcpu_info_mfn;
-
- /* Allocate & map the page for it if it hasn't been already */
- if ( mfn_eq(new_vcpu_info_mfn, INVALID_MFN) )
- {
- gfn_t gfn = mfn_to_gfn(d, vcpu_info_mfn);
- unsigned long gfn_l = gfn_x(gfn);
- struct page_info *page;
-
- if ( !(page = alloc_domheap_page(cd, 0)) )
- return -ENOMEM;
-
- new_vcpu_info_mfn = page_to_mfn(page);
- set_gpfn_from_mfn(mfn_x(new_vcpu_info_mfn), gfn_l);
-
- ret = p2m->set_entry(p2m, gfn, new_vcpu_info_mfn,
- PAGE_ORDER_4K, p2m_ram_rw,
- p2m->default_access, -1);
- if ( ret )
- return ret;
-
- ret = map_vcpu_info(cd_vcpu, gfn_l,
- PAGE_OFFSET(d_vcpu->vcpu_info));
- if ( ret )
- return ret;
- }
-
- copy_domain_page(new_vcpu_info_mfn, vcpu_info_mfn);
+ ret = map_vcpu_info(cd_vcpu, mfn_to_gfn(d, vcpu_info_mfn),
+ PAGE_OFFSET(d_vcpu->vcpu_info));
+ if ( ret )
+ return ret;
}
ret = copy_vpmu(d_vcpu, cd_vcpu);
--
2.42.0
On Mon, Oct 2, 2023 at 11:12 AM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> Instead let map_vcpu_info() and it's call to get_page_from_gfn()
> populate the page in the child as needed. Also remove the bogus
> copy_domain_page(): should be placed before the call to map_vcpu_info(),
> as the later can update the contents of the vcpu_info page.
>
> Note that this eliminates a bug in copy_vcpu_settings(): The function did
> allocate a new page regardless of the GFN already having a mapping, thus in
> particular breaking the case of two vCPU-s having their info areas on the same
> page.
>
> Fixes: 41548c5472a3 ('mem_sharing: VM forking')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Re-sending due to mailserver issues:
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
Hi,
While preparing to commit this series, I have noticed that there are no
Acked-by/Reviewed-by from Tamas (or at least not present in my inbox).
@Tamas, can you provide one?
Cheers,
On 02/10/2023 16:11, Roger Pau Monne wrote:
> Instead let map_vcpu_info() and it's call to get_page_from_gfn()
> populate the page in the child as needed. Also remove the bogus
> copy_domain_page(): should be placed before the call to map_vcpu_info(),
> as the later can update the contents of the vcpu_info page.
>
> Note that this eliminates a bug in copy_vcpu_settings(): The function did
> allocate a new page regardless of the GFN already having a mapping, thus in
> particular breaking the case of two vCPU-s having their info areas on the same
> page.
>
> Fixes: 41548c5472a3 ('mem_sharing: VM forking')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Only build tested.
> ---
> Changes since v4:
> - New in this version.
> ---
> xen/arch/x86/mm/mem_sharing.c | 36 ++++++-----------------------------
> 1 file changed, 6 insertions(+), 30 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index ae5366d4476e..5f8f1fb4d871 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1689,48 +1689,24 @@ static int copy_vcpu_settings(struct domain *cd, const struct domain *d)
> unsigned int i;
> struct p2m_domain *p2m = p2m_get_hostp2m(cd);
> int ret = -EINVAL;
> + mfn_t vcpu_info_mfn;
>
> for ( i = 0; i < cd->max_vcpus; i++ )
> {
> struct vcpu *d_vcpu = d->vcpu[i];
> struct vcpu *cd_vcpu = cd->vcpu[i];
> - mfn_t vcpu_info_mfn;
>
> if ( !d_vcpu || !cd_vcpu )
> continue;
>
> - /* Copy & map in the vcpu_info page if the guest uses one */
> + /* Map in the vcpu_info page if the guest uses one */
> vcpu_info_mfn = d_vcpu->vcpu_info_mfn;
> if ( !mfn_eq(vcpu_info_mfn, INVALID_MFN) )
> {
> - mfn_t new_vcpu_info_mfn = cd_vcpu->vcpu_info_mfn;
> -
> - /* Allocate & map the page for it if it hasn't been already */
> - if ( mfn_eq(new_vcpu_info_mfn, INVALID_MFN) )
> - {
> - gfn_t gfn = mfn_to_gfn(d, vcpu_info_mfn);
> - unsigned long gfn_l = gfn_x(gfn);
> - struct page_info *page;
> -
> - if ( !(page = alloc_domheap_page(cd, 0)) )
> - return -ENOMEM;
> -
> - new_vcpu_info_mfn = page_to_mfn(page);
> - set_gpfn_from_mfn(mfn_x(new_vcpu_info_mfn), gfn_l);
> -
> - ret = p2m->set_entry(p2m, gfn, new_vcpu_info_mfn,
> - PAGE_ORDER_4K, p2m_ram_rw,
> - p2m->default_access, -1);
> - if ( ret )
> - return ret;
> -
> - ret = map_vcpu_info(cd_vcpu, gfn_l,
> - PAGE_OFFSET(d_vcpu->vcpu_info));
> - if ( ret )
> - return ret;
> - }
> -
> - copy_domain_page(new_vcpu_info_mfn, vcpu_info_mfn);
> + ret = map_vcpu_info(cd_vcpu, mfn_to_gfn(d, vcpu_info_mfn),
> + PAGE_OFFSET(d_vcpu->vcpu_info));
> + if ( ret )
> + return ret;
> }
>
> ret = copy_vpmu(d_vcpu, cd_vcpu);
--
Julien Grall
On Thu, Oct 5, 2023 at 1:11 PM Julien Grall <julien@xen.org> wrote: > > Hi, > > While preparing to commit this series, I have noticed that there are no > Acked-by/Reviewed-by from Tamas (or at least not present in my inbox). > > @Tamas, can you provide one? I see an "Acked-by" from Tamas two days ago. -George
Hi George, On 05/10/2023 13:42, George Dunlap wrote: > On Thu, Oct 5, 2023 at 1:11 PM Julien Grall <julien@xen.org> wrote: >> >> Hi, >> >> While preparing to commit this series, I have noticed that there are no >> Acked-by/Reviewed-by from Tamas (or at least not present in my inbox). >> >> @Tamas, can you provide one? > > I see an "Acked-by" from Tamas two days ago. Sadly, it is also not on lore.kernel.org or our xenproject mail archives. In [1], Tamas pointed out he had some e-mail trouble. So anything sent by Tamas before last Tuesday is not present in my inbox or any mail archives. Not clear why the Citrix folks received it. Cheers, [1] https://lore.kernel.org/all/CABfawhn0vH6rS8-SJQJVZtto2HA61By1bCG3w9bJMJR3x+rXsg@mail.gmail.com/ -- Julien Grall
© 2016 - 2026 Red Hat, Inc.