subject:"Re\: \[PATCH v2\] core\: statistics\: x86\: account for MSR_X2APIC

Re: [PATCH v2] core: statistics: x86: account for MSR_X2APIC_ICR

2018-10-08 Thread Ralf Ramsauer

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 
>> ---
>>   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[] = {
>>  _pio_attr.kattr.attr,
>>  _xapic_attr.kattr.attr,
>>  _cr_attr.kattr.attr,
>> -_msr_attr.kattr.attr,
>>  _cpuid_attr.kattr.attr,
>>  _xsetbv_attr.kattr.attr,
>>  _exception_attr.kattr.attr,
>> +_msr_other_attr.kattr.attr,
>> +_msr_x2apic_icr_attr.kattr.attr,
>>   #elif defined(CONFIG_ARM) || defined(CONFIG_ARM64)
>>  _maintenance_attr.kattr.attr,
>>  _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 = _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

Re: [PATCH v2] core: statistics: x86: account for MSR_X2APIC_ICR

2018-10-08 Thread Jan Kiszka

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 
> ---
>   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[] = {
>   _pio_attr.kattr.attr,
>   _xapic_attr.kattr.attr,
>   _cr_attr.kattr.attr,
> - _msr_attr.kattr.attr,
>   _cpuid_attr.kattr.attr,
>   _xsetbv_attr.kattr.attr,
>   _exception_attr.kattr.attr,
> + _msr_other_attr.kattr.attr,
> + _msr_x2apic_icr_attr.kattr.attr,
>   #elif defined(CONFIG_ARM) || defined(CONFIG_ARM64)
>   _maintenance_attr.kattr.attr,
>   _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.

>   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 = _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

Re: [PATCH v2] core: statistics: x86: account for MSR_X2APIC_ICR

Re: [PATCH v2] core: statistics: x86: account for MSR_X2APIC_ICR

2 matches

Site Navigation

Mail list logo

Footer information