LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Shawn Anastasio <shawn@anastas.io>
To: Sam Bobroff <sbobroff@linux.ibm.com>, Oliver <oohall@gmail.com>
Cc: linux-pci@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	rppt@linux.ibm.com, Paul Mackerras <paulus@samba.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	xyjxie@linux.vnet.ibm.com,
	linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [EXTERNAL] Re: [PATCH v3 1/3] PCI: Introduce pcibios_ignore_alignment_request
Date: Thu, 30 May 2019 17:33:05 -0500	[thread overview]
Message-ID: <8785226a-b2eb-8157-e724-d575c66b7bef@anastas.io> (raw)
In-Reply-To: <20190530065556.GA29428@tungsten.ozlabs.ibm.com>



On 5/30/19 1:55 AM, Sam Bobroff wrote:
> On Tue, May 28, 2019 at 03:36:34PM +1000, Oliver wrote:
>> On Tue, May 28, 2019 at 2:03 PM Shawn Anastasio <shawn@anastas.io> wrote:
>>>
>>> Introduce a new pcibios function pcibios_ignore_alignment_request
>>> which allows the PCI core to defer to platform-specific code to
>>> determine whether or not to ignore alignment requests for PCI resources.
>>>
>>> The existing behavior is to simply ignore alignment requests when
>>> PCI_PROBE_ONLY is set. This is behavior is maintained by the
>>> default implementation of pcibios_ignore_alignment_request.
>>>
>>> Signed-off-by: Shawn Anastasio <shawn@anastas.io>
>>> ---
>>>   drivers/pci/pci.c   | 9 +++++++--
>>>   include/linux/pci.h | 1 +
>>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index 8abc843b1615..8207a09085d1 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -5882,6 +5882,11 @@ resource_size_t __weak pcibios_default_alignment(void)
>>>          return 0;
>>>   }
>>>
>>> +int __weak pcibios_ignore_alignment_request(void)
>>> +{
>>> +       return pci_has_flag(PCI_PROBE_ONLY);
>>> +}
>>> +
>>>   #define RESOURCE_ALIGNMENT_PARAM_SIZE COMMAND_LINE_SIZE
>>>   static char resource_alignment_param[RESOURCE_ALIGNMENT_PARAM_SIZE] = {0};
>>>   static DEFINE_SPINLOCK(resource_alignment_lock);
>>> @@ -5906,9 +5911,9 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
>>>          p = resource_alignment_param;
>>>          if (!*p && !align)
>>>                  goto out;
>>> -       if (pci_has_flag(PCI_PROBE_ONLY)) {
>>> +       if (pcibios_ignore_alignment_request()) {
>>>                  align = 0;
>>> -               pr_info_once("PCI: Ignoring requested alignments (PCI_PROBE_ONLY)\n");
>>> +               pr_info_once("PCI: Ignoring requested alignments\n");
>>>                  goto out;
>>>          }
>>
>> I think the logic here is questionable to begin with. If the user has
>> explicitly requested re-aligning a resource via the command line then
>> we should probably do it even if PCI_PROBE_ONLY is set. When it breaks
>> they get to keep the pieces.
>>
>> That said, the real issue here is that PCI_PROBE_ONLY probably
>> shouldn't be set under qemu/kvm. Under the other hypervisor (PowerVM)
>> hotplugged devices are configured by firmware before it's passed to
>> the guest and we need to keep the FW assignments otherwise things
>> break. QEMU however doesn't do any BAR assignments and relies on that
>> being handled by the guest. At boot time this is done by SLOF, but
>> Linux only keeps SLOF around until it's extracted the device-tree.
>> Once that's done SLOF gets blown away and the kernel needs to do it's
>> own BAR assignments. I'm guessing there's a hack in there to make it
>> work today, but it's a little surprising that it works at all...
>>
>> IIRC Sam Bobroff was looking at hotplug under pseries recently so he
>> might have something to add. He's sick at the moment, but I'll ask him
>> to take a look at this once he's back among the living
> 
> There seems to be some code already in the kernel that will disable
> PCI_PROBE_ONLY based on a device tree property, so I did a quick test
> today and it seems to work. Only a trivial tweak is needed in QEMU to
> do it (have spapr_dt_chosen() add a node called "linux,pci-probe-only"
> with a value of 0), and that would allow us to set it only for QEMU (and
> not PowerVM) if that's what we want to do. Is that useful?
> 
> (I haven't done any real testing yet but the guest booted up OK.)

It was my understanding that PCI_PROBE_ONLY should actually be set
initially so that Linux uses SLOF's BAR assignments. The issue here
is that PCI_PROBE_ONLY shouldn't be honored after initial bringup
on KVM so that hotplugged PCI devices can have custom BAR alignments.

Of course, if there's no need to honor SLOF's initial assignments,
I assume disabling PCI_PROBE_ONLY would work fine. In fact, I'm
not entirely sure why it's done in the first place. Does anybody
know?

If there is actually a valid reason for preserving SLOF's initial
assignments, then it seems like the correct solution is to disable
PCI_PROBE_ONLY after initial PCI bringup or ignore it in
pci_specified_resource_alignment() like I do in this patch set.

Bjorn Helgaas also suggested marking individual resources provided
by SLOF/PHYP with IORESOURCE_PCI_FIXED which would remove the need
to use PCI_PROBE_ONLY altogether.

Any thoughts?

- Shawn

  reply	other threads:[~2019-05-30 22:33 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-28  4:03 [PATCH v3 0/3] Allow custom PCI resource alignment on pseries Shawn Anastasio
2019-05-28  4:03 ` [PATCH v3 1/3] PCI: Introduce pcibios_ignore_alignment_request Shawn Anastasio
2019-05-28  5:36   ` Oliver
2019-05-28  5:50     ` Shawn Anastasio
2019-05-28  6:27     ` Alexey Kardashevskiy
2019-05-28  7:39       ` Shawn Anastasio
2019-05-30  3:39         ` Alexey Kardashevskiy
2019-05-30 22:49           ` Shawn Anastasio
2019-05-31  3:56             ` Alexey Kardashevskiy
2019-06-03  2:23               ` Shawn Anastasio
2019-06-03  5:02                 ` Alexey Kardashevskiy
2019-06-03  8:35                   ` Alexey Kardashevskiy
2019-06-03  9:12                     ` Shawn Anastasio
2019-05-29 14:00     ` Bjorn Helgaas
2019-05-30  6:55     ` [EXTERNAL] " Sam Bobroff
2019-05-30 22:33       ` Shawn Anastasio [this message]
2019-05-28  4:03 ` [PATCH v3 2/3] powerpc/64: Enable pcibios_after_init hook on ppc64 Shawn Anastasio
2019-05-28  4:03 ` [PATCH v3 3/3] powerpc/pseries: Allow user-specified PCI resource alignment after init Shawn Anastasio
2019-05-29 14:02   ` Bjorn Helgaas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8785226a-b2eb-8157-e724-d575c66b7bef@anastas.io \
    --to=shawn@anastas.io \
    --cc=bhelgaas@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=oohall@gmail.com \
    --cc=paulus@samba.org \
    --cc=rppt@linux.ibm.com \
    --cc=sbobroff@linux.ibm.com \
    --cc=xyjxie@linux.vnet.ibm.com \
    --subject='Re: [EXTERNAL] Re: [PATCH v3 1/3] PCI: Introduce pcibios_ignore_alignment_request' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).