Hi Varun,
I agree things are less than ideal at the moment. We take advantage as an open source project to use the Synopsys online Coverity service and this points to the master branch (actually currently the mirror on github) which means these coverity tests are ran on changes after they have been submitted. Less than ideal I know but better than not being ran at all. Really we would like these to be ran as part of the OpenCI as part of patch reviews but we are not there yet. Not sure if the Synopsys online Coverity service can be setup to support this via Gerrit reviews or if this will have to be done after patch submittals. If it has to be done after then some clever CI scripting would need to be done to identify where the offending change came from.
Would welcome other ideas to make this experience better, but for now this is better than nothing.
Cheers
Joanna
On 22/07/2020, 18:09, "TF-A on behalf of Varun Wadekar via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote:
Hi,
Is there a way to create a ticket and assign Coverity flagged issues to the code owner automatically?
-Varun
-----Original Message-----
From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of scan-admin--- via TF-A
Sent: Wednesday, July 22, 2020 8:17 AM
To: tf-a(a)lists.trustedfirmware.org
Subject: [TF-A] New Defects reported by Coverity Scan for ARM-software/arm-trusted-firmware
External email: Use caution opening links or attachments
Hi,
Please find the latest report on new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan.
3 new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan.
New defect(s) Reported-by: Coverity Scan Showing 3 of 3 defect(s)
** CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
/drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer()
________________________________________________________________________________________________________
*** CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
/drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer()
267 size_t n;
268
269 ASSERT_SYM_PTR_ALIGN(out);
270
271 for (n = 0U; n < nb_elt; n++) {
272 out[2 * n] = (uint32_t)rates[n];
>>> CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
>>> "(uint64_t)rates[n] >> 32" is 0 regardless of the values of its operands. This occurs as the operand of assignment.
273 out[2 * n + 1] = (uint32_t)((uint64_t)rates[n] >> 32);
274 }
275 }
276
277 static void scmi_clock_describe_rates(struct scmi_msg *msg)
278 {
** CID 360934: Integer handling issues (BAD_SHIFT)
/drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer()
________________________________________________________________________________________________________
*** CID 360934: Integer handling issues (BAD_SHIFT)
/drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer()
267 size_t n;
268
269 ASSERT_SYM_PTR_ALIGN(out);
270
271 for (n = 0U; n < nb_elt; n++) {
272 out[2 * n] = (uint32_t)rates[n];
>>> CID 360934: Integer handling issues (BAD_SHIFT)
>>> In expression "(uint64_t)rates[n] >> 32", right shifting "rates[n]" by more than 31 bits always yields zero. The shift amount is 32.
273 out[2 * n + 1] = (uint32_t)((uint64_t)rates[n] >> 32);
274 }
275 }
276
277 static void scmi_clock_describe_rates(struct scmi_msg *msg)
278 {
** CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
/drivers/st/scmi-msg/clock.c: 191 in scmi_clock_rate_get()
________________________________________________________________________________________________________
*** CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
/drivers/st/scmi-msg/clock.c: 191 in scmi_clock_rate_get()
185 return;
186 }
187
188 rate = plat_scmi_clock_get_rate(msg->agent_id, clock_id);
189
190 return_values.rate[0] = (uint32_t)rate;
>>> CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
>>> "(uint64_t)rate >> 32" is 0 regardless of the values of its operands. This occurs as the operand of assignment.
191 return_values.rate[1] = (uint32_t)((uint64_t)rate >> 32);
192
193 scmi_write_response(msg, &return_values, sizeof(return_values));
194 }
195
196 static void scmi_clock_rate_set(struct scmi_msg *msg)
________________________________________________________________________________________________________
To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P…
--
TF-A mailing list
TF-A(a)lists.trustedfirmware.org
https://lists.trustedfirmware.org/mailman/listinfo/tf-a
--
TF-A mailing list
TF-A(a)lists.trustedfirmware.org
https://lists.trustedfirmware.org/mailman/listinfo/tf-a
Hi Wei Li,
Thanks for your change which looks correct.
Can you possibly submit it to review.trustedfirmware.org?
We can follow up with the review from the gerrit interface.
Thanks, Olivier.
________________________________________
From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> on behalf of Wei Li via TF-A <tf-a(a)lists.trustedfirmware.org>
Sent: 22 July 2020 14:25
To: tf-a(a)lists.trustedfirmware.org
Cc: huawei.libin(a)huawei.com; guohanjun(a)huawei.com
Subject: [TF-A] [PATCH] Add support for ARMv8.3-SPE
When ARMv8.3-SPE is implemented, ID_AA64DFR0_EL1.PMSVer is read as
0b0010, update the version check to support ARMv8.3-SPE or higher.
Signed-off-by: Wei Li <liwei391(a)huawei.com>
---
lib/extensions/spe/spe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/extensions/spe/spe.c b/lib/extensions/spe/spe.c
index 78876c66b..aa0bcd8ea 100644
--- a/lib/extensions/spe/spe.c
+++ b/lib/extensions/spe/spe.c
@@ -25,7 +25,7 @@ bool spe_supported(void)
uint64_t features;
features = read_id_aa64dfr0_el1() >> ID_AA64DFR0_PMS_SHIFT;
- return (features & ID_AA64DFR0_PMS_MASK) == 1U;
+ return (features & ID_AA64DFR0_PMS_MASK) != 0U;
}
void spe_enable(bool el2_unused)
--
2.17.1
--
TF-A mailing list
TF-A(a)lists.trustedfirmware.org
https://lists.trustedfirmware.org/mailman/listinfo/tf-a
Hi Raghu and Andrew,
Thanks for the inputs. I have added the mm_vm_dump() call to the mm_init() function but nothing was dumped to uart log. I guess I am missing something trivial. It looks like mm_root_table_count() returns 0. Any suggestions?
diff --git a/src/mm.c b/src/mm.c
index 2e352e9..7c56a09 100644
--- a/src/mm.c
+++ b/src/mm.c
@@ -1041,5 +1041,7 @@ bool mm_init(struct mpool *ppool)
mm_identity_map(stage1_locked, layout_data_begin(), layout_data_end(),
MM_MODE_R | MM_MODE_W, ppool);
+ mm_vm_dump(&ptable);
+
return arch_mm_init(ptable.root);
}
Thanks,
Madhukar
-----Original Message-----
From: Hafnium <hafnium-bounces(a)lists.trustedfirmware.org> On Behalf Of Andrew Walbran via Hafnium
Sent: Friday, July 17, 2020 6:10 AM
To: Raghu K <raghu.ncstate(a)icloud.com>
Cc: hafnium(a)lists.trustedfirmware.org
Subject: Re: [Hafnium] Debugging page table creation in Hafnium
Yep, mm_vm_dump sounds like what you're looking for. You can add a call where you like and it will go to the log UART.
On Thu, 16 Jul 2020 at 19:14, Raghu K via Hafnium < hafnium(a)lists.trustedfirmware.org> wrote:
> Quick search indicates mm_vm_dump() and the functions it calls in
> src/mm.c should do what you want. i've not tried it or don't know the
> format, but this may be what you are looking for.
>
> -Raghu
>
> On 7/16/20 11:03 AM, Madhukar Pappireddy via Hafnium wrote:
> > Hi,
> >
> > I was wondering if there is support in Hafnium to dump page tables
> > to a
> log file. I am new to the Hafnium project and would appreciate any help.
> Below is an example from TF-A which provides such provision.
> >
> > ......<snip>
> > VERBOSE: Translation tables state:
> > VERBOSE: Xlat regime: EL3
> > VERBOSE: Max allowed PA: 0xfffffffff
> > VERBOSE: Max allowed VA: 0xfffffffff
> > VERBOSE: Max mapped PA: 0x2f1fffff
> > VERBOSE: Max mapped VA: 0x2f1fffff
> > VERBOSE: Initial lookup level: 1
> > VERBOSE: Entries @initial lookup level: 64
> > VERBOSE: Used 3 sub-tables out of 5 (spare: 2)
> > [LV1] VA:0x0 size:0x40000000
> > [LV2] VA:0x0 size:0x200000
> > [LV3] VA:0x0 PA:0x0 size:0x1000 MEM-RO-EXEC-S
> > [LV3] VA:0x1000 PA:0x1000 size:0x1000 MEM-RO-EXEC-S
> > [LV3] VA:0x2000 PA:0x2000 size:0x1000 MEM-RO-EXEC-S
> > [LV3] VA:0x3000 PA:0x3000 size:0x1000 MEM-RO-EXEC-S
> > [LV3] VA:0x4000 PA:0x4000 size:0x1000 MEM-RO-EXEC-S
> > [LV3] VA:0x5000 PA:0x5000 size:0x1000 MEM-RO-EXEC-S
> > [LV3] VA:0x6000 PA:0x6000 size:0x1000 MEM-RO-EXEC-S
> > [LV3] VA:0x7000 PA:0x7000 size:0x1000 MEM-RO-EXEC-S
> > [LV3] VA:0x8000 PA:0x8000 size:0x1000 MEM-RO-XN-S
> > [LV3] VA:0x9000 PA:0x9000 size:0x1000 MEM-RO-XN-S
> > [LV3] VA:0xa000 PA:0xa000 size:0x1000 MEM-RO-XN-S
> > [LV3] VA:0xb000 size:0x1000
> > [LV3] (500 invalid descriptors omitted)
> > [LV2] VA:0x200000 size:0x200000
> > [LV2] (30 invalid descriptors omitted)
> > [LV2] VA:0x4000000 size:0x200000 ..... <snip>
> >
> > Thanks,
> > Madhukar
> >
>
> --
> Hafnium mailing list
> Hafnium(a)lists.trustedfirmware.org
> https://lists.trustedfirmware.org/mailman/listinfo/hafnium
>
Hi Varun,
AIU, Usually 2 keys are used to protect 2 different software contexts for enforcing the security boundary between them. This is the case for kernel and userspace. In TF-A, every BL image is allowed to choose the IA key to use, hence every BL image can have a separate IA key. Each BL image is executing within a single security domain and hence there is less use create a boundary within the BL image by using the 2nd key registers.
The Compiler by default selects either IA or IB instruction key register to use by default. It cannot automatically make use of both key registers and it doesn't support the use of DA or DB yet I think.
There are some function attributes which allow to use a different key at a function level, but the functions need to be marked out manually. But the requirement for creating the sub-domain within a BL image by specifying a different key for certain functions has not yet been seen AFAIK.
Best Regards
Soby Mathew
From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of Varun Wadekar via TF-A
Sent: 13 July 2020 22:20
To: tf-a(a)lists.trustedfirmware.org
Cc: Kalyani Chidambaram Vaidyanathan <kalyanic(a)nvidia.com>
Subject: [TF-A] Pointer Authentication keys
Hi,
>From the initial read of the Arm ARM, there are multiple keys (instruction, data) provided for authenticating pointers. But the current implementation only writes IA key [1].
I would like to understand the thought process behind programming only one key here. AFAIU, we should enable all keys - IA, IB, DA, DB.
-Varun
[1] https://review.trustedfirmware.org/plugins/gitiles/TF-A/trusted-firmware-a/…
Hi Julius,
Sandrine is away on vacation so I thought I would follow up as we had a chat in one of our team meetings about the point below.
> On 07/07/2020, 23:05, "TF-A on behalf of Julius Werner via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote:
>> IOW, you're fine with both options but you'd prefer to have different
>> people setting MR+1 and COR+1 (or CR+1 in the special cases mentioned
>> above), right? Just want to double-check I understood your position.
>Right.
So when we had +1/+2 CR before these changes we endeavoured to ensure that these were done by different people and if a maintainer gave a +1 they resisted also giving a +2 as we recognise having multiple eyes is always advantageous. So the general feeling was that this should be the recommended best practice. However, the feeling was also that this specific recommendation should not be strictly enforced by gerrit checking as on occasions it is possible to give both on simple patches and maintainers and code owners in this case can be trusted to make the correct choice.
The enforcing of them in Gerrit should be viewed as an aid to try and avoid mistakes rather than strict control for as you say we don't expect people to intentionally break the rules. Perhaps a warning in this case can be made to remind folks what's best practice here.
For now though as mentioned as we practice the new rules and recommendations we will use an honour system rather than strict enforcement along with updating the documented rules and recomendations.
Thanks
Joanna
Hi Raghu,
The Exception Handling framework (EHF) was designed to provide prioritization between the EL3 interrupts and EA although EA will always have highest priority since it cannot be blocked. In the case you describe, the driver in EL3 which is handling the RAS errors would need to ensure serialization of the events delivered to the S-EL0 payload somehow. This could be either via holding the event in a queue in EL3 till EL0 is done with processing the first event or the EL0 payload is capable of re-entry and can manage a queue internally. In case of MM, I suppose re-entry is not an option and hence a holding queue in EL3 driver needs to be implemented.
The current implementation in sgi_ras.c doesn't do this currently as this was a PoC to showcase the RAS flow.
Best Regards
Soby Mathew
> -----Original Message-----
> From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of Raghu K
> via TF-A
> Sent: 13 July 2020 22:28
> To: tf-a(a)lists.trustedfirmware.org
> Subject: [TF-A] Deadlock in SGI RAS handling
>
> Hi All,
>
> I was going through some code in sgi_ras.c and was wondering if the
> situation mentioned below could cause a deadlock or if i'm missing
> something. It seems like it is possible to deadlock if we enter MM in S-
> EL0(say through an MM_COMMUNICATE SMC or perhaps an initial RAS
> interrupt) followed by a SYNC EA or ASYNC EA on the same core. sgi_ras.c
> seems like it registers the same handler for both interrupts and aborts.
> While interrupts can be blocked/masked, SYNC EA's cannot be blocked(not
> that i know of), and i don't see SErrors being blocked on the path to the EA
> handler and entry to MM. If this situation does occur, it seems like we could
> deadlock when the EA attempts to enter MM again in the interrupt handler.
> Is there something that would prevent this situation from happening?
>
>
> Thanks
> Raghu
> --
> TF-A mailing list
> TF-A(a)lists.trustedfirmware.org
> https://lists.trustedfirmware.org/mailman/listinfo/tf-a
Hi All,
I was going through some code in sgi_ras.c and was wondering if the
situation mentioned below could cause a deadlock or if i'm missing
something. It seems like it is possible to deadlock if we enter MM in
S-EL0(say through an MM_COMMUNICATE SMC or perhaps an initial RAS
interrupt) followed by a SYNC EA or ASYNC EA on the same core. sgi_ras.c
seems like it registers the same handler for both interrupts and aborts.
While interrupts can be blocked/masked, SYNC EA's cannot be blocked(not
that i know of), and i don't see SErrors being blocked on the path to
the EA handler and entry to MM. If this situation does occur, it seems
like we could deadlock when the EA attempts to enter MM again in the
interrupt handler.
Is there something that would prevent this situation from happening?
Thanks
Raghu