Re: [PATCH v2 4/6] pci-assign: Properly handle more overlapping accesses

[Alex Williamson <alex.williamson@redhat.com>]

On Fri, 2011-04-29 at 11:05 +0200, Jan Kiszka wrote:
> Ensure that accesses exceeding PCI_CAPABILITY_LIST and
> PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not
> virtualize. Again, we do not optimize these checks and accesses a lot,
> they are considered to be slow paths.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/device-assignment.c |   34 +++++++++++++++++++++++++++++-----
>  1 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index cea072e..37c77e3 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -442,7 +442,29 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
>          ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
>          /* used for update-mappings (BAR emulation) */
>          pci_default_write_config(d, address, val, len);
> -        return;
> +
> +        /* Ensure that writes to overlapping areas we don't virtualize still
> +         * hit the device. */
> +        switch (address) {
> +        case PCI_CAPABILITY_LIST:
> +            if (len > 1) {
> +                len -= 1;
> +                address += 1;
> +                val >>= 8;
> +                break; /* continue writing to the device */
> +            }
> +            return;
> +        case PCI_INTERRUPT_LINE:
> +            if (len > 2) {
> +                len -= 2;
> +                address += 2;
> +                val >>= 16;
> +                break; /* continue writing to the device */
> +            }
> +            return;
> +        default:
> +            return;
> +        }
>      }

It seems like we could be more symmetric with the below read.  Maybe
something like:

if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
    ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
    pci_default_write_config(d, address, val, len);
    return;
} else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) ||
           ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
    uint32_t real_val =  assigned_dev_pci_read(d, address, len);
    pci_default_write_config(d, address, val, len);
    val = merge_bits(val, real_val, address, len, PCI_CAPABILITY_LIST, 0xff);
    val = merge_bits(val, real_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
}

We might write out to real hardware when we could avoid it, but I don't
think it matters.  Thanks,

Alex

>      DEBUG("NON BAR (%x.%x): address=%04x val=0x%08x len=%d\n",
> @@ -467,7 +489,7 @@ again:
>  static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
>                                               int len)
>  {
> -    uint32_t val = 0;
> +    uint32_t val = 0, virt_val;
>      int fd;
>      ssize_t ret;
>      AssignedDevice *pci_dev = DO_UPCAST(AssignedDevice, dev, d);
> @@ -484,12 +506,10 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
>       *  - vendor & device ID
>       *  - base address registers
>       *  - ROM base address & capability pointer
> -     *  - interrupt line & pin
>       */
>      if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) ||
>          ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
> -        ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
> -        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
> +        ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
>          val = pci_default_read_config(d, address, len);
>          DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
>                (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len);
> @@ -523,6 +543,10 @@ do_log:
>                           address, len, PCI_COMMAND, 0xffff);
>      }
>  
> +    virt_val = pci_default_read_config(d, address, len);
> +    val = merge_bits(val, virt_val, address, len, PCI_CAPABILITY_LIST, 0xff);
> +    val = merge_bits(val, virt_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
> +
>      if (!pci_dev->cap.available) {
>          /* kill the special capabilities */
>          if (address == PCI_COMMAND && len == 4) {