Re: [PATCH V7 17/29] pci: export functions for cpr

[Michael S. Tsirkin]

On Thu, Jan 06, 2022 at 05:48:28PM -0500, Steven Sistare wrote:
> On 1/5/2022 3:16 PM, Michael S. Tsirkin wrote:
> > On Wed, Jan 05, 2022 at 12:22:25PM -0500, Steven Sistare wrote:
> >> On 12/22/2021 6:07 PM, Michael S. Tsirkin wrote:
> >>> On Wed, Dec 22, 2021 at 11:05:22AM -0800, Steve Sistare wrote:
> >>>> Export msix_is_pending, msix_init_vector_notifiers, and 
> >>>> pci_update_mappings
> >>>> for use by cpr.  No functional change.
> >>>>
> >>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >>>
> >>> With things like that, I prefer when the API is exported
> >>> together with the patch that uses it.
> >>> This was I can see why we are exporting these APIs.
> >>> Esp wrt pci_update_mappings, it's designed as an
> >>> internal API.
> >>
> >> Hi Michael, thanks very much for reviewing these patches.
> >>
> >> Serendipitously, I stopped calling pci_update_mappings from vfio code 
> >> earlier
> >> in the series.  I will revert its scope.
> >>
> >> I would prefer to keep this patch separate from the use of these functions 
> >> in
> >> "vfio-pci cpr part 2 msi", to make the latter smaller and easier to 
> >> understand.
> >> How about if I say more in this commit message? :
> >>
> >>   Export msix_is_pending and msix_init_vector_notifiers for use in vfio 
> >> cpr.
> >>   Both are needed in the vfio-pci post-load function during cpr-load.
> >>   msix_is_pending is checked to enable the PBA memory region.
> >>   msix_init_vector_notifiers is called to register notifier callbacks, 
> >> without
> >>   the other side effects of msix_set_vector_notifiers.
> >>
> >> - Steve
> > 
> > Well the reason the side effects are there is to avoid losing events,
> > no? I'd like to figure out a bit better why we don't need them,
> 
> Currently I do not call vfio_msix_vector_do_use during resume, but
> instead execute a subset of its actions in vfio_claim_vectors, which is
> defined in vfio-cpr: cpr part 2.
> 
> > and when should users call msix_init_vector_notifiers versus
> > msix_set_vector_notifiers.
> 
> If I call msix_set_vector_notifiers, it calls the use notifier
> vfio_msix_vector_use, which calls vfio_msix_vector_do_use.  The latter
> gets confused and breaks the vectors because vector-related fields are
> only partially initialized.  The details are unimportant, because --
> 
> Instead of adding msix_init_vector_notifiers, I will call
> msix_set_vector_notifiers, but bail from vfio_msix_vector_do_use if resuming.
> Tested and works.
> 
> Thus this patch becomes simply "pci: export msix_is_pending".  I can keep it,
> or fold it into "vfio-pci: cpr part 2 (msi)".  Your call.
> 
> - Steve

Keep it, it's ok.

