* [Qemu-devel] [PATCH v6 01/11] pci core: assert ENOSPC when add capability
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
@ 2016-05-24 4:04 ` Cao jin
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 02/11] fix some coding style problems Cao jin
` (11 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-05-24 4:04 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>
Reviewed-by: Marcel Apfelbaum <marcel@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 06d3117..26f387c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2152,10 +2152,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 is 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* [Qemu-devel] [PATCH v6 02/11] fix some coding style problems
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 01/11] pci core: assert ENOSPC when add capability Cao jin
@ 2016-05-24 4:04 ` Cao jin
2016-06-01 8:09 ` Markus Armbruster
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 03/11] change pvscsi_init_msi() type to void Cao jin
` (10 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-24 4:04 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>
Reviewed-by: 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 | 17 +++++++++++++++++
6 files changed, 36 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 7b582e9..41ca47b 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -68,10 +68,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);
@@ -79,6 +81,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. */
@@ -86,6 +89,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..97f35c0 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -165,6 +165,23 @@ 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.
+ * -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 v6 02/11] fix some coding style problems
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 02/11] fix some coding style problems Cao jin
@ 2016-06-01 8:09 ` Markus Armbruster
2016-06-01 8:33 ` Cao jin
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2016-06-01 8:09 UTC (permalink / raw)
To: Cao jin
Cc: qemu-devel, Dmitry Fleytman, Marcel Apfelbaum, Jason Wang,
Michael S. Tsirkin
Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> 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>
>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
[...]
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index e0e64c2..97f35c0 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -165,6 +165,23 @@ 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.
@errp only appears in PATCH 11. The easiest fix is to add this comment
only then.
> + * 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.
> + * -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)
> {
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v6 02/11] fix some coding style problems
2016-06-01 8:09 ` Markus Armbruster
@ 2016-06-01 8:33 ` Cao jin
0 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-06-01 8:33 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Dmitry Fleytman, Marcel Apfelbaum, Jason Wang,
Michael S. Tsirkin
On 06/01/2016 04:09 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>> 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>
>>
>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> [...]
>> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
>> index e0e64c2..97f35c0 100644
>> --- a/hw/pci/msi.c
>> +++ b/hw/pci/msi.c
>> @@ -165,6 +165,23 @@ 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.
>
> @errp only appears in PATCH 11. The easiest fix is to add this comment
> only then.
>
eh...sorry for my carelessness...
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v6 03/11] change pvscsi_init_msi() type to void
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 01/11] pci core: assert ENOSPC when add capability Cao jin
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 02/11] fix some coding style problems Cao jin
@ 2016-05-24 4:04 ` Cao jin
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 04/11] megasas: Fix Cao jin
` (9 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-05-24 4:04 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Markus Armbruster, Marcel Apfelbaum
Nobody use its return value, so change the type to void.
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@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 e690b4e..c2a387a 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1040,7 +1040,7 @@ pvscsi_io_read(void *opaque, hwaddr addr, unsigned size)
}
-static bool
+static void
pvscsi_init_msi(PVSCSIState *s)
{
int res;
@@ -1054,8 +1054,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* [Qemu-devel] [PATCH v6 04/11] megasas: Fix
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
` (2 preceding siblings ...)
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 03/11] change pvscsi_init_msi() type to void Cao jin
@ 2016-05-24 4:04 ` Cao jin
2016-06-01 8:10 ` Markus Armbruster
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 05/11] mptsas: change .realize function name Cao jin
` (8 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-24 4:04 UTC (permalink / raw)
To: qemu-devel; +Cc: Hannes Reinecke, Paolo Bonzini, Marcel Apfelbaum
msi_init returns non-zero value on both failure and success.
cc: Hannes Reinecke <hare@suse.de>
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@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 v6 04/11] megasas: Fix
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 04/11] megasas: Fix Cao jin
@ 2016-06-01 8:10 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2016-06-01 8:10 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, Marcel Apfelbaum, Paolo Bonzini, Hannes Reinecke
Title "megasas: Fix" is no good, possibly an editing accident. Suggest
something like "megasas: Fix check for msi_init() failure"
Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> msi_init returns non-zero value on both failure and success.
>
> cc: Hannes Reinecke <hare@suse.de>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
>
> Reviewed-by: Marcel Apfelbaum <marcel@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) &&
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v6 05/11] mptsas: change .realize function name
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
` (3 preceding siblings ...)
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 04/11] megasas: Fix Cao jin
@ 2016-05-24 4:04 ` Cao jin
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 06/11] usb xhci: change msi/msix property type Cao jin
` (7 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-05-24 4:04 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* [Qemu-devel] [PATCH v6 06/11] usb xhci: change msi/msix property type
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
` (4 preceding siblings ...)
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 05/11] mptsas: change .realize function name Cao jin
@ 2016-05-24 4:04 ` Cao jin
2016-06-01 8:25 ` Markus Armbruster
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 07/11] intel-hda: change msi " Cao jin
` (6 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-24 4:04 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 43ba615..bbe5cca 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,
};
@@ -3648,10 +3648,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,
@@ -3872,8 +3874,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* Re: [Qemu-devel] [PATCH v6 06/11] usb xhci: change msi/msix property type
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 06/11] usb xhci: change msi/msix property type Cao jin
@ 2016-06-01 8:25 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2016-06-01 8:25 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, Marcel Apfelbaum, Gerd Hoffmann, Michael S. Tsirkin
Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>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 43ba615..bbe5cca 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,
> };
> @@ -3648,10 +3648,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) {
Easier:
if (xhci->msi != ON_OFF_AUTO_OFF) {
Hmm, you switch to this simpler conditional in PATCH 11, when you move
this code. I'd use the simpler conditional from the start. Since it
doesn't affect the final state, this is a suggestion, not a demand.
> msi_init(dev, 0x70, xhci->numintrs, true, false);
Shouldn't we check for errors here? Hmm, you do it in PATCH 11. Okay,
but I'd add a /* TODO check for errors */ comment here.
> }
> - if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) {
> + if (xhci->msix == ON_OFF_AUTO_ON ||
> + xhci->msix == ON_OFF_AUTO_AUTO) {
Likewise.
> msix_init(dev, xhci->numintrs,
> &xhci->mem, 0, OFF_MSIX_TABLE,
> &xhci->mem, 0, OFF_MSIX_PBA,
Likewise.
> @@ -3872,8 +3874,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,
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v6 07/11] intel-hda: change msi property type
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
` (5 preceding siblings ...)
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 06/11] usb xhci: change msi/msix property type Cao jin
@ 2016-05-24 4:04 ` Cao jin
2016-06-01 8:39 ` Markus Armbruster
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 08/11] mptsas: " Cao jin
` (5 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-24 4:04 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* Re: [Qemu-devel] [PATCH v6 07/11] intel-hda: change msi property type
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 07/11] intel-hda: change msi " Cao jin
@ 2016-06-01 8:39 ` Markus Armbruster
2016-06-02 8:42 ` Cao jin
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2016-06-01 8:39 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, Marcel Apfelbaum, Gerd Hoffmann, Michael S. Tsirkin
Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>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;
I'm going to review all uses of this member.
> 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);
Same suggestions as for PATCH 06:
* Use the d->msi != ON_OFF_AUTO_OFF
* Add /* TODO check for errors */ now, drop it when you add the check in
PATCH 11.
> }
>
> @@ -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(),
> };
Not covered:
static void intel_hda_update_irq(IntelHDAState *d)
{
--> int msi = d->msi && msi_enabled(&d->pci);
int level;
intel_hda_update_int_sts(d);
if (d->int_sts & (1U << 31) && d->int_ctl & (1U << 31)) {
level = 1;
} else {
level = 0;
}
dprint(d, 2, "%s: level %d [%s]\n", __FUNCTION__,
level, msi ? "msi" : "intx");
if (msi) {
if (level) {
msi_notify(&d->pci, 0);
}
} else {
pci_set_irq(&d->pci, level);
}
}
This is wrong, because the meaning of the test changes from
(user didn't specify msi=off) && (guest enabled MSI)
to
(user didn't specify msi=on or msi=off) && (guest enabled MSI)
What about:
int msi = msi_enabled(&d->pci);
If I understand it correctly, it can only be true when we added MSI
capability, and we do that only when msi=auto (default) or msi=on.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v6 07/11] intel-hda: change msi property type
2016-06-01 8:39 ` Markus Armbruster
@ 2016-06-02 8:42 ` Cao jin
0 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-06-02 8:42 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Marcel Apfelbaum, Gerd Hoffmann, Michael S. Tsirkin
On 06/01/2016 04:39 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>
> Not covered:
>
> static void intel_hda_update_irq(IntelHDAState *d)
> {
> --> int msi = d->msi && msi_enabled(&d->pci);
> int level;
>
> intel_hda_update_int_sts(d);
> if (d->int_sts & (1U << 31) && d->int_ctl & (1U << 31)) {
> level = 1;
> } else {
> level = 0;
> }
> dprint(d, 2, "%s: level %d [%s]\n", __FUNCTION__,
> level, msi ? "msi" : "intx");
> if (msi) {
> if (level) {
> msi_notify(&d->pci, 0);
> }
> } else {
> pci_set_irq(&d->pci, level);
> }
> }
>
> This is wrong, because the meaning of the test changes from
>
> (user didn't specify msi=off) && (guest enabled MSI)
>
> to
>
> (user didn't specify msi=on or msi=off) && (guest enabled MSI)
>
Thanks for pointing the error, seems I lost the fact that
ON_OFF_AUTO_AUTO equals 0.
> What about:
>
> int msi = msi_enabled(&d->pci);
>
> If I understand it correctly, it can only be true when we added MSI
> capability, and we do that only when msi=auto (default) or msi=on.
>
I think the modification is ok.
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v6 08/11] mptsas: change msi property type
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
` (6 preceding siblings ...)
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 07/11] intel-hda: change msi " Cao jin
@ 2016-05-24 4:04 ` Cao jin
2016-06-01 8:43 ` Markus Armbruster
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 09/11] megasas: change msi/msix " Cao jin
` (4 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-24 4:04 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S. Tsirkin, Markus Armbruster,
Marcel Apfelbaum
>From uint32 to enum OnOffAuto, and give it a shorter name.
cc: Paolo Bonzini <pbonzini@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/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* Re: [Qemu-devel] [PATCH v6 08/11] mptsas: change msi property type
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 08/11] mptsas: " Cao jin
@ 2016-06-01 8:43 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2016-06-01 8:43 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin
Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>From uint32 to enum OnOffAuto, and give it a shorter name.
>
> cc: Paolo Bonzini <pbonzini@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/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;
> }
Same suggestions as for PATCH 06:
* Use the s->msi != ON_OFF_AUTO_OFF
* Add /* TODO check for errors */ now, drop it when you add the check in
PATCH 11.
> @@ -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;
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v6 09/11] megasas: change msi/msix property type
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
` (7 preceding siblings ...)
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 08/11] mptsas: " Cao jin
@ 2016-05-24 4:04 ` Cao jin
2016-06-01 9:14 ` Markus Armbruster
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 10/11] pci bridge dev: change msi " Cao jin
` (3 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-24 4:04 UTC (permalink / raw)
To: qemu-devel
Cc: Hannes Reinecke, Paolo Bonzini, Michael S. Tsirkin,
Markus Armbruster, Marcel Apfelbaum
>From bit to enum OnOffAuto.
cc: Hannes Reinecke <hare@suse.de>
cc: Paolo Bonzini <pbonzini@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/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* Re: [Qemu-devel] [PATCH v6 09/11] megasas: change msi/msix property type
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 09/11] megasas: change msi/msix " Cao jin
@ 2016-06-01 9:14 ` Markus Armbruster
2016-06-02 10:15 ` Cao jin
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2016-06-01 9:14 UTC (permalink / raw)
To: Cao jin
Cc: qemu-devel, Marcel Apfelbaum, Paolo Bonzini, Hannes Reinecke,
Michael S. Tsirkin
Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>From bit to enum OnOffAuto.
>
> cc: Hannes Reinecke <hare@suse.de>
> cc: Paolo Bonzini <pbonzini@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/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;
s->msi != ON_OFF_AUTO_OFF, please.
> }
As we'll see below, s->msi changes on realize().
Before the first realize(), it's whatever the user put there.
Before this patch: megasas_mask_use_msi() returns false when the
user explicitly requested msi=off, and true when the user did
nothing or requested msi=on.
After this patch: it returns false when the user explicitly
requested msi=off, and true true when the user did nothing or
requested msi=on or msi=auto. Okay.
Afterwards, it reflects MSI capability.
Before this patch: megasas_mask_use_msi() returns false when the
device has no MSI capability, either because the user suppressed it
with msi=off, or because we couldn't add it.
After this patch: same. Okay.
>
> 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;
s->msi != ON_OFF_AUTO_OFF, please.
> }
Same correctness argument as for megasas_mask_use_msi().
>
> 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);
Unlike the device models we've seen in earlier patches, this one
overwrites its configuration to reflect actual device state. Hmm. I'll
revisit this in my review of PATCH 11.
> @@ -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(),
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v6 09/11] megasas: change msi/msix property type
2016-06-01 9:14 ` Markus Armbruster
@ 2016-06-02 10:15 ` Cao jin
2016-06-02 13:59 ` Markus Armbruster
0 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-06-02 10:15 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Marcel Apfelbaum, Paolo Bonzini, Hannes Reinecke,
Michael S. Tsirkin
On 06/01/2016 05:14 PM, Markus Armbruster wrote:
>>
>> 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;
>
> s->msi != ON_OFF_AUTO_OFF, please.
>
>> }
>
> Same correctness argument as for megasas_mask_use_msi().
>
Hi Markus,
I Cant find function names megasas_mask_use_msi, only macro that I
deleted. Don`t quite follow the meaning, do I need to do anything about it?
>>
>> 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);
>
> Unlike the device models we've seen in earlier patches, this one
> overwrites its configuration to reflect actual device state. Hmm. I'll
> revisit this in my review of PATCH 11.
>
I guess it is still OK?
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v6 09/11] megasas: change msi/msix property type
2016-06-02 10:15 ` Cao jin
@ 2016-06-02 13:59 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2016-06-02 13:59 UTC (permalink / raw)
To: Cao jin
Cc: Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin, qemu-devel,
Hannes Reinecke
Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> On 06/01/2016 05:14 PM, Markus Armbruster wrote:
>
>>>
>>> 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;
>>
>> s->msi != ON_OFF_AUTO_OFF, please.
>>
>>> }
>>
>> Same correctness argument as for megasas_mask_use_msi().
>>
>
> Hi Markus,
> I Cant find function names megasas_mask_use_msi, only macro that I
> deleted. Don`t quite follow the meaning, do I need to do anything
> about it?
I meant megasas_use_msi(). Sorry for sowing confusion again :)
>
>>>
>>> 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);
>>
>> Unlike the device models we've seen in earlier patches, this one
>> overwrites its configuration to reflect actual device state. Hmm. I'll
>> revisit this in my review of PATCH 11.
>>
>
> I guess it is still OK?
This patch is okay. I was just observing that this device is different.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v6 10/11] pci bridge dev: change msi property type
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
` (8 preceding siblings ...)
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 09/11] megasas: change msi/msix " Cao jin
@ 2016-05-24 4:04 ` Cao jin
2016-06-01 9:17 ` Markus Armbruster
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 11/11] pci: Convert msi_init() to Error and fix callers to check it Cao jin
` (2 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-24 4:04 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>
---
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 41ca47b..fa0c50c 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -42,9 +42,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;
@@ -66,7 +67,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);
@@ -74,7 +75,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) {
@@ -147,8 +149,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 v6 10/11] pci bridge dev: change msi property type
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 10/11] pci bridge dev: change msi " Cao jin
@ 2016-06-01 9:17 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2016-06-01 9:17 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, Marcel Apfelbaum, Michael S. Tsirkin
Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>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>
> ---
> 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 41ca47b..fa0c50c 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -42,9 +42,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;
>
> @@ -66,7 +67,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;
Overwrites configuration to reflect state, like megasas. I'll come back
to this in my review of PATCH 11.
> }
>
> err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
> @@ -74,7 +75,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) &&
bridge_dev->msi != ON_OFF_AUTO_OFF, please, to avoid unnecessary churn
in PATCH 11.
> msi_nonbroken) {
> err = msi_init(dev, 0, 1, true, true);
> if (err < 0) {
> @@ -147,8 +149,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
* [Qemu-devel] [PATCH v6 11/11] pci: Convert msi_init() to Error and fix callers to check it
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
` (9 preceding siblings ...)
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 10/11] pci bridge dev: change msi " Cao jin
@ 2016-05-24 4:04 ` Cao jin
2016-06-01 12:37 ` Markus Armbruster
2016-06-01 3:06 ` [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
2016-06-01 6:59 ` Cao jin
12 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-05-24 4:04 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>
---
hw/audio/intel-hda.c | 20 ++++++++++++++----
hw/ide/ich.c | 17 +++++++++------
hw/net/vmxnet3.c | 42 +++++++++++++++-----------------------
hw/pci-bridge/ioh3420.c | 5 ++++-
hw/pci-bridge/pci_bridge_dev.c | 17 ++++++++++-----
hw/pci-bridge/xio3130_downstream.c | 5 ++++-
hw/pci-bridge/xio3130_upstream.c | 5 ++++-
hw/pci/msi.c | 8 ++++++--
hw/scsi/megasas.c | 22 +++++++++++++++-----
hw/scsi/mptsas.c | 26 +++++++++++++++++------
hw/scsi/vmw_pvscsi.c | 6 +++++-
hw/usb/hcd-xhci.c | 21 +++++++++++++++----
hw/vfio/pci.c | 7 +++++--
include/hw/pci/msi.h | 3 ++-
14 files changed, 140 insertions(+), 64 deletions(-)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 61362e5..9c7173c 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -26,6 +26,7 @@
#include "intel-hda.h"
#include "intel-hda-defs.h"
#include "sysemu/dma.h"
+#include "qapi/error.h"
/* --------------------------------------------------------------------- */
/* hda bus */
@@ -1131,6 +1132,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 +1141,23 @@ 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_append_hint(&err, "msi=on fails to create device."
+ " Please use msi=auto or off and try again");
+ 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..4b8198e 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -68,7 +68,7 @@
#include <hw/isa/isa.h>
#include "sysemu/block-backend.h"
#include "sysemu/dma.h"
-
+#include "qapi/error.h"
#include <hw/ide/pci.h>
#include <hw/ide/ahci.h>
@@ -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..9cff21c 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -32,6 +32,7 @@
#include "vmware_utils.h"
#include "vmxnet_tx_pkt.h"
#include "vmxnet_rx_pkt.h"
+#include "qapi/error.h"
#define PCI_DEVICE_ID_VMWARE_VMXNET3_REVISION 0x1
#define VMXNET3_MSIX_BAR_SIZE 0x2000
@@ -2189,27 +2190,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 +2251,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 +2283,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..c08cca3 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -25,6 +25,7 @@
#include "hw/pci/msi.h"
#include "hw/pci/pcie.h"
#include "ioh3420.h"
+#include "qapi/error.h"
#define PCI_DEVICE_ID_IOH_EPORT 0x3420 /* D0:F0 express mode */
#define PCI_DEVICE_ID_IOH_REV 0x2
@@ -97,6 +98,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 +111,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 fa0c50c..7f9131f 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -54,6 +54,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);
@@ -75,12 +76,18 @@ 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) &&
- msi_nonbroken) {
- err = msi_init(dev, 0, 1, true, true);
- if (err < 0) {
+ if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
+ /* it means SHPC exists */
+ err = msi_init(dev, 0, 1, true, true, &local_err);
+ if (err < 0 && bridge_dev->msi == ON_OFF_AUTO_ON) {
+ /* If user set msi=on, then device creation fail */
+ error_append_hint(&local_err, "msi=on fails to create device."
+ " Please use msi=auto or off and try again");
+ error_report_err(local_err);
goto msi_error;
+ } else if (err < 0 && bridge_dev->msi == ON_OFF_AUTO_AUTO) {
+ /* If user doesn`t set it on, switch to non-msi variant silently */
+ error_free(local_err);
}
}
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index e6d653d..5f45fec 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -24,6 +24,7 @@
#include "hw/pci/msi.h"
#include "hw/pci/pcie.h"
#include "xio3130_downstream.h"
+#include "qapi/error.h"
#define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */
#define XIO3130_REVISION 0x1
@@ -60,14 +61,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..3602bce 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -24,6 +24,7 @@
#include "hw/pci/msi.h"
#include "hw/pci/pcie.h"
#include "xio3130_upstream.h"
+#include "qapi/error.h"
#define PCI_DEVICE_ID_TI_XIO3130U 0x8232 /* upstream port */
#define XIO3130_REVISION 0x2
@@ -56,14 +57,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 97f35c0..ed0b38e 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -22,6 +22,7 @@
#include "hw/pci/msi.h"
#include "hw/xen/xen.h"
#include "qemu/range.h"
+#include "qapi/error.h"
/* PCI_MSI_ADDRESS_LO */
#define PCI_MSI_ADDRESS_LO_MASK (~0x3)
@@ -183,7 +184,8 @@ bool msi_enabled(const PCIDevice *dev)
* 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;
@@ -191,6 +193,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;
}
@@ -214,7 +217,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..32ea65a 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -29,7 +29,7 @@
#include "hw/scsi/scsi.h"
#include "block/scsi.h"
#include "trace.h"
-
+#include "qapi/error.h"
#include "mfi.h"
#define MEGASAS_VERSION_GEN1 "1.70"
@@ -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,21 @@ 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_append_hint(&err, "msi=on fails to create device."
+ " Please use msi=auto or off and try again");
+ 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 +2361,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..c013a07 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -32,7 +32,7 @@
#include "hw/scsi/scsi.h"
#include "block/scsi.h"
#include "trace.h"
-
+#include "qapi/error.h"
#include "mptsas.h"
#include "mpi.h"
@@ -1274,10 +1274,29 @@ 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_append_hint(&err, "msi=on fails to create device."
+ " Please use msi=auto or off and try again");
+ 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;
+ } else if (!err) {
+ s->msi_in_use = true;
+ }
+ }
+
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 +1304,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 c2a387a..b040575 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1044,12 +1044,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 bbe5cca..d137ee2 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -26,6 +26,7 @@
#include "hw/pci/msi.h"
#include "hw/pci/msix.h"
#include "trace.h"
+#include "qapi/error.h"
//#define DEBUG_XHCI
//#define DEBUG_DATA
@@ -3581,6 +3582,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);
@@ -3591,6 +3593,21 @@ 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_append_hint(&err, "msi=on fails to create device."
+ " Please use msi=auto or off and try again");
+ 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;
}
@@ -3648,10 +3665,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..67d93f7 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -32,6 +32,7 @@
#include "sysemu/sysemu.h"
#include "pci.h"
#include "trace.h"
+#include "qapi/error.h"
#define MSIX_CAP_LENGTH 12
@@ -1171,6 +1172,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 +1186,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 v6 11/11] pci: Convert msi_init() to Error and fix callers to check it
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 11/11] pci: Convert msi_init() to Error and fix callers to check it Cao jin
@ 2016-06-01 12:37 ` Markus Armbruster
2016-06-03 8:28 ` Cao jin
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2016-06-01 12:37 UTC (permalink / raw)
To: Cao jin
Cc: qemu-devel, Michael S. Tsirkin, Jason Wang, Marcel Apfelbaum,
Alex Williamson, Hannes Reinecke, Dmitry Fleytman, Paolo Bonzini,
John Snow, Gerd Hoffmann
Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> msi_init() reports errors with error_report(), which is wrong
> when it's used in realize().
msix_init() has the same problem. Perhaps you can tackle it later.
> 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:
Please use ' (U+0027 APOSTROPHE) instead of ` (U+0060 GRAVE ACCENT) as
apostrophe: don't, not don`t.
> 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>
> ---
> hw/audio/intel-hda.c | 20 ++++++++++++++----
> hw/ide/ich.c | 17 +++++++++------
> hw/net/vmxnet3.c | 42 +++++++++++++++-----------------------
> hw/pci-bridge/ioh3420.c | 5 ++++-
> hw/pci-bridge/pci_bridge_dev.c | 17 ++++++++++-----
> hw/pci-bridge/xio3130_downstream.c | 5 ++++-
> hw/pci-bridge/xio3130_upstream.c | 5 ++++-
> hw/pci/msi.c | 8 ++++++--
> hw/scsi/megasas.c | 22 +++++++++++++++-----
> hw/scsi/mptsas.c | 26 +++++++++++++++++------
> hw/scsi/vmw_pvscsi.c | 6 +++++-
> hw/usb/hcd-xhci.c | 21 +++++++++++++++----
> hw/vfio/pci.c | 7 +++++--
> include/hw/pci/msi.h | 3 ++-
> 14 files changed, 140 insertions(+), 64 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 61362e5..9c7173c 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -26,6 +26,7 @@
> #include "intel-hda.h"
> #include "intel-hda-defs.h"
> #include "sysemu/dma.h"
> +#include "qapi/error.h"
>
> /* --------------------------------------------------------------------- */
> /* hda bus */
> @@ -1131,6 +1132,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 +1141,23 @@ 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 */
Grammar nit: fails
Suggest: Can't satisfy user's explicit msi=on request, fail
> + error_append_hint(&err, "msi=on fails to create device."
> + " Please use msi=auto or off and try again");
This produces something like
qemu: -device intel-hda,msi=on: MSI is not supported by interrupt controller
msi=on fails to create device. Please use msi=auto or off and try again
where the last line lacks punctuation and a newline. Moreover, we
already told the user that it failed (first line), and telling him again
in different words isn't necessary. Suggest:
qemu: -device intel-hda,msi=on: MSI is not supported by interrupt controller
You have to use msi=auto (default) or msi=off with this machine type.
> + 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);
> + }
The conditional is superfluous.
We call msi_init() only if d->msi != ON_OFF_AUTO_OFF. If it sets @err
and d->msi == ON_OFF_AUTO_ON, we don't get here. Therefore, err implies
d->msi == ON_OFF_AUTO_AUTO, and the conditional can be simplified to
} else if (err) {
error_free(err);
}
But protecting error_free() like that is pointless, as it does nothing
when err is null. Simplify further to
}
assert(!err || d->msi == ON_OFF_AUTO_AUTO);
/* With msi=auto, we fall back to MSI off silently */
error_free(err);
The assertion is more for aiding the reader than for catching mistakes.
The error checking could be tightened as follows:
ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60,
1, true, false, &err);
assert(!ret || ret == -ENOTSUP);
if (ret && d->msi == ON_OFF_AUTO_ON) {
/* Can't satisfy user's explicit msi=on request, fail */
error_append_hint(&err, "You have to use msi=auto (default)"
" or msi=off with this machine type.\n");
error_propagate(errp, err);
return;
}
assert(!err || d->msi == ON_OFF_AUTO_AUTO);
/* With msi=auto, we fall back to MSI off 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);
> - }
Code motion to avoid having to clean up d->mmio on error. Good.
>
> hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
> intel_hda_response, intel_hda_xfer);
This is MSI device pattern #1: we have a property that lets the user
pick off, on or auto (on with silent fallback to off). We'll see a few
more below. The lack of consistency is disgusting, but cleaning it up
is out of scope for your series (assuming it's even possible now).
> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
> index 0a13334..4b8198e 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -68,7 +68,7 @@
> #include <hw/isa/isa.h>
> #include "sysemu/block-backend.h"
> #include "sysemu/dma.h"
> -
> +#include "qapi/error.h"
> #include <hw/ide/pci.h>
> #include <hw/ide/ahci.h>
>
> @@ -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);
> + }
Tighter error checking:
ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, NULL);
/* Fall back to INTx silently on -ENOTSUP */
assert(!ret || ret == -ENOTSUP);
More concise, too. No need for the include "qapi/error.h" then.
>
> 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);
> }
>
This is MSI device pattern #2: on whenever possible, else off.
> static void pci_ich9_uninit(PCIDevice *dev)
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 7a38e47..9cff21c 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -32,6 +32,7 @@
> #include "vmware_utils.h"
> #include "vmxnet_tx_pkt.h"
> #include "vmxnet_rx_pkt.h"
> +#include "qapi/error.h"
>
> #define PCI_DEVICE_ID_VMWARE_VMXNET3_REVISION 0x1
> #define VMXNET3_MSIX_BAR_SIZE 0x2000
> @@ -2189,27 +2190,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 +2251,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 +2283,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;
> }
Tighter error checking:
ret = msi_init(..., NULL);
/* Fall back to INTx silently on -ENOTSUP */
assert(!ret || ret == -ENOTSUP);
s->msi_used = !ret;
Can't see why we'd want to preserve the VMW_WRPRN() here. If it's
interesting to know that this device falls back to MSI off, it's
interesting to know for other devices as well, and that means the debug
should go into msi_init().
But if you'd prefer to keep it here:
ret = msi_init(..., &err);
/* Fall back to INTx silently on -ENOTSUP */
assert(!ret || ret == -ENOTSUP);
if (ret) {
VMW_WRPRN("Failed to initialize MSI: %s", error_get_pretty(err));
error_free(err);
}
s->msi_used = !ret;
Your choice.
>
> - if (!vmxnet3_init_msi(s)) {
> - VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent.");
The "configuration is inconsistent" part is useless. Your patch
replaces it. Good.
> + if (!vmxnet3_init_msix(s)) {
> + VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
It doesn't replace it here, but that's appropriate, because it doesn't
touch MSI-X code, it only moves it.
> }
>
> vmxnet3_net_init(s);
Pattern #2.
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index b4a7806..c08cca3 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -25,6 +25,7 @@
> #include "hw/pci/msi.h"
> #include "hw/pci/pcie.h"
> #include "ioh3420.h"
> +#include "qapi/error.h"
>
> #define PCI_DEVICE_ID_IOH_EPORT 0x3420 /* D0:F0 express mode */
> #define PCI_DEVICE_ID_IOH_REV 0x2
> @@ -97,6 +98,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 +111,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) {
Tighter error checking:
assert(rc == -ENOTSUP);
> + error_report_err(err);
> goto err_bridge;
> }
>
This is MSI device pattern #3: on whenever possible, else error.
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index fa0c50c..7f9131f 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -54,6 +54,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);
>
> @@ -75,12 +76,18 @@ 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) &&
> - msi_nonbroken) {
> - err = msi_init(dev, 0, 1, true, true);
> - if (err < 0) {
> + if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
Unnecessary churn, please use != ON_OFF_AUTO_OFF in PATCH 10.
> + /* it means SHPC exists */
Does it? Why?
> + err = msi_init(dev, 0, 1, true, true, &local_err);
> + if (err < 0 && bridge_dev->msi == ON_OFF_AUTO_ON) {
> + /* If user set msi=on, then device creation fail */
> + error_append_hint(&local_err, "msi=on fails to create device."
> + " Please use msi=auto or off and try again");
> + error_report_err(local_err);
> goto msi_error;
> + } else if (err < 0 && bridge_dev->msi == ON_OFF_AUTO_AUTO) {
> + /* If user doesn`t set it on, switch to non-msi variant silently */
> + error_free(local_err);
> }
Comments on intel_hda_realize() apply.
> }
>
Pattern #1.
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index e6d653d..5f45fec 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -24,6 +24,7 @@
> #include "hw/pci/msi.h"
> #include "hw/pci/pcie.h"
> #include "xio3130_downstream.h"
> +#include "qapi/error.h"
>
> #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */
> #define XIO3130_REVISION 0x1
> @@ -60,14 +61,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;
Comment on ioh3420_initfn() applies.
> }
>
Pattern #2.
> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
> index d976844..3602bce 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -24,6 +24,7 @@
> #include "hw/pci/msi.h"
> #include "hw/pci/pcie.h"
> #include "xio3130_upstream.h"
> +#include "qapi/error.h"
>
> #define PCI_DEVICE_ID_TI_XIO3130U 0x8232 /* upstream port */
> #define XIO3130_REVISION 0x2
> @@ -56,14 +57,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;
Comment on ioh3420_initfn() applies.
> }
>
Pattern #2.
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index 97f35c0..ed0b38e 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -22,6 +22,7 @@
> #include "hw/pci/msi.h"
> #include "hw/xen/xen.h"
> #include "qemu/range.h"
> +#include "qapi/error.h"
>
> /* PCI_MSI_ADDRESS_LO */
> #define PCI_MSI_ADDRESS_LO_MASK (~0x3)
> @@ -183,7 +184,8 @@ bool msi_enabled(const PCIDevice *dev)
> * 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;
> @@ -191,6 +193,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;
> }
>
> @@ -214,7 +217,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..32ea65a 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -29,7 +29,7 @@
> #include "hw/scsi/scsi.h"
> #include "block/scsi.h"
> #include "trace.h"
> -
> +#include "qapi/error.h"
> #include "mfi.h"
>
> #define MEGASAS_VERSION_GEN1 "1.70"
> @@ -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,21 @@ 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;
This assignment...
> + error_append_hint(&err, "msi=on fails to create device."
> + " Please use msi=auto or off and try again");
> + 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 */
... belongs here.
> + error_free(err);
> + }
Comments on intel_hda_realize() apply.
> + }
> +
> 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 +2361,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)) {
This is MSI device pattern #4: we have a property that lets the user
pick off, on or auto (on with silent fallback to off). We overwrite
auto to off in fallback.
This pattern is problematic. Say we create the device with msi=auto,
and realize changes it to msi=off. If we unrealize, then realize again,
we won't use MSI even when we could. We currenrly don't re-realize
device that way, and possible never will, but realize messing up device
configuration is unclean all the same. Not this patch's fault, and not
for this series to clean up.
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index afee576..c013a07 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -32,7 +32,7 @@
> #include "hw/scsi/scsi.h"
> #include "block/scsi.h"
> #include "trace.h"
> -
> +#include "qapi/error.h"
> #include "mptsas.h"
> #include "mpi.h"
>
> @@ -1274,10 +1274,29 @@ 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_append_hint(&err, "msi=on fails to create device."
> + " Please use msi=auto or off and try again");
> + 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;
> + } else if (!err) {
> + s->msi_in_use = true;
> + }
> + }
> +
Comments on intel_hda_realize() apply.
> 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 +1304,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);
Pattern #1.
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index c2a387a..b040575 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1044,12 +1044,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;
This is MSI device pattern #5: on whenever possible, else off, but
report an error when falling back to off.
Before your patch, this is pattern #2. Why do you add error reporting
here? You don't in other instances of pattern #2.
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index bbe5cca..d137ee2 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -26,6 +26,7 @@
> #include "hw/pci/msi.h"
> #include "hw/pci/msix.h"
> #include "trace.h"
> +#include "qapi/error.h"
>
> //#define DEBUG_XHCI
> //#define DEBUG_DATA
> @@ -3581,6 +3582,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);
>
> @@ -3591,6 +3593,21 @@ 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_append_hint(&err, "msi=on fails to create device."
> + " Please use msi=auto or off and try again");
> + error_propagate(errp, err);
> + return;
> + }
> + else if (ret < 0 && xhci->msi == ON_OFF_AUTO_AUTO) {
No line break between } and else. Please use scripts/checkpatch.pl.
> + /* If user doesn`t set it on, switch to non-msi variant silently */
> + error_free(err);
> + }
Comments on intel_hda_realize() apply.
> + }
> +
> if (xhci->numintrs > MAXINTRS) {
> xhci->numintrs = MAXINTRS;
> }
> @@ -3648,10 +3665,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,
Pattern #1.
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d091d8c..67d93f7 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -32,6 +32,7 @@
> #include "sysemu/sysemu.h"
> #include "pci.h"
> #include "trace.h"
> +#include "qapi/error.h"
>
> #define MSIX_CAP_LENGTH 12
>
> @@ -1171,6 +1172,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 +1186,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);
This is not an assertion, because bad host hardware can trigger it.
Okay.
> return ret;
> }
> vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
Otherwise pattern #2.
> 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);
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v6 11/11] pci: Convert msi_init() to Error and fix callers to check it
2016-06-01 12:37 ` Markus Armbruster
@ 2016-06-03 8:28 ` Cao jin
2016-06-03 11:30 ` Markus Armbruster
0 siblings, 1 reply; 28+ messages in thread
From: Cao jin @ 2016-06-03 8:28 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Michael S. Tsirkin, Jason Wang, Marcel Apfelbaum,
Alex Williamson, Hannes Reinecke, Dmitry Fleytman, Paolo Bonzini,
John Snow, Gerd Hoffmann
Hi Markus,
Thanks very much for your thorough review for the whole series, really
really impressed:)
On 06/01/2016 08:37 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>> msi_init() reports errors with error_report(), which is wrong
>> when it's used in realize().
>
> msix_init() has the same problem. Perhaps you can tackle it later.
>
Sure, I will take care of it after this one has passed the review.
>> + 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);
>> + }
>
> The conditional is superfluous.
>
> We call msi_init() only if d->msi != ON_OFF_AUTO_OFF. If it sets @err
> and d->msi == ON_OFF_AUTO_ON, we don't get here. Therefore, err implies
> d->msi == ON_OFF_AUTO_AUTO, and the conditional can be simplified to
>
> } else if (err) {
> error_free(err);
> }
>
> But protecting error_free() like that is pointless, as it does nothing
> when err is null. Simplify further to
>
> }
> assert(!err || d->msi == ON_OFF_AUTO_AUTO);
> /* With msi=auto, we fall back to MSI off silently */
> error_free(err);
>
> The assertion is more for aiding the reader than for catching mistakes.
>
It take me a little while to understand the following tightened error
checking:)
> The error checking could be tightened as follows:
>
> ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60,
> 1, true, false, &err);
> assert(!ret || ret == -ENOTSUP);
I guess you are assuming msi_init return 0 on success(all the following
example code are), and actually it is the behaviour of msix_init, you
mentioned the difference between them before. So I think it should be
assert(ret < 0 || ret == -ENOTSUP);
Right?
And I think it is better to add a comments on it, for newbie
understanding, like this:
/* -EINVAL means capability overlap, which is programming error for this
device, so, assert it */
Is the comment ok?
And I will add a new patch in this series to make msi_init return 0 on
success, and -error on failure, make it consistent with msix_init, so
your example code will apply.
> if (ret && d->msi == ON_OFF_AUTO_ON) {
> /* Can't satisfy user's explicit msi=on request, fail */
> error_append_hint(&err, "You have to use msi=auto (default)"
> " or msi=off with this machine type.\n");
> error_propagate(errp, err);
> return;
> }
> assert(!err || d->msi == ON_OFF_AUTO_AUTO);
> /* With msi=auto, we fall back to MSI off silently */
> error_free(err);
>
>> + }
>> +
>>
>> @@ -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);
>> + }
>
> Tighter error checking:
>
> ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, NULL);
> /* Fall back to INTx silently on -ENOTSUP */
> assert(!ret || ret == -ENOTSUP);
>
> More concise, too. No need for the include "qapi/error.h" then.
>
Learned, and /assert/ is for -EINVAL, so I will add a comment as I
mentioned above for easy understanding, So will I do for all the
following example code:)
>
>> + if (!vmxnet3_init_msix(s)) {
>> + VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
>
> It doesn't replace it here, but that's appropriate, because it doesn't
> touch MSI-X code, it only moves it.
>
will replace it when tackle msix_init
>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>> index fa0c50c..7f9131f 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>> @@ -54,6 +54,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);
>>
>> @@ -75,12 +76,18 @@ 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) &&
>> - msi_nonbroken) {
>> - err = msi_init(dev, 0, 1, true, true);
>> - if (err < 0) {
>> + if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
>
> Unnecessary churn, please use != ON_OFF_AUTO_OFF in PATCH 10.
>
>> + /* it means SHPC exists */
>
> Does it? Why?
>
The comments says: /* MSI is not applicable without SHPC */. And also
before the patch, we can see there are only following combination available:
[shpc: on, msi:on] [shpc: on, msi:off] [shpc: off, msi: off]
But there is no:
[shpc: off, msi: on]
So if msi != OFF, it implies shcp is on, right?
>
>> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
>> index c2a387a..b040575 100644
>> --- a/hw/scsi/vmw_pvscsi.c
>> +++ b/hw/scsi/vmw_pvscsi.c
>> @@ -1044,12 +1044,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;
>
> This is MSI device pattern #5: on whenever possible, else off, but
> report an error when falling back to off.
>
> Before your patch, this is pattern #2. Why do you add error reporting
> here? You don't in other instances of pattern #2.
>
I dunno...maybe just flash into my mind randomly:-[ will get rid of it
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v6 11/11] pci: Convert msi_init() to Error and fix callers to check it
2016-06-03 8:28 ` Cao jin
@ 2016-06-03 11:30 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2016-06-03 11:30 UTC (permalink / raw)
To: Cao jin
Cc: Michael S. Tsirkin, Jason Wang, qemu-devel, Dmitry Fleytman,
Alex Williamson, Hannes Reinecke, Marcel Apfelbaum, Paolo Bonzini,
John Snow, Gerd Hoffmann
Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> Hi Markus,
>
> Thanks very much for your thorough review for the whole series, really
> really impressed:)
Thank you :)
> On 06/01/2016 08:37 PM, Markus Armbruster wrote:
>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>
>>> msi_init() reports errors with error_report(), which is wrong
>>> when it's used in realize().
>>
>> msix_init() has the same problem. Perhaps you can tackle it later.
>>
>
> Sure, I will take care of it after this one has passed the review.
>
>>> + 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);
>>> + }
>>
>> The conditional is superfluous.
>>
>> We call msi_init() only if d->msi != ON_OFF_AUTO_OFF. If it sets @err
>> and d->msi == ON_OFF_AUTO_ON, we don't get here. Therefore, err implies
>> d->msi == ON_OFF_AUTO_AUTO, and the conditional can be simplified to
>>
>> } else if (err) {
>> error_free(err);
>> }
>>
>> But protecting error_free() like that is pointless, as it does nothing
>> when err is null. Simplify further to
>>
>> }
>> assert(!err || d->msi == ON_OFF_AUTO_AUTO);
>> /* With msi=auto, we fall back to MSI off silently */
>> error_free(err);
>>
>> The assertion is more for aiding the reader than for catching mistakes.
>>
>
> It take me a little while to understand the following tightened error
> checking:)
>
>> The error checking could be tightened as follows:
>>
>> ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60,
>> 1, true, false, &err);
>> assert(!ret || ret == -ENOTSUP);
>
> I guess you are assuming msi_init return 0 on success(all the
> following example code are), and actually it is the behaviour of
> msix_init, you mentioned the difference between them before. So I
> think it should be
>
> assert(ret < 0 || ret == -ENOTSUP);
>
> Right?
You're right in that my assertion is wrong: msi_init() returns an offset
on success (>= 0). It should be
assert(ret >= 0 || ret == -ENOTSUP);
In English: if msi_init() fails (ret < 0), any failure other than
-ENOTSUP is a programming error.
> And I think it is better to add a comments on it, for newbie
> understanding, like this:
>
> /* -EINVAL means capability overlap, which is programming error for
> this device, so, assert it */
>
> Is the comment ok?
If you feel a comment is needed, perhaps: Any error other than -ENOTSUP
(board's MSI support is broken) is a programming error.
> And I will add a new patch in this series to make msi_init return 0 on
> success, and -error on failure, make it consistent with msix_init, so
> your example code will apply.
Only possible if none of its users needs the offset.
Making the two consistent would be nice.
>> if (ret && d->msi == ON_OFF_AUTO_ON) {
>> /* Can't satisfy user's explicit msi=on request, fail */
>> error_append_hint(&err, "You have to use msi=auto (default)"
>> " or msi=off with this machine type.\n");
>> error_propagate(errp, err);
>> return;
>> }
>> assert(!err || d->msi == ON_OFF_AUTO_AUTO);
>> /* With msi=auto, we fall back to MSI off silently */
>> error_free(err);
>>
>>> + }
>>> +
>
>>>
>>> @@ -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);
>>> + }
>>
>> Tighter error checking:
>>
>> ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, NULL);
>> /* Fall back to INTx silently on -ENOTSUP */
>> assert(!ret || ret == -ENOTSUP);
>>
>> More concise, too. No need for the include "qapi/error.h" then.
>>
>
> Learned, and /assert/ is for -EINVAL, so I will add a comment as I
> mentioned above for easy understanding, So will I do for all the
> following example code:)
>
>>
>>> + if (!vmxnet3_init_msix(s)) {
>>> + VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
>>
>> It doesn't replace it here, but that's appropriate, because it doesn't
>> touch MSI-X code, it only moves it.
>>
>
> will replace it when tackle msix_init
>
>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>>> index fa0c50c..7f9131f 100644
>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>> @@ -54,6 +54,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);
>>>
>>> @@ -75,12 +76,18 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
if (bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) {
dev->config[PCI_INTERRUPT_PIN] = 0x1;
memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
shpc_bar_size(dev));
err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
if (err) {
goto shpc_error;
}
} else {
/* MSI is not applicable without SHPC */
---> bridge_dev->msi = ON_OFF_AUTO_OFF;
SHPC is required for MSI.
SHPC is controlled by property "shpc", default on.
Switching MSI off for shpc=off,msi=auto is fine. But shpc=off,msi=on
should be an error.
This device messes with its configuration, like megasas does. I
explained there why that's problematic, but that fixing it is not this
patch's business.
}
err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
if (err) {
>>> goto slotid_error;
>>> }
>>>
>>> - 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) {
>>> + if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
>>
>> Unnecessary churn, please use != ON_OFF_AUTO_OFF in PATCH 10.
>>
>>> + /* it means SHPC exists */
>>
>> Does it? Why?
It does because of the spot I marked ---> above.
Is a comment necessary here?
> The comments says: /* MSI is not applicable without SHPC */. And also
> before the patch, we can see there are only following combination
> available:
> [shpc: on, msi:on] [shpc: on, msi:off] [shpc: off, msi: off]
>
> But there is no:
> [shpc: off, msi: on]
>
> So if msi != OFF, it implies shcp is on, right?
Before the patch:
shpc msi | msi' SHPC MSI
off off | off no no
off on | off no no
on off | off yes no
on on | on yes yes*
where
shpc is the value of property "shpc"
msi is the value of property "msi" before realize
msi' is the value after realize,
SHPC is whether the device has the SHPC structure
MSI is whether it has the MSI capability
yes* means yes unless !msi_nonbroken
After the patch:
shpc msi | msi' SHPC MSI
off off | off no no unchanged
off on | off no no unchanged, but should be an error
off auto | off no no new, good
on off | off yes no unchanged
on on | on yes yes !msi_nonbroken is now an error, good
on auto | auto yes yes* new, good
I can't help to wonder whether we really need property "shpc". Is the
combination "have SHPC but not MSI" useful? Do we have to maintain
backward compatibility? You may not be the right person to answer these
questions. That's okay, your patch can simply preserve the status quo.
>>> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
>>> index c2a387a..b040575 100644
>>> --- a/hw/scsi/vmw_pvscsi.c
>>> +++ b/hw/scsi/vmw_pvscsi.c
>>> @@ -1044,12 +1044,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;
>>
>> This is MSI device pattern #5: on whenever possible, else off, but
>> report an error when falling back to off.
>>
>> Before your patch, this is pattern #2. Why do you add error reporting
>> here? You don't in other instances of pattern #2.
>>
>
> I dunno...maybe just flash into my mind randomly:-[ will get rid of it
Thanks!
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init()
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
` (10 preceding siblings ...)
2016-05-24 4:04 ` [Qemu-devel] [PATCH v6 11/11] pci: Convert msi_init() to Error and fix callers to check it Cao jin
@ 2016-06-01 3:06 ` Cao jin
2016-06-01 6:59 ` Cao jin
12 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-06-01 3:06 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Jason Wang, Markus Armbruster,
Marcel Apfelbaum, Alex Williamson, Hannes Reinecke,
Dmitry Fleytman, Paolo Bonzini, John Snow, Gerd Hoffmann
ping?
On 05/24/2016 12:04 PM, Cao jin wrote:
> v6 changelog:
> 1. make "assert ENOSPC" the 1st one in the series, and remove ENOSPC line
> of comments of msi_init(). also fix to other minor comments.
> 2. Because semantics changes, add hint message for devices who have msi/msix
> property, to help old machine user to know what should do
> 3. update pci_bridge_dev hehaviour, because it has non-msi variant, but it can
> never fall back to INTx before the patch. make it behaviour like others
> 4. mptsas: forget to assign s->msi_in_use before, now make it.
>
> About test: Only compiled every patch.
>
> 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>
>
> Cao jin (11):
> pci core: assert ENOSPC when add capability
> fix some coding style problems
> change pvscsi_init_msi() type to void
> megasas: Fix
> mptsas: change .realize function name
> usb xhci: change msi/msix property type
> intel-hda: change msi property type
> mptsas: change msi property type
> megasas: change msi/msix property type
> pci bridge dev: change msi property type
> pci: Convert msi_init() to Error and fix callers to check it
>
> hw/audio/intel-hda.c | 23 ++++++++++++++----
> hw/ide/ich.c | 17 +++++++++-----
> hw/net/vmxnet3.c | 44 ++++++++++++++--------------------
> hw/pci-bridge/ioh3420.c | 12 ++++++++--
> hw/pci-bridge/pci_bridge_dev.c | 31 +++++++++++++++++-------
> hw/pci-bridge/xio3130_downstream.c | 11 +++++++--
> hw/pci-bridge/xio3130_upstream.c | 8 ++++++-
> hw/pci/msi.c | 25 ++++++++++++++++++--
> hw/pci/pci.c | 6 ++---
> hw/scsi/megasas.c | 48 +++++++++++++++++++++-----------------
> hw/scsi/mptsas.c | 32 ++++++++++++++++++-------
> hw/scsi/mptsas.h | 3 ++-
> hw/scsi/vmw_pvscsi.c | 10 ++++----
> hw/usb/hcd-xhci.c | 33 +++++++++++++++++++-------
> hw/vfio/pci.c | 7 ++++--
> include/hw/pci/msi.h | 3 ++-
> 16 files changed, 209 insertions(+), 104 deletions(-)
>
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init()
2016-05-24 4:04 [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
` (11 preceding siblings ...)
2016-06-01 3:06 ` [Qemu-devel] [PATCH v6 00/11] Add param Error ** for msi_init() Cao jin
@ 2016-06-01 6:59 ` Cao jin
12 siblings, 0 replies; 28+ messages in thread
From: Cao jin @ 2016-06-01 6:59 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Jason Wang, Markus Armbruster,
Marcel Apfelbaum, Alex Williamson, Hannes Reinecke,
Dmitry Fleytman, Paolo Bonzini, John Snow, Gerd Hoffmann
ping again...
because got many feedback "<xxx@redhat.com> was undeliverable."
On 05/24/2016 12:04 PM, Cao jin wrote:
> v6 changelog:
> 1. make "assert ENOSPC" the 1st one in the series, and remove ENOSPC line
> of comments of msi_init(). also fix to other minor comments.
> 2. Because semantics changes, add hint message for devices who have msi/msix
> property, to help old machine user to know what should do
> 3. update pci_bridge_dev hehaviour, because it has non-msi variant, but it can
> never fall back to INTx before the patch. make it behaviour like others
> 4. mptsas: forget to assign s->msi_in_use before, now make it.
>
> About test: Only compiled every patch.
>
> 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>
>
> Cao jin (11):
> pci core: assert ENOSPC when add capability
> fix some coding style problems
> change pvscsi_init_msi() type to void
> megasas: Fix
> mptsas: change .realize function name
> usb xhci: change msi/msix property type
> intel-hda: change msi property type
> mptsas: change msi property type
> megasas: change msi/msix property type
> pci bridge dev: change msi property type
> pci: Convert msi_init() to Error and fix callers to check it
>
> hw/audio/intel-hda.c | 23 ++++++++++++++----
> hw/ide/ich.c | 17 +++++++++-----
> hw/net/vmxnet3.c | 44 ++++++++++++++--------------------
> hw/pci-bridge/ioh3420.c | 12 ++++++++--
> hw/pci-bridge/pci_bridge_dev.c | 31 +++++++++++++++++-------
> hw/pci-bridge/xio3130_downstream.c | 11 +++++++--
> hw/pci-bridge/xio3130_upstream.c | 8 ++++++-
> hw/pci/msi.c | 25 ++++++++++++++++++--
> hw/pci/pci.c | 6 ++---
> hw/scsi/megasas.c | 48 +++++++++++++++++++++-----------------
> hw/scsi/mptsas.c | 32 ++++++++++++++++++-------
> hw/scsi/mptsas.h | 3 ++-
> hw/scsi/vmw_pvscsi.c | 10 ++++----
> hw/usb/hcd-xhci.c | 33 +++++++++++++++++++-------
> hw/vfio/pci.c | 7 ++++--
> include/hw/pci/msi.h | 3 ++-
> 16 files changed, 209 insertions(+), 104 deletions(-)
>
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 28+ messages in thread