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 09:43:11AM +0000, Chen, Jiqian wrote:
> On 2025/3/31 16:53, Roger Pau Monné wrote:
> > 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.
> I am only talking about the current implementation of init_msix(), which does 
> not need a cleanup function.
> But if you want such a function, even if it is not needed now, I will add it 
> in the next version.

I think it would be cleaner, so that we could remove the MSI-X
specific logic from vpci_deassign_device().

> > 
> >>>
> >>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> >>>>  
> >>>>      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.
> Got it, I originally thought you wanted a general helper function.
> But it seems the case is each capability would have its own separate cleanup 
> function instead.

Sorry, maybe that wasn't clear.  The generic function would be a
helper to zap all handlers from a given PCI config space range, ie:

vpci_remove_registers(struct vpci *vpci, unsigned int offset,
                      unsigned int size);

Maybe it's even worth to just convert vpci_remove_register() into
vpci_remove_registers() and allow it to zap multiple registers at
once?  As vpci_remove_register() is just used for the tests harness.

That function would be used by each capability cleanup routine to
clean it's PCI config space.

Thanks, Roger.