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

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 04:55:33PM +0300, Avi Kivity wrote:
> 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

To make sure: 0 size is ok?

> but unreadable.

variables will make it clearer.

base = pci_bridge_get_base(bridge, type);
limit = pci_bridge_get_limit(bridge, type);
return limit >= bridge ? limit - base + 1 : 0;


>  Doesn't work for limit == 2^64-1, is this a
> possible value?

You mean when base == 0?  Yes, but what can I do?
This seems to be an API limitation of the memory API:
memory_region_init(sec_bus->address_space_mem, "pci_pridge_pci", INT64_MAX);
and the same for aliases: max size seems to be INT64_MAX,
the last byte isn't covered.

The only way to fix this would be to pass first/last instead
of offset/size. Too late for such an API change?

> >+
> >+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?

Yes.
These are controlled by VGA Enable and VGA palette snooping enable
bits in the bridge, which we don't yet implement. It's on the TODO,
at the moment vga devices behind a bridge don't work.
the relevant bits from the spec:

------------------

The VGA Enable bit in the Bridge Control register (see Section 3.2.5.18)
is used to control response by the bridge to both the VGA frame buffer
addresses and to the VGA register addresses. When a VGA compatible
device is located downstream of a PCI-to-PCI bridge, the VGA Enable bit
must be set. When set, the bridge will positively decode and forward
memory accesses to VGA frame buffer addresses and I/O accesses to VGA
registers from the primary to secondary interface and block forwarding
of these same accesses from the secondary to primary interface (see
Section 4.5.1).

VGA memory addresses:
0A 0000h through 0B FFFFh
VGA I/O addresses (including ISA aliases address - AD[15::10] are not
decoded):
AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh

We also may need to foward palette snooping accesses:

behaviors of each device type are described below.
The VGA palette addresses are as follows (inclusive of ISA aliases -
AD[15::10] are not decoded):
AD[9::0] = 3C6h, 3C8h, and 3C9h

------------------


> >+
> >+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)

OK, I'll stick them here.

> >+
> >+#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