| Message ID | f784da8cd1e993b58435ebba8b8d003c65e4192a.1289895735.git.yamahata@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
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 <yamahata@valinux.co.jp> There's a bug here: you can not assume that device that has no io BAR claims no io transactions. 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. > --- > 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
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 <yamahata@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 >
On Wed, Nov 17, 2010 at 11:03:14AM +0900, Isaku Yamahata wrote: > 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 <yamahata@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? Look at appendix D in PCI spec. There are many programming classes like display controllers that claim specific addresses without using a BAR. We could code it all up with a huge table. But what we have is much simpler and works well AFAIK. > Such device should set the bit by itself, not by pci generic layer. Since this is PCI spec specified behaviour, I think it's better to have it in a common place. Devices are sure to get it wrong. > > 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? - Don't change behaviour at all. What is the motivation for the change? Why do we bother? What we have is spec compliant, I think, so it's hard for me to believe pcie *needs* the new behaviour.
On Wed, Nov 17, 2010 at 02:02:00PM +0200, Michael S. Tsirkin wrote: > > > 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? > > - Don't change behaviour at all. > > What is the motivation for the change? Why do we bother? What we have > is spec compliant, I think, so it's hard for me to believe pcie *needs* > the new behaviour. AER wants SERR bit to be writable and you requested it as below. I thought, you wanted me to revise PCI_COMMAND and PCI_STATUS initialization. If I misunderstood, can you please elaborate on it? If you accept the following PCI_COMMAND line, I'm fine with dropping this clean up patch. http://lists.nongnu.org/archive/html/qemu-devel/2010-11/msg00131.html > > +void pcie_aer_init(PCIDevice *dev, uint16_t offset) > > +{ > > + PCIExpressDevice *exp; > > + > > + pci_word_test_and_set_mask(dev->wmask + PCI_COMMAND, PCI_COMMAND_SERR); > > + pci_word_test_and_set_mask(dev->w1cmask + PCI_STATUS, > > + PCI_STATUS_SIG_SYSTEM_ERROR); > > + > > I would say we should just set these for all devices. > But if we do my concern is that guest might write 1 to this register, > then we migrate to an old guest and that one can not clear this bit. > Thoughts? Let's add a flag so old machine types can disable this? > > > Also - what about other bits in the status register?
On Thu, Nov 18, 2010 at 11:08:40AM +0900, Isaku Yamahata wrote: > On Wed, Nov 17, 2010 at 02:02:00PM +0200, Michael S. Tsirkin wrote: > > > > 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? > > > > - Don't change behaviour at all. > > > > What is the motivation for the change? Why do we bother? What we have > > is spec compliant, I think, so it's hard for me to believe pcie *needs* > > the new behaviour. > > AER wants SERR bit to be writable and you requested it as below. > I thought, you wanted me to revise PCI_COMMAND and PCI_STATUS initialization. > If I misunderstood, can you please elaborate on it? > If you accept the following PCI_COMMAND line, > I'm fine with dropping this clean up patch. > > http://lists.nongnu.org/archive/html/qemu-devel/2010-11/msg00131.html > > > +void pcie_aer_init(PCIDevice *dev, uint16_t offset) > > > +{ > > > + PCIExpressDevice *exp; > > > + > > > + pci_word_test_and_set_mask(dev->wmask + PCI_COMMAND, PCI_COMMAND_SERR); > > > + pci_word_test_and_set_mask(dev->w1cmask + PCI_STATUS, > > > + PCI_STATUS_SIG_SYSTEM_ERROR); > > > + > > > > I would say we should just set these for all devices. > > But if we do my concern is that guest might write 1 to this register, > > then we migrate to an old guest and that one can not clear this bit. > > Thoughts? Let's add a flag so old machine types can disable this? > > > > > > Also - what about other bits in the status register? I think what you did for PCI_STATUS addressed this comment. Thanks! Yes, for AER we need to enable SERR, and just for SERR, I think a global property or a bus property will be the simplest (I think properties are inherited from bus to device, right?) Something like pci_command_serr_enable, should be a bit property. Default to on and disable for 0.13 and back. Hmm? Also, need to check this and fail aer initialization if not there. Maybe simply don't add aer capability if aer_init fails? Or make it yet another property ...
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 + */ 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) &&
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 <yamahata@valinux.co.jp> --- Changes v8 -> v9 - patch squash --- hw/pci.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 57 insertions(+), 1 deletions(-)