v5:
> > + AES_KEY key;
> > + if ((s->ctrl & TYPE) == 0) {
> > + AES_set_encrypt_key(keydata, keylen, &key);
> > + AES_set_decrypt_key(keydata, keylen, &s->internal_key);
> > + AES_encrypt(s->data, s->result, &key);
> Here we call AES_set_encrypt_key() and AES_set_decrypt_key()
> before calling AES_encrypt()...
> > + s->result_index = 16;
> > + } else if ((s->ctrl & TYPE) == 1 << 8) {
> > + AES_set_decrypt_key(keydata, keylen, &key);
> > + AES_set_decrypt_key(keydata, keylen, &s->internal_key);
> > + AES_decrypt(s->data, s->result, &key);
>
> ...here we call AES_set_decrypt_key() twice before
> calling AES_decrypt(). This looks a bit odd: should we either
> (a) call both AES_set_decrypt_key() and AES_set_encrypt_key()
> in each half of the if(), or (b) call AES_set_encyrypt_key()
> twice in the AES_encrypt() code path ?
>
> (Coverity is sometimes wrong, as it's only using a heuristic
> here, so the other option is "the code as written is correct",
> but in that case a comment might be helpful for human readers.)
> thanks
> -- PMM
The AES engine stores an internal key which it uses only for decryption
when in the last TYPE. This results in the odd-looking call pairs.
I've added the requested comment.
- Jackson
v4:
Spacing and style standard changes in GCR SOC, TRNG SOC, and AES SOC
v3:
Addresses a few more comments by Peter. Really appreciate the review, thank you.
ICC SOC:
Gave each device a unique name
UART SOC:
Removed references to DeviceState and gave each device a unique name
GCR SOC:
Set object property links statically, instead of by reference to device id
TRNG:
Added comments explaining interrupt generation logic. See further rationale
under v2.
It's certainly possible I'm missing something here, but I think what I have is
correct.
TRNG SOC:
Removed reference to DeviceState
AES:
Simplified integer load & stores, added assertion before writing to
memory_region_init_ram
AES SOC:
Removed reference to DeviceState
v2:
Addresses comments by Peter.
For each device:
- Switched soc to use sysbus_realize
- Standardized switch case bracing, indentation, and error case
- Added valid min and max access size
- Changed endianness to DEVICE_LITTLE_ENDIAN
- Added reset method, if not already implemented
- Added migration support
- Split soc integration into separate commit
Machine Implementation:
Added user guide URL
Removed refclk. According to
https://developer.arm.com/documentation/ddi0403/d/System-Level-Architecture/System-Address-Map/The-system-timer--SysTick/SysTick-Control-and-Status-Register--SYST-CSR
the systick clock can be either the processor clock or an implementation
defined external reference clock. As far as I can tell based on the user guide,
the MAX78000 does not define an external reference clock. I have not confirmed
this in hardware.
Changed IRQ count to 120 and noted that the user guide is unclear on this number
Fixed unimplemented device lengths and names
ICC:
Removed ICC_Invalidate
Added number to device names
UART:
Fixed interrupts, allowing async UART
Added number to device names
GCR:
Changed string-based device search to prop links for device reset
Changed cpu_physical_memory_write to address_space_write
TRNG:
Changed source of randomness to qemu_guest_getrandom_nofail
Did not change interrupt generation
> Your interrupt generation code in this device doesn't look
> right: the interrupt is supposed to be generated when each
> new random number is ready, so in our "generation takes
> zero time" model a read from DATA should provoke a new
> interrupt immediately (assuming the interrupt is enabled):
> you need to simulate the ready status bit going low and
> then high again.
I believe the interrupt generation is fine in this case;
by latching it high so long as the interrupt is enabled,
the interrupt handler gets called repeatedly until it
receives the desired amount of data and disables the interrupt.
I've tested this and it works with the maxim sdk's
implementation of asynchronous trng.
> See also my comments on an earlier patch about the usual
> logic being to have an update function which does
> "set interrupt to (condition && enabled)".
In this case there is only one possible interrupt condition
(is there random data ready), which is always true when enabled.
As such I don't think a handler function is really necessary
v1:
This patch series implements basic support for the MAX78000FTHR machine
https://github.com/JacksonDonaldson/max78000Test
Contains instructions for building a test program against the MAX78000FTHR
as well as results of the test suite run on QEMU and hardware.
Jackson Donaldson (11):
MAX78000: Add MAX78000FTHR Machine
MAX78000: ICC Implementation
MAX78000: Add ICC to SOC
MAX78000: UART Implementation
MAX78000: Add UART to SOC
MAX78000: GCR Implementation
MAX78000: Add GCR to SOC
MAX78000: TRNG Implementation
MAX78000: Add TRNG to SOC
MAX78000: AES implementation
MAX78000: Add AES to SOC
hw/arm/Kconfig | 15 ++
hw/arm/max78000_soc.c | 232 +++++++++++++++++++++
hw/arm/max78000fthr.c | 50 +++++
hw/arm/meson.build | 2 +
hw/char/Kconfig | 3 +
hw/char/max78000_uart.c | 285 ++++++++++++++++++++++++++
hw/char/meson.build | 1 +
hw/misc/Kconfig | 12 ++
hw/misc/max78000_aes.c | 229 +++++++++++++++++++++
hw/misc/max78000_gcr.c | 351 ++++++++++++++++++++++++++++++++
hw/misc/max78000_icc.c | 120 +++++++++++
hw/misc/max78000_trng.c | 139 +++++++++++++
hw/misc/meson.build | 4 +
include/hw/arm/max78000_soc.h | 50 +++++
include/hw/char/max78000_uart.h | 78 +++++++
include/hw/misc/max78000_aes.h | 68 +++++++
include/hw/misc/max78000_gcr.h | 131 ++++++++++++
include/hw/misc/max78000_icc.h | 33 +++
include/hw/misc/max78000_trng.h | 35 ++++
19 files changed, 1838 insertions(+)
create mode 100644 hw/arm/max78000_soc.c
create mode 100644 hw/arm/max78000fthr.c
create mode 100644 hw/char/max78000_uart.c
create mode 100644 hw/misc/max78000_aes.c
create mode 100644 hw/misc/max78000_gcr.c
create mode 100644 hw/misc/max78000_icc.c
create mode 100644 hw/misc/max78000_trng.c
create mode 100644 include/hw/arm/max78000_soc.h
create mode 100644 include/hw/char/max78000_uart.h
create mode 100644 include/hw/misc/max78000_aes.h
create mode 100644 include/hw/misc/max78000_gcr.h
create mode 100644 include/hw/misc/max78000_icc.h
create mode 100644 include/hw/misc/max78000_trng.h
--
2.34.1
