Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

On Tue, Oct 17, 2017 at 06:13:35PM -0500, Juan Alvarez wrote:
> On 10/17/17 1:52 PM, Bjorn Helgaas wrote:
> > I'm suggesting that maybe there should be a generic way to prevent
> > binding to VF devices that works for everybody and doesn't require any
> > arch-specific code at all.
> 
> The patch that you have suggested in kernel 4.12 is also a generic way.
> 
> https://marc.info/?l=linux-kernel=149002335105804=2

The patch mentioned above is now upstream:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0e7df22401a3

> Perhaps we can use the same constructs that this patch uses at our level.
> Nonetheless, that will take a rework to this patch and possibly an export
> of a function to set drivers_autoprobe globally. I know that was frowned
> upon on first email thread. Let me know if this is an acceptable
> solution.

I would definitely like to see you use a solution that is somehow
connected with the sriov->drivers_autoprobe mechanism added by
0e7df22401a3.

0e7df22401a3 is a per-PF solution, and you're looking for a
system-wide switch.  I could imagine a system-wide flag exposed in
sysfs and used by pci_device_can_probe().  Or maybe some arch hook in
the pci_device_add() path that clears sriov->drivers_autoprobe for
every PF.

I think the important thing is that it is coordinated as much as
possible with other driver binding controls and not unnecessarily
arch-dependent.

Bjorn

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

On 10/17/17 8:36 PM, Alexey Kardashevskiy wrote:
> PowerNV KVM guest is a pseries machine so this code will execute there.
>
The configure sriov path will fail and not enable sriov if resources are
not met. I.e. the IOV Bar is not set in PF IOV Resources, which in this
case gets assigned by firmware.

We have separated the calls to put PowerNV and PSeries as machine dependent
calls. Furthermore, we are adding device node properties in the device tree to 
identify if
this is an SR-IOV slot on Phyp Pseries platform. Verification will be in place 
to
distinguish between platforms that support SR-IOV in PSeries.
>> Which is the reason for
>> the separation of calls to the machine dependent stuff.
>>> We also use the pseries platform when running under KVM.
>>>
>>> cheers
>>>
>> If anyone plans to enable SR-IOV on Pseries platform, firmware must provide 
>> the
>> resources to enable the VFs and map them with system resources.
> This is what the PowerNV platform does.
Again, we have separated the machine dependent calls to different platforms. In 
our
case we don't use opal and our dependent on phyp to associate resources.
>
>> A new version
>> of the PAPR Document will be added to document these system resources.
> The guest simply gets yet another PCI device, how is IOV different here?
>
> In regard of EEH, the API does not change afaik, it is up to the hypervisor
> (KVM+QEMU) to handle IOV case correctly.

I don't understand your question entirely, can you rephrase?

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Alexey Kardashevskiy]

On 18/10/17 12:01, Alexey Kardashevskiy wrote:
> On 18/10/17 01:11, Bryant G. Ly wrote:
>> Adding Juan back into the cc: jjalv...@linux.vnet.ibm.com
>>
>>
>> On 10/16/17 10:38 PM, Michael Ellerman wrote:
>>> "Bryant G. Ly"  writes:
 On 10/12/17 1:29 PM, Bjorn Helgaas wrote:
>>> ...
> If that's the case, how to you ever bind a driver to these VFs?  The
> changelog says you don't want VF drivers to load *immediately*, so I
> assume you do want them to load eventually.
>
 The VF's that get dynamically created within the configure SR-IOV call, 
 on the Pseries Platform, wont be matched with a driver. - We do not
 want it to match.

 The Power Hypervisor will load the VFs. The VF's will get assigned(by 
 the user) > via the HMC or Novalink in this environment which will
 then trigger PHYP to load the VF device node to the device tree.
>>> What about the other "Power Hypervisor"? ie. KVM running on Power.
>> This path is only exercised when configuring SR-IOV for Pseries LPAR,
>> therefore it will not affect PowerNV or KVM(opal).
> 
> PowerNV KVM guest is a pseries machine so this code will execute there.
> 
>> Which is the reason for
>> the separation of calls to the machine dependent stuff.
>>> We also use the pseries platform when running under KVM.
>>>
>>> cheers
>>>
>> If anyone plans to enable SR-IOV on Pseries platform, firmware must provide 
>> the
>> resources to enable the VFs and map them with system resources.
> 
> This is what the PowerNV platform does.
> 
>> A new version
>> of the PAPR Document will be added to document these system resources.
> 
> The guest simply gets yet another PCI device, how is IOV different here?
> 
> In regard of EEH, the API does not change afaik, it is up to the hypervisor
> (KVM+QEMU) to handle IOV case correctly.
> 
> 
>> Lastly,
>> we were not aware that there is an intention to enable SR-IOV in adapters 
>> assigned
>> to a VM with Pseries running on KVM.
> 
> There is no any special enablement of IOV for a VM on powernv, once
> configured in the powernv host, we can just pass VFs to QEMU and therefore
> to a pseries guest, it is just a normal PCI device.
> 
> Do you assign a PF to a VM and create VFs from inside the VM? Or only pHyp
> is allowed to do that? Sorry, I know nothing about pHyp on this matter.
> 

Never mind this last question, I've just read the entire thread about this
hosting partition thing.



-- 
Alexey

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Alexey Kardashevskiy]

On 18/10/17 01:11, Bryant G. Ly wrote:
> Adding Juan back into the cc: jjalv...@linux.vnet.ibm.com
> 
> 
> On 10/16/17 10:38 PM, Michael Ellerman wrote:
>> "Bryant G. Ly"  writes:
>>> On 10/12/17 1:29 PM, Bjorn Helgaas wrote:
>> ...
 If that's the case, how to you ever bind a driver to these VFs?  The
 changelog says you don't want VF drivers to load *immediately*, so I
 assume you do want them to load eventually.

>>> The VF's that get dynamically created within the configure SR-IOV call, 
>>> on the Pseries Platform, wont be matched with a driver. - We do not
>>> want it to match.
>>>
>>> The Power Hypervisor will load the VFs. The VF's will get assigned(by 
>>> the user) > via the HMC or Novalink in this environment which will
>>> then trigger PHYP to load the VF device node to the device tree.
>> What about the other "Power Hypervisor"? ie. KVM running on Power.
> This path is only exercised when configuring SR-IOV for Pseries LPAR,
> therefore it will not affect PowerNV or KVM(opal).

PowerNV KVM guest is a pseries machine so this code will execute there.

> Which is the reason for
> the separation of calls to the machine dependent stuff.
>> We also use the pseries platform when running under KVM.
>>
>> cheers
>>
> If anyone plans to enable SR-IOV on Pseries platform, firmware must provide 
> the
> resources to enable the VFs and map them with system resources.

This is what the PowerNV platform does.

> A new version
> of the PAPR Document will be added to document these system resources.

The guest simply gets yet another PCI device, how is IOV different here?

In regard of EEH, the API does not change afaik, it is up to the hypervisor
(KVM+QEMU) to handle IOV case correctly.


> Lastly,
> we were not aware that there is an intention to enable SR-IOV in adapters 
> assigned
> to a VM with Pseries running on KVM.

There is no any special enablement of IOV for a VM on powernv, once
configured in the powernv host, we can just pass VFs to QEMU and therefore
to a pseries guest, it is just a normal PCI device.

Do you assign a PF to a VM and create VFs from inside the VM? Or only pHyp
is allowed to do that? Sorry, I know nothing about pHyp on this matter.


> Furthermore, this could be left as a todo
> for the future if this type of configuration is needed.




-- 
Alexey

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

On 10/17/17 1:52 PM, Bjorn Helgaas wrote:
> Right.  But that patch isn't really on the path to inclusion (mainly
> because it's test framework and doesn't fix a bug or add support for
> new hardware or features).
I was not aware of this decision and this will cause changes to this patch.
>
> I'm suggesting that maybe there should be a generic way to prevent
> binding to VF devices that works for everybody and doesn't require any
> arch-specific code at all.

The patch that you have suggested in kernel 4.12 is also a generic way.

https://marc.info/?l=linux-kernel=149002335105804=2

Perhaps we can use the same constructs that this patch uses at our level. 
Nonetheless,
that will take a rework to this patch and possibly an export of a function to 
set drivers_autoprobe
globally. I know that was frowned upon on first email thread. Let me know if 
this is an
acceptable solution.
>> So like existing way of enabling SRIOV we still rely on the PF driver to
>> enable VFs - but in this case the attachment phase is done via a user
>> action via a management console in our case (novalink or hmc) triggered
>> event that will essentially act like a hotplug.
>>
>> So in the fine details of that user triggered action the system
>> firmware will bind the VFs, allowing resources to be allocated to
>> the VF.  - Which essentially does all the attaching as we know it
>> today but is managed by PHYP not by the kernel.
> What exactly does "firmware binding the VFs" mean?  I guess this must
> mean assigning a VF to a partition, injecting a hotplug add event to
> that partition, and making the VF visible in config space?
 Firmware basically adds a device node to the device tree as defined
 in the (PAPR) Power Architecture Platform Requirements document.
>>> From the point of view of the kernel that consumes this device tree
>>> and owns the VF, I guess this looks like a hot-add.  
>> Correct, this is intended. We want to minimize change and only focus
>> on configure SR-IOV path in the PF on PSeries platform.
>>> It would be nice
>>> if this could be exposed to that kernel by having the firmware inject
>>> a normal PCIe hotplug interrupt, but I'm guessing it's not that
>>> simple.
>> I don't understand entirely your suggestion. However, in the case of
>> interrupts PHYP owns all the resources and will be associated accordingly
>> with a partitionable endpoint (PE).
> Assume you have a Linux kernel, and PHYP assigns a VF to it.  What
> happens in that Linux kernel? 

When  kernel boots it goes through the lists of buses in the device tree. For 
each (physical,virtual)
bus the kernel reads the list of devices. After resources are read and assigned 
it will probe the devices.

If the kernel is up and running then this will start its flow through the dlpar 
add bus code.
Which then will read the device node and same operation as boot will entail of 
going
through the list of devices under the bus.

One should note that dynamically created VFs in configure SR-IOV path will have 
is_virtfn
set and others do not, therefore pcibios_bus_add_device will only be exercised 
in certain
cases.
>  How does it discover this new device?
This is treated as a normal hot plug operation for the Operating System that
the device gets assigned which can be  IBMi, AIX or Linux in this environment.
>
> I'm suggesting that it would be cool if that kernel received a hotplug
> interrupt from the Downstream Port leading to the new VF, and the
> pciehp driver could read the Slot Status register and see that
> "Presence Detect Changed" was set.  If that happened, the pciehp
> driver should automatically enumerate the new device and there
> wouldn't be much if any powerpc-specific code in the path.
To re-state Steve's earlier comment, PHYP will enumerate the virtual pci bus 
and is not
directly correlated to the real PCI bus that the PF is on. We have no control 
of the pci
bus enumeration for the device node added to the device tree.

Juan J. Alvarez

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

On 10/17/17 11:26 AM, Bjorn Helgaas wrote:
> To pop back up to the top of the stack, I think the main point of this
> patch is to keep from binding a driver to the VFs in the kernel that
> set PCI_SRIOV_CTRL_VFE.  This patch does that by setting
> pdev->match_driver to -1 in powerpc-specific code.
Correct, to add to your comment, we will only add to the PSeries platform
 (in PowerPC)  configure SR-IOV path.
> I think all systems, not just powerpc, want to prevent this VF driver
> binding, so I hope there's a generic solution that everybody can use.
The patch that we made this change dependent on:

https://patchwork.kernel.org/patch/9882915/

Is a generic form of not binding a pci device to a device driver
and can be used in another environment if needed.
>
 So like existing way of enabling SRIOV we still rely on the PF driver to
 enable VFs - but in this case the attachment phase is done via a user
 action via a management console in our case (novalink or hmc) triggered
 event that will essentially act like a hotplug.

 So in the fine details of that user triggered action the system
 firmware will bind the VFs, allowing resources to be allocated to
 the VF.  - Which essentially does all the attaching as we know it
 today but is managed by PHYP not by the kernel.
>>> What exactly does "firmware binding the VFs" mean?  I guess this must
>>> mean assigning a VF to a partition, injecting a hotplug add event to
>>> that partition, and making the VF visible in config space?
>> Firmware basically adds a device node to the device tree as defined
>> in the (PAPR) Power Architecture Platform Requirements document.
> From the point of view of the kernel that consumes this device tree
> and owns the VF, I guess this looks like a hot-add.  
Correct, this is intended. We want to minimize change and only focus
on configure SR-IOV path in the PF on PSeries platform.
> It would be nice
> if this could be exposed to that kernel by having the firmware inject
> a normal PCIe hotplug interrupt, but I'm guessing it's not that
> simple.
I don't understand entirely your suggestion. However, in the case of
interrupts PHYP owns all the resources and will be associated accordingly
with a partitionable endpoint (PE).
> But if I understand correctly, this patch series isn't concerned with
> that kernel; it's concerned with the kernel that owns the PF and sets
> PCI_SRIOV_CTRL_VFE.
Correct, we will enable SR-IOV in the PF through the Linux kernel and map 
system resources
for the new VFs in powerpc platform specific kernel code. Furthermore, not 
binding
the device drivers for the VFs that get mapped within the configure SR-IOV path
is a requirement for this PSeries SR-IOV environment.

Juan J. Alvarez

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

On 10/17/17 8:51 AM, Bjorn Helgaas wrote:
> This is where I get confused.  I guess the Linux that sets
> PCI_SRIOV_CTRL_VFE to enable the VFs can also perform config accesses
> to the VFs, since it can enumerate them and build pci_dev structs for
> them, right?

Correct, we are not changing anything related to how the VF gets initialized
in the kernel.
>
> And the Linux in the "Hosting Partition" is a guest that cannot see a
> VF until a management console attaches the VF to the Hosting
> Partition?  

Correct, this is how PHYP does normal adapter assignment.
> I'm not a VFIO or KVM expert but that sounds vaguely like
> what they would do when assigning a VF to a guest.
I do not know the specifics on VFIO and KVM. However, in this
case there is no KVM running on the Linux LPAR. PHYP owns all
the system resources.
>
>> So like existing way of enabling SRIOV we still rely on the PF driver to
>> enable VFs - but in this case the attachment phase is done via a user
>> action via a management console in our case (novalink or hmc) triggered
>> event that will essentially act like a hotplug.
>>
>> So in the fine details of that user triggered action the system
>> firmware will bind the VFs, allowing resources to be allocated to
>> the VF.  - Which essentially does all the attaching as we know it
>> today but is managed by PHYP not by the kernel.
> What exactly does "firmware binding the VFs" mean?  I guess this must
> mean assigning a VF to a partition, injecting a hotplug add event to
> that partition, and making the VF visible in config space?
Firmware basically adds a device node to the device tree as defined
in the (PAPR) Power Architecture Platform Requirements document.

Juan

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

On 10/17/17 8:51 AM, Bjorn Helgaas wrote:

> This is where I get confused.  I guess the Linux that sets
> PCI_SRIOV_CTRL_VFE to enable the VFs can also perform config accesses
> to the VFs, since it can enumerate them and build pci_dev structs for
> them, right?
Correct, we are not changing anything related to how the VF gets initialized
in the Linux kernel.
>
> And the Linux in the "Hosting Partition" is a guest that cannot see a
> VF until a management console attaches the VF to the Hosting
> Partition? 
Correct, this is how PHYP does normal adapter assignment.

>  I'm not a VFIO or KVM expert but that sounds vaguely like
> what they would do when assigning a VF to a guest.
I do not know the specifics on VFIO and KVM. However, in this
case there is no KVM running on the Linux (LPAR) logical partition.
PHYP owns all the system resources.
>
>> So like existing way of enabling SRIOV we still rely on the PF driver to
>> enable VFs - but in this case the attachment phase is done via a user
>> action via a management console in our case (novalink or hmc) triggered
>> event that will essentially act like a hotplug.
>>
>> So in the fine details of that user triggered action the system
>> firmware will bind the VFs, allowing resources to be allocated to
>> the VF.  - Which essentially does all the attaching as we know it
>> today but is managed by PHYP not by the kernel.
> What exactly does "firmware binding the VFs" mean?  I guess this must
> mean assigning a VF to a partition, injecting a hotplug add event to
> that partition, and making the VF visible in config space?
Firmware basically adds a device node to the device tree as defined
in the (PAPR) Power Architecture Platform Requirements document.

Juan J. Alvarez

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

On Tue, Oct 17, 2017 at 12:23:01PM -0500, Juan Alvarez wrote:
> On 10/17/17 11:26 AM, Bjorn Helgaas wrote:
> > To pop back up to the top of the stack, I think the main point of this
> > patch is to keep from binding a driver to the VFs in the kernel that
> > set PCI_SRIOV_CTRL_VFE.  This patch does that by setting
> > pdev->match_driver to -1 in powerpc-specific code.
> Correct, to add to your comment, we will only add to the PSeries platform
>  (in PowerPC)  configure SR-IOV path.
> > I think all systems, not just powerpc, want to prevent this VF driver
> > binding, so I hope there's a generic solution that everybody can use.
> The patch that we made this change dependent on:
> 
> https://patchwork.kernel.org/patch/9882915/
> 
> Is a generic form of not binding a pci device to a device driver
> and can be used in another environment if needed.

Right.  But that patch isn't really on the path to inclusion (mainly
because it's test framework and doesn't fix a bug or add support for
new hardware or features).

I'm suggesting that maybe there should be a generic way to prevent
binding to VF devices that works for everybody and doesn't require any
arch-specific code at all.

>  So like existing way of enabling SRIOV we still rely on the PF driver to
>  enable VFs - but in this case the attachment phase is done via a user
>  action via a management console in our case (novalink or hmc) triggered
>  event that will essentially act like a hotplug.
> 
>  So in the fine details of that user triggered action the system
>  firmware will bind the VFs, allowing resources to be allocated to
>  the VF.  - Which essentially does all the attaching as we know it
>  today but is managed by PHYP not by the kernel.
> >>> What exactly does "firmware binding the VFs" mean?  I guess this must
> >>> mean assigning a VF to a partition, injecting a hotplug add event to
> >>> that partition, and making the VF visible in config space?
> >> Firmware basically adds a device node to the device tree as defined
> >> in the (PAPR) Power Architecture Platform Requirements document.
> > From the point of view of the kernel that consumes this device tree
> > and owns the VF, I guess this looks like a hot-add.  
> Correct, this is intended. We want to minimize change and only focus
> on configure SR-IOV path in the PF on PSeries platform.
> > It would be nice
> > if this could be exposed to that kernel by having the firmware inject
> > a normal PCIe hotplug interrupt, but I'm guessing it's not that
> > simple.
>
> I don't understand entirely your suggestion. However, in the case of
> interrupts PHYP owns all the resources and will be associated accordingly
> with a partitionable endpoint (PE).

Assume you have a Linux kernel, and PHYP assigns a VF to it.  What
happens in that Linux kernel?  How does it discover this new device?

I'm suggesting that it would be cool if that kernel received a hotplug
interrupt from the Downstream Port leading to the new VF, and the
pciehp driver could read the Slot Status register and see that
"Presence Detect Changed" was set.  If that happened, the pciehp
driver should automatically enumerate the new device and there
wouldn't be much if any powerpc-specific code in the path.

Bjorn

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

On Tue, Oct 17, 2017 at 09:33:34AM -0500, Juan Alvarez wrote:
> On 10/17/17 8:51 AM, Bjorn Helgaas wrote:
> > This is where I get confused.  I guess the Linux that sets
> > PCI_SRIOV_CTRL_VFE to enable the VFs can also perform config accesses
> > to the VFs, since it can enumerate them and build pci_dev structs for
> > them, right?
> 
> Correct, we are not changing anything related to how the VF gets initialized
> in the kernel.
> >
> > And the Linux in the "Hosting Partition" is a guest that cannot see a
> > VF until a management console attaches the VF to the Hosting
> > Partition?  
> 
> Correct, this is how PHYP does normal adapter assignment.
> 
> > I'm not a VFIO or KVM expert but that sounds vaguely like
> > what they would do when assigning a VF to a guest.
>
> I do not know the specifics on VFIO and KVM. However, in this
> case there is no KVM running on the Linux LPAR. PHYP owns all
> the system resources.

To pop back up to the top of the stack, I think the main point of this
patch is to keep from binding a driver to the VFs in the kernel that
set PCI_SRIOV_CTRL_VFE.  This patch does that by setting
pdev->match_driver to -1 in powerpc-specific code.

I think all systems, not just powerpc, want to prevent this VF driver
binding, so I hope there's a generic solution that everybody can use.

> >> So like existing way of enabling SRIOV we still rely on the PF driver to
> >> enable VFs - but in this case the attachment phase is done via a user
> >> action via a management console in our case (novalink or hmc) triggered
> >> event that will essentially act like a hotplug.
> >>
> >> So in the fine details of that user triggered action the system
> >> firmware will bind the VFs, allowing resources to be allocated to
> >> the VF.  - Which essentially does all the attaching as we know it
> >> today but is managed by PHYP not by the kernel.
> >
> > What exactly does "firmware binding the VFs" mean?  I guess this must
> > mean assigning a VF to a partition, injecting a hotplug add event to
> > that partition, and making the VF visible in config space?
>
> Firmware basically adds a device node to the device tree as defined
> in the (PAPR) Power Architecture Platform Requirements document.

>From the point of view of the kernel that consumes this device tree
and owns the VF, I guess this looks like a hot-add.  It would be nice
if this could be exposed to that kernel by having the firmware inject
a normal PCIe hotplug interrupt, but I'm guessing it's not that
simple.

But if I understand correctly, this patch series isn't concerned with
that kernel; it's concerned with the kernel that owns the PF and sets
PCI_SRIOV_CTRL_VFE.

Bjorn

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bryant G. Ly]

Adding Juan back into the cc: jjalv...@linux.vnet.ibm.com


On 10/16/17 10:38 PM, Michael Ellerman wrote:
> "Bryant G. Ly"  writes:
>> On 10/12/17 1:29 PM, Bjorn Helgaas wrote:
> ...
>>> If that's the case, how to you ever bind a driver to these VFs?  The
>>> changelog says you don't want VF drivers to load *immediately*, so I
>>> assume you do want them to load eventually.
>>>
>> The VF's that get dynamically created within the configure SR-IOV call, 
>> on the Pseries Platform, wont be matched with a driver. - We do not
>> want it to match.
>>
>> The Power Hypervisor will load the VFs. The VF's will get assigned(by 
>> the user) > via the HMC or Novalink in this environment which will
>> then trigger PHYP to load the VF device node to the device tree.
> What about the other "Power Hypervisor"? ie. KVM running on Power.
This path is only exercised when configuring SR-IOV for Pseries LPAR,
therefore it will not affect PowerNV or KVM(opal). Which is the reason for
the separation of calls to the machine dependent stuff.
> We also use the pseries platform when running under KVM.
>
> cheers
>
If anyone plans to enable SR-IOV on Pseries platform, firmware must provide the
resources to enable the VFs and map them with system resources. A new version
of the PAPR Document will be added to document these system resources. Lastly,
we were not aware that there is an intention to enable SR-IOV in adapters 
assigned
to a VM with Pseries running on KVM. Furthermore, this could be left as a todo
for the future if this type of configuration is needed.

-Bryant

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

On Fri, Oct 13, 2017 at 02:12:32PM -0500, Bryant G. Ly wrote:
> 
> 
> On 10/13/17 1:05 PM, Alex Williamson wrote:
> >On Fri, 13 Oct 2017 07:01:48 -0500
> >Steven Royer  wrote:
> >
> >>On 2017-10-13 06:53, Steven Royer wrote:
> >>>On 2017-10-12 22:34, Bjorn Helgaas wrote:
> [+cc Alex, Bodong, Eli, Saeed]
> 
> On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote:
> >On 10/12/17 1:29 PM, Bjorn Helgaas wrote:
> >>On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote:
> >>>Bjorn Helgaas  writes:
> On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote:
> >>reading the code what -1/0/1 mean.

> >>Apparently here you *do* want the "-1 means the PCI core will never
> >>set match_driver to 1" functionality, so maybe you do depend on it.
> >We depend on the patch because we want that ability to never set
> >match_driver,
> >for SRIOV on PowerVM.
> Is this really new PowerVM-specific functionality?  ISTR recent
> discussions
> about inhibiting driver binding in a generic way, e.g.,
> http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bod...@mellanox.com
> >>If that's the case, how to you ever bind a driver to these VFs?  The
> >>changelog says you don't want VF drivers to load *immediately*, so I
> >>assume you do want them to load eventually.
> >The VF's that get dynamically created within the configure SR-IOV
> >call, on the Pseries Platform, wont be matched with a driver. - We
> >do not want it to match.
> >
> >The Power Hypervisor will load the VFs. The VF's will get
> >assigned(by the user) via the HMC or Novalink in this environment
> >which will then trigger PHYP to load the VF device node to the
> >device tree.
> I don't know what it means for the Hypervisor to "load the VFs."  Can
> you explain that in PCI-speak?
> 
> The things I know about are:
> 
>    - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs
>    - now the VFs respond to config accesses
>    - the PCI core enumerates the VFs by reading their config space
>    - the PCI core builds pci_dev structs for the VFs
>    - the PCI core adds these pci_devs to the bus
>    - we try to bind drivers to the VFs
>    - the VF driver probe function may read VF config space and VF BARs
>    - the VF may be assigned to a guest VM
> 
> Where does "loading the VFs" fit in?  I don't know what HMC, Novalink,
> or PHYP are.  I don't *need* to know what they are, as long as you can
> explain what's happening in terms of the PCI concepts and generic
> Linux VMs
> and device assignment.
> 
> Bjorn
> >>>The VFs will be hotplugged into the VM separately from the enable
> >>>SR-IOV, so the driver will load as part of the hotplug operation.
> >>>
> >>>Steve
> >>One more point of clarification: when the hotplug happens, the VF will
> >>show up on a virtual PCI bus that is not directly correlated to the real
> >>PCI bus that the PF is on.  On that virtual PCI bus, the driver will
> >>match because it won't be set to -1.
> So lets refer to Bjorn's list of things for SRIOV.
> 
>   - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs
>   - now the VFs respond to config accesses
>   - the PCI core enumerates the VFs by reading their config space
>   - the PCI core builds pci_dev structs for the VFs
>   - the PCI core adds these pci_devs to the bus
> 
> So everything is the same up to here.
>   - we try to bind drivers to the VFs
>   - the VF driver probe function may read VF config space and VF BARs
>   - the VF may be assigned to a guest VM
> 
> PowerVM environment is very different than traditional KVM in terms
> of SRIOV.  In our environment the VFs are not usable or view-able by
> the Hosting Partition in this case Linux. This is a very important
> point in that the Host CAN NOT do anything to any of the VFs
> available.

This is where I get confused.  I guess the Linux that sets
PCI_SRIOV_CTRL_VFE to enable the VFs can also perform config accesses
to the VFs, since it can enumerate them and build pci_dev structs for
them, right?

And the Linux in the "Hosting Partition" is a guest that cannot see a
VF until a management console attaches the VF to the Hosting
Partition?  I'm not a VFIO or KVM expert but that sounds vaguely like
what they would do when assigning a VF to a guest.

> So like existing way of enabling SRIOV we still rely on the PF driver to
> enable VFs - but in this case the attachment phase is done via a user
> action via a management console in our case (novalink or hmc) triggered
> event that will essentially act like a hotplug.
> 
> So in the fine details of that user triggered action the system
> firmware will bind the VFs, allowing resources to be allocated to
> the VF.  - Which essentially does all the attaching as we know it
> today but is managed by PHYP not by the 

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Michael Ellerman]

"Bryant G. Ly"  writes:
> On 10/12/17 1:29 PM, Bjorn Helgaas wrote:
...
>>
>> If that's the case, how to you ever bind a driver to these VFs?  The
>> changelog says you don't want VF drivers to load *immediately*, so I
>> assume you do want them to load eventually.
>>
> The VF's that get dynamically created within the configure SR-IOV call, 
> on the Pseries Platform, wont be matched with a driver. - We do not
> want it to match.
>
> The Power Hypervisor will load the VFs. The VF's will get assigned(by 
> the user) > via the HMC or Novalink in this environment which will
> then trigger PHYP to load the VF device node to the device tree.

What about the other "Power Hypervisor"? ie. KVM running on Power.

We also use the pseries platform when running under KVM.

cheers

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bryant G. Ly]

On 10/13/17 1:05 PM, Alex Williamson wrote:

On Fri, 13 Oct 2017 07:01:48 -0500
Steven Royer  wrote:


On 2017-10-13 06:53, Steven Royer wrote:

On 2017-10-12 22:34, Bjorn Helgaas wrote:

[+cc Alex, Bodong, Eli, Saeed]

On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote:

On 10/12/17 1:29 PM, Bjorn Helgaas wrote:

On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote:

Bjorn Helgaas  writes:
  

On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote:

This patch adds the machine dependent call for
pcibios_bus_add_device, since the previous patch
separated the calls out between the PowerNV and PowerVM.

The difference here is that for the PowerVM environment
we do not want match_driver set because in this environment
we do not want the VF device drivers to load immediately, due to
firmware loading the device node when VF device is assigned to the
logical partition.

This patch will depend on the patch linked below, which is under
review.

https://patchwork.kernel.org/patch/9882915/

Signed-off-by: Bryant G. Ly 
Signed-off-by: Juan J. Alvarez 
---
  arch/powerpc/platforms/pseries/eeh_pseries.c | 24 
  1 file changed, 24 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c 
b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 6b812ad990e4..45946ee90985 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX];
  static DEFINE_SPINLOCK(slot_errbuf_lock);
  static int eeh_error_buf_size;
