[PATCH 0/2] igb: Add FLR support

[PATCH 0/2] igb: Add FLR support

[Cédric Le Goater]

From: Cédric Le Goater <clg@redhat.com>

Hello,

Here is a little series adding FLR to the new IGB models.

Thanks,

C.

Cédric Le Goater (2):
  igb: Add a VF reset handler
  igb: Add Function Level Reset to PF and VF

 hw/net/igb_common.h |  1 +
 hw/net/igb_core.h   |  3 +++
 hw/net/igb.c        |  9 +++++++++
 hw/net/igb_core.c   |  6 ++++--
 hw/net/igbvf.c      | 13 +++++++++++++
 hw/net/trace-events |  1 +
 6 files changed, 31 insertions(+), 2 deletions(-)

-- 
2.41.0

[PATCH 1/2] igb: Add a VF reset handler

[Cédric Le Goater]

From: Cédric Le Goater <clg@redhat.com>

Export the igb_vf_reset() helper routine from the PF model to let the
IGBVF model implement its own device reset.

Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
Suggested-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/net/igb_common.h |  1 +
 hw/net/igb_core.h   |  3 +++
 hw/net/igb.c        |  6 ++++++
 hw/net/igb_core.c   |  6 ++++--
 hw/net/igbvf.c      | 10 ++++++++++
 hw/net/trace-events |  1 +
 6 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/hw/net/igb_common.h b/hw/net/igb_common.h
index 5c261ba9d39c..b316a5bcfa5c 100644
--- a/hw/net/igb_common.h
+++ b/hw/net/igb_common.h
@@ -152,5 +152,6 @@ enum {
 
 uint64_t igb_mmio_read(void *opaque, hwaddr addr, unsigned size);
 void igb_mmio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size);
+void igb_vf_reset(void *opaque, uint16_t vfn);
 
 #endif
diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h
index 9cbbfd516bd5..bf8c46f26b51 100644
--- a/hw/net/igb_core.h
+++ b/hw/net/igb_core.h
@@ -130,6 +130,9 @@ igb_core_set_link_status(IGBCore *core);
 void
 igb_core_pci_uninit(IGBCore *core);
 
+void
+igb_core_vf_reset(IGBCore *core, uint16_t vfn);
+
 bool
 igb_can_receive(IGBCore *core);
 
diff --git a/hw/net/igb.c b/hw/net/igb.c
index 8ff832acfc3b..e70a66ee038e 100644
--- a/hw/net/igb.c
+++ b/hw/net/igb.c
@@ -122,6 +122,12 @@ igb_mmio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
     igb_core_write(&s->core, addr, val, size);
 }
 
+void igb_vf_reset(void *opaque, uint16_t vfn)
+{
+    IGBState *s = opaque;
+    igb_core_vf_reset(&s->core, vfn);
+}
+
 static bool
 igb_io_get_reg_index(IGBState *s, uint32_t *idx)
 {
diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index 8b6b75c522f6..d710b9d32dcb 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -2153,11 +2153,13 @@ static void igb_set_vfmailbox(IGBCore *core, int index, uint32_t val)
     }
 }
 
-static void igb_vf_reset(IGBCore *core, uint16_t vfn)
+void igb_core_vf_reset(IGBCore *core, uint16_t vfn)
 {
     uint16_t qn0 = vfn;
     uint16_t qn1 = vfn + IGB_NUM_VM_POOLS;
 
+    trace_igb_core_vf_reset(vfn);
+
     /* disable Rx and Tx for the VF*/
     core->mac[RXDCTL0 + (qn0 * 16)] &= ~E1000_RXDCTL_QUEUE_ENABLE;
     core->mac[RXDCTL0 + (qn1 * 16)] &= ~E1000_RXDCTL_QUEUE_ENABLE;
@@ -2236,7 +2238,7 @@ static void igb_set_vtctrl(IGBCore *core, int index, uint32_t val)
 
     if (val & E1000_CTRL_RST) {
         vfn = (index - PVTCTRL0) / 0x40;
-        igb_vf_reset(core, vfn);
+        igb_core_vf_reset(core, vfn);
     }
 }
 
diff --git a/hw/net/igbvf.c b/hw/net/igbvf.c
index d55e1e8a6abf..07343fa14a89 100644
--- a/hw/net/igbvf.c
+++ b/hw/net/igbvf.c
@@ -273,6 +273,13 @@ static void igbvf_pci_realize(PCIDevice *dev, Error **errp)
     pcie_ari_init(dev, 0x150);
 }
 
+static void igbvf_qdev_reset_hold(Object *obj)
+{
+    PCIDevice *vf = PCI_DEVICE(obj);
+
+    igb_vf_reset(pcie_sriov_get_pf(vf), pcie_sriov_vf_number(vf));
+}
+
 static void igbvf_pci_uninit(PCIDevice *dev)
 {
     IgbVfState *s = IGBVF(dev);
@@ -287,6 +294,7 @@ static void igbvf_class_init(ObjectClass *class, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(class);
     PCIDeviceClass *c = PCI_DEVICE_CLASS(class);
+    ResettableClass *rc = RESETTABLE_CLASS(class);
 
     c->realize = igbvf_pci_realize;
     c->exit = igbvf_pci_uninit;
@@ -295,6 +303,8 @@ static void igbvf_class_init(ObjectClass *class, void *data)
     c->revision = 1;
     c->class_id = PCI_CLASS_NETWORK_ETHERNET;
 
+    rc->phases.hold = igbvf_qdev_reset_hold;
+
     dc->desc = "Intel 82576 Virtual Function";
     dc->user_creatable = false;
 
diff --git a/hw/net/trace-events b/hw/net/trace-events
index 6b5ba669a25d..1cfb0dda92be 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -274,6 +274,7 @@ igb_core_mdic_read(uint32_t addr, uint32_t data) "MDIC READ: PHY[%u] = 0x%x"
 igb_core_mdic_read_unhandled(uint32_t addr) "MDIC READ: PHY[%u] UNHANDLED"
 igb_core_mdic_write(uint32_t addr, uint32_t data) "MDIC WRITE: PHY[%u] = 0x%x"
 igb_core_mdic_write_unhandled(uint32_t addr) "MDIC WRITE: PHY[%u] UNHANDLED"
+igb_core_vf_reset(uint16_t vfn) "VF%d"
 
 igb_link_set_ext_params(bool asd_check, bool speed_select_bypass, bool pfrstd) "Set extended link params: ASD check: %d, Speed select bypass: %d, PF reset done: %d"
 
-- 
2.41.0

Re: [PATCH 1/2] igb: Add a VF reset handler

[Philippe Mathieu-Daudé]

On 29/8/23 11:05, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@redhat.com>
> 
> Export the igb_vf_reset() helper routine from the PF model to let the

Preferably splitting in 2 patches to KISS,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> IGBVF model implement its own device reset.
> 
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Suggested-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/net/igb_common.h |  1 +
>   hw/net/igb_core.h   |  3 +++
>   hw/net/igb.c        |  6 ++++++
>   hw/net/igb_core.c   |  6 ++++--
>   hw/net/igbvf.c      | 10 ++++++++++
>   hw/net/trace-events |  1 +
>   6 files changed, 25 insertions(+), 2 deletions(-)

Re: [PATCH 1/2] igb: Add a VF reset handler

[Akihiko Odaki]

On 2023/08/29 18:05, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@redhat.com>
> 
> Export the igb_vf_reset() helper routine from the PF model to let the
> IGBVF model implement its own device reset.
> 
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Suggested-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>

RE: [PATCH 1/2] igb: Add a VF reset handler

[Sriram Yagnaraman]

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Tuesday, 29 August 2023 11:05
> 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 1/2] igb: Add a VF reset handler
> 
> From: Cédric Le Goater <clg@redhat.com>
> 
> Export the igb_vf_reset() helper routine from the PF model to let the IGBVF
> model implement its own device reset.
> 
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Suggested-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  hw/net/igb_common.h |  1 +
>  hw/net/igb_core.h   |  3 +++
>  hw/net/igb.c        |  6 ++++++
>  hw/net/igb_core.c   |  6 ++++--
>  hw/net/igbvf.c      | 10 ++++++++++
>  hw/net/trace-events |  1 +
>  6 files changed, 25 insertions(+), 2 deletions(-)
> 

Reviewed-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>

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

[Cédric Le Goater]

From: Cédric Le Goater <clg@redhat.com>

The Intel 82576EB GbE Controller say that the Physical and Virtual
Functions support Function Level Reset. Add the capability to each
device model.

Cc:  Sriram Yagnaraman <sriram.yagnaraman@est.tech>
Fixes: 3a977deebe6b ("Intrdocue igb device emulation")
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Tested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
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 e70a66ee038e..b8c170ad9b1a 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)) {
@@ -433,6 +434,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 07343fa14a89..55e321e4ec20 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);
+
     if (pcie_aer_init(dev, 1, 0x100, 0x40, errp) < 0) {
         hw_error("Failed to initialize AER capability");
     }
