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@xxxxxxxxxxxxxxx>
> > > > ---
> > > >
> > > > 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
