Re: [PATCH 2/2] pci-assign: Use pci_device_set_intx_routing_notifier

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

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

On 2012-08-03 17:35, Alex Williamson wrote:
> On Fri, 2012-08-03 at 12:52 +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Replace the hack in pci_default_write_config with upstream's generic
>> callback mechanism to get informed about changes on the PCI INTx
>> routing.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  hw/Makefile.objs       |    3 +--
>>  hw/device-assignment.c |   48 ++++++++++++++++--------------------------------
>>  hw/device-assignment.h |   33 ---------------------------------
>>  hw/pc.h                |    3 ---
>>  hw/pci.c               |   11 -----------
>>  hw/piix_pci.c          |    3 ---
>>  6 files changed, 17 insertions(+), 84 deletions(-)
>>  delete mode 100644 hw/device-assignment.h
>>
>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>> index 30f9ba6..fa8bb08 100644
>> --- a/hw/Makefile.objs
>> +++ b/hw/Makefile.objs
>> @@ -3,7 +3,7 @@ hw-obj-y += loader.o
>>  hw-obj-$(CONFIG_VIRTIO) += virtio-console.o
>>  hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>>  hw-obj-y += fw_cfg.o
>> -hw-obj-$(CONFIG_PCI) += pci_bridge.o pci_bridge_dev.o
>> +hw-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
>>  hw-obj-$(CONFIG_PCI) += msix.o msi.o
>>  hw-obj-$(CONFIG_PCI) += shpc.o
>>  hw-obj-$(CONFIG_PCI) += slotid_cap.o
>> @@ -164,7 +164,6 @@ obj-$(CONFIG_SOFTMMU) += vhost_net.o
>>  obj-$(CONFIG_VHOST_NET) += vhost.o
>>  obj-$(CONFIG_REALLY_VIRTFS) += 9pfs/
>>  obj-$(CONFIG_NO_PCI) += pci-stub.o
>> -obj-$(CONFIG_PCI) += pci.o
>>  obj-$(CONFIG_VGA) += vga.o
>>  obj-$(CONFIG_SOFTMMU) += device-hotplug.o
>>  obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o
>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>> index d14c327..7a90027 100644
>> --- a/hw/device-assignment.c
>> +++ b/hw/device-assignment.c
>> @@ -36,7 +36,6 @@
>>  #include "pc.h"
>>  #include "qemu-error.h"
>>  #include "console.h"
>> -#include "device-assignment.h"
>>  #include "loader.h"
>>  #include "monitor.h"
>>  #include "range.h"
>> @@ -143,6 +142,8 @@ typedef struct AssignedDevice {
>>      QLIST_ENTRY(AssignedDevice) next;
>>  } AssignedDevice;
>>  
>> +static void assigned_dev_update_irq_routing(PCIDevice *dev);
>> +
>>  static void assigned_dev_load_option_rom(AssignedDevice *dev);
>>  
>>  static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev);
>> @@ -869,8 +870,13 @@ static int assign_irq(AssignedDevice *dev)
>>      int r = 0;
>>  
>>      /* Interrupt PIN 0 means don't use INTx */
>> -    if (assigned_dev_pci_read_byte(&dev->dev, PCI_INTERRUPT_PIN) == 0)
>> +    if (assigned_dev_pci_read_byte(&dev->dev, PCI_INTERRUPT_PIN) == 0) {
>> +        pci_device_set_intx_routing_notifier(&dev->dev, NULL);
>>          return 0;
>> +    }
>> +
>> +    pci_device_set_intx_routing_notifier(&dev->dev,
>> +                                         assigned_dev_update_irq_routing);
>>  
>>      intx_route = pci_device_route_intx_to_irq(&dev->dev, 0);
>>      assert(intx_route.mode != PCI_INTX_INVERTED);
>> @@ -944,43 +950,19 @@ static void deassign_device(AssignedDevice *dev)
>>                  dev->dev.qdev.id, strerror(-r));
>>  }
>>  
>> -#if 0
>> -AssignedDevInfo *get_assigned_device(int pcibus, int slot)
>> -{
>> -    AssignedDevice *assigned_dev = NULL;
>> -    AssignedDevInfo *adev = NULL;
>> -
>> -    QLIST_FOREACH(adev, &adev_head, next) {
>> -        assigned_dev = adev->assigned_dev;
>> -        if (pci_bus_num(assigned_dev->dev.bus) == pcibus &&
>> -            PCI_SLOT(assigned_dev->dev.devfn) == slot)
>> -            return adev;
>> -    }
>> -
>> -    return NULL;
>> -}
>> -#endif
>> -
>>  /* The pci config space got updated. Check if irq numbers have changed
>>   * for our devices
>>   */
>> -void assigned_dev_update_irqs(void)
>> +static void assigned_dev_update_irq_routing(PCIDevice *dev)
>>  {
>> -    AssignedDevice *dev, *next;
>> +    AssignedDevice *assigned_dev = DO_UPCAST(AssignedDevice, dev, dev);
>>      Error *err = NULL;
>>      int r;
>>  
>> -    dev = QLIST_FIRST(&devs);
>> -    while (dev) {
>> -        next = QLIST_NEXT(dev, next);
>> -        if (dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX) {
>> -            r = assign_irq(dev);
>> -            if (r < 0) {
>> -                qdev_unplug(&dev->dev.qdev, &err);
>> -                assert(!err);
>> -            }
>> -        }
>> -        dev = next;
>> +    r = assign_irq(assigned_dev);
>> +    if (r < 0) {
>> +        qdev_unplug(&dev->qdev, &err);
>> +        assert(!err);
>>      }
>>  }
>>  
>> @@ -1009,6 +991,7 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
>>              perror("assigned_dev_update_msi: deassign irq");
>>  
>>          assigned_dev->irq_requested_type = 0;
>> +        pci_device_set_intx_routing_notifier(pci_dev, NULL);
>>      }
>>  
>>      if (ctrl_byte & PCI_MSI_FLAGS_ENABLE) {
>> @@ -1151,6 +1134,7 @@ static void assigned_dev_update_msix(PCIDevice *pci_dev)
>>              perror("assigned_dev_update_msix: deassign irq");
>>  
>>          assigned_dev->irq_requested_type = 0;
>> +        pci_device_set_intx_routing_notifier(pci_dev, NULL);
>>      }
>>  
>>      if (ctrl_word & PCI_MSIX_FLAGS_ENABLE) {
>> diff --git a/hw/device-assignment.h b/hw/device-assignment.h
>> deleted file mode 100644
>> index 3fcb804..0000000
>> --- a/hw/device-assignment.h
>> +++ /dev/null
>> @@ -1,33 +0,0 @@
>> -/*
>> - * Copyright (c) 2007, Neocleus Corporation.
>> - * Copyright (c) 2007, Intel Corporation.
>> - *
>> - * This program is free software; you can redistribute it and/or modify it
>> - * under the terms and conditions of the GNU General Public License,
>> - * version 2, as published by the Free Software Foundation.
>> - *
>> - * This program is distributed in the hope it will be useful, but WITHOUT
>> - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> - * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> - * more details.
>> - *
>> - * You should have received a copy of the GNU General Public License along with
>> - * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
>> - * Place - Suite 330, Boston, MA 02111-1307 USA.
>> - *
>> - *  Data structures for storing PCI state
>> - *
>> - *  Adapted to kvm by Qumranet
>> - *
>> - *  Copyright (c) 2007, Neocleus, Alex Novik (alex@neocleus.com)
>> - *  Copyright (c) 2007, Neocleus, Guy Zana (guy@neocleus.com)
>> - *  Copyright (C) 2008, Qumranet, Amit Shah (amit.shah@qumranet.com)
>> - *  Copyright (C) 2008, Red Hat, Amit Shah (amit.shah@redhat.com)
>> - */
>> -
>> -#ifndef __DEVICE_ASSIGNMENT_H__
>> -#define __DEVICE_ASSIGNMENT_H__
>> -
>> -void assigned_dev_update_irqs(void);
>> -
>> -#endif              /* __DEVICE_ASSIGNMENT_H__ */
>> diff --git a/hw/pc.h b/hw/pc.h
>> index 5b36eb5..31ccb6f 100644
>> --- a/hw/pc.h
>> +++ b/hw/pc.h
>> @@ -149,9 +149,6 @@ void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr);
>>  extern int no_hpet;
>>  
>>  /* piix_pci.c */
>> -/* config space register for IRQ routing */
>> -#define PIIX_CONFIG_IRQ_ROUTE 0x60
>> -
>>  struct PCII440FXState;
>>  typedef struct PCII440FXState PCII440FXState;
>>  
>> diff --git a/hw/pci.c b/hw/pci.c
>> index 0b22913..4d95984 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -29,9 +29,6 @@
>>  #include "net.h"
>>  #include "sysemu.h"
>>  #include "loader.h"
>> -#include "hw/pc.h"
>> -#include "kvm.h"
>> -#include "device-assignment.h"
>>  #include "range.h"
>>  #include "qmp-commands.h"
>>  #include "msi.h"
>> @@ -1048,14 +1045,6 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>>          d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
>>          d->config[addr + i] &= ~(val & w1cmask); /* W1C: Write 1 to Clear */
>>      }
>> -
>> -#ifdef CONFIG_KVM_DEVICE_ASSIGNMENT
>> -    if (kvm_enabled() && kvm_irqchip_in_kernel() &&
>> -        addr >= PIIX_CONFIG_IRQ_ROUTE &&
>> -	addr < PIIX_CONFIG_IRQ_ROUTE + 4)
>> -        assigned_dev_update_irqs();
>> -#endif /* CONFIG_KVM_DEVICE_ASSIGNMENT */
>> -
>>      if (ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) ||
>>          ranges_overlap(addr, l, PCI_ROM_ADDRESS, 4) ||
>>          ranges_overlap(addr, l, PCI_ROM_ADDRESS1, 4) ||
>> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
>> index 355814f..c497a01 100644
>> --- a/hw/piix_pci.c
>> +++ b/hw/piix_pci.c
>> @@ -250,8 +250,6 @@ static int i440fx_initfn(PCIDevice *dev)
>>      return 0;
>>  }
>>  
>> -static PIIX3State *piix3_dev;
>> -
>>  static PCIBus *i440fx_common_init(const char *device_name,
>>                                    PCII440FXState **pi440fx_state,
>>                                    int *piix3_devfn,
>> @@ -331,7 +329,6 @@ static PCIBus *i440fx_common_init(const char *device_name,
>>          ram_size = 255;
>>      (*pi440fx_state)->dev.config[0x57]=ram_size;
>>  
>> -    piix3_dev = piix3;
>>      i440fx_update_memory_mappings(f);
>>  
>>      return b;
> 
> For vfio I chose to leave the intx routing notifier in place for msi &
> msix as we can easily ignore it when not using intx.  I don't see much
> advantage to one over the other though.

Probably. When you don't disable, then you have to check in the callback
if the IRQ is actually registered, but that's a nit as well.

> 
> Acked-by: Alex Williamson <alex.williamson@redhat.com>
> 

Thanks,
Jan


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