Re: [PATCH v2 02/10] KVM: define common __KVM_GUESTDBG_USE_SW/HW_BP values

2015-04-14 Thread Christoffer Dall 
On Mon, Apr 13, 2015 at 03:51:33PM +0100, Alex Bennée wrote:
 
 Christoffer Dall christoffer.d...@linaro.org writes:
 
  On Tue, Mar 31, 2015 at 04:08:00PM +0100, Alex Bennée wrote:
  Currently x86, powerpc and soon arm64 use the same two architecture
  specific bits for guest debug support for software and hardware
  breakpoints. This makes the shared values explicit while leaving the
  gate open for another architecture to use some other value if they
  really really want to.
  
  Signed-off-by: Alex Bennée alex.ben...@linaro.org
  
  diff --git a/arch/powerpc/include/uapi/asm/kvm.h 
  b/arch/powerpc/include/uapi/asm/kvm.h
  index ab4d473..1731569 100644
  --- a/arch/powerpc/include/uapi/asm/kvm.h
  +++ b/arch/powerpc/include/uapi/asm/kvm.h
  @@ -310,8 +310,8 @@ struct kvm_guest_debug_arch {
* and upper 16 bits are architecture specific. Architecture specific 
  defines
* that ioctl is for setting hardware breakpoint or software breakpoint.
*/
  -#define KVM_GUESTDBG_USE_SW_BP0x0001
  -#define KVM_GUESTDBG_USE_HW_BP0x0002
  +#define KVM_GUESTDBG_USE_SW_BP__KVM_GUESTDBG_USE_SW_BP
  +#define KVM_GUESTDBG_USE_HW_BP__KVM_GUESTDBG_USE_HW_BP
   
   /* definition of registers in kvm_run */
   struct kvm_sync_regs {
  diff --git a/arch/x86/include/uapi/asm/kvm.h 
  b/arch/x86/include/uapi/asm/kvm.h
  index d7dcef5..1438202 100644
  --- a/arch/x86/include/uapi/asm/kvm.h
  +++ b/arch/x86/include/uapi/asm/kvm.h
  @@ -250,8 +250,8 @@ struct kvm_debug_exit_arch {
 __u64 dr7;
   };
   
  -#define KVM_GUESTDBG_USE_SW_BP0x0001
  -#define KVM_GUESTDBG_USE_HW_BP0x0002
  +#define KVM_GUESTDBG_USE_SW_BP__KVM_GUESTDBG_USE_SW_BP
  +#define KVM_GUESTDBG_USE_HW_BP__KVM_GUESTDBG_USE_HW_BP
   #define KVM_GUESTDBG_INJECT_DB0x0004
   #define KVM_GUESTDBG_INJECT_BP0x0008
   
  diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
  index 5eedf84..ce2db14 100644
  --- a/include/uapi/linux/kvm.h
  +++ b/include/uapi/linux/kvm.h
  @@ -525,8 +525,16 @@ struct kvm_s390_irq {
   
   /* for KVM_SET_GUEST_DEBUG */
   
  -#define KVM_GUESTDBG_ENABLE   0x0001
  -#define KVM_GUESTDBG_SINGLESTEP   0x0002
  +#define KVM_GUESTDBG_ENABLE   (1  0)
  +#define KVM_GUESTDBG_SINGLESTEP   (1  1)
  +
  +/*
  + * Architecture specific stuff uses the top 16 bits of the field,
 
  can you be more specific than 'stuff' here?  features?
 
  + * however there is some shared commonality for the common cases
 
  I don't like this sentence; shared commonality is a pleonasm and the use
  of however makes it sounds like there's some caveat here.
 
 OK I can see that - after I looked it up ;-)
 
  If the top 16 bits are indeed arhictecture specific, then I think they
  should just be defined in their architecture specific headers.  Unless
  the idea here is that there's a fixed set of of flags that architectures
  can choose to support, in which case it should simply be defined in the
  common header.
 
 Well an architecture might not support some features and want to use
 those bits for something else? I didn't want to force the bottom two
 of the architecture specific bits to wasted if the features don't exist.
 
In that case I think the definition is local to each architecture and
should indeed just be duplicated.  The __ definitions complicate more
than they help as they are exported to userspace etc.  The KVM
maintainers may have a different view on this though.

-Christoffer
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 02/10] KVM: define common __KVM_GUESTDBG_USE_SW/HW_BP values

2015-04-13 Thread Christoffer Dall 
On Tue, Mar 31, 2015 at 04:08:00PM +0100, Alex Bennée wrote:
 Currently x86, powerpc and soon arm64 use the same two architecture
 specific bits for guest debug support for software and hardware
 breakpoints. This makes the shared values explicit while leaving the
 gate open for another architecture to use some other value if they
 really really want to.
 
 Signed-off-by: Alex Bennée alex.ben...@linaro.org
 
 diff --git a/arch/powerpc/include/uapi/asm/kvm.h 
 b/arch/powerpc/include/uapi/asm/kvm.h
 index ab4d473..1731569 100644
 --- a/arch/powerpc/include/uapi/asm/kvm.h
 +++ b/arch/powerpc/include/uapi/asm/kvm.h
 @@ -310,8 +310,8 @@ struct kvm_guest_debug_arch {
   * and upper 16 bits are architecture specific. Architecture specific defines
   * that ioctl is for setting hardware breakpoint or software breakpoint.
   */
 -#define KVM_GUESTDBG_USE_SW_BP   0x0001
 -#define KVM_GUESTDBG_USE_HW_BP   0x0002
 +#define KVM_GUESTDBG_USE_SW_BP   __KVM_GUESTDBG_USE_SW_BP
 +#define KVM_GUESTDBG_USE_HW_BP   __KVM_GUESTDBG_USE_HW_BP
  
  /* definition of registers in kvm_run */
  struct kvm_sync_regs {
 diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
 index d7dcef5..1438202 100644
 --- a/arch/x86/include/uapi/asm/kvm.h
 +++ b/arch/x86/include/uapi/asm/kvm.h
 @@ -250,8 +250,8 @@ struct kvm_debug_exit_arch {
   __u64 dr7;
  };
  
 -#define KVM_GUESTDBG_USE_SW_BP   0x0001
 -#define KVM_GUESTDBG_USE_HW_BP   0x0002
 +#define KVM_GUESTDBG_USE_SW_BP   __KVM_GUESTDBG_USE_SW_BP
 +#define KVM_GUESTDBG_USE_HW_BP   __KVM_GUESTDBG_USE_HW_BP
  #define KVM_GUESTDBG_INJECT_DB   0x0004
  #define KVM_GUESTDBG_INJECT_BP   0x0008
  
 diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
 index 5eedf84..ce2db14 100644
 --- a/include/uapi/linux/kvm.h
 +++ b/include/uapi/linux/kvm.h
 @@ -525,8 +525,16 @@ struct kvm_s390_irq {
  
  /* for KVM_SET_GUEST_DEBUG */
  
 -#define KVM_GUESTDBG_ENABLE  0x0001
 -#define KVM_GUESTDBG_SINGLESTEP  0x0002
 +#define KVM_GUESTDBG_ENABLE  (1  0)
 +#define KVM_GUESTDBG_SINGLESTEP  (1  1)
 +
 +/*
 + * Architecture specific stuff uses the top 16 bits of the field,

can you be more specific than 'stuff' here?  features?

 + * however there is some shared commonality for the common cases

I don't like this sentence; shared commonality is a pleonasm and the use
of however makes it sounds like there's some caveat here.

If the top 16 bits are indeed arhictecture specific, then I think they
should just be defined in their architecture specific headers.  Unless
the idea here is that there's a fixed set of of flags that architectures
can choose to support, in which case it should simply be defined in the
common header.


 + */
 +#define __KVM_GUESTDBG_USE_SW_BP (1  16)
 +#define __KVM_GUESTDBG_USE_HW_BP (1  17)
 +
  
  struct kvm_guest_debug {
   __u32 control;
 -- 
 2.3.4
 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 02/10] KVM: define common __KVM_GUESTDBG_USE_SW/HW_BP values

2015-04-13 Thread Alex Bennée 

Christoffer Dall christoffer.d...@linaro.org writes:

 On Tue, Mar 31, 2015 at 04:08:00PM +0100, Alex Bennée wrote:
 Currently x86, powerpc and soon arm64 use the same two architecture
 specific bits for guest debug support for software and hardware
 breakpoints. This makes the shared values explicit while leaving the
 gate open for another architecture to use some other value if they
 really really want to.
 
 Signed-off-by: Alex Bennée alex.ben...@linaro.org
 
 diff --git a/arch/powerpc/include/uapi/asm/kvm.h 
 b/arch/powerpc/include/uapi/asm/kvm.h
 index ab4d473..1731569 100644
 --- a/arch/powerpc/include/uapi/asm/kvm.h
 +++ b/arch/powerpc/include/uapi/asm/kvm.h
 @@ -310,8 +310,8 @@ struct kvm_guest_debug_arch {
   * and upper 16 bits are architecture specific. Architecture specific 
 defines
   * that ioctl is for setting hardware breakpoint or software breakpoint.
   */
 -#define KVM_GUESTDBG_USE_SW_BP  0x0001
 -#define KVM_GUESTDBG_USE_HW_BP  0x0002
 +#define KVM_GUESTDBG_USE_SW_BP  __KVM_GUESTDBG_USE_SW_BP
 +#define KVM_GUESTDBG_USE_HW_BP  __KVM_GUESTDBG_USE_HW_BP
  
  /* definition of registers in kvm_run */
  struct kvm_sync_regs {
 diff --git a/arch/x86/include/uapi/asm/kvm.h 
 b/arch/x86/include/uapi/asm/kvm.h
 index d7dcef5..1438202 100644
 --- a/arch/x86/include/uapi/asm/kvm.h
 +++ b/arch/x86/include/uapi/asm/kvm.h
 @@ -250,8 +250,8 @@ struct kvm_debug_exit_arch {
  __u64 dr7;
  };
  
 -#define KVM_GUESTDBG_USE_SW_BP  0x0001
 -#define KVM_GUESTDBG_USE_HW_BP  0x0002
 +#define KVM_GUESTDBG_USE_SW_BP  __KVM_GUESTDBG_USE_SW_BP
 +#define KVM_GUESTDBG_USE_HW_BP  __KVM_GUESTDBG_USE_HW_BP
  #define KVM_GUESTDBG_INJECT_DB  0x0004
  #define KVM_GUESTDBG_INJECT_BP  0x0008
  
 diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
 index 5eedf84..ce2db14 100644
 --- a/include/uapi/linux/kvm.h
 +++ b/include/uapi/linux/kvm.h
 @@ -525,8 +525,16 @@ struct kvm_s390_irq {
  
  /* for KVM_SET_GUEST_DEBUG */
  
 -#define KVM_GUESTDBG_ENABLE 0x0001
 -#define KVM_GUESTDBG_SINGLESTEP 0x0002
 +#define KVM_GUESTDBG_ENABLE (1  0)
 +#define KVM_GUESTDBG_SINGLESTEP (1  1)
 +
 +/*
 + * Architecture specific stuff uses the top 16 bits of the field,

 can you be more specific than 'stuff' here?  features?

 + * however there is some shared commonality for the common cases

 I don't like this sentence; shared commonality is a pleonasm and the use
 of however makes it sounds like there's some caveat here.

OK I can see that - after I looked it up ;-)

 If the top 16 bits are indeed arhictecture specific, then I think they
 should just be defined in their architecture specific headers.  Unless
 the idea here is that there's a fixed set of of flags that architectures
 can choose to support, in which case it should simply be defined in the
 common header.

Well an architecture might not support some features and want to use
those bits for something else? I didn't want to force the bottom two
of the architecture specific bits to wasted if the features don't exist.



 + */
 +#define __KVM_GUESTDBG_USE_SW_BP(1  16)
 +#define __KVM_GUESTDBG_USE_HW_BP(1  17)
 +
  
  struct kvm_guest_debug {
  __u32 control;
 -- 
 2.3.4
 

-- 
Alex Bennée
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 02/10] KVM: define common __KVM_GUESTDBG_USE_SW/HW_BP values

2015-04-13 Thread Andrew Jones 
On Mon, Apr 13, 2015 at 03:51:33PM +0100, Alex Bennée wrote:
 
 Christoffer Dall christoffer.d...@linaro.org writes:
 
  On Tue, Mar 31, 2015 at 04:08:00PM +0100, Alex Bennée wrote:
  Currently x86, powerpc and soon arm64 use the same two architecture
  specific bits for guest debug support for software and hardware
  breakpoints. This makes the shared values explicit while leaving the
  gate open for another architecture to use some other value if they
  really really want to.
  
  Signed-off-by: Alex Bennée alex.ben...@linaro.org
  
  diff --git a/arch/powerpc/include/uapi/asm/kvm.h 
  b/arch/powerpc/include/uapi/asm/kvm.h
  index ab4d473..1731569 100644
  --- a/arch/powerpc/include/uapi/asm/kvm.h
  +++ b/arch/powerpc/include/uapi/asm/kvm.h
  @@ -310,8 +310,8 @@ struct kvm_guest_debug_arch {
* and upper 16 bits are architecture specific. Architecture specific 
  defines
* that ioctl is for setting hardware breakpoint or software breakpoint.
*/
  -#define KVM_GUESTDBG_USE_SW_BP0x0001
  -#define KVM_GUESTDBG_USE_HW_BP0x0002
  +#define KVM_GUESTDBG_USE_SW_BP__KVM_GUESTDBG_USE_SW_BP
  +#define KVM_GUESTDBG_USE_HW_BP__KVM_GUESTDBG_USE_HW_BP
   
   /* definition of registers in kvm_run */
   struct kvm_sync_regs {
  diff --git a/arch/x86/include/uapi/asm/kvm.h 
  b/arch/x86/include/uapi/asm/kvm.h
  index d7dcef5..1438202 100644
  --- a/arch/x86/include/uapi/asm/kvm.h
  +++ b/arch/x86/include/uapi/asm/kvm.h
  @@ -250,8 +250,8 @@ struct kvm_debug_exit_arch {
 __u64 dr7;
   };
   
  -#define KVM_GUESTDBG_USE_SW_BP0x0001
  -#define KVM_GUESTDBG_USE_HW_BP0x0002
  +#define KVM_GUESTDBG_USE_SW_BP__KVM_GUESTDBG_USE_SW_BP
  +#define KVM_GUESTDBG_USE_HW_BP__KVM_GUESTDBG_USE_HW_BP
   #define KVM_GUESTDBG_INJECT_DB0x0004
   #define KVM_GUESTDBG_INJECT_BP0x0008
   
  diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
  index 5eedf84..ce2db14 100644
  --- a/include/uapi/linux/kvm.h
  +++ b/include/uapi/linux/kvm.h
  @@ -525,8 +525,16 @@ struct kvm_s390_irq {
   
   /* for KVM_SET_GUEST_DEBUG */
   
  -#define KVM_GUESTDBG_ENABLE   0x0001
  -#define KVM_GUESTDBG_SINGLESTEP   0x0002
  +#define KVM_GUESTDBG_ENABLE   (1  0)
  +#define KVM_GUESTDBG_SINGLESTEP   (1  1)
  +
  +/*
  + * Architecture specific stuff uses the top 16 bits of the field,
 
  can you be more specific than 'stuff' here?  features?
 
  + * however there is some shared commonality for the common cases
 
  I don't like this sentence; shared commonality is a pleonasm and the use
  of however makes it sounds like there's some caveat here.
 
 OK I can see that - after I looked it up ;-)
 
  If the top 16 bits are indeed arhictecture specific, then I think they
  should just be defined in their architecture specific headers.  Unless
  the idea here is that there's a fixed set of of flags that architectures
  can choose to support, in which case it should simply be defined in the
  common header.
 
 Well an architecture might not support some features and want to use
 those bits for something else? I didn't want to force the bottom two
 of the architecture specific bits to wasted if the features don't exist.

This change comes from a discussion we had on v1 of this series
http://www.spinics.net/lists/kvm-arm/msg12409.html

 
 
 
  + */
  +#define __KVM_GUESTDBG_USE_SW_BP  (1  16)
  +#define __KVM_GUESTDBG_USE_HW_BP  (1  17)
  +
   
   struct kvm_guest_debug {
 __u32 control;
  -- 
  2.3.4
  
 
 -- 
 Alex Bennée
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev