[Qemu-devel] Re: pci_default_config_write() clean up.

["Michael S. Tsirkin" <mst@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