[PATCH v2 0/3] xen/pvh: Dom0 support

[PATCH v2 0/3] xen/pvh: Dom0 support

[Roger Pau Monne]

Hello,

The following patches enable booting a PVH Dom0. So far I've only tested
them in my test box, so I expect more issues will show up as this gets
further testing. In any case, this is the bare minimum to get a PVH Dom0
working.

In order to test them Xen 4.11 RC or plain master/staging should be
used. Compile a Linux kernel with this patches and PVH support and add
dom0=pvh to the Xen command line. The following message will be
displayed during Dom0 boot by the Linux kernel if PVH mode is used:

"Booting paravirtualized kernel on Xen PVH"

Thanks, Roger.

Roger Pau Monne (3):
  xen/pvh: enable and set default MTRR type
  xen/store: do not store local values in xen_start_info
  xen: share start flags between PV and PVH

 arch/arm/xen/enlighten.c          | 7 ++++---
 arch/x86/xen/enlighten.c          | 7 +++++++
 arch/x86/xen/enlighten_pv.c       | 1 +
 arch/x86/xen/enlighten_pvh.c      | 4 ++++
 drivers/xen/xenbus/xenbus_probe.c | 5 ++---
 include/xen/xen.h                 | 4 +++-
 6 files changed, 21 insertions(+), 7 deletions(-)

-- 
2.17.0

[PATCH v2 1/3] xen/pvh: enable and set default MTRR type

[Roger Pau Monne]

On PVH MTRR is not initialized by the firmware (because there's no
firmware), so the kernel is started with MTRR disabled which means all
memory accesses are UC.

So far there have been no issues (ie: slowdowns) caused by this
because PVH only supported DomU mode without passed-through devices,
so Xen was using WB as the default memory type instead of UC.

Fix this by enabling MTRR and setting the default type to WB. Linux
will use PAT to set the actual memory cache attributes.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: xen-devel@lists.xenproject.org
---
 arch/x86/xen/enlighten_pvh.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
index aa1c6a6831a9..e039d1809809 100644
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -6,6 +6,7 @@
 #include <asm/io_apic.h>
 #include <asm/hypervisor.h>
 #include <asm/e820/api.h>
+#include <asm/mtrr.h>
 #include <asm/x86_init.h>
 
 #include <asm/xen/interface.h>
@@ -98,6 +99,8 @@ void __init xen_prepare_pvh(void)
 
 	xen_pvh = 1;
 
+	wrmsr_safe(MSR_MTRRdefType, 0x800 | MTRR_TYPE_WRBACK, 0);
+
 	msr = cpuid_ebx(xen_cpuid_base() + 2);
 	pfn = __pa(hypercall_page);
 	wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));
-- 
2.17.0

[PATCH v2 2/3] xen/store: do not store local values in xen_start_info

[Roger Pau Monne]

There's no need to store the xenstore page or event channel in
xen_start_info if they are locally initialized.

This also fixes PVH local xenstore initialization due to the lack of
xen_start_info in that case.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: xen-devel@lists.xenproject.org
---
 drivers/xen/xenbus/xenbus_probe.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index ec9eb4fba59c..f2088838f690 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -710,7 +710,7 @@ static int __init xenstored_local_init(void)
 	if (!page)
 		goto out_err;
 
-	xen_store_gfn = xen_start_info->store_mfn = virt_to_gfn((void *)page);
+	xen_store_gfn = virt_to_gfn((void *)page);
 
 	/* Next allocate a local port which xenstored can bind to */
 	alloc_unbound.dom        = DOMID_SELF;
@@ -722,8 +722,7 @@ static int __init xenstored_local_init(void)
 		goto out_err;
 
 	BUG_ON(err);
-	xen_store_evtchn = xen_start_info->store_evtchn =
-		alloc_unbound.port;
+	xen_store_evtchn = alloc_unbound.port;
 
 	return 0;
 
-- 
2.17.0

[PATCH v2 3/3] xen: share start flags between PV and PVH

[Roger Pau Monne]

Use a global variable to store the start flags for both PV and PVH.
This allows the xen_initial_domain macro to work properly on PVH.

Note that ARM is also switched to use the new variable.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.org
---
Changes since v1:
 - Switch ARM to also use the new xen_start_flags.
