Re: [Qemu-devel] [PATCH 07/11] kvm/x86: added hyper-v crash data and ctl msr's get/set'ers

["Denis V. Lunev" <den@openvz.org>]

On 23/06/15 02:52, Peter Hornyack wrote:
> On Mon, Jun 22, 2015 at 9:05 AM, Denis V. Lunev <den@openvz.org> wrote:
>> From: Andrey Smetanin <asmetanin@virtuozzo.com>
>>
>> Added hyper-v crash msr's(HV_X64_MSR_CRASH*) data and control
>> geters and setters.
>>
>> Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Gleb Natapov <gleb@kernel.org>
>> ---
>>   arch/x86/kvm/hyperv.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   arch/x86/kvm/x86.c    |  4 ++++
>>   2 files changed, 70 insertions(+)
>>
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index f65fb622..0a7d373 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -46,6 +46,59 @@ static bool kvm_hv_msr_partition_wide(u32 msr)
>>          return r;
>>   }
>>
>> +static int kvm_hv_msr_get_crash_ctl(struct kvm_vcpu *vcpu, u64 *pdata)
>> +{
>> +       struct kvm_arch_hyperv *hv = &vcpu->kvm->arch.hyperv;
>> +
>> +       *pdata = hv->hv_crash_ctl;
> I see that this is implemented so that userspace controls the Hyper-V
> crash capabilities that are enabled - userspace must set
> HV_X64_MSR_CRASH_CTL_NOTIFY before the guest reads
> HV_X64_MSR_CRASH_CTL. I just want to check that you considered an
> alternative: a simpler implementation would have the crash
> capabilities statically defined by kvm, and
> HV_X64_MSR_CRASH_CTL_CONTENTS could just be returned here (and
> hv_crash_ctl could be removed from struct kvm_arch_hyperv).
>
> The current implementation is potentially more flexible but makes the
> MSR handling a little more awkward since the host_initiated bool needs
> to be passed around (patch 09). I guess either approach seems ok to
> me.
>
> Also, if this patchset is used then it looks like
> HV_X64_MSR_CRASH_CTL_CONTENTS can be removed.

Paolo,

do you have any opinion on this? I prefer
configurable way but there is no problem
to enable it by default dropping 'hv_panic'
property.

Regards,
     Den

>> +       return 0;
>> +}
>> +
>> +static int kvm_hv_msr_set_crash_ctl(struct kvm_vcpu *vcpu, u64 data)
>> +{
>> +       struct kvm_arch_hyperv *hv = &vcpu->kvm->arch.hyperv;
>> +
>> +       hv->hv_crash_ctl = data;
>> +       if ((data & HV_X64_MSR_CRASH_CTL_NOTIFY)) {
>> +               vcpu_debug(vcpu, "hv crash (0x%llx 0x%llx 0x%llx 0x%llx "
>> +                         "0x%llx)\n", hv->hv_crash_param[0],
>> +                         hv->hv_crash_param[1],
>> +                         hv->hv_crash_param[2],
>> +                         hv->hv_crash_param[3],
>> +                         hv->hv_crash_param[4]);
>> +
>> +               /* Send notification about crash to user space */
>> +               kvm_make_request(KVM_REQ_HV_CRASH, vcpu);
>> +               return 0;
> Returning from here seems unnecessary - if more crash capabilities are
> added in the future, the guest might want to invoke multiple
> capabilities at once, so we'd want to check for those here too.
>
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int kvm_hv_msr_set_crash_data(struct kvm_vcpu *vcpu,
>> +                                    u32 index, u64 data)
>> +{
>> +       struct kvm_arch_hyperv *hv = &vcpu->kvm->arch.hyperv;
>> +
>> +       if (WARN_ON_ONCE(index >= ARRAY_SIZE(hv->hv_crash_param)))
>> +               return -EINVAL;
>> +
>> +       hv->hv_crash_param[index] = data;
>> +       return 0;
>> +}
>> +
>> +static int kvm_hv_msr_get_crash_data(struct kvm_vcpu *vcpu,
>> +                                    u32 index, u64 *pdata)
>> +{
>> +       struct kvm_arch_hyperv *hv = &vcpu->kvm->arch.hyperv;
>> +
>> +       if (WARN_ON_ONCE(index >= ARRAY_SIZE(hv->hv_crash_param)))
>> +               return -EINVAL;
>> +
>> +       *pdata = hv->hv_crash_param[index];
>> +       return 0;
>> +}
>> +
>>   static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>>   {
>>          struct kvm *kvm = vcpu->kvm;
>> @@ -98,6 +152,12 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>>                  mark_page_dirty(kvm, gfn);
>>                  break;
>>          }
>> +       case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
>> +               return kvm_hv_msr_set_crash_data(vcpu,
>> +                                                msr - HV_X64_MSR_CRASH_P0,
>> +                                                data);
>> +       case HV_X64_MSR_CRASH_CTL:
>> +               return kvm_hv_msr_set_crash_ctl(vcpu, data);
>>          default:
>>                  vcpu_unimpl(vcpu, "Hyper-V unimpl wrmsr: 0x%x data 0x%llx\n",
>>                                    msr, data);
>> @@ -170,6 +230,12 @@ static int kvm_hv_get_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>>          case HV_X64_MSR_REFERENCE_TSC:
>>                  data = hv->hv_tsc_page;
>>                  break;
>> +       case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
>> +               return kvm_hv_msr_get_crash_data(vcpu,
>> +                                                msr - HV_X64_MSR_CRASH_P0,
>> +                                                pdata);
>> +       case HV_X64_MSR_CRASH_CTL:
>> +               return kvm_hv_msr_get_crash_ctl(vcpu, pdata);
>>          default:
>>                  vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
>>                  return 1;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 2755c37..2046b78 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2208,6 +2208,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>                   */
>>                  break;
>>          case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
>> +       case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
>> +       case HV_X64_MSR_CRASH_CTL:
>>                  return kvm_hv_set_msr_common(vcpu, msr, data);
>>                  break;
>>          case MSR_IA32_BBL_CR_CTL3:
>> @@ -2451,6 +2453,8 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>>                  data = 0x20000000;
>>                  break;
>>          case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
>> +       case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
>> +       case HV_X64_MSR_CRASH_CTL:
>>                  return kvm_hv_get_msr_common(vcpu, msr, pdata);
>>                  break;
>>          case MSR_IA32_BBL_CR_CTL3:
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in