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.c...@amd.com>
> >>>>
> >>>> 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.
