Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-12-01 Thread Eduardo Habkost 
On Tue, Dec 01, 2015 at 10:53:07PM +0200, Marcel Apfelbaum wrote:
> On 12/01/2015 08:20 PM, Eduardo Habkost wrote:
> >On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> >>Add bus property to PC machines and use it when looking
> >>for primary PCI root bus (bus 0).
> >>
> >>Signed-off-by: Marcel Apfelbaum 
> >>---
> >>  hw/i386/acpi-build.c | 3 +--
> >>  hw/i386/pc.c | 2 +-
> >>  hw/i386/pc_piix.c| 1 +
> >>  hw/i386/pc_q35.c | 1 +
> >>  include/hw/i386/pc.h | 1 +
> >>  5 files changed, 5 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>index 736b252..bca3f06 100644
> >>--- a/hw/i386/acpi-build.c
> >>+++ b/hw/i386/acpi-build.c
> >>@@ -950,8 +950,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>  /* Reserve space for header */
> >>  acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> >>
> >>-/* Extra PCI root buses are implemented  only for i440fx */
> >>-bus = find_i440fx();
> >>+bus = PC_MACHINE(machine)->bus;
> >
> >You can use acpi_get_i386_pci_host()->bus here, so we can reduce
> >the amount of PC-specific code inside acpi-build.c.
> >
> >(Making acpi_get_i386_pci_host() more generic and not depend on
> >piix- and q35-specific checks is also on my plans)
> 
> Well, at this point, looking at the names PC_MACHINE and 
> acpi_get_i386_pci_host,
> I don't see much difference :)

I'm a bit confused by how generic acpi-build.c is supposed to be.
It has some code that seems to be an attempt to be more generic
(e.g. the "if (pci_host)" check at build_ssdt()), but lots of
other code that only work with PC machines. Maybe we should stop
pretending and simply use PCMachineState everywhere inside
acpi-build.c?

> I think we can change this later when acpi_get_i386_pci_host will be generic.

No problem to me. :)

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-12-01 Thread Marcel Apfelbaum 

On 12/01/2015 08:20 PM, Eduardo Habkost wrote:

On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:

Add bus property to PC machines and use it when looking
for primary PCI root bus (bus 0).

Signed-off-by: Marcel Apfelbaum 
---
  hw/i386/acpi-build.c | 3 +--
  hw/i386/pc.c | 2 +-
  hw/i386/pc_piix.c| 1 +
  hw/i386/pc_q35.c | 1 +
  include/hw/i386/pc.h | 1 +
  5 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 736b252..bca3f06 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -950,8 +950,7 @@ build_ssdt(GArray *table_data, GArray *linker,
  /* Reserve space for header */
  acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));

-/* Extra PCI root buses are implemented  only for i440fx */
-bus = find_i440fx();
+bus = PC_MACHINE(machine)->bus;


You can use acpi_get_i386_pci_host()->bus here, so we can reduce
the amount of PC-specific code inside acpi-build.c.

(Making acpi_get_i386_pci_host() more generic and not depend on
piix- and q35-specific checks is also on my plans)


Well, at this point, looking at the names PC_MACHINE and acpi_get_i386_pci_host,
I don't see much difference :)
I think we can change this later when acpi_get_i386_pci_host will be generic.

