RE: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Bhushan Bharat-R65777]

 -Original Message-
 From: Alexander Graf [mailto:ag...@suse.de]
 Sent: Friday, October 04, 2013 4:46 PM
 To: Bhushan Bharat-R65777
 Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org
 Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to
 epapr_hypercall()
 
 
 On 04.10.2013, at 06:26, Bhushan Bharat-R65777 wrote:
 
 
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Thursday, October 03, 2013 12:04 AM
  To: Alexander Graf
  Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org;
  k...@vger.kernel.org; Bhushan
  Bharat-R65777
  Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to
  epapr_hypercall()
 
  On Wed, 2013-10-02 at 19:54 +0200, Alexander Graf wrote:
  On 02.10.2013, at 19:49, Scott Wood wrote:
 
  On Wed, 2013-10-02 at 19:46 +0200, Alexander Graf wrote:
  On 02.10.2013, at 19:42, Scott Wood wrote:
 
  On Wed, 2013-10-02 at 19:17 +0200, Alexander Graf wrote:
  On 02.10.2013, at 19:04, Scott Wood wrote:
 
  On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote:
  On 02.10.2013, at 18:40, Scott Wood wrote:
 
  On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
  Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't
  have
  epapr_hcalls.S compiled into the code base then and the bl above
  would reference an unknown function.
 
  KVM_GUEST selects EPAPR_PARAVIRT.
 
  But you can not select KVM_GUEST and still call these inline
  functions,
  no?
 
  No.
 
  Like kvm_arch_para_features().
 
  Where does that get called without KVM_GUEST?
 
  How would that work currently, with the call to kvm_hypercall()
  in arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)?
 
  It wouldn't ever get called because kvm_hypercall() ends up
  always
  returning EV_UNIMPLEMENTED when #ifndef CONFIG_KVM_GUEST.
 
  OK, so the objection is to removing that stub?  Where would we
  actually want to call this without knowing that KVM_GUEST or
  EPAPR_PARAVIRT are enabled?
 
  In probing code. I usually prefer
 
  if (kvm_feature_available(X)) {
   ...
  }
 
  over
 
  #ifdef CONFIG_KVM_GUEST
  if (kvm_feature_available(X)) {
   ...
  }
  #endif
 
  at least when I can avoid it. With the current code the compiler
  would be
  smart enough to just optimize out the complete branch.
 
  Sure.  My point is, where would you be calling that where the
  entire file isn't predicated on (or selecting) CONFIG_KVM_GUEST or 
  similar?
 
  We don't do these stubs for every single function in the kernel --
  only ones where the above is a reasonable use case.
 
  Yeah, I'm fine on dropping it, but we need to make that a conscious
  decision
  and verify that no caller relies on it.
 
  kvm_para_has_feature() is called from arch/powerpc/kernel/kvm.c,
  arch/x86/kernel/kvm.c, and arch/x86/kernel/kvmclock.c, all of which
  are enabled by CONFIG_KVM_GUEST.
 
  I did find one example of kvm_para_available() being used in an
  unexpected place
  -- sound/pci/intel8x0.c.  It defines its own non-CONFIG_KVM_GUEST
  stub, even though x86 defines kvm_para_available() using inline CPUID
  stuff which should work without CONFIG_KVM_GUEST.
  I'm not sure why it even needs to do that, though -- shouldn't the
  subsequent PCI subsystem vendor/device check should be sufficient?
  No hypercalls are involved.
 
  That said, the possibility that some random driver might want to make
  use of paravirt features is a decent argument for keeping the stub.
 
 
  I am not sure where we are agreeing on?
  Do we want to remove the stub in arch/powerpc/include/asm/kvm_para.h ? as
 there is no caller without KVM_GUEST and in future caller ensure this to be
 called only from code selected by KVM_GUEST?
 
  Or let this stub stay to avoid any random driver calling this ?
 
 I think the most reasonable way forward is to add a stub for non-CONFIG_EPAPR 
 to
 the epapr code, then replace the kvm bits with generic epapr bits (which your
 patches already do).

Please describe which stub you are talking about.

Thanks
-Bharat

 
 With that we should be 100% equivalent to today's code, just with a lot less
 lines of code :).
 
 
 Alex
 


--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Alexander Graf]

On 07.10.2013, at 17:15, Bhushan Bharat-R65777 r65...@freescale.com wrote:

 
 
 -Original Message-
 From: Alexander Graf [mailto:ag...@suse.de]
 Sent: Friday, October 04, 2013 4:46 PM
 To: Bhushan Bharat-R65777
 Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org
 Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to
 epapr_hypercall()
 
 
 On 04.10.2013, at 06:26, Bhushan Bharat-R65777 wrote:
 
 
 
 -Original Message-
 From: Wood Scott-B07421
 Sent: Thursday, October 03, 2013 12:04 AM
 To: Alexander Graf
 Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org;
 k...@vger.kernel.org; Bhushan
 Bharat-R65777
 Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to
 epapr_hypercall()
 
 On Wed, 2013-10-02 at 19:54 +0200, Alexander Graf wrote:
 On 02.10.2013, at 19:49, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 19:46 +0200, Alexander Graf wrote:
 On 02.10.2013, at 19:42, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 19:17 +0200, Alexander Graf wrote:
 On 02.10.2013, at 19:04, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote:
 On 02.10.2013, at 18:40, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
 Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't
 have
 epapr_hcalls.S compiled into the code base then and the bl above
 would reference an unknown function.
 
 KVM_GUEST selects EPAPR_PARAVIRT.
 
 But you can not select KVM_GUEST and still call these inline
 functions,
 no?
 
 No.
 
 Like kvm_arch_para_features().
 
 Where does that get called without KVM_GUEST?
 
 How would that work currently, with the call to kvm_hypercall()
 in arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)?
 
 It wouldn't ever get called because kvm_hypercall() ends up
 always
 returning EV_UNIMPLEMENTED when #ifndef CONFIG_KVM_GUEST.
 
 OK, so the objection is to removing that stub?  Where would we
 actually want to call this without knowing that KVM_GUEST or
 EPAPR_PARAVIRT are enabled?
 
 In probing code. I usually prefer
 
 if (kvm_feature_available(X)) {
 ...
 }
 
 over
 
 #ifdef CONFIG_KVM_GUEST
 if (kvm_feature_available(X)) {
 ...
 }
 #endif
 
 at least when I can avoid it. With the current code the compiler
 would be
 smart enough to just optimize out the complete branch.
 
 Sure.  My point is, where would you be calling that where the
 entire file isn't predicated on (or selecting) CONFIG_KVM_GUEST or 
 similar?
 
 We don't do these stubs for every single function in the kernel --
 only ones where the above is a reasonable use case.
 
 Yeah, I'm fine on dropping it, but we need to make that a conscious
 decision
 and verify that no caller relies on it.
 
 kvm_para_has_feature() is called from arch/powerpc/kernel/kvm.c,
 arch/x86/kernel/kvm.c, and arch/x86/kernel/kvmclock.c, all of which
 are enabled by CONFIG_KVM_GUEST.
 
 I did find one example of kvm_para_available() being used in an
 unexpected place
 -- sound/pci/intel8x0.c.  It defines its own non-CONFIG_KVM_GUEST
 stub, even though x86 defines kvm_para_available() using inline CPUID
 stuff which should work without CONFIG_KVM_GUEST.
 I'm not sure why it even needs to do that, though -- shouldn't the
 subsequent PCI subsystem vendor/device check should be sufficient?
 No hypercalls are involved.
 
 That said, the possibility that some random driver might want to make
 use of paravirt features is a decent argument for keeping the stub.
 
 
 I am not sure where we are agreeing on?
 Do we want to remove the stub in arch/powerpc/include/asm/kvm_para.h ? as
 there is no caller without KVM_GUEST and in future caller ensure this to be
 called only from code selected by KVM_GUEST?
 
 Or let this stub stay to avoid any random driver calling this ?
 
 I think the most reasonable way forward is to add a stub for 
 non-CONFIG_EPAPR to
 the epapr code, then replace the kvm bits with generic epapr bits (which your
 patches already do).
 
 Please describe which stub you are talking about.

kvm_hypercall is always available, regardless of the config option, which makes 
all its subfunctions always available as well.


Alex

---

#ifdef CONFIG_KVM_GUEST

#include linux/of.h

static inline int kvm_para_available(void)
{
struct device_node *hyper_node;

hyper_node = of_find_node_by_path(/hypervisor);
if (!hyper_node)
return 0;

if (!of_device_is_compatible(hyper_node, linux,kvm))
return 0;

return 1;
}

extern unsigned long kvm_hypercall(unsigned long *in,
   unsigned long *out,
   unsigned long nr);

#else

static inline int kvm_para_available(void)
{
return 0;
}

static unsigned long kvm_hypercall(unsigned long *in,
   unsigned long *out,
   unsigned long nr)
{
return EV_UNIMPLEMENTED;
}

#endif

--
To unsubscribe from this list: send the line unsubscribe

RE: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Bhushan Bharat-R65777]

  at least when I can avoid it. With the current code the compiler
  would be
  smart enough to just optimize out the complete branch.
 
  Sure.  My point is, where would you be calling that where the
  entire file isn't predicated on (or selecting) CONFIG_KVM_GUEST or
 similar?
 
  We don't do these stubs for every single function in the kernel
  -- only ones where the above is a reasonable use case.
 
  Yeah, I'm fine on dropping it, but we need to make that a
  conscious decision
  and verify that no caller relies on it.
 
  kvm_para_has_feature() is called from arch/powerpc/kernel/kvm.c,
  arch/x86/kernel/kvm.c, and arch/x86/kernel/kvmclock.c, all of which
  are enabled by CONFIG_KVM_GUEST.
 
  I did find one example of kvm_para_available() being used in an
  unexpected place
  -- sound/pci/intel8x0.c.  It defines its own non-CONFIG_KVM_GUEST
  stub, even though x86 defines kvm_para_available() using inline
  CPUID stuff which should work without CONFIG_KVM_GUEST.
  I'm not sure why it even needs to do that, though -- shouldn't the
  subsequent PCI subsystem vendor/device check should be sufficient?
  No hypercalls are involved.
 
  That said, the possibility that some random driver might want to
  make use of paravirt features is a decent argument for keeping the stub.
 
 
  I am not sure where we are agreeing on?
  Do we want to remove the stub in arch/powerpc/include/asm/kvm_para.h
  ? as
  there is no caller without KVM_GUEST and in future caller ensure this
  to be called only from code selected by KVM_GUEST?
 
  Or let this stub stay to avoid any random driver calling this ?
 
  I think the most reasonable way forward is to add a stub for
  non-CONFIG_EPAPR to the epapr code, then replace the kvm bits with
  generic epapr bits (which your patches already do).
 
  Please describe which stub you are talking about.
 
 kvm_hypercall is always available, regardless of the config option, which 
 makes
 all its subfunctions always available as well.

This patch renames kvm_hypercall() to epapr_hypercall() and which is always 
available. And the kvm_hypercall() friends now directly calls epapr_hypercall().
IIUC, So what you are trying to say is let the kvm_hypercall() friends keep on 
calling kvm_hypercall() itself and a sub something like this:

#ifdef CONFIG_KVM_GUEST
 
static unsigned long kvm_hypercall(unsigned long *in,
unsigned long *out,
unsigned long nr)
{
return epapr_hypercall(in, out. nr);
}
 
 #else
static unsigned long kvm_hypercall(unsigned long *in,
unsigned long *out,
unsigned long nr) {
 return EV_UNIMPLEMENTED;
}
-

I am still not really convinced about why we want to keep this stub where we 
know this is not called outside KVM_GUEST and calling this without KVM_GUEST is 
debatable.

Thanks
-Bharat

Thanks
-Bharat

 
 
 Alex
 
 ---
 
 #ifdef CONFIG_KVM_GUEST
 
 #include linux/of.h
 
 static inline int kvm_para_available(void) {
 struct device_node *hyper_node;
 
 hyper_node = of_find_node_by_path(/hypervisor);
 if (!hyper_node)
 return 0;
 
 if (!of_device_is_compatible(hyper_node, linux,kvm))
 return 0;
 
 return 1;
 }
 
 extern unsigned long kvm_hypercall(unsigned long *in,
unsigned long *out,
unsigned long nr);
 
 #else
 
 static inline int kvm_para_available(void) {
 return 0;
 }
 
 static unsigned long kvm_hypercall(unsigned long *in,
unsigned long *out,
unsigned long nr) {
 return EV_UNIMPLEMENTED;
 }
 
 #endif
 


--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Alexander Graf]

