On Tue, 12 Sep 2017 02:49:33 +0000 "Lifei (Louis)" <[email protected]> wrote:
> Hi all > > In commit a52a4c471703e995ceb06f6157d70747823e8a0d said: > > The VFIO configuration space stays untouched, so the guest OS may choose > to skip restoring the BAR addresses as they would seem intact. The PCI > device may be left non-operational. > > While the guest OS choose to restore the BAR addresses only when pci device > no_soft_reset > is not set. So we may not reset the BAR address when no_soft_reset is set. Please see: https://git.qemu.org/?p=qemu.git;a=blob;f=README#l68 https://wiki.qemu.org/Contribute/SubmitAPatch#Do_not_send_as_an_attachment (manually copied from attachment) > From 845914cf5bcd48872224df7f08e53ca2a19a577e Mon Sep 17 00:00:00 2001 > From: Louis Lifei <[email protected]> > Date: Tue, 12 Sep 2017 10:17:59 +0800 > Subject: [PATCH] vfio/pci: don't reset bar address when no_soft_rst set The description above belongs here. The proper way to reference the commit is: a52a4c471703 ("vfio/pci: fix out-of-sync BAR information on reset") > Signed-off-by: Louis Lifei <[email protected]> > --- > hw/vfio/pci.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 31e1edf..6766f6b 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2029,14 +2029,19 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev) > error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name); > } > > - for (nr = 0; nr < PCI_NUM_REGIONS - 1; ++nr) { > - off_t addr = vdev->config_offset + PCI_BASE_ADDRESS_0 + (4 * nr); > - uint32_t val = 0; > - uint32_t len = sizeof(val); > - > - if (pwrite(vdev->vbasedev.fd, &val, len, addr) != len) { > - error_report("%s(%s) reset bar %d failed: %m", __func__, > - vdev->vbasedev.name, nr); > + /* When No_Soft_Reset bit is set to "1", no additional operating > + * system intervention is required beyond writing the PowerState bits. > + */ Follow the existing comment style please, multi-line comments: /* * First line * Nth line */ > + if (vdev->has_pm_reset) { > + for (nr = 0; nr < PCI_NUM_REGIONS - 1; ++nr) { > + off_t addr = vdev->config_offset + PCI_BASE_ADDRESS_0 + (4 * nr); > + uint32_t val = 0; > + uint32_t len = sizeof(val); > + > + if (pwrite(vdev->vbasedev.fd, &val, len, addr) != len) { > + error_report("%s(%s) reset bar %d failed: %m", __func__, > + vdev->vbasedev.name, nr); > + } > } > } > } I don't understand the premise of the patch, if a device reports no_soft_reset, then vfio will not use a PM reset to reset the device. Conversely, simply because a device sets has_pm_reset, does not mean that every reset is a PM reset. The device might also support FLR or we might have performed a secondary bus reset. Therefore whether a device supports has_pm_reset at this point seems fairly irrelevant. Can you clarify exactly what path is getting to this point, having only done a PM reset on a device which advertises no_soft_reset? Thanks, Alex
