Re: [PATCH v5 01/10] mem_sharing/fork: do not attempt to populate vcpu_info page

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxxxx>
> ---
> 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