On 10/8/18 2:48 PM, Jan Kiszka wrote:
> On 08.10.18 13:36, Ralf Ramsauer wrote:
>> In some configurations, MSR_X2APIC_ICR is the most frequently accessed MSR.
>> Accounting it allows for getting an overview of its noise.
>>
>> Achieve this by splitting up the MSR count to msr_x2apic_icr and
>> msr_other. The sum of msr_x2apic_icr and msr_other denotes the sum of
>> all MSRs.
>>
>> Instead of accounting MSRs in the vcpu_handle_exit() path, move it over
>> to vcpu.c and implement it with a tiny inlined helper, which is called
>> by vcpu_handle_msr_{write,read}. With this, we have the same path for
>> both, svm and vmx.
>>
>> Concerning the ABI, move MSR_* exits to the end of the end in order to
>> prepare for more individual split ups of MSR accounting.
>>
>> Signed-off-by: Ralf Ramsauer <ralf.ramsa...@oth-regensburg.de>
>> ---
>> driver/sysfs.c | 7 ++++--
>> hypervisor/arch/x86/svm.c | 1 -
>> hypervisor/arch/x86/vcpu.c | 26 +++++++++++++++++-----
>> hypervisor/arch/x86/vmx.c | 2 --
>> include/arch/x86/asm/jailhouse_hypercall.h | 12 +++++-----
>> 5 files changed, 32 insertions(+), 16 deletions(-)
>>
>> diff --git a/driver/sysfs.c b/driver/sysfs.c
>> index 876fddc2..4232769e 100644
>> --- a/driver/sysfs.c
>> +++ b/driver/sysfs.c
>> @@ -104,11 +104,13 @@ JAILHOUSE_CPU_STATS_ATTR(vmexits_hypercall,
>> JAILHOUSE_CPU_STATS_ATTR(vmexits_pio, JAILHOUSE_CPU_STAT_VMEXITS_PIO);
>> JAILHOUSE_CPU_STATS_ATTR(vmexits_xapic, JAILHOUSE_CPU_STAT_VMEXITS_XAPIC);
>> JAILHOUSE_CPU_STATS_ATTR(vmexits_cr, JAILHOUSE_CPU_STAT_VMEXITS_CR);
>> -JAILHOUSE_CPU_STATS_ATTR(vmexits_msr, JAILHOUSE_CPU_STAT_VMEXITS_MSR);
>> JAILHOUSE_CPU_STATS_ATTR(vmexits_cpuid, JAILHOUSE_CPU_STAT_VMEXITS_CPUID);
>> JAILHOUSE_CPU_STATS_ATTR(vmexits_xsetbv,
>> JAILHOUSE_CPU_STAT_VMEXITS_XSETBV);
>> JAILHOUSE_CPU_STATS_ATTR(vmexits_exception,
>> JAILHOUSE_CPU_STAT_VMEXITS_EXCEPTION);
>> +JAILHOUSE_CPU_STATS_ATTR(vmexits_msr_other,
>> JAILHOUSE_CPU_STAT_VMEXITS_MSR_OTHER);
>> +JAILHOUSE_CPU_STATS_ATTR(vmexits_msr_x2apic_icr,
>> + JAILHOUSE_CPU_STAT_VMEXITS_MSR_X2APIC_ICR);
>> #elif defined(CONFIG_ARM) || defined(CONFIG_ARM64)
>> JAILHOUSE_CPU_STATS_ATTR(vmexits_maintenance,
>> JAILHOUSE_CPU_STAT_VMEXITS_MAINTENANCE);
>> JAILHOUSE_CPU_STATS_ATTR(vmexits_virt_irq,
>> JAILHOUSE_CPU_STAT_VMEXITS_VIRQ);
>> @@ -128,10 +130,11 @@ static struct attribute *no_attrs[] = {
>> &vmexits_pio_attr.kattr.attr,
>> &vmexits_xapic_attr.kattr.attr,
>> &vmexits_cr_attr.kattr.attr,
>> - &vmexits_msr_attr.kattr.attr,
>> &vmexits_cpuid_attr.kattr.attr,
>> &vmexits_xsetbv_attr.kattr.attr,
>> &vmexits_exception_attr.kattr.attr,
>> + &vmexits_msr_other_attr.kattr.attr,
>> + &vmexits_msr_x2apic_icr_attr.kattr.attr,
>> #elif defined(CONFIG_ARM) || defined(CONFIG_ARM64)
>> &vmexits_maintenance_attr.kattr.attr,
>> &vmexits_virt_irq_attr.kattr.attr,
>> diff --git a/hypervisor/arch/x86/svm.c b/hypervisor/arch/x86/svm.c
>> index 1808e74e..e40ff0ec 100644
>> --- a/hypervisor/arch/x86/svm.c
>> +++ b/hypervisor/arch/x86/svm.c
>> @@ -924,7 +924,6 @@ void vcpu_handle_exit(struct per_cpu *cpu_data)
>> vcpu_handle_cpuid();
>> goto vmentry;
>> case VMEXIT_MSR:
>> - cpu_public->stats[JAILHOUSE_CPU_STAT_VMEXITS_MSR]++;
>
> Now you will need to instrument svm_handle_msr_write /wrt MSR_EFER.
Argh, I see...
>
>> if (!vmcb->exitinfo1)
>> res = vcpu_handle_msr_read();
>> else
>> diff --git a/hypervisor/arch/x86/vcpu.c b/hypervisor/arch/x86/vcpu.c
>> index 42184e9d..ee21d28e 100644
>> --- a/hypervisor/arch/x86/vcpu.c
>> +++ b/hypervisor/arch/x86/vcpu.c
>> @@ -265,11 +265,24 @@ invalid_access:
>> return false;
>> }
>>
>> +static inline void account_msr_exit(u64 msr)
>
> Let's do
>
> static inline void account_msr_exit(struct public_per_cpu *cpu_public, u64
> msr)
>
> and let the caller pass cpu_data->public.
>
>> +{
>> + struct public_per_cpu *cpu_public = &this_cpu_data()->public;
>> +
>> + if (msr == MSR_X2APIC_ICR)
>> + cpu_public->stats[JAILHOUSE_CPU_STAT_VMEXITS_MSR_X2APIC_ICR]++;
>> + else
>> + cpu_public->stats[JAILHOUSE_CPU_STAT_VMEXITS_MSR_OTHER]++;
>> +}
>> +
>> bool vcpu_handle_msr_read(void)
>> {
>> struct per_cpu *cpu_data = this_cpu_data();
>> + unsigned long msr = cpu_data->guest_regs.rcx;
>> +
>> + account_msr_exit(msr);
>>
>> - switch (cpu_data->guest_regs.rcx) {
>> + switch (msr) {
>> case MSR_X2APIC_BASE ... MSR_X2APIC_END:
>> x2apic_handle_read();
>> break;
>> @@ -281,8 +294,7 @@ bool vcpu_handle_msr_read(void)
>> cpu_data->mtrr_def_type);
>> break;
>> default:
>> - panic_printk("FATAL: Unhandled MSR read: %lx\n",
>> - cpu_data->guest_regs.rcx);
>> + panic_printk("FATAL: Unhandled MSR read: %lx\n", msr);
>> return false;
>> }
>>
>> @@ -293,10 +305,13 @@ bool vcpu_handle_msr_read(void)
>> bool vcpu_handle_msr_write(void)
>> {
>> struct per_cpu *cpu_data = this_cpu_data();
>> + unsigned long msr = cpu_data->guest_regs.rcx;
>> unsigned int bit_pos, pa;
>> unsigned long val;
>>
>> - switch (cpu_data->guest_regs.rcx) {
>> + account_msr_exit(msr);
>> +
>> + switch (msr) {
>> case MSR_X2APIC_BASE ... MSR_X2APIC_END:
>> if (!x2apic_handle_write())
>> return false;
>> @@ -329,8 +344,7 @@ bool vcpu_handle_msr_write(void)
>> cpu_data->pat : 0);
>> break;
>> default:
>> - panic_printk("FATAL: Unhandled MSR write: %lx\n",
>> - cpu_data->guest_regs.rcx);
>> + panic_printk("FATAL: Unhandled MSR write: %lx\n", msr);
>> return false;
>> }
>>
>> diff --git a/hypervisor/arch/x86/vmx.c b/hypervisor/arch/x86/vmx.c
>> index 29967015..e9d66331 100644
>> --- a/hypervisor/arch/x86/vmx.c
>> +++ b/hypervisor/arch/x86/vmx.c
>> @@ -1183,12 +1183,10 @@ void vcpu_handle_exit(struct per_cpu *cpu_data)
>> return;
>> break;
>> case EXIT_REASON_MSR_READ:
>> - cpu_data->public.stats[JAILHOUSE_CPU_STAT_VMEXITS_MSR]++;
>> if (vcpu_handle_msr_read())
>> return;
>> break;
>> case EXIT_REASON_MSR_WRITE:
>> - cpu_data->public.stats[JAILHOUSE_CPU_STAT_VMEXITS_MSR]++;
>> if (cpu_data->guest_regs.rcx == MSR_IA32_PERF_GLOBAL_CTRL) {
>> /* ignore writes */
>> vcpu_skip_emulated_instruction(X86_INST_LEN_WRMSR);
>
> Same story as with MSR_EFER on AMD.
Ok. Hmm. It's probably better to move account_msr_exit() to a header,
and call it (again) directly from vcpu_handle_exit(). We need to hook
there for EFER in any case. And instead of introducing a corner case for
EFER, let's better hold accounting things at one place only.
Ralf
>
>> diff --git a/include/arch/x86/asm/jailhouse_hypercall.h
>> b/include/arch/x86/asm/jailhouse_hypercall.h
>> index 3a52599f..9e72b277 100644
>> --- a/include/arch/x86/asm/jailhouse_hypercall.h
>> +++ b/include/arch/x86/asm/jailhouse_hypercall.h
>> @@ -58,11 +58,13 @@
>> #define JAILHOUSE_CPU_STAT_VMEXITS_PIO
>> JAILHOUSE_GENERIC_CPU_STATS
>> #define JAILHOUSE_CPU_STAT_VMEXITS_XAPIC JAILHOUSE_GENERIC_CPU_STATS + 1
>> #define JAILHOUSE_CPU_STAT_VMEXITS_CR
>> JAILHOUSE_GENERIC_CPU_STATS + 2
>> -#define JAILHOUSE_CPU_STAT_VMEXITS_MSR
>> JAILHOUSE_GENERIC_CPU_STATS + 3
>> -#define JAILHOUSE_CPU_STAT_VMEXITS_CPUID JAILHOUSE_GENERIC_CPU_STATS + 4
>> -#define JAILHOUSE_CPU_STAT_VMEXITS_XSETBV JAILHOUSE_GENERIC_CPU_STATS + 5
>> -#define JAILHOUSE_CPU_STAT_VMEXITS_EXCEPTION
>> JAILHOUSE_GENERIC_CPU_STATS + 6
>> -#define JAILHOUSE_NUM_CPU_STATS
>> JAILHOUSE_GENERIC_CPU_STATS + 7
>> +#define JAILHOUSE_CPU_STAT_VMEXITS_CPUID JAILHOUSE_GENERIC_CPU_STATS + 3
>> +#define JAILHOUSE_CPU_STAT_VMEXITS_XSETBV JAILHOUSE_GENERIC_CPU_STATS + 4
>> +#define JAILHOUSE_CPU_STAT_VMEXITS_EXCEPTION
>> JAILHOUSE_GENERIC_CPU_STATS + 5
>> +#define JAILHOUSE_CPU_STAT_VMEXITS_MSR_OTHER
>> JAILHOUSE_GENERIC_CPU_STATS + 6
>> +#define JAILHOUSE_CPU_STAT_VMEXITS_MSR_X2APIC_ICR \
>> + JAILHOUSE_GENERIC_CPU_STATS + 7
>> +#define JAILHOUSE_NUM_CPU_STATS
>> JAILHOUSE_GENERIC_CPU_STATS + 8
>>
>> /* CPUID interface */
>> #define JAILHOUSE_CPUID_SIGNATURE 0x40000000
>>
>
> Jan
>
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to jailhouse-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
