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:
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.
