Re: [Xen-devel] [PATCH v22 02/14] x86/VPMU: Add public xenpmu.h
>>> On 27.05.15 at 17:18, wrote:
> On 05/27/2015 10:26 AM, Jan Beulich wrote:
> On 27.05.15 at 15:44, wrote:
>>> Sorry, I meant amd/intel members of the union below (I forgot we were
>>> already in the arch header file):
>>>
>>> +/*
>>> + * 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;
>>> +};
>>>
>>> I think they are first used in patch 11 so I assume you also want me to
>>> just keep the pad here (with a comment explaining why it is here) until
>>> that patch.
>> Ah, those ones I simply recalled having checked in the previous
>> version already.
>>
>
> But should they they also not be defined until later patch, to be
> consistent with how lapic_lvtpc's definition is deferred?
I think that would be better.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v22 02/14] x86/VPMU: Add public xenpmu.h
On 05/27/2015 10:26 AM, Jan Beulich wrote:
On 27.05.15 at 15:44, wrote:
Sorry, I meant amd/intel members of the union below (I forgot we were
already in the arch header file):
+/*
+ * 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;
+};
I think they are first used in patch 11 so I assume you also want me to
just keep the pad here (with a comment explaining why it is here) until
that patch.
Ah, those ones I simply recalled having checked in the previous
version already.
But should they they also not be defined until later patch, to be
consistent with how lapic_lvtpc's definition is deferred?
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v22 02/14] x86/VPMU: Add public xenpmu.h
>>> On 27.05.15 at 15:44, wrote:
> Sorry, I meant amd/intel members of the union below (I forgot we were
> already in the arch header file):
>
> +/*
> + * 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;
> +};
>
> I think they are first used in patch 11 so I assume you also want me to
> just keep the pad here (with a comment explaining why it is here) until
> that patch.
Ah, those ones I simply recalled having checked in the previous
version already.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v22 02/14] x86/VPMU: Add public xenpmu.h
On 05/27/2015 08:28 AM, Jan Beulich wrote:
On 26.05.15 at 19:50, wrote:
On 05/26/2015 12:13 PM, Jan Beulich wrote:
On 21.05.15 at 19:57, wrote:
+ * 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.
+ * WO 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;
+
+/* WO 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 or XENPMU_lvtpc_set.
+ */
+union {
+uint32_t lapic_lvtpc;
Considering this isn't being used in this patch, could I ask you to
move it where it is being used (keeping only the pad member and
perhaps a placeholder comment here), so verifying that the
read-once requirement for the hypervisor side is met becomes
more obvious?
I can certainly delay defining this field until later patch but how is
this filed different from xen_pmu_arch (not seen here) which is also
read-once? Wouldn't you then want to have that definition deferred as well?
I'm confused - the only xen_pmu_arch I see in this patch is
"struct xen_pmu_arch" (and its uses), which the field above is
actually part of, and which is also visible in the context above. So
I doubt that's what you're referring to. But yes, fields with read-
once requirements would better be defined in the patch(es) using
them, so reviewers don't need to hunt them down.
Sorry, I meant amd/intel members of the union below (I forgot we were
already in the arch header file):
+/*
+ * 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;
+};
I think they are first used in patch 11 so I assume you also want me to
just keep the pad here (with a comment explaining why it is here) until
that patch.
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v22 02/14] x86/VPMU: Add public xenpmu.h
>>> On 26.05.15 at 19:50, wrote:
> On 05/26/2015 12:13 PM, Jan Beulich wrote:
> On 21.05.15 at 19:57, wrote:
>>>
>>> + * 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.
>>> + * WO 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;
>>> +
>>> +/* WO 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 or XENPMU_lvtpc_set.
>>> + */
>>> +union {
>>> +uint32_t lapic_lvtpc;
>> Considering this isn't being used in this patch, could I ask you to
>> move it where it is being used (keeping only the pad member and
>> perhaps a placeholder comment here), so verifying that the
>> read-once requirement for the hypervisor side is met becomes
>> more obvious?
>
> I can certainly delay defining this field until later patch but how is
> this filed different from xen_pmu_arch (not seen here) which is also
> read-once? Wouldn't you then want to have that definition deferred as well?
I'm confused - the only xen_pmu_arch I see in this patch is
"struct xen_pmu_arch" (and its uses), which the field above is
actually part of, and which is also visible in the context above. So
I doubt that's what you're referring to. But yes, fields with read-
once requirements would better be defined in the patch(es) using
them, so reviewers don't need to hunt them down.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v22 02/14] x86/VPMU: Add public xenpmu.h
On 05/26/2015 12:13 PM, Jan Beulich wrote:
On 21.05.15 at 19:57, wrote:
+ * 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.
+ * WO 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;
+
+/* WO 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 or XENPMU_lvtpc_set.
+ */
+union {
+uint32_t lapic_lvtpc;
Considering this isn't being used in this patch, could I ask you to
move it where it is being used (keeping only the pad member and
perhaps a placeholder comment here), so verifying that the
read-once requirement for the hypervisor side is met becomes
more obvious?
I can certainly delay defining this field until later patch but how is
this filed different from xen_pmu_arch (not seen here) which is also
read-once? Wouldn't you then want to have that definition deferred as well?
-boris
With these adjusted, Acked-by: Jan Beulich .
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v22 02/14] x86/VPMU: Add public xenpmu.h
>>> On 21.05.15 at 19:57, wrote:
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -434,6 +434,11 @@ struct xen_arch_domainconfig {
>
> #endif
>
> +#ifndef __ASSEMBLY__
> +/* Stub definition of PMU structure */
> +typedef struct xen_pmu_arch {} xen_pmu_arch_t;
I'm afraid structures without members aren't standard C, so this
needs a dummy field as being in a public interface. I'm sorry for
not noticing this earlier.
> +/*
> + * 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
s/can/should/ ?
> + * 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.
> + * WO 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;
> +
> +/* WO 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 or XENPMU_lvtpc_set.
> + */
> +union {
> +uint32_t lapic_lvtpc;
Considering this isn't being used in this patch, could I ask you to
move it where it is being used (keeping only the pad member and
perhaps a placeholder comment here), so verifying that the
read-once requirement for the hypervisor side is met becomes
more obvious?
With these adjusted, Acked-by: Jan Beulich .
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v22 02/14] x86/VPMU: Add public xenpmu.h
Add pmu.h header files, move various macros and structures that will be
shared between hypervisor and PV guests to it.
Move MSR banks out of architectural PMU structures to allow for larger sizes
in the future. The banks are allocated immediately after the context and
PMU structures store offsets to them.
While making these updates, also:
* Remove unused vpmu_domain() macro from vpmu.h
* Convert msraddr_to_bitpos() into an inline and make it a little faster by
realizing that all Intel's PMU-related MSRs are in the lower MSR range.
Signed-off-by: Boris Ostrovsky
Acked-by: Kevin Tian
---
Changes in v22:
* Clarified comments in public header files describing access permissions for
shared fields
xen/arch/x86/hvm/svm/vpmu.c | 83 +++--
xen/arch/x86/hvm/vmx/vpmu_core2.c| 123 +--
xen/arch/x86/hvm/vpmu.c | 8 ++
xen/arch/x86/oprofile/op_model_ppro.c| 6 +-
xen/include/Makefile | 3 +-
xen/include/asm-x86/hvm/vmx/vpmu_core2.h | 32
xen/include/asm-x86/hvm/vpmu.h | 16 ++--
xen/include/public/arch-arm.h| 5 ++
xen/include/public/arch-x86/pmu.h| 122 ++
xen/include/public/pmu.h | 58 +++
xen/include/xlat.lst | 6 ++
11 files changed, 328 insertions(+), 134 deletions(-)
delete mode 100644 xen/include/asm-x86/hvm/vmx/vpmu_core2.h
create mode 100644 xen/include/public/arch-x86/pmu.h
create mode 100644 xen/include/public/pmu.h
diff --git a/xen/arch/x86/hvm/svm/vpmu.c b/xen/arch/x86/hvm/svm/vpmu.c
index 6764070..a8b79df 100644
--- a/xen/arch/x86/hvm/svm/vpmu.c
+++ b/xen/arch/x86/hvm/svm/vpmu.c
@@ -30,10 +30,7 @@
#include
#include
#include
-
-#define F10H_NUM_COUNTERS 4
-#define F15H_NUM_COUNTERS 6
-#define MAX_NUM_COUNTERS F15H_NUM_COUNTERS
+#include
#define MSR_F10H_EVNTSEL_GO_SHIFT 40
#define MSR_F10H_EVNTSEL_EN_SHIFT 22
@@ -49,6 +46,9 @@ static const u32 __read_mostly *counters;
static const u32 __read_mostly *ctrls;
static bool_t __read_mostly k7_counters_mirrored;
+#define F10H_NUM_COUNTERS 4
+#define F15H_NUM_COUNTERS 6
+
/* PMU Counter MSRs. */
static const u32 AMD_F10H_COUNTERS[] = {
MSR_K7_PERFCTR0,
@@ -83,12 +83,14 @@ static const u32 AMD_F15H_CTRLS[] = {
MSR_AMD_FAM15H_EVNTSEL5
};
-/* storage for context switching */
-struct amd_vpmu_context {
-u64 counters[MAX_NUM_COUNTERS];
-u64 ctrls[MAX_NUM_COUNTERS];
-bool_t msr_bitmap_set;
-};
+/* Use private context as a flag for MSR bitmap */
+#define msr_bitmap_on(vpmu)do {\
+ (vpmu)->priv_context = (void *)-1L; \
+ } while (0)
+#define msr_bitmap_off(vpmu) do {\
+ (vpmu)->priv_context = NULL;\
+ } while (0)
+#define is_msr_bitmap_on(vpmu) ((vpmu)->priv_context != NULL)
static inline int get_pmu_reg_type(u32 addr)
{
@@ -142,7 +144,6 @@ static void amd_vpmu_set_msr_bitmap(struct vcpu *v)
{
unsigned int i;
struct vpmu_struct *vpmu = vcpu_vpmu(v);
-struct amd_vpmu_context *ctxt = vpmu->context;
for ( i = 0; i < num_counters; i++ )
{
@@ -150,14 +151,13 @@ static void amd_vpmu_set_msr_bitmap(struct vcpu *v)
svm_intercept_msr(v, ctrls[i], MSR_INTERCEPT_WRITE);
}
-ctxt->msr_bitmap_set = 1;
+msr_bitmap_on(vpmu);
}
static void amd_vpmu_unset_msr_bitmap(struct vcpu *v)
{
unsigned int i;
struct vpmu_struct *vpmu = vcpu_vpmu(v);
-struct amd_vpmu_context *ctxt = vpmu->context;
for ( i = 0; i < num_counters; i++ )
{
@@ -165,7 +165,7 @@ static void amd_vpmu_unset_msr_bitmap(struct vcpu *v)
svm_intercept_msr(v, ctrls[i], MSR_INTERCEPT_RW);
}
-ctxt->msr_bitmap_set = 0;
+msr_bitmap_off(vpmu);
}
static int amd_vpmu_do_interrupt(struct cpu_user_regs *regs)
@@ -177,19 +177,22 @@ static inline void context_load(struct vcpu *v)
{
unsigned int i;
struct vpmu_struct *vpmu = vcpu_vpmu(v);
-struct amd_vpmu_context *ctxt = vpmu->context;
+struct xen_pmu_amd_ctxt *ctxt = vpmu->context;
+uint64_t *counter_regs = vpmu_reg_pointer(ctxt, counters);
+uint64_t *ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
for ( i = 0; i < num_counters; i++ )
{
-wrmsrl(counters[i], ctxt->counters[i]);
-wrmsrl(ctrls[i], ctxt->ctrls[i]);
+wrmsrl(counters[i], counter_regs[i]);
+wrmsrl(ctrls[i], ctrl_regs[i]);
}
}
static void amd_vpmu_load(struct vcpu *v)
{
struct vpmu_struct *vpmu = vcpu_vpmu(v);
-struct amd_vpmu_context *ctxt = vpmu->context;
+struct xen_pmu_amd_ctxt *ctxt = vpmu->context;
+uint64_t *ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
vpmu_reset(vpmu, VPMU_FROZEN);
@@ -198,7 +201,7 @@ static vo
