[PULL 07/28] x86/cpu: use standard-headers/asm-x86.kvm_para.h

[PULL 07/28] x86/cpu: use standard-headers/asm-x86.kvm_para.h

[Michael S. Tsirkin]

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(-)

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;
     }
 
-- 
MST

Re: [PULL 07/28] x86/cpu: use standard-headers/asm-x86.kvm_para.h

[Peter Maydell]

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

Re: [PULL 07/28] x86/cpu: use standard-headers/asm-x86.kvm_para.h

[Peter Maydell]

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

Re: [PULL 07/28] x86/cpu: use standard-headers/asm-x86.kvm_para.h

[Michael S. Tsirkin]

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

Re: [PULL 07/28] x86/cpu: use standard-headers/asm-x86.kvm_para.h

[Peter Maydell]

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

Re: [PULL 07/28] x86/cpu: use standard-headers/asm-x86.kvm_para.h

[Michael S. Tsirkin]

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.

-- 
MST

Re: [PULL 07/28] x86/cpu: use standard-headers/asm-x86.kvm_para.h

[Peter Maydell]

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

Re: [PULL 07/28] x86/cpu: use standard-headers/asm-x86.kvm_para.h

[Michael S. Tsirkin]

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

Re: [PULL 07/28] x86/cpu: use standard-headers/asm-x86.kvm_para.h

[Peter Maydell]

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

Re: [PULL 07/28] x86/cpu: use standard-headers/asm-x86.kvm_para.h

[Michael S. Tsirkin]

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

Re: [PULL 07/28] x86/cpu: use standard-headers/asm-x86.kvm_para.h

[Paolo Bonzini]

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