| Message ID | 20110904134101.GA27239@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
On 09/04/2011 05:21 PM, Michael S. Tsirkin wrote: > > > > > >+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? Yes. > > > 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; So long as we're nitpicking, limit + 1 - base. > > > > 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. Right. > The only way to fix this would be to pass first/last instead > of offset/size. Too late for such an API change? Way too late. And also won't work, since often the offset is determined by one party and the size by another. > VGA I/O addresses (including ISA aliases address - AD[15::10] are not > decoded): > AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh These "not decoded" bits mean you need to instantiate tons of aliases to implement correctly. I plan to add core support for that eventually.
On Sun, Sep 04, 2011 at 05:36:56PM +0300, Avi Kivity wrote: > On 09/04/2011 05:21 PM, Michael S. Tsirkin wrote: > >> > > >> >+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? > > Yes. > > > > >> 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; > > So long as we're nitpicking, limit + 1 - base. If this makes you happier, OK :) > > > > > >> 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. > > Right. > > >The only way to fix this would be to pass first/last instead > >of offset/size. Too late for such an API change? > > Way too late. And also won't work, since often the offset is > determined by one party and the size by another. For things like BARs, yes - but these don't need to be that big normally. We could add an additinal API that gets first/last parameters. last < first means 0 size. Feasible? > >VGA I/O addresses (including ISA aliases address - AD[15::10] are not > >decoded): > >AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh > > These "not decoded" bits mean you need to instantiate tons of > aliases to implement correctly. > I plan to add core support for that eventually. There's a flag to enable 16-bit decode actually: bit 4 in bridge control register. How does VGA work at the moment without a bridge? Ignores the ISA aliases? then we can do that too I think. > -- > error compiling committee.c: too many arguments to function
On 09/04/2011 05:54 PM, Michael S. Tsirkin wrote: > > Way too late. And also won't work, since often the offset is > > determined by one party and the size by another. > > For things like BARs, yes - but these don't need to be > that big normally. We could add an additinal API > that gets first/last parameters. last< first means 0 size. > Feasible? Let's defer this for now. Does PCI actually have 64-bit addresses? Note that most/all cpus don't. We may need 65-bit arithmetic for that. > > > >VGA I/O addresses (including ISA aliases address - AD[15::10] are not > > >decoded): > > >AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh > > > > These "not decoded" bits mean you need to instantiate tons of > > aliases to implement correctly. > > I plan to add core support for that eventually. > > There's a flag to enable 16-bit decode actually: > bit 4 in bridge control register. > How does VGA work at the moment without a bridge? Ignores the ISA aliases? > then we can do that too I think. > Of course it doesn't ignore it. See the 440fx implementation, if you disable VGA access (via the SMRAM register), vga goes away. (vga registers its legacy space as a subregion of pci_address_space(dev))
On Sun, Sep 04, 2011 at 06:14:22PM +0300, Avi Kivity wrote: > On 09/04/2011 05:54 PM, Michael S. Tsirkin wrote: > >> Way too late. And also won't work, since often the offset is > >> determined by one party and the size by another. > > > >For things like BARs, yes - but these don't need to be > >that big normally. We could add an additinal API > >that gets first/last parameters. last< first means 0 size. > >Feasible? > > Let's defer this for now. > > Does PCI actually have 64-bit addresses? Yes. > Note that most/all cpus don't. > We may need 65-bit arithmetic for that. Jut using start/limit carefully should be enough ... > > > >> >VGA I/O addresses (including ISA aliases address - AD[15::10] are not > >> >decoded): > >> >AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh > >> > >> These "not decoded" bits mean you need to instantiate tons of > >> aliases to implement correctly. > >> I plan to add core support for that eventually. > > > >There's a flag to enable 16-bit decode actually: > >bit 4 in bridge control register. > >How does VGA work at the moment without a bridge? Ignores the ISA aliases? > >then we can do that too I think. > > > > Of course it doesn't ignore it. See the 440fx implementation, if > you disable VGA access (via the SMRAM register), vga goes away. Yes but that's for VGA RAM, right? I'm talking about the IO addresses: are tons of aliases created as you suggest? > (vga registers its legacy space as a subregion of pci_address_space(dev)) > > -- > error compiling committee.c: too many arguments to function
On 09/04/2011 06:24 PM, Michael S. Tsirkin wrote: > > > > Of course it doesn't ignore it. See the 440fx implementation, if > > you disable VGA access (via the SMRAM register), vga goes away. > > Yes but that's for VGA RAM, right? I'm talking about the IO addresses: > are tons of aliases created as you suggest? No.
On Sun, Sep 04, 2011 at 06:37:08PM +0300, Avi Kivity wrote: > On 09/04/2011 06:24 PM, Michael S. Tsirkin wrote: > >> > >> Of course it doesn't ignore it. See the 440fx implementation, if > >> you disable VGA access (via the SMRAM register), vga goes away. > > > >Yes but that's for VGA RAM, right? I'm talking about the IO addresses: > >are tons of aliases created as you suggest? > > No. So a full 16 bit decode is done for now? > -- > error compiling committee.c: too many arguments to function
On 09/04/2011 06:45 PM, Michael S. Tsirkin wrote: > On Sun, Sep 04, 2011 at 06:37:08PM +0300, Avi Kivity wrote: > > On 09/04/2011 06:24 PM, Michael S. Tsirkin wrote: > > >> > > >> Of course it doesn't ignore it. See the 440fx implementation, if > > >> you disable VGA access (via the SMRAM register), vga goes away. > > > > > >Yes but that's for VGA RAM, right? I'm talking about the IO addresses: > > >are tons of aliases created as you suggest? > > > > No. > > So a full 16 bit decode is done for now? > Correct. But isn't it needed? Otherwise why don't vga accesses alias with a virtio device at 0xc3c0?
On Sun, Sep 04, 2011 at 06:46:46PM +0300, Avi Kivity wrote: > On 09/04/2011 06:45 PM, Michael S. Tsirkin wrote: > >On Sun, Sep 04, 2011 at 06:37:08PM +0300, Avi Kivity wrote: > >> On 09/04/2011 06:24 PM, Michael S. Tsirkin wrote: > >> >> > >> >> Of course it doesn't ignore it. See the 440fx implementation, if > >> >> you disable VGA access (via the SMRAM register), vga goes away. > >> > > >> >Yes but that's for VGA RAM, right? I'm talking about the IO addresses: > >> >are tons of aliases created as you suggest? > >> > >> No. > > > >So a full 16 bit decode is done for now? > > > > Correct. Then we probably can ignore the issue and do 16 bit decode all over for now. > But isn't it needed? Otherwise why don't vga accesses > alias with a virtio device at 0xc3c0? It really depends on the device I think. > -- > error compiling committee.c: too many arguments to function
On 09/04/2011 07:19 PM, Michael S. Tsirkin wrote: > > But isn't it needed? Otherwise why don't vga accesses > > alias with a virtio device at 0xc3c0? > > It really depends on the device I think. > It's probably the bus. ISA may not decode A10-15, but if the pci/isa bridge does, then it doesn't matter.
On Sun, Sep 04, 2011 at 07:22:54PM +0300, Avi Kivity wrote: > On 09/04/2011 07:19 PM, Michael S. Tsirkin wrote: > >> But isn't it needed? Otherwise why don't vga accesses > >> alias with a virtio device at 0xc3c0? > > > >It really depends on the device I think. > > > > It's probably the bus. ISA may not decode A10-15, but if the > pci/isa bridge does, then it doesn't matter. The bridge has a 16 bit decode bit. I'm guessing we should make BIOS enable that. > -- > error compiling committee.c: too many arguments to function
On 09/04/2011 08:03 PM, Michael S. Tsirkin wrote: > On Sun, Sep 04, 2011 at 07:22:54PM +0300, Avi Kivity wrote: > > On 09/04/2011 07:19 PM, Michael S. Tsirkin wrote: > > >> But isn't it needed? Otherwise why don't vga accesses > > >> alias with a virtio device at 0xc3c0? > > > > > >It really depends on the device I think. > > > > > > > It's probably the bus. ISA may not decode A10-15, but if the > > pci/isa bridge does, then it doesn't matter. > > The bridge has a 16 bit decode bit. I'm guessing we should > make BIOS enable that. > I'm guessing you're right.
diff --git a/hw/pci.c b/hw/pci.c index 57ff7b1..56dfa18 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -889,7 +889,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, r = &pci_dev->io_regions[region_num]; r->addr = PCI_BAR_UNMAPPED; r->size = size; - r->filtered_size = size; r->type = type; r->memory = NULL; @@ -920,41 +919,6 @@ pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num) return pci_dev->io_regions[region_num].addr; } -static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size, - uint8_t type) -{ - pcibus_t base = *addr; - pcibus_t limit = *addr + *size - 1; - PCIDevice *br; - - for (br = d->bus->parent_dev; br; br = br->bus->parent_dev) { - uint16_t cmd = pci_get_word(d->config + PCI_COMMAND); - - if (type & PCI_BASE_ADDRESS_SPACE_IO) { - if (!(cmd & PCI_COMMAND_IO)) { - goto no_map; - } - } else { - if (!(cmd & PCI_COMMAND_MEMORY)) { - goto no_map; - } - } - - base = MAX(base, pci_bridge_get_base(br, type)); - limit = MIN(limit, pci_bridge_get_limit(br, type)); - } - - if (base > limit) { - goto no_map; - } - *addr = base; - *size = limit - base + 1; - return; -no_map: - *addr = PCI_BAR_UNMAPPED; - *size = 0; -} - static pcibus_t pci_bar_address(PCIDevice *d, int reg, uint8_t type, pcibus_t size) { @@ -1024,7 +988,7 @@ static void pci_update_mappings(PCIDevice *d) { PCIIORegion *r; int i; - pcibus_t new_addr, filtered_size; + pcibus_t new_addr; for(i = 0; i < PCI_NUM_REGIONS; i++) { r = &d->io_regions[i]; @@ -1035,14 +999,8 @@ static void pci_update_mappings(PCIDevice *d) new_addr = pci_bar_address(d, i, r->type, r->size); - /* bridge filtering */ - filtered_size = r->size; - if (new_addr != PCI_BAR_UNMAPPED) { - pci_bridge_filter(d, &new_addr, &filtered_size, r->type); - } - /* This bar isn't changed */ - if (new_addr == r->addr && filtered_size == r->filtered_size) + if (new_addr == r->addr) continue; /* now do the real mapping */ @@ -1050,15 +1008,7 @@ static void pci_update_mappings(PCIDevice *d) memory_region_del_subregion(r->address_space, r->memory); } r->addr = new_addr; - r->filtered_size = filtered_size; if (r->addr != PCI_BAR_UNMAPPED) { - /* - * TODO: currently almost all the map funcions assumes - * filtered_size == size and addr & ~(size - 1) == addr. - * However with bridge filtering, they aren't always true. - * Teach them such cases, such that filtered_size < size and - * addr & (size - 1) != 0. - */ if (r->type & PCI_BASE_ADDRESS_SPACE_IO) { memory_region_add_subregion_overlap(r->address_space, r->addr, @@ -1576,22 +1526,6 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model, return res; } -static void pci_bridge_update_mappings_fn(PCIBus *b, PCIDevice *d) -{ - pci_update_mappings(d); -} - -void pci_bridge_update_mappings(PCIBus *b) -{ - PCIBus *child; - - pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn); - - QLIST_FOREACH(child, &b->child, sibling) { - pci_bridge_update_mappings(child); - } -} - /* Whether a given bus number is in range of the secondary * bus of the given bridge device. */ static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num) diff --git a/hw/pci.h b/hw/pci.h index 391217e..65e1568 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -90,7 +90,6 @@ typedef struct PCIIORegion { pcibus_t addr; /* current PCI mapping address. -1 means not mapped */ #define PCI_BAR_UNMAPPED (~(pcibus_t)0) pcibus_t size; - pcibus_t filtered_size; uint8_t type; MemoryRegion *memory; MemoryRegion *address_space; @@ -277,7 +276,6 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp, void do_pci_info_print(Monitor *mon, const QObject *data); void do_pci_info(Monitor *mon, QObject **ret_data); -void pci_bridge_update_mappings(PCIBus *b); void pci_device_deassert_intx(PCIDevice *dev); diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c index e0b339e..469dfe8 100644 --- a/hw/pci_bridge.c +++ b/hw/pci_bridge.c @@ -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; +} + +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); +} + +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); +} + +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); + +#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 +} + /* default write_config function for PCI-to-PCI bridge */ void pci_bridge_write_config(PCIDevice *d, uint32_t address, uint32_t val, int len) @@ -151,7 +220,7 @@ void pci_bridge_write_config(PCIDevice *d, /* memory base/limit, prefetchable base/limit and io base/limit upper 16 */ ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) { - pci_bridge_update_mappings(&s->sec_bus); + pci_bridge_update_mappings(s); } newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL); @@ -247,9 +316,14 @@ int pci_bridge_initfn(PCIDevice *dev) sec_bus->parent_dev = dev; sec_bus->map_irq = br->map_irq; /* TODO: use memory API to perform memory filtering. */ - sec_bus->address_space_mem = parent->address_space_mem; - sec_bus->address_space_io = parent->address_space_io; - + sec_bus->address_space_mem = g_new(MemoryRegion, 1); + memory_region_init(sec_bus->address_space_mem, "pci_pridge_pci", INT64_MAX); + sec_bus->address_space_io = g_new(MemoryRegion, 1); + memory_region_init(sec_bus->address_space_io, "pci_bridge_io", 65536); + sec_bus->alias_pref_mem = g_new(MemoryRegion, 1); + sec_bus->alias_mem = g_new(MemoryRegion, 1); + sec_bus->alias_io = g_new(MemoryRegion, 1); + pci_bridge_region_init(br); QLIST_INIT(&sec_bus->child); QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling); return 0; @@ -259,8 +333,17 @@ int pci_bridge_initfn(PCIDevice *dev) int pci_bridge_exitfn(PCIDevice *pci_dev) { PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev); + PCIBus *sec_bus = &s->sec_bus; assert(QLIST_EMPTY(&s->sec_bus.child)); QLIST_REMOVE(&s->sec_bus, sibling); + pci_bridge_region_cleanup(s); + g_free(sec_bus->alias_pref_mem); + g_free(sec_bus->alias_mem); + g_free(sec_bus->alias_io); + memory_region_destroy(sec_bus->address_space_mem); + g_free(sec_bus->address_space_mem); + memory_region_destroy(sec_bus->address_space_io); + g_free(sec_bus->address_space_io); /* qbus_free() is called automatically by qdev_free() */ return 0; } diff --git a/hw/pci_internals.h b/hw/pci_internals.h index c7fd23d..578c8d2 100644 --- a/hw/pci_internals.h +++ b/hw/pci_internals.h @@ -27,6 +27,9 @@ struct PCIBus { target_phys_addr_t mem_base; MemoryRegion *address_space_mem; MemoryRegion *address_space_io; + MemoryRegion *alias_pref_mem; + MemoryRegion *alias_mem; + MemoryRegion *alias_io; QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */ QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */
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? Is the region name for debugging only? When does priority matter? In case of overlap?