qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Marcel Apfelbaum <marcel@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, mst@redhat.com
Subject: Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them
Date: Sun, 15 May 2016 14:23:08 +0300	[thread overview]
Message-ID: <57385C1C.1050908@redhat.com> (raw)
In-Reply-To: <87y47hhw1g.fsf@dusky.pond.sub.org>

On 05/11/2016 10:51 AM, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel@redhat.com> writes:
>
>> On 05/10/2016 11:28 AM, Markus Armbruster wrote:
>>> Marcel Apfelbaum <marcel@redhat.com> writes:
>>>
>>>> This series aims to allow more devices to be used with '-device'
>>>> by sorting the devices based on a predefined creation order flag
>>>> before creating them.
>>>>
>>>> Devices like IOMMU need to be created before others, so they can leverage
>>>> the DeviceCreationPriority flag introduced by the first patch to DeviceClass.
>>>>
>>>> The second patch sorts the devices by their DeviceCreationPriority
>>>> before creating them.
>>>>
>>>> Finally, the last patch demonstrates how it can be used to ensure
>>>> the creation of host-bridges before the pci-bridges and pci-bridges before
>>>> the others.
>>>>
>>>> I preferred to combine all the priorities into a single enum
>>>> to better manage the creation order.
>>>>
>>>> This is an RFC because I only wanted to know if it seems like the right way to go.
>>>> Comments are appreciated,
>>>
>>
>> Hi Markus,
>> Thanks for looking into this.
>>
>>
>>> Can you explain why requiring the user to specify -device in a sane
>>> order isn't good enough?
>>>
>>
>> Point taken, the truth is I didn't like the 'order' restriction in the
>> first place.
>>
>> If the device creation depends on the id of some other devices (e.g we
>> need the bus id to plug a device into it), for IOMMU devices it gets a
>> little tricky. You can add the IOMMU device before other PCI devices
>> but it will not work (because some internal implementation). This is
>> why we added using -machine pc,iommu=on.  I suppose we have other
>> examples as well. This is not user friendly IMO.
>>
>> To solve the specific IOMMU problem we can check that there are no PCI
>> devices created yet, but I am not sure is a better approach and is
>> strictly related to this device.
>>
>> The goal is to be able to add more devices with -device and I thought
>> this kind of creation in steps may help.


Hi Markus,

>
> In my opinion, there are two sane ways to do command line options.
>
> One is to make order relevant, and process them strictly left to right.
>
> The other is to do the right thing regardless of order.  This requires
> some kind of dependency tracking if there are any.
>

I personally like this way more, however I confess I do not aim to solve this
globally, my scope is making more user friendly the stuff I work with.

> QEMU, of course, does neither: we process them in left to right unless
> we don't, and users juggle them until the errors go away.
>

And this is why I thought, we have some case the order is important,
some cases it is not. Adding one more case to "order not important" set
looks like a little win.

> I'm afraid this patch adds to "unless we don't" without covering much
> ground towards "do the right thing regardless of order".  Static
> priorities are a rather crude approximation of dependencies.  Is it the
> best we can do for user now?
>

As stated above, I am perfectly aware the static priorities angle does not
solve all the problems, but it helps and may be a good step in the right direction.

The static priorities can make sense, the host bridge needs to be created before
the other pci bridges, which should also be created before the devices itself.

IOMMU can also leverage static priorities, it needs to be created after the host bridge,
but before anything else.

I am sure we can think of other cases those will help too. To answer your question,
no, is not the best we can do, but maybe it worth it.

Thanks,
Marcel

  reply	other threads:[~2016-05-15 11:23 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-05-09 17:47 [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them Marcel Apfelbaum
2016-05-09 17:47 ` [Qemu-devel] [PATCH RFC 1/3] qdev: add device creation priority flag Marcel Apfelbaum
2016-05-09 17:47 ` [Qemu-devel] [PATCH RFC 2/3] vl.c: create devices by their " Marcel Apfelbaum
2016-05-09 17:47 ` [Qemu-devel] [PATCH RFC 3/3] hw/pci-bridge: add the corresponding " Marcel Apfelbaum
2016-05-10  8:28 ` [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them Markus Armbruster
2016-05-10 11:05   ` Marcel Apfelbaum
2016-05-11  7:51     ` Markus Armbruster
2016-05-15 11:23       ` Marcel Apfelbaum [this message]
2016-05-10 15:36   ` Michael S. Tsirkin
2016-05-10 17:13     ` Paolo Bonzini
2016-05-11 13:55       ` Michael S. Tsirkin
2016-05-11 15:09         ` Paolo Bonzini
2016-05-11 15:19           ` Michael S. Tsirkin
2016-05-11 15:25             ` Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=57385C1C.1050908@redhat.com \
    --to=marcel@redhat.com \
    --cc=armbru@redhat.com \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).