Re: [PATCH] igb: Add Function Level Reset to PF and VF

["Cédric Le Goater" <clg@redhat.com>]

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 <clg@redhat.com>
>>> Sent: Friday, 26 May 2023 19:31
>>> To: qemu-devel@nongnu.org
>>> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>; Sriram Yagnaraman
>>> <sriram.yagnaraman@est.tech>; Jason Wang <jasowang@redhat.com>; Cédric
>>> Le Goater <clg@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.odaki@daynix.com>
>>> Fixes: 3a977deebe6b ("Intrdocue igb device emulation")
>>> Signed-off-by: Cédric Le Goater <clg@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.

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