[Qemu-devel] Re: [PATCH v9 1/8] pci: revise pci command register initialization

[Isaku Yamahata]

On Tue, Nov 16, 2010 at 12:50:19PM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 16, 2010 at 05:26:05PM +0900, Isaku Yamahata wrote:
> > This patch cleans up command register initialization with
> > comments. It also fixes the initialization of io/memory bit of command 
> > register.
> > Those bits for type 1 device is RW.
> > Those bits for type 0 device is
> >   RO = 0 if it has no io/memory BAR
> >   RW if it has io/memory BAR
> > 
> > Signed-off-by: Isaku Yamahata <address@hidden>
> 
> There's a bug here: you can not assume that device that has
> no io BAR claims no io transactions.

Which device are you mentioning?
Such device should set the bit by itself, not by pci generic layer.


> Another bug is that migrating from qemu where a bit is writeable to one
> where it's RO creates a situation where a RW bit becomes RO, or the
> reverse, which might confuse guests.  So we will need a compatibility
> flag and set it for old machine types.

We needs to keep compatibility. Which way do you prefer?

- don't care: no way

- introduce global property to indicate compat qemu version or flags
  something like if (compat version <= 0.13) old behaviour...
  or if (flags & ...)

- introduce global-pci property

- introduce pci bus property
  Users needs to specify this property for all pci devices.

- Don't change common code(pci.c), and provide a helper function.
  Each device which needs new behavior like pcie calls it.
  Probably each device may provide property to specify compat behavior

- any other?

> 
> > ---
> > Changes v8 -> v9
> > - patch squash
> > ---
> >  hw/pci.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 57 insertions(+), 1 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 962886e..2fc8ab1 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -544,8 +544,53 @@ static void pci_init_wmask(PCIDevice *dev)
> >  
> >      dev->wmask[PCI_CACHE_LINE_SIZE] = 0xff;
> >      dev->wmask[PCI_INTERRUPT_LINE] = 0xff;
> > +
> > +    /*
> > +     * bit 0: PCI_COMMAND_IO
> > +     *        type 0: if IO BAR is used, RW
> > +     *                This is handled by pci_register_bar()
> > +     *        type 1: RW:
> > +     *                This is fixed by pci_init_wmask_bridge()
> > +     * bit 1: PCI_COMMAND_MEMORY
> > +     *        type 0: if IO BAR is used, RW
> > +     *                This is handled by pci_register_bar()
> > +     *        type 1: RW
> > +     *                This is fixed by pci_init_wmask_bridge()
> > +     * bit 2: PCI_COMMAND_MASTER
> > +     *        type 0: RW if bus master
> > +     *        type 1: RW
> > +     * bit 3: PCI_COMMAND_SPECIAL
> > +     *        RO=0, optionally RW: Such device should set this bit itself
> > +     * bit 4: PCI_COMMAND_INVALIDATE
> > +     *        RO=0, optionally RW: Such device should set this bit itself
> > +     * bit 5: PCI_COMMAND_VGA_PALETTE
> > +     *        RO=0, optionally RW: Such device should set this bit itself
> > +     * bit 6: PCI_COMMAND_PARITY
> > +     *        RW with exceptions: Such device should clear this bit itself
> > +     *        Given that qemu doesn't emulate pci bus cycles, so that there
> > +     *        is no place to generate parity error. So just making this
> > +     *        register RW is okay because there is no place which refers
> > +     *        this bit.
> > +     *        TODO: When device assignment tried to inject PERR# into qemu,
> > +     *              some extra work would be needed.
> > +     * bit 7: PCI_COMMAND_WAIT: reserved (PCI 3.0)
> > +     *        RO=0
> > +     * bit 8: PCI_COMMAND_SERR
> > +     *        RW with exceptions: Such device should clear this bit itself
> > +     *        Given that qemu doesn't emulate pci bus cycles, so that there
> > +     *        is no place to generate system error. So just making this
> > +     *        register RW is okay because there is no place which refers
> > +     *        this bit.
> > +     *        TODO: When device assignment tried to inject SERR# into qemu,
> > +     *              some extra work would be needed.
> > +     * bit 9: PCI_COMMAND_FAST_BACK
> > +     *        RO=0, optionally RW: Such device should set this bit itself
> > +     * bit 10: PCI_COMMAND_INTX_DISABLE
> > +     *         RW
> > +     * bit 11-15: reserved
> > +     */
> 
> Let's document non-obvious things, like maybe COMMAND_PARITY/COMMAND_SERR.
> I don't cherish writing each bit up in two places.
> 
> >      pci_set_word(dev->wmask + PCI_COMMAND,
> > -                 PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER |
> > +                 PCI_COMMAND_MASTER | PCI_COMMAND_PARITY | 
> > PCI_COMMAND_SERR |
> >                   PCI_COMMAND_INTX_DISABLE);
> >  
> >      memset(dev->wmask + PCI_CONFIG_HEADER_SIZE, 0xff,
> > @@ -554,6 +599,9 @@ static void pci_init_wmask(PCIDevice *dev)
> >  
> >  static void pci_init_wmask_bridge(PCIDevice *d)
> >  {
> > +    pci_word_test_and_set_mask(d->wmask + PCI_COMMAND,
> > +                               PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
> > +
> >      /* PCI_PRIMARY_BUS, PCI_SECONDARY_BUS, PCI_SUBORDINATE_BUS and
> >         PCI_SEC_LETENCY_TIMER */
> >      memset(d->wmask + PCI_PRIMARY_BUS, 0xff, 4);
> > @@ -791,6 +839,14 @@ void pci_register_bar(PCIDevice *pci_dev, int 
> > region_num,
> >      if (region_num == PCI_ROM_SLOT) {
> >          /* ROM enable bit is writeable */
> >          wmask |= PCI_ROM_ADDRESS_ENABLE;
> > +    } else {
> > +        if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> > +            pci_word_test_and_set_mask(pci_dev->wmask + PCI_COMMAND,
> > +                                       PCI_COMMAND_IO);
> > +        } else {
> > +            pci_word_test_and_set_mask(pci_dev->wmask + PCI_COMMAND,
> > +                                       PCI_COMMAND_MEMORY);
> > +        }
> >      }
> >      pci_set_long(pci_dev->config + addr, type);
> >      if (!(r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> > -- 
> > 1.7.1.1
> 

-- 
yamahata