Cao jin <caoj.f...@cn.fujitsu.com> writes:
> Hi Markus,
> I have to say, you really did a amazing review for this "trivial
> "patch, thanks a lot & really appreciate it:)
Thanks! I'm afraid the problem you picked isn't trivial, but I hope
it's still simple enough to be a useful exercise to get you going with
the code.
> On 12/07/2015 05:59 PM, Markus Armbruster wrote:
>> Cao jin <caoj.f...@cn.fujitsu.com> writes:
>>
>>> msi_init() is a supporting function in PCI device initialization, in order
>>> to
>>> convert .init() to .realize(), it should be modified first. Also modify the
>>> callers
>>>
>>> Bonus: add more comment for msi_init().
>>
>> Incomplete. See notes on impact inline.
>>
>>> Signed-off-by: Cao jin <caoj.f...@cn.fujitsu.com>
>>> ---
>>> hw/audio/intel-hda.c | 7 ++++++-
>>> hw/ide/ich.c | 2 +-
>>> hw/net/vmxnet3.c | 3 ++-
>>> hw/pci-bridge/ioh3420.c | 6 +++++-
>>> hw/pci-bridge/pci_bridge_dev.c | 6 +++++-
>>> hw/pci-bridge/xio3130_downstream.c | 7 ++++++-
>>> hw/pci-bridge/xio3130_upstream.c | 7 ++++++-
>>> hw/pci/msi.c | 17 +++++++++++++----
>>> hw/scsi/megasas.c | 12 +++++++++---
>>> hw/scsi/vmw_pvscsi.c | 3 ++-
>>> hw/usb/hcd-xhci.c | 5 ++++-
>>> hw/vfio/pci.c | 3 ++-
>>> include/hw/pci/msi.h | 4 ++--
>>> 13 files changed, 63 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
>>> index 433463e..9d733da 100644
>>> --- a/hw/audio/intel-hda.c
>>> +++ b/hw/audio/intel-hda.c
>>> @@ -1130,6 +1130,7 @@ static void intel_hda_realize(PCIDevice *pci, Error
>>> **errp)
>>> {
>>> IntelHDAState *d = INTEL_HDA(pci);
>>> uint8_t *conf = d->pci.config;
>>> + int ret;
>>>
>>> d->name = object_get_typename(OBJECT(d));
>>>
>>> @@ -1142,7 +1143,11 @@ static void intel_hda_realize(PCIDevice *pci, Error
>>> **errp)
>>> "intel-hda", 0x4000);
>>> pci_register_bar(&d->pci, 0, 0, &d->mmio);
>>> if (d->msi) {
>>> - msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
>>> + ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true,
>>> + false, errp);
>>> + if(ret < 0) {
>>
>> Please use scripts/checkpatch.pl to check your patches. It's
>> occasionally wrong, so use your judgement.
>>
>
> Thanks for the tips, seems I got dizzy looking because many trivial
> place need to be modified...
>
>>> + return;
>>
>> This returns with the device in a half-realized state. Do we have to
>> undo prior side effects to put it back into unrealized state? See also
>> ioh3420_initfn() below.
>>
>> Before: msi_init() failure is ignored. After: it makes device
>> realization fail. To assess impact, we need to understand how
>> msi_init() can fail.
>>
>
> It seems I missed the reality: devices are default to be hot-pluggable
> & most devices are hot-pluggable:-[ Because when cold plugged, process
> will exit on device-init failing, so, the half-realized state doesn`t
> matter in this condition.
> Will rework it later.
In theory, realize() should always fail cleanly. In practice, unclean
realize() failure doesn't matter when it's fatal anyway. Some devices
are only used where it's always fatal. See also "Error handling in
realize() methods" I just sent to the list; I hope we can come up with
some guidance on when shortcuts in realize() methods are tolerable.
>>> + }
>>> }
>>>
>>> hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
>>> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
>>> index 16925fa..94b1809 100644
>>> --- a/hw/ide/ich.c
>>> +++ b/hw/ide/ich.c
>>> @@ -145,7 +145,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error
>>> **errp)
>>> /* 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);
>>> + msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, errp);
>>
>> Do we have to put the device back into unrealized state on failure?
>>
>>> }
>>>
>>> static void pci_ich9_uninit(PCIDevice *dev)
>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>> index 5e3a233..4269141 100644
>>> --- a/hw/net/vmxnet3.c
>>> +++ b/hw/net/vmxnet3.c
>>> @@ -2140,9 +2140,10 @@ vmxnet3_init_msi(VMXNET3State *s)
>>> {
>>> PCIDevice *d = PCI_DEVICE(s);
>>> int res;
>>> + Error *local_err = NULL;
>>>
>>> res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
>>> - VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>>> + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, &local_err);
>>> if (0 > res) {
>>> VMW_WRPRN("Failed to initialize MSI, error %d", res);
>>> s->msi_used = false;
>>
>> The error is neither propagated nor handled, and the error object leaks.
>>
>> Since this function can't handle it, it needs to propagate it. Requires
>> adding an Error ** parameter.
>>
>
> [*]Actually, here is my consideration: a device-realize function(take
> the following ioh3420 for example) will call many supporting functions
> like msi_init(), so I am planning, every supporting function goes into
> a patch first, then every "device convert to realize()" goes into a
> patch, otherwise, it may will be a big patch for the reviewer. That`s
> why I didn`t add Error ** param, and propagate it, and plan to do it
> in "convert to realize()" patch. But for now, I think this patch
> should at least be successfully compiled & won`t impact the existed
> things.
>
> Yes, it seems may have leaks when error happens, but will be fixed
> when the "convert to realize()" patch comes out.
>
> I am not sure whether the plan is ok, So, How do you think?
If you don't want to propagate the error further in this patch, you need
to free it:
if (0 > res) {
VMW_WRPRN("Failed to initialize MSI, error %d", res);
error_free(local_err);
s->msi_used = false;
While there, you could improve the error message by printing
error_get_pretty(local_err)) instead of res, but I wouldn't bother,
since we'll have to replace it with error_propagate() anyway.
>>> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
>>> index eead195..05b004a 100644
>>> --- a/hw/pci-bridge/ioh3420.c
>>> +++ b/hw/pci-bridge/ioh3420.c
>>> @@ -96,6 +96,7 @@ static int ioh3420_initfn(PCIDevice *d)
>>> PCIEPort *p = PCIE_PORT(d);
>>> PCIESlot *s = PCIE_SLOT(d);
>>> int rc;
>>> + Error *local_err = NULL;
>>>
>>> pci_bridge_initfn(d, TYPE_PCIE_BUS);
>>> pcie_port_init_reg(d);
>>> @@ -105,12 +106,15 @@ 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);
>>> + IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
>>> + &local_err);
>>> if (rc < 0) {
>>> goto err_bridge;
>>
>> The error is neither propagated nor handled, and the error object leaks.
>>
>> Since this is an init method, it should report errors with error_report
>> and return -1. The latter is there, but the former is missing. You
>> need to error_report_err(local_err).
>>
>
> Refer previous comment[*]: Was planning propagate it in "convert to
> realize()" patch later.
Again, if you don't want to propagate the error further in this patch,
you need to free it. Actually, you have to report and free it; see
msi_init() below for why. The obvious way to do that:
if (rc < 0) {
error_report_err(local_err);
goto err_bridge;
}
By the way, I use err instead of local_err. Matter of taste.
>>> }
>>> +
>>> rc = pcie_cap_init(d, IOH_EP_EXP_OFFSET,
>>> PCI_EXP_TYPE_ROOT_PORT, p->port);
>>
>> Aside, not for this patch: this function (and many more) will have to be
>> converted to Error as well before we can convert to realize.
>>
>
> Yes, I am planning every supporting function goes into a
> patch. Because msi & msix are too similar, so I put them together and
> send out first...
>
>>> if (rc < 0) {
>>> goto err_msi;
>>
>> Further down:
>>
>> err:
>> pcie_chassis_del_slot(s);
>> err_pcie_cap:
>> pcie_cap_exit(d);
>> err_msi:
>> msi_uninit(d);
>> err_bridge:
>> pci_bridge_exitfn(d);
>> return rc;
>> }
>>
>> Note that we carefully undo side effects on failure.
>>
>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>>> index bc3e1b7..13e8583 100644
>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>> @@ -51,6 +51,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);
>>>
>>> @@ -66,13 +67,15 @@ 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_supported) {
>>> - err = msi_init(dev, 0, 1, true, true);
>>> + err = msi_init(dev, 0, 1, true, true, &local_err);
>>> if (err < 0) {
>>> goto msi_error;
>>
>> The error is neither propagated nor handled, and the error object leaks.
>>
>> Again, you need error_report_err(local_err).
>>
>
> Refer the previous comment[*]: was planning propagate it in "convert
> to realize()" patch later.
>
>>> }
>>> @@ -84,6 +87,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 b4dd25f..8e91034 100644
>>> --- a/hw/pci-bridge/xio3130_downstream.c
>>> +++ b/hw/pci-bridge/xio3130_downstream.c
>>> @@ -59,21 +59,25 @@ static int xio3130_downstream_initfn(PCIDevice *d)
>>> PCIEPort *p = PCIE_PORT(d);
>>> PCIESlot *s = PCIE_SLOT(d);
>>> int rc;
>>> + Error *local_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,
>>> + &local_err);
>>> if (rc < 0) {
>>> goto err_bridge;
>>
>> Likewise.
>>
>>> }
>>> +
>>> 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) {
>>> @@ -103,6 +107,7 @@ err_msi:
>>> msi_uninit(d);
>>> err_bridge:
>>> pci_bridge_exitfn(d);
>>> +
>>> return rc;
>>> }
>>>
>>> diff --git a/hw/pci-bridge/xio3130_upstream.c
>>> b/hw/pci-bridge/xio3130_upstream.c
>>> index 434c8fd..bae49f6 100644
>>> --- a/hw/pci-bridge/xio3130_upstream.c
>>> +++ b/hw/pci-bridge/xio3130_upstream.c
>>> @@ -55,26 +55,31 @@ static int xio3130_upstream_initfn(PCIDevice *d)
>>> {
>>> PCIEPort *p = PCIE_PORT(d);
>>> int rc;
>>> + Error *local_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,
>>> + &local_err);
>>> if (rc < 0) {
>>> goto err_bridge;
>>
>> Likewise.
>>
>>> }
>>> +
>>> 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) {
>>> goto err_msi;
>>> }
>>> +
>>> pcie_cap_flr_init(d);
>>> pcie_cap_deverr_init(d);
>>> rc = pcie_aer_init(d, XIO3130_AER_OFFSET);
>>> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
>>> index c1dd531..961f114 100644
>>> --- a/hw/pci/msi.c
>>> +++ b/hw/pci/msi.c
>>> @@ -150,15 +150,23 @@ bool msi_enabled(const PCIDevice *dev)
>>> PCI_MSI_FLAGS_ENABLE);
>>> }
>>>
>>> -int msi_init(struct PCIDevice *dev, uint8_t offset,
>>> - unsigned int nr_vectors, bool msi64bit, bool
>>> msi_per_vector_mask)
>>> +/*
>>> + * @nr_vectors: Multiple Message Capable field of Message Control register
>>> + * @msi64bit: support 64-bit message address or not
>>> + * @msi_per_vector_mask: support per-vector masking or not
>>> + *
>>> + * return: MSI capability offset in config space
>>> + */
>>> +int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int
>>> nr_vectors,
>>> + bool msi64bit, bool msi_per_vector_mask, Error **errp)
>>
>> Let's see how this function can fail.
>>
>>> {
>>> unsigned int vectors_order;
>>> - uint16_t flags;
>>> + uint16_t flags; /* Message Control register value */
>>> uint8_t cap_size;
>>> int config_offset;
>>>
>>> if (!msi_supported) {
>>> + error_setg(errp, "MSI is not supported by interrupt controller");
>>> return -ENOTSUP;
>>
>> Attempt to use MSI when !msi_supported.
>>
>> Before: silently return -ENOTSUP.
>>
>> After: additionally pass a suitable error.
>>
>>> }
>>>
>>> @@ -182,7 +190,7 @@ 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;
>>
>> Can't set PCI_CAP_ID_MSI.
>>
>> Before: report with error_report_err() and return -errno.
>>
>> After: pass a suitable error and return -errno.
>>
>>> }
>>> @@ -205,6 +213,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
>>> pci_set_long(dev->wmask + msi_mask_off(dev, msi64bit),
>>> 0xffffffff >> (PCI_MSI_VECTORS_MAX - nr_vectors));
>>> }
>>> +
>>> return config_offset;
>>> }
>>
>> To assess the patch's impact, you need to compare before / after for
>> both failure modes and each caller. I suspect the result will promote
>> your patch from "prepare for realize" to "fix to catch and report
>> errors".
Let's do that for ioh3420_initfn().
Before:
* when !msi_supported, ioh3420_initfn() fails silently
* when pci_add_capability() fails, we report with error_report_err(),
and ioh3420_initfn() fails
After (your patch):
* when !msi_supported, ioh3420_initfn() fails silently
* when pci_add_capability2() fails, ioh3420_initfn() fails silently,
regression
After (with ioh3420_initfn() calling error_report_err(), as per my
review):
* when !msi_supported, ioh3420_initfn() reports with error_report_err()
and fails, bug fix (mention in commit message)
* when pci_add_capability2() fails, reports with error_report_err() and
fails
Do that for every caller of msi_init(), then summarize the patch's
impact change in the commit message.
> Thanks a lot for the direction:) but I still have a question: if I
> start off by per-device, then will modify every supporting function,
> and common supporting function will impact other device, so will need
> to convert other device together, and this will result in a huge patch
> contains converting of many devices and supporting functions, what do
> you think of it?
A huge patch would be much harder to review. Limiting this patch to
just msi_init() is sensible. But the patch mustn't break things. Not
even if you plan to fix the breakage in later patches.
[...]
