[Xen-devel] [PATCH 0/3] Minor Coverity fixes

[Xen-devel] [PATCH 0/3] Minor Coverity fixes

[Artem Mygaiev]

From: Artem Mygaiev <artem_mygaiev@epam.com>

There is a big amount of insignificant or false positives reported by 
Coverity, fixing some of them can at least make code more consistent or
at least bit more readable.

Artem Mygaiev (3):
  Consistent use for lock variables
  Remove useless ASSERT condition
  Free allocated resource on error

 xen/drivers/char/scif-uart.c       | 2 +-
 xen/drivers/passthrough/arm/smmu.c | 8 +++++---
 xen/xsm/flask/avc.c                | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

-- 
2.20.1


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

[Xen-devel] [PATCH 1/3] Consistent use for lock variable

[Artem Mygaiev]

... for both lock and unlock

Coverity-ID: 1381840
Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com>
---
 xen/xsm/flask/avc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/xsm/flask/avc.c b/xen/xsm/flask/avc.c
index 87ea38b7a0..3a9507f62a 100644
--- a/xen/xsm/flask/avc.c
+++ b/xen/xsm/flask/avc.c
@@ -320,7 +320,7 @@ static inline int avc_reclaim_node(void)
         head = &avc_cache.slots[hvalue];
         lock = &avc_cache.slots_lock[hvalue];
 
-        spin_lock_irqsave(&avc_cache.slots_lock[hvalue], flags);
+        spin_lock_irqsave(lock, flags);
         rcu_read_lock(&avc_rcu_lock);
         hlist_for_each_entry(node, next, head, list)
         {
-- 
2.20.1


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

[Xen-devel] [PATCH 2/3] Remove useless ASSERT condition

[Artem Mygaiev]

cnt is unsigned, so always >=0

Coverity-ID: 1381848
Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com>
---
 xen/drivers/char/scif-uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index fa0b8274ca..9d3f66b55b 100644
--- a/xen/drivers/char/scif-uart.c
+++ b/xen/drivers/char/scif-uart.c
@@ -205,7 +205,7 @@ static int scif_uart_tx_ready(struct serial_port *port)
 
      /* Check number of data bytes stored in TX FIFO */
     cnt = scif_readw(uart, SCIF_SCFDR) >> 8;
-    ASSERT( cnt >= 0 && cnt <= params->fifo_size );
+    ASSERT( cnt <= params->fifo_size );
 
     return (params->fifo_size - cnt);
 }
-- 
2.20.1


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

[Xen-devel] [PATCH 3/3] Free allocated resource on error

[Artem Mygaiev]

Also do not set mark device as SMMU protected when there are no more
SMMU resources left

Coverity-ID: 1381862
Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com>
---
 xen/drivers/passthrough/arm/smmu.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index f151b9f5b5..cf42335eed 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -804,9 +804,6 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 	master->of_node			= masterspec->np;
 	master->cfg.num_streamids	= masterspec->args_count;
 
-	/* Xen: Let Xen know that the device is protected by an SMMU */
-	dt_device_set_protected(masterspec->np);
-
 	for (i = 0; i < master->cfg.num_streamids; ++i) {
 		u16 streamid = masterspec->args[i];
 
@@ -815,10 +812,15 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 			dev_err(dev,
 				"stream ID for master device %s greater than maximum allowed (%d)\n",
 				masterspec->np->name, smmu->num_mapping_groups);
+			devm_free(master);
 			return -ERANGE;
 		}
 		master->cfg.streamids[i] = streamid;
 	}
+
+        /* Xen: Let Xen know that the device is protected by an SMMU */
+        dt_device_set_protected(masterspec->np);
+
 	return insert_smmu_master(smmu, master);
 }
 
-- 
2.20.1


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

Re: [Xen-devel] [PATCH 2/3] Remove useless ASSERT condition

[Julien Grall]

Hi Artem,

On 09/10/2019 15:20, Artem Mygaiev wrote:
> cnt is unsigned, so always >=0
> 
> Coverity-ID: 1381848
> Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com>

Acked-by: Julien Grall <julien.grall@arm.com>

> ---
>   xen/drivers/char/scif-uart.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
> index fa0b8274ca..9d3f66b55b 100644
> --- a/xen/drivers/char/scif-uart.c
> +++ b/xen/drivers/char/scif-uart.c
> @@ -205,7 +205,7 @@ static int scif_uart_tx_ready(struct serial_port *port)
>   
>        /* Check number of data bytes stored in TX FIFO */
>       cnt = scif_readw(uart, SCIF_SCFDR) >> 8;
> -    ASSERT( cnt >= 0 && cnt <= params->fifo_size );
> +    ASSERT( cnt <= params->fifo_size );
>   
>       return (params->fifo_size - cnt);
>   }
> 

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 3/3] Free allocated resource on error

[Julien Grall]

Hi Artem,

On 09/10/2019 15:20, Artem Mygaiev wrote:
> Also do not set mark device as SMMU protected when there are no more
> SMMU resources left

This is a sanity check on the content of the node, not a lack of resource in
this case.

TBH, the current placement is probably not correct. But I am not convinced the 
new placement is better.

Xen 4.12 and earlier will ignore any failure and continue, so we cannot use this 
device at all. Indeed, Xen will configure the SMMU to deny any transaction. If 
you fail to initialize the device, then it will be in an unusable state. In this 
case, we don't want a domain (including Dom0) to use it at all. This could be 
achieved by calling dt_device_set_protected.

Xen 4.13 will stop booting if any of the SMMU fails to configure (this include 
Master Device cannot be assigned). So there are no difference there.

My preference is to cater for all Xen versions so we can consider the patch for 
a backport and potentially revert of the Xen 4.13 behavior (i.e. crashing when 
one IOMMU is not correctly setup). So we would need to move the call at the 
beginning of the function.

> 
> Coverity-ID: 1381862
> Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com>
> ---
>   xen/drivers/passthrough/arm/smmu.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index f151b9f5b5..cf42335eed 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -804,9 +804,6 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
>   	master->of_node			= masterspec->np;
>   	master->cfg.num_streamids	= masterspec->args_count;
>   
> -	/* Xen: Let Xen know that the device is protected by an SMMU */
> -	dt_device_set_protected(masterspec->np);
> -
>   	for (i = 0; i < master->cfg.num_streamids; ++i) {
>   		u16 streamid = masterspec->args[i];
>   
> @@ -815,10 +812,15 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
>   			dev_err(dev,
>   				"stream ID for master device %s greater than maximum allowed (%d)\n",
>   				masterspec->np->name, smmu->num_mapping_groups);
> +			devm_free(master);
>   			return -ERANGE;
>   		}
>   		master->cfg.streamids[i] = streamid;
>   	}
> +
> +        /* Xen: Let Xen know that the device is protected by an SMMU */
> +        dt_device_set_protected(masterspec->np);

This code is using Linux coding style, so it should be hard tab here.

> +
>   	return insert_smmu_master(smmu, master);
>   }
>   
> 

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 2/3] Remove useless ASSERT condition

[Julien Grall]

On 09/10/2019 15:56, Julien Grall wrote:
> Hi Artem,

Hi Artem,

> On 09/10/2019 15:20, Artem Mygaiev wrote:
>> cnt is unsigned, so always >=0
>>
>> Coverity-ID: 1381848
>> Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com>
> 
> Acked-by: Julien Grall <julien.grall@arm.com>

I was going through my todo list and noticed I have never committed this 
patch. Apologies for that.

I have committed it with a slight change in the commit title:

"xen/char: scif-uart: Remove useless ASSERT condition"

In the future, please try to add the component in the title :).

Cheers,

> 
>> ---
>>   xen/drivers/char/scif-uart.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
>> index fa0b8274ca..9d3f66b55b 100644
>> --- a/xen/drivers/char/scif-uart.c
>> +++ b/xen/drivers/char/scif-uart.c
>> @@ -205,7 +205,7 @@ static int scif_uart_tx_ready(struct serial_port 
>> *port)
>>        /* Check number of data bytes stored in TX FIFO */
>>       cnt = scif_readw(uart, SCIF_SCFDR) >> 8;
>> -    ASSERT( cnt >= 0 && cnt <= params->fifo_size );
>> +    ASSERT( cnt <= params->fifo_size );
>>       return (params->fifo_size - cnt);
>>   }
>>
> 
> Cheers,
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 1/3] Consistent use for lock variable

[Julien Grall]

Hi Artem,

Apologies for the late answer.

On 09/10/2019 15:20, Artem Mygaiev wrote:
> ... for both lock and unlock

I would suggest the following commit message:

xen/xsm: Use the same lock for lock and unlock

The function avc_reclaim_mode() is not using the same variable for 
locking and unlocking. While the underlying spinlock is the same, 
coverity will get confused and think the lock was not released.

Update the code to use the same variable for the lock and unlock part.

> 
> Coverity-ID: 1381840
> Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com>

Acked-by: Julien Grall <julien@xen.org>

We also need an hack from Daniel. @Daniel, are you happy with the change?

> ---
>   xen/xsm/flask/avc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/xsm/flask/avc.c b/xen/xsm/flask/avc.c
> index 87ea38b7a0..3a9507f62a 100644
> --- a/xen/xsm/flask/avc.c
> +++ b/xen/xsm/flask/avc.c
> @@ -320,7 +320,7 @@ static inline int avc_reclaim_node(void)
>           head = &avc_cache.slots[hvalue];
>           lock = &avc_cache.slots_lock[hvalue];
>   
> -        spin_lock_irqsave(&avc_cache.slots_lock[hvalue], flags);
> +        spin_lock_irqsave(lock, flags);
>           rcu_read_lock(&avc_rcu_lock);
>           hlist_for_each_entry(node, next, head, list)
>           {
> 

Cheers,

-- 
Julien Grall

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