Bug 1226

Summary: are special_sprs tests in caller.py backwards?
Product: Libre-SOC's first SoC Reporter: Jacob Lifshay <programmerjake>
Component: Source CodeAssignee: Luke Kenneth Casson Leighton <lkcl>
Status: RESOLVED INVALID    
Severity: minor CC: libre-soc-bugs, programmerjake
Priority: ---    
Version: unspecified   
Hardware: PC   
OS: Linux   
See Also: https://bugs.libre-soc.org/show_bug.cgi?id=1169
NLnet milestone: --- total budget (EUR) for completion of task and all subtasks: 0
budget (EUR) for this task, excluding subtasks' budget: 0 parent task for budget allocation:
child tasks for budget allocation: The table of payments (in EUR) for this task; TOML format:

Description Jacob Lifshay 2023-11-30 10:46:35 GMT 
there's several different places where ISACaller tries to read/write SPRs and specially handle some SPRs using special_sprs:
e.g. when reading insn inputs:

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/isa/caller.py;hb=2eb5abb845808c436797f3a5449f847fefdacca8#l2236

2236         for special in info.special_regs:
2237             if special in special_sprs:
2238                 inputs[special] = self.spr[special]
2239             else:
2240                 inputs[special] = self.namespace[special]

note the `if special in special_sprs: use self.spr else: use self.namespace`

that looks backwards to me, shouldn't the special SPRs be kept in namespace and all the rest in self.spr?!

This causes a problem when running emulated sc without initializing SRR0 (I didn't initialize it cuz we're user space, who cares what value a register we can't access has):

        # "special" registers
        for special in info.special_regs:
            if special in special_sprs:
                inputs[special] = self.spr[special]
            else:
>               inputs[special] = self.namespace[special]
E               KeyError: 'SRR0'

if the code had instead tried to access self.spr['SRR0'], then SPR.__getitem__ would have filled in a default value rather than failing.
Comment 1 Luke Kenneth Casson Leighton 2023-11-30 12:55:10 GMT 
> note the `if special in special_sprs: use self.spr else: use self.namespace`

yes.

> that looks backwards to me, shouldn't the special SPRs be kept in namespace
> and all the rest in self.spr?!

no.

> 
> This causes a problem when running emulated sc without initializing SRR0 (I
> didn't initialize it cuz we're user space, who cares what value a register
> we can't access has):

you found that it does indeed matter.
uninitialised registers matter.


>         # "special" registers
>         for special in info.special_regs:
>             if special in special_sprs:
>                 inputs[special] = self.spr[special]
>             else:
> >               inputs[special] = self.namespace[special]
> E               KeyError: 'SRR0'

thqt is the correct behaviour.

> if the code had instead tried to access self.spr['SRR0'], then
> SPR.__getitem__ would have filled in a default value rather than failing.

which it should not be doing.

now you know to pass in an initial value to the unit test.
Comment 2 Jacob Lifshay 2023-12-01 00:14:53 GMT 
(In reply to Luke Kenneth Casson Leighton from comment #1)
> you found that it does indeed matter.
> uninitialised registers matter.

well, even if the register is initialized using mtspr it still breaks.

This makes it very annoying when trying to run ELF files since you have to manually specify *all* SPRs your ELF file might touch.

I think this isn't a good default in that case, especially considering people will want to run arbitrary ELF files using something like pypowersim and not have to figure out exactly which SPRs they have to set first.

I came up with a short test that reproduces the problem (as well as actually running the spr tests and adding expectedstate while I was at it):
https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;h=605608bae9626e3a9a034cd0a55eec317db9c2ee