| Message ID | 1568245086-70601-5-git-send-email-decui@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Enhance pci-hyperv to support hibernation | expand |
On Wed, Sep 11, 2019 at 11:38:23PM +0000, Dexuan Cui wrote: > A VM can have multiple hbus. It looks incorrect for the second hbus's > hv_pci_protocol_negotiation() to set the global variable > 'pci_protocol_version' (which was set by the first hbus), even if the > same value is written. > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > --- > drivers/pci/controller/pci-hyperv.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) This is a fix that seems unrelated to the rest of the series. AFAICS the version also affects code paths in the driver, which means that in case you have busses with different versions the current code is wrong in this respect. You have to capture this concept in the commit log, it reads as an optional change but it looks like a potential bug. Lorenzo > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index 2655df2..55730c5 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -76,11 +76,6 @@ enum pci_protocol_version_t { > PCI_PROTOCOL_VERSION_1_1, > }; > > -/* > - * Protocol version negotiated by hv_pci_protocol_negotiation(). > - */ > -static enum pci_protocol_version_t pci_protocol_version; > - > #define PCI_CONFIG_MMIO_LENGTH 0x2000 > #define CFG_PAGE_OFFSET 0x1000 > #define CFG_PAGE_SIZE (PCI_CONFIG_MMIO_LENGTH - CFG_PAGE_OFFSET) > @@ -429,6 +424,8 @@ enum hv_pcibus_state { > > struct hv_pcibus_device { > struct pci_sysdata sysdata; > + /* Protocol version negotiated with the host */ > + enum pci_protocol_version_t protocol_version; > enum hv_pcibus_state state; > refcount_t remove_lock; > struct hv_device *hdev; > @@ -942,7 +939,7 @@ static void hv_irq_unmask(struct irq_data *data) > * negative effect (yet?). > */ > > - if (pci_protocol_version >= PCI_PROTOCOL_VERSION_1_2) { > + if (hbus->protocol_version >= PCI_PROTOCOL_VERSION_1_2) { > /* > * PCI_PROTOCOL_VERSION_1_2 supports the VP_SET version of the > * HVCALL_RETARGET_INTERRUPT hypercall, which also coincides > @@ -1112,7 +1109,7 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) > ctxt.pci_pkt.completion_func = hv_pci_compose_compl; > ctxt.pci_pkt.compl_ctxt = ∁ > > - switch (pci_protocol_version) { > + switch (hbus->protocol_version) { > case PCI_PROTOCOL_VERSION_1_1: > size = hv_compose_msi_req_v1(&ctxt.int_pkts.v1, > dest, > @@ -2116,6 +2113,7 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev, > enum pci_protocol_version_t version[], > int num_version) > { > + struct hv_pcibus_device *hbus = hv_get_drvdata(hdev); > struct pci_version_request *version_req; > struct hv_pci_compl comp_pkt; > struct pci_packet *pkt; > @@ -2155,10 +2153,10 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev, > } > > if (comp_pkt.completion_status >= 0) { > - pci_protocol_version = version[i]; > + hbus->protocol_version = version[i]; > dev_info(&hdev->device, > "PCI VMBus probing: Using version %#x\n", > - pci_protocol_version); > + hbus->protocol_version); > goto exit; > } > > @@ -2442,7 +2440,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev) > u32 wslot; > int ret; > > - size_res = (pci_protocol_version < PCI_PROTOCOL_VERSION_1_2) > + size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2) > ? sizeof(*res_assigned) : sizeof(*res_assigned2); > > pkt = kmalloc(sizeof(*pkt) + size_res, GFP_KERNEL); > @@ -2461,7 +2459,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev) > pkt->completion_func = hv_pci_generic_compl; > pkt->compl_ctxt = &comp_pkt; > > - if (pci_protocol_version < PCI_PROTOCOL_VERSION_1_2) { > + if (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2) { > res_assigned = > (struct pci_resources_assigned *)&pkt->message; > res_assigned->message_type.type = > @@ -2812,7 +2810,7 @@ static int hv_pci_resume(struct hv_device *hdev) > return ret; > > /* Only use the version that was in use before hibernation. */ > - version[0] = pci_protocol_version; > + version[0] = hbus->protocol_version; > ret = hv_pci_protocol_negotiation(hdev, version, 1); > if (ret) > goto out; > -- > 1.8.3.1 >
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Sent: Thursday, September 26, 2019 9:29 AM > > On Wed, Sep 11, 2019 at 11:38:23PM +0000, Dexuan Cui wrote: > > A VM can have multiple hbus. It looks incorrect for the second hbus's > > hv_pci_protocol_negotiation() to set the global variable > > 'pci_protocol_version' (which was set by the first hbus), even if the > > same value is written. > > > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > > This is a fix that seems unrelated to the rest of the series. Correct. > AFAICS the version also affects code paths in the driver, which > means that in case you have busses with different versions the > current code is wrong in this respect. Correct. > You have to capture this concept in the commit log, it reads as > an optional change but it looks like a potential bug. > > Lorenzo Agreed. Let me improve the commit log in v2. Thanks, -- Dexuan
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index 2655df2..55730c5 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -76,11 +76,6 @@ enum pci_protocol_version_t { PCI_PROTOCOL_VERSION_1_1, }; -/* - * Protocol version negotiated by hv_pci_protocol_negotiation(). - */ -static enum pci_protocol_version_t pci_protocol_version; - #define PCI_CONFIG_MMIO_LENGTH 0x2000 #define CFG_PAGE_OFFSET 0x1000 #define CFG_PAGE_SIZE (PCI_CONFIG_MMIO_LENGTH - CFG_PAGE_OFFSET) @@ -429,6 +424,8 @@ enum hv_pcibus_state { struct hv_pcibus_device { struct pci_sysdata sysdata; + /* Protocol version negotiated with the host */ + enum pci_protocol_version_t protocol_version; enum hv_pcibus_state state; refcount_t remove_lock; struct hv_device *hdev; @@ -942,7 +939,7 @@ static void hv_irq_unmask(struct irq_data *data) * negative effect (yet?). */ - if (pci_protocol_version >= PCI_PROTOCOL_VERSION_1_2) { + if (hbus->protocol_version >= PCI_PROTOCOL_VERSION_1_2) { /* * PCI_PROTOCOL_VERSION_1_2 supports the VP_SET version of the * HVCALL_RETARGET_INTERRUPT hypercall, which also coincides @@ -1112,7 +1109,7 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) ctxt.pci_pkt.completion_func = hv_pci_compose_compl; ctxt.pci_pkt.compl_ctxt = ∁ - switch (pci_protocol_version) { + switch (hbus->protocol_version) { case PCI_PROTOCOL_VERSION_1_1: size = hv_compose_msi_req_v1(&ctxt.int_pkts.v1, dest, @@ -2116,6 +2113,7 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev, enum pci_protocol_version_t version[], int num_version) { + struct hv_pcibus_device *hbus = hv_get_drvdata(hdev); struct pci_version_request *version_req; struct hv_pci_compl comp_pkt; struct pci_packet *pkt; @@ -2155,10 +2153,10 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev, } if (comp_pkt.completion_status >= 0) { - pci_protocol_version = version[i]; + hbus->protocol_version = version[i]; dev_info(&hdev->device, "PCI VMBus probing: Using version %#x\n", - pci_protocol_version); + hbus->protocol_version); goto exit; } @@ -2442,7 +2440,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev) u32 wslot; int ret; - size_res = (pci_protocol_version < PCI_PROTOCOL_VERSION_1_2) + size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2) ? sizeof(*res_assigned) : sizeof(*res_assigned2); pkt = kmalloc(sizeof(*pkt) + size_res, GFP_KERNEL); @@ -2461,7 +2459,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev) pkt->completion_func = hv_pci_generic_compl; pkt->compl_ctxt = &comp_pkt; - if (pci_protocol_version < PCI_PROTOCOL_VERSION_1_2) { + if (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2) { res_assigned = (struct pci_resources_assigned *)&pkt->message; res_assigned->message_type.type = @@ -2812,7 +2810,7 @@ static int hv_pci_resume(struct hv_device *hdev) return ret; /* Only use the version that was in use before hibernation. */ - version[0] = pci_protocol_version; + version[0] = hbus->protocol_version; ret = hv_pci_protocol_negotiation(hdev, version, 1); if (ret) goto out;
A VM can have multiple hbus. It looks incorrect for the second hbus's hv_pci_protocol_negotiation() to set the global variable 'pci_protocol_version' (which was set by the first hbus), even if the same value is written. Signed-off-by: Dexuan Cui <decui@microsoft.com> --- drivers/pci/controller/pci-hyperv.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)