[PATCH v1.1 for-4.11 0/3] vpci bugfixes

[PATCH v1.1 for-4.11 0/3] vpci bugfixes

[Roger Pau Monne]

Hello,

This tree patches are bugfixes for the vPCI code merged last week. They
where spotted by Coverity.

Version v1.1 is due to the fact that I've failed to Cc the maintainers
in v1, sorry for the spam.

Thanks, Roger.

Roger Pau Monne (3):
  vpci/bars: fix error message
  vpci/msix: fix incorrect usage of bitmask
  vpci/msi: fix size of the vectors fields

 xen/drivers/vpci/header.c | 2 +-
 xen/drivers/vpci/msix.c   | 2 +-
 xen/include/xen/vpci.h    | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.16.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v1.1 for-4.11 1/3] vpci/bars: fix error message

[Roger Pau Monne]

Error message is incorrectly using map when it should be using
map->map instead.

Reported-by: Coverity
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/drivers/vpci/header.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 91a71ca66e..0ec4c082a6 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -63,7 +63,7 @@ static int map_range(unsigned long s, unsigned long e, void *data,
         {
             printk(XENLOG_G_WARNING
                    "Failed to identity %smap [%lx, %lx] for d%d: %d\n",
-                   map ? "" : "un", s, e, map->d->domain_id, rc);
+                   map->map ? "" : "un", s, e, map->d->domain_id, rc);
             break;
         }
         ASSERT(rc < size);
-- 
2.16.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v1.1 for-4.11 2/3] vpci/msix: fix incorrect usage of bitmask

[Roger Pau Monne]

The bitmask to clear the low bits of the address field should be
~0xffffffffull, the current mask clears both the low and the high bits
of the address field, which is a bug.

Reported-by: Coverity
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/drivers/vpci/msix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 3b378c2e51..bcf63256f6 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -328,7 +328,7 @@ static int msix_write(struct vcpu *v, unsigned long addr, unsigned int len,
             entry->addr = data;
             break;
         }
-        entry->addr &= ~0xffffffff;
+        entry->addr &= ~0xffffffffull;
         entry->addr |= data;
         break;
 
-- 
2.16.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v1.1 for-4.11 3/3] vpci/msi: fix size of the vectors fields

[Roger Pau Monne]

The current size (5bits) is not enough to store the maximum number of
vectors (32), bump it by one bit.

Note that the size of the struct is still the same.

Reported-by: Coverity
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/include/xen/vpci.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index cb39e0ebea..fac12a1c42 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -100,7 +100,7 @@ struct vpci {
         /* Data. */
         uint16_t data;
         /* Maximum number of vectors supported by the device. */
-        uint8_t max_vectors : 5;
+        uint8_t max_vectors : 6;
         /* Enabled? */
         bool enabled        : 1;
         /* Supports per-vector masking? */
@@ -108,7 +108,7 @@ struct vpci {
         /* 64-bit address capable? */
         bool address64      : 1;
         /* Number of vectors configured. */
-        uint8_t vectors     : 5;
+        uint8_t vectors     : 6;
         /* Arch-specific data. */
         struct vpci_arch_msi arch;
     } *msi;
-- 
2.16.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1.1 for-4.11 1/3] vpci/bars: fix error message

[Jan Beulich]

>>> On 26.03.18 at 13:28, <roger.pau@citrix.com> wrote:
> Error message is incorrectly using map when it should be using
> map->map instead.
> 
> Reported-by: Coverity

Coverity ID: 1430811

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1.1 for-4.11 2/3] vpci/msix: fix incorrect usage of bitmask

[Jan Beulich]

>>> On 26.03.18 at 13:28, <roger.pau@citrix.com> wrote:
> The bitmask to clear the low bits of the address field should be
> ~0xffffffffull, the current mask clears both the low and the high bits
> of the address field, which is a bug.
> 
> Reported-by: Coverity
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1.1 for-4.11 3/3] vpci/msi: fix size of the vectors fields

[Jan Beulich]

>>> On 26.03.18 at 13:28, <roger.pau@citrix.com> wrote:
> The current size (5bits) is not enough to store the maximum number of
> vectors (32), bump it by one bit.
> 
> Note that the size of the struct is still the same.

Coverity ID: 1430810

> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -100,7 +100,7 @@ struct vpci {
>          /* Data. */
>          uint16_t data;
>          /* Maximum number of vectors supported by the device. */
> -        uint8_t max_vectors : 5;
> +        uint8_t max_vectors : 6;
>          /* Enabled? */
>          bool enabled        : 1;
>          /* Supports per-vector masking? */

To aid simplicity of generated code, I had specifically asked for the
current 5-1-1-1-5 arrangement of bit field members. Now that the
5s need bumping to 6, we'll want 6-1-1-6-1, so please move
"enabled" down (also resulting in all feature flags coming before
all state ones). With that
Reviewed-by: Jan Beulich <jbeulich@suse.com>


Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1.1 for-4.11 0/3] vpci bugfixes

[Jan Beulich]

>>> On 26.03.18 at 13:28, <roger.pau@citrix.com> wrote:
> This tree patches are bugfixes for the vPCI code merged last week. They
> where spotted by Coverity.

Thanks for dealing with them. You having omitted Coverity IDs I
suppose the report you've looked at was from the XenServer internal
instance. That would also explain why you have a fix for an issue the
open source instance didn't spot. It spotted another issue though:

*** CID 1430809:    (BAD_SHIFT)
/xen/drivers/vpci/vpci.c: 382 in vpci_read()
376                                              size - data_offset);
377     
378             data = merge_result(data, tmp_data, size - data_offset, data_offset);
379         }
380         spin_unlock(&pdev->vpci->lock);
381     
>>>     CID 1430809:    (BAD_SHIFT)
>>>     In expression "0xffffffffU >> 32U - 8U * size", right shifting by more than 31 bits has undefined behavior.  The shift amount, "32U - 8U * size", is 32.
382         return data & (0xffffffff >> (32 - 8 * size));
383     }

And indeed there's no way I can see that it could prove size to
only ever be 1, 2, or 4. I can't figure whether they've actually
found a code path where size could end up being zero here. I
think/hope a suitable ASSERT() would help.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1.1 for-4.11 0/3] vpci bugfixes

[Roger Pau Monné]

On Mon, Mar 26, 2018 at 06:39:21AM -0600, Jan Beulich wrote:
> >>> On 26.03.18 at 13:28, <roger.pau@citrix.com> wrote:
> > This tree patches are bugfixes for the vPCI code merged last week. They
> > where spotted by Coverity.
> 
> Thanks for dealing with them. You having omitted Coverity IDs I
> suppose the report you've looked at was from the XenServer internal
> instance. That would also explain why you have a fix for an issue the
> open source instance didn't spot. It spotted another issue though:
> 
> *** CID 1430809:    (BAD_SHIFT)
> /xen/drivers/vpci/vpci.c: 382 in vpci_read()
> 376                                              size - data_offset);
> 377     
> 378             data = merge_result(data, tmp_data, size - data_offset, data_offset);
> 379         }
> 380         spin_unlock(&pdev->vpci->lock);
> 381     
> >>>     CID 1430809:    (BAD_SHIFT)
> >>>     In expression "0xffffffffU >> 32U - 8U * size", right shifting by more than 31 bits has undefined behavior.  The shift amount, "32U - 8U * size", is 32.
> 382         return data & (0xffffffff >> (32 - 8 * size));
> 383     }
> 
> And indeed there's no way I can see that it could prove size to
> only ever be 1, 2, or 4. I can't figure whether they've actually
> found a code path where size could end up being zero here. I
> think/hope a suitable ASSERT() would help.

I've also seen that one, but was wondering whether this should be
fixed in the handler dispatcher code instead. But seeing that
vpci_read/write can be called from both the IO or the MMIO handlers, I
guess it's best to just add an ASSERT(size); to both the read and the
write handlers.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel