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.sist...@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
>>>> ---
>>>> 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
>>>
>
