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

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Jun 12, 2011 at 09:50:45PM +0200, Hervé Poussineau wrote:
> Michael S. Tsirkin a écrit :
> >On Sun, Jun 12, 2011 at 02:40:35PM +0200, Hervé Poussineau wrote:
> >>Andreas,
> >>
> >>Andreas Färber a écrit :
> >>>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 <hpoussin@reactos.org>
> >>>>>>>
> >>>>>>>v1:
> >>>>>>>* Rebased.
> >>>>>>>
> >>>>>>>Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> >>>>>>>Cc: Michael S. Tsirkin <mst@redhat.com>
> >>>>>>>Signed-off-by: Andreas Färber <andreas.faerber@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?
> >>>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
> >>>
> >>Try do do info qtree with and without this patch, and check the
> >>i82378 device
> >>With this patch, I have:
> >>       bar 0: mem at 0x80000000 [0x8000ffff]
> >>       bar 1: mem at 0xc0000000 [0xc0ffffff]
> >>
> >>Without it, I have:
> >>       bar 0: mem at 0xffffffffffffffff [0xfffe]
> >>       bar 1: mem at 0xffffffffffffffff [0xfffffe]
> >>
> >>I think that firmware doesn't initialize BARs for this device.
> >
> >Interesting. So what set the BARs to these values (0x80000000
> >and 0xc0000000)
> Currently, those default values are stored in qdev properties of device,
> and device fills in BARs addresses in its init function
> 
> +static int pci_i82378_init(PCIDevice *pci_dev)
> ...
> +    /* Make addresses read only */
> +    pci_set_word(pci_dev->wmask + PCI_COMMAND,
> +                 PCI_COMMAND_SPECIAL);
> +    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 0 * 4, pci->isa_io_base);
> +    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 1 * 4, pci->isa_mem_base);
> 
> Regards
> 
> Hervé
> 

I see. So the right thing to do wrt BARs is probably
to restore these values on system reset as well?

It might be a good idea to replace 'Make addresses read only'
with 'only special cycle enable bit is writeable'.

-- 
MST