| Message ID | 1527086545-68024-8-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/28] hw/pci-host/q35: Replace hardcoded value with macro | expand |
On 23 May 2018 at 15:43, Michael S. Tsirkin <mst@redhat.com> wrote: > Switch to the header we imported from Linux, > this allows us to drop a hack in kvm_i386.h. > More code will be dropped in the next patch. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -688,8 +688,6 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */ > #define CPUID_7_0_EDX_SPEC_CTRL_SSBD (1U << 31) /* Speculative Store Bypass Disable */ > > -#define KVM_HINTS_DEDICATED (1U << 0) > - > #define CPUID_8000_0008_EBX_IBPB (1U << 12) /* Indirect Branch Prediction Barrier */ > > #define CPUID_XSAVE_XSAVEOPT (1U << 0) Hi -- this seems like it will break compilation when we next update our copy of the Linux kernel headers, because (as of 4.17-rc6, at least), asm-x86/kvm_para.h doesn't define KVM_HINTS_DEDICATED. Here's the diff I get as part of my attempt to run update-linux-headers: --- a/include/standard-headers/asm-x86/kvm_para.h +++ b/include/standard-headers/asm-x86/kvm_para.h @@ -29,7 +29,7 @@ #define KVM_FEATURE_PV_TLB_FLUSH 9 #define KVM_FEATURE_ASYNC_PF_VMEXIT 10 -#define KVM_HINTS_DEDICATED 0 +#define KVM_HINTS_REALTIME 0 /* The last 8 bits are used to indicate how to interpret the flags field * in pvclock structure. If no bits are set, all flags are ignored. I'm not sure what's going on here -- commit 633711e8287 in the kernel just renames the constant, but doesn't that break userspace API ? thanks -- PMM
On 25 May 2018 at 12:06, Peter Maydell <peter.maydell@linaro.org> wrote: > On 23 May 2018 at 15:43, Michael S. Tsirkin <mst@redhat.com> wrote: >> Switch to the header we imported from Linux, >> this allows us to drop a hack in kvm_i386.h. >> More code will be dropped in the next patch. >> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >> --- a/target/i386/cpu.h >> +++ b/target/i386/cpu.h >> @@ -688,8 +688,6 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; >> #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */ >> #define CPUID_7_0_EDX_SPEC_CTRL_SSBD (1U << 31) /* Speculative Store Bypass Disable */ >> >> -#define KVM_HINTS_DEDICATED (1U << 0) >> - >> #define CPUID_8000_0008_EBX_IBPB (1U << 12) /* Indirect Branch Prediction Barrier */ >> >> #define CPUID_XSAVE_XSAVEOPT (1U << 0) > > Hi -- this seems like it will break compilation when we next > update our copy of the Linux kernel headers, because (as of > 4.17-rc6, at least), asm-x86/kvm_para.h doesn't define > KVM_HINTS_DEDICATED. For the moment I'm using this workaround (I wanted to do a header update for something else I'm working on): --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -48,6 +48,11 @@ #include "exec/memattrs.h" #include "trace.h" +/* Work around this kernel header constant changing its name */ +#ifndef KVM_HINTS_REALTIME +#define KVM_HINTS_REALTIME KVM_HINTS_DEDICATED +#endif + //#define DEBUG_KVM #ifdef DEBUG_KVM @@ -387,7 +392,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, ret &= ~(1U << KVM_FEATURE_PV_UNHALT); } } else if (function == KVM_CPUID_FEATURES && reg == R_EDX) { - ret |= 1U << KVM_HINTS_DEDICATED; + ret |= 1U << KVM_HINTS_REALTIME; found = 1; } thanks -- PMM
On Fri, May 25, 2018 at 12:53:44PM +0100, Peter Maydell wrote: > On 25 May 2018 at 12:06, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 23 May 2018 at 15:43, Michael S. Tsirkin <mst@redhat.com> wrote: > >> Switch to the header we imported from Linux, > >> this allows us to drop a hack in kvm_i386.h. > >> More code will be dropped in the next patch. > >> > >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > >> --- a/target/i386/cpu.h > >> +++ b/target/i386/cpu.h > >> @@ -688,8 +688,6 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > >> #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */ > >> #define CPUID_7_0_EDX_SPEC_CTRL_SSBD (1U << 31) /* Speculative Store Bypass Disable */ > >> > >> -#define KVM_HINTS_DEDICATED (1U << 0) > >> - > >> #define CPUID_8000_0008_EBX_IBPB (1U << 12) /* Indirect Branch Prediction Barrier */ > >> > >> #define CPUID_XSAVE_XSAVEOPT (1U << 0) > > > > Hi -- this seems like it will break compilation when we next > > update our copy of the Linux kernel headers, because (as of > > 4.17-rc6, at least), asm-x86/kvm_para.h doesn't define > > KVM_HINTS_DEDICATED. > > For the moment I'm using this workaround (I wanted to do a header > update for something else I'm working on): > > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -48,6 +48,11 @@ > #include "exec/memattrs.h" > #include "trace.h" > > +/* Work around this kernel header constant changing its name */ > +#ifndef KVM_HINTS_REALTIME > +#define KVM_HINTS_REALTIME KVM_HINTS_DEDICATED > +#endif > + > //#define DEBUG_KVM > > #ifdef DEBUG_KVM I don't think we need this chunk. > @@ -387,7 +392,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, > uint32_t function, > ret &= ~(1U << KVM_FEATURE_PV_UNHALT); > } > } else if (function == KVM_CPUID_FEATURES && reg == R_EDX) { > - ret |= 1U << KVM_HINTS_DEDICATED; > + ret |= 1U << KVM_HINTS_REALTIME; > found = 1; > } That's the right change when we update this header. > thanks > -- PMM
On Fri, May 25, 2018 at 12:06:17PM +0100, Peter Maydell wrote: > On 23 May 2018 at 15:43, Michael S. Tsirkin <mst@redhat.com> wrote: > > Switch to the header we imported from Linux, > > this allows us to drop a hack in kvm_i386.h. > > More code will be dropped in the next patch. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > --- a/target/i386/cpu.h > > +++ b/target/i386/cpu.h > > @@ -688,8 +688,6 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > > #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */ > > #define CPUID_7_0_EDX_SPEC_CTRL_SSBD (1U << 31) /* Speculative Store Bypass Disable */ > > > > -#define KVM_HINTS_DEDICATED (1U << 0) > > - > > #define CPUID_8000_0008_EBX_IBPB (1U << 12) /* Indirect Branch Prediction Barrier */ > > > > #define CPUID_XSAVE_XSAVEOPT (1U << 0) > > Hi -- this seems like it will break compilation when we next > update our copy of the Linux kernel headers, That just means we'll need to update kvm.c when we do it. > because (as of > 4.17-rc6, at least), asm-x86/kvm_para.h doesn't define > KVM_HINTS_DEDICATED. Here's the diff I get as part of > my attempt to run update-linux-headers: > > --- a/include/standard-headers/asm-x86/kvm_para.h > +++ b/include/standard-headers/asm-x86/kvm_para.h > @@ -29,7 +29,7 @@ > #define KVM_FEATURE_PV_TLB_FLUSH 9 > #define KVM_FEATURE_ASYNC_PF_VMEXIT 10 > > -#define KVM_HINTS_DEDICATED 0 > +#define KVM_HINTS_REALTIME 0 > > /* The last 8 bits are used to indicate how to interpret the flags field > * in pvclock structure. If no bits are set, all flags are ignored. > > I'm not sure what's going on here -- commit 633711e8287 in > the kernel just renames the constant, but doesn't that > break userspace API ? > > thanks > -- PMM
On 25 May 2018 at 13:18, Michael S. Tsirkin <mst@redhat.com> wrote: > On Fri, May 25, 2018 at 12:53:44PM +0100, Peter Maydell wrote: >> For the moment I'm using this workaround (I wanted to do a header >> update for something else I'm working on): >> >> --- a/target/i386/kvm.c >> +++ b/target/i386/kvm.c >> @@ -48,6 +48,11 @@ >> #include "exec/memattrs.h" >> #include "trace.h" >> >> +/* Work around this kernel header constant changing its name */ >> +#ifndef KVM_HINTS_REALTIME >> +#define KVM_HINTS_REALTIME KVM_HINTS_DEDICATED >> +#endif >> + >> //#define DEBUG_KVM >> >> #ifdef DEBUG_KVM > > I don't think we need this chunk. My view is that header update commits should be exactly and only the result of running update-linux-headers, no manual tweaking. If you don't add this chunk before the update, compilation with the update will fail. You can remove the chunk after the update, of course... thanks -- PMM
On Fri, May 25, 2018 at 01:21:24PM +0100, Peter Maydell wrote: > On 25 May 2018 at 13:18, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, May 25, 2018 at 12:53:44PM +0100, Peter Maydell wrote: > >> For the moment I'm using this workaround (I wanted to do a header > >> update for something else I'm working on): > >> > >> --- a/target/i386/kvm.c > >> +++ b/target/i386/kvm.c > >> @@ -48,6 +48,11 @@ > >> #include "exec/memattrs.h" > >> #include "trace.h" > >> > >> +/* Work around this kernel header constant changing its name */ > >> +#ifndef KVM_HINTS_REALTIME > >> +#define KVM_HINTS_REALTIME KVM_HINTS_DEDICATED > >> +#endif > >> + > >> //#define DEBUG_KVM > >> > >> #ifdef DEBUG_KVM > > > > I don't think we need this chunk. > > My view is that header update commits should be exactly and only > the result of running update-linux-headers, no manual tweaking. > If you don't add this chunk before the update, compilation with the > update will fail. You can remove the chunk after the update, of course... > > thanks > -- PMM I see. I guess you did all the work already, do you still need help or will you just go ahead and post it? Or even commit directly, it's a trivial enough patch.
On 25 May 2018 at 13:27, Michael S. Tsirkin <mst@redhat.com> wrote: > I see. I guess you did all the work already, do you still need help > or will you just go ahead and post it? Or even commit directly, > it's a trivial enough patch. I'll send a series later this afternoon that does an update to 4.17-rc6; it has a couple of other prerequisites in it, to handle __aligned_u64 in the update script (patch already posted this morning, but I'll put it in the series), and to deal with the kernel headers not yet defining VIRTIO_GPU_CAPSET_VIRGL2. thanks -- PMM
On Fri, May 25, 2018 at 01:30:00PM +0100, Peter Maydell wrote: > On 25 May 2018 at 13:27, Michael S. Tsirkin <mst@redhat.com> wrote: > > I see. I guess you did all the work already, do you still need help > > or will you just go ahead and post it? Or even commit directly, > > it's a trivial enough patch. > > I'll send a series later this afternoon that does an update > to 4.17-rc6; it has a couple of other prerequisites in it, > to handle __aligned_u64 in the update script (patch already > posted this morning, but I'll put it in the series), and to deal > with the kernel headers not yet defining VIRTIO_GPU_CAPSET_VIRGL2. You mean like this: http://patchwork.ozlabs.org/patch/907121/ ? > thanks > -- PMM
On 25 May 2018 at 13:35, Michael S. Tsirkin <mst@redhat.com> wrote: > On Fri, May 25, 2018 at 01:30:00PM +0100, Peter Maydell wrote: >> On 25 May 2018 at 13:27, Michael S. Tsirkin <mst@redhat.com> wrote: >> > I see. I guess you did all the work already, do you still need help >> > or will you just go ahead and post it? Or even commit directly, >> > it's a trivial enough patch. >> >> I'll send a series later this afternoon that does an update >> to 4.17-rc6; it has a couple of other prerequisites in it, >> to handle __aligned_u64 in the update script (patch already >> posted this morning, but I'll put it in the series), and to deal >> with the kernel headers not yet defining VIRTIO_GPU_CAPSET_VIRGL2. > > > You mean like this: > http://patchwork.ozlabs.org/patch/907121/ > > ? Yes; I missed that (found a mail thread pointing out the problem but not the patch with the fix); I'll pick it up for the series. thanks -- PMM
On 25/05/2018 13:06, Peter Maydell wrote: > --- a/include/standard-headers/asm-x86/kvm_para.h > +++ b/include/standard-headers/asm-x86/kvm_para.h > @@ -29,7 +29,7 @@ > #define KVM_FEATURE_PV_TLB_FLUSH 9 > #define KVM_FEATURE_ASYNC_PF_VMEXIT 10 > > -#define KVM_HINTS_DEDICATED 0 > +#define KVM_HINTS_REALTIME 0 > > /* The last 8 bits are used to indicate how to interpret the flags field > * in pvclock structure. If no bits are set, all flags are ignored. > > I'm not sure what's going on here -- commit 633711e8287 in > the kernel just renames the constant, but doesn't that > break userspace API ? No, but only because the constant with the old name was not in any released version. It was added in 4.17-rc1. Thanks, Paolo
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index 23669c4..0b64b8e 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -22,7 +22,6 @@ #ifdef NEED_CPU_H # ifdef CONFIG_KVM # include <linux/kvm.h> -# include <linux/kvm_para.h> # define CONFIG_KVM_IS_POSSIBLE # endif #else diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 8ac13f6..6645046 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -688,8 +688,6 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */ #define CPUID_7_0_EDX_SPEC_CTRL_SSBD (1U << 31) /* Speculative Store Bypass Disable */ -#define KVM_HINTS_DEDICATED (1U << 0) - #define CPUID_8000_0008_EBX_IBPB (1U << 12) /* Indirect Branch Prediction Barrier */ #define CPUID_XSAVE_XSAVEOPT (1U << 0) diff --git a/target/i386/kvm_i386.h b/target/i386/kvm_i386.h index 1de9876..e5df24c 100644 --- a/target/i386/kvm_i386.h +++ b/target/i386/kvm_i386.h @@ -30,12 +30,6 @@ #define kvm_pic_in_kernel() 0 #define kvm_ioapic_in_kernel() 0 -/* These constants must never be used at runtime if kvm_enabled() is false. - * They exist so we don't need #ifdefs around KVM-specific code that already - * checks kvm_enabled() properly. - */ -#define KVM_CPUID_FEATURES 0 - #endif /* CONFIG_KVM */ bool kvm_allows_irq0_override(void); diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c index 7dac319..0bf1c60 100644 --- a/hw/i386/kvm/clock.c +++ b/hw/i386/kvm/clock.c @@ -26,7 +26,7 @@ #include "qapi/error.h" #include <linux/kvm.h> -#include <linux/kvm_para.h> +#include "standard-headers/asm-x86/kvm_para.h" #define TYPE_KVM_CLOCK "kvmclock" #define KVM_CLOCK(obj) OBJECT_CHECK(KVMClockState, (obj), TYPE_KVM_CLOCK) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index d95310f..9426041 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -40,9 +40,7 @@ #include "qom/qom-qobject.h" #include "sysemu/arch_init.h" -#if defined(CONFIG_KVM) -#include <linux/kvm_para.h> -#endif +#include "standard-headers/asm-x86/kvm_para.h" #include "sysemu/sysemu.h" #include "hw/qdev-properties.h" diff --git a/target/i386/kvm.c b/target/i386/kvm.c index 0c656a9..6511329 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -18,7 +18,7 @@ #include <sys/utsname.h> #include <linux/kvm.h> -#include <linux/kvm_para.h> +#include "standard-headers/asm-x86/kvm_para.h" #include "qemu-common.h" #include "cpu.h" @@ -387,7 +387,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, ret &= ~(1U << KVM_FEATURE_PV_UNHALT); } } else if (function == KVM_CPUID_FEATURES && reg == R_EDX) { - ret |= KVM_HINTS_DEDICATED; + ret |= 1U << KVM_HINTS_DEDICATED; found = 1; }
Switch to the header we imported from Linux, this allows us to drop a hack in kvm_i386.h. More code will be dropped in the next patch. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- include/sysemu/kvm.h | 1 - target/i386/cpu.h | 2 -- target/i386/kvm_i386.h | 6 ------ hw/i386/kvm/clock.c | 2 +- target/i386/cpu.c | 4 +--- target/i386/kvm.c | 4 ++-- 6 files changed, 4 insertions(+), 15 deletions(-)