Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space

[Michael S. Tsirkin]

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 <address@hidden>
> > 
> > v1:
> > * Rebased.
> > 
> > Signed-off-by: Hervé Poussineau <address@hidden>
> > Cc: Michael S. Tsirkin <address@hidden>
> > Signed-off-by: Andreas Färber <address@hidden>
> > ---
> >  
> >  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