Re: [PATCH 2/3] qemu-kvm: device assignment: Some clean up about MSI-X code

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

On Thu, Nov 04, 2010 at 02:18:20PM +0800, Sheng Yang wrote:
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>

If you have the energy for cleanups, how about
removing most of msi/msix code in device-assignment and using msi.c and
msix.c instead?

> ---
>  hw/device-assignment.c |   77 ++++++++++++++++++++++++++++++++---------------
>  1 files changed, 52 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 2605bd1..8a98876 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -1106,15 +1106,10 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev, unsigned int ctrl_pos)
>  #endif
>  
>  #ifdef KVM_CAP_DEVICE_MSIX
> -static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
> +static int get_msix_entries_max_nr(AssignedDevice *adev)
>  {
> -    AssignedDevice *adev = container_of(pci_dev, AssignedDevice, dev);
> -    uint16_t entries_nr = 0, entries_max_nr;
> -    int pos = 0, i, r = 0;
> -    uint32_t msg_addr, msg_upper_addr, msg_data, msg_ctrl;
> -    struct kvm_assigned_msix_nr msix_nr;
> -    struct kvm_assigned_msix_entry msix_entry;
> -    void *va = adev->msix_table_page;
> +    int pos, entries_max_nr;
> +    PCIDevice *pci_dev = &adev->dev;
>  
>      if (adev->cap.available & ASSIGNED_DEVICE_CAP_MSI)
>          pos = pci_dev->cap.start + PCI_CAPABILITY_CONFIG_MSI_LENGTH;
> @@ -1125,6 +1120,17 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
>      entries_max_nr &= PCI_MSIX_TABSIZE;
>      entries_max_nr += 1;
>  
> +    return entries_max_nr;
> +}
> +
> +static int get_msix_valid_entries_nr(AssignedDevice *adev,
> +				     uint16_t entries_max_nr)
> +{
> +    void *va = adev->msix_table_page;
> +    uint32_t msg_data, msg_ctrl;
> +    uint16_t entries_nr = 0;
> +    int i;
> +
>      /* Get the usable entry number for allocating */
>      for (i = 0; i < entries_max_nr; i++) {
>          memcpy(&msg_ctrl, va + i * 16 + 12, 4);
> @@ -1134,11 +1140,20 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
>              continue;
>          entries_nr ++;
>      }
> +    return entries_nr;
> +}
> +
> +static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev,
> +                                         uint16_t entries_nr,
> +                                         uint16_t entries_max_nr)
> +{
> +    AssignedDevice *adev = container_of(pci_dev, AssignedDevice, dev);
> +    int i, r = 0;
> +    uint32_t msg_addr, msg_upper_addr, msg_data, msg_ctrl;
> +    struct kvm_assigned_msix_nr msix_nr;
> +    struct kvm_assigned_msix_entry msix_entry;
> +    void *va = adev->msix_table_page;
>  
> -    if (entries_nr == 0) {
> -        fprintf(stderr, "MSI-X entry number is zero!\n");
> -        return -EINVAL;
> -    }
>      msix_nr.assigned_dev_id = calc_assigned_dev_id(adev->h_segnr, adev->h_busnr,
>                                            (uint8_t)adev->h_devfn);
>      msix_nr.entry_nr = entries_nr;
> @@ -1180,8 +1195,8 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
>          adev->entry[entries_nr].u.msi.address_lo = msg_addr;
>          adev->entry[entries_nr].u.msi.address_hi = msg_upper_addr;
>          adev->entry[entries_nr].u.msi.data = msg_data;
> -        DEBUG("MSI-X data 0x%x, MSI-X addr_lo 0x%x\n!", msg_data, msg_addr);
> -	kvm_add_routing_entry(&adev->entry[entries_nr]);
> +        DEBUG("MSI-X data 0x%x, MSI-X addr_lo 0x%x!\n", msg_data, msg_addr);
> +        kvm_add_routing_entry(&adev->entry[entries_nr]);
>  
>          msix_entry.gsi = adev->entry[entries_nr].gsi;
>          msix_entry.entry = i;
> @@ -1190,7 +1205,7 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
>              fprintf(stderr, "fail to set MSI-X entry! %s\n", strerror(-r));
>              break;
>          }
> -        DEBUG("MSI-X entry gsi 0x%x, entry %d\n!",
> +        DEBUG("MSI-X entry gsi 0x%x, entry %d!\n",
>                  msix_entry.gsi, msix_entry.entry);
>          entries_nr ++;
>      }
> @@ -1203,12 +1218,12 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
>      return r;
>  }
>  
> -static void assigned_dev_update_msix(PCIDevice *pci_dev, unsigned int ctrl_pos)
> +static void assigned_dev_update_msix(PCIDevice *pci_dev, int enable_msix)
>  {
>      struct kvm_assigned_irq assigned_irq_data;
>      AssignedDevice *assigned_dev = container_of(pci_dev, AssignedDevice, dev);
> -    uint16_t *ctrl_word = (uint16_t *)(pci_dev->config + ctrl_pos);
>      int r;
> +    uint16_t entries_nr, entries_max_nr;
>  
>      memset(&assigned_irq_data, 0, sizeof assigned_irq_data);
>      assigned_irq_data.assigned_dev_id  =
> @@ -1219,8 +1234,7 @@ static void assigned_dev_update_msix(PCIDevice *pci_dev, unsigned int ctrl_pos)
>       * try to catch this by only deassigning irqs if the guest is using
>       * MSIX or intends to start. */
>      if ((assigned_dev->irq_requested_type & KVM_DEV_IRQ_GUEST_MSIX) ||
> -        (*ctrl_word & PCI_MSIX_ENABLE)) {
> -
> +            enable_msix) {
>          assigned_irq_data.flags = assigned_dev->irq_requested_type;
>          free_dev_irq_entries(assigned_dev);
>          r = kvm_deassign_irq(kvm_context, &assigned_irq_data);
> @@ -1231,14 +1245,25 @@ static void assigned_dev_update_msix(PCIDevice *pci_dev, unsigned int ctrl_pos)
>          assigned_dev->irq_requested_type = 0;
>      }
>  
> -    if (*ctrl_word & PCI_MSIX_ENABLE) {
> -        assigned_irq_data.flags = KVM_DEV_IRQ_HOST_MSIX |
> -                                  KVM_DEV_IRQ_GUEST_MSIX;
> -
> -        if (assigned_dev_update_msix_mmio(pci_dev) < 0) {
> +    entries_max_nr = get_msix_entries_max_nr(assigned_dev);
> +    if (entries_max_nr == 0) {
> +        fprintf(stderr, "assigned_dev_update_msix: MSI-X entries_max_nr == 0");
> +        return;
> +    }
> +    entries_nr = get_msix_valid_entries_nr(assigned_dev, entries_max_nr);
> +    if (entries_nr == 0) {
> +        if (enable_msix)
> +            fprintf(stderr, "MSI-X entry number is zero!\n");
> +        return;
> +    }
> +    if (enable_msix) {
> +        if (assigned_dev_update_msix_mmio(pci_dev,
> +                    entries_nr, entries_max_nr) < 0) {
>              perror("assigned_dev_update_msix_mmio");
>              return;
>          }
> +        assigned_irq_data.flags = KVM_DEV_IRQ_HOST_MSIX |
> +                                  KVM_DEV_IRQ_GUEST_MSIX;
>          if (kvm_assign_irq(kvm_context, &assigned_irq_data) < 0) {
>              perror("assigned_dev_enable_msix: assign irq");
>              return;
> @@ -1254,6 +1279,7 @@ static void assigned_device_pci_cap_write_config(PCIDevice *pci_dev, uint32_t ad
>  {
>      AssignedDevice *assigned_dev = container_of(pci_dev, AssignedDevice, dev);
>      unsigned int pos = pci_dev->cap.start, ctrl_pos;
> +    uint16_t *ctrl_word;
>  
>      pci_default_cap_write_config(pci_dev, address, val, len);
>  #ifdef KVM_CAP_IRQ_ROUTING
> @@ -1270,7 +1296,8 @@ static void assigned_device_pci_cap_write_config(PCIDevice *pci_dev, uint32_t ad
>          ctrl_pos = pos + 3;
>          if (address <= ctrl_pos && address + len > ctrl_pos) {
>              ctrl_pos--; /* control is word long */
> -            assigned_dev_update_msix(pci_dev, ctrl_pos);
> +            ctrl_word = (uint16_t *)(pci_dev->config + ctrl_pos);
> +            assigned_dev_update_msix(pci_dev, (*ctrl_word & PCI_MSIX_ENABLE));
>  	}
>          pos += PCI_CAPABILITY_CONFIG_MSIX_LENGTH;
>      }
> -- 
> 1.7.0.1