[v16] hw/pci: SR-IOV related fixes and improvements

[PATCH v16 02/13] hw/ppc/spapr_pci: Do not create DT for disabled PCI device

Posted by Akihiko Odaki 2 months, 1 week ago

Disabled means it is a disabled SR-IOV VF or it is powered off, and
hidden from the guest.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/ppc/spapr_pci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7cf9904c3546..f63182a03c41 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1296,6 +1296,10 @@ static void spapr_dt_pci_device_cb(PCIBus *bus, PCIDevice *pdev,
         return;
     }
 
+    if (!pdev->enabled) {
+        return;
+    }
+
     err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
     if (err < 0) {
         p->err = err;

-- 
2.46.0

Re: [PATCH v16 02/13] hw/ppc/spapr_pci: Do not create DT for disabled PCI device

Posted by Cédric Le Goater 2 months ago

Hello,

Adding :

   Harsh for QEMU/PPC pseries machine,
   Shivaprasad for KVM/PPC VFIO and IOMMU support.

Could you please give us your feedback on these changes ?

Thanks,

C.

  

  On 9/13/24 05:44, Akihiko Odaki wrote:
> Disabled means it is a disabled SR-IOV VF or it is powered off, and
> hidden from the guest.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   hw/ppc/spapr_pci.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7cf9904c3546..f63182a03c41 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1296,6 +1296,10 @@ static void spapr_dt_pci_device_cb(PCIBus *bus, PCIDevice *pdev,
>           return;
>       }
>   
> +    if (!pdev->enabled) {
> +        return;
> +    }
> +
>       err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
>       if (err < 0) {
>           p->err = err;
>

Re: [PATCH v16 02/13] hw/ppc/spapr_pci: Do not create DT for disabled PCI device

Posted by Shivaprasad G Bhat 1 month, 1 week ago

On 9/18/24 7:57 PM, Cédric Le Goater wrote:
> Hello,
>
> Adding :
>
>   Harsh for QEMU/PPC pseries machine,
>   Shivaprasad for KVM/PPC VFIO and IOMMU support.
>
> Could you please give us your feedback on these changes ?
>
> Thanks,
>
> C.
>
>
>
>  On 9/13/24 05:44, Akihiko Odaki wrote:
>> Disabled means it is a disabled SR-IOV VF or it is powered off, and
>> hidden from the guest.

I see you are taking care of not powering on VFs in the following 8th 
patch in

the series. Without it, this patch doesn't hold. Hope this patch and the 
8th patch

  go together.


Reviewed-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>


Thanks,

Shivaprasad

>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/ppc/spapr_pci.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 7cf9904c3546..f63182a03c41 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -1296,6 +1296,10 @@ static void spapr_dt_pci_device_cb(PCIBus 
>> *bus, PCIDevice *pdev,
>>           return;
>>       }
>> +    if (!pdev->enabled) {
>> +        return;
>> +    }
>>       err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
>>       if (err < 0) {
>>           p->err = err;
>>

Re: [PATCH v16 02/13] hw/ppc/spapr_pci: Do not create DT for disabled PCI device

Posted by Shivaprasad G Bhat 1 month, 1 week ago

On 10/11/24 10:52 PM, Shivaprasad G Bhat wrote:
>
> On 9/18/24 7:57 PM, Cédric Le Goater wrote:
>> Hello,
>>
>> Adding :
>>
>>   Harsh for QEMU/PPC pseries machine,
>>   Shivaprasad for KVM/PPC VFIO and IOMMU support.
>>
>> Could you please give us your feedback on these changes ?
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>  On 9/13/24 05:44, Akihiko Odaki wrote:
>>> Disabled means it is a disabled SR-IOV VF or it is powered off, and
>>> hidden from the guest.
>
> I see you are taking care of not powering on VFs in the following 8th 
> patch in
>
> the series. Without it, this patch doesn't hold. Hope this patch and 
> the 8th patch
>
>  go together.
>
>
> Reviewed-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
>
>

While review/testing the patch again with the [8/13], I see the same 
check is needed

in spapr_pci_dt_populate() before the call to spapr_dt_pci_device() to 
take care of the

hotplug path. Kindly add the same there too. So, my Review-by would be 
with that.


Thanks,

Shivaprasad


> Thanks,
>
> Shivaprasad
>
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>>   hw/ppc/spapr_pci.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 7cf9904c3546..f63182a03c41 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -1296,6 +1296,10 @@ static void spapr_dt_pci_device_cb(PCIBus 
>>> *bus, PCIDevice *pdev,
>>>           return;
>>>       }
>>> +    if (!pdev->enabled) {
>>> +        return;
>>> +    }
>>>       err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
>>>       if (err < 0) {
>>>           p->err = err;
>>>

Re: [PATCH v16 02/13] hw/ppc/spapr_pci: Do not create DT for disabled PCI device

Posted by Harsh Prateek Bora 2 months ago


On 9/18/24 19:57, Cédric Le Goater wrote:
> Hello,
> 
> Adding :
> 
>    Harsh for QEMU/PPC pseries machine,
>    Shivaprasad for KVM/PPC VFIO and IOMMU support.
> 
> Could you please give us your feedback on these changes ?
> 
> Thanks,
> 
> C.
> 
> 
> 
>   On 9/13/24 05:44, Akihiko Odaki wrote:
>> Disabled means it is a disabled SR-IOV VF or it is powered off, and
>> hidden from the guest.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/ppc/spapr_pci.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 7cf9904c3546..f63182a03c41 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -1296,6 +1296,10 @@ static void spapr_dt_pci_device_cb(PCIBus *bus, 
>> PCIDevice *pdev,
>>           return;
>>       }
>> +    if (!pdev->enabled) {
>> +        return;
>> +    }
>> +

While I will let Shivaprasad comment from IO perspective, I would like 
to suggest merging this condition with the error condition check 
preceding it.

regards,
Harsh

>>       err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
>>       if (err < 0) {
>>           p->err = err;
>>
> 
>