Thanks,
Marcel




  if (bus) {
  QLIST_FOREACH(bus, >child, sibling) {
  uint8_t bus_num = pci_bus_num(bus);

[...]

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-12-01 Thread Eduardo Habkost 
On Tue, Dec 01, 2015 at 06:50:15PM +0200, Marcel Apfelbaum wrote:
> On 12/01/2015 05:09 PM, Eduardo Habkost wrote:
> >On Tue, Dec 01, 2015 at 04:55:57PM +0200, Marcel Apfelbaum wrote:
> >>On 12/01/2015 04:48 PM, Eduardo Habkost wrote:
> >>>On Tue, Dec 01, 2015 at 04:07:33PM +0200, Marcel Apfelbaum wrote:
> On 11/30/2015 05:07 PM, Eduardo Habkost wrote:
> >On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:
> >>On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
> >>>On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> Add bus property to PC machines and use it when looking
> for primary PCI root bus (bus 0).
> 
> Signed-off-by: Marcel Apfelbaum 
> >>>
> >>>I can't pretend I have reviewed the q35 part, but the changes are
> >>>an improvement to the existing code that depended on
> >>>find_i440fx().
> >>>
> >>>Acked-by: Eduardo Habkost 
> >>
> >>Thanks!
> >>
> >>>
> >>>BTW, what's missing to allow us to change acpi_set_pci_info() to
> >>>use PCMachine::bus instead of find_i440fx(), too? How much of the
> >>>PCI hotplug stuff is different in q35?
> >>
> >>It is pretty different.
> >>i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since 
> >>is
> >>"native", no acpi info is necessary.
> >>
> >>Having said that, if we have an PCIe-PCI bridge, the pci devices behind 
> >>it
> >>cannot be hotplugged/unplugged right now.
> >>
> >>Once we decide to add hotplug support for this scenario, maybe we can 
> >>get rid of
> >>find_i440fx().
> >
> >Thanks for the explanation. I wonder if there's a better way to
> >check if ACPI-based hotplug is needed by looking at
> >PCMachineState or PCIBus, so we don't couple the ACPI code to
> >piix.c.
> >
> 
> I suppose we can do something about it, like adding a property to 
> PCMachineState,
> lets say bool acpi_hotplug and set it false for Q35.
> 
> Then we have:
>  pcm = PC_MACHINE(current_machine);
>  if(pcm->acpi_hotplug) {
>  bus  = pcm->bus;
>  ...
>  }
> 
> Sounds acceptable? If yes, I'll send a patch on top since is not directly 
> related.S
> >>>
> >>>There's no existing field or method in PCIBus that can be already
> >>>used for that?
> >>
> >>Hmm, you can derive the info you need from pci_bus_is_express.
> >>If express-> no acpi_hotplug. This is not 100% true, but since
> >>we don't support acpi hotplug on PCIe machines, it should be OK for now.
> >
> >What about just checking if AcpiPmInfo.pcihp_io_base is set?
> >
> 
> Because this contradicts the "do not probe for piix" requirement.
> pcihp_io_base depends on piix query for pm (piix4_pm_find).
> So pcihp_io_base is an i440fx only "artifact".
> 

Yes, but at least the piix4-specific code would be contained
inside acpi_get_pm_info(). (And making acpi_get_pm_info() generic
is also part of my plans.)

However, you have a good point:

> Also, acpi_set_pci_info is called before acpi_build that populates
> acpi_get_pm_info. All of that can be taken care of, of course.

So we need to be careful about ordering, there. But it looks
doable without adding yet another PCMachineState field.

> 
> At the end of the day, as long as the functionality is preserved,
> I personally have no objection in re-factoring.

Working on it. :)

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-12-01 Thread Marcel Apfelbaum 

On 12/01/2015 05:09 PM, Eduardo Habkost wrote:

On Tue, Dec 01, 2015 at 04:55:57PM +0200, Marcel Apfelbaum wrote:

On 12/01/2015 04:48 PM, Eduardo Habkost wrote:

On Tue, Dec 01, 2015 at 04:07:33PM +0200, Marcel Apfelbaum wrote:

On 11/30/2015 05:07 PM, Eduardo Habkost wrote:

On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:

On 11/27/2015 07:28 PM, Eduardo Habkost wrote:

On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:

Add bus property to PC machines and use it when looking
for primary PCI root bus (bus 0).

Signed-off-by: Marcel Apfelbaum 


I can't pretend I have reviewed the q35 part, but the changes are
an improvement to the existing code that depended on
find_i440fx().

Acked-by: Eduardo Habkost 


Thanks!



BTW, what's missing to allow us to change acpi_set_pci_info() to
use PCMachine::bus instead of find_i440fx(), too? How much of the
PCI hotplug stuff is different in q35?


It is pretty different.
i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
"native", no acpi info is necessary.

Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
cannot be hotplugged/unplugged right now.

Once we decide to add hotplug support for this scenario, maybe we can get rid of
find_i440fx().


Thanks for the explanation. I wonder if there's a better way to
check if ACPI-based hotplug is needed by looking at
PCMachineState or PCIBus, so we don't couple the ACPI code to
piix.c.



I suppose we can do something about it, like adding a property to 
PCMachineState,
lets say bool acpi_hotplug and set it false for Q35.

Then we have:
 pcm = PC_MACHINE(current_machine);
 if(pcm->acpi_hotplug) {
 bus  = pcm->bus;
 ...
 }

Sounds acceptable? If yes, I'll send a patch on top since is not directly 
related.S


There's no existing field or method in PCIBus that can be already
used for that?


Hmm, you can derive the info you need from pci_bus_is_express.
If express-> no acpi_hotplug. This is not 100% true, but since
we don't support acpi hotplug on PCIe machines, it should be OK for now.


What about just checking if AcpiPmInfo.pcihp_io_base is set?



Because this contradicts the "do not probe for piix" requirement.
pcihp_io_base depends on piix query for pm (piix4_pm_find).
So pcihp_io_base is an i440fx only "artifact".

Also, acpi_set_pci_info is called before acpi_build that populates
acpi_get_pm_info. All of that can be taken care of, of course.

At the end of the day, as long as the functionality is preserved,
I personally have no objection in re-factoring.

Thanks,
Marcel

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-12-01 Thread Eduardo Habkost 
On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> Add bus property to PC machines and use it when looking
> for primary PCI root bus (bus 0).
> 
> Signed-off-by: Marcel Apfelbaum 
> ---
>  hw/i386/acpi-build.c | 3 +--
>  hw/i386/pc.c | 2 +-
>  hw/i386/pc_piix.c| 1 +
>  hw/i386/pc_q35.c | 1 +
>  include/hw/i386/pc.h | 1 +
>  5 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 736b252..bca3f06 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -950,8 +950,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>  /* Reserve space for header */
>  acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
>  
> -/* Extra PCI root buses are implemented  only for i440fx */
> -bus = find_i440fx();
> +bus = PC_MACHINE(machine)->bus;

You can use acpi_get_i386_pci_host()->bus here, so we can reduce
the amount of PC-specific code inside acpi-build.c.

(Making acpi_get_i386_pci_host() more generic and not depend on
piix- and q35-specific checks is also on my plans)

>  if (bus) {
>  QLIST_FOREACH(bus, >child, sibling) {
>  uint8_t bus_num = pci_bus_num(bus);
[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-12-01 Thread Eduardo Habkost 
On Tue, Dec 01, 2015 at 04:55:57PM +0200, Marcel Apfelbaum wrote:
> On 12/01/2015 04:48 PM, Eduardo Habkost wrote:
> >On Tue, Dec 01, 2015 at 04:07:33PM +0200, Marcel Apfelbaum wrote:
> >>On 11/30/2015 05:07 PM, Eduardo Habkost wrote:
> >>>On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:
> On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
> >On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> >>Add bus property to PC machines and use it when looking
> >>for primary PCI root bus (bus 0).
> >>
> >>Signed-off-by: Marcel Apfelbaum 
> >
> >I can't pretend I have reviewed the q35 part, but the changes are
> >an improvement to the existing code that depended on
> >find_i440fx().
> >
> >Acked-by: Eduardo Habkost 
> 
> Thanks!
> 
> >
> >BTW, what's missing to allow us to change acpi_set_pci_info() to
> >use PCMachine::bus instead of find_i440fx(), too? How much of the
> >PCI hotplug stuff is different in q35?
> 
> It is pretty different.
> i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
> "native", no acpi info is necessary.
> 
> Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
> cannot be hotplugged/unplugged right now.
> 
> Once we decide to add hotplug support for this scenario, maybe we can get 
> rid of
> find_i440fx().
> >>>
> >>>Thanks for the explanation. I wonder if there's a better way to
> >>>check if ACPI-based hotplug is needed by looking at
> >>>PCMachineState or PCIBus, so we don't couple the ACPI code to
> >>>piix.c.
> >>>
> >>
> >>I suppose we can do something about it, like adding a property to 
> >>PCMachineState,
> >>lets say bool acpi_hotplug and set it false for Q35.
> >>
> >>Then we have:
> >> pcm = PC_MACHINE(current_machine);
> >> if(pcm->acpi_hotplug) {
> >> bus  = pcm->bus;
> >> ...
> >> }
> >>
> >>Sounds acceptable? If yes, I'll send a patch on top since is not directly 
> >>related.S
> >
> >There's no existing field or method in PCIBus that can be already
> >used for that?
> 
> Hmm, you can derive the info you need from pci_bus_is_express.
> If express-> no acpi_hotplug. This is not 100% true, but since
> we don't support acpi hotplug on PCIe machines, it should be OK for now.

What about just checking if AcpiPmInfo.pcihp_io_base is set?

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-12-01 Thread Marcel Apfelbaum 

On 12/01/2015 04:48 PM, Eduardo Habkost wrote:

On Tue, Dec 01, 2015 at 04:07:33PM +0200, Marcel Apfelbaum wrote:

On 11/30/2015 05:07 PM, Eduardo Habkost wrote:

On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:

On 11/27/2015 07:28 PM, Eduardo Habkost wrote:

On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:

Add bus property to PC machines and use it when looking
for primary PCI root bus (bus 0).

Signed-off-by: Marcel Apfelbaum 


I can't pretend I have reviewed the q35 part, but the changes are
an improvement to the existing code that depended on
find_i440fx().

Acked-by: Eduardo Habkost 


Thanks!



BTW, what's missing to allow us to change acpi_set_pci_info() to
use PCMachine::bus instead of find_i440fx(), too? How much of the
PCI hotplug stuff is different in q35?


It is pretty different.
i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
"native", no acpi info is necessary.

Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
cannot be hotplugged/unplugged right now.

Once we decide to add hotplug support for this scenario, maybe we can get rid of
find_i440fx().


Thanks for the explanation. I wonder if there's a better way to
check if ACPI-based hotplug is needed by looking at
PCMachineState or PCIBus, so we don't couple the ACPI code to
piix.c.



I suppose we can do something about it, like adding a property to 
PCMachineState,
lets say bool acpi_hotplug and set it false for Q35.

Then we have:
 pcm = PC_MACHINE(current_machine);
 if(pcm->acpi_hotplug) {
 bus  = pcm->bus;
 ...
 }

Sounds acceptable? If yes, I'll send a patch on top since is not directly 
related.S


There's no existing field or method in PCIBus that can be already
used for that?


Hmm, you can derive the info you need from pci_bus_is_express.
If express-> no acpi_hotplug. This is not 100% true, but since
we don't support acpi hotplug on PCIe machines, it should be OK for now.

 If not, that sounds good to me. But I would move

the field to PCMachineClass instead of PCMachineState.


Sure.



Would you like me to do it? I am planning to submit other changes
to remove q35/piix dependencies from acpi-build.c.



If you have the time, go for it :)
Is a "one liner", if applied on top of this series.

Thanks,
Marcel

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-12-01 Thread Eduardo Habkost 
On Tue, Dec 01, 2015 at 04:07:33PM +0200, Marcel Apfelbaum wrote:
> On 11/30/2015 05:07 PM, Eduardo Habkost wrote:
> >On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:
> >>On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
> >>>On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> Add bus property to PC machines and use it when looking
> for primary PCI root bus (bus 0).
> 
> Signed-off-by: Marcel Apfelbaum 
> >>>
> >>>I can't pretend I have reviewed the q35 part, but the changes are
> >>>an improvement to the existing code that depended on
> >>>find_i440fx().
> >>>
> >>>Acked-by: Eduardo Habkost 
> >>
> >>Thanks!
> >>
> >>>
> >>>BTW, what's missing to allow us to change acpi_set_pci_info() to
> >>>use PCMachine::bus instead of find_i440fx(), too? How much of the
> >>>PCI hotplug stuff is different in q35?
> >>
> >>It is pretty different.
> >>i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
> >>"native", no acpi info is necessary.
> >>
> >>Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
> >>cannot be hotplugged/unplugged right now.
> >>
> >>Once we decide to add hotplug support for this scenario, maybe we can get 
> >>rid of
> >>find_i440fx().
> >
> >Thanks for the explanation. I wonder if there's a better way to
> >check if ACPI-based hotplug is needed by looking at
> >PCMachineState or PCIBus, so we don't couple the ACPI code to
> >piix.c.
> >
> 
> I suppose we can do something about it, like adding a property to 
> PCMachineState,
> lets say bool acpi_hotplug and set it false for Q35.
> 
> Then we have:
> pcm = PC_MACHINE(current_machine);
> if(pcm->acpi_hotplug) {
> bus  = pcm->bus;
> ...
> }
> 
> Sounds acceptable? If yes, I'll send a patch on top since is not directly 
> related.S

There's no existing field or method in PCIBus that can be already
used for that? If not, that sounds good to me. But I would move
the field to PCMachineClass instead of PCMachineState.

Would you like me to do it? I am planning to submit other changes
to remove q35/piix dependencies from acpi-build.c.

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-12-01 Thread Marcel Apfelbaum 

On 11/30/2015 05:07 PM, Eduardo Habkost wrote:

On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:

On 11/27/2015 07:28 PM, Eduardo Habkost wrote:

On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:

Add bus property to PC machines and use it when looking
for primary PCI root bus (bus 0).

Signed-off-by: Marcel Apfelbaum 


I can't pretend I have reviewed the q35 part, but the changes are
an improvement to the existing code that depended on
find_i440fx().

Acked-by: Eduardo Habkost 


Thanks!



BTW, what's missing to allow us to change acpi_set_pci_info() to
use PCMachine::bus instead of find_i440fx(), too? How much of the
PCI hotplug stuff is different in q35?


It is pretty different.
i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
"native", no acpi info is necessary.

Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
cannot be hotplugged/unplugged right now.

Once we decide to add hotplug support for this scenario, maybe we can get rid of
find_i440fx().


Thanks for the explanation. I wonder if there's a better way to
check if ACPI-based hotplug is needed by looking at
PCMachineState or PCIBus, so we don't couple the ACPI code to
piix.c.



I suppose we can do something about it, like adding a property to 
PCMachineState,
lets say bool acpi_hotplug and set it false for Q35.

Then we have:
pcm = PC_MACHINE(current_machine);
if(pcm->acpi_hotplug) {
bus  = pcm->bus;
...
}

Sounds acceptable? If yes, I'll send a patch on top since is not directly 
related.

Thanks,
Marcel

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-11-30 Thread Eduardo Habkost 
On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:
> On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
> >On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> >>Add bus property to PC machines and use it when looking
> >>for primary PCI root bus (bus 0).
> >>
> >>Signed-off-by: Marcel Apfelbaum 
> >
> >I can't pretend I have reviewed the q35 part, but the changes are
> >an improvement to the existing code that depended on
> >find_i440fx().
> >
> >Acked-by: Eduardo Habkost 
> 
> Thanks!
> 
> >
> >BTW, what's missing to allow us to change acpi_set_pci_info() to
> >use PCMachine::bus instead of find_i440fx(), too? How much of the
> >PCI hotplug stuff is different in q35?
> 
> It is pretty different.
> i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
> "native", no acpi info is necessary.
> 
> Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
> cannot be hotplugged/unplugged right now.
> 
> Once we decide to add hotplug support for this scenario, maybe we can get rid 
> of
> find_i440fx().

Thanks for the explanation. I wonder if there's a better way to
check if ACPI-based hotplug is needed by looking at
PCMachineState or PCIBus, so we don't couple the ACPI code to
piix.c.

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-11-29 Thread Marcel Apfelbaum 

On 11/27/2015 07:28 PM, Eduardo Habkost wrote:

On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:

Add bus property to PC machines and use it when looking
for primary PCI root bus (bus 0).

Signed-off-by: Marcel Apfelbaum 


I can't pretend I have reviewed the q35 part, but the changes are
an improvement to the existing code that depended on
find_i440fx().

Acked-by: Eduardo Habkost 


Thanks!



BTW, what's missing to allow us to change acpi_set_pci_info() to
use PCMachine::bus instead of find_i440fx(), too? How much of the
PCI hotplug stuff is different in q35?


It is pretty different.
i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
"native", no acpi info is necessary.

Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
cannot be hotplugged/unplugged right now.

Once we decide to add hotplug support for this scenario, maybe we can get rid of
find_i440fx().

Thanks,
Marcel




---
  hw/i386/acpi-build.c | 3 +--
  hw/i386/pc.c | 2 +-
  hw/i386/pc_piix.c| 1 +
  hw/i386/pc_q35.c | 1 +
  include/hw/i386/pc.h | 1 +
  5 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 736b252..bca3f06 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -950,8 +950,7 @@ build_ssdt(GArray *table_data, GArray *linker,
  /* Reserve space for header */
  acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));

-/* Extra PCI root buses are implemented  only for i440fx */
-bus = find_i440fx();
+bus = PC_MACHINE(machine)->bus;
  if (bus) {
  QLIST_FOREACH(bus, >child, sibling) {
  uint8_t bus_num = pci_bus_num(bus);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5e20e07..07697ed 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1174,7 +1174,7 @@ void pc_guest_info_machine_done(Notifier *notifier, void 
*data)
  PcGuestInfoState *guest_info_state = container_of(notifier,
PcGuestInfoState,
machine_done);
-PCIBus *bus = find_i440fx();
+PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;

  if (bus) {
  int extra_hosts = 0;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 07d0baa..ca6ef9a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -197,6 +197,7 @@ static void pc_init1(MachineState *machine,
pcms->below_4g_mem_size,
pcms->above_4g_mem_size,
pci_memory, ram_memory);
+pcms->bus = pci_bus;
  } else {
  pci_bus = NULL;
  i440fx_state = NULL;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0fdae09..822b3e5 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -192,6 +192,7 @@ static void pc_q35_init(MachineState *machine)
  qdev_init_nofail(DEVICE(q35_host));
  phb = PCI_HOST_BRIDGE(q35_host);
  host_bus = phb->bus;
+pcms->bus = phb->bus;
  /* create ISA bus */
  lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
ICH9_LPC_FUNC), true,
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 854c330..e42771c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -41,6 +41,7 @@ struct PCMachineState {
  OnOffAuto smm;
  bool enforce_aligned_dimm;
  ram_addr_t below_4g_mem_size, above_4g_mem_size;
+PCIBus *bus;
  };

  #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
--
2.1.0

Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-11-27 Thread Eduardo Habkost 
On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
> Add bus property to PC machines and use it when looking
> for primary PCI root bus (bus 0).
> 
> Signed-off-by: Marcel Apfelbaum 

I can't pretend I have reviewed the q35 part, but the changes are
an improvement to the existing code that depended on
find_i440fx().

Acked-by: Eduardo Habkost 

BTW, what's missing to allow us to change acpi_set_pci_info() to
use PCMachine::bus instead of find_i440fx(), too? How much of the
PCI hotplug stuff is different in q35?

> ---
>  hw/i386/acpi-build.c | 3 +--
>  hw/i386/pc.c | 2 +-
>  hw/i386/pc_piix.c| 1 +
>  hw/i386/pc_q35.c | 1 +
>  include/hw/i386/pc.h | 1 +
>  5 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 736b252..bca3f06 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -950,8 +950,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>  /* Reserve space for header */
>  acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
>  
> -/* Extra PCI root buses are implemented  only for i440fx */
> -bus = find_i440fx();
> +bus = PC_MACHINE(machine)->bus;
>  if (bus) {
>  QLIST_FOREACH(bus, >child, sibling) {
>  uint8_t bus_num = pci_bus_num(bus);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 5e20e07..07697ed 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1174,7 +1174,7 @@ void pc_guest_info_machine_done(Notifier *notifier, 
> void *data)
>  PcGuestInfoState *guest_info_state = container_of(notifier,
>PcGuestInfoState,
>machine_done);
> -PCIBus *bus = find_i440fx();
> +PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
>  
>  if (bus) {
>  int extra_hosts = 0;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 07d0baa..ca6ef9a 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -197,6 +197,7 @@ static void pc_init1(MachineState *machine,
>pcms->below_4g_mem_size,
>pcms->above_4g_mem_size,
>pci_memory, ram_memory);
> +pcms->bus = pci_bus;
>  } else {
>  pci_bus = NULL;
>  i440fx_state = NULL;
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 0fdae09..822b3e5 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -192,6 +192,7 @@ static void pc_q35_init(MachineState *machine)
>  qdev_init_nofail(DEVICE(q35_host));
>  phb = PCI_HOST_BRIDGE(q35_host);
>  host_bus = phb->bus;
> +pcms->bus = phb->bus;
>  /* create ISA bus */
>  lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
>ICH9_LPC_FUNC), true,
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 854c330..e42771c 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -41,6 +41,7 @@ struct PCMachineState {
>  OnOffAuto smm;
>  bool enforce_aligned_dimm;
>  ram_addr_t below_4g_mem_size, above_4g_mem_size;
> +PCIBus *bus;
>  };
>  
>  #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> -- 
> 2.1.0
> 

-- 
Eduardo

[Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

2015-11-26 Thread Marcel Apfelbaum 
Add bus property to PC machines and use it when looking
for primary PCI root bus (bus 0).

Signed-off-by: Marcel Apfelbaum 
---
 hw/i386/acpi-build.c | 3 +--
 hw/i386/pc.c | 2 +-
 hw/i386/pc_piix.c| 1 +
 hw/i386/pc_q35.c | 1 +
 include/hw/i386/pc.h | 1 +
 5 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 736b252..bca3f06 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -950,8 +950,7 @@ build_ssdt(GArray *table_data, GArray *linker,
 /* Reserve space for header */
 acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
 
-/* Extra PCI root buses are implemented  only for i440fx */
-bus = find_i440fx();
+bus = PC_MACHINE(machine)->bus;
 if (bus) {
 QLIST_FOREACH(bus, >child, sibling) {
 uint8_t bus_num = pci_bus_num(bus);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5e20e07..07697ed 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1174,7 +1174,7 @@ void pc_guest_info_machine_done(Notifier *notifier, void 
*data)
 PcGuestInfoState *guest_info_state = container_of(notifier,
   PcGuestInfoState,
   machine_done);
-PCIBus *bus = find_i440fx();
+PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
 
 if (bus) {
 int extra_hosts = 0;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 07d0baa..ca6ef9a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -197,6 +197,7 @@ static void pc_init1(MachineState *machine,
   pcms->below_4g_mem_size,
   pcms->above_4g_mem_size,
   pci_memory, ram_memory);
+pcms->bus = pci_bus;
 } else {
 pci_bus = NULL;
 i440fx_state = NULL;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0fdae09..822b3e5 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -192,6 +192,7 @@ static void pc_q35_init(MachineState *machine)
 qdev_init_nofail(DEVICE(q35_host));
 phb = PCI_HOST_BRIDGE(q35_host);
 host_bus = phb->bus;
+pcms->bus = phb->bus;
 /* create ISA bus */
 lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
   ICH9_LPC_FUNC), true,
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 854c330..e42771c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -41,6 +41,7 @@ struct PCMachineState {
 OnOffAuto smm;
 bool enforce_aligned_dimm;
 ram_addr_t below_4g_mem_size, above_4g_mem_size;
+PCIBus *bus;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
-- 
2.1.0