From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mxila-0005Fg-G9 for qemu-devel@nongnu.org; Tue, 13 Oct 2009 10:54:22 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MxilV-0005DH-DO for qemu-devel@nongnu.org; Tue, 13 Oct 2009 10:54:22 -0400 Received: from [199.232.76.173] (port=53504 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MxilV-0005DE-9m for qemu-devel@nongnu.org; Tue, 13 Oct 2009 10:54:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32727) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MxilT-0007ep-Ts for qemu-devel@nongnu.org; Tue, 13 Oct 2009 10:54:16 -0400 Date: Tue, 13 Oct 2009 16:52:05 +0200 From: "Michael S. Tsirkin" Message-ID: <20091013145205.GD17895@redhat.com> References: <1255069742-15724-1-git-send-email-yamahata@valinux.co.jp> <1255069742-15724-29-git-send-email-yamahata@valinux.co.jp> <20091009104232.GC12027@redhat.com> <20091013143123.GH2306%yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091013143123.GH2306%yamahata@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH V5 28/29] pci: initialize pci config headers depending it pci header type. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: qemu-devel@nongnu.org On Tue, Oct 13, 2009 at 11:31:23PM +0900, Isaku Yamahata wrote: > On Fri, Oct 09, 2009 at 12:42:32PM +0200, Michael S. Tsirkin wrote: > > On Fri, Oct 09, 2009 at 03:29:01PM +0900, Isaku Yamahata wrote: > > > - Only sets default subsystem id for header type 00.(normal header type) > > > because header type 01 doesn't have subsystem id, and uses the register > > > for other purpose. So setting default subsystem id doesn't make sense. > > > > > > - initialize wmask more for header type 01.(bridge header type) > > > Without those wmasks, linux was confused not boot. > > > > It was? Can you investigate please? > > Spec says most of these are optional. > > Linux kernel thinks the bridge doesn't support to forwarding > bus command disabling devices behind the bridge. Sorry, had trouble parsing this sentence. > If pci-ide is behind the bridge, kernel panics failing to find > root file system. > Yes, but why does it help to make bus master enable writeable when we never initiate pci transactions from bridge itself? > > > Signed-off-by: Isaku Yamahata > > > --- > > > hw/cirrus_vga.c | 1 - > > > hw/pci.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++------ > > > hw/pci.h | 33 +++++++++++++++++++++++++++++ > > > 3 files changed, 88 insertions(+), 8 deletions(-) > > > > > > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c > > > index ef72c62..c1bafd3 100644 > > > --- a/hw/cirrus_vga.c > > > +++ b/hw/cirrus_vga.c > > > @@ -180,7 +180,6 @@ > > > #define PCI_COMMAND_PALETTESNOOPING 0x0020 > > > #define PCI_COMMAND_PARITYDETECTION 0x0040 > > > #define PCI_COMMAND_ADDRESSDATASTEPPING 0x0080 > > > -#define PCI_COMMAND_SERR 0x0100 > > > #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 3a17cd8..6b51bba 100644 > > > --- a/hw/pci.c > > > +++ b/hw/pci.c > > > @@ -430,14 +430,52 @@ static void pci_init_wmask(PCIDevice *dev) > > > int i; > > > int config_size = pci_config_size(dev); > > > > > > + pci_set_word(dev->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); > > > > At least INTX disable must not be writeable unless > > we implement the masking functionality, and we don't > > OK. > > > > + > > > dev->wmask[PCI_CACHE_LINE_SIZE] = 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) > > > + > > > + for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i++) > > > > I dislike this change. We don't use the old value so ++i > > is cleaner. > > Ok. It was an accidental one change I didn't meant. > > > > > dev->wmask[i] = 0xff; > > > } > > > > > > +static void pci_init_wmask_bridge(PCIDevice *d) > > > +{ > > > + /* PCI_PRIMARY_BUS, PCI_SECONDARY_BUS, PCI_SUBORDINATE_BUS and > > > + PCI_SEC_LETENCY_TIMER */ > > > + memset(d->wmask + PCI_PRIMARY_BUS, 0xff, 4); > > > + > > > + /* sec status isn't emulated (yet) */ > > > + d->wmask[PCI_SEC_STATUS] = 0; > > > > 0 is default value. Replace with /* TODO: emulate PCI_SEC_STATUS */ > > (which bits do we have to emulate there?) > > Ok. > > > > > + /* base and limit */ > > > + d->wmask[PCI_IO_BASE] = PCI_IO_RANGE_MASK & 0xff; > > > + d->wmask[PCI_IO_LIMIT] = PCI_IO_RANGE_MASK & 0xff; > > > > > > & 0xff not necessary. > > OK. > > > > + pci_set_word(d->wmask + PCI_MEMORY_BASE, > > > + PCI_MEMORY_RANGE_MASK & 0xffff); > > > + 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); > > > + > > > + pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff); > > > > Can we just memset(xxx, 0xff, yyy) the whole range, > > and just clear a single byte that is 0? > > It can be done for PCI_RREF_{BASE, LIMIT}_UPPER32 with length 4. > However all base/limit needs to be 0xfff0 or 0xfffffff0. > (not 0, but 0xf0) So it doesn't make code cleaner much. > > > > > > +} > > > + > > > static void pci_config_alloc(PCIDevice *pci_dev) > > > { > > > int config_size = pci_config_size(pci_dev); > > > @@ -462,7 +500,8 @@ static void pci_config_free(PCIDevice *pci_dev) > > > 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, > > > + uint8_t header_type) > > > { > > > if (devfn < 0) { > > > for(devfn = bus->devfn_min ; devfn < 256; devfn += 8) { > > > @@ -479,9 +518,16 @@ 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); > > > + > > > + header_type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION; > > > + if (header_type == PCI_HEADER_TYPE_NORMAL) { > > > + pci_set_default_subsystem_id(pci_dev); > > > + } > > > pci_init_cmask(pci_dev); > > > pci_init_wmask(pci_dev); > > > + if (header_type == PCI_HEADER_TYPE_BRIDGE) { > > > + pci_init_wmask_bridge(pci_dev); > > > + } > > > > > > if (!config_read) > > > config_read = pci_default_read_config; > > > @@ -504,7 +550,8 @@ 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_HEADER_TYPE_NORMAL); > > > return pci_dev; > > > } > > > static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr) > > > @@ -1093,7 +1140,8 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) > > > bus = FROM_QBUS(PCIBus, qdev_get_parent_bus(qdev)); > > > devfn = pci_dev->devfn; > > > pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn, > > > - info->config_read, info->config_write); > > > + info->config_read, info->config_write, > > > + info->header_type); > > > assert(pci_dev); > > > rc = info->init(pci_dev); > > > if (rc != 0) > > > diff --git a/hw/pci.h b/hw/pci.h > > > index 3cd93e9..a933ef1 100644 > > > --- a/hw/pci.h > > > +++ b/hw/pci.h > > > @@ -100,6 +100,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 */ > > > @@ -162,7 +170,25 @@ 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_MASK (~0x0fUL) > > > +#define PCI_MEMORY_BASE 0x20 /* Memory range behind */ > > > +#define PCI_MEMORY_LIMIT 0x22 > > > +#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_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 > > > > > > /* Size of the standard PCI config header */ > > > #define PCI_CONFIG_HEADER_SIZE 0x40 > > > @@ -370,6 +396,13 @@ typedef struct { > > > PCIConfigReadFunc *config_read; > > > PCIConfigWriteFunc *config_write; > > > > > > + /* pci config header type */ > > > + uint8_t header_type; /* this is necessary for initialization > > > + * code to know its header type before > > > + * device specific code can initialize > > > + * configuration space. > > > + */ > > > + > > > /* pcie stuff */ > > > int is_express; /* is this device pci express? > > > * initialization code needs to know this before > > > > -- > yamahata