Re: [PATCH 5/5] powerpc: using reset hcall when kvm,has-reset

[Scott Wood <scottwood@freescale.com>]

On 07/15/2013 10:16:41 AM, Bhushan Bharat-R65777 wrote:
> 
> 
> > -----Original Message-----
> > From: Alexander Graf [mailto:agraf@suse.de]
> > Sent: Monday, July 15, 2013 8:40 PM
> > To: Bhushan Bharat-R65777
> > Cc: kvm@vger.kernel.org; kvm-ppc@vger.kernel.org; Wood  
> Scott-B07421; Yoder
> > Stuart-B08248
> > Subject: Re: [PATCH 5/5] powerpc: using reset hcall when  
> kvm,has-reset
> >
> >
> > On 15.07.2013, at 17:05, Bhushan Bharat-R65777 wrote:
> >
> > >
> > >
> > >> -----Original Message-----
> > >> From: Alexander Graf [mailto:agraf@suse.de]
> > >> Sent: Monday, July 15, 2013 5:20 PM
> > >> To: Bhushan Bharat-R65777
> > >> Cc: kvm@vger.kernel.org; kvm-ppc@vger.kernel.org; Wood  
> Scott-B07421;
> > >> Yoder Stuart-B08248; Bhushan Bharat-R65777
> > >> Subject: Re: [PATCH 5/5] powerpc: using reset hcall when
> > >> kvm,has-reset
> > >>
> > >>
> > >> On 15.07.2013, at 13:11, Bharat Bhushan wrote:
> > >>
> > >>> Detect the availability of the reset hcalls by looking at
> > >>> kvm,has-reset property on the /hypervisor node in the device  
> tree
> > >>> passed to the VM and patches the reset mechanism to use reset  
> hcall.
> > >>>
> > >>> This patch uses the reser hcall when kvm,has-reset is there in
> > >>
> > >> Your patch description is pretty broken :).
> > >>
> > >>>
> > >>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > >>> ---
> > >>> arch/powerpc/kernel/epapr_paravirt.c |   12 ++++++++++++
> > >>> 1 files changed, 12 insertions(+), 0 deletions(-)
> > >>>
> > >>> diff --git a/arch/powerpc/kernel/epapr_paravirt.c
> > >>> b/arch/powerpc/kernel/epapr_paravirt.c
> > >>> index d44a571..651d701 100644
> > >>> --- a/arch/powerpc/kernel/epapr_paravirt.c
> > >>> +++ b/arch/powerpc/kernel/epapr_paravirt.c
> > >>> @@ -22,6 +22,8 @@
> > >>> #include <asm/cacheflush.h>
> > >>> #include <asm/code-patching.h>
> > >>> #include <asm/machdep.h>
> > >>> +#include <asm/kvm_para.h>
> > >>> +#include <asm/kvm_host.h>
> > >>
> > >> Why would we need kvm_host.h? This is guest code.
> > >>
> > >>>
> > >>> #if !defined(CONFIG_64BIT) || defined(CONFIG_PPC_BOOK3E_64)  
> extern
> > >>> void epapr_ev_idle(void); @@ -30,6 +32,14 @@ extern u32
> > >>> epapr_ev_idle_start[];
> > >>>
> > >>> bool epapr_paravirt_enabled;
> > >>>
> > >>> +void epapr_hypercall_reset(char *cmd) {
> > >>> +	long ret;
> > >>> +	ret = kvm_hypercall0(KVM_HC_VM_RESET);
> > >>
> > >> Is this available without CONFIG_KVM_GUEST? kvm_hypercall()  
> simply
> > >> returns "unimplemented" for everything when that config option  
> is not set.
> > >
> > > We are here because we patched the ppc_md.restart to point to new  
> handler.
> > > So I think we should patch the ppc_md.restart only if  
> CONFIG_KVM_GUEST is
> > true.
> >
> > We should only patch it if kvm_para_available(). That should guard  
> us against
> > everything.

It also should depend on whether the reset hcall is advertised in the  
device tree.

> > >>> +	printk("error: system reset returned with error %ld\n",  
> ret);
> > >>
> > >> So we should fall back to the normal reset handler here.
> > >
> > > Do you mean return normally from here, no BUG() etc?
> >
> > If we guard the patching against everything, we can treat a broken  
> hcall as BUG.
> > However, if we don't we want to fall back to the normal guts based  
> reset.
> 
> Will let Scott comment on this?
> 
> But ppc_md.restart can point to only one handler and during paravirt  
> patching we changed this to new handler. So we cannot jump back to  
> guts type handler

I don't think it's worth implementing a fall-back scheme -- if KVM  
advertises that the reset hcall exists, then it had better exist.

BTW, this is not part of ePAPR so it should not be in  
epapr_paravirt.c.  It should go in kvm.c.

-Scott