diff mbox series

[1/2] x86/mem_sharing: copy cpuid during vm forking

Message ID 6d5ca8a57a2745e933f00706bff306844611f64d.1609781242.git.tamas.lengyel@intel.com (mailing list archive)
State New
Headers show
Series [1/2] x86/mem_sharing: copy cpuid during vm forking | expand

Commit Message

Lengyel, Tamas Jan. 4, 2021, 5:41 p.m. UTC 
Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
---
 xen/arch/x86/mm/mem_sharing.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jan Beulich Jan. 5, 2021, 8:40 a.m. UTC | #1 
On 04.01.2021 18:41, Tamas K Lengyel wrote:
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1764,6 +1764,7 @@ static int fork(struct domain *cd, struct domain *d)
>  
>          domain_pause(d);
>          cd->max_pages = d->max_pages;
> +        memcpy(cd->arch.cpuid, d->arch.cpuid, sizeof(*d->arch.cpuid));

Can such copying please be done using assignment rather than
memcpy(), for the added type safety? (I wouldn't mind doing
this while committing, so long as you don't mind.)

Jan
Andrew Cooper Jan. 5, 2021, 11:04 a.m. UTC | #2 
On 04/01/2021 17:41, Tamas K Lengyel wrote:
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
> ---
>  xen/arch/x86/mm/mem_sharing.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index c428fd16ce..4a02c7d6f2 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1764,6 +1764,7 @@ static int fork(struct domain *cd, struct domain *d)
>  
>          domain_pause(d);
>          cd->max_pages = d->max_pages;
> +        memcpy(cd->arch.cpuid, d->arch.cpuid, sizeof(*d->arch.cpuid));
>          cd->parent = d;
>      }
>  

You need to extend this to d->arch.msr and v->arch.msrs or someone is
going to have a very subtle bug to debug in the future.

~Andrew
Lengyel, Tamas Jan. 5, 2021, 3:50 p.m. UTC | #3 
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: Tuesday, January 5, 2021 6:05 AM
> To: Lengyel, Tamas <tamas.lengyel@intel.com>; xen-
> devel@lists.xenproject.org
> Cc: Tamas K Lengyel <tamas@tklengyel.com>; Jan Beulich
> <jbeulich@suse.com>; George Dunlap <george.dunlap@citrix.com>; Roger
> Pau Monné <roger.pau@citrix.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH 1/2] x86/mem_sharing: copy cpuid during vm forking
> 
> On 04/01/2021 17:41, Tamas K Lengyel wrote:
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
> > ---
> >  xen/arch/x86/mm/mem_sharing.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/xen/arch/x86/mm/mem_sharing.c
> > b/xen/arch/x86/mm/mem_sharing.c index c428fd16ce..4a02c7d6f2
> 100644
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -1764,6 +1764,7 @@ static int fork(struct domain *cd, struct domain
> > *d)
> >
> >          domain_pause(d);
> >          cd->max_pages = d->max_pages;
> > +        memcpy(cd->arch.cpuid, d->arch.cpuid,
> > + sizeof(*d->arch.cpuid));
> >          cd->parent = d;
> >      }
> >
> 
> You need to extend this to d->arch.msr and v->arch.msrs or someone is
> going to have a very subtle bug to debug in the future.

I need more information why v->arch.msrs would need to be copied manually. If it's saved/reloaded by hvm_save/hvm_load then we are already covered. If not, then why would we need to do that for forks but not for domain save/restore?

Tamas
diff mbox series

Patch

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index c428fd16ce..4a02c7d6f2 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1764,6 +1764,7 @@  static int fork(struct domain *cd, struct domain *d)
 
         domain_pause(d);
         cd->max_pages = d->max_pages;
+        memcpy(cd->arch.cpuid, d->arch.cpuid, sizeof(*d->arch.cpuid));
         cd->parent = d;
     }