On 07.10.2013, at 17:43, Bhushan Bharat-R65777 r65...@freescale.com wrote:

 at least when I can avoid it. With the current code the compiler
 would be
 smart enough to just optimize out the complete branch.
 
 Sure.  My point is, where would you be calling that where the
 entire file isn't predicated on (or selecting) CONFIG_KVM_GUEST or
 similar?
 
 We don't do these stubs for every single function in the kernel
 -- only ones where the above is a reasonable use case.
 
 Yeah, I'm fine on dropping it, but we need to make that a
 conscious decision
 and verify that no caller relies on it.
 
 kvm_para_has_feature() is called from arch/powerpc/kernel/kvm.c,
 arch/x86/kernel/kvm.c, and arch/x86/kernel/kvmclock.c, all of which
 are enabled by CONFIG_KVM_GUEST.
 
 I did find one example of kvm_para_available() being used in an
 unexpected place
 -- sound/pci/intel8x0.c.  It defines its own non-CONFIG_KVM_GUEST
 stub, even though x86 defines kvm_para_available() using inline
 CPUID stuff which should work without CONFIG_KVM_GUEST.
 I'm not sure why it even needs to do that, though -- shouldn't the
 subsequent PCI subsystem vendor/device check should be sufficient?
 No hypercalls are involved.
 
 That said, the possibility that some random driver might want to
 make use of paravirt features is a decent argument for keeping the stub.
 
 
 I am not sure where we are agreeing on?
 Do we want to remove the stub in arch/powerpc/include/asm/kvm_para.h
 ? as
 there is no caller without KVM_GUEST and in future caller ensure this
 to be called only from code selected by KVM_GUEST?
 
 Or let this stub stay to avoid any random driver calling this ?
 
 I think the most reasonable way forward is to add a stub for
 non-CONFIG_EPAPR to the epapr code, then replace the kvm bits with
 generic epapr bits (which your patches already do).
 
 Please describe which stub you are talking about.
 
 kvm_hypercall is always available, regardless of the config option, which 
 makes
 all its subfunctions always available as well.
 
 This patch renames kvm_hypercall() to epapr_hypercall() and which is always 
 available. And the kvm_hypercall() friends now directly calls 
 epapr_hypercall().
 IIUC, So what you are trying to say is let the kvm_hypercall() friends keep 
 on calling kvm_hypercall() itself and a sub something like this:

No, what I'm saying is that we either

  a) drop the whole #ifndef code path consciously. This would have to be a 
separate patch with a separate discussion. It's orthogonal to combining 
kvm_hypercall() and epapr_hypercall()

  b) add the #ifndef path to epapr_hypercall()

I prefer b, Scott prefers b.


Alex

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Bhushan Bharat-R65777]

 -Original Message-
 From: kvm-ppc-ow...@vger.kernel.org [mailto:kvm-ppc-ow...@vger.kernel.org] On
 Behalf Of Alexander Graf
 Sent: Monday, October 07, 2013 9:16 PM
 To: Bhushan Bharat-R65777
 Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org
 Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to
 epapr_hypercall()
 
 
 On 07.10.2013, at 17:43, Bhushan Bharat-R65777 r65...@freescale.com wrote:
 
  at least when I can avoid it. With the current code the
  compiler would be
  smart enough to just optimize out the complete branch.
 
  Sure.  My point is, where would you be calling that where the
  entire file isn't predicated on (or selecting) CONFIG_KVM_GUEST
  or
  similar?
 
  We don't do these stubs for every single function in the kernel
  -- only ones where the above is a reasonable use case.
 
  Yeah, I'm fine on dropping it, but we need to make that a
  conscious decision
  and verify that no caller relies on it.
 
  kvm_para_has_feature() is called from arch/powerpc/kernel/kvm.c,
  arch/x86/kernel/kvm.c, and arch/x86/kernel/kvmclock.c, all of
  which are enabled by CONFIG_KVM_GUEST.
 
  I did find one example of kvm_para_available() being used in an
  unexpected place
  -- sound/pci/intel8x0.c.  It defines its own non-CONFIG_KVM_GUEST
  stub, even though x86 defines kvm_para_available() using inline
  CPUID stuff which should work without CONFIG_KVM_GUEST.
  I'm not sure why it even needs to do that, though -- shouldn't
  the subsequent PCI subsystem vendor/device check should be sufficient?
  No hypercalls are involved.
 
  That said, the possibility that some random driver might want to
  make use of paravirt features is a decent argument for keeping the 
  stub.
 
 
  I am not sure where we are agreeing on?
  Do we want to remove the stub in
  arch/powerpc/include/asm/kvm_para.h
  ? as
  there is no caller without KVM_GUEST and in future caller ensure
  this to be called only from code selected by KVM_GUEST?
 
  Or let this stub stay to avoid any random driver calling this ?
 
  I think the most reasonable way forward is to add a stub for
  non-CONFIG_EPAPR to the epapr code, then replace the kvm bits with
  generic epapr bits (which your patches already do).
 
  Please describe which stub you are talking about.
 
  kvm_hypercall is always available, regardless of the config option,
  which makes all its subfunctions always available as well.
 
  This patch renames kvm_hypercall() to epapr_hypercall() and which is always
 available. And the kvm_hypercall() friends now directly calls 
 epapr_hypercall().
  IIUC, So what you are trying to say is let the kvm_hypercall() friends keep 
  on
 calling kvm_hypercall() itself and a sub something like this:
 
 No, what I'm saying is that we either
 
   a) drop the whole #ifndef code path consciously. This would have to be a
 separate patch with a separate discussion. It's orthogonal to combining
 kvm_hypercall() and epapr_hypercall()
 
   b) add the #ifndef path to epapr_hypercall()

Do you mean like this in arch/powerpc/include/asm/epapr_hcalls.h

#ifdef CONFIG_KVM_GUEST
static inline unsigned long epapr_hypercall(unsigned long *in,
   unsigned long *out,
   unsigned long nr)
{
 // code for this function
} 
#else
static inline unsigned long epapr_hypercall(unsigned long *in,
   unsigned long *out,
   unsigned long nr)
{
return EV_UNIMPLEMENTED;
}
#endif

 
 I prefer b, Scott prefers b.
 
 
 Alex
 
 --
 To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body
 of a message to majord...@vger.kernel.org More majordomo info at
 http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Alexander Graf]

On 07.10.2013, at 18:04, Bhushan Bharat-R65777 r65...@freescale.com wrote:

 
 
 -Original Message-
 From: kvm-ppc-ow...@vger.kernel.org [mailto:kvm-ppc-ow...@vger.kernel.org] On
 Behalf Of Alexander Graf
 Sent: Monday, October 07, 2013 9:16 PM
 To: Bhushan Bharat-R65777
 Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org
 Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to
 epapr_hypercall()
 
 
 On 07.10.2013, at 17:43, Bhushan Bharat-R65777 r65...@freescale.com wrote:
 
 at least when I can avoid it. With the current code the
 compiler would be
 smart enough to just optimize out the complete branch.
 
 Sure.  My point is, where would you be calling that where the
 entire file isn't predicated on (or selecting) CONFIG_KVM_GUEST
 or
 similar?
 
 We don't do these stubs for every single function in the kernel
 -- only ones where the above is a reasonable use case.
 
 Yeah, I'm fine on dropping it, but we need to make that a
 conscious decision
 and verify that no caller relies on it.
 
 kvm_para_has_feature() is called from arch/powerpc/kernel/kvm.c,
 arch/x86/kernel/kvm.c, and arch/x86/kernel/kvmclock.c, all of
 which are enabled by CONFIG_KVM_GUEST.
 
 I did find one example of kvm_para_available() being used in an
 unexpected place
 -- sound/pci/intel8x0.c.  It defines its own non-CONFIG_KVM_GUEST
 stub, even though x86 defines kvm_para_available() using inline
 CPUID stuff which should work without CONFIG_KVM_GUEST.
 I'm not sure why it even needs to do that, though -- shouldn't
 the subsequent PCI subsystem vendor/device check should be sufficient?
 No hypercalls are involved.
 
 That said, the possibility that some random driver might want to
 make use of paravirt features is a decent argument for keeping the 
 stub.
 
 
 I am not sure where we are agreeing on?
 Do we want to remove the stub in
 arch/powerpc/include/asm/kvm_para.h
 ? as
 there is no caller without KVM_GUEST and in future caller ensure
 this to be called only from code selected by KVM_GUEST?
 
 Or let this stub stay to avoid any random driver calling this ?
 
 I think the most reasonable way forward is to add a stub for
 non-CONFIG_EPAPR to the epapr code, then replace the kvm bits with
 generic epapr bits (which your patches already do).
 
 Please describe which stub you are talking about.
 
 kvm_hypercall is always available, regardless of the config option,
 which makes all its subfunctions always available as well.
 
 This patch renames kvm_hypercall() to epapr_hypercall() and which is always
 available. And the kvm_hypercall() friends now directly calls 
 epapr_hypercall().
 IIUC, So what you are trying to say is let the kvm_hypercall() friends keep 
 on
 calling kvm_hypercall() itself and a sub something like this:
 
 No, what I'm saying is that we either
 
  a) drop the whole #ifndef code path consciously. This would have to be a
 separate patch with a separate discussion. It's orthogonal to combining
 kvm_hypercall() and epapr_hypercall()
 
  b) add the #ifndef path to epapr_hypercall()
 
 Do you mean like this in arch/powerpc/include/asm/epapr_hcalls.h
 
 #ifdef CONFIG_KVM_GUEST

CONFIG_EPAPR_PARAVIRT

Apart from that, yes, I think that's what we want.


Alex

 static inline unsigned long epapr_hypercall(unsigned long *in,
   unsigned long *out,
   unsigned long nr)
 {
 // code for this function
 } 
 #else
 static inline unsigned long epapr_hypercall(unsigned long *in,
   unsigned long *out,
   unsigned long nr)
 {
   return EV_UNIMPLEMENTED;
 }
 #endif


--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Bhushan Bharat-R65777]

 -Original Message-
 From: Alexander Graf [mailto:ag...@suse.de]
 Sent: Monday, October 07, 2013 9:43 PM
 To: Bhushan Bharat-R65777
 Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org
 Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to
 epapr_hypercall()
 
 
 On 07.10.2013, at 18:04, Bhushan Bharat-R65777 r65...@freescale.com wrote:
 
 
 
  -Original Message-
  From: kvm-ppc-ow...@vger.kernel.org
  [mailto:kvm-ppc-ow...@vger.kernel.org] On Behalf Of Alexander Graf
  Sent: Monday, October 07, 2013 9:16 PM
  To: Bhushan Bharat-R65777
  Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org
  Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to
  epapr_hypercall()
 
 
  On 07.10.2013, at 17:43, Bhushan Bharat-R65777 r65...@freescale.com 
  wrote:
 
  at least when I can avoid it. With the current code the
  compiler would be
  smart enough to just optimize out the complete branch.
 
  Sure.  My point is, where would you be calling that where the
  entire file isn't predicated on (or selecting)
  CONFIG_KVM_GUEST or
  similar?
 
  We don't do these stubs for every single function in the
  kernel
  -- only ones where the above is a reasonable use case.
 
  Yeah, I'm fine on dropping it, but we need to make that a
  conscious decision
  and verify that no caller relies on it.
 
  kvm_para_has_feature() is called from
  arch/powerpc/kernel/kvm.c, arch/x86/kernel/kvm.c, and
  arch/x86/kernel/kvmclock.c, all of which are enabled by
 CONFIG_KVM_GUEST.
 
  I did find one example of kvm_para_available() being used in an
  unexpected place
  -- sound/pci/intel8x0.c.  It defines its own
  non-CONFIG_KVM_GUEST stub, even though x86 defines
  kvm_para_available() using inline CPUID stuff which should work 
  without
 CONFIG_KVM_GUEST.
  I'm not sure why it even needs to do that, though -- shouldn't
  the subsequent PCI subsystem vendor/device check should be 
  sufficient?
  No hypercalls are involved.
 
  That said, the possibility that some random driver might want
  to make use of paravirt features is a decent argument for keeping the
 stub.
 
 
  I am not sure where we are agreeing on?
  Do we want to remove the stub in
  arch/powerpc/include/asm/kvm_para.h
  ? as
  there is no caller without KVM_GUEST and in future caller ensure
  this to be called only from code selected by KVM_GUEST?
 
  Or let this stub stay to avoid any random driver calling this ?
 
  I think the most reasonable way forward is to add a stub for
  non-CONFIG_EPAPR to the epapr code, then replace the kvm bits
  with generic epapr bits (which your patches already do).
 
  Please describe which stub you are talking about.
 
  kvm_hypercall is always available, regardless of the config option,
  which makes all its subfunctions always available as well.
 
  This patch renames kvm_hypercall() to epapr_hypercall() and which is
  always
  available. And the kvm_hypercall() friends now directly calls
 epapr_hypercall().
  IIUC, So what you are trying to say is let the kvm_hypercall()
  friends keep on
  calling kvm_hypercall() itself and a sub something like this:
 
  No, what I'm saying is that we either
 
   a) drop the whole #ifndef code path consciously. This would have to
  be a separate patch with a separate discussion. It's orthogonal to
  combining
  kvm_hypercall() and epapr_hypercall()
 
   b) add the #ifndef path to epapr_hypercall()
 
  Do you mean like this in arch/powerpc/include/asm/epapr_hcalls.h
 
  #ifdef CONFIG_KVM_GUEST
 
 CONFIG_EPAPR_PARAVIRT

