| Message ID | 20170401004624.30886-14-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/31/2017 09:46 PM, Eduardo Habkost wrote: > unimplemented-device needs to be created and mapped using > create_unimplemented_device(), and won't work with -device. > Remove the user_creatable flag from the device class. > > Cc: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/misc/unimp.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/hw/misc/unimp.c b/hw/misc/unimp.c > index 284f096beb..bcbb585888 100644 > --- a/hw/misc/unimp.c > +++ b/hw/misc/unimp.c > @@ -90,11 +90,6 @@ static void unimp_class_init(ObjectClass *klass, void *data) > > dc->realize = unimp_realize; > dc->props = unimp_properties; > - /* > - * FIXME: Set only for compatibility on q35 machine-type. > - * Probably never meant to be user-creatable > - */ > - dc->user_creatable = true; > } > > static const TypeInfo unimp_info = { >
On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote: > unimplemented-device needs to be created and mapped using > create_unimplemented_device() This isn't correct -- create_unimplemented_device() is just a utility function; you can create, configure, initialize and map it "by hand" if you want. > and won't work with -device. This is true, though, as with any sysbus device that has a memory region. > Remove the user_creatable flag from the device class. I'm obviously missing context, because in master it doesn't have code to set the user_creatable flag in the first place. Wouldn't it be better to just not add that, rather than add it and then delete it? thanks -- PMM
On Mon, Apr 03, 2017 at 01:57:06PM +0100, Peter Maydell wrote: > On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@redhat.com> wrote: > > unimplemented-device needs to be created and mapped using > > create_unimplemented_device() > > This isn't correct -- create_unimplemented_device() is > just a utility function; you can create, configure, initialize > and map it "by hand" if you want. OK, I will rewrite it. > > > and won't work with -device. > > This is true, though, as with any sysbus device that has > a memory region. > > > Remove the user_creatable flag from the device class. > > I'm obviously missing context, because in master it doesn't > have code to set the user_creatable flag in the first place. This is in patch 01/19: cannot_instantiate_with_device_add_yet was replaced with !user_creatable. > Wouldn't it be better to just not add that, rather than > add it and then delete it? The device was already user-creatable, but it was not explicit in the code. Patch 03/19 sets user_creatable=false on sysbus but adds explicit user_creatable=true on the 20 devices that are currently accepted by qemu-system-x86_64. The remaining patches clear user_creatable on individual devices types.
On 3 April 2017 at 14:34, Eduardo Habkost <ehabkost@redhat.com> wrote: >> Wouldn't it be better to just not add that, rather than >> add it and then delete it? > > The device was already user-creatable It doesn't seem to be: ./build/x86/arm-softmmu/qemu-system-arm -M virt -device unimplemented-device,size=1024,name=foo qemu-system-arm: Device unimplemented-device can not be dynamically instantiated thanks -- PMM
On Mon, Apr 03, 2017 at 02:38:15PM +0100, Peter Maydell wrote: > On 3 April 2017 at 14:34, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> Wouldn't it be better to just not add that, rather than > >> add it and then delete it? > > > > The device was already user-creatable > > It doesn't seem to be: > > ./build/x86/arm-softmmu/qemu-system-arm -M virt -device > unimplemented-device,size=1024,name=foo > qemu-system-arm: Device unimplemented-device can not be dynamically instantiated This restriction is implemented by the "virt" machine, not by the device type itself. This, on the other hand, currently works: $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo
On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Apr 03, 2017 at 02:38:15PM +0100, Peter Maydell wrote: >> On 3 April 2017 at 14:34, Eduardo Habkost <ehabkost@redhat.com> wrote: >> >> Wouldn't it be better to just not add that, rather than >> >> add it and then delete it? >> > >> > The device was already user-creatable >> >> It doesn't seem to be: >> >> ./build/x86/arm-softmmu/qemu-system-arm -M virt -device >> unimplemented-device,size=1024,name=foo >> qemu-system-arm: Device unimplemented-device can not be dynamically instantiated > > This restriction is implemented by the "virt" machine, not by the > device type itself. > > This, on the other hand, currently works: > $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo That's a bug in the q35 machine or the handling of -device on non-platform-bus systems, though, isn't it? The default for all sysbus devices has always been "you can't use -device with this", there's never been a requirement to mark them as such separately (because it would require touching the files for a huge number of devices for no particularly good reason when you can default the whole set of devices subclassing SysBusDevice). thanks -- PMM
On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote: > On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Mon, Apr 03, 2017 at 02:38:15PM +0100, Peter Maydell wrote: > >> On 3 April 2017 at 14:34, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> >> Wouldn't it be better to just not add that, rather than > >> >> add it and then delete it? > >> > > >> > The device was already user-creatable > >> > >> It doesn't seem to be: > >> > >> ./build/x86/arm-softmmu/qemu-system-arm -M virt -device > >> unimplemented-device,size=1024,name=foo > >> qemu-system-arm: Device unimplemented-device can not be dynamically instantiated > > > > This restriction is implemented by the "virt" machine, not by the > > device type itself. > > > > This, on the other hand, currently works: > > $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo > > That's a bug in the q35 machine or the handling of -device > on non-platform-bus systems, though, isn't it? The default > for all sysbus devices has always been "you can't use > -device with this", [...] This was true until 2014, only. commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device to have cannot_instantiate_with_device_add_yet=false. See patch 03/19 for more detailed info. > [...] there's never been a requirement to > mark them as such separately (because it would require > touching the files for a huge number of devices for no > particularly good reason when you can default the whole > set of devices subclassing SysBusDevice). Yes, and this is the whole point of this series. At the end of this series, only the few devices that are actually usable with some machine will have an explicit user_creatable=true assignment. See cover letter for details.
On 3 April 2017 at 19:30, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote: >> On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > This, on the other hand, currently works: >> > $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo >> >> That's a bug in the q35 machine or the handling of -device >> on non-platform-bus systems, though, isn't it? The default >> for all sysbus devices has always been "you can't use >> -device with this", [...] > > This was true until 2014, only. commit > 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device > to have cannot_instantiate_with_device_add_yet=false. See patch > 03/19 for more detailed info. That commit shouldn't cause this problem -- unless the machine sets has_dynamic_sysbus then the machine_init_notify() that commit adds will cause it to complain. I think the bug was introduced much later, in bf8d4924 in 2016, when q35 had the has_dynamic_sysbus flag added but without any code to restruct this to a whitelist of working devices. (In fact you can see in that commit a very minimal attempt to blacklist a few devices.) The commit message says only intel-iommu was supposed to be -device creatable, so it should have been the only thing on the whitelist. Commit 9e3f9733 shows how this should be done (that's where spapr got has_dynamic_sysbus). Maybe we should just fix the q35 bug first? >> [...] there's never been a requirement to >> mark them as such separately (because it would require >> touching the files for a huge number of devices for no >> particularly good reason when you can default the whole >> set of devices subclassing SysBusDevice). > > Yes, and this is the whole point of this series. At the end of > this series, only the few devices that are actually usable with > some machine will have an explicit user_creatable=true > assignment. See cover letter for details. ...OK, but in that case why not just set that where it should be set, rather than having a big long patchset that touches a lot of devices that in the end are right back where they started? I think a lot of why I'm confused by this patchset is that it seems like it's changing behaviour on devices like this one which don't need any changes... thanks -- PMM
On Mon, Apr 03, 2017 at 08:42:12PM +0100, Peter Maydell wrote: > On 3 April 2017 at 19:30, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote: > >> On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> > This, on the other hand, currently works: > >> > $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo > >> > >> That's a bug in the q35 machine or the handling of -device > >> on non-platform-bus systems, though, isn't it? The default > >> for all sysbus devices has always been "you can't use > >> -device with this", [...] > > > > This was true until 2014, only. commit > > 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device > > to have cannot_instantiate_with_device_add_yet=false. See patch > > 03/19 for more detailed info. > > That commit shouldn't cause this problem -- unless the > machine sets has_dynamic_sysbus then the machine_init_notify() > that commit adds will cause it to complain. > > I think the bug was introduced much later, in bf8d4924 in 2016, > when q35 had the has_dynamic_sysbus flag added but without > any code to restruct this to a whitelist of working > devices. (In fact you can see in that commit a very > minimal attempt to blacklist a few devices.) It's true that the problem happened when q35 set has_dynamic_sysbus=1 without a whitelist. The point of this series is to make a per-machine-type whitelist unnecessary. > > The commit message says only intel-iommu was supposed to > be -device creatable, so it should have been the only > thing on the whitelist. There was a thread about it (Subject: "q35 and sysbus devices"), and nobody knew for sure if the q35 whitelist was supposed to have *-iommu only, or not. The conclusion of that thread is that we can simply use cannot_instantiate_with_device_add_yet to build the whitelist, if the field was set consistently. This series renames cannot_instantiate_with_device_add_yet to user_creatable, and sets it consistently. > Commit 9e3f9733 shows how this > should be done (that's where spapr got has_dynamic_sysbus). > > Maybe we should just fix the q35 bug first? This fixes the q35 bug by setting user_creatable correctly on sys-bus-devices, and makes a q35-specific whitelist unnecessary. > > >> [...] there's never been a requirement to > >> mark them as such separately (because it would require > >> touching the files for a huge number of devices for no > >> particularly good reason when you can default the whole > >> set of devices subclassing SysBusDevice). > > > > Yes, and this is the whole point of this series. At the end of > > this series, only the few devices that are actually usable with > > some machine will have an explicit user_creatable=true > > assignment. See cover letter for details. > > ...OK, but in that case why not just set that where it should > be set, rather than having a big long patchset that touches > a lot of devices that in the end are right back where > they started? I think a lot of why I'm confused by > this patchset is that it seems like it's changing > behaviour on devices like this one which don't need > any changes... The devices don't get back right where they started: they start with cannot_instantiate_with_device_add_yet=false (meaning they are user-creatable in q35), and end up with user_creatable=false (meaning they become non-user-creatable in all machines). I could squash patches 04/19 to 19/19 into patch 03/19, it's true. But then I would probably not get confirmation from you (and other developers) that unimplemented-device (and other devices) is really not supposed to be user-creatable. I wouldn't know if I am breaking a valid q35 use case, or not.
On 03.04.2017 22:05, Eduardo Habkost wrote: > On Mon, Apr 03, 2017 at 08:42:12PM +0100, Peter Maydell wrote: >> On 3 April 2017 at 19:30, Eduardo Habkost <ehabkost@redhat.com> wrote: >>> On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote: >>>> On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote: >>>>> This, on the other hand, currently works: >>>>> $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo >>>> >>>> That's a bug in the q35 machine or the handling of -device >>>> on non-platform-bus systems, though, isn't it? The default >>>> for all sysbus devices has always been "you can't use >>>> -device with this", [...] >>> >>> This was true until 2014, only. commit >>> 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device >>> to have cannot_instantiate_with_device_add_yet=false. See patch >>> 03/19 for more detailed info. >> >> That commit shouldn't cause this problem -- unless the >> machine sets has_dynamic_sysbus then the machine_init_notify() >> that commit adds will cause it to complain. >> >> I think the bug was introduced much later, in bf8d4924 in 2016, >> when q35 had the has_dynamic_sysbus flag added but without >> any code to restruct this to a whitelist of working >> devices. (In fact you can see in that commit a very >> minimal attempt to blacklist a few devices.) > > It's true that the problem happened when q35 set > has_dynamic_sysbus=1 without a whitelist. The point of this > series is to make a per-machine-type whitelist unnecessary. > >> >> The commit message says only intel-iommu was supposed to >> be -device creatable, so it should have been the only >> thing on the whitelist. > > There was a thread about it (Subject: "q35 and sysbus devices"), > and nobody knew for sure if the q35 whitelist was supposed to > have *-iommu only, or not. > > The conclusion of that thread is that we can simply use > cannot_instantiate_with_device_add_yet to build the whitelist, if > the field was set consistently. This series renames > cannot_instantiate_with_device_add_yet to user_creatable, and > sets it consistently. I don't think that this will always work. While you can clearly mark those devices that can never be added dynamically, there might also be some devices that only work on certain machines. For example, there might be devices that only work on the "ppce500" machine, but do not work on the "pseries" machine. So we always need some kind of whitelisting for the machines that have a dynamic sysbus. Thomas
> Am 04.04.2017 um 09:05 schrieb Thomas Huth <thuth@redhat.com>: > >> On 03.04.2017 22:05, Eduardo Habkost wrote: >>> On Mon, Apr 03, 2017 at 08:42:12PM +0100, Peter Maydell wrote: >>>> On 3 April 2017 at 19:30, Eduardo Habkost <ehabkost@redhat.com> wrote: >>>>> On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote: >>>>>> On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote: >>>>>> This, on the other hand, currently works: >>>>>> $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo >>>>> >>>>> That's a bug in the q35 machine or the handling of -device >>>>> on non-platform-bus systems, though, isn't it? The default >>>>> for all sysbus devices has always been "you can't use >>>>> -device with this", [...] >>>> >>>> This was true until 2014, only. commit >>>> 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device >>>> to have cannot_instantiate_with_device_add_yet=false. See patch >>>> 03/19 for more detailed info. >>> >>> That commit shouldn't cause this problem -- unless the >>> machine sets has_dynamic_sysbus then the machine_init_notify() >>> that commit adds will cause it to complain. >>> >>> I think the bug was introduced much later, in bf8d4924 in 2016, >>> when q35 had the has_dynamic_sysbus flag added but without >>> any code to restruct this to a whitelist of working >>> devices. (In fact you can see in that commit a very >>> minimal attempt to blacklist a few devices.) >> >> It's true that the problem happened when q35 set >> has_dynamic_sysbus=1 without a whitelist. The point of this >> series is to make a per-machine-type whitelist unnecessary. >> >>> >>> The commit message says only intel-iommu was supposed to >>> be -device creatable, so it should have been the only >>> thing on the whitelist. >> >> There was a thread about it (Subject: "q35 and sysbus devices"), >> and nobody knew for sure if the q35 whitelist was supposed to >> have *-iommu only, or not. >> >> The conclusion of that thread is that we can simply use >> cannot_instantiate_with_device_add_yet to build the whitelist, if >> the field was set consistently. This series renames >> cannot_instantiate_with_device_add_yet to user_creatable, and >> sets it consistently. > > I don't think that this will always work. While you can clearly mark > those devices that can never be added dynamically, there might also be > some devices that only work on certain machines. For example, there > might be devices that only work on the "ppce500" machine, but do not > work on the "pseries" machine. So we always need some kind of > whitelisting for the machines that have a dynamic sysbus. It's even worse. Sysbus needs explicit logic to wire up memory regions and irqs. That logic is board specific (or at least was last time I checked). So your board *needs* explicit logic for every sysbus device it spawns. And that's why you also get a board whitelist. Alex
On Tue, Apr 04, 2017 at 09:12:59AM +0200, Alexander Graf wrote: > > > > Am 04.04.2017 um 09:05 schrieb Thomas Huth <thuth@redhat.com>: > > > >> On 03.04.2017 22:05, Eduardo Habkost wrote: > >>> On Mon, Apr 03, 2017 at 08:42:12PM +0100, Peter Maydell wrote: > >>>> On 3 April 2017 at 19:30, Eduardo Habkost <ehabkost@redhat.com> wrote: > >>>>> On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote: > >>>>>> On 3 April 2017 at 14:54, Eduardo Habkost <ehabkost@redhat.com> wrote: > >>>>>> This, on the other hand, currently works: > >>>>>> $ qemu-system-x86_64 -M q35 -device unimplemented-device,size=1024,name=foo > >>>>> > >>>>> That's a bug in the q35 machine or the handling of -device > >>>>> on non-platform-bus systems, though, isn't it? The default > >>>>> for all sysbus devices has always been "you can't use > >>>>> -device with this", [...] > >>>> > >>>> This was true until 2014, only. commit > >>>> 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device > >>>> to have cannot_instantiate_with_device_add_yet=false. See patch > >>>> 03/19 for more detailed info. > >>> > >>> That commit shouldn't cause this problem -- unless the > >>> machine sets has_dynamic_sysbus then the machine_init_notify() > >>> that commit adds will cause it to complain. > >>> > >>> I think the bug was introduced much later, in bf8d4924 in 2016, > >>> when q35 had the has_dynamic_sysbus flag added but without > >>> any code to restruct this to a whitelist of working > >>> devices. (In fact you can see in that commit a very > >>> minimal attempt to blacklist a few devices.) > >> > >> It's true that the problem happened when q35 set > >> has_dynamic_sysbus=1 without a whitelist. The point of this > >> series is to make a per-machine-type whitelist unnecessary. > >> > >>> > >>> The commit message says only intel-iommu was supposed to > >>> be -device creatable, so it should have been the only > >>> thing on the whitelist. > >> > >> There was a thread about it (Subject: "q35 and sysbus devices"), > >> and nobody knew for sure if the q35 whitelist was supposed to > >> have *-iommu only, or not. > >> > >> The conclusion of that thread is that we can simply use > >> cannot_instantiate_with_device_add_yet to build the whitelist, if > >> the field was set consistently. This series renames > >> cannot_instantiate_with_device_add_yet to user_creatable, and > >> sets it consistently. > > > > I don't think that this will always work. While you can clearly mark > > those devices that can never be added dynamically, there might also be > > some devices that only work on certain machines. For example, there > > might be devices that only work on the "ppce500" machine, but do not > > work on the "pseries" machine. So we always need some kind of > > whitelisting for the machines that have a dynamic sysbus. > > It's even worse. Sysbus needs explicit logic to wire up memory > regions and irqs. That logic is board specific (or at least was > last time I checked). > > So your board *needs* explicit logic for every sysbus device it > spawns. And that's why you also get a board whitelist. You're correct. My suggestion is that we (humans, not the machine code) can build the whitelist inside q35 code by looking at the remaining user-creatable devices (after this series is reviewed). If I suggested that fixing cannot_instantiate_with_device_add_yet is sufficient to fix the q35 bug, that was a mistake.
diff --git a/hw/misc/unimp.c b/hw/misc/unimp.c index 284f096beb..bcbb585888 100644 --- a/hw/misc/unimp.c +++ b/hw/misc/unimp.c @@ -90,11 +90,6 @@ static void unimp_class_init(ObjectClass *klass, void *data) dc->realize = unimp_realize; dc->props = unimp_properties; - /* - * FIXME: Set only for compatibility on q35 machine-type. - * Probably never meant to be user-creatable - */ - dc->user_creatable = true; } static const TypeInfo unimp_info = {
unimplemented-device needs to be created and mapped using create_unimplemented_device(), and won't work with -device. Remove the user_creatable flag from the device class. Cc: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/misc/unimp.c | 5 ----- 1 file changed, 5 deletions(-)