pci-hyperv: Use only 16 bit integer for PCI domain

Message ID 1492706123-22913-1-git-send-email-haiyangz@exchange.microsoft.com
State Changes Requested
Headers show

Commit Message

Haiyang Zhang April 20, 2017, 4:35 p.m.
From: Haiyang Zhang <haiyangz@microsoft.com>

This patch uses the lower 16 bits of the serial number as PCI
domain, otherwise some drivers may not be able to handle it.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/pci/host/pci-hyperv.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

Comments

Bjorn Helgaas April 20, 2017, 6:33 p.m. | #1
On Thu, Apr 20, 2017 at 11:35 AM, Haiyang Zhang
<haiyangz@exchange.microsoft.com> wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
>
> This patch uses the lower 16 bits of the serial number as PCI
> domain, otherwise some drivers may not be able to handle it.

Can you give any more details about this?  Which drivers, for
instance?  Why do drivers care about the domain at all?  Can we or
should we make this more explicit and consistent in the PCI core,
e.g., pci_domain_nr() is currently defined to return "int"; maybe it
should be u32?  (Although I think "int" is the same size as "u32" on
all arches anyway).

> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  drivers/pci/host/pci-hyperv.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index e73880c..b18dff3 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1334,9 +1334,11 @@ static void put_pcichild(struct hv_pci_dev *hpdev,
>          * can have shorter names than based on the bus instance UUID.
>          * Only the first device serial number is used for domain, so the
>          * domain number will not change after the first device is added.
> +        * The lower 16 bits of the serial number is used, otherwise some
> +        * drivers may not be able to handle it.
>          */
>         if (list_empty(&hbus->children))
> -               hbus->sysdata.domain = desc->ser;
> +               hbus->sysdata.domain = desc->ser & 0xFFFF;
>         list_add_tail(&hpdev->list_entry, &hbus->children);
>         spin_unlock_irqrestore(&hbus->device_list_lock, flags);
>         return hpdev;
> --
> 1.7.1
>
Haiyang Zhang April 20, 2017, 6:37 p.m. | #2
> -----Original Message-----

> From: Bjorn Helgaas [mailto:bhelgaas@google.com]

> Sent: Thursday, April 20, 2017 2:33 PM

> To: Haiyang Zhang <haiyangz@microsoft.com>

> Cc: linux-pci@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;

> Stephen Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de;

> vkuznets@redhat.com; driverdev-devel@linuxdriverproject.org; linux-

> kernel@vger.kernel.org

> Subject: Re: [PATCH] pci-hyperv: Use only 16 bit integer for PCI domain

> 

> On Thu, Apr 20, 2017 at 11:35 AM, Haiyang Zhang

> <haiyangz@exchange.microsoft.com> wrote:

> > From: Haiyang Zhang <haiyangz@microsoft.com>

> >

> > This patch uses the lower 16 bits of the serial number as PCI

> > domain, otherwise some drivers may not be able to handle it.

> 

> Can you give any more details about this?  Which drivers, for

> instance?  Why do drivers care about the domain at all?  Can we or

> should we make this more explicit and consistent in the PCI core,

> e.g., pci_domain_nr() is currently defined to return "int"; maybe it

> should be u32?  (Although I think "int" is the same size as "u32" on

> all arches anyway).


It's Nvidia driver.

Piotr, could you explain why the driver expects 16 bit domain number?

Thanks,
- Haiyang
Christoph Hellwig April 20, 2017, 7:12 p.m. | #3
On Thu, Apr 20, 2017 at 06:37:35PM +0000, Haiyang Zhang wrote:
> It's Nvidia driver.

Which of the many nvidia drivers in the tree?  Just fix it instead of
coming up with stupid workarounds like this.
John Hubbard April 24, 2017, 11:06 p.m. | #4
On 04/20/2017 11:37 AM, Haiyang Zhang wrote:
>> -----Original Message-----
>> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
>> Sent: Thursday, April 20, 2017 2:33 PM
>> To: Haiyang Zhang <haiyangz@microsoft.com>
>> Cc: linux-pci@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
>> Stephen Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de;
>> vkuznets@redhat.com; driverdev-devel@linuxdriverproject.org; linux-
>> kernel@vger.kernel.org
>> Subject: Re: [PATCH] pci-hyperv: Use only 16 bit integer for PCI domain
>>
>> On Thu, Apr 20, 2017 at 11:35 AM, Haiyang Zhang
>> <haiyangz@exchange.microsoft.com> wrote:
>>> From: Haiyang Zhang <haiyangz@microsoft.com>
>>>
>>> This patch uses the lower 16 bits of the serial number as PCI
>>> domain, otherwise some drivers may not be able to handle it.
>>
>> Can you give any more details about this?  Which drivers, for
>> instance?  Why do drivers care about the domain at all?  Can we or
>> should we make this more explicit and consistent in the PCI core,
>> e.g., pci_domain_nr() is currently defined to return "int"; maybe it
>> should be u32?  (Although I think "int" is the same size as "u32" on
>> all arches anyway).
> 
> It's Nvidia driver.
> 
> Piotr, could you explain why the driver expects 16 bit domain number?

Hi Haiyang and all,

First, a tiny nit about the patch: it would be good to add "Fixing a problem that was introduced 
with commit <4a9b0933bdfc>", in the patch commit message.

Piotr and I just now worked through both the driver and the ACPI/PCI history a little bit, and it 
brings up an interesting question: would it be better for the kernel, long-term, if we changed 
pci_domain_nr() and its callers to use 16 bit values (it's a mini-project, but not too hard)? I ask, 
because:

    a) the ACPI specification[1] says that PCI domains ("PCI Segment Groups") are 16 bits. The other 
16 bits are reserved. I'm concerned that if we don't clamp these to 16 bits in the kernel, virtual 
machines and other experimenters may continue to do things that cause problems--especially if 
ACPI/PCI ever tries to use those reserved 16 bits.

    b) A whirlwind survey of a few non-x86 arches shows that they are casting or truncating the PCI 
domain to 16 bits (here, if other, real linux-pci experts have some input, that would help!)

    c) Looking back at the original commit that added PCI domain support, Linux has specified the 
storage size as 32 bits, right from the start...but it looks like merely a convenience, rather than 
an exact match for a specification.

Please, let me emphasize that the driver can be changed to use 32 bits as well, no problem. But I 
really do want the kernel to have the most accurate and correct code, too, and it really looks (so 
far) like it wants to be 16 bits.

Also...it would be nice if we could use Haiyang's patch as at least a temporary fix, because distros 
are just today releasing the previous code, and HyperV will start breaking "occasionally", depending 
on whether the 32-bit virtual (fake) PCI domain fits within 16 bits. (If not, then we can rush out a 
driver update to fix it, but there will be a window of time with some breakage there.)


[1] http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf , seciton 6.5.6, page 397

thanks,

--
John Hubbard
NVIDIA

> 
> Thanks,
> - Haiyang
>
Dan Carpenter April 25, 2017, 1 p.m. | #5
On Mon, Apr 24, 2017 at 04:06:37PM -0700, John Hubbard wrote:
> First, a tiny nit about the patch: it would be good to add "Fixing a problem
> that was introduced with commit <4a9b0933bdfc>", in the patch commit
> message.
> 

Please use the Fixes tag.

Fixes: 123456789012 ("blah blah blah")

regards,
dan carpenter
Christoph Hellwig April 25, 2017, 1:14 p.m. | #6
Hi John,

please fix your quoting of the previous mails, thanks!


What ACPI defines does not matter at all.  Linux uses 32-bit domains
IDs, and on x86 specifily uses those for non-ACPI enumarated domains
(e.g. VMD).

You've also not demontrated any issue with any Linux driver yet.

> Also...it would be nice if we could use Haiyang's patch as at least a
> temporary fix, because distros are just today releasing the previous code,
> and HyperV will start breaking "occasionally", depending on whether the
> 32-bit virtual (fake) PCI domain fits within 16 bits. (If not, then we can
> rush out a driver update to fix it, but there will be a window of time with
> some breakage there.)

Just send the fix to whatever driver is broken to the driver maintainer.
But I can't find a single broken driver in the tree, and as you know
nothing else matters for Linux anyway.
John Hubbard April 25, 2017, 6:19 p.m. | #7
On Tue, 25 Apr 2017, Christoph Hellwig wrote:

> Hi John,
> 
> please fix your quoting of the previous mails, thanks!

Shoot, sorry about any quoting issues. I'm sufficiently new to conversing 
on these lists that I'm not even sure which mistake I made.

> 
> 
> What ACPI defines does not matter at all.  Linux uses 32-bit domains
> IDs, and on x86 specifily uses those for non-ACPI enumarated domains
> (e.g. VMD).
> 
> You've also not demontrated any issue with any Linux driver yet.

The NVIDIA out-of-tree driver has historically treated domains as 16-bit. 
So this showed up when people tried to run that driver in a hyper-v VM.

> 
> > Also...it would be nice if we could use Haiyang's patch as at least a
> > temporary fix, because distros are just today releasing the previous code,
> > and HyperV will start breaking "occasionally", depending on whether the
> > 32-bit virtual (fake) PCI domain fits within 16 bits. (If not, then we can
> > rush out a driver update to fix it, but there will be a window of time with
> > some breakage there.)
> 
> Just send the fix to whatever driver is broken to the driver maintainer.

Done: that would be us. :)

> But I can't find a single broken driver in the tree, and as you know
> nothing else matters for Linux anyway.
> 

Yes, I looked at Nouveau, and I see that they allow for a 32-bit domain, 
so I agree that we haven't found any in-tree drivers that have a problem.

Anyway, thanks for the answers and explanations.

--
thanks,
john h

Patch

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index e73880c..b18dff3 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -1334,9 +1334,11 @@  static void put_pcichild(struct hv_pci_dev *hpdev,
 	 * can have shorter names than based on the bus instance UUID.
 	 * Only the first device serial number is used for domain, so the
 	 * domain number will not change after the first device is added.
+	 * The lower 16 bits of the serial number is used, otherwise some
+	 * drivers may not be able to handle it.
 	 */
 	if (list_empty(&hbus->children))
-		hbus->sysdata.domain = desc->ser;
+		hbus->sysdata.domain = desc->ser & 0xFFFF;
 	list_add_tail(&hpdev->list_entry, &hbus->children);
 	spin_unlock_irqrestore(&hbus->device_list_lock, flags);
 	return hpdev;