xive: Make some device types not user creatable

[PATCH] xive: Make some device types not user creatable

Posted by Greg Kurz 6 years, 1 month ago

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 = {

Re: [PATCH] xive: Make some device types not user creatable

Posted by Cédric Le Goater 6 years, 1 month ago

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 = {
>

Re: [PATCH] xive: Make some device types not user creatable

Posted by Markus Armbruster 6 years, 1 month ago

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.

Re: [PATCH] xive: Make some device types not user creatable

Posted by David Gibson 6 years, 1 month ago

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

Re: [PATCH] xive: Make some device types not user creatable

Posted by Philippe Mathieu-Daudé 6 years, 1 month ago

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 = {
> 
>

Re: [PATCH] xive: Make some device types not user creatable

Posted by Greg Kurz 6 years, 1 month ago

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 = {
> > 
> >