| Message ID | 20180226171121.18974-2-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2018-02-26 18:11+0100, Vitaly Kuznetsov: > From: Ladi Prosek <lprosek@redhat.com> > > The assist page has been used only for the paravirtual EOI so far, hence > the "APIC" in the MSR name. Renaming to match the Hyper-V TLFS where it's > called "Virtual VP Assist MSR". > > Signed-off-by: Ladi Prosek <lprosek@redhat.com> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/include/uapi/asm/hyperv.h | 10 +++++----- > arch/x86/kvm/hyperv.c | 8 ++++---- > arch/x86/kvm/lapic.h | 2 +- > arch/x86/kvm/x86.c | 2 +- > 4 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > index 1c12aaf33915..45cc62352040 100644 > --- a/arch/x86/include/uapi/asm/hyperv.h > +++ b/arch/x86/include/uapi/asm/hyperv.h > @@ -189,7 +189,7 @@ > #define HV_X64_MSR_EOI 0x40000070 > #define HV_X64_MSR_ICR 0x40000071 > #define HV_X64_MSR_TPR 0x40000072 > -#define HV_X64_MSR_APIC_ASSIST_PAGE 0x40000073 > +#define HV_X64_MSR_VP_ASSIST_PAGE 0x40000073 > > /* Define synthetic interrupt controller model specific registers. */ > #define HV_X64_MSR_SCONTROL 0x40000080 > @@ -275,10 +275,10 @@ struct hv_tsc_emulation_status { > #define HVCALL_POST_MESSAGE 0x005c > #define HVCALL_SIGNAL_EVENT 0x005d > > -#define HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE 0x00000001 > -#define HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT 12 > -#define HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_MASK \ > - (~((1ull << HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT) - 1)) Removing definitions from userspace api isn't a good idea. I have no idea why hyper.h is a userspace api, though -- Linux doesn't define any of those, so we could copy the definitions to a private header, rename, and never look at this file again.
On Wed, Mar 07, 2018 at 05:19:44PM +0100, Radim Krčmář wrote: > 2018-02-26 18:11+0100, Vitaly Kuznetsov: > > From: Ladi Prosek <lprosek@redhat.com> > > > > The assist page has been used only for the paravirtual EOI so far, hence > > the "APIC" in the MSR name. Renaming to match the Hyper-V TLFS where it's > > called "Virtual VP Assist MSR". > > > > Signed-off-by: Ladi Prosek <lprosek@redhat.com> > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > --- > > arch/x86/include/uapi/asm/hyperv.h | 10 +++++----- > > arch/x86/kvm/hyperv.c | 8 ++++---- > > arch/x86/kvm/lapic.h | 2 +- > > arch/x86/kvm/x86.c | 2 +- > > 4 files changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > > index 1c12aaf33915..45cc62352040 100644 > > --- a/arch/x86/include/uapi/asm/hyperv.h > > +++ b/arch/x86/include/uapi/asm/hyperv.h > > @@ -189,7 +189,7 @@ > > #define HV_X64_MSR_EOI 0x40000070 > > #define HV_X64_MSR_ICR 0x40000071 > > #define HV_X64_MSR_TPR 0x40000072 > > -#define HV_X64_MSR_APIC_ASSIST_PAGE 0x40000073 > > +#define HV_X64_MSR_VP_ASSIST_PAGE 0x40000073 > > > > /* Define synthetic interrupt controller model specific registers. */ > > #define HV_X64_MSR_SCONTROL 0x40000080 > > @@ -275,10 +275,10 @@ struct hv_tsc_emulation_status { > > #define HVCALL_POST_MESSAGE 0x005c > > #define HVCALL_SIGNAL_EVENT 0x005d > > > > -#define HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE 0x00000001 > > -#define HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT 12 > > -#define HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_MASK \ > > - (~((1ull << HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT) - 1)) > > Removing definitions from userspace api isn't a good idea. > > I have no idea why hyper.h is a userspace api, though -- Linux doesn't > define any of those, so we could copy the definitions to a private > header, rename, and never look at this file again. That was a thinko when it was moved to uapi, and it has already been identified as a problem, so now QEMU has its own header with the definitions it needs, and I'm unaware of any other userspace project that depends on this stuff. So I've been planning to remove it from uapi but still haven't got around to posting the patch :( Roman.
2018-03-07 19:48+0300, Roman Kagan: > On Wed, Mar 07, 2018 at 05:19:44PM +0100, Radim Krčmář wrote: > > 2018-02-26 18:11+0100, Vitaly Kuznetsov: > > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > > > @@ -275,10 +275,10 @@ struct hv_tsc_emulation_status { > > > #define HVCALL_POST_MESSAGE 0x005c > > > #define HVCALL_SIGNAL_EVENT 0x005d > > > > > > -#define HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE 0x00000001 > > > -#define HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT 12 > > > -#define HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_MASK \ > > > - (~((1ull << HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT) - 1)) > > > > Removing definitions from userspace api isn't a good idea. > > > > I have no idea why hyper.h is a userspace api, though -- Linux doesn't > > define any of those, so we could copy the definitions to a private > > header, rename, and never look at this file again. > > That was a thinko when it was moved to uapi, and it has already been > identified as a problem, so now QEMU has its own header with the > definitions it needs, and I'm unaware of any other userspace project > that depends on this stuff. So I've been planning to remove it from > uapi but still haven't got around to posting the patch :( Great, let's be bold here.
Radim Krčmář <rkrcmar@redhat.com> writes: > 2018-03-07 19:48+0300, Roman Kagan: >> On Wed, Mar 07, 2018 at 05:19:44PM +0100, Radim Krčmář wrote: >> > 2018-02-26 18:11+0100, Vitaly Kuznetsov: >> > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h >> > > @@ -275,10 +275,10 @@ struct hv_tsc_emulation_status { >> > > #define HVCALL_POST_MESSAGE 0x005c >> > > #define HVCALL_SIGNAL_EVENT 0x005d >> > > >> > > -#define HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE 0x00000001 >> > > -#define HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT 12 >> > > -#define HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_MASK \ >> > > - (~((1ull << HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT) - 1)) >> > >> > Removing definitions from userspace api isn't a good idea. >> > >> > I have no idea why hyper.h is a userspace api, though -- Linux doesn't >> > define any of those, so we could copy the definitions to a private >> > header, rename, and never look at this file again. >> >> That was a thinko when it was moved to uapi, and it has already been >> identified as a problem, so now QEMU has its own header with the >> definitions it needs, and I'm unaware of any other userspace project >> that depends on this stuff. So I've been planning to remove it from >> uapi but still haven't got around to posting the patch :( > > Great, let's be bold here. asm/hyperv.h is not uapi. I would include a patch renaming arch/x86/include/uapi/asm/hyperv.h to arch/x86/include/asm/hyperv.h but we already have 'mshyperv.h' there and I don't quite understand the difference. We can either merge them or come up with a rule distinguishing them. K. Y., Michael, what do you think?
> >> > Removing definitions from userspace api isn't a good idea. > >> > > >> > I have no idea why hyper.h is a userspace api, though -- Linux doesn't > >> > define any of those, so we could copy the definitions to a private > >> > header, rename, and never look at this file again. > >> > >> That was a thinko when it was moved to uapi, and it has already been > >> identified as a problem, so now QEMU has its own header with the > >> definitions it needs, and I'm unaware of any other userspace project > >> that depends on this stuff. So I've been planning to remove it from > >> uapi but still haven't got around to posting the patch :( > > > > Great, let's be bold here. > > asm/hyperv.h is not uapi. > > I would include a patch renaming arch/x86/include/uapi/asm/hyperv.h to > arch/x86/include/asm/hyperv.h but we already have 'mshyperv.h' there and > I don't quite understand the difference. We can either merge them or > come up with a rule distinguishing them. > > K. Y., Michael, what do you think? Good timing for this topic, as I'm now looking at cloning these two files into the arch/arm64 tree for Hyper-V on ARM64. It would be great to get a plan agreed on so I can be consistent on the arm64 side. I would suggest keeping two files: one with just the data structures and #defines that come from the Hyper-V Top-Level Functional Spec (TLFS), and the other with additional data structures, macros, function prototypes, etc. that are specific to Linux guest code. uapi/asm/hyperv.h is already the first one, and asm/mshyperv.h is mostly the second one, though it has some things that should probably move to the first one. There are also a few stray definitions from the Hyper-V TLFS in drivers/pci/host/pci-hyperv.c (HVCALL_RETARGET_INTERRUPT and HV_PARTITION_ID_SELF, for example) that really belong in the first file. And since we're already changing the location of the first file, let's rename it to hyperv-tlfs.h or something similar. Michael > > -- > Vitaly
"Michael Kelley (EOSG)" <Michael.H.Kelley@microsoft.com> writes: >> >> > Removing definitions from userspace api isn't a good idea. >> >> > >> >> > I have no idea why hyper.h is a userspace api, though -- Linux doesn't >> >> > define any of those, so we could copy the definitions to a private >> >> > header, rename, and never look at this file again. >> >> >> >> That was a thinko when it was moved to uapi, and it has already been >> >> identified as a problem, so now QEMU has its own header with the >> >> definitions it needs, and I'm unaware of any other userspace project >> >> that depends on this stuff. So I've been planning to remove it from >> >> uapi but still haven't got around to posting the patch :( >> > >> > Great, let's be bold here. >> >> asm/hyperv.h is not uapi. >> >> I would include a patch renaming arch/x86/include/uapi/asm/hyperv.h to >> arch/x86/include/asm/hyperv.h but we already have 'mshyperv.h' there and >> I don't quite understand the difference. We can either merge them or >> come up with a rule distinguishing them. >> >> K. Y., Michael, what do you think? > > Good timing for this topic, as I'm now looking at cloning these two > files into the arch/arm64 tree for Hyper-V on ARM64. It would be great > to get a plan agreed on so I can be consistent on the arm64 side. > > I would suggest keeping two files: one with just the data structures and > #defines that come from the Hyper-V Top-Level Functional Spec (TLFS), > and the other with additional data structures, macros, function prototypes, > etc. that are specific to Linux guest code. uapi/asm/hyperv.h is already > the first one, and asm/mshyperv.h is mostly the second one, though it has > some things that should probably move to the first one. There are also a > few stray definitions from the Hyper-V TLFS in drivers/pci/host/pci-hyperv.c > (HVCALL_RETARGET_INTERRUPT and HV_PARTITION_ID_SELF, for example) > that really belong in the first file. > > And since we're already changing the location of the first file, let's rename > it to hyperv-tlfs.h or something similar. > Sounds good, I'll add a patch or two to my eVMCS series. Thanks!
diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h index 1c12aaf33915..45cc62352040 100644 --- a/arch/x86/include/uapi/asm/hyperv.h +++ b/arch/x86/include/uapi/asm/hyperv.h @@ -189,7 +189,7 @@ #define HV_X64_MSR_EOI 0x40000070 #define HV_X64_MSR_ICR 0x40000071 #define HV_X64_MSR_TPR 0x40000072 -#define HV_X64_MSR_APIC_ASSIST_PAGE 0x40000073 +#define HV_X64_MSR_VP_ASSIST_PAGE 0x40000073 /* Define synthetic interrupt controller model specific registers. */ #define HV_X64_MSR_SCONTROL 0x40000080 @@ -275,10 +275,10 @@ struct hv_tsc_emulation_status { #define HVCALL_POST_MESSAGE 0x005c #define HVCALL_SIGNAL_EVENT 0x005d -#define HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE 0x00000001 -#define HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT 12 -#define HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_MASK \ - (~((1ull << HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT) - 1)) +#define HV_X64_MSR_VP_ASSIST_PAGE_ENABLE 0x00000001 +#define HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_SHIFT 12 +#define HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_MASK \ + (~((1ull << HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_SHIFT) - 1)) #define HV_X64_MSR_TSC_REFERENCE_ENABLE 0x00000001 #define HV_X64_MSR_TSC_REFERENCE_ADDRESS_SHIFT 12 diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index 8e38a6ef84cc..d99465d7002f 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -1010,17 +1010,17 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host) return 1; hv->vp_index = (u32)data; break; - case HV_X64_MSR_APIC_ASSIST_PAGE: { + case HV_X64_MSR_VP_ASSIST_PAGE: { u64 gfn; unsigned long addr; - if (!(data & HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE)) { + if (!(data & HV_X64_MSR_VP_ASSIST_PAGE_ENABLE)) { hv->hv_vapic = data; if (kvm_lapic_enable_pv_eoi(vcpu, 0)) return 1; break; } - gfn = data >> HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT; + gfn = data >> HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_SHIFT; addr = kvm_vcpu_gfn_to_hva(vcpu, gfn); if (kvm_is_error_hva(addr)) return 1; @@ -1130,7 +1130,7 @@ static int kvm_hv_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) return kvm_hv_vapic_msr_read(vcpu, APIC_ICR, pdata); case HV_X64_MSR_TPR: return kvm_hv_vapic_msr_read(vcpu, APIC_TASKPRI, pdata); - case HV_X64_MSR_APIC_ASSIST_PAGE: + case HV_X64_MSR_VP_ASSIST_PAGE: data = hv->hv_vapic; break; case HV_X64_MSR_VP_RUNTIME: diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h index 56c36014f7b7..edce055e9fd7 100644 --- a/arch/x86/kvm/lapic.h +++ b/arch/x86/kvm/lapic.h @@ -109,7 +109,7 @@ int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data); static inline bool kvm_hv_vapic_assist_page_enabled(struct kvm_vcpu *vcpu) { - return vcpu->arch.hyperv.hv_vapic & HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE; + return vcpu->arch.hyperv.hv_vapic & HV_X64_MSR_VP_ASSIST_PAGE_ENABLE; } int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 1a3ed81031f1..1d7be99ced4a 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1034,7 +1034,7 @@ static u32 emulated_msrs[] = { HV_X64_MSR_VP_RUNTIME, HV_X64_MSR_SCONTROL, HV_X64_MSR_STIMER0_CONFIG, - HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, + HV_X64_MSR_VP_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, MSR_KVM_PV_EOI_EN, MSR_IA32_TSC_ADJUST,