---
 arch/arm/xen/enlighten.c     | 7 ++++---
 arch/x86/xen/enlighten.c     | 7 +++++++
 arch/x86/xen/enlighten_pv.c  | 1 +
 arch/x86/xen/enlighten_pvh.c | 1 +
 include/xen/xen.h            | 4 +++-
 5 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index ba7f4c8f5c3e..a99d5edf7bdf 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -59,6 +59,9 @@ struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
 
 static __read_mostly unsigned int xen_events_irq;
 
+uint32_t xen_start_flags;
+EXPORT_SYMBOL(xen_start_flags);
+
 int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
 			       unsigned long addr,
 			       xen_pfn_t *gfn, int nr,
@@ -282,9 +285,7 @@ void __init xen_early_init(void)
 	xen_setup_features();
 
 	if (xen_feature(XENFEAT_dom0))
-		xen_start_info->flags |= SIF_INITDOMAIN|SIF_PRIVILEGED;
-	else
-		xen_start_info->flags &= ~(SIF_INITDOMAIN|SIF_PRIVILEGED);
+		xen_start_flags |= SIF_INITDOMAIN|SIF_PRIVILEGED;
 
 	if (!console_set_on_cmdline && !xen_initial_domain())
 		add_preferred_console("hvc", 0, NULL);
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index c9081c6671f0..3b5318505c69 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -64,6 +64,13 @@ struct shared_info xen_dummy_shared_info;
 __read_mostly int xen_have_vector_callback;
 EXPORT_SYMBOL_GPL(xen_have_vector_callback);
 
+/*
+ * NB: needs to live in .data because it's used by xen_prepare_pvh which runs
+ * before clearing the bss.
+ */
+uint32_t xen_start_flags __attribute__((section(".data"))) = 0;
+EXPORT_SYMBOL(xen_start_flags);
+
 /*
  * Point at some empty memory to start with. We map the real shared_info
  * page as soon as fixmap is up and running.
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index c36d23aa6c35..04a6914b8b85 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1227,6 +1227,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
 		return;
 
 	xen_domain_type = XEN_PV_DOMAIN;
+	xen_start_flags = xen_start_info->flags;
 
 	xen_setup_features();
 
diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
index e039d1809809..2653eb9b5dd8 100644
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -98,6 +98,7 @@ void __init xen_prepare_pvh(void)
 	}
 
 	xen_pvh = 1;
+	xen_start_flags = pvh_start_info.flags;
 
 	wrmsr_safe(MSR_MTRRdefType, 0x800 | MTRR_TYPE_WRBACK, 0);
 
diff --git a/include/xen/xen.h b/include/xen/xen.h
index 9d4340c907d1..15fa01c50a53 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -29,8 +29,10 @@ extern bool xen_pvh;
 #include <xen/interface/xen.h>
 #include <asm/xen/hypervisor.h>
 
+extern uint32_t xen_start_flags;
+
 #define xen_initial_domain()	(xen_domain() && \
-				 xen_start_info && xen_start_info->flags & SIF_INITDOMAIN)
+				 (xen_start_flags & SIF_INITDOMAIN))
 #else  /* !CONFIG_XEN_DOM0 */
 #define xen_initial_domain()	(0)
 #endif	/* CONFIG_XEN_DOM0 */
-- 
2.17.0

Re: [Xen-devel] [PATCH v2 1/3] xen/pvh: enable and set default MTRR type

[Andrew Cooper]

On 09/05/18 11:21, Roger Pau Monne wrote:
> On PVH MTRR is not initialized by the firmware (because there's no
> firmware), so the kernel is started with MTRR disabled which means all
> memory accesses are UC.
>
> So far there have been no issues (ie: slowdowns) caused by this
> because PVH only supported DomU mode without passed-through devices,
> so Xen was using WB as the default memory type instead of UC.
>
> Fix this by enabling MTRR and setting the default type to WB. Linux
> will use PAT to set the actual memory cache attributes.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I'd argue that this is a bug in PVH starting state.

Do you know what mechanism is used to bodge things to WB in the first
place?  I'm not sure that setting the default MTRR type is going to be a
clever idea in hindsight when we come to doing PCI Passthrough support.

~Andrew

Re: [Xen-devel] [PATCH v2 1/3] xen/pvh: enable and set default MTRR type

[Roger Pau Monné]

