* [Qemu-devel] [PATCH 1/6] xics/spapr: Drop unused function declaration
2019-06-17 13:46 [Qemu-devel] [PATCH 0/6] xics/kvm: Improve error handling Greg Kurz
@ 2019-06-17 13:46 ` Greg Kurz
2019-06-17 14:18 ` Cédric Le Goater
2019-06-20 8:00 ` David Gibson
2019-06-17 13:46 ` [Qemu-devel] [PATCH 2/6] xics/spapr: Rename xics_kvm_init() Greg Kurz
` (4 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Greg Kurz @ 2019-06-17 13:46 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
Commit 9fb6eb7ca50c added the declaration of xics_spapr_connect(), which
has no implementation and no users.
This is a leftover from a previous iteration of this patch. Drop it.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
include/hw/ppc/xics_spapr.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
index d968f2499ca7..330448126223 100644
--- a/include/hw/ppc/xics_spapr.h
+++ b/include/hw/ppc/xics_spapr.h
@@ -37,6 +37,5 @@ int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
void xics_spapr_init(SpaprMachineState *spapr);
-void xics_spapr_connect(SpaprMachineState *spapr);
#endif /* XICS_SPAPR_H */
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] xics/spapr: Drop unused function declaration
2019-06-17 13:46 ` [Qemu-devel] [PATCH 1/6] xics/spapr: Drop unused function declaration Greg Kurz
@ 2019-06-17 14:18 ` Cédric Le Goater
2019-06-20 8:00 ` David Gibson
1 sibling, 0 replies; 19+ messages in thread
From: Cédric Le Goater @ 2019-06-17 14:18 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
On 17/06/2019 15:46, Greg Kurz wrote:
> Commit 9fb6eb7ca50c added the declaration of xics_spapr_connect(), which
> has no implementation and no users.
>
> This is a leftover from a previous iteration of this patch. Drop it.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> include/hw/ppc/xics_spapr.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> index d968f2499ca7..330448126223 100644
> --- a/include/hw/ppc/xics_spapr.h
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -37,6 +37,5 @@ int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
> void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
> bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
> void xics_spapr_init(SpaprMachineState *spapr);
> -void xics_spapr_connect(SpaprMachineState *spapr);
>
> #endif /* XICS_SPAPR_H */
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] xics/spapr: Drop unused function declaration
2019-06-17 13:46 ` [Qemu-devel] [PATCH 1/6] xics/spapr: Drop unused function declaration Greg Kurz
2019-06-17 14:18 ` Cédric Le Goater
@ 2019-06-20 8:00 ` David Gibson
1 sibling, 0 replies; 19+ messages in thread
From: David Gibson @ 2019-06-20 8:00 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1158 bytes --]
On Mon, Jun 17, 2019 at 03:46:35PM +0200, Greg Kurz wrote:
> Commit 9fb6eb7ca50c added the declaration of xics_spapr_connect(), which
> has no implementation and no users.
>
> This is a leftover from a previous iteration of this patch. Drop it.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied, thanks.
> ---
> include/hw/ppc/xics_spapr.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> index d968f2499ca7..330448126223 100644
> --- a/include/hw/ppc/xics_spapr.h
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -37,6 +37,5 @@ int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
> void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
> bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
> void xics_spapr_init(SpaprMachineState *spapr);
> -void xics_spapr_connect(SpaprMachineState *spapr);
>
> #endif /* XICS_SPAPR_H */
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 2/6] xics/spapr: Rename xics_kvm_init()
2019-06-17 13:46 [Qemu-devel] [PATCH 0/6] xics/kvm: Improve error handling Greg Kurz
2019-06-17 13:46 ` [Qemu-devel] [PATCH 1/6] xics/spapr: Drop unused function declaration Greg Kurz
@ 2019-06-17 13:46 ` Greg Kurz
2019-06-17 14:18 ` Cédric Le Goater
2019-06-20 8:02 ` David Gibson
2019-06-17 13:46 ` [Qemu-devel] [PATCH 3/6] xics/kvm: Skip rollback when KVM XICS is absent Greg Kurz
` (3 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Greg Kurz @ 2019-06-17 13:46 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
Switch to using the connect/disconnect terminology like we already do for
XIVE.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/intc/xics_kvm.c | 2 +-
hw/ppc/spapr_irq.c | 2 +-
include/hw/ppc/xics_spapr.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index c7f8f5edd257..534515143ea8 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -331,7 +331,7 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
}
}
-int xics_kvm_init(SpaprMachineState *spapr, Error **errp)
+int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
{
int rc;
CPUState *cs;
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 84b9b32fe40f..ff3df0bbd8cf 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -237,7 +237,7 @@ static const char *spapr_irq_get_nodename_xics(SpaprMachineState *spapr)
static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
{
if (kvm_enabled()) {
- xics_kvm_init(spapr, errp);
+ xics_kvm_connect(spapr, errp);
}
}
diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
index 330448126223..5dabc9a1388f 100644
--- a/include/hw/ppc/xics_spapr.h
+++ b/include/hw/ppc/xics_spapr.h
@@ -33,7 +33,7 @@
void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt,
uint32_t phandle);
-int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
+int xics_kvm_connect(SpaprMachineState *spapr, Error **errp);
void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
void xics_spapr_init(SpaprMachineState *spapr);
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] xics/spapr: Rename xics_kvm_init()
2019-06-17 13:46 ` [Qemu-devel] [PATCH 2/6] xics/spapr: Rename xics_kvm_init() Greg Kurz
@ 2019-06-17 14:18 ` Cédric Le Goater
2019-06-20 8:02 ` David Gibson
1 sibling, 0 replies; 19+ messages in thread
From: Cédric Le Goater @ 2019-06-17 14:18 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
On 17/06/2019 15:46, Greg Kurz wrote:
> Switch to using the connect/disconnect terminology like we already do for
> XIVE.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> hw/intc/xics_kvm.c | 2 +-
> hw/ppc/spapr_irq.c | 2 +-
> include/hw/ppc/xics_spapr.h | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index c7f8f5edd257..534515143ea8 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -331,7 +331,7 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
> }
> }
>
> -int xics_kvm_init(SpaprMachineState *spapr, Error **errp)
> +int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
> {
> int rc;
> CPUState *cs;
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 84b9b32fe40f..ff3df0bbd8cf 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -237,7 +237,7 @@ static const char *spapr_irq_get_nodename_xics(SpaprMachineState *spapr)
> static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
> {
> if (kvm_enabled()) {
> - xics_kvm_init(spapr, errp);
> + xics_kvm_connect(spapr, errp);
> }
> }
>
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> index 330448126223..5dabc9a1388f 100644
> --- a/include/hw/ppc/xics_spapr.h
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -33,7 +33,7 @@
>
> void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt,
> uint32_t phandle);
> -int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
> +int xics_kvm_connect(SpaprMachineState *spapr, Error **errp);
> void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
> bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
> void xics_spapr_init(SpaprMachineState *spapr);
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] xics/spapr: Rename xics_kvm_init()
2019-06-17 13:46 ` [Qemu-devel] [PATCH 2/6] xics/spapr: Rename xics_kvm_init() Greg Kurz
2019-06-17 14:18 ` Cédric Le Goater
@ 2019-06-20 8:02 ` David Gibson
1 sibling, 0 replies; 19+ messages in thread
From: David Gibson @ 2019-06-20 8:02 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2161 bytes --]
On Mon, Jun 17, 2019 at 03:46:41PM +0200, Greg Kurz wrote:
> Switch to using the connect/disconnect terminology like we already do for
> XIVE.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied, thanks.
> ---
> hw/intc/xics_kvm.c | 2 +-
> hw/ppc/spapr_irq.c | 2 +-
> include/hw/ppc/xics_spapr.h | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index c7f8f5edd257..534515143ea8 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -331,7 +331,7 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
> }
> }
>
> -int xics_kvm_init(SpaprMachineState *spapr, Error **errp)
> +int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
> {
> int rc;
> CPUState *cs;
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 84b9b32fe40f..ff3df0bbd8cf 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -237,7 +237,7 @@ static const char *spapr_irq_get_nodename_xics(SpaprMachineState *spapr)
> static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
> {
> if (kvm_enabled()) {
> - xics_kvm_init(spapr, errp);
> + xics_kvm_connect(spapr, errp);
> }
> }
>
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> index 330448126223..5dabc9a1388f 100644
> --- a/include/hw/ppc/xics_spapr.h
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -33,7 +33,7 @@
>
> void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt,
> uint32_t phandle);
> -int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
> +int xics_kvm_connect(SpaprMachineState *spapr, Error **errp);
> void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
> bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
> void xics_spapr_init(SpaprMachineState *spapr);
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 3/6] xics/kvm: Skip rollback when KVM XICS is absent
2019-06-17 13:46 [Qemu-devel] [PATCH 0/6] xics/kvm: Improve error handling Greg Kurz
2019-06-17 13:46 ` [Qemu-devel] [PATCH 1/6] xics/spapr: Drop unused function declaration Greg Kurz
2019-06-17 13:46 ` [Qemu-devel] [PATCH 2/6] xics/spapr: Rename xics_kvm_init() Greg Kurz
@ 2019-06-17 13:46 ` Greg Kurz
2019-06-17 14:18 ` Cédric Le Goater
2019-06-20 8:04 ` David Gibson
2019-06-17 13:46 ` [Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init() Greg Kurz
` (2 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Greg Kurz @ 2019-06-17 13:46 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
There is no need to rollback anything at this point, so just return an
error.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/intc/xics_kvm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 534515143ea8..377ff88701c2 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -348,7 +348,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
error_setg(errp,
"KVM and IRQ_XICS capability must be present for in-kernel XICS");
- goto fail;
+ return -1;
}
rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] xics/kvm: Skip rollback when KVM XICS is absent
2019-06-17 13:46 ` [Qemu-devel] [PATCH 3/6] xics/kvm: Skip rollback when KVM XICS is absent Greg Kurz
@ 2019-06-17 14:18 ` Cédric Le Goater
2019-06-20 8:04 ` David Gibson
1 sibling, 0 replies; 19+ messages in thread
From: Cédric Le Goater @ 2019-06-17 14:18 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
On 17/06/2019 15:46, Greg Kurz wrote:
> There is no need to rollback anything at this point, so just return an
> error.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> hw/intc/xics_kvm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 534515143ea8..377ff88701c2 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -348,7 +348,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
> if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
> error_setg(errp,
> "KVM and IRQ_XICS capability must be present for in-kernel XICS");
> - goto fail;
> + return -1;
> }
>
> rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] xics/kvm: Skip rollback when KVM XICS is absent
2019-06-17 13:46 ` [Qemu-devel] [PATCH 3/6] xics/kvm: Skip rollback when KVM XICS is absent Greg Kurz
2019-06-17 14:18 ` Cédric Le Goater
@ 2019-06-20 8:04 ` David Gibson
1 sibling, 0 replies; 19+ messages in thread
From: David Gibson @ 2019-06-20 8:04 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]
On Mon, Jun 17, 2019 at 03:46:46PM +0200, Greg Kurz wrote:
> There is no need to rollback anything at this point, so just return an
> error.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied, thanks.
> ---
> hw/intc/xics_kvm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 534515143ea8..377ff88701c2 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -348,7 +348,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
> if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
> error_setg(errp,
> "KVM and IRQ_XICS capability must be present for in-kernel XICS");
> - goto fail;
> + return -1;
> }
>
> rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init()
2019-06-17 13:46 [Qemu-devel] [PATCH 0/6] xics/kvm: Improve error handling Greg Kurz
` (2 preceding siblings ...)
2019-06-17 13:46 ` [Qemu-devel] [PATCH 3/6] xics/kvm: Skip rollback when KVM XICS is absent Greg Kurz
@ 2019-06-17 13:46 ` Greg Kurz
2019-06-17 14:19 ` Cédric Le Goater
2019-06-20 8:05 ` David Gibson
2019-06-17 13:46 ` [Qemu-devel] [PATCH 5/6] xics/kvm: Add error propagation to ic*_set_kvm_state() functions Greg Kurz
2019-06-17 13:47 ` [Qemu-devel] [PATCH 6/6] xics/kvm: Add proper rollback to xics_kvm_init() Greg Kurz
5 siblings, 2 replies; 19+ messages in thread
From: Greg Kurz @ 2019-06-17 13:46 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
Passing both errp and &local_err to functions is a recipe for messing
things up.
Since we must use &local_err for icp_kvm_realize(), use &local_err
everywhere where rollback must happen and have a single call to
error_propagate() them all. While here, add errno to the error
message.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/intc/xics_kvm.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 377ff88701c2..c9e25fb051bb 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -353,32 +353,36 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
if (rc < 0) {
- error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,set-xive");
+ error_setg_errno(&local_err, -rc,
+ "kvmppc_define_rtas_kernel_token: ibm,set-xive");
goto fail;
}
rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_GET_XIVE, "ibm,get-xive");
if (rc < 0) {
- error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,get-xive");
+ error_setg_errno(&local_err, -rc,
+ "kvmppc_define_rtas_kernel_token: ibm,get-xive");
goto fail;
}
rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_ON, "ibm,int-on");
if (rc < 0) {
- error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-on");
+ error_setg_errno(&local_err, -rc,
+ "kvmppc_define_rtas_kernel_token: ibm,int-on");
goto fail;
}
rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_OFF, "ibm,int-off");
if (rc < 0) {
- error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-off");
+ error_setg_errno(&local_err, -rc,
+ "kvmppc_define_rtas_kernel_token: ibm,int-off");
goto fail;
}
/* Create the KVM XICS device */
rc = kvm_create_device(kvm_state, KVM_DEV_TYPE_XICS, false);
if (rc < 0) {
- error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
+ error_setg_errno(&local_err, -rc, "Error on KVM_CREATE_DEVICE for XICS");
goto fail;
}
@@ -393,7 +397,6 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
icp_kvm_realize(DEVICE(spapr_cpu_state(cpu)->icp), &local_err);
if (local_err) {
- error_propagate(errp, local_err);
goto fail;
}
}
@@ -410,6 +413,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
return 0;
fail:
+ error_propagate(errp, local_err);
kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init()
2019-06-17 13:46 ` [Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init() Greg Kurz
@ 2019-06-17 14:19 ` Cédric Le Goater
2019-06-20 8:05 ` David Gibson
1 sibling, 0 replies; 19+ messages in thread
From: Cédric Le Goater @ 2019-06-17 14:19 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
On 17/06/2019 15:46, Greg Kurz wrote:
> Passing both errp and &local_err to functions is a recipe for messing
> things up.
>
> Since we must use &local_err for icp_kvm_realize(), use &local_err
> everywhere where rollback must happen and have a single call to
> error_propagate() them all. While here, add errno to the error
> message.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> hw/intc/xics_kvm.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 377ff88701c2..c9e25fb051bb 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -353,32 +353,36 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>
> rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
> if (rc < 0) {
> - error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,set-xive");
> + error_setg_errno(&local_err, -rc,
> + "kvmppc_define_rtas_kernel_token: ibm,set-xive");
> goto fail;
> }
>
> rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_GET_XIVE, "ibm,get-xive");
> if (rc < 0) {
> - error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,get-xive");
> + error_setg_errno(&local_err, -rc,
> + "kvmppc_define_rtas_kernel_token: ibm,get-xive");
> goto fail;
> }
>
> rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_ON, "ibm,int-on");
> if (rc < 0) {
> - error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-on");
> + error_setg_errno(&local_err, -rc,
> + "kvmppc_define_rtas_kernel_token: ibm,int-on");
> goto fail;
> }
>
> rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_OFF, "ibm,int-off");
> if (rc < 0) {
> - error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-off");
> + error_setg_errno(&local_err, -rc,
> + "kvmppc_define_rtas_kernel_token: ibm,int-off");
> goto fail;
> }
>
> /* Create the KVM XICS device */
> rc = kvm_create_device(kvm_state, KVM_DEV_TYPE_XICS, false);
> if (rc < 0) {
> - error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> + error_setg_errno(&local_err, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> goto fail;
> }
>
> @@ -393,7 +397,6 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>
> icp_kvm_realize(DEVICE(spapr_cpu_state(cpu)->icp), &local_err);
> if (local_err) {
> - error_propagate(errp, local_err);
> goto fail;
> }
> }
> @@ -410,6 +413,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
> return 0;
>
> fail:
> + error_propagate(errp, local_err);
> kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
> kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
> kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init()
2019-06-17 13:46 ` [Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init() Greg Kurz
2019-06-17 14:19 ` Cédric Le Goater
@ 2019-06-20 8:05 ` David Gibson
1 sibling, 0 replies; 19+ messages in thread
From: David Gibson @ 2019-06-20 8:05 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3330 bytes --]
On Mon, Jun 17, 2019 at 03:46:52PM +0200, Greg Kurz wrote:
> Passing both errp and &local_err to functions is a recipe for messing
> things up.
>
> Since we must use &local_err for icp_kvm_realize(), use &local_err
> everywhere where rollback must happen and have a single call to
> error_propagate() them all. While here, add errno to the error
> message.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied, thanks.
> ---
> hw/intc/xics_kvm.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 377ff88701c2..c9e25fb051bb 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -353,32 +353,36 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>
> rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
> if (rc < 0) {
> - error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,set-xive");
> + error_setg_errno(&local_err, -rc,
> + "kvmppc_define_rtas_kernel_token: ibm,set-xive");
> goto fail;
> }
>
> rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_GET_XIVE, "ibm,get-xive");
> if (rc < 0) {
> - error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,get-xive");
> + error_setg_errno(&local_err, -rc,
> + "kvmppc_define_rtas_kernel_token: ibm,get-xive");
> goto fail;
> }
>
> rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_ON, "ibm,int-on");
> if (rc < 0) {
> - error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-on");
> + error_setg_errno(&local_err, -rc,
> + "kvmppc_define_rtas_kernel_token: ibm,int-on");
> goto fail;
> }
>
> rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_OFF, "ibm,int-off");
> if (rc < 0) {
> - error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-off");
> + error_setg_errno(&local_err, -rc,
> + "kvmppc_define_rtas_kernel_token: ibm,int-off");
> goto fail;
> }
>
> /* Create the KVM XICS device */
> rc = kvm_create_device(kvm_state, KVM_DEV_TYPE_XICS, false);
> if (rc < 0) {
> - error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> + error_setg_errno(&local_err, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> goto fail;
> }
>
> @@ -393,7 +397,6 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>
> icp_kvm_realize(DEVICE(spapr_cpu_state(cpu)->icp), &local_err);
> if (local_err) {
> - error_propagate(errp, local_err);
> goto fail;
> }
> }
> @@ -410,6 +413,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
> return 0;
>
> fail:
> + error_propagate(errp, local_err);
> kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
> kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
> kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 5/6] xics/kvm: Add error propagation to ic*_set_kvm_state() functions
2019-06-17 13:46 [Qemu-devel] [PATCH 0/6] xics/kvm: Improve error handling Greg Kurz
` (3 preceding siblings ...)
2019-06-17 13:46 ` [Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init() Greg Kurz
@ 2019-06-17 13:46 ` Greg Kurz
2019-06-18 7:16 ` Cédric Le Goater
2019-06-20 8:31 ` David Gibson
2019-06-17 13:47 ` [Qemu-devel] [PATCH 6/6] xics/kvm: Add proper rollback to xics_kvm_init() Greg Kurz
5 siblings, 2 replies; 19+ messages in thread
From: Greg Kurz @ 2019-06-17 13:46 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
This allows errors happening there to be propagated up to spapr_irq,
just like XIVE already does.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/intc/xics.c | 39 ++++++++++++++++++++++++++++++++++-----
hw/intc/xics_kvm.c | 37 ++++++++++++++++++++++---------------
include/hw/ppc/xics.h | 6 +++---
3 files changed, 59 insertions(+), 23 deletions(-)
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 1dc3a0f12280..69152e3d9ff6 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -267,7 +267,14 @@ static int icp_post_load(void *opaque, int version_id)
ICPState *icp = opaque;
if (kvm_irqchip_in_kernel()) {
- return icp_set_kvm_state(icp);
+ Error *local_err = NULL;
+ int ret;
+
+ ret = icp_set_kvm_state(icp, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ return ret;
+ }
}
return 0;
@@ -300,7 +307,12 @@ static void icp_reset_handler(void *dev)
qemu_set_irq(icp->output, 0);
if (kvm_irqchip_in_kernel()) {
- icp_set_kvm_state(ICP(dev));
+ Error *local_err = NULL;
+
+ icp_set_kvm_state(ICP(dev), &local_err);
+ if (local_err) {
+ error_report_err(local_err);
+ }
}
}
@@ -555,7 +567,12 @@ static void ics_simple_reset(DeviceState *dev)
icsc->parent_reset(dev);
if (kvm_irqchip_in_kernel()) {
- ics_set_kvm_state(ICS_BASE(dev));
+ Error *local_err = NULL;
+
+ ics_set_kvm_state(ICS_BASE(dev), &local_err);
+ if (local_err) {
+ error_report_err(local_err);
+ }
}
}
@@ -671,7 +688,14 @@ static int ics_base_post_load(void *opaque, int version_id)
ICSState *ics = opaque;
if (kvm_irqchip_in_kernel()) {
- return ics_set_kvm_state(ics);
+ Error *local_err = NULL;
+ int ret;
+
+ ret = ics_set_kvm_state(ics, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ return ret;
+ }
}
return 0;
@@ -757,8 +781,13 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI;
if (kvm_irqchip_in_kernel()) {
+ Error *local_err = NULL;
+
ics_reset_irq(ics->irqs + srcno);
- ics_set_kvm_state_one(ics, srcno);
+ ics_set_kvm_state_one(ics, srcno, &local_err);
+ if (local_err) {
+ error_report_err(local_err);
+ }
}
}
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index c9e25fb051bb..4bfbe1a84092 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -106,7 +106,7 @@ void icp_synchronize_state(ICPState *icp)
}
}
-int icp_set_kvm_state(ICPState *icp)
+int icp_set_kvm_state(ICPState *icp, Error **errp)
{
uint64_t state;
int ret;
@@ -126,10 +126,11 @@ int icp_set_kvm_state(ICPState *icp)
| ((uint64_t)icp->pending_priority << KVM_REG_PPC_ICP_PPRI_SHIFT);
ret = kvm_set_one_reg(icp->cs, KVM_REG_PPC_ICP_STATE, &state);
- if (ret != 0) {
- error_report("Unable to restore KVM interrupt controller state (0x%"
- PRIx64 ") for CPU %ld: %s", state, kvm_arch_vcpu_id(icp->cs),
- strerror(errno));
+ if (ret < 0) {
+ error_setg_errno(errp, -ret,
+ "Unable to restore KVM interrupt controller state (0x%"
+ PRIx64 ") for CPU %ld", state,
+ kvm_arch_vcpu_id(icp->cs));
return ret;
}
@@ -240,10 +241,9 @@ void ics_synchronize_state(ICSState *ics)
ics_get_kvm_state(ics);
}
-int ics_set_kvm_state_one(ICSState *ics, int srcno)
+int ics_set_kvm_state_one(ICSState *ics, int srcno, Error **errp)
{
uint64_t state;
- Error *local_err = NULL;
ICSIRQState *irq = &ics->irqs[srcno];
int ret;
@@ -278,16 +278,15 @@ int ics_set_kvm_state_one(ICSState *ics, int srcno)
}
ret = kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
- srcno + ics->offset, &state, true, &local_err);
- if (local_err) {
- error_report_err(local_err);
+ srcno + ics->offset, &state, true, errp);
+ if (ret < 0) {
return ret;
}
return 0;
}
-int ics_set_kvm_state(ICSState *ics)
+int ics_set_kvm_state(ICSState *ics, Error **errp)
{
int i;
@@ -297,10 +296,12 @@ int ics_set_kvm_state(ICSState *ics)
}
for (i = 0; i < ics->nr_irqs; i++) {
+ Error *local_err = NULL;
int ret;
- ret = ics_set_kvm_state_one(ics, i);
- if (ret) {
+ ret = ics_set_kvm_state_one(ics, i, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
return ret;
}
}
@@ -402,12 +403,18 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
}
/* Update the KVM sources */
- ics_set_kvm_state(spapr->ics);
+ ics_set_kvm_state(spapr->ics, &local_err);
+ if (local_err) {
+ goto fail;
+ }
/* Connect the presenters to the initial VCPUs of the machine */
CPU_FOREACH(cs) {
PowerPCCPU *cpu = POWERPC_CPU(cs);
- icp_set_kvm_state(spapr_cpu_state(cpu)->icp);
+ icp_set_kvm_state(spapr_cpu_state(cpu)->icp, &local_err);
+ if (local_err) {
+ goto fail;
+ }
}
return 0;
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index eb65ad7e43b7..1eb7b5cd6847 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -190,13 +190,13 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
/* KVM */
void icp_get_kvm_state(ICPState *icp);
-int icp_set_kvm_state(ICPState *icp);
+int icp_set_kvm_state(ICPState *icp, Error **errp);
void icp_synchronize_state(ICPState *icp);
void icp_kvm_realize(DeviceState *dev, Error **errp);
void ics_get_kvm_state(ICSState *ics);
-int ics_set_kvm_state_one(ICSState *ics, int srcno);
-int ics_set_kvm_state(ICSState *ics);
+int ics_set_kvm_state_one(ICSState *ics, int srcno, Error **errp);
+int ics_set_kvm_state(ICSState *ics, Error **errp);
void ics_synchronize_state(ICSState *ics);
void ics_kvm_set_irq(ICSState *ics, int srcno, int val);
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] xics/kvm: Add error propagation to ic*_set_kvm_state() functions
2019-06-17 13:46 ` [Qemu-devel] [PATCH 5/6] xics/kvm: Add error propagation to ic*_set_kvm_state() functions Greg Kurz
@ 2019-06-18 7:16 ` Cédric Le Goater
2019-06-20 8:31 ` David Gibson
1 sibling, 0 replies; 19+ messages in thread
From: Cédric Le Goater @ 2019-06-18 7:16 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
On 17/06/2019 15:46, Greg Kurz wrote:
> This allows errors happening there to be propagated up to spapr_irq,
> just like XIVE already does.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> hw/intc/xics.c | 39 ++++++++++++++++++++++++++++++++++-----
> hw/intc/xics_kvm.c | 37 ++++++++++++++++++++++---------------
> include/hw/ppc/xics.h | 6 +++---
> 3 files changed, 59 insertions(+), 23 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 1dc3a0f12280..69152e3d9ff6 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -267,7 +267,14 @@ static int icp_post_load(void *opaque, int version_id)
> ICPState *icp = opaque;
>
> if (kvm_irqchip_in_kernel()) {
> - return icp_set_kvm_state(icp);
> + Error *local_err = NULL;
> + int ret;
> +
> + ret = icp_set_kvm_state(icp, &local_err);
> + if (ret < 0) {
> + error_report_err(local_err);
> + return ret;
> + }
> }
>
> return 0;
> @@ -300,7 +307,12 @@ static void icp_reset_handler(void *dev)
> qemu_set_irq(icp->output, 0);
>
> if (kvm_irqchip_in_kernel()) {
> - icp_set_kvm_state(ICP(dev));
> + Error *local_err = NULL;
> +
> + icp_set_kvm_state(ICP(dev), &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + }
> }
> }
>
> @@ -555,7 +567,12 @@ static void ics_simple_reset(DeviceState *dev)
> icsc->parent_reset(dev);
>
> if (kvm_irqchip_in_kernel()) {
> - ics_set_kvm_state(ICS_BASE(dev));
> + Error *local_err = NULL;
> +
> + ics_set_kvm_state(ICS_BASE(dev), &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + }
> }
> }
>
> @@ -671,7 +688,14 @@ static int ics_base_post_load(void *opaque, int version_id)
> ICSState *ics = opaque;
>
> if (kvm_irqchip_in_kernel()) {
> - return ics_set_kvm_state(ics);
> + Error *local_err = NULL;
> + int ret;
> +
> + ret = ics_set_kvm_state(ics, &local_err);
> + if (ret < 0) {
> + error_report_err(local_err);
> + return ret;
> + }
> }
>
> return 0;
> @@ -757,8 +781,13 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
> lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI;
>
> if (kvm_irqchip_in_kernel()) {
> + Error *local_err = NULL;
> +
> ics_reset_irq(ics->irqs + srcno);
> - ics_set_kvm_state_one(ics, srcno);
> + ics_set_kvm_state_one(ics, srcno, &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + }
> }
> }
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index c9e25fb051bb..4bfbe1a84092 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -106,7 +106,7 @@ void icp_synchronize_state(ICPState *icp)
> }
> }
>
> -int icp_set_kvm_state(ICPState *icp)
> +int icp_set_kvm_state(ICPState *icp, Error **errp)
> {
> uint64_t state;
> int ret;
> @@ -126,10 +126,11 @@ int icp_set_kvm_state(ICPState *icp)
> | ((uint64_t)icp->pending_priority << KVM_REG_PPC_ICP_PPRI_SHIFT);
>
> ret = kvm_set_one_reg(icp->cs, KVM_REG_PPC_ICP_STATE, &state);
> - if (ret != 0) {
> - error_report("Unable to restore KVM interrupt controller state (0x%"
> - PRIx64 ") for CPU %ld: %s", state, kvm_arch_vcpu_id(icp->cs),
> - strerror(errno));
> + if (ret < 0) {
> + error_setg_errno(errp, -ret,
> + "Unable to restore KVM interrupt controller state (0x%"
> + PRIx64 ") for CPU %ld", state,
> + kvm_arch_vcpu_id(icp->cs));
> return ret;
> }
>
> @@ -240,10 +241,9 @@ void ics_synchronize_state(ICSState *ics)
> ics_get_kvm_state(ics);
> }
>
> -int ics_set_kvm_state_one(ICSState *ics, int srcno)
> +int ics_set_kvm_state_one(ICSState *ics, int srcno, Error **errp)
> {
> uint64_t state;
> - Error *local_err = NULL;
> ICSIRQState *irq = &ics->irqs[srcno];
> int ret;
>
> @@ -278,16 +278,15 @@ int ics_set_kvm_state_one(ICSState *ics, int srcno)
> }
>
> ret = kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> - srcno + ics->offset, &state, true, &local_err);
> - if (local_err) {
> - error_report_err(local_err);
> + srcno + ics->offset, &state, true, errp);
> + if (ret < 0) {
> return ret;
> }
>
> return 0;
> }
>
> -int ics_set_kvm_state(ICSState *ics)
> +int ics_set_kvm_state(ICSState *ics, Error **errp)
> {
> int i;
>
> @@ -297,10 +296,12 @@ int ics_set_kvm_state(ICSState *ics)
> }
>
> for (i = 0; i < ics->nr_irqs; i++) {
> + Error *local_err = NULL;
> int ret;
>
> - ret = ics_set_kvm_state_one(ics, i);
> - if (ret) {
> + ret = ics_set_kvm_state_one(ics, i, &local_err);
> + if (ret < 0) {
> + error_propagate(errp, local_err);
> return ret;
> }
> }
> @@ -402,12 +403,18 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
> }
>
> /* Update the KVM sources */
> - ics_set_kvm_state(spapr->ics);
> + ics_set_kvm_state(spapr->ics, &local_err);
> + if (local_err) {
> + goto fail;
> + }
>
> /* Connect the presenters to the initial VCPUs of the machine */
> CPU_FOREACH(cs) {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> - icp_set_kvm_state(spapr_cpu_state(cpu)->icp);
> + icp_set_kvm_state(spapr_cpu_state(cpu)->icp, &local_err);
> + if (local_err) {
> + goto fail;
> + }
> }
>
> return 0;
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index eb65ad7e43b7..1eb7b5cd6847 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -190,13 +190,13 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
>
> /* KVM */
> void icp_get_kvm_state(ICPState *icp);
> -int icp_set_kvm_state(ICPState *icp);
> +int icp_set_kvm_state(ICPState *icp, Error **errp);
> void icp_synchronize_state(ICPState *icp);
> void icp_kvm_realize(DeviceState *dev, Error **errp);
>
> void ics_get_kvm_state(ICSState *ics);
> -int ics_set_kvm_state_one(ICSState *ics, int srcno);
> -int ics_set_kvm_state(ICSState *ics);
> +int ics_set_kvm_state_one(ICSState *ics, int srcno, Error **errp);
> +int ics_set_kvm_state(ICSState *ics, Error **errp);
> void ics_synchronize_state(ICSState *ics);
> void ics_kvm_set_irq(ICSState *ics, int srcno, int val);
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] xics/kvm: Add error propagation to ic*_set_kvm_state() functions
2019-06-17 13:46 ` [Qemu-devel] [PATCH 5/6] xics/kvm: Add error propagation to ic*_set_kvm_state() functions Greg Kurz
2019-06-18 7:16 ` Cédric Le Goater
@ 2019-06-20 8:31 ` David Gibson
1 sibling, 0 replies; 19+ messages in thread
From: David Gibson @ 2019-06-20 8:31 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 7157 bytes --]
On Mon, Jun 17, 2019 at 03:46:57PM +0200, Greg Kurz wrote:
> This allows errors happening there to be propagated up to spapr_irq,
> just like XIVE already does.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied, thanks.
> ---
> hw/intc/xics.c | 39 ++++++++++++++++++++++++++++++++++-----
> hw/intc/xics_kvm.c | 37 ++++++++++++++++++++++---------------
> include/hw/ppc/xics.h | 6 +++---
> 3 files changed, 59 insertions(+), 23 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 1dc3a0f12280..69152e3d9ff6 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -267,7 +267,14 @@ static int icp_post_load(void *opaque, int version_id)
> ICPState *icp = opaque;
>
> if (kvm_irqchip_in_kernel()) {
> - return icp_set_kvm_state(icp);
> + Error *local_err = NULL;
> + int ret;
> +
> + ret = icp_set_kvm_state(icp, &local_err);
> + if (ret < 0) {
> + error_report_err(local_err);
> + return ret;
> + }
> }
>
> return 0;
> @@ -300,7 +307,12 @@ static void icp_reset_handler(void *dev)
> qemu_set_irq(icp->output, 0);
>
> if (kvm_irqchip_in_kernel()) {
> - icp_set_kvm_state(ICP(dev));
> + Error *local_err = NULL;
> +
> + icp_set_kvm_state(ICP(dev), &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + }
> }
> }
>
> @@ -555,7 +567,12 @@ static void ics_simple_reset(DeviceState *dev)
> icsc->parent_reset(dev);
>
> if (kvm_irqchip_in_kernel()) {
> - ics_set_kvm_state(ICS_BASE(dev));
> + Error *local_err = NULL;
> +
> + ics_set_kvm_state(ICS_BASE(dev), &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + }
> }
> }
>
> @@ -671,7 +688,14 @@ static int ics_base_post_load(void *opaque, int version_id)
> ICSState *ics = opaque;
>
> if (kvm_irqchip_in_kernel()) {
> - return ics_set_kvm_state(ics);
> + Error *local_err = NULL;
> + int ret;
> +
> + ret = ics_set_kvm_state(ics, &local_err);
> + if (ret < 0) {
> + error_report_err(local_err);
> + return ret;
> + }
> }
>
> return 0;
> @@ -757,8 +781,13 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
> lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI;
>
> if (kvm_irqchip_in_kernel()) {
> + Error *local_err = NULL;
> +
> ics_reset_irq(ics->irqs + srcno);
> - ics_set_kvm_state_one(ics, srcno);
> + ics_set_kvm_state_one(ics, srcno, &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + }
> }
> }
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index c9e25fb051bb..4bfbe1a84092 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -106,7 +106,7 @@ void icp_synchronize_state(ICPState *icp)
> }
> }
>
> -int icp_set_kvm_state(ICPState *icp)
> +int icp_set_kvm_state(ICPState *icp, Error **errp)
> {
> uint64_t state;
> int ret;
> @@ -126,10 +126,11 @@ int icp_set_kvm_state(ICPState *icp)
> | ((uint64_t)icp->pending_priority << KVM_REG_PPC_ICP_PPRI_SHIFT);
>
> ret = kvm_set_one_reg(icp->cs, KVM_REG_PPC_ICP_STATE, &state);
> - if (ret != 0) {
> - error_report("Unable to restore KVM interrupt controller state (0x%"
> - PRIx64 ") for CPU %ld: %s", state, kvm_arch_vcpu_id(icp->cs),
> - strerror(errno));
> + if (ret < 0) {
> + error_setg_errno(errp, -ret,
> + "Unable to restore KVM interrupt controller state (0x%"
> + PRIx64 ") for CPU %ld", state,
> + kvm_arch_vcpu_id(icp->cs));
> return ret;
> }
>
> @@ -240,10 +241,9 @@ void ics_synchronize_state(ICSState *ics)
> ics_get_kvm_state(ics);
> }
>
> -int ics_set_kvm_state_one(ICSState *ics, int srcno)
> +int ics_set_kvm_state_one(ICSState *ics, int srcno, Error **errp)
> {
> uint64_t state;
> - Error *local_err = NULL;
> ICSIRQState *irq = &ics->irqs[srcno];
> int ret;
>
> @@ -278,16 +278,15 @@ int ics_set_kvm_state_one(ICSState *ics, int srcno)
> }
>
> ret = kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> - srcno + ics->offset, &state, true, &local_err);
> - if (local_err) {
> - error_report_err(local_err);
> + srcno + ics->offset, &state, true, errp);
> + if (ret < 0) {
> return ret;
> }
>
> return 0;
> }
>
> -int ics_set_kvm_state(ICSState *ics)
> +int ics_set_kvm_state(ICSState *ics, Error **errp)
> {
> int i;
>
> @@ -297,10 +296,12 @@ int ics_set_kvm_state(ICSState *ics)
> }
>
> for (i = 0; i < ics->nr_irqs; i++) {
> + Error *local_err = NULL;
> int ret;
>
> - ret = ics_set_kvm_state_one(ics, i);
> - if (ret) {
> + ret = ics_set_kvm_state_one(ics, i, &local_err);
> + if (ret < 0) {
> + error_propagate(errp, local_err);
> return ret;
> }
> }
> @@ -402,12 +403,18 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
> }
>
> /* Update the KVM sources */
> - ics_set_kvm_state(spapr->ics);
> + ics_set_kvm_state(spapr->ics, &local_err);
> + if (local_err) {
> + goto fail;
> + }
>
> /* Connect the presenters to the initial VCPUs of the machine */
> CPU_FOREACH(cs) {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> - icp_set_kvm_state(spapr_cpu_state(cpu)->icp);
> + icp_set_kvm_state(spapr_cpu_state(cpu)->icp, &local_err);
> + if (local_err) {
> + goto fail;
> + }
> }
>
> return 0;
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index eb65ad7e43b7..1eb7b5cd6847 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -190,13 +190,13 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
>
> /* KVM */
> void icp_get_kvm_state(ICPState *icp);
> -int icp_set_kvm_state(ICPState *icp);
> +int icp_set_kvm_state(ICPState *icp, Error **errp);
> void icp_synchronize_state(ICPState *icp);
> void icp_kvm_realize(DeviceState *dev, Error **errp);
>
> void ics_get_kvm_state(ICSState *ics);
> -int ics_set_kvm_state_one(ICSState *ics, int srcno);
> -int ics_set_kvm_state(ICSState *ics);
> +int ics_set_kvm_state_one(ICSState *ics, int srcno, Error **errp);
> +int ics_set_kvm_state(ICSState *ics, Error **errp);
> void ics_synchronize_state(ICSState *ics);
> void ics_kvm_set_irq(ICSState *ics, int srcno, int val);
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 6/6] xics/kvm: Add proper rollback to xics_kvm_init()
2019-06-17 13:46 [Qemu-devel] [PATCH 0/6] xics/kvm: Improve error handling Greg Kurz
` (4 preceding siblings ...)
2019-06-17 13:46 ` [Qemu-devel] [PATCH 5/6] xics/kvm: Add error propagation to ic*_set_kvm_state() functions Greg Kurz
@ 2019-06-17 13:47 ` Greg Kurz
2019-06-18 7:18 ` Cédric Le Goater
2019-06-20 8:32 ` David Gibson
5 siblings, 2 replies; 19+ messages in thread
From: Greg Kurz @ 2019-06-17 13:47 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
Make xics_kvm_disconnect() able to undo the changes of a partial execution
of xics_kvm_connect() and use it to perform rollback.
Note that kvmppc_define_rtas_kernel_token(0) never fails, no matter the
RTAS call has been defined or not.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/intc/xics_kvm.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 4bfbe1a84092..51433b19b076 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -421,10 +421,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
fail:
error_propagate(errp, local_err);
- kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
- kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
- kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
- kvmppc_define_rtas_kernel_token(0, "ibm,int-off");
+ xics_kvm_disconnect(spapr, NULL);
return -1;
}
@@ -448,8 +445,10 @@ void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp)
* removed from the list of devices of the VM. The VCPU presenters
* are also detached from the device.
*/
- close(kernel_xics_fd);
- kernel_xics_fd = -1;
+ if (kernel_xics_fd != -1) {
+ close(kernel_xics_fd);
+ kernel_xics_fd = -1;
+ }
kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] xics/kvm: Add proper rollback to xics_kvm_init()
2019-06-17 13:47 ` [Qemu-devel] [PATCH 6/6] xics/kvm: Add proper rollback to xics_kvm_init() Greg Kurz
@ 2019-06-18 7:18 ` Cédric Le Goater
2019-06-20 8:32 ` David Gibson
1 sibling, 0 replies; 19+ messages in thread
From: Cédric Le Goater @ 2019-06-18 7:18 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
On 17/06/2019 15:47, Greg Kurz wrote:
> Make xics_kvm_disconnect() able to undo the changes of a partial execution
> of xics_kvm_connect() and use it to perform rollback.
>
> Note that kvmppc_define_rtas_kernel_token(0) never fails, no matter the
> RTAS call has been defined or not.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> hw/intc/xics_kvm.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 4bfbe1a84092..51433b19b076 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -421,10 +421,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>
> fail:
> error_propagate(errp, local_err);
> - kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
> - kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
> - kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
> - kvmppc_define_rtas_kernel_token(0, "ibm,int-off");
> + xics_kvm_disconnect(spapr, NULL);
> return -1;
> }
>
> @@ -448,8 +445,10 @@ void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp)
> * removed from the list of devices of the VM. The VCPU presenters
> * are also detached from the device.
> */
> - close(kernel_xics_fd);
> - kernel_xics_fd = -1;
> + if (kernel_xics_fd != -1) {
> + close(kernel_xics_fd);
> + kernel_xics_fd = -1;
> + }
>
> kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
> kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] xics/kvm: Add proper rollback to xics_kvm_init()
2019-06-17 13:47 ` [Qemu-devel] [PATCH 6/6] xics/kvm: Add proper rollback to xics_kvm_init() Greg Kurz
2019-06-18 7:18 ` Cédric Le Goater
@ 2019-06-20 8:32 ` David Gibson
1 sibling, 0 replies; 19+ messages in thread
From: David Gibson @ 2019-06-20 8:32 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]
On Mon, Jun 17, 2019 at 03:47:03PM +0200, Greg Kurz wrote:
> Make xics_kvm_disconnect() able to undo the changes of a partial execution
> of xics_kvm_connect() and use it to perform rollback.
>
> Note that kvmppc_define_rtas_kernel_token(0) never fails, no matter the
> RTAS call has been defined or not.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied, thanks.
> ---
> hw/intc/xics_kvm.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 4bfbe1a84092..51433b19b076 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -421,10 +421,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>
> fail:
> error_propagate(errp, local_err);
> - kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
> - kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
> - kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
> - kvmppc_define_rtas_kernel_token(0, "ibm,int-off");
> + xics_kvm_disconnect(spapr, NULL);
> return -1;
> }
>
> @@ -448,8 +445,10 @@ void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp)
> * removed from the list of devices of the VM. The VCPU presenters
> * are also detached from the device.
> */
> - close(kernel_xics_fd);
> - kernel_xics_fd = -1;
> + if (kernel_xics_fd != -1) {
> + close(kernel_xics_fd);
> + kernel_xics_fd = -1;
> + }
>
> kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
> kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread