Add ARM_MUSCA_A1 as a new target platform #8845

gaborkertesz · 2018-11-22T19:43:30Z

Description

This commit enables Musca-A1 for mbed. This platform is a Cortex-M33
based target with security extension enabled.
This makes possible to have TF-M on the secure domain, while mbed on
the non-secure domain.

TF-M with its secure bootloader (McuBoot) are submitted by pre-built
binaries.
TF-M binary is concatenated with the non-secure mbed binary, then
signed for McuBoot. Then the signed binary is concatenated to McuBoot.
These post-build steps are done in post binary hook.

TF-M resources are imported in original form, without any change.

TF-M resources:

TFM/TFM_API
tools/targets/ARM_MUSCA_A1/imgtool_lib
tools/targets/ARM_MUSCA_A1/imgtool.py
tools/targets/ARM_MUSCA_A1/assemble.py

TF-M origin:

https://git.trustedfirmware.org/trusted-firmware-m.git/TF-M 8e0fa7a926bf170f0287e0dabcf3f411bdf88ce1

McuBoot and TF-M boots first and it's printing to UART by
115200 BaudRate, which is not configurable, since these are
pre-built binaries.
After secure initilazation, the non-secure mbed starts to boot.

All GreenTea tests are passed by ARMC6 compiler:
mbedgt: test case results: 513 OK
mbedgt: completed in 12944.13 sec

All GreenTea tests are passed by GCC_ARM compiler:
mbedgt: test case results: 513 OK
mbedgt: completed in 12943.59 sec

The following APIs are implemented:
Low-Power Ticker, MicroSec Ticker, Serial, Sleep, Interruptin

Due to a HW limitation, GPIO in Musca-A1 is Secure only,
so secure service should be used for GPIO in NS domain.
At this stage TF-M doesn't include this service, so GPIO is
added by dummy implementation in order to pass tests.

mbed-ls pull request adding this new platform:
ARMmbed/mbed-ls#404

Pull request type

[ ] Fix
[ ] Refactor
[X] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

This commit enables Musca-A1 for mbed. This platform is a Cortex-M33 based target with security extension enabled. This makes possible to have TF-M on the secure domain, while mbed on the non-secure domain. TF-M with its secure bootloader (McuBoot) are submitted by pre-built binaries. TF-M binary is concatenated with the non-secure mbed binary, then signed for McuBoot. Then the signed binary is concatenated to McuBoot. These post-build steps are done in post binary hook. TF-M resources are imported in original form, without any change. TF-M resources: TFM/TFM_API tools/targets/ARM_MUSCA_A1/imgtool_lib tools/targets/ARM_MUSCA_A1/imgtool.py tools/targets/ARM_MUSCA_A1/assemble.py TF-M origin: https://git.trustedfirmware.org/trusted-firmware-m.git/ TF-M 8e0fa7a926bf170f0287e0dabcf3f411bdf88ce1 Change-Id: I5f1e1b5a6c82f232992eda747fd472ac85d95fbb Signed-off-by: Gabor Kertesz <[email protected]>

ashok-rao · 2018-11-23T10:33:04Z

@0xc0170 @cmonr : This is the 1st PR for Musca-A1 platform. Can you please review / tag relevant reviewers to take a look at this? Thanks.

0xc0170 · 2018-11-23T10:48:29Z

targets/TARGET_ARM_SSG/TARGET_MUSCA_A1/PeripheralNames.h

@@ -0,0 +1,46 @@
+/* mbed Microcontroller Library
+ * Copyright (c) 2006-2017 ARM Limited


can you fix the year (2018) in all new files. Worth also adding SPDX identifier: SPDX-License-Identifier: Apache-2.0

All files that are created this year and haven't been released to anywhere yet, will be updated accordingly.

Fix commit has been submitted.

targets/targets.json

0xc0170 · 2018-11-23T10:51:47Z

targets/TARGET_ARM_SSG/TARGET_MUSCA_A1/serial_api.c

+#ifndef uart0_tx_irq_handler
+#error "uart0_tx_irq_handler should be defined, check device_cfg.h!"
+#endif
+void uart0_tx_irq_handler(void)


are these using static vector placement rather than dynamic (NVIC Set/Get vector functions)? If yes, why?

Yes, it's static vector placement. This is a mechanism to make sure the weak function defined in the startup assembly is overriden. So if the expected IRQ handler is not defined, it will be caught by compile time with a proper error message.

Correct. I was more after why static one is chosen (we use dynamic one) ?

However there is no explicit argument against dynamic vector allocation, this pattern prefers to define platform configuration as early as possible. Since we know the expected behavior in build time, we preferred to set in that step.

It's a commonly used pattern now in our platforms, already upstreamed in CM3DS:
https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/emac-drivers/TARGET_ARM_SSG/COMPONENT_SMSC9220/smsc9220_emac.cpp#L35
https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_ARM_SSG/TARGET_CM3DS_MPS2/us_ticker.c#L111

0xc0170 · 2018-11-23T10:52:59Z

@ashok-rao 5.11 target? If yes, we should add a version label

ashok-rao

Thanks @gaborkertesz . Looking great. I've requested some changes and some questions below:

__init.py -> there are no contents, was this file added in error to the commit? Can you please check?
PeripheralPins.c -> seems to be missing or not required here? Can you please confirm?
Can you please attach greentea test logs for ARMC6 and GCC to this PR?

ashok-rao · 2018-11-23T10:55:14Z

targets/TARGET_ARM_SSG/TARGET_MUSCA_A1/PinNames.h

+    PA23 = 23,
+    PA24 = 24,
+    PA25 = 25,
+


@gaborkertesz : Can you add the pins brought out on the Arduino headers as well?

__init.py: This is added by purpose in order to import the musca post build Python script as a module. If it's not really needed and I'm wrong, I can remove this of course.

PeripheralPins.c: I think the required feature is implemented
in pinmap.c.

Can't attach the txt files here, it says: "Something went really wrong, and we can't process that file." Any other suggestion?

There are 2 init.py scripts -> tools/targets/ARM_MUSCA_A1/init.py (empty) and tools/targets/init.py in the PR ..

Will check.

ZIP it up and you can upload the ZIP.

Yes and we're talking about tools/targets/ARM_MUSCA_A1/init.py now. That's the one I created in order to import tools.targets.ARM_MUSCA_A1.ARM_MUSCA_A1_TFM in the existing tools/targets/init.py

ZIP also doesn't work.
ARMC6: http://p.arm.com/?130
GCC: http://p.arm.com/?--g

ashok-rao · 2018-11-23T10:56:15Z

targets/TARGET_ARM_SSG/TARGET_MUSCA_A1/PinNames.h

@@ -0,0 +1,94 @@
+/* mbed Microcontroller Library
+ * Copyright (c) 2006-2017 ARM Limited


Can you please change the license header and add the SPDX identifier?

All files that are created this year and haven't been released to anywhere yet, will be updated accordingly.

Fix commit has been submitted.

targets/targets.json

ashok-rao · 2018-11-23T11:07:33Z

@ashok-rao 5.11 target? If yes, we should add a version label

@0xc0170 : Not expecting this to get in for 5.11.0 .. We are expecting this to get in for 5.11.1 atleast.

Since only mbed 5 is supported by Musca-A1, remove legacy 2. Change-Id: I042e3f019b3bff205e554ffd7a8ab638f286328f

This commit adds digital Arduino connector names. Change-Id: I8ea8bfa5f0c83fab42777e38db5f6baf312d9a81

Update every non-module imported file's licence header. Update to 2018 and SPDX Licence Id. Fixes some minor documentation issues. Change-Id: I2189282ead2bac7b455733394a11c632a37babbe

0xc0170

targets/TARGET_ARM_SSG/TARGET_MUSCA_A1/TFM - this folder contains some binaries. Can you add Readme.md to describe what is there

plus also licenses for these (add LICENSE file)

Good example can be found targets/TARGET_RDA/TARGET_UNO_91H/lib

bulislaw · 2018-11-26T14:43:46Z

All the PRs need to be engineering ready (marked as "needs: CI") by the end of the day (Austin time). Otherwise it won't make 5.11 and will need to come in the next release (5.12 for features, 5.11.1 for fixes and new platforms).

0xc0170 · 2018-11-26T15:12:24Z

Moved to 5.11.1 release

bridadan

Tools implementation looks ok besides the questions/issues I raised below. Not 100% sure about all of the details, especially since this is one of the first (the first?) platform implementing TF-M. Also, the lack of IAR support seems to be a missing piece (unless I missed a change in requirements).

It'd be worth having @theotherjimmy take a look as well (binary_hook added, new python package requirement, general arrangement, etc).

bridadan · 2018-11-26T15:01:54Z

targets/targets.json

+        "device_has": ["INTERRUPTIN", "LPTICKER", "SERIAL", "SLEEP", "USTICKER"],
+        "macros": ["MBED_TZ_DEFAULT_ACCESS=1", "__STARTUP_CLEAR_BSS", "MBED_FAULT_HANDLER_DISABLED", "CMSIS_NVIC_VIRTUAL", "LPTICKER_DELAY_TICKS=1"],
+        "extra_labels": ["ARM_SSG", "MUSCA_A1"],
+        "supported_toolchains": ["ARMC6", "GCC_ARM"],


@0xc0170 Isn't IAR required for a release_version of "5" as well?

IAR Is required for release_version 5 for all ARMv6M and ARMv7M devices. Only ARMC6 is must for ARMv8M.

Ok good to know. May need to tweak this a bit in some tooling checks: https://github.com/ARMmbed/mbed-os/blob/master/tools/build_api.py#L152

I believe it was done : https://github.com/ARMmbed/mbed-os/blob/master/tools/build_api.py#L188

bridadan · 2018-11-26T15:06:34Z

tools/targets/ARM_MUSCA_A1/ARM_MUSCA_A1_TFM.py

+try:
+ import Crypto
+except ImportError, e:
+ print('Please install "pycryptodome" Python module! ("pip install --user pycryptodome")')


Looks like you have added a new Python dependency here but you haven't used the Crypto module in this file. Can you please remove the import?

bridadan · 2018-11-26T15:17:18Z

tools/targets/ARM_MUSCA_A1/ARM_MUSCA_A1_TFM.py

+    temporary_files = [concatenated_binary_file, signed_binary_file]
+
+    #1. Run assemble.py to concatenate secure TFM and non-secure mbed binaries
+    sys.argv = ['assemble.py',


This seems to be a pretty non-standard way of passing arguments to a function. Instead of setting sys.argv, would it be possible to provide these parameters to the relevant function in the assemble module?

To add to this, sys.argv is destructively modified; the old value is lost. At the very least, don't do that.

bridadan · 2018-11-26T15:17:30Z

tools/targets/ARM_MUSCA_A1/ARM_MUSCA_A1_TFM.py

+    assemble.main()
+
+    #2. Run imgtool.py to sign the concatenated binary
+    sys.argv = ['imgtool.py',


This seems to be a pretty non-standard way of passing arguments to a function. Instead of setting sys.argv, would it be possible to provide these parameters to the relevant function in the imgotol module?

bridadan · 2018-11-26T15:20:22Z

tools/targets/ARM_MUSCA_A1/imgtool_lib/image.py

+    for write_size in [1, 2, 4, 8]
+}
+
+boot_magic = bytearray([


Is there any documentation for boot_magic?

bridadan · 2018-11-26T15:25:45Z

tools/targets/ARM_MUSCA_A1/imgtool_lib/version.py

+    print(decode_version("1.2"))
+    print(decode_version("1.0"))
+    print(decode_version("0.0.2+75"))
+    print(decode_version("0.0.0+00"))


This seems to be some leftover test code? Probably best to remove this.

bridadan · 2018-11-26T15:32:46Z

tools/targets/ARM_MUSCA_A1/imgtool_lib/keys.py

+from __future__ import print_function
+from Crypto.Hash import SHA256
+from Crypto.PublicKey import RSA
+from Crypto.Signature import PKCS1_v1_5, PKCS1_PSS


I see you're using the Crypto package here. Can you please add the pycryptodome package to the requirements.txt? https://github.com/ARMmbed/mbed-os/blob/master/requirements.txt

bridadan · 2018-11-26T15:34:27Z

tools/targets/ARM_MUSCA_A1/root-rsa-2048.pem

+y+rnmQKBgCh0gfyBT/OKPkfKv+Bbt8HcoxgEj+TyH+vYdeTbP9ZSJ6y5utMbPg7z
+iLLbJ+9IcSwPCwJSmI+I0Om4xEp4ZblCrzAG7hWvG2NNzxQjmoOOrAANyTvJR/ap
+N+UkQA4WrMSKEYyBlRS/hR9Unz31vMc2k9Re0ukWhWh/QksQGDfJ
+-----END RSA PRIVATE KEY-----


Disclaimer: not a security expert 😄 But typically you don't expose private keys, is this something like an "insecure default" key?

theotherjimmy · 2018-11-26T15:37:40Z

tools/targets/ARM_MUSCA_A1/ARM_MUSCA_A1_TFM.py

+        fh.write(mcuboot_bin_file)
+        fh.seek(mcuboot_image_size)
+        fh.write(tfm_mbed_signed_bin_file)
+        fh.close


Not required, and missing ()

theotherjimmy · 2018-11-27T15:05:56Z

@bridadan IAR support requires EWARM8 for v8m support, I think.

theotherjimmy · 2018-11-28T16:05:39Z

tools/targets/ARM_MUSCA_A1/ARM_MUSCA_A1_TFM.py

+    #1. Run assemble.py to concatenate secure TFM and non-secure mbed binaries
+    sys.argv = ['assemble.py',
+                '-l', flash_layout_file,
+                '-s', tfm_binary_file,


This argument, and the prior argument, flash_layout_file, should be configurable by the user.

theotherjimmy · 2018-11-28T16:08:14Z

tools/targets/ARM_MUSCA_A1/ARM_MUSCA_A1_TFM.py

+    sys.argv = ['imgtool.py',
+                'sign', '--layout', flash_layout_file,
+                '-k', join(run_dir, 'root-rsa-2048.pem'),
+                '--align', '1', '-v', '1.0', '-H', '0x400', '--pad', '0x100000',


These "magic values" (--align argument, -v argument, -H argument, --pad argument, flash_layout_file, and -k argument) should all be configurable by the user, if they will ever change. The fact that they're arguments to the TF-M script implies to me that they might change.

theotherjimmy · 2018-11-28T16:09:47Z

tools/targets/ARM_MUSCA_A1/ARM_MUSCA_A1_TFM.py

+ #
+
+try:
+ import Crypto


This indentation looks incorrect. Could you make this 4 spaces to match the rest of the file?

theotherjimmy · 2018-11-28T16:10:29Z

tools/targets/ARM_MUSCA_A1/ARM_MUSCA_A1_TFM.py

+    dirname
+)
+run_dir = dirname(os.path.realpath(__file__))
+sys.path.append(run_dir)


This should not be necessary.

theotherjimmy · 2018-11-28T16:12:09Z

tools/targets/ARM_MUSCA_A1/ARM_MUSCA_A1_TFM.py

+
+import assemble
+import imgtool
+import re


Could you separate python standard imports from the local module imports?

theotherjimmy · 2018-11-28T16:12:41Z

tools/targets/ARM_MUSCA_A1/ARM_MUSCA_A1_TFM.py

+sys.path.append(run_dir)
+
+import assemble
+import imgtool


Could you import these modules (which I assume are the local ones) using a relative import?

dreemkiller

Mbed OS will not be using the following components of TF-M, and they should not be integrated into the Mbed OS repo:

MCUBoot - Mbed OS has it's own secure boot requirements and will not be using MCU Boot. It should be removed and replaced with an Mbed OS bootloader before merging
psa_sst_api.h and related components should be removed. Mbed OS TDBStore, implementing the documented PSA Storage APIs, will replace it.
Mbed OS will not us the TF-M Audit logging service, and it should be removed
Mbed OS will choose it's own version of Mbed Crypto, so all crypto related components should be removed

deepikabhavnani · 2018-11-29T21:51:26Z

@ARMmbed/mbed-os-maintainers - FYI, this PR uses pycryptodome module for Musca target build, dependency of which is not captured anywhere

cmonr · 2018-11-30T05:18:40Z

FYI, this PR uses pycryptodome module for Musca target build, dependency of which is not captured anywhere

If that's the case, then requirements.txt needs to be updated accordingly.

If it's needed to simply build, I'm not sure what additional requirements this had on the CI
Fyi @ARMmbed/mbed-os-test-team

dreemkiller · 2018-12-12T17:35:07Z

The plan moving forward with this PR is as follows:

A development branch of Mbed OS has been created based on the source code behind this PR located here: https://github.com/kfnta/mbed-os/tree/miki_musca_base
The TF-M SPM will be taken as is, with changes only to the build system and whatever tweaks are necessary.
The Mbed OS team will replace the TF-M crypto implementation with a snapshot of Mbed Crypto.
The TF-M PSA Storage implementation will be replaced with a solution based on existing MBed OS storage technology that matches better with the needs of the Mbed OS program.
MCU boot will not be enabled in Mbed OS 5.12 release. Later releases will feature a custom Mbed OS bootloader that fulfills Pelion client needs.

These changes will be merged into the Mbed OS master branch for the 5.12 release.
A new snapshot from TF-M sources will be taken for each quarterly releases.
Bug fixes will be released with Mbed OS minor releases.

Partners who wish to support PSA partitioning with Mbed OS on their platforms should continue to contribute to the TF-M project. Periodic snapshots of TF-M will be taken by the Mbed OS team, and assistance from the partners will be required in porting platform-specific features.

cmonr · 2018-12-14T00:40:00Z

Partners who wish to support PSA partitioning with Mbed OS on their platforms should continue to contribute to the TF-M project. Periodic snapshots of TF-M will be taken by the Mbed OS team, and assistance from the partners will be required in porting platform-specific features.

@ashok-rao @MarceloSalazar @screamerbg Fyi.

The TF-M SPM will be taken as is, with changes only to the build system and whatever tweaks are necessary.

Does this mean that this PR needs to be stripped down. or is something new going to come in?

The Mbed OS team will replace the TF-M crypto implementation with a snapshot of Mbed Crypto.

PSA Mbed OS team, or Mbed OS Core, or a different team?

dreemkiller · 2018-12-14T07:51:43Z

Something new is going to come in, and the work will be done by the PSA MBed OS team. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Cruz Monrreal <[email protected]> Sent: Friday, December 14, 2018 12:40 AM To: ARMmbed/mbed-os Cc: Derek Miller; Comment Subject: Re: [ARMmbed/mbed-os] Add ARM_MUSCA_A1 as a new target platform (#8845) Partners who wish to support PSA partitioning with Mbed OS on their platforms should continue to contribute to the TF-M project. Periodic snapshots of TF-M will be taken by the Mbed OS team, and assistance from the partners will be required in porting platform-specific features. @ashok-rao<https://github.com/ashok-rao> @MarceloSalazar<https://github.com/MarceloSalazar> @screamerbg<https://github.com/screamerbg> Fyi. The TF-M SPM will be taken as is, with changes only to the build system and whatever tweaks are necessary. Does this mean that this PR needs to be stripped down. or is something new going to come in? The Mbed OS team will replace the TF-M crypto implementation with a snapshot of Mbed Crypto. PSA Mbed OS team, or Mbed OS Core, or a different team? — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#8845 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AD1LXFdxtcax3B6NzZK0Ipc6T5cjRWRyks5u4vNogaJpZM4Yv1ZM>. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

cmonr · 2018-12-14T16:25:51Z

Thanks @dreemkiller. Will close this for now, expecting a new PR.

ashok-rao · 2018-12-17T10:19:31Z

@gaborkertesz @tamasolaszi .. FYI .

gaborkertesz mentioned this pull request Nov 22, 2018

Add ARM_MUSCA_A1 target ARMmbed/mbed-ls#404

Merged

0xc0170 added the needs: review label Nov 23, 2018

0xc0170 requested a review from ashok-rao November 23, 2018 10:47

0xc0170 requested changes Nov 23, 2018

View reviewed changes

0xc0170 added the scope: new-target label Nov 23, 2018

0xc0170 requested review from a team and deepikabhavnani November 23, 2018 10:53

ashok-rao suggested changes Nov 23, 2018

View reviewed changes

gaborkertesz added 3 commits November 26, 2018 10:38

Remove non-supported mbed version

205f4e8

Since only mbed 5 is supported by Musca-A1, remove legacy 2. Change-Id: I042e3f019b3bff205e554ffd7a8ab638f286328f

Add Arduino connector names

6e43be1

This commit adds digital Arduino connector names. Change-Id: I8ea8bfa5f0c83fab42777e38db5f6baf312d9a81

Fix mbed files licensees

1cd5be4

Update every non-module imported file's licence header. Update to 2018 and SPDX Licence Id. Fixes some minor documentation issues. Change-Id: I2189282ead2bac7b455733394a11c632a37babbe

0xc0170 requested changes Nov 26, 2018

View reviewed changes

0xc0170 added needs: work release-version: 5.11.0-rc1 and removed needs: review labels Nov 26, 2018

0xc0170 added release-version: 5.11.1 and removed release-version: 5.11.0-rc1 labels Nov 26, 2018

bridadan reviewed Nov 26, 2018

View reviewed changes

theotherjimmy reviewed Nov 26, 2018

View reviewed changes

theotherjimmy reviewed Nov 28, 2018

View reviewed changes

dreemkiller suggested changes Nov 28, 2018

View reviewed changes

0xc0170 added release-version: 5.12.0-rc1 and removed release-version: 5.11.1 labels Dec 13, 2018

cmonr closed this Dec 14, 2018

cmonr removed the needs: work label Dec 14, 2018

cmonr removed release-version: 5.12.0-rc1 scope: new-target labels Dec 20, 2018

mikisch81 mentioned this pull request Jan 2, 2019

Add ARM_MUSCA_A1 target #9221

Merged

		@@ -0,0 +1,46 @@
		/* mbed Microcontroller Library
		* Copyright (c) 2006-2017 ARM Limited

		@@ -0,0 +1,94 @@
		/* mbed Microcontroller Library
		* Copyright (c) 2006-2017 ARM Limited

Add ARM_MUSCA_A1 as a new target platform #8845

Add ARM_MUSCA_A1 as a new target platform #8845

Uh oh!

Conversation

gaborkertesz commented Nov 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

ashok-rao commented Nov 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Nov 23, 2018

Uh oh!

ashok-rao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ashok-rao commented Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

bulislaw commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Nov 26, 2018

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborkertesz commented Nov 22, 2018 •

edited

Loading

ashok-rao commented Nov 23, 2018 •

edited

Loading

bulislaw commented Nov 26, 2018 •

edited

Loading

deepikabhavnani commented Nov 29, 2018 •

edited

Loading