> >>>> ---
> >>>>  hw/pci/msix.c         | 20 ++++++++++++++------
> >>>>  hw/pci/pci.c          |  3 +--
> >>>>  include/hw/pci/msix.h |  5 +++++
> >>>>  include/hw/pci/pci.h  |  1 +
> >>>>  4 files changed, 21 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> >>>> index ae9331c..73f4259 100644
> >>>> --- a/hw/pci/msix.c
> >>>> +++ b/hw/pci/msix.c
> >>>> @@ -64,7 +64,7 @@ static uint8_t *msix_pending_byte(PCIDevice *dev, int 
> >>>> vector)
> >>>>      return dev->msix_pba + vector / 8;
> >>>>  }
> >>>>  
> >>>> -static int msix_is_pending(PCIDevice *dev, int vector)
> >>>> +int msix_is_pending(PCIDevice *dev, unsigned int vector)
> >>>>  {
> >>>>      return *msix_pending_byte(dev, vector) & msix_pending_mask(vector);
> >>>>  }
> >>>> @@ -579,6 +579,17 @@ static void 
> >>>> msix_unset_notifier_for_vector(PCIDevice *dev, unsigned int vector)
> >>>>      dev->msix_vector_release_notifier(dev, vector);
> >>>>  }
> >>>>  
> >>>> +void msix_init_vector_notifiers(PCIDevice *dev,
> >>>> +                                MSIVectorUseNotifier use_notifier,
> >>>> +                                MSIVectorReleaseNotifier 
> >>>> release_notifier,
> >>>> +                                MSIVectorPollNotifier poll_notifier)
> >>>> +{
> >>>> +    assert(use_notifier && release_notifier);
> >>>> +    dev->msix_vector_use_notifier = use_notifier;
> >>>> +    dev->msix_vector_release_notifier = release_notifier;
> >>>> +    dev->msix_vector_poll_notifier = poll_notifier;
> >>>> +}
> >>>> +
> >>>>  int msix_set_vector_notifiers(PCIDevice *dev,
> >>>>                                MSIVectorUseNotifier use_notifier,
> >>>>                                MSIVectorReleaseNotifier release_notifier,
> >>>> @@ -586,11 +597,8 @@ int msix_set_vector_notifiers(PCIDevice *dev,
> >>>>  {
> >>>>      int vector, ret;
> >>>>  
> >>>> -    assert(use_notifier && release_notifier);
> >>>> -
> >>>> -    dev->msix_vector_use_notifier = use_notifier;
> >>>> -    dev->msix_vector_release_notifier = release_notifier;
> >>>> -    dev->msix_vector_poll_notifier = poll_notifier;
> >>>> +    msix_init_vector_notifiers(dev, use_notifier, release_notifier,
> >>>> +                               poll_notifier);
> >>>>  
> >>>>      if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> >>>>          (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
> >>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>>> index e5993c1..0fd21e1 100644
> >>>> --- a/hw/pci/pci.c
> >>>> +++ b/hw/pci/pci.c
> >>>> @@ -225,7 +225,6 @@ static const TypeInfo pcie_bus_info = {
> >>>>  };
> >>>>  
> >>>>  static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num);
> >>>> -static void pci_update_mappings(PCIDevice *d);
> >>>>  static void pci_irq_handler(void *opaque, int irq_num, int level);
> >>>>  static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, 
> >>>> Error **);
> >>>>  static void pci_del_option_rom(PCIDevice *pdev);
> >>>> @@ -1366,7 +1365,7 @@ static pcibus_t pci_bar_address(PCIDevice *d,
> >>>>      return new_addr;
> >>>>  }
> >>>>  
> >>>> -static void pci_update_mappings(PCIDevice *d)
> >>>> +void pci_update_mappings(PCIDevice *d)
> >>>>  {
> >>>>      PCIIORegion *r;
> >>>>      int i;
> >>>> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> >>>> index 4c4a60c..46606cf 100644
> >>>> --- a/include/hw/pci/msix.h
> >>>> +++ b/include/hw/pci/msix.h
> >>>> @@ -32,6 +32,7 @@ int msix_present(PCIDevice *dev);
> >>>>  bool msix_is_masked(PCIDevice *dev, unsigned vector);
> >>>>  void msix_set_pending(PCIDevice *dev, unsigned vector);
> >>>>  void msix_clr_pending(PCIDevice *dev, int vector);
> >>>> +int msix_is_pending(PCIDevice *dev, unsigned vector);
> >>>>  
> >>>>  int msix_vector_use(PCIDevice *dev, unsigned vector);
> >>>>  void msix_vector_unuse(PCIDevice *dev, unsigned vector);
> >>>> @@ -41,6 +42,10 @@ void msix_notify(PCIDevice *dev, unsigned vector);
> >>>>  
> >>>>  void msix_reset(PCIDevice *dev);
> >>>>  
> >>>> +void msix_init_vector_notifiers(PCIDevice *dev,
> >>>> +                                MSIVectorUseNotifier use_notifier,
> >>>> +                                MSIVectorReleaseNotifier 
> >>>> release_notifier,
> >>>> +                                MSIVectorPollNotifier poll_notifier);
> >>>>  int msix_set_vector_notifiers(PCIDevice *dev,
> >>>>                                MSIVectorUseNotifier use_notifier,
> >>>>                                MSIVectorReleaseNotifier release_notifier,
> >>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> >>>> index e7cdf2d..cc63dd4 100644
> >>>> --- a/include/hw/pci/pci.h
> >>>> +++ b/include/hw/pci/pci.h
> >>>> @@ -910,5 +910,6 @@ extern const VMStateDescription vmstate_pci_device;
> >>>>  
> >>>>  MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
> >>>>  void pci_set_power(PCIDevice *pci_dev, bool state);
> >>>> +void pci_update_mappings(PCIDevice *d);
> >>>>  
> >>>>  #endif
> >>>> -- 
> >>>> 1.8.3.1
> >>>
> >