Skip to contentSkip to conversation

viridian cleanup

PATCH In review Paul Durrant 3 participants 26 messages Oct 31, 2018 – Nov 5, 2018

lore
Share & tools
lore
#0 lore raw

v2 cover viridian cleanup

I plan to add implementations for more viridian enlightenments in the near future. This series is just some cleanup I've been doing in preparation.

Paul Durrant (9):
viridian: move the code into its own sub-directory
viridian: remove MSR perf counters
viridian: remove comments referencing section number in the spec.
viridian: remove duplicate union types
viridian: separate interrupt related enlightenment implementations...
viridian: separate time related enlightenment implementations...
viridian: define type for the 'virtual VP assist page' tools/misc: fix hard tabs in xen-hvmctx.c
viridian: introduce struct viridian_page

 MAINTAINERS                                |   2 +-
 tools/misc/xen-hvmctx.c                    |  18 +-
 xen/arch/x86/hvm/Makefile                  |   2 +-
 xen/arch/x86/hvm/viridian/Makefile         |   3 +
 xen/arch/x86/hvm/viridian/synic.c          | 234 ++++++++++++++
 xen/arch/x86/hvm/viridian/time.c           | 245 +++++++++++++++
 xen/arch/x86/hvm/{ => viridian}/viridian.c | 482 +++--------------------------
 xen/include/asm-x86/hvm/viridian.h         | 144 ++++++---
 xen/include/asm-x86/perfc_defn.h           |  26 --
 xen/include/public/arch-x86/hvm/save.h     |   2 +-
 10 files changed, 638 insertions(+), 520 deletions(-)
 create mode 100644 xen/arch/x86/hvm/viridian/Makefile
 create mode 100644 xen/arch/x86/hvm/viridian/synic.c
 create mode 100644 xen/arch/x86/hvm/viridian/time.c
 rename xen/arch/x86/hvm/{ => viridian}/viridian.c (54%)
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
#1 lore raw

PATCH v2 1/9 viridian: move the code into its own sub-directory

Subsequent patches will introduce support for more viridian enlightenments which will make a single source module quite lengthy.

This patch therefore creates a new arch/x86/hvm/viridian sub-directory and moves viridian.c into that.

The patch also fixes some bad whitespace whilst moving the code and adjusts the MAINTAINERS file.

---
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

v2:
- Adjust MAINTAINERS and add Jan's A-b

---
 MAINTAINERS                                | 2 +-
 xen/arch/x86/hvm/Makefile                  | 2 +-
 xen/arch/x86/hvm/viridian/Makefile         | 1 +
 xen/arch/x86/hvm/{ => viridian}/viridian.c | 4 ++--
 4 files changed, 5 insertions(+), 4 deletions(-)
 create mode 100644 xen/arch/x86/hvm/viridian/Makefile
 rename xen/arch/x86/hvm/{ => viridian}/viridian.c (99%)
MAINTAINERS +1 -1
index 1970100b37..c2ba74d0c9 100644
@@ -487,7 +487,7 @@ F:	xen/arch/x86/mm/shadow/
 X86 VIRIDIAN ENLIGHTENMENTS
 M:	Paul Durrant <[email protected]>
 S:	Supported
-F:	xen/arch/x86/hvm/viridian.c
+F:	xen/arch/x86/hvm/viridian/
 F:	xen/include/asm-x86/hvm/viridian.h
 
 XENTRACE
xen/arch/x86/hvm/Makefile +1 -1
index 5e04bc1429..86b106f8e7 100644
@@ -1,5 +1,6 @@
 subdir-y += svm
 subdir-y += vmx
+subdir-y += viridian
 
 obj-y += asid.o
 obj-y += dm.o
@@ -23,7 +24,6 @@ obj-y += rtc.o
 obj-y += save.o
 obj-y += stdvga.o
 obj-y += vioapic.o
-obj-y += viridian.o
 obj-y += vlapic.o
 obj-y += vm_event.o
 obj-y += vmsi.o
xen/arch/x86/hvm/viridian/Makefile new file +1 -0
new file mode 100644
index 0000000000..09fd0a5f3c
@@ -0,0 +1 @@
+obj-y += viridian.o
xen/arch/x86/hvm/viridian/viridian.c renamed from xen/arch/x86/hvm/viridian.c +2 -2
similarity index 99%
rename from xen/arch/x86/hvm/viridian.c
rename to xen/arch/x86/hvm/viridian/viridian.c
index f42b1f063e..3e9beda831 100644
@@ -4,7 +4,7 @@
  * An implementation of some Viridian enlightenments. See Microsoft's
  * Hypervisor Top Level Functional Specification (v5.0a) at:
  *
- * https://github.com/Microsoft/Virtualization-Documentation/raw/master/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0.pdf 
+ * https://github.com/Microsoft/Virtualization-Documentation/raw/master/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0.pdf
  *
  * for more information.
  */
@@ -334,7 +334,7 @@ static void dump_reference_tsc(const struct domain *d)
     const union viridian_reference_tsc *rt;
 
     rt = &d->arch.hvm.viridian.reference_tsc;
-    
+
     printk(XENLOG_G_INFO "d%d: VIRIDIAN REFERENCE_TSC: enabled: %x pfn: %lx\n",
            d->domain_id,
            rt->fields.enabled, (unsigned long)rt->fields.pfn);
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
#2 lore raw

PATCH v2 2/9 viridian: remove MSR perf counters

In reply to Paul Durrant

They're not really useful so maintaining them is pointless.

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

---
 xen/arch/x86/hvm/viridian/viridian.c | 21 ---------------------
 xen/include/asm-x86/perfc_defn.h     | 26 --------------------------
 2 files changed, 47 deletions(-)
xen/arch/x86/hvm/viridian/viridian.c +0 -21
index 3e9beda831..c5722d6992 100644
@@ -11,7 +11,6 @@
 
 #include <xen/sched.h>
 #include <xen/version.h>
-#include <xen/perfc.h>
 #include <xen/hypercall.h>
 #include <xen/domain_page.h>
 #include <asm/guest_access.h>
@@ -560,13 +559,11 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
     switch ( idx )
     {
     case HV_X64_MSR_GUEST_OS_ID:
-        perfc_incr(mshv_wrmsr_osid);
         d->arch.hvm.viridian.guest_os_id.raw = val;
         dump_guest_os_id(d);
         break;
 
     case HV_X64_MSR_HYPERCALL:
-        perfc_incr(mshv_wrmsr_hc_page);
         d->arch.hvm.viridian.hypercall_gpa.raw = val;
         dump_hypercall(d);
         if ( d->arch.hvm.viridian.hypercall_gpa.fields.enabled )
@@ -574,18 +571,15 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
         break;
 
     case HV_X64_MSR_VP_INDEX:
-        perfc_incr(mshv_wrmsr_vp_index);
         break;
 
     case HV_X64_MSR_EOI:
-        perfc_incr(mshv_wrmsr_eoi);
         vlapic_EOI_set(vcpu_vlapic(v));
         break;
 
     case HV_X64_MSR_ICR: {
         u32 eax = (u32)val, edx = (u32)(val >> 32);
         struct vlapic *vlapic = vcpu_vlapic(v);
-        perfc_incr(mshv_wrmsr_icr);
         eax &= ~(1 << 12);
         edx &= 0xff000000;
         vlapic_set_reg(vlapic, APIC_ICR2, edx);
@@ -595,12 +589,10 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
     }
 
     case HV_X64_MSR_TPR:
-        perfc_incr(mshv_wrmsr_tpr);
         vlapic_set_reg(vcpu_vlapic(v), APIC_TASKPRI, (uint8_t)val);
         break;
 
     case HV_X64_MSR_VP_ASSIST_PAGE:
-        perfc_incr(mshv_wrmsr_apic_msr);
         teardown_vp_assist(v); /* release any previous mapping */
         v->arch.hvm.viridian.vp_assist.msr.raw = val;
         dump_vp_assist(v);
@@ -612,7 +604,6 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
         if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
             return X86EMUL_EXCEPTION;
 
-        perfc_incr(mshv_wrmsr_tsc_msr);
         d->arch.hvm.viridian.reference_tsc.raw = val;
         dump_reference_tsc(d);
         if ( d->arch.hvm.viridian.reference_tsc.fields.enabled )
@@ -704,17 +695,14 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
     switch ( idx )
     {
     case HV_X64_MSR_GUEST_OS_ID:
-        perfc_incr(mshv_rdmsr_osid);
         *val = d->arch.hvm.viridian.guest_os_id.raw;
         break;
 
     case HV_X64_MSR_HYPERCALL:
-        perfc_incr(mshv_rdmsr_hc_page);
         *val = d->arch.hvm.viridian.hypercall_gpa.raw;
         break;
 
     case HV_X64_MSR_VP_INDEX:
-        perfc_incr(mshv_rdmsr_vp_index);
         *val = v->vcpu_id;
         break;
 
@@ -722,7 +710,6 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
         if ( viridian_feature_mask(d) & HVMPV_no_freq )
             return X86EMUL_EXCEPTION;
 
-        perfc_incr(mshv_rdmsr_tsc_frequency);
         *val = (uint64_t)d->arch.tsc_khz * 1000ull;
         break;
 
@@ -730,23 +717,19 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
         if ( viridian_feature_mask(d) & HVMPV_no_freq )
             return X86EMUL_EXCEPTION;
 
-        perfc_incr(mshv_rdmsr_apic_frequency);
         *val = 1000000000ull / APIC_BUS_CYCLE_NS;
         break;
 
     case HV_X64_MSR_ICR:
-        perfc_incr(mshv_rdmsr_icr);
         *val = (((uint64_t)vlapic_get_reg(vcpu_vlapic(v), APIC_ICR2) << 32) |
                 vlapic_get_reg(vcpu_vlapic(v), APIC_ICR));
         break;
 
     case HV_X64_MSR_TPR:
-        perfc_incr(mshv_rdmsr_tpr);
         *val = vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI);
         break;
 
     case HV_X64_MSR_VP_ASSIST_PAGE:
-        perfc_incr(mshv_rdmsr_apic_msr);
         *val = v->arch.hvm.viridian.vp_assist.msr.raw;
         break;
 
@@ -754,7 +737,6 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
         if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
             return X86EMUL_EXCEPTION;
 
-        perfc_incr(mshv_rdmsr_tsc_msr);
         *val = d->arch.hvm.viridian.reference_tsc.raw;
         break;
 
@@ -771,7 +753,6 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
             printk(XENLOG_G_INFO "d%d: VIRIDIAN MSR_TIME_REF_COUNT: accessed\n",
                    d->domain_id);
 
-        perfc_incr(mshv_rdmsr_time_ref_count);
         *val = raw_trc_val(d) + trc->off;
         break;
     }
@@ -876,7 +857,6 @@ int viridian_hypercall(struct cpu_user_regs *regs)
         /*
          * See section 14.5.1 of the specification.
          */
-        perfc_incr(mshv_call_long_wait);
         do_sched_op(SCHEDOP_yield, guest_handle_from_ptr(NULL, void));
         status = HV_STATUS_SUCCESS;
         break;
@@ -895,7 +875,6 @@ int viridian_hypercall(struct cpu_user_regs *regs)
         /*
          * See sections 9.4.2 and 9.4.4 of the specification.
          */
-        perfc_incr(mshv_call_flush);
 
         /* These hypercalls should never use the fast-call convention. */
         status = HV_STATUS_INVALID_PARAMETER;
xen/include/asm-x86/perfc_defn.h +0 -26
index 6db8746906..1a9ea3f89e 100644
@@ -112,32 +112,6 @@ PERFCOUNTER(shadow_unsync,         "shadow OOS unsyncs")
 PERFCOUNTER(shadow_unsync_evict,   "shadow OOS evictions")
 PERFCOUNTER(shadow_resync,         "shadow OOS resyncs")
 
-PERFCOUNTER(mshv_call_sw_addr_space,    "MS Hv Switch Address Space")
-PERFCOUNTER(mshv_call_flush_tlb_list,   "MS Hv Flush TLB list")
-PERFCOUNTER(mshv_call_flush_tlb_all,    "MS Hv Flush TLB all")
-PERFCOUNTER(mshv_call_long_wait,        "MS Hv Notify long wait")
-PERFCOUNTER(mshv_call_flush,            "MS Hv Flush TLB")
-PERFCOUNTER(mshv_rdmsr_osid,            "MS Hv rdmsr Guest OS ID")
-PERFCOUNTER(mshv_rdmsr_hc_page,         "MS Hv rdmsr hypercall page")
-PERFCOUNTER(mshv_rdmsr_vp_index,        "MS Hv rdmsr vp index")
-PERFCOUNTER(mshv_rdmsr_tsc_frequency,   "MS Hv rdmsr TSC frequency")
-PERFCOUNTER(mshv_rdmsr_apic_frequency,  "MS Hv rdmsr APIC frequency")
-PERFCOUNTER(mshv_rdmsr_time_ref_count,  "MS Hv rdmsr time ref count")
-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_rdmsr_apic_msr,        "MS Hv rdmsr APIC msr")
-PERFCOUNTER(mshv_rdmsr_tsc_msr,         "MS Hv rdmsr TSC msr")
-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(mshv_wrmsr_apic_msr,        "MS Hv wrmsr APIC msr")
-PERFCOUNTER(mshv_wrmsr_tsc_msr,         "MS Hv wrmsr TSC msr")
-
 PERFCOUNTER(realmode_emulations, "realmode instructions emulated")
 PERFCOUNTER(realmode_exits,      "vmexits from realmode")
 
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
#3 lore raw

PATCH v2 3/9 viridian: remove comments referencing section number in the spec.

In reply to Paul Durrant

Microsoft has a habit of re-numbering sections in the spec. so avoid referring to section numbers in comments. Also remove the URL for the spec. from the boilerplate... Again, Microsoft has a habit of changing these too.

This patch also cleans up some > 80 character lines.

Purely cosmetic. No functional change.

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

v2:
- Fix comment style

---
 xen/arch/x86/hvm/viridian/viridian.c | 84 ++++++++++++++++--------------------
 xen/include/asm-x86/hvm/viridian.h   |  4 --
 2 files changed, 36 insertions(+), 52 deletions(-)
xen/arch/x86/hvm/viridian/viridian.c +36 -48
index c5722d6992..f5f5fbcdfe 100644
@@ -1,12 +1,8 @@
-/******************************************************************************
+/**************************************************************************
  * viridian.c
  *
  * An implementation of some Viridian enlightenments. See Microsoft's
- * Hypervisor Top Level Functional Specification (v5.0a) at:
- *
- * https://github.com/Microsoft/Virtualization-Documentation/raw/master/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0.pdf
- *
- * for more information.
+ * Hypervisor Top Level Functional Specification for more information.
  */
 
 #include <xen/sched.h>
@@ -102,12 +98,7 @@
 /* Viridian Hypercall Flags. */
 #define HV_FLUSH_ALL_PROCESSORS 1
 
-/*
- * Viridian Partition Privilege Flags.
- *
- * This is taken from section 4.2.2 of the specification, and fixed for
- * style and correctness.
- */
+/* Viridian Partition Privilege Flags */
 typedef struct {
     /* Access to virtual MSRs */
     uint64_t AccessVpRunTimeReg:1;
@@ -168,7 +159,7 @@ typedef union _HV_CRASH_CTL_REG_CONTENTS
 #define CPUID4A_MSR_BASED_APIC         (1 << 3)
 #define CPUID4A_RELAX_TIMER_INT        (1 << 5)
 
-/* Viridian CPUID leaf 6: Implementation HW features detected and in use. */
+/* Viridian CPUID leaf 6: Implementation HW features detected and in use */
 #define CPUID6A_APIC_OVERLAY    (1 << 0)
 #define CPUID6A_MSR_BITMAPS     (1 << 1)
 #define CPUID6A_NESTED_PAGING   (1 << 3)
@@ -204,7 +195,6 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
     switch ( leaf )
     {
     case 0:
-        /* See section 2.4.1 of the specification */
         res->a = 0x40000006; /* Maximum leaf */
         memcpy(&res->b, "Micr", 4);
         memcpy(&res->c, "osof", 4);
@@ -212,13 +202,14 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
         break;
 
     case 1:
-        /* See section 2.4.2 of the specification */
         memcpy(&res->a, "Hv#1", 4);
         break;
 
     case 2:
-        /* Hypervisor information, but only if the guest has set its
-           own version number. */
+        /*
+         * Hypervisor information, but only if the guest has set its
+         * own version number.
+         */
         if ( d->arch.hvm.viridian.guest_os_id.raw == 0 )
             break;
         res->a = viridian_build;
@@ -230,9 +221,9 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
     case 3:
     {
         /*
-         * Section 2.4.4 details this leaf and states that EAX and EBX
-         * are defined to be the low and high parts of the partition
-         * privilege mask respectively.
+         * The specification states that EAX and EBX are defined to be
+         * the low and high parts of the partition privilege mask
+         * respectively.
          */
         HV_PARTITION_PRIVILEGE_MASK mask = {
             .AccessIntrCtrlRegs = 1,
@@ -382,11 +373,6 @@ static void initialize_vp_assist(struct vcpu *v)
 
     ASSERT(!v->arch.hvm.viridian.vp_assist.va);
 
-    /*
-     * See section 7.8.7 of the specification for details of this
-     * enlightenment.
-     */
-
     if ( !page )
         goto fail;
 
@@ -409,8 +395,8 @@ static void initialize_vp_assist(struct vcpu *v)
     return;
 
  fail:
-    gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n", gmfn,
-             mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
+    gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
+             gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
 }
 
 static void teardown_vp_assist(struct vcpu *v)
@@ -498,14 +484,15 @@ static void update_reference_tsc(struct domain *d, bool_t initialize)
         clear_page(p);
 
     /*
-     * This enlightenment must be disabled is the host TSC is not invariant.
-     * However it is also disabled if vtsc is true (which means rdtsc is being
-     * emulated). This generally happens when guest TSC freq and host TSC freq
-     * don't match. The TscScale value could be adjusted to cope with this,
-     * allowing vtsc to be turned off, but support for this is not yet present
-     * in the hypervisor. Thus is it is possible that migrating a Windows VM
-     * between hosts of differing TSC frequencies may result in large
-     * differences in guest performance.
+     * This enlightenment must be disabled is the host TSC is not
+     * invariant. However it is also disabled if vtsc is true (which
+     * means rdtsc is being emulated). This generally happens when guest
+     * TSC freq and host TSC freq don't match. The TscScale value could be
+     * adjusted to cope with this, allowing vtsc to be turned off, but
+     * support for this is not yet present in the hypervisor. Thus is it
+     * is possible that migrating a Windows VM between hosts of differing
+     * TSC frequencies may result in large differences in guest
+     * performance.
      */
     if ( !host_tsc_is_safe() || d->arch.vtsc )
     {
@@ -515,10 +502,10 @@ static void update_reference_tsc(struct domain *d, bool_t initialize)
          * this mechanism is no longer a reliable source of time and that
          * the VM should fall back to a different source.
          *
-         * Server 2012 (6.2 kernel) and 2012 R2 (6.3 kernel) actually violate
-         * the spec. and rely on a value of 0 to indicate that this
-         * enlightenment should no longer be used. These two kernel
-         * versions are currently the only ones to make use of this
+         * Server 2012 (6.2 kernel) and 2012 R2 (6.3 kernel) actually
+         * violate the specification and rely on a value of 0 to indicate
+         * that this enlightenment should no longer be used. These two
+         * kernel versions are currently the only ones to make use of this
          * enlightenment, so just use 0 here.
          */
         p->TscSequence = 0;
@@ -646,7 +633,8 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
 
     default:
         gdprintk(XENLOG_INFO,
-                 "Write %016"PRIx64" to unimplemented MSR %#x\n", val, idx);
+                 "Write %016"PRIx64" to unimplemented MSR %#x\n", val,
+                 idx);
         return X86EMUL_EXCEPTION;
     }
 
@@ -872,10 +860,6 @@ int viridian_hypercall(struct cpu_user_regs *regs)
             uint64_t vcpu_mask;
         } input_params;
 
-        /*
-         * See sections 9.4.2 and 9.4.4 of the specification.
-         */
-
         /* These hypercalls should never use the fast-call convention. */
         status = HV_STATUS_INVALID_PARAMETER;
         if ( input.fast )
@@ -883,7 +867,8 @@ int viridian_hypercall(struct cpu_user_regs *regs)
 
         /* Get input parameters. */
         if ( hvm_copy_from_guest_phys(&input_params, input_params_gpa,
-                                      sizeof(input_params)) != HVMTRANS_okay )
+                                      sizeof(input_params)) !=
+             HVMTRANS_okay )
             break;
 
         /*
@@ -961,7 +946,8 @@ out:
     return HVM_HCALL_completed;
 }
 
-static int viridian_save_domain_ctxt(struct vcpu *v, hvm_domain_context_t *h)
+static int viridian_save_domain_ctxt(struct vcpu *v,
+                                     hvm_domain_context_t *h)
 {
     const struct domain *d = v->domain;
     struct hvm_viridian_domain_context ctxt = {
@@ -977,7 +963,8 @@ static int viridian_save_domain_ctxt(struct vcpu *v, hvm_domain_context_t *h)
     return (hvm_save_entry(VIRIDIAN_DOMAIN, 0, h, &ctxt) != 0);
 }
 
-static int viridian_load_domain_ctxt(struct domain *d, hvm_domain_context_t *h)
+static int viridian_load_domain_ctxt(struct domain *d,
+                                     hvm_domain_context_t *h)
 {
     struct hvm_viridian_domain_context ctxt;
 
@@ -1011,7 +998,8 @@ static int viridian_save_vcpu_ctxt(struct vcpu *v, hvm_domain_context_t *h)
     return hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt);
 }
 
-static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
+static int viridian_load_vcpu_ctxt(struct domain *d,
+                                   hvm_domain_context_t *h)
 {
     unsigned int vcpuid = hvm_load_instance(h);
     struct vcpu *v;
xen/include/asm-x86/hvm/viridian.h +0 -4
index 071fb445bb..f6008f9bdb 100644
@@ -77,10 +77,6 @@ union viridian_reference_tsc
     } fields;
 };
 
-/*
- * Type defintion as in Microsoft Hypervisor Top-Level Functional
- * Specification v4.0a, section 15.4.2.
- */
 typedef struct _HV_REFERENCE_TSC_PAGE
 {
     uint32_t TscSequence;
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
#4 lore raw

PATCH v2 4/9 viridian: remove duplicate union types

In reply to Paul Durrant

The 'viridian_vp_assist', 'viridian_hypercall_gpa' and 'viridian_reference_tsc' union types are identical in layout. The layout is also common throughout the specification [1].

This patch declares a common 'viridian_page_msr' type and converts the rest of the code to use that type for both the hypercall and VP assist pages.

Also, rename 'viridian_guest_os_id' to 'viridian_guest_os_id_msr' since it also is a union representing an MSR value.

No functional change.

[1] https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0C.pdf

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

---
 xen/arch/x86/hvm/viridian/viridian.c |  8 ++++----
 xen/include/asm-x86/hvm/viridian.h   | 36 ++++++++----------------------------
 2 files changed, 12 insertions(+), 32 deletions(-)
xen/arch/x86/hvm/viridian/viridian.c +4 -4
index f5f5fbcdfe..2dc86dd0f3 100644
@@ -286,7 +286,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
 
 static void dump_guest_os_id(const struct domain *d)
 {
-    const union viridian_guest_os_id *goi;
+    const union viridian_guest_os_id_msr *goi;
 
     goi = &d->arch.hvm.viridian.guest_os_id;
 
@@ -300,7 +300,7 @@ static void dump_guest_os_id(const struct domain *d)
 
 static void dump_hypercall(const struct domain *d)
 {
-    const union viridian_hypercall_gpa *hg;
+    const union viridian_page_msr *hg;
 
     hg = &d->arch.hvm.viridian.hypercall_gpa;
 
@@ -311,7 +311,7 @@ static void dump_hypercall(const struct domain *d)
 
 static void dump_vp_assist(const struct vcpu *v)
 {
-    const union viridian_vp_assist *va;
+    const union viridian_page_msr *va;
 
     va = &v->arch.hvm.viridian.vp_assist.msr;
 
@@ -321,7 +321,7 @@ static void dump_vp_assist(const struct vcpu *v)
 
 static void dump_reference_tsc(const struct domain *d)
 {
-    const union viridian_reference_tsc *rt;
+    const union viridian_page_msr *rt;
 
     rt = &d->arch.hvm.viridian.reference_tsc;
 
xen/include/asm-x86/hvm/viridian.h +8 -28
index f6008f9bdb..359fdf5a83 100644
@@ -9,8 +9,9 @@
 #ifndef __ASM_X86_HVM_VIRIDIAN_H__
 #define __ASM_X86_HVM_VIRIDIAN_H__
 
-union viridian_vp_assist
-{   uint64_t raw;
+union viridian_page_msr
+{
+    uint64_t raw;
     struct
     {
         uint64_t enabled:1;
@@ -22,14 +23,14 @@ union viridian_vp_assist
 struct viridian_vcpu
 {
     struct {
-        union viridian_vp_assist msr;
+        union viridian_page_msr msr;
         void *va;
         bool pending;
     } vp_assist;
     uint64_t crash_param[5];
 };
 
-union viridian_guest_os_id
+union viridian_guest_os_id_msr
 {
     uint64_t raw;
     struct
@@ -43,16 +44,6 @@ union viridian_guest_os_id
     } fields;
 };
 
-union viridian_hypercall_gpa
-{   uint64_t raw;
-    struct
-    {
-        uint64_t enabled:1;
-        uint64_t reserved_preserved:11;
-        uint64_t pfn:48;
-    } fields;
-};
-
 struct viridian_time_ref_count
 {
     unsigned long flags;
@@ -66,17 +57,6 @@ struct viridian_time_ref_count
     int64_t off;
 };
 
-union viridian_reference_tsc
-{
-    uint64_t raw;
-    struct
-    {
-        uint64_t enabled:1;
-        uint64_t reserved_preserved:11;
-        uint64_t pfn:48;
-    } fields;
-};
-
 typedef struct _HV_REFERENCE_TSC_PAGE
 {
     uint32_t TscSequence;
@@ -88,10 +68,10 @@ typedef struct _HV_REFERENCE_TSC_PAGE
 
 struct viridian_domain
 {
-    union viridian_guest_os_id guest_os_id;
-    union viridian_hypercall_gpa hypercall_gpa;
+    union viridian_guest_os_id_msr guest_os_id;
+    union viridian_page_msr hypercall_gpa;
     struct viridian_time_ref_count time_ref_count;
-    union viridian_reference_tsc reference_tsc;
+    union viridian_page_msr reference_tsc;
 };
 
 void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
#5 lore raw

PATCH v2 5/9 viridian: separate interrupt related enlightenment implementations...

In reply to Paul Durrant

...into new 'synic' module.

The SynIC (synthetic interrupt controller) is specified [1] to be a super- set of a virtualized LAPIC, and its definition encompasses all enlightenments related to virtual interrupt control.

This patch reduces the size of the main viridian source module by giving these enlightenments their own module. This is done in anticipation of implementation of more such enlightenments and a desire not to further lengthen then main source module when this work is done.

Whilst moving the code:

- Fix various style issues.
- Move the MSR definitions into the header (since they are now needed in more than one source module).

[1] https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0C.pdf

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

v2:
- Sort headers alphabetically and separate asm/ amd xen/ sections
- Constify vcpu context during load

---
 xen/arch/x86/hvm/viridian/Makefile   |   1 +
 xen/arch/x86/hvm/viridian/synic.c    | 225 ++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/viridian/viridian.c | 229 ++---------------------------------
 xen/include/asm-x86/hvm/viridian.h   |  76 ++++++++++++
 4 files changed, 312 insertions(+), 219 deletions(-)
 create mode 100644 xen/arch/x86/hvm/viridian/synic.c
xen/arch/x86/hvm/viridian/Makefile +1 -0
index 09fd0a5f3c..fca8e16e20 100644
@@ -1 +1,2 @@
+obj-y += synic.o
 obj-y += viridian.o
xen/arch/x86/hvm/viridian/synic.c new file +225 -0

225 lines not on this page · raw message

xen/arch/x86/hvm/viridian/viridian.c +10 -219

292 lines not on this page · raw message

xen/include/asm-x86/hvm/viridian.h +76 -0

88 lines not on this page · raw message

Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Reviewed-by on v2 5/9
#6 lore raw

Re: [PATCH v2 5/9] viridian: separate interrupt related enlightenment implementations...

Quoted 13 lines · Paul Durrant

...into new 'synic' module.

The SynIC (synthetic interrupt controller) is specified [1] to be a super- set of a virtualized LAPIC, and its definition encompasses all enlightenments related to virtual interrupt control.

This patch reduces the size of the main viridian source module by giving these enlightenments their own module. This is done in anticipation of implementation of more such enlightenments and a desire not to further lengthen then main source module when this work is done.

Whilst moving the code:

Quoted 7 lines

- Fix various style issues.
- Move the MSR definitions into the header (since they are now needed in more than one source module).

[1] https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0C.pdf

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#7 lore raw

Re: [PATCH v2 5/9] viridian: separate interrupt related enlightenment implementations...

In reply to Paul Durrant
Quoted 12 lines
Quoted 1 line
Quoted 1 line · On 31.10.18 at 13:43, <[email protected]> wrote:
xen/arch/x86/hvm/viridian/synic.c new file +1 -0
@@ -0,0 +1,225 @@
+/**************************************************************************

*
+ * synic.c
+ *
+ * An implementation of some interrupt related Viridian enlightenments.
+ * See Microsoft's Hypervisor Top Level Functional Specification.
+ * for more information.
+ */

xen/arch/x86/hvm/viridian/synic.c line 17

Quoted 10 lines
xen/arch/x86/hvm/viridian/synic.c new file +10 -0
+
+#include <xen/domain_page.h>
+#include <xen/hypercall.h>
+#include <xen/sched.h>
+#include <xen/version.h>
+
+#include <asm/apic.h>
+#include <asm/hvm/support.h>
+
+static void dump_vp_assist(const struct vcpu *v)

With the name change to synic, is the _vp_ infix here and in a few other places retained intentionally?

xen/include/asm-x86/hvm/viridian.h line 15

Quoted 10 lines
xen/include/asm-x86/hvm/viridian.h +4 -0
@@ -9,6 +9,74 @@
 #ifndef __ASM_X86_HVM_VIRIDIAN_H__
 #define __ASM_X86_HVM_VIRIDIAN_H__
 
+#include <asm/hvm/save.h>
+
+/* Viridian MSR numbers. */
+#define HV_X64_MSR_GUEST_OS_ID                   0x40000000

Are these needed outside of xen/arch/x86/hvm/viridian/ ? If not, please introduce a local header there rather than polluting every CU's name space. Other definitions and declaration local to the Viridian implementation should then go there too, rather than here.

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#8 lore raw

Re: [PATCH v2 5/9] viridian: separate interrupt related enlightenment implementations...

Quoted 30 lines

-----Original Message-----
From: Jan Beulich [mailto:[email protected]]
Sent: 05 November 2018 12:51
To: Paul Durrant <[email protected]>
Cc: Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; xen-devel <[email protected]>
Subject: Re: [PATCH v2 5/9] viridian: separate interrupt related enlightenment implementations...

xen/arch/x86/hvm/viridian/synic.c

Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 31.10.18 at 13:43, <[email protected]> wrote:
xen/arch/x86/hvm/viridian/synic.c new file +0 -0
@@ -0,0 +1,225 @@

+/************************************************************************ **

Quoted 12 lines

*
+ * synic.c
+ *
+ * An implementation of some interrupt related Viridian enlightenments.
+ * See Microsoft's Hypervisor Top Level Functional Specification.
+ * for more information.
+ */

xen/arch/x86/hvm/viridian/synic.c new file +5 -0
+
+#include <xen/domain_page.h>
+#include <xen/hypercall.h>
+#include <xen/sched.h>
+#include <xen/version.h>
Quoted 8 lines

xen/arch/x86/hvm/viridian/synic.c line 17

Quoted 5 lines
xen/arch/x86/hvm/viridian/synic.c new file +5 -0
+
+#include <asm/apic.h>
+#include <asm/hvm/support.h>
+
+static void dump_vp_assist(const struct vcpu *v)

With the name change to synic, is the _vp_ infix here and in a few other places retained intentionally?

Yes, it's meant to dump after programming the VP assist MSR, so I think the name is still correct.

Quoted 7 lines
Quoted 7 lines
xen/include/asm-x86/hvm/viridian.h +0 -0
@@ -9,6 +9,74 @@
 #ifndef __ASM_X86_HVM_VIRIDIAN_H__
 #define __ASM_X86_HVM_VIRIDIAN_H__
 
Quoted 10 lines

xen/include/asm-x86/hvm/viridian.h line 15

Quoted 4 lines
xen/include/asm-x86/hvm/viridian.h +4 -0
+#include <asm/hvm/save.h>
+
+/* Viridian MSR numbers. */
+#define HV_X64_MSR_GUEST_OS_ID                   0x40000000

Are these needed outside of xen/arch/x86/hvm/viridian/ ? If not, please introduce a local header there rather than polluting every CU's name space. Other definitions and declaration local to the Viridian implementation should then go there too, rather than here.

Ok. They are only needed under that sub-dir so I'll add the extra header.

Paul

Quoted 3 lines

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#9 lore raw

PATCH v2 6/9 viridian: separate time related enlightenment implementations...

In reply to Paul Durrant

...into new 'time' module.

This patch reduces the size of the main viridian source module by moving time related enlightenments into their own source module. This is done in anticipation of implementation of more such enightenments and a desire to not further lengthen the main source module when this work is done.

While moving the code:

- Move the declaration of HV_REFERENCE_TSC_PAGE from the header file into the new source module, since it is only used there.
- Clean up a bool_t.

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

v2:
- Sort headers alphabetically and separate asm/ amd xen/ sections
- Constify domain context during load

---
 xen/arch/x86/hvm/viridian/Makefile   |   1 +
 xen/arch/x86/hvm/viridian/time.c     | 245 +++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/viridian/viridian.c | 174 +------------------------
 xen/include/asm-x86/hvm/viridian.h   |  17 ++-
 4 files changed, 261 insertions(+), 176 deletions(-)
 create mode 100644 xen/arch/x86/hvm/viridian/time.c
xen/arch/x86/hvm/viridian/Makefile +1 -0
index fca8e16e20..3ecdffe2f6 100644
@@ -1,2 +1,3 @@
 obj-y += synic.o
+obj-y += time.o
 obj-y += viridian.o
xen/arch/x86/hvm/viridian/time.c new file +245 -0

245 lines not on this page · raw message

xen/arch/x86/hvm/viridian/viridian.c +7 -167

234 lines not on this page · raw message

xen/include/asm-x86/hvm/viridian.h +8 -9
index 7e21ca5537..9a26f6bed0 100644
@@ -125,15 +125,6 @@ struct viridian_time_ref_count
     int64_t off;
 };
 
-typedef struct _HV_REFERENCE_TSC_PAGE
-{
-    uint32_t TscSequence;
-    uint32_t Reserved1;
-    uint64_t TscScale;
-    int64_t  TscOffset;
-    uint64_t Reserved2[509];
-} HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;
-
 struct viridian_domain
 {
     union viridian_guest_os_id_msr guest_os_id;
@@ -154,6 +145,14 @@ viridian_hypercall(struct cpu_user_regs *regs);
 void viridian_time_ref_count_freeze(struct domain *d);
 void viridian_time_ref_count_thaw(struct domain *d);
 
+int viridian_time_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val);
+int viridian_time_rdmsr(const struct vcpu *v, uint32_t idx, uint64_t *val);
+
+void viridian_time_save_domain_ctxt(
+    const struct domain *d, struct hvm_viridian_domain_context *ctxt);
+void viridian_time_load_domain_ctxt(
+    struct domain *d, const struct hvm_viridian_domain_context *ctxt);
+
 void viridian_vcpu_deinit(struct vcpu *v);
 void viridian_domain_deinit(struct domain *d);
 
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Reviewed-by on v2 6/9
#10 lore raw

Re: [PATCH v2 6/9] viridian: separate time related enlightenment implementations...

Quoted 15 lines · Paul Durrant

...into new 'time' module.

This patch reduces the size of the main viridian source module by moving time related enlightenments into their own source module. This is done in anticipation of implementation of more such enightenments and a desire to not further lengthen the main source module when this work is done.

While moving the code:

- Move the declaration of HV_REFERENCE_TSC_PAGE from the header file into the new source module, since it is only used there.
- Clean up a bool_t.

[...]

Quoted 9 lines
xen/arch/x86/hvm/viridian/time.c new file +9 -0
+int viridian_time_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val)
+{
+    struct domain *d = v->domain;
+
+    switch ( idx )
+    {
+    case HV_X64_MSR_REFERENCE_TSC:
+        if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
+            return X86EMUL_EXCEPTION;

xen/arch/x86/hvm/viridian/time.c line 159

Quoted 10 lines
xen/arch/x86/hvm/viridian/time.c new file +10 -0
+
+        d->arch.hvm.viridian.reference_tsc.raw = val;
+        dump_reference_tsc(d);
+        if ( d->arch.hvm.viridian.reference_tsc.fields.enabled )
+            update_reference_tsc(d, true);
+        break;
+
+    default:
+        gdprintk(XENLOG_INFO,
+                 "%s: unimplemented MSR %#x (%016"PRIx64")\n",

I believe this can fit into previous line just fine.

Anyway, don't let this minor block this patch.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#11 lore raw

Re: [PATCH v2 6/9] viridian: separate time related enlightenment implementations...

In reply to Paul Durrant
Quoted 19 lines
Quoted 1 line
Quoted 1 line · On 31.10.18 at 13:43, <[email protected]> wrote:
xen/arch/x86/hvm/viridian/time.c new file +1 -0
@@ -0,0 +1,245 @@
+/**************************************************************************

*
+ * time.c
+ *
+ * An implementation of some time related Viridian enlightenments.
+ * See Microsoft's Hypervisor Top Level Functional Specification.
+ * for more information.
+ */

xen/arch/x86/hvm/viridian/time.c new file +7 -0
+
+#include <xen/domain_page.h>
+#include <xen/hypercall.h>
+#include <xen/sched.h>
+#include <xen/version.h>
+
+#include <asm/apic.h>

xen/arch/x86/hvm/viridian/time.c line 24

Quoted 10 lines
xen/arch/x86/hvm/viridian/time.c new file +10 -0
+#include <asm/hvm/support.h>
+
+typedef struct _HV_REFERENCE_TSC_PAGE
+{
+    uint32_t TscSequence;
+    uint32_t Reserved1;
+    uint64_t TscScale;
+    int64_t  TscOffset;
+    uint64_t Reserved2[509];
+} HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;

And you want to continue to have it all upper case, despite us normally using such identifiers for #define-s only?

Apart from that same remark regarding the placement of the function declarations as for the previous patch.

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#12 lore raw

Re: [PATCH v2 6/9] viridian: separate time related enlightenment implementations...

Quoted 35 lines

-----Original Message-----
From: Jan Beulich [mailto:[email protected]]
Sent: 05 November 2018 12:57
To: Paul Durrant <[email protected]>
Cc: Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; xen-devel <[email protected]>
Subject: Re: [PATCH v2 6/9] viridian: separate time related enlightenment implementations...

xen/arch/x86/hvm/viridian/time.c

Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 31.10.18 at 13:43, <[email protected]> wrote:
xen/arch/x86/hvm/viridian/time.c new file +0 -0
@@ -0,0 +1,245 @@

+/************************************************************************ **

Quoted 19 lines

*
+ * time.c
+ *
+ * An implementation of some time related Viridian enlightenments.
+ * See Microsoft's Hypervisor Top Level Functional Specification.
+ * for more information.
+ */

xen/arch/x86/hvm/viridian/time.c new file +12 -0
+
+#include <xen/domain_page.h>
+#include <xen/hypercall.h>
+#include <xen/sched.h>
+#include <xen/version.h>
+
+#include <asm/apic.h>
+#include <asm/hvm/support.h>
+
+typedef struct _HV_REFERENCE_TSC_PAGE
+{
+    uint32_t TscSequence;
Quoted 9 lines

xen/arch/x86/hvm/viridian/time.c line 24

Quoted 5 lines
xen/arch/x86/hvm/viridian/time.c new file +5 -0
+    uint32_t Reserved1;
+    uint64_t TscScale;
+    int64_t  TscOffset;
+    uint64_t Reserved2[509];
+} HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;

And you want to continue to have it all upper case, despite us normally using such identifiers for #define-s only?

It's a definition lifted from the spec. so it's usual Microsoft style... i.e. upper case types and camel-case names. I didn't define UINT32, INT64 and UINT64 types - as used by the spec - but I could do so if you'd prefer the consistency.

Quoted 3 lines

Apart from that same remark regarding the placement of the function declarations as for the previous patch.

I assume you mean in the proposed new header, which is fine.

Paul

Quoted 2 lines

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#13 lore raw

Re: [PATCH v2 6/9] viridian: separate time related enlightenment implementations...

Quoted 40 lines
Quoted 30 lines
Quoted 1 line · On 05.11.18 at 14:26, <[email protected]> wrote:

From: Jan Beulich [mailto:[email protected]]
Sent: 05 November 2018 12:57

xen/arch/x86/hvm/viridian/time.c

Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 31.10.18 at 13:43, <[email protected]> wrote:
xen/arch/x86/hvm/viridian/time.c new file +0 -0
@@ -0,0 +1,245 @@

+/************************************************************************ **

Quoted 19 lines

*
+ * time.c
+ *
+ * An implementation of some time related Viridian enlightenments.
+ * See Microsoft's Hypervisor Top Level Functional Specification.
+ * for more information.
+ */

xen/arch/x86/hvm/viridian/time.c new file +12 -0
+
+#include <xen/domain_page.h>
+#include <xen/hypercall.h>
+#include <xen/sched.h>
+#include <xen/version.h>
+
+#include <asm/apic.h>
+#include <asm/hvm/support.h>
+
+typedef struct _HV_REFERENCE_TSC_PAGE
+{
+    uint32_t TscSequence;
Quoted 9 lines

xen/arch/x86/hvm/viridian/time.c line 24

Quoted 5 lines
xen/arch/x86/hvm/viridian/time.c new file +5 -0
+    uint32_t Reserved1;
+    uint64_t TscScale;
+    int64_t  TscOffset;
+    uint64_t Reserved2[509];
+} HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;

And you want to continue to have it all upper case, despite us normally using such identifiers for #define-s only?

Quoted 4 lines

It's a definition lifted from the spec. so it's usual Microsoft style... i.e. upper case types and camel-case names. I didn't define UINT32, INT64 and UINT64 types - as used by the spec - but I could do so if you'd prefer the consistency.

Oh, no, I'm not looking forward to see UINT<n> appear. I was rather asking whether the odd all-upper-case naming could be changed, to be in line with our style rather than Microsoft's.

Quoted 4 lines
Quoted 2 lines

Apart from that same remark regarding the placement of the function declarations as for the previous patch.

I assume you mean in the proposed new header, which is fine.

Yes, that's what I mean.

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#14 lore raw

Re: [PATCH v2 6/9] viridian: separate time related enlightenment implementations...

Quoted 53 lines

-----Original Message-----
From: Jan Beulich [mailto:[email protected]]
Sent: 05 November 2018 13:37
To: Paul Durrant <[email protected]>
Cc: Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; xen-devel <[email protected]>
Subject: RE: [PATCH v2 6/9] viridian: separate time related enlightenment implementations...

Quoted 10 lines
Quoted 10 lines
Quoted 1 line · On 05.11.18 at 14:26, <[email protected]> wrote:

From: Jan Beulich [mailto:[email protected]]
Sent: 05 November 2018 12:57

xen/arch/x86/hvm/viridian/time.c

Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 31.10.18 at 13:43, <[email protected]> wrote:
xen/arch/x86/hvm/viridian/time.c new file +0 -0
@@ -0,0 +1,245 @@

+/************************************************************************

Quoted 30 lines
Quoted 20 lines

**

Quoted 19 lines

*
+ * time.c
+ *
+ * An implementation of some time related Viridian enlightenments.
+ * See Microsoft's Hypervisor Top Level Functional Specification.
+ * for more information.
+ */

xen/arch/x86/hvm/viridian/time.c new file +12 -0
+
+#include <xen/domain_page.h>
+#include <xen/hypercall.h>
+#include <xen/sched.h>
+#include <xen/version.h>
+
+#include <asm/apic.h>
+#include <asm/hvm/support.h>
+
+typedef struct _HV_REFERENCE_TSC_PAGE
+{
+    uint32_t TscSequence;
Quoted 9 lines

xen/arch/x86/hvm/viridian/time.c line 24

Quoted 5 lines
xen/arch/x86/hvm/viridian/time.c new file +5 -0
+    uint32_t Reserved1;
+    uint64_t TscScale;
+    int64_t  TscOffset;
+    uint64_t Reserved2[509];
+} HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;

And you want to continue to have it all upper case, despite us normally using such identifiers for #define-s only?

Quoted 2 lines

It's a definition lifted from the spec. so it's usual Microsoft style... i.e. upper case types and camel-case names. I didn't define UINT32,

INT64 and

Quoted 9 lines
Quoted 1 line

UINT64 types - as used by the spec - but I could do so if you'd prefer

the

Quoted 1 line

consistency.

Oh, no, I'm not looking forward to see UINT<n> appear. I was rather asking whether the odd all-upper-case naming could be changed, to be in line with our style rather than Microsoft's.

Where typedefs are actually given in the spec I prefer to lift them, even though they do not adhere to our coding style. I could but a document in the viridian subdirectory stating this if you'd like.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Apart from that same remark regarding the placement of the function declarations as for the previous patch.

I assume you mean in the proposed new header, which is fine.

Yes, that's what I mean.

Ok.

Paul

Quoted 2 lines

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#15 lore raw

Re: [PATCH v2 6/9] viridian: separate time related enlightenment implementations...

Quoted 4 lines
Quoted 1 line
Quoted 1 line · On 05.11.18 at 14:54, <[email protected]> wrote:

Where typedefs are actually given in the spec I prefer to lift them, even though they do not adhere to our coding style. I could but a document in the viridian subdirectory stating this if you'd like.

I don't think that's worth it.

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#16 lore raw

PATCH v2 7/9 viridian: define type for the 'virtual VP assist page'

In reply to Paul Durrant

The specification [1] defines a type so we should use it, rather than just OR-ing and AND-ing magic bits.

No functional change.

NOTE: The type defined in the specification does include an anonymous
sub-struct in the page type but, as we currently use only the first element, the struct declaration has been omitted.

[1] https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0C.pdf

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

---
 xen/arch/x86/hvm/viridian/synic.c  | 52 +++++++++++++++++++++++---------------
 xen/include/asm-x86/hvm/viridian.h |  2 +-
 2 files changed, 33 insertions(+), 21 deletions(-)
xen/arch/x86/hvm/viridian/synic.c +32 -20
index e01ac941ea..5500d44186 100644
@@ -14,6 +14,18 @@
 #include <asm/apic.h>
 #include <asm/hvm/support.h>
 
+typedef struct _HV_VIRTUAL_APIC_ASSIST
+{
+    uint32_t no_eoi:1;
+    uint32_t reserved_zero:31;
+} HV_VIRTUAL_APIC_ASSIST;
+
+typedef union _HV_VP_ASSIST_PAGE
+{
+    HV_VIRTUAL_APIC_ASSIST ApicAssist;
+    uint8_t ReservedZBytePadding[PAGE_SIZE];
+} HV_VP_ASSIST_PAGE;
+
 static void dump_vp_assist(const struct vcpu *v)
 {
     const union viridian_page_msr *va = &v->arch.hvm.viridian.vp_assist.msr;
@@ -30,9 +42,9 @@ static void initialize_vp_assist(struct vcpu *v)
     struct domain *d = v->domain;
     unsigned long gmfn = v->arch.hvm.viridian.vp_assist.msr.fields.pfn;
     struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
-    void *va;
+    HV_VP_ASSIST_PAGE *ptr;
 
-    ASSERT(!v->arch.hvm.viridian.vp_assist.va);
+    ASSERT(!v->arch.hvm.viridian.vp_assist.ptr);
 
     if ( !page )
         goto fail;
@@ -43,16 +55,16 @@ static void initialize_vp_assist(struct vcpu *v)
         goto fail;
     }
 
-    va = __map_domain_page_global(page);
-    if ( !va )
+    ptr = __map_domain_page_global(page);
+    if ( !ptr )
     {
         put_page_and_type(page);
         goto fail;
     }
 
-    clear_page(va);
+    clear_page(ptr);
 
-    v->arch.hvm.viridian.vp_assist.va = va;
+    v->arch.hvm.viridian.vp_assist.ptr = ptr;
     return;
 
  fail:
@@ -62,25 +74,25 @@ static void initialize_vp_assist(struct vcpu *v)
 
 static void teardown_vp_assist(struct vcpu *v)
 {
-    void *va = v->arch.hvm.viridian.vp_assist.va;
+    HV_VP_ASSIST_PAGE *ptr = v->arch.hvm.viridian.vp_assist.ptr;
     struct page_info *page;
 
-    if ( !va )
+    if ( !ptr )
         return;
 
-    v->arch.hvm.viridian.vp_assist.va = NULL;
+    v->arch.hvm.viridian.vp_assist.ptr = NULL;
 
-    page = mfn_to_page(domain_page_map_to_mfn(va));
+    page = mfn_to_page(domain_page_map_to_mfn(ptr));
 
-    unmap_domain_page_global(va);
+    unmap_domain_page_global(ptr);
     put_page_and_type(page);
 }
 
 void viridian_apic_assist_set(struct vcpu *v)
 {
-    uint32_t *va = v->arch.hvm.viridian.vp_assist.va;
+    HV_VP_ASSIST_PAGE *ptr = v->arch.hvm.viridian.vp_assist.ptr;
 
-    if ( !va )
+    if ( !ptr )
         return;
 
     /*
@@ -92,18 +104,18 @@ void viridian_apic_assist_set(struct vcpu *v)
         domain_crash(v->domain);
 
     v->arch.hvm.viridian.vp_assist.pending = true;
-    *va |= 1u;
+    ptr->ApicAssist.no_eoi = 1;
 }
 
 bool viridian_apic_assist_completed(struct vcpu *v)
 {
-    uint32_t *va = v->arch.hvm.viridian.vp_assist.va;
+    HV_VP_ASSIST_PAGE *ptr = v->arch.hvm.viridian.vp_assist.ptr;
 
-    if ( !va )
+    if ( !ptr )
         return false;
 
     if ( v->arch.hvm.viridian.vp_assist.pending &&
-         !(*va & 1u) )
+         !ptr->ApicAssist.no_eoi )
     {
         /* An EOI has been avoided */
         v->arch.hvm.viridian.vp_assist.pending = false;
@@ -115,12 +127,12 @@ bool viridian_apic_assist_completed(struct vcpu *v)
 
 void viridian_apic_assist_clear(struct vcpu *v)
 {
-    uint32_t *va = v->arch.hvm.viridian.vp_assist.va;
+    HV_VP_ASSIST_PAGE *ptr = v->arch.hvm.viridian.vp_assist.ptr;
 
-    if ( !va )
+    if ( !ptr )
         return;
 
-    *va &= ~1u;
+    ptr->ApicAssist.no_eoi = 0;
     v->arch.hvm.viridian.vp_assist.pending = false;
 }
 
xen/include/asm-x86/hvm/viridian.h +1 -1
index 9a26f6bed0..11abf77198 100644
@@ -92,7 +92,7 @@ struct viridian_vcpu
 {
     struct {
         union viridian_page_msr msr;
-        void *va;
+        void *ptr;
         bool pending;
     } vp_assist;
     uint64_t crash_param[5];
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Reviewed-by on v2 7/9
#17 lore raw

Re: [PATCH v2 7/9] viridian: define type for the 'virtual VP assist page'

Quoted 5 lines · Paul Durrant

The specification [1] defines a type so we should use it, rather than just OR-ing and AND-ing magic bits.

No functional change.

Quoted 7 lines

NOTE: The type defined in the specification does include an anonymous
sub-struct in the page type but, as we currently use only the first element, the struct declaration has been omitted.

[1] https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0C.pdf

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#18 lore raw

Re: [PATCH v2 7/9] viridian: define type for the 'virtual VP assist page'

In reply to Paul Durrant

xen/include/asm-x86/hvm/viridian.h struct viridian_vcpu · line 95

Quoted 9 lines
Quoted 1 line
Quoted 1 line · On 31.10.18 at 13:43, <[email protected]> wrote:
xen/include/asm-x86/hvm/viridian.h +1 -1
@@ -92,7 +92,7 @@ struct viridian_vcpu
 {
     struct {
         union viridian_page_msr msr;
-        void *va;
+        void *ptr;

Why void rather than the actual type? You can't use the typedef (for it being local to the .c file), but you can use the union tag here in a forward declaring manner.

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#19 lore raw

Re: [PATCH v2 7/9] viridian: define type for the 'virtual VP assist page'

Quoted 10 lines

-----Original Message-----
From: Jan Beulich [mailto:[email protected]]
Sent: 05 November 2018 13:00
To: Paul Durrant <[email protected]>
Cc: Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; xen-devel <[email protected]>
Subject: Re: [PATCH v2 7/9] viridian: define type for the 'virtual VP assist page'

Quoted 1 line
Quoted 1 line
Quoted 1 line · On 31.10.18 at 13:43, <[email protected]> wrote:
Quoted 13 lines

xen/include/asm-x86/hvm/viridian.h struct viridian_vcpu · line 95

Quoted 8 lines
xen/include/asm-x86/hvm/viridian.h +1 -1
@@ -92,7 +92,7 @@ struct viridian_vcpu
 {
     struct {
         union viridian_page_msr msr;
-        void *va;
+        void *ptr;

Why void rather than the actual type? You can't use the typedef (for it being local to the .c file), but you can use the union tag here in a forward declaring manner.

Ok. I'll have a look at that.

Paul

Quoted 2 lines

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#20 lore raw

PATCH v2 8/9 tools/misc: fix hard tabs in xen-hvmctx.c

In reply to Paul Durrant

Also add emacs boilerplate to avoid future problems.

Purely cosmetic. No functional change.

---
Cc: Ian Jackson <[email protected]>
Cc: Wei Liu <[email protected]>

v2:
- New in v2

---
 tools/misc/xen-hvmctx.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
tools/misc/xen-hvmctx.c +13 -3
index 40e77851be..823aa7d736 100644
@@ -371,8 +371,8 @@ static void dump_viridian_vcpu(void)
     HVM_SAVE_TYPE(VIRIDIAN_VCPU) p;
     READ(p);
     printf("    VIRIDIAN_VCPU: vp_assist_msr 0x%llx, vp_assist_pending %s\n",
-	   (unsigned long long) p.vp_assist_msr,
-	   p.vp_assist_pending ? "true" : "false");
+           (unsigned long long) p.vp_assist_msr,
+           p.vp_assist_pending ? "true" : "false");
 }
 
 static void dump_vmce_vcpu(void)
@@ -468,4 +468,14 @@ int main(int argc, char **argv)
     } while ( desc.typecode != HVM_SAVE_CODE(END) && off < len );
 
     return 0;
-} 
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
#22 lore raw

PATCH v2 9/9 viridian: introduce struct viridian_page

In reply to Paul Durrant

The 'vp_assist' page is currently an example of a guest page which needs to be kept mapped throughout the life-time of a guest, but there are other such examples in the specifiction [1]. This patch therefore introduces a generic 'viridian_page' type and converts the current vp_assist/apic_assist related code to use it. Subsequent patches implementing other enlightments can then also make use of it.

This patch also renames the 'vp_assist_pending' field in struct hvm_viridian_vcpu_context to 'apic_assist_pending' to more accurately reflect its meaning. The term 'vp_assist' applies to the whole page rather than just the EOI-avoidance enlightenment. New versons of the specification have defined data structures for other enlightenments within the same page.

No functional change.

[1] https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0C.pdf

---
Cc: Ian Jackson <[email protected]>
Cc: Wei Liu <[email protected]>
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>

---
 tools/misc/xen-hvmctx.c                |  4 +--
 xen/arch/x86/hvm/viridian/synic.c      | 47 ++++++++++++++++------------------
 xen/include/asm-x86/hvm/viridian.h     | 13 ++++++----
 xen/include/public/arch-x86/hvm/save.h |  2 +-
 4 files changed, 33 insertions(+), 33 deletions(-)
tools/misc/xen-hvmctx.c +2 -2
index 823aa7d736..4f336a6cea 100644
@@ -370,9 +370,9 @@ static void dump_viridian_vcpu(void)
 {
     HVM_SAVE_TYPE(VIRIDIAN_VCPU) p;
     READ(p);
-    printf("    VIRIDIAN_VCPU: vp_assist_msr 0x%llx, vp_assist_pending %s\n",
+    printf("    VIRIDIAN_VCPU: vp_assist_msr 0x%llx, apic_assist_pending %s\n",
            (unsigned long long) p.vp_assist_msr,
-           p.vp_assist_pending ? "true" : "false");
+           p.apic_assist_pending ? "true" : "false");
 }
 
 static void dump_vmce_vcpu(void)
xen/arch/x86/hvm/viridian/synic.c +22 -25

126 lines not on this page · raw message

xen/include/asm-x86/hvm/viridian.h +8 -5
index 11abf77198..378959920d 100644
@@ -88,13 +88,16 @@ union viridian_page_msr
     } fields;
 };
 
+struct viridian_page
+{
+    union viridian_page_msr msr;
+    void *ptr;
+};
+
 struct viridian_vcpu
 {
-    struct {
-        union viridian_page_msr msr;
-        void *ptr;
-        bool pending;
-    } vp_assist;
+    struct viridian_page vp_assist;
+    bool apic_assist_pending;
     uint64_t crash_param[5];
 };
 
xen/include/public/arch-x86/hvm/save.h +1 -1
index 4691d4d4aa..80e762c335 100644
@@ -600,7 +600,7 @@ DECLARE_HVM_SAVE_TYPE(VIRIDIAN_DOMAIN, 15, struct hvm_viridian_domain_context);
 
 struct hvm_viridian_vcpu_context {
     uint64_t vp_assist_msr;
-    uint8_t  vp_assist_pending;
+    uint8_t  apic_assist_pending;
     uint8_t  _pad[7];
 };
 
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Reviewed-by on v2 9/9
#23 lore raw

Re: [PATCH v2 9/9] viridian: introduce struct viridian_page

Quoted 13 lines · Paul Durrant

The 'vp_assist' page is currently an example of a guest page which needs to be kept mapped throughout the life-time of a guest, but there are other such examples in the specifiction [1]. This patch therefore introduces a generic 'viridian_page' type and converts the current vp_assist/apic_assist related code to use it. Subsequent patches implementing other enlightments can then also make use of it.

This patch also renames the 'vp_assist_pending' field in struct hvm_viridian_vcpu_context to 'apic_assist_pending' to more accurately reflect its meaning. The term 'vp_assist' applies to the whole page rather than just the EOI-avoidance enlightenment. New versons of the specification have defined data structures for other enlightenments within the same page.

Quoted 6 lines

No functional change.

[1] https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0C.pdf

xen/include/public/arch-x86/hvm/save.h DECLARE_HVM_SAVE_TYPE(VIRIDIAN_DOMAIN, 15, struct hvm_viridian_domain_context); · line 603

Quoted 10 lines
xen/include/public/arch-x86/hvm/save.h +1 -1
index 4691d4d4aa..80e762c335 100644
@@ -600,7 +600,7 @@ DECLARE_HVM_SAVE_TYPE(VIRIDIAN_DOMAIN, 15, struct hvm_viridian_domain_context);
 
 struct hvm_viridian_vcpu_context {
     uint64_t vp_assist_msr;
-    uint8_t  vp_assist_pending;
+    uint8_t  apic_assist_pending;

FTR I believe it is safe to do so because this is (eventually) used by domctl interfaces.

Wei.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#24 lore raw

Re: [PATCH v2 9/9] viridian: introduce struct viridian_page

In reply to Paul Durrant
Quoted 7 lines
Quoted 1 line
Quoted 1 line · On 31.10.18 at 13:43, <[email protected]> wrote:

The 'vp_assist' page is currently an example of a guest page which needs to be kept mapped throughout the life-time of a guest, but there are other such examples in the specifiction [1]. This patch therefore introduces a generic 'viridian_page' type and converts the current vp_assist/apic_assist related code to use it. Subsequent patches implementing other enlightments can then also make use of it.

This sounds generic (as in "not synic specific), yet ...

Quoted 10 lines
xen/arch/x86/hvm/viridian/synic.c +1 -1
@@ -37,14 +37,13 @@ static void dump_vp_assist(const struct vcpu *v)
            v, (unsigned long)va->fields.pfn);
 }
 
-static void initialize_vp_assist(struct vcpu *v)
+static void initialize_guest_page(struct vcpu *v, struct viridian_page *vp)
 {
     struct domain *d = v->domain;

xen/arch/x86/hvm/viridian/synic.c static void dump_vp_assist(const struct vcpu *v) · line 49

Quoted 10 lines
xen/arch/x86/hvm/viridian/synic.c +2 -3
-    unsigned long gmfn = v->arch.hvm.viridian.vp_assist.msr.fields.pfn;
+    unsigned long gmfn = vp->msr.fields.pfn;
     struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
-    HV_VP_ASSIST_PAGE *ptr;
 
-    ASSERT(!v->arch.hvm.viridian.vp_assist.ptr);
+    ASSERT(!vp->ptr);
 
     if ( !page )
         goto fail;

... you retain the implementation here, when it would now perhaps more logically live in viridian.c (again). Is this intentional?

xen/arch/x86/hvm/viridian/synic.c void viridian_synic_load_vcpu_ctxt( · line 223

Quoted 9 lines
xen/arch/x86/hvm/viridian/synic.c +2 -2
@@ -221,9 +218,9 @@ void viridian_synic_load_vcpu_ctxt(
 {
     v->arch.hvm.viridian.vp_assist.msr.raw = ctxt->vp_assist_msr;
     if ( v->arch.hvm.viridian.vp_assist.msr.fields.enabled )
-        initialize_vp_assist(v);
+        initialize_guest_page(v, &v->arch.hvm.viridian.vp_assist);
 
-    v->arch.hvm.viridian.vp_assist.pending = !!ctxt->vp_assist_pending;
+    v->arch.hvm.viridian.apic_assist_pending = !!ctxt->apic_assist_pending;

No need for !! anymore with ...

Quoted 11 lines
xen/include/asm-x86/hvm/viridian.h +5 -0
@@ -88,13 +88,16 @@ union viridian_page_msr
     } fields;
 };
 
+struct viridian_page
+{
+    union viridian_page_msr msr;
+    void *ptr;
+};

xen/include/asm-x86/hvm/viridian.h union viridian_page_msr · line 100

Quoted 10 lines
xen/include/asm-x86/hvm/viridian.h +3 -5
+
 struct viridian_vcpu
 {
-    struct {
-        union viridian_page_msr msr;
-        void *ptr;
-        bool pending;
-    } vp_assist;
+    struct viridian_page vp_assist;
+    bool apic_assist_pending;

... this being (and having been) bool.

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#25 lore raw

Re: [PATCH v2 9/9] viridian: introduce struct viridian_page

Quoted 15 lines

-----Original Message-----
From: Jan Beulich [mailto:[email protected]]
Sent: 05 November 2018 13:06
To: Paul Durrant <[email protected]>
Cc: Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; Ian Jackson <[email protected]>; xen-devel <[email protected]>
Subject: Re: [PATCH v2 9/9] viridian: introduce struct viridian_page

Quoted 2 lines
Quoted 1 line
Quoted 1 line · On 31.10.18 at 13:43, <[email protected]> wrote:

The 'vp_assist' page is currently an example of a guest page which needs

to

Quoted 3 lines

be kept mapped throughout the life-time of a guest, but there are other such examples in the specifiction [1]. This patch therefore introduces a generic 'viridian_page' type and converts the current

Quoted 7 lines

vp_assist/apic_assist

Quoted 1 line

related code to use it. Subsequent patches implementing other

enlightments

Quoted 1 line

can then also make use of it.

This sounds generic (as in "not synic specific), yet ...

Actually Microsoft lump pretty much all the interrupt enlightenments under the synic section in the spec. so I thought it better to put them all together.

Quoted 17 lines

xen/arch/x86/hvm/viridian/synic.c static void dump_vp_assist(const struct vcpu *v) · line 40

Quoted 8 lines
xen/arch/x86/hvm/viridian/synic.c +1 -1
@@ -37,14 +37,13 @@ static void dump_vp_assist(const struct vcpu *v)
            v, (unsigned long)va->fields.pfn);
 }
 
-static void initialize_vp_assist(struct vcpu *v)
+static void initialize_guest_page(struct vcpu *v, struct viridian_page

*vp)

xen/arch/x86/hvm/viridian/synic.c static void dump_vp_assist(const struct vcpu *v)

Quoted 5 lines
xen/arch/x86/hvm/viridian/synic.c +1 -1
 {
     struct domain *d = v->domain;
-    unsigned long gmfn = v->arch.hvm.viridian.vp_assist.msr.fields.pfn;
+    unsigned long gmfn = vp->msr.fields.pfn;
     struct page_info *page = get_page_from_gfn(d, gmfn, NULL,

P2M_ALLOC);

Quoted 2 lines
xen/arch/x86/hvm/viridian/synic.c +0 -1
-    HV_VP_ASSIST_PAGE *ptr;
 
Quoted 9 lines

xen/arch/x86/hvm/viridian/synic.c static void dump_vp_assist(const struct vcpu *v) · line 49

Quoted 5 lines
xen/arch/x86/hvm/viridian/synic.c +1 -1
-    ASSERT(!v->arch.hvm.viridian.vp_assist.ptr);
+    ASSERT(!vp->ptr);
 
     if ( !page )
         goto fail;

... you retain the implementation here, when it would now perhaps more logically live in viridian.c (again). Is this intentional?

Not really, it's down the patch ordering. I agree it makes more sense to put it under viridian.c now. I'll move it.

Quoted 28 lines

xen/arch/x86/hvm/viridian/synic.c void viridian_synic_load_vcpu_ctxt( · line 224

Quoted 10 lines
xen/arch/x86/hvm/viridian/synic.c +2 -2
@@ -221,9 +218,9 @@ void viridian_synic_load_vcpu_ctxt(
 {
     v->arch.hvm.viridian.vp_assist.msr.raw = ctxt->vp_assist_msr;
     if ( v->arch.hvm.viridian.vp_assist.msr.fields.enabled )
-        initialize_vp_assist(v);
+        initialize_guest_page(v, &v->arch.hvm.viridian.vp_assist);
 
-    v->arch.hvm.viridian.vp_assist.pending = !!ctxt->vp_assist_pending;
+    v->arch.hvm.viridian.apic_assist_pending = !!ctxt-
 apic_assist_pending;

No need for !! anymore with ...

Quoted 15 lines
xen/include/asm-x86/hvm/viridian.h +6 -1
@@ -88,13 +88,16 @@ union viridian_page_msr
     } fields;
 };
 
+struct viridian_page
+{
+    union viridian_page_msr msr;
+    void *ptr;
+};
+
 struct viridian_vcpu
 {
-    struct {
Quoted 9 lines

xen/include/asm-x86/hvm/viridian.h union viridian_page_msr · line 100

Quoted 6 lines
xen/include/asm-x86/hvm/viridian.h +2 -4
-        union viridian_page_msr msr;
-        void *ptr;
-        bool pending;
-    } vp_assist;
+    struct viridian_page vp_assist;
+    bool apic_assist_pending;

... this being (and having been) bool.

True, I'll get rid of that.

Paul

Quoted 2 lines

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel