From mboxrd@z Thu Jan  1 00:00:00 1970
From: Yongji Xie <xyjxie@linux.vnet.ibm.com>
Subject: Re: [RFC v5 3/7] PCI: Add a new option for resource_alignment to
 reassign alignment
Date: Wed, 6 Apr 2016 10:43:57 +0800
Message-ID: <570477ED.7030400@linux.vnet.ibm.com>
References: <1459863813-2830-1-git-send-email-xyjxie@linux.vnet.ibm.com>
 <1459863813-2830-4-git-send-email-xyjxie@linux.vnet.ibm.com>
 <20160406014309.GA17201@gwshan>
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============7336945173360162929=="
Cc: nikunj@linux.vnet.ibm.com, zhong@linux.vnet.ibm.com, kvm@vger.kernel.org,
 linux-doc@vger.kernel.org, aik@ozlabs.ru, linux-pci@vger.kernel.org,
 corbet@lwn.net, linux-kernel@vger.kernel.org, warrier@linux.vnet.ibm.com,
 alex.williamson@redhat.com, paulus@samba.org, bhelgaas@google.com,
 linuxppc-dev@lists.ozlabs.org
To: Gavin Shan <gwshan@linux.vnet.ibm.com>
Return-path: <linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org>
In-Reply-To: <20160406014309.GA17201@gwshan>
List-Unsubscribe: <https://lists.ozlabs.org/options/linuxppc-dev>,
 <mailto:linuxppc-dev-request@lists.ozlabs.org?subject=unsubscribe>
List-Archive: <http://lists.ozlabs.org/pipermail/linuxppc-dev/>
List-Post: <mailto:linuxppc-dev@lists.ozlabs.org>
List-Help: <mailto:linuxppc-dev-request@lists.ozlabs.org?subject=help>
List-Subscribe: <https://lists.ozlabs.org/listinfo/linuxppc-dev>,
 <mailto:linuxppc-dev-request@lists.ozlabs.org?subject=subscribe>
Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org
Sender: "Linuxppc-dev"
 <linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org>
List-Id: kvm.vger.kernel.org

This is a multi-part message in MIME format.
--===============7336945173360162929==
Content-Type: multipart/alternative;
 boundary="------------080603050105070708030307"

This is a multi-part message in MIME format.
--------------080603050105070708030307
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2016/4/6 9:43, Gavin Shan wrote:
> On Tue, Apr 05, 2016 at 09:43:31PM +0800, Yongji Xie wrote:
>> When using resource_alignment kernel parameter,
>> the current implement reassigns the alignment by
>> changing resources' size which can potentially
>> break some drivers. For example, the driver uses
>> the size to locate some register whose length is
>> related to the size.
>>
>> This adds a new option "noresize" for the
>> parameter to solve this problem.
>>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> ---
>> Documentation/kernel-parameters.txt |    5 ++++-
>> drivers/pci/pci.c                   |   37 +++++++++++++++++++++++++----------
>> 2 files changed, 31 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index 9a53c92..d8b29ab 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -2912,13 +2912,16 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>> 				window. The default value is 64 megabytes.
>> 		resource_alignment=
>> 				Format:
>> -				[<order of align>@][<domain>:]<bus>:<slot>.<func>[; ...]
>> +				[<order of align>@][<domain>:]<bus>:<slot>.<func>
>> +				[:noresize][; ...]
> If I'm correct enough, it breaks existing application using this interface. "noresize"
> would be a optinal parallel property to "align". So the string would have below format
> if nobody else objects:
>
> 	order,noresize@domain:bus:slot.func

Sorry. I don't get your point. Why [:noresize] would break
existing application using this interface? Doesn't
[:noresize] mean an optional property?

[<order of align>@] is also an optional property.
Sometimes, we would not specify it. So we should
not put "noresize" into [<order of align>@] in case
that we want to use "noresize" but we don't want
to specify <order of align>.

