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

login
register
mail settings
Submitter Bharat Bhushan
Date July 15, 2013, 11:11 a.m.
Message ID <1373886679-19581-6-git-send-email-Bharat.Bhushan@freescale.com>
Download mbox | patch
Permalink /patch/259007/
State New
Headers show

Comments

Bharat Bhushan - July 15, 2013, 11:11 a.m.
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

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/kernel/epapr_paravirt.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
Alexander Graf - July 15, 2013, 11:50 a.m.
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.

> +	printk("error: system reset returned with error %ld\n", ret);

So we should fall back to the normal reset handler here.


Alex

> +	BUG();
> +}
> +
> static int __init epapr_paravirt_init(void)
> {
> 	struct device_node *hyper_node;
> @@ -58,6 +68,8 @@ static int __init epapr_paravirt_init(void)
> 	if (of_get_property(hyper_node, "has-idle", NULL))
> 		ppc_md.power_save = epapr_ev_idle;
> #endif
> +	if (of_get_property(hyper_node, "kvm,has-reset", NULL))
> +		ppc_md.restart = epapr_hypercall_reset;
> 
> 	epapr_paravirt_enabled = true;
> 
> -- 
> 1.7.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bharat Bhushan - July 15, 2013, 3:05 p.m.
> -----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. 


> 
> > +	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? 

-Bharat

> 
> 
> Alex
> 
> > +	BUG();
> > +}
> > +
> > static int __init epapr_paravirt_init(void) {
> > 	struct device_node *hyper_node;
> > @@ -58,6 +68,8 @@ static int __init epapr_paravirt_init(void)
> > 	if (of_get_property(hyper_node, "has-idle", NULL))
> > 		ppc_md.power_save = epapr_ev_idle;
> > #endif
> > +	if (of_get_property(hyper_node, "kvm,has-reset", NULL))
> > +		ppc_md.restart = epapr_hypercall_reset;
> >
> > 	epapr_paravirt_enabled = true;
> >
> > --
> > 1.7.0.4
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> > the body of a message to majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - July 15, 2013, 3:09 p.m.
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.

> 
> 
>> 
>>> +	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.


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bharat Bhushan - July 15, 2013, 3:16 p.m.
> -----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.
> 
> >
> >
> >>
> >>> +	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 

-Bharat


--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - July 15, 2013, 6:21 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - July 15, 2013, 8:28 p.m.
On 15.07.2013, at 20:21, Scott Wood wrote:

> 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.

Ah, figured that part was obvious :).

> 
>> > >>> +	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.

If we also check for kvm_para_available() I agree. Otherwise QEMU might advertise the reset hcall, but the guest kernel may not implement KVM hypercalls. In that case the device tree check will succeed, but the actual hypercall will not.


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - July 15, 2013, 8:52 p.m.
On 07/15/2013 03:28:46 PM, Alexander Graf wrote:
> 
> On 15.07.2013, at 20:21, Scott Wood wrote:
> 
> > On 07/15/2013 10:16:41 AM, Bhushan Bharat-R65777 wrote:
> >> > >>> +	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.
> 
> If we also check for kvm_para_available() I agree. Otherwise QEMU  
> might advertise the reset hcall, but the guest kernel may not  
> implement KVM hypercalls. In that case the device tree check will  
> succeed, but the actual hypercall will not.

Wouldn't that be a bug in QEMU?  Or in KVM for exposing the hcall  
capability without implementing them?

Not that I have anything against checking kvm_para_available()...   
though that (or its functional equivalent that returns a pointer to the  
node) should really be a prerequisite for even being able to interpret  
KVM-specific properties in the hypervisor node.

-Scott
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - July 15, 2013, 8:55 p.m.
On 15.07.2013, at 22:52, Scott Wood wrote:

> On 07/15/2013 03:28:46 PM, Alexander Graf wrote:
>> On 15.07.2013, at 20:21, Scott Wood wrote:
>> > On 07/15/2013 10:16:41 AM, Bhushan Bharat-R65777 wrote:
>> >> > >>> +	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.
>> If we also check for kvm_para_available() I agree. Otherwise QEMU might advertise the reset hcall, but the guest kernel may not implement KVM hypercalls. In that case the device tree check will succeed, but the actual hypercall will not.
> 
> Wouldn't that be a bug in QEMU?  Or in KVM for exposing the hcall capability without implementing them?

No, because it would be the guest that doesn't know how to handle kvm hypercalls.

> Not that I have anything against checking kvm_para_available()...  though that (or its functional equivalent that returns a pointer to the node) should really be a prerequisite for even being able to interpret KVM-specific properties in the hypervisor node.

That's my point :).


Alex

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

Patch

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>
 
 #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);
+	printk("error: system reset returned with error %ld\n", ret);
+	BUG();
+}
+
 static int __init epapr_paravirt_init(void)
 {
 	struct device_node *hyper_node;
@@ -58,6 +68,8 @@  static int __init epapr_paravirt_init(void)
 	if (of_get_property(hyper_node, "has-idle", NULL))
 		ppc_md.power_save = epapr_ev_idle;
 #endif
+	if (of_get_property(hyper_node, "kvm,has-reset", NULL))
+		ppc_md.restart = epapr_hypercall_reset;
 
 	epapr_paravirt_enabled = true;