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

2018-05-15 Thread Jan Beulich 
>>> On 15.05.18 at 13:43,  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(_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

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

2018-05-15 Thread 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é 
> 
> Cc: Ian Jackson 
> Cc: Wei Liu 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> ---
>  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(_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: [Xen-devel] [PATCH 4/5] libxc/pvh: set default MTRR type to write-back

2018-05-14 Thread 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: [Xen-devel] [PATCH 4/5] libxc/pvh: set default MTRR type to write-back

2018-05-14 Thread 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: [Xen-devel] [PATCH 4/5] libxc/pvh: set default MTRR type to write-back

2018-05-14 Thread 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: [Xen-devel] [PATCH 4/5] libxc/pvh: set default MTRR type to write-back

2018-05-14 Thread 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: [Xen-devel] [PATCH 4/5] libxc/pvh: set default MTRR type to write-back

2018-05-14 Thread 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

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

2018-05-10 Thread 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é 

Cc: Ian Jackson 
Cc: Wei Liu 
Cc: Jan Beulich 
Cc: Andrew Cooper 
---
 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(_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