Re: [Qemu-devel] [PATCH v3 3/8] pci: Convert msix_init() to Error and fix callers to check it

[Cao jin]

On 09/30/2016 03:01 PM, Markus Armbruster wrote:

Cao jin  writes:


On 09/29/2016 10:17 PM, Markus Armbruster wrote:




Intentional change?  I like the old one better.



Oh...it was lost by me. I was planning move these comments into a
separate patch, but later feel that it is not worth the trouble, so I
undo the movement, it is lost during the process.


Let's restore it then.



sure, I will send new version to fix all these comments, after get 
confirmation from Dmitry about the last patch: vmxnet3


--
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v3 3/8] pci: Convert msix_init() to Error and fix callers to check it

[Markus Armbruster]

Cao jin  writes:

> On 09/29/2016 10:17 PM, Markus Armbruster wrote:
>> Cao jin  writes:
>>
>
>>>
>>> -/* Initialize the MSI-X structures */
>>> +/* Make PCI device @dev MSI-X capable
>>> + * @nentries is the max number of MSI-X vectors that the device support.
>>> + * @table_bar is the MemoryRegion that MSI-X table structure resides.
>>> + * @table_bar_nr is number of base address register corresponding to 
>>> @table_bar.
>>> + * @table_offset indicates the offset that the MSI-X table structure 
>>> starts with
>>> + * in @table_bar.
>>> + * @pba_bar is the MemoryRegion that the Pending Bit Array structure 
>>> resides.
>>> + * @pba_bar_nr is number of base address register corresponding to 
>>> @pba_bar.
>>> + * @pba_offset indicates the offset that the Pending Bit Array structure
>>> + * starts with in @pba_bar.
>>> + * Non-zero @cap_pos puts capability MSI-X at that offset in PCI config 
>>> space.
>>> + *
>>> + * Return 0 on success; return -errno on error:
>>
>> Previous version had:
>>
>>+ * @errp is for returning errors.
>>+ *
>>+ * Return 0 on success; set @errp and return -errno on error.
>>
>> Intentional change?  I like the old one better.
>>
>
> Oh...it was lost by me. I was planning move these comments into a
> separate patch, but later feel that it is not worth the trouble, so I
> undo the movement, it is lost during the process.

Let's restore it then.

>> Might conflict with Eric Auger's "Convert VFIO-PCI to realize" series,
>> but resolving that shouldn't be hard.
>>
>> [...]
>>
>> The conversion looks good to me now.

Re: [Qemu-devel] [PATCH v3 3/8] pci: Convert msix_init() to Error and fix callers to check it

[Cao jin]

On 09/29/2016 10:17 PM, Markus Armbruster wrote:

Cao jin  writes:





-/* Initialize the MSI-X structures */
+/* Make PCI device @dev MSI-X capable
+ * @nentries is the max number of MSI-X vectors that the device support.
+ * @table_bar is the MemoryRegion that MSI-X table structure resides.
+ * @table_bar_nr is number of base address register corresponding to 
@table_bar.
+ * @table_offset indicates the offset that the MSI-X table structure starts 
with
+ * in @table_bar.
+ * @pba_bar is the MemoryRegion that the Pending Bit Array structure resides.
+ * @pba_bar_nr is number of base address register corresponding to @pba_bar.
+ * @pba_offset indicates the offset that the Pending Bit Array structure
+ * starts with in @pba_bar.
+ * Non-zero @cap_pos puts capability MSI-X at that offset in PCI config space.
+ *
+ * Return 0 on success; return -errno on error:


Previous version had:

   + * @errp is for returning errors.
   + *
   + * Return 0 on success; set @errp and return -errno on error.

Intentional change?  I like the old one better.



Oh...it was lost by me. I was planning move these comments into a 
separate patch, but later feel that it is not worth the trouble, so I 
undo the movement, it is lost during the process.




Might conflict with Eric Auger's "Convert VFIO-PCI to realize" series,
but resolving that shouldn't be hard.

[...]

The conversion looks good to me now.


.



--
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v3 3/8] pci: Convert msix_init() to Error and fix callers to check it

[Markus Armbruster]

Cao jin  writes:

> msix_init() reports errors with error_report(), which is wrong when
> it's used in realize().  The same issue was fixed for msi_init() in
> commit 1108b2f.
>
> For some devices(like e1000e, vmxnet3) who won't fail because of
> msix_init's failure, suppress the error report by passing NULL error object.
>
> Bonus: add comment for msix_init.
>
> CC: Jiri Pirko 
> CC: Gerd Hoffmann 
> CC: Dmitry Fleytman 
> CC: Jason Wang 
> CC: Michael S. Tsirkin 
> CC: Hannes Reinecke 
> CC: Paolo Bonzini 
> CC: Alex Williamson 
> CC: Markus Armbruster 
> CC: Marcel Apfelbaum 
> Signed-off-by: Cao jin 
[...]
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 0cee631..d6b38fe 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -21,6 +21,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/xen/xen.h"
>  #include "qemu/range.h"
> +#include "qapi/error.h"
>  
>  #define MSIX_CAP_LENGTH 12
>  
> @@ -238,11 +239,28 @@ static void msix_mask_all(struct PCIDevice *dev, 
> unsigned nentries)
>  }
>  }
>  
> -/* Initialize the MSI-X structures */
> +/* Make PCI device @dev MSI-X capable
> + * @nentries is the max number of MSI-X vectors that the device support.
> + * @table_bar is the MemoryRegion that MSI-X table structure resides.
> + * @table_bar_nr is number of base address register corresponding to 
> @table_bar.
> + * @table_offset indicates the offset that the MSI-X table structure starts 
> with
> + * in @table_bar.
> + * @pba_bar is the MemoryRegion that the Pending Bit Array structure resides.
> + * @pba_bar_nr is number of base address register corresponding to @pba_bar.
> + * @pba_offset indicates the offset that the Pending Bit Array structure
> + * starts with in @pba_bar.
> + * Non-zero @cap_pos puts capability MSI-X at that offset in PCI config 
> space.
> + *
> + * Return 0 on success; return -errno on error:

Previous version had:

  + * @errp is for returning errors.
  + *
  + * Return 0 on success; set @errp and return -errno on error.

Intentional change?  I like the old one better.

> + * -ENOTSUP means lacking msi support for a msi-capable platform.
> + * -EINVAL means capability overlap, happens when @cap_pos is non-zero,
> + * also means a programming error, except device assignment, which can check
> + * if a real HW is broken.*/
>  int msix_init(struct PCIDevice *dev, unsigned short nentries,
>MemoryRegion *table_bar, uint8_t table_bar_nr,
>unsigned table_offset, MemoryRegion *pba_bar,
> -  uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
> +  uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
> +  Error **errp)
>  {
>  int cap;
>  unsigned table_size, pba_size;
[...]
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 7bfa17c..87f4e11 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1349,6 +1349,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev)
>  static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
>  {
>  int ret;
> +Error *err = NULL;
>  
>  vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
>  sizeof(unsigned long));
> @@ -1356,12 +1357,14 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int 
> pos)
>  vdev->bars[vdev->msix->table_bar].region.mem,
>  vdev->msix->table_bar, vdev->msix->table_offset,
>  vdev->bars[vdev->msix->pba_bar].region.mem,
> -vdev->msix->pba_bar, vdev->msix->pba_offset, pos);
> +vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
> +);
>  if (ret < 0) {
>  if (ret == -ENOTSUP) {
>  return 0;
>  }
> -error_report("vfio: msix_init failed");
> +error_prepend(, "vfio: msix_init failed: ");
> +error_report_err(err);
>  return ret;
>  }
>  

Might conflict with Eric Auger's "Convert VFIO-PCI to realize" series,
but resolving that shouldn't be hard.

[...]

The conversion looks good to me now.