Re: [U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Simon Glass]

Hi Alex,

On 7 June 2018 at 12:47, Alexander Graf  wrote:
>
>
> On 07.06.18 22:41, Simon Glass wrote:
>> Hi Alex,
>>
>> On 7 June 2018 at 12:36, Alexander Graf  wrote:
>>>
>>>
>>> On 07.06.18 22:25, Simon Glass wrote:
 Hi Alex,

 On 3 June 2018 at 04:13, Alexander Graf  wrote:
>
>
> On 25.05.18 04:42, Simon Glass wrote:
>> Hi Alex,
>>
>> On 24 May 2018 at 06:24, Alexander Graf  wrote:
>>>
>>>
>>> On 16.05.18 17:42, Simon Glass wrote:
 At present this code casts addresses to pointers so cannot be used with
 sandbox. Update it to use mapmem instead.

 Signed-off-by: Simon Glass 
>>>
>>> I really dislike the whole fact that you have to call map_sysmem() at
>>> all. I don't quite understand the whole point of it either - it just
>>> seems to clutter the code and make it harder to follow.
>>
>> The purpose is to map U-Boot addresses (e.g. 0x1234) to actual
>> user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).
>>
>> Otherwise we cannot write tests which use particular addresses, and
>> people have to worry about the host memory layout when using sandbox.
>
> Not if we write a smart enough linker script. I can try to see when I
> get around to give you an example. But basically all we need to do is
> reserve a section for guest ram at a constant virtual address.

 Yes, but ideally that would be 0, or something small.
>>>
>>> You can't do 0 because 0 is protected on a good number of OSs. And if it
>>> can't be 0, better use something that makes pointers easy to read.
>>
>> Yes this is one reason for map_sysmem().
>>
>>>

>
>>> Can't we just simply make sandbox behave like any other target instead?
>>
>> Actually that's the goal of the sandbox support. Memory is modelled as
>> a contiguous chunk starting at 0x0, regardless of what the OS actually
>> gives U-Boot in terms of addresses.
>
> Most platforms don't have RAM start at 0x0 (and making sure nobody
> assumes it does start at 0 is a good thing). The only bit we need to
> make sure is that it always starts at *the same* address on every
> invocation. But if that address is 256MB, things should still be fine.

 Yes but putting a 1000 base address on everything is a bit of a pain.
>>>
>>> Why? It's what we do on arm systems that have ram starting at higher
>>> offsets already.
>>
>> It's a pain because you have to type 1 and 5-6 zeroes before you can
>> get to the address you want. Otherwise sandbox just segfaults, which
>> is annoying.
>
> It's the same as any other device that does not have RAM starting at 0.
> The benefit of it is that you manage to catch NULL pointer accesses
> quite easily, which I guess is something you'll want from a testing target.

You're confusing the U-Boot memory address with the pointer address.
If you use NULL in sandbox it will fault. But address 0 is valid.

>
> Also, you shouldn't use hard addresses in the first place. That's why we
> have $kernel_addr_r and friends. As long as you stick to those, nothing
> should change for you at all.

See for example test_fit.py where it is very useful to know the
addresses we are using.

Of course we can remove this constraint, but it does make things more
painful in sandbox. Also IMO using open casts to convert between
addresses and pointers is not desirable, as they are not really tagged
in any way. With map_sysmem(), etc., they are explicit and obvious.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Alexander Graf]

On 07.06.18 22:41, Simon Glass wrote:
> Hi Alex,
> 
> On 7 June 2018 at 12:36, Alexander Graf  wrote:
>>
>>
>> On 07.06.18 22:25, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 3 June 2018 at 04:13, Alexander Graf  wrote:


 On 25.05.18 04:42, Simon Glass wrote:
> Hi Alex,
>
> On 24 May 2018 at 06:24, Alexander Graf  wrote:
>>
>>
>> On 16.05.18 17:42, Simon Glass wrote:
>>> At present this code casts addresses to pointers so cannot be used with
>>> sandbox. Update it to use mapmem instead.
>>>
>>> Signed-off-by: Simon Glass 
>>
>> I really dislike the whole fact that you have to call map_sysmem() at
>> all. I don't quite understand the whole point of it either - it just
>> seems to clutter the code and make it harder to follow.
>
> The purpose is to map U-Boot addresses (e.g. 0x1234) to actual
> user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).
>
> Otherwise we cannot write tests which use particular addresses, and
> people have to worry about the host memory layout when using sandbox.

 Not if we write a smart enough linker script. I can try to see when I
 get around to give you an example. But basically all we need to do is
 reserve a section for guest ram at a constant virtual address.
>>>
>>> Yes, but ideally that would be 0, or something small.
>>
>> You can't do 0 because 0 is protected on a good number of OSs. And if it
>> can't be 0, better use something that makes pointers easy to read.
> 
> Yes this is one reason for map_sysmem().
> 
>>
>>>