Yes, I was getting confused why only KVM_GUEST as this not specific to 
KVM-GUEST.
Thank you

 
 Apart from that, yes, I think that's what we want.
 
 
 Alex
 
  static inline unsigned long epapr_hypercall(unsigned long *in,
unsigned long *out,
unsigned long nr) { // code for this
  function } #else static inline unsigned long epapr_hypercall(unsigned
  long *in,
unsigned long *out,
unsigned long nr) {
  return EV_UNIMPLEMENTED;
  }
  #endif
 
 


--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Alexander Graf]

On 04.10.2013, at 06:26, Bhushan Bharat-R65777 wrote:

 
 
 -Original Message-
 From: Wood Scott-B07421
 Sent: Thursday, October 03, 2013 12:04 AM
 To: Alexander Graf
 Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; k...@vger.kernel.org; 
 Bhushan
 Bharat-R65777
 Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to
 epapr_hypercall()
 
 On Wed, 2013-10-02 at 19:54 +0200, Alexander Graf wrote:
 On 02.10.2013, at 19:49, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 19:46 +0200, Alexander Graf wrote:
 On 02.10.2013, at 19:42, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 19:17 +0200, Alexander Graf wrote:
 On 02.10.2013, at 19:04, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote:
 On 02.10.2013, at 18:40, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
 Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have
 epapr_hcalls.S compiled into the code base then and the bl above would 
 reference
 an unknown function.
 
 KVM_GUEST selects EPAPR_PARAVIRT.
 
 But you can not select KVM_GUEST and still call these inline 
 functions,
 no?
 
 No.
 
 Like kvm_arch_para_features().
 
 Where does that get called without KVM_GUEST?
 
 How would that work currently, with the call to kvm_hypercall()
 in arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)?
 
 It wouldn't ever get called because kvm_hypercall() ends up always
 returning EV_UNIMPLEMENTED when #ifndef CONFIG_KVM_GUEST.
 
 OK, so the objection is to removing that stub?  Where would we
 actually want to call this without knowing that KVM_GUEST or
 EPAPR_PARAVIRT are enabled?
 
 In probing code. I usually prefer
 
 if (kvm_feature_available(X)) {
  ...
 }
 
 over
 
 #ifdef CONFIG_KVM_GUEST
 if (kvm_feature_available(X)) {
  ...
 }
 #endif
 
 at least when I can avoid it. With the current code the compiler would be
 smart enough to just optimize out the complete branch.
 
 Sure.  My point is, where would you be calling that where the entire
 file isn't predicated on (or selecting) CONFIG_KVM_GUEST or similar?
 
 We don't do these stubs for every single function in the kernel --
 only ones where the above is a reasonable use case.
 
 Yeah, I'm fine on dropping it, but we need to make that a conscious decision
 and verify that no caller relies on it.
 
 kvm_para_has_feature() is called from arch/powerpc/kernel/kvm.c,
 arch/x86/kernel/kvm.c, and arch/x86/kernel/kvmclock.c, all of which are 
 enabled
 by CONFIG_KVM_GUEST.
 
 I did find one example of kvm_para_available() being used in an unexpected 
 place
 -- sound/pci/intel8x0.c.  It defines its own non-CONFIG_KVM_GUEST stub, even
 though x86 defines kvm_para_available() using inline CPUID stuff which should
 work without CONFIG_KVM_GUEST.
 I'm not sure why it even needs to do that, though -- shouldn't the subsequent
 PCI subsystem vendor/device check should be sufficient?  No hypercalls are
 involved.
 
 That said, the possibility that some random driver might want to make use of
 paravirt features is a decent argument for keeping the stub.
 
 
 I am not sure where we are agreeing on?
 Do we want to remove the stub in arch/powerpc/include/asm/kvm_para.h ? as 
 there is no caller without KVM_GUEST and in future caller ensure this to be 
 called only from code selected by KVM_GUEST?
 
 Or let this stub stay to avoid any random driver calling this ?

I think the most reasonable way forward is to add a stub for non-CONFIG_EPAPR 
to the epapr code, then replace the kvm bits with generic epapr bits (which 
your patches already do).

With that we should be 100% equivalent to today's code, just with a lot less 
lines of code :).


Alex

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Bhushan Bharat-R65777]

 -Original Message-
 From: Wood Scott-B07421
 Sent: Thursday, October 03, 2013 12:04 AM
 To: Alexander Graf
 Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; k...@vger.kernel.org; 
 Bhushan
 Bharat-R65777
 Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to
 epapr_hypercall()
 
 On Wed, 2013-10-02 at 19:54 +0200, Alexander Graf wrote:
  On 02.10.2013, at 19:49, Scott Wood wrote:
 
   On Wed, 2013-10-02 at 19:46 +0200, Alexander Graf wrote:
   On 02.10.2013, at 19:42, Scott Wood wrote:
  
   On Wed, 2013-10-02 at 19:17 +0200, Alexander Graf wrote:
   On 02.10.2013, at 19:04, Scott Wood wrote:
  
   On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote:
   On 02.10.2013, at 18:40, Scott Wood wrote:
  
   On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
   Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have
 epapr_hcalls.S compiled into the code base then and the bl above would 
 reference
 an unknown function.
  
   KVM_GUEST selects EPAPR_PARAVIRT.
  
   But you can not select KVM_GUEST and still call these inline 
   functions,
 no?
  
   No.
  
   Like kvm_arch_para_features().
  
   Where does that get called without KVM_GUEST?
  
   How would that work currently, with the call to kvm_hypercall()
   in arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)?
  
   It wouldn't ever get called because kvm_hypercall() ends up always
 returning EV_UNIMPLEMENTED when #ifndef CONFIG_KVM_GUEST.
  
   OK, so the objection is to removing that stub?  Where would we
   actually want to call this without knowing that KVM_GUEST or
   EPAPR_PARAVIRT are enabled?
  
   In probing code. I usually prefer
  
   if (kvm_feature_available(X)) {
 ...
   }
  
   over
  
   #ifdef CONFIG_KVM_GUEST
   if (kvm_feature_available(X)) {
 ...
   }
   #endif
  
   at least when I can avoid it. With the current code the compiler would be
 smart enough to just optimize out the complete branch.
  
   Sure.  My point is, where would you be calling that where the entire
   file isn't predicated on (or selecting) CONFIG_KVM_GUEST or similar?
  
   We don't do these stubs for every single function in the kernel --
   only ones where the above is a reasonable use case.
 
  Yeah, I'm fine on dropping it, but we need to make that a conscious decision
 and verify that no caller relies on it.
 
 kvm_para_has_feature() is called from arch/powerpc/kernel/kvm.c,
 arch/x86/kernel/kvm.c, and arch/x86/kernel/kvmclock.c, all of which are 
 enabled
 by CONFIG_KVM_GUEST.
 
 I did find one example of kvm_para_available() being used in an unexpected 
 place
 -- sound/pci/intel8x0.c.  It defines its own non-CONFIG_KVM_GUEST stub, even
 though x86 defines kvm_para_available() using inline CPUID stuff which should
 work without CONFIG_KVM_GUEST.
 I'm not sure why it even needs to do that, though -- shouldn't the subsequent
 PCI subsystem vendor/device check should be sufficient?  No hypercalls are
 involved.
 
 That said, the possibility that some random driver might want to make use of
 paravirt features is a decent argument for keeping the stub.
 

I am not sure where we are agreeing on?
Do we want to remove the stub in arch/powerpc/include/asm/kvm_para.h ? as there 
is no caller without KVM_GUEST and in future caller ensure this to be called 
only from code selected by KVM_GUEST?

Or let this stub stay to avoid any random driver calling this ?

Thanks
-Bharat

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Alexander Graf]

On 23.09.2013, at 07:23, Bharat Bhushan wrote:

 kvm_hypercall() have nothing KVM specific, so renamed to epapr_hypercall().
 Also this in moved to arch/powerpc/include/asm/epapr_hcalls.h
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
 arch/powerpc/include/asm/epapr_hcalls.h |   36 +++
 arch/powerpc/include/asm/kvm_para.h |   23 -
 arch/powerpc/kernel/kvm.c   |   41 +-
 3 files changed, 44 insertions(+), 56 deletions(-)
 
 diff --git a/arch/powerpc/include/asm/epapr_hcalls.h 
 b/arch/powerpc/include/asm/epapr_hcalls.h
 index d3d6342..8a85f6f 100644
 --- a/arch/powerpc/include/asm/epapr_hcalls.h
 +++ b/arch/powerpc/include/asm/epapr_hcalls.h
 @@ -454,5 +454,41 @@ static inline unsigned int ev_idle(void)
 
   return r3;
 }
 +
 +static inline unsigned long epapr_hypercall(unsigned long *in,
 + unsigned long *out,
 + unsigned long nr)
 +{
 + unsigned long register r0 asm(r0);
 + unsigned long register r3 asm(r3) = in[0];
 + unsigned long register r4 asm(r4) = in[1];
 + unsigned long register r5 asm(r5) = in[2];
 + unsigned long register r6 asm(r6) = in[3];
 + unsigned long register r7 asm(r7) = in[4];
 + unsigned long register r8 asm(r8) = in[5];
 + unsigned long register r9 asm(r9) = in[6];
 + unsigned long register r10 asm(r10) = in[7];
 + unsigned long register r11 asm(r11) = nr;
 + unsigned long register r12 asm(r12);
 +
 + asm volatile(blepapr_hypercall_start
 +  : =r(r0), =r(r3), =r(r4), =r(r5), =r(r6),
 +=r(r7), =r(r8), =r(r9), =r(r10), =r(r11),
 +=r(r12)
 +  : r(r3), r(r4), r(r5), r(r6), r(r7), r(r8),
 +r(r9), r(r10), r(r11)
 +  : memory, cc, xer, ctr, lr);
 +
 + out[0] = r4;
 + out[1] = r5;
 + out[2] = r6;
 + out[3] = r7;
 + out[4] = r8;
 + out[5] = r9;
 + out[6] = r10;
 + out[7] = r11;
 +
 + return r3;
 +}
 #endif /* !__ASSEMBLY__ */
 #endif /* _EPAPR_HCALLS_H */
 diff --git a/arch/powerpc/include/asm/kvm_para.h 
 b/arch/powerpc/include/asm/kvm_para.h
 index 2b11965..c18660e 100644
 --- a/arch/powerpc/include/asm/kvm_para.h
 +++ b/arch/powerpc/include/asm/kvm_para.h
 @@ -39,10 +39,6 @@ static inline int kvm_para_available(void)
   return 1;
 }
 
 -extern unsigned long kvm_hypercall(unsigned long *in,
 -unsigned long *out,
 -unsigned long nr);
 -
 #else
 
 static inline int kvm_para_available(void)
 @@ -50,13 +46,6 @@ static inline int kvm_para_available(void)
   return 0;
 }
 
 -static unsigned long kvm_hypercall(unsigned long *in,
 -unsigned long *out,
 -unsigned long nr)
 -{
 - return EV_UNIMPLEMENTED;
 -}
 -
 #endif
 
 static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
 @@ -65,7 +54,7 @@ static inline long kvm_hypercall0_1(unsigned int nr, 
 unsigned long *r2)
   unsigned long out[8];
   unsigned long r;
 
 - r = kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
 + r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));

Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have epapr_hcalls.S 
compiled into the code base then and the bl above would reference an unknown 
function.


Alex

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Scott Wood]

On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
 On 23.09.2013, at 07:23, Bharat Bhushan wrote:
  static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
  @@ -65,7 +54,7 @@ static inline long kvm_hypercall0_1(unsigned int nr, 
  unsigned long *r2)
  unsigned long out[8];
  unsigned long r;
  
  -   r = kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
  +   r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
 
 Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have 
 epapr_hcalls.S compiled into the code base then and the bl above would 
 reference an unknown function.

KVM_GUEST selects EPAPR_PARAVIRT.

-Scott



--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Alexander Graf]

On 02.10.2013, at 18:40, Scott Wood wrote:

 On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
 On 23.09.2013, at 07:23, Bharat Bhushan wrote:
 static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
 @@ -65,7 +54,7 @@ static inline long kvm_hypercall0_1(unsigned int nr, 
 unsigned long *r2)
 unsigned long out[8];
 unsigned long r;
 
 -   r = kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
 +   r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
 
 Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have 
 epapr_hcalls.S compiled into the code base then and the bl above would 
 reference an unknown function.
 
 KVM_GUEST selects EPAPR_PARAVIRT.

But you can not select KVM_GUEST and still call these inline functions, no? 
Like kvm_arch_para_features().


Alex

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Scott Wood]

