Re: [PATCH v1 3/3] vpci/msi: Remove registers when init_msi() fails

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

On Mon, Mar 31, 2025 at 08:13:50AM +0000, Chen, Jiqian wrote:
> On 2025/3/27 20:44, Roger Pau Monné wrote:
> > On Thu, Mar 27, 2025 at 03:32:14PM +0800, Jiqian Chen wrote:
> >> When init_msi() fails, the new codes will try to hide MSI
> >> capability, so it can't rely on vpci_deassign_device() to
> >> remove all MSI related registers anymore, those registers
> >> must be cleaned up in failure path of init_msi.
> >>
> >> To do that, use a vpci_register array to record all MSI
> >> registers and call vpci_remove_register() to remove registers.
> > 
> > As I'm just seeing 3 patches on the series, isn't there one missing
> > for MSI-X at least?
> No, because init_msix only call vpci_add_register once, there is no need to 
> remove registers when it fails.

Seems a bit fragile, what about if there's further code added to
init_msix() that could return an error after the vpci_add_register()
call?  It would be safer to have a cleanup function that removes the
config space handlers, plus the MMIO one (see the call to
register_mmio_handler()), and the addition to the
d->arch.hvm.msix_tables list.

> > 
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> >> ---
> >>  xen/drivers/vpci/msi.c | 57 +++++++++++++++++++++++++++++-------------
> >>  1 file changed, 39 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> >> index 9d7a9fd8dba1..30ef97efb7b0 100644
> >> --- a/xen/drivers/vpci/msi.c
> >> +++ b/xen/drivers/vpci/msi.c
> >> @@ -195,6 +195,9 @@ static void cf_check mask_write(
> >>  
> >>  static int cf_check init_msi(struct pci_dev *pdev)
> >>  {
> >> +    unsigned int offset;
> >> +    unsigned int reg_index = 0;
> >> +    struct vpci_register registers[VPCI_CAP_MAX_REGISTER];
> >>      unsigned int pos = pdev->msi_pos;
> >>      uint16_t control;
> >>      int ret;
> >> @@ -206,15 +209,13 @@ static int cf_check init_msi(struct pci_dev *pdev)
> >>      if ( !pdev->vpci->msi )
> >>          return -ENOMEM;
> >>  
> >> +    offset = msi_control_reg(pos);
> >>      ret = vpci_add_register(pdev->vpci, control_read, control_write,
> >> -                            msi_control_reg(pos), 2, pdev->vpci->msi);
> >> +                            offset, 2, pdev->vpci->msi);
> >>      if ( ret )
> >> -        /*
> >> -         * NB: there's no need to free the msi struct or remove the 
> >> register
> >> -         * handlers form the config space, the caller will take care of 
> >> the
> >> -         * cleanup.
> >> -         */
> >> -        return ret;
> >> +        goto fail;
> >> +    registers[reg_index].offset = offset;
> >> +    registers[reg_index++].size = 2;
> >>  
> >>      /* Get the maximum number of vectors the device supports. */
> >>      control = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
> >> @@ -234,33 +235,42 @@ static int cf_check init_msi(struct pci_dev *pdev)
> >>      pdev->vpci->msi->address64 = is_64bit_address(control);
> >>      pdev->vpci->msi->masking = is_mask_bit_support(control);
> >>  
> >> +    offset = msi_lower_address_reg(pos);
> >>      ret = vpci_add_register(pdev->vpci, address_read, address_write,
> >> -                            msi_lower_address_reg(pos), 4, 
> >> pdev->vpci->msi);
> >> +                            offset, 4, pdev->vpci->msi);
> >>      if ( ret )
> >> -        return ret;
> >> +        goto fail;
> >> +    registers[reg_index].offset = offset;
> >> +    registers[reg_index++].size = 4;
> >>  
> >> +    offset = msi_data_reg(pos, pdev->vpci->msi->address64);
> >>      ret = vpci_add_register(pdev->vpci, data_read, data_write,
> >> -                            msi_data_reg(pos, 
> >> pdev->vpci->msi->address64), 2,
> >> -                            pdev->vpci->msi);
> >> +                            offset, 2, pdev->vpci->msi);
> >>      if ( ret )
> >> -        return ret;
> >> +        goto fail;
> >> +    registers[reg_index].offset = offset;
> >> +    registers[reg_index++].size = 2;
> >>  
> >>      if ( pdev->vpci->msi->address64 )
> >>      {
> >> +        offset = msi_upper_address_reg(pos);
> >>          ret = vpci_add_register(pdev->vpci, address_hi_read, 
> >> address_hi_write,
> >> -                                msi_upper_address_reg(pos), 4, 
> >> pdev->vpci->msi);
> >> +                                offset, 4, pdev->vpci->msi);
> >>          if ( ret )
> >> -            return ret;
> >> +            goto fail;
> >> +        registers[reg_index].offset = offset;
> >> +        registers[reg_index++].size = 4;
> >>      }
> >>  
> >>      if ( pdev->vpci->msi->masking )
> >>      {
> >> +        offset = msi_mask_bits_reg(pos, pdev->vpci->msi->address64);
> >>          ret = vpci_add_register(pdev->vpci, mask_read, mask_write,
> >> -                                msi_mask_bits_reg(pos,
> >> -                                                  
> >> pdev->vpci->msi->address64),
> >> -                                4, pdev->vpci->msi);
> >> +                                offset, 4, pdev->vpci->msi);
> >>          if ( ret )
> >> -            return ret;
> >> +            goto fail;
> >> +        registers[reg_index].offset = offset;
> >> +        registers[reg_index++].size = 4;
> > 
> > As commented on the previous patch, I don't like much the usage of
> > this registers array to store which handlers have been added.  It
> > would be best if you just removed every possible handler that could be
> > added, without keeping track of them.
> Make sense, it will indeed be simpler if it is fine to remove all possible 
> registers.
> 
> > 
> > Thinking about it, do we maybe need a helper vcpi function that wipes
> > all handlers from a specific range?  I think it could be helpful here,
> > as the size of the capabilities is well-known, so on error it would be
> > easier to just call such a generic handler to wipe all hooks added to
> > the region covered by the failing capability.
> But I am not sure if that helper function is suitable for all capabilities.
> Like Rebar, its structure can range from 12 bytes long(for a single BAR) to 
> 52 bytes long(for all six BARs).
> If a device supports Rebar and only has a single resizable BAR, does hardware 
> still reserved the range from 13 bytes to 52 bytes for that device?

No, we would need to fetch the size of the capability in the cleanup
function, or figure it otherwise.  Note the same applies to MSI
capability, which has a variable size depending on whether 64bit
addresses and masking is supported.

> I mean if I remove the registers(with range 13 to 52), is it possible that I 
> deleted registers belonging to other abilities?

It is, indeed.  You need to know or calculate the size of the
capability to be removed, but that's likely easier and more robust
that keeping an array with the list of added registers?

Thanks, Roger.