[PATCH 0/5] PVH MTRR initial state

[PATCH 0/5] PVH MTRR initial state

[Roger Pau Monne]

Hello,

The following patches set a sane initial MTRR state for both Dom0 and
DomU PVH guests. Note that for Dom0 the host MTRR state is used, OTOH
for DomU the default MTRR type is set to write-back.

This should avoid guests having to setup some kind of MTRR state in
order to boot.

Thanks, Roger.

Roger Pau Monne (5):
  hvm/mtrr: add emacs local variables block with formatting info
  hvm/mtrr: use the hardware number of variable ranges for Dom0
  hvm/mtrr: copy hardware state for Dom0
  libxc/pvh: set default MTRR type to write-back
  docs/pvh: document initial MTRR state

 docs/misc/pvh.markdown   | 15 +++++++++++++
 tools/libxc/xc_dom_x86.c | 44 ++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/mtrr.c  | 46 ++++++++++++++++++++++++++++++++++++++--
 3 files changed, 103 insertions(+), 2 deletions(-)

-- 
2.17.0


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

[PATCH 1/5] hvm/mtrr: add emacs local variables block with formatting info

[Roger Pau Monne]

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/mtrr.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index b721c6330f..b3c08c3977 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -853,3 +853,13 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
 
     return MTRR_TYPE_UNCACHABLE;
 }
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.17.0


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

[PATCH 2/5] hvm/mtrr: use the hardware number of variable ranges for Dom0

[Roger Pau Monne]

Expand the size of the variable ranges array to match the size of the
underlying hardware, this is a preparatory change for copying the
hardware MTRR state for Dom0.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/mtrr.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index b3c08c3977..95a3deabea 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -154,14 +154,17 @@ uint8_t pat_type_2_pte_flags(uint8_t pat_type)
 int hvm_vcpu_cacheattr_init(struct vcpu *v)
 {
     struct mtrr_state *m = &v->arch.hvm_vcpu.mtrr;
+    unsigned int num_var_ranges =
+        is_hardware_domain(v->domain) ? (mtrr_state.mtrr_cap & 0xff)
+                                      : MTRR_VCNT;
 
     memset(m, 0, sizeof(*m));
 
-    m->var_ranges = xzalloc_array(struct mtrr_var_range, MTRR_VCNT);
+    m->var_ranges = xzalloc_array(struct mtrr_var_range, num_var_ranges);
     if ( m->var_ranges == NULL )
         return -ENOMEM;
 
-    m->mtrr_cap = (1u << 10) | (1u << 8) | MTRR_VCNT;
+    m->mtrr_cap = (1u << 10) | (1u << 8) | num_var_ranges;
 
     v->arch.hvm_vcpu.pat_cr =
         ((uint64_t)PAT_TYPE_WRBACK) |               /* PAT0: WB */
@@ -683,6 +686,9 @@ static int hvm_save_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
                                 | (mtrr_state->enabled << 10);
         hw_mtrr.msr_mtrr_cap = mtrr_state->mtrr_cap;
 
+        if ( (mtrr_state->mtrr_cap & 0xff) != MTRR_VCNT )
+            return -EINVAL;
+
         for ( i = 0; i < MTRR_VCNT; i++ )
         {
             /* save physbase */
@@ -727,6 +733,9 @@ static int hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
 
     mtrr_state->mtrr_cap = hw_mtrr.msr_mtrr_cap;
 
+    if ( (mtrr_state->mtrr_cap & 0xff) != MTRR_VCNT )
+        return -EINVAL;
+
     for ( i = 0; i < NUM_FIXED_MSR; i++ )
         mtrr_fix_range_msr_set(d, mtrr_state, i, hw_mtrr.msr_mtrr_fixed[i]);
 
-- 
2.17.0


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

[PATCH 3/5] hvm/mtrr: copy hardware state for Dom0

[Roger Pau Monne]

Copy the state found on the hardware when creating a PVH Dom0. Since
the memory map provided to a PVH Dom0 is based on the native one using
the same set of MTRR ranges should provide Dom0 with a sane MTRR state
without having to manually build it in Xen.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/mtrr.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 95a3deabea..1cb000388a 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -176,6 +176,29 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v)
         ((uint64_t)PAT_TYPE_UC_MINUS << 48) |       /* PAT6: UC- */
         ((uint64_t)PAT_TYPE_UNCACHABLE << 56);      /* PAT7: UC */
 
+    if ( is_hardware_domain(v->domain) )
+    {
+        /* Copy values from the host. */
+        struct domain *d = v->domain;
+        unsigned int i;
+
+        if ( mtrr_state.have_fixed )
+            for ( i = 0; i < NUM_FIXED_MSR; i++ )
+                mtrr_fix_range_msr_set(d, m, i,
+                                      ((uint64_t *)mtrr_state.fixed_ranges)[i]);
+
+        for ( i = 0; i < num_var_ranges; i++ )
+        {
+            mtrr_var_range_msr_set(d, m, MSR_IA32_MTRR_PHYSBASE(i),
+                                   mtrr_state.var_ranges[i].base);
+            mtrr_var_range_msr_set(d, m, MSR_IA32_MTRR_PHYSMASK(i),
+                                   mtrr_state.var_ranges[i].mask);
+        }
+
+        mtrr_def_type_msr_set(d, m, mtrr_state.def_type |
+                                    (mtrr_state.enabled << 10));
+    }
+
     return 0;
 }
 
-- 
2.17.0


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

[PATCH 4/5] libxc/pvh: set default MTRR type to write-back

[Roger Pau Monne]

And enable MTRR. This allows to provide a sane initial MTRR state for
PVH DomUs. This will have to be expanded when pci-passthrough support
is added to PVH guests, so that MMIO regions of devices are set as
UC.

Note that initial MTRR setup is done by hvmloader for HVM guests,
that's not used by PVH guests.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
----
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxc/xc_dom_x86.c | 44 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index e33a28847d..d28ff4d7e9 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -53,6 +53,9 @@
 #define X86_CR0_PE 0x01
 #define X86_CR0_ET 0x10
 
+#define MTRR_TYPE_WRBACK     6
+#define MTRR_DEF_TYPE_ENABLE (1u << 11)
+
 #define SPECIALPAGE_PAGING   0
 #define SPECIALPAGE_ACCESS   1
 #define SPECIALPAGE_SHARING  2
@@ -931,6 +934,20 @@ static int vcpu_x86_64(struct xc_dom_image *dom)
     return rc;
 }
 
+const static void *hvm_get_save_record(const void *ctx, unsigned int type,
+                                       unsigned int instance)
+{
+    const struct hvm_save_descriptor *header;
+
+    for ( header = ctx;
+          header->typecode != HVM_SAVE_CODE(END);
+          ctx += sizeof(*header) + header->length, header = ctx )
+        if ( header->typecode == type && header->instance == instance )
+            return ctx + sizeof(*header);
+
+    return NULL;
+}
+
 static int vcpu_hvm(struct xc_dom_image *dom)
 {
     struct {
@@ -938,9 +955,12 @@ static int vcpu_hvm(struct xc_dom_image *dom)
         HVM_SAVE_TYPE(HEADER) header;
         struct hvm_save_descriptor cpu_d;
         HVM_SAVE_TYPE(CPU) cpu;
+        struct hvm_save_descriptor mtrr_d;
+        HVM_SAVE_TYPE(MTRR) mtrr;
         struct hvm_save_descriptor end_d;
         HVM_SAVE_TYPE(END) end;
     } bsp_ctx;
+    const HVM_SAVE_TYPE(MTRR) *mtrr_record;
     uint8_t *full_ctx = NULL;
     int rc;
 
@@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
     if ( dom->start_info_seg.pfn )
         bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
 
+    /* Set the MTRR. */
+    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
+    bsp_ctx.mtrr_d.instance = 0;
+    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
+
+    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
+    if ( !mtrr_record )
+    {
+        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+                     "%s: unable to get MTRR save record", __func__);
+        goto out;
+    }
+
+    memcpy(&bsp_ctx.mtrr, mtrr_record, sizeof(bsp_ctx.mtrr));
+
+    /* TODO: maybe this should be a firmware option instead? */
+    if ( !dom->device_model )
+        /*
+         * Enable MTRR, set default type to WB.
+         * TODO: add MMIO areas as UC when passthrough is supported.
+         */
+        bsp_ctx.mtrr.msr_mtrr_def_type = MTRR_TYPE_WRBACK |
+                                         MTRR_DEF_TYPE_ENABLE;
+
     /* Set the end descriptor. */
     bsp_ctx.end_d.typecode = HVM_SAVE_CODE(END);
     bsp_ctx.end_d.instance = 0;
