Re: [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it

[Marcel Apfelbaum <marcel@redhat.com>]

On 06/13/2016 02:09 PM, Cao jin wrote:
>
>
> On 06/13/2016 06:16 PM, Marcel Apfelbaum wrote:
>> On 06/10/2016 12:54 PM, Cao jin wrote:
>
>>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
>>> index 692283f..a06d184 100644
>>> --- a/hw/net/e1000e.c
>>> +++ b/hw/net/e1000e.c
>>> @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s)
>>>   {
>>>       int res;
>>>
>>> -    res = msi_init(PCI_DEVICE(s),
>>> -                   0xD0,   /* MSI capability offset              */
>>> -                   1,      /* MAC MSI interrupts                 */
>>> -                   true,   /* 64-bit message addresses supported */
>>> -                   false); /* Per vector mask supported          */
>>> +    res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
>>>
>>
>> Why did you chose to remove author's comments?
>>
>
> Because msi_init() has its function comments now, which is the same is the author`s comments, I guess author add these comments because function has no comment before, remove comments also is to save
> screen space:)
>
> some macros of some devices is also unnecessary I think, because we have comments of msi_init().
>

Right.

>>> +
>>> +#define VMXNET3_USE_64BIT         (true)
>>> +#define VMXNET3_PER_VECTOR_MASK   (false)
>>> +
>
> like these macros, but it does't take too much space as above one I feel, so I didn't touch them.
>
>>> @@ -56,14 +57,17 @@ 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) {
>>> +        assert(rc == -ENOTSUP);
>>> +        error_report_err(err);
>>
>> Hi Jin, Markus
>>
>> It looks a little weird to me to check for negative error code
>> and then assert for a specific error as the only "valid error".
>> Maybe we should assert inside msi_init to leave the callers "clean"?
>>
> Hi Marcel,
>
> I think it is because: except assigned device(vfio), the emulated pci devices won`t have config space overlapped(-EINVAL), unless programming error.
>

Understood, thanks for the explanation.

For the PCI part:
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

> If implemented as you said, I guess there need a judgement (if..else..) to check if current device is assigned in msi_init(), or else assert the error
>



>> I am well aware a lot of time was spent for this series, but I just
>> spotted it and I want to be sure we are doing it right.
>>
>> Thanks,
>> Marcel
>>
>