On 04.07.2025 09:08, Jiqian Chen wrote:
> ---
> v6->v7 changes:
> * Change the pointer parameter of cleanup_msi() to be const.
> * When vpci_remove_registers() in cleanup_msi() fails, not to return
> directly, instead try to free msi and re-add ctrl handler.
> * Pass pdev->vpci into vpci_add_register() instead of pdev->vpci->msi in
> init_msi() since we need that every handler realize that msi is NULL
> when msi is free but handlers are still in there.
Imo this latter change would better have been a separate patch. I'm not
going to insist though (also I really can't, for not being a maintainer
of this file).
> @@ -193,6 +234,42 @@ static void cf_check mask_write(
> msi->mask = val;
> }
>
> +static int cf_check cleanup_msi(const struct pci_dev *pdev)
> +{
> + int rc;
> + unsigned int end;
> + struct vpci *vpci = pdev->vpci;
> + const unsigned int msi_pos = pdev->msi_pos;
> + const unsigned int ctrl = msi_control_reg(msi_pos);
> +
> + if ( !msi_pos || !vpci->msi )
> + return 0;
> +
> + 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;
What's this "- 2" for? If there's no masking support, you want to cut off
_at_ the mask register, not 2 bytes ahead of it? Just like you cut off at
the pending bits register when there is masking support.
> + rc = vpci_remove_registers(vpci, ctrl, end - ctrl);
> + if ( rc )
> + printk(XENLOG_WARNING "%pd %pp: fail to remove MSI handlers rc=%d\n",
> + pdev->domain, &pdev->sbdf, rc);
> +
> + XFREE(vpci->msi);
> +
> + /*
> + * The driver may not traverse the capability list and think device
> + * supports MSI by default. So here let the control register of MSI
> + * be Read-Only is to ensure MSI disabled.
> + */
> + rc = vpci_add_register(vpci, vpci_hw_read16, NULL, ctrl, 2, NULL);
> + if ( rc )
> + printk(XENLOG_ERR "%pd %pp: fail to add MSI ctrl handler rc=%d\n",
> + pdev->domain, &pdev->sbdf, rc);
Imo the uses of XENLOG_ERR and XENLOG_WARNING want to change places. The latter
is extremely likely to be a follow-on failure from the first one failing. Plus
the latter failing is covered by what you add to control_read(). Which leaves
as the only case where this is really an error (and XENLOG_ERR might then be
warranted in both places) if the former succeeds and only the latter fails.
Hmm, then again vpci_init_capabilities() would too issue an error message in
that case. Perhaps keep as is then.
Jan