>> Can't we just simply make sandbox behave like any other target instead?
>
> Actually that's the goal of the sandbox support. Memory is modelled as
> a contiguous chunk starting at 0x0, regardless of what the OS actually
> gives U-Boot in terms of addresses.

 Most platforms don't have RAM start at 0x0 (and making sure nobody
 assumes it does start at 0 is a good thing). The only bit we need to
 make sure is that it always starts at *the same* address on every
 invocation. But if that address is 256MB, things should still be fine.
>>>
>>> Yes but putting a 1000 base address on everything is a bit of a pain.
>>
>> Why? It's what we do on arm systems that have ram starting at higher
>> offsets already.
> 
> It's a pain because you have to type 1 and 5-6 zeroes before you can
> get to the address you want. Otherwise sandbox just segfaults, which
> is annoying.

It's the same as any other device that does not have RAM starting at 0.
The benefit of it is that you manage to catch NULL pointer accesses
quite easily, which I guess is something you'll want from a testing target.

Also, you shouldn't use hard addresses in the first place. That's why we
have $kernel_addr_r and friends. As long as you stick to those, nothing
should change for you at all.


Alex
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Simon Glass]

Hi Alex,

On 7 June 2018 at 12:36, Alexander Graf  wrote:
>
>
> On 07.06.18 22:25, Simon Glass wrote:
>> Hi Alex,
>>
>> On 3 June 2018 at 04:13, Alexander Graf  wrote:
>>>
>>>
>>> On 25.05.18 04:42, Simon Glass wrote:
 Hi Alex,

 On 24 May 2018 at 06:24, Alexander Graf  wrote:
>
>
> On 16.05.18 17:42, Simon Glass wrote:
>> At present this code casts addresses to pointers so cannot be used with
>> sandbox. Update it to use mapmem instead.
>>
>> Signed-off-by: Simon Glass 
>
> I really dislike the whole fact that you have to call map_sysmem() at
> all. I don't quite understand the whole point of it either - it just
> seems to clutter the code and make it harder to follow.

 The purpose is to map U-Boot addresses (e.g. 0x1234) to actual
 user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).

 Otherwise we cannot write tests which use particular addresses, and
 people have to worry about the host memory layout when using sandbox.
>>>
>>> Not if we write a smart enough linker script. I can try to see when I
>>> get around to give you an example. But basically all we need to do is
>>> reserve a section for guest ram at a constant virtual address.
>>
>> Yes, but ideally that would be 0, or something small.
>
> You can't do 0 because 0 is protected on a good number of OSs. And if it
> can't be 0, better use something that makes pointers easy to read.

Yes this is one reason for map_sysmem().

>
>>
>>>
> Can't we just simply make sandbox behave like any other target instead?

 Actually that's the goal of the sandbox support. Memory is modelled as
 a contiguous chunk starting at 0x0, regardless of what the OS actually
 gives U-Boot in terms of addresses.
>>>
>>> Most platforms don't have RAM start at 0x0 (and making sure nobody
>>> assumes it does start at 0 is a good thing). The only bit we need to
>>> make sure is that it always starts at *the same* address on every
>>> invocation. But if that address is 256MB, things should still be fine.
>>
>> Yes but putting a 1000 base address on everything is a bit of a pain.
>
> Why? It's what we do on arm systems that have ram starting at higher
> offsets already.

It's a pain because you have to type 1 and 5-6 zeroes before you can
get to the address you want. Otherwise sandbox just segfaults, which
is annoying.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Alexander Graf]

On 07.06.18 22:25, Simon Glass wrote:
> Hi Alex,
> 
> On 3 June 2018 at 04:13, Alexander Graf  wrote:
>>
>>
>> On 25.05.18 04:42, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 24 May 2018 at 06:24, Alexander Graf  wrote:


 On 16.05.18 17:42, Simon Glass wrote:
> At present this code casts addresses to pointers so cannot be used with
> sandbox. Update it to use mapmem instead.
>
> Signed-off-by: Simon Glass 

 I really dislike the whole fact that you have to call map_sysmem() at
 all. I don't quite understand the whole point of it either - it just
 seems to clutter the code and make it harder to follow.
>>>
>>> The purpose is to map U-Boot addresses (e.g. 0x1234) to actual
>>> user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).
>>>
>>> Otherwise we cannot write tests which use particular addresses, and
>>> people have to worry about the host memory layout when using sandbox.
>>
>> Not if we write a smart enough linker script. I can try to see when I
>> get around to give you an example. But basically all we need to do is
>> reserve a section for guest ram at a constant virtual address.
> 
> Yes, but ideally that would be 0, or something small.

You can't do 0 because 0 is protected on a good number of OSs. And if it
can't be 0, better use something that makes pointers easy to read.

> 
>>
 Can't we just simply make sandbox behave like any other target instead?
>>>
>>> Actually that's the goal of the sandbox support. Memory is modelled as
>>> a contiguous chunk starting at 0x0, regardless of what the OS actually
>>> gives U-Boot in terms of addresses.
>>
>> Most platforms don't have RAM start at 0x0 (and making sure nobody
>> assumes it does start at 0 is a good thing). The only bit we need to
>> make sure is that it always starts at *the same* address on every
>> invocation. But if that address is 256MB, things should still be fine.
> 
> Yes but putting a 1000 base address on everything is a bit of a pain.

Why? It's what we do on arm systems that have ram starting at higher
offsets already.

Alex
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Simon Glass]

Hi Alex,

On 3 June 2018 at 04:13, Alexander Graf  wrote:
>
>
> On 25.05.18 04:42, Simon Glass wrote:
>> Hi Alex,
>>
>> On 24 May 2018 at 06:24, Alexander Graf  wrote:
>>>
>>>
>>> On 16.05.18 17:42, Simon Glass wrote:
 At present this code casts addresses to pointers so cannot be used with
 sandbox. Update it to use mapmem instead.

 Signed-off-by: Simon Glass 
>>>
>>> I really dislike the whole fact that you have to call map_sysmem() at
>>> all. I don't quite understand the whole point of it either - it just
>>> seems to clutter the code and make it harder to follow.
>>
>> The purpose is to map U-Boot addresses (e.g. 0x1234) to actual
>> user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).
>>
>> Otherwise we cannot write tests which use particular addresses, and
>> people have to worry about the host memory layout when using sandbox.
>
> Not if we write a smart enough linker script. I can try to see when I
> get around to give you an example. But basically all we need to do is
> reserve a section for guest ram at a constant virtual address.

Yes, but ideally that would be 0, or something small.

>
>>> Can't we just simply make sandbox behave like any other target instead?
>>
>> Actually that's the goal of the sandbox support. Memory is modelled as
>> a contiguous chunk starting at 0x0, regardless of what the OS actually
>> gives U-Boot in terms of addresses.
>
> Most platforms don't have RAM start at 0x0 (and making sure nobody
> assumes it does start at 0 is a good thing). The only bit we need to
> make sure is that it always starts at *the same* address on every
> invocation. But if that address is 256MB, things should still be fine.

Yes but putting a 1000 base address on everything is a bit of a pain.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Alexander Graf]

On 25.05.18 04:42, Simon Glass wrote:
> Hi Alex,
> 
> On 24 May 2018 at 06:24, Alexander Graf  wrote:
>>
>>
>> On 16.05.18 17:42, Simon Glass wrote:
>>> At present this code casts addresses to pointers so cannot be used with
>>> sandbox. Update it to use mapmem instead.
>>>
>>> Signed-off-by: Simon Glass 
>>
>> I really dislike the whole fact that you have to call map_sysmem() at
>> all. I don't quite understand the whole point of it either - it just
>> seems to clutter the code and make it harder to follow.
> 
> The purpose is to map U-Boot addresses (e.g. 0x1234) to actual
> user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).
> 
> Otherwise we cannot write tests which use particular addresses, and
> people have to worry about the host memory layout when using sandbox.

Not if we write a smart enough linker script. I can try to see when I
get around to give you an example. But basically all we need to do is
reserve a section for guest ram at a constant virtual address.

>> Can't we just simply make sandbox behave like any other target instead?
> 
> Actually that's the goal of the sandbox support. Memory is modelled as
> a contiguous chunk starting at 0x0, regardless of what the OS actually
> gives U-Boot in terms of addresses.

Most platforms don't have RAM start at 0x0 (and making sure nobody
assumes it does start at 0 is a good thing). The only bit we need to
make sure is that it always starts at *the same* address on every
invocation. But if that address is 256MB, things should still be fine.


Alex
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Simon Glass]

Hi Alex,

On 24 May 2018 at 06:24, Alexander Graf  wrote:
>
>
> On 16.05.18 17:42, Simon Glass wrote:
>> At present this code casts addresses to pointers so cannot be used with
>> sandbox. Update it to use mapmem instead.
>>
>> Signed-off-by: Simon Glass 
>
> I really dislike the whole fact that you have to call map_sysmem() at
> all. I don't quite understand the whole point of it either - it just
> seems to clutter the code and make it harder to follow.

The purpose is to map U-Boot addresses (e.g. 0x1234) to actual
user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).

Otherwise we cannot write tests which use particular addresses, and
people have to worry about the host memory layout when using sandbox.

>
> Can't we just simply make sandbox behave like any other target instead?

Actually that's the goal of the sandbox support. Memory is modelled as
a contiguous chunk starting at 0x0, regardless of what the OS actually
gives U-Boot in terms of addresses.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Alexander Graf]

On 16.05.18 17:42, Simon Glass wrote:
> At present this code casts addresses to pointers so cannot be used with
> sandbox. Update it to use mapmem instead.
> 
> Signed-off-by: Simon Glass 

I really dislike the whole fact that you have to call map_sysmem() at
all. I don't quite understand the whole point of it either - it just
seems to clutter the code and make it harder to follow.

Can't we just simply make sandbox behave like any other target instead?


Alex
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot