[1/2] piix: Set I440FXState member pci_info.w32 in one place

Range pci_info.w32 records the location of the PCI hole.

It's initialized to empty when QOM zeroes I440FXState.  That's a fine
value for a still unknown PCI hole.

i440fx_init() sets pci_info.w32.begin = below_4g_mem_size.  Changes
the PCI hole from empty to [below_4g_mem_size, UINT64_MAX].  That's a
bogus value.

i440fx_pcihost_initfn() sets pci_info.end = IO_APIC_DEFAULT_ADDRESS.
Since i440fx_init() ran already, this changes the PCI hole to
[below_4g_mem_size, IO_APIC_DEFAULT_ADDRESS-1].  That's the correct
value.

Setting the bounds of the PCI hole in two separate places is
confusing, and begs the question whether the bogus intermediate value
could be used by something, or what would happen if we somehow managed
to realize an i440FX device without having run the board init function
i440fx_init() first.

Avoid the confusion by setting the (constant) upper bound along with
the lower bound in i440fx_init().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/pci-host/piix.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

On 06/15/2016 11:56 AM, Markus Armbruster wrote:
> Range pci_info.w32 records the location of the PCI hole.
> 
> It's initialized to empty when QOM zeroes I440FXState.  That's a fine
> value for a still unknown PCI hole.
> 
> i440fx_init() sets pci_info.w32.begin = below_4g_mem_size.  Changes
> the PCI hole from empty to [below_4g_mem_size, UINT64_MAX].  That's a
> bogus value.
> 
> i440fx_pcihost_initfn() sets pci_info.end = IO_APIC_DEFAULT_ADDRESS.
> Since i440fx_init() ran already, this changes the PCI hole to
> [below_4g_mem_size, IO_APIC_DEFAULT_ADDRESS-1].  That's the correct
> value.
> 
> Setting the bounds of the PCI hole in two separate places is
> confusing, and begs the question whether the bogus intermediate value
> could be used by something, or what would happen if we somehow managed
> to realize an i440FX device without having run the board init function
> i440fx_init() first.
> 
> Avoid the confusion by setting the (constant) upper bound along with
> the lower bound in i440fx_init().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/pci-host/piix.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

On 06/15/2016 08:56 PM, Markus Armbruster wrote:
> Range pci_info.w32 records the location of the PCI hole.
>
> It's initialized to empty when QOM zeroes I440FXState.  That's a fine
> value for a still unknown PCI hole.
>
> i440fx_init() sets pci_info.w32.begin = below_4g_mem_size.  Changes
> the PCI hole from empty to [below_4g_mem_size, UINT64_MAX].  That's a
> bogus value.
>
> i440fx_pcihost_initfn() sets pci_info.end = IO_APIC_DEFAULT_ADDRESS.
> Since i440fx_init() ran already, this changes the PCI hole to
> [below_4g_mem_size, IO_APIC_DEFAULT_ADDRESS-1].  That's the correct
> value.
>
> Setting the bounds of the PCI hole in two separate places is
> confusing, and begs the question whether the bogus intermediate value
> could be used by something, or what would happen if we somehow managed
> to realize an i440FX device without having run the board init function
> i440fx_init() first.
>
> Avoid the confusion by setting the (constant) upper bound along with
> the lower bound in i440fx_init().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/pci-host/piix.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index df2b0e2..c63c424 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -263,7 +263,6 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
>   static void i440fx_pcihost_initfn(Object *obj)
>   {
>       PCIHostState *s = PCI_HOST_BRIDGE(obj);
> -    I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
>
>       memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
>                             "pci-conf-idx", 4);
> @@ -285,8 +284,6 @@ static void i440fx_pcihost_initfn(Object *obj)
>       object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "int",
>                           i440fx_pcihost_get_pci_hole64_end,
>                           NULL, NULL, NULL, NULL);
> -
> -    d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
>   }
>
>   static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
> @@ -348,6 +345,7 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
>
>       i440fx = I440FX_PCI_HOST_BRIDGE(dev);
>       i440fx->pci_info.w32.begin = below_4g_mem_size;
> +    i440fx->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
>
>       /* setup pci memory mapping */
>       pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
>

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

Thanks,
Marcel