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

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jun 27, 2011 at 08:19:55PM +0200, Jan Kiszka wrote:
> -/* There can be multiple VNDR capabilities per device, we need to find the
> - * one that starts closet to the given address without going over. */
> -static uint8_t find_vndr_start(PCIDevice *pci_dev, uint32_t address)
> +static uint32_t assigned_dev_pci_read_config(PCIDevice *pci_dev,
> +                                             uint32_t address, int len)
>  {
> -    uint8_t cap, pos;
> +    AssignedDevice *assigned_dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> +    uint32_t virt_val = pci_default_read_config(pci_dev, address, len);
> +    uint32_t real_val, direct_mask;
>  
> -    for (cap = pos = 0;
> -         (pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_VNDR, pos));
> -         pos += PCI_CAP_LIST_NEXT) {
> -        if (pos <= address) {
> -            cap = MAX(pos, cap);
> -        }
> +    switch (len) {
> +    case 4:
> +        direct_mask =
> +            pci_get_long(assigned_dev->direct_config_read + address);
> +        break;
> +    case 2:
> +        direct_mask =
> +            pci_get_word(assigned_dev->direct_config_read + address);
> +        break;
> +    case 1:
> +        direct_mask =
> +            pci_get_byte(assigned_dev->direct_config_read + address);
> +        break;
> +    default:
> +        abort();
>      }

One small issue here is that I think read config is not guaranteed to
get a length aligned address, while pci_get_XXX assume length
alignment. Further, because of alignment you can get beyond
the end of array.

I don't think it ever happens in practice on the PC, so we can
add an assert, but it's might be easier to look at how
pci_default_read_config handles all these issues:

    uint32_t val = 0;
    assert(len == 1 || len == 2 || len == 4);
    len = MIN(len, pci_config_size(d) - address);
    memcpy(&val, d->config + address, len);
    return le32_to_cpu(val);

You can replace config with direct access, and check full access
with:

	allones = ~0x0;
	memcmp(&allones, &val, len)
and full emulation:
	zero = 0x0;
	memcmp(&zero, &val, len)

This is not done correctly by current code btw, I think.

> -    return cap;
> -}
> -
> -static uint32_t assigned_device_pci_cap_read_config(PCIDevice *pci_dev,
> -                                                    uint32_t address, int len)
> -{
> -    uint8_t cap, cap_id = pci_dev->config_map[address];
> -    uint32_t val;
> -
> -    switch (cap_id) {
> -
> -    case PCI_CAP_ID_VPD:
> -        cap = pci_find_capability(pci_dev, cap_id);
> -        val = assigned_dev_pci_read(pci_dev, address, len);
> -        return merge_bits(val, pci_get_long(pci_dev->config + address),
> -                          address, len, cap + PCI_CAP_LIST_NEXT, 0xff);
> -
> -    case PCI_CAP_ID_VNDR:
> -        cap = find_vndr_start(pci_dev, address);
> -        val = assigned_dev_pci_read(pci_dev, address, len);
> -        return merge_bits(val, pci_get_long(pci_dev->config + address),
> -                          address, len, cap + PCI_CAP_LIST_NEXT, 0xff);
> +    if (direct_mask) {
> +        real_val = assigned_dev_pci_read(pci_dev, address, len);
> +        return (virt_val & ~direct_mask) | (real_val & direct_mask);
> +    } else {
> +        return virt_val;
>      }
> -
> -    return pci_default_read_config(pci_dev, address, len);
>  }
>  
> -static void assigned_device_pci_cap_write_config(PCIDevice *pci_dev,
> -                                                 uint32_t address,
> -                                                 uint32_t val, int len)
> +static void assigned_dev_pci_write_config(PCIDevice *pci_dev, uint32_t address,
> +                                          uint32_t val, int len)
>  {
> -    uint8_t cap_id = pci_dev->config_map[address];
> +    AssignedDevice *assigned_dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> +    uint32_t direct_mask, full_access;
>  
>      pci_default_write_config(pci_dev, address, val, len);
> -    switch (cap_id) {
> -    case PCI_CAP_ID_MSI:
> +
> +    if (assigned_dev->cap.available & ASSIGNED_DEVICE_CAP_MSI) {
>          if (range_covers_byte(address, len,
>                                pci_dev->msi_cap + PCI_MSI_FLAGS)) {
>              assigned_dev_update_msi(pci_dev);
>          }
> -        break;
> -
> -    case PCI_CAP_ID_MSIX:
> +    } else if (assigned_dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX) {
>          if (range_covers_byte(address, len,
>                                pci_dev->msix_cap + PCI_MSIX_FLAGS + 1)) {
>              assigned_dev_update_msix(pci_dev);
>          }
> -        break;
> +    }
>  
> -    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();
> +    }
> +    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);
>  }
>  

I need to think about it a bit more, but did you consider
that with VPD reads have side-effects? I'm not
saying I see a bug but it makes me a bit nervious.

Specifically, if direct mask disables access
to a specific byte, you will still happily
go ahead and read it from the device if there's a word access that
covers it.

>  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);
> +    memset(dev->direct_config_read + PCI_CARDBUS_CIS, 0xff, 8);
> +    memset(dev->direct_config_read + PCI_CAPABILITY_LIST + 1, 0xff, 7);
> +    memset(dev->direct_config_read + PCI_MIN_GNT, 0xff, 2);
> +    memset(dev->direct_config_write + PCI_COMMAND, 0xff, 12);
> +    memset(dev->direct_config_write + PCI_CARDBUS_CIS, 0xff, 8);
> +    memset(dev->direct_config_write + PCI_CAPABILITY_LIST + 1, 0xff, 7);
> +    memset(dev->direct_config_write + PCI_MIN_GNT, 0xff, 2);
> +
>      if (get_real_device(dev, dev->host.seg, dev->host.bus,
>                          dev->host.dev, dev->host.func)) {
>          error_report("pci-assign: Error: Couldn't get real device (%s)!",
> diff --git a/hw/device-assignment.h b/hw/device-assignment.h
> index 3d20207..ed5defb 100644
> --- a/hw/device-assignment.h
> +++ b/hw/device-assignment.h
> @@ -104,12 +104,13 @@ typedef struct AssignedDevice {
>  #define ASSIGNED_DEVICE_MSIX_MASKED (1 << 2)
>          uint32_t state;
>      } cap;
> +    uint8_t direct_config_read[PCI_CONFIG_SPACE_SIZE];
> +    uint8_t direct_config_write[PCI_CONFIG_SPACE_SIZE];
>      int irq_entries_nr;
>      struct kvm_irq_routing_entry *entry;
>      void *msix_table_page;
>      target_phys_addr_t msix_table_addr;
>      int mmio_index;
> -    uint32_t emulate_cmd_mask;
>      char *configfd_name;
>      int32_t bootindex;
>      QLIST_ENTRY(AssignedDevice) next;
> -- 
> 1.7.1