Re: [Qemu-devel] [Qemu-arm] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Cédric Le Goater]

On 09/26/2016 10:25 AM, KONRAD Frederic wrote:
> 
> 
> Le 24/09/2016 à 10:55, Edgar E. Iglesias a écrit :
>> On Sat, Sep 24, 2016 at 10:25:39AM +0200, Cédric Le Goater wrote:
>>> On 09/23/2016 08:26 PM, mar.krzeminski wrote:
>>>> Hi Cedric,
>>>>
>>>> W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze:
>>>>> On 09/23/2016 10:17 AM, Peter Maydell wrote:
>>>>>> On 23 September 2016 at 08:19, Cédric Le Goater <address@hidden> wrote:
>>>>>>> But the goal is to boot from the device, so I added a memory region 
>>>>>>> alias
>>>>>>> at 0 to trigger the flash module mmios at boot time, as this is where
>>>>>>> u-boot expects to be.
>>>>>>>
>>>>>>> and I fell in this trap :/
>>>>>>>
>>>>>>>          aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0
>>>>>>>          Bad ram pointer (nil)
>>>>>>>          Aborted (core dumped)
>>>>>>>
>>>>>>> There is a failure in get_page_addr_code(), possibly because qemu uses
>>>>>>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my
>>>>>>> understanding of qemu's internal.
>>>>>> This is a bug in how we report the problem, but the underlying
>>>>>> issue here is attempting to execute from something that's not RAM
>>>>>> or ROM. You can't execute code out of something backed by MMIO.
>>>>> OK. So I see two solutions. T
>>>>>
>>>>> The "brutal" one which is to copy the flash contents in a rom blob
>>>>> at 0, but there is still an issue in getting access to the storage
>>>>> anyhow, as it is internal to m25p80. Or we should get the name of the
>>>>> backing file of the drive but I am not sure we are expected to do
>>>>> that as I don't see any API for it.
>>>>>
>>>>> The other solution is something like this patch which lets the storage
>>>>> of the flash device be assigned externally.
>>>> Since I do not like dirty hacks in the code, I want just to suggest a
>>>> workaround, that probably you will not like ;]
>>>
>>> It's a feature ! :)
>>
>> CC: Mark and Fred
>>
>> I agree, I think these are fair attempts to try to solve a real problem.
>>
>>
>>> I am just trying to find a solution to this issue. So, we could also
>>> introduce a routine :
>>>
>>> +uint8_t *m25p80_get_storage(DeviceState *dev, uint32_t *size)
>>> +{
>>> +    Flash *s = M25P80(dev);
>>> +
>>> +    *size = s->size;
>>> +    return s->storage;
>>> +}
>>>
>>> and use rom_add_blob_fixed() with the return values. Maybe this is less
>>> intrusive in the m25p80 device model flow. Thoughts ?
>>>
>>>> As Qemu expects that first running code will be in ROM or RAM memory,
>>>> you can implement in your board -bios option that you will use to
>>>> pass u-boot binary to rom memory, or even use generic loader functionality
>>>> when it reach master.
>>>
>>> but if we use -bios <file> to have a ROM, we will need to pass a second
>>> time <file> as a drive to have a CS0 flash slave:
>>>
>>>     -bios "flash-palmetto-test"  \
>>>     -drive file="flash-palmetto-test",format=raw,if=mtd \
>>>     -drive file="palmetto.pnor",format=raw,if=mtd
>>>
>>> This feels awkward. The virt platform and vexpress forbid that for
>>> instance.
>>>
>>> Are there any other platform with similar need ?
>>
>> Yes, ZynqMP has a linear memory mapping of SPI memory contents. It gets
>> slightly more complicated there as the mapping supports various modes
>> including parallel SPIs with striping done by the controller (i.e
>> bytes as seen via the linearly mapped area are interleaved from multiple
>> SPI memories).
>>
>> So a plain RAM mapping of m25p80_get_storage won't work.
>>
>> A ROM version of the linear data might work at controller level. The 
>> controller
>> would have to populate the ROM area at startup (slow) and keep it in sync 
>> for all
>> writes. We would also need to signal write events so that TCG can flush it's
>> caches. TCG only keeps track of modifications being made to cached code if 
>> changes
>> are done with writes to RAM area (not if they happen indirectly via other 
>> registers).
>>
>> Another solution is to implement support in TCG to execute from MMIO mapped 
>> areas.
>> We'd also need to solve the problem of signalling content changes to TCG.
>>
>> I think these approaches have some overlap. Personally, I think TCG 
>> executing from
>> MMIO areas would be quite useful. It's also useful for QEMU & SystemC 
>> integration.
> 
> Hi,
> 
> The solution we are preparing for this is to allow a MMIO region to execute 
> code 
> as Edgar just suggested by adding two callbacks:

Yes it seems the best option to boot. 

The SPI controller I am working on maintains a set of mapping windows for 
each flash slave. But it's up to software to make sure the window size and 
the flash size are in sync. So, in a qemu world, we will need some flexibility 
if we want to access directly the flash storage. I don't think this patch is 
the right approach though. Let's boot first. 

>   - one which allows to get a pointer from an MMIO region nothing if
>     execution is not possible from it and some hint to know if it's
>     readable, writable.
>   - one function to "invalidate" the pointer which will flush the tb,
>     etc.

I don't know enough about TCG to comment, I am very willing to give it 
a try when ever you have something ready.

Thanks,

C.

> We have fixed a second issue fairly linked to this: if we do a DMA access to 
> the 
> MMIO areas it is split in 1,2,4 bytes transaction which is really slow.