RE: [Xen-devel] [PATCH] Tidy up the viridian code a little and flesh out the APIC assist MSR

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

Yep. I'm hoping to start making use of apic assist once I've got specific 
evtchn -> hvm irq mappings working. I can get windows to give me edge triggered 
interrupts so hopefully I can persuade it to skip the eoi by setting the apic 
assist bit prior to injection. Keeping track of the pfn will, of course, become 
quite important at this point :-) For now, I don't think windows reads the msr 
again after boot so losing the value across a save/restore probably doesn't 
matter much.

  Paul

> -----Original Message-----
> From: Keir Fraser [mailto:keir.xen@xxxxxxxxx]
> Sent: 15 September 2011 07:32
> To: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH] Tidy up the viridian code a little
> and flesh out the APIC assist MSR
>
> Might need a shiny new Viridian struct type for our save format,
> which can then be extended with more Viridian-y stuff as required.
>
>  -- Keir
>
> On 15/09/2011 15:19, "Paul Durrant" <Paul.Durrant@xxxxxxxxxx> wrote:
>
> > Good point. I guess I may need to explicitly save/restore the apic
> assist pfn.
> > I'll check.
> >
> >   Paul
> >
> >> -----Original Message-----
> >> From: Keir Fraser [mailto:keir.xen@xxxxxxxxx]
> >> Sent: 14 September 2011 23:38
> >> To: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxx
> >> Subject: Re: [Xen-devel] [PATCH] Tidy up the viridian code a
> little
> >> and flesh out the APIC assist MSR
> >>
> >> On 15/09/2011 06:13, "Paul Durrant" <paul.durrant@xxxxxxxxxx>
> wrote:
> >>
> >>> # HG changeset patch
> >>> # User Paul Durrant <paul.durrant@xxxxxxxxxx> # Date 1316063461
> -
> >> 3600
> >>> # Node ID a08073c5cf28ab76893102cc7a8d20bf3e5e15ae
> >>> # Parent  e90438f6e6d1585a71b18784a99c162b5d95f390
> >>> Tidy up the viridian code a little and flesh out the APIC assist
> >> MSR
> >>> handling code.
> >>>
> >>> We don't say we that handle that MSR but Windows assumes it. In
> >>> Windows 7 it just wrote to the MSR and we used to handle that
> ok.
> >>> Windows 8 also reads from the MSR so we need to keep a record of
> >> the contents.
> >>
> >> Does this work properly across save/restore?
> >>
> >>  -- Keir
> >>
> >>> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> >>>
> >>> diff -r e90438f6e6d1 -r a08073c5cf28 xen/arch/x86/hvm/viridian.c
> >>> --- a/xen/arch/x86/hvm/viridian.c Wed Sep 14 11:38:13 2011 +0100
> >>> +++ b/xen/arch/x86/hvm/viridian.c Thu Sep 15 06:11:01 2011 +0100
> >>> @@ -96,9 +96,43 @@ int cpuid_viridian_leaves(unsigned int l
> >>>      return 1;
> >>>  }
> >>>
> >>> -static void enable_hypercall_page(void)
> >>> +void dump_guest_os_id(struct domain *d)
> >>>  {
> >>> -    struct domain *d = current->domain;
> >>> +    gdprintk(XENLOG_INFO, "GUEST_OS_ID:\n");
> >>> +    gdprintk(XENLOG_INFO, "\tvendor: %x\n",
> >>> +            d-
> >>> arch.hvm_domain.viridian.guest_os_id.fields.vendor);
> >>> +    gdprintk(XENLOG_INFO, "\tos: %x\n",
> >>> +            d->arch.hvm_domain.viridian.guest_os_id.fields.os);
> >>> +    gdprintk(XENLOG_INFO, "\tmajor: %x\n",
> >>> +            d-
> >>> arch.hvm_domain.viridian.guest_os_id.fields.major);
> >>> +    gdprintk(XENLOG_INFO, "\tminor: %x\n",
> >>> +            d-
> >>> arch.hvm_domain.viridian.guest_os_id.fields.minor);
> >>> +    gdprintk(XENLOG_INFO, "\tsp: %x\n",
> >>> +            d-
> >>> arch.hvm_domain.viridian.guest_os_id.fields.service_pack);
> >>> +    gdprintk(XENLOG_INFO, "\tbuild: %x\n",
> >>> +
> >>> +d->arch.hvm_domain.viridian.guest_os_id.fields.build_number);
> >>> +}
> >>> +
> >>> +void dump_hypercall(struct domain *d) {
> >>> +    gdprintk(XENLOG_INFO, "HYPERCALL:\n");
> >>> +    gdprintk(XENLOG_INFO, "\tenabled: %x\n",
> >>> +            d-
> >>> arch.hvm_domain.viridian.hypercall_gpa.fields.enabled);
> >>> +    gdprintk(XENLOG_INFO, "\tpfn: %lx\n",
> >>> +            (unsigned
> >>> long)d->arch.hvm_domain.viridian.hypercall_gpa.fields.pfn);
> >>> +}
> >>> +
> >>> +void dump_apic_assist(struct vcpu *v) {
> >>> +    gdprintk(XENLOG_INFO, "APIC_ASSIST[%d]:\n", v->vcpu_id);
> >>> +    gdprintk(XENLOG_INFO, "\tenabled: %x\n",
> >>> +            v-
> >>> arch.hvm_vcpu.viridian.apic_assist.fields.enabled);
> >>> +    gdprintk(XENLOG_INFO, "\tpfn: %lx\n",
> >>> +            (unsigned
> >>> +long)v->arch.hvm_vcpu.viridian.apic_assist.fields.pfn);
> >>> +}
> >>> +
> >>> +static void enable_hypercall_page(struct domain *d) {
> >>>      unsigned long gmfn =
> >>> d->arch.hvm_domain.viridian.hypercall_gpa.fields.pfn;
> >>>      unsigned long mfn = gmfn_to_mfn(d, gmfn);
> >>>      uint8_t *p;
> >>> @@ -130,9 +164,43 @@ static void enable_hypercall_page(void)
> >>>      put_page_and_type(mfn_to_page(mfn));
> >>>  }
> >>>
> >>> +void initialize_apic_assist(struct vcpu *v) {
> >>> +    struct domain *d = v->domain;
> >>> +    unsigned long gmfn = v-
> >>> arch.hvm_vcpu.viridian.apic_assist.fields.pfn;
> >>> +    unsigned long mfn = gmfn_to_mfn(d, gmfn);
> >>> +    uint8_t *p;
> >>> +
> >>> +    /*
> >>> +     * We don't support the APIC assist page, and that fact is
> >> reflected in
> >>> +     * our CPUID flags. However, Newer versions of Windows have
> a
> >> bug which
> >>> +     * means that they don't recognise that, and tries to use
> the
> >> page
> >>> +     * anyway. We therefore have to fake up just enough to keep
> >>> + windows
> >>> happy.
> >>> +     *
> >>> +     * See
> >>> + http://msdn.microsoft.com/en-
> us/library/ff538657%28VS.85%29.aspx
> >>> for
> >>> +     * details of how Windows uses the page.
> >>> +     */
> >>> +
> >>> +    if ( !mfn_valid(mfn) ||
> >>> +         !get_page_and_type(mfn_to_page(mfn), d,
> >> PGT_writable_page) )
> >>> +    {
> >>> +        gdprintk(XENLOG_WARNING, "Bad GMFN %lx (MFN %lx)\n",
> >> gmfn, mfn);
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    p = map_domain_page(mfn);
> >>> +
> >>> +    *(u32 *)p = 0;
> >>> +
> >>> +    unmap_domain_page(p);
> >>> +
> >>> +    put_page_and_type(mfn_to_page(mfn));
> >>> +}
> >>> +
> >>>  int wrmsr_viridian_regs(uint32_t idx, uint64_t val)  {
> >>> -    struct domain *d = current->domain;
> >>> +    struct vcpu *v = current;
> >>> +    struct domain *d = v->domain;
> >>>
> >>>      if ( !is_viridian_domain(d) )
> >>>          return 0;
> >>> @@ -142,44 +210,29 @@ int wrmsr_viridian_regs(uint32_t idx, ui
> >>>      case VIRIDIAN_MSR_GUEST_OS_ID:
> >>>          perfc_incr(mshv_wrmsr_osid);
> >>>          d->arch.hvm_domain.viridian.guest_os_id.raw = val;
> >>> -        gdprintk(XENLOG_INFO, "Guest os:\n");
> >>> -        gdprintk(XENLOG_INFO, "\tvendor: %x\n",
> >>> -               d-
> >>> arch.hvm_domain.viridian.guest_os_id.fields.vendor);
> >>> -        gdprintk(XENLOG_INFO, "\tos: %x\n",
> >>> -               d-
> >>> arch.hvm_domain.viridian.guest_os_id.fields.os);
> >>> -        gdprintk(XENLOG_INFO, "\tmajor: %x\n",
> >>> -               d-
> >>> arch.hvm_domain.viridian.guest_os_id.fields.major);
> >>> -        gdprintk(XENLOG_INFO, "\tminor: %x\n",
> >>> -               d-
> >>> arch.hvm_domain.viridian.guest_os_id.fields.minor);
> >>> -        gdprintk(XENLOG_INFO, "\tsp: %x\n",
> >>> -               d-
> >>> arch.hvm_domain.viridian.guest_os_id.fields.service_pack);
> >>> -        gdprintk(XENLOG_INFO, "\tbuild: %x\n",
> >>> -               d-
> >>> arch.hvm_domain.viridian.guest_os_id.fields.build_number);
> >>> +        dump_guest_os_id(d);
> >>>          break;
> >>>
> >>>      case VIRIDIAN_MSR_HYPERCALL:
> >>>          perfc_incr(mshv_wrmsr_hc_page);
> >>> -        gdprintk(XENLOG_INFO, "Set hypercall page
> %"PRIx64".\n",
> >> val);
> >>> -        if ( d->arch.hvm_domain.viridian.guest_os_id.raw == 0 )
> >>> -            break;
> >>>          d->arch.hvm_domain.viridian.hypercall_gpa.raw = val;
> >>> +        dump_hypercall(d);
> >>>          if ( d-
> >>> arch.hvm_domain.viridian.hypercall_gpa.fields.enabled )
> >>> -            enable_hypercall_page();
> >>> +            enable_hypercall_page(d);
> >>>          break;
> >>>
> >>>      case VIRIDIAN_MSR_VP_INDEX:
> >>>          perfc_incr(mshv_wrmsr_vp_index);
> >>> -        gdprintk(XENLOG_INFO, "Set VP index %"PRIu64".\n",
> val);
> >>>          break;
> >>>
> >>>      case VIRIDIAN_MSR_EOI:
> >>>          perfc_incr(mshv_wrmsr_eoi);
> >>> -        vlapic_EOI_set(vcpu_vlapic(current));
> >>> +        vlapic_EOI_set(vcpu_vlapic(v));
> >>>          break;
> >>>
> >>>      case VIRIDIAN_MSR_ICR: {
> >>>          u32 eax = (u32)val, edx = (u32)(val >> 32);
> >>> -        struct vlapic *vlapic = vcpu_vlapic(current);
> >>> +        struct vlapic *vlapic = vcpu_vlapic(v);
> >>>          perfc_incr(mshv_wrmsr_icr);
> >>>          eax &= ~(1 << 12);
> >>>          edx &= 0xff000000;
> >>> @@ -191,31 +244,15 @@ int wrmsr_viridian_regs(uint32_t idx, ui
> >>>
> >>>      case VIRIDIAN_MSR_TPR:
> >>>          perfc_incr(mshv_wrmsr_tpr);
> >>> -        vlapic_set_reg(vcpu_vlapic(current), APIC_TASKPRI,
> >> (uint8_t)val);
> >>> +        vlapic_set_reg(vcpu_vlapic(v), APIC_TASKPRI,
> >> (uint8_t)val);
> >>>          break;
> >>>
> >>>      case VIRIDIAN_MSR_APIC_ASSIST:
> >>> -        /*
> >>> -         * We don't support the APIC assist page, and that fact
> >> is reflected
> >>> in
> >>> -         * our CPUID flags. However, Windows 7 build 7000 has a
> >> bug which
> >>> means
> >>> -         * that it doesn't recognise that, and tries to use the
> >> page anyway.
> >>> We
> >>> -         * therefore have to fake up just enough to keep win7
> >> happy.
> >>> -         * Fortunately, that's really easy: just setting the
> >> first four bytes
> >>> -         * in the page to zero effectively disables the page
> >> again, so that's
> >>> -         * what we do. Semantically, the first four bytes are
> >> supposed to be
> >>> a
> >>> -         * flag saying whether the guest really needs to issue
> an
> >> EOI.
> >>> Setting
> >>> -         * that flag to zero means that it must always issue
> one,
> >> which is
> >>> what
> >>> -         * we want. Once a page has been repurposed as an APIC
> >> assist page
> >>> the
> >>> -         * guest isn't allowed to set anything in it, so the
> flag
> >> remains
> >>> zero
> >>> -         * and all is fine. The guest is allowed to clear flags
> >> in the page,
> >>> -         * but that doesn't cause us any problems.
> >>> -         */
> >>> -        if ( val & 1 ) /* APIC assist page enabled? */
> >>> -        {
> >>> -            uint32_t word = 0;
> >>> -            paddr_t page_start = val & ~1ul;
> >>> -            (void)hvm_copy_to_guest_phys(page_start, &word,
> >> sizeof(word));
> >>> -        }
> >>> +        perfc_incr(mshv_wrmsr_apic_msr);
> >>> +        v->arch.hvm_vcpu.viridian.apic_assist.raw = val;
> >>> +        dump_apic_assist(v);
> >>> +        if (v-
> >arch.hvm_vcpu.viridian.apic_assist.fields.enabled)
> >>> +            initialize_apic_assist(v);
> >>>          break;
> >>>
> >>>      default:
> >>> @@ -228,20 +265,21 @@ int wrmsr_viridian_regs(uint32_t idx, ui
> >> int
> >>> rdmsr_viridian_regs(uint32_t idx, uint64_t *val)  {
> >>>      struct vcpu *v = current;
> >>> +    struct domain *d = v->domain;
> >>>
> >>> -    if ( !is_viridian_domain(v->domain) )
> >>> +    if ( !is_viridian_domain(d) )
> >>>          return 0;
> >>>
> >>>      switch ( idx )
> >>>      {
> >>>      case VIRIDIAN_MSR_GUEST_OS_ID:
> >>>          perfc_incr(mshv_rdmsr_osid);
> >>> -        *val = v->domain-
> >>> arch.hvm_domain.viridian.guest_os_id.raw;
> >>> +        *val = d->arch.hvm_domain.viridian.guest_os_id.raw;
> >>>          break;
> >>>
> >>>      case VIRIDIAN_MSR_HYPERCALL:
> >>>          perfc_incr(mshv_rdmsr_hc_page);
> >>> -        *val = v->domain-
> >>> arch.hvm_domain.viridian.hypercall_gpa.raw;
> >>> +        *val = d->arch.hvm_domain.viridian.hypercall_gpa.raw;
> >>>          break;
> >>>
> >>>      case VIRIDIAN_MSR_VP_INDEX:
> >>> @@ -260,6 +298,11 @@ int rdmsr_viridian_regs(uint32_t idx, ui
> >>>          *val = vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI);
> >>>          break;
> >>>
> >>> +    case VIRIDIAN_MSR_APIC_ASSIST:
> >>> +        perfc_incr(mshv_rdmsr_apic_msr);
> >>> +        *val = v->arch.hvm_vcpu.viridian.apic_assist.raw;
> >>> +        break;
> >>> +
> >>>      default:
> >>>          return 0;
> >>>      }
> >>> diff -r e90438f6e6d1 -r a08073c5cf28 xen/include/asm-
> >> x86/hvm/vcpu.h
> >>> --- a/xen/include/asm-x86/hvm/vcpu.h Wed Sep 14 11:38:13 2011
> >> +0100
> >>> +++ b/xen/include/asm-x86/hvm/vcpu.h Thu Sep 15 06:11:01 2011
> >> +0100
> >>> @@ -23,6 +23,7 @@
> >>>  #include <xen/tasklet.h>
> >>>  #include <asm/hvm/io.h>
> >>>  #include <asm/hvm/vlapic.h>
> >>> +#include <asm/hvm/viridian.h>
> >>>  #include <asm/hvm/vmx/vmcs.h>
> >>>  #include <asm/hvm/vmx/vvmx.h>
> >>>  #include <asm/hvm/svm/vmcb.h>
> >>> @@ -163,6 +164,8 @@ struct hvm_vcpu {
> >>>      int           inject_trap;       /* -1 for nothing to
> inject
> >> */
> >>>      int           inject_error_code;
> >>>      unsigned long inject_cr2;
> >>> +
> >>> +    struct viridian_vcpu viridian;
> >>>  };
> >>>
> >>>  #endif /* __ASM_X86_HVM_VCPU_H__ */ diff -r e90438f6e6d1 -r
> >>> a08073c5cf28 xen/include/asm-x86/hvm/viridian.h
> >>> --- a/xen/include/asm-x86/hvm/viridian.h Wed Sep 14 11:38:13
> 2011
> >>> +0100
> >>> +++ b/xen/include/asm-x86/hvm/viridian.h Thu Sep 15 06:11:01
> 2011
> >>> +++ +0100
> >>> @@ -9,6 +9,21 @@
> >>>  #ifndef __ASM_X86_HVM_VIRIDIAN_H__
> >>>  #define __ASM_X86_HVM_VIRIDIAN_H__
> >>>
> >>> +union viridian_apic_assist
> >>> +{   uint64_t raw;
> >>> +    struct
> >>> +    {
> >>> +        uint64_t enabled:1;
> >>> +        uint64_t reserved_preserved:11;
> >>> +        uint64_t pfn:48;
> >>> +    } fields;
> >>> +};
> >>> +
> >>> +struct viridian_vcpu
> >>> +{
> >>> +    union viridian_apic_assist apic_assist; };
> >>> +
> >>>  union viridian_guest_os_id
> >>>  {
> >>>      uint64_t raw;
> >>> diff -r e90438f6e6d1 -r a08073c5cf28 xen/include/asm-
> >> x86/perfc_defn.h
> >>> --- a/xen/include/asm-x86/perfc_defn.h Wed Sep 14 11:38:13 2011
> >> +0100
> >>> +++ b/xen/include/asm-x86/perfc_defn.h Thu Sep 15 06:11:01 2011
> >> +0100
> >>> @@ -120,12 +120,14 @@ PERFCOUNTER(mshv_rdmsr_hc_page,
> >>>  PERFCOUNTER(mshv_rdmsr_vp_index,        "MS Hv rdmsr vp index")
> >>>  PERFCOUNTER(mshv_rdmsr_icr,             "MS Hv rdmsr icr")
> >>>  PERFCOUNTER(mshv_rdmsr_tpr,             "MS Hv rdmsr tpr")
> >>> +PERFCOUNTER(mshv_rdmsr_apic_assist,     "MS Hv rdmsr APIC
> >> assist")
> >>>  PERFCOUNTER(mshv_wrmsr_osid,            "MS Hv wrmsr Guest OS
> >> ID")
> >>>  PERFCOUNTER(mshv_wrmsr_hc_page,         "MS Hv wrmsr hypercall
> >> page")
> >>>  PERFCOUNTER(mshv_wrmsr_vp_index,        "MS Hv wrmsr vp index")
> >>>  PERFCOUNTER(mshv_wrmsr_icr,             "MS Hv wrmsr icr")
> >>>  PERFCOUNTER(mshv_wrmsr_tpr,             "MS Hv wrmsr tpr")
> >>>  PERFCOUNTER(mshv_wrmsr_eoi,             "MS Hv wrmsr eoi")
> >>> +PERFCOUNTER(mshv_wrmsr_apic_assist,     "MS Hv wrmsr APIC
> >> assist")
> >>>
> >>>  PERFCOUNTER(realmode_emulations, "realmode instructions
> >> emulated")
> >>>  PERFCOUNTER(realmode_exits,      "vmexits from realmode")
> >>>
> >>> _______________________________________________
> >>> Xen-devel mailing list
> >>> Xen-devel@xxxxxxxxxxxxxxxxxxx
> >>> http://lists.xensource.com/xen-devel
> >>
> >
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel