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