* [Xen-devel] [PATCH 0/3] Minor Coverity fixes
@ 2019-10-09 14:20 Artem Mygaiev
2019-10-09 14:20 ` [Xen-devel] [PATCH 1/3] Consistent use for lock variable Artem Mygaiev
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Artem Mygaiev @ 2019-10-09 14:20 UTC (permalink / raw)
To: xen-devel
Cc: Artem Mygaiev, Daniel De Graaf, Stefano Stabellini,
Volodymyr Babchuk, Julien Grall
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Xen-devel] [PATCH 1/3] Consistent use for lock variable
2019-10-09 14:20 [Xen-devel] [PATCH 0/3] Minor Coverity fixes Artem Mygaiev
@ 2019-10-09 14:20 ` Artem Mygaiev
2020-01-18 13:09 ` Julien Grall
2019-10-09 14:20 ` [Xen-devel] [PATCH 2/3] Remove useless ASSERT condition Artem Mygaiev
2019-10-09 14:20 ` [Xen-devel] [PATCH 3/3] Free allocated resource on error Artem Mygaiev
2 siblings, 1 reply; 8+ messages in thread
From: Artem Mygaiev @ 2019-10-09 14:20 UTC (permalink / raw)
To: xen-devel; +Cc: Artem Mygaiev, Daniel De Graaf, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Xen-devel] [PATCH 2/3] Remove useless ASSERT condition
2019-10-09 14:20 [Xen-devel] [PATCH 0/3] Minor Coverity fixes Artem Mygaiev
2019-10-09 14:20 ` [Xen-devel] [PATCH 1/3] Consistent use for lock variable Artem Mygaiev
@ 2019-10-09 14:20 ` Artem Mygaiev
2019-10-09 14:56 ` Julien Grall
2019-10-09 14:20 ` [Xen-devel] [PATCH 3/3] Free allocated resource on error Artem Mygaiev
2 siblings, 1 reply; 8+ messages in thread
From: Artem Mygaiev @ 2019-10-09 14:20 UTC (permalink / raw)
To: xen-devel
Cc: Artem Mygaiev, Julien Grall, Stefano Stabellini,
Volodymyr Babchuk, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Xen-devel] [PATCH 3/3] Free allocated resource on error
2019-10-09 14:20 [Xen-devel] [PATCH 0/3] Minor Coverity fixes Artem Mygaiev
2019-10-09 14:20 ` [Xen-devel] [PATCH 1/3] Consistent use for lock variable Artem Mygaiev
2019-10-09 14:20 ` [Xen-devel] [PATCH 2/3] Remove useless ASSERT condition Artem Mygaiev
@ 2019-10-09 14:20 ` Artem Mygaiev
2019-10-09 15:24 ` Julien Grall
2 siblings, 1 reply; 8+ messages in thread
From: Artem Mygaiev @ 2019-10-09 14:20 UTC (permalink / raw)
To: xen-devel
Cc: Artem Mygaiev, Julien Grall, Stefano Stabellini,
Volodymyr Babchuk, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH 2/3] Remove useless ASSERT condition
2019-10-09 14:20 ` [Xen-devel] [PATCH 2/3] Remove useless ASSERT condition Artem Mygaiev
@ 2019-10-09 14:56 ` Julien Grall
2020-01-18 12:58 ` Julien Grall
0 siblings, 1 reply; 8+ messages in thread
From: Julien Grall @ 2019-10-09 14:56 UTC (permalink / raw)
To: Artem Mygaiev, xen-devel
Cc: Artem Mygaiev, Stefano Stabellini, Volodymyr Babchuk
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] Free allocated resource on error
2019-10-09 14:20 ` [Xen-devel] [PATCH 3/3] Free allocated resource on error Artem Mygaiev
@ 2019-10-09 15:24 ` Julien Grall
0 siblings, 0 replies; 8+ messages in thread
From: Julien Grall @ 2019-10-09 15:24 UTC (permalink / raw)
To: Artem Mygaiev, xen-devel
Cc: Artem Mygaiev, Stefano Stabellini, Volodymyr Babchuk
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH 2/3] Remove useless ASSERT condition
2019-10-09 14:56 ` Julien Grall
@ 2020-01-18 12:58 ` Julien Grall
0 siblings, 0 replies; 8+ messages in thread
From: Julien Grall @ 2020-01-18 12:58 UTC (permalink / raw)
To: Julien Grall, Artem Mygaiev, xen-devel
Cc: Artem Mygaiev, Stefano Stabellini, Volodymyr Babchuk
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] Consistent use for lock variable
2019-10-09 14:20 ` [Xen-devel] [PATCH 1/3] Consistent use for lock variable Artem Mygaiev
@ 2020-01-18 13:09 ` Julien Grall
0 siblings, 0 replies; 8+ messages in thread
From: Julien Grall @ 2020-01-18 13:09 UTC (permalink / raw)
To: Artem Mygaiev, xen-devel; +Cc: Artem Mygaiev, Daniel De Graaf
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-01-18 13:09 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09 14:20 [Xen-devel] [PATCH 0/3] Minor Coverity fixes Artem Mygaiev
2019-10-09 14:20 ` [Xen-devel] [PATCH 1/3] Consistent use for lock variable Artem Mygaiev
2020-01-18 13:09 ` Julien Grall
2019-10-09 14:20 ` [Xen-devel] [PATCH 2/3] Remove useless ASSERT condition Artem Mygaiev
2019-10-09 14:56 ` Julien Grall
2020-01-18 12:58 ` Julien Grall
2019-10-09 14:20 ` [Xen-devel] [PATCH 3/3] Free allocated resource on error Artem Mygaiev
2019-10-09 15:24 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.