Re: [PATCH v3 13/18] target/arm: Update contiguous first-fault and no-fault loads

[Richard Henderson]

On 4/27/20 9:32 AM, Peter Maydell wrote:
>>>> +     * From this point on, all memory operations are MemSingleNF.
>>>> +     *
>>>> +     * Per the MemSingleNF pseudocode, a no-fault load from Device memory
>>>> +     * must not actually hit the bus -- it returns (UNKNOWN, FAULT) 
>>>> instead.
>>>> +     * If you map non-RAM with Normal memory attributes and do a NF
>>>> +     * load then it should access the bus -- but doing so is illegal.
>>>> +     *
>>>> +     * While we do not have access to the memory attributes from the PTE
>>>> +     * to tell Device memory from Normal memory, we can validly assume 
>>>> that
>>>> +     * non-RAM has been mapped as Device memory.  Thus we indicate fault
>>>> +     * on all MMIO.
>>>
>>> I still don't understand why this is right. All non-RAM is MMIO
>>> but not all MMIO is non-RAM; so you might have something that's
>>> MMIO (at least for the moment) and has been mapped Normal. That
>>> shouldn't fault.
>>
>> Everything that must go through the slow path has TLB_MMIO set.
> 
> Yes. But not everything that goes through the slow path is Device memory.
> We can (should) fault on all accesses to Device memory, but we can't
> fault on all accesses that are slow-pathed, because some of them could
> be entirely valid Normal memory.

We *can* indicate fault from MemSingleNF for any reason whatsoever, or no
reason whatsoever.

> // Implementation may suppress NF load for any reason
> if ConstrainUnpredictableBool(Unpredictable_NONFAULT) then
>   return (bits(8*size) UNKNOWN, TRUE);

What I'm trying to talk about above, is the third statement in MemSingleNF,

> // Non-fault load from Device memory must not be performed externally
> if memaddrdesc.memattrs.memtype == MemType_Device then
>   return (bits(8*size) UNKNOWN, TRUE);

and the reason we can't actually test MemType_Device here.

If you have better wording for that, I'm all ears.  But I don't think there's
an actual bug here.


r~