On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote:
 On 02.10.2013, at 18:40, Scott Wood wrote:
 
  On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
  On 23.09.2013, at 07:23, Bharat Bhushan wrote:
  static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
  @@ -65,7 +54,7 @@ static inline long kvm_hypercall0_1(unsigned int nr, 
  unsigned long *r2)
unsigned long out[8];
unsigned long r;
  
  - r = kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
  + r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
  
  Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have 
  epapr_hcalls.S compiled into the code base then and the bl above would 
  reference an unknown function.
  
  KVM_GUEST selects EPAPR_PARAVIRT.
 
 But you can not select KVM_GUEST and still call these inline functions, no?

No.

  Like kvm_arch_para_features().

Where does that get called without KVM_GUEST?

How would that work currently, with the call to kvm_hypercall() in
arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)?

-Scott



--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Alexander Graf]

On 02.10.2013, at 19:04, Scott Wood wrote:

 On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote:
 On 02.10.2013, at 18:40, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
 On 23.09.2013, at 07:23, Bharat Bhushan wrote:
 static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
 @@ -65,7 +54,7 @@ static inline long kvm_hypercall0_1(unsigned int nr, 
 unsigned long *r2)
   unsigned long out[8];
   unsigned long r;
 
 - r = kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
 + r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
 
 Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have 
 epapr_hcalls.S compiled into the code base then and the bl above would 
 reference an unknown function.
 
 KVM_GUEST selects EPAPR_PARAVIRT.
 
 But you can not select KVM_GUEST and still call these inline functions, no?
 
 No.
 
 Like kvm_arch_para_features().
 
 Where does that get called without KVM_GUEST?
 
 How would that work currently, with the call to kvm_hypercall() in
 arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)?

It wouldn't ever get called because kvm_hypercall() ends up always returning 
EV_UNIMPLEMENTED when #ifndef CONFIG_KVM_GUEST.


Alex

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Scott Wood]

On Wed, 2013-10-02 at 19:17 +0200, Alexander Graf wrote:
 On 02.10.2013, at 19:04, Scott Wood wrote:
 
  On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote:
  On 02.10.2013, at 18:40, Scott Wood wrote:
  
  On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
  Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have 
  epapr_hcalls.S compiled into the code base then and the bl above would 
  reference an unknown function.
  
  KVM_GUEST selects EPAPR_PARAVIRT.
  
  But you can not select KVM_GUEST and still call these inline functions, no?
  
  No.
  
  Like kvm_arch_para_features().
  
  Where does that get called without KVM_GUEST?
  
  How would that work currently, with the call to kvm_hypercall() in
  arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)?
 
 It wouldn't ever get called because kvm_hypercall() ends up always returning 
 EV_UNIMPLEMENTED when #ifndef CONFIG_KVM_GUEST.

OK, so the objection is to removing that stub?  Where would we actually
want to call this without knowing that KVM_GUEST or EPAPR_PARAVIRT are
enabled?

-SCott



--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Alexander Graf]

On 02.10.2013, at 19:42, Scott Wood wrote:

 On Wed, 2013-10-02 at 19:17 +0200, Alexander Graf wrote:
 On 02.10.2013, at 19:04, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote:
 On 02.10.2013, at 18:40, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
 Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have 
 epapr_hcalls.S compiled into the code base then and the bl above would 
 reference an unknown function.
 
 KVM_GUEST selects EPAPR_PARAVIRT.
 
 But you can not select KVM_GUEST and still call these inline functions, no?
 
 No.
 
 Like kvm_arch_para_features().
 
 Where does that get called without KVM_GUEST?
 
 How would that work currently, with the call to kvm_hypercall() in
 arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)?
 
 It wouldn't ever get called because kvm_hypercall() ends up always returning 
 EV_UNIMPLEMENTED when #ifndef CONFIG_KVM_GUEST.
 
 OK, so the objection is to removing that stub?  Where would we actually
 want to call this without knowing that KVM_GUEST or EPAPR_PARAVIRT are
 enabled?

In probing code. I usually prefer

if (kvm_feature_available(X)) {
   ...
}

over

#ifdef CONFIG_KVM_GUEST
if (kvm_feature_available(X)) {
   ...
}
#endif

at least when I can avoid it. With the current code the compiler would be smart 
enough to just optimize out the complete branch.


Alex

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Scott Wood]

On Wed, 2013-10-02 at 19:46 +0200, Alexander Graf wrote:
 On 02.10.2013, at 19:42, Scott Wood wrote:
 
  On Wed, 2013-10-02 at 19:17 +0200, Alexander Graf wrote:
  On 02.10.2013, at 19:04, Scott Wood wrote:
  
  On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote:
  On 02.10.2013, at 18:40, Scott Wood wrote:
  
  On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
  Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have 
  epapr_hcalls.S compiled into the code base then and the bl above would 
  reference an unknown function.
  
  KVM_GUEST selects EPAPR_PARAVIRT.
  
  But you can not select KVM_GUEST and still call these inline functions, 
  no?
  
  No.
  
  Like kvm_arch_para_features().
  
  Where does that get called without KVM_GUEST?
  
  How would that work currently, with the call to kvm_hypercall() in
  arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)?
  
  It wouldn't ever get called because kvm_hypercall() ends up always 
  returning EV_UNIMPLEMENTED when #ifndef CONFIG_KVM_GUEST.
  
  OK, so the objection is to removing that stub?  Where would we actually
  want to call this without knowing that KVM_GUEST or EPAPR_PARAVIRT are
  enabled?
 
 In probing code. I usually prefer
 
 if (kvm_feature_available(X)) {
...
 }
 
 over
 
 #ifdef CONFIG_KVM_GUEST
 if (kvm_feature_available(X)) {
...
 }
 #endif
 
 at least when I can avoid it. With the current code the compiler would be 
 smart enough to just optimize out the complete branch.

Sure.  My point is, where would you be calling that where the entire
file isn't predicated on (or selecting) CONFIG_KVM_GUEST or similar?

We don't do these stubs for every single function in the kernel -- only
ones where the above is a reasonable use case.

-Scott



--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Alexander Graf]

On 02.10.2013, at 19:49, Scott Wood wrote:

 On Wed, 2013-10-02 at 19:46 +0200, Alexander Graf wrote:
 On 02.10.2013, at 19:42, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 19:17 +0200, Alexander Graf wrote:
 On 02.10.2013, at 19:04, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote:
 On 02.10.2013, at 18:40, Scott Wood wrote:
 
 On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
 Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have 
 epapr_hcalls.S compiled into the code base then and the bl above would 
 reference an unknown function.
 
 KVM_GUEST selects EPAPR_PARAVIRT.
 
 But you can not select KVM_GUEST and still call these inline functions, 
 no?
 
 No.
 
 Like kvm_arch_para_features().
 
 Where does that get called without KVM_GUEST?
 
 How would that work currently, with the call to kvm_hypercall() in
 arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)?
 
 It wouldn't ever get called because kvm_hypercall() ends up always 
 returning EV_UNIMPLEMENTED when #ifndef CONFIG_KVM_GUEST.
 
 OK, so the objection is to removing that stub?  Where would we actually
 want to call this without knowing that KVM_GUEST or EPAPR_PARAVIRT are
 enabled?
 
 In probing code. I usually prefer
 
 if (kvm_feature_available(X)) {
   ...
 }
 
 over
 
 #ifdef CONFIG_KVM_GUEST
 if (kvm_feature_available(X)) {
   ...
 }
 #endif
 
 at least when I can avoid it. With the current code the compiler would be 
 smart enough to just optimize out the complete branch.
 
 Sure.  My point is, where would you be calling that where the entire
 file isn't predicated on (or selecting) CONFIG_KVM_GUEST or similar?
 
 We don't do these stubs for every single function in the kernel -- only
 ones where the above is a reasonable use case.

Yeah, I'm fine on dropping it, but we need to make that a conscious decision 
and verify that no caller relies on it.


Alex

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Scott Wood]

On Wed, 2013-10-02 at 19:54 +0200, Alexander Graf wrote:
 On 02.10.2013, at 19:49, Scott Wood wrote:
 
  On Wed, 2013-10-02 at 19:46 +0200, Alexander Graf wrote:
  On 02.10.2013, at 19:42, Scott Wood wrote:
  
  On Wed, 2013-10-02 at 19:17 +0200, Alexander Graf wrote:
  On 02.10.2013, at 19:04, Scott Wood wrote:
  
  On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote:
  On 02.10.2013, at 18:40, Scott Wood wrote:
  
  On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
  Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have 
  epapr_hcalls.S compiled into the code base then and the bl above 
  would reference an unknown function.
  
  KVM_GUEST selects EPAPR_PARAVIRT.
  
  But you can not select KVM_GUEST and still call these inline 
  functions, no?
  
  No.
  
  Like kvm_arch_para_features().
  
  Where does that get called without KVM_GUEST?
  
  How would that work currently, with the call to kvm_hypercall() in
  arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)?
  
  It wouldn't ever get called because kvm_hypercall() ends up always 
  returning EV_UNIMPLEMENTED when #ifndef CONFIG_KVM_GUEST.
  
  OK, so the objection is to removing that stub?  Where would we actually
  want to call this without knowing that KVM_GUEST or EPAPR_PARAVIRT are
  enabled?
  
  In probing code. I usually prefer
  
  if (kvm_feature_available(X)) {
...
  }
  
  over
  
  #ifdef CONFIG_KVM_GUEST
  if (kvm_feature_available(X)) {
...
  }
  #endif
  
  at least when I can avoid it. With the current code the compiler would be 
  smart enough to just optimize out the complete branch.
  
  Sure.  My point is, where would you be calling that where the entire
  file isn't predicated on (or selecting) CONFIG_KVM_GUEST or similar?
  
  We don't do these stubs for every single function in the kernel -- only
  ones where the above is a reasonable use case.
 
 Yeah, I'm fine on dropping it, but we need to make that a conscious decision 
 and verify that no caller relies on it.

kvm_para_has_feature() is called from arch/powerpc/kernel/kvm.c,
arch/x86/kernel/kvm.c, and arch/x86/kernel/kvmclock.c, all of which are
enabled by CONFIG_KVM_GUEST.

I did find one example of kvm_para_available() being used in an
unexpected place -- sound/pci/intel8x0.c.  It defines its own
non-CONFIG_KVM_GUEST stub, even though x86 defines kvm_para_available()
using inline CPUID stuff which should work without CONFIG_KVM_GUEST.
I'm not sure why it even needs to do that, though -- shouldn't the
subsequent PCI subsystem vendor/device check should be sufficient?  No
hypercalls are involved.

That said, the possibility that some random driver might want to make
use of paravirt features is a decent argument for keeping the stub.

-Scott



--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Scott Wood]

On Mon, 2013-09-23 at 10:53 +0530, Bharat Bhushan wrote:
 kvm_hypercall() have nothing KVM specific, so renamed to epapr_hypercall().
 Also this in moved to arch/powerpc/include/asm/epapr_hcalls.h
[snip]
 + out[0] = r4;
 + out[1] = r5;
 + out[2] = r6;
 + out[3] = r7;
 + out[4] = r8;
 + out[5] = r9;
 + out[6] = r10;
 + out[7] = r11;
 +
 + return r3;
 +}
  #endif /* !__ASSEMBLY__ */
  #endif /* _EPAPR_HCALLS_H */
[snip]
 - out[0] = r4;
 - out[1] = r5;
 - out[2] = r6;
 - out[3] = r7;
 - out[4] = r8;
 - out[5] = r9;
 - out[6] = r10;
 - out[7] = r11;
 -
 - return r3;
 -}
 -EXPORT_SYMBOL_GPL(kvm_hypercall);

Where did the export go?

I wish git could detect copies/moves of chunks of code between existing
files rather than just when a new file is created...

-Scott



--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Scott Wood]

On Mon, 2013-09-23 at 17:45 -0500, Scott Wood wrote:
 On Mon, 2013-09-23 at 10:53 +0530, Bharat Bhushan wrote:
  kvm_hypercall() have nothing KVM specific, so renamed to epapr_hypercall().
  Also this in moved to arch/powerpc/include/asm/epapr_hcalls.h
 [snip]
  +   out[0] = r4;
  +   out[1] = r5;
  +   out[2] = r6;
  +   out[3] = r7;
  +   out[4] = r8;
  +   out[5] = r9;
  +   out[6] = r10;
  +   out[7] = r11;
  +
  +   return r3;
  +}
   #endif /* !__ASSEMBLY__ */
   #endif /* _EPAPR_HCALLS_H */
 [snip]
  -   out[0] = r4;
  -   out[1] = r5;
  -   out[2] = r6;
  -   out[3] = r7;
  -   out[4] = r8;
  -   out[5] = r9;
  -   out[6] = r10;
  -   out[7] = r11;
  -
  -   return r3;
  -}
  -EXPORT_SYMBOL_GPL(kvm_hypercall);
 
 Where did the export go?

Never mind -- it's been converted to inline.

-Scott



--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html