+void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
+{
+   struct pci_dn *pdn = pci_get_pdn(pdev);
+
+   if (!pdev->is_virtfn)
+   return;
+
+   pdn->device_id  =  pdev->device;
+   pdn->vendor_id  =  pdev->vendor;
+   pdn->class_code =  pdev->class;
+
+   /*
+* The following operations will fail if VF's sysfs files
+* aren't created or its resources aren't finalized.
+*/
+   eeh_add_device_early(pdn);
+   eeh_add_device_late(pdev);
+   eeh_sysfs_add_device(pdev);
+   pdev->match_driver = -1;

match_driver is a bool, which should be assigned "true" or "false".

Above he mentioned a dependency on:

   [04/10] PCI: extend pci device match_driver state
   https://patchwork.kernel.org/patch/9882915/


Which makes it an int.

Oh, right, I missed that, thanks.
  

Or has that patch been rejected or something?

I haven't *rejected* it, but it's low on my priority list, so you
shouldn't depend on it unless it adds functionality you really need.
If I did apply that particular patch, I would want some rework because
it currently obfuscates the match_driver logic.  There's no clue when
reading the code what -1/0/1 mean.

So do you prefer enum's? - If so I can make a change for that.

Apparently here you *do* want the "-1 means the PCI core will never
set match_driver to 1" functionality, so maybe you do depend on it.

We depend on the patch because we want that ability to never set
match_driver,
for SRIOV on PowerVM.

Is this really new PowerVM-specific functionality?  ISTR recent
discussions
about inhibiting driver binding in a generic way, e.g.,
http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bod...@mellanox.com
   

If that's the case, how to you ever bind a driver to these VFs?  The
changelog says you don't want VF drivers to load *immediately*, so I
assume you do want them to load eventually.
  

The VF's that get dynamically created within the configure SR-IOV
call, on the Pseries Platform, wont be matched with a driver. - We
do not want it to match.

The Power Hypervisor will load the VFs. The VF's will get
assigned(by the user) via the HMC or Novalink in this environment
which will then trigger PHYP to load the VF device node to the
device tree.

I don't know what it means for the Hypervisor to "load the VFs."  Can
you explain that in PCI-speak?

The things I know about are:

   - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs
   - now the VFs respond to config accesses
   - the PCI core enumerates the VFs by reading their config space
   - the PCI core builds pci_dev structs for the VFs
   - the PCI core adds these pci_devs to the bus
   - we try to bind drivers to the VFs
   - the VF driver probe function may read VF config space and VF BARs
   - the VF may be assigned to a guest VM

Where does "loading the VFs" fit in?  I don't know what HMC, Novalink,
or PHYP are.  I don't *need* to know what they are, as long as you can
explain what's happening in terms of the PCI concepts and generic
Linux VMs
and device assignment.

Bjorn

The VFs will be hotplugged into the VM separately from the enable
SR-IOV, so the driver will load as part of the hotplug operation.

Steve

One more point of clarification: when the hotplug happens, the VF will

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Alex Williamson]

On Fri, 13 Oct 2017 07:01:48 -0500
Steven Royer  wrote:

> On 2017-10-13 06:53, Steven Royer wrote:
> > On 2017-10-12 22:34, Bjorn Helgaas wrote:  
> >> [+cc Alex, Bodong, Eli, Saeed]
> >> 
> >> On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote:  
> >>> On 10/12/17 1:29 PM, Bjorn Helgaas wrote:  
> >>> >On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote:  
> >>> >>Bjorn Helgaas  writes:
> >>> >>  
> >>> >>>On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote:  
> >>> This patch adds the machine dependent call for
> >>> pcibios_bus_add_device, since the previous patch
> >>> separated the calls out between the PowerNV and PowerVM.
> >>> 
> >>> The difference here is that for the PowerVM environment
> >>> we do not want match_driver set because in this environment
> >>> we do not want the VF device drivers to load immediately, due to
> >>> firmware loading the device node when VF device is assigned to the
> >>> logical partition.
> >>> 
> >>> This patch will depend on the patch linked below, which is under
> >>> review.
> >>> 
> >>> https://patchwork.kernel.org/patch/9882915/
> >>> 
> >>> Signed-off-by: Bryant G. Ly 
> >>> Signed-off-by: Juan J. Alvarez 
> >>> ---
> >>>   arch/powerpc/platforms/pseries/eeh_pseries.c | 24 
> >>>  
> >>>   1 file changed, 24 insertions(+)
> >>> 
> >>> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c 
> >>> b/arch/powerpc/platforms/pseries/eeh_pseries.c
> >>> index 6b812ad990e4..45946ee90985 100644
> >>> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> >>> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> >>> @@ -64,6 +64,27 @@ static unsigned char 
> >>> slot_errbuf[RTAS_ERROR_LOG_MAX];
> >>>   static DEFINE_SPINLOCK(slot_errbuf_lock);
> >>>   static int eeh_error_buf_size;
> >>> +void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
> >>> +{
> >>> + struct pci_dn *pdn = pci_get_pdn(pdev);
> >>> +
> >>> + if (!pdev->is_virtfn)
> >>> + return;
> >>> +
> >>> + pdn->device_id  =  pdev->device;
> >>> + pdn->vendor_id  =  pdev->vendor;
> >>> + pdn->class_code =  pdev->class;
> >>> +
> >>> + /*
> >>> +  * The following operations will fail if VF's sysfs files
> >>> +  * aren't created or its resources aren't finalized.
> >>> +  */
> >>> + eeh_add_device_early(pdn);
> >>> + eeh_add_device_late(pdev);
> >>> + eeh_sysfs_add_device(pdev);
> >>> + pdev->match_driver = -1;  
> >>> >>>match_driver is a bool, which should be assigned "true" or "false".  
> >>> >>Above he mentioned a dependency on:
> >>> >>
> >>> >>   [04/10] PCI: extend pci device match_driver state
> >>> >>   https://patchwork.kernel.org/patch/9882915/
> >>> >>
> >>> >>
> >>> >>Which makes it an int.  
> >>> >Oh, right, I missed that, thanks.
> >>> >  
> >>> >>Or has that patch been rejected or something?  
> >>> >I haven't *rejected* it, but it's low on my priority list, so you
> >>> >shouldn't depend on it unless it adds functionality you really need.
> >>> >If I did apply that particular patch, I would want some rework because
> >>> >it currently obfuscates the match_driver logic.  There's no clue when
> >>> >reading the code what -1/0/1 mean.  
> >>> So do you prefer enum's? - If so I can make a change for that.  
> >>> >Apparently here you *do* want the "-1 means the PCI core will never
> >>> >set match_driver to 1" functionality, so maybe you do depend on it.  
> >>> We depend on the patch because we want that ability to never set
> >>> match_driver,
> >>> for SRIOV on PowerVM.  
> >> 
> >> Is this really new PowerVM-specific functionality?  ISTR recent 
> >> discussions
> >> about inhibiting driver binding in a generic way, e.g.,
> >> http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bod...@mellanox.com
> >>   
> >>> >If that's the case, how to you ever bind a driver to these VFs?  The
> >>> >changelog says you don't want VF drivers to load *immediately*, so I
> >>> >assume you do want them to load eventually.
> >>> >  
> >>> The VF's that get dynamically created within the configure SR-IOV
> >>> call, on the Pseries Platform, wont be matched with a driver. - We
> >>> do not want it to match.
> >>> 
> >>> The Power Hypervisor will load the VFs. The VF's will get
> >>> assigned(by the user) via the HMC or Novalink in this environment
> >>> which will then trigger PHYP to load the VF device node to the
> >>> device tree.  
> >> 
> >> I don't know what it means for the Hypervisor to "load the VFs."  Can
> >> you explain that in PCI-speak?
> >> 
> >> The things I know about are:
> >> 
> >>   - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs
> >>   - now the VFs respond to 

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Steven Royer]

On 2017-10-13 06:53, Steven Royer wrote:

On 2017-10-12 22:34, Bjorn Helgaas wrote:

[+cc Alex, Bodong, Eli, Saeed]

On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote:

On 10/12/17 1:29 PM, Bjorn Helgaas wrote:
>On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote:
>>Bjorn Helgaas  writes:
>>
>>>On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote:
This patch adds the machine dependent call for
pcibios_bus_add_device, since the previous patch
separated the calls out between the PowerNV and PowerVM.

The difference here is that for the PowerVM environment
we do not want match_driver set because in this environment
we do not want the VF device drivers to load immediately, due to
firmware loading the device node when VF device is assigned to the
logical partition.

This patch will depend on the patch linked below, which is under
review.

https://patchwork.kernel.org/patch/9882915/

Signed-off-by: Bryant G. Ly 
Signed-off-by: Juan J. Alvarez 
---
  arch/powerpc/platforms/pseries/eeh_pseries.c | 24 
  1 file changed, 24 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c 
