Re: [PATCH V3 12/22] vfio-pci: cpr part 1

[Steven Sistare <steven.sistare@oracle.com>]

On 5/24/2021 2:29 PM, Steven Sistare wrote:
> On 5/21/2021 6:24 PM, Alex Williamson wrote:> On Fri,  7 May 2021 05:25:10 -0700
>> Steve Sistare <steven.sistare@oracle.com> wrote:
>>
>>>[...]
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index 7a4fb6c..f7ac9f03 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -29,6 +29,8 @@
>>>  #include "hw/qdev-properties.h"
>>>  #include "hw/qdev-properties-system.h"
>>>  #include "migration/vmstate.h"
>>> +#include "migration/cpr.h"
>>> +#include "qemu/env.h"
>>>  #include "qemu/error-report.h"
>>>  #include "qemu/main-loop.h"
>>>  #include "qemu/module.h"
>>> @@ -1612,6 +1614,14 @@ static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled)
>>>      }
>>>  }
>>>  
>>> +static void vfio_config_sync(VFIOPCIDevice *vdev, uint32_t offset, size_t len)
>>> +{
>>> +    if (pread(vdev->vbasedev.fd, vdev->pdev.config + offset, len,
>>> +          vdev->config_offset + offset) != len) {
>>> +        error_report("vfio_config_sync pread failed");
>>> +    }
>>> +}
>>> +
>>>  static void vfio_bar_prepare(VFIOPCIDevice *vdev, int nr)
>>>  {
>>>      VFIOBAR *bar = &vdev->bars[nr];
>>> @@ -1652,6 +1662,7 @@ static void vfio_bars_prepare(VFIOPCIDevice *vdev)
>>>  static void vfio_bar_register(VFIOPCIDevice *vdev, int nr)
>>>  {
>>>      VFIOBAR *bar = &vdev->bars[nr];
>>> +    PCIDevice *pdev = &vdev->pdev;
>>>      char *name;
>>>  
>>>      if (!bar->size) {
>>> @@ -1672,7 +1683,10 @@ static void vfio_bar_register(VFIOPCIDevice *vdev, int nr)
>>>          }
>>>      }
>>>  
>>> -    pci_register_bar(&vdev->pdev, nr, bar->type, bar->mr);
>>> +    pci_register_bar(pdev, nr, bar->type, bar->mr);
>>> +    if (pdev->reused) {
>>> +        vfio_config_sync(vdev, pci_bar(pdev, nr), 8);
>>
>> Assuming 64-bit BARs?  This might be the first case where we actually
>> rely on the kernel BAR values, IIRC we usually use QEMU's emulation.
> 
> No asssumptions.  vfio_config_sync() preads a piece of config space using a single 
> system call, copying directly to the qemu buffer, not looking at words or calling any
> action functions.
> 
>[...] 
>>> @@ -3098,6 +3115,11 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>>      vfio_register_req_notifier(vdev);
>>>      vfio_setup_resetfn_quirk(vdev);
>>>  
>>> +    vfio_config_sync(vdev, pdev->msix_cap + PCI_MSIX_FLAGS, 2);
>>> +    if (pdev->reused) {
>>> +        pci_update_mappings(pdev);
>>> +    }
>>> +
>>
>> Are the msix flag sync and mapping update related?  They seem
>> independent to me.  A blank line and comment would be helpful.
> 
> OK.
> 
>> I expect we'd need to call msix_enabled() somewhere for the msix flag
>> sync to be effective.
> 
> Yes, vfio_pci_post_load in cpr part 2 calls msix_enabled.
> 
>> Is there an assumption here of msi-x only support or is it not needed
>> for msi or intx?
> 
> The code supports msi-x and msi.  However, I should only be sync'ing PCI_MSIX_FLAGS
> if pdev->cap_present & QEMU_PCI_CAP_MSIX.  And, I am missing a sync for PCI_MSI_FLAGS.
> I'll fix that.

Hi Alex, FYI, I am making more changes here.  The calls to vfio_config_sync fix pdev->config[]
words that are initialized during vfio_realize(), by pread'ing from the live kernel config.
However, it makes more sense to suppress the undesired re-initialization, rather than undo
the damage later.  Thus I will add a few more 'if (!pdev->reused)' guards in msix and pci bar
init functions, and delete vfio_config_sync.

Most of the config is preserved in the kernel across restart.  However, the bits that are
purely emulated (indicated by the emulated_config_bits mask) may be rejected when they 
are written through to the kernel, and thus are currently lost on restart.  I need to save 
pdev->config[] in the vmstate file, and in vfio_pci_post_load, merge it with the kernel 
config using emulated_config_bits.

Sound sane?

- Steve