On Wed, May 09, 2018 at 11:56:40AM +0100, Andrew Cooper wrote:
> On 09/05/18 11:21, Roger Pau Monne wrote:
> > On PVH MTRR is not initialized by the firmware (because there's no
> > firmware), so the kernel is started with MTRR disabled which means all
> > memory accesses are UC.
> >
> > So far there have been no issues (ie: slowdowns) caused by this
> > because PVH only supported DomU mode without passed-through devices,
> > so Xen was using WB as the default memory type instead of UC.
> >
> > Fix this by enabling MTRR and setting the default type to WB. Linux
> > will use PAT to set the actual memory cache attributes.
> >
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I'd argue that this is a bug in PVH starting state.

Do you mean that MTRR should be setup before starting the guest?

> Do you know what mechanism is used to bodge things to WB in the first
> place?

If you mean when passthorugh is not used (ie: no IOMMU), then it's at
epte_get_entry_emt, grep for need_iommu(d) (line ~801).

> I'm not sure that setting the default MTRR type is going to be a
> clever idea in hindsight when we come to doing PCI Passthrough support.

Setting the default type to WB is also set by hvmloader, it's just
that hvmloader also sets some of the fixed and variable ranges to UC
in order to cover the iomem areas.

The expectations when doing pci-passthrough is that the guest will
always use paging and PAT in order to set the appropriate cache
attributes, or else the guest itself will have to program the UC MTRR
ranges, I admit that's not very nice however.

What about enabling the default MTRR type and setting it to WB in the
toolstack for PVH? IMO doing it Xen itself would be wrong.

Thanks, Roger.

Re: [Xen-devel] [PATCH v2 1/3] xen/pvh: enable and set default MTRR type

[Roger Pau Monné]

On Wed, May 09, 2018 at 12:30:16PM +0100, Roger Pau Monné wrote:
> On Wed, May 09, 2018 at 11:56:40AM +0100, Andrew Cooper wrote:
> > On 09/05/18 11:21, Roger Pau Monne wrote:
> > I'm not sure that setting the default MTRR type is going to be a
> > clever idea in hindsight when we come to doing PCI Passthrough support.
> 
> Setting the default type to WB is also set by hvmloader, it's just
> that hvmloader also sets some of the fixed and variable ranges to UC
> in order to cover the iomem areas.
> 
> The expectations when doing pci-passthrough is that the guest will
> always use paging and PAT in order to set the appropriate cache
> attributes, or else the guest itself will have to program the UC MTRR
> ranges, I admit that's not very nice however.
> 
> What about enabling the default MTRR type and setting it to WB in the
> toolstack for PVH? IMO doing it Xen itself would be wrong.

I have the following patch to set the default MTRR type, but I think
if we go down this road then we will also have to set UC MTRRs for
MMIO areas, which again seems fine to me.

---8<---
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index e33a28847d..3cb1a1720f 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -938,6 +938,8 @@ 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;
@@ -1014,6 +1016,15 @@ 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);
+    /* XXX: maybe this should be a firmware option instead? */
+    if ( !dom->device_model )
+        /* Enable MTRR (bit 11) and set the default type to WB (6). */
+        bsp_ctx.mtrr.msr_mtrr_def_type = (1u << 11) | 6;
+
     /* Set the end descriptor. */
     bsp_ctx.end_d.typecode = HVM_SAVE_CODE(END);
     bsp_ctx.end_d.instance = 0;

Re: [PATCH v2 3/3] xen: share start flags between PV and PVH

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

Hi Roger,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.17-rc4 next-20180509]
[cannot apply to xen-tip/linux-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Roger-Pau-Monne/xen-pvh-Dom0-support/20180509-222240
config: x86_64-randconfig-x009-201818 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   arch/x86/xen/enlighten_pvh.c: In function 'xen_prepare_pvh':
>> arch/x86/xen/enlighten_pvh.c:101:2: error: 'xen_start_flags' undeclared (first use in this function); did you mean 'xen_start_info'?
     xen_start_flags = pvh_start_info.flags;
     ^~~~~~~~~~~~~~~
     xen_start_info
   arch/x86/xen/enlighten_pvh.c:101:2: note: each undeclared identifier is reported only once for each function it appears in

vim +101 arch/x86/xen/enlighten_pvh.c

    84	
    85	/*
    86	 * This routine (and those that it might call) should not use
    87	 * anything that lives in .bss since that segment will be cleared later.
    88	 */
    89	void __init xen_prepare_pvh(void)
    90	{
    91		u32 msr;
    92		u64 pfn;
    93	
    94		if (pvh_start_info.magic != XEN_HVM_START_MAGIC_VALUE) {
    95			xen_raw_printk("Error: Unexpected magic value (0x%08x)\n",
    96					pvh_start_info.magic);
    97			BUG();
    98		}
    99	
   100		xen_pvh = 1;
 > 101		xen_start_flags = pvh_start_info.flags;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29793 bytes --]

Re: [PATCH v2 3/3] xen: share start flags between PV and PVH

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3933 bytes --]

