* Re: [PATCH] xive: Make some device types not user creatable
2019-10-04 7:38 [PATCH] xive: Make some device types not user creatable Greg Kurz
@ 2019-10-04 8:27 ` Cédric Le Goater
2019-10-04 9:17 ` Philippe Mathieu-Daudé
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Cédric Le Goater @ 2019-10-04 8:27 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
On 04/10/2019 09:38, Greg Kurz wrote:
> Some device types of the XIVE model are exposed to the QEMU command
> line:
>
> $ ppc64-softmmu/qemu-system-ppc64 -device help | grep xive
> name "xive-end-source", desc "XIVE END Source"
> name "xive-source", desc "XIVE Interrupt Source"
> name "xive-tctx", desc "XIVE Interrupt Thread Context"
>
> These are internal devices that shouldn't be instantiable by the
> user. By the way, they can't be because their respective realize
> functions expect link properties that can't be set from the command
> line:
>
> qemu-system-ppc64: -device xive-source: required link 'xive' not found:
> Property '.xive' not found
> qemu-system-ppc64: -device xive-end-source: required link 'xive' not found:
> Property '.xive' not found
> qemu-system-ppc64: -device xive-tctx: required link 'cpu' not found:
> Property '.cpu' not found
>
> Hide them by setting dc->user_creatable to false in their respective
> class init functions.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/intc/xive.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 29df06df1136..6c54a35fd4bb 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -670,6 +670,7 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
> dc->realize = xive_tctx_realize;
> dc->unrealize = xive_tctx_unrealize;
> dc->vmsd = &vmstate_xive_tctx;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xive_tctx_info = {
> @@ -1118,6 +1119,7 @@ static void xive_source_class_init(ObjectClass *klass, void *data)
> dc->props = xive_source_properties;
> dc->realize = xive_source_realize;
> dc->vmsd = &vmstate_xive_source;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xive_source_info = {
> @@ -1853,6 +1855,7 @@ static void xive_end_source_class_init(ObjectClass *klass, void *data)
> dc->desc = "XIVE END Source";
> dc->props = xive_end_source_properties;
> dc->realize = xive_end_source_realize;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xive_end_source_info = {
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xive: Make some device types not user creatable
2019-10-04 7:38 [PATCH] xive: Make some device types not user creatable Greg Kurz
2019-10-04 8:27 ` Cédric Le Goater
@ 2019-10-04 9:17 ` Philippe Mathieu-Daudé
2019-10-04 10:00 ` Greg Kurz
2019-10-05 10:21 ` David Gibson
2019-10-07 9:02 ` Markus Armbruster
3 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-04 9:17 UTC (permalink / raw)
To: Greg Kurz, David Gibson, Thomas Huth, Eduardo Habkost
Cc: Markus Armbruster, qemu-ppc, Cédric Le Goater, qemu-devel
Hi Greg,
On 10/4/19 9:38 AM, Greg Kurz wrote:
> Some device types of the XIVE model are exposed to the QEMU command
> line:
>
> $ ppc64-softmmu/qemu-system-ppc64 -device help | grep xive
> name "xive-end-source", desc "XIVE END Source"
> name "xive-source", desc "XIVE Interrupt Source"
> name "xive-tctx", desc "XIVE Interrupt Thread Context"
>
> These are internal devices that shouldn't be instantiable by the
> user. By the way, they can't be because their respective realize
> functions expect link properties that can't be set from the command
> line:
>
> qemu-system-ppc64: -device xive-source: required link 'xive' not found:
> Property '.xive' not found
> qemu-system-ppc64: -device xive-end-source: required link 'xive' not found:
> Property '.xive' not found
> qemu-system-ppc64: -device xive-tctx: required link 'cpu' not found:
> Property '.cpu' not found
Why do you have to test that manually, isn't it what
tests/device-introspect-test.c::test_one_device does?
> Hide them by setting dc->user_creatable to false in their respective
> class init functions.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/intc/xive.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 29df06df1136..6c54a35fd4bb 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -670,6 +670,7 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
> dc->realize = xive_tctx_realize;
> dc->unrealize = xive_tctx_unrealize;
> dc->vmsd = &vmstate_xive_tctx;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xive_tctx_info = {
> @@ -1118,6 +1119,7 @@ static void xive_source_class_init(ObjectClass *klass, void *data)
> dc->props = xive_source_properties;
> dc->realize = xive_source_realize;
> dc->vmsd = &vmstate_xive_source;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xive_source_info = {
> @@ -1853,6 +1855,7 @@ static void xive_end_source_class_init(ObjectClass *klass, void *data)
> dc->desc = "XIVE END Source";
> dc->props = xive_end_source_properties;
> dc->realize = xive_end_source_realize;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xive_end_source_info = {
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xive: Make some device types not user creatable
2019-10-04 9:17 ` Philippe Mathieu-Daudé
@ 2019-10-04 10:00 ` Greg Kurz
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kurz @ 2019-10-04 10:00 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Thomas Huth, Eduardo Habkost, qemu-devel, Markus Armbruster,
qemu-ppc, Cédric Le Goater, David Gibson
On Fri, 4 Oct 2019 11:17:30 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Hi Greg,
>
> On 10/4/19 9:38 AM, Greg Kurz wrote:
> > Some device types of the XIVE model are exposed to the QEMU command
> > line:
> >
> > $ ppc64-softmmu/qemu-system-ppc64 -device help | grep xive
> > name "xive-end-source", desc "XIVE END Source"
> > name "xive-source", desc "XIVE Interrupt Source"
> > name "xive-tctx", desc "XIVE Interrupt Thread Context"
> >
> > These are internal devices that shouldn't be instantiable by the
> > user. By the way, they can't be because their respective realize
> > functions expect link properties that can't be set from the command
> > line:
> >
> > qemu-system-ppc64: -device xive-source: required link 'xive' not found:
> > Property '.xive' not found
> > qemu-system-ppc64: -device xive-end-source: required link 'xive' not found:
> > Property '.xive' not found
> > qemu-system-ppc64: -device xive-tctx: required link 'cpu' not found:
> > Property '.cpu' not found
>
> Why do you have to test that manually, isn't it what
> tests/device-introspect-test.c::test_one_device does?
>
Heh probably because I wasn't aware of it :)
And BTW, test_one_device() can't help here since it only cares
about 'device_add foo,help' not crashing QEMU:
help = qtest_hmp(qts, "device_add \"%s,help\"", type);
as explained in a comment at the beginning of the file.
/*
* Covers QMP device-list-properties and HMP device_add help. We
* currently don't check that their output makes sense, only that QEMU
* survives. Useful since we've had an astounding number of crash
* bugs around here.
*/
> > Hide them by setting dc->user_creatable to false in their respective
> > class init functions.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > hw/intc/xive.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> > index 29df06df1136..6c54a35fd4bb 100644
> > --- a/hw/intc/xive.c
> > +++ b/hw/intc/xive.c
> > @@ -670,6 +670,7 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
> > dc->realize = xive_tctx_realize;
> > dc->unrealize = xive_tctx_unrealize;
> > dc->vmsd = &vmstate_xive_tctx;
> > + dc->user_creatable = false;
> > }
> >
> > static const TypeInfo xive_tctx_info = {
> > @@ -1118,6 +1119,7 @@ static void xive_source_class_init(ObjectClass *klass, void *data)
> > dc->props = xive_source_properties;
> > dc->realize = xive_source_realize;
> > dc->vmsd = &vmstate_xive_source;
> > + dc->user_creatable = false;
> > }
> >
> > static const TypeInfo xive_source_info = {
> > @@ -1853,6 +1855,7 @@ static void xive_end_source_class_init(ObjectClass *klass, void *data)
> > dc->desc = "XIVE END Source";
> > dc->props = xive_end_source_properties;
> > dc->realize = xive_end_source_realize;
> > + dc->user_creatable = false;
> > }
> >
> > static const TypeInfo xive_end_source_info = {
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xive: Make some device types not user creatable
2019-10-04 7:38 [PATCH] xive: Make some device types not user creatable Greg Kurz
2019-10-04 8:27 ` Cédric Le Goater
2019-10-04 9:17 ` Philippe Mathieu-Daudé
@ 2019-10-05 10:21 ` David Gibson
2019-10-07 9:02 ` Markus Armbruster
3 siblings, 0 replies; 6+ messages in thread
From: David Gibson @ 2019-10-05 10:21 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]
On Fri, Oct 04, 2019 at 09:38:50AM +0200, Greg Kurz wrote:
65;5603;1c> Some device types of the XIVE model are exposed to the QEMU command
> line:
>
> $ ppc64-softmmu/qemu-system-ppc64 -device help | grep xive
> name "xive-end-source", desc "XIVE END Source"
> name "xive-source", desc "XIVE Interrupt Source"
> name "xive-tctx", desc "XIVE Interrupt Thread Context"
>
> These are internal devices that shouldn't be instantiable by the
> user. By the way, they can't be because their respective realize
> functions expect link properties that can't be set from the command
> line:
>
> qemu-system-ppc64: -device xive-source: required link 'xive' not found:
> Property '.xive' not found
> qemu-system-ppc64: -device xive-end-source: required link 'xive' not found:
> Property '.xive' not found
> qemu-system-ppc64: -device xive-tctx: required link 'cpu' not found:
> Property '.cpu' not found
>
> Hide them by setting dc->user_creatable to false in their respective
> class init functions.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied to ppc-for-4.2.
> ---
> hw/intc/xive.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 29df06df1136..6c54a35fd4bb 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -670,6 +670,7 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
> dc->realize = xive_tctx_realize;
> dc->unrealize = xive_tctx_unrealize;
> dc->vmsd = &vmstate_xive_tctx;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xive_tctx_info = {
> @@ -1118,6 +1119,7 @@ static void xive_source_class_init(ObjectClass *klass, void *data)
> dc->props = xive_source_properties;
> dc->realize = xive_source_realize;
> dc->vmsd = &vmstate_xive_source;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xive_source_info = {
> @@ -1853,6 +1855,7 @@ static void xive_end_source_class_init(ObjectClass *klass, void *data)
> dc->desc = "XIVE END Source";
> dc->props = xive_end_source_properties;
> dc->realize = xive_end_source_realize;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xive_end_source_info = {
>
--
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] 6+ messages in thread* Re: [PATCH] xive: Make some device types not user creatable
2019-10-04 7:38 [PATCH] xive: Make some device types not user creatable Greg Kurz
` (2 preceding siblings ...)
2019-10-05 10:21 ` David Gibson
@ 2019-10-07 9:02 ` Markus Armbruster
3 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2019-10-07 9:02 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-devel, qemu-ppc, Cédric Le Goater, David Gibson
Greg Kurz <groug@kaod.org> writes:
> Some device types of the XIVE model are exposed to the QEMU command
> line:
>
> $ ppc64-softmmu/qemu-system-ppc64 -device help | grep xive
> name "xive-end-source", desc "XIVE END Source"
> name "xive-source", desc "XIVE Interrupt Source"
> name "xive-tctx", desc "XIVE Interrupt Thread Context"
>
> These are internal devices that shouldn't be instantiable by the
> user. By the way, they can't be because their respective realize
> functions expect link properties that can't be set from the command
> line:
>
> qemu-system-ppc64: -device xive-source: required link 'xive' not found:
> Property '.xive' not found
> qemu-system-ppc64: -device xive-end-source: required link 'xive' not found:
> Property '.xive' not found
> qemu-system-ppc64: -device xive-tctx: required link 'cpu' not found:
> Property '.cpu' not found
>
> Hide them by setting dc->user_creatable to false in their respective
> class init functions.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/intc/xive.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 29df06df1136..6c54a35fd4bb 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -670,6 +670,7 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
> dc->realize = xive_tctx_realize;
> dc->unrealize = xive_tctx_unrealize;
> dc->vmsd = &vmstate_xive_tctx;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xive_tctx_info = {
> @@ -1118,6 +1119,7 @@ static void xive_source_class_init(ObjectClass *klass, void *data)
> dc->props = xive_source_properties;
> dc->realize = xive_source_realize;
> dc->vmsd = &vmstate_xive_source;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xive_source_info = {
> @@ -1853,6 +1855,7 @@ static void xive_end_source_class_init(ObjectClass *klass, void *data)
> dc->desc = "XIVE END Source";
> dc->props = xive_end_source_properties;
> dc->realize = xive_end_source_realize;
> + dc->user_creatable = false;
> }
>
> static const TypeInfo xive_end_source_info = {
These all need a comment like the existing ->user_creatable = false
have.
Your commit message mentions link properties. Based on that:
/*
* Reason: link property 'NAME-OF-PROP' needs to be wired up.
*/
Rather minimal, though. Several existing similar cases are a bit more
specific, which is nice:
/*
* Reason: part of WHATEVER, needs to be wired up by FUNCTION().
*/
or if there is or could be more than one FUNCTION():
/*
* Reason: part of WHATEVER, needs to be wired up, e.g. by FUNCTION().
*/
David queued your patch already. If it goes into master without such
comments, please post them as a follow-up patch.
^ permalink raw reply [flat|nested] 6+ messages in thread