Re: [PATCH 3/3] qemu-kvm: device assignment: emulate MSI-X mask bits

[Sheng Yang <sheng@linux.intel.com>]

On Friday 05 November 2010 17:05:32 Michael S. Tsirkin wrote:
> On Fri, Nov 05, 2010 at 11:20:37AM +0800, Sheng Yang wrote:
> > On Thursday 04 November 2010 17:44:27 Michael S. Tsirkin wrote:
> > 
> > Thanks very much for reviewing this! Seems nobody cares about userspace
> > one before...
> > 
> > > On Thu, Nov 04, 2010 at 02:18:21PM +0800, Sheng Yang wrote:
> > > > This patch emulated MSI-X per vector mask bit on assigned device.
> > > > 
> > > > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > > > ---
> > > > 
> > > >  hw/device-assignment.c |  161
> > > >  ++++++++++++++++++++++++++++++++++++++++++++++-- 1 files
> > 
> > changed, 155
> > 
> > > >  insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > > > index 8a98876..639aa0b 100644
> > > > --- a/hw/device-assignment.c
> > > > +++ b/hw/device-assignment.c
> > > > @@ -62,6 +62,11 @@ static void
> > > > assigned_dev_load_option_rom(AssignedDevice *dev);
> > > > 
> > > >  static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev);
> > > > 
> > > > +static uint32_t calc_assigned_dev_id(uint16_t seg, uint8_t bus,
> > > > uint8_t devfn) +{
> > > > +    return (uint32_t)seg << 16 | (uint32_t)bus << 8 |
> > > > (uint32_t)devfn; +}
> > > > +
> > > > 
> > > >  static uint32_t assigned_dev_ioport_rw(AssignedDevRegion
> > > >  *dev_region,
> > > >  
> > > >                                         uint32_t addr, int len,
> > > >                                         uint32_t *val)
> > > >  
> > > >  {
> > > > 
> > > > @@ -264,6 +269,9 @@ static void assigned_dev_iomem_map(PCIDevice
> > > > *pci_dev, int region_num,
> > > > 
> > > >      AssignedDevRegion *region = &r_dev->v_addrs[region_num];
> > > >      PCIRegion *real_region =
> > > >      &r_dev->real_device.regions[region_num]; int ret = 0;
> > > > 
> > > > +#ifdef KVM_CAP_DEVICE_MSIX_MASK
> > > > +    struct kvm_assigned_msix_mmio msix_mmio;
> > > > +#endif
> > > > 
> > > >      DEBUG("e_phys=%08" FMT_PCIBUS " r_virt=%p type=%d len=%08"
> > > >      FMT_PCIBUS " region_num=%d \n",
> > > >      
> > > >            e_phys, region->u.r_virtbase, type, e_size, region_num);
> > > > 
> > > > @@ -282,6 +290,16 @@ static void assigned_dev_iomem_map(PCIDevice
> > > > *pci_dev, int region_num,
> > > > 
> > > >              cpu_register_physical_memory(e_phys + offset,
> > > >              
> > > >                      TARGET_PAGE_SIZE, r_dev->mmio_index);
> > > > 
> > > > +#ifdef KVM_CAP_DEVICE_MSIX_MASK
> > > > +	    memset(&msix_mmio, 0, sizeof(struct kvm_assigned_msix_mmio));
> > > > +	    msix_mmio.assigned_dev_id =
> > > > calc_assigned_dev_id(r_dev->h_segnr, +			    r_dev->h_busnr,
> > > > r_dev->h_devfn);
> > > > +	    msix_mmio.base_addr = e_phys + offset;
> > > > +            /* We required kernel MSI-X support */
> > > > +	    ret = kvm_assign_reg_msix_mmio(kvm_context, &msix_mmio);
> > > > +	    if (ret)
> > > > +                fprintf(stderr, "fail to register in-kernel
> > > > msix_mmio!\n"); +#endif
> > > > 
> > > >          }
> > > >      
> > > >      }
> > > > 
> > > > @@ -824,11 +842,6 @@ static void free_assigned_device(AssignedDevice
> > > > *dev)
> > > > 
> > > >      }
> > > >  
> > > >  }
> > > > 
> > > > -static uint32_t calc_assigned_dev_id(uint16_t seg, uint8_t bus,
> > > > uint8_t devfn) -{
> > > > -    return (uint32_t)seg << 16 | (uint32_t)bus << 8 |
> > > > (uint32_t)devfn; -}
> > > > -
> > > > 
> > > >  static void assign_failed_examine(AssignedDevice *dev)
> > > >  {
> > > >  
> > > >      char name[PATH_MAX], dir[PATH_MAX], driver[PATH_MAX] = {}, *ns;
> > > > 
> > > > @@ -1123,6 +1136,27 @@ static int
> > > > get_msix_entries_max_nr(AssignedDevice *adev)
> > > > 
> > > >      return entries_max_nr;
> > > >  
> > > >  }
> > > > 
> > > > +#ifdef KVM_CAP_DEVICE_MSIX_MASK
> > > > +static int assigned_dev_msix_entry_masked(AssignedDevice *adev, int
> > > > entry) +{
> > > > +    struct kvm_assigned_msix_entry msix_entry;
> > > > +    int r;
> > > > +
> > > > +    memset(&msix_entry, 0, sizeof msix_entry);
> > > > +    msix_entry.assigned_dev_id = calc_assigned_dev_id(adev->h_segnr,
> > > > +            adev->h_busnr, (uint8_t)adev->h_devfn);
> > > > +    msix_entry.entry = entry;
> > > > +    msix_entry.flags = KVM_MSIX_FLAG_QUERY_MASK;
> > > > +    r = kvm_assign_get_msix_entry(kvm_context, &msix_entry);
> > > > +    if (r) {
> > > > +        fprintf(stderr, "assigned_dev_msix_entry_masked: "
> > > > +			"Fail to get mask bit of entry %d\n", entry);
> > > > +        return 1;
> > > 
> > > This error handling seems pretty useless. assert?
> > 
> > Maybe we can continue with it. Assert seems a little strict.
> 
> Well need to consider whether to return 1 or 0 then.

Tell it it's masked then routing change may be continue. 

> 
> > > > +    }
> > > > +    return (msix_entry.flags & KVM_MSIX_FLAG_MASK);
> > > > +}
> > > > +#endif
> > > > +
> > > > 
> > > >  static int get_msix_valid_entries_nr(AssignedDevice *adev,
> > > >  
> > > >  				     uint16_t entries_max_nr)
> > > >  
> > > >  {
> > > > 
> > > > @@ -1136,7 +1170,11 @@ static int
> > > > get_msix_valid_entries_nr(AssignedDevice *adev,
> > > > 
> > > >          memcpy(&msg_ctrl, va + i * 16 + 12, 4);
> > > >          memcpy(&msg_data, va + i * 16 + 8, 4);
> > > >          /* Ignore unused entry even it's unmasked */
> > > > 
> > > > +#ifdef KVM_CAP_DEVICE_MSIX_MASK
> > > > +        if (assigned_dev_msix_entry_masked(adev, i))
> > > > +#else
> > > > 
> > > >          if (msg_data == 0)
> > > > 
> > > > +#endif
> > > 
> > > So, we are replacing msg_data == 0 check with masked check?
> > > If yes why not do this for non-KVM_CAP_DEVICE_MSIX_MASK too?
> > 
> > Because the old one doesn't have idea about mask bit...
> 
> So track mask bits in userspace.

That's not the target of this patch. And it still won't work.
> 
> > > >              continue;
> > > >          
> > > >          entries_nr ++;
> > > >      
> > > >      }
> > > > 
> > > > @@ -1165,6 +1203,8 @@ static int
> > > > assigned_dev_update_msix_mmio(PCIDevice *pci_dev,
> > > > 
> > > >      }
> > > >      
> > > >      free_dev_irq_entries(adev);
> > > > 
> > > > +    memset(pci_dev->msix_entry_used, 0, KVM_MAX_MSIX_PER_DEV *
> > > > +
> > > > sizeof(*pci_dev->msix_entry_used));
> > > > 
> > > >      adev->irq_entries_nr = entries_nr;
> > > >      adev->entry = calloc(entries_nr, sizeof(struct
> > > >      kvm_irq_routing_entry)); if (!adev->entry) {
> > > > 
> > > > @@ -1179,7 +1219,11 @@ static int
> > > > assigned_dev_update_msix_mmio(PCIDevice *pci_dev,
> > > > 
> > > >              break;
> > > >          
> > > >          memcpy(&msg_ctrl, va + i * 16 + 12, 4);
> > > >          memcpy(&msg_data, va + i * 16 + 8, 4);
> > > > 
> > > > +#ifdef KVM_CAP_DEVICE_MSIX_MASK
> > > > +        if (assigned_dev_msix_entry_masked(adev, i))
> > > > +#else
> > > > 
> > > >          if (msg_data == 0)
> > > > 
> > > > +#endif
> > > 
> > > You can't use ifdef to check that kernel supports an ioctl.
> > > You must check this at runtime.
> > 
> > Maybe we can convert them in bulk later.
> 
> Well you are adding a bug, all writes on old kernels
> will spew out a ton of stderr.
> We can't just assume new kernels.
> 
> > > >              continue;
> > > >          
> > > >          memcpy(&msg_addr, va + i * 16, 4);
> > > > 
> > > > @@ -1200,6 +1244,7 @@ static int
> > > > assigned_dev_update_msix_mmio(PCIDevice *pci_dev,
> > > > 
> > > >          msix_entry.gsi = adev->entry[entries_nr].gsi;
> > > >          msix_entry.entry = i;
> > > > 
> > > > +        pci_dev->msix_entry_used[i] = 1;
> > > > 
> > > >          r = kvm_assign_set_msix_entry(kvm_context, &msix_entry);
> > > >          if (r) {
> > > >          
> > > >              fprintf(stderr, "fail to set MSI-X entry! %s\n",
> > > >              strerror(-r));
> > > > 
> > > > @@ -1243,6 +1288,8 @@ static void assigned_dev_update_msix(PCIDevice
> > > > *pci_dev, int enable_msix)
> > > > 
> > > >              perror("assigned_dev_update_msix: deassign irq");
> > > >          
> > > >          assigned_dev->irq_requested_type = 0;
> > > > 
> > > > +        memset(pci_dev->msix_entry_used, 0, KVM_MAX_MSIX_PER_DEV *
> > > > +
> > > > sizeof(*pci_dev->msix_entry_used));
> > > > 
> > > >      }
> > > >      
> > > >      entries_max_nr = get_msix_entries_max_nr(assigned_dev);
> > > > 
> > > > @@ -1250,10 +1297,16 @@ static void
> > > > assigned_dev_update_msix(PCIDevice *pci_dev, int enable_msix)
> > > > 
> > > >          fprintf(stderr, "assigned_dev_update_msix: MSI-X
> > > >          entries_max_nr == 0"); return;
> > > >      
> > > >      }
> > > > 
> > > > +    /*
> > > > +     * Guest may try to enable MSI-X before setting MSI-X entry
> > > > done, so +     * let's wait until guest unmask the entries.
> > > > +     */
> > > 
> > > Well it can also set up any number of entries, enable msix then
> > > set up more entries. Now what?
> > 
> > It's the same. If it want to set up more entries, it have to unmask them.
> > Then we would get them.
> 
> Why can't we handle the first enable in the same way?

Don't understand the question, but I guess the answer is pci_enable_msix().
> 
> > > >      entries_nr = get_msix_valid_entries_nr(assigned_dev,
> > > >      entries_max_nr); if (entries_nr == 0) {
> > > > 
> > > > +#ifndef KVM_CAP_DEVICE_MSIX_MASK
> > > > 
> > > >          if (enable_msix)
> > > >          
> > > >              fprintf(stderr, "MSI-X entry number is zero!\n");
> > > 
> > > And what happens then?
> > 
> > MSI-X can't work for old ones without MSIX mask support.
> 
> Old ones?

Old userspace without KVM_CAP_DEVICE_MSIX_MASK
> 
> > Reload the guest module
> > may help.
> 
> Guest might not have any concept of modules.

Just an workaround. Not a suggestion method.

--
regards
Yang, Sheng

> 
> > --
> > regards
> > Yang, Sheng