* [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-15 0:03 ` Sridhar Pitchai 0 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-15 0:03 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Bjorn Helgaas, Jake Oshins, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, KY Srinivasan, Michael Kelley (EOSG), devel@linuxdriverproject.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org V2hlbmV2ZXIgUENJIGJ1cyBpcyBhZGRlZCwgSHlwZXJWIGd1YXJhbnRlZXMgdGhlIEJVUyBpZCBp cyB1bmlxdWUuIEV2ZW4NCndpdGggdGhhdCB3aGVuIGEgZmlyc3QgZGV2aWNlIGlzIGFkZGVkIHRv IHRoZSBidXMsIGl0IG92ZXJyaWRlcyBidXMgZG9tYWluDQpJRCB3aXRoIHRoZSBkZXZpY2Ugc2Vy aWFsIG51bWJlci4gU29tZXRpbWUgdGhpcyBjYW4gcmVzdWx0IGluIEJVUyBJRCBub3QNCmJlaW5n IHVuaXF1ZS4gSW4gdGhpcyBjYXNlLCB3aGVuIFBDSV9CVVMgYW5kIGEgZGV2aWNlIHRvIGJ1cyBp cyBhZGRlZCwgdGhlDQpmaXJzdCBkZXZpY2Ugb3ZlcndyaXRlcyB0aGUgYnVzIGRvbWFpbiBJRCB0 byB0aGUgZGV2aWNlIHNlcmlhbCBudW1iZXIsDQp3aGljaCBpcyAwLiBTaW5jZSB0aGVyZSBleHNp c3QgYSBQQ0kgYnVzIHdpdGggZG9tYWluIElEIDAgYWxyZWFkeSB0aGUgUENJDQpidXMgYWRkaXRp b24gZmFpbHMuIFRoaXMgcGF0Y2ggbWFrZSBzdXJlIHdoZW4gYSBkZXZpY2UgaXMgYWRkZWQgdG8g YSBidXMsDQppdCBuZXZlciB1cGRhdGVkIHRoZSBidXMgZG9tYWluIElELiBTaW5jZSB3ZSBoYXZl IHRoZSB0cmFuc3BhcmVudCBTUklPVg0KbW9kZSBub3csIHRoZSBzaG9ydCBWRiBkZXZpY2UgbmFt ZSBpcyBubyBsb25nZXIgbmVlZGVkLg0KDQpGaXhlczogNGE5YjA5MzNiZGZjKCJQQ0k6aHY6VXNl IGRldmljZSBzZXJpYWwgbnVtYmVyIGFzIFBDSSBkb21haW4iKQ0KQ2M6IHN0YWJsZUB2Z2VyLmtl cm5lbC5vcmcNClNpZ25lZC1vZmYtYnk6IFNyaWRoYXIgUGl0Y2hhaSA8c3JwaXRjaGFAbWljcm9z b2Z0LmNvbT4NCi0tLQ0KDQpDaGFuZ2VzIGluIHYzOg0KKiBmaXggdGhlIGNvbW1pdCBjb21tZW50 LiBbS1kgU3Jpbml2YXNhbiwgTWljaGFlbCBLZWxsZXldDQotLS0NCiBkcml2ZXJzL3BjaS9ob3N0 L3BjaS1oeXBlcnYuYyB8IDExIC0tLS0tLS0tLS0tDQogMSBmaWxlIGNoYW5nZWQsIDExIGRlbGV0 aW9ucygtKQ0KDQpkaWZmIC0tZ2l0IGEvZHJpdmVycy9wY2kvaG9zdC9wY2ktaHlwZXJ2LmMgYi9k cml2ZXJzL3BjaS9ob3N0L3BjaS1oeXBlcnYuYw0KaW5kZXggMmZhZjM4ZS4uYWM2N2U1NiAxMDA2 NDQNCi0tLSBhL2RyaXZlcnMvcGNpL2hvc3QvcGNpLWh5cGVydi5jDQorKysgYi9kcml2ZXJzL3Bj aS9ob3N0L3BjaS1oeXBlcnYuYw0KQEAgLTE1MTgsMTcgKzE1MTgsNiBAQCBzdGF0aWMgc3RydWN0 IGh2X3BjaV9kZXYgKm5ld19wY2ljaGlsZF9kZXZpY2Uoc3RydWN0IGh2X3BjaWJ1c19kZXZpY2Ug KmhidXMsDQogCWdldF9wY2ljaGlsZChocGRldiwgaHZfcGNpZGV2X3JlZl9jaGlsZGxpc3QpOw0K IAlzcGluX2xvY2tfaXJxc2F2ZSgmaGJ1cy0+ZGV2aWNlX2xpc3RfbG9jaywgZmxhZ3MpOw0KIA0K LQkvKg0KLQkgKiBXaGVuIGEgZGV2aWNlIGlzIGJlaW5nIGFkZGVkIHRvIHRoZSBidXMsIHdlIHNl dCB0aGUgUENJIGRvbWFpbg0KLQkgKiBudW1iZXIgdG8gYmUgdGhlIGRldmljZSBzZXJpYWwgbnVt YmVyLCB3aGljaCBpcyBub24temVybyBhbmQNCi0JICogdW5pcXVlIG9uIHRoZSBzYW1lIFZNLiAg VGhlIHNlcmlhbCBudW1iZXJzIHN0YXJ0IHdpdGggMSwgYW5kDQotCSAqIGluY3JlYXNlIGJ5IDEg Zm9yIGVhY2ggZGV2aWNlLiAgU28gZGV2aWNlIG5hbWVzIGluY2x1ZGluZyB0aGlzDQotCSAqIGNh biBoYXZlIHNob3J0ZXIgbmFtZXMgdGhhbiBiYXNlZCBvbiB0aGUgYnVzIGluc3RhbmNlIFVVSUQu DQotCSAqIE9ubHkgdGhlIGZpcnN0IGRldmljZSBzZXJpYWwgbnVtYmVyIGlzIHVzZWQgZm9yIGRv bWFpbiwgc28gdGhlDQotCSAqIGRvbWFpbiBudW1iZXIgd2lsbCBub3QgY2hhbmdlIGFmdGVyIHRo ZSBmaXJzdCBkZXZpY2UgaXMgYWRkZWQuDQotCSAqLw0KLQlpZiAobGlzdF9lbXB0eSgmaGJ1cy0+ Y2hpbGRyZW4pKQ0KLQkJaGJ1cy0+c3lzZGF0YS5kb21haW4gPSBkZXNjLT5zZXI7DQogCWxpc3Rf YWRkX3RhaWwoJmhwZGV2LT5saXN0X2VudHJ5LCAmaGJ1cy0+Y2hpbGRyZW4pOw0KIAlzcGluX3Vu bG9ja19pcnFyZXN0b3JlKCZoYnVzLT5kZXZpY2VfbGlzdF9sb2NrLCBmbGFncyk7DQogCXJldHVy biBocGRldjsNCi0tIA0KMi43LjQgDQoNCg== ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-15 0:03 ` Sridhar Pitchai 0 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-15 0:03 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even with that when a first device is added to the bus, it overrides bus domain ID with the device serial number. Sometime this can result in BUS ID not being unique. In this case, when PCI_BUS and a device to bus is added, the first device overwrites the bus domain ID to the device serial number, which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI bus addition fails. This patch make sure when a device is added to a bus, it never updated the bus domain ID. Since we have the transparent SRIOV mode now, the short VF device name is no longer needed. Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain") Cc: stable@vger.kernel.org Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com> --- Changes in v3: * fix the commit comment. [KY Srinivasan, Michael Kelley] --- drivers/pci/host/pci-hyperv.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c index 2faf38e..ac67e56 100644 --- a/drivers/pci/host/pci-hyperv.c +++ b/drivers/pci/host/pci-hyperv.c @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus, get_pcichild(hpdev, hv_pcidev_ref_childlist); spin_lock_irqsave(&hbus->device_list_lock, flags); - /* - * When a device is being added to the bus, we set the PCI domain - * number to be the device serial number, which is non-zero and - * unique on the same VM. The serial numbers start with 1, and - * increase by 1 for each device. So device names including this - * 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. - */ - if (list_empty(&hbus->children)) - hbus->sysdata.domain = desc->ser; list_add_tail(&hpdev->list_entry, &hbus->children); spin_unlock_irqrestore(&hbus->device_list_lock, flags); return hpdev; -- 2.7.4 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption 2018-03-15 0:03 ` Sridhar Pitchai @ 2018-03-15 12:05 ` Lorenzo Pieralisi -1 siblings, 0 replies; 26+ messages in thread From: Lorenzo Pieralisi @ 2018-03-15 12:05 UTC (permalink / raw) To: Sridhar Pitchai Cc: Bjorn Helgaas, Jake Oshins, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, KY Srinivasan, Michael Kelley (EOSG), devel@linuxdriverproject.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org On Thu, Mar 15, 2018 at 12:03:07AM +0000, Sridhar Pitchai wrote: > Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even "Whenever a PCI bus is added" > with that when a first device is added to the bus, it overrides bus domain > ID with the device serial number. Sometime this can result in BUS ID not Define "Sometime". > being unique. In this case, when PCI_BUS and a device to bus is added, the > first device overwrites the bus domain ID to the device serial number, > which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI s/exsist/exist > bus addition fails. This patch make sure when a device is added to a bus, > it never updated the bus domain ID. s/updated/updates > Since we have the transparent SRIOV mode now, the short VF device name > is no longer needed. I still do not understand what this means and how it is related to the patch below, it may be clear to you, it is not to me, at all. > Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain") Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") I asked you an explicit question. Commit above was added for a reason I assume. This patch implies that kernel has been broken since v4.11 which is almost a year ago and nobody every noticed ? Or there are systems where commit above is _necessary_ and this patch would break them ? I want a detailed explanation that highlights *why* it is safe to apply this patch and send it to stable kernels, commit log above won't do. Thanks, Lorenzo > Cc: stable@vger.kernel.org > Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com> > --- > > Changes in v3: > * fix the commit comment. [KY Srinivasan, Michael Kelley] > --- > drivers/pci/host/pci-hyperv.c | 11 ----------- > 1 file changed, 11 deletions(-) > > diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c > index 2faf38e..ac67e56 100644 > --- a/drivers/pci/host/pci-hyperv.c > +++ b/drivers/pci/host/pci-hyperv.c > @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus, > get_pcichild(hpdev, hv_pcidev_ref_childlist); > spin_lock_irqsave(&hbus->device_list_lock, flags); > > - /* > - * When a device is being added to the bus, we set the PCI domain > - * number to be the device serial number, which is non-zero and > - * unique on the same VM. The serial numbers start with 1, and > - * increase by 1 for each device. So device names including this > - * 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. > - */ > - if (list_empty(&hbus->children)) > - hbus->sysdata.domain = desc->ser; > list_add_tail(&hpdev->list_entry, &hbus->children); > spin_unlock_irqrestore(&hbus->device_list_lock, flags); > return hpdev; > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-15 12:05 ` Lorenzo Pieralisi 0 siblings, 0 replies; 26+ messages in thread From: Lorenzo Pieralisi @ 2018-03-15 12:05 UTC (permalink / raw) To: Sridhar Pitchai Cc: Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org On Thu, Mar 15, 2018 at 12:03:07AM +0000, Sridhar Pitchai wrote: > Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even "Whenever a PCI bus is added" > with that when a first device is added to the bus, it overrides bus domain > ID with the device serial number. Sometime this can result in BUS ID not Define "Sometime". > being unique. In this case, when PCI_BUS and a device to bus is added, the > first device overwrites the bus domain ID to the device serial number, > which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI s/exsist/exist > bus addition fails. This patch make sure when a device is added to a bus, > it never updated the bus domain ID. s/updated/updates > Since we have the transparent SRIOV mode now, the short VF device name > is no longer needed. I still do not understand what this means and how it is related to the patch below, it may be clear to you, it is not to me, at all. > Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain") Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") I asked you an explicit question. Commit above was added for a reason I assume. This patch implies that kernel has been broken since v4.11 which is almost a year ago and nobody every noticed ? Or there are systems where commit above is _necessary_ and this patch would break them ? I want a detailed explanation that highlights *why* it is safe to apply this patch and send it to stable kernels, commit log above won't do. Thanks, Lorenzo > Cc: stable@vger.kernel.org > Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com> > --- > > Changes in v3: > * fix the commit comment. [KY Srinivasan, Michael Kelley] > --- > drivers/pci/host/pci-hyperv.c | 11 ----------- > 1 file changed, 11 deletions(-) > > diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c > index 2faf38e..ac67e56 100644 > --- a/drivers/pci/host/pci-hyperv.c > +++ b/drivers/pci/host/pci-hyperv.c > @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus, > get_pcichild(hpdev, hv_pcidev_ref_childlist); > spin_lock_irqsave(&hbus->device_list_lock, flags); > > - /* > - * When a device is being added to the bus, we set the PCI domain > - * number to be the device serial number, which is non-zero and > - * unique on the same VM. The serial numbers start with 1, and > - * increase by 1 for each device. So device names including this > - * 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. > - */ > - if (list_empty(&hbus->children)) > - hbus->sysdata.domain = desc->ser; > list_add_tail(&hpdev->list_entry, &hbus->children); > spin_unlock_irqrestore(&hbus->device_list_lock, flags); > return hpdev; > -- > 2.7.4 > _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption 2018-03-15 12:05 ` Lorenzo Pieralisi @ 2018-03-15 17:56 ` Sridhar Pitchai -1 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-15 17:56 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Bjorn Helgaas, Jake Oshins, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, KY Srinivasan, Michael Kelley (EOSG), devel@linuxdriverproject.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Hi Lorenzo, Answering the question inline. Kindly let me know if it clarifies. I will send out another patch after we = agree on the clarification. Thanks Sridhar Pitchai -----Original Message----- From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>=20 Sent: Thursday, March 15, 2018 5:05 AM To: Sridhar Pitchai <Sridhar.Pitchai@microsoft.com> Cc: Bjorn Helgaas <bhelgaas@google.com>; Jake Oshins <jakeo@microsoft.com>;= Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@micros= oft.com>; Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.co= m>; Michael Kelley (EOSG) <Michael.H.Kelley@microsoft.com>; devel@linuxdriv= erproject.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption On Thu, Mar 15, 2018 at 12:03:07AM +0000, Sridhar Pitchai wrote: > Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even "Whenever a PCI bus is added" Sridhar>> yes > with that when a first device is added to the bus, it overrides bus domai= n > ID with the device serial number. Sometime this can result in BUS ID not Define "Sometime". Sridhar>> HyperV when it creates a PCI bus it guarantees it provide a uniqu= e ID for it. But, that unique BUS ID is replaced with device serial number.= 0 is a valid device serial number, and if there exists a PCI bus with doma= in ID 0 (Gen 1 version of hyperV VM have this for para virtual devices), th= is will result in PCI bus id not being unique.=20 > being unique. In this case, when PCI_BUS and a device to bus is added, th= e > first device overwrites the bus domain ID to the device serial number, > which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI s/exsist/exist Sridhar>> yes > bus addition fails. This patch make sure when a device is added to a bus, > it never updated the bus domain ID.=20 s/updated/updates Sridhar >> yes > Since we have the transparent SRIOV mode now, the short VF device name > is no longer needed. I still do not understand what this means and how it is related to the patch below, it may be clear to you, it is not to me, at all. Sridhar >> the patch below, was introduced to make the device name small, b= y taking only 16bits of the serial number. Since we are not going to have t= he serial number updated to the BUS id, this has to be removed. > Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain") Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") Sridhr >> yes I asked you an explicit question. Commit above was added for a reason I assume. This patch implies that kernel has been broken since v4.11 which is almost a year ago and nobody every noticed ? Or there are systems where commit above is _necessary_ and this patch would break them ? I want a detailed explanation that highlights *why* it is safe to apply this patch and send it to stable kernels, commit log above won't do. Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modifie= d by the child device when it is added. This cannot produce a unique domain= ID all the time. Here in the bug, we see the collision between the serial = number and already existing PCI bus. The cleaner way is never touch the dom= ain ID provided by hyperV during the PCI bus creation. As long as hyperV ma= ke sure it provides a unique domain ID for the PCI for a VM it will not bre= ak, and HyperV will guarantees that the domain for the PCI bus for a given = VM will be always unique. The original patch was also intending to have a unique domain ID for the PC= I bus, by taking the serial number of the device, but it is not sufficient,= when the device serial number is number which is the domain ID of the exis= ting PCI bus. With the current kernel we can repro this issue by adding a device with a s= erial number matching the existing PCI bus domain id. (in this case that ha= ppens to be zero). Thanks, Lorenzo > Cc: stable@vger.kernel.org > Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com> > --- >=20 > Changes in v3: > * fix the commit comment. [KY Srinivasan, Michael Kelley] > --- > drivers/pci/host/pci-hyperv.c | 11 ----------- > 1 file changed, 11 deletions(-) >=20 > diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.= c > index 2faf38e..ac67e56 100644 > --- a/drivers/pci/host/pci-hyperv.c > +++ b/drivers/pci/host/pci-hyperv.c > @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(stru= ct hv_pcibus_device *hbus, > get_pcichild(hpdev, hv_pcidev_ref_childlist); > spin_lock_irqsave(&hbus->device_list_lock, flags); > =20 > - /* > - * When a device is being added to the bus, we set the PCI domain > - * number to be the device serial number, which is non-zero and > - * unique on the same VM. The serial numbers start with 1, and > - * increase by 1 for each device. So device names including this > - * 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. > - */ > - if (list_empty(&hbus->children)) > - hbus->sysdata.domain =3D desc->ser; > list_add_tail(&hpdev->list_entry, &hbus->children); > spin_unlock_irqrestore(&hbus->device_list_lock, flags); > return hpdev; > --=20 > 2.7.4=20 >=20 ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-15 17:56 ` Sridhar Pitchai 0 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-15 17:56 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org Hi Lorenzo, Answering the question inline. Kindly let me know if it clarifies. I will send out another patch after we agree on the clarification. Thanks Sridhar Pitchai -----Original Message----- From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Sent: Thursday, March 15, 2018 5:05 AM To: Sridhar Pitchai <Sridhar.Pitchai@microsoft.com> Cc: Bjorn Helgaas <bhelgaas@google.com>; Jake Oshins <jakeo@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Michael Kelley (EOSG) <Michael.H.Kelley@microsoft.com>; devel@linuxdriverproject.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption On Thu, Mar 15, 2018 at 12:03:07AM +0000, Sridhar Pitchai wrote: > Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even "Whenever a PCI bus is added" Sridhar>> yes > with that when a first device is added to the bus, it overrides bus domain > ID with the device serial number. Sometime this can result in BUS ID not Define "Sometime". Sridhar>> HyperV when it creates a PCI bus it guarantees it provide a unique ID for it. But, that unique BUS ID is replaced with device serial number. 0 is a valid device serial number, and if there exists a PCI bus with domain ID 0 (Gen 1 version of hyperV VM have this for para virtual devices), this will result in PCI bus id not being unique. > being unique. In this case, when PCI_BUS and a device to bus is added, the > first device overwrites the bus domain ID to the device serial number, > which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI s/exsist/exist Sridhar>> yes > bus addition fails. This patch make sure when a device is added to a bus, > it never updated the bus domain ID. s/updated/updates Sridhar >> yes > Since we have the transparent SRIOV mode now, the short VF device name > is no longer needed. I still do not understand what this means and how it is related to the patch below, it may be clear to you, it is not to me, at all. Sridhar >> the patch below, was introduced to make the device name small, by taking only 16bits of the serial number. Since we are not going to have the serial number updated to the BUS id, this has to be removed. > Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain") Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") Sridhr >> yes I asked you an explicit question. Commit above was added for a reason I assume. This patch implies that kernel has been broken since v4.11 which is almost a year ago and nobody every noticed ? Or there are systems where commit above is _necessary_ and this patch would break them ? I want a detailed explanation that highlights *why* it is safe to apply this patch and send it to stable kernels, commit log above won't do. Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child device when it is added. This cannot produce a unique domain ID all the time. Here in the bug, we see the collision between the serial number and already existing PCI bus. The cleaner way is never touch the domain ID provided by hyperV during the PCI bus creation. As long as hyperV make sure it provides a unique domain ID for the PCI for a VM it will not break, and HyperV will guarantees that the domain for the PCI bus for a given VM will be always unique. The original patch was also intending to have a unique domain ID for the PCI bus, by taking the serial number of the device, but it is not sufficient, when the device serial number is number which is the domain ID of the existing PCI bus. With the current kernel we can repro this issue by adding a device with a serial number matching the existing PCI bus domain id. (in this case that happens to be zero). Thanks, Lorenzo > Cc: stable@vger.kernel.org > Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com> > --- > > Changes in v3: > * fix the commit comment. [KY Srinivasan, Michael Kelley] > --- > drivers/pci/host/pci-hyperv.c | 11 ----------- > 1 file changed, 11 deletions(-) > > diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c > index 2faf38e..ac67e56 100644 > --- a/drivers/pci/host/pci-hyperv.c > +++ b/drivers/pci/host/pci-hyperv.c > @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus, > get_pcichild(hpdev, hv_pcidev_ref_childlist); > spin_lock_irqsave(&hbus->device_list_lock, flags); > > - /* > - * When a device is being added to the bus, we set the PCI domain > - * number to be the device serial number, which is non-zero and > - * unique on the same VM. The serial numbers start with 1, and > - * increase by 1 for each device. So device names including this > - * 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. > - */ > - if (list_empty(&hbus->children)) > - hbus->sysdata.domain = desc->ser; > list_add_tail(&hpdev->list_entry, &hbus->children); > spin_unlock_irqrestore(&hbus->device_list_lock, flags); > return hpdev; > -- > 2.7.4 > _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption 2018-03-15 17:56 ` Sridhar Pitchai @ 2018-03-15 18:24 ` Sridhar Pitchai -1 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-15 18:24 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Bjorn Helgaas, Jake Oshins, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, KY Srinivasan, Michael Kelley (EOSG), devel@linuxdriverproject.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org QXBvbG9naWVzIGZvciBub3QgYWxpZ25pbmcgbXkgcmVwbHksIEkganVzdCByZWFsaXplZCBhZnRl ciBsb29raW5nIGludG8NCnRoZSBtYWlsIGFyY2hpdmUgaW4gTEtNTC4gIEkgaGF2ZSBhbGlnbmVk IHRoZSByZXBsYXkgbm93Lg0KDQoNCk9uIDMvMTUvMTgsIDEwOjU2IEFNLCAiU3JpZGhhciBQaXRj aGFpIiA8U3JpZGhhci5QaXRjaGFpQG1pY3Jvc29mdC5jb20+IHdyb3RlOg0KDQpIaSBMb3Jlbnpv LA0KQW5zd2VyaW5nIHRoZSBxdWVzdGlvbiBpbmxpbmUuDQpLaW5kbHkgbGV0IG1lIGtub3cgaWYg aXQgY2xhcmlmaWVzLiBJIHdpbGwgc2VuZCBvdXQgYW5vdGhlciBwYXRjaCBhZnRlciB3ZSBhZ3Jl ZSBvbiB0aGUgY2xhcmlmaWNhdGlvbi4NCg0KVGhhbmtzDQpTcmlkaGFyIFBpdGNoYWkNCg0KLS0t LS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCkZyb206IExvcmVuem8gUGllcmFsaXNpIDxsb3Jlbnpv LnBpZXJhbGlzaUBhcm0uY29tPg0KU2VudDogVGh1cnNkYXksIE1hcmNoIDE1LCAyMDE4IDU6MDUg QU0NClRvOiBTcmlkaGFyIFBpdGNoYWkgPFNyaWRoYXIuUGl0Y2hhaUBtaWNyb3NvZnQuY29tPg0K Q2M6IEJqb3JuIEhlbGdhYXMgPGJoZWxnYWFzQGdvb2dsZS5jb20+OyBKYWtlIE9zaGlucyA8amFr ZW9AbWljcm9zb2Z0LmNvbT47IEhhaXlhbmcgWmhhbmcgPGhhaXlhbmd6QG1pY3Jvc29mdC5jb20+ OyBTdGVwaGVuIEhlbW1pbmdlciA8c3RoZW1taW5AbWljcm9zb2Z0LmNvbT47IERleHVhbiBDdWkg PGRlY3VpQG1pY3Jvc29mdC5jb20+OyBLWSBTcmluaXZhc2FuIDxreXNAbWljcm9zb2Z0LmNvbT47 IE1pY2hhZWwgS2VsbGV5IChFT1NHKSA8TWljaGFlbC5ILktlbGxleUBtaWNyb3NvZnQuY29tPjsg ZGV2ZWxAbGludXhkcml2ZXJwcm9qZWN0Lm9yZzsgbGludXgtcGNpQHZnZXIua2VybmVsLm9yZzsg bGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZw0KU3ViamVjdDogUmU6IFtQQVRDSCB2M11QQ0k6 IGh2OiBmaXggUENJLUJVUyBkb21haW5JRCBjb3JydXB0aW9uDQoNCk9uIFRodSwgTWFyIDE1LCAy MDE4IGF0IDEyOjAzOjA3QU0gKzAwMDAsIFNyaWRoYXIgUGl0Y2hhaSB3cm90ZToNCldoZW5ldmVy IFBDSSBidXMgaXMgYWRkZWQsIEh5cGVyViBndWFyYW50ZWVzIHRoZSBCVVMgaWQgaXMgdW5pcXVl LiBFdmVuDQoNCiJXaGVuZXZlciBhIFBDSSBidXMgaXMgYWRkZWQiDQpTcmlkaGFyPj4geWVzDQoN CndpdGggdGhhdCB3aGVuIGEgZmlyc3QgZGV2aWNlIGlzIGFkZGVkIHRvIHRoZSBidXMsIGl0IG92 ZXJyaWRlcyBidXMgZG9tYWluDQpJRCB3aXRoIHRoZSBkZXZpY2Ugc2VyaWFsIG51bWJlci4gU29t ZXRpbWUgdGhpcyBjYW4gcmVzdWx0IGluIEJVUyBJRCBub3QNCg0KRGVmaW5lICJTb21ldGltZSIu DQoNClNyaWRoYXI+PiBIeXBlclYgd2hlbiBpdCBjcmVhdGVzIGEgUENJIGJ1cyBpdCBndWFyYW50 ZWVzIGl0IHByb3ZpZGUgYSB1bmlxdWUgSUQgZm9yIGl0Lg0KQnV0LCB0aGF0IHVuaXF1ZSBCVVMg SUQgaXMgcmVwbGFjZWQgd2l0aCBkZXZpY2Ugc2VyaWFsIG51bWJlci4gMCBpcyBhIHZhbGlkDQpk ZXZpY2Ugc2VyaWFsIG51bWJlciwgYW5kIGlmIHRoZXJlIGV4aXN0cyBhIFBDSSBidXMgd2l0aCBk b21haW4gSUQgMCAoR2VuIDENCnZlcnNpb24gb2YgaHlwZXJWIFZNIGhhdmUgdGhpcyBmb3IgcGFy YSB2aXJ0dWFsIGRldmljZXMpLCB0aGlzIHdpbGwgcmVzdWx0IGluDQpQQ0kgYnVzIGlkIG5vdCBi ZWluZyB1bmlxdWUuDQoNCg0KYmVpbmcgdW5pcXVlLiBJbiB0aGlzIGNhc2UsIHdoZW4gUENJX0JV UyBhbmQgYSBkZXZpY2UgdG8gYnVzIGlzIGFkZGVkLCB0aGUNCmZpcnN0IGRldmljZSBvdmVyd3Jp dGVzIHRoZSBidXMgZG9tYWluIElEIHRvIHRoZSBkZXZpY2Ugc2VyaWFsIG51bWJlciwNCndoaWNo IGlzIDAuIFNpbmNlIHRoZXJlIGV4c2lzdCBhIFBDSSBidXMgd2l0aCBkb21haW4gSUQgMCBhbHJl YWR5IHRoZSBQQ0kNCg0Kcy9leHNpc3QvZXhpc3QNCg0KU3JpZGhhcj4+IHllcw0KDQpidXMgYWRk aXRpb24gZmFpbHMuIFRoaXMgcGF0Y2ggbWFrZSBzdXJlIHdoZW4gYSBkZXZpY2UgaXMgYWRkZWQg dG8gYSBidXMsDQppdCBuZXZlciB1cGRhdGVkIHRoZSBidXMgZG9tYWluIElELg0KDQpzL3VwZGF0 ZWQvdXBkYXRlcw0KU3JpZGhhciA+PiB5ZXMNCg0KU2luY2Ugd2UgaGF2ZSB0aGUgdHJhbnNwYXJl bnQgU1JJT1YgbW9kZSBub3csIHRoZSBzaG9ydCBWRiBkZXZpY2UgbmFtZQ0KaXMgbm8gbG9uZ2Vy IG5lZWRlZC4NCg0KSSBzdGlsbCBkbyBub3QgdW5kZXJzdGFuZCB3aGF0IHRoaXMgbWVhbnMgYW5k IGhvdyBpdCBpcyByZWxhdGVkIHRvIHRoZQ0KcGF0Y2ggYmVsb3csIGl0IG1heSBiZSBjbGVhciB0 byB5b3UsIGl0IGlzIG5vdCB0byBtZSwgYXQgYWxsLg0KDQpTcmlkaGFyID4+IHRoZSBwYXRjaCBi ZWxvdywgd2FzIGludHJvZHVjZWQgdG8gbWFrZSB0aGUgZGV2aWNlIG5hbWUgc21hbGwsIGJ5IHRh a2luZyBvbmx5DQoxNmJpdHMgb2YgdGhlIHNlcmlhbCBudW1iZXIuIFNpbmNlIHdlIGFyZSBub3Qg Z29pbmcgdG8gaGF2ZSB0aGUgc2VyaWFsIG51bWJlcg0KdXBkYXRlZCB0byB0aGUgQlVTIGlkLCB0 aGlzIGhhcyB0byBiZSByZW1vdmVkLg0KDQpGaXhlczogNGE5YjA5MzNiZGZjKCJQQ0k6aHY6VXNl IGRldmljZSBzZXJpYWwgbnVtYmVyIGFzIFBDSSBkb21haW4iKQ0KDQpGaXhlczogNGE5YjA5MzNi ZGZjICgiUENJOiBodjogVXNlIGRldmljZSBzZXJpYWwgbnVtYmVyIGFzIFBDSSBkb21haW4iKQ0K U3JpZGhyID4+IHllcw0KDQpJIGFza2VkIHlvdSBhbiBleHBsaWNpdCBxdWVzdGlvbi4gQ29tbWl0 IGFib3ZlIHdhcyBhZGRlZCBmb3IgYSByZWFzb24NCkkgYXNzdW1lLiBUaGlzIHBhdGNoIGltcGxp ZXMgdGhhdCBrZXJuZWwgaGFzIGJlZW4gYnJva2VuIHNpbmNlIHY0LjExDQp3aGljaCBpcyBhbG1v c3QgYSB5ZWFyIGFnbyBhbmQgbm9ib2R5IGV2ZXJ5IG5vdGljZWQgPyBPciB0aGVyZSBhcmUNCnN5 c3RlbXMgd2hlcmUgY29tbWl0IGFib3ZlIGlzIF9uZWNlc3NhcnlfIGFuZCB0aGlzIHBhdGNoIHdv dWxkIGJyZWFrDQp0aGVtID8NCg0KSSB3YW50IGEgZGV0YWlsZWQgZXhwbGFuYXRpb24gdGhhdCBo aWdobGlnaHRzICp3aHkqIGl0IGlzIHNhZmUgdG8gYXBwbHkNCnRoaXMgcGF0Y2ggYW5kIHNlbmQg aXQgdG8gc3RhYmxlIGtlcm5lbHMsIGNvbW1pdCBsb2cgYWJvdmUgd29uJ3QgZG8uDQoNClNyaWRo YXI+PiBIeXBlclYgcHJvdmlkZXMgYSB1bmlxdWUgZG9tYWluIElEIGZvciBQQ0kgQlVTLiBCdXQg aXQgaXMgbW9kaWZpZWQgYnkgdGhlIGNoaWxkDQpkZXZpY2Ugd2hlbiBpdCBpcyBhZGRlZC4gVGhp cyBjYW5ub3QgcHJvZHVjZSBhIHVuaXF1ZSBkb21haW4gSUQgYWxsIHRoZSB0aW1lLg0KSGVyZSBp biB0aGUgYnVnLCB3ZSBzZWUgdGhlIGNvbGxpc2lvbiBiZXR3ZWVuIHRoZSBzZXJpYWwgbnVtYmVy IGFuZCBhbHJlYWR5DQpleGlzdGluZyBQQ0kgYnVzLiBUaGUgY2xlYW5lciB3YXkgaXMgbmV2ZXIg dG91Y2ggdGhlIGRvbWFpbiBJRCBwcm92aWRlZCBieQ0KaHlwZXJWIGR1cmluZyB0aGUgUENJIGJ1 cyBjcmVhdGlvbi4gQXMgbG9uZyBhcyBoeXBlclYgbWFrZSBzdXJlIGl0IHByb3ZpZGVzIGENCnVu aXF1ZSBkb21haW4gSUQgZm9yIHRoZSBQQ0kgZm9yIGEgVk0gaXQgd2lsbCBub3QgYnJlYWssIGFu ZCBIeXBlclYgd2lsbA0KZ3VhcmFudGVlcyB0aGF0IHRoZSBkb21haW4gZm9yIHRoZSBQQ0kgYnVz IGZvciBhIGdpdmVuIFZNIHdpbGwgYmUgYWx3YXlzIHVuaXF1ZS4NClRoZSBvcmlnaW5hbCBwYXRj aCB3YXMgYWxzbyBpbnRlbmRpbmcgdG8gaGF2ZSBhIHVuaXF1ZSBkb21haW4gSUQgZm9yIHRoZSBQ Q0kNCmJ1cywgYnkgdGFraW5nIHRoZSBzZXJpYWwgbnVtYmVyIG9mIHRoZSBkZXZpY2UsIGJ1dCBp dCBpcyBub3Qgc3VmZmljaWVudCwgd2hlbg0KdGhlIGRldmljZSBzZXJpYWwgbnVtYmVyIGlzIG51 bWJlciB3aGljaCBpcyB0aGUgZG9tYWluIElEIG9mIHRoZSBleGlzdGluZyBQQ0kNCmJ1cy4gIFdp dGggdGhlIGN1cnJlbnQga2VybmVsIHdlIGNhbiByZXBybyB0aGlzIGlzc3VlIGJ5IGFkZGluZyBh IGRldmljZSB3aXRoIGENCnNlcmlhbCBudW1iZXIgbWF0Y2hpbmcgdGhlIGV4aXN0aW5nIFBDSSBi dXMgZG9tYWluIGlkLiAoaW4gdGhpcyBjYXNlIHRoYXQNCmhhcHBlbnMgdG8gYmUgemVybykuDQoN Cg0KVGhhbmtzLA0KTG9yZW56bw0KDQpDYzogc3RhYmxlQHZnZXIua2VybmVsLm9yZw0KU2lnbmVk LW9mZi1ieTogU3JpZGhhciBQaXRjaGFpIDxzcnBpdGNoYUBtaWNyb3NvZnQuY29tPg0KLS0tDQpD aGFuZ2VzIGluIHYzOg0KKiBmaXggdGhlIGNvbW1pdCBjb21tZW50LiBbS1kgU3Jpbml2YXNhbiwg TWljaGFlbCBLZWxsZXldDQotLS0NCiAgZHJpdmVycy9wY2kvaG9zdC9wY2ktaHlwZXJ2LmMgfCAx MSAtLS0tLS0tLS0tLQ0KICAxIGZpbGUgY2hhbmdlZCwgMTEgZGVsZXRpb25zKC0pDQpkaWZmIC0t Z2l0IGEvZHJpdmVycy9wY2kvaG9zdC9wY2ktaHlwZXJ2LmMgYi9kcml2ZXJzL3BjaS9ob3N0L3Bj aS1oeXBlcnYuYw0KaW5kZXggMmZhZjM4ZS4uYWM2N2U1NiAxMDA2NDQNCi0tLSBhL2RyaXZlcnMv cGNpL2hvc3QvcGNpLWh5cGVydi5jDQorKysgYi9kcml2ZXJzL3BjaS9ob3N0L3BjaS1oeXBlcnYu Yw0KQEAgLTE1MTgsMTcgKzE1MTgsNiBAQCBzdGF0aWMgc3RydWN0IGh2X3BjaV9kZXYgKm5ld19w Y2ljaGlsZF9kZXZpY2Uoc3RydWN0IGh2X3BjaWJ1c19kZXZpY2UgKmhidXMsDQogIAlnZXRfcGNp Y2hpbGQoaHBkZXYsIGh2X3BjaWRldl9yZWZfY2hpbGRsaXN0KTsNCiAgCXNwaW5fbG9ja19pcnFz YXZlKCZoYnVzLT5kZXZpY2VfbGlzdF9sb2NrLCBmbGFncyk7DQogIA0KLQkvKg0KLQkgKiBXaGVu IGEgZGV2aWNlIGlzIGJlaW5nIGFkZGVkIHRvIHRoZSBidXMsIHdlIHNldCB0aGUgUENJIGRvbWFp bg0KLQkgKiBudW1iZXIgdG8gYmUgdGhlIGRldmljZSBzZXJpYWwgbnVtYmVyLCB3aGljaCBpcyBu b24temVybyBhbmQNCi0JICogdW5pcXVlIG9uIHRoZSBzYW1lIFZNLiAgVGhlIHNlcmlhbCBudW1i ZXJzIHN0YXJ0IHdpdGggMSwgYW5kDQotCSAqIGluY3JlYXNlIGJ5IDEgZm9yIGVhY2ggZGV2aWNl LiAgU28gZGV2aWNlIG5hbWVzIGluY2x1ZGluZyB0aGlzDQotCSAqIGNhbiBoYXZlIHNob3J0ZXIg bmFtZXMgdGhhbiBiYXNlZCBvbiB0aGUgYnVzIGluc3RhbmNlIFVVSUQuDQotCSAqIE9ubHkgdGhl IGZpcnN0IGRldmljZSBzZXJpYWwgbnVtYmVyIGlzIHVzZWQgZm9yIGRvbWFpbiwgc28gdGhlDQot CSAqIGRvbWFpbiBudW1iZXIgd2lsbCBub3QgY2hhbmdlIGFmdGVyIHRoZSBmaXJzdCBkZXZpY2Ug aXMgYWRkZWQuDQotCSAqLw0KLQlpZiAobGlzdF9lbXB0eSgmaGJ1cy0+Y2hpbGRyZW4pKQ0KLQkJ aGJ1cy0+c3lzZGF0YS5kb21haW4gPSBkZXNjLT5zZXI7DQogIAlsaXN0X2FkZF90YWlsKCZocGRl di0+bGlzdF9lbnRyeSwgJmhidXMtPmNoaWxkcmVuKTsNCiAgCXNwaW5fdW5sb2NrX2lycXJlc3Rv cmUoJmhidXMtPmRldmljZV9saXN0X2xvY2ssIGZsYWdzKTsNCiAgCXJldHVybiBocGRldjsNCi0t DQoyLjcuNCANCg0K ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-15 18:24 ` Sridhar Pitchai 0 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-15 18:24 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org Apologies for not aligning my reply, I just realized after looking into the mail archive in LKML. I have aligned the replay now. On 3/15/18, 10:56 AM, "Sridhar Pitchai" <Sridhar.Pitchai@microsoft.com> wrote: Hi Lorenzo, Answering the question inline. Kindly let me know if it clarifies. I will send out another patch after we agree on the clarification. Thanks Sridhar Pitchai -----Original Message----- From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Sent: Thursday, March 15, 2018 5:05 AM To: Sridhar Pitchai <Sridhar.Pitchai@microsoft.com> Cc: Bjorn Helgaas <bhelgaas@google.com>; Jake Oshins <jakeo@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Michael Kelley (EOSG) <Michael.H.Kelley@microsoft.com>; devel@linuxdriverproject.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption On Thu, Mar 15, 2018 at 12:03:07AM +0000, Sridhar Pitchai wrote: Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even "Whenever a PCI bus is added" Sridhar>> yes with that when a first device is added to the bus, it overrides bus domain ID with the device serial number. Sometime this can result in BUS ID not Define "Sometime". Sridhar>> HyperV when it creates a PCI bus it guarantees it provide a unique ID for it. But, that unique BUS ID is replaced with device serial number. 0 is a valid device serial number, and if there exists a PCI bus with domain ID 0 (Gen 1 version of hyperV VM have this for para virtual devices), this will result in PCI bus id not being unique. being unique. In this case, when PCI_BUS and a device to bus is added, the first device overwrites the bus domain ID to the device serial number, which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI s/exsist/exist Sridhar>> yes bus addition fails. This patch make sure when a device is added to a bus, it never updated the bus domain ID. s/updated/updates Sridhar >> yes Since we have the transparent SRIOV mode now, the short VF device name is no longer needed. I still do not understand what this means and how it is related to the patch below, it may be clear to you, it is not to me, at all. Sridhar >> the patch below, was introduced to make the device name small, by taking only 16bits of the serial number. Since we are not going to have the serial number updated to the BUS id, this has to be removed. Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain") Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") Sridhr >> yes I asked you an explicit question. Commit above was added for a reason I assume. This patch implies that kernel has been broken since v4.11 which is almost a year ago and nobody every noticed ? Or there are systems where commit above is _necessary_ and this patch would break them ? I want a detailed explanation that highlights *why* it is safe to apply this patch and send it to stable kernels, commit log above won't do. Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child device when it is added. This cannot produce a unique domain ID all the time. Here in the bug, we see the collision between the serial number and already existing PCI bus. The cleaner way is never touch the domain ID provided by hyperV during the PCI bus creation. As long as hyperV make sure it provides a unique domain ID for the PCI for a VM it will not break, and HyperV will guarantees that the domain for the PCI bus for a given VM will be always unique. The original patch was also intending to have a unique domain ID for the PCI bus, by taking the serial number of the device, but it is not sufficient, when the device serial number is number which is the domain ID of the existing PCI bus. With the current kernel we can repro this issue by adding a device with a serial number matching the existing PCI bus domain id. (in this case that happens to be zero). Thanks, Lorenzo Cc: stable@vger.kernel.org Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com> --- Changes in v3: * fix the commit comment. [KY Srinivasan, Michael Kelley] --- drivers/pci/host/pci-hyperv.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c index 2faf38e..ac67e56 100644 --- a/drivers/pci/host/pci-hyperv.c +++ b/drivers/pci/host/pci-hyperv.c @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus, get_pcichild(hpdev, hv_pcidev_ref_childlist); spin_lock_irqsave(&hbus->device_list_lock, flags); - /* - * When a device is being added to the bus, we set the PCI domain - * number to be the device serial number, which is non-zero and - * unique on the same VM. The serial numbers start with 1, and - * increase by 1 for each device. So device names including this - * 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. - */ - if (list_empty(&hbus->children)) - hbus->sysdata.domain = desc->ser; list_add_tail(&hpdev->list_entry, &hbus->children); spin_unlock_irqrestore(&hbus->device_list_lock, flags); return hpdev; -- 2.7.4 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption 2018-03-15 18:24 ` Sridhar Pitchai @ 2018-03-20 17:56 ` Sridhar Pitchai -1 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-20 17:56 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Bjorn Helgaas, Jake Oshins, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, KY Srinivasan, Michael Kelley (EOSG), devel@linuxdriverproject.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org SGkgTG9yZW56bywNCkFyZSB3ZSBnb29kIHdpdGggdGhlIGV4cGxhbmF0aW9uPyBDYW4gSSBzZW5k IHRoZSBwYXRjaCB3aXRoIHRoZSB1cGRhdGVkIGNvbW1pdCBjb21tZW50cz8NCg0KVGhhbmtzDQpT cmlkaGFyDQoNCu+7v09uIDMvMTUvMTgsIDExOjI0IEFNLCAiU3JpZGhhciBQaXRjaGFpIiA8U3Jp ZGhhci5QaXRjaGFpQG1pY3Jvc29mdC5jb20+IHdyb3RlOg0KDQogICAgQXBvbG9naWVzIGZvciBu b3QgYWxpZ25pbmcgbXkgcmVwbHksIEkganVzdCByZWFsaXplZCBhZnRlciBsb29raW5nIGludG8N CiAgICB0aGUgbWFpbCBhcmNoaXZlIGluIExLTUwuICBJIGhhdmUgYWxpZ25lZCB0aGUgcmVwbGF5 IG5vdy4NCiAgICANCiAgICANCiAgICBPbiAzLzE1LzE4LCAxMDo1NiBBTSwgIlNyaWRoYXIgUGl0 Y2hhaSIgPFNyaWRoYXIuUGl0Y2hhaUBtaWNyb3NvZnQuY29tPiB3cm90ZToNCiAgICANCiAgICBI aSBMb3JlbnpvLA0KICAgIEFuc3dlcmluZyB0aGUgcXVlc3Rpb24gaW5saW5lLg0KICAgIEtpbmRs eSBsZXQgbWUga25vdyBpZiBpdCBjbGFyaWZpZXMuIEkgd2lsbCBzZW5kIG91dCBhbm90aGVyIHBh dGNoIGFmdGVyIHdlIGFncmVlIG9uIHRoZSBjbGFyaWZpY2F0aW9uLg0KICAgIA0KICAgIFRoYW5r cw0KICAgIFNyaWRoYXIgUGl0Y2hhaQ0KICAgIA0KICAgIC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0t LS0tDQogICAgRnJvbTogTG9yZW56byBQaWVyYWxpc2kgPGxvcmVuem8ucGllcmFsaXNpQGFybS5j b20+DQogICAgU2VudDogVGh1cnNkYXksIE1hcmNoIDE1LCAyMDE4IDU6MDUgQU0NCiAgICBUbzog U3JpZGhhciBQaXRjaGFpIDxTcmlkaGFyLlBpdGNoYWlAbWljcm9zb2Z0LmNvbT4NCiAgICBDYzog Qmpvcm4gSGVsZ2FhcyA8YmhlbGdhYXNAZ29vZ2xlLmNvbT47IEpha2UgT3NoaW5zIDxqYWtlb0Bt aWNyb3NvZnQuY29tPjsgSGFpeWFuZyBaaGFuZyA8aGFpeWFuZ3pAbWljcm9zb2Z0LmNvbT47IFN0 ZXBoZW4gSGVtbWluZ2VyIDxzdGhlbW1pbkBtaWNyb3NvZnQuY29tPjsgRGV4dWFuIEN1aSA8ZGVj dWlAbWljcm9zb2Z0LmNvbT47IEtZIFNyaW5pdmFzYW4gPGt5c0BtaWNyb3NvZnQuY29tPjsgTWlj aGFlbCBLZWxsZXkgKEVPU0cpIDxNaWNoYWVsLkguS2VsbGV5QG1pY3Jvc29mdC5jb20+OyBkZXZl bEBsaW51eGRyaXZlcnByb2plY3Qub3JnOyBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBsaW51 eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnDQogICAgU3ViamVjdDogUmU6IFtQQVRDSCB2M11QQ0k6 IGh2OiBmaXggUENJLUJVUyBkb21haW5JRCBjb3JydXB0aW9uDQogICAgDQogICAgT24gVGh1LCBN YXIgMTUsIDIwMTggYXQgMTI6MDM6MDdBTSArMDAwMCwgU3JpZGhhciBQaXRjaGFpIHdyb3RlOg0K ICAgIFdoZW5ldmVyIFBDSSBidXMgaXMgYWRkZWQsIEh5cGVyViBndWFyYW50ZWVzIHRoZSBCVVMg aWQgaXMgdW5pcXVlLiBFdmVuDQogICAgDQogICAgIldoZW5ldmVyIGEgUENJIGJ1cyBpcyBhZGRl ZCINCiAgICBTcmlkaGFyPj4geWVzDQogICAgDQogICAgd2l0aCB0aGF0IHdoZW4gYSBmaXJzdCBk ZXZpY2UgaXMgYWRkZWQgdG8gdGhlIGJ1cywgaXQgb3ZlcnJpZGVzIGJ1cyBkb21haW4NCiAgICBJ RCB3aXRoIHRoZSBkZXZpY2Ugc2VyaWFsIG51bWJlci4gU29tZXRpbWUgdGhpcyBjYW4gcmVzdWx0 IGluIEJVUyBJRCBub3QNCiAgICANCiAgICBEZWZpbmUgIlNvbWV0aW1lIi4NCiAgICANCiAgICBT cmlkaGFyPj4gSHlwZXJWIHdoZW4gaXQgY3JlYXRlcyBhIFBDSSBidXMgaXQgZ3VhcmFudGVlcyBp dCBwcm92aWRlIGEgdW5pcXVlIElEIGZvciBpdC4NCiAgICBCdXQsIHRoYXQgdW5pcXVlIEJVUyBJ RCBpcyByZXBsYWNlZCB3aXRoIGRldmljZSBzZXJpYWwgbnVtYmVyLiAwIGlzIGEgdmFsaWQNCiAg ICBkZXZpY2Ugc2VyaWFsIG51bWJlciwgYW5kIGlmIHRoZXJlIGV4aXN0cyBhIFBDSSBidXMgd2l0 aCBkb21haW4gSUQgMCAoR2VuIDENCiAgICB2ZXJzaW9uIG9mIGh5cGVyViBWTSBoYXZlIHRoaXMg Zm9yIHBhcmEgdmlydHVhbCBkZXZpY2VzKSwgdGhpcyB3aWxsIHJlc3VsdCBpbg0KICAgIFBDSSBi dXMgaWQgbm90IGJlaW5nIHVuaXF1ZS4NCiAgICANCiAgICANCiAgICBiZWluZyB1bmlxdWUuIElu IHRoaXMgY2FzZSwgd2hlbiBQQ0lfQlVTIGFuZCBhIGRldmljZSB0byBidXMgaXMgYWRkZWQsIHRo ZQ0KICAgIGZpcnN0IGRldmljZSBvdmVyd3JpdGVzIHRoZSBidXMgZG9tYWluIElEIHRvIHRoZSBk ZXZpY2Ugc2VyaWFsIG51bWJlciwNCiAgICB3aGljaCBpcyAwLiBTaW5jZSB0aGVyZSBleHNpc3Qg YSBQQ0kgYnVzIHdpdGggZG9tYWluIElEIDAgYWxyZWFkeSB0aGUgUENJDQogICAgDQogICAgcy9l eHNpc3QvZXhpc3QNCiAgICANCiAgICBTcmlkaGFyPj4geWVzDQogICAgDQogICAgYnVzIGFkZGl0 aW9uIGZhaWxzLiBUaGlzIHBhdGNoIG1ha2Ugc3VyZSB3aGVuIGEgZGV2aWNlIGlzIGFkZGVkIHRv IGEgYnVzLA0KICAgIGl0IG5ldmVyIHVwZGF0ZWQgdGhlIGJ1cyBkb21haW4gSUQuDQogICAgDQog ICAgcy91cGRhdGVkL3VwZGF0ZXMNCiAgICBTcmlkaGFyID4+IHllcw0KICAgIA0KICAgIFNpbmNl IHdlIGhhdmUgdGhlIHRyYW5zcGFyZW50IFNSSU9WIG1vZGUgbm93LCB0aGUgc2hvcnQgVkYgZGV2 aWNlIG5hbWUNCiAgICBpcyBubyBsb25nZXIgbmVlZGVkLg0KICAgIA0KICAgIEkgc3RpbGwgZG8g bm90IHVuZGVyc3RhbmQgd2hhdCB0aGlzIG1lYW5zIGFuZCBob3cgaXQgaXMgcmVsYXRlZCB0byB0 aGUNCiAgICBwYXRjaCBiZWxvdywgaXQgbWF5IGJlIGNsZWFyIHRvIHlvdSwgaXQgaXMgbm90IHRv IG1lLCBhdCBhbGwuDQogICAgDQogICAgU3JpZGhhciA+PiB0aGUgcGF0Y2ggYmVsb3csIHdhcyBp bnRyb2R1Y2VkIHRvIG1ha2UgdGhlIGRldmljZSBuYW1lIHNtYWxsLCBieSB0YWtpbmcgb25seQ0K ICAgIDE2Yml0cyBvZiB0aGUgc2VyaWFsIG51bWJlci4gU2luY2Ugd2UgYXJlIG5vdCBnb2luZyB0 byBoYXZlIHRoZSBzZXJpYWwgbnVtYmVyDQogICAgdXBkYXRlZCB0byB0aGUgQlVTIGlkLCB0aGlz IGhhcyB0byBiZSByZW1vdmVkLg0KICAgIA0KICAgIEZpeGVzOiA0YTliMDkzM2JkZmMoIlBDSTpo djpVc2UgZGV2aWNlIHNlcmlhbCBudW1iZXIgYXMgUENJIGRvbWFpbiIpDQogICAgDQogICAgRml4 ZXM6IDRhOWIwOTMzYmRmYyAoIlBDSTogaHY6IFVzZSBkZXZpY2Ugc2VyaWFsIG51bWJlciBhcyBQ Q0kgZG9tYWluIikNCiAgICBTcmlkaHIgPj4geWVzDQogICAgDQogICAgSSBhc2tlZCB5b3UgYW4g ZXhwbGljaXQgcXVlc3Rpb24uIENvbW1pdCBhYm92ZSB3YXMgYWRkZWQgZm9yIGEgcmVhc29uDQog ICAgSSBhc3N1bWUuIFRoaXMgcGF0Y2ggaW1wbGllcyB0aGF0IGtlcm5lbCBoYXMgYmVlbiBicm9r ZW4gc2luY2UgdjQuMTENCiAgICB3aGljaCBpcyBhbG1vc3QgYSB5ZWFyIGFnbyBhbmQgbm9ib2R5 IGV2ZXJ5IG5vdGljZWQgPyBPciB0aGVyZSBhcmUNCiAgICBzeXN0ZW1zIHdoZXJlIGNvbW1pdCBh Ym92ZSBpcyBfbmVjZXNzYXJ5XyBhbmQgdGhpcyBwYXRjaCB3b3VsZCBicmVhaw0KICAgIHRoZW0g Pw0KICAgIA0KICAgIEkgd2FudCBhIGRldGFpbGVkIGV4cGxhbmF0aW9uIHRoYXQgaGlnaGxpZ2h0 cyAqd2h5KiBpdCBpcyBzYWZlIHRvIGFwcGx5DQogICAgdGhpcyBwYXRjaCBhbmQgc2VuZCBpdCB0 byBzdGFibGUga2VybmVscywgY29tbWl0IGxvZyBhYm92ZSB3b24ndCBkby4NCiAgICANCiAgICBT cmlkaGFyPj4gSHlwZXJWIHByb3ZpZGVzIGEgdW5pcXVlIGRvbWFpbiBJRCBmb3IgUENJIEJVUy4g QnV0IGl0IGlzIG1vZGlmaWVkIGJ5IHRoZSBjaGlsZA0KICAgIGRldmljZSB3aGVuIGl0IGlzIGFk ZGVkLiBUaGlzIGNhbm5vdCBwcm9kdWNlIGEgdW5pcXVlIGRvbWFpbiBJRCBhbGwgdGhlIHRpbWUu DQogICAgSGVyZSBpbiB0aGUgYnVnLCB3ZSBzZWUgdGhlIGNvbGxpc2lvbiBiZXR3ZWVuIHRoZSBz ZXJpYWwgbnVtYmVyIGFuZCBhbHJlYWR5DQogICAgZXhpc3RpbmcgUENJIGJ1cy4gVGhlIGNsZWFu ZXIgd2F5IGlzIG5ldmVyIHRvdWNoIHRoZSBkb21haW4gSUQgcHJvdmlkZWQgYnkNCiAgICBoeXBl clYgZHVyaW5nIHRoZSBQQ0kgYnVzIGNyZWF0aW9uLiBBcyBsb25nIGFzIGh5cGVyViBtYWtlIHN1 cmUgaXQgcHJvdmlkZXMgYQ0KICAgIHVuaXF1ZSBkb21haW4gSUQgZm9yIHRoZSBQQ0kgZm9yIGEg Vk0gaXQgd2lsbCBub3QgYnJlYWssIGFuZCBIeXBlclYgd2lsbA0KICAgIGd1YXJhbnRlZXMgdGhh dCB0aGUgZG9tYWluIGZvciB0aGUgUENJIGJ1cyBmb3IgYSBnaXZlbiBWTSB3aWxsIGJlIGFsd2F5 cyB1bmlxdWUuDQogICAgVGhlIG9yaWdpbmFsIHBhdGNoIHdhcyBhbHNvIGludGVuZGluZyB0byBo YXZlIGEgdW5pcXVlIGRvbWFpbiBJRCBmb3IgdGhlIFBDSQ0KICAgIGJ1cywgYnkgdGFraW5nIHRo ZSBzZXJpYWwgbnVtYmVyIG9mIHRoZSBkZXZpY2UsIGJ1dCBpdCBpcyBub3Qgc3VmZmljaWVudCwg d2hlbg0KICAgIHRoZSBkZXZpY2Ugc2VyaWFsIG51bWJlciBpcyBudW1iZXIgd2hpY2ggaXMgdGhl IGRvbWFpbiBJRCBvZiB0aGUgZXhpc3RpbmcgUENJDQogICAgYnVzLiAgV2l0aCB0aGUgY3VycmVu dCBrZXJuZWwgd2UgY2FuIHJlcHJvIHRoaXMgaXNzdWUgYnkgYWRkaW5nIGEgZGV2aWNlIHdpdGgg YQ0KICAgIHNlcmlhbCBudW1iZXIgbWF0Y2hpbmcgdGhlIGV4aXN0aW5nIFBDSSBidXMgZG9tYWlu IGlkLiAoaW4gdGhpcyBjYXNlIHRoYXQNCiAgICBoYXBwZW5zIHRvIGJlIHplcm8pLg0KICAgIA0K ICAgIA0KICAgIFRoYW5rcywNCiAgICBMb3JlbnpvDQogICAgDQogICAgQ2M6IHN0YWJsZUB2Z2Vy Lmtlcm5lbC5vcmcNCiAgICBTaWduZWQtb2ZmLWJ5OiBTcmlkaGFyIFBpdGNoYWkgPHNycGl0Y2hh QG1pY3Jvc29mdC5jb20+DQogICAgLS0tDQogICAgQ2hhbmdlcyBpbiB2MzoNCiAgICAqIGZpeCB0 aGUgY29tbWl0IGNvbW1lbnQuIFtLWSBTcmluaXZhc2FuLCBNaWNoYWVsIEtlbGxleV0NCiAgICAt LS0NCiAgICAgIGRyaXZlcnMvcGNpL2hvc3QvcGNpLWh5cGVydi5jIHwgMTEgLS0tLS0tLS0tLS0N CiAgICAgIDEgZmlsZSBjaGFuZ2VkLCAxMSBkZWxldGlvbnMoLSkNCiAgICBkaWZmIC0tZ2l0IGEv ZHJpdmVycy9wY2kvaG9zdC9wY2ktaHlwZXJ2LmMgYi9kcml2ZXJzL3BjaS9ob3N0L3BjaS1oeXBl cnYuYw0KICAgIGluZGV4IDJmYWYzOGUuLmFjNjdlNTYgMTAwNjQ0DQogICAgLS0tIGEvZHJpdmVy cy9wY2kvaG9zdC9wY2ktaHlwZXJ2LmMNCiAgICArKysgYi9kcml2ZXJzL3BjaS9ob3N0L3BjaS1o eXBlcnYuYw0KICAgIEBAIC0xNTE4LDE3ICsxNTE4LDYgQEAgc3RhdGljIHN0cnVjdCBodl9wY2lf ZGV2ICpuZXdfcGNpY2hpbGRfZGV2aWNlKHN0cnVjdCBodl9wY2lidXNfZGV2aWNlICpoYnVzLA0K ICAgICAgCWdldF9wY2ljaGlsZChocGRldiwgaHZfcGNpZGV2X3JlZl9jaGlsZGxpc3QpOw0KICAg ICAgCXNwaW5fbG9ja19pcnFzYXZlKCZoYnVzLT5kZXZpY2VfbGlzdF9sb2NrLCBmbGFncyk7DQog ICAgICANCiAgICAtCS8qDQogICAgLQkgKiBXaGVuIGEgZGV2aWNlIGlzIGJlaW5nIGFkZGVkIHRv IHRoZSBidXMsIHdlIHNldCB0aGUgUENJIGRvbWFpbg0KICAgIC0JICogbnVtYmVyIHRvIGJlIHRo ZSBkZXZpY2Ugc2VyaWFsIG51bWJlciwgd2hpY2ggaXMgbm9uLXplcm8gYW5kDQogICAgLQkgKiB1 bmlxdWUgb24gdGhlIHNhbWUgVk0uICBUaGUgc2VyaWFsIG51bWJlcnMgc3RhcnQgd2l0aCAxLCBh bmQNCiAgICAtCSAqIGluY3JlYXNlIGJ5IDEgZm9yIGVhY2ggZGV2aWNlLiAgU28gZGV2aWNlIG5h bWVzIGluY2x1ZGluZyB0aGlzDQogICAgLQkgKiBjYW4gaGF2ZSBzaG9ydGVyIG5hbWVzIHRoYW4g YmFzZWQgb24gdGhlIGJ1cyBpbnN0YW5jZSBVVUlELg0KICAgIC0JICogT25seSB0aGUgZmlyc3Qg ZGV2aWNlIHNlcmlhbCBudW1iZXIgaXMgdXNlZCBmb3IgZG9tYWluLCBzbyB0aGUNCiAgICAtCSAq IGRvbWFpbiBudW1iZXIgd2lsbCBub3QgY2hhbmdlIGFmdGVyIHRoZSBmaXJzdCBkZXZpY2UgaXMg YWRkZWQuDQogICAgLQkgKi8NCiAgICAtCWlmIChsaXN0X2VtcHR5KCZoYnVzLT5jaGlsZHJlbikp DQogICAgLQkJaGJ1cy0+c3lzZGF0YS5kb21haW4gPSBkZXNjLT5zZXI7DQogICAgICAJbGlzdF9h ZGRfdGFpbCgmaHBkZXYtPmxpc3RfZW50cnksICZoYnVzLT5jaGlsZHJlbik7DQogICAgICAJc3Bp bl91bmxvY2tfaXJxcmVzdG9yZSgmaGJ1cy0+ZGV2aWNlX2xpc3RfbG9jaywgZmxhZ3MpOw0KICAg ICAgCXJldHVybiBocGRldjsNCiAgICAtLQ0KICAgIDIuNy40IA0KICAgIA0KICAgIA0KDQo= ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-20 17:56 ` Sridhar Pitchai 0 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-20 17:56 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org Hi Lorenzo, Are we good with the explanation? Can I send the patch with the updated commit comments? Thanks Sridhar On 3/15/18, 11:24 AM, "Sridhar Pitchai" <Sridhar.Pitchai@microsoft.com> wrote: Apologies for not aligning my reply, I just realized after looking into the mail archive in LKML. I have aligned the replay now. On 3/15/18, 10:56 AM, "Sridhar Pitchai" <Sridhar.Pitchai@microsoft.com> wrote: Hi Lorenzo, Answering the question inline. Kindly let me know if it clarifies. I will send out another patch after we agree on the clarification. Thanks Sridhar Pitchai -----Original Message----- From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Sent: Thursday, March 15, 2018 5:05 AM To: Sridhar Pitchai <Sridhar.Pitchai@microsoft.com> Cc: Bjorn Helgaas <bhelgaas@google.com>; Jake Oshins <jakeo@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Michael Kelley (EOSG) <Michael.H.Kelley@microsoft.com>; devel@linuxdriverproject.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption On Thu, Mar 15, 2018 at 12:03:07AM +0000, Sridhar Pitchai wrote: Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even "Whenever a PCI bus is added" Sridhar>> yes with that when a first device is added to the bus, it overrides bus domain ID with the device serial number. Sometime this can result in BUS ID not Define "Sometime". Sridhar>> HyperV when it creates a PCI bus it guarantees it provide a unique ID for it. But, that unique BUS ID is replaced with device serial number. 0 is a valid device serial number, and if there exists a PCI bus with domain ID 0 (Gen 1 version of hyperV VM have this for para virtual devices), this will result in PCI bus id not being unique. being unique. In this case, when PCI_BUS and a device to bus is added, the first device overwrites the bus domain ID to the device serial number, which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI s/exsist/exist Sridhar>> yes bus addition fails. This patch make sure when a device is added to a bus, it never updated the bus domain ID. s/updated/updates Sridhar >> yes Since we have the transparent SRIOV mode now, the short VF device name is no longer needed. I still do not understand what this means and how it is related to the patch below, it may be clear to you, it is not to me, at all. Sridhar >> the patch below, was introduced to make the device name small, by taking only 16bits of the serial number. Since we are not going to have the serial number updated to the BUS id, this has to be removed. Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain") Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") Sridhr >> yes I asked you an explicit question. Commit above was added for a reason I assume. This patch implies that kernel has been broken since v4.11 which is almost a year ago and nobody every noticed ? Or there are systems where commit above is _necessary_ and this patch would break them ? I want a detailed explanation that highlights *why* it is safe to apply this patch and send it to stable kernels, commit log above won't do. Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child device when it is added. This cannot produce a unique domain ID all the time. Here in the bug, we see the collision between the serial number and already existing PCI bus. The cleaner way is never touch the domain ID provided by hyperV during the PCI bus creation. As long as hyperV make sure it provides a unique domain ID for the PCI for a VM it will not break, and HyperV will guarantees that the domain for the PCI bus for a given VM will be always unique. The original patch was also intending to have a unique domain ID for the PCI bus, by taking the serial number of the device, but it is not sufficient, when the device serial number is number which is the domain ID of the existing PCI bus. With the current kernel we can repro this issue by adding a device with a serial number matching the existing PCI bus domain id. (in this case that happens to be zero). Thanks, Lorenzo Cc: stable@vger.kernel.org Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com> --- Changes in v3: * fix the commit comment. [KY Srinivasan, Michael Kelley] --- drivers/pci/host/pci-hyperv.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c index 2faf38e..ac67e56 100644 --- a/drivers/pci/host/pci-hyperv.c +++ b/drivers/pci/host/pci-hyperv.c @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus, get_pcichild(hpdev, hv_pcidev_ref_childlist); spin_lock_irqsave(&hbus->device_list_lock, flags); - /* - * When a device is being added to the bus, we set the PCI domain - * number to be the device serial number, which is non-zero and - * unique on the same VM. The serial numbers start with 1, and - * increase by 1 for each device. So device names including this - * 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. - */ - if (list_empty(&hbus->children)) - hbus->sysdata.domain = desc->ser; list_add_tail(&hpdev->list_entry, &hbus->children); spin_unlock_irqrestore(&hbus->device_list_lock, flags); return hpdev; -- 2.7.4 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption 2018-03-20 17:56 ` Sridhar Pitchai @ 2018-03-20 18:32 ` Lorenzo Pieralisi -1 siblings, 0 replies; 26+ messages in thread From: Lorenzo Pieralisi @ 2018-03-20 18:32 UTC (permalink / raw) To: Sridhar Pitchai Cc: Bjorn Helgaas, Jake Oshins, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, KY Srinivasan, Michael Kelley (EOSG), devel@linuxdriverproject.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org On Tue, Mar 20, 2018 at 05:56:15PM +0000, Sridhar Pitchai wrote: > Hi Lorenzo, > Are we good with the explanation? Can I send the patch with the > updated commit comments? Almost. [...] > Since we have the transparent SRIOV mode now, the short VF device name > is no longer needed. Can you correlate transparent SRIOV mode to the point you are making below ? Please explain what transparent SRIOV mode allows you to remove and why. The rest of the explanation seems OK. Please follow this email format: http://vger.kernel.org/lkml/#s3-9 Thanks, Lorenzo > I still do not understand what this means and how it is related to the > patch below, it may be clear to you, it is not to me, at all. > > Sridhar >> the patch below, was introduced to make the device name small, by taking only > 16bits of the serial number. Since we are not going to have the serial number > updated to the BUS id, this has to be removed. > > Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain") > > Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") > Sridhr >> yes > > I asked you an explicit question. Commit above was added for a reason > I assume. This patch implies that kernel has been broken since v4.11 > which is almost a year ago and nobody every noticed ? Or there are > systems where commit above is _necessary_ and this patch would break > them ? > > I want a detailed explanation that highlights *why* it is safe to apply > this patch and send it to stable kernels, commit log above won't do. > > Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child > device when it is added. This cannot produce a unique domain ID all the time. > Here in the bug, we see the collision between the serial number and already > existing PCI bus. The cleaner way is never touch the domain ID provided by > hyperV during the PCI bus creation. As long as hyperV make sure it provides a > unique domain ID for the PCI for a VM it will not break, and HyperV will > guarantees that the domain for the PCI bus for a given VM will be always unique. > The original patch was also intending to have a unique domain ID for the PCI > bus, by taking the serial number of the device, but it is not sufficient, when > the device serial number is number which is the domain ID of the existing PCI > bus. With the current kernel we can repro this issue by adding a device with a > serial number matching the existing PCI bus domain id. (in this case that > happens to be zero). > > > Thanks, > Lorenzo > > Cc: stable@vger.kernel.org > Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com> > --- > Changes in v3: > * fix the commit comment. [KY Srinivasan, Michael Kelley] > --- > drivers/pci/host/pci-hyperv.c | 11 ----------- > 1 file changed, 11 deletions(-) > diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c > index 2faf38e..ac67e56 100644 > --- a/drivers/pci/host/pci-hyperv.c > +++ b/drivers/pci/host/pci-hyperv.c > @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus, > get_pcichild(hpdev, hv_pcidev_ref_childlist); > spin_lock_irqsave(&hbus->device_list_lock, flags); > > - /* > - * When a device is being added to the bus, we set the PCI domain > - * number to be the device serial number, which is non-zero and > - * unique on the same VM. The serial numbers start with 1, and > - * increase by 1 for each device. So device names including this > - * 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. > - */ > - if (list_empty(&hbus->children)) > - hbus->sysdata.domain = desc->ser; > list_add_tail(&hpdev->list_entry, &hbus->children); > spin_unlock_irqrestore(&hbus->device_list_lock, flags); > return hpdev; > -- > 2.7.4 > > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-20 18:32 ` Lorenzo Pieralisi 0 siblings, 0 replies; 26+ messages in thread From: Lorenzo Pieralisi @ 2018-03-20 18:32 UTC (permalink / raw) To: Sridhar Pitchai Cc: Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org On Tue, Mar 20, 2018 at 05:56:15PM +0000, Sridhar Pitchai wrote: > Hi Lorenzo, > Are we good with the explanation? Can I send the patch with the > updated commit comments? Almost. [...] > Since we have the transparent SRIOV mode now, the short VF device name > is no longer needed. Can you correlate transparent SRIOV mode to the point you are making below ? Please explain what transparent SRIOV mode allows you to remove and why. The rest of the explanation seems OK. Please follow this email format: http://vger.kernel.org/lkml/#s3-9 Thanks, Lorenzo > I still do not understand what this means and how it is related to the > patch below, it may be clear to you, it is not to me, at all. > > Sridhar >> the patch below, was introduced to make the device name small, by taking only > 16bits of the serial number. Since we are not going to have the serial number > updated to the BUS id, this has to be removed. > > Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain") > > Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") > Sridhr >> yes > > I asked you an explicit question. Commit above was added for a reason > I assume. This patch implies that kernel has been broken since v4.11 > which is almost a year ago and nobody every noticed ? Or there are > systems where commit above is _necessary_ and this patch would break > them ? > > I want a detailed explanation that highlights *why* it is safe to apply > this patch and send it to stable kernels, commit log above won't do. > > Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child > device when it is added. This cannot produce a unique domain ID all the time. > Here in the bug, we see the collision between the serial number and already > existing PCI bus. The cleaner way is never touch the domain ID provided by > hyperV during the PCI bus creation. As long as hyperV make sure it provides a > unique domain ID for the PCI for a VM it will not break, and HyperV will > guarantees that the domain for the PCI bus for a given VM will be always unique. > The original patch was also intending to have a unique domain ID for the PCI > bus, by taking the serial number of the device, but it is not sufficient, when > the device serial number is number which is the domain ID of the existing PCI > bus. With the current kernel we can repro this issue by adding a device with a > serial number matching the existing PCI bus domain id. (in this case that > happens to be zero). > > > Thanks, > Lorenzo > > Cc: stable@vger.kernel.org > Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com> > --- > Changes in v3: > * fix the commit comment. [KY Srinivasan, Michael Kelley] > --- > drivers/pci/host/pci-hyperv.c | 11 ----------- > 1 file changed, 11 deletions(-) > diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c > index 2faf38e..ac67e56 100644 > --- a/drivers/pci/host/pci-hyperv.c > +++ b/drivers/pci/host/pci-hyperv.c > @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus, > get_pcichild(hpdev, hv_pcidev_ref_childlist); > spin_lock_irqsave(&hbus->device_list_lock, flags); > > - /* > - * When a device is being added to the bus, we set the PCI domain > - * number to be the device serial number, which is non-zero and > - * unique on the same VM. The serial numbers start with 1, and > - * increase by 1 for each device. So device names including this > - * 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. > - */ > - if (list_empty(&hbus->children)) > - hbus->sysdata.domain = desc->ser; > list_add_tail(&hpdev->list_entry, &hbus->children); > spin_unlock_irqrestore(&hbus->device_list_lock, flags); > return hpdev; > -- > 2.7.4 > > > _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption 2018-03-20 18:32 ` Lorenzo Pieralisi @ 2018-03-20 23:00 ` Sridhar Pitchai -1 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-20 23:00 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Bjorn Helgaas, Jake Oshins, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, KY Srinivasan, Michael Kelley (EOSG), devel@linuxdriverproject.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org SGkgTG9yZW56bywNCiAgIFRyYW5zcGFyZW50IFNSSU9WIGlzIGV4cG9zaW5nIHRoZSBOSUMgZGly ZWN0bHkgdG8gdGhlIGtlcm5lbCB2aWENCnBhcmEtdmlydHVhbCBkZXZpY2UsIHVubGlrZSBjcmVh dGluZyBhIG5ldGRldiBhbmQgYXNzb2NpYXRpbmcgaXQgd2l0aCB0aGUgYm9uZA0KZHJpdmVyLiBG dXJ0aGVyIGRlc2NyaXB0aW9ucyBoZXJlLA0KICAgIGh0dHBzOi8vZ2l0Lmtlcm5lbC5vcmcvcHVi L3NjbS9saW51eC9rZXJuZWwvZ2l0L25leHQvbGludXgtbmV4dC5naXQvY29tbWl0Lz9pZD0wYzE5 NTU2N2E4ZjZlODJlYTU1MzVjZDlmMWQ1NGExNjI2ZGQyMzNlDQoNClByZXZpb3VzbHksIHdoZW4g dXNpbmcgdGhlIGJvbmQgZHJpdmVyLCB1bmlxdWUgYW5kIHBlcnNpc3RlbnQgVkYgTklDIG5hbWUN CndhcyByZXF1aXJlZCwgc28gd2UgdXNlZCBzZXJpYWwgbnVtYmVyIGFzIFBDSSBkb21haW4gd2hp Y2ggaXMgaW5jbHVkZWQgYXMNCnBhcnQgb2YgdGhlIFZGIE5JQyBuYW1lLiAgVHJhbnNwYXJlbnQg U1JJT1YgbW9kZSBwdXRzIFZGIE5JQyBiYXNlZCBvbiBNQUMNCm1hdGNoIGFzIGEgc2xhdmUgb2Yg c3ludGhldGljIE5JQywgc28gVkYgTklD4oCZcyBuYW1lIGlzIG5vIGxvbmdlciBpbXBvcnRhbnQu DQoNClRoYW5rcywNClNyaWRoYXINCg0K77u/T24gMy8yMC8xOCwgMTE6MzIgQU0sICJMb3Jlbnpv IFBpZXJhbGlzaSIgPGxvcmVuem8ucGllcmFsaXNpQGFybS5jb20+IHdyb3RlOg0KDQogICAgT24g VHVlLCBNYXIgMjAsIDIwMTggYXQgMDU6NTY6MTVQTSArMDAwMCwgU3JpZGhhciBQaXRjaGFpIHdy b3RlOg0KICAgID4gSGkgTG9yZW56bywNCiAgICANCiAgICA+IEFyZSB3ZSBnb29kIHdpdGggdGhl IGV4cGxhbmF0aW9uPyBDYW4gSSBzZW5kIHRoZSBwYXRjaCB3aXRoIHRoZQ0KICAgID4gdXBkYXRl ZCBjb21taXQgY29tbWVudHM/DQogICAgDQogICAgQWxtb3N0Lg0KICAgIA0KICAgIFsuLi5dDQog ICAgDQogICAgPiAgICAgU2luY2Ugd2UgaGF2ZSB0aGUgdHJhbnNwYXJlbnQgU1JJT1YgbW9kZSBu b3csIHRoZSBzaG9ydCBWRiBkZXZpY2UgbmFtZQ0KICAgID4gICAgIGlzIG5vIGxvbmdlciBuZWVk ZWQuDQogICAgDQogICAgQ2FuIHlvdSBjb3JyZWxhdGUgdHJhbnNwYXJlbnQgU1JJT1YgbW9kZSB0 byB0aGUgcG9pbnQgeW91IGFyZSBtYWtpbmcNCiAgICBiZWxvdyA/IFBsZWFzZSBleHBsYWluIHdo YXQgdHJhbnNwYXJlbnQgU1JJT1YgbW9kZSBhbGxvd3MgeW91IHRvIHJlbW92ZQ0KICAgIGFuZCB3 aHkuIFRoZSByZXN0IG9mIHRoZSBleHBsYW5hdGlvbiBzZWVtcyBPSy4NCiAgICANCiAgICBQbGVh c2UgZm9sbG93IHRoaXMgZW1haWwgZm9ybWF0Og0KICAgIA0KICAgIGh0dHBzOi8vbmEwMS5zYWZl bGlua3MucHJvdGVjdGlvbi5vdXRsb29rLmNvbS8/dXJsPWh0dHAlM0ElMkYlMkZ2Z2VyLmtlcm5l bC5vcmclMkZsa21sJTJGJTIzczMtOSZkYXRhPTA0JTdDMDElN0NTcmlkaGFyLlBpdGNoYWklNDBt aWNyb3NvZnQuY29tJTdDYzVjZGNiNzk1MWY2NDMxOGU1MjcwOGQ1OGU5MGU2ZjIlN0M3MmY5ODhi Zjg2ZjE0MWFmOTFhYjJkN2NkMDExZGI0NyU3QzElN0MwJTdDNjM2NTcxNjc1MzY2MTgxNzM4JTdD VW5rbm93biU3Q1RXRnBiR1pzYjNkOGV5SldJam9pTUM0d0xqQXdNREFpTENKUUlqb2lWMmx1TXpJ aUxDSkJUaUk2SWsxaGFXd2lmUSUzRCUzRCU3Qy0xJnNkYXRhPXlCZHFjNE5RWnNPN085dmZnSnNy NW9sVThHZkxOakY1ZTlFQWFDYjd2cTQlM0QmcmVzZXJ2ZWQ9MA0KICAgIA0KICAgIFRoYW5rcywN CiAgICBMb3JlbnpvDQogICAgDQogICAgPiAgICAgSSBzdGlsbCBkbyBub3QgdW5kZXJzdGFuZCB3 aGF0IHRoaXMgbWVhbnMgYW5kIGhvdyBpdCBpcyByZWxhdGVkIHRvIHRoZQ0KICAgID4gICAgIHBh dGNoIGJlbG93LCBpdCBtYXkgYmUgY2xlYXIgdG8geW91LCBpdCBpcyBub3QgdG8gbWUsIGF0IGFs bC4NCiAgICA+ICAgICANCiAgICA+ICAgICBTcmlkaGFyID4+IHRoZSBwYXRjaCBiZWxvdywgd2Fz IGludHJvZHVjZWQgdG8gbWFrZSB0aGUgZGV2aWNlIG5hbWUgc21hbGwsIGJ5IHRha2luZyBvbmx5 DQogICAgPiAgICAgMTZiaXRzIG9mIHRoZSBzZXJpYWwgbnVtYmVyLiBTaW5jZSB3ZSBhcmUgbm90 IGdvaW5nIHRvIGhhdmUgdGhlIHNlcmlhbCBudW1iZXINCiAgICA+ICAgICB1cGRhdGVkIHRvIHRo ZSBCVVMgaWQsIHRoaXMgaGFzIHRvIGJlIHJlbW92ZWQuDQogICAgPiAgICAgDQogICAgPiAgICAg Rml4ZXM6IDRhOWIwOTMzYmRmYygiUENJOmh2OlVzZSBkZXZpY2Ugc2VyaWFsIG51bWJlciBhcyBQ Q0kgZG9tYWluIikNCiAgICA+ICAgICANCiAgICA+ICAgICBGaXhlczogNGE5YjA5MzNiZGZjICgi UENJOiBodjogVXNlIGRldmljZSBzZXJpYWwgbnVtYmVyIGFzIFBDSSBkb21haW4iKQ0KICAgID4g ICAgIFNyaWRociA+PiB5ZXMNCiAgICA+ICAgICANCiAgICA+ICAgICBJIGFza2VkIHlvdSBhbiBl eHBsaWNpdCBxdWVzdGlvbi4gQ29tbWl0IGFib3ZlIHdhcyBhZGRlZCBmb3IgYSByZWFzb24NCiAg ICA+ICAgICBJIGFzc3VtZS4gVGhpcyBwYXRjaCBpbXBsaWVzIHRoYXQga2VybmVsIGhhcyBiZWVu IGJyb2tlbiBzaW5jZSB2NC4xMQ0KICAgID4gICAgIHdoaWNoIGlzIGFsbW9zdCBhIHllYXIgYWdv IGFuZCBub2JvZHkgZXZlcnkgbm90aWNlZCA/IE9yIHRoZXJlIGFyZQ0KICAgID4gICAgIHN5c3Rl bXMgd2hlcmUgY29tbWl0IGFib3ZlIGlzIF9uZWNlc3NhcnlfIGFuZCB0aGlzIHBhdGNoIHdvdWxk IGJyZWFrDQogICAgPiAgICAgdGhlbSA/DQogICAgPiAgICAgDQogICAgPiAgICAgSSB3YW50IGEg ZGV0YWlsZWQgZXhwbGFuYXRpb24gdGhhdCBoaWdobGlnaHRzICp3aHkqIGl0IGlzIHNhZmUgdG8g YXBwbHkNCiAgICA+ICAgICB0aGlzIHBhdGNoIGFuZCBzZW5kIGl0IHRvIHN0YWJsZSBrZXJuZWxz LCBjb21taXQgbG9nIGFib3ZlIHdvbid0IGRvLg0KICAgID4gICAgIA0KICAgID4gICAgIFNyaWRo YXI+PiBIeXBlclYgcHJvdmlkZXMgYSB1bmlxdWUgZG9tYWluIElEIGZvciBQQ0kgQlVTLiBCdXQg aXQgaXMgbW9kaWZpZWQgYnkgdGhlIGNoaWxkDQogICAgPiAgICAgZGV2aWNlIHdoZW4gaXQgaXMg YWRkZWQuIFRoaXMgY2Fubm90IHByb2R1Y2UgYSB1bmlxdWUgZG9tYWluIElEIGFsbCB0aGUgdGlt ZS4NCiAgICA+ICAgICBIZXJlIGluIHRoZSBidWcsIHdlIHNlZSB0aGUgY29sbGlzaW9uIGJldHdl ZW4gdGhlIHNlcmlhbCBudW1iZXIgYW5kIGFscmVhZHkNCiAgICA+ICAgICBleGlzdGluZyBQQ0kg YnVzLiBUaGUgY2xlYW5lciB3YXkgaXMgbmV2ZXIgdG91Y2ggdGhlIGRvbWFpbiBJRCBwcm92aWRl ZCBieQ0KICAgID4gICAgIGh5cGVyViBkdXJpbmcgdGhlIFBDSSBidXMgY3JlYXRpb24uIEFzIGxv bmcgYXMgaHlwZXJWIG1ha2Ugc3VyZSBpdCBwcm92aWRlcyBhDQogICAgPiAgICAgdW5pcXVlIGRv bWFpbiBJRCBmb3IgdGhlIFBDSSBmb3IgYSBWTSBpdCB3aWxsIG5vdCBicmVhaywgYW5kIEh5cGVy ViB3aWxsDQogICAgPiAgICAgZ3VhcmFudGVlcyB0aGF0IHRoZSBkb21haW4gZm9yIHRoZSBQQ0kg YnVzIGZvciBhIGdpdmVuIFZNIHdpbGwgYmUgYWx3YXlzIHVuaXF1ZS4NCiAgICA+ICAgICBUaGUg b3JpZ2luYWwgcGF0Y2ggd2FzIGFsc28gaW50ZW5kaW5nIHRvIGhhdmUgYSB1bmlxdWUgZG9tYWlu IElEIGZvciB0aGUgUENJDQogICAgPiAgICAgYnVzLCBieSB0YWtpbmcgdGhlIHNlcmlhbCBudW1i ZXIgb2YgdGhlIGRldmljZSwgYnV0IGl0IGlzIG5vdCBzdWZmaWNpZW50LCB3aGVuDQogICAgPiAg ICAgdGhlIGRldmljZSBzZXJpYWwgbnVtYmVyIGlzIG51bWJlciB3aGljaCBpcyB0aGUgZG9tYWlu IElEIG9mIHRoZSBleGlzdGluZyBQQ0kNCiAgICA+ICAgICBidXMuICBXaXRoIHRoZSBjdXJyZW50 IGtlcm5lbCB3ZSBjYW4gcmVwcm8gdGhpcyBpc3N1ZSBieSBhZGRpbmcgYSBkZXZpY2Ugd2l0aCBh DQogICAgPiAgICAgc2VyaWFsIG51bWJlciBtYXRjaGluZyB0aGUgZXhpc3RpbmcgUENJIGJ1cyBk b21haW4gaWQuIChpbiB0aGlzIGNhc2UgdGhhdA0KICAgID4gICAgIGhhcHBlbnMgdG8gYmUgemVy bykuDQogICAgPiAgICAgDQogICAgPiAgICAgDQogICAgPiAgICAgVGhhbmtzLA0KICAgID4gICAg IExvcmVuem8NCiAgICA+ICAgICANCiAgICA+ICAgICBDYzogc3RhYmxlQHZnZXIua2VybmVsLm9y Zw0KICAgID4gICAgIFNpZ25lZC1vZmYtYnk6IFNyaWRoYXIgUGl0Y2hhaSA8c3JwaXRjaGFAbWlj cm9zb2Z0LmNvbT4NCiAgICA+ICAgICAtLS0NCiAgICA+ICAgICBDaGFuZ2VzIGluIHYzOg0KICAg ID4gICAgICogZml4IHRoZSBjb21taXQgY29tbWVudC4gW0tZIFNyaW5pdmFzYW4sIE1pY2hhZWwg S2VsbGV5XQ0KICAgID4gICAgIC0tLQ0KICAgID4gICAgICAgZHJpdmVycy9wY2kvaG9zdC9wY2kt aHlwZXJ2LmMgfCAxMSAtLS0tLS0tLS0tLQ0KICAgID4gICAgICAgMSBmaWxlIGNoYW5nZWQsIDEx IGRlbGV0aW9ucygtKQ0KICAgID4gICAgIGRpZmYgLS1naXQgYS9kcml2ZXJzL3BjaS9ob3N0L3Bj aS1oeXBlcnYuYyBiL2RyaXZlcnMvcGNpL2hvc3QvcGNpLWh5cGVydi5jDQogICAgPiAgICAgaW5k ZXggMmZhZjM4ZS4uYWM2N2U1NiAxMDA2NDQNCiAgICA+ICAgICAtLS0gYS9kcml2ZXJzL3BjaS9o b3N0L3BjaS1oeXBlcnYuYw0KICAgID4gICAgICsrKyBiL2RyaXZlcnMvcGNpL2hvc3QvcGNpLWh5 cGVydi5jDQogICAgPiAgICAgQEAgLTE1MTgsMTcgKzE1MTgsNiBAQCBzdGF0aWMgc3RydWN0IGh2 X3BjaV9kZXYgKm5ld19wY2ljaGlsZF9kZXZpY2Uoc3RydWN0IGh2X3BjaWJ1c19kZXZpY2UgKmhi dXMsDQogICAgPiAgICAgICAJZ2V0X3BjaWNoaWxkKGhwZGV2LCBodl9wY2lkZXZfcmVmX2NoaWxk bGlzdCk7DQogICAgPiAgICAgICAJc3Bpbl9sb2NrX2lycXNhdmUoJmhidXMtPmRldmljZV9saXN0 X2xvY2ssIGZsYWdzKTsNCiAgICA+ICAgICAgIA0KICAgID4gICAgIC0JLyoNCiAgICA+ICAgICAt CSAqIFdoZW4gYSBkZXZpY2UgaXMgYmVpbmcgYWRkZWQgdG8gdGhlIGJ1cywgd2Ugc2V0IHRoZSBQ Q0kgZG9tYWluDQogICAgPiAgICAgLQkgKiBudW1iZXIgdG8gYmUgdGhlIGRldmljZSBzZXJpYWwg bnVtYmVyLCB3aGljaCBpcyBub24temVybyBhbmQNCiAgICA+ICAgICAtCSAqIHVuaXF1ZSBvbiB0 aGUgc2FtZSBWTS4gIFRoZSBzZXJpYWwgbnVtYmVycyBzdGFydCB3aXRoIDEsIGFuZA0KICAgID4g ICAgIC0JICogaW5jcmVhc2UgYnkgMSBmb3IgZWFjaCBkZXZpY2UuICBTbyBkZXZpY2UgbmFtZXMg aW5jbHVkaW5nIHRoaXMNCiAgICA+ICAgICAtCSAqIGNhbiBoYXZlIHNob3J0ZXIgbmFtZXMgdGhh biBiYXNlZCBvbiB0aGUgYnVzIGluc3RhbmNlIFVVSUQuDQogICAgPiAgICAgLQkgKiBPbmx5IHRo ZSBmaXJzdCBkZXZpY2Ugc2VyaWFsIG51bWJlciBpcyB1c2VkIGZvciBkb21haW4sIHNvIHRoZQ0K ICAgID4gICAgIC0JICogZG9tYWluIG51bWJlciB3aWxsIG5vdCBjaGFuZ2UgYWZ0ZXIgdGhlIGZp cnN0IGRldmljZSBpcyBhZGRlZC4NCiAgICA+ICAgICAtCSAqLw0KICAgID4gICAgIC0JaWYgKGxp c3RfZW1wdHkoJmhidXMtPmNoaWxkcmVuKSkNCiAgICA+ICAgICAtCQloYnVzLT5zeXNkYXRhLmRv bWFpbiA9IGRlc2MtPnNlcjsNCiAgICA+ICAgICAgIAlsaXN0X2FkZF90YWlsKCZocGRldi0+bGlz dF9lbnRyeSwgJmhidXMtPmNoaWxkcmVuKTsNCiAgICA+ICAgICAgIAlzcGluX3VubG9ja19pcnFy ZXN0b3JlKCZoYnVzLT5kZXZpY2VfbGlzdF9sb2NrLCBmbGFncyk7DQogICAgPiAgICAgICAJcmV0 dXJuIGhwZGV2Ow0KICAgID4gICAgIC0tDQogICAgPiAgICAgMi43LjQgDQogICAgPiAgICAgDQog ICAgPiAgICAgDQogICAgPiANCiAgICANCg0K ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-20 23:00 ` Sridhar Pitchai 0 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-20 23:00 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org Hi Lorenzo, Transparent SRIOV is exposing the NIC directly to the kernel via para-virtual device, unlike creating a netdev and associating it with the bond driver. Further descriptions here, https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0c195567a8f6e82ea5535cd9f1d54a1626dd233e Previously, when using the bond driver, unique and persistent VF NIC name was required, so we used serial number as PCI domain which is included as part of the VF NIC name. Transparent SRIOV mode puts VF NIC based on MAC match as a slave of synthetic NIC, so VF NIC’s name is no longer important. Thanks, Sridhar On 3/20/18, 11:32 AM, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com> wrote: On Tue, Mar 20, 2018 at 05:56:15PM +0000, Sridhar Pitchai wrote: > Hi Lorenzo, > Are we good with the explanation? Can I send the patch with the > updated commit comments? Almost. [...] > Since we have the transparent SRIOV mode now, the short VF device name > is no longer needed. Can you correlate transparent SRIOV mode to the point you are making below ? Please explain what transparent SRIOV mode allows you to remove and why. The rest of the explanation seems OK. Please follow this email format: https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.kernel.org%2Flkml%2F%23s3-9&data=04%7C01%7CSridhar.Pitchai%40microsoft.com%7Cc5cdcb7951f64318e52708d58e90e6f2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636571675366181738%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=yBdqc4NQZsO7O9vfgJsr5olU8GfLNjF5e9EAaCb7vq4%3D&reserved=0 Thanks, Lorenzo > I still do not understand what this means and how it is related to the > patch below, it may be clear to you, it is not to me, at all. > > Sridhar >> the patch below, was introduced to make the device name small, by taking only > 16bits of the serial number. Since we are not going to have the serial number > updated to the BUS id, this has to be removed. > > Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain") > > Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") > Sridhr >> yes > > I asked you an explicit question. Commit above was added for a reason > I assume. This patch implies that kernel has been broken since v4.11 > which is almost a year ago and nobody every noticed ? Or there are > systems where commit above is _necessary_ and this patch would break > them ? > > I want a detailed explanation that highlights *why* it is safe to apply > this patch and send it to stable kernels, commit log above won't do. > > Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child > device when it is added. This cannot produce a unique domain ID all the time. > Here in the bug, we see the collision between the serial number and already > existing PCI bus. The cleaner way is never touch the domain ID provided by > hyperV during the PCI bus creation. As long as hyperV make sure it provides a > unique domain ID for the PCI for a VM it will not break, and HyperV will > guarantees that the domain for the PCI bus for a given VM will be always unique. > The original patch was also intending to have a unique domain ID for the PCI > bus, by taking the serial number of the device, but it is not sufficient, when > the device serial number is number which is the domain ID of the existing PCI > bus. With the current kernel we can repro this issue by adding a device with a > serial number matching the existing PCI bus domain id. (in this case that > happens to be zero). > > > Thanks, > Lorenzo > > Cc: stable@vger.kernel.org > Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com> > --- > Changes in v3: > * fix the commit comment. [KY Srinivasan, Michael Kelley] > --- > drivers/pci/host/pci-hyperv.c | 11 ----------- > 1 file changed, 11 deletions(-) > diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c > index 2faf38e..ac67e56 100644 > --- a/drivers/pci/host/pci-hyperv.c > +++ b/drivers/pci/host/pci-hyperv.c > @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus, > get_pcichild(hpdev, hv_pcidev_ref_childlist); > spin_lock_irqsave(&hbus->device_list_lock, flags); > > - /* > - * When a device is being added to the bus, we set the PCI domain > - * number to be the device serial number, which is non-zero and > - * unique on the same VM. The serial numbers start with 1, and > - * increase by 1 for each device. So device names including this > - * 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. > - */ > - if (list_empty(&hbus->children)) > - hbus->sysdata.domain = desc->ser; > list_add_tail(&hpdev->list_entry, &hbus->children); > spin_unlock_irqrestore(&hbus->device_list_lock, flags); > return hpdev; > -- > 2.7.4 > > > _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption 2018-03-20 23:00 ` Sridhar Pitchai @ 2018-03-21 16:26 ` Lorenzo Pieralisi -1 siblings, 0 replies; 26+ messages in thread From: Lorenzo Pieralisi @ 2018-03-21 16:26 UTC (permalink / raw) To: Sridhar Pitchai Cc: Bjorn Helgaas, Jake Oshins, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, KY Srinivasan, Michael Kelley (EOSG), devel@linuxdriverproject.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org On Tue, Mar 20, 2018 at 11:00:36PM +0000, Sridhar Pitchai wrote: > Hi Lorenzo, > Transparent SRIOV is exposing the NIC directly to the kernel via > para-virtual device, unlike creating a netdev and associating it > with the bond driver. Further descriptions here, > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0c195567a8f6e82ea5535cd9f1d54a1626dd233e > > Previously, when using the bond driver, unique and persistent VF NIC > name was required, so we used serial number as PCI domain which is > included as part of the VF NIC name. Transparent SRIOV mode puts VF > NIC based on MAC match as a slave of synthetic NIC, so VF NIC’s name > is no longer important. Please read the link I sent you in relation to email formatting. Then add your description above in a way that anyone not 100% familiar with hyperv can understand it - that's what the commit log is for. You are sending this patch to stable kernels, patch above has been in the kernel from v4.14. The patch you are fixing since v4.11, you ought to be careful since you do not want to have broken kernel versions owing to stable patches mismatches, that's why I asked and I will ask again, are you sure you won't trigger a regression by sending this fix to stable ? I assume the bond driver mechanism is now done and dusted. Thanks, Lorenzo > Thanks, > Sridhar > > On 3/20/18, 11:32 AM, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com> wrote: > > On Tue, Mar 20, 2018 at 05:56:15PM +0000, Sridhar Pitchai wrote: > > Hi Lorenzo, > > > Are we good with the explanation? Can I send the patch with the > > updated commit comments? > > Almost. > > [...] > > > Since we have the transparent SRIOV mode now, the short VF device name > > is no longer needed. > > Can you correlate transparent SRIOV mode to the point you are making > below ? Please explain what transparent SRIOV mode allows you to remove > and why. The rest of the explanation seems OK. > > Please follow this email format: > > https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.kernel.org%2Flkml%2F%23s3-9&data=04%7C01%7CSridhar.Pitchai%40microsoft.com%7Cc5cdcb7951f64318e52708d58e90e6f2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636571675366181738%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=yBdqc4NQZsO7O9vfgJsr5olU8GfLNjF5e9EAaCb7vq4%3D&reserved=0 > > Thanks, > Lorenzo > > > I still do not understand what this means and how it is related to the > > patch below, it may be clear to you, it is not to me, at all. > > > > Sridhar >> the patch below, was introduced to make the device name small, by taking only > > 16bits of the serial number. Since we are not going to have the serial number > > updated to the BUS id, this has to be removed. > > > > Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain") > > > > Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") > > Sridhr >> yes > > > > I asked you an explicit question. Commit above was added for a reason > > I assume. This patch implies that kernel has been broken since v4.11 > > which is almost a year ago and nobody every noticed ? Or there are > > systems where commit above is _necessary_ and this patch would break > > them ? > > > > I want a detailed explanation that highlights *why* it is safe to apply > > this patch and send it to stable kernels, commit log above won't do. > > > > Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child > > device when it is added. This cannot produce a unique domain ID all the time. > > Here in the bug, we see the collision between the serial number and already > > existing PCI bus. The cleaner way is never touch the domain ID provided by > > hyperV during the PCI bus creation. As long as hyperV make sure it provides a > > unique domain ID for the PCI for a VM it will not break, and HyperV will > > guarantees that the domain for the PCI bus for a given VM will be always unique. > > The original patch was also intending to have a unique domain ID for the PCI > > bus, by taking the serial number of the device, but it is not sufficient, when > > the device serial number is number which is the domain ID of the existing PCI > > bus. With the current kernel we can repro this issue by adding a device with a > > serial number matching the existing PCI bus domain id. (in this case that > > happens to be zero). > > > > > > Thanks, > > Lorenzo > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com> > > --- > > Changes in v3: > > * fix the commit comment. [KY Srinivasan, Michael Kelley] > > --- > > drivers/pci/host/pci-hyperv.c | 11 ----------- > > 1 file changed, 11 deletions(-) > > diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c > > index 2faf38e..ac67e56 100644 > > --- a/drivers/pci/host/pci-hyperv.c > > +++ b/drivers/pci/host/pci-hyperv.c > > @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus, > > get_pcichild(hpdev, hv_pcidev_ref_childlist); > > spin_lock_irqsave(&hbus->device_list_lock, flags); > > > > - /* > > - * When a device is being added to the bus, we set the PCI domain > > - * number to be the device serial number, which is non-zero and > > - * unique on the same VM. The serial numbers start with 1, and > > - * increase by 1 for each device. So device names including this > > - * 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. > > - */ > > - if (list_empty(&hbus->children)) > > - hbus->sysdata.domain = desc->ser; > > list_add_tail(&hpdev->list_entry, &hbus->children); > > spin_unlock_irqrestore(&hbus->device_list_lock, flags); > > return hpdev; > > -- > > 2.7.4 > > > > > > > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-21 16:26 ` Lorenzo Pieralisi 0 siblings, 0 replies; 26+ messages in thread From: Lorenzo Pieralisi @ 2018-03-21 16:26 UTC (permalink / raw) To: Sridhar Pitchai Cc: Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org On Tue, Mar 20, 2018 at 11:00:36PM +0000, Sridhar Pitchai wrote: > Hi Lorenzo, > Transparent SRIOV is exposing the NIC directly to the kernel via > para-virtual device, unlike creating a netdev and associating it > with the bond driver. Further descriptions here, > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0c195567a8f6e82ea5535cd9f1d54a1626dd233e > > Previously, when using the bond driver, unique and persistent VF NIC > name was required, so we used serial number as PCI domain which is > included as part of the VF NIC name. Transparent SRIOV mode puts VF > NIC based on MAC match as a slave of synthetic NIC, so VF NIC’s name > is no longer important. Please read the link I sent you in relation to email formatting. Then add your description above in a way that anyone not 100% familiar with hyperv can understand it - that's what the commit log is for. You are sending this patch to stable kernels, patch above has been in the kernel from v4.14. The patch you are fixing since v4.11, you ought to be careful since you do not want to have broken kernel versions owing to stable patches mismatches, that's why I asked and I will ask again, are you sure you won't trigger a regression by sending this fix to stable ? I assume the bond driver mechanism is now done and dusted. Thanks, Lorenzo > Thanks, > Sridhar > > On 3/20/18, 11:32 AM, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com> wrote: > > On Tue, Mar 20, 2018 at 05:56:15PM +0000, Sridhar Pitchai wrote: > > Hi Lorenzo, > > > Are we good with the explanation? Can I send the patch with the > > updated commit comments? > > Almost. > > [...] > > > Since we have the transparent SRIOV mode now, the short VF device name > > is no longer needed. > > Can you correlate transparent SRIOV mode to the point you are making > below ? Please explain what transparent SRIOV mode allows you to remove > and why. The rest of the explanation seems OK. > > Please follow this email format: > > https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.kernel.org%2Flkml%2F%23s3-9&data=04%7C01%7CSridhar.Pitchai%40microsoft.com%7Cc5cdcb7951f64318e52708d58e90e6f2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636571675366181738%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=yBdqc4NQZsO7O9vfgJsr5olU8GfLNjF5e9EAaCb7vq4%3D&reserved=0 > > Thanks, > Lorenzo > > > I still do not understand what this means and how it is related to the > > patch below, it may be clear to you, it is not to me, at all. > > > > Sridhar >> the patch below, was introduced to make the device name small, by taking only > > 16bits of the serial number. Since we are not going to have the serial number > > updated to the BUS id, this has to be removed. > > > > Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain") > > > > Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") > > Sridhr >> yes > > > > I asked you an explicit question. Commit above was added for a reason > > I assume. This patch implies that kernel has been broken since v4.11 > > which is almost a year ago and nobody every noticed ? Or there are > > systems where commit above is _necessary_ and this patch would break > > them ? > > > > I want a detailed explanation that highlights *why* it is safe to apply > > this patch and send it to stable kernels, commit log above won't do. > > > > Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is modified by the child > > device when it is added. This cannot produce a unique domain ID all the time. > > Here in the bug, we see the collision between the serial number and already > > existing PCI bus. The cleaner way is never touch the domain ID provided by > > hyperV during the PCI bus creation. As long as hyperV make sure it provides a > > unique domain ID for the PCI for a VM it will not break, and HyperV will > > guarantees that the domain for the PCI bus for a given VM will be always unique. > > The original patch was also intending to have a unique domain ID for the PCI > > bus, by taking the serial number of the device, but it is not sufficient, when > > the device serial number is number which is the domain ID of the existing PCI > > bus. With the current kernel we can repro this issue by adding a device with a > > serial number matching the existing PCI bus domain id. (in this case that > > happens to be zero). > > > > > > Thanks, > > Lorenzo > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Sridhar Pitchai <srpitcha@microsoft.com> > > --- > > Changes in v3: > > * fix the commit comment. [KY Srinivasan, Michael Kelley] > > --- > > drivers/pci/host/pci-hyperv.c | 11 ----------- > > 1 file changed, 11 deletions(-) > > diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c > > index 2faf38e..ac67e56 100644 > > --- a/drivers/pci/host/pci-hyperv.c > > +++ b/drivers/pci/host/pci-hyperv.c > > @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus, > > get_pcichild(hpdev, hv_pcidev_ref_childlist); > > spin_lock_irqsave(&hbus->device_list_lock, flags); > > > > - /* > > - * When a device is being added to the bus, we set the PCI domain > > - * number to be the device serial number, which is non-zero and > > - * unique on the same VM. The serial numbers start with 1, and > > - * increase by 1 for each device. So device names including this > > - * 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. > > - */ > > - if (list_empty(&hbus->children)) > > - hbus->sysdata.domain = desc->ser; > > list_add_tail(&hpdev->list_entry, &hbus->children); > > spin_unlock_irqrestore(&hbus->device_list_lock, flags); > > return hpdev; > > -- > > 2.7.4 > > > > > > > > _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption 2018-03-21 16:26 ` Lorenzo Pieralisi @ 2018-03-23 16:41 ` Sridhar Pitchai -1 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-23 16:41 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Bjorn Helgaas, Jake Oshins, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, KY Srinivasan, Michael Kelley (EOSG), devel@linuxdriverproject.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org ICAgIFBsZWFzZSByZWFkIHRoZSBsaW5rIEkgc2VudCB5b3UgaW4gcmVsYXRpb24gdG8gZW1haWwg Zm9ybWF0dGluZy4NCiAgICANCiAgICBUaGVuIGFkZCB5b3VyIGRlc2NyaXB0aW9uIGFib3ZlIGlu IGEgd2F5IHRoYXQgYW55b25lIG5vdCAxMDAlIGZhbWlsaWFyDQogICAgd2l0aCBoeXBlcnYgY2Fu IHVuZGVyc3RhbmQgaXQgLSB0aGF0J3Mgd2hhdCB0aGUgY29tbWl0IGxvZyBpcyBmb3IuDQogICAg DQogICAgWW91IGFyZSBzZW5kaW5nIHRoaXMgcGF0Y2ggdG8gc3RhYmxlIGtlcm5lbHMsIHBhdGNo IGFib3ZlIGhhcyBiZWVuIGluDQogICAgdGhlIGtlcm5lbCBmcm9tIHY0LjE0LiBUaGUgcGF0Y2gg eW91IGFyZSBmaXhpbmcgc2luY2UgdjQuMTEsIHlvdSBvdWdodA0KICAgIHRvIGJlIGNhcmVmdWwg c2luY2UgeW91IGRvIG5vdCB3YW50IHRvIGhhdmUgYnJva2VuIGtlcm5lbCB2ZXJzaW9ucyBvd2lu Zw0KICAgIHRvIHN0YWJsZSBwYXRjaGVzIG1pc21hdGNoZXMsIHRoYXQncyB3aHkgSSBhc2tlZCBh bmQgSSB3aWxsIGFzayBhZ2FpbiwNCiAgICBhcmUgeW91IHN1cmUgeW91IHdvbid0IHRyaWdnZXIg YSByZWdyZXNzaW9uIGJ5IHNlbmRpbmcgdGhpcyBmaXggdG8NCiAgICBzdGFibGUgPw0KICAgIA0K ICAgIEkgYXNzdW1lIHRoZSBib25kIGRyaXZlciBtZWNoYW5pc20gaXMgbm93IGRvbmUgYW5kIGR1 c3RlZC4NCiAgICANClRoYXQgaXMgY29ycmVjdC4gSSBoYXZlIHNlbnQgYSB2NCB2ZXJzaW9uIG9m IHRoZSBwYXRjaC4gSSBhbSBzZW5kaW5nIHRoaXMNCnBhdGNoIGZvciBzdGFibGUga2VybmVsLiBX ZSBoYXZlIHRlc3RlZCBhbmQgSSBhbSBzdXJlIHRoaXMgc2hvdWxkIG5vdCB0cmlnZ2VyDQpyZWdy ZXNzaW9uIGJ5IHNlbmRpbmcgdGhpcyBmaXggdG8gc3RhYmxlLg0KDQpUaGFua3MNClNyaWRoYXIg UGl0Y2hhaSAgICAgDQogDQoNCg== ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-23 16:41 ` Sridhar Pitchai 0 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-23 16:41 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org Please read the link I sent you in relation to email formatting. Then add your description above in a way that anyone not 100% familiar with hyperv can understand it - that's what the commit log is for. You are sending this patch to stable kernels, patch above has been in the kernel from v4.14. The patch you are fixing since v4.11, you ought to be careful since you do not want to have broken kernel versions owing to stable patches mismatches, that's why I asked and I will ask again, are you sure you won't trigger a regression by sending this fix to stable ? I assume the bond driver mechanism is now done and dusted. That is correct. I have sent a v4 version of the patch. I am sending this patch for stable kernel. We have tested and I am sure this should not trigger regression by sending this fix to stable. Thanks Sridhar Pitchai _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption 2018-03-23 16:41 ` Sridhar Pitchai @ 2018-03-23 16:47 ` Greg KH -1 siblings, 0 replies; 26+ messages in thread From: Greg KH @ 2018-03-23 16:47 UTC (permalink / raw) To: Sridhar Pitchai Cc: Lorenzo Pieralisi, Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org On Fri, Mar 23, 2018 at 04:41:02PM +0000, Sridhar Pitchai wrote: > Please read the link I sent you in relation to email formatting. > > Then add your description above in a way that anyone not 100% familiar > with hyperv can understand it - that's what the commit log is for. > > You are sending this patch to stable kernels, patch above has been in > the kernel from v4.14. The patch you are fixing since v4.11, you ought > to be careful since you do not want to have broken kernel versions owing > to stable patches mismatches, that's why I asked and I will ask again, > are you sure you won't trigger a regression by sending this fix to > stable ? > > I assume the bond driver mechanism is now done and dusted. > > That is correct. I have sent a v4 version of the patch. I am sending this > patch for stable kernel. We have tested and I am sure this should not trigger > regression by sending this fix to stable. <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-23 16:47 ` Greg KH 0 siblings, 0 replies; 26+ messages in thread From: Greg KH @ 2018-03-23 16:47 UTC (permalink / raw) To: Sridhar Pitchai Cc: Lorenzo Pieralisi, Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Bjorn Helgaas, Jake Oshins, devel@linuxdriverproject.org On Fri, Mar 23, 2018 at 04:41:02PM +0000, Sridhar Pitchai wrote: > Please read the link I sent you in relation to email formatting. > > Then add your description above in a way that anyone not 100% familiar > with hyperv can understand it - that's what the commit log is for. > > You are sending this patch to stable kernels, patch above has been in > the kernel from v4.14. The patch you are fixing since v4.11, you ought > to be careful since you do not want to have broken kernel versions owing > to stable patches mismatches, that's why I asked and I will ask again, > are you sure you won't trigger a regression by sending this fix to > stable ? > > I assume the bond driver mechanism is now done and dusted. > > That is correct. I have sent a v4 version of the patch. I am sending this > patch for stable kernel. We have tested and I am sure this should not trigger > regression by sending this fix to stable. <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter> _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption 2018-03-23 16:47 ` Greg KH @ 2018-03-26 17:32 ` Sridhar Pitchai -1 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-26 17:32 UTC (permalink / raw) To: Greg KH Cc: Lorenzo Pieralisi, Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org ICANCiAgICA+IFRoaXMgaXMgbm90IHRoZSBjb3JyZWN0IHdheSB0byBzdWJtaXQgcGF0Y2hlcyBm b3IgaW5jbHVzaW9uIGluIHRoZQ0KICAgID4gc3RhYmxlIGtlcm5lbCB0cmVlLiAgUGxlYXNlIHJl YWQ6DQogICAgPiAgPHVybD4NCiAgICA+IGZvciBob3cgdG8gZG8gdGhpcyBwcm9wZXJseS4NCg0K TXkgYmFkLiBJIGFtIHNlbmRpbmcgdGhlIHBhdGNoIHRvIHRoZSBtYWlubGluZSBrZXJuZWwgd2l0 aCBhIENjOiB0YWcgZm9yIHN0YWJsZQ0KYW5kIGhvcGluZyB0aGUgcGF0Y2ggd2lsbCBiZSBhdXRv bWF0aWNhbGx5IG1lcmdlZCB0byB0aGUgc3RhYmxlIGtlcm5lbHMuDQoNClRoYW5rcywNClNyaWRo YXIgUGl0Y2hhaQ0KDQo= ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-26 17:32 ` Sridhar Pitchai 0 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-26 17:32 UTC (permalink / raw) To: Greg KH Cc: Lorenzo Pieralisi, Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org > This is not the correct way to submit patches for inclusion in the > stable kernel tree. Please read: > <url> > for how to do this properly. My bad. I am sending the patch to the mainline kernel with a Cc: tag for stable and hoping the patch will be automatically merged to the stable kernels. Thanks, Sridhar Pitchai ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption 2018-03-20 23:00 ` Sridhar Pitchai @ 2018-03-21 17:30 ` Bjorn Helgaas -1 siblings, 0 replies; 26+ messages in thread From: Bjorn Helgaas @ 2018-03-21 17:30 UTC (permalink / raw) To: Sridhar Pitchai Cc: Lorenzo Pieralisi, Bjorn Helgaas, Jake Oshins, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, KY Srinivasan, Michael Kelley (EOSG), devel@linuxdriverproject.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org [I composed most of this before seeing Lorenzo's response, so sorry about the duplication. Maybe seeing things stated a different way will help :)] On Tue, Mar 20, 2018 at 11:00:36PM +0000, Sridhar Pitchai wrote: > Hi Lorenzo, > Transparent SRIOV is exposing the NIC directly to the kernel via > para-virtual device, unlike creating a netdev and associating it with the bond > driver. Further descriptions here, > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0c195567a8f6e82ea5535cd9f1d54a1626dd233e > > Previously, when using the bond driver, unique and persistent VF NIC name > was required, so we used serial number as PCI domain which is included as > part of the VF NIC name. Transparent SRIOV mode puts VF NIC based on MAC > match as a slave of synthetic NIC, so VF NIC’s name is no longer important. Hi Sridhar, A few hints about submitting patches more efficiently: 1) You never have to ask "Are we OK with the explanation? If so, I'll send a patch with updated changelog." That forces an extra round-trip. Simply post a new version with your proposed update. If Lorenzo has more questions, he'll say so and you can do another version. 2) When Lorenzo is asking for clarification, he's not really asking for the clarification in an email response, because the email thread will soon be forgotten and lost in the archives. What we really want is for the permanent git changelog to make sense to someone in the future. The easiest way is to post a new patch version with a revised changelog that answers the questions. 3) Please capitalize and punctuate consistently with previous history, e.g., "PCI: hv: Fix domain ID corruption" for your title, "SR-IOV" (not "SRIOV") and "bus" (not "BUS") in changelog. Both "para-virtual" and "paravirtual" are used in the kernel, but "paravirtual" is much more common. Run "git log" and "git log --oneline" on your file and follow the same style. 4) When you reference a previous commit, please use this style: 0c195567a8f6 ("netvsc: transparent VF management"), i.e., 12-char SHA1 followed by title. You seem to have removed some spaces from the commit you mention in the "Fixes" tag. And a few content questions/observations of my own: 1) "Fix domain ID corruption" isn't a very good title because it suggests you're fixing a memory corruption or similar defect. But in fact, I think you're removing something that used to be a feature (added by 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")) but is now no longer needed and in fact now causes a problem. 2) Your changelog does mention 4a9b0933bdfc, which is good, but there must be some other <commit X> that makes it safe to remove 4a9b0933bdfc, i.e., <commit X> removes the need for using the device serial number as the PCI domain. <Commit X> *must* be mentioned in the changelog. Otherwise, people may backport this patch to a kernel that doesn't include <commit X>, and things will break. 3) I don't understand what you mean by "transparent SR-IOV mode". Is that something different than regular SR-IOV? If so, what exactly is the difference? I don't think the PCIe specs mention a "transparent mode", so is it a Hyper-V thing? It seems important, but I don't see any pci-hyperv.c commits that mention it. Here's a stab at the sort of changelog I would be looking for. Obviously I don't understand much about Hyper-V and pci-hyperv.c, so please correct the things I got wrong: When Linux runs as a guest in a Hyper-V VM, pci-hyperv.c paravirtualizes access to PCI devices assigned to the guest. For each of those devices, hv_pci_probe() creates a virtual PCI bus in its own unique PCI domain. 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") overrode that unique PCI domain to be the Hyper-V device serial number to make device names more convenient <or whatever the real reason is; I don't quite understand this part>. One problem with 4a9b0933bdfc is that the Hyper-V device serial number is not necessarily unique, so we may end up with two buses with the same domain and bus number, and adding the second bus fails. We no longer need to override the PCI domain numbers because <commit X> removed the need for that. Revert 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") so we can reliably support multiple devices being assigned to a guest. This revert should only be backported to kernels that contain <commit X>. Bjorn ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-21 17:30 ` Bjorn Helgaas 0 siblings, 0 replies; 26+ messages in thread From: Bjorn Helgaas @ 2018-03-21 17:30 UTC (permalink / raw) To: Sridhar Pitchai Cc: Lorenzo Pieralisi, Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org [I composed most of this before seeing Lorenzo's response, so sorry about the duplication. Maybe seeing things stated a different way will help :)] On Tue, Mar 20, 2018 at 11:00:36PM +0000, Sridhar Pitchai wrote: > Hi Lorenzo, > Transparent SRIOV is exposing the NIC directly to the kernel via > para-virtual device, unlike creating a netdev and associating it with the bond > driver. Further descriptions here, > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0c195567a8f6e82ea5535cd9f1d54a1626dd233e > > Previously, when using the bond driver, unique and persistent VF NIC name > was required, so we used serial number as PCI domain which is included as > part of the VF NIC name. Transparent SRIOV mode puts VF NIC based on MAC > match as a slave of synthetic NIC, so VF NIC’s name is no longer important. Hi Sridhar, A few hints about submitting patches more efficiently: 1) You never have to ask "Are we OK with the explanation? If so, I'll send a patch with updated changelog." That forces an extra round-trip. Simply post a new version with your proposed update. If Lorenzo has more questions, he'll say so and you can do another version. 2) When Lorenzo is asking for clarification, he's not really asking for the clarification in an email response, because the email thread will soon be forgotten and lost in the archives. What we really want is for the permanent git changelog to make sense to someone in the future. The easiest way is to post a new patch version with a revised changelog that answers the questions. 3) Please capitalize and punctuate consistently with previous history, e.g., "PCI: hv: Fix domain ID corruption" for your title, "SR-IOV" (not "SRIOV") and "bus" (not "BUS") in changelog. Both "para-virtual" and "paravirtual" are used in the kernel, but "paravirtual" is much more common. Run "git log" and "git log --oneline" on your file and follow the same style. 4) When you reference a previous commit, please use this style: 0c195567a8f6 ("netvsc: transparent VF management"), i.e., 12-char SHA1 followed by title. You seem to have removed some spaces from the commit you mention in the "Fixes" tag. And a few content questions/observations of my own: 1) "Fix domain ID corruption" isn't a very good title because it suggests you're fixing a memory corruption or similar defect. But in fact, I think you're removing something that used to be a feature (added by 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")) but is now no longer needed and in fact now causes a problem. 2) Your changelog does mention 4a9b0933bdfc, which is good, but there must be some other <commit X> that makes it safe to remove 4a9b0933bdfc, i.e., <commit X> removes the need for using the device serial number as the PCI domain. <Commit X> *must* be mentioned in the changelog. Otherwise, people may backport this patch to a kernel that doesn't include <commit X>, and things will break. 3) I don't understand what you mean by "transparent SR-IOV mode". Is that something different than regular SR-IOV? If so, what exactly is the difference? I don't think the PCIe specs mention a "transparent mode", so is it a Hyper-V thing? It seems important, but I don't see any pci-hyperv.c commits that mention it. Here's a stab at the sort of changelog I would be looking for. Obviously I don't understand much about Hyper-V and pci-hyperv.c, so please correct the things I got wrong: When Linux runs as a guest in a Hyper-V VM, pci-hyperv.c paravirtualizes access to PCI devices assigned to the guest. For each of those devices, hv_pci_probe() creates a virtual PCI bus in its own unique PCI domain. 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") overrode that unique PCI domain to be the Hyper-V device serial number to make device names more convenient <or whatever the real reason is; I don't quite understand this part>. One problem with 4a9b0933bdfc is that the Hyper-V device serial number is not necessarily unique, so we may end up with two buses with the same domain and bus number, and adding the second bus fails. We no longer need to override the PCI domain numbers because <commit X> removed the need for that. Revert 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") so we can reliably support multiple devices being assigned to a guest. This revert should only be backported to kernels that contain <commit X>. Bjorn _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption 2018-03-21 17:30 ` Bjorn Helgaas @ 2018-03-23 16:09 ` Sridhar Pitchai -1 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-23 16:09 UTC (permalink / raw) To: Bjorn Helgaas Cc: Lorenzo Pieralisi, Bjorn Helgaas, Jake Oshins, Haiyang Zhang, Stephen Hemminger, Dexuan Cui, KY Srinivasan, Michael Kelley (EOSG), devel@linuxdriverproject.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org ICAgID4gUHJldmlvdXNseSwgd2hlbiB1c2luZyB0aGUgYm9uZCBkcml2ZXIsIHVuaXF1ZSBhbmQg cGVyc2lzdGVudCBWRiBOSUMgbmFtZQ0KICAgID4gd2FzIHJlcXVpcmVkLCBzbyB3ZSB1c2VkIHNl cmlhbCBudW1iZXIgYXMgUENJIGRvbWFpbiB3aGljaCBpcyBpbmNsdWRlZCBhcw0KICAgID4gcGFy dCBvZiB0aGUgVkYgTklDIG5hbWUuICBUcmFuc3BhcmVudCBTUklPViBtb2RlIHB1dHMgVkYgTklD IGJhc2VkIG9uIE1BQw0KICAgID4gbWF0Y2ggYXMgYSBzbGF2ZSBvZiBzeW50aGV0aWMgTklDLCBz byBWRiBOSUPigJlzIG5hbWUgaXMgbm8gbG9uZ2VyIGltcG9ydGFudC4NCiAgICANCiAgICBIaSBT cmlkaGFyLA0KICAgIA0KICAgIEEgZmV3IGhpbnRzIGFib3V0IHN1Ym1pdHRpbmcgcGF0Y2hlcyBt b3JlIGVmZmljaWVudGx5Og0KICAgIA0KICAgIDEpIFlvdSBuZXZlciBoYXZlIHRvIGFzayAiQXJl IHdlIE9LIHdpdGggdGhlIGV4cGxhbmF0aW9uPyAgSWYgc28sIEknbGwNCiAgICBzZW5kIGEgcGF0 Y2ggd2l0aCB1cGRhdGVkIGNoYW5nZWxvZy4iICBUaGF0IGZvcmNlcyBhbiBleHRyYQ0KICAgIHJv dW5kLXRyaXAuICBTaW1wbHkgcG9zdCBhIG5ldyB2ZXJzaW9uIHdpdGggeW91ciBwcm9wb3NlZCB1 cGRhdGUuICBJZg0KICAgIExvcmVuem8gaGFzIG1vcmUgcXVlc3Rpb25zLCBoZSdsbCBzYXkgc28g YW5kIHlvdSBjYW4gZG8gYW5vdGhlcg0KICAgIHZlcnNpb24uDQogICAgDQogICAgMikgV2hlbiBM b3JlbnpvIGlzIGFza2luZyBmb3IgY2xhcmlmaWNhdGlvbiwgaGUncyBub3QgcmVhbGx5IGFza2lu Zw0KICAgIGZvciB0aGUgY2xhcmlmaWNhdGlvbiBpbiBhbiBlbWFpbCByZXNwb25zZSwgYmVjYXVz ZSB0aGUgZW1haWwgdGhyZWFkDQogICAgd2lsbCBzb29uIGJlIGZvcmdvdHRlbiBhbmQgbG9zdCBp biB0aGUgYXJjaGl2ZXMuICBXaGF0IHdlIHJlYWxseSB3YW50DQogICAgaXMgZm9yIHRoZSBwZXJt YW5lbnQgZ2l0IGNoYW5nZWxvZyB0byBtYWtlIHNlbnNlIHRvIHNvbWVvbmUgaW4gdGhlDQogICAg ZnV0dXJlLiAgVGhlIGVhc2llc3Qgd2F5IGlzIHRvIHBvc3QgYSBuZXcgcGF0Y2ggdmVyc2lvbiB3 aXRoIGENCiAgICByZXZpc2VkIGNoYW5nZWxvZyB0aGF0IGFuc3dlcnMgdGhlIHF1ZXN0aW9ucy4N CiAgICANCiAgICAzKSBQbGVhc2UgY2FwaXRhbGl6ZSBhbmQgcHVuY3R1YXRlIGNvbnNpc3RlbnRs eSB3aXRoIHByZXZpb3VzIGhpc3RvcnksDQogICAgZS5nLiwgIlBDSTogaHY6IEZpeCBkb21haW4g SUQgY29ycnVwdGlvbiIgZm9yIHlvdXIgdGl0bGUsICJTUi1JT1YiDQogICAgKG5vdCAiU1JJT1Yi KSBhbmQgImJ1cyIgKG5vdCAiQlVTIikgaW4gY2hhbmdlbG9nLiAgQm90aCAicGFyYS12aXJ0dWFs Ig0KICAgIGFuZCAicGFyYXZpcnR1YWwiIGFyZSB1c2VkIGluIHRoZSBrZXJuZWwsIGJ1dCAicGFy YXZpcnR1YWwiIGlzIG11Y2gNCiAgICBtb3JlIGNvbW1vbi4gIFJ1biAiZ2l0IGxvZyIgYW5kICJn aXQgbG9nIC0tb25lbGluZSIgb24geW91ciBmaWxlIGFuZA0KICAgIGZvbGxvdyB0aGUgc2FtZSBz dHlsZS4NCiAgICANCiAgICA0KSBXaGVuIHlvdSByZWZlcmVuY2UgYSBwcmV2aW91cyBjb21taXQs IHBsZWFzZSB1c2UgdGhpcyBzdHlsZToNCiAgICAwYzE5NTU2N2E4ZjYgKCJuZXR2c2M6IHRyYW5z cGFyZW50IFZGIG1hbmFnZW1lbnQiKSwgaS5lLiwgMTItY2hhciBTSEExDQogICAgZm9sbG93ZWQg YnkgdGl0bGUuICBZb3Ugc2VlbSB0byBoYXZlIHJlbW92ZWQgc29tZSBzcGFjZXMgZnJvbSB0aGUN CiAgICBjb21taXQgeW91IG1lbnRpb24gaW4gdGhlICJGaXhlcyIgdGFnLg0KICAgIA0KICAgIEFu ZCBhIGZldyBjb250ZW50IHF1ZXN0aW9ucy9vYnNlcnZhdGlvbnMgb2YgbXkgb3duOg0KICAgIA0K ICAgIDEpICJGaXggZG9tYWluIElEIGNvcnJ1cHRpb24iIGlzbid0IGEgdmVyeSBnb29kIHRpdGxl IGJlY2F1c2UgaXQNCiAgICBzdWdnZXN0cyB5b3UncmUgZml4aW5nIGEgbWVtb3J5IGNvcnJ1cHRp b24gb3Igc2ltaWxhciBkZWZlY3QuICBCdXQgaW4NCiAgICBmYWN0LCBJIHRoaW5rIHlvdSdyZSBy ZW1vdmluZyBzb21ldGhpbmcgdGhhdCB1c2VkIHRvIGJlIGEgZmVhdHVyZQ0KICAgIChhZGRlZCBi eSA0YTliMDkzM2JkZmMgKCJQQ0k6IGh2OiBVc2UgZGV2aWNlIHNlcmlhbCBudW1iZXIgYXMgUENJ DQogICAgZG9tYWluIikpIGJ1dCBpcyBub3cgbm8gbG9uZ2VyIG5lZWRlZCBhbmQgaW4gZmFjdCBu b3cgY2F1c2VzIGENCiAgICBwcm9ibGVtLg0KICAgIA0KICAgIDIpIFlvdXIgY2hhbmdlbG9nIGRv ZXMgbWVudGlvbiA0YTliMDkzM2JkZmMsIHdoaWNoIGlzIGdvb2QsIGJ1dA0KICAgIHRoZXJlIG11 c3QgYmUgc29tZSBvdGhlciA8Y29tbWl0IFg+IHRoYXQgbWFrZXMgaXQgc2FmZSB0byByZW1vdmUN CiAgICA0YTliMDkzM2JkZmMsIGkuZS4sIDxjb21taXQgWD4gcmVtb3ZlcyB0aGUgbmVlZCBmb3Ig dXNpbmcgdGhlIGRldmljZQ0KICAgIHNlcmlhbCBudW1iZXIgYXMgdGhlIFBDSSBkb21haW4uICA8 Q29tbWl0IFg+ICptdXN0KiBiZSBtZW50aW9uZWQgaW4NCiAgICB0aGUgY2hhbmdlbG9nLiAgT3Ro ZXJ3aXNlLCBwZW9wbGUgbWF5IGJhY2twb3J0IHRoaXMgcGF0Y2ggdG8gYSBrZXJuZWwNCiAgICB0 aGF0IGRvZXNuJ3QgaW5jbHVkZSA8Y29tbWl0IFg+LCBhbmQgdGhpbmdzIHdpbGwgYnJlYWsuDQog ICAgDQogICAgMykgSSBkb24ndCB1bmRlcnN0YW5kIHdoYXQgeW91IG1lYW4gYnkgInRyYW5zcGFy ZW50IFNSLUlPViBtb2RlIi4gIElzDQogICAgdGhhdCBzb21ldGhpbmcgZGlmZmVyZW50IHRoYW4g cmVndWxhciBTUi1JT1Y/ICBJZiBzbywgd2hhdCBleGFjdGx5IGlzDQogICAgdGhlIGRpZmZlcmVu Y2U/ICBJIGRvbid0IHRoaW5rIHRoZSBQQ0llIHNwZWNzIG1lbnRpb24gYSAidHJhbnNwYXJlbnQN CiAgICBtb2RlIiwgc28gaXMgaXQgYSBIeXBlci1WIHRoaW5nPyAgSXQgc2VlbXMgaW1wb3J0YW50 LCBidXQgSSBkb24ndCBzZWUNCiAgICBhbnkgcGNpLWh5cGVydi5jIGNvbW1pdHMgdGhhdCBtZW50 aW9uIGl0Lg0KICAgIA0KICAgIEhlcmUncyBhIHN0YWIgYXQgdGhlIHNvcnQgb2YgY2hhbmdlbG9n IEkgd291bGQgYmUgbG9va2luZyBmb3IuDQogICAgT2J2aW91c2x5IEkgZG9uJ3QgdW5kZXJzdGFu ZCBtdWNoIGFib3V0IEh5cGVyLVYgYW5kIHBjaS1oeXBlcnYuYywgc28NCiAgICBwbGVhc2UgY29y cmVjdCB0aGUgdGhpbmdzIEkgZ290IHdyb25nOg0KICAgIA0KICAgICAgV2hlbiBMaW51eCBydW5z IGFzIGEgZ3Vlc3QgaW4gYSBIeXBlci1WIFZNLCBwY2ktaHlwZXJ2LmMNCiAgICAgIHBhcmF2aXJ0 dWFsaXplcyBhY2Nlc3MgdG8gUENJIGRldmljZXMgYXNzaWduZWQgdG8gdGhlIGd1ZXN0LiAgRm9y DQogICAgICBlYWNoIG9mIHRob3NlIGRldmljZXMsIGh2X3BjaV9wcm9iZSgpIGNyZWF0ZXMgYSB2 aXJ0dWFsIFBDSSBidXMgaW4NCiAgICAgIGl0cyBvd24gdW5pcXVlIFBDSSBkb21haW4uDQogICAg DQogICAgICA0YTliMDkzM2JkZmMgKCJQQ0k6IGh2OiBVc2UgZGV2aWNlIHNlcmlhbCBudW1iZXIg YXMgUENJIGRvbWFpbiIpDQogICAgICBvdmVycm9kZSB0aGF0IHVuaXF1ZSBQQ0kgZG9tYWluIHRv IGJlIHRoZSBIeXBlci1WIGRldmljZSBzZXJpYWwNCiAgICAgIG51bWJlciB0byBtYWtlIGRldmlj ZSBuYW1lcyBtb3JlIGNvbnZlbmllbnQgPG9yIHdoYXRldmVyIHRoZSByZWFsDQogICAgICByZWFz b24gaXM7IEkgZG9uJ3QgcXVpdGUgdW5kZXJzdGFuZCB0aGlzIHBhcnQ+Lg0KICAgIA0KICAgICAg T25lIHByb2JsZW0gd2l0aCA0YTliMDkzM2JkZmMgaXMgdGhhdCB0aGUgSHlwZXItViBkZXZpY2Ug c2VyaWFsDQogICAgICBudW1iZXIgaXMgbm90IG5lY2Vzc2FyaWx5IHVuaXF1ZSwgc28gd2UgbWF5 IGVuZCB1cCB3aXRoIHR3byBidXNlcw0KICAgICAgd2l0aCB0aGUgc2FtZSBkb21haW4gYW5kIGJ1 cyBudW1iZXIsIGFuZCBhZGRpbmcgdGhlIHNlY29uZCBidXMNCiAgICAgIGZhaWxzLg0KICAgIA0K ICAgICAgV2Ugbm8gbG9uZ2VyIG5lZWQgdG8gb3ZlcnJpZGUgdGhlIFBDSSBkb21haW4gbnVtYmVy cyBiZWNhdXNlIDxjb21taXQNCiAgICAgIFg+IHJlbW92ZWQgdGhlIG5lZWQgZm9yIHRoYXQuDQog ICAgDQogICAgICBSZXZlcnQgNGE5YjA5MzNiZGZjICgiUENJOiBodjogVXNlIGRldmljZSBzZXJp YWwgbnVtYmVyIGFzIFBDSQ0KICAgICAgZG9tYWluIikgc28gd2UgY2FuIHJlbGlhYmx5IHN1cHBv cnQgbXVsdGlwbGUgZGV2aWNlcyBiZWluZyBhc3NpZ25lZA0KICAgICAgdG8gYSBndWVzdC4NCiAg ICANCiAgICAgIFRoaXMgcmV2ZXJ0IHNob3VsZCBvbmx5IGJlIGJhY2twb3J0ZWQgdG8ga2VybmVs cyB0aGF0IGNvbnRhaW4NCiAgICAgIDxjb21taXQgWD4uDQogICAgDQogICAgQmpvcm4NCg0KVGhh bmtzIGZvciB5b3VyIGNvbW1lbnRzIEJqb3JuLiBJIHRvb2sgc29tZSB0aW1lIHRvIGdvIG92ZXIg dGhlIGFsbCB0aGUNCmNvbW1lbnRzIGFuZCBzZW5kaW5nIGEgbmV3IHZlcnNpb24gb2YgdGhlIHBh dGNoIHdpdGggYWxsIHRoZSBjb21tZW50cyANCmluY29ycG9yYXRlZC4NCg0KVGhhbmtzDQpTcmlk aGFyICAgIA0KDQo= ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption @ 2018-03-23 16:09 ` Sridhar Pitchai 0 siblings, 0 replies; 26+ messages in thread From: Sridhar Pitchai @ 2018-03-23 16:09 UTC (permalink / raw) To: Bjorn Helgaas Cc: Lorenzo Pieralisi, Stephen Hemminger, linux-pci@vger.kernel.org, Haiyang Zhang, linux-kernel@vger.kernel.org, Michael Kelley (EOSG), Jake Oshins, Bjorn Helgaas, devel@linuxdriverproject.org > Previously, when using the bond driver, unique and persistent VF NIC name > was required, so we used serial number as PCI domain which is included as > part of the VF NIC name. Transparent SRIOV mode puts VF NIC based on MAC > match as a slave of synthetic NIC, so VF NIC’s name is no longer important. Hi Sridhar, A few hints about submitting patches more efficiently: 1) You never have to ask "Are we OK with the explanation? If so, I'll send a patch with updated changelog." That forces an extra round-trip. Simply post a new version with your proposed update. If Lorenzo has more questions, he'll say so and you can do another version. 2) When Lorenzo is asking for clarification, he's not really asking for the clarification in an email response, because the email thread will soon be forgotten and lost in the archives. What we really want is for the permanent git changelog to make sense to someone in the future. The easiest way is to post a new patch version with a revised changelog that answers the questions. 3) Please capitalize and punctuate consistently with previous history, e.g., "PCI: hv: Fix domain ID corruption" for your title, "SR-IOV" (not "SRIOV") and "bus" (not "BUS") in changelog. Both "para-virtual" and "paravirtual" are used in the kernel, but "paravirtual" is much more common. Run "git log" and "git log --oneline" on your file and follow the same style. 4) When you reference a previous commit, please use this style: 0c195567a8f6 ("netvsc: transparent VF management"), i.e., 12-char SHA1 followed by title. You seem to have removed some spaces from the commit you mention in the "Fixes" tag. And a few content questions/observations of my own: 1) "Fix domain ID corruption" isn't a very good title because it suggests you're fixing a memory corruption or similar defect. But in fact, I think you're removing something that used to be a feature (added by 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")) but is now no longer needed and in fact now causes a problem. 2) Your changelog does mention 4a9b0933bdfc, which is good, but there must be some other <commit X> that makes it safe to remove 4a9b0933bdfc, i.e., <commit X> removes the need for using the device serial number as the PCI domain. <Commit X> *must* be mentioned in the changelog. Otherwise, people may backport this patch to a kernel that doesn't include <commit X>, and things will break. 3) I don't understand what you mean by "transparent SR-IOV mode". Is that something different than regular SR-IOV? If so, what exactly is the difference? I don't think the PCIe specs mention a "transparent mode", so is it a Hyper-V thing? It seems important, but I don't see any pci-hyperv.c commits that mention it. Here's a stab at the sort of changelog I would be looking for. Obviously I don't understand much about Hyper-V and pci-hyperv.c, so please correct the things I got wrong: When Linux runs as a guest in a Hyper-V VM, pci-hyperv.c paravirtualizes access to PCI devices assigned to the guest. For each of those devices, hv_pci_probe() creates a virtual PCI bus in its own unique PCI domain. 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") overrode that unique PCI domain to be the Hyper-V device serial number to make device names more convenient <or whatever the real reason is; I don't quite understand this part>. One problem with 4a9b0933bdfc is that the Hyper-V device serial number is not necessarily unique, so we may end up with two buses with the same domain and bus number, and adding the second bus fails. We no longer need to override the PCI domain numbers because <commit X> removed the need for that. Revert 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain") so we can reliably support multiple devices being assigned to a guest. This revert should only be backported to kernels that contain <commit X>. Bjorn Thanks for your comments Bjorn. I took some time to go over the all the comments and sending a new version of the patch with all the comments incorporated. Thanks Sridhar _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2018-03-26 17:32 UTC | newest] Thread overview: 26+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-15 0:03 [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption Sridhar Pitchai 2018-03-15 0:03 ` Sridhar Pitchai 2018-03-15 12:05 ` Lorenzo Pieralisi 2018-03-15 12:05 ` Lorenzo Pieralisi 2018-03-15 17:56 ` Sridhar Pitchai 2018-03-15 17:56 ` Sridhar Pitchai 2018-03-15 18:24 ` Sridhar Pitchai 2018-03-15 18:24 ` Sridhar Pitchai 2018-03-20 17:56 ` Sridhar Pitchai 2018-03-20 17:56 ` Sridhar Pitchai 2018-03-20 18:32 ` Lorenzo Pieralisi 2018-03-20 18:32 ` Lorenzo Pieralisi 2018-03-20 23:00 ` Sridhar Pitchai 2018-03-20 23:00 ` Sridhar Pitchai 2018-03-21 16:26 ` Lorenzo Pieralisi 2018-03-21 16:26 ` Lorenzo Pieralisi 2018-03-23 16:41 ` Sridhar Pitchai 2018-03-23 16:41 ` Sridhar Pitchai 2018-03-23 16:47 ` Greg KH 2018-03-23 16:47 ` Greg KH 2018-03-26 17:32 ` Sridhar Pitchai 2018-03-26 17:32 ` Sridhar Pitchai 2018-03-21 17:30 ` Bjorn Helgaas 2018-03-21 17:30 ` Bjorn Helgaas 2018-03-23 16:09 ` Sridhar Pitchai 2018-03-23 16:09 ` Sridhar Pitchai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.