[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 20/23] pci: make bar update function aware of pc
From: |
Michael S. Tsirkin |
Subject: |
[Qemu-devel] Re: [PATCH 20/23] pci: make bar update function aware of pci bridge. |
Date: |
Mon, 5 Oct 2009 13:59:51 +0200 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
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 <address@hidden>
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
[Qemu-devel] [PATCH 12/23] pci: 64bit bar support., Isaku Yamahata, 2009/10/05
[Qemu-devel] Re: [PATCH 12/23] pci: 64bit bar support., Michael S. Tsirkin, 2009/10/05