[Qemu-devel] [PATCH] qdev: obey no_user

[Qemu-devel] [PATCH] qdev: obey no_user

[Christian Borntraeger]

since

commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96
qdev: refactor device creation to allow bus_info to be set only in class

A user can specify a device that is no_user.
For example on my i386 box, I can add a 2nd kvmvapic device.

This patch checks for no-user and rejects the device_add.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/qdev-monitor.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index a1b4d6a..b2c34e7 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -426,6 +426,11 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     }
 
     k = DEVICE_CLASS(obj);
+    if (k->no_user) {
+        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "a driver name");
+        error_printf_unless_qmp("Try with argument 'help' for a list.\n");
+        return NULL;
+    }
 
     /* find bus */
     path = qemu_opt_get(opts, "bus");
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Christian Borntraeger]

Ping?


On 17/12/12 17:00, Christian Borntraeger wrote:
> since
> 
> commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96
> qdev: refactor device creation to allow bus_info to be set only in class
> 
> A user can specify a device that is no_user.
> For example on my i386 box, I can add a 2nd kvmvapic device.
> 
> This patch checks for no-user and rejects the device_add.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/qdev-monitor.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index a1b4d6a..b2c34e7 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -426,6 +426,11 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      }
> 
>      k = DEVICE_CLASS(obj);
> +    if (k->no_user) {
> +        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "a driver name");
> +        error_printf_unless_qmp("Try with argument 'help' for a list.\n");
> +        return NULL;
> +    }
> 
>      /* find bus */
>      path = qemu_opt_get(opts, "bus");
> 

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Andreas Färber]

Am 17.12.2012 17:00, schrieb Christian Borntraeger:
> since
> 
> commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96
> qdev: refactor device creation to allow bus_info to be set only in class
> 
> A user can specify a device that is no_user.
> For example on my i386 box, I can add a 2nd kvmvapic device.
> 
> This patch checks for no-user and rejects the device_add.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/qdev-monitor.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index a1b4d6a..b2c34e7 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -426,6 +426,11 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      }
>  
>      k = DEVICE_CLASS(obj);
> +    if (k->no_user) {

Ack for this check...

> +        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "a driver name");
> +        error_printf_unless_qmp("Try with argument 'help' for a list.\n");

...but I'm not sure if this error reporting with QERR_... is still in
the desired form, cc'ing Luiz.

Andreas

> +        return NULL;
> +    }
>  
>      /* find bus */
>      path = qemu_opt_get(opts, "bus");

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Luiz Capitulino]

On Mon, 07 Jan 2013 15:20:17 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 17.12.2012 17:00, schrieb Christian Borntraeger:
> > since
> > 
> > commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96
> > qdev: refactor device creation to allow bus_info to be set only in class
> > 
> > A user can specify a device that is no_user.
> > For example on my i386 box, I can add a 2nd kvmvapic device.
> > 
> > This patch checks for no-user and rejects the device_add.
> > 
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> >  hw/qdev-monitor.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> > index a1b4d6a..b2c34e7 100644
> > --- a/hw/qdev-monitor.c
> > +++ b/hw/qdev-monitor.c
> > @@ -426,6 +426,11 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >      }
> >  
> >      k = DEVICE_CLASS(obj);
> > +    if (k->no_user) {
> 
> Ack for this check...
> 
> > +        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "a driver name");
> > +        error_printf_unless_qmp("Try with argument 'help' for a list.\n");
> 
> ...but I'm not sure if this error reporting with QERR_... is still in
> the desired form, cc'ing Luiz.

The device_add command hasn't been converted to the qapi yet, so it's fine
to use qerror_report(). However, I'd drop the error_printf_unless_qmp() call,
as this has the potential of making the conversion harder.

> 
> Andreas
> 
> > +        return NULL;
> > +    }
> >  
> >      /* find bus */
> >      path = qemu_opt_get(opts, "bus");
> 

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Anthony Liguori]

Christian Borntraeger <borntraeger@de.ibm.com> writes:

> since
>
> commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96
> qdev: refactor device creation to allow bus_info to be set only in class
>
> A user can specify a device that is no_user.
> For example on my i386 box, I can add a 2nd kvmvapic device.
>
> This patch checks for no-user and rejects the device_add.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

I specifically broke this when QOM was introduced because no_user
precludes a management tool from constructing a machine directlt.

The real problem you're trying to solve is that it's an error to have
two kvmvapic devices, not that users shouldn't be allowed to create them
via -device.

Regards,

Anthony Liguori

> ---
>  hw/qdev-monitor.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index a1b4d6a..b2c34e7 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -426,6 +426,11 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      }
>  
>      k = DEVICE_CLASS(obj);
> +    if (k->no_user) {
> +        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "a driver name");
> +        error_printf_unless_qmp("Try with argument 'help' for a list.\n");
> +        return NULL;
> +    }
>  
>      /* find bus */
>      path = qemu_opt_get(opts, "bus");
> -- 
> 1.7.11.4

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Peter Maydell]

On 7 January 2013 19:32, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Christian Borntraeger <borntraeger@de.ibm.com> writes:
>
>> since
>>
>> commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96
>> qdev: refactor device creation to allow bus_info to be set only in class
>>
>> A user can specify a device that is no_user.
>> For example on my i386 box, I can add a 2nd kvmvapic device.
>>
>> This patch checks for no-user and rejects the device_add.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
> I specifically broke this when QOM was introduced because no_user
> precludes a management tool from constructing a machine directlt.

You can't do that anyway...

> The real problem you're trying to solve is that it's an error to have
> two kvmvapic devices, not that users shouldn't be allowed to create them
> via -device.

That's not the only thing no-user gets used for. A bunch of the
ARM sysbus devices have it set presumably because it's just
flat impossible for a user to correctly create and wire in a sysbus
device at all, so you might as well not confuse matters by listing
them in '-device help' output. (We're not consistent about that,
though.)

It seems to me like arbitrarily allowing the monitor to construct
no-user devices isn't really the right way to attack the problem
of "allow complete machine construction by management tools"...

-- PMM

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 January 2013 19:32, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Christian Borntraeger <borntraeger@de.ibm.com> writes:
>>
>>> since
>>>
>>> commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96
>>> qdev: refactor device creation to allow bus_info to be set only in class
>>>
>>> A user can specify a device that is no_user.
>>> For example on my i386 box, I can add a 2nd kvmvapic device.
>>>
>>> This patch checks for no-user and rejects the device_add.
>>>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>
>> I specifically broke this when QOM was introduced because no_user
>> precludes a management tool from constructing a machine directlt.
>
> You can't do that anyway...
>
>> The real problem you're trying to solve is that it's an error to have
>> two kvmvapic devices, not that users shouldn't be allowed to create them
>> via -device.
>
> That's not the only thing no-user gets used for. A bunch of the
> ARM sysbus devices have it set presumably because it's just
> flat impossible for a user to correctly create and wire in a sysbus
> device at all, so you might as well not confuse matters by listing
> them in '-device help' output. (We're not consistent about that,
> though.)
>
> It seems to me like arbitrarily allowing the monitor to construct
> no-user devices isn't really the right way to attack the problem
> of "allow complete machine construction by management tools"...

There is no such thing as a 'no-user' device.  It's a silly distinction
that has never had a consistent meaning.

There's really no good reason why kvmvapic isn't supported by -device
other than it was an implementation short cut.

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Peter Maydell]

On 7 January 2013 20:12, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> It seems to me like arbitrarily allowing the monitor to construct
>> no-user devices isn't really the right way to attack the problem
>> of "allow complete machine construction by management tools"...
>
> There is no such thing as a 'no-user' device.  It's a silly distinction
> that has never had a consistent meaning.

Then let's just rip that flag out completely.

-- PMM

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 January 2013 20:12, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> It seems to me like arbitrarily allowing the monitor to construct
>>> no-user devices isn't really the right way to attack the problem
>>> of "allow complete machine construction by management tools"...
>>
>> There is no such thing as a 'no-user' device.  It's a silly distinction
>> that has never had a consistent meaning.
>
> Then let's just rip that flag out completely.

I have no objection to that.  I would have done that but it wasn't easy
to script at the time.

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Andreas Färber]

Am 07.01.2013 21:16, schrieb Peter Maydell:
> On 7 January 2013 20:12, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> It seems to me like arbitrarily allowing the monitor to construct
>>> no-user devices isn't really the right way to attack the problem
>>> of "allow complete machine construction by management tools"...
>>
>> There is no such thing as a 'no-user' device.  It's a silly distinction
>> that has never had a consistent meaning.
> 
> Then let's just rip that flag out completely.

That's a bad idea, given that we are about to make the CPU a device.

So yes, there are devices that are not meant to be instantiated more
than once (e.g., sclp-console). And there are devices that are not meant
to be instantiated by the user at all.

BTW this patch exposed that there is silly code in this function: When
the bus is NULL, it is initialized as the SysBus. But we all know that
SysBus is not hotplug-capable in the first place, so we can never
device_add on SysBus.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Peter Maydell]

On 7 January 2013 22:10, Andreas Färber <afaerber@suse.de> wrote:
> Am 07.01.2013 21:16, schrieb Peter Maydell:
>> On 7 January 2013 20:12, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>> It seems to me like arbitrarily allowing the monitor to construct
>>>> no-user devices isn't really the right way to attack the problem
>>>> of "allow complete machine construction by management tools"...
>>>
>>> There is no such thing as a 'no-user' device.  It's a silly distinction
>>> that has never had a consistent meaning.
>>
>> Then let's just rip that flag out completely.
>
> That's a bad idea, given that we are about to make the CPU a device.

We already have a pile of devices which the user can't usefully
use -device on...CPUs would be just another one, right?

-- PMM

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Andreas Färber]

Am 07.01.2013 23:20, schrieb Peter Maydell:
> On 7 January 2013 22:10, Andreas Färber <afaerber@suse.de> wrote:
>> Am 07.01.2013 21:16, schrieb Peter Maydell:
>>> On 7 January 2013 20:12, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>> It seems to me like arbitrarily allowing the monitor to construct
>>>>> no-user devices isn't really the right way to attack the problem
>>>>> of "allow complete machine construction by management tools"...
>>>>
>>>> There is no such thing as a 'no-user' device.  It's a silly distinction
>>>> that has never had a consistent meaning.
>>>
>>> Then let's just rip that flag out completely.
>>
>> That's a bad idea, given that we are about to make the CPU a device.
> 
> We already have a pile of devices which the user can't usefully
> use -device on...CPUs would be just another one, right?

Not sure what you're arguing for here? The CPU is the worst example of a
device I know in that it is not limited to a particular bus and messes
with global state and threads.

Also, I'm sure that there will be objects/devices that a management tool
is not supposed to mess with either, once we start using
object_initialize() more.

If we agree that "no_user = 1" is not ideal or used inconsistently, then
we should IMO talk about how to replace/amend it and not conclude to
just rip it out and leave users without sensible error messages while
praying for The Omnipotent Management Tool that AFAICS we don't have
today...

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 07.01.2013 23:20, schrieb Peter Maydell:
>> On 7 January 2013 22:10, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 07.01.2013 21:16, schrieb Peter Maydell:
>>>> On 7 January 2013 20:12, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>>> It seems to me like arbitrarily allowing the monitor to construct
>>>>>> no-user devices isn't really the right way to attack the problem
>>>>>> of "allow complete machine construction by management tools"...
>>>>>
>>>>> There is no such thing as a 'no-user' device.  It's a silly distinction
>>>>> that has never had a consistent meaning.
>>>>
>>>> Then let's just rip that flag out completely.
>>>
>>> That's a bad idea, given that we are about to make the CPU a device.
>> 
>> We already have a pile of devices which the user can't usefully
>> use -device on...CPUs would be just another one, right?
>
> Not sure what you're arguing for here? The CPU is the worst example of a
> device I know in that it is not limited to a particular bus and messes
> with global state and threads.
>
> Also, I'm sure that there will be objects/devices that a management tool
> is not supposed to mess with either, once we start using
> object_initialize() more.
>
> If we agree that "no_user = 1" is not ideal or used inconsistently, then
> we should IMO talk about how to replace/amend it and not conclude to
> just rip it out and leave users without sensible error messages while
> praying for The Omnipotent Management Tool that AFAICS we don't have
> today...

If you want to replace 'no_user' with
'cannot_instantiate_with_device_add_yet_due_to_internal_bugs' I'm okay
with that.

I'm all for having friendly errors for users.  But no_user doesn't just
serve a 'broken' flag today.  Random devices are marked as no_user that
really shouldn't be like isa-fdc.

Regards,

Anthony Liguori

>
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Christian Borntraeger]

On 07/01/13 20:32, Anthony Liguori wrote:
> Christian Borntraeger <borntraeger@de.ibm.com> writes:
> 
>> since
>>
>> commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96
>> qdev: refactor device creation to allow bus_info to be set only in class
>>
>> A user can specify a device that is no_user.
>> For example on my i386 box, I can add a 2nd kvmvapic device.
>>
>> This patch checks for no-user and rejects the device_add.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> I specifically broke this when QOM was introduced because no_user
> precludes a management tool from constructing a machine directlt.

Breaking it but leaving it in the code doesnt seem to be the right thing.
The commit message from 18b6dade doesnt give any hint that this is now
broken and nobody audited the callers of no_user that they handle things
gracefully.

So whats the plan? Totally remove no_user tree-wide?

Christian

Re: [Qemu-devel] [PATCH] qdev: obey no_user

[Anthony Liguori]

Christian Borntraeger <borntraeger@de.ibm.com> writes:

> On 07/01/13 20:32, Anthony Liguori wrote:
>> Christian Borntraeger <borntraeger@de.ibm.com> writes:
>> 
>>> since
>>>
>>> commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96
>>> qdev: refactor device creation to allow bus_info to be set only in class
>>>
>>> A user can specify a device that is no_user.
>>> For example on my i386 box, I can add a 2nd kvmvapic device.
>>>
>>> This patch checks for no-user and rejects the device_add.
>>>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> 
>> I specifically broke this when QOM was introduced because no_user
>> precludes a management tool from constructing a machine directlt.
>
> Breaking it but leaving it in the code doesnt seem to be the right thing.
> The commit message from 18b6dade doesnt give any hint that this is now
> broken and nobody audited the callers of no_user that they handle things
> gracefully.
>
> So whats the plan? Totally remove no_user tree-wide?

One of the reasons I left no_user is that it's exposed to users via
'info qdm'.  I don't think it matters anymore so we can probably safely
remove it.

Regards,

Anthony Liguori

>
> Christian