On 05/19/2015 02:48 AM, Jan Beulich wrote:
On 18.05.15 at 18:12, <boris.ostrov...@oracle.com> wrote:
On 05/18/2015 11:15 AM, Jan Beulich wrote:
On 08.05.15 at 23:06, <boris.ostrov...@oracle.com> wrote:
+/*
+ * Architecture-specific information describing state of the processor at
+ * the time of PMU interrupt.
+ * Fields of this structure marked as RW for guest can only be written by the
+ * guest when PMU_CACHED bit in pmu_flags is set (which is done by the
+ * hypervisor during PMU interrupt). Hypervisor will read updated data in
+ * XENPMU_flush hypercall and clear PMU_CACHED bit.
+ */
+struct xen_pmu_arch {
+ union {
+ /*
+ * Processor's registers at the time of interrupt.
+ * RW for hypervisor, RO for guests.
+ */
+ struct xen_pmu_regs regs;
+ /* Padding for adding new registers to xen_pmu_regs in the future */
+#define XENPMU_REGS_PAD_SZ 64
+ uint8_t pad[XENPMU_REGS_PAD_SZ];
+ } r;
+
+ /* RW for hypervisor, RO for guest */
+ uint64_t pmu_flags;
+
+ /*
+ * APIC LVTPC register.
+ * RW for both hypervisor and guest.
+ * Only APIC_LVT_MASKED bit is loaded by the hypervisor into hardware
+ * during XENPMU_flush.
+ */
+ union {
+ uint32_t lapic_lvtpc;
+ uint64_t pad;
+ } l;
+
+ /*
+ * Vendor-specific PMU registers.
+ * RW for both hypervisor and guest.
+ * Guest's updates to this field are verified and then loaded by the
+ * hypervisor into hardware during XENPMU_flush
+ */
+ union {
+ struct xen_pmu_amd_ctxt amd;
+ struct xen_pmu_intel_ctxt intel;
+
+ /*
+ * Padding for contexts (fixed parts only, does not include MSR banks
+ * that are specified by offsets)
+ */
+#define XENPMU_CTXT_PAD_SZ 128
+ uint8_t pad[XENPMU_CTXT_PAD_SZ];
+ } c;
+};
Marking all the fields RW for the hypervisor is certainly correct from
a permissions pov, but requires close auditing that the hypervisor
doesn't ever read a field twice, potentially getting different results
and hence inconsistent internal state. Therefore - do all of the fields
_need_ to be RW for the hypervisor? If not, marking the ones
where this isn't needed as WO would be much preferred, to limit
the scope of whats needs to be audited.
Right, all arch-independent bits are WO for hypervisor as are
xen_pmu_regs above. I in fact meant to label them as such but for
reasons that I can't remember now decided to mark them as RW.
Okay, that simplifies things for review purposes: Of the left
fields, lapic_lvtpc can easily be verified to be read just once, and
the vendor specific context gets copied into hypervisor memory
once before doing verification and loading. Which leaves pmu_flags:
Can you clarify how the read-write behavior there is expected to
be (perhaps by slightly extending the respective comment)? I ask
in particular because I don't recall having seen any read-once
enforcement in the code.
pmu_flags are not read-once by neither hypervisor nor the guest. The
hypervisor uses PMU_CACHED flag (which it sets in the PMU interrupt) to
later determine whether or not to perform things like VPMU load (if the
flag is set then there is no reason to load). This may happen more than
once.
The rest of flags are WO by the hypervisor.
For the guest PMU_CACHED is indication that it shouldn't write MSRs
directly but rather do this into the shared page.
I don't believe that if guest writes flags (PMU_CACHED specifically) it
will have any effect on the hypervisor or other guests. It will
certainly affect the guest itself as it will probably mess up its VPMU
state. For example, we may end up loading the VPMU context when it
shouldn't be and that may result in an unexpected interrupt for the guest.
I can easily add a hypervisor-private flag for this purpose (into
vpmu_struct.flags) to make things more cleanly delineated (i.e.
pmu_flags will be strictly WO by hypervisor and RO by the guest). I
still want to keep PMU_CACHED flag for the guest though so that it knows
where to write MSRs (Linux PV guests don't need this right now but other
guests may find this useful).
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