-- 
2.41.0

RE: [PATCH 2/2] igb: Add Function Level Reset to PF and VF

[Sriram Yagnaraman]

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Tuesday, 29 August 2023 11:05
> 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 2/2] igb: Add Function Level Reset to PF and VF
> 
> From: Cédric Le Goater <clg@redhat.com>
> 
> The Intel 82576EB GbE Controller say that the Physical and Virtual Functions
> support Function Level Reset. Add the capability to each device model.
> 
> Cc:  Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> Fixes: 3a977deebe6b ("Intrdocue igb device emulation")
> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Tested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 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(+)
> 

Reviewed-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>

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

[Jason Wang]

On Tue, Aug 29, 2023 at 5:06 PM Cédric Le Goater <clg@kaod.org> wrote:
>
> From: Cédric Le Goater <clg@redhat.com>
>
> The Intel 82576EB GbE Controller say that the Physical and Virtual
> Functions support Function Level Reset. Add the capability to each
> device model.
>

Do we need to do migration compatibility for this?

Thanks

> Cc:  Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> Fixes: 3a977deebe6b ("Intrdocue igb device emulation")
> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Tested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 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 e70a66ee038e..b8c170ad9b1a 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)) {
> @@ -433,6 +434,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 07343fa14a89..55e321e4ec20 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);
> +
>      if (pcie_aer_init(dev, 1, 0x100, 0x40, errp) < 0) {
>          hw_error("Failed to initialize AER capability");
>      }
> --
> 2.41.0
>
>

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

[Cédric Le Goater]

On 10/20/23 06:24, Jason Wang wrote:
> On Tue, Aug 29, 2023 at 5:06 PM Cédric Le Goater <clg@kaod.org> wrote:
>>
>> From: Cédric Le Goater <clg@redhat.com>
>>
>> The Intel 82576EB GbE Controller say that the Physical and Virtual
>> Functions support Function Level Reset. Add the capability to each
>> device model.
>>
> 
> Do we need to do migration compatibility for this?

Yes. it does. the config space is now different.

Thanks,

C.


> 
> Thanks
> 
>> Cc:  Sriram Yagnaraman <sriram.yagnaraman@est.tech>
>> Fixes: 3a977deebe6b ("Intrdocue igb device emulation")
>> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Tested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> 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 e70a66ee038e..b8c170ad9b1a 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)) {
>> @@ -433,6 +434,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 07343fa14a89..55e321e4ec20 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);
>> +
>>       if (pcie_aer_init(dev, 1, 0x100, 0x40, errp) < 0) {
>>           hw_error("Failed to initialize AER capability");
>>       }
>> --
>> 2.41.0
>>
>>
> 

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

[Cédric Le Goater]

On 10/20/23 09:40, Cédric Le Goater wrote:
> On 10/20/23 06:24, Jason Wang wrote:
>> On Tue, Aug 29, 2023 at 5:06 PM Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>> From: Cédric Le Goater <clg@redhat.com>
>>>
>>> The Intel 82576EB GbE Controller say that the Physical and Virtual
>>> Functions support Function Level Reset. Add the capability to each
>>> device model.
>>>
>>
>> Do we need to do migration compatibility for this?
> 
> Yes. it does. the config space is now different.

Jason,

