> -----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/
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.
> >
> > 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
> >>>>>
> >>>>
> >>>
> >>
> >
