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

[Andreas Färber]

Hervé,

Am 22.12.2010 um 07:49 schrieb Michael S. Tsirkin:

> 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 <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.
>
> 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?

I haven't noticed a follow-up patch of yours.

Since I don't know what to do here, I'll have to take this out of the  
PReP queue for now.
Without this patch, I get to at least the second bootloader icon, the  
PCI graphics still work. What particular symptoms did you observe wrt  
the i82378 that we can reproduce?

Thanks,
Andreas

> 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