Hi, > > > > > Actually I move the judge in function assigned_dev_register_msix_mmio. > > Because assigned_dev_register_msix_mmio do not address the return value, > > if dev->msix_table is null, this will result a segfault. Right? > > I see the confusion, there is a bug there but I think it should be fixed > by setting msix_table to NULL in the MAP_FAILED case. The intent of > assigned_dev_msix_reset() is to put the msix table in the state it would > be in at reset. Without a memset() that doesn't happen. I believe the > reason we test msix_table in assigned_dev_msix_reset() is so that the > function could be called from anywhere, such as reset_assigned_device() > even though it's currently not called from there. So, if the memset() > gets removed, then the whole function should be dissolved. I'd prefer > to keep it and store or recalculate the size for memset. > Yep, agreed. Thank you so much. I want to post a formal patch, can I?
> > > > memset(dev->msix_table, 0, MSIX_PAGE_SIZE); > > > > > > This memset may no longer cover the entire table > > > > > Yeah, this memset is useless. Do it in assigned_dev_register_msix_mmio. > > Not useless, see above. > > > > > > > > > for (i = 0, entry = dev->msix_table; i < dev->msix_max; i++, > > > > entry++) > { > > > > @@ -1604,13 +1600,31 @@ static void > > > assigned_dev_msix_reset(AssignedDevice *dev) > > > > > > > > static int assigned_dev_register_msix_mmio(AssignedDevice *dev) > > > > { > > > > - dev->msix_table = mmap(NULL, MSIX_PAGE_SIZE, > > > PROT_READ|PROT_WRITE, > > > > + int nr_pages; > > > > + int size; > > > > + int entry_per_page = MSIX_PAGE_SIZE / sizeof(struct > MSIXTableEntry); > > > > + > > > > + if (dev->msix_max > entry_per_page) { > > > > + nr_pages = dev->msix_max / entry_per_page; > > > > + if (dev->msix_max % entry_per_page) { > > > > + nr_pages += 1; > > > > + } > > > > + } else { > > > > + nr_pages = 1; > > > > + } > > > > + > > > > + size = MSIX_PAGE_SIZE * nr_pages; > > > > > > Agree with the ROUND_UP comments: > > > > > > size = ROUND_UP(dev->msix_max * sizeof(struct MSIXTableEntry), > > > MSIX_PAGE_SIZE); > > > > > Nice. I don't know the macro before. Thank you so much! > > > > + dev->msix_table = mmap(NULL, size, PROT_READ|PROT_WRITE, > > > > MAP_ANONYMOUS|MAP_PRIVATE, 0, > 0); > > > > if (dev->msix_table == MAP_FAILED) { > > > > error_report("fail allocate msix_table! %s", strerror(errno)); > > > > return -EFAULT; > > > > } > > > > + if (!dev->msix_table) { > > > > + return -EFAULT; > > > > + } > > > > > > This is a bogus test for mmap(2) > > > > > > > > > > > + memset(dev->msix_table, 0, size); > > > > > > This is unnecessary when assigned_dev_msix_reset() is fixed to memset > > > the whole mmap. > > > > > > > assigned_dev_msix_reset(dev); > > > > > > > > memory_region_init_io(&dev->mmio, OBJECT(dev), > > > &assigned_dev_msix_mmio_ops, > > > > > > This memory_region_init_io also requires a size parameter that isn't > > > updated for the new size. > > > > > Nice catch. > > > As noted by other comments, the munmap() size isn't addressed. > > > > > Get it. > > > IMHO, the correct way to fix this would be to update pci-assign to use > > > the msix infrastructure, but legacy KVM assignment is being phased out > > > and replaced by VFIO. Is there something that ties you to pci-assign > > > instead of using vfio-pci? Thanks, > > > > > I will have a try. Alex, What's your opinion about my former letter about > > the > MSI-X maximum entry. > > From other thread: > > > BTW, do you think the KVM should upsize the max support MSI-X entry to > 2048. > > Because the MSI-X supports a maximum table size of 2048 entries, which is > descript in > > PCI specification 3.0 version 6.8.3.2: "MSI-X Configuration". > > > > The history patch about downsize the MSI-X entry size to 256: > > > http://thread.gmane.org/gmane.comp.emulators.kvm.devel/38852/focus=388 > 49 > > No, I think we should deprecate KVM device assignment and use VFIO. > Thanks, > OK. I will send the result of using VFIO later. Best regards, -Gonglei