[Qemu-devel] Re: [PATCH V5 28/29] pci: initialize pci config headers depending it pci header type.

["Michael S. Tsirkin" <mst@redhat.com>]

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 <yamahata@valinux.co.jp>
> > > ---
> > >  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