Re: [Qemu-devel] [PATCH 4/4] kvm: i386: Add classic PCI device assignment

[Jan Kiszka]

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

signature.asc
Description: OpenPGP digital signature