Re: [PATCH 5/7] IB/hfi1: use pcie_flr instead of duplicating it

[Bjorn Helgaas]

On Thu, Apr 27, 2017 at 08:47:58AM +0200, Christoph Hellwig wrote:
> On Tue, Apr 25, 2017 at 02:39:55PM -0500, Bjorn Helgaas wrote:
> > This still leaves these:
> > 
> >   [PATCH 4/7] ixgbe: use pcie_flr instead of duplicating it
> >   [PATCH 6/7] crypto: qat: use pcie_flr instead of duplicating it
> >   [PATCH 7/7] liquidio: use pcie_flr instead of duplicating it
> > 
> > I haven't seen any response to 4 and 6.  Felix reported an unused
> > variable in 7.  Let me know if you'd like me to do anything with
> > these.
> 
> Now that Jeff ACKed 4 it might be worth to add it to the pci tree last
> minute.  I'll resend liquidio and qat to the respective maintainers for
> the next merge window.

I applied 4 with Jeff's ack to pci/virtualization for v4.12, thanks!

Re: [PATCH 5/7] IB/hfi1: use pcie_flr instead of duplicating it

[Bjorn Helgaas]

On Mon, Apr 24, 2017 at 04:35:07PM +0200, Christoph Hellwig wrote:
> On Mon, Apr 24, 2017 at 02:16:31PM +, Byczkowski, Jakub wrote:
> > Tested-by: Jakub Byczkowski 
> 
> Are you (and Doug) ok with queueing this up in the PCI tree?

Applied this with Jakub's tested-by and Doug's ack to pci/virtualization
for v4.12.

This still leaves these:

  [PATCH 4/7] ixgbe: use pcie_flr instead of duplicating it
  [PATCH 6/7] crypto: qat: use pcie_flr instead of duplicating it
  [PATCH 7/7] liquidio: use pcie_flr instead of duplicating it

I haven't seen any response to 4 and 6.  Felix reported an unused
variable in 7.  Let me know if you'd like me to do anything with
these.

Bjorn

Re: export pcie_flr and remove copies of it in drivers V2

[Bjorn Helgaas]

On Fri, Apr 14, 2017 at 09:11:24PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this exports the PCI layer pcie_flr helper, and removes various opencoded
> copies of it.
> 
> Changes since V1:
>  - rebase on top of the pci/virtualization branch
>  - fixed the probe case in __pci_dev_reset
>  - added ACKs from Bjorn

Applied the first three patches:
 
  bc13871ef35a PCI: Export pcie_flr()
  e641c375d414 PCI: Call pcie_flr() from reset_intel_82599_sfp_virtfn()
  40e0901ea4bf PCI: Call pcie_flr() from reset_chelsio_generic_dev()

to pci/virtualization for v4.12, thanks!

Re: export pcie_flr and remove copies of it in drivers

[Bjorn Helgaas]

On Fri, Apr 14, 2017 at 9:41 AM, Bjorn Helgaas <helg...@kernel.org> wrote:
> On Thu, Apr 13, 2017 at 04:53:32PM +0200, Christoph Hellwig wrote:
>> Hi all,
>>
>> this exports the PCI layer pcie_flr helper, and removes various opencoded
>> copies of it.
>
> Looks good to me (except the comment on probe).  If you want to apply
> the whole series via netdev or some non-PCI tree, here's my ack for
> the drivers/pci parts, assuming the probe thing is resolved:
>
> Acked-by: Bjorn Helgaas <bhelg...@google.com>
>
> Otherwise, I'd be glad to take the series given acks for the non-PCI
> parts.  Just let me know.

I do already have a patch (c5e4f0192ad2 ("PCI: Avoid FLR for Intel
82579 NICs")) on my pci/virtualization branch that touches pcie_flr()
and will conflict with this one.

Re: export pcie_flr and remove copies of it in drivers

[Bjorn Helgaas]

On Thu, Apr 13, 2017 at 04:53:32PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this exports the PCI layer pcie_flr helper, and removes various opencoded
> copies of it.

Looks good to me (except the comment on probe).  If you want to apply
the whole series via netdev or some non-PCI tree, here's my ack for
the drivers/pci parts, assuming the probe thing is resolved:

Acked-by: Bjorn Helgaas <bhelg...@google.com>

Otherwise, I'd be glad to take the series given acks for the non-PCI
parts.  Just let me know.

Bjorn

Re: [PATCH 1/7] PCI: export pcie_flr

[Bjorn Helgaas]

[+cc Alex]

On Thu, Apr 13, 2017 at 04:53:33PM +0200, Christoph Hellwig wrote:
> Currently we opencode the FLR sequence in lots of place, export a core
> helper instead.  We split out the probing for FLR support as all the
> non-core callers already know their hardware.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/pci/pci.c   | 34 +-
>  include/linux/pci.h |  1 +
>  2 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7904d02ffdb9..3256a63c5d08 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3773,24 +3773,38 @@ static void pci_flr_wait(struct pci_dev *dev)
>(i - 1) * 100);
>  }
>  
> -static int pcie_flr(struct pci_dev *dev, int probe)
> +/**
> + * pcie_has_flr - check if a device supports function level resets
> + * @dev: device to check
> + *
> + * Returns true if the device advertises support for PCIe function level
> + * resets.
> + */
> +static bool pcie_has_flr(struct pci_dev *dev)
>  {
>   u32 cap;
>  
>   pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
> - if (!(cap & PCI_EXP_DEVCAP_FLR))
> - return -ENOTTY;
> -
> - if (probe)
> - return 0;
> + return cap & PCI_EXP_DEVCAP_FLR;
> +}
>  
> +/**
> + * pcie_flr - initiate a PCIe function level reset
> + * @dev: device to reset
> + *
> + * Initiate a function level reset on @dev.  The caller should ensure the
> + * device supports FLR before calling this function, e.g. by using the
> + * pcie_has_flr helper.

s/pcie_has_flr/pcie_has_flr()/

> + */
> +void pcie_flr(struct pci_dev *dev)
> +{
>   if (!pci_wait_for_pending_transaction(dev))
>   dev_err(>dev, "timed out waiting for pending transaction; 
> performing function level reset anyway\n");
>  
>   pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
>   pci_flr_wait(dev);
> - return 0;
>  }
> +EXPORT_SYMBOL_GPL(pcie_flr);
>  
>  static int pci_af_flr(struct pci_dev *dev, int probe)
>  {
> @@ -3971,9 +3985,11 @@ static int __pci_dev_reset(struct pci_dev *dev, int 
> probe)
>   if (rc != -ENOTTY)
>   goto done;
>  
> - rc = pcie_flr(dev, probe);
> - if (rc != -ENOTTY)
> + if (pcie_has_flr(dev)) {
> + pcie_flr(dev);
> + rc = 0;
>   goto done;
> + }

This performs an FLR (if supported) always, regardless of "probe".
I think it should look something like this instead:

  if (pcie_has_flr(dev)) {
if (!probe)
  pcie_flr(dev);
rc = 0;
goto done;
  }

>   rc = pci_af_flr(dev, probe);
>   if (rc != -ENOTTY)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index eb3da1a04e6c..f35e51eddad0 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1052,6 +1052,7 @@ int pcie_get_mps(struct pci_dev *dev);
>  int pcie_set_mps(struct pci_dev *dev, int mps);
>  int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
> enum pcie_link_width *width);
> +void pcie_flr(struct pci_dev *dev);
>  int __pci_reset_function(struct pci_dev *dev);
>  int __pci_reset_function_locked(struct pci_dev *dev);
>  int pci_reset_function(struct pci_dev *dev);
> -- 
> 2.11.0
> 

Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup data

[Bjorn Helgaas]

On Fri, Mar 03, 2017 at 03:15:34PM -0600, Tom Lendacky wrote:
> On 3/3/2017 2:42 PM, Bjorn Helgaas wrote:
> >On Thu, Mar 02, 2017 at 10:13:10AM -0500, Brijesh Singh wrote:
> >>From: Tom Lendacky <thomas.lenda...@amd.com>
> >>
> >>The use of ioremap will force the setup data to be mapped decrypted even
> >>though setup data is encrypted.  Switch to using memremap which will be
> >>able to perform the proper mapping.
> >
> >How should callers decide whether to use ioremap() or memremap()?
> >
> >memremap() existed before SME and SEV, and this code is used even if
> >SME and SEV aren't supported, so the rationale for this change should
> >not need the decryption argument.
> 
> When SME or SEV is active an ioremap() will remove the encryption bit
> from the pagetable entry when it is mapped.  This allows MMIO, which
> doesn't support SME/SEV, to be performed successfully.  So my take is
> that ioremap() should be used for MMIO and memremap() for pages in RAM.

OK, thanks.  The commit message should say something like "this is
RAM, not MMIO, so we should map it with memremap(), not ioremap()".
That's the part that determines whether the change is correct.

You can mention the encryption part, too, but it's definitely
secondary because the change has to make sense on its own, without
SME/SEV.

The following commits (from https://github.com/codomania/tip/branches)
all do basically the same thing so the changelogs (and summaries)
should all be basically the same:

  cb0d0d1eb0a6 x86: Change early_ioremap to early_memremap for BOOT data
  91acb68b8333 x86/pci: Use memremap when walking setup data
  4f687503e23f x86: Access the setup data through sysfs decrypted
  e90246b8c229 x86: Access the setup data through debugfs decrypted

I would collect them all together and move them to the beginning of
your series, since they don't depend on anything else.

Also, change "x86/pci: " to "x86/PCI" so it matches the previous
convention.

> >>Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>

Acked-by: Bjorn Helgaas <bhelg...@google.com>

> >>---
> >> arch/x86/pci/common.c |4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> >>index a4fdfa7..0b06670 100644
> >>--- a/arch/x86/pci/common.c
> >>+++ b/arch/x86/pci/common.c
> >>@@ -691,7 +691,7 @@ int pcibios_add_device(struct pci_dev *dev)
> >>
> >>pa_data = boot_params.hdr.setup_data;
> >>while (pa_data) {
> >>-   data = ioremap(pa_data, sizeof(*rom));
> >>+   data = memremap(pa_data, sizeof(*rom), MEMREMAP_WB);
> >
> >I can't quite connect the dots here.  ioremap() on x86 would do
> >ioremap_nocache().  memremap(MEMREMAP_WB) would do arch_memremap_wb(),
> >which is ioremap_cache().  Is making a cacheable mapping the important
> >difference?
> 
> The memremap(MEMREMAP_WB) will actually check to see if it can perform
> a __va(pa_data) in try_ram_remap() and then fallback to the
> arch_memremap_wb().  So it's actually the __va() vs the ioremap_cache()
> that is the difference.
> 
> Thanks,
> Tom
> 
> >
> >>if (!data)
> >>return -ENOMEM;
> >>
> >>@@ -710,7 +710,7 @@ int pcibios_add_device(struct pci_dev *dev)
> >>}
> >>}
> >>pa_data = data->next;
> >>-   iounmap(data);
> >>+   memunmap(data);
> >>}
> >>set_dma_domain_ops(dev);
> >>set_dev_domain_options(dev);
> >>

Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup data