To avoid an extra compat property, would it be ok to let the VF peek into
the PF capabilities to set FLR or not ? Something like below.

Thanks,

C.


@@ -238,6 +238,12 @@ static const MemoryRegionOps mmio_ops =
      },
  };
  
+static bool igbvf_check_pf_flr(PCIDevice *dev)
+{
+    return !!(pci_get_long(dev->config + dev->exp.exp_cap + PCI_EXP_DEVCAP)
+              & PCI_EXP_DEVCAP_FLR);
+}
+
  static void igbvf_pci_realize(PCIDevice *dev, Error **errp)
  {
      IgbVfState *s = IGBVF(dev);
@@ -267,7 +273,9 @@ static void igbvf_pci_realize(PCIDevice
          hw_error("Failed to initialize PCIe capability");
      }
  
-    pcie_cap_flr_init(dev);
+    if (igbvf_check_pf_flr(pcie_sriov_get_pf(dev))) {
+        pcie_cap_flr_init(dev);
+    }
  
      if (pcie_aer_init(dev, 1, 0x100, 0x40, errp) < 0) {
          hw_error("Failed to initialize AER capability");


> 
> Thanks,
> 
> C.
> 
> 
>>
>> Thanks
>>
>>> Cc:  Sriram Yagnaraman <sriram.yagnaraman@est.tech>
>>> Fixes: 3a977deebe6b ("Intrdocue igb device emulation")
>>> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> Tested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> 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 e70a66ee038e..b8c170ad9b1a 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)) {
>>> @@ -433,6 +434,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 07343fa14a89..55e321e4ec20 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);
>>> +
>>>       if (pcie_aer_init(dev, 1, 0x100, 0x40, errp) < 0) {
>>>           hw_error("Failed to initialize AER capability");
>>>       }
>>> -- 
>>> 2.41.0
>>>
>>>
>>
> 

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

[Jason Wang]

On Fri, Oct 20, 2023 at 5:41 PM Cédric Le Goater <clg@redhat.com> wrote:
>
> On 10/20/23 09:40, Cédric Le Goater wrote:
> > On 10/20/23 06:24, Jason Wang wrote:
> >> On Tue, Aug 29, 2023 at 5:06 PM Cédric Le Goater <clg@kaod.org> wrote:
> >>>
> >>> From: Cédric Le Goater <clg@redhat.com>
> >>>
> >>> The Intel 82576EB GbE Controller say that the Physical and Virtual
> >>> Functions support Function Level Reset. Add the capability to each
> >>> device model.
> >>>
> >>
> >> Do we need to do migration compatibility for this?
> >
> > Yes. it does. the config space is now different.
>
> Jason,
>
> To avoid an extra compat property, would it be ok to let the VF peek into
> the PF capabilities to set FLR or not ? Something like below.

I might be wrong, but it looks to me it's still a behaviour change?

Thanks

>
> Thanks,
>
> C.
>
>
> @@ -238,6 +238,12 @@ static const MemoryRegionOps mmio_ops =
>       },
>   };
>
> +static bool igbvf_check_pf_flr(PCIDevice *dev)
> +{
> +    return !!(pci_get_long(dev->config + dev->exp.exp_cap + PCI_EXP_DEVCAP)
> +              & PCI_EXP_DEVCAP_FLR);
> +}
> +
>   static void igbvf_pci_realize(PCIDevice *dev, Error **errp)
>   {
>       IgbVfState *s = IGBVF(dev);
> @@ -267,7 +273,9 @@ static void igbvf_pci_realize(PCIDevice
>           hw_error("Failed to initialize PCIe capability");
>       }
>
> -    pcie_cap_flr_init(dev);
> +    if (igbvf_check_pf_flr(pcie_sriov_get_pf(dev))) {
> +        pcie_cap_flr_init(dev);
> +    }
>
>       if (pcie_aer_init(dev, 1, 0x100, 0x40, errp) < 0) {
>           hw_error("Failed to initialize AER capability");
>
>
> >
> > Thanks,
> >
> > C.
> >
> >
> >>
> >> Thanks
> >>
> >>> Cc:  Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> >>> Fixes: 3a977deebe6b ("Intrdocue igb device emulation")
> >>> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>> Tested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>> 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 e70a66ee038e..b8c170ad9b1a 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)) {
> >>> @@ -433,6 +434,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 07343fa14a89..55e321e4ec20 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);
> >>> +
> >>>       if (pcie_aer_init(dev, 1, 0x100, 0x40, errp) < 0) {
> >>>           hw_error("Failed to initialize AER capability");
> >>>       }
> >>> --
> >>> 2.41.0
> >>>
> >>>
> >>
> >
>

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

[Akihiko Odaki]

On 2023/10/23 12:11, Jason Wang wrote:
> On Fri, Oct 20, 2023 at 5:41 PM Cédric Le Goater <clg@redhat.com> wrote:
>>
>> On 10/20/23 09:40, Cédric Le Goater wrote:
>>> On 10/20/23 06:24, Jason Wang wrote:
>>>> On Tue, Aug 29, 2023 at 5:06 PM Cédric Le Goater <clg@kaod.org> wrote:
>>>>>
>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>>
>>>>> The Intel 82576EB GbE Controller say that the Physical and Virtual
>>>>> Functions support Function Level Reset. Add the capability to each
>>>>> device model.
>>>>>
>>>>
>>>> Do we need to do migration compatibility for this?
>>>
>>> Yes. it does. the config space is now different.
>>
>> Jason,
>>
>> To avoid an extra compat property, would it be ok to let the VF peek into
>> the PF capabilities to set FLR or not ? Something like below.
> 
> I might be wrong, but it looks to me it's still a behaviour change?

I think it's fine as long as the FLR capability of the PF is initialized 
with a compat property; there is no need of another property for the VFs.

Regards,
Akihiko Odaki

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

[Cédric Le Goater]

On 10/23/23 12:57, Akihiko Odaki wrote:
> On 2023/10/23 12:11, Jason Wang wrote:
>> On Fri, Oct 20, 2023 at 5:41 PM Cédric Le Goater <clg@redhat.com> wrote:
>>>
>>> On 10/20/23 09:40, Cédric Le Goater wrote:
>>>> On 10/20/23 06:24, Jason Wang wrote:
>>>>> On Tue, Aug 29, 2023 at 5:06 PM Cédric Le Goater <clg@kaod.org> wrote:
>>>>>>
>>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>>>
>>>>>> The Intel 82576EB GbE Controller say that the Physical and Virtual
>>>>>> Functions support Function Level Reset. Add the capability to each
>>>>>> device model.
>>>>>>
>>>>>
>>>>> Do we need to do migration compatibility for this?
>>>>
>>>> Yes. it does. the config space is now different.
>>>
>>> Jason,
>>>
>>> To avoid an extra compat property, would it be ok to let the VF peek into
>>> the PF capabilities to set FLR or not ? Something like below.
>>
>> I might be wrong, but it looks to me it's still a behaviour change?
> 
> I think it's fine as long as the FLR capability of the PF is initialized with a compat property; there is no need of another property for the VFs.

Yes. I wasn't clear enough in the statement above. I am trying to
avoid *2* compat properties, one in each model.

We could also check the PF property in the VF. I think this better.
I will send a v2.

Thanks,

C.

Re: [PATCH 0/2] igb: Add FLR support

[Cédric Le Goater]

Hello Jason,

On 8/29/23 11:05, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@redhat.com>
> 
> Hello,
> 
> Here is a little series adding FLR to the new IGB models.

Gentle ping to remind you to grab this small series for the
next network PR. It has been reviewed. Or I can if you are
done for 8.2.

Thanks,

C.

> Thanks,
> 
> C.
> 
> Cédric Le Goater (2):
>    igb: Add a VF reset handler
>    igb: Add Function Level Reset to PF and VF
> 
>   hw/net/igb_common.h |  1 +
>   hw/net/igb_core.h   |  3 +++
>   hw/net/igb.c        |  9 +++++++++
>   hw/net/igb_core.c   |  6 ++++--
>   hw/net/igbvf.c      | 13 +++++++++++++
>   hw/net/trace-events |  1 +
>   6 files changed, 31 insertions(+), 2 deletions(-)
>