diff mbox series

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

Message ID 20230829090529.184438-3-clg@kaod.org
State New
Headers show
Series igb: Add FLR support | expand

Commit Message

Cédric Le Goater Aug. 29, 2023, 9:05 a.m. UTC 
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(+)

Comments

Sriram Yagnaraman Aug. 30, 2023, 10:12 a.m. UTC | #1 
> -----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>
Jason Wang Oct. 20, 2023, 4:24 a.m. UTC | #2 
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
>
>
Cédric Le Goater Oct. 20, 2023, 7:40 a.m. UTC | #3 
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
>>
>>
>
Cédric Le Goater Oct. 20, 2023, 9:41 a.m. UTC | #4 
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
>>>
>>>
>>
>
Jason Wang Oct. 23, 2023, 3:11 a.m. UTC | #5 
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
> >>>
> >>>
> >>
> >
>
Akihiko Odaki Oct. 23, 2023, 10:57 a.m. UTC | #6 
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
Cédric Le Goater Oct. 23, 2023, 11:28 a.m. UTC | #7 
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.
diff mbox series

Patch

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");
     }