On Wed, 9 May 2018, Roger Pau Monne wrote:
> Use a global variable to store the start flags for both PV and PVH.
> This allows the xen_initial_domain macro to work properly on PVH.
> 
> Note that ARM is also switched to use the new variable.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: xen-devel@lists.xenproject.org
> ---
> Changes since v1:
>  - Switch ARM to also use the new xen_start_flags.

The ARM side looks good


> ---
>  arch/arm/xen/enlighten.c     | 7 ++++---
>  arch/x86/xen/enlighten.c     | 7 +++++++
>  arch/x86/xen/enlighten_pv.c  | 1 +
>  arch/x86/xen/enlighten_pvh.c | 1 +
>  include/xen/xen.h            | 4 +++-
>  5 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index ba7f4c8f5c3e..a99d5edf7bdf 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -59,6 +59,9 @@ struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
>  
>  static __read_mostly unsigned int xen_events_irq;
>  
> +uint32_t xen_start_flags;
> +EXPORT_SYMBOL(xen_start_flags);
> +
>  int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
>  			       unsigned long addr,
>  			       xen_pfn_t *gfn, int nr,
> @@ -282,9 +285,7 @@ void __init xen_early_init(void)
>  	xen_setup_features();
>  
>  	if (xen_feature(XENFEAT_dom0))
> -		xen_start_info->flags |= SIF_INITDOMAIN|SIF_PRIVILEGED;
> -	else
> -		xen_start_info->flags &= ~(SIF_INITDOMAIN|SIF_PRIVILEGED);
> +		xen_start_flags |= SIF_INITDOMAIN|SIF_PRIVILEGED;
>  
>  	if (!console_set_on_cmdline && !xen_initial_domain())
>  		add_preferred_console("hvc", 0, NULL);
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index c9081c6671f0..3b5318505c69 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -64,6 +64,13 @@ struct shared_info xen_dummy_shared_info;
>  __read_mostly int xen_have_vector_callback;
>  EXPORT_SYMBOL_GPL(xen_have_vector_callback);
>  
> +/*
> + * NB: needs to live in .data because it's used by xen_prepare_pvh which runs
> + * before clearing the bss.
> + */
> +uint32_t xen_start_flags __attribute__((section(".data"))) = 0;
> +EXPORT_SYMBOL(xen_start_flags);
> +
>  /*
>   * Point at some empty memory to start with. We map the real shared_info
>   * page as soon as fixmap is up and running.
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index c36d23aa6c35..04a6914b8b85 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -1227,6 +1227,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
>  		return;
>  
>  	xen_domain_type = XEN_PV_DOMAIN;
> +	xen_start_flags = xen_start_info->flags;
>  
>  	xen_setup_features();
>  
> diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
> index e039d1809809..2653eb9b5dd8 100644
> --- a/arch/x86/xen/enlighten_pvh.c
> +++ b/arch/x86/xen/enlighten_pvh.c
> @@ -98,6 +98,7 @@ void __init xen_prepare_pvh(void)
>  	}
>  
>  	xen_pvh = 1;
> +	xen_start_flags = pvh_start_info.flags;
>  
>  	wrmsr_safe(MSR_MTRRdefType, 0x800 | MTRR_TYPE_WRBACK, 0);
>  
> diff --git a/include/xen/xen.h b/include/xen/xen.h
> index 9d4340c907d1..15fa01c50a53 100644
> --- a/include/xen/xen.h
> +++ b/include/xen/xen.h
> @@ -29,8 +29,10 @@ extern bool xen_pvh;
>  #include <xen/interface/xen.h>
>  #include <asm/xen/hypervisor.h>
>  
> +extern uint32_t xen_start_flags;
> +
>  #define xen_initial_domain()	(xen_domain() && \
> -				 xen_start_info && xen_start_info->flags & SIF_INITDOMAIN)
> +				 (xen_start_flags & SIF_INITDOMAIN))
>  #else  /* !CONFIG_XEN_DOM0 */
>  #define xen_initial_domain()	(0)
>  #endif	/* CONFIG_XEN_DOM0 */
> -- 
> 2.17.0
> 

