[PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

Tue Mar 20 17:56:15 UTC 2018

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 at 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 at 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 at arm.com>
    Sent: Thursday, March 15, 2018 5:05 AM
    To: Sridhar Pitchai <Sridhar.Pitchai at microsoft.com>
    Cc: Bjorn Helgaas <bhelgaas at google.com>; Jake Oshins <jakeo at microsoft.com>; Haiyang Zhang <haiyangz at microsoft.com>; Stephen Hemminger <sthemmin at microsoft.com>; Dexuan Cui <decui at microsoft.com>; KY Srinivasan <kys at microsoft.com>; Michael Kelley (EOSG) <Michael.H.Kelley at microsoft.com>; devel at linuxdriverproject.org; linux-pci at vger.kernel.org; linux-kernel at 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 at vger.kernel.org
    Signed-off-by: Sridhar Pitchai <srpitcha at 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 