On Wed, Dec 22, 2010 at 11:50:14AM +0900, Isaku Yamahata wrote:
> 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.
>
>
> > The second part makes existing code conditional on that value.
>
> What issue are you addressing?
Yes I'd like to know that too:
> 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,
>
>
> >
> > 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))) {
You want to make memory/io bits in the command register read-only
and hardwired to 1? This seems out of spec, no?
> > + /* Reset region address, as it it is not read-only */
> > + 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
> >
> >
>
> --
> yamahata
