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

[Isaku Yamahata]

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,


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

-- 
yamahata