Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity <avi@redhat.com>]

On 09/04/2011 04:41 PM, Michael S. Tsirkin wrote:
> On Sun, Sep 04, 2011 at 04:05:14PM +0300, Avi Kivity wrote:
> >  It follows naturally:
>
> OK, so it seems the following is more or less what you suggest?
> I'm not sure I create/destroy subregions properly.
> Both the alias and the subregion get the same start value?

Yes (so addresses are not shifted).

> Is the region name for debugging only?

For everything except RAM regions (there, the name is also used for 
save/restore).

> When does priority matter? In case of overlap?

Yes.  In this case, since overlap resolution is not defined by the spec, 
the actual priority does not matter.

> @@ -135,6 +135,75 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
>       return limit;
>   }
>
> +static pcibus_t pci_bridge_get_size(const PCIDevice *bridge, uint8_t type)
> +{
> +    return pci_bridge_get_limit(bridge, type)>=
> +        pci_bridge_get_base(bridge, type) ?
> +        pci_bridge_get_limit(bridge, type) -
> +        pci_bridge_get_base(bridge, type)  + 1 : 0;
> +}

Correct but unreadable.  Doesn't work for limit == 2^64-1, is this a 
possible value?

> +
> +static void pci_bridge_region_init(PCIBridge *br)
> +{
> +    PCIBus *sec_bus =&br->sec_bus;
> +    PCIBus *parent = br->dev.bus;
> +    memory_region_init_alias(sec_bus->alias_pref_mem, "pci_bridge_pref_mem",
> +	     sec_bus->address_space_mem,
> +	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH),
> +	     pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH));
> +    memory_region_add_subregion_overlap(parent->address_space_mem,
> +	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH),
> +             sec_bus->alias_pref_mem, 1);
> +    memory_region_init_alias(sec_bus->alias_mem, "pci_bridge_memory",
> +	     sec_bus->address_space_mem,
> +	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY),
> +	     pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY));
> +    memory_region_add_subregion_overlap(parent->address_space_mem,
> +	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY),
> +             sec_bus->alias_mem, 1);
> +    memory_region_init_alias(sec_bus->alias_io, "pci_bridge_io",
> +	     sec_bus->address_space_io,
> +	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_IO),
> +	     pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_IO));
> +    memory_region_add_subregion_overlap(parent->address_space_io,
> +	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_IO),
> +             sec_bus->alias_io, 1);
> +}

This looks right.  Might want to use pci_address_space() instead of 
->address_space_mem.

Don't you have to do something similar for the vga window?

> +
> +static void pci_bridge_region_cleanup(PCIBridge *br)
> +{
> +    PCIBus *sec_bus =&br->sec_bus;
> +    PCIBus *parent = br->dev.bus;
> +    memory_region_del_subregion(parent->address_space_mem,
> +                                sec_bus->alias_pref_mem);
> +    memory_region_destroy(sec_bus->alias_pref_mem);
> +    memory_region_del_subregion(parent->address_space_mem,
> +                                sec_bus->alias_mem);
> +    memory_region_destroy(sec_bus->alias_mem);
> +    memory_region_del_subregion(parent->address_space_io,
> +                                sec_bus->alias_io);
> +    memory_region_destroy(sec_bus->alias_io);
> +}

This is fine too.

> +
> +static void pci_bridge_update_mappings(PCIBridge *br)
> +{
> +    /* TODO: this doesn't handle the case of one VCPU
> +     * updating the bridge while another accesses an unaffected
> +     * region. To fix we'll need new memory region APIs. */
> +    pci_bridge_region_cleanup(br);
> +    pci_bridge_region_init(br);

memory_region_transaction_{begin,commit}()

(isn't 100% implemented, but at least the API is in place)
> +
> +#if 0
> +    TODO: do we need to propagate updates to child buses?
> +
> +    pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn);
> +
> +    QLIST_FOREACH(child,&b->child, sibling) {
> +        pci_bridge_update_mappings(child);
> +    }
> +#endif
> +}

Don't need this.

-- 
error compiling committee.c: too many arguments to function