Patchwork [v3,07/10] piix3 piix4: Clean up use of cannot_instantiate_with_device_add_yet

login
register
mail settings
Submitter Markus Armbruster
Date Oct. 30, 2013, 4:28 p.m.
Message ID <1383150524-16250-8-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/287284/
State New
Headers show

Comments

Markus Armbruster - Oct. 30, 2013, 4:28 p.m.
From: Markus Armbruster <armbru@redhat.com>

A PIIX3/PIIX4 southbridge has multiple functions.  We model each
function as a separate qdev.  Two of them need some special wiring set
up in pc_init1() or mips_malta_init() to work: the ISA bridge at 01.0,
and the SMBus controller at 01.3.

The IDE controller at 01.1 (piix3-ide, piix3-ide-xen, piix4-ide) has
always had cannot_instantiate_with_device_add_yet set, but there is no
obvious reason why device_add could not work for them.  Drop it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/acpi/piix4.c    |  6 +++++-
 hw/ide/piix.c      |  3 ---
 hw/isa/piix4.c     |  6 +++++-
 hw/pci-host/piix.c | 12 ++++++++++--
 4 files changed, 20 insertions(+), 7 deletions(-)
Marcel Apfelbaum - Oct. 31, 2013, 9 a.m.
On Wed, 2013-10-30 at 17:28 +0100, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> A PIIX3/PIIX4 southbridge has multiple functions.  We model each
> function as a separate qdev.  Two of them need some special wiring set
> up in pc_init1() or mips_malta_init() to work: the ISA bridge at 01.0,
> and the SMBus controller at 01.3.
> 
> The IDE controller at 01.1 (piix3-ide, piix3-ide-xen, piix4-ide) has
> always had cannot_instantiate_with_device_add_yet set, but there is no
> obvious reason why device_add could not work for them.  Drop it.
Question:
piix3-ide (and probably piix4-ide) has io ports hard coded in pci_piix_init_ports.
PIIX3/PIIX4 already has a piix3-ide that uses the io ports.
Adding more piix3-ide devices would work? 
What am I missing?

Another question:
It seems that piix3-ide it was meant to be a function within PIIX3,
if so, we need a cannot_instantiate_with_device_add_ever?