>
>> 				Specifies alignment and device to reassign
>> 				aligned memory resources.
>> 				If <order of align> is not specified,
>> 				PAGE_SIZE is used as alignment.
>> 				PCI-PCI bridge can be specified, if resource
>> 				windows need to be expanded.
>> +				noresize: Don't change the resources' sizes when
>> +				reassigning alignment.
>> 		ecrc=		Enable/disable PCIe ECRC (transaction layer
>> 				end-to-end CRC checking).
>> 				bios: Use BIOS/firmware settings. This is the
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 1db9267..7271ce3 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4594,11 +4594,13 @@ static DEFINE_SPINLOCK(resource_alignment_lock);
>> /**
>>   * pci_specified_resource_alignment - get resource alignment specified by user.
>>   * @dev: the PCI device to get
>> + * @resize: whether or not to change resources' size when reassigning alignment
>>   *
>>   * RETURNS: Resource alignment if it is specified.
>>   *          Zero if it is not specified.
>>   */
>> -static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
>> +static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
>> +		bool *resize)
>> {
>> 	int seg, bus, slot, func, align_order, count;
>> 	resource_size_t align = 0;
>> @@ -4632,6 +4634,11 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
>> 			}
>> 		}
>> 		p += count;
>> +		if (!strncmp(p, ":noresize", 9)) {
>> +			*resize = false;
>> +			p += 9;
>> +		} else
>> +			*resize = true;
>> 		if (seg == pci_domain_nr(dev->bus) &&
>> 			bus == dev->bus->number &&
>> 			slot == PCI_SLOT(dev->devfn) &&
>> @@ -4664,11 +4671,12 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>> {
>> 	int i;
>> 	struct resource *r;
>> +	bool resize = true;
>> 	resource_size_t align, size;
>> 	u16 command;
>>
>> 	/* check if specified PCI is target device to reassign */
>> -	align = pci_specified_resource_alignment(dev);
>> +	align = pci_specified_resource_alignment(dev, &resize);
>> 	if (!align)
>> 		return;
>>
>> @@ -4690,15 +4698,24 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>> 		if (!(r->flags & IORESOURCE_MEM))
>> 			continue;
>> 		size = resource_size(r);
>> -		if (size < align) {
>> -			size = align;
>> -			dev_info(&dev->dev,
>> -				"Rounding up size of resource #%d to %#llx.\n",
>> -				i, (unsigned long long)size);
>> +		if (resize) {
>> +			if (size < align) {
>> +				size = align;
>> +				dev_info(&dev->dev,
>> +					"Rounding up size of resource #%d to %#llx.\n",
>> +					i, (unsigned long long)size);
>> +			}
>> +			r->flags |= IORESOURCE_UNSET;
>> +			r->end = size - 1;
>> +			r->start = 0;
>> +		} else {
>> +			if (size > align)
>> +				align = size;
>> +			r->flags &= ~IORESOURCE_SIZEALIGN;
>> +			r->flags |= IORESOURCE_STARTALIGN | IORESOURCE_UNSET;
>> +			r->start = align;
>> +			r->end = r->start + size - 1;
> It could be something like below if you like:
>
> 			r->start = max(align, size);
> 			r->end = r->start + size - 1;

Sounds good!

Thanks,
Yongji

>> 		}
>> -		r->flags |= IORESOURCE_UNSET;
>> -		r->end = size - 1;
>> -		r->start = 0;
>> 	}
>> 	/* Need to disable bridge's resource window,
>> 	 * to enable the kernel to reassign new resource
> Thanks,
> Gavin
>
>> -- 
>> 1.7.9.5
>>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


--------------080603050105070708030307
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: 7bit

<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    On 2016/4/6 9:43, Gavin Shan wrote:<br>
    <blockquote cite="mid:20160406014309.GA17201@gwshan" type="cite">
      <pre wrap="">On Tue, Apr 05, 2016 at 09:43:31PM +0800, Yongji Xie wrote:
</pre>
      <blockquote type="cite">
        <pre wrap="">When using resource_alignment kernel parameter,
the current implement reassigns the alignment by
changing resources' size which can potentially
break some drivers. For example, the driver uses
the size to locate some register whose length is
related to the size.

This adds a new option "noresize" for the
parameter to solve this problem.

Signed-off-by: Yongji Xie <a class="moz-txt-link-rfc2396E" href="mailto:xyjxie@linux.vnet.ibm.com">&lt;xyjxie@linux.vnet.ibm.com&gt;</a>
---
Documentation/kernel-parameters.txt |    5 ++++-
drivers/pci/pci.c                   |   37 +++++++++++++++++++++++++----------
2 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 9a53c92..d8b29ab 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2912,13 +2912,16 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
				window. The default value is 64 megabytes.
		resource_alignment=
				Format:
-				[&lt;order of align&gt;@][&lt;domain&gt;:]&lt;bus&gt;:&lt;slot&gt;.&lt;func&gt;[; ...]
+				[&lt;order of align&gt;@][&lt;domain&gt;:]&lt;bus&gt;:&lt;slot&gt;.&lt;func&gt;
+				[:noresize][; ...]
</pre>
      </blockquote>
      <pre wrap="">
If I'm correct enough, it breaks existing application using this interface. "noresize"
would be a optinal parallel property to "align". So the string would have below format
if nobody else objects:

	order,<a class="moz-txt-link-abbreviated" href="mailto:noresize@domain:bus:slot.func">noresize@domain:bus:slot.func</a></pre>
    </blockquote>
    <br>
    Sorry. I don't get your point. Why [:noresize] would break <br>
    existing application using this interface? Doesn't <br>
    [:noresize] mean an optional property?<br>
    <br>
    [&lt;order of align&gt;@] is also an optional property. <br>
    Sometimes, we would not specify it. So we should<br>
    not put "noresize" into [&lt;order of align&gt;@] in case<br>
    that we want to use "noresize" but we don't want<br>
    to specify &lt;order of align&gt;.<br>
    <br>
    <span style="color: rgb(51, 51, 51); font-family: arial; font-size:
      24px; font-style: normal; font-variant: normal; font-weight:
      normal; letter-spacing: normal; line-height: 24px; orphans: auto;
      text-align: start; text-indent: 0px; text-transform: none;
      white-space: normal; widows: 1; word-spacing: 0px;
      -webkit-text-stroke-width: 0px; display: inline !important; float:
      none; background-color: rgb(255, 255, 255);"></span>
    <blockquote cite="mid:20160406014309.GA17201@gwshan" type="cite">
      <pre wrap="">

</pre>
      <blockquote type="cite">
        <pre wrap="">				Specifies alignment and device to reassign
				aligned memory resources.
				If &lt;order of align&gt; is not specified,
				PAGE_SIZE is used as alignment.
				PCI-PCI bridge can be specified, if resource
				windows need to be expanded.
+				noresize: Don't change the resources' sizes when
+				reassigning alignment.
		ecrc=		Enable/disable PCIe ECRC (transaction layer
				end-to-end CRC checking).
				bios: Use BIOS/firmware settings. This is the
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1db9267..7271ce3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4594,11 +4594,13 @@ static DEFINE_SPINLOCK(resource_alignment_lock);
/**
 * pci_specified_resource_alignment - get resource alignment specified by user.
 * @dev: the PCI device to get
+ * @resize: whether or not to change resources' size when reassigning alignment
 *
 * RETURNS: Resource alignment if it is specified.
 *          Zero if it is not specified.
 */
-static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
+static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
+		bool *resize)
{
	int seg, bus, slot, func, align_order, count;
	resource_size_t align = 0;
@@ -4632,6 +4634,11 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
			}
		}
		p += count;