Re: [Xen-devel] [PATCH v2 1/3] xen/pvh: enable and set default MTRR type

[Wei Liu]

On Wed, May 09, 2018 at 04:11:39PM +0100, Roger Pau Monné wrote:
> On Wed, May 09, 2018 at 12:30:16PM +0100, Roger Pau Monné wrote:
> > On Wed, May 09, 2018 at 11:56:40AM +0100, Andrew Cooper wrote:
> > > On 09/05/18 11:21, Roger Pau Monne wrote:
> > > I'm not sure that setting the default MTRR type is going to be a
> > > clever idea in hindsight when we come to doing PCI Passthrough support.
> > 
> > Setting the default type to WB is also set by hvmloader, it's just
> > that hvmloader also sets some of the fixed and variable ranges to UC
> > in order to cover the iomem areas.
> > 
> > The expectations when doing pci-passthrough is that the guest will
> > always use paging and PAT in order to set the appropriate cache
> > attributes, or else the guest itself will have to program the UC MTRR
> > ranges, I admit that's not very nice however.
> > 
> > What about enabling the default MTRR type and setting it to WB in the
> > toolstack for PVH? IMO doing it Xen itself would be wrong.
> 
> I have the following patch to set the default MTRR type, but I think
> if we go down this road then we will also have to set UC MTRRs for
> MMIO areas, which again seems fine to me.
> 

Can you please document the default type(s) to pvh.markdown once
the issue is resolved?

Wei.

Re: [Xen-devel] [PATCH v2 1/3] xen/pvh: enable and set default MTRR type

[Juergen Gross]

On 09/05/18 17:11, Roger Pau Monné wrote:
> On Wed, May 09, 2018 at 12:30:16PM +0100, Roger Pau Monné wrote:
>> On Wed, May 09, 2018 at 11:56:40AM +0100, Andrew Cooper wrote:
>>> On 09/05/18 11:21, Roger Pau Monne wrote:
>>> I'm not sure that setting the default MTRR type is going to be a
>>> clever idea in hindsight when we come to doing PCI Passthrough support.
>>
>> Setting the default type to WB is also set by hvmloader, it's just
>> that hvmloader also sets some of the fixed and variable ranges to UC
>> in order to cover the iomem areas.
>>
>> The expectations when doing pci-passthrough is that the guest will
>> always use paging and PAT in order to set the appropriate cache
>> attributes, or else the guest itself will have to program the UC MTRR
>> ranges, I admit that's not very nice however.
>>
>> What about enabling the default MTRR type and setting it to WB in the
>> toolstack for PVH? IMO doing it Xen itself would be wrong.
> 
> I have the following patch to set the default MTRR type, but I think
> if we go down this road then we will also have to set UC MTRRs for
> MMIO areas, which again seems fine to me.

I like this route much better.


Juergen

> 
> ---8<---
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index e33a28847d..3cb1a1720f 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -938,6 +938,8 @@ 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;
> @@ -1014,6 +1016,15 @@ 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);
> +    /* XXX: maybe this should be a firmware option instead? */
> +    if ( !dom->device_model )
> +        /* Enable MTRR (bit 11) and set the default type to WB (6). */
> +        bsp_ctx.mtrr.msr_mtrr_def_type = (1u << 11) | 6;
> +
>      /* Set the end descriptor. */
>      bsp_ctx.end_d.typecode = HVM_SAVE_CODE(END);
>      bsp_ctx.end_d.instance = 0;
> 
> 

Re: [PATCH v2 2/3] xen/store: do not store local values in xen_start_info

[Juergen Gross]

On 09/05/18 12:21, Roger Pau Monne wrote:
> There's no need to store the xenstore page or event channel in
> xen_start_info if they are locally initialized.
> 
> This also fixes PVH local xenstore initialization due to the lack of
> xen_start_info in that case.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH v2 2/3] xen/store: do not store local values in xen_start_info

[Juergen Gross]

On 09/05/18 12:21, Roger Pau Monne wrote:
> There's no need to store the xenstore page or event channel in
> xen_start_info if they are locally initialized.
> 
> This also fixes PVH local xenstore initialization due to the lack of
> xen_start_info in that case.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Pushed to xen/tip.git for-linus-4.18


Juergen