Hope it helped,
Marcel

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/acpi/piix4.c    |  6 +++++-
>  hw/ide/piix.c      |  3 ---
>  hw/isa/piix4.c     |  6 +++++-
>  hw/pci-host/piix.c | 12 ++++++++++--
>  4 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index c29a703..368a76b 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -508,9 +508,13 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
>      k->revision = 0x03;
>      k->class_id = PCI_CLASS_BRIDGE_OTHER;
>      dc->desc = "PM";
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      dc->vmsd = &vmstate_acpi;
>      dc->props = piix4_pm_properties;
> +    /*
> +     * Reason: part of PIIX4 southbridge, needs to be wired up,
> +     * e.g. by mips_malta_init()
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo piix4_pm_info = {
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 27b08e1..9b5960b 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -248,7 +248,6 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
>      k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
>      k->class_id = PCI_CLASS_STORAGE_IDE;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>  }
>  
>  static const TypeInfo piix3_ide_info = {
> @@ -267,7 +266,6 @@ static void piix3_ide_xen_class_init(ObjectClass *klass, void *data)
>      k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
>      k->class_id = PCI_CLASS_STORAGE_IDE;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      dc->unplug = pci_piix3_xen_ide_unplug;
>  }
>  
> @@ -289,7 +287,6 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
>      k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
>      k->class_id = PCI_CLASS_STORAGE_IDE;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>  }
>  
>  static const TypeInfo piix4_ide_info = {
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index d9dac61..def6fe3 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -113,8 +113,12 @@ static void piix4_class_init(ObjectClass *klass, void *data)
>      k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
>      k->class_id = PCI_CLASS_BRIDGE_ISA;
>      dc->desc = "ISA bridge";
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      dc->vmsd = &vmstate_piix4;
> +    /*
> +     * Reason: part of PIIX4 southbridge, needs to be wired up,
> +     * e.g. by mips_malta_init()
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo piix4_info = {
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 8089fd6..1526fd4 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -644,7 +644,6 @@ static void piix3_class_init(ObjectClass *klass, void *data)
>  
>      dc->desc        = "ISA bridge";
>      dc->vmsd        = &vmstate_piix3;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      k->no_hotplug   = 1;
>      k->init         = piix3_initfn;
>      k->config_write = piix3_write_config;
> @@ -652,6 +651,11 @@ static void piix3_class_init(ObjectClass *klass, void *data)
>      /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
>      k->device_id    = PCI_DEVICE_ID_INTEL_82371SB_0;
>      k->class_id     = PCI_CLASS_BRIDGE_ISA;
> +    /*
> +     * Reason: part of PIIX3 southbridge, needs to be wired up by
> +     * pc_piix.c's pc_init1()
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo piix3_info = {
> @@ -668,7 +672,6 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
>  
>      dc->desc        = "ISA bridge";
>      dc->vmsd        = &vmstate_piix3;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      k->no_hotplug   = 1;
>      k->init         = piix3_initfn;
>      k->config_write = piix3_write_config_xen;
> @@ -676,6 +679,11 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
>      /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
>      k->device_id    = PCI_DEVICE_ID_INTEL_82371SB_0;
>      k->class_id     = PCI_CLASS_BRIDGE_ISA;
> +    /*
> +     * Reason: part of PIIX3 southbridge, needs to be wired up by
> +     * pc_piix.c's pc_init1()
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  };
>  
>  static const TypeInfo piix3_xen_info = {
Markus Armbruster - Oct. 31, 2013, 10:29 a.m.
Marcel Apfelbaum <marcel.a@redhat.com> writes:

> On Wed, 2013-10-30 at 17:28 +0100, armbru@redhat.com wrote:
>> From: Markus Armbruster <armbru@redhat.com>
>> 
>> A PIIX3/PIIX4 southbridge has multiple functions.  We model each
>> function as a separate qdev.  Two of them need some special wiring set
>> up in pc_init1() or mips_malta_init() to work: the ISA bridge at 01.0,
>> and the SMBus controller at 01.3.
>> 
>> The IDE controller at 01.1 (piix3-ide, piix3-ide-xen, piix4-ide) has
>> always had cannot_instantiate_with_device_add_yet set, but there is no
>> obvious reason why device_add could not work for them.  Drop it.
> Question:
> piix3-ide (and probably piix4-ide) has io ports hard coded in
> pci_piix_init_ports.
> PIIX3/PIIX4 already has a piix3-ide that uses the io ports.
> Adding more piix3-ide devices would work? 
> What am I missing?

If I understand physical hardware correctly, the southbridge routes the
legacy ISA IDE ports to the first suitable device.  We don't model that
correctly, and end up with all IDE controller device models claiming
them.

There's a similar case in PATCH 09/10: i8042.  It also has hardcoded ISA
ports.  Nevertheless, I drop cannot_instantiate_with_device_add_yet with
the following rationale:

    * i8042: drop, even though its I/O base is hardcoded (because you
      could conceivably still add one to a board that has none) [...]

Same argument applies here: I figure you could add a piix3-ide to a
board that has no IDE controller.

General rule: when in doubt, cannot_instantiate_with_device_add_yet
stays off, because that's the way Anthony wants it (if I understand him
correctly).

> Another question:
> It seems that piix3-ide it was meant to be a function within PIIX3,

Correct.

> if so, we need a cannot_instantiate_with_device_add_ever?

I agree with you that there's something missing here, but I don't think
it's cannot_instantiate_with_device_add_ever :)

It would be nice if we'd model PCI functions and PCI devices properly.
If we did, we wouldn't let users plug a function into a PCI bus.
Instead, we'd let them combine functions into devices, and plug devices
into slots[*].  But it's not what we have now.

In the current state of affairs, we approximate "combine functions into
devices" by letting users plug a bunch of function device models into
the same PCI slot, with different function addresses.  That requires
cannot_instantiate_with_device_add_ever = false.

Users generally don't plug southbridge IDE controller functions, because
a board with PCI generally has a southbridge containing one already.
But as long as a user *could* plug it successfully into *some* board,
cannot_instantiate_with_device_add_ever stays off as per the general
rule above.

Previous discussion:
https://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg02000.html

> Hope it helped,

Thinking reviewers are always appreciated!


[*] Then hot plug of multi-function devices could be made to work.
Marcel Apfelbaum - Oct. 31, 2013, 1:12 p.m.
On Thu, 2013-10-31 at 11:29 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > On Wed, 2013-10-30 at 17:28 +0100, armbru@redhat.com wrote:
> >> From: Markus Armbruster <armbru@redhat.com>
> >> 
> >> A PIIX3/PIIX4 southbridge has multiple functions.  We model each
> >> function as a separate qdev.  Two of them need some special wiring set
> >> up in pc_init1() or mips_malta_init() to work: the ISA bridge at 01.0,
> >> and the SMBus controller at 01.3.
> >> 
> >> The IDE controller at 01.1 (piix3-ide, piix3-ide-xen, piix4-ide) has
> >> always had cannot_instantiate_with_device_add_yet set, but there is no
> >> obvious reason why device_add could not work for them.  Drop it.
> > Question:
> > piix3-ide (and probably piix4-ide) has io ports hard coded in
> > pci_piix_init_ports.
> > PIIX3/PIIX4 already has a piix3-ide that uses the io ports.
> > Adding more piix3-ide devices would work? 
> > What am I missing?
> 
> If I understand physical hardware correctly, the southbridge routes the
> legacy ISA IDE ports to the first suitable device.  We don't model that
> correctly, and end up with all IDE controller device models claiming
> them.
> 
> There's a similar case in PATCH 09/10: i8042.  It also has hardcoded ISA
> ports.  Nevertheless, I drop cannot_instantiate_with_device_add_yet with
> the following rationale:
> 
>     * i8042: drop, even though its I/O base is hardcoded (because you
>       could conceivably still add one to a board that has none) [...]
> 
> Same argument applies here: I figure you could add a piix3-ide to a
> board that has no IDE controller.
> 
> General rule: when in doubt, cannot_instantiate_with_device_add_yet
> stays off, because that's the way Anthony wants it (if I understand him
> correctly).
> 
> > Another question:
> > It seems that piix3-ide it was meant to be a function within PIIX3,
> 
> Correct.
> 
> > if so, we need a cannot_instantiate_with_device_add_ever?
> 
> I agree with you that there's something missing here, but I don't think
> it's cannot_instantiate_with_device_add_ever :)
> 
> It would be nice if we'd model PCI functions and PCI devices properly.
> If we did, we wouldn't let users plug a function into a PCI bus.
> Instead, we'd let them combine functions into devices, and plug devices
> into slots[*].  But it's not what we have now.
> 
> In the current state of affairs, we approximate "combine functions into
> devices" by letting users plug a bunch of function device models into
> the same PCI slot, with different function addresses.  That requires
> cannot_instantiate_with_device_add_ever = false.
> 
> Users generally don't plug southbridge IDE controller functions, because
> a board with PCI generally has a southbridge containing one already.
> But as long as a user *could* plug it successfully into *some* board,
> cannot_instantiate_with_device_add_ever stays off as per the general
> rule above.
Thanks for the explanation, with this rule in mind:

    Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>

Thanks,
Marcel

> 
> Previous discussion:
> https://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg02000.html
> 
> > Hope it helped,
> 
> Thinking reviewers are always appreciated!
> 
> 
> [*] Then hot plug of multi-function devices could be made to work.

Patch

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index c29a703..368a76b 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -508,9 +508,13 @@  static void piix4_pm_class_init(ObjectClass *klass, void *data)
     k->revision = 0x03;
     k->class_id = PCI_CLASS_BRIDGE_OTHER;
     dc->desc = "PM";
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     dc->vmsd = &vmstate_acpi;
     dc->props = piix4_pm_properties;
+    /*
+     * Reason: part of PIIX4 southbridge, needs to be wired up,
+     * e.g. by mips_malta_init()
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo piix4_pm_info = {
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 27b08e1..9b5960b 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -248,7 +248,6 @@  static void piix3_ide_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
 }
 
 static const TypeInfo piix3_ide_info = {
@@ -267,7 +266,6 @@  static void piix3_ide_xen_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     dc->unplug = pci_piix3_xen_ide_unplug;
 }
 
@@ -289,7 +287,6 @@  static void piix4_ide_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
 }
 
 static const TypeInfo piix4_ide_info = {
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index d9dac61..def6fe3 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -113,8 +113,12 @@  static void piix4_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
     k->class_id = PCI_CLASS_BRIDGE_ISA;
     dc->desc = "ISA bridge";
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     dc->vmsd = &vmstate_piix4;
+    /*
+     * Reason: part of PIIX4 southbridge, needs to be wired up,
+     * e.g. by mips_malta_init()
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo piix4_info = {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 8089fd6..1526fd4 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -644,7 +644,6 @@  static void piix3_class_init(ObjectClass *klass, void *data)
 
     dc->desc        = "ISA bridge";
     dc->vmsd        = &vmstate_piix3;
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     k->no_hotplug   = 1;
     k->init         = piix3_initfn;
     k->config_write = piix3_write_config;
@@ -652,6 +651,11 @@  static void piix3_class_init(ObjectClass *klass, void *data)
     /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
     k->device_id    = PCI_DEVICE_ID_INTEL_82371SB_0;
     k->class_id     = PCI_CLASS_BRIDGE_ISA;
+    /*
+     * Reason: part of PIIX3 southbridge, needs to be wired up by
+     * pc_piix.c's pc_init1()
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo piix3_info = {
@@ -668,7 +672,6 @@  static void piix3_xen_class_init(ObjectClass *klass, void *data)
 
     dc->desc        = "ISA bridge";
     dc->vmsd        = &vmstate_piix3;
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     k->no_hotplug   = 1;
     k->init         = piix3_initfn;
     k->config_write = piix3_write_config_xen;
@@ -676,6 +679,11 @@  static void piix3_xen_class_init(ObjectClass *klass, void *data)
     /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
     k->device_id    = PCI_DEVICE_ID_INTEL_82371SB_0;
     k->class_id     = PCI_CLASS_BRIDGE_ISA;
+    /*
+     * Reason: part of PIIX3 southbridge, needs to be wired up by
+     * pc_piix.c's pc_init1()
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
 };
 
 static const TypeInfo piix3_xen_info = {