-- 
2.17.0


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

[PATCH 5/5] docs/pvh: document initial MTRR state

[Roger Pau Monne]

Provided to both Dom0 and DomUs.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 docs/misc/pvh.markdown | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/docs/misc/pvh.markdown b/docs/misc/pvh.markdown
index e85fb15374..639401a887 100644
--- a/docs/misc/pvh.markdown
+++ b/docs/misc/pvh.markdown
@@ -92,3 +92,18 @@ event channels. Delivery of those interrupts can be configured in the same way
 as HVM guests, check xen/include/public/hvm/params.h and
 xen/include/public/hvm/hvm\_op.h for more information about available delivery
 methods.
+
+## MTRR ##
+
+### Unprivileged guests ###
+
+PVH guests are booted with the default MTRR type set to write-back and MTRR
+enabled. This allows DomUs to start with a sane MTRR state. Note that this will
+have to be revisited when pci-passthrough is added to PVH in order to set MMIO
+regions as UC.
+
+### Hardware domain ###
+
+A PVH hardware domain is booted with the same MTRR state as the one found on
+the host. This is done because the hardware domain memory map is already a
+modified copy of the host memory map, so the same MTRR setup should work.
-- 
2.17.0


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

Re: [PATCH 2/5] hvm/mtrr: use the hardware number of variable ranges for Dom0

[Jan Beulich]

>>> On 10.05.18 at 19:15, <roger.pau@citrix.com> wrote:
> ---
>  xen/arch/x86/hvm/mtrr.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

What about hvm_msr_{read,write}_intercept()'s uses of MTRR_VCNT?

> @@ -683,6 +686,9 @@ static int hvm_save_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
>                                  | (mtrr_state->enabled << 10);
>          hw_mtrr.msr_mtrr_cap = mtrr_state->mtrr_cap;
>  
> +        if ( (mtrr_state->mtrr_cap & 0xff) != MTRR_VCNT )
> +            return -EINVAL;

I certainly agree with this one, but ...

> @@ -727,6 +733,9 @@ static int hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
>  
>      mtrr_state->mtrr_cap = hw_mtrr.msr_mtrr_cap;
>  
> +    if ( (mtrr_state->mtrr_cap & 0xff) != MTRR_VCNT )
> +        return -EINVAL;

... isn't this more strict than it needs to be? A smaller incoming count
ought to be okay I would think. Relaxing things here would then of
course also require relaxing things on the save side.

Jan



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

Re: [PATCH 3/5] hvm/mtrr: copy hardware state for Dom0

[Jan Beulich]

>>> On 10.05.18 at 19:15, <roger.pau@citrix.com> wrote:
> Copy the state found on the hardware when creating a PVH Dom0. Since
> the memory map provided to a PVH Dom0 is based on the native one using
> the same set of MTRR ranges should provide Dom0 with a sane MTRR state
> without having to manually build it in Xen.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/hvm/mtrr.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> index 95a3deabea..1cb000388a 100644
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -176,6 +176,29 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v)
>          ((uint64_t)PAT_TYPE_UC_MINUS << 48) |       /* PAT6: UC- */
>          ((uint64_t)PAT_TYPE_UNCACHABLE << 56);      /* PAT7: UC */
>  
> +    if ( is_hardware_domain(v->domain) )
> +    {
> +        /* Copy values from the host. */
> +        struct domain *d = v->domain;
> +        unsigned int i;
> +
> +        if ( mtrr_state.have_fixed )
> +            for ( i = 0; i < NUM_FIXED_MSR; i++ )
> +                mtrr_fix_range_msr_set(d, m, i,
> +                                      ((uint64_t *)mtrr_state.fixed_ranges)[i]);

The presence/absence of fixed range MTRRs needs to be reflected in the
capabilities MSR. Strictly speaking in their absence MSR access attempts to
the fixed range MSRs should also cause #GP, as should any attempt to
enable them in defType.

> +        for ( i = 0; i < num_var_ranges; i++ )
> +        {
> +            mtrr_var_range_msr_set(d, m, MSR_IA32_MTRR_PHYSBASE(i),
> +                                   mtrr_state.var_ranges[i].base);
> +            mtrr_var_range_msr_set(d, m, MSR_IA32_MTRR_PHYSMASK(i),
> +                                   mtrr_state.var_ranges[i].mask);
> +        }
> +
> +        mtrr_def_type_msr_set(d, m, mtrr_state.def_type |
> +                                    (mtrr_state.enabled << 10));

In the interest of no further proliferation of this and similar literal numbers,
could I ask you to introduce #define-s into msr-index.h?

Jan


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

Re: [PATCH 4/5] libxc/pvh: set default MTRR type to write-back

[Wei Liu]

On Thu, May 10, 2018 at 06:15:04PM +0100, Roger Pau Monne wrote:
> @@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>      if ( dom->start_info_seg.pfn )
>          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
>  
> +    /* Set the MTRR. */
> +    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
> +    bsp_ctx.mtrr_d.instance = 0;
> +    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
> +
> +    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
> +    if ( !mtrr_record )
> +    {
> +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> +                     "%s: unable to get MTRR save record", __func__);
> +        goto out;
> +    }

Will this break cross version migration when the older hypervisor
doesn't have such record?

Wei.

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

Re: [PATCH 4/5] libxc/pvh: set default MTRR type to write-back

[Roger Pau Monné]

On Mon, May 14, 2018 at 05:00:51PM +0100, Wei Liu wrote:
> On Thu, May 10, 2018 at 06:15:04PM +0100, Roger Pau Monne wrote:
> > @@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
> >      if ( dom->start_info_seg.pfn )
> >          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
> >  
> > +    /* Set the MTRR. */
> > +    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
> > +    bsp_ctx.mtrr_d.instance = 0;
> > +    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
> > +
> > +    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
> > +    if ( !mtrr_record )
> > +    {
> > +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> > +                     "%s: unable to get MTRR save record", __func__);
> > +        goto out;
> > +    }
> 
> Will this break cross version migration when the older hypervisor
> doesn't have such record?

This migration record is already present, it's not introduced in this
patch series. I'm simply making use of it in order to set a valid
initial MTRR state.

Thanks, Roger.

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

Re: [PATCH 5/5] docs/pvh: document initial MTRR state

[Wei Liu]

On Thu, May 10, 2018 at 06:15:05PM +0100, Roger Pau Monne wrote:
> Provided to both Dom0 and DomUs.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  docs/misc/pvh.markdown | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/docs/misc/pvh.markdown b/docs/misc/pvh.markdown
> index e85fb15374..639401a887 100644
> --- a/docs/misc/pvh.markdown
> +++ b/docs/misc/pvh.markdown
> @@ -92,3 +92,18 @@ event channels. Delivery of those interrupts can be configured in the same way
>  as HVM guests, check xen/include/public/hvm/params.h and
>  xen/include/public/hvm/hvm\_op.h for more information about available delivery
>  methods.
> +
> +## MTRR ##
> +
> +### Unprivileged guests ###
> +
> +PVH guests are booted with the default MTRR type set to write-back and MTRR
> +enabled. This allows DomUs to start with a sane MTRR state. Note that this will
> +have to be revisited when pci-passthrough is added to PVH in order to set MMIO
> +regions as UC.

My reading is "revisited" implies the default type will change. In fact
it shouldn't. We should clarify: for ram it will remain WB, for MMIO
holes it will be UC.

Please correct me if I'm wrong.

Wei.

> +
> +### Hardware domain ###
> +
> +A PVH hardware domain is booted with the same MTRR state as the one found on
> +the host. This is done because the hardware domain memory map is already a
> +modified copy of the host memory map, so the same MTRR setup should work.
> -- 
> 2.17.0
> 

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

