Re: [PATCH v2 7/8] vpci/msi: Free MSI resources when init_msi() fails

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Apr 16, 2025 at 06:16:36AM +0000, Chen, Jiqian wrote:
> On 2025/4/15 21:29, Roger Pau Monné wrote:
> > On Wed, Apr 09, 2025 at 02:45:27PM +0800, 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
> >> cleaned up in failure path of init_msi.
> >>
> >> To do that, add a new function to free MSI resources.
> >>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> >> ---
> >> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> >> ---
> >> v1->v2 changes:
> >> * Added a new function fini_msi to free all MSI resources instead of using 
> >> an array to record registered registers.
> >> ---
> >>  xen/drivers/vpci/msi.c | 47 +++++++++++++++++++++++++++++++++---------
> >>  1 file changed, 37 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> >> index ca89ae9b9c22..48a466dba0ef 100644
> >> --- a/xen/drivers/vpci/msi.c
> >> +++ b/xen/drivers/vpci/msi.c
> >> @@ -193,6 +193,33 @@ static void cf_check mask_write(
> >>      msi->mask = val;
> >>  }
> >>  
> >> +/* 12 is size of MSI structure for 32-bit Message Address without PVM */
> >> +#define MSI_STRUCTURE_SIZE_32 12
> > 
> > I'm confused by this.  The minimum size of the capability is 4 bytes
> > for the capability pointer, plus 4 bytes for the message address,
> > plus 2 bytes for the message data.  So that's 10 bytes in total.
> The remain 2 bytes is Extended Message Data, PCIe spec has this register's 
> definition even though it is optional.

I was looking at the PCI Local Bus Specification rev 3.0 (because that
has nicer figures IMO), and such field is not listed there.  It's
listed in the PCI Express 6.0.1 specification.  I have to admit I've
completely forgot about such optional feature.

We never expose this bit to the guest in the control register, and
consequently we never explicitly handle accesses to what would be the
extended message data register, neither allow guests to enable it.

If/when we add support for such field we will likely need to adjust
fini_msi() to also zap it if present.

Thanks, Roger.