| Message ID | 1254737223-16129-21-git-send-email-yamahata@valinux.co.jp |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Oct 05, 2009 at 07:07:00PM +0900, Isaku Yamahata wrote: > header type of 01 has differenct BAR to type 00. > It has only BAR0,1 and expantion rom whose offset address > is different from type 00 one. > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> The existing code is actually correct, and I think this breaks it. I think it's a good idea to add pci_bar_config_offset, though. Split this up to a separate patch? > --- > hw/pci.c | 56 +++++++++++++++++++++++++++++++++++++++++++++----------- > hw/pci.h | 2 ++ > 2 files changed, 47 insertions(+), 11 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index b8d2f8f..af864c6 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -505,6 +505,33 @@ static inline int pci_bar_is_mem64(const PCIIORegion *r) > PCI_ADDRESS_SPACE_MEM_TYPE_64; > } > > +/* > + * return offset in pci configuration space for a given BAR of region_num. > + * header type > + * normal = 0: bar 0-5 and rom > + * bridge = 1: bar 0,1 and rom > + * cardbus = 2: bar 0 > + */ > +static uint32_t pci_bar_config_offset(PCIDevice *d, int region_num) > +{ > + uint8_t header_type = > + d->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION; > + > + assert((header_type == PCI_HEADER_TYPE_NORMAL && > + ((0 <= region_num && region_num < 6) || > + region_num == PCI_ROM_SLOT)) || && already binds stronger than ||, so don't put () around && > + (header_type == PCI_HEADER_TYPE_BRIDGE && > + ((0 <= region_num && region_num < 2) || > + region_num == PCI_ROM_SLOT)) || > + (header_type == PCI_HEADER_TYPE_CARDBUS && region_num == 0)); This is not the best place to put assertions. If you really need them, pci_register_bar is a better place. > + > + if (region_num != PCI_ROM_ADDRESS) > + return PCI_BASE_ADDRESS_0 + region_num * 4; BTW, how does it work for 64 bit regions? > + > + return header_type == PCI_HEADER_TYPE_BRIDGE? > + PCI_ROM_ADDRESS1: PCI_ROM_ADDRESS; space before : > +} > + > void pci_register_bar(PCIDevice *pci_dev, int region_num, > pcibus_t size, int type, > PCIMapIORegionFunc *map_func) > @@ -528,13 +555,11 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, > r->type = type; > r->map_func = map_func; > > + addr = pci_bar_config_offset(pci_dev, region_num); > wmask = ~(size - 1); > if (region_num == PCI_ROM_SLOT) { > - addr = 0x30; > /* ROM enable bit is writeable */ > wmask |= PCI_ROM_ADDRESS_ENABLE; > - } else { > - addr = 0x10 + region_num * 4; > } > pci_set_long(pci_dev->config + addr, type); > if (pci_bar_is_mem64(r)) { > @@ -556,11 +581,7 @@ static void pci_update_mappings(PCIDevice *d) > cmd = pci_get_word(d->config + PCI_COMMAND); > for(i = 0; i < PCI_NUM_REGIONS; i++) { > r = &d->io_regions[i]; > - if (i == PCI_ROM_SLOT) { > - config_ofs = 0x30; > - } else { > - config_ofs = 0x10 + i * 4; > - } > + config_ofs = pci_bar_config_offset(d, i); > if (r->size != 0) { > if (r->type & PCI_ADDRESS_SPACE_IO) { > if (cmd & PCI_COMMAND_IO) { > @@ -1123,10 +1144,23 @@ static void pci_bridge_write_config(PCIDevice *d, > uint32_t address, uint32_t val, int len) > { > PCIBridge *s = (PCIBridge *)d; > + PCIBus *bus = s->bus; > + struct pci_config_update update; > > - pci_default_write_config(d, address, val, len); > - s->bus->bus_num = d->config[PCI_SECONDARY_BUS]; > - s->bus->sub_bus = d->config[PCI_SUBORDINATE_BUS]; > + pci_write_config_init(&update, d, address, val, len); > + pci_write_config_update(&update); > + if (pci_config_changed(&update, > + PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_2 + 4) || > + pci_config_changed_with_size(&update, PCI_ROM_ADDRESS1, 4) || > + pci_config_changed_with_size(&update, PCI_COMMAND, 1)) { > + pci_update_mappings(d); > + } This is wrong I think. You must also take into account memory base/limit registers, and redo mapping when these change. If you do, you should note several things: - BARs for devices placed behind a bridge who's memory is outside the bridge base/limit are effectively disabled. - Base addresses for pci devices overlap base/limit for bridge. For this reason, default_write_config was already doing the right thing, doing update when the bridge base/limit are changed. > + if (pci_config_changed_with_size(&update, PCI_SECONDARY_BUS, 1)) { > + bus->bus_num = d->config[PCI_SECONDARY_BUS]; > + } > + if (pci_config_changed_with_size(&update, PCI_SUBORDINATE_BUS, 1)) { > + bus->sub_bus = d->config[PCI_SUBORDINATE_BUS]; > + } > } > > PCIBus *pci_find_bus(PCIBus *bus, int bus_num) > diff --git a/hw/pci.h b/hw/pci.h > index 37c2c23..1d45437 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -118,6 +118,7 @@ typedef struct PCIIORegion { > #define PCI_HEADER_TYPE_CARDBUS 2 > #define PCI_HEADER_TYPE_MULTI_FUNCTION 0x80 > #define PCI_BASE_ADDRESS_0 0x10 /* 32 bits */ > +#define PCI_BASE_ADDRESS_2 0x18 /* 32 bits */ > #define PCI_BASE_ADDRESS_5 0x24 /* 32 bits */ > #define PCI_PRIMARY_BUS 0x18 /* Primary bus number */ > #define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */ > @@ -163,6 +164,7 @@ typedef struct PCIIORegion { > > /* Header type 1 (PCI-to-PCI bridges) */ > #define PCI_SUBORDINATE_BUS 0x1a /* Highest bus number behind the bridge */ > +#define PCI_ROM_ADDRESS1 0x38 /* Same as PCI_ROM_ADDRESS, but for htype 1 */ > > /* Size of the standard PCI config header */ > #define PCI_CONFIG_HEADER_SIZE 0x40 > -- > 1.6.0.2
On Mon, Oct 05, 2009 at 01:59:51PM +0200, Michael S. Tsirkin wrote: > On Mon, Oct 05, 2009 at 07:07:00PM +0900, Isaku Yamahata wrote: > > header type of 01 has differenct BAR to type 00. > > It has only BAR0,1 and expantion rom whose offset address > > is different from type 00 one. > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > The existing code is actually correct, and I think this breaks it. I > think it's a good idea to add pci_bar_config_offset, though. > Split this up to a separate patch? > > > --- > > hw/pci.c | 56 +++++++++++++++++++++++++++++++++++++++++++++----------- > > hw/pci.h | 2 ++ > > 2 files changed, 47 insertions(+), 11 deletions(-) > > > > diff --git a/hw/pci.c b/hw/pci.c > > index b8d2f8f..af864c6 100644 > > --- a/hw/pci.c > > +++ b/hw/pci.c > > @@ -505,6 +505,33 @@ static inline int pci_bar_is_mem64(const PCIIORegion *r) > > PCI_ADDRESS_SPACE_MEM_TYPE_64; > > } > > > > +/* > > + * return offset in pci configuration space for a given BAR of region_num. > > + * header type > > + * normal = 0: bar 0-5 and rom > > + * bridge = 1: bar 0,1 and rom > > + * cardbus = 2: bar 0 > > + */ > > +static uint32_t pci_bar_config_offset(PCIDevice *d, int region_num) > > +{ > > + uint8_t header_type = > > + d->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION; > > + > > + assert((header_type == PCI_HEADER_TYPE_NORMAL && > > + ((0 <= region_num && region_num < 6) || > > + region_num == PCI_ROM_SLOT)) || > > && already binds stronger than ||, so don't put () around && gcc wants parens like hw/pci.c:531: error: suggest parentheses around && within || note:qemu configure script enables -Werror by defaults. > > + (header_type == PCI_HEADER_TYPE_BRIDGE && > > + ((0 <= region_num && region_num < 2) || > > + region_num == PCI_ROM_SLOT)) || > > + (header_type == PCI_HEADER_TYPE_CARDBUS && region_num == 0)); > > This is not the best place to put assertions. > If you really need them, pci_register_bar is a better place. > > > + > > + if (region_num != PCI_ROM_ADDRESS) > > + return PCI_BASE_ADDRESS_0 + region_num * 4; > > BTW, how does it work for 64 bit regions? > > > + > > + return header_type == PCI_HEADER_TYPE_BRIDGE? > > + PCI_ROM_ADDRESS1: PCI_ROM_ADDRESS; > > space before : > > > +} > > + > > void pci_register_bar(PCIDevice *pci_dev, int region_num, > > pcibus_t size, int type, > > PCIMapIORegionFunc *map_func) > > @@ -528,13 +555,11 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, > > r->type = type; > > r->map_func = map_func; > > > > + addr = pci_bar_config_offset(pci_dev, region_num); > > wmask = ~(size - 1); > > if (region_num == PCI_ROM_SLOT) { > > - addr = 0x30; > > /* ROM enable bit is writeable */ > > wmask |= PCI_ROM_ADDRESS_ENABLE; > > - } else { > > - addr = 0x10 + region_num * 4; > > } > > pci_set_long(pci_dev->config + addr, type); > > if (pci_bar_is_mem64(r)) { > > @@ -556,11 +581,7 @@ static void pci_update_mappings(PCIDevice *d) > > cmd = pci_get_word(d->config + PCI_COMMAND); > > for(i = 0; i < PCI_NUM_REGIONS; i++) { > > r = &d->io_regions[i]; > > - if (i == PCI_ROM_SLOT) { > > - config_ofs = 0x30; > > - } else { > > - config_ofs = 0x10 + i * 4; > > - } > > + config_ofs = pci_bar_config_offset(d, i); > > if (r->size != 0) { > > if (r->type & PCI_ADDRESS_SPACE_IO) { > > if (cmd & PCI_COMMAND_IO) { > > @@ -1123,10 +1144,23 @@ static void pci_bridge_write_config(PCIDevice *d, > > uint32_t address, uint32_t val, int len) > > { > > PCIBridge *s = (PCIBridge *)d; > > + PCIBus *bus = s->bus; > > + struct pci_config_update update; > > > > - pci_default_write_config(d, address, val, len); > > - s->bus->bus_num = d->config[PCI_SECONDARY_BUS]; > > - s->bus->sub_bus = d->config[PCI_SUBORDINATE_BUS]; > > + pci_write_config_init(&update, d, address, val, len); > > + pci_write_config_update(&update); > > + if (pci_config_changed(&update, > > + PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_2 + 4) || > > + pci_config_changed_with_size(&update, PCI_ROM_ADDRESS1, 4) || > > + pci_config_changed_with_size(&update, PCI_COMMAND, 1)) { > > + pci_update_mappings(d); > > + } > > This is wrong I think. You must also take into account memory > base/limit registers, and redo mapping when these change. > If you do, you should note several things: > - BARs for devices placed behind a bridge who's memory > is outside the bridge base/limit are effectively disabled. I deliberately didn't implemented bridge io/memory filtering because linux doesn't depend on it. I'll add some comment on this. Linux boots happily without filtering emulation. However Linux was confused without correct emulation of reading/writing to/from base/limit. so wmask needs to be initialized. If other OS needs filtering emulation, it will be implemented. I don't know other OSes. Especially windows. I suppose Solaris doesn't because apb_pci.c uses bridge. > - Base addresses for pci devices overlap base/limit for > bridge. For this reason, default_write_config > was already doing the right thing, doing update > when the bridge base/limit are changed. I don't understand "the right thing" that the old code is doing and don't understand why the patch is wrong. BAR0 or BAR1 case: Okay. pci_default_write_config() properly update BAR0/1 mappings. BAR2-5 case = memory/io base/limit case,: pci_default_write_config() calls pci_update_mapping(), but pci_update_mapping() does nothing because bar 2 - 5 must not registered. It's just overhead to do nothing. PCI_ROM_ADDRESS = IO base/limit upper 16 case: If the bug is fixed, pci_default_write_config() calls pci_update_mapping(). but it does nothing. PCI_ROM_ADDRESS1 case And when PCI_ROM_ADDRESS1 (!= PCI_ROM_ADDRESS) is changed, pci_update_mapping() should be called. but pci_default_write_config() doesn't. > > + if (pci_config_changed_with_size(&update, PCI_SECONDARY_BUS, 1)) { > > + bus->bus_num = d->config[PCI_SECONDARY_BUS]; > > + } > > + if (pci_config_changed_with_size(&update, PCI_SUBORDINATE_BUS, 1)) { > > + bus->sub_bus = d->config[PCI_SUBORDINATE_BUS]; > > + } > > } > > > > PCIBus *pci_find_bus(PCIBus *bus, int bus_num) > > diff --git a/hw/pci.h b/hw/pci.h > > index 37c2c23..1d45437 100644 > > --- a/hw/pci.h > > +++ b/hw/pci.h > > @@ -118,6 +118,7 @@ typedef struct PCIIORegion { > > #define PCI_HEADER_TYPE_CARDBUS 2 > > #define PCI_HEADER_TYPE_MULTI_FUNCTION 0x80 > > #define PCI_BASE_ADDRESS_0 0x10 /* 32 bits */ > > +#define PCI_BASE_ADDRESS_2 0x18 /* 32 bits */ > > #define PCI_BASE_ADDRESS_5 0x24 /* 32 bits */ > > #define PCI_PRIMARY_BUS 0x18 /* Primary bus number */ > > #define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */ > > @@ -163,6 +164,7 @@ typedef struct PCIIORegion { > > > > /* Header type 1 (PCI-to-PCI bridges) */ > > #define PCI_SUBORDINATE_BUS 0x1a /* Highest bus number behind the bridge */ > > +#define PCI_ROM_ADDRESS1 0x38 /* Same as PCI_ROM_ADDRESS, but for htype 1 */ > > > > /* Size of the standard PCI config header */ > > #define PCI_CONFIG_HEADER_SIZE 0x40 >
On Fri, Oct 09, 2009 at 12:27:35PM +0900, Isaku Yamahata wrote: > On Mon, Oct 05, 2009 at 01:59:51PM +0200, Michael S. Tsirkin wrote: > > On Mon, Oct 05, 2009 at 07:07:00PM +0900, Isaku Yamahata wrote: > > > header type of 01 has differenct BAR to type 00. > > > It has only BAR0,1 and expantion rom whose offset address > > > is different from type 00 one. > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > > The existing code is actually correct, and I think this breaks it. I > > think it's a good idea to add pci_bar_config_offset, though. > > Split this up to a separate patch? > > > > > --- > > > hw/pci.c | 56 +++++++++++++++++++++++++++++++++++++++++++++----------- > > > hw/pci.h | 2 ++ > > > 2 files changed, 47 insertions(+), 11 deletions(-) > > > > > > diff --git a/hw/pci.c b/hw/pci.c > > > index b8d2f8f..af864c6 100644 > > > --- a/hw/pci.c > > > +++ b/hw/pci.c > > > @@ -505,6 +505,33 @@ static inline int pci_bar_is_mem64(const PCIIORegion *r) > > > PCI_ADDRESS_SPACE_MEM_TYPE_64; > > > } > > > > > > +/* > > > + * return offset in pci configuration space for a given BAR of region_num. > > > + * header type > > > + * normal = 0: bar 0-5 and rom > > > + * bridge = 1: bar 0,1 and rom > > > + * cardbus = 2: bar 0 > > > + */ > > > +static uint32_t pci_bar_config_offset(PCIDevice *d, int region_num) > > > +{ > > > + uint8_t header_type = > > > + d->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION; > > > + > > > + assert((header_type == PCI_HEADER_TYPE_NORMAL && > > > + ((0 <= region_num && region_num < 6) || > > > + region_num == PCI_ROM_SLOT)) || > > > > && already binds stronger than ||, so don't put () around && > > gcc wants parens like > hw/pci.c:531: error: suggest parentheses around && within || > note:qemu configure script enables -Werror by defaults. I see. > > > > + (header_type == PCI_HEADER_TYPE_BRIDGE && > > > + ((0 <= region_num && region_num < 2) || > > > + region_num == PCI_ROM_SLOT)) || > > > + (header_type == PCI_HEADER_TYPE_CARDBUS && region_num == 0)); > > > > This is not the best place to put assertions. > > If you really need them, pci_register_bar is a better place. > > > > > + > > > + if (region_num != PCI_ROM_ADDRESS) > > > + return PCI_BASE_ADDRESS_0 + region_num * 4; > > > > BTW, how does it work for 64 bit regions? > > > > > + > > > + return header_type == PCI_HEADER_TYPE_BRIDGE? > > > + PCI_ROM_ADDRESS1: PCI_ROM_ADDRESS; > > > > space before : > > > > > +} > > > + > > > void pci_register_bar(PCIDevice *pci_dev, int region_num, > > > pcibus_t size, int type, > > > PCIMapIORegionFunc *map_func) > > > @@ -528,13 +555,11 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, > > > r->type = type; > > > r->map_func = map_func; > > > > > > + addr = pci_bar_config_offset(pci_dev, region_num); > > > wmask = ~(size - 1); > > > if (region_num == PCI_ROM_SLOT) { > > > - addr = 0x30; > > > /* ROM enable bit is writeable */ > > > wmask |= PCI_ROM_ADDRESS_ENABLE; > > > - } else { > > > - addr = 0x10 + region_num * 4; > > > } > > > pci_set_long(pci_dev->config + addr, type); > > > if (pci_bar_is_mem64(r)) { > > > @@ -556,11 +581,7 @@ static void pci_update_mappings(PCIDevice *d) > > > cmd = pci_get_word(d->config + PCI_COMMAND); > > > for(i = 0; i < PCI_NUM_REGIONS; i++) { > > > r = &d->io_regions[i]; > > > - if (i == PCI_ROM_SLOT) { > > > - config_ofs = 0x30; > > > - } else { > > > - config_ofs = 0x10 + i * 4; > > > - } > > > + config_ofs = pci_bar_config_offset(d, i); > > > if (r->size != 0) { > > > if (r->type & PCI_ADDRESS_SPACE_IO) { > > > if (cmd & PCI_COMMAND_IO) { > > > @@ -1123,10 +1144,23 @@ static void pci_bridge_write_config(PCIDevice *d, > > > uint32_t address, uint32_t val, int len) > > > { > > > PCIBridge *s = (PCIBridge *)d; > > > + PCIBus *bus = s->bus; > > > + struct pci_config_update update; > > > > > > - pci_default_write_config(d, address, val, len); > > > - s->bus->bus_num = d->config[PCI_SECONDARY_BUS]; > > > - s->bus->sub_bus = d->config[PCI_SUBORDINATE_BUS]; > > > + pci_write_config_init(&update, d, address, val, len); > > > + pci_write_config_update(&update); > > > + if (pci_config_changed(&update, > > > + PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_2 + 4) || > > > + pci_config_changed_with_size(&update, PCI_ROM_ADDRESS1, 4) || > > > + pci_config_changed_with_size(&update, PCI_COMMAND, 1)) { > > > + pci_update_mappings(d); > > > + } > > > > This is wrong I think. You must also take into account memory > > base/limit registers, and redo mapping when these change. > > If you do, you should note several things: > > - BARs for devices placed behind a bridge who's memory > > is outside the bridge base/limit are effectively disabled. > > I deliberately didn't implemented bridge io/memory filtering > because linux doesn't depend on it. I'll add some comment on this. > Linux boots happily without filtering emulation. > However Linux was confused without correct emulation of > reading/writing to/from base/limit. so wmask needs to be initialized. > > If other OS needs filtering emulation, it will be implemented. > I don't know other OSes. Especially windows. > I suppose Solaris doesn't because apb_pci.c uses bridge. Filtering is the only way to disable e.g. prefetchable memory in a bridge accoring to PCI spec, and I know that some BIOSes take advantage of this. Frankly, I think we should just try and stick to spec. It's not hard at all. > > - Base addresses for pci devices overlap base/limit for > > bridge. For this reason, default_write_config > > was already doing the right thing, doing update > > when the bridge base/limit are changed. > > I don't understand "the right thing" that the old code is doing > and don't understand why the patch is wrong. The patch is wrong in that it does not implement filtering. As you show below, handling for PCI_ROM_ADDRESS1 is missing, let's fix that, it's a single line change. > BAR0 or BAR1 case: > Okay. pci_default_write_config() properly update BAR0/1 mappings. > > BAR2-5 case = memory/io base/limit case,: > pci_default_write_config() calls pci_update_mapping(), > but pci_update_mapping() does nothing because bar 2 - 5 must not registered. > It's just overhead to do nothing. > > PCI_ROM_ADDRESS = IO base/limit upper 16 case: > If the bug is fixed, pci_default_write_config() calls pci_update_mapping(). > but it does nothing. > > PCI_ROM_ADDRESS1 case > And when PCI_ROM_ADDRESS1 (!= PCI_ROM_ADDRESS) is changed, > pci_update_mapping() should be called. but pci_default_write_config() doesn't. > Ah, I see. Let's just teach default_config_write to call pci_update_mapping when PCI_ROM_ADDRESS1 is changed. For regular devices this does no harm. > > > > > + if (pci_config_changed_with_size(&update, PCI_SECONDARY_BUS, 1)) { > > > + bus->bus_num = d->config[PCI_SECONDARY_BUS]; > > > + } > > > + if (pci_config_changed_with_size(&update, PCI_SUBORDINATE_BUS, 1)) { > > > + bus->sub_bus = d->config[PCI_SUBORDINATE_BUS]; > > > + } > > > } > > > > > > PCIBus *pci_find_bus(PCIBus *bus, int bus_num) > > > diff --git a/hw/pci.h b/hw/pci.h > > > index 37c2c23..1d45437 100644 > > > --- a/hw/pci.h > > > +++ b/hw/pci.h > > > @@ -118,6 +118,7 @@ typedef struct PCIIORegion { > > > #define PCI_HEADER_TYPE_CARDBUS 2 > > > #define PCI_HEADER_TYPE_MULTI_FUNCTION 0x80 > > > #define PCI_BASE_ADDRESS_0 0x10 /* 32 bits */ > > > +#define PCI_BASE_ADDRESS_2 0x18 /* 32 bits */ > > > #define PCI_BASE_ADDRESS_5 0x24 /* 32 bits */ > > > #define PCI_PRIMARY_BUS 0x18 /* Primary bus number */ > > > #define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */ > > > @@ -163,6 +164,7 @@ typedef struct PCIIORegion { > > > > > > /* Header type 1 (PCI-to-PCI bridges) */ > > > #define PCI_SUBORDINATE_BUS 0x1a /* Highest bus number behind the bridge */ > > > +#define PCI_ROM_ADDRESS1 0x38 /* Same as PCI_ROM_ADDRESS, but for htype 1 */ > > > > > > /* Size of the standard PCI config header */ > > > #define PCI_CONFIG_HEADER_SIZE 0x40 > > > > -- > yamahata
On Sat, Oct 10, 2009 at 10:20:36PM +0200, Michael S. Tsirkin wrote: > > > > @@ -1123,10 +1144,23 @@ static void pci_bridge_write_config(PCIDevice *d, > > > > uint32_t address, uint32_t val, int len) > > > > { > > > > PCIBridge *s = (PCIBridge *)d; > > > > + PCIBus *bus = s->bus; > > > > + struct pci_config_update update; > > > > > > > > - pci_default_write_config(d, address, val, len); > > > > - s->bus->bus_num = d->config[PCI_SECONDARY_BUS]; > > > > - s->bus->sub_bus = d->config[PCI_SUBORDINATE_BUS]; > > > > + pci_write_config_init(&update, d, address, val, len); > > > > + pci_write_config_update(&update); > > > > + if (pci_config_changed(&update, > > > > + PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_2 + 4) || > > > > + pci_config_changed_with_size(&update, PCI_ROM_ADDRESS1, 4) || > > > > + pci_config_changed_with_size(&update, PCI_COMMAND, 1)) { > > > > + pci_update_mappings(d); > > > > + } > > > > > > This is wrong I think. You must also take into account memory > > > base/limit registers, and redo mapping when these change. > > > If you do, you should note several things: > > > - BARs for devices placed behind a bridge who's memory > > > is outside the bridge base/limit are effectively disabled. > > > > I deliberately didn't implemented bridge io/memory filtering > > because linux doesn't depend on it. I'll add some comment on this. > > Linux boots happily without filtering emulation. > > However Linux was confused without correct emulation of > > reading/writing to/from base/limit. so wmask needs to be initialized. > > > > If other OS needs filtering emulation, it will be implemented. > > I don't know other OSes. Especially windows. > > I suppose Solaris doesn't because apb_pci.c uses bridge. > > Filtering is the only way to disable e.g. prefetchable memory in a > bridge accoring to PCI spec, and I know that some BIOSes take advantage > of this. Frankly, I think we should just try and stick to spec. > It's not hard at all. BIOS for real hardware? At least pcbios and seabios doesn't, so the above doesn't make sense. Implementing filtering would be another story.
On Wed, Oct 14, 2009 at 12:14:17AM +0900, Isaku Yamahata wrote: > On Sat, Oct 10, 2009 at 10:20:36PM +0200, Michael S. Tsirkin wrote: > > > > > @@ -1123,10 +1144,23 @@ static void pci_bridge_write_config(PCIDevice *d, > > > > > uint32_t address, uint32_t val, int len) > > > > > { > > > > > PCIBridge *s = (PCIBridge *)d; > > > > > + PCIBus *bus = s->bus; > > > > > + struct pci_config_update update; > > > > > > > > > > - pci_default_write_config(d, address, val, len); > > > > > - s->bus->bus_num = d->config[PCI_SECONDARY_BUS]; > > > > > - s->bus->sub_bus = d->config[PCI_SUBORDINATE_BUS]; > > > > > + pci_write_config_init(&update, d, address, val, len); > > > > > + pci_write_config_update(&update); > > > > > + if (pci_config_changed(&update, > > > > > + PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_2 + 4) || > > > > > + pci_config_changed_with_size(&update, PCI_ROM_ADDRESS1, 4) || > > > > > + pci_config_changed_with_size(&update, PCI_COMMAND, 1)) { > > > > > + pci_update_mappings(d); > > > > > + } > > > > > > > > This is wrong I think. You must also take into account memory > > > > base/limit registers, and redo mapping when these change. > > > > If you do, you should note several things: > > > > - BARs for devices placed behind a bridge who's memory > > > > is outside the bridge base/limit are effectively disabled. > > > > > > I deliberately didn't implemented bridge io/memory filtering > > > because linux doesn't depend on it. I'll add some comment on this. > > > Linux boots happily without filtering emulation. > > > However Linux was confused without correct emulation of > > > reading/writing to/from base/limit. so wmask needs to be initialized. > > > > > > If other OS needs filtering emulation, it will be implemented. > > > I don't know other OSes. Especially windows. > > > I suppose Solaris doesn't because apb_pci.c uses bridge. > > > > Filtering is the only way to disable e.g. prefetchable memory in a > > bridge accoring to PCI spec, and I know that some BIOSes take advantage > > of this. Frankly, I think we should just try and stick to spec. > > It's not hard at all. > > BIOS for real hardware? > At least pcbios and seabios doesn't, At the moment. But why assume things? > so the above doesn't make sense. > Implementing filtering would be another story. What do you call filtering? I think that all you have to do is 1. scan all child devices on bridge header write, and call update 2. when updating regions, check parent bus base/limit registers, scanning all bars upwards, and unmap registers that move outside the range, map registers that move inside the range. Where's the difficulty? > -- > yamahata
diff --git a/hw/pci.c b/hw/pci.c index b8d2f8f..af864c6 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -505,6 +505,33 @@ static inline int pci_bar_is_mem64(const PCIIORegion *r) PCI_ADDRESS_SPACE_MEM_TYPE_64; } +/* + * return offset in pci configuration space for a given BAR of region_num. + * header type + * normal = 0: bar 0-5 and rom + * bridge = 1: bar 0,1 and rom + * cardbus = 2: bar 0 + */ +static uint32_t pci_bar_config_offset(PCIDevice *d, int region_num) +{ + uint8_t header_type = + d->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION; + + assert((header_type == PCI_HEADER_TYPE_NORMAL && + ((0 <= region_num && region_num < 6) || + region_num == PCI_ROM_SLOT)) || + (header_type == PCI_HEADER_TYPE_BRIDGE && + ((0 <= region_num && region_num < 2) || + region_num == PCI_ROM_SLOT)) || + (header_type == PCI_HEADER_TYPE_CARDBUS && region_num == 0)); + + if (region_num != PCI_ROM_ADDRESS) + return PCI_BASE_ADDRESS_0 + region_num * 4; + + return header_type == PCI_HEADER_TYPE_BRIDGE? + PCI_ROM_ADDRESS1: PCI_ROM_ADDRESS; +} + void pci_register_bar(PCIDevice *pci_dev, int region_num, pcibus_t size, int type, PCIMapIORegionFunc *map_func) @@ -528,13 +555,11 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, r->type = type; r->map_func = map_func; + addr = pci_bar_config_offset(pci_dev, region_num); wmask = ~(size - 1); if (region_num == PCI_ROM_SLOT) { - addr = 0x30; /* ROM enable bit is writeable */ wmask |= PCI_ROM_ADDRESS_ENABLE; - } else { - addr = 0x10 + region_num * 4; } pci_set_long(pci_dev->config + addr, type); if (pci_bar_is_mem64(r)) { @@ -556,11 +581,7 @@ static void pci_update_mappings(PCIDevice *d) cmd = pci_get_word(d->config + PCI_COMMAND); for(i = 0; i < PCI_NUM_REGIONS; i++) { r = &d->io_regions[i]; - if (i == PCI_ROM_SLOT) { - config_ofs = 0x30; - } else { - config_ofs = 0x10 + i * 4; - } + config_ofs = pci_bar_config_offset(d, i); if (r->size != 0) { if (r->type & PCI_ADDRESS_SPACE_IO) { if (cmd & PCI_COMMAND_IO) { @@ -1123,10 +1144,23 @@ static void pci_bridge_write_config(PCIDevice *d, uint32_t address, uint32_t val, int len) { PCIBridge *s = (PCIBridge *)d; + PCIBus *bus = s->bus; + struct pci_config_update update; - pci_default_write_config(d, address, val, len); - s->bus->bus_num = d->config[PCI_SECONDARY_BUS]; - s->bus->sub_bus = d->config[PCI_SUBORDINATE_BUS]; + pci_write_config_init(&update, d, address, val, len); + pci_write_config_update(&update); + if (pci_config_changed(&update, + PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_2 + 4) || + pci_config_changed_with_size(&update, PCI_ROM_ADDRESS1, 4) || + pci_config_changed_with_size(&update, PCI_COMMAND, 1)) { + pci_update_mappings(d); + } + if (pci_config_changed_with_size(&update, PCI_SECONDARY_BUS, 1)) { + bus->bus_num = d->config[PCI_SECONDARY_BUS]; + } + if (pci_config_changed_with_size(&update, PCI_SUBORDINATE_BUS, 1)) { + bus->sub_bus = d->config[PCI_SUBORDINATE_BUS]; + } } PCIBus *pci_find_bus(PCIBus *bus, int bus_num) diff --git a/hw/pci.h b/hw/pci.h index 37c2c23..1d45437 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -118,6 +118,7 @@ typedef struct PCIIORegion { #define PCI_HEADER_TYPE_CARDBUS 2 #define PCI_HEADER_TYPE_MULTI_FUNCTION 0x80 #define PCI_BASE_ADDRESS_0 0x10 /* 32 bits */ +#define PCI_BASE_ADDRESS_2 0x18 /* 32 bits */ #define PCI_BASE_ADDRESS_5 0x24 /* 32 bits */ #define PCI_PRIMARY_BUS 0x18 /* Primary bus number */ #define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */ @@ -163,6 +164,7 @@ typedef struct PCIIORegion { /* Header type 1 (PCI-to-PCI bridges) */ #define PCI_SUBORDINATE_BUS 0x1a /* Highest bus number behind the bridge */ +#define PCI_ROM_ADDRESS1 0x38 /* Same as PCI_ROM_ADDRESS, but for htype 1 */ /* Size of the standard PCI config header */ #define PCI_CONFIG_HEADER_SIZE 0x40
header type of 01 has differenct BAR to type 00. It has only BAR0,1 and expantion rom whose offset address is different from type 00 one. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/pci.c | 56 +++++++++++++++++++++++++++++++++++++++++++++----------- hw/pci.h | 2 ++ 2 files changed, 47 insertions(+), 11 deletions(-)