Re: [PATCH 5/5] docs/pvh: document initial MTRR state

[Jan Beulich]

>>> On 14.05.18 at 18:03, <wei.liu2@citrix.com> wrote:
> On Thu, May 10, 2018 at 06:15:05PM +0100, Roger Pau Monne wrote:
>> --- a/docs/misc/pvh.markdown
>> +++ b/docs/misc/pvh.markdown
>> @@ -92,3 +92,18 @@ event channels. Delivery of those interrupts can be configured in the same way
>>  as HVM guests, check xen/include/public/hvm/params.h and
>>  xen/include/public/hvm/hvm\_op.h for more information about available delivery
>>  methods.
>> +
>> +## MTRR ##
>> +
>> +### Unprivileged guests ###
>> +
>> +PVH guests are booted with the default MTRR type set to write-back and MTRR
>> +enabled. This allows DomUs to start with a sane MTRR state. Note that this will
>> +have to be revisited when pci-passthrough is added to PVH in order to set MMIO
>> +regions as UC.
> 
> My reading is "revisited" implies the default type will change. In fact
> it shouldn't. We should clarify: for ram it will remain WB, for MMIO
> holes it will be UC.

Why would changing the default late be a problem? A firmware update on
bare hardware might also have such an effect. The default type read from
the MSR must not change across the lifetime of a VM, but imo may change
across reboots of it.

Jan



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

Re: [PATCH 5/5] docs/pvh: document initial MTRR state

[Roger Pau Monné]

On Mon, May 14, 2018 at 05:03:52PM +0100, Wei Liu wrote:
> On Thu, May 10, 2018 at 06:15:05PM +0100, Roger Pau Monne wrote:
> > Provided to both Dom0 and DomUs.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Julien Grall <julien.grall@arm.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Tim Deegan <tim@xen.org>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  docs/misc/pvh.markdown | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/docs/misc/pvh.markdown b/docs/misc/pvh.markdown
> > index e85fb15374..639401a887 100644
> > --- a/docs/misc/pvh.markdown
> > +++ b/docs/misc/pvh.markdown
> > @@ -92,3 +92,18 @@ event channels. Delivery of those interrupts can be configured in the same way
> >  as HVM guests, check xen/include/public/hvm/params.h and
> >  xen/include/public/hvm/hvm\_op.h for more information about available delivery
> >  methods.
> > +
> > +## MTRR ##
> > +
> > +### Unprivileged guests ###
> > +
> > +PVH guests are booted with the default MTRR type set to write-back and MTRR
> > +enabled. This allows DomUs to start with a sane MTRR state. Note that this will
> > +have to be revisited when pci-passthrough is added to PVH in order to set MMIO
> > +regions as UC.
> 
> My reading is "revisited" implies the default type will change. In fact
> it shouldn't. We should clarify: for ram it will remain WB, for MMIO
> holes it will be UC.
>
> Please correct me if I'm wrong.

That's correct. I've used "revisited" here in the sense that Xen might
change the default type to UC and set the RAM regions as WB using
variable MTRR ranges for example.

I simply wanted to remark that the way RAM is set to WB is currently
done using the default MTRR type. RAM will always be set of WB for PVH
in MTRR, however the way to achieve it might change.

What about adding:

"Xen guarantees that RAM regions will always have the WB cache type
set in the initial MTRR state, either set by the default MTRR type or
by other means."

Thanks, Roger.

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

Re: [PATCH 5/5] docs/pvh: document initial MTRR state

[Wei Liu]

On Mon, May 14, 2018 at 10:13:47AM -0600, Jan Beulich wrote:
> >>> On 14.05.18 at 18:03, <wei.liu2@citrix.com> wrote:
> > On Thu, May 10, 2018 at 06:15:05PM +0100, Roger Pau Monne wrote:
> >> --- a/docs/misc/pvh.markdown
> >> +++ b/docs/misc/pvh.markdown
> >> @@ -92,3 +92,18 @@ event channels. Delivery of those interrupts can be configured in the same way
> >>  as HVM guests, check xen/include/public/hvm/params.h and
> >>  xen/include/public/hvm/hvm\_op.h for more information about available delivery
> >>  methods.
> >> +
> >> +## MTRR ##
> >> +
> >> +### Unprivileged guests ###
> >> +
> >> +PVH guests are booted with the default MTRR type set to write-back and MTRR
> >> +enabled. This allows DomUs to start with a sane MTRR state. Note that this will
> >> +have to be revisited when pci-passthrough is added to PVH in order to set MMIO
> >> +regions as UC.
> > 
> > My reading is "revisited" implies the default type will change. In fact
> > it shouldn't. We should clarify: for ram it will remain WB, for MMIO
> > holes it will be UC.
> 
> Why would changing the default late be a problem? A firmware update on
> bare hardware might also have such an effect. The default type read from
> the MSR must not change across the lifetime of a VM, but imo may change
> across reboots of it.
> 

Then setting a default here doesn't really help OS developers because
they will always need to write code to set the correct type -- not that
this is a big issue, but as I understand it the point here is to avoid
that.

Wei.

> Jan
> 
> 

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

Re: [PATCH 5/5] docs/pvh: document initial MTRR state

[Wei Liu]

On Mon, May 14, 2018 at 05:16:10PM +0100, Roger Pau Monné wrote:
> On Mon, May 14, 2018 at 05:03:52PM +0100, Wei Liu wrote:
> > On Thu, May 10, 2018 at 06:15:05PM +0100, Roger Pau Monne wrote:
> > > Provided to both Dom0 and DomUs.
> > > 
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > ---
> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Cc: Jan Beulich <jbeulich@suse.com>
> > > Cc: Julien Grall <julien.grall@arm.com>
> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > > Cc: Tim Deegan <tim@xen.org>
> > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > >  docs/misc/pvh.markdown | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/docs/misc/pvh.markdown b/docs/misc/pvh.markdown
> > > index e85fb15374..639401a887 100644
> > > --- a/docs/misc/pvh.markdown
> > > +++ b/docs/misc/pvh.markdown
> > > @@ -92,3 +92,18 @@ event channels. Delivery of those interrupts can be configured in the same way
> > >  as HVM guests, check xen/include/public/hvm/params.h and
> > >  xen/include/public/hvm/hvm\_op.h for more information about available delivery
> > >  methods.
> > > +
> > > +## MTRR ##
> > > +
> > > +### Unprivileged guests ###
> > > +
> > > +PVH guests are booted with the default MTRR type set to write-back and MTRR
> > > +enabled. This allows DomUs to start with a sane MTRR state. Note that this will
> > > +have to be revisited when pci-passthrough is added to PVH in order to set MMIO
> > > +regions as UC.
> > 
> > My reading is "revisited" implies the default type will change. In fact
> > it shouldn't. We should clarify: for ram it will remain WB, for MMIO
> > holes it will be UC.
> >
> > Please correct me if I'm wrong.
> 
> That's correct. I've used "revisited" here in the sense that Xen might
> change the default type to UC and set the RAM regions as WB using
> variable MTRR ranges for example.
> 
> I simply wanted to remark that the way RAM is set to WB is currently
> done using the default MTRR type. RAM will always be set of WB for PVH
> in MTRR, however the way to achieve it might change.
> 
> What about adding:
> 
> "Xen guarantees that RAM regions will always have the WB cache type
> set in the initial MTRR state, either set by the default MTRR type or
> by other means."
> 

Sounds good to me.

Wei.

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

Re: [PATCH 3/5] hvm/mtrr: copy hardware state for Dom0

[Roger Pau Monné]

