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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ----
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel