x86/msr: Drop {pv,hvm}_max_vcpu_msrs objects

Commit Message

Andrew Cooper Feb. 24, 2020, 2:22 p.m. UTC

It turns out that these are unused, and we dup a type-dependent block of
zeros.  Use xzalloc() instead.

Read/write MSRs are typically 0 to being with, and non-zero defaults would
need dealing with at suitable INIT/RESET points (e.g. arch_vcpu_regs_init).

Signed-off-by: Andrew Cooper <[e-mail]>
---
CC: Jan Beulich <[e-mail]>
CC: Wei Liu <[e-mail]>
CC: Roger Pau Monné <[e-mail]>
---
 xen/arch/x86/msr.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

Comments

Roger Pau Monné Feb. 24, 2020, 2:32 p.m. UTC | #1

On Mon, Feb 24, 2020 at 02:22:31PM +0000, Andrew Cooper wrote:
> It turns out that these are unused, and we dup a type-dependent block of
> zeros.  Use xzalloc() instead.
> 
> Read/write MSRs are typically 0 to being with, and non-zero defaults would
> need dealing with at suitable INIT/RESET points (e.g. arch_vcpu_regs_init).
> 
> Signed-off-by: Andrew Cooper <[e-mail]>

Reviewed-by: Roger Pau Monné <[e-mail]>

Thanks, Roger.

Andrew Cooper Feb. 24, 2020, 2:34 p.m. UTC | #2

On 24/02/2020 14:32, Roger Pau Monné wrote:
> On Mon, Feb 24, 2020 at 02:22:31PM +0000, Andrew Cooper wrote:
>> It turns out that these are unused, and we dup a type-dependent block of
>> zeros.  Use xzalloc() instead.
>>
>> Read/write MSRs are typically 0 to being with, and non-zero defaults would
>> need dealing with at suitable INIT/RESET points (e.g. arch_vcpu_regs_init).
>>
>> Signed-off-by: Andrew Cooper <[e-mail]>
> Reviewed-by: Roger Pau Monné <[e-mail]>

Thanks.

FWIW, I rewrote the second paragraph to not mix up begin and being.

Also, this patch logically depends on "xen/xmalloc Unify type handling
in macros".

~Andrew

Jan Beulich Feb. 25, 2020, 2:42 p.m. UTC | #3

On 24.02.2020 15:22, Andrew Cooper wrote:
> @@ -103,10 +100,7 @@ int init_domain_msr_policy(struct domain *d)
>  
>  int init_vcpu_msr_policy(struct vcpu *v)
>  {
> -    struct domain *d = v->domain;
> -    struct vcpu_msrs *msrs =
> -        xmemdup(is_pv_domain(d) ?  &pv_max_vcpu_msrs
> -                                : &hvm_max_vcpu_msrs);
> +    struct vcpu_msrs *msrs = xzalloc(*msrs);

As said on the other thread, I don't think this visual (even if
not factual) use of an uninitialized variable is not helpful. I
don't see anything wrong with continuing the traditional

    struct vcpu_msrs *msrs = xzalloc(struct vcpu_msrs);

way. It would be a different thing if the variable to be
initialized was passed into the macro, and hence there
wouldn't be any explicit assignment (or initialization)
at the use sites ...

For the suggested alternative form
Reviewed-by: Jan Beulich <[e-mail]>

Jan

Patch

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index dd26c87758..3ebf777c53 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -35,9 +35,6 @@  struct msr_policy __read_mostly     raw_msr_policy,
                   __read_mostly hvm_max_msr_policy,
                   __read_mostly  pv_max_msr_policy;
 
-struct vcpu_msrs __read_mostly hvm_max_vcpu_msrs,
-                 __read_mostly  pv_max_vcpu_msrs;
-
 static void __init calculate_raw_policy(void)
 {
     /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
@@ -103,10 +100,7 @@  int init_domain_msr_policy(struct domain *d)
 
 int init_vcpu_msr_policy(struct vcpu *v)
 {
-    struct domain *d = v->domain;
-    struct vcpu_msrs *msrs =
-        xmemdup(is_pv_domain(d) ?  &pv_max_vcpu_msrs
-                                : &hvm_max_vcpu_msrs);
+    struct vcpu_msrs *msrs = xzalloc(*msrs);
 
     if ( !msrs )
         return -ENOMEM;