[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 22/23] pci: initialize wmask according to pci he
From: |
Michael S. Tsirkin |
Subject: |
[Qemu-devel] Re: [PATCH 22/23] pci: initialize wmask according to pci header type. |
Date: |
Mon, 5 Oct 2009 16:09:03 +0200 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
On Mon, Oct 05, 2009 at 07:07:02PM +0900, Isaku Yamahata wrote:
> - initialize wmask according to pci header type.
> So far, header of type 01 was not correctly initialized.
I think it was correct. Which register has incorrect value?
It works because BAR for regular device is at the same location as
base/limit for bridge. value at which offset was wrong. Since no one
uses bridges and everyone uses regular devices, there's value in using
common code. So let's just add a comment explaing why code works, and be
done with it.
> - only sets default subsystem id for header type 00.
> header type 01 doesn't have subsystem id, and uses the register
> for other purpose. So setting default subsystem id doesn't make sense.
But then it's easier to reuse regular device code and override the value
to 0 in bridge after the fact than fork a ton of code.
> Signed-off-by: Isaku Yamahata <address@hidden>
> ---
> hw/cirrus_vga.c | 2 +-
> hw/pci.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> hw/pci.h | 36 ++++++++++++++++++++
> 3 files changed, 126 insertions(+), 10 deletions(-)
>
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 2a6aba8..55ef4c1 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -180,7 +180,7 @@
> #define PCI_COMMAND_PALETTESNOOPING 0x0020
> #define PCI_COMMAND_PARITYDETECTION 0x0040
> #define PCI_COMMAND_ADDRESSDATASTEPPING 0x0080
> -#define PCI_COMMAND_SERR 0x0100
> +// #define PCI_COMMAND_SERR 0x0100 /* duplicated */
so remove it?
> #define PCI_COMMAND_BACKTOBACKTRANS 0x0200
> // PCI 0x08, 0xff000000 (0x09-0x0b:class,0x08:rev)
> #define PCI_CLASS_BASE_DISPLAY 0x03
> diff --git a/hw/pci.c b/hw/pci.c
> index fb93b99..2694e83 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -381,16 +381,32 @@ static void pci_init_cmask(PCIDevice *dev)
> dev->cmask[PCI_CAPABILITY_LIST] = 0xff;
> }
>
> -static void pci_init_wmask(PCIDevice *dev)
> +static void pci_conf_init_type00(PCIDevice *dev)
I think it was a good idea to split wmask init from conf init. Small
functions doing one thing and doing it well. Let's not merge them.
> {
> int i;
> uint32_t config_size = pcie_config_size(dev);
>
> + pci_set_default_subsystem_id(dev);
> +
> + pci_set_word(&dev->wmask[PCI_COMMAND],
dev->wmask + COMMAND
> + PCI_COMMAND_IO |
> + PCI_COMMAND_MEMORY |
> + PCI_COMMAND_MASTER |
> + PCI_COMMAND_SPECIAL |
> + PCI_COMMAND_INVALIDATE |
> + PCI_COMMAND_VGA_PALETTE |
> + PCI_COMMAND_PARITY |
> + PCI_COMMAND_WAIT |
> + PCI_COMMAND_SERR |
> + PCI_COMMAND_FAST_BACK |
> + PCI_COMMAND_INTX_DISABLE);
> +
> dev->wmask[PCI_CACHE_LINE_SIZE] = 0xff;
> + dev->wmask[PCI_LATENCY_TIMER] = 0xff;
> dev->wmask[PCI_INTERRUPT_LINE] = 0xff;
> - dev->wmask[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY
> - | PCI_COMMAND_MASTER;
> - for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i)
> +
> + /* device dependent part */
> + for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i++)
> dev->wmask[i] = 0xff;
> }
>
> @@ -410,10 +426,12 @@ static void pci_config_alloc(PCIDevice *pci_dev)
> }
>
> /* -1 for devfn means auto assign */
> +typedef void (*pci_conf_init_t)(PCIDevice *d);
> static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> const char *name, int devfn,
> PCIConfigReadFunc *config_read,
> - PCIConfigWriteFunc *config_write)
> + PCIConfigWriteFunc *config_write,
> + pci_conf_init_t conf_init)
> {
> if (devfn < 0) {
> for(devfn = bus->devfn_min ; devfn < 256; devfn += 8) {
> @@ -430,9 +448,8 @@ static PCIDevice *do_pci_register_device(PCIDevice
> *pci_dev, PCIBus *bus,
> pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
> memset(pci_dev->irq_state, 0, sizeof(pci_dev->irq_state));
> pci_config_alloc(pci_dev);
> - pci_set_default_subsystem_id(pci_dev);
> pci_init_cmask(pci_dev);
> - pci_init_wmask(pci_dev);
> + conf_init(pci_dev);
Oh no, not another callback.
Let users just customize it *after* device is registered.
>
> if (!config_read)
> config_read = pci_default_read_config;
> @@ -455,9 +472,11 @@ PCIDevice *pci_register_device(PCIBus *bus, const char
> *name,
>
> pci_dev = qemu_mallocz(instance_size);
> pci_dev = do_pci_register_device(pci_dev, bus, name, devfn,
> - config_read, config_write);
> + config_read, config_write,
> + pci_conf_init_type00);
> return pci_dev;
> }
> +
> static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr)
> {
> return addr + pci_mem_base;
> @@ -1194,6 +1213,59 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num,
> int slot, int function)
> return bus->devices[PCI_DEVFN(slot, function)];
> }
>
> +static void pci_conf_init_type01(PCIDevice *d)
> +{
> + uint32_t addr;
> + uint32_t config_size = pcie_config_size(d);
> +
> + pci_set_word(&d->wmask[PCI_COMMAND],
> + PCI_COMMAND_IO |
> + PCI_COMMAND_MEMORY |
> + PCI_COMMAND_MASTER |
> + PCI_COMMAND_SPECIAL |
> + PCI_COMMAND_INVALIDATE |
> + PCI_COMMAND_VGA_PALETTE |
> + PCI_COMMAND_PARITY |
> + PCI_COMMAND_WAIT |
> + PCI_COMMAND_SERR |
> + PCI_COMMAND_FAST_BACK |
> + PCI_COMMAND_INTX_DISABLE);
> +
> + d->wmask[PCI_CACHE_LINE_SIZE] = 0xff;
> + d->wmask[PCI_LATENCY_TIMER] = 0xff;
> +
> + d->wmask[PCI_PRIMARY_BUS] = 0xff;
> + d->wmask[PCI_SECONDARY_BUS] = 0xff;
> + d->wmask[PCI_SUBORDINATE_BUS] = 0xff;
> + d->wmask[PCI_SEC_LATENCY_TIMER] = 0xff;
> + d->wmask[PCI_IO_BASE] = PCI_IO_RANGE_MASK & 0xff;
> + d->wmask[PCI_IO_LIMIT] = PCI_IO_RANGE_MASK & 0xff;
> +
> + /* sec status isn't emulated (yet) */
> + d->wmask[PCI_SEC_STATUS] = 0;
> +
> + pci_set_word(&d->wmask[PCI_MEMORY_BASE], PCI_MEMORY_RANGE_MASK & 0xffff);
d->wmask + PCI_MEMORY_BASE, same elsewhere.
Especially because you don't address a single byte with pci_set_word,
so you really pass it pointer into array, not pointer to specific field
which just happens to be inside array.
> + pci_set_word(&d->wmask[PCI_MEMORY_LIMIT], PCI_MEMORY_RANGE_MASK &
> 0xffff);
> + pci_set_word(&d->wmask[PCI_PREF_MEMORY_BASE], PCI_PREF_RANGE_MASK &
> 0xffff);
> + pci_set_word(&d->wmask[PCI_PREF_MEMORY_LIMIT], PCI_PREF_RANGE_MASK &
> 0xffff);
> + pci_set_long(&d->wmask[PCI_PREF_BASE_UPPER32], 0xffffffff);
> + pci_set_long(&d->wmask[PCI_PREF_LIMIT_UPPER32], 0xffffffff);
This is a lot of code, but the result is that a ton of registers
are filled with all ones. Let's just memset a range to 0xff.
When you do it this way, you will see that code is basically
identical to regular devices.
> +
> + d->wmask[PCI_INTERRUPT_LINE] = 0xff;
> + pci_set_word(&d->wmask[PCI_BRIDGE_CONTROL], 0xffff);
> +
> + /* device dependent part */
> + for (addr = PCI_CONFIG_HEADER_SIZE; addr < config_size; addr++)
> + d->wmask[addr] = 0xff;
> +}
> +
> +static void pci_conf_init_type02(PCIDevice *d)
> +{
> + fprintf(stderr,
> + "ERROR: pci header type 02(CARDBUS) isn't supported yet.\n");
> + abort();
> +}
> +
so why write it? type 0x3 isn't supported either.
> int pci_bridge_initfn(PCIDevice *pci_dev)
> {
> uint8_t *pci_conf = pci_dev->config;
> @@ -1263,11 +1335,19 @@ static int pci_qdev_init(DeviceState *qdev,
> DeviceInfo *base)
> PCIDeviceInfo *info = container_of(base, PCIDeviceInfo, qdev);
> PCIBus *bus;
> int devfn;
> + const pci_conf_init_t conf_init[] = {
> + pci_conf_init_type00,
> + pci_conf_init_type01,
> + pci_conf_init_type02,
> + };
>
> bus = FROM_QBUS(PCIBus, qdev_get_parent_bus(qdev));
> devfn = pci_dev->devfn;
> + assert(0 <= info->header_type);
so make it unsigned?
> + assert(info->header_type <= ARRAY_SIZE(conf_init));
> pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn,
> - info->config_read, info->config_write);
> + info->config_read, info->config_write,
> + conf_init[info->header_type]);
Overdoing table-driven here. Simple switch would catch
errors faster, e.g. with default: value.
> assert(pci_dev);
> return info->init(pci_dev);
> }
> diff --git a/hw/pci.h b/hw/pci.h
> index 5cf0b59..aadd1a5 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -106,6 +106,14 @@ typedef struct PCIIORegion {
> #define PCI_COMMAND_IO 0x1 /* Enable response in I/O space
> */
> #define PCI_COMMAND_MEMORY 0x2 /* Enable response in Memory space */
> #define PCI_COMMAND_MASTER 0x4 /* Enable bus master */
> +#define PCI_COMMAND_SPECIAL 0x8 /* Enable response to special cycles */
> +#define PCI_COMMAND_INVALIDATE 0x10 /* Use memory write and invalidate */
> +#define PCI_COMMAND_VGA_PALETTE 0x20 /* Enable palette snooping */
> +#define PCI_COMMAND_PARITY 0x40 /* Enable parity checking */
> +#define PCI_COMMAND_WAIT 0x80 /* Enable address/data stepping */
> +#define PCI_COMMAND_SERR 0x100 /* Enable SERR */
> +#define PCI_COMMAND_FAST_BACK 0x200 /* Enable back-to-back writes */
> +#define PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable */
> #define PCI_STATUS 0x06 /* 16 bits */
> #define PCI_REVISION_ID 0x08 /* 8 bits */
> #define PCI_CLASS_PROG 0x09 /* Reg. Level Programming
> Interface */
> @@ -164,7 +172,32 @@ typedef struct PCIIORegion {
>
> /* Header type 1 (PCI-to-PCI bridges) */
> #define PCI_SUBORDINATE_BUS 0x1a /* Highest bus number behind the
> bridge */
> +#define PCI_SEC_LATENCY_TIMER 0x1b /* Latency timer for secondary
> interface */
> +#define PCI_IO_BASE 0x1c /* I/O range behind the bridge */
> +#define PCI_IO_LIMIT 0x1d
> +#define PCI_IO_RANGE_TYPE_MASK 0x0fUL /* I/O bridging type */
> +#define PCI_IO_RANGE_TYPE_16 0x00
> +#define PCI_IO_RANGE_TYPE_32 0x01
> +#define PCI_IO_RANGE_MASK (~0x0fUL)
> +#define PCI_MEMORY_BASE 0x20 /* Memory range behind */
> +#define PCI_MEMORY_LIMIT 0x22
> +#define PCI_MEMORY_RANGE_TYPE_MASK 0x0fUL
> +#define PCI_MEMORY_RANGE_MASK (~0x0fUL)
> +#define PCI_PREF_MEMORY_BASE 0x24 /* Prefetchable memory range behind
> */
> +#define PCI_PREF_MEMORY_LIMIT 0x26
> +#define PCI_PREF_RANGE_TYPE_MASK 0x0fUL
> +#define PCI_PREF_RANGE_TYPE_32 0x00
> +#define PCI_PREF_RANGE_TYPE_64 0x01
> +#define PCI_PREF_RANGE_MASK (~0x0fUL)
> +#define PCI_PREF_BASE_UPPER32 0x28 /* Upper half of prefetchable memory
> range */
> +#define PCI_PREF_LIMIT_UPPER32 0x2c
> +#define PCI_IO_BASE_UPPER16 0x30 /* Upper half of I/O addresses */
> +#define PCI_IO_LIMIT_UPPER16 0x32
> +/* 0x34 same as for htype 0 */
> +/* 0x35-0x3b is reserved */
> #define PCI_ROM_ADDRESS1 0x38 /* Same as PCI_ROM_ADDRESS, but for
> htype 1 */
> +/* 0x3c-0x3d are same as for htype 0 */
> +#define PCI_BRIDGE_CONTROL 0x3e
There's a ton of registers here we do not really need, since you can
initialize a range to 0xff without looking at what is in the range. And
frankly I prefer it this way rather than spend time checking each value.
> /* Size of the standard PCI config header */
> #define PCI_CONFIG_HEADER_SIZE 0x40
> @@ -377,6 +410,9 @@ typedef struct {
> PCIConfigReadFunc *config_read;
> PCIConfigWriteFunc *config_write;
>
> + /* pci config header type */
> + uint8_t header_type;
> +
We don't need this in PCIDeviceInfo, this is part of
config space already.
> /* pcie stuff */
> int pcie;
> } PCIDeviceInfo;
> --
> 1.6.0.2
- [Qemu-devel] [PATCH 02/23] pci: use appropriate PRIs in PCI_DPRINTF() for portability., (continued)
- [Qemu-devel] [PATCH 02/23] pci: use appropriate PRIs in PCI_DPRINTF() for portability., Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 05/23] pci: use PCI_SLOT() and PCI_FUNC()., Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 03/23] pci: introduce constant PCI_NUM_PINS for the number of interrupt pins, 4., Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 01/23] pci: fix PCI_DPRINTF() wrt variadic macro., Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 10/23] pci: introduce FMT_PCIBUS for printf format for pcibus_t., Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 17/23] pci: fix pci_default_write_config(), Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 13/23] pci: make pci configuration transaction more accurate., Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 22/23] pci: initialize wmask according to pci header type., Isaku Yamahata, 2009/10/05
- [Qemu-devel] Re: [PATCH 22/23] pci: initialize wmask according to pci header type.,
Michael S. Tsirkin <=
- [Qemu-devel] [PATCH 06/23] pci: define a constant to represent a unmapped bar and use it., Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 08/23] pci: use helper functions to access pci config space., Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 07/23] pci: helper functions to access PCIDevice::config, Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 21/23] pci/brdige: qdevfy and initialize secondary bus and subordinate bus., Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 23/23] pci/monitor: print out bridge's filtering values and so on., Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 04/23] pci: use the symbolic constant, PCI_ROM_ADDRESS_ENABLE instead of 1., Isaku Yamahata, 2009/10/05
- [Qemu-devel] [PATCH 18/23] pci: add helper functions for pci config write function., Isaku Yamahata, 2009/10/05