* [Qemu-devel] [PATCH v5 01/11] fix some coding style problems
2016-05-06 4:20 [Qemu-devel] [PATCH v5 00/11] Add param Error ** for msi_init() Cao jin
@ 2016-05-06 4:20 ` Cao jin
2016-05-15 12:36 ` Marcel Apfelbaum
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 02/11] change pvscsi_init_msi() type to void Cao jin
` (9 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-06 4:20 UTC (permalink / raw)
To: qemu-devel
Cc: Dmitry Fleytman, Jason Wang, Michael S. Tsirkin,
Markus Armbruster, Marcel Apfelbaum
It has:
1. More newlines make the code block well separated.
2. Add more comments for msi_init.
3. Fix a indentation in vmxnet3.c.
4. ioh3420 & xio3130_downstream: put PCI Express capability init function
together, make it more readable.
cc: Dmitry Fleytman <dmitry@daynix.com>
cc: Jason Wang <jasowang@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.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 | 19 +++++++++++++++++++
6 files changed, 38 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);
+
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..dd9d957 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -165,6 +165,25 @@ 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.
+ * -ENOSPC means running out of PCI config space, happens when @offset is 0,
+ * which means a programming error.
+ * -EINVAL means capability overlap, happens when @offset is non-zero,
+ * also means a programming error, except device assignment, which can check
+ * if a real HW is broken.
+ */
int msi_init(struct PCIDevice *dev, uint8_t offset,
unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
{
--
2.1.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 01/11] fix some coding style problems
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 01/11] fix some coding style problems Cao jin
@ 2016-05-15 12:36 ` Marcel Apfelbaum
0 siblings, 0 replies; 28+ messages in thread
From: Marcel Apfelbaum @ 2016-05-15 12:36 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: Dmitry Fleytman, Jason Wang, Michael S. Tsirkin,
Markus Armbruster
On 05/06/2016 07:20 AM, Cao jin wrote:
> It has:
> 1. More newlines make the code block well separated.
> 2. Add more comments for msi_init.
> 3. Fix a indentation in vmxnet3.c.
> 4. ioh3420 & xio3130_downstream: put PCI Express capability init function
> together, make it more readable.
>
> cc: Dmitry Fleytman <dmitry@daynix.com>
> cc: Jason Wang <jasowang@redhat.com>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.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 | 19 +++++++++++++++++++
> 6 files changed, 38 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);
> +
> 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..dd9d957 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -165,6 +165,25 @@ 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.
> + * -ENOSPC means running out of PCI config space, happens when @offset is 0,
> + * which means a programming error.
> + * -EINVAL means capability overlap, happens when @offset is non-zero,
> + * also means a programming error, except device assignment, which can check
> + * if a real HW is broken.
> + */
> int msi_init(struct PCIDevice *dev, uint8_t offset,
> unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
> {
>
Hi,
Looks OK to me.
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v5 02/11] change pvscsi_init_msi() type to void
2016-05-06 4:20 [Qemu-devel] [PATCH v5 00/11] Add param Error ** for msi_init() Cao jin
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 01/11] fix some coding style problems Cao jin
@ 2016-05-06 4:20 ` Cao jin
2016-05-06 5:48 ` Cao jin
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 03/11] megasas: Fix Cao jin
` (8 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-06 4:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini
Nobody use its return value, so change the type to void.
cc: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Dmitry Fleytman <dmitry@daynix.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/vmw_pvscsi.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 9abc086..4ce3581 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1039,7 +1039,7 @@ pvscsi_io_read(void *opaque, hwaddr addr, unsigned size)
}
-static bool
+static void
pvscsi_init_msi(PVSCSIState *s)
{
int res;
@@ -1053,8 +1053,6 @@ pvscsi_init_msi(PVSCSIState *s)
} else {
s->msi_used = true;
}
-
- return s->msi_used;
}
static void
--
2.1.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 02/11] change pvscsi_init_msi() type to void
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 02/11] change pvscsi_init_msi() type to void Cao jin
@ 2016-05-06 5:48 ` Cao jin
0 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-05-06 5:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Marcel Apfelbaum, dmitry, Markus Armbruster
forget to cc some maintainers
On 05/06/2016 12:20 PM, Cao jin wrote:
> Nobody use its return value, so change the type to void.
>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> Acked-by: Dmitry Fleytman <dmitry@daynix.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> hw/scsi/vmw_pvscsi.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index 9abc086..4ce3581 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1039,7 +1039,7 @@ pvscsi_io_read(void *opaque, hwaddr addr, unsigned size)
> }
>
>
> -static bool
> +static void
> pvscsi_init_msi(PVSCSIState *s)
> {
> int res;
> @@ -1053,8 +1053,6 @@ pvscsi_init_msi(PVSCSIState *s)
> } else {
> s->msi_used = true;
> }
> -
> - return s->msi_used;
> }
>
> static void
>
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v5 03/11] megasas: Fix
2016-05-06 4:20 [Qemu-devel] [PATCH v5 00/11] Add param Error ** for msi_init() Cao jin
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 01/11] fix some coding style problems Cao jin
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 02/11] change pvscsi_init_msi() type to void Cao jin
@ 2016-05-06 4:20 ` Cao jin
2016-05-06 5:43 ` Cao jin
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 04/11] mptsas: change .realize function name Cao jin
` (7 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-06 4:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Hannes Reinecke, Paolo Bonzini
msi_init returns non-zero value on both failure and success.
cc: Hannes Reinecke <hare@suse.de>
cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/megasas.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index a63a581..56fb645 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2348,7 +2348,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
"megasas-queue", 0x40000);
if (megasas_use_msi(s) &&
- msi_init(dev, 0x50, 1, true, false)) {
+ msi_init(dev, 0x50, 1, true, false) < 0) {
s->flags &= ~MEGASAS_MASK_USE_MSI;
}
if (megasas_use_msix(s) &&
--
2.1.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 03/11] megasas: Fix
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 03/11] megasas: Fix Cao jin
@ 2016-05-06 5:43 ` Cao jin
2016-05-15 12:37 ` Marcel Apfelbaum
0 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-06 5:43 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Hannes Reinecke, Markus Armbruster,
Marcel Apfelbaum
sorry, forget to cc some maintainers
On 05/06/2016 12:20 PM, Cao jin wrote:
> msi_init returns non-zero value on both failure and success.
>
> cc: Hannes Reinecke <hare@suse.de>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> hw/scsi/megasas.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index a63a581..56fb645 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2348,7 +2348,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
> "megasas-queue", 0x40000);
>
> if (megasas_use_msi(s) &&
> - msi_init(dev, 0x50, 1, true, false)) {
> + msi_init(dev, 0x50, 1, true, false) < 0) {
> s->flags &= ~MEGASAS_MASK_USE_MSI;
> }
> if (megasas_use_msix(s) &&
>
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 03/11] megasas: Fix
2016-05-06 5:43 ` Cao jin
@ 2016-05-15 12:37 ` Marcel Apfelbaum
0 siblings, 0 replies; 28+ messages in thread
From: Marcel Apfelbaum @ 2016-05-15 12:37 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: Paolo Bonzini, Hannes Reinecke, Markus Armbruster
On 05/06/2016 08:43 AM, Cao jin wrote:
> sorry, forget to cc some maintainers
>
> On 05/06/2016 12:20 PM, Cao jin wrote:
>> msi_init returns non-zero value on both failure and success.
>>
>> cc: Hannes Reinecke <hare@suse.de>
>> cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>> hw/scsi/megasas.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index a63a581..56fb645 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -2348,7 +2348,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>> "megasas-queue", 0x40000);
>>
>> if (megasas_use_msi(s) &&
>> - msi_init(dev, 0x50, 1, true, false)) {
>> + msi_init(dev, 0x50, 1, true, false) < 0) {
>> s->flags &= ~MEGASAS_MASK_USE_MSI;
>> }
>> if (megasas_use_msix(s) &&
>>
>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v5 04/11] mptsas: change .realize function name
2016-05-06 4:20 [Qemu-devel] [PATCH v5 00/11] Add param Error ** for msi_init() Cao jin
` (2 preceding siblings ...)
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 03/11] megasas: Fix Cao jin
@ 2016-05-06 4:20 ` Cao jin
2016-05-06 5:53 ` Cao jin
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 05/11] usb xhci: change msi/msix property type Cao jin
` (6 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-06 4:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini
All the other devices` .realize function name are xxx_realize, except this one.
cc: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/mptsas.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 499c146..1c18c84 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1270,7 +1270,7 @@ static const struct SCSIBusInfo mptsas_scsi_info = {
.load_request = mptsas_load_request,
};
-static void mptsas_scsi_init(PCIDevice *dev, Error **errp)
+static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
{
DeviceState *d = DEVICE(dev);
MPTSASState *s = MPT_SAS(dev);
@@ -1413,7 +1413,7 @@ static void mptsas1068_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
- pc->realize = mptsas_scsi_init;
+ pc->realize = mptsas_scsi_realize;
pc->exit = mptsas_scsi_uninit;
pc->romfile = 0;
pc->vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
--
2.1.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 04/11] mptsas: change .realize function name
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 04/11] mptsas: change .realize function name Cao jin
@ 2016-05-06 5:53 ` Cao jin
0 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-05-06 5:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Markus Armbruster, Marcel Apfelbaum
forget to cc some maintainers
On 05/06/2016 12:20 PM, Cao jin wrote:
> All the other devices` .realize function name are xxx_realize, except this one.
>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> hw/scsi/mptsas.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index 499c146..1c18c84 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -1270,7 +1270,7 @@ static const struct SCSIBusInfo mptsas_scsi_info = {
> .load_request = mptsas_load_request,
> };
>
> -static void mptsas_scsi_init(PCIDevice *dev, Error **errp)
> +static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
> {
> DeviceState *d = DEVICE(dev);
> MPTSASState *s = MPT_SAS(dev);
> @@ -1413,7 +1413,7 @@ static void mptsas1068_class_init(ObjectClass *oc, void *data)
> DeviceClass *dc = DEVICE_CLASS(oc);
> PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
>
> - pc->realize = mptsas_scsi_init;
> + pc->realize = mptsas_scsi_realize;
> pc->exit = mptsas_scsi_uninit;
> pc->romfile = 0;
> pc->vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
>
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v5 05/11] usb xhci: change msi/msix property type
2016-05-06 4:20 [Qemu-devel] [PATCH v5 00/11] Add param Error ** for msi_init() Cao jin
` (3 preceding siblings ...)
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 04/11] mptsas: change .realize function name Cao jin
@ 2016-05-06 4:20 ` Cao jin
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 06/11] intel-hda: change msi " Cao jin
` (5 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-05-06 4:20 UTC (permalink / raw)
To: qemu-devel
Cc: Gerd Hoffmann, Michael S. Tsirkin, Markus Armbruster,
Marcel Apfelbaum
>From bit to enum OnOffAuto.
cc: Gerd Hoffmann <kraxel@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/usb/hcd-xhci.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index bcde8a2..6d70289 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -461,6 +461,8 @@ struct XHCIState {
uint32_t numslots;
uint32_t flags;
uint32_t max_pstreams_mask;
+ OnOffAuto msi;
+ OnOffAuto msix;
/* Operational Registers */
uint32_t usbcmd;
@@ -498,9 +500,7 @@ typedef struct XHCIEvRingSeg {
} XHCIEvRingSeg;
enum xhci_flags {
- XHCI_FLAG_USE_MSI = 1,
- XHCI_FLAG_USE_MSI_X,
- XHCI_FLAG_SS_FIRST,
+ XHCI_FLAG_SS_FIRST = 1,
XHCI_FLAG_FORCE_PCIE_ENDCAP,
XHCI_FLAG_ENABLE_STREAMS,
};
@@ -3645,10 +3645,12 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
assert(ret >= 0);
}
- if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) {
+ if (xhci->msi == ON_OFF_AUTO_ON ||
+ xhci->msi == ON_OFF_AUTO_AUTO) {
msi_init(dev, 0x70, xhci->numintrs, true, false);
}
- if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) {
+ if (xhci->msix == ON_OFF_AUTO_ON ||
+ xhci->msix == ON_OFF_AUTO_AUTO) {
msix_init(dev, xhci->numintrs,
&xhci->mem, 0, OFF_MSIX_TABLE,
&xhci->mem, 0, OFF_MSIX_PBA,
@@ -3869,8 +3871,8 @@ static const VMStateDescription vmstate_xhci = {
};
static Property xhci_properties[] = {
- DEFINE_PROP_BIT("msi", XHCIState, flags, XHCI_FLAG_USE_MSI, true),
- DEFINE_PROP_BIT("msix", XHCIState, flags, XHCI_FLAG_USE_MSI_X, true),
+ DEFINE_PROP_ON_OFF_AUTO("msi", XHCIState, msi, ON_OFF_AUTO_AUTO),
+ DEFINE_PROP_ON_OFF_AUTO("msix", XHCIState, msix, ON_OFF_AUTO_AUTO),
DEFINE_PROP_BIT("superspeed-ports-first",
XHCIState, flags, XHCI_FLAG_SS_FIRST, true),
DEFINE_PROP_BIT("force-pcie-endcap", XHCIState, flags,
--
2.1.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v5 06/11] intel-hda: change msi property type
2016-05-06 4:20 [Qemu-devel] [PATCH v5 00/11] Add param Error ** for msi_init() Cao jin
` (4 preceding siblings ...)
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 05/11] usb xhci: change msi/msix property type Cao jin
@ 2016-05-06 4:20 ` Cao jin
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 07/11] mptsas: " Cao jin
` (4 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-05-06 4:20 UTC (permalink / raw)
To: qemu-devel
Cc: Gerd Hoffmann, Michael S. Tsirkin, Markus Armbruster,
Marcel Apfelbaum
>From uint32 to enum OnOffAuto.
cc: Gerd Hoffmann <kraxel@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/audio/intel-hda.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index d372d4a..61362e5 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -187,7 +187,7 @@ struct IntelHDAState {
/* properties */
uint32_t debug;
- uint32_t msi;
+ OnOffAuto msi;
bool old_msi_addr;
};
@@ -1142,7 +1142,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d,
"intel-hda", 0x4000);
pci_register_bar(&d->pci, 0, 0, &d->mmio);
- if (d->msi) {
+ if (d->msi == ON_OFF_AUTO_AUTO ||
+ d->msi == ON_OFF_AUTO_ON) {
msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
}
@@ -1234,7 +1235,7 @@ static const VMStateDescription vmstate_intel_hda = {
static Property intel_hda_properties[] = {
DEFINE_PROP_UINT32("debug", IntelHDAState, debug, 0),
- DEFINE_PROP_UINT32("msi", IntelHDAState, msi, 1),
+ DEFINE_PROP_ON_OFF_AUTO("msi", IntelHDAState, msi, ON_OFF_AUTO_AUTO),
DEFINE_PROP_BOOL("old_msi_addr", IntelHDAState, old_msi_addr, false),
DEFINE_PROP_END_OF_LIST(),
};
--
2.1.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v5 07/11] mptsas: change msi property type
2016-05-06 4:20 [Qemu-devel] [PATCH v5 00/11] Add param Error ** for msi_init() Cao jin
` (5 preceding siblings ...)
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 06/11] intel-hda: change msi " Cao jin
@ 2016-05-06 4:20 ` Cao jin
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 08/11] megasas: change msi/msix " Cao jin
` (3 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-05-06 4:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Markus Armbruster
>From uint32 to enum OnOffAuto, and give it a shorter name.
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/mptsas.c | 4 ++--
hw/scsi/mptsas.h | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 1c18c84..afee576 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1285,7 +1285,7 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s,
"mptsas-diag", 0x10000);
- if (s->msi_available &&
+ if ((s->msi == ON_OFF_AUTO_AUTO || s->msi == ON_OFF_AUTO_ON) &&
msi_init(dev, 0, 1, true, false) >= 0) {
s->msi_in_use = true;
}
@@ -1404,7 +1404,7 @@ static const VMStateDescription vmstate_mptsas = {
static Property mptsas_properties[] = {
DEFINE_PROP_UINT64("sas_address", MPTSASState, sas_addr, 0),
/* TODO: test MSI support under Windows */
- DEFINE_PROP_BIT("msi", MPTSASState, msi_available, 0, true),
+ DEFINE_PROP_ON_OFF_AUTO("msi", MPTSASState, msi, ON_OFF_AUTO_AUTO),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/scsi/mptsas.h b/hw/scsi/mptsas.h
index 595f81f..0436a33 100644
--- a/hw/scsi/mptsas.h
+++ b/hw/scsi/mptsas.h
@@ -27,7 +27,8 @@ struct MPTSASState {
MemoryRegion diag_io;
QEMUBH *request_bh;
- uint32_t msi_available;
+ /* properties */
+ OnOffAuto msi;
uint64_t sas_addr;
bool msi_in_use;
--
2.1.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v5 08/11] megasas: change msi/msix property type
2016-05-06 4:20 [Qemu-devel] [PATCH v5 00/11] Add param Error ** for msi_init() Cao jin
` (6 preceding siblings ...)
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 07/11] mptsas: " Cao jin
@ 2016-05-06 4:20 ` Cao jin
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 09/11] pci bridge dev: change msi " Cao jin
` (2 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-05-06 4:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Hannes Reinecke, Paolo Bonzini, Markus Armbruster
>From bit to enum OnOffAuto.
cc: Hannes Reinecke <hare@suse.de>
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/megasas.c | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 56fb645..e71a28b 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -48,11 +48,7 @@
#define MEGASAS_FLAG_USE_JBOD 0
#define MEGASAS_MASK_USE_JBOD (1 << MEGASAS_FLAG_USE_JBOD)
-#define MEGASAS_FLAG_USE_MSI 1
-#define MEGASAS_MASK_USE_MSI (1 << MEGASAS_FLAG_USE_MSI)
-#define MEGASAS_FLAG_USE_MSIX 2
-#define MEGASAS_MASK_USE_MSIX (1 << MEGASAS_FLAG_USE_MSIX)
-#define MEGASAS_FLAG_USE_QUEUE64 3
+#define MEGASAS_FLAG_USE_QUEUE64 1
#define MEGASAS_MASK_USE_QUEUE64 (1 << MEGASAS_FLAG_USE_QUEUE64)
static const char *mfi_frame_desc[] = {
@@ -96,6 +92,8 @@ typedef struct MegasasState {
int busy;
int diag;
int adp_reset;
+ OnOffAuto msi;
+ OnOffAuto msix;
MegasasCmd *event_cmd;
int event_locale;
@@ -159,12 +157,12 @@ static bool megasas_use_queue64(MegasasState *s)
static bool megasas_use_msi(MegasasState *s)
{
- return s->flags & MEGASAS_MASK_USE_MSI;
+ return s->msi == ON_OFF_AUTO_AUTO || s->msi == ON_OFF_AUTO_ON;
}
static bool megasas_use_msix(MegasasState *s)
{
- return s->flags & MEGASAS_MASK_USE_MSIX;
+ return s->msix == ON_OFF_AUTO_AUTO || s->msix == ON_OFF_AUTO_ON;
}
static bool megasas_is_jbod(MegasasState *s)
@@ -2349,12 +2347,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
if (megasas_use_msi(s) &&
msi_init(dev, 0x50, 1, true, false) < 0) {
- s->flags &= ~MEGASAS_MASK_USE_MSI;
+ s->msi = ON_OFF_AUTO_OFF;
}
if (megasas_use_msix(s) &&
msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
&s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
- s->flags &= ~MEGASAS_MASK_USE_MSIX;
+ s->msix = ON_OFF_AUTO_OFF;
}
if (pci_is_express(dev)) {
pcie_endpoint_cap_init(dev, 0xa0);
@@ -2422,10 +2420,8 @@ static Property megasas_properties_gen1[] = {
MEGASAS_DEFAULT_FRAMES),
DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial),
DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0),
- DEFINE_PROP_BIT("use_msi", MegasasState, flags,
- MEGASAS_FLAG_USE_MSI, false),
- DEFINE_PROP_BIT("use_msix", MegasasState, flags,
- MEGASAS_FLAG_USE_MSIX, false),
+ DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO),
+ DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO),
DEFINE_PROP_BIT("use_jbod", MegasasState, flags,
MEGASAS_FLAG_USE_JBOD, false),
DEFINE_PROP_END_OF_LIST(),
@@ -2438,10 +2434,8 @@ static Property megasas_properties_gen2[] = {
MEGASAS_GEN2_DEFAULT_FRAMES),
DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial),
DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0),
- DEFINE_PROP_BIT("use_msi", MegasasState, flags,
- MEGASAS_FLAG_USE_MSI, true),
- DEFINE_PROP_BIT("use_msix", MegasasState, flags,
- MEGASAS_FLAG_USE_MSIX, true),
+ DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO),
+ DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO),
DEFINE_PROP_BIT("use_jbod", MegasasState, flags,
MEGASAS_FLAG_USE_JBOD, false),
DEFINE_PROP_END_OF_LIST(),
--
2.1.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v5 09/11] pci bridge dev: change msi property type
2016-05-06 4:20 [Qemu-devel] [PATCH v5 00/11] Add param Error ** for msi_init() Cao jin
` (7 preceding siblings ...)
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 08/11] megasas: change msi/msix " Cao jin
@ 2016-05-06 4:20 ` Cao jin
2016-05-15 13:25 ` Marcel Apfelbaum
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 10/11] pci core: assert ENOSPC when add capability Cao jin
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 11/11] pci: Convert msi_init() to Error and fix callers to check it Cao jin
10 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-06 4:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin, Markus Armbruster, Marcel Apfelbaum
>From bit to enum OnOffAuto.
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
Actually, I am not quite sure this device need this change, RFC.
hw/pci-bridge/pci_bridge_dev.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 32f4daa..9e31f0e 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -41,9 +41,10 @@ struct PCIBridgeDev {
MemoryRegion bar;
uint8_t chassis_nr;
-#define PCI_BRIDGE_DEV_F_MSI_REQ 0
-#define PCI_BRIDGE_DEV_F_SHPC_REQ 1
+#define PCI_BRIDGE_DEV_F_SHPC_REQ 0
uint32_t flags;
+
+ OnOffAuto msi;
};
typedef struct PCIBridgeDev PCIBridgeDev;
@@ -65,7 +66,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
}
} else {
/* MSI is not applicable without SHPC */
- bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ);
+ bridge_dev->msi = ON_OFF_AUTO_OFF;
}
err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
@@ -73,7 +74,8 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
goto slotid_error;
}
- if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
+ if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
+ bridge_dev->msi == ON_OFF_AUTO_ON) &&
msi_nonbroken) {
err = msi_init(dev, 0, 1, true, true);
if (err < 0) {
@@ -146,8 +148,8 @@ static Property pci_bridge_dev_properties[] = {
/* Note: 0 is not a legal chassis number. */
DEFINE_PROP_UINT8(PCI_BRIDGE_DEV_PROP_CHASSIS_NR, PCIBridgeDev, chassis_nr,
0),
- DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, flags,
- PCI_BRIDGE_DEV_F_MSI_REQ, true),
+ DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi,
+ ON_OFF_AUTO_AUTO),
DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
PCI_BRIDGE_DEV_F_SHPC_REQ, true),
DEFINE_PROP_END_OF_LIST(),
--
2.1.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 09/11] pci bridge dev: change msi property type
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 09/11] pci bridge dev: change msi " Cao jin
@ 2016-05-15 13:25 ` Marcel Apfelbaum
2016-05-17 7:39 ` Cao jin
0 siblings, 1 reply; 28+ messages in thread
From: Marcel Apfelbaum @ 2016-05-15 13:25 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: Michael S. Tsirkin, Markus Armbruster
On 05/06/2016 07:20 AM, Cao jin wrote:
> From bit to enum OnOffAuto.
>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>
> Actually, I am not quite sure this device need this change, RFC.
>
Well, it already has the 'msi' property, so we may want to make it standard 'OnOffAuto'.
One problem I can see is the change of semantics. Until now msi=on means 'auto'. From now on
it means 'force msi=on', fail otherwise. If I got this right, old machines having msi=on
will failed to start on platforms with msibroken=true, right?
Maybe we should preserve the semantics for old machines? (this patch does not actually
affect the semantics, but patch 11/11 should, otherwise why change it to OnOffAuto, right? )
Thanks,
Marcel
> hw/pci-bridge/pci_bridge_dev.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 32f4daa..9e31f0e 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -41,9 +41,10 @@ struct PCIBridgeDev {
>
> MemoryRegion bar;
> uint8_t chassis_nr;
> -#define PCI_BRIDGE_DEV_F_MSI_REQ 0
> -#define PCI_BRIDGE_DEV_F_SHPC_REQ 1
> +#define PCI_BRIDGE_DEV_F_SHPC_REQ 0
> uint32_t flags;
> +
> + OnOffAuto msi;
> };
> typedef struct PCIBridgeDev PCIBridgeDev;
>
> @@ -65,7 +66,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> }
> } else {
> /* MSI is not applicable without SHPC */
> - bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ);
> + bridge_dev->msi = ON_OFF_AUTO_OFF;
> }
>
> err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
> @@ -73,7 +74,8 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> goto slotid_error;
> }
>
> - if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
> + if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
> + bridge_dev->msi == ON_OFF_AUTO_ON) &&
> msi_nonbroken) {
> err = msi_init(dev, 0, 1, true, true);
> if (err < 0) {
> @@ -146,8 +148,8 @@ static Property pci_bridge_dev_properties[] = {
> /* Note: 0 is not a legal chassis number. */
> DEFINE_PROP_UINT8(PCI_BRIDGE_DEV_PROP_CHASSIS_NR, PCIBridgeDev, chassis_nr,
> 0),
> - DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, flags,
> - PCI_BRIDGE_DEV_F_MSI_REQ, true),
> + DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi,
> + ON_OFF_AUTO_AUTO),
> DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
> PCI_BRIDGE_DEV_F_SHPC_REQ, true),
> DEFINE_PROP_END_OF_LIST(),
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 09/11] pci bridge dev: change msi property type
2016-05-15 13:25 ` Marcel Apfelbaum
@ 2016-05-17 7:39 ` Cao jin
2016-05-17 7:38 ` Michael S. Tsirkin
0 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-17 7:39 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: Michael S. Tsirkin, Markus Armbruster
On 05/15/2016 09:25 PM, Marcel Apfelbaum wrote:
> On 05/06/2016 07:20 AM, Cao jin wrote:
>> From bit to enum OnOffAuto.
>>
>> cc: Michael S. Tsirkin <mst@redhat.com>
>> cc: Markus Armbruster <armbru@redhat.com>
>> cc: Marcel Apfelbaum <marcel@redhat.com>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>
>> Actually, I am not quite sure this device need this change, RFC.
>>
>
> Well, it already has the 'msi' property, so we may want to make it
> standard 'OnOffAuto'.
> One problem I can see is the change of semantics. Until now msi=on means
> 'auto'. From now on
> it means 'force msi=on', fail otherwise. If I got this right, old
> machines having msi=on
> will failed to start on platforms with msibroken=true, right?
Exactly, and patch 11 indeed has semantics change. According to what we
discussed before: "if user want msi=on explicitly on command line, then
msi_init`s failure should result in failing to create device", this
semantics change seems can`t be avoid.
> Maybe we should preserve the semantics for old machines? (this patch
> does not actually
> affect the semantics, but patch 11/11 should, otherwise why change it to
> OnOffAuto, right? )
IMHO, in this case, keep compat maybe a burden on us, and make
command-line confusing. See my understanding:
before:
command line format is msi=on/off, and 'on' means auto
now:
we change command line to msi=on/off/auto, and keep compat is meaning
'on' still means auto? If so, why need 'auto'
So, I am thinking, maybe we can help old machine user update their
configuration, I mean: when they set msi=on on platforms with
msibroken=true, they definitely fail, so we should give a clear hint in
error message to help them know what should do to start the machine,
like "please use msi=auto and try again"
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v5 09/11] pci bridge dev: change msi property type
2016-05-17 7:39 ` Cao jin
@ 2016-05-17 7:38 ` Michael S. Tsirkin
2016-05-23 2:22 ` Cao jin
0 siblings, 1 reply; 28+ messages in thread
From: Michael S. Tsirkin @ 2016-05-17 7:38 UTC (permalink / raw)
To: Cao jin; +Cc: Marcel Apfelbaum, qemu-devel, Markus Armbruster
On Tue, May 17, 2016 at 03:39:14PM +0800, Cao jin wrote:
>
>
> On 05/15/2016 09:25 PM, Marcel Apfelbaum wrote:
> >On 05/06/2016 07:20 AM, Cao jin wrote:
> >> From bit to enum OnOffAuto.
> >>
> >>cc: Michael S. Tsirkin <mst@redhat.com>
> >>cc: Markus Armbruster <armbru@redhat.com>
> >>cc: Marcel Apfelbaum <marcel@redhat.com>
> >>Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> >>---
> >>
> >>Actually, I am not quite sure this device need this change, RFC.
> >>
> >
> >Well, it already has the 'msi' property, so we may want to make it
> >standard 'OnOffAuto'.
> >One problem I can see is the change of semantics. Until now msi=on means
> >'auto'. From now on
> >it means 'force msi=on', fail otherwise. If I got this right, old
> >machines having msi=on
> >will failed to start on platforms with msibroken=true, right?
>
> Exactly, and patch 11 indeed has semantics change. According to what we
> discussed before: "if user want msi=on explicitly on command line, then
> msi_init`s failure should result in failing to create device", this
> semantics change seems can`t be avoid.
>
> >Maybe we should preserve the semantics for old machines? (this patch
> >does not actually
> >affect the semantics, but patch 11/11 should, otherwise why change it to
> >OnOffAuto, right? )
>
> IMHO, in this case, keep compat maybe a burden on us, and make command-line
> confusing. See my understanding:
>
> before:
> command line format is msi=on/off, and 'on' means auto
>
> now:
> we change command line to msi=on/off/auto, and keep compat is meaning 'on'
> still means auto? If so, why need 'auto'
>
> So, I am thinking, maybe we can help old machine user update their
> configuration, I mean: when they set msi=on on platforms with
> msibroken=true, they definitely fail, so we should give a clear hint in
> error message to help them know what should do to start the machine, like
> "please use msi=auto and try again"
Personally I don't consider this a big issue. I don't think
many people specify msi=on.
> --
> Yours Sincerely,
>
> Cao jin
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v5 09/11] pci bridge dev: change msi property type
2016-05-17 7:38 ` Michael S. Tsirkin
@ 2016-05-23 2:22 ` Cao jin
2016-05-23 9:55 ` Marcel Apfelbaum
0 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-23 2:22 UTC (permalink / raw)
To: Michael S. Tsirkin, Marcel Apfelbaum, Markus Armbruster; +Cc: qemu-devel
On 05/17/2016 03:38 PM, Michael S. Tsirkin wrote:
> On Tue, May 17, 2016 at 03:39:14PM +0800, Cao jin wrote:
>>
>>
> Personally I don't consider this a big issue. I don't think
> many people specify msi=on.
>
I also think so:) If so, the semantics won`t change.
Marcel, do you think it is ok if I add some info in error message to
help user to start machine?
And Markus, do you have comments about it?
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v5 09/11] pci bridge dev: change msi property type
2016-05-23 2:22 ` Cao jin
@ 2016-05-23 9:55 ` Marcel Apfelbaum
0 siblings, 0 replies; 28+ messages in thread
From: Marcel Apfelbaum @ 2016-05-23 9:55 UTC (permalink / raw)
To: Cao jin, Michael S. Tsirkin, Markus Armbruster; +Cc: qemu-devel
On 05/23/2016 05:22 AM, Cao jin wrote:
>
>
> On 05/17/2016 03:38 PM, Michael S. Tsirkin wrote:
>> On Tue, May 17, 2016 at 03:39:14PM +0800, Cao jin wrote:
>>>
>>>
>
>> Personally I don't consider this a big issue. I don't think
>> many people specify msi=on.
>>
>
> I also think so:) If so, the semantics won`t change.
>
> Marcel, do you think it is ok if I add some info in error message to help user to start machine?
Of course, giving the user a hint is always a good think IMO.
Thanks,
Marcel
>
> And Markus, do you have comments about it?
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v5 10/11] pci core: assert ENOSPC when add capability
2016-05-06 4:20 [Qemu-devel] [PATCH v5 00/11] Add param Error ** for msi_init() Cao jin
` (8 preceding siblings ...)
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 09/11] pci bridge dev: change msi " Cao jin
@ 2016-05-06 4:20 ` Cao jin
2016-05-15 13:10 ` Marcel Apfelbaum
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 11/11] pci: Convert msi_init() to Error and fix callers to check it Cao jin
10 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-06 4:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin, Marcel Apfelbaum, Markus Armbruster
ENOSPC is programming error, assert it for debugging.
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/pci/pci.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index f0f41dc..fc8b377 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2151,10 +2151,8 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
if (!offset) {
offset = pci_find_space(pdev, size);
- if (!offset) {
- error_setg(errp, "out of PCI config space");
- return -ENOSPC;
- }
+ /* out of PCI config space should be programming error */
+ assert(offset);
} else {
/* Verify that capabilities don't overlap. Note: device assignment
* depends on this check to verify that the device is not broken.
--
2.1.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 10/11] pci core: assert ENOSPC when add capability
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 10/11] pci core: assert ENOSPC when add capability Cao jin
@ 2016-05-15 13:10 ` Marcel Apfelbaum
2016-05-17 3:00 ` Cao jin
0 siblings, 1 reply; 28+ messages in thread
From: Marcel Apfelbaum @ 2016-05-15 13:10 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: Michael S. Tsirkin, Markus Armbruster
On 05/06/2016 07:20 AM, Cao jin wrote:
> ENOSPC is programming error, assert it for debugging.
>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
> cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> hw/pci/pci.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index f0f41dc..fc8b377 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2151,10 +2151,8 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
>
> if (!offset) {
> offset = pci_find_space(pdev, size);
> - if (!offset) {
> - error_setg(errp, "out of PCI config space");
> - return -ENOSPC;
> - }
> + /* out of PCI config space should be programming error */
'is', not 'should be'
> + assert(offset);
> } else {
> /* Verify that capabilities don't overlap. Note: device assignment
> * depends on this check to verify that the device is not broken.
>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 10/11] pci core: assert ENOSPC when add capability
2016-05-15 13:10 ` Marcel Apfelbaum
@ 2016-05-17 3:00 ` Cao jin
0 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-05-17 3:00 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: Michael S. Tsirkin, Markus Armbruster
On 05/15/2016 09:10 PM, Marcel Apfelbaum wrote:
> On 05/06/2016 07:20 AM, Cao jin wrote:
>> ENOSPC is programming error, assert it for debugging.
>>
>> + /* out of PCI config space should be programming error */
>
> 'is', not 'should be'
Will fix it. Thank You, Marcel.
I guess I should put this one as the 1st patch in the series, and remove
the "ENOSPC" line in the comment of msi_init(), sorry for this stupid fault
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v5 11/11] pci: Convert msi_init() to Error and fix callers to check it
2016-05-06 4:20 [Qemu-devel] [PATCH v5 00/11] Add param Error ** for msi_init() Cao jin
` (9 preceding siblings ...)
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 10/11] pci core: assert ENOSPC when add capability Cao jin
@ 2016-05-06 4:20 ` Cao jin
2016-05-15 13:41 ` Marcel Apfelbaum
10 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-06 4:20 UTC (permalink / raw)
To: qemu-devel
Cc: Gerd Hoffmann, John Snow, Dmitry Fleytman, Jason Wang,
Michael S. Tsirkin, Hannes Reinecke, Paolo Bonzini,
Alex Williamson, Markus Armbruster, Marcel Apfelbaum
msi_init() reports errors with error_report(), which is wrong
when it's used in realize().
Fix by converting it to Error.
Fix its callers to handle failure instead of ignoring it.
For those callers who don`t handle the failure, it might happen:
when user want msi on, but he doesn`t get what he want because of
msi_init fails silently.
cc: Gerd Hoffmann <kraxel@redhat.com>
cc: John Snow <jsnow@redhat.com>
cc: Dmitry Fleytman <dmitry@daynix.com>
cc: Jason Wang <jasowang@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Hannes Reinecke <hare@suse.de>
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Alex Williamson <alex.williamson@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
the affected device is modified in this way:
1. intel hd audio: move msi_init() above, save the strength to free the
MemoryRegion when it fails.
2. ich9 ahci: move msi_init() above, save the strenth to free the resource
allocated when calling achi_realize(). It doesn`t have msi property, so
msi_init failure leads to fall back to INTx silently. Just free the error
object
3. vmxnet3: move msi_init() above. Remove the unecessary vmxnet3_init_msi().
It doesn`t have msi property, so msi_init() failure leads to fall back to
INTx silently. Just free the error object.
4. ioh3420/xio3130_downstream/xio3130_upstream: they are pcie components, msi
or msix is forced, catch error and report it right there.
5. pci_bridge_dev: msi_init`s failure is fatal, follow the behaviour.
6. megasas_scsi: move msi_init() above, save the strength to free the
MemoryRegion when it fails.
7. mptsas: Move msi_init() above, save the strength to free the MemoryRegion
when it fails.
8. pvscsi: it doesn`t have msi property, msi_init fail leads to fall back to
INTx silently.
9. usb-xhci: move msi_init() above, save the strength to free the MemoryRegion
when it fails.
10. vfio-pci: keep the previous behaviour, and just catch & report error.
hw/audio/intel-hda.c | 18 +++++++++++++----
hw/ide/ich.c | 15 +++++++++-----
hw/net/vmxnet3.c | 41 +++++++++++++++-----------------------
hw/pci-bridge/ioh3420.c | 4 +++-
hw/pci-bridge/pci_bridge_dev.c | 7 ++++---
hw/pci-bridge/xio3130_downstream.c | 4 +++-
hw/pci-bridge/xio3130_upstream.c | 4 +++-
hw/pci/msi.c | 9 +++++----
hw/scsi/megasas.c | 18 +++++++++++++----
hw/scsi/mptsas.c | 20 ++++++++++++++-----
hw/scsi/vmw_pvscsi.c | 6 +++++-
hw/usb/hcd-xhci.c | 18 +++++++++++++----
hw/vfio/pci.c | 6 ++++--
include/hw/pci/msi.h | 3 ++-
14 files changed, 112 insertions(+), 61 deletions(-)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 61362e5..0a46358 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1131,6 +1131,7 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
{
IntelHDAState *d = INTEL_HDA(pci);
uint8_t *conf = d->pci.config;
+ Error *err = NULL;
d->name = object_get_typename(OBJECT(d));
@@ -1139,13 +1140,22 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
/* HDCTL off 0x40 bit 0 selects signaling mode (1-HDA, 0 - Ac97) 18.1.19 */
conf[0x40] = 0x01;
+ if (d->msi != ON_OFF_AUTO_OFF) {
+ msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1,
+ true, false, &err);
+ if (err && d->msi == ON_OFF_AUTO_ON) {
+ /* If user set msi=on, then device creation fail */
+ error_propagate(errp, err);
+ return;
+ } else if (err && d->msi == ON_OFF_AUTO_AUTO) {
+ /* If user doesn`t set it on, switch to non-msi variant silently */
+ error_free(err);
+ }
+ }
+
memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d,
"intel-hda", 0x4000);
pci_register_bar(&d->pci, 0, 0, &d->mmio);
- if (d->msi == ON_OFF_AUTO_AUTO ||
- d->msi == ON_OFF_AUTO_ON) {
- msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
- }
hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
intel_hda_response, intel_hda_xfer);
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 0a13334..aec8262 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -111,6 +111,16 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
int sata_cap_offset;
uint8_t *sata_cap;
d = ICH_AHCI(dev);
+ Error *err = NULL;
+
+ /* Although the AHCI 1.3 specification states that the first capability
+ * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9
+ * AHCI device puts the MSI capability first, pointing to 0x80. */
+ msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, &err);
+ if (err) {
+ /* Fall back to INTx silently */
+ error_free(err);
+ }
ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
@@ -142,11 +152,6 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
pci_set_long(sata_cap + SATA_CAP_BAR,
(ICH9_IDP_BAR + 0x4) | (ICH9_IDP_INDEX_LOG2 << 4));
d->ahci.idp_offset = ICH9_IDP_INDEX;
-
- /* Although the AHCI 1.3 specification states that the first capability
- * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9
- * AHCI device puts the MSI capability first, pointing to 0x80. */
- msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false);
}
static void pci_ich9_uninit(PCIDevice *dev)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 7a38e47..ab9a938 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2189,27 +2189,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
}
}
-#define VMXNET3_USE_64BIT (true)
-#define VMXNET3_PER_VECTOR_MASK (false)
-
-static bool
-vmxnet3_init_msi(VMXNET3State *s)
-{
- PCIDevice *d = PCI_DEVICE(s);
- int res;
-
- res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
- VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
- if (0 > res) {
- VMW_WRPRN("Failed to initialize MSI, error %d", res);
- s->msi_used = false;
- } else {
- s->msi_used = true;
- }
-
- return s->msi_used;
-}
-
static void
vmxnet3_cleanup_msi(VMXNET3State *s)
{
@@ -2271,10 +2250,15 @@ static uint8_t *vmxnet3_device_serial_num(VMXNET3State *s)
return dsnp;
}
+
+#define VMXNET3_USE_64BIT (true)
+#define VMXNET3_PER_VECTOR_MASK (false)
+
static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
{
DeviceState *dev = DEVICE(pci_dev);
VMXNET3State *s = VMXNET3(pci_dev);
+ Error *err = NULL;
VMW_CBPRN("Starting init...");
@@ -2298,12 +2282,19 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
/* Interrupt pin A */
pci_dev->config[PCI_INTERRUPT_PIN] = 0x01;
- if (!vmxnet3_init_msix(s)) {
- VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
+ msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
+ VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, &err);
+ if (err) {
+ /* Fall back to INTx silently */
+ VMW_WRPRN("Failed to initialize MSI: %s", error_get_pretty(err));
+ error_free(err);
+ s->msi_used = false;
+ } else {
+ s->msi_used = true;
}
- if (!vmxnet3_init_msi(s)) {
- VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent.");
+ if (!vmxnet3_init_msix(s)) {
+ VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
}
vmxnet3_net_init(s);
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index b4a7806..d752e62 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -97,6 +97,7 @@ static int ioh3420_initfn(PCIDevice *d)
PCIEPort *p = PCIE_PORT(d);
PCIESlot *s = PCIE_SLOT(d);
int rc;
+ Error *err = NULL;
pci_bridge_initfn(d, TYPE_PCIE_BUS);
pcie_port_init_reg(d);
@@ -109,8 +110,9 @@ static int ioh3420_initfn(PCIDevice *d)
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);
+ IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
if (rc < 0) {
+ error_report_err(err);
goto err_bridge;
}
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 9e31f0e..af71c98 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -53,6 +53,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
PCIBridge *br = PCI_BRIDGE(dev);
PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
int err;
+ Error *local_err = NULL;
pci_bridge_initfn(dev, TYPE_PCI_BUS);
@@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
goto slotid_error;
}
- if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
- bridge_dev->msi == ON_OFF_AUTO_ON) &&
+ if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
msi_nonbroken) {
- err = msi_init(dev, 0, 1, true, true);
+ err = msi_init(dev, 0, 1, true, true, &local_err);
if (err < 0) {
+ error_report_err(local_err);
goto msi_error;
}
}
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index e6d653d..0982801 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -60,14 +60,16 @@ static int xio3130_downstream_initfn(PCIDevice *d)
PCIEPort *p = PCIE_PORT(d);
PCIESlot *s = PCIE_SLOT(d);
int rc;
+ Error *err = NULL;
pci_bridge_initfn(d, TYPE_PCIE_BUS);
pcie_port_init_reg(d);
rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
- XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
+ XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
if (rc < 0) {
+ error_report_err(err);
goto err_bridge;
}
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index d976844..1d2c597 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -56,14 +56,16 @@ static int xio3130_upstream_initfn(PCIDevice *d)
{
PCIEPort *p = PCIE_PORT(d);
int rc;
+ Error *err = NULL;
pci_bridge_initfn(d, TYPE_PCIE_BUS);
pcie_port_init_reg(d);
rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
- XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
+ XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
if (rc < 0) {
+ error_report_err(err);
goto err_bridge;
}
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index dd9d957..662be56 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -178,14 +178,13 @@ bool msi_enabled(const PCIDevice *dev)
* set @errp and return -errno on error.
*
* -ENOTSUP means lacking msi support for a msi-capable platform.
- * -ENOSPC means running out of PCI config space, happens when @offset is 0,
- * which means a programming error.
* -EINVAL means capability overlap, happens when @offset is non-zero,
* also means a programming error, except device assignment, which can check
* if a real HW is broken.
*/
int msi_init(struct PCIDevice *dev, uint8_t offset,
- unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
+ unsigned int nr_vectors, bool msi64bit,
+ bool msi_per_vector_mask, Error **errp)
{
unsigned int vectors_order;
uint16_t flags;
@@ -193,6 +192,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
int config_offset;
if (!msi_nonbroken) {
+ error_setg(errp, "MSI is not supported by interrupt controller");
return -ENOTSUP;
}
@@ -216,7 +216,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
}
cap_size = msi_cap_sizeof(flags);
- config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size);
+ config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset,
+ cap_size, errp);
if (config_offset < 0) {
return config_offset;
}
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index e71a28b..3585a6a 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2330,6 +2330,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s);
uint8_t *pci_conf;
int i, bar_type;
+ Error *err = NULL;
pci_conf = dev->config;
@@ -2338,6 +2339,19 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
/* Interrupt pin 1 */
pci_conf[PCI_INTERRUPT_PIN] = 0x01;
+ if (megasas_use_msi(s)) {
+ msi_init(dev, 0x50, 1, true, false, &err);
+ if (err && s->msi == ON_OFF_AUTO_ON) {
+ /* If user set msi=on, then device creation fail */
+ s->msi = ON_OFF_AUTO_OFF;
+ error_propagate(errp, err);
+ return;
+ } else if (err && s->msi == ON_OFF_AUTO_AUTO) {
+ /* If user doesn`t set it on, switch to non-msi variant silently */
+ error_free(err);
+ }
+ }
+
memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s,
"megasas-mmio", 0x4000);
memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
@@ -2345,10 +2359,6 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
"megasas-queue", 0x40000);
- if (megasas_use_msi(s) &&
- msi_init(dev, 0x50, 1, true, false) < 0) {
- s->msi = ON_OFF_AUTO_OFF;
- }
if (megasas_use_msix(s) &&
msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
&s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index afee576..aaee687 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1274,10 +1274,25 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
{
DeviceState *d = DEVICE(dev);
MPTSASState *s = MPT_SAS(dev);
+ Error *err = NULL;
dev->config[PCI_LATENCY_TIMER] = 0;
dev->config[PCI_INTERRUPT_PIN] = 0x01;
+ if (s->msi != ON_OFF_AUTO_OFF) {
+ msi_init(dev, 0, 1, true, false, &err);
+ if (err && s->msi == ON_OFF_AUTO_ON) {
+ /* If user set msi=on, then device creation fail */
+ error_propagate(errp, err);
+ s->msi_in_use = false;
+ return;
+ } else if (err && s->msi == ON_OFF_AUTO_AUTO) {
+ /* If user doesn`t set it on, switch to non-msi variant silently */
+ error_free(err);
+ s->msi_in_use = false;
+ }
+ }
+
memory_region_init_io(&s->mmio_io, OBJECT(s), &mptsas_mmio_ops, s,
"mptsas-mmio", 0x4000);
memory_region_init_io(&s->port_io, OBJECT(s), &mptsas_port_ops, s,
@@ -1285,11 +1300,6 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s,
"mptsas-diag", 0x10000);
- if ((s->msi == ON_OFF_AUTO_AUTO || s->msi == ON_OFF_AUTO_ON) &&
- msi_init(dev, 0, 1, true, false) >= 0) {
- s->msi_in_use = true;
- }
-
pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->port_io);
pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY |
PCI_BASE_ADDRESS_MEM_TYPE_32, &s->mmio_io);
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 4ce3581..2d38d6c 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1043,12 +1043,16 @@ static void
pvscsi_init_msi(PVSCSIState *s)
{
int res;
+ Error *err = NULL;
PCIDevice *d = PCI_DEVICE(s);
res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS,
- PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
+ PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err);
if (res < 0) {
trace_pvscsi_init_msi_fail(res);
+ error_append_hint(&err, "MSI capability fail to init,"
+ " will use INTx intead\n");
+ error_report_err(err);
s->msi_used = false;
} else {
s->msi_used = true;
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 6d70289..f17f242 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3578,6 +3578,7 @@ static void usb_xhci_init(XHCIState *xhci)
static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
{
int i, ret;
+ Error *err = NULL;
XHCIState *xhci = XHCI(dev);
@@ -3588,6 +3589,19 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
usb_xhci_init(xhci);
+ if (xhci->msi != ON_OFF_AUTO_OFF) {
+ ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
+ if (ret < 0 && xhci->msi == ON_OFF_AUTO_ON) {
+ /* If user set msi=on, then device creation fail */
+ error_propagate(errp, err);
+ return;
+ }
+ else if (ret < 0 && xhci->msi == ON_OFF_AUTO_AUTO) {
+ /* If user doesn`t set it on, switch to non-msi variant silently */
+ error_free(err);
+ }
+ }
+
if (xhci->numintrs > MAXINTRS) {
xhci->numintrs = MAXINTRS;
}
@@ -3645,10 +3659,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
assert(ret >= 0);
}
- if (xhci->msi == ON_OFF_AUTO_ON ||
- xhci->msi == ON_OFF_AUTO_AUTO) {
- msi_init(dev, 0x70, xhci->numintrs, true, false);
- }
if (xhci->msix == ON_OFF_AUTO_ON ||
xhci->msix == ON_OFF_AUTO_AUTO) {
msix_init(dev, xhci->numintrs,
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d091d8c..55ceb67 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1171,6 +1171,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
uint16_t ctrl;
bool msi_64bit, msi_maskbit;
int ret, entries;
+ Error *err = NULL;
if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
@@ -1184,12 +1185,13 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
trace_vfio_msi_setup(vdev->vbasedev.name, pos);
- ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit);
+ ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit, &err);
if (ret < 0) {
if (ret == -ENOTSUP) {
return 0;
}
- error_report("vfio: msi_init failed");
+ error_prepend(&err, "vfio: msi_init failed: ");
+ error_report_err(err);
return ret;
}
vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
index 8124908..4837bcf 100644
--- a/include/hw/pci/msi.h
+++ b/include/hw/pci/msi.h
@@ -35,7 +35,8 @@ void msi_set_message(PCIDevice *dev, MSIMessage msg);
MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector);
bool msi_enabled(const PCIDevice *dev);
int msi_init(struct PCIDevice *dev, uint8_t offset,
- unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);
+ unsigned int nr_vectors, bool msi64bit,
+ bool msi_per_vector_mask, Error **errp);
void msi_uninit(struct PCIDevice *dev);
void msi_reset(PCIDevice *dev);
void msi_notify(PCIDevice *dev, unsigned int vector);
--
2.1.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 11/11] pci: Convert msi_init() to Error and fix callers to check it
2016-05-06 4:20 ` [Qemu-devel] [PATCH v5 11/11] pci: Convert msi_init() to Error and fix callers to check it Cao jin
@ 2016-05-15 13:41 ` Marcel Apfelbaum
2016-05-17 10:08 ` Cao jin
0 siblings, 1 reply; 28+ messages in thread
From: Marcel Apfelbaum @ 2016-05-15 13:41 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: Gerd Hoffmann, John Snow, Dmitry Fleytman, Jason Wang,
Michael S. Tsirkin, Hannes Reinecke, Paolo Bonzini,
Alex Williamson, Markus Armbruster
On 05/06/2016 07:20 AM, Cao jin wrote:
> msi_init() reports errors with error_report(), which is wrong
> when it's used in realize().
>
> Fix by converting it to Error.
>
> Fix its callers to handle failure instead of ignoring it.
>
> For those callers who don`t handle the failure, it might happen:
> when user want msi on, but he doesn`t get what he want because of
> msi_init fails silently.
>
> cc: Gerd Hoffmann <kraxel@redhat.com>
> cc: John Snow <jsnow@redhat.com>
> cc: Dmitry Fleytman <dmitry@daynix.com>
> cc: Jason Wang <jasowang@redhat.com>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Hannes Reinecke <hare@suse.de>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: Alex Williamson <alex.williamson@redhat.com>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> the affected device is modified in this way:
> 1. intel hd audio: move msi_init() above, save the strength to free the
> MemoryRegion when it fails.
> 2. ich9 ahci: move msi_init() above, save the strenth to free the resource
> allocated when calling achi_realize(). It doesn`t have msi property, so
> msi_init failure leads to fall back to INTx silently. Just free the error
> object
> 3. vmxnet3: move msi_init() above. Remove the unecessary vmxnet3_init_msi().
> It doesn`t have msi property, so msi_init() failure leads to fall back to
> INTx silently. Just free the error object.
> 4. ioh3420/xio3130_downstream/xio3130_upstream: they are pcie components, msi
> or msix is forced, catch error and report it right there.
> 5. pci_bridge_dev: msi_init`s failure is fatal, follow the behaviour.
> 6. megasas_scsi: move msi_init() above, save the strength to free the
> MemoryRegion when it fails.
> 7. mptsas: Move msi_init() above, save the strength to free the MemoryRegion
> when it fails.
> 8. pvscsi: it doesn`t have msi property, msi_init fail leads to fall back to
> INTx silently.
> 9. usb-xhci: move msi_init() above, save the strength to free the MemoryRegion
> when it fails.
> 10. vfio-pci: keep the previous behaviour, and just catch & report error.
>
> hw/audio/intel-hda.c | 18 +++++++++++++----
> hw/ide/ich.c | 15 +++++++++-----
> hw/net/vmxnet3.c | 41 +++++++++++++++-----------------------
> hw/pci-bridge/ioh3420.c | 4 +++-
> hw/pci-bridge/pci_bridge_dev.c | 7 ++++---
> hw/pci-bridge/xio3130_downstream.c | 4 +++-
> hw/pci-bridge/xio3130_upstream.c | 4 +++-
> hw/pci/msi.c | 9 +++++----
> hw/scsi/megasas.c | 18 +++++++++++++----
> hw/scsi/mptsas.c | 20 ++++++++++++++-----
> hw/scsi/vmw_pvscsi.c | 6 +++++-
> hw/usb/hcd-xhci.c | 18 +++++++++++++----
> hw/vfio/pci.c | 6 ++++--
> include/hw/pci/msi.h | 3 ++-
> 14 files changed, 112 insertions(+), 61 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 61362e5..0a46358 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -1131,6 +1131,7 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
> {
> IntelHDAState *d = INTEL_HDA(pci);
> uint8_t *conf = d->pci.config;
> + Error *err = NULL;
>
> d->name = object_get_typename(OBJECT(d));
>
> @@ -1139,13 +1140,22 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
> /* HDCTL off 0x40 bit 0 selects signaling mode (1-HDA, 0 - Ac97) 18.1.19 */
> conf[0x40] = 0x01;
>
> + if (d->msi != ON_OFF_AUTO_OFF) {
> + msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1,
> + true, false, &err);
> + if (err && d->msi == ON_OFF_AUTO_ON) {
> + /* If user set msi=on, then device creation fail */
> + error_propagate(errp, err);
> + return;
The semantics now changed, old machines with msi=on on platforms with msi_nonbroken=false
will fail now, right? Is this acceptable or we need a compat way to kepp the semantics for old machines?
> + } else if (err && d->msi == ON_OFF_AUTO_AUTO) {
> + /* If user doesn`t set it on, switch to non-msi variant silently */
> + error_free(err);
> + }
> + }
> +
> memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d,
> "intel-hda", 0x4000);
> pci_register_bar(&d->pci, 0, 0, &d->mmio);
> - if (d->msi == ON_OFF_AUTO_AUTO ||
> - d->msi == ON_OFF_AUTO_ON) {
> - msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
> - }
>
> hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
> intel_hda_response, intel_hda_xfer);
> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
> index 0a13334..aec8262 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -111,6 +111,16 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
> int sata_cap_offset;
> uint8_t *sata_cap;
> d = ICH_AHCI(dev);
> + Error *err = NULL;
> +
> + /* Although the AHCI 1.3 specification states that the first capability
> + * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9
> + * AHCI device puts the MSI capability first, pointing to 0x80. */
> + msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, &err);
> + if (err) {
> + /* Fall back to INTx silently */
> + error_free(err);
> + }
>
> ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
>
> @@ -142,11 +152,6 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
> pci_set_long(sata_cap + SATA_CAP_BAR,
> (ICH9_IDP_BAR + 0x4) | (ICH9_IDP_INDEX_LOG2 << 4));
> d->ahci.idp_offset = ICH9_IDP_INDEX;
> -
> - /* Although the AHCI 1.3 specification states that the first capability
> - * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9
> - * AHCI device puts the MSI capability first, pointing to 0x80. */
> - msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false);
> }
>
> static void pci_ich9_uninit(PCIDevice *dev)
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 7a38e47..ab9a938 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2189,27 +2189,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
> }
> }
>
> -#define VMXNET3_USE_64BIT (true)
> -#define VMXNET3_PER_VECTOR_MASK (false)
> -
> -static bool
> -vmxnet3_init_msi(VMXNET3State *s)
> -{
> - PCIDevice *d = PCI_DEVICE(s);
> - int res;
> -
> - res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
> - VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
> - if (0 > res) {
> - VMW_WRPRN("Failed to initialize MSI, error %d", res);
> - s->msi_used = false;
> - } else {
> - s->msi_used = true;
> - }
> -
> - return s->msi_used;
> -}
> -
> static void
> vmxnet3_cleanup_msi(VMXNET3State *s)
> {
> @@ -2271,10 +2250,15 @@ static uint8_t *vmxnet3_device_serial_num(VMXNET3State *s)
> return dsnp;
> }
>
> +
> +#define VMXNET3_USE_64BIT (true)
> +#define VMXNET3_PER_VECTOR_MASK (false)
> +
> static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
> {
> DeviceState *dev = DEVICE(pci_dev);
> VMXNET3State *s = VMXNET3(pci_dev);
> + Error *err = NULL;
>
> VMW_CBPRN("Starting init...");
>
> @@ -2298,12 +2282,19 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
> /* Interrupt pin A */
> pci_dev->config[PCI_INTERRUPT_PIN] = 0x01;
>
> - if (!vmxnet3_init_msix(s)) {
> - VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
> + msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
> + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, &err);
> + if (err) {
> + /* Fall back to INTx silently */
> + VMW_WRPRN("Failed to initialize MSI: %s", error_get_pretty(err));
> + error_free(err);
> + s->msi_used = false;
> + } else {
> + s->msi_used = true;
> }
>
> - if (!vmxnet3_init_msi(s)) {
> - VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent.");
> + if (!vmxnet3_init_msix(s)) {
> + VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
> }
>
> vmxnet3_net_init(s);
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index b4a7806..d752e62 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -97,6 +97,7 @@ static int ioh3420_initfn(PCIDevice *d)
> PCIEPort *p = PCIE_PORT(d);
> PCIESlot *s = PCIE_SLOT(d);
> int rc;
> + Error *err = NULL;
>
> pci_bridge_initfn(d, TYPE_PCIE_BUS);
> pcie_port_init_reg(d);
> @@ -109,8 +110,9 @@ static int ioh3420_initfn(PCIDevice *d)
>
> 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);
> + IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
> if (rc < 0) {
> + error_report_err(err);
> goto err_bridge;
> }
OK
>
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 9e31f0e..af71c98 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -53,6 +53,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> PCIBridge *br = PCI_BRIDGE(dev);
> PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
> int err;
> + Error *local_err = NULL;
>
> pci_bridge_initfn(dev, TYPE_PCI_BUS);
>
> @@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> goto slotid_error;
> }
>
> - if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
> - bridge_dev->msi == ON_OFF_AUTO_ON) &&
> + if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
So we made the msi property OnOffAuto, but we don't make a difference between ON and Auto?
> msi_nonbroken) {
> - err = msi_init(dev, 0, 1, true, true);
> + err = msi_init(dev, 0, 1, true, true, &local_err);
> if (err < 0) {
> + error_report_err(local_err);
> goto msi_error;
> }
> }
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index e6d653d..0982801 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -60,14 +60,16 @@ static int xio3130_downstream_initfn(PCIDevice *d)
> PCIEPort *p = PCIE_PORT(d);
> PCIESlot *s = PCIE_SLOT(d);
> int rc;
> + Error *err = NULL;
>
> pci_bridge_initfn(d, TYPE_PCIE_BUS);
> pcie_port_init_reg(d);
>
> rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
> XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
> - XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
> + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
> if (rc < 0) {
> + error_report_err(err);
> goto err_bridge;
> }
>
OK
> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
> index d976844..1d2c597 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -56,14 +56,16 @@ static int xio3130_upstream_initfn(PCIDevice *d)
> {
> PCIEPort *p = PCIE_PORT(d);
> int rc;
> + Error *err = NULL;
>
> pci_bridge_initfn(d, TYPE_PCIE_BUS);
> pcie_port_init_reg(d);
>
> rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
> XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
> - XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
> + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
> if (rc < 0) {
> + error_report_err(err);
> goto err_bridge;
> }
>
OK
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index dd9d957..662be56 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -178,14 +178,13 @@ bool msi_enabled(const PCIDevice *dev)
> * set @errp and return -errno on error.
> *
> * -ENOTSUP means lacking msi support for a msi-capable platform.
> - * -ENOSPC means running out of PCI config space, happens when @offset is 0,
> - * which means a programming error.
> * -EINVAL means capability overlap, happens when @offset is non-zero,
> * also means a programming error, except device assignment, which can check
> * if a real HW is broken.
> */
> int msi_init(struct PCIDevice *dev, uint8_t offset,
> - unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
> + unsigned int nr_vectors, bool msi64bit,
> + bool msi_per_vector_mask, Error **errp)
> {
> unsigned int vectors_order;
> uint16_t flags;
> @@ -193,6 +192,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
> int config_offset;
>
> if (!msi_nonbroken) {
> + error_setg(errp, "MSI is not supported by interrupt controller");
> return -ENOTSUP;
> }
>
> @@ -216,7 +216,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
> }
>
> cap_size = msi_cap_sizeof(flags);
> - config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size);
> + config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset,
> + cap_size, errp);
> if (config_offset < 0) {
> return config_offset;
> }
OK
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index e71a28b..3585a6a 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2330,6 +2330,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
> MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s);
> uint8_t *pci_conf;
> int i, bar_type;
> + Error *err = NULL;
>
> pci_conf = dev->config;
>
> @@ -2338,6 +2339,19 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
> /* Interrupt pin 1 */
> pci_conf[PCI_INTERRUPT_PIN] = 0x01;
>
> + if (megasas_use_msi(s)) {
> + msi_init(dev, 0x50, 1, true, false, &err);
> + if (err && s->msi == ON_OFF_AUTO_ON) {
> + /* If user set msi=on, then device creation fail */
> + s->msi = ON_OFF_AUTO_OFF;
> + error_propagate(errp, err);
> + return;
The same question regarding the change of semantics.
> + } else if (err && s->msi == ON_OFF_AUTO_AUTO) {
> + /* If user doesn`t set it on, switch to non-msi variant silently */
> + error_free(err);
> + }
> + }
> +
> memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s,
> "megasas-mmio", 0x4000);
> memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
> @@ -2345,10 +2359,6 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
> memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
> "megasas-queue", 0x40000);
>
> - if (megasas_use_msi(s) &&
> - msi_init(dev, 0x50, 1, true, false) < 0) {
> - s->msi = ON_OFF_AUTO_OFF;
> - }
> if (megasas_use_msix(s) &&
> msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
> &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index afee576..aaee687 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -1274,10 +1274,25 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
> {
> DeviceState *d = DEVICE(dev);
> MPTSASState *s = MPT_SAS(dev);
> + Error *err = NULL;
>
> dev->config[PCI_LATENCY_TIMER] = 0;
> dev->config[PCI_INTERRUPT_PIN] = 0x01;
>
> + if (s->msi != ON_OFF_AUTO_OFF) {
> + msi_init(dev, 0, 1, true, false, &err);
> + if (err && s->msi == ON_OFF_AUTO_ON) {
> + /* If user set msi=on, then device creation fail */
> + error_propagate(errp, err);
> + s->msi_in_use = false;
Same here
> + return;
> + } else if (err && s->msi == ON_OFF_AUTO_AUTO) {
> + /* If user doesn`t set it on, switch to non-msi variant silently */
> + error_free(err);
> + s->msi_in_use = false;
> + }
> + }
> +
> memory_region_init_io(&s->mmio_io, OBJECT(s), &mptsas_mmio_ops, s,
> "mptsas-mmio", 0x4000);
> memory_region_init_io(&s->port_io, OBJECT(s), &mptsas_port_ops, s,
> @@ -1285,11 +1300,6 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
> memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s,
> "mptsas-diag", 0x10000);
>
> - if ((s->msi == ON_OFF_AUTO_AUTO || s->msi == ON_OFF_AUTO_ON) &&
> - msi_init(dev, 0, 1, true, false) >= 0) {
> - s->msi_in_use = true;
> - }
> -
> pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->port_io);
> pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY |
> PCI_BASE_ADDRESS_MEM_TYPE_32, &s->mmio_io);
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index 4ce3581..2d38d6c 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1043,12 +1043,16 @@ static void
> pvscsi_init_msi(PVSCSIState *s)
> {
> int res;
> + Error *err = NULL;
> PCIDevice *d = PCI_DEVICE(s);
>
> res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS,
> - PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
> + PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err);
> if (res < 0) {
> trace_pvscsi_init_msi_fail(res);
> + error_append_hint(&err, "MSI capability fail to init,"
> + " will use INTx intead\n");
> + error_report_err(err);
> s->msi_used = false;
> } else {
> s->msi_used = true;
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 6d70289..f17f242 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -3578,6 +3578,7 @@ static void usb_xhci_init(XHCIState *xhci)
> static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
> {
> int i, ret;
> + Error *err = NULL;
>
> XHCIState *xhci = XHCI(dev);
>
> @@ -3588,6 +3589,19 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>
> usb_xhci_init(xhci);
>
> + if (xhci->msi != ON_OFF_AUTO_OFF) {
> + ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
> + if (ret < 0 && xhci->msi == ON_OFF_AUTO_ON) {
> + /* If user set msi=on, then device creation fail */
> + error_propagate(errp, err);
> + return;
> + }
> + else if (ret < 0 && xhci->msi == ON_OFF_AUTO_AUTO) {
> + /* If user doesn`t set it on, switch to non-msi variant silently */
> + error_free(err);
> + }
> + }
> +
> if (xhci->numintrs > MAXINTRS) {
> xhci->numintrs = MAXINTRS;
> }
> @@ -3645,10 +3659,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
> assert(ret >= 0);
> }
>
> - if (xhci->msi == ON_OFF_AUTO_ON ||
> - xhci->msi == ON_OFF_AUTO_AUTO) {
> - msi_init(dev, 0x70, xhci->numintrs, true, false);
> - }
> if (xhci->msix == ON_OFF_AUTO_ON ||
> xhci->msix == ON_OFF_AUTO_AUTO) {
> msix_init(dev, xhci->numintrs,
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d091d8c..55ceb67 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1171,6 +1171,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
> uint16_t ctrl;
> bool msi_64bit, msi_maskbit;
> int ret, entries;
> + Error *err = NULL;
>
> if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
> vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
> @@ -1184,12 +1185,13 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
>
> trace_vfio_msi_setup(vdev->vbasedev.name, pos);
>
> - ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit);
> + ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit, &err);
> if (ret < 0) {
> if (ret == -ENOTSUP) {
> return 0;
> }
> - error_report("vfio: msi_init failed");
> + error_prepend(&err, "vfio: msi_init failed: ");
> + error_report_err(err);
> return ret;
> }
OK
> vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
> diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
> index 8124908..4837bcf 100644
> --- a/include/hw/pci/msi.h
> +++ b/include/hw/pci/msi.h
> @@ -35,7 +35,8 @@ void msi_set_message(PCIDevice *dev, MSIMessage msg);
> MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector);
> bool msi_enabled(const PCIDevice *dev);
> int msi_init(struct PCIDevice *dev, uint8_t offset,
> - unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);
> + unsigned int nr_vectors, bool msi64bit,
> + bool msi_per_vector_mask, Error **errp);
> void msi_uninit(struct PCIDevice *dev);
> void msi_reset(PCIDevice *dev);
> void msi_notify(PCIDevice *dev, unsigned int vector);
>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 11/11] pci: Convert msi_init() to Error and fix callers to check it
2016-05-15 13:41 ` Marcel Apfelbaum
@ 2016-05-17 10:08 ` Cao jin
2016-05-23 10:06 ` Marcel Apfelbaum
0 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-17 10:08 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel
Cc: Gerd Hoffmann, John Snow, Dmitry Fleytman, Jason Wang,
Michael S. Tsirkin, Hannes Reinecke, Paolo Bonzini,
Alex Williamson, Markus Armbruster
On 05/15/2016 09:41 PM, Marcel Apfelbaum wrote:
>
>>
>> diff --git a/hw/pci-bridge/pci_bridge_dev.c
>> b/hw/pci-bridge/pci_bridge_dev.c
>> index 9e31f0e..af71c98 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>> @@ -53,6 +53,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>> PCIBridge *br = PCI_BRIDGE(dev);
>> PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>> int err;
>> + Error *local_err = NULL;
>>
>> pci_bridge_initfn(dev, TYPE_PCI_BUS);
>>
>> @@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>> goto slotid_error;
>> }
>>
>> - if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
>> - bridge_dev->msi == ON_OFF_AUTO_ON) &&
>> + if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
>
> So we made the msi property OnOffAuto, but we don't make a difference
> between ON and Auto?
>
That is why I am not quite sure about this device, msi has a relation
with shpc.
From its previous behaviour, it can be seen that it don`t treat 'on' as
'auto'.(I am not sure why it is different with others)
Its previous behaviour treat msi_init`s failure as fatal, and lead to
device creation fail. According to Markus`s comments(if I understand him
right), this device has no non-msi variants. I think my patch follows
the previous behaviour.
And also according to function comments:
/* MSI is not applicable without SHPC */
which means device either has both, or neither(if I understand it
right), so that is why I don`t make a difference
>> msi_nonbroken) {
>> - err = msi_init(dev, 0, 1, true, true);
>> + err = msi_init(dev, 0, 1, true, true, &local_err);
>> if (err < 0) {
>> + error_report_err(local_err);
>> goto msi_error;
>> }
>> }
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 11/11] pci: Convert msi_init() to Error and fix callers to check it
2016-05-17 10:08 ` Cao jin
@ 2016-05-23 10:06 ` Marcel Apfelbaum
2016-05-23 12:51 ` Cao jin
0 siblings, 1 reply; 28+ messages in thread
From: Marcel Apfelbaum @ 2016-05-23 10:06 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: Gerd Hoffmann, John Snow, Dmitry Fleytman, Jason Wang,
Michael S. Tsirkin, Hannes Reinecke, Paolo Bonzini,
Alex Williamson, Markus Armbruster
On 05/17/2016 01:08 PM, Cao jin wrote:
>
>
> On 05/15/2016 09:41 PM, Marcel Apfelbaum wrote:
>
>>
>>>
>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c
>>> b/hw/pci-bridge/pci_bridge_dev.c
>>> index 9e31f0e..af71c98 100644
>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>> @@ -53,6 +53,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>> PCIBridge *br = PCI_BRIDGE(dev);
>>> PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>> int err;
>>> + Error *local_err = NULL;
>>>
>>> pci_bridge_initfn(dev, TYPE_PCI_BUS);
>>>
>>> @@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>> goto slotid_error;
>>> }
>>>
>>> - if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
>>> - bridge_dev->msi == ON_OFF_AUTO_ON) &&
>>> + if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
>>
>> So we made the msi property OnOffAuto, but we don't make a difference
>> between ON and Auto?
>>
>
> That is why I am not quite sure about this device, msi has a relation with shpc.
>
> From its previous behaviour, it can be seen that it don`t treat 'on' as 'auto'.(I am not sure why it is different with others)
>
> Its previous behaviour treat msi_init`s failure as fatal, and lead to device creation fail. According to Markus`s comments(if I understand him right), this device has no non-msi variants.
Actually it has. The bridge needs msi for the SHPC controller, as you said.
If you look into shpc_interrupt_update function (hw/pci/shpc.c) you can see it can fall
back to legacy interrupts.
The only complication here is that msi is needed only if shpc is present.
So maybe having msi=on/off/auto is OK.
msi=auto => if shpc not present or msi broken results in msi = off, else msi = on
msi=on => fails if shpc present and msi broken
msi=off => use legacy interrupts if shpc is present
Basically the msi flag has no meaning if shpc not present.
Thanks,
Marcel
I think my
> patch follows the previous behaviour.
>
> And also according to function comments:
> /* MSI is not applicable without SHPC */
> which means device either has both, or neither(if I understand it right), so that is why I don`t make a difference
>
>>> msi_nonbroken) {
>>> - err = msi_init(dev, 0, 1, true, true);
>>> + err = msi_init(dev, 0, 1, true, true, &local_err);
>>> if (err < 0) {
>>> + error_report_err(local_err);
>>> goto msi_error;
>>> }
>>> }
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 11/11] pci: Convert msi_init() to Error and fix callers to check it
2016-05-23 10:06 ` Marcel Apfelbaum
@ 2016-05-23 12:51 ` Cao jin
0 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-05-23 12:51 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel
Cc: Gerd Hoffmann, John Snow, Dmitry Fleytman, Jason Wang,
Michael S. Tsirkin, Hannes Reinecke, Paolo Bonzini,
Alex Williamson, Markus Armbruster
On 05/23/2016 06:06 PM, Marcel Apfelbaum wrote:
> On 05/17/2016 01:08 PM, Cao jin wrote:
>>
>>
>> On 05/15/2016 09:41 PM, Marcel Apfelbaum wrote:
>>
>>
>> That is why I am not quite sure about this device, msi has a relation
>> with shpc.
>>
>> From its previous behaviour, it can be seen that it don`t treat 'on'
>> as 'auto'.(I am not sure why it is different with others)
>>
>> Its previous behaviour treat msi_init`s failure as fatal, and lead to
>> device creation fail. According to Markus`s comments(if I understand
>> him right), this device has no non-msi variants.
>
> Actually it has. The bridge needs msi for the SHPC controller, as you said.
> If you look into shpc_interrupt_update function (hw/pci/shpc.c) you can
> see it can fall
> back to legacy interrupts.
>
> The only complication here is that msi is needed only if shpc is present.
> So maybe having msi=on/off/auto is OK.
>
> msi=auto => if shpc not present or msi broken results in msi = off, else
> msi = on
> msi=on => fails if shpc present and msi broken
> msi=off => use legacy interrupts if shpc is present
>
>
> Basically the msi flag has no meaning if shpc not present.
>
Thanks for the hint, v6 is on the way:)
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread