Hi Blue,
thanks for the review. I addressed most of them, the others a commented
below.
On 2012-08-27 20:56, Blue Swirl wrote:
>> +typedef struct AssignedDevice {
>> + PCIDevice dev;
>> + PCIHostDeviceAddress host;
>> + uint32_t dev_id;
>> + uint32_t features;
>> + int intpin;
>> + AssignedDevRegion v_addrs[PCI_NUM_REGIONS - 1];
>> + PCIDevRegions real_device;
>> + PCIINTxRoute intx_route;
>> + AssignedIRQType assigned_irq_type;
>> + struct {
>> +#define ASSIGNED_DEVICE_CAP_MSI (1 << 0)
>> +#define ASSIGNED_DEVICE_CAP_MSIX (1 << 1)
>> + uint32_t available;
>> +#define ASSIGNED_DEVICE_MSI_ENABLED (1 << 0)
>> +#define ASSIGNED_DEVICE_MSIX_ENABLED (1 << 1)
>> +#define ASSIGNED_DEVICE_MSIX_MASKED (1 << 2)
>> + uint32_t state;
>> + } cap;
>> + uint8_t emulate_config_read[PCI_CONFIG_SPACE_SIZE];
>> + uint8_t emulate_config_write[PCI_CONFIG_SPACE_SIZE];
>> + int msi_virq_nr;
>> + int *msi_virq;
>> + MSIXTableEntry *msix_table;
>> + target_phys_addr_t msix_table_addr;
>> + uint16_t msix_max;
>> + MemoryRegion mmio;
>> + char *configfd_name;
>
> const? Not if this would mean more casts.
DEFINE_PROP_STRING, where this is used, doesn't allow this.
...
>> + } else {
>> + uint32_t port = addr + dev_region->u.r_baseport;
>> +
>> + if (data) {
>> + DEBUG("out data=%lx, size=%d, e_phys=%lx, host=%x\n",
>> + *data, size, addr, port);
>> + switch (size) {
>> + case 1:
>> + outb(*data, port);
>> + break;
>> + case 2:
>> + outw(*data, port);
>> + break;
>> + case 4:
>> + outl(*data, port);
>> + break;
>
> Maybe add case 8: and default: with abort(), also below.
PIO is never 8 bytes long, the generic layer protects us.
...
>> +
>> + fclose(f);
>> +
>> + /* read and fill vendor ID */
>> + v = get_real_vendor_id(dir, &id);
>> + if (v) {
>> + return 1;
>> + }
>> + pci_dev->dev.config[0] = id & 0xff;
>> + pci_dev->dev.config[1] = (id & 0xff00) >> 8;
>> +
>> + /* read and fill device ID */
>> + v = get_real_device_id(dir, &id);
>> + if (v) {
>> + return 1;
>> + }
>> + pci_dev->dev.config[2] = id & 0xff;
>> + pci_dev->dev.config[3] = (id & 0xff00) >> 8;
>> +
>> + pci_word_test_and_clear_mask(pci_dev->emulate_config_write + PCI_COMMAND,
>> + PCI_COMMAND_MASTER | PCI_COMMAND_INTX_DISABLE);
>> +
>> + dev->region_number = r;
>> + return 0;
>> +}
>
> Pretty long function, how about refactoring?
Possibly, but I'd prefer to do such changes in-tree, after the more
important refactoring on MSI[-X] is done.
...
>> + if (ctrl_byte & PCI_MSI_FLAGS_ENABLE) {
>> + uint8_t *pos = pci_dev->config + pci_dev->msi_cap;
>> + MSIMessage msg;
>> + int virq;
>> +
>> + msg.address = pci_get_long(pos + PCI_MSI_ADDRESS_LO);
>> + msg.data = pci_get_word(pos + PCI_MSI_DATA_32);
>> + virq = kvm_irqchip_add_msi_route(kvm_state, msg);
>> + if (virq < 0) {
>> + perror("assigned_dev_update_msi: kvm_irqchip_add_msi_route");
>> + return;
>> + }
>> +
>> + assigned_dev->msi_virq = g_malloc(sizeof(*assigned_dev->msi_virq));
>
> Is this ever freed?
Yep, in free_msi_virqs. If you think you spotted a path where this is
not the case, let me know.
...
>> +
>> +static Property da_properties[] = {
>
> const?
Nope, properties must remain writable.
>
>> + DEFINE_PROP_PCI_HOST_DEVADDR("host", AssignedDevice, host),
>> + DEFINE_PROP_BIT("prefer_msi", AssignedDevice, features,
>> + ASSIGNED_DEVICE_PREFER_MSI_BIT, false),
>> + DEFINE_PROP_BIT("share_intx", AssignedDevice, features,
>> + ASSIGNED_DEVICE_SHARE_INTX_BIT, true),
>> + DEFINE_PROP_INT32("bootindex", AssignedDevice, bootindex, -1),
>> + DEFINE_PROP_STRING("configfd", AssignedDevice, configfd_name),
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
Jan
Attachment:
signature.asc
Description: OpenPGP digital signature