[Bjorn Helgaas]

On Thu, Mar 02, 2017 at 10:13:10AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> The use of ioremap will force the setup data to be mapped decrypted even
> though setup data is encrypted.  Switch to using memremap which will be
> able to perform the proper mapping.

How should callers decide whether to use ioremap() or memremap()?

memremap() existed before SME and SEV, and this code is used even if
SME and SEV aren't supported, so the rationale for this change should
not need the decryption argument.

> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/pci/common.c |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index a4fdfa7..0b06670 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -691,7 +691,7 @@ int pcibios_add_device(struct pci_dev *dev)
>  
>   pa_data = boot_params.hdr.setup_data;
>   while (pa_data) {
> - data = ioremap(pa_data, sizeof(*rom));
> + data = memremap(pa_data, sizeof(*rom), MEMREMAP_WB);

I can't quite connect the dots here.  ioremap() on x86 would do
ioremap_nocache().  memremap(MEMREMAP_WB) would do arch_memremap_wb(),
which is ioremap_cache().  Is making a cacheable mapping the important
difference?

>   if (!data)
>   return -ENOMEM;
>  
> @@ -710,7 +710,7 @@ int pcibios_add_device(struct pci_dev *dev)
>   }
>   }
>   pa_data = data->next;
> - iounmap(data);
> + memunmap(data);
>   }
>   set_dma_domain_ops(dev);
>   set_dev_domain_options(dev);
> 

Re: [RFC PATCH v2 00/32] x86: Secure Encrypted Virtualization (AMD)

[Bjorn Helgaas]

On Thu, Mar 02, 2017 at 10:12:01AM -0500, Brijesh Singh wrote:
> This RFC series provides support for AMD's new Secure Encrypted Virtualization
> (SEV) feature. This RFC is build upon Secure Memory Encryption (SME) RFCv4 
> [1].

What kernel version is this series based on?

Re: [PATCH 1/2] PCI/IOV: Add function to allow Function Dependency Link override.

[Bjorn Helgaas]

On Mon, Aug 22, 2016 at 07:49:09AM -0700, David Daney wrote:
> On 08/22/2016 07:36 AM, Bjorn Helgaas wrote:
> >Hi David & Omer,
> >
> >On Fri, Aug 19, 2016 at 03:32:12PM -0700, Omer Khaliq wrote:
> >>From: David Daney <david.da...@cavium.com>
> >>
> >>Some hardware presents an incorrect SR-IOV Function Dependency Link,
> >>add a function to allow this to be overridden in the PF driver for
> >>such devices.
> >>
> >>Signed-off-by: David Daney <david.da...@cavium.com>
> >>Signed-off-by: Omer Khaliq <okha...@caviumnetworks.com>
> >>---
> >>  drivers/pci/iov.c   | 14 ++
> >>  include/linux/pci.h |  1 +
> >>  2 files changed, 15 insertions(+)
> >>
> >>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >>index 2194b44..81f0672 100644
> >>--- a/drivers/pci/iov.c
> >>+++ b/drivers/pci/iov.c
> >>@@ -640,6 +640,20 @@ int pci_enable_sriov(struct pci_dev *dev, int 
> >>nr_virtfn)
> >>  EXPORT_SYMBOL_GPL(pci_enable_sriov);
> >>
> >>  /**
> >>+ * pci_sriov_fdl_override - fix incorrect Function Dependency Link
> >>+ * @dev: the PCI device
> >>+ * @fdl: the corrected Function Dependency Link value
> >>+ *
> >>+ * For hardware presenting an incorrect Function Dependency Link in
> >>+ * the SR-IOV Extended Capability, allow a driver to override it.
> >>+ */
> >>+void pci_sriov_fdl_override(struct pci_dev *dev, u8 fdl)
> >>+{
> >>+   dev->sriov->link = fdl;
> >>+}
> >>+EXPORT_SYMBOL_GPL(pci_sriov_fdl_override);
> >
> >We usually use quirks to work around problems in config space.  That's
> >a nice mechanism because we don't have to add new PCI core interfaces
> >and it makes it clear that we're working around a hardware problem.
> >
> >Can you use a quirk here?  We allocate dev->sriov in the
> >pci_init_capabilities() path, so it looks like a pci_fixup_final quirk
> >should work.
> >
> 
> The struct pci_sriov definition is private to drivers/pci, so in
> order to use a quirk to fix this, we would have to put it in
> drivers/pci/quirks.c.  I was trying to keep this very device
> specific code in the driver, which requires an accessor to be able
> to manipulate the dev->sriov->link field.
> 
> If you prefer a quirk in drivers/pci/quirks.c, we can certainly do
> that instead.

Oh, I didn't notice that pci_sriov was declared in drivers/pci/pci.h
instead of linux/pci.h.  I do think I would prefer a quirk, and I
think it's fine to put it in drivers/pci/quirks.c.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] PCI/IOV: Add function to allow Function Dependency Link override.

[Bjorn Helgaas]

Hi David & Omer,

On Fri, Aug 19, 2016 at 03:32:12PM -0700, Omer Khaliq wrote:
> From: David Daney 
> 
> Some hardware presents an incorrect SR-IOV Function Dependency Link,
> add a function to allow this to be overridden in the PF driver for
> such devices.
> 
> Signed-off-by: David Daney 
> Signed-off-by: Omer Khaliq 
> ---
>  drivers/pci/iov.c   | 14 ++
>  include/linux/pci.h |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 2194b44..81f0672 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -640,6 +640,20 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>  EXPORT_SYMBOL_GPL(pci_enable_sriov);
>  
>  /**
> + * pci_sriov_fdl_override - fix incorrect Function Dependency Link
> + * @dev: the PCI device
> + * @fdl: the corrected Function Dependency Link value
> + *
> + * For hardware presenting an incorrect Function Dependency Link in
> + * the SR-IOV Extended Capability, allow a driver to override it.
> + */
> +void pci_sriov_fdl_override(struct pci_dev *dev, u8 fdl)
> +{
> + dev->sriov->link = fdl;
> +}
> +EXPORT_SYMBOL_GPL(pci_sriov_fdl_override);

We usually use quirks to work around problems in config space.  That's
a nice mechanism because we don't have to add new PCI core interfaces
and it makes it clear that we're working around a hardware problem.

Can you use a quirk here?  We allocate dev->sriov in the
pci_init_capabilities() path, so it looks like a pci_fixup_final quirk
should work.

> +
> +/**
>   * pci_disable_sriov - disable the SR-IOV capability
>   * @dev: the PCI device
>   */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2599a98..da8a5b3 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1823,6 +1823,7 @@ int pci_num_vf(struct pci_dev *dev);
>  int pci_vfs_assigned(struct pci_dev *dev);
>  int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>  int pci_sriov_get_totalvfs(struct pci_dev *dev);
> +void pci_sriov_fdl_override(struct pci_dev *dev, u8 fdl);
>  resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
>  #else
>  static inline int pci_iov_virtfn_bus(struct pci_dev *dev, int id)
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html