Re: [PATCH 12/13] pci-assign: Generic config space access management

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 5165 bytes --]

On 2011-06-28 00:48, Alex Williamson wrote:
>> -    case PCI_CAP_ID_VPD:
>> -    case PCI_CAP_ID_VNDR:
>> -        assigned_dev_pci_write(pci_dev, address, val, len);
>> +    switch (len) {
>> +    case 4:
>> +        direct_mask =
>> +            pci_get_long(assigned_dev->direct_config_write + address);
>> +        full_access = 0xffffffff;
>> +        break;
>> +    case 2:
>> +        direct_mask =
>> +            pci_get_word(assigned_dev->direct_config_write + address);
>> +        full_access = 0xffff;
>>          break;
>> +    case 1:
>> +        direct_mask =
>> +            pci_get_byte(assigned_dev->direct_config_write + address);
>> +        full_access = 0xff;
>> +        break;
>> +    default:
>> +        abort();
>> +    }
> 
> Shouldn't there be a if (!direct_mask) return; here?

Yep.

> 
>> +    if (direct_mask != full_access) {
>> +        val &= direct_mask;
>> +        val |= assigned_dev_pci_read(pci_dev, address, len) & ~direct_mask;
>>      }
>> +    assigned_dev_pci_write(pci_dev, address, val, len);
>>  }
>>  
>>  static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
>> @@ -1404,6 +1247,10 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
>>              return ret;
>>          }
>>  
>> +        /* direct read except for next cap pointer */
>> +        memset(dev->direct_config_read + pos, 0xff, PCI_PM_SIZEOF);
>> +        dev->direct_config_read[pos + PCI_CAP_LIST_NEXT] = 0;
>> +
>>          pmc = pci_get_word(pci_dev->config + pos + PCI_CAP_FLAGS);
>>          pmc &= (PCI_PM_CAP_VER_MASK | PCI_PM_CAP_DSI);
>>          pci_set_word(pci_dev->config + pos + PCI_CAP_FLAGS, pmc);
>> @@ -1438,6 +1285,10 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
>>              return ret;
>>          }
>>  
>> +        /* direct read except for next cap pointer */
>> +        memset(dev->direct_config_read + pos, 0xff, size);
>> +        dev->direct_config_read[pos + PCI_CAP_LIST_NEXT] = 0;
>> +
>>          type = pci_get_word(pci_dev->config + pos + PCI_EXP_FLAGS);
>>          type = (type & PCI_EXP_FLAGS_TYPE) >> 8;
>>          if (type != PCI_EXP_TYPE_ENDPOINT &&
>> @@ -1513,6 +1364,10 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
>>              return ret;
>>          }
>>  
>> +        /* direct read except for next cap pointer */
>> +        memset(dev->direct_config_read + pos, 0xff, 8);
>> +        dev->direct_config_read[pos + PCI_CAP_LIST_NEXT] = 0;
>> +
>>          /* Command register, clear upper bits, including extended modes */
>>          cmd = pci_get_word(pci_dev->config + pos + PCI_X_CMD);
>>          cmd &= (PCI_X_CMD_DPERR_E | PCI_X_CMD_ERO | PCI_X_CMD_MAX_READ |
>> @@ -1534,6 +1389,13 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
>>          if ((ret = pci_add_capability(pci_dev, PCI_CAP_ID_VPD, pos, 8)) < 0) {
>>              return ret;
>>          }
>> +
>> +        /* direct read except for next cap pointer */
>> +        memset(dev->direct_config_read + pos, 0xff, 8);
>> +        dev->direct_config_read[pos + PCI_CAP_LIST_NEXT] = 0;
>> +
>> +        /* direct write for cap content */
>> +        memset(dev->direct_config_write + pos + 2, 0xff, 6);
>>      }
>>  
>>      /* Devices can have multiple vendor capabilities, get them all */
>> @@ -1545,6 +1407,21 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
>>                                        pos, len)) < 0) {
>>              return ret;
>>          }
>> +
>> +        /* direct read except for next cap pointer */
>> +        memset(dev->direct_config_read + pos, 0xff, len);
>> +        dev->direct_config_read[pos + PCI_CAP_LIST_NEXT] = 0;
>> +
>> +        /* direct write for cap content */
>> +        memset(dev->direct_config_write + pos + 2, 0xff, len - 2);
>> +    }
>> +
>> +    /* If real and virtual capability list status bits differ, virtualize the
>> +     * access. */
>> +    if ((pci_get_word(pci_dev->config + PCI_STATUS) & PCI_STATUS_CAP_LIST) !=
>> +        (assigned_dev_pci_read_byte(pci_dev, PCI_STATUS) &
>> +         PCI_STATUS_CAP_LIST)) {
>> +        dev->direct_config_read[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
>>      }
>>  
>>      return 0;
>> @@ -1694,6 +1571,26 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
>>          return -1;
>>      }
>>  
>> +    /*
>> +     * Set up basic config space access control. Will be further refined during
>> +     * device initialization.
>> +     *
>> +     * Direct read/write access is grangted for:
>> +     *  - status & command registers, revision ID & class code, cacheline size,
>> +     *    latency timer, header type, BIST
>> +     *  - cardbus CIS pointer, subsystem vendor & ID
>> +     *  - reserved fields
>> +     *  - Min_Gnt & Max_Lat
>> +     */
>> +    memset(dev->direct_config_read + PCI_COMMAND, 0xff, 12);
> 
> This is more efficient, but maybe we should memset each field for
> grep-ability.

Will play with that. May save some comments.

> 
> Nice change overall.  Thanks,

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]