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

[Hervé Poussineau]

Isaku Yamahata a écrit :
> 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?
> 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é

> >  
> >  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))) {
> > +            /* 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
> >
> >
> >     
>
>