On Wed, Dec 22, 2010 at 07:30:33AM +0100, Hervé Poussineau wrote:
> Isaku Yamahata a écrit :
> >On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
> >>From: Hervé Poussineau <hpous...@reactos.org>
> >>
> >>v1:
> >>* Rebased.
> >>
> >>Signed-off-by: Hervé Poussineau <hpous...@reactos.org>
> >>Cc: Michael S. Tsirkin <m...@redhat.com>
> >>Signed-off-by: Andreas Färber <andreas.faer...@web.de>
> >>---
> >> Hello Michael,
> >> Could you please take a look at this? I'm out of my field here.
> >> The intention of the first part appears to be to save (val & ~mask),
> >> whereas the inline helper would've returned (val & mask).
> >
> >Such behavior is intended.
> >The returned value is just discarded in this case.
> >test-and-clear means
> > clear the bits
> > return if those cleared bits were really set.
> >
What about this first chunk? Is it necessary.
> >> The second part makes existing code conditional on that value.
> >
> >What issue are you addressing?
> >Although the spec doesn't says about the default value of BAR registers
> >after reset, the current code assumes that almost all the pci devices clear
> >those registers.
> >Anyway after cold/warm reset firmware sets up BARs, so it doesn't matter.
> >You, however, seem to want to keep BARs over resets.
> >
> >thanks,
> >
> >
> As you have seen, the intend here is to be able to keep BARs over resets.
> It is required for some really specific devices, like a PCI to ISA
> bridge, where MMIO is always at the same address.
> In that case, the device keeps PCI_COMMAND_MEMORY and/or
> PCI_COMMAND_IO flags as read-only.
>
> Hervé
Aha. Are the BARs still writeable? If not maybe that's the right thing
to check? If yes maybe the device simply should have a reset
handler to rewrite them?
Also I would like the above text (maybe a bit shortened) to replace the comment:
/* Reset region address, as it it is not read-only */
the general idea is to document the reason the code
is what it is, not restate what it does.
Something similar should also go into the commit message I think.
> >> Regards,
> >> Andreas
> >> hw/pci.c | 22 ++++++++++++++--------
> >> 1 files changed, 14 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/hw/pci.c b/hw/pci.c
> >>index ef00d20..4db4b1f 100644
> >>--- a/hw/pci.c
> >>+++ b/hw/pci.c
> >>@@ -140,6 +140,7 @@ static void pci_update_irq_status(PCIDevice *dev)
> >> static void pci_device_reset(PCIDevice *dev)
> >> {
> >> int r;
> >>+ uint16_t cmd;
> >> /* TODO: call the below unconditionally once all pci devices
> >> * are qdevified */
> >> if (dev->qdev.info) {
> >>@@ -149,9 +150,10 @@ static void pci_device_reset(PCIDevice *dev)
> >> dev->irq_state = 0;
> >> pci_update_irq_status(dev);
> >> /* Clear all writeable bits */
> >>- pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
> >>- pci_get_word(dev->wmask + PCI_COMMAND) |
> >>- pci_get_word(dev->w1cmask + PCI_COMMAND));
> >>+ cmd = pci_get_word(dev->config + PCI_COMMAND);
> >>+ cmd &= ~(pci_get_word(dev->wmask + PCI_COMMAND) |
> >>+ pci_get_word(dev->w1cmask + PCI_COMMAND));
> >>+ pci_set_word(dev->config + PCI_COMMAND, cmd);
> >> pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
> >> pci_get_word(dev->wmask + PCI_STATUS) |
> >> pci_get_word(dev->w1cmask + PCI_STATUS));
> >>@@ -163,11 +165,15 @@ static void pci_device_reset(PCIDevice *dev)
> >> continue;
> >> }
> >>- if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> >>- region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> >>- pci_set_quad(dev->config + pci_bar(dev, r), region->type);
> >>- } else {
> >>- pci_set_long(dev->config + pci_bar(dev, r), region->type);
> >>+ if ((region->type == PCI_BASE_ADDRESS_SPACE_IO && !(cmd &
> >>PCI_COMMAND_MEMORY)) ||
> >>+ (region->type == PCI_BASE_ADDRESS_SPACE_MEMORY && !(cmd &
> >>PCI_COMMAND_IO))) {
These lines are too long.
> >>+ /* Reset region address, as it it is not read-only */
Checking wmask would be more straight-forward?
> >>+ if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> >>+ region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> >>+ pci_set_quad(dev->config + pci_bar(dev, r), region->type);
> >>+ } else {
> >>+ pci_set_long(dev->config + pci_bar(dev, r), region->type);
> >>+ }
> >> }
> >> }
> >> pci_update_mappings(dev);
> >>--
> >>1.7.3.4
> >>
> >>
> >