+		if (!strncmp(p, ":noresize", 9)) {
+			*resize = false;
+			p += 9;
+		} else
+			*resize = true;
		if (seg == pci_domain_nr(dev-&gt;bus) &amp;&amp;
			bus == dev-&gt;bus-&gt;number &amp;&amp;
			slot == PCI_SLOT(dev-&gt;devfn) &amp;&amp;
@@ -4664,11 +4671,12 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
{
	int i;
	struct resource *r;
+	bool resize = true;
	resource_size_t align, size;
	u16 command;

	/* check if specified PCI is target device to reassign */
-	align = pci_specified_resource_alignment(dev);
+	align = pci_specified_resource_alignment(dev, &amp;resize);
	if (!align)
		return;

@@ -4690,15 +4698,24 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
		if (!(r-&gt;flags &amp; IORESOURCE_MEM))
			continue;
		size = resource_size(r);
-		if (size &lt; align) {
-			size = align;
-			dev_info(&amp;dev-&gt;dev,
-				"Rounding up size of resource #%d to %#llx.\n",
-				i, (unsigned long long)size);
+		if (resize) {
+			if (size &lt; align) {
+				size = align;
+				dev_info(&amp;dev-&gt;dev,
+					"Rounding up size of resource #%d to %#llx.\n",
+					i, (unsigned long long)size);
+			}
+			r-&gt;flags |= IORESOURCE_UNSET;
+			r-&gt;end = size - 1;
+			r-&gt;start = 0;
+		} else {
+			if (size &gt; align)
+				align = size;
+			r-&gt;flags &amp;= ~IORESOURCE_SIZEALIGN;
+			r-&gt;flags |= IORESOURCE_STARTALIGN | IORESOURCE_UNSET;
+			r-&gt;start = align;
+			r-&gt;end = r-&gt;start + size - 1;
</pre>
      </blockquote>
      <pre wrap="">
It could be something like below if you like:

			r-&gt;start = max(align, size);
			r-&gt;end = r-&gt;start + size - 1;
</pre>
    </blockquote>
    <br>
    Sounds good!<br>
    <br>
    Thanks,<br>
    Yongji<br>
    <br>
    <blockquote cite="mid:20160406014309.GA17201@gwshan" type="cite">
      <pre wrap="">
</pre>
      <blockquote type="cite">
        <pre wrap="">		}
-		r-&gt;flags |= IORESOURCE_UNSET;
-		r-&gt;end = size - 1;
-		r-&gt;start = 0;
	}
	/* Need to disable bridge's resource window,
	 * to enable the kernel to reassign new resource
</pre>
      </blockquote>
      <pre wrap="">
Thanks,
Gavin

</pre>
      <blockquote type="cite">
        <pre wrap="">-- 
1.7.9.5

</pre>
      </blockquote>
      <pre wrap="">
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to <a class="moz-txt-link-abbreviated" href="mailto:majordomo@vger.kernel.org">majordomo@vger.kernel.org</a>
More majordomo info at  <a class="moz-txt-link-freetext" href="http://vger.kernel.org/majordomo-info.html">http://vger.kernel.org/majordomo-info.html</a>

</pre>
    </blockquote>
    <br>
  </body>
</html>

--------------080603050105070708030307--


--===============7336945173360162929==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KTGludXhwcGMt
ZGV2IG1haWxpbmcgbGlzdApMaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZwpodHRwczovL2xp
c3RzLm96bGFicy5vcmcvbGlzdGluZm8vbGludXhwcGMtZGV2

--===============7336945173360162929==--