On Mon, May 14, 2018 at 08:26:30AM -0600, Jan Beulich wrote:
> >>> On 10.05.18 at 19:15, <roger.pau@citrix.com> wrote:
> > Copy the state found on the hardware when creating a PVH Dom0. Since
> > the memory map provided to a PVH Dom0 is based on the native one using
> > the same set of MTRR ranges should provide Dom0 with a sane MTRR state
> > without having to manually build it in Xen.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> >  xen/arch/x86/hvm/mtrr.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> > index 95a3deabea..1cb000388a 100644
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -176,6 +176,29 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v)
> >          ((uint64_t)PAT_TYPE_UC_MINUS << 48) |       /* PAT6: UC- */
> >          ((uint64_t)PAT_TYPE_UNCACHABLE << 56);      /* PAT7: UC */
> >  
> > +    if ( is_hardware_domain(v->domain) )
> > +    {
> > +        /* Copy values from the host. */
> > +        struct domain *d = v->domain;
> > +        unsigned int i;
> > +
> > +        if ( mtrr_state.have_fixed )
> > +            for ( i = 0; i < NUM_FIXED_MSR; i++ )
> > +                mtrr_fix_range_msr_set(d, m, i,
> > +                                      ((uint64_t *)mtrr_state.fixed_ranges)[i]);
> 
> The presence/absence of fixed range MTRRs needs to be reflected in the
> capabilities MSR. Strictly speaking in their absence MSR access attempts to
> the fixed range MSRs should also cause #GP, as should any attempt to
> enable them in defType.

My intention was to always provide the fixed range MTRR capability,
regardless of whether the underlying hardware has it or not. Then of
course fixed ranges won't be enabled by default in the deftype MSR if
the underlying hardware also hasn't got them enabled.

> > +        for ( i = 0; i < num_var_ranges; i++ )
> > +        {
> > +            mtrr_var_range_msr_set(d, m, MSR_IA32_MTRR_PHYSBASE(i),
> > +                                   mtrr_state.var_ranges[i].base);
> > +            mtrr_var_range_msr_set(d, m, MSR_IA32_MTRR_PHYSMASK(i),
> > +                                   mtrr_state.var_ranges[i].mask);
> > +        }
> > +
> > +        mtrr_def_type_msr_set(d, m, mtrr_state.def_type |
> > +                                    (mtrr_state.enabled << 10));
> 
> In the interest of no further proliferation of this and similar literal numbers,
> could I ask you to introduce #define-s into msr-index.h?

Sure.

Thanks, Roger.

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

Re: [PATCH 4/5] libxc/pvh: set default MTRR type to write-back

[Wei Liu]

On Mon, May 14, 2018 at 05:02:47PM +0100, Roger Pau Monné wrote:
> On Mon, May 14, 2018 at 05:00:51PM +0100, Wei Liu wrote:
> > On Thu, May 10, 2018 at 06:15:04PM +0100, Roger Pau Monne wrote:
> > > @@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
> > >      if ( dom->start_info_seg.pfn )
> > >          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
> > >  
> > > +    /* Set the MTRR. */
> > > +    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
> > > +    bsp_ctx.mtrr_d.instance = 0;
> > > +    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
> > > +
> > > +    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
> > > +    if ( !mtrr_record )
> > > +    {
> > > +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> > > +                     "%s: unable to get MTRR save record", __func__);
> > > +        goto out;
> > > +    }
> > 
> > Will this break cross version migration when the older hypervisor
> > doesn't have such record?
> 
> This migration record is already present, it's not introduced in this
> patch series. I'm simply making use of it in order to set a valid
> initial MTRR state.
> 

Then I'm even more confused: does this mean the record is not properly
honoured in the hypervisor? I.e. this is a hypervisor bug?

Wei.

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

Re: [PATCH 4/5] libxc/pvh: set default MTRR type to write-back

[Wei Liu]

On Mon, May 14, 2018 at 05:42:53PM +0100, Wei Liu wrote:
> On Mon, May 14, 2018 at 05:02:47PM +0100, Roger Pau Monné wrote:
> > On Mon, May 14, 2018 at 05:00:51PM +0100, Wei Liu wrote:
> > > On Thu, May 10, 2018 at 06:15:04PM +0100, Roger Pau Monne wrote:
> > > > @@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
> > > >      if ( dom->start_info_seg.pfn )
> > > >          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
> > > >  
> > > > +    /* Set the MTRR. */
> > > > +    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
> > > > +    bsp_ctx.mtrr_d.instance = 0;
> > > > +    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
> > > > +
> > > > +    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
> > > > +    if ( !mtrr_record )
> > > > +    {
> > > > +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> > > > +                     "%s: unable to get MTRR save record", __func__);
> > > > +        goto out;
> > > > +    }
> > > 
> > > Will this break cross version migration when the older hypervisor
> > > doesn't have such record?
> > 
> > This migration record is already present, it's not introduced in this
> > patch series. I'm simply making use of it in order to set a valid
> > initial MTRR state.
> > 
> 
> Then I'm even more confused: does this mean the record is not properly
> honoured in the hypervisor? I.e. this is a hypervisor bug?
> 

Or maybe this means the setting of the default type should happen
somewhere else? Let me read all the code to make sure I'm not talking
nonsense and misguide you here.

Wei.

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

Re: [PATCH 4/5] libxc/pvh: set default MTRR type to write-back

[Roger Pau Monné]

On Mon, May 14, 2018 at 05:42:53PM +0100, Wei Liu wrote:
> On Mon, May 14, 2018 at 05:02:47PM +0100, Roger Pau Monné wrote:
> > On Mon, May 14, 2018 at 05:00:51PM +0100, Wei Liu wrote:
> > > On Thu, May 10, 2018 at 06:15:04PM +0100, Roger Pau Monne wrote:
> > > > @@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
> > > >      if ( dom->start_info_seg.pfn )
> > > >          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
> > > >  
> > > > +    /* Set the MTRR. */
> > > > +    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
> > > > +    bsp_ctx.mtrr_d.instance = 0;
> > > > +    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
> > > > +
> > > > +    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
> > > > +    if ( !mtrr_record )
> > > > +    {
> > > > +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> > > > +                     "%s: unable to get MTRR save record", __func__);
> > > > +        goto out;
> > > > +    }
> > > 
> > > Will this break cross version migration when the older hypervisor
> > > doesn't have such record?
> > 
> > This migration record is already present, it's not introduced in this
> > patch series. I'm simply making use of it in order to set a valid
> > initial MTRR state.
> > 
> 
> Then I'm even more confused: does this mean the record is not properly
> honoured in the hypervisor? I.e. this is a hypervisor bug?

I'm kind of lost. This code I'm adding is not used for migration at
all. This is used in order to set the initial vCPU state for PVH
guests (which is done using xc_domain_hvm_setcontext). In order to
provide an initial MTRR state we need to create a MTRR record and
provide it to the hypervisor. I'm not introducing the MTRR record in
this patch, I'm just making use of it in order to set a sane initial
MTRR state for PVH guests.

libxc already does the same in order to set the initial CPU register
state for PVH/HVM guests.

Thanks, Roger.

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

Re: [PATCH 5/5] docs/pvh: document initial MTRR state

[Jan Beulich]

>>> On 14.05.18 at 18:18, <wei.liu2@citrix.com> wrote:
> On Mon, May 14, 2018 at 10:13:47AM -0600, Jan Beulich wrote:
>> >>> On 14.05.18 at 18:03, <wei.liu2@citrix.com> wrote:
>> > On Thu, May 10, 2018 at 06:15:05PM +0100, Roger Pau Monne wrote:
>> >> --- a/docs/misc/pvh.markdown
>> >> +++ b/docs/misc/pvh.markdown
>> >> @@ -92,3 +92,18 @@ event channels. Delivery of those interrupts can be configured in the same way
>> >>  as HVM guests, check xen/include/public/hvm/params.h and
>> >>  xen/include/public/hvm/hvm\_op.h for more information about available delivery
>> >>  methods.
>> >> +
>> >> +## MTRR ##
>> >> +
>> >> +### Unprivileged guests ###
>> >> +
>> >> +PVH guests are booted with the default MTRR type set to write-back and MTRR
>> >> +enabled. This allows DomUs to start with a sane MTRR state. Note that this will
>> >> +have to be revisited when pci-passthrough is added to PVH in order to set MMIO
>> >> +regions as UC.
>> > 
>> > My reading is "revisited" implies the default type will change. In fact
>> > it shouldn't. We should clarify: for ram it will remain WB, for MMIO
>> > holes it will be UC.
>> 
>> Why would changing the default late be a problem? A firmware update on
>> bare hardware might also have such an effect. The default type read from
>> the MSR must not change across the lifetime of a VM, but imo may change
>> across reboots of it.
>> 
> 
> Then setting a default here doesn't really help OS developers because
> they will always need to write code to set the correct type -- not that
> this is a big issue, but as I understand it the point here is to avoid
> that.

Hmm, my understanding of the purpose of the series was that it aims at
establishing some sane (i.e. reasonable for an OS to expect) state, instead
of a firm "this will always be this way" one. Furthermore OSes generally
shouldn't find a need to fiddle with MTRRs, provided firmware has done a
proper job setting them up.

Jan



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

Re: [PATCH 3/5] hvm/mtrr: copy hardware state for Dom0

[Jan Beulich]

>>> On 14.05.18 at 18:33, <roger.pau@citrix.com> wrote:
> On Mon, May 14, 2018 at 08:26:30AM -0600, Jan Beulich wrote:
>> >>> On 10.05.18 at 19:15, <roger.pau@citrix.com> wrote:
>> > Copy the state found on the hardware when creating a PVH Dom0. Since
>> > the memory map provided to a PVH Dom0 is based on the native one using
>> > the same set of MTRR ranges should provide Dom0 with a sane MTRR state
>> > without having to manually build it in Xen.
>> > 
>> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> > ---
>> > Cc: Jan Beulich <jbeulich@suse.com>
>> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> > ---
>> >  xen/arch/x86/hvm/mtrr.c | 23 +++++++++++++++++++++++
>> >  1 file changed, 23 insertions(+)
>> > 
>> > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
>> > index 95a3deabea..1cb000388a 100644
>> > --- a/xen/arch/x86/hvm/mtrr.c
>> > +++ b/xen/arch/x86/hvm/mtrr.c
>> > @@ -176,6 +176,29 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v)
>> >          ((uint64_t)PAT_TYPE_UC_MINUS << 48) |       /* PAT6: UC- */
>> >          ((uint64_t)PAT_TYPE_UNCACHABLE << 56);      /* PAT7: UC */
>> >  
>> > +    if ( is_hardware_domain(v->domain) )
>> > +    {
>> > +        /* Copy values from the host. */
>> > +        struct domain *d = v->domain;
>> > +        unsigned int i;
>> > +
>> > +        if ( mtrr_state.have_fixed )
>> > +            for ( i = 0; i < NUM_FIXED_MSR; i++ )
>> > +                mtrr_fix_range_msr_set(d, m, i,
>> > +                                      ((uint64_t *)mtrr_state.fixed_ranges)[i]);
>> 
>> The presence/absence of fixed range MTRRs needs to be reflected in the
>> capabilities MSR. Strictly speaking in their absence MSR access attempts to
>> the fixed range MSRs should also cause #GP, as should any attempt to
>> enable them in defType.
> 
> My intention was to always provide the fixed range MTRR capability,
> regardless of whether the underlying hardware has it or not. Then of
> course fixed ranges won't be enabled by default in the deftype MSR if
> the underlying hardware also hasn't got them enabled.

What would the result be of the OS writing to any of these MSRs, or
setting the respective enable bit?

Jan


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

Re: [PATCH 5/5] docs/pvh: document initial MTRR state

[Wei Liu]

On Tue, May 15, 2018 at 01:51:03AM -0600, Jan Beulich wrote:
> >>> On 14.05.18 at 18:18, <wei.liu2@citrix.com> wrote:
> > On Mon, May 14, 2018 at 10:13:47AM -0600, Jan Beulich wrote:
> >> >>> On 14.05.18 at 18:03, <wei.liu2@citrix.com> wrote:
> >> > On Thu, May 10, 2018 at 06:15:05PM +0100, Roger Pau Monne wrote:
> >> >> --- a/docs/misc/pvh.markdown
> >> >> +++ b/docs/misc/pvh.markdown
> >> >> @@ -92,3 +92,18 @@ event channels. Delivery of those interrupts can be configured in the same way
> >> >>  as HVM guests, check xen/include/public/hvm/params.h and
> >> >>  xen/include/public/hvm/hvm\_op.h for more information about available delivery
> >> >>  methods.
> >> >> +
> >> >> +## MTRR ##
> >> >> +
> >> >> +### Unprivileged guests ###
> >> >> +
> >> >> +PVH guests are booted with the default MTRR type set to write-back and MTRR
> >> >> +enabled. This allows DomUs to start with a sane MTRR state. Note that this will
> >> >> +have to be revisited when pci-passthrough is added to PVH in order to set MMIO
> >> >> +regions as UC.
> >> > 
> >> > My reading is "revisited" implies the default type will change. In fact
> >> > it shouldn't. We should clarify: for ram it will remain WB, for MMIO
> >> > holes it will be UC.
> >> 
> >> Why would changing the default late be a problem? A firmware update on
> >> bare hardware might also have such an effect. The default type read from
> >> the MSR must not change across the lifetime of a VM, but imo may change
> >> across reboots of it.
> >> 
> > 
> > Then setting a default here doesn't really help OS developers because
> > they will always need to write code to set the correct type -- not that
> > this is a big issue, but as I understand it the point here is to avoid
> > that.
> 
> Hmm, my understanding of the purpose of the series was that it aims at
> establishing some sane (i.e. reasonable for an OS to expect) state, instead
> of a firm "this will always be this way" one. Furthermore OSes generally
> shouldn't find a need to fiddle with MTRRs, provided firmware has done a
> proper job setting them up.

AIUI this series is the result of discussion of  [PATCH v2 1/3] xen/pvh:
enable and set default MTRR type.

It appears because the default is not sane, other pieces of software
(hvmloader, ovmf, now PVH kernel) have to set it again and again and
again.

Fundamentally I don't think we disagree with each other. If we go with
Roger's suggestion in the other sub-thread, we can ensure sane types for
RAM and MMIO and avoid debating here at all.

Wei.

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

Re: [PATCH 5/5] docs/pvh: document initial MTRR state

[Roger Pau Monné]

On Tue, May 15, 2018 at 01:51:03AM -0600, Jan Beulich wrote:
> >>> On 14.05.18 at 18:18, <wei.liu2@citrix.com> wrote:
> > On Mon, May 14, 2018 at 10:13:47AM -0600, Jan Beulich wrote:
> >> >>> On 14.05.18 at 18:03, <wei.liu2@citrix.com> wrote:
> >> > On Thu, May 10, 2018 at 06:15:05PM +0100, Roger Pau Monne wrote:
> >> >> --- a/docs/misc/pvh.markdown
> >> >> +++ b/docs/misc/pvh.markdown
> >> >> @@ -92,3 +92,18 @@ event channels. Delivery of those interrupts can be configured in the same way
> >> >>  as HVM guests, check xen/include/public/hvm/params.h and
> >> >>  xen/include/public/hvm/hvm\_op.h for more information about available delivery
> >> >>  methods.
> >> >> +
> >> >> +## MTRR ##
> >> >> +
> >> >> +### Unprivileged guests ###
> >> >> +
> >> >> +PVH guests are booted with the default MTRR type set to write-back and MTRR
> >> >> +enabled. This allows DomUs to start with a sane MTRR state. Note that this will
> >> >> +have to be revisited when pci-passthrough is added to PVH in order to set MMIO
> >> >> +regions as UC.
> >> > 
> >> > My reading is "revisited" implies the default type will change. In fact
> >> > it shouldn't. We should clarify: for ram it will remain WB, for MMIO
> >> > holes it will be UC.
> >> 
> >> Why would changing the default late be a problem? A firmware update on
> >> bare hardware might also have such an effect. The default type read from
> >> the MSR must not change across the lifetime of a VM, but imo may change
> >> across reboots of it.
> >> 
> > 
> > Then setting a default here doesn't really help OS developers because
> > they will always need to write code to set the correct type -- not that
> > this is a big issue, but as I understand it the point here is to avoid
> > that.
> 
> Hmm, my understanding of the purpose of the series was that it aims at
> establishing some sane (i.e. reasonable for an OS to expect) state, instead
> of a firm "this will always be this way" one. Furthermore OSes generally
> shouldn't find a need to fiddle with MTRRs, provided firmware has done a
> proper job setting them up.

Indeed that's the purpose. Most OSes don't really care about the
details of the MTRR setup, and they just expect RAM regions to be set
to WB and MMIO holes to UC AFAICT.

I don't think Xen has to provide any guarantee about the details of
the MTRR state, apart from stating that RAM will be WB and MMIO UC.

I can leave the text as-is, or add the paragraph suggested in another
email to clarify if the current writing is prone to misunderstanding.

Roger.

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

Re: [PATCH 3/5] hvm/mtrr: copy hardware state for Dom0

[Roger Pau Monné]

On Tue, May 15, 2018 at 01:52:43AM -0600, Jan Beulich wrote:
> >>> On 14.05.18 at 18:33, <roger.pau@citrix.com> wrote:
> > On Mon, May 14, 2018 at 08:26:30AM -0600, Jan Beulich wrote:
> >> >>> On 10.05.18 at 19:15, <roger.pau@citrix.com> wrote:
> >> > Copy the state found on the hardware when creating a PVH Dom0. Since
> >> > the memory map provided to a PVH Dom0 is based on the native one using
> >> > the same set of MTRR ranges should provide Dom0 with a sane MTRR state
> >> > without having to manually build it in Xen.
> >> > 
> >> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> > ---
> >> > Cc: Jan Beulich <jbeulich@suse.com>
> >> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >> > ---
> >> >  xen/arch/x86/hvm/mtrr.c | 23 +++++++++++++++++++++++
> >> >  1 file changed, 23 insertions(+)
> >> > 
> >> > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> >> > index 95a3deabea..1cb000388a 100644
> >> > --- a/xen/arch/x86/hvm/mtrr.c
> >> > +++ b/xen/arch/x86/hvm/mtrr.c
> >> > @@ -176,6 +176,29 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v)
> >> >          ((uint64_t)PAT_TYPE_UC_MINUS << 48) |       /* PAT6: UC- */
> >> >          ((uint64_t)PAT_TYPE_UNCACHABLE << 56);      /* PAT7: UC */
> >> >  
> >> > +    if ( is_hardware_domain(v->domain) )
> >> > +    {
> >> > +        /* Copy values from the host. */
> >> > +        struct domain *d = v->domain;
> >> > +        unsigned int i;
> >> > +
> >> > +        if ( mtrr_state.have_fixed )
> >> > +            for ( i = 0; i < NUM_FIXED_MSR; i++ )
> >> > +                mtrr_fix_range_msr_set(d, m, i,
> >> > +                                      ((uint64_t *)mtrr_state.fixed_ranges)[i]);
> >> 
> >> The presence/absence of fixed range MTRRs needs to be reflected in the
> >> capabilities MSR. Strictly speaking in their absence MSR access attempts to
> >> the fixed range MSRs should also cause #GP, as should any attempt to
> >> enable them in defType.
> > 
> > My intention was to always provide the fixed range MTRR capability,
> > regardless of whether the underlying hardware has it or not. Then of
> > course fixed ranges won't be enabled by default in the deftype MSR if
> > the underlying hardware also hasn't got them enabled.
> 
> What would the result be of the OS writing to any of these MSRs, or
> setting the respective enable bit?

Likely the cache attributes for the guest will change if it sets some
fixed ranges and enables the FE bit. But I'm not sure why is that a
problem.

If a guest plays with the MTRR ranges it must know what it's doing
anyway, and the fact that the underlying hardware has fixed range
support or not doesn't affect the changes that the guest might be
trying to perform to it's virtual MTRR registers/state.

Likewise the guest could change the variable ranges and mess up with
the types, but that's just going to affect the guest cache attributes,
not the host (Xen) ones.

Thanks, Roger.

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

Re: [PATCH 5/5] docs/pvh: document initial MTRR state

[Jan Beulich]

>>> On 15.05.18 at 10:30, <roger.pau@citrix.com> wrote:
> On Tue, May 15, 2018 at 01:51:03AM -0600, Jan Beulich wrote:
>> >>> On 14.05.18 at 18:18, <wei.liu2@citrix.com> wrote:
>> > On Mon, May 14, 2018 at 10:13:47AM -0600, Jan Beulich wrote:
>> >> >>> On 14.05.18 at 18:03, <wei.liu2@citrix.com> wrote:
>> >> > On Thu, May 10, 2018 at 06:15:05PM +0100, Roger Pau Monne wrote:
>> >> >> --- a/docs/misc/pvh.markdown
>> >> >> +++ b/docs/misc/pvh.markdown
>> >> >> @@ -92,3 +92,18 @@ event channels. Delivery of those interrupts can be 
> configured in the same way
>> >> >>  as HVM guests, check xen/include/public/hvm/params.h and
>> >> >>  xen/include/public/hvm/hvm\_op.h for more information about available 
> delivery
>> >> >>  methods.
>> >> >> +
>> >> >> +## MTRR ##
>> >> >> +
>> >> >> +### Unprivileged guests ###
>> >> >> +
>> >> >> +PVH guests are booted with the default MTRR type set to write-back and 
> MTRR
>> >> >> +enabled. This allows DomUs to start with a sane MTRR state. Note that 
> this will
>> >> >> +have to be revisited when pci-passthrough is added to PVH in order to set 
> MMIO
>> >> >> +regions as UC.
>> >> > 
>> >> > My reading is "revisited" implies the default type will change. In fact
>> >> > it shouldn't. We should clarify: for ram it will remain WB, for MMIO
>> >> > holes it will be UC.
>> >> 
>> >> Why would changing the default late be a problem? A firmware update on
>> >> bare hardware might also have such an effect. The default type read from
>> >> the MSR must not change across the lifetime of a VM, but imo may change
>> >> across reboots of it.
>> >> 
>> > 
>> > Then setting a default here doesn't really help OS developers because
>> > they will always need to write code to set the correct type -- not that
>> > this is a big issue, but as I understand it the point here is to avoid
>> > that.
>> 
>> Hmm, my understanding of the purpose of the series was that it aims at
>> establishing some sane (i.e. reasonable for an OS to expect) state, instead
>> of a firm "this will always be this way" one. Furthermore OSes generally
>> shouldn't find a need to fiddle with MTRRs, provided firmware has done a
>> proper job setting them up.
> 
> Indeed that's the purpose. Most OSes don't really care about the
> details of the MTRR setup, and they just expect RAM regions to be set
> to WB and MMIO holes to UC AFAICT.
> 
> I don't think Xen has to provide any guarantee about the details of
> the MTRR state, apart from stating that RAM will be WB and MMIO UC.
> 
> I can leave the text as-is, or add the paragraph suggested in another
> email to clarify if the current writing is prone to misunderstanding.

I indeed think the text as still visible above is not sufficiently clear (as in:
is not leaving sufficient leeway for future adjustments), so I'd prefer if
the clarification from the other sub-thread was used (as replacement or
addition).

Jan



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

Re: [PATCH 3/5] hvm/mtrr: copy hardware state for Dom0

[Jan Beulich]

>>> On 15.05.18 at 10:35, <roger.pau@citrix.com> wrote:
> On Tue, May 15, 2018 at 01:52:43AM -0600, Jan Beulich wrote:
>> >>> On 14.05.18 at 18:33, <roger.pau@citrix.com> wrote:
>> > On Mon, May 14, 2018 at 08:26:30AM -0600, Jan Beulich wrote:
>> >> >>> On 10.05.18 at 19:15, <roger.pau@citrix.com> wrote:
>> >> > Copy the state found on the hardware when creating a PVH Dom0. Since
>> >> > the memory map provided to a PVH Dom0 is based on the native one using
>> >> > the same set of MTRR ranges should provide Dom0 with a sane MTRR state
>> >> > without having to manually build it in Xen.
>> >> > 
>> >> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> >> > ---
>> >> > Cc: Jan Beulich <jbeulich@suse.com>
>> >> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> >> > ---
>> >> >  xen/arch/x86/hvm/mtrr.c | 23 +++++++++++++++++++++++
>> >> >  1 file changed, 23 insertions(+)
>> >> > 
>> >> > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
>> >> > index 95a3deabea..1cb000388a 100644
>> >> > --- a/xen/arch/x86/hvm/mtrr.c
>> >> > +++ b/xen/arch/x86/hvm/mtrr.c
>> >> > @@ -176,6 +176,29 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v)
>> >> >          ((uint64_t)PAT_TYPE_UC_MINUS << 48) |       /* PAT6: UC- */
>> >> >          ((uint64_t)PAT_TYPE_UNCACHABLE << 56);      /* PAT7: UC */
>> >> >  
>> >> > +    if ( is_hardware_domain(v->domain) )
>> >> > +    {
>> >> > +        /* Copy values from the host. */
>> >> > +        struct domain *d = v->domain;
>> >> > +        unsigned int i;
>> >> > +
>> >> > +        if ( mtrr_state.have_fixed )
>> >> > +            for ( i = 0; i < NUM_FIXED_MSR; i++ )
>> >> > +                mtrr_fix_range_msr_set(d, m, i,
>> >> > +                                      ((uint64_t *)mtrr_state.fixed_ranges)[i]);
>> >> 
>> >> The presence/absence of fixed range MTRRs needs to be reflected in the
>> >> capabilities MSR. Strictly speaking in their absence MSR access attempts to
>> >> the fixed range MSRs should also cause #GP, as should any attempt to
>> >> enable them in defType.
>> > 
>> > My intention was to always provide the fixed range MTRR capability,
>> > regardless of whether the underlying hardware has it or not. Then of
>> > course fixed ranges won't be enabled by default in the deftype MSR if
>> > the underlying hardware also hasn't got them enabled.
>> 
>> What would the result be of the OS writing to any of these MSRs, or
>> setting the respective enable bit?
> 
> Likely the cache attributes for the guest will change if it sets some
> fixed ranges and enables the FE bit. But I'm not sure why is that a
> problem.

"The guest" being Dom0 here, don't forget. I simply don't see how you
would properly mimic the behavior without there actually being fixed
range MTRRs. Plus it contradicts the patch description.

Jan

> If a guest plays with the MTRR ranges it must know what it's doing
> anyway, and the fact that the underlying hardware has fixed range
> support or not doesn't affect the changes that the guest might be
> trying to perform to it's virtual MTRR registers/state.
> 
> Likewise the guest could change the variable ranges and mess up with
> the types, but that's just going to affect the guest cache attributes,
> not the host (Xen) ones.
> 
> Thanks, Roger.


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

Re: [PATCH 3/5] hvm/mtrr: copy hardware state for Dom0

[Roger Pau Monné]

On Tue, May 15, 2018 at 02:48:16AM -0600, Jan Beulich wrote:
> >>> On 15.05.18 at 10:35, <roger.pau@citrix.com> wrote:
> > On Tue, May 15, 2018 at 01:52:43AM -0600, Jan Beulich wrote:
> >> >>> On 14.05.18 at 18:33, <roger.pau@citrix.com> wrote:
> >> > On Mon, May 14, 2018 at 08:26:30AM -0600, Jan Beulich wrote:
> >> >> >>> On 10.05.18 at 19:15, <roger.pau@citrix.com> wrote:
> >> >> > Copy the state found on the hardware when creating a PVH Dom0. Since
> >> >> > the memory map provided to a PVH Dom0 is based on the native one using
> >> >> > the same set of MTRR ranges should provide Dom0 with a sane MTRR state
> >> >> > without having to manually build it in Xen.
> >> >> > 
> >> >> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> >> > ---
> >> >> > Cc: Jan Beulich <jbeulich@suse.com>
> >> >> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >> >> > ---
> >> >> >  xen/arch/x86/hvm/mtrr.c | 23 +++++++++++++++++++++++
> >> >> >  1 file changed, 23 insertions(+)
> >> >> > 
> >> >> > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> >> >> > index 95a3deabea..1cb000388a 100644
> >> >> > --- a/xen/arch/x86/hvm/mtrr.c
> >> >> > +++ b/xen/arch/x86/hvm/mtrr.c
> >> >> > @@ -176,6 +176,29 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v)
> >> >> >          ((uint64_t)PAT_TYPE_UC_MINUS << 48) |       /* PAT6: UC- */
> >> >> >          ((uint64_t)PAT_TYPE_UNCACHABLE << 56);      /* PAT7: UC */
> >> >> >  
> >> >> > +    if ( is_hardware_domain(v->domain) )
> >> >> > +    {
> >> >> > +        /* Copy values from the host. */
> >> >> > +        struct domain *d = v->domain;
> >> >> > +        unsigned int i;
> >> >> > +
> >> >> > +        if ( mtrr_state.have_fixed )
> >> >> > +            for ( i = 0; i < NUM_FIXED_MSR; i++ )
> >> >> > +                mtrr_fix_range_msr_set(d, m, i,
> >> >> > +                                      ((uint64_t *)mtrr_state.fixed_ranges)[i]);
> >> >> 
> >> >> The presence/absence of fixed range MTRRs needs to be reflected in the
> >> >> capabilities MSR. Strictly speaking in their absence MSR access attempts to
> >> >> the fixed range MSRs should also cause #GP, as should any attempt to
> >> >> enable them in defType.
> >> > 
> >> > My intention was to always provide the fixed range MTRR capability,
> >> > regardless of whether the underlying hardware has it or not. Then of
> >> > course fixed ranges won't be enabled by default in the deftype MSR if
> >> > the underlying hardware also hasn't got them enabled.
> >> 
> >> What would the result be of the OS writing to any of these MSRs, or
> >> setting the respective enable bit?
> > 
> > Likely the cache attributes for the guest will change if it sets some
> > fixed ranges and enables the FE bit. But I'm not sure why is that a
> > problem.
> 
> "The guest" being Dom0 here, don't forget. I simply don't see how you
> would properly mimic the behavior without there actually being fixed
> range MTRRs. Plus it contradicts the patch description.

Please bear with me.

The reason of this patchset is to provide PVH Dom0 with a sane initial
MTRR state, not to allow a PVH Dom0 to set the host MTRR state
directly.

So the fact that the underlying hardware doesn't have support for
fixed MTRR ranges shouldn't affect Xen's capability to provide such
feature to Dom0.

I see no reason to allow Dom0 to directly control the host MTRR
values. A PVH Dom0 has it's own physical memory map and can set
whatever cache attributes it wishes without affecting the host MTRR
types.

Thanks, Roger.

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

Re: [PATCH 3/5] hvm/mtrr: copy hardware state for Dom0

[Jan Beulich]

>>> On 15.05.18 at 11:16, <roger.pau@citrix.com> wrote:
> On Tue, May 15, 2018 at 02:48:16AM -0600, Jan Beulich wrote:
>> >>> On 15.05.18 at 10:35, <roger.pau@citrix.com> wrote:
>> > On Tue, May 15, 2018 at 01:52:43AM -0600, Jan Beulich wrote:
>> >> >>> On 14.05.18 at 18:33, <roger.pau@citrix.com> wrote:
>> >> > On Mon, May 14, 2018 at 08:26:30AM -0600, Jan Beulich wrote:
>> >> >> >>> On 10.05.18 at 19:15, <roger.pau@citrix.com> wrote:
>> >> >> > Copy the state found on the hardware when creating a PVH Dom0. Since
>> >> >> > the memory map provided to a PVH Dom0 is based on the native one using
>> >> >> > the same set of MTRR ranges should provide Dom0 with a sane MTRR state
>> >> >> > without having to manually build it in Xen.
>> >> >> > 
>> >> >> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> >> >> > ---
>> >> >> > Cc: Jan Beulich <jbeulich@suse.com>
>> >> >> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> >> >> > ---
>> >> >> >  xen/arch/x86/hvm/mtrr.c | 23 +++++++++++++++++++++++
>> >> >> >  1 file changed, 23 insertions(+)
>> >> >> > 
>> >> >> > diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
>> >> >> > index 95a3deabea..1cb000388a 100644
>> >> >> > --- a/xen/arch/x86/hvm/mtrr.c
>> >> >> > +++ b/xen/arch/x86/hvm/mtrr.c
>> >> >> > @@ -176,6 +176,29 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v)
>> >> >> >          ((uint64_t)PAT_TYPE_UC_MINUS << 48) |       /* PAT6: UC- */
>> >> >> >          ((uint64_t)PAT_TYPE_UNCACHABLE << 56);      /* PAT7: UC */
>> >> >> >  
>> >> >> > +    if ( is_hardware_domain(v->domain) )
>> >> >> > +    {
>> >> >> > +        /* Copy values from the host. */
>> >> >> > +        struct domain *d = v->domain;
>> >> >> > +        unsigned int i;
>> >> >> > +
>> >> >> > +        if ( mtrr_state.have_fixed )
>> >> >> > +            for ( i = 0; i < NUM_FIXED_MSR; i++ )
>> >> >> > +                mtrr_fix_range_msr_set(d, m, i,
>> >> >> > +                                      ((uint64_t *)mtrr_state.fixed_ranges)[i]);
>> >> >> 
>> >> >> The presence/absence of fixed range MTRRs needs to be reflected in the
>> >> >> capabilities MSR. Strictly speaking in their absence MSR access attempts to
>> >> >> the fixed range MSRs should also cause #GP, as should any attempt to
>> >> >> enable them in defType.
>> >> > 
>> >> > My intention was to always provide the fixed range MTRR capability,
>> >> > regardless of whether the underlying hardware has it or not. Then of
>> >> > course fixed ranges won't be enabled by default in the deftype MSR if
>> >> > the underlying hardware also hasn't got them enabled.
>> >> 
>> >> What would the result be of the OS writing to any of these MSRs, or
>> >> setting the respective enable bit?
>> > 
>> > Likely the cache attributes for the guest will change if it sets some
>> > fixed ranges and enables the FE bit. But I'm not sure why is that a
>> > problem.
>> 
>> "The guest" being Dom0 here, don't forget. I simply don't see how you
>> would properly mimic the behavior without there actually being fixed
>> range MTRRs. Plus it contradicts the patch description.
> 
> Please bear with me.
> 
> The reason of this patchset is to provide PVH Dom0 with a sane initial
> MTRR state, not to allow a PVH Dom0 to set the host MTRR state
> directly.
> 
> So the fact that the underlying hardware doesn't have support for
> fixed MTRR ranges shouldn't affect Xen's capability to provide such
> feature to Dom0.
> 
> I see no reason to allow Dom0 to directly control the host MTRR
> values. A PVH Dom0 has it's own physical memory map and can set
> whatever cache attributes it wishes without affecting the host MTRR
> types.

Oh, right, I've been confused by the mix of copying of host state and
and leaving untouched of virtual capabilities. I'm sorry for the noise.

Jan


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

Re: [PATCH 4/5] libxc/pvh: set default MTRR type to write-back

[Wei Liu]

On Thu, May 10, 2018 at 06:15:04PM +0100, Roger Pau Monne wrote:
> And enable MTRR. This allows to provide a sane initial MTRR state for
> PVH DomUs. This will have to be expanded when pci-passthrough support
> is added to PVH guests, so that MMIO regions of devices are set as
> UC.
> 
> Note that initial MTRR setup is done by hvmloader for HVM guests,
> that's not used by PVH guests.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ----
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  tools/libxc/xc_dom_x86.c | 44 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index e33a28847d..d28ff4d7e9 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -53,6 +53,9 @@
>  #define X86_CR0_PE 0x01
>  #define X86_CR0_ET 0x10
>  
> +#define MTRR_TYPE_WRBACK     6
> +#define MTRR_DEF_TYPE_ENABLE (1u << 11)
> +
>  #define SPECIALPAGE_PAGING   0
>  #define SPECIALPAGE_ACCESS   1
>  #define SPECIALPAGE_SHARING  2
> @@ -931,6 +934,20 @@ static int vcpu_x86_64(struct xc_dom_image *dom)
>      return rc;
>  }
>  
> +const static void *hvm_get_save_record(const void *ctx, unsigned int type,
> +                                       unsigned int instance)
> +{
> +    const struct hvm_save_descriptor *header;
> +
> +    for ( header = ctx;
> +          header->typecode != HVM_SAVE_CODE(END);
> +          ctx += sizeof(*header) + header->length, header = ctx )
> +        if ( header->typecode == type && header->instance == instance )
> +            return ctx + sizeof(*header);
> +
> +    return NULL;
> +}
> +
>  static int vcpu_hvm(struct xc_dom_image *dom)
>  {
>      struct {
> @@ -938,9 +955,12 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>          HVM_SAVE_TYPE(HEADER) header;
>          struct hvm_save_descriptor cpu_d;
>          HVM_SAVE_TYPE(CPU) cpu;
> +        struct hvm_save_descriptor mtrr_d;
> +        HVM_SAVE_TYPE(MTRR) mtrr;
>          struct hvm_save_descriptor end_d;
>          HVM_SAVE_TYPE(END) end;
>      } bsp_ctx;
> +    const HVM_SAVE_TYPE(MTRR) *mtrr_record;
>      uint8_t *full_ctx = NULL;
>      int rc;
>  
> @@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>      if ( dom->start_info_seg.pfn )
>          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
>  
> +    /* Set the MTRR. */
> +    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
> +    bsp_ctx.mtrr_d.instance = 0;
> +    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
> +
> +    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
> +    if ( !mtrr_record )
> +    {
> +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> +                     "%s: unable to get MTRR save record", __func__);
> +        goto out;
> +    }
> +
> +    memcpy(&bsp_ctx.mtrr, mtrr_record, sizeof(bsp_ctx.mtrr));
> +
> +    /* TODO: maybe this should be a firmware option instead? */
> +    if ( !dom->device_model )
> +        /*
> +         * Enable MTRR, set default type to WB.
> +         * TODO: add MMIO areas as UC when passthrough is supported.
> +         */
> +        bsp_ctx.mtrr.msr_mtrr_def_type = MTRR_TYPE_WRBACK |
> +                                         MTRR_DEF_TYPE_ENABLE;
> +

Hrm... I'm not entirely happy with this in toolstack code but there
doesn't seem to be a better way to do this at the moment, considering
hypervisor doesn't distinguish HVM and PVH guests.

Anyway, the code looks correct to me. I would rather see something in
hypervisor to deal with this, but I won't object to this either.

Wei.

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

Re: [PATCH 4/5] libxc/pvh: set default MTRR type to write-back

[Jan Beulich]

>>> On 15.05.18 at 13:43, <wei.liu2@citrix.com> wrote:
> On Thu, May 10, 2018 at 06:15:04PM +0100, Roger Pau Monne wrote:
>> @@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>>      if ( dom->start_info_seg.pfn )
>>          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
>>  
>> +    /* Set the MTRR. */
>> +    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
>> +    bsp_ctx.mtrr_d.instance = 0;
>> +    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
>> +
>> +    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
>> +    if ( !mtrr_record )
>> +    {
>> +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
>> +                     "%s: unable to get MTRR save record", __func__);
>> +        goto out;
>> +    }
>> +
>> +    memcpy(&bsp_ctx.mtrr, mtrr_record, sizeof(bsp_ctx.mtrr));
>> +
>> +    /* TODO: maybe this should be a firmware option instead? */
>> +    if ( !dom->device_model )
>> +        /*
>> +         * Enable MTRR, set default type to WB.
>> +         * TODO: add MMIO areas as UC when passthrough is supported.
>> +         */
>> +        bsp_ctx.mtrr.msr_mtrr_def_type = MTRR_TYPE_WRBACK |
>> +                                         MTRR_DEF_TYPE_ENABLE;
>> +
> 
> Hrm... I'm not entirely happy with this in toolstack code but there
> doesn't seem to be a better way to do this at the moment, considering
> hypervisor doesn't distinguish HVM and PVH guests.
> 
> Anyway, the code looks correct to me. I would rather see something in
> hypervisor to deal with this, but I won't object to this either.

But doing it in the hypervisor would be a layering violation imo: The
hypervisor should set MTRR state to power-on / reset defaults, which
it does. It's firmware which is supposed to adapt their values to actual
system characteristics (RAM and MMIO ranges), and libxc has to play
the role of firmware here short of there being any in the guest itself.

Jan



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