| Message ID | 1459855602-16727-2-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Cao jin <caoj.fnst@cn.fujitsu.com> writes: > This patch comes along with patch "Add param Error ** for msi_init". What do you want to say with this sentence? I think it could be dropped without loss. > Add more newlines to make the code block well separated; add more > comments for msi_init; and fix a indentation. > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > CC: Dmitry Fleytman <dmitry@daynix.com> > CC: Jason Wang <jasowang@redhat.com> > CC: Michael S. Tsirkin <mst@redhat.com> > > --- > hw/net/vmxnet3.c | 2 +- > hw/pci-bridge/ioh3420.c | 7 ++++++- > hw/pci-bridge/pci_bridge_dev.c | 4 ++++ > hw/pci-bridge/xio3130_downstream.c | 6 +++++- > hw/pci-bridge/xio3130_upstream.c | 3 +++ > hw/pci/msi.c | 13 +++++++++++++ > 6 files changed, 32 insertions(+), 3 deletions(-) > > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > index 093a71e..7a38e47 100644 > --- a/hw/net/vmxnet3.c > +++ b/hw/net/vmxnet3.c > @@ -348,7 +348,7 @@ typedef struct { > /* Interrupt management */ > > /* > - *This function returns sign whether interrupt line is in asserted state > + * This function returns sign whether interrupt line is in asserted state > * This depends on the type of interrupt used. For INTX interrupt line will > * be asserted until explicit deassertion, for MSI(X) interrupt line will > * be deasserted automatically due to notification semantics of the MSI(X) > diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c > index 0937fa3..b4a7806 100644 > --- a/hw/pci-bridge/ioh3420.c > +++ b/hw/pci-bridge/ioh3420.c > @@ -106,12 +106,14 @@ static int ioh3420_initfn(PCIDevice *d) > if (rc < 0) { > goto err_bridge; > } > + > rc = msi_init(d, IOH_EP_MSI_OFFSET, IOH_EP_MSI_NR_VECTOR, > IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT, > IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT); > if (rc < 0) { > goto err_bridge; > } > + > rc = pcie_cap_init(d, IOH_EP_EXP_OFFSET, PCI_EXP_TYPE_ROOT_PORT, p->port); > if (rc < 0) { > goto err_msi; > @@ -120,18 +122,21 @@ static int ioh3420_initfn(PCIDevice *d) > pcie_cap_arifwd_init(d); > pcie_cap_deverr_init(d); > pcie_cap_slot_init(d, s->slot); > + pcie_cap_root_init(d); > + > pcie_chassis_create(s->chassis); > rc = pcie_chassis_add_slot(s); > if (rc < 0) { > goto err_pcie_cap; > } > - pcie_cap_root_init(d); > + Code motion, not covered by commit message. Should this be in a later patch? > rc = pcie_aer_init(d, IOH_EP_AER_OFFSET, PCI_ERR_SIZEOF); > if (rc < 0) { > goto err; > } > pcie_aer_root_init(d); > ioh3420_aer_vector_update(d); > + > return 0; > > err: > diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c > index 862a236..32f4daa 100644 > --- a/hw/pci-bridge/pci_bridge_dev.c > +++ b/hw/pci-bridge/pci_bridge_dev.c > @@ -67,10 +67,12 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) > /* MSI is not applicable without SHPC */ > bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ); > } > + > err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0); > if (err) { > goto slotid_error; > } > + > if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) && > msi_nonbroken) { > err = msi_init(dev, 0, 1, true, true); > @@ -78,6 +80,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) > goto msi_error; > } > } > + > if (shpc_present(dev)) { > /* TODO: spec recommends using 64 bit prefetcheable BAR. > * Check whether that works well. */ > @@ -85,6 +88,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) > PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar); > } > return 0; > + > msi_error: > slotid_cap_cleanup(dev); > slotid_error: > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c > index cf1ee63..e6d653d 100644 > --- a/hw/pci-bridge/xio3130_downstream.c > +++ b/hw/pci-bridge/xio3130_downstream.c > @@ -70,11 +70,13 @@ static int xio3130_downstream_initfn(PCIDevice *d) > if (rc < 0) { > goto err_bridge; > } > + > rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET, > XIO3130_SSVID_SVID, XIO3130_SSVID_SSID); > if (rc < 0) { > goto err_bridge; > } > + > rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM, > p->port); > if (rc < 0) { > @@ -83,12 +85,14 @@ static int xio3130_downstream_initfn(PCIDevice *d) > pcie_cap_flr_init(d); > pcie_cap_deverr_init(d); > pcie_cap_slot_init(d, s->slot); > + pcie_cap_arifwd_init(d); > + > pcie_chassis_create(s->chassis); > rc = pcie_chassis_add_slot(s); > if (rc < 0) { > goto err_pcie_cap; > } > - pcie_cap_arifwd_init(d); > + > rc = pcie_aer_init(d, XIO3130_AER_OFFSET, PCI_ERR_SIZEOF); > if (rc < 0) { > goto err; Likewise. > diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c > index 164ef58..d976844 100644 > --- a/hw/pci-bridge/xio3130_upstream.c > +++ b/hw/pci-bridge/xio3130_upstream.c > @@ -66,11 +66,13 @@ static int xio3130_upstream_initfn(PCIDevice *d) > if (rc < 0) { > goto err_bridge; > } > + > rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET, > XIO3130_SSVID_SVID, XIO3130_SSVID_SSID); > if (rc < 0) { > goto err_bridge; > } > + > rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_UPSTREAM, > p->port); > if (rc < 0) { > @@ -78,6 +80,7 @@ static int xio3130_upstream_initfn(PCIDevice *d) > } > pcie_cap_flr_init(d); > pcie_cap_deverr_init(d); > + > rc = pcie_aer_init(d, XIO3130_AER_OFFSET, PCI_ERR_SIZEOF); > if (rc < 0) { > goto err; > diff --git a/hw/pci/msi.c b/hw/pci/msi.c > index e0e64c2..e2a701b 100644 > --- a/hw/pci/msi.c > +++ b/hw/pci/msi.c > @@ -165,6 +165,19 @@ bool msi_enabled(const PCIDevice *dev) > PCI_MSI_FLAGS_ENABLE); > } > > +/* > + * Make PCI device @dev MSI-capable. > + * Non-zero @offset puts capability MSI at that offset in PCI config > + * space. > + * @nr_vectors is the number of MSI vectors (1, 2, 4, 8, 16 or 32). > + * If @msi64bit, make the device capable of sending a 64-bit message > + * address. > + * If @msi_per_vector_mask, make the device support per-vector masking. > + * @errp is for returning errors. > + * Return the offset of capability MSI in config space on success, > + * set @errp and return -errno on error. > + * -ENOTSUP means lacking msi support for a msi-capable platform. Missing: -ENOSPC and -EINVAL. Intentional? > + */ > int msi_init(struct PCIDevice *dev, uint8_t offset, > unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask) > {
On 04/08/2016 02:29 PM, Markus Armbruster wrote: > Cao jin <caoj.fnst@cn.fujitsu.com> writes: > >> This patch comes along with patch "Add param Error ** for msi_init". > > What do you want to say with this sentence? I think it could be dropped > without loss. > According to what I learned here(http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00116.html), generally, I don`t send this kind of coding style patch separately:). So I add that interpretation per suggestion. Will remove it in next version. >> Add more newlines to make the code block well separated; add more >> comments for msi_init; and fix a indentation. >> goto err_pcie_cap; >> } >> - pcie_cap_root_init(d); >> + > > Code motion, not covered by commit message. Should this be in a later > patch? > Yes...I forget to mention it in commit message. It is just put relevant code together, make the code more logical. Because it is so trivial, and this patch is for all the trivial modification , so maybe I can cover it in the commit message:) >> +/* >> + * Make PCI device @dev MSI-capable. >> + * Non-zero @offset puts capability MSI at that offset in PCI config >> + * space. >> + * @nr_vectors is the number of MSI vectors (1, 2, 4, 8, 16 or 32). >> + * If @msi64bit, make the device capable of sending a 64-bit message >> + * address. >> + * If @msi_per_vector_mask, make the device support per-vector masking. >> + * @errp is for returning errors. >> + * Return the offset of capability MSI in config space on success, >> + * set @errp and return -errno on error. >> + * -ENOTSUP means lacking msi support for a msi-capable platform. > > Missing: -ENOSPC and -EINVAL. Intentional? > Yep...a kind of. Because I think -ENOTSUP deserve more comments, and the other two, the meaning are as the name implies. I will make it up
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index 093a71e..7a38e47 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -348,7 +348,7 @@ typedef struct { /* Interrupt management */ /* - *This function returns sign whether interrupt line is in asserted state + * This function returns sign whether interrupt line is in asserted state * This depends on the type of interrupt used. For INTX interrupt line will * be asserted until explicit deassertion, for MSI(X) interrupt line will * be deasserted automatically due to notification semantics of the MSI(X) diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c index 0937fa3..b4a7806 100644 --- a/hw/pci-bridge/ioh3420.c +++ b/hw/pci-bridge/ioh3420.c @@ -106,12 +106,14 @@ static int ioh3420_initfn(PCIDevice *d) if (rc < 0) { goto err_bridge; } + rc = msi_init(d, IOH_EP_MSI_OFFSET, IOH_EP_MSI_NR_VECTOR, IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT, IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT); if (rc < 0) { goto err_bridge; } + rc = pcie_cap_init(d, IOH_EP_EXP_OFFSET, PCI_EXP_TYPE_ROOT_PORT, p->port); if (rc < 0) { goto err_msi; @@ -120,18 +122,21 @@ static int ioh3420_initfn(PCIDevice *d) pcie_cap_arifwd_init(d); pcie_cap_deverr_init(d); pcie_cap_slot_init(d, s->slot); + pcie_cap_root_init(d); + pcie_chassis_create(s->chassis); rc = pcie_chassis_add_slot(s); if (rc < 0) { goto err_pcie_cap; } - pcie_cap_root_init(d); + rc = pcie_aer_init(d, IOH_EP_AER_OFFSET, PCI_ERR_SIZEOF); if (rc < 0) { goto err; } pcie_aer_root_init(d); ioh3420_aer_vector_update(d); + return 0; err: diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c index 862a236..32f4daa 100644 --- a/hw/pci-bridge/pci_bridge_dev.c +++ b/hw/pci-bridge/pci_bridge_dev.c @@ -67,10 +67,12 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) /* MSI is not applicable without SHPC */ bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ); } + err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0); if (err) { goto slotid_error; } + if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) && msi_nonbroken) { err = msi_init(dev, 0, 1, true, true); @@ -78,6 +80,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) goto msi_error; } } + if (shpc_present(dev)) { /* TODO: spec recommends using 64 bit prefetcheable BAR. * Check whether that works well. */ @@ -85,6 +88,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar); } return 0; + msi_error: slotid_cap_cleanup(dev); slotid_error: diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c index cf1ee63..e6d653d 100644 --- a/hw/pci-bridge/xio3130_downstream.c +++ b/hw/pci-bridge/xio3130_downstream.c @@ -70,11 +70,13 @@ static int xio3130_downstream_initfn(PCIDevice *d) if (rc < 0) { goto err_bridge; } + rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET, XIO3130_SSVID_SVID, XIO3130_SSVID_SSID); if (rc < 0) { goto err_bridge; } + rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM, p->port); if (rc < 0) { @@ -83,12 +85,14 @@ static int xio3130_downstream_initfn(PCIDevice *d) pcie_cap_flr_init(d); pcie_cap_deverr_init(d); pcie_cap_slot_init(d, s->slot); + pcie_cap_arifwd_init(d); + pcie_chassis_create(s->chassis); rc = pcie_chassis_add_slot(s); if (rc < 0) { goto err_pcie_cap; } - pcie_cap_arifwd_init(d); + rc = pcie_aer_init(d, XIO3130_AER_OFFSET, PCI_ERR_SIZEOF); if (rc < 0) { goto err; diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c index 164ef58..d976844 100644 --- a/hw/pci-bridge/xio3130_upstream.c +++ b/hw/pci-bridge/xio3130_upstream.c @@ -66,11 +66,13 @@ static int xio3130_upstream_initfn(PCIDevice *d) if (rc < 0) { goto err_bridge; } + rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET, XIO3130_SSVID_SVID, XIO3130_SSVID_SSID); if (rc < 0) { goto err_bridge; } + rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_UPSTREAM, p->port); if (rc < 0) { @@ -78,6 +80,7 @@ static int xio3130_upstream_initfn(PCIDevice *d) } pcie_cap_flr_init(d); pcie_cap_deverr_init(d); + rc = pcie_aer_init(d, XIO3130_AER_OFFSET, PCI_ERR_SIZEOF); if (rc < 0) { goto err; diff --git a/hw/pci/msi.c b/hw/pci/msi.c index e0e64c2..e2a701b 100644 --- a/hw/pci/msi.c +++ b/hw/pci/msi.c @@ -165,6 +165,19 @@ bool msi_enabled(const PCIDevice *dev) PCI_MSI_FLAGS_ENABLE); } +/* + * Make PCI device @dev MSI-capable. + * Non-zero @offset puts capability MSI at that offset in PCI config + * space. + * @nr_vectors is the number of MSI vectors (1, 2, 4, 8, 16 or 32). + * If @msi64bit, make the device capable of sending a 64-bit message + * address. + * If @msi_per_vector_mask, make the device support per-vector masking. + * @errp is for returning errors. + * Return the offset of capability MSI in config space on success, + * set @errp and return -errno on error. + * -ENOTSUP means lacking msi support for a msi-capable platform. + */ int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask) {
This patch comes along with patch "Add param Error ** for msi_init". Add more newlines to make the code block well separated; add more comments for msi_init; and fix a indentation. Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> CC: Dmitry Fleytman <dmitry@daynix.com> CC: Jason Wang <jasowang@redhat.com> CC: Michael S. Tsirkin <mst@redhat.com> --- hw/net/vmxnet3.c | 2 +- hw/pci-bridge/ioh3420.c | 7 ++++++- hw/pci-bridge/pci_bridge_dev.c | 4 ++++ hw/pci-bridge/xio3130_downstream.c | 6 +++++- hw/pci-bridge/xio3130_upstream.c | 3 +++ hw/pci/msi.c | 13 +++++++++++++ 6 files changed, 32 insertions(+), 3 deletions(-)