Re: [Qemu-devel] [PATCH V3 2/7] nios2: Add architecture emulation support

[Marek Vasut]

On 10/19/2016 05:50 PM, Richard Henderson wrote:
> On 10/18/2016 08:23 PM, Marek Vasut wrote:
>>> The documentation appears less than clear about whether or not loads
>>> into r0 recognize exceptions from the load, as opposed to simply not
>>> modifying r0.
>>
>> What about [1] page 10, quote:
>>
>> The Nios II architecture provides thirty-two 32-bit general-purpose
>> registers, r0 through r31. Some registers have names recognized by the
>> assembler. For example, the zero register (r0) always returns the
>> value zero, and writing to zero has no effect.
> 
> I read that, but is that "... writing to zero has no effect [on the zero
> register]" but loads still take mmu faults, or, as other architecture
> manuals usually call out explicitly, "loads to the zero register are
> prefetch instructions and do not fault".
> 
> Other architectures are split on this issue.  For instance HPPA, Sparc,
> MIPS all fault when loading to the zero register from a protected page. 
> Alpha *initially* reported such faults, but later changed the spec to be
> a prefetch (and therefore kernels supporting ev4 are supposed to
> recognize such loads and return without reporting the SIGSEGV).
> 
> Do you have hardware that you can test this against?  If a load to zero
> is a prefetch, the nios2 gcc port is missing the prefetch pattern.  ;-)

Ha, I see, thanks for explaining :) I have a real system, so I tried
doing the following:

int main()
{
 asm volatile("ldb r0, 0x10(r0)");
}

This will trigger a segfault:

test: unhandled page fault (11) at 0x00000000, cause 14

CPU: 0 PID: 1133 Comm: test Not tainted
4.8.0-rc2-next-20160816-00017-g40615ac #2
task: cfcef160 task.stack: cfcbe000
r1: 00000000 r2: 00000000 r3: 000023cc r4: 00000001
r5: 7f8d7e64 r6: 7f8d7e6c r7: 991f2697 r8: 001263cc
r9: e6925b67 r10: 2aac8ca0 r11: 00000003 r12: 00000000
r13: 2aabcc9c r14: 2aac8428 r15: 00000000
ra: 2aaeee94 fp:  00000000 sp: 7f8d7df0 gp: 0000c000
ea: 000023cc estatus: 00000003
Segmentation fault

> Either way, a comment would clear this up for future maintainers.

Will add one.

>>>> +/* Branch instructions */
>>>> +static void br(DisasContext *dc, uint32_t code, uint32_t flags)
>>>> +{
>>>> +    I_TYPE(instr, code);
>>>> +
>>>> +    gen_goto_tb(dc, 0, dc->pc + 4 + (int16_t)(instr.imm16 & 0xFFFC));
>>>
>>> Probably clearer as pc + 4 + (instr.imm16s & -4)
>>
>> I disagree, the 0xfffc in my opinion clearly states it removes bottom
>> two bits. Doing bitops with signed number looks real iffy.
> 
> It also removes the top 16 bits, requiring you to sign-extend again.
> 
> You might like 0xfffffffc better, but that does require that you count
> f's appropriately for the type.  That's why I like -4: it's obvious (or
> should be) that it masks to a multiple of 4, and type promotion extends
> bits to the left as needed for the type of the other AND argument.

But then you can also use ~3 , right ? In which case, you'd avoid doing
bitwise operations with negative numbers, which is nice.

-- 
Best regards,
Marek Vasut