On 2023/05/30 17:30, Sriram Yagnaraman wrote:
-----Original Message-----
From: Akihiko Odaki <akihiko.od...@daynix.com>
Sent: Tuesday, 30 May 2023 04:02
To: Cédric Le Goater <c...@redhat.com>; Sriram Yagnaraman
<sriram.yagnara...@est.tech>; qemu-devel@nongnu.org
Cc: Jason Wang <jasow...@redhat.com>
Subject: Re: [PATCH] igb: Add Function Level Reset to PF and VF
On 2023/05/30 0:07, Cédric Le Goater wrote:
On 5/29/23 09:45, Akihiko Odaki wrote:
On 2023/05/29 16:01, Cédric Le Goater wrote:
On 5/29/23 04:45, Akihiko Odaki wrote:
On 2023/05/28 19:50, Sriram Yagnaraman wrote:
-----Original Message-----
From: Cédric Le Goater <c...@redhat.com>
Sent: Friday, 26 May 2023 19:31
To: qemu-devel@nongnu.org
Cc: Akihiko Odaki <akihiko.od...@daynix.com>; Sriram Yagnaraman
<sriram.yagnara...@est.tech>; Jason Wang
<jasow...@redhat.com>;
Cédric Le Goater <c...@redhat.com>
Subject: [PATCH] igb: Add Function Level Reset to PF and VF
The Intel 82576EB GbE Controller say that the Physical and
Virtual Functions support Function Level Reset. Add the
capability to each device model.
Cc: Akihiko Odaki <akihiko.od...@daynix.com>
Fixes: 3a977deebe6b ("Intrdocue igb device emulation")
Signed-off-by: Cédric Le Goater <c...@redhat.com>
---
hw/net/igb.c | 3 +++
hw/net/igbvf.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/hw/net/igb.c b/hw/net/igb.c index
1c989d767725..08e389338dca
100644
--- a/hw/net/igb.c
+++ b/hw/net/igb.c
@@ -101,6 +101,7 @@ static void igb_write_config(PCIDevice *dev,
uint32_t addr,
trace_igb_write_config(addr, val, len);
pci_default_write_config(dev, addr, val, len);
+ pcie_cap_flr_write_config(dev, addr, val, len);
if (range_covers_byte(addr, len, PCI_COMMAND) &&
(dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
@@
-427,6
+428,8 @@ static void igb_pci_realize(PCIDevice *pci_dev, Error
**errp)
}
/* PCIe extended capabilities (in order) */
+ pcie_cap_flr_init(pci_dev);
+
if (pcie_aer_init(pci_dev, 1, 0x100, 0x40, errp) < 0) {
hw_error("Failed to initialize AER capability");
}
diff --git a/hw/net/igbvf.c b/hw/net/igbvf.c index
284ea611848b..0a58dad06802 100644
--- a/hw/net/igbvf.c
+++ b/hw/net/igbvf.c
@@ -204,6 +204,7 @@ static void igbvf_write_config(PCIDevice
*dev, uint32_t addr, uint32_t val, {
trace_igbvf_write_config(addr, val, len);
pci_default_write_config(dev, addr, val, len);
+ pcie_cap_flr_write_config(dev, addr, val, len);
}
static uint64_t igbvf_mmio_read(void *opaque, hwaddr addr,
unsigned size) @@ -266,6 +267,8 @@ static void
igbvf_pci_realize(PCIDevice *dev, Error
**errp)
hw_error("Failed to initialize PCIe capability");
}
+ pcie_cap_flr_init(dev);
Sorry for my naive question, and perhaps not related to your
patch, IGBVF device class doesn't seem to have any reset functions
registered via igbvf_class_init(). So, I am guessing an FLR will
not trigger igb_vf_reset(), which is probably what we want.
It does through the VTCTRL registers.
You're right. Advertising FLR capability without implementing it
can confuse the guest though such probability is quite a low in
practice. The reset should be implemented first.
I was looking at an issue from a VFIO perspective which does a FLR
on a device when pass through. Software and FLR are equivalent for a
VF.
They should be equivalent according to the datasheet, but
unfortunately current igbvf implementation does nothing when reset.
What Sriram proposes is to add code to actually write VTCTRL when FLR
occurred and make FLR and software reset equivalent. And I think that
should be done before this change; you should advertise FLR
capability after the reset is actually implemented.
AFAICT, the VFs are reset correctly by the OS when created or probed
and by QEMU when they are passthrough in a nested guest OS (with this
patch).
igb_vf_reset() is clearly called in QEMU, see routine
e1000_reset_hw_vf() in Linux.
I don't think this patch makes difference for e1000_reset_hw_vf() as it does not
rely on FLR.
I don't think a reset op is necessary because VFs are software
constructs but I don't mind really. If so, then, I wouldn't mimic what
the OS does by writing the RST bit in the CTRL register of the VF, I
would simply install igb_vf_reset() as a reset handler.
Thinking about the reason why VFIO performs FLR, probably VFIO expects the
FLR clears all of states the kernel set to prevent the VF from leaking kernel
addresses or addresses of other user space which the VF was assigned to in the
past, for example.
Implementing the reset operation is not necessary to make it function but to
make it secure, particularly we promise the guest that we clear the VF state by
advertising FLR.
Regards,
Akihiko Odaki
I did some digging, and I can see that the linux igbvf device driver registers
for FLR and performs a SW reset anyhow.
https://lore.kernel.org/all/20230301105706.547921-1-kamil.mazi...@intel.com/
The register function in the Linux driver should be considered as
something different from FLR. FLR we have discussed is a PCIe capability
that the hardware advertises.
I have not checked what the other drivers do though, I can send a patch if you
think it is worth having a reset operation on the igbvf device.
I think it's better if Cédric writes such a patch and place it before
the patch to advertise FLR in a series. It will be easier to make the
patches in order this way.
Regards,
Akihiko Odaki
Thanks,
C.
Regards,
Akihiko Odaki
I am not sure a VF needs more really, since it is all controlled by
the PF. > C.
+
if (pcie_aer_init(dev, 1, 0x100, 0x40, errp) < 0) {
hw_error("Failed to initialize AER capability");
}
--
2.40.1
