From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1M1zvu-0006eN-MH for qemu-devel@nongnu.org; Thu, 07 May 2009 05:30:26 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1M1zvp-0006aV-Bv for qemu-devel@nongnu.org; Thu, 07 May 2009 05:30:25 -0400 Received: from [199.232.76.173] (port=43567 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1M1zvp-0006aS-2s for qemu-devel@nongnu.org; Thu, 07 May 2009 05:30:21 -0400 Received: from mx2.redhat.com ([66.187.237.31]:50356) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1M1zvn-0006Gf-VZ for qemu-devel@nongnu.org; Thu, 07 May 2009 05:30:20 -0400 Date: Thu, 7 May 2009 12:29:08 +0300 From: "Michael S. Tsirkin" Message-ID: <20090507092908.GC32039@redhat.com> References: <20090507084019.GA25512%yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090507084019.GA25512%yamahata@valinux.co.jp> Subject: [Qemu-devel] Re: pci_default_config_write() clean up. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: mtosatti@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com In my opinion, the approach of a mask-filling function is cleaner, maintaining the tables manually at you do will be more fragile. For example: On Thu, May 07, 2009 at 05:40:19PM +0900, Isaku Yamahata wrote: > +static const struct PCIConfigReg > +pci_default_config_regs_type_01[PCI_CONFIG_SPACE_SIZE] = > +{ > + /* Vendor ID, Device ID: read only */ > + [PCI_COMMAND] = { > + .wmask = > + (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), > + .changed = pci_update_mappings, > + }, > + [PCI_COMMAND + 1] = { > + .wmask = > + (PCI_COMMAND_SERR | > + PCI_COMMAND_FAST_BACK | > + PCI_COMMAND_INTX_DISABLE) >> 8, > + .changed = NULL, > + }, > + [PCI_STATUS ... (PCI_STATUS + 1)] = { > + /* nothing is emulated at this moment */ > + .wmask = 0, > + .changed = NULL, > + }, > + /* revision id, class code: read only */ > + [PCI_CACHE_LINE_SIZE] = { > + .wmask = 0xff, > + .changed = NULL, > + }, > + [PCI_LATENCY_TIMER] = { > + .wmask = 0xff, > + .changed = NULL, > + }, > + /* header type: read only */ > + [PCI_BIST] = { > + .wmask = 0, /* BIST emulation isn't implemented */ > + .changed = NULL, > + }, > + [PCI_BASE_ADDRESS_0 ... (PCI_BASE_ADDRESS_1 + 3)] = { > + .wmask = 0, /* this will be updated by pci_register_io_region() */ > + .changed = pci_update_mappings, > + }, > + [PCI_PRIMARY_BUS] = { > + .wmask = 0xff, > + .changed = NULL, > + }, > + [PCI_SECONDARY_BUS] = { > + .wmask = 0xff, > + .changed = pci_conf_secondary_bus_changed, > + }, > + [PCI_SUBORDINATE_BUS] = { > + .wmask = 0xff, > + .changed = NULL, > + }, > + [PCI_SEC_LATENCY_TIMER] = { > + .wmask = 0xff, > + .changed = NULL, > + }, > + [PCI_IO_BASE] = { > + .wmask = PCI_IO_RANGE_MASK & 0xff, > + .changed = NULL, > + }, > + [PCI_IO_LIMIT] = { > + .wmask = PCI_IO_RANGE_MASK & 0xff, > + .changed = NULL, > + }, > + [PCI_SEC_STATUS ... (PCI_SEC_STATUS + 1)] = { > + .wmask = 0xff, > + .changed = NULL, > + }, This looks wrong: I think the status register is read-only or clear on write. > + [PCI_MEMORY_BASE] = { > + .wmask = PCI_MEMORY_RANGE_MASK & 0xff, > + .changed = NULL, > + }, > + [PCI_MEMORY_BASE + 1] = { > + .wmask = 0xff, > + .changed = NULL, > + }, > + [PCI_MEMORY_LIMIT] = { > + .wmask = PCI_MEMORY_RANGE_MASK & 0xff, > + .changed = NULL, > + }, > + [PCI_MEMORY_LIMIT + 1] = { > + .wmask = 0xff, You really should have PCI_MEMORY_RANGE_MASK >> 8 here, right? > + .changed = NULL, > + }, > + [PCI_PREF_MEMORY_BASE] = { > + .wmask = PCI_PREF_RANGE_MASK & 0xff, > + .changed = NULL, > + }, > + [PCI_PREF_MEMORY_BASE + 1] = { > + .wmask = 0xff, > + .changed = NULL, > + }, > + [PCI_PREF_MEMORY_LIMIT] = { > + .wmask = PCI_PREF_RANGE_MASK & 0xff, > + .changed = NULL, > + }, > + [PCI_PREF_MEMORY_LIMIT + 1] = { > + .wmask = 0xff, > + .changed = NULL, > + }, Same here .... etc ... > + [PCI_PREF_BASE_UPPER32 ... (PCI_PREF_BASE_UPPER32 + 3)] = { > + .wmask = 0xff, > + .changed = NULL, > + }, > + [PCI_PREF_LIMIT_UPPER32 ... (PCI_PREF_LIMIT_UPPER32 + 3)] = { > + .wmask = 0xff, > + .changed = NULL, > + }, AFAIK the "..." construct is a GNU extension. Would be better to stick with C99? > + [PCI_IO_BASE_UPPER16] = { > + .wmask = 0, /* only support 64K io port */ > + .changed = NULL, > + }, > + [PCI_IO_LIMIT_UPPER16] = { > + .wmask = 0, /* only support 64K io port */ > + .changed = NULL, > + }, > + [PCI_CAPABILITY_LIST] = { > + .wmask = 0, > + .changed = NULL, > + }, > + [PCI_ROM_ADDRESS1 ... (PCI_ROM_ADDRESS1 + 3)] = { > + .wmask = 0, /* this will be updated by pci_register_io_region() */ > + .changed = pci_update_mappings, > + }, > + /* 0x35 - 0x37 reserved */ > + [PCI_INTERRUPT_LINE] = { > + .wmask = 0xff, > + .changed = NULL, > + }, > + [PCI_INTERRUPT_PIN] = { > + .wmask = 0, > + .changed = NULL, > + }, > + [PCI_BRIDGE_CONTROL] = { > + .wmask = 0xff, > + .changed = NULL, > + }, > + > + /* device dependent part */ > + [PCI_CONFIG_HEADER_SIZE ... (PCI_CONFIG_SPACE_SIZE - 1)] = { > + .wmask = 0xff, > + .changed = NULL, > + }, > +}; > + > PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, > pci_map_irq_fn map_irq, const char *name) > { > PCIBridge *s; > s = (PCIBridge *)pci_register_device(bus, name, sizeof(PCIBridge), > - devfn, NULL, pci_bridge_write_config); > + devfn, NULL, NULL); > > pci_config_set_vendor_id(s->dev.config, vid); > pci_config_set_device_id(s->dev.config, did); > + memcpy(s->dev.config_regs, pci_default_config_regs_type_01, > + sizeof(s->dev.config_regs)); > > s->dev.config[0x04] = 0x06; // command = bus master, pci mem > s->dev.config[0x05] = 0x00; > diff --git a/hw/pci.h b/hw/pci.h > index ff858a1..c220a19 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -98,16 +98,52 @@ typedef struct PCIIORegion { > #define PCI_COMMAND 0x04 /* 16 bits */ > #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_DEVICE 0x0a /* Device class */ > +#define PCI_CACHE_LINE_SIZE 0x0c /* 8 bits */ > +#define PCI_LATENCY_TIMER 0x0d /* 8 bits */ > #define PCI_HEADER_TYPE 0x0e /* 8 bits */ > #define PCI_HEADER_TYPE_NORMAL 0 > #define PCI_HEADER_TYPE_BRIDGE 1 > #define PCI_HEADER_TYPE_CARDBUS 2 > #define PCI_HEADER_TYPE_MULTI_FUNCTION 0x80 > +#define PCI_BIST 0x0f /* 8 bits */ > +#define PCI_BIST_CODE_MASK 0x0f /* Return result */ > +#define PCI_BIST_START 0x40 /* 1 to start BIST, 2 secs or less */ > +#define PCI_BIST_CAPABLE 0x80 /* 1 if BIST capable */ > +#define PCI_BASE_ADDRESS_0 0x10 /* 32 bits */ > +#define PCI_BASE_ADDRESS_1 0x14 /* 32 bits [htype 0,1 only] */ > +#define PCI_BASE_ADDRESS_2 0x18 /* 32 bits [htype 0 only] */ > +#define PCI_BASE_ADDRESS_3 0x1c /* 32 bits */ > +#define PCI_BASE_ADDRESS_4 0x20 /* 32 bits */ > +#define PCI_BASE_ADDRESS_5 0x24 /* 32 bits */ > +#define PCI_BASE_ADDRESS_SPACE 0x01 /* 0 = memory, 1 = I/O */ > +#define PCI_BASE_ADDRESS_SPACE_IO 0x01 > +#define PCI_BASE_ADDRESS_SPACE_MEMORY 0x00 > +#define PCI_BASE_ADDRESS_MEM_TYPE_MASK 0x06 > +#define PCI_BASE_ADDRESS_MEM_TYPE_32 0x00 /* 32 bit address */ > +#define PCI_BASE_ADDRESS_MEM_TYPE_1M 0x02 /* Below 1M [obsolete] */ > +#define PCI_BASE_ADDRESS_MEM_TYPE_64 0x04 /* 64 bit address */ > +#define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08 /* prefetchable? */ > +#define PCI_BASE_ADDRESS_MEM_MASK (~0x0fUL) > +#define PCI_BASE_ADDRESS_IO_MASK (~0x03UL) > +#define PCI_PRIMARY_BUS 0x18 /* Primary bus number */ > +#define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */ > +#define PCI_SEC_STATUS 0x1e /* Secondary status register, only bit 14 used */ > +#define PCI_CARDBUS_CIS 0x28 > #define PCI_SUBSYSTEM_VENDOR_ID 0x2c /* 16 bits */ > #define PCI_SUBSYSTEM_ID 0x2e /* 16 bits */ > +#define PCI_CAPABILITY_LIST 0x34 /* Offset of first capability list entry */ > #define PCI_INTERRUPT_LINE 0x3c /* 8 bits */ > #define PCI_INTERRUPT_PIN 0x3d /* 8 bits */ > #define PCI_MIN_GNT 0x3e /* 8 bits */ > @@ -117,6 +153,10 @@ typedef struct PCIIORegion { > #define PCI_SUBVENDOR_ID 0x2c /* obsolete, use PCI_SUBSYSTEM_VENDOR_ID */ > #define PCI_SUBDEVICE_ID 0x2e /* obsolete, use PCI_SUBSYSTEM_ID */ > > +#define PCI_ROM_ADDRESS 0x30 /* Bits 31..11 are address, 10..1 reserved */ > +#define PCI_ROM_ADDRESS_ENABLE 0x01 > +#define PCI_ROM_ADDRESS_MASK (~0x7ffUL) > + > /* Bits in the PCI Status Register (PCI 2.3 spec) */ > #define PCI_STATUS_RESERVED1 0x007 > #define PCI_STATUS_INT_STATUS 0x008 > @@ -137,9 +177,65 @@ typedef struct PCIIORegion { > > #define PCI_COMMAND_RESERVED_MASK_HI (PCI_COMMAND_RESERVED >> 8) > > +/* Header type 1 (PCI-to-PCI bridges) */ > +#define PCI_PRIMARY_BUS 0x18 /* Primary bus number */ > +#define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */ > +#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_SEC_STATUS 0x1e /* Secondary status register, only bit 14 used */ > +#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 > +#define PCI_BRIDGE_CTL_PARITY 0x01 /* Enable parity detection on secondary interface */ > +#define PCI_BRIDGE_CTL_SERR 0x02 /* The same for SERR forwarding */ > +#define PCI_BRIDGE_CTL_ISA 0x04 /* Enable ISA mode */ > +#define PCI_BRIDGE_CTL_VGA 0x08 /* Forward VGA addresses */ > +#define PCI_BRIDGE_CTL_MASTER_ABORT 0x20 /* Report master aborts */ > +#define PCI_BRIDGE_CTL_BUS_RESET 0x40 /* Secondary bus reset */ > +#define PCI_BRIDGE_CTL_FAST_BACK 0x80 /* Fast Back2Back enabled on secondary interface */ > + > +/* Bits in the PCI Command Register (PCI 2.3 spec) */ > +#define PCI_COMMAND_RESERVED_BRIDGE 0xf880 > + > +#define PCI_COMMAND_RESERVED_MASK_HI_BRIDGE (PCI_COMMAND_RESERVED >> 8) > + > +/* Size of the standard PCI config header */ > +#define PCI_CONFIG_HEADER_SIZE 0x40 > +/* Size of the standard PCI config space */ > +#define PCI_CONFIG_SPACE_SIZE 0x100 > + > +typedef void (*pci_config_changed_t)(struct PCIDevice *d); This does not pass the info on the write performed. In turn this means that this callback can't be used to properly emulate clear on write registers in status. > +struct PCIConfigReg { > + uint8_t wmask; > + pci_config_changed_t changed; > +}; > + > struct PCIDevice { > /* PCI config space */ > - uint8_t config[256]; > + uint8_t config[PCI_CONFIG_SPACE_SIZE]; > + struct PCIConfigReg config_regs[PCI_CONFIG_SPACE_SIZE]; > > /* the following fields are read only */ > PCIBus *bus; -- MST