b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 6b812ad990e4..45946ee90985 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX];
  static DEFINE_SPINLOCK(slot_errbuf_lock);
  static int eeh_error_buf_size;
+void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
+{
+   struct pci_dn *pdn = pci_get_pdn(pdev);
+
+   if (!pdev->is_virtfn)
+   return;
+
+   pdn->device_id  =  pdev->device;
+   pdn->vendor_id  =  pdev->vendor;
+   pdn->class_code =  pdev->class;
+
+   /*
+* The following operations will fail if VF's sysfs files
+* aren't created or its resources aren't finalized.
+*/
+   eeh_add_device_early(pdn);
+   eeh_add_device_late(pdev);
+   eeh_sysfs_add_device(pdev);
+   pdev->match_driver = -1;
>>>match_driver is a bool, which should be assigned "true" or "false".
>>Above he mentioned a dependency on:
>>
>>   [04/10] PCI: extend pci device match_driver state
>>   https://patchwork.kernel.org/patch/9882915/
>>
>>
>>Which makes it an int.
>Oh, right, I missed that, thanks.
>
>>Or has that patch been rejected or something?
>I haven't *rejected* it, but it's low on my priority list, so you
>shouldn't depend on it unless it adds functionality you really need.
>If I did apply that particular patch, I would want some rework because
>it currently obfuscates the match_driver logic.  There's no clue when
>reading the code what -1/0/1 mean.
So do you prefer enum's? - If so I can make a change for that.
>Apparently here you *do* want the "-1 means the PCI core will never
>set match_driver to 1" functionality, so maybe you do depend on it.
We depend on the patch because we want that ability to never set
match_driver,
for SRIOV on PowerVM.


Is this really new PowerVM-specific functionality?  ISTR recent 
discussions

about inhibiting driver binding in a generic way, e.g.,
http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bod...@mellanox.com


>If that's the case, how to you ever bind a driver to these VFs?  The
>changelog says you don't want VF drivers to load *immediately*, so I
>assume you do want them to load eventually.
>
The VF's that get dynamically created within the configure SR-IOV
call, on the Pseries Platform, wont be matched with a driver. - We
do not want it to match.

The Power Hypervisor will load the VFs. The VF's will get
assigned(by the user) via the HMC or Novalink in this environment
which will then trigger PHYP to load the VF device node to the
device tree.


I don't know what it means for the Hypervisor to "load the VFs."  Can
you explain that in PCI-speak?

The things I know about are:

  - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs
  - now the VFs respond to config accesses
  - the PCI core enumerates the VFs by reading their config space
  - the PCI core builds pci_dev structs for the VFs
  - the PCI core adds these pci_devs to the bus
  - we try to bind drivers to the VFs
  - the VF driver probe function may read VF config space and VF BARs
  - the VF may be assigned to a guest VM

Where does "loading the VFs" fit in?  I don't know what HMC, Novalink,
or PHYP are.  I don't *need* to know what they are, as long as you can
explain what's happening in terms of the PCI concepts and generic 
Linux VMs

and device assignment.

Bjorn


The VFs will be hotplugged into the VM separately from the enable
SR-IOV, so the driver will load as part of the hotplug operation.

Steve

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Steven Royer]

On 2017-10-12 22:34, Bjorn Helgaas wrote:

[+cc Alex, Bodong, Eli, Saeed]

On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote:

On 10/12/17 1:29 PM, Bjorn Helgaas wrote:
>On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote:
>>Bjorn Helgaas  writes:
>>
>>>On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote:
This patch adds the machine dependent call for
pcibios_bus_add_device, since the previous patch
separated the calls out between the PowerNV and PowerVM.

The difference here is that for the PowerVM environment
we do not want match_driver set because in this environment
we do not want the VF device drivers to load immediately, due to
firmware loading the device node when VF device is assigned to the
logical partition.

This patch will depend on the patch linked below, which is under
review.

https://patchwork.kernel.org/patch/9882915/

Signed-off-by: Bryant G. Ly 
Signed-off-by: Juan J. Alvarez 
---
  arch/powerpc/platforms/pseries/eeh_pseries.c | 24 
  1 file changed, 24 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c 
b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 6b812ad990e4..45946ee90985 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX];
  static DEFINE_SPINLOCK(slot_errbuf_lock);
  static int eeh_error_buf_size;
