From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 1/3] x86: fix guest CPUID handling Date: Wed, 30 Apr 2014 18:03:06 +0100 Message-ID: <53612CCA.4060403@citrix.com> References: <536120AF020000780000DD1B@mail.emea.novell.com> <53612347020000780000DD53@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3785248715404056192==" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WfXuZ-00008M-VE for xen-devel@lists.xenproject.org; Wed, 30 Apr 2014 17:03:12 +0000 In-Reply-To: <53612347020000780000DD53@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Ian Campbell , xen-devel , Keir Fraser , Ian Jackson , Tim Deegan List-Id: xen-devel@lists.xenproject.org --===============3785248715404056192== Content-Type: multipart/alternative; boundary="------------020206090605060001080208" --------------020206090605060001080208 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 30/04/14 15:22, Jan Beulich wrote: > The way XEN_DOMCTL_set_cpuid got handled so far allowed for surprises > to the caller. With this set of operations > - set leaf A (using array index 0) > - set leaf B (using array index 1) > - clear leaf A (clearing array index 0) > - set leaf B (using array index 0) > - clear leaf B (clearing array index 0) > the entry for leaf B at array index 1 would still be in place, while > the caller would expect it to be cleared. > > While looking at the use sites of d->arch.cpuid[] I also noticed that > the allocation of the array needlessly uses the zeroing form - the > relevant fields of the array elements get set in a loop immediately > following the allocation. > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper > > --- 2014-04-16.orig/xen/arch/x86/domain.c 2014-02-05 15:21:00.000000000 +0100 > +++ 2014-04-16/xen/arch/x86/domain.c 2014-04-30 13:53:18.000000000 +0200 > @@ -549,7 +549,7 @@ int arch_domain_create(struct domain *d, > > if ( !is_idle_domain(d) ) > { > - d->arch.cpuids = xzalloc_array(cpuid_input_t, MAX_CPUID_INPUT); > + d->arch.cpuids = xmalloc_array(cpuid_input_t, MAX_CPUID_INPUT); > rc = -ENOMEM; > if ( d->arch.cpuids == NULL ) > goto fail; > --- 2014-04-16.orig/xen/arch/x86/domctl.c 2014-02-05 15:21:01.000000000 +0100 > +++ 2014-04-16/xen/arch/x86/domctl.c 2014-04-30 13:43:30.000000000 +0200 > @@ -1006,14 +1006,18 @@ long arch_do_domctl( > case XEN_DOMCTL_set_cpuid: > { > xen_domctl_cpuid_t *ctl = &domctl->u.cpuid; > - cpuid_input_t *cpuid = NULL; > + cpuid_input_t *cpuid, *unused = NULL; > > for ( i = 0; i < MAX_CPUID_INPUT; i++ ) > { > cpuid = &d->arch.cpuids[i]; > > if ( cpuid->input[0] == XEN_CPUID_INPUT_UNUSED ) > - break; > + { > + if ( !unused ) > + unused = cpuid; > + continue; > + } > > if ( (cpuid->input[0] == ctl->input[0]) && > ((cpuid->input[1] == XEN_CPUID_INPUT_UNUSED) || > @@ -1021,15 +1025,12 @@ long arch_do_domctl( > break; > } > > - if ( i == MAX_CPUID_INPUT ) > - { > - ret = -ENOENT; > - } > + if ( i < MAX_CPUID_INPUT ) > + *cpuid = *ctl; > + else if ( unused ) > + *unused = *ctl; > else > - { > - memcpy(cpuid, ctl, sizeof(cpuid_input_t)); > - ret = 0; > - } > + ret = -ENOENT; > } > break; > > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------020206090605060001080208 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 30/04/14 15:22, Jan Beulich wrote:
The way XEN_DOMCTL_set_cpuid got handled so far allowed for surprises
to the caller. With this set of operations
- set leaf A (using array index 0)
- set leaf B (using array index 1)
- clear leaf A (clearing array index 0)
- set leaf B (using array index 0)
- clear leaf B (clearing array index 0)
the entry for leaf B at array index 1 would still be in place, while
the caller would expect it to be cleared.

While looking at the use sites of d->arch.cpuid[] I also noticed that
the allocation of the array needlessly uses the zeroing form - the
relevant fields of the array elements get set in a loop immediately
following the allocation.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

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


--- 2014-04-16.orig/xen/arch/x86/domain.c	2014-02-05 15:21:00.000000000 +0100
+++ 2014-04-16/xen/arch/x86/domain.c	2014-04-30 13:53:18.000000000 +0200
@@ -549,7 +549,7 @@ int arch_domain_create(struct domain *d,
 
     if ( !is_idle_domain(d) )
     {
-        d->arch.cpuids = xzalloc_array(cpuid_input_t, MAX_CPUID_INPUT);
+        d->arch.cpuids = xmalloc_array(cpuid_input_t, MAX_CPUID_INPUT);
         rc = -ENOMEM;
         if ( d->arch.cpuids == NULL )
             goto fail;
--- 2014-04-16.orig/xen/arch/x86/domctl.c	2014-02-05 15:21:01.000000000 +0100
+++ 2014-04-16/xen/arch/x86/domctl.c	2014-04-30 13:43:30.000000000 +0200
@@ -1006,14 +1006,18 @@ long arch_do_domctl(
     case XEN_DOMCTL_set_cpuid:
     {
         xen_domctl_cpuid_t *ctl = &domctl->u.cpuid;
-        cpuid_input_t *cpuid = NULL; 
+        cpuid_input_t *cpuid, *unused = NULL;
 
         for ( i = 0; i < MAX_CPUID_INPUT; i++ )
         {
             cpuid = &d->arch.cpuids[i];
 
             if ( cpuid->input[0] == XEN_CPUID_INPUT_UNUSED )
-                break;
+            {
+                if ( !unused )
+                    unused = cpuid;
+                continue;
+            }
 
             if ( (cpuid->input[0] == ctl->input[0]) &&
                  ((cpuid->input[1] == XEN_CPUID_INPUT_UNUSED) ||
@@ -1021,15 +1025,12 @@ long arch_do_domctl(
                 break;
         }
         
-        if ( i == MAX_CPUID_INPUT )
-        {
-            ret = -ENOENT;
-        }
+        if ( i < MAX_CPUID_INPUT )
+            *cpuid = *ctl;
+        else if ( unused )
+            *unused = *ctl;
         else
-        {
-            memcpy(cpuid, ctl, sizeof(cpuid_input_t));
-            ret = 0;
-        }
+            ret = -ENOENT;
     }
     break;
 






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

--------------020206090605060001080208-- --===============3785248715404056192== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============3785248715404056192==--