Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support bootloader version 3 in command offset 0x8E #55

Conversation

stefan-b-jakobsson
Copy link
Collaborator

@stefan-b-jakobsson stefan-b-jakobsson commented Oct 13, 2024

Command offset 0x8E returns the bootloader version.

This adds support, so that the version is returned if you have bootloader 3 installed. Currently that doesn't work.

@stople
Copy link
Collaborator

stople commented Oct 13, 2024

Bootloader v1/v2/v3, start of flash:
v1: 8A 01 -> 0x018A -> movw r16,r20
v2: 8A 02 -> 0x028A -> muls r24,r26 (attiny does not have hardware multiplier)
v3: 01 C0 -> 0xC001 -> rjmp .+2

What if:
v4: 8A C0 -> 0xC08A -> rjmp .+276
(this address is inside the bootloader)

Can it be a better strategy to start by comparing if 1FFE is 8A? If not, then check if 1E00 is 8A?

(as this machine is little endian, and the instructions are 16 bit wide, the most significant byte of the opcode is the second byte. Thus, using the first byte to check for "8A" gives a bit random results, if the address also can be used to store an instruction. Rjmp, first instruction of bootloader v3 and newer, is 0xCxxx", which in memory is "xx Cx").

@stople
Copy link
Collaborator

stople commented Oct 13, 2024

That said. We should probably ban instructions in the form "8A xx" as the first bootloader instruction, as older smc versions will then report some random bootloader version. Thus, with this convention, we can keep this PR as is

@stefan-b-jakobsson
Copy link
Collaborator Author

Banning the bootloader to begin with 0x8a is a good idea. I don't think the AVRA assembler supports that, but we can at least let one of the scripts run by the Github action look for this.

This is one possible solution: https://github.com/stefan-b-jakobsson/x16-smc-bootloader/tree/20241013-prohibit-0x8a

@stople
Copy link
Collaborator

stople commented Oct 13, 2024

Sounds good. Note that, due to AVR having a little endian architecture, the banned instructions are the "xx8A" instructions. Which can be C08A (rjmp .+276). The first instruction will likely always be a rjmp instruction ("Cxxx"). Thus, we can use all rjmp instructions, except +276 (+ N * 256). If this actually turns out to be a problem, we can always jump to some other address inside the bootloader which jumps to the correct address, I assume we will always find room for 2 extra bytes.

@stefan-b-jakobsson
Copy link
Collaborator Author

Changing the order of the if statements is also a good idea. I did that as well.

@stople
Copy link
Collaborator

stople commented Oct 13, 2024

Well, I kind of changed my mind when realizing that older SMC versions will then report inconsistent versions compared to newer SMC versions IF newer bootloaders have a first instruction which is a "8A xx" instruction. But, given that we can choose to ban this instruction from now on (and can get pretty sure there will not be made any "rogue" bootloaders (well, we actually have some experience here.....)), I guess the order should not really matter.

For best backward compatibility, in general, I guess it is best to start with the old stuff, and then move to the new, if the old indicate that this should be done. Up to you.

@stople
Copy link
Collaborator

stople commented Oct 28, 2024

I did some thinking in terms of this PR:

Version rules:

All instructions in the form "8A xx" must be banned as the first instruction.

This means that proper bootloaders will only have 8a in address 1E00 or 1FFE.

This means that the order of checks is not important for "proper" bootloaders.

A rogue (improper) bootloader will behave like this:

If 1E00 is checked first:
-Old and new SMC versions will report a consistent version, which is 1E01. Neither is the correct, which is at 1FFF

If 1FFE is checked first:
-Old and new SMC versions will report different versions
--Old will report from 1E01, which is incorrect
--New will report from 1FFF, which is correct (although bootloader is rogue)

Due to how rogue bootloaders will be handled, I think reading 1E00 first, may be the best and most consistent option. In other words, I suggest to revert the latest commit. (Although rogue bootloaders should off course never happen....)

@stefan-b-jakobsson
Copy link
Collaborator Author

A third alternative would be:

If 0x1e00 = 0x8a & 0x1e01 = 0x01 or 0x02: Read bootloader version from 0x1e01
Else if 0x1ffe = 0x8a & 0x1fff >= 0x03: Read bootloader version from 0x1fff
Else return no bootloader (0xff)

@stople
Copy link
Collaborator

stople commented Oct 29, 2024

Sure, if we can afford keeping the test so strict. This will also add some extra bytes to the code size.

But, note that, if there is a test for "if 1e00 = 8a & 1e01 = 1 or 2", it may suggest that other 8axx can be used. Which will give inconsistent results based on smc version.

Note that bootloader v3 already will give inconsistent version based on smc version, but, that is due to this test have not been implemented yet.

Option 1 (1e00 before 1ffe):

Pros: Straight forward, incremental, backward compatible for old bootloaders, minimal code size

Cons: Rogue bootloaders (8axx code at 1e00) shows 1e01 as version (but, version is bad anyway)

Option 2 (1ffe before 1e00):

Note: Rogue bootloaders shows 1fff as version, while for old smc version it shows 1e01.

Pros: Intended rogue version is revealed

Cons: Version is rogue and may look like a good version. Version is inconsistent on old vs new SMC (random 1e01 data vs claimed version). But, all newer bootloaders will be inconsistent based on SMC version anyway (FF vs claimed version)

Option 3: Strict range check, 1e00 before 1ffe:

Note: Rogue bootloaders will most likely show intended version

Pros: Intent is more clear in the code. Bigger chance that intended bootloader version is shown

Cons: Bigger code size, may suggest that bootloader can start with 8axx

One factor is how "rogue versions" is handled (first instruxtion is 8a xx). This should not be used by proper versions, but, we know that bad versions can suddenly be present. If this is the case, I would argue that it is not that important to show the intended version. The best option could perhaps be to have a sanity check and report e.g. FE which indicates "version inconsistency". Yet, this occupies bytes in flash which we maybe do not have. And, to confirm a rogue (and hence, "bad" bootloader), we have learnt that the best tool is to read the entire bootloader and compare the checksum against known bootloaders.

In fact: SMC does not really need to have this sanity check. SMC can be simple and stupid. The programming tools can have the sanity check. If current SMC version is 47.2.3 or newer, this can be the programming tool logic:

  • Check for bootloader v1/v2/v2bad/v3 using crc-16
  • If crc-16 is not recognized:
    • Check for "rogue bootloader" (both 1e00 and 1ffe is 8a)
      • We can treat this as "corrupt bootloader"
    • If 1e00 = 8a & 1e01 > 1
      • v1/v2 style, but, unrecognized version
        • Corrupt bootloader
    • else if 1ffe = 8a & 1fff < 3
      • v3 style, but, <3 is undefined
        • Corrupt bootloader

@stople
Copy link
Collaborator

stople commented Oct 29, 2024

Sorry for the long, previous comment. To summarize: I suggest to keep the SMC bootloader version test, in SMC firmware, simple and dumb, to save space, but add sanity checks for corrupt versions in the programming tools, by reading all bytes of the bootloader. Thus, I suggest any of option 1 and 2.

@stefan-b-jakobsson
Copy link
Collaborator Author

OK, I reverted the code to checking 0x1e00 first, and 0x1ffe second.

The update program doesn't currently verify the bootloader by calculating and comparing its CRC. I guess that's a good improvement.

@stople
Copy link
Collaborator

stople commented Oct 30, 2024

<3 is undefined

Maybe I should get a new hobby 😄

@stefan-b-jakobsson stefan-b-jakobsson merged commit 70e3a59 into X16Community:main Oct 30, 2024
1 check passed
@stople
Copy link
Collaborator

stople commented Oct 30, 2024

I forgot: This is a change of the machine code, and behavior, and thus, the version number should probably have been bumped as well. @stefan-b-jakobsson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants