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

["Hervé Poussineau" <hpoussin@reactos.org>]

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