+void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
+{
+   struct pci_dn *pdn = pci_get_pdn(pdev);
+
+   if (!pdev->is_virtfn)
+   return;
+
+   pdn->device_id  =  pdev->device;
+   pdn->vendor_id  =  pdev->vendor;
+   pdn->class_code =  pdev->class;
+
+   /*
+* The following operations will fail if VF's sysfs files
+* aren't created or its resources aren't finalized.
+*/
+   eeh_add_device_early(pdn);
+   eeh_add_device_late(pdev);
+   eeh_sysfs_add_device(pdev);
+   pdev->match_driver = -1;
>>>match_driver is a bool, which should be assigned "true" or "false".
>>Above he mentioned a dependency on:
>>
>>   [04/10] PCI: extend pci device match_driver state
>>   https://patchwork.kernel.org/patch/9882915/
>>
>>
>>Which makes it an int.
>Oh, right, I missed that, thanks.
>
>>Or has that patch been rejected or something?
>I haven't *rejected* it, but it's low on my priority list, so you
>shouldn't depend on it unless it adds functionality you really need.
>If I did apply that particular patch, I would want some rework because
>it currently obfuscates the match_driver logic.  There's no clue when
>reading the code what -1/0/1 mean.
So do you prefer enum's? - If so I can make a change for that.
>Apparently here you *do* want the "-1 means the PCI core will never
>set match_driver to 1" functionality, so maybe you do depend on it.
We depend on the patch because we want that ability to never set
match_driver,
for SRIOV on PowerVM.


Is this really new PowerVM-specific functionality?  ISTR recent 
discussions

about inhibiting driver binding in a generic way, e.g.,
http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bod...@mellanox.com


>If that's the case, how to you ever bind a driver to these VFs?  The
>changelog says you don't want VF drivers to load *immediately*, so I
>assume you do want them to load eventually.
>
The VF's that get dynamically created within the configure SR-IOV
call, on the Pseries Platform, wont be matched with a driver. - We
do not want it to match.

The Power Hypervisor will load the VFs. The VF's will get
assigned(by the user) via the HMC or Novalink in this environment
which will then trigger PHYP to load the VF device node to the
device tree.


I don't know what it means for the Hypervisor to "load the VFs."  Can
you explain that in PCI-speak?

The things I know about are:

  - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs
  - now the VFs respond to config accesses
  - the PCI core enumerates the VFs by reading their config space
  - the PCI core builds pci_dev structs for the VFs
  - the PCI core adds these pci_devs to the bus
  - we try to bind drivers to the VFs
  - the VF driver probe function may read VF config space and VF BARs
  - the VF may be assigned to a guest VM

Where does "loading the VFs" fit in?  I don't know what HMC, Novalink,
or PHYP are.  I don't *need* to know what they are, as long as you can
explain what's happening in terms of the PCI concepts and generic Linux 
VMs

and device assignment.

Bjorn


The VFs will be hotplugged into the VM separately from the enable 
SR-IOV, so the driver will load as part of the hotplug operation.


Steve

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

[+cc Alex, Bodong, Eli, Saeed]

On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote:
> On 10/12/17 1:29 PM, Bjorn Helgaas wrote:
> >On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote:
> >>Bjorn Helgaas  writes:
> >>
> >>>On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote:
> This patch adds the machine dependent call for
> pcibios_bus_add_device, since the previous patch
> separated the calls out between the PowerNV and PowerVM.
> 
> The difference here is that for the PowerVM environment
> we do not want match_driver set because in this environment
> we do not want the VF device drivers to load immediately, due to
> firmware loading the device node when VF device is assigned to the
> logical partition.
> 
> This patch will depend on the patch linked below, which is under
> review.
> 
> https://patchwork.kernel.org/patch/9882915/
> 
> Signed-off-by: Bryant G. Ly 
> Signed-off-by: Juan J. Alvarez 
> ---
>   arch/powerpc/platforms/pseries/eeh_pseries.c | 24 
>  
>   1 file changed, 24 insertions(+)
> 
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c 
> b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 6b812ad990e4..45946ee90985 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX];
>   static DEFINE_SPINLOCK(slot_errbuf_lock);
>   static int eeh_error_buf_size;
> +void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
> +{
> + struct pci_dn *pdn = pci_get_pdn(pdev);
> +
> + if (!pdev->is_virtfn)
> + return;
> +
> + pdn->device_id  =  pdev->device;
> + pdn->vendor_id  =  pdev->vendor;
> + pdn->class_code =  pdev->class;
> +
> + /*
> +  * The following operations will fail if VF's sysfs files
> +  * aren't created or its resources aren't finalized.
> +  */
> + eeh_add_device_early(pdn);
> + eeh_add_device_late(pdev);
> + eeh_sysfs_add_device(pdev);
> + pdev->match_driver = -1;
> >>>match_driver is a bool, which should be assigned "true" or "false".
> >>Above he mentioned a dependency on:
> >>
> >>   [04/10] PCI: extend pci device match_driver state
> >>   https://patchwork.kernel.org/patch/9882915/
> >>
> >>
> >>Which makes it an int.
> >Oh, right, I missed that, thanks.
> >
> >>Or has that patch been rejected or something?
> >I haven't *rejected* it, but it's low on my priority list, so you
> >shouldn't depend on it unless it adds functionality you really need.
> >If I did apply that particular patch, I would want some rework because
> >it currently obfuscates the match_driver logic.  There's no clue when
> >reading the code what -1/0/1 mean.
> So do you prefer enum's? - If so I can make a change for that.
> >Apparently here you *do* want the "-1 means the PCI core will never
> >set match_driver to 1" functionality, so maybe you do depend on it.
> We depend on the patch because we want that ability to never set
> match_driver,
> for SRIOV on PowerVM.

Is this really new PowerVM-specific functionality?  ISTR recent discussions
about inhibiting driver binding in a generic way, e.g.,
http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bod...@mellanox.com

> >If that's the case, how to you ever bind a driver to these VFs?  The
> >changelog says you don't want VF drivers to load *immediately*, so I
> >assume you do want them to load eventually.
> >
> The VF's that get dynamically created within the configure SR-IOV
> call, on the Pseries Platform, wont be matched with a driver. - We
> do not want it to match.
> 
> The Power Hypervisor will load the VFs. The VF's will get
> assigned(by the user) via the HMC or Novalink in this environment
> which will then trigger PHYP to load the VF device node to the
> device tree.

I don't know what it means for the Hypervisor to "load the VFs."  Can
you explain that in PCI-speak?

The things I know about are:

  - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs
  - now the VFs respond to config accesses
  - the PCI core enumerates the VFs by reading their config space
  - the PCI core builds pci_dev structs for the VFs
  - the PCI core adds these pci_devs to the bus
  - we try to bind drivers to the VFs
  - the VF driver probe function may read VF config space and VF BARs
  - the VF may be assigned to a guest VM

Where does "loading the VFs" fit in?  I don't know what HMC, Novalink,
or PHYP are.  I don't *need* to know what they are, as long as you can
explain what's happening in terms of the PCI concepts and generic Linux VMs
and device assignment.

Bjorn

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bryant G. Ly]

On 10/12/17 1:29 PM, Bjorn Helgaas wrote:

On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote:

Bjorn Helgaas  writes:


On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote:

This patch adds the machine dependent call for
pcibios_bus_add_device, since the previous patch
separated the calls out between the PowerNV and PowerVM.

The difference here is that for the PowerVM environment
we do not want match_driver set because in this environment
we do not want the VF device drivers to load immediately, due to
firmware loading the device node when VF device is assigned to the
logical partition.

This patch will depend on the patch linked below, which is under
review.

https://patchwork.kernel.org/patch/9882915/

Signed-off-by: Bryant G. Ly 
Signed-off-by: Juan J. Alvarez 
---
  arch/powerpc/platforms/pseries/eeh_pseries.c | 24 
  1 file changed, 24 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c 
b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 6b812ad990e4..45946ee90985 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX];
  static DEFINE_SPINLOCK(slot_errbuf_lock);
  static int eeh_error_buf_size;
  
+void pseries_pcibios_bus_add_device(struct pci_dev *pdev)

+{
+   struct pci_dn *pdn = pci_get_pdn(pdev);
+
+   if (!pdev->is_virtfn)
+   return;
+
+   pdn->device_id  =  pdev->device;
+   pdn->vendor_id  =  pdev->vendor;
+   pdn->class_code =  pdev->class;
+
+   /*
+* The following operations will fail if VF's sysfs files
+* aren't created or its resources aren't finalized.
+*/
+   eeh_add_device_early(pdn);
+   eeh_add_device_late(pdev);
+   eeh_sysfs_add_device(pdev);
+   pdev->match_driver = -1;

match_driver is a bool, which should be assigned "true" or "false".

Above he mentioned a dependency on:

   [04/10] PCI: extend pci device match_driver state
   https://patchwork.kernel.org/patch/9882915/


Which makes it an int.

Oh, right, I missed that, thanks.


Or has that patch been rejected or something?

I haven't *rejected* it, but it's low on my priority list, so you
shouldn't depend on it unless it adds functionality you really need.
If I did apply that particular patch, I would want some rework because
it currently obfuscates the match_driver logic.  There's no clue when
reading the code what -1/0/1 mean.

So do you prefer enum's? - If so I can make a change for that.

Apparently here you *do* want the "-1 means the PCI core will never
set match_driver to 1" functionality, so maybe you do depend on it.
We depend on the patch because we want that ability to never set 
match_driver,

for SRIOV on PowerVM.


If that's the case, how to you ever bind a driver to these VFs?  The
changelog says you don't want VF drivers to load *immediately*, so I
assume you do want them to load eventually.

The VF's that get dynamically created within the configure SR-IOV call, 
on the
Pseries Platform, wont be matched with a driver. - We do not want it to 
match.


The Power Hypervisor will load the VFs. The VF's will get assigned(by 
the user)

via the HMC or Novalink in this environment which will then trigger PHYP
to load the VF device node to the device tree.

-Bryant

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote:
> Bjorn Helgaas  writes:
> 
> > On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote:
> >> This patch adds the machine dependent call for
> >> pcibios_bus_add_device, since the previous patch
> >> separated the calls out between the PowerNV and PowerVM.
> >> 
> >> The difference here is that for the PowerVM environment
> >> we do not want match_driver set because in this environment
> >> we do not want the VF device drivers to load immediately, due to
> >> firmware loading the device node when VF device is assigned to the
> >> logical partition.
> >> 
> >> This patch will depend on the patch linked below, which is under
> >> review.
> >> 
> >> https://patchwork.kernel.org/patch/9882915/
> >> 
> >> Signed-off-by: Bryant G. Ly 
> >> Signed-off-by: Juan J. Alvarez 
> >> ---
> >>  arch/powerpc/platforms/pseries/eeh_pseries.c | 24 
> >>  1 file changed, 24 insertions(+)
> >> 
> >> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c 
> >> b/arch/powerpc/platforms/pseries/eeh_pseries.c
> >> index 6b812ad990e4..45946ee90985 100644
> >> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> >> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> >> @@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX];
> >>  static DEFINE_SPINLOCK(slot_errbuf_lock);
> >>  static int eeh_error_buf_size;
> >>  
> >> +void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
> >> +{
> >> +  struct pci_dn *pdn = pci_get_pdn(pdev);
> >> +
> >> +  if (!pdev->is_virtfn)
> >> +  return;
> >> +
> >> +  pdn->device_id  =  pdev->device;
> >> +  pdn->vendor_id  =  pdev->vendor;
> >> +  pdn->class_code =  pdev->class;
> >> +
> >> +  /*
> >> +   * The following operations will fail if VF's sysfs files
> >> +   * aren't created or its resources aren't finalized.
> >> +   */
> >> +  eeh_add_device_early(pdn);
> >> +  eeh_add_device_late(pdev);
> >> +  eeh_sysfs_add_device(pdev);
> >> +  pdev->match_driver = -1;
> >
> > match_driver is a bool, which should be assigned "true" or "false".
> 
> Above he mentioned a dependency on:
> 
>   [04/10] PCI: extend pci device match_driver state
>   https://patchwork.kernel.org/patch/9882915/
> 
> 
> Which makes it an int.

Oh, right, I missed that, thanks.

> Or has that patch been rejected or something?

I haven't *rejected* it, but it's low on my priority list, so you
shouldn't depend on it unless it adds functionality you really need.
If I did apply that particular patch, I would want some rework because
it currently obfuscates the match_driver logic.  There's no clue when
reading the code what -1/0/1 mean.

Apparently here you *do* want the "-1 means the PCI core will never
set match_driver to 1" functionality, so maybe you do depend on it.

If that's the case, how to you ever bind a driver to these VFs?  The
changelog says you don't want VF drivers to load *immediately*, so I
assume you do want them to load eventually.

Bjorn

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Michael Ellerman]

Bjorn Helgaas  writes:

> On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote:
>> This patch adds the machine dependent call for
>> pcibios_bus_add_device, since the previous patch
>> separated the calls out between the PowerNV and PowerVM.
>> 
>> The difference here is that for the PowerVM environment
>> we do not want match_driver set because in this environment
>> we do not want the VF device drivers to load immediately, due to
>> firmware loading the device node when VF device is assigned to the
>> logical partition.
>> 
>> This patch will depend on the patch linked below, which is under
>> review.
>> 
>> https://patchwork.kernel.org/patch/9882915/
>> 
>> Signed-off-by: Bryant G. Ly 
>> Signed-off-by: Juan J. Alvarez 
>> ---
>>  arch/powerpc/platforms/pseries/eeh_pseries.c | 24 
>>  1 file changed, 24 insertions(+)
>> 
>> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c 
>> b/arch/powerpc/platforms/pseries/eeh_pseries.c
>> index 6b812ad990e4..45946ee90985 100644
>> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
>> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
>> @@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX];
>>  static DEFINE_SPINLOCK(slot_errbuf_lock);
>>  static int eeh_error_buf_size;
>>  
>> +void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
>> +{
>> +struct pci_dn *pdn = pci_get_pdn(pdev);
>> +
>> +if (!pdev->is_virtfn)
>> +return;
>> +
>> +pdn->device_id  =  pdev->device;
>> +pdn->vendor_id  =  pdev->vendor;
>> +pdn->class_code =  pdev->class;
>> +
>> +/*
>> + * The following operations will fail if VF's sysfs files
>> + * aren't created or its resources aren't finalized.
>> + */
>> +eeh_add_device_early(pdn);
>> +eeh_add_device_late(pdev);
>> +eeh_sysfs_add_device(pdev);
>> +pdev->match_driver = -1;
>
> match_driver is a bool, which should be assigned "true" or "false".

Above he mentioned a dependency on:

  [04/10] PCI: extend pci device match_driver state
  https://patchwork.kernel.org/patch/9882915/


Which makes it an int.

Or has that patch been rejected or something?

cheers

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote:
> This patch adds the machine dependent call for
> pcibios_bus_add_device, since the previous patch
> separated the calls out between the PowerNV and PowerVM.
> 
> The difference here is that for the PowerVM environment
> we do not want match_driver set because in this environment
> we do not want the VF device drivers to load immediately, due to
> firmware loading the device node when VF device is assigned to the
> logical partition.
> 
> This patch will depend on the patch linked below, which is under
> review.
> 
> https://patchwork.kernel.org/patch/9882915/
> 
> Signed-off-by: Bryant G. Ly 
> Signed-off-by: Juan J. Alvarez 
> ---
>  arch/powerpc/platforms/pseries/eeh_pseries.c | 24 
>  1 file changed, 24 insertions(+)
> 
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c 
> b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 6b812ad990e4..45946ee90985 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX];
>  static DEFINE_SPINLOCK(slot_errbuf_lock);
>  static int eeh_error_buf_size;
>  
> +void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
> +{
> + struct pci_dn *pdn = pci_get_pdn(pdev);
> +
> + if (!pdev->is_virtfn)
> + return;
> +
> + pdn->device_id  =  pdev->device;
> + pdn->vendor_id  =  pdev->vendor;
> + pdn->class_code =  pdev->class;
> +
> + /*
> +  * The following operations will fail if VF's sysfs files
> +  * aren't created or its resources aren't finalized.
> +  */
> + eeh_add_device_early(pdn);
> + eeh_add_device_late(pdev);
> + eeh_sysfs_add_device(pdev);
> + pdev->match_driver = -1;

match_driver is a bool, which should be assigned "true" or "false".

> +}
> +
>  /**
>   * pseries_eeh_init - EEH platform dependent initialization
>   *
> @@ -120,6 +141,9 @@ static int pseries_eeh_init(void)
>   /* Set EEH probe mode */
>   eeh_add_flag(EEH_PROBE_MODE_DEVTREE | EEH_ENABLE_IO_FOR_LOG);
>  
> + /* Set EEH machine dependent code */
> + ppc_md.pcibios_bus_add_device = pseries_pcibios_bus_add_device;
> +
>   return 0;
>  }
>  
> -- 
> 2.11.0 (Apple Git-81)
> 

[PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bryant G. Ly]

This patch adds the machine dependent call for
pcibios_bus_add_device, since the previous patch
separated the calls out between the PowerNV and PowerVM.

The difference here is that for the PowerVM environment
we do not want match_driver set because in this environment
we do not want the VF device drivers to load immediately, due to
firmware loading the device node when VF device is assigned to the
logical partition.

This patch will depend on the patch linked below, which is under
review.

https://patchwork.kernel.org/patch/9882915/

Signed-off-by: Bryant G. Ly 
Signed-off-by: Juan J. Alvarez 
---
 arch/powerpc/platforms/pseries/eeh_pseries.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c 
b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 6b812ad990e4..45946ee90985 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX];
 static DEFINE_SPINLOCK(slot_errbuf_lock);
 static int eeh_error_buf_size;
 
+void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
+{
+   struct pci_dn *pdn = pci_get_pdn(pdev);
+
+   if (!pdev->is_virtfn)
+   return;
+
+   pdn->device_id  =  pdev->device;
+   pdn->vendor_id  =  pdev->vendor;
+   pdn->class_code =  pdev->class;
+
+   /*
+* The following operations will fail if VF's sysfs files
+* aren't created or its resources aren't finalized.
+*/
+   eeh_add_device_early(pdn);
+   eeh_add_device_late(pdev);
+   eeh_sysfs_add_device(pdev);
+   pdev->match_driver = -1;
+}
+
 /**
  * pseries_eeh_init - EEH platform dependent initialization
  *
@@ -120,6 +141,9 @@ static int pseries_eeh_init(void)
/* Set EEH probe mode */
eeh_add_flag(EEH_PROBE_MODE_DEVTREE | EEH_ENABLE_IO_FOR_LOG);
 
+   /* Set EEH machine dependent code */
+   ppc_md.pcibios_bus_add_device = pseries_pcibios_bus_add_device;
+
return 0;
 }
 
-- 
2.11.0 (Apple Git-81)