Re: [Xen-devel] [PATCH v15 06/14] x86/hvm: Introduce hvm_save_mtrr_msr_one func

2018-08-07 Thread Jan Beulich 
>>> On 07.08.18 at 17:02,  wrote:
>>  
>> > 
>> > -hvm_get_guest_pat(v, _mtrr.msr_pat_cr);
>> > +memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges,
>> > NUM_FIXED_MSR);
>> You want to BUILD_BUG_ON() array sizes differing, and then use
>> sizeof() in the call to memcpy().
>> 
> In this case sizes are different:
> msr_mtrr_fixed[NUM_FIXED_MSR];
> fixed_ranges[NUM_FIXED_RANGES];
> #define NUM_FIXED_RANGES 88
> #define NUM_FIXED_MSR 11

The base type of msr_mtrr_fixed[] is uint64_t, while fixed_ranges[]'s
is uint8_t. I had specifically used sizeof() in my previous reply (instead
of ARRAY_SIZE()) to avoid exactly this kind of confusion.

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v15 06/14] x86/hvm: Introduce hvm_save_mtrr_msr_one func

2018-08-07 Thread Isaila Alexandru 
> 
> > 
> > -hvm_get_guest_pat(v, _mtrr.msr_pat_cr);
> > +memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges,
> > NUM_FIXED_MSR);
> You want to BUILD_BUG_ON() array sizes differing, and then use
> sizeof() in the call to memcpy().
> 
In this case sizes are different:
msr_mtrr_fixed[NUM_FIXED_MSR];
fixed_ranges[NUM_FIXED_RANGES];
#define NUM_FIXED_RANGES 88
#define NUM_FIXED_MSR 11

so it will most likely assert a message.

Alex




___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v15 06/14] x86/hvm: Introduce hvm_save_mtrr_msr_one func

2018-08-07 Thread Jan Beulich 
>>> On 03.08.18 at 15:53,  wrote:
> +for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
> +{
> +/* save physbase */
> +hw_mtrr.msr_mtrr_var[i * 2] = mtrr_state->var_ranges->base;
> +/* save physmask */
> +hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr_state->var_ranges->mask;
> +}

One of the intended side effects of using structure field on the rhs
was to be able to drop the (now redundant) comments.

> -hvm_get_guest_pat(v, _mtrr.msr_pat_cr);
> +memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges, NUM_FIXED_MSR);

You want to BUILD_BUG_ON() array sizes differing, and then use
sizeof() in the call to memcpy().

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v15 06/14] x86/hvm: Introduce hvm_save_mtrr_msr_one func

2018-08-03 Thread Alexandru Isaila 
This is used to save data from a single instance.

Signed-off-by: Alexandru Isaila 

---
Changes since v14:
- Fix style violations
- Use structure fields over cast
- Use memcpy for fixed_ranges.

Note: This patch is based on Roger Pau Monne's series[1]
---
 xen/arch/x86/hvm/mtrr.c | 77 +
 1 file changed, 40 insertions(+), 37 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 48facbb..2d5af72 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -718,52 +718,55 @@ int hvm_set_mem_pinned_cacheattr(struct domain *d, 
uint64_t gfn_start,
 return 0;
 }
 
-static int hvm_save_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
+static int hvm_save_mtrr_msr_one(struct vcpu *v, hvm_domain_context_t *h)
 {
-struct vcpu *v;
+const struct mtrr_state *mtrr_state = >arch.hvm_vcpu.mtrr;
+struct hvm_hw_mtrr hw_mtrr = {
+.msr_mtrr_def_type = mtrr_state->def_type |
+ MASK_INSR(mtrr_state->fixed_enabled,
+   MTRRdefType_FE) |
+ MASK_INSR(mtrr_state->enabled, MTRRdefType_E),
+.msr_mtrr_cap  = mtrr_state->mtrr_cap,
+};
+unsigned int i;
 
-/* save mtrr */
-for_each_vcpu(d, v)
+if ( MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT) >
+ (ARRAY_SIZE(hw_mtrr.msr_mtrr_var) / 2) )
 {
-const struct mtrr_state *mtrr_state = >arch.hvm_vcpu.mtrr;
-struct hvm_hw_mtrr hw_mtrr = {
-.msr_mtrr_def_type = mtrr_state->def_type |
- MASK_INSR(mtrr_state->fixed_enabled,
-   MTRRdefType_FE) |
- MASK_INSR(mtrr_state->enabled, MTRRdefType_E),
-.msr_mtrr_cap  = mtrr_state->mtrr_cap,
-};
-unsigned int i;
+dprintk(XENLOG_G_ERR,
+"HVM save: %pv: too many (%lu) variable range MTRRs\n",
+v, MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT));
+return -EINVAL;
+}
 
-if ( MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT) >
- (ARRAY_SIZE(hw_mtrr.msr_mtrr_var) / 2) )
-{
-dprintk(XENLOG_G_ERR,
-"HVM save: %pv: too many (%lu) variable range MTRRs\n",
-v, MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT));
-return -EINVAL;
-}
+hvm_get_guest_pat(v, _mtrr.msr_pat_cr);
+
+for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
+{
+/* save physbase */
+hw_mtrr.msr_mtrr_var[i * 2] = mtrr_state->var_ranges->base;
+/* save physmask */
+hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr_state->var_ranges->mask;
+}
 
-hvm_get_guest_pat(v, _mtrr.msr_pat_cr);
+memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges, NUM_FIXED_MSR);
 
-for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
-{
-/* save physbase */
-hw_mtrr.msr_mtrr_var[i*2] =
-((uint64_t*)mtrr_state->var_ranges)[i*2];
-/* save physmask */
-hw_mtrr.msr_mtrr_var[i*2+1] =
-((uint64_t*)mtrr_state->var_ranges)[i*2+1];
-}
+return hvm_save_entry(MTRR, v->vcpu_id, h, _mtrr);
+}
 
-for ( i = 0; i < NUM_FIXED_MSR; i++ )
-hw_mtrr.msr_mtrr_fixed[i] =
-((uint64_t*)mtrr_state->fixed_ranges)[i];
+static int hvm_save_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
+{
+struct vcpu *v;
+int err = 0;
 
-if ( hvm_save_entry(MTRR, v->vcpu_id, h, _mtrr) != 0 )
-return 1;
+/* save mtrr */
+for_each_vcpu(d, v)
+{
+   err = hvm_save_mtrr_msr_one(v, h);
+   if ( err )
+   break;
 }
-return 0;
+return err;
 }
 
 static int hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
-- 
2.7.4


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel