On 2025/5/22 15:12, Roger Pau Monné wrote: > On Thu, May 22, 2025 at 02:21:16AM +0000, Chen, Jiqian wrote: >> On 2025/5/21 19:23, Roger Pau Monné wrote: >>> On Wed, May 21, 2025 at 07:00:37AM +0000, Chen, Jiqian wrote: >>>> On 2025/5/20 17:43, Roger Pau Monné wrote: >>>>> On Tue, May 20, 2025 at 11:14:27AM +0200, Jan Beulich wrote: >>>>>> On 20.05.2025 11:09, Roger Pau Monné wrote: >>>>>>> On Tue, May 20, 2025 at 08:40:28AM +0200, Jan Beulich wrote: >>>>>>>> On 09.05.2025 11:05, Jiqian Chen wrote: >>>>>>>>> When init_msi() fails, the previous new changes will hide MSI >>>>>>>>> capability, it can't rely on vpci_deassign_device() to remove >>>>>>>>> all MSI related resources anymore, those resources must be >>>>>>>>> removed in cleanup function of MSI. >>>>>>>> >>>>>>>> That's because vpci_deassign_device() simply isn't called anymore? >>>>>>>> Could do with wording along these lines then. But (also applicable >>>>>>>> to the previous patch) - doesn't this need to come earlier? And is >>>>>>>> it sufficient to simply remove the register intercepts? Don't you >>>>>>>> need to put in place ones dropping all writes and making all reads >>>>>>>> return either 0 or ~0 (covering in particular Dom0, while for DomU-s >>>>>>>> this may already be the case by default behavior)? >>>>>>> >>>>>>> For domUs this is already the default behavior. >>>>>>> >>>>>>> For dom0 I think it should be enough to hide the capability from the >>>>>>> linked list, but not hide all the capability related >>>>>>> registers. IMO a well behaved dom0 won't try to access capabilities >>>>>>> disconnected from the linked list, >>>>>> >>>>>> Just that I've seen drivers knowing where their device has certain >>>>>> capabilities, thus not bothering to look up the respective >>>>>> capability. >>>>> >>>>> OK, so let's make the control register read-only in case of failure. >>>>> >>>>> If MSI(-X) is already enabled we should also make the entries >>>>> read-only, and while that's not very complicated for MSI, it does get >>>>> more convoluted for MSI-X. I'm fine with just making the control >>>>> register read-only for the time being. >>>> If I understand correctly, I need to avoid control register being removed >>>> and set the write hook of control register to be vpci_ignored_write and >>>> avoid freeing vpci->msi? >>>> >>>> " >>>> if ( !msi_pos || !vpci->msi ) >>>> return; >>>> >>>> + spin_lock(&vpci->lock); >>>> + control = vpci_get_register(vpci, msi_control_reg(msi_pos), 2); >>>> + if ( control ) >>>> + control->write = vpci_ignored_write; >>>> + spin_unlock(&vpci->lock); >>>> + >>>> if ( vpci->msi->masking ) >>>> end = msi_pending_bits_reg(msi_pos, vpci->msi->address64); >>>> else >>>> end = msi_mask_bits_reg(msi_pos, vpci->msi->address64) - 2; >>>> >>>> - size = end - msi_control_reg(msi_pos); >>>> + start = msi_control_reg(msi_pos) + 2; >>>> + size = end - start; >>>> >>>> - vpci_remove_registers(vpci, msi_control_reg(msi_pos), size); >>>> - XFREE(vpci->msi); >>>> + vpci_remove_registers(vpci, start, size); >>> >>> I think you want to first purge all the MSI range, and then add the >>> control register, also you want to keep the XFREE(), and set the >>> register as: >> Understood. >> >>> >>> vpci_add_register(vpci, vpci_hw_read16, NULL, msi_control_reg(msi_pos), >>> 2, NULL); >> And one more question, how do I process return value of vpci_add_register >> since definition of cleanup hook is "void"? >> Print a error message if fail? > > Well, we should consider the cleanup function returning an error code. > vpci_remove_registers() can also fail, and the error is currently > ignored. Both cases should result in failing to assign the device to > the domain IMO. OK, will change in next version. Thank you!
> > Thanks, Roger. -- Best regards, Jiqian Chen.
