Re: [PATCH v4 09/10] vpci/msi: Free MSI resources when init_msi() fails
On Thu, May 22, 2025 at 02:21:16AM +, Chen, Jiqian wrote: > On 2025/5/21 19:23, Roger Pau Monné wrote: > > On Wed, May 21, 2025 at 07:00:37AM +, 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. Thanks, Roger.
Re: [PATCH v4 09/10] vpci/msi: Free MSI resources when init_msi() fails
On 2025/5/22 15:12, Roger Pau Monné wrote: > On Thu, May 22, 2025 at 02:21:16AM +, Chen, Jiqian wrote: >> On 2025/5/21 19:23, Roger Pau Monné wrote: >>> On Wed, May 21, 2025 at 07:00:37AM +, 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.
Re: [PATCH v4 09/10] vpci/msi: Free MSI resources when init_msi() fails
On 2025/5/21 19:23, Roger Pau Monné wrote: > On Wed, May 21, 2025 at 07:00:37AM +, 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? > > So that you make it strictly hardware read-only, and not use the data > in vpci->msi. > > Regards, Roger. -- Best regards, Jiqian Chen.
Re: [PATCH v4 09/10] vpci/msi: Free MSI resources when init_msi() fails
On Wed, May 21, 2025 at 07:00:37AM +, 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: vpci_add_register(vpci, vpci_hw_read16, NULL, msi_control_reg(msi_pos), 2, NULL); So that you make it strictly hardware read-only, and not use the data in vpci->msi. Regards, Roger.
Re: [PATCH v4 09/10] vpci/msi: Free MSI resources when init_msi() fails
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); " > > Thanks, Roger. -- Best regards, Jiqian Chen.
Re: [PATCH v4 09/10] vpci/msi: Free MSI resources when init_msi() fails
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. Thanks, Roger.
Re: [PATCH v4 09/10] vpci/msi: Free MSI resources when init_msi() fails
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. > and in general we allow dom0 access > to as much as possible. > > For dom0 Xen could drop writes to the MSI(-X) enable bit, thus forcing > MSI(-X) to stay disabled. I however don't see this as a mandatory > step for the work here. You're the maintainer, so you have the final say. Jan
Re: [PATCH v4 09/10] vpci/msi: Free MSI resources when init_msi() fails
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, and in general we allow dom0 access to as much as possible. For dom0 Xen could drop writes to the MSI(-X) enable bit, thus forcing MSI(-X) to stay disabled. I however don't see this as a mandatory step for the work here. Regards, Roger.
Re: [PATCH v4 09/10] vpci/msi: Free MSI resources when init_msi() fails
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)? Jan
