[Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Alexander Graf]

When running the s390x virtio machine we can potentially use uninitialized
memory for the virtio device backing ram. That can lead to weird breakge.

So let's better initialize it to 0 properly.

Reported-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/s390-virtio.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 37945d5..d936809 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -165,6 +165,8 @@ static void s390_init(ram_addr_t my_ram_size,
     ram_addr_t initrd_size = 0;
     int shift = 0;
     uint8_t *storage_keys;
+    void *virtio_region;
+    target_phys_addr_t virtio_region_len;
     int i;
 
     /* s390x ram size detection needs a 16bit multiplier + an increment. So
@@ -184,6 +186,13 @@ static void s390_init(ram_addr_t my_ram_size,
     memory_region_init_ram(ram, NULL, "s390.ram", my_ram_size);
     memory_region_add_subregion(sysmem, 0, ram);
 
+    /* clear virtio region */
+    virtio_region_len = my_ram_size - ram_size;
+    virtio_region = cpu_physical_memory_map(ram_size, &virtio_region_len, true);
+    memset(virtio_region, 0, virtio_region_len);
+    cpu_physical_memory_unmap(virtio_region, virtio_region_len, 1,
+                              virtio_region_len);
+
     /* allocate storage keys */
     storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
 
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Peter Maydell]

On 10 November 2011 01:19, Alexander Graf <agraf@suse.de> wrote:
> @@ -184,6 +186,13 @@ static void s390_init(ram_addr_t my_ram_size,
>     memory_region_init_ram(ram, NULL, "s390.ram", my_ram_size);
>     memory_region_add_subregion(sysmem, 0, ram);
>
> +    /* clear virtio region */
> +    virtio_region_len = my_ram_size - ram_size;
> +    virtio_region = cpu_physical_memory_map(ram_size, &virtio_region_len, true);
> +    memset(virtio_region, 0, virtio_region_len);
> +    cpu_physical_memory_unmap(virtio_region, virtio_region_len, 1,
> +                              virtio_region_len);
> +

This looks a bit fishy -- cpu_physical_memory_map() takes a
target_phys_addr_t but you're passing it a ram_addr_t.

Also isn't this second-guessing the ram_addr_t of the region allocated
inside memory_region_init_ram() ?

-- PMM

Re: [Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Alexander Graf]

On 10.11.2011, at 02:36, Peter Maydell wrote:

> On 10 November 2011 01:19, Alexander Graf <agraf@suse.de> wrote:
>> @@ -184,6 +186,13 @@ static void s390_init(ram_addr_t my_ram_size,
>>     memory_region_init_ram(ram, NULL, "s390.ram", my_ram_size);
>>     memory_region_add_subregion(sysmem, 0, ram);
>> 
>> +    /* clear virtio region */
>> +    virtio_region_len = my_ram_size - ram_size;
>> +    virtio_region = cpu_physical_memory_map(ram_size, &virtio_region_len, true);
>> +    memset(virtio_region, 0, virtio_region_len);
>> +    cpu_physical_memory_unmap(virtio_region, virtio_region_len, 1,
>> +                              virtio_region_len);
>> +
> 
> This looks a bit fishy -- cpu_physical_memory_map() takes a
> target_phys_addr_t but you're passing it a ram_addr_t.

Meh. Always those types ... :)

> Also isn't this second-guessing the ram_addr_t of the region allocated
> inside memory_region_init_ram() ?

Not sure I understand? On s390-virtio we have to following ram layout:

[ ----- RAM ---- | --- virtio device structs --- ]

The size of the latter is added to my_ram_size by s390_virtio_bus_init. All I'm trying to do is make sure that this latter part of RAM is always 0, while the first part can still be dynamically allocated.


Alex

Re: [Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Peter Maydell]

On 10 November 2011 01:45, Alexander Graf <agraf@suse.de> wrote:
> On 10.11.2011, at 02:36, Peter Maydell wrote:
>> This looks a bit fishy -- cpu_physical_memory_map() takes a
>> target_phys_addr_t but you're passing it a ram_addr_t.
>
> Meh. Always those types ... :)

In the simple case ("ram starts at 0, not multiply mapped, guest
and host pointer widths the same") target_phys_addr_t's and
ram_addr_t's are the same, but this isn't necessarily so; indeed
one ram_addr_t can be mapped to multiple target_phys_addr_t's
in some system models. So the two things aren't trivially
interconvertible. (As it happens I think in this case you're
actually just doing physical address calculations using the wrong
type...)

> On 10.11.2011, at 02:36, Peter Maydell wrote:
>> Also isn't this second-guessing the ram_addr_t of the region allocated
>> inside memory_region_init_ram() ?
>
> Not sure I understand? On s390-virtio we have to following ram layout:
>
> [ ----- RAM ---- | --- virtio device structs --- ]
>
> The size of the latter is added to my_ram_size by
> s390_virtio_bus_init. All I'm trying to do is make sure that this
> latter part of RAM is always 0, while the first part can still be
> dynamically allocated.

That comment was written on the assumption that you were actually
trying to manipulate a ram_addr_t in your variable of type
ram_addr_t; to expand on it a bit:

I think that conceptually the only way to get the ram_addr_t for a
bit of RAM should be that you got it back from qemu_ram_alloc()
(or that you did the obvious arithmetic to get a ram_addr_t within
a block given the start of one). As it happens qemu_ram_alloc()
starts ram_addr_t's at 0 and works up with no gaps, but that's
an implementation detail. (Also if anybody ever did something that
meant there was another qemu_ram_alloc() call before the one done
inside that memory_region_init_ram() then things would break.)

In summary, either:
(1) you need to ask the memory region for its ram_addr_t
[there doesn't seem to be an API for this at the moment],
do arithmetic on ram_addr_t's and use qemu_get_ram_ptr() to
get a host pointer corresponding to that ram_addr_t
or (2) you need to do your arithmetic in target_phys_addr_t's
(and then you can say your RAM starts at physaddr 0, which
is guaranteed, rather than at ram_addr_t 0, which isn't)
and use cpu_physical_memory_map/unmap().

-- PMM

Re: [Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Alexander Graf]

On 11/10/2011 03:19 AM, Peter Maydell wrote:
> On 10 November 2011 01:45, Alexander Graf<agraf@suse.de>  wrote:
>> On 10.11.2011, at 02:36, Peter Maydell wrote:
>>> This looks a bit fishy -- cpu_physical_memory_map() takes a
>>> target_phys_addr_t but you're passing it a ram_addr_t.
>> Meh. Always those types ... :)
> In the simple case ("ram starts at 0, not multiply mapped, guest
> and host pointer widths the same") target_phys_addr_t's and
> ram_addr_t's are the same, but this isn't necessarily so; indeed
> one ram_addr_t can be mapped to multiple target_phys_addr_t's
> in some system models. So the two things aren't trivially
> interconvertible. (As it happens I think in this case you're
> actually just doing physical address calculations using the wrong
> type...)

This is the machine init function. The s390 virtio machine's ram layout 
is defined to be exactly as I posted in the previous post. So there 
won't be any ram starts at != 0 or multiple mappings :).

>> On 10.11.2011, at 02:36, Peter Maydell wrote:
>>> Also isn't this second-guessing the ram_addr_t of the region allocated
>>> inside memory_region_init_ram() ?
>> Not sure I understand? On s390-virtio we have to following ram layout:
>>
>> [ ----- RAM ---- | --- virtio device structs --- ]
>>
>> The size of the latter is added to my_ram_size by
>> s390_virtio_bus_init. All I'm trying to do is make sure that this
>> latter part of RAM is always 0, while the first part can still be
>> dynamically allocated.
> That comment was written on the assumption that you were actually
> trying to manipulate a ram_addr_t in your variable of type
> ram_addr_t; to expand on it a bit:
>
> I think that conceptually the only way to get the ram_addr_t for a
> bit of RAM should be that you got it back from qemu_ram_alloc()
> (or that you did the obvious arithmetic to get a ram_addr_t within
> a block given the start of one). As it happens qemu_ram_alloc()
> starts ram_addr_t's at 0 and works up with no gaps, but that's
> an implementation detail. (Also if anybody ever did something that
> meant there was another qemu_ram_alloc() call before the one done
> inside that memory_region_init_ram() then things would break.)
>
> In summary, either:
> (1) you need to ask the memory region for its ram_addr_t
> [there doesn't seem to be an API for this at the moment],
> do arithmetic on ram_addr_t's and use qemu_get_ram_ptr() to
> get a host pointer corresponding to that ram_addr_t
> or (2) you need to do your arithmetic in target_phys_addr_t's
> (and then you can say your RAM starts at physaddr 0, which
> is guaranteed, rather than at ram_addr_t 0, which isn't)
> and use cpu_physical_memory_map/unmap().

I still don't get it. What I want is:

   for (i = ram_size; i < my_ram_size; i++)
     stb_phys(env, i, 0);

cpu_physical_memory_map gives me a pointer to the memory region between 
ram_size and my_ram_size. I memset(0) it. I don't see how I could 
possibly have different offsets anywhere. If cpu_physical_memory_map 
would take anything different than physical address, a lot of 
assumptions in QEMU code would break.


Alex

Re: [Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Peter Maydell]

On 11 November 2011 15:59, Alexander Graf <agraf@suse.de> wrote:
> This is the machine init function. The s390 virtio machine's ram layout is
> defined to be exactly as I posted in the previous post. So there won't be
> any ram starts at != 0 or multiple mappings :).

That's the layout of the RAM in target_phys_addr_t space.
The layout of the ram in ram_addr_t space is not guaranteed
to be the same, it just happens to be in this case.

>>> On 10.11.2011, at 02:36, Peter Maydell wrote:
>> In summary, either:
>> (1) you need to ask the memory region for its ram_addr_t
>> [there doesn't seem to be an API for this at the moment],
>> do arithmetic on ram_addr_t's and use qemu_get_ram_ptr() to
>> get a host pointer corresponding to that ram_addr_t
>> or (2) you need to do your arithmetic in target_phys_addr_t's
>> (and then you can say your RAM starts at physaddr 0, which
>> is guaranteed, rather than at ram_addr_t 0, which isn't)
>> and use cpu_physical_memory_map/unmap().
>
> I still don't get it. What I want is:
>
>  for (i = ram_size; i < my_ram_size; i++)
>    stb_phys(env, i, 0);
>
> cpu_physical_memory_map gives me a pointer to the memory region between
> ram_size and my_ram_size. I memset(0) it. I don't see how I could possibly
> have different offsets anywhere. If cpu_physical_memory_map would take
> anything different than physical address, a lot of assumptions in QEMU code
> would break.

Yes, so that's option (2) and you need to be using a target_phys_addr_t.

-- PMM

Re: [Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Alexander Graf]

On 11/11/2011 05:11 PM, Peter Maydell wrote:
> On 11 November 2011 15:59, Alexander Graf<agraf@suse.de>  wrote:
>> This is the machine init function. The s390 virtio machine's ram layout is
>> defined to be exactly as I posted in the previous post. So there won't be
>> any ram starts at != 0 or multiple mappings :).
> That's the layout of the RAM in target_phys_addr_t space.
> The layout of the ram in ram_addr_t space is not guaranteed
> to be the same, it just happens to be in this case.
>
>>>> On 10.11.2011, at 02:36, Peter Maydell wrote:
>>> In summary, either:
>>> (1) you need to ask the memory region for its ram_addr_t
>>> [there doesn't seem to be an API for this at the moment],
>>> do arithmetic on ram_addr_t's and use qemu_get_ram_ptr() to
>>> get a host pointer corresponding to that ram_addr_t
>>> or (2) you need to do your arithmetic in target_phys_addr_t's
>>> (and then you can say your RAM starts at physaddr 0, which
>>> is guaranteed, rather than at ram_addr_t 0, which isn't)
>>> and use cpu_physical_memory_map/unmap().
>> I still don't get it. What I want is:
>>
>>   for (i = ram_size; i<  my_ram_size; i++)
>>     stb_phys(env, i, 0);
>>
>> cpu_physical_memory_map gives me a pointer to the memory region between
>> ram_size and my_ram_size. I memset(0) it. I don't see how I could possibly
>> have different offsets anywhere. If cpu_physical_memory_map would take
>> anything different than physical address, a lot of assumptions in QEMU code
>> would break.
> Yes, so that's option (2) and you need to be using a target_phys_addr_t.

But ram_size is ram_addr_t and is the ram size that I have available to 
use, so it's exactly the address that I want. I don't see your point. 
Should go jump through random useless hoops of doing

   target_phys_addr_t ram_end = ram_size;

just because there are some subtile semantic differences between the two 
variables? They're integers at the end of the day. Both of them.


Alex

Re: [Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Peter Maydell]

On 11 November 2011 16:24, Alexander Graf <agraf@suse.de> wrote:
> On 11/11/2011 05:11 PM, Peter Maydell wrote:
>> Yes, so that's option (2) and you need to be using a target_phys_addr_t.
>
> But ram_size is ram_addr_t and is the ram size that I have available to use,
> so it's exactly the address that I want. I don't see your point. Should go
> jump through random useless hoops of doing
>
>  target_phys_addr_t ram_end = ram_size;
>
> just because there are some subtile semantic differences between the two
> variables? They're integers at the end of the day. Both of them.

Because handing a ram_addr_t to something that wants a target_phys_addr_t
is a red flag that something might be wrong. (As is handing a variable
called _size to something that wants an address.)

Force in Newtons and Force in pound-feet are also both just integers
at the end of the day, but that doesn't make them interchangeable:
http://en.wikipedia.org/wiki/Mars_Climate_Orbiter#Communications_loss

-- PMM

Re: [Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Alexander Graf]

On 11/11/2011 05:44 PM, Peter Maydell wrote:
> On 11 November 2011 16:24, Alexander Graf<agraf@suse.de>  wrote:
>> On 11/11/2011 05:11 PM, Peter Maydell wrote:
>>> Yes, so that's option (2) and you need to be using a target_phys_addr_t.
>> But ram_size is ram_addr_t and is the ram size that I have available to use,
>> so it's exactly the address that I want. I don't see your point. Should go
>> jump through random useless hoops of doing
>>
>>   target_phys_addr_t ram_end = ram_size;
>>
>> just because there are some subtile semantic differences between the two
>> variables? They're integers at the end of the day. Both of them.
> Because handing a ram_addr_t to something that wants a target_phys_addr_t
> is a red flag that something might be wrong. (As is handing a variable
> called _size to something that wants an address.)
>
> Force in Newtons and Force in pound-feet are also both just integers
> at the end of the day, but that doesn't make them interchangeable:
> http://en.wikipedia.org/wiki/Mars_Climate_Orbiter#Communications_loss

Haha :). In this case, I pass ram_size indirectly to ram_alloc just a 
few lines above though, mapping it to 0. So while if this was normal 
device code I would agree with your reservation, in this case I don't 
think adding indirection buys us anything.


Alex

Re: [Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Alexander Graf]

On 11/11/2011 05:44 PM, Peter Maydell wrote:
> On 11 November 2011 16:24, Alexander Graf<agraf@suse.de>  wrote:
>> On 11/11/2011 05:11 PM, Peter Maydell wrote:
>>> Yes, so that's option (2) and you need to be using a target_phys_addr_t.
>> But ram_size is ram_addr_t and is the ram size that I have available to use,
>> so it's exactly the address that I want. I don't see your point. Should go
>> jump through random useless hoops of doing
>>
>>   target_phys_addr_t ram_end = ram_size;
>>
>> just because there are some subtile semantic differences between the two
>> variables? They're integers at the end of the day. Both of them.
> Because handing a ram_addr_t to something that wants a target_phys_addr_t
> is a red flag that something might be wrong. (As is handing a variable
> called _size to something that wants an address.)
>
> Force in Newtons and Force in pound-feet are also both just integers
> at the end of the day, but that doesn't make them interchangeable:
> http://en.wikipedia.org/wiki/Mars_Climate_Orbiter#Communications_loss

How about this patch on top?

Alex


diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index d936809..ad37a9d 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -188,7 +188,8 @@ static void s390_init(ram_addr_t my_ram_size,

      /* clear virtio region */
      virtio_region_len = my_ram_size - ram_size;
-    virtio_region = cpu_physical_memory_map(ram_size, 
&virtio_region_len, true);
+    virtio_region = cpu_physical_memory_map((target_phys_addr_t)ram_size,
+ &virtio_region_len, true);
      memset(virtio_region, 0, virtio_region_len);
      cpu_physical_memory_unmap(virtio_region, virtio_region_len, 1,
                                virtio_region_len);

Re: [Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Alexander Graf]

On 11/11/2011 05:44 PM, Peter Maydell wrote:
> On 11 November 2011 16:24, Alexander Graf<agraf@suse.de>  wrote:
>> On 11/11/2011 05:11 PM, Peter Maydell wrote:
>>> Yes, so that's option (2) and you need to be using a target_phys_addr_t.
>> But ram_size is ram_addr_t and is the ram size that I have available to use,
>> so it's exactly the address that I want. I don't see your point. Should go
>> jump through random useless hoops of doing
>>
>>   target_phys_addr_t ram_end = ram_size;
>>
>> just because there are some subtile semantic differences between the two
>> variables? They're integers at the end of the day. Both of them.
> Because handing a ram_addr_t to something that wants a target_phys_addr_t
> is a red flag that something might be wrong. (As is handing a variable
> called _size to something that wants an address.)
>
> Force in Newtons and Force in pound-feet are also both just integers
> at the end of the day, but that doesn't make them interchangeable:
> http://en.wikipedia.org/wiki/Mars_Climate_Orbiter#Communications_loss

Or actually rather this one:


Alex

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index d936809..61b67e8 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -167,6 +167,7 @@ static void s390_init(ram_addr_t my_ram_size,
      uint8_t *storage_keys;
      void *virtio_region;
      target_phys_addr_t virtio_region_len;
+    target_phys_addr_t virtio_region_start;
      int i;

      /* s390x ram size detection needs a 16bit multiplier + an 
increment. So
@@ -188,7 +189,9 @@ static void s390_init(ram_addr_t my_ram_size,

      /* clear virtio region */
      virtio_region_len = my_ram_size - ram_size;
-    virtio_region = cpu_physical_memory_map(ram_size, 
&virtio_region_len, true);
+    virtio_region_start = ram_size;
+    virtio_region = cpu_physical_memory_map(virtio_region_start,
+ &virtio_region_len, true);
      memset(virtio_region, 0, virtio_region_len);
      cpu_physical_memory_unmap(virtio_region, virtio_region_len, 1,
                                virtio_region_len);

Re: [Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Peter Maydell]

On 11 November 2011 17:40, Alexander Graf <agraf@suse.de> wrote:
> Or actually rather this one:
>
>
> Alex
>
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index d936809..61b67e8 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -167,6 +167,7 @@ static void s390_init(ram_addr_t my_ram_size,
>     uint8_t *storage_keys;
>     void *virtio_region;
>     target_phys_addr_t virtio_region_len;
> +    target_phys_addr_t virtio_region_start;
>     int i;
>
>     /* s390x ram size detection needs a 16bit multiplier + an increment. So
> @@ -188,7 +189,9 @@ static void s390_init(ram_addr_t my_ram_size,
>
>     /* clear virtio region */
>     virtio_region_len = my_ram_size - ram_size;
> -    virtio_region = cpu_physical_memory_map(ram_size, &virtio_region_len,
> true);
> +    virtio_region_start = ram_size;
> +    virtio_region = cpu_physical_memory_map(virtio_region_start,
> + &virtio_region_len, true);
>     memset(virtio_region, 0, virtio_region_len);
>     cpu_physical_memory_unmap(virtio_region, virtio_region_len, 1,
>                               virtio_region_len);

Yes, that looks OK to me.

-- PMM

Re: [Qemu-devel] [PATCH] s390x: initialize virtio dev region

[Andreas Färber]

Am 11.11.2011 18:40, schrieb Alexander Graf:
> On 11/11/2011 05:44 PM, Peter Maydell wrote:
>> On 11 November 2011 16:24, Alexander Graf<agraf@suse.de>  wrote:
>>> On 11/11/2011 05:11 PM, Peter Maydell wrote:
>>>> Yes, so that's option (2) and you need to be using a
>>>> target_phys_addr_t.
>>> But ram_size is ram_addr_t and is the ram size that I have available
>>> to use,
>>> so it's exactly the address that I want. I don't see your point.
>>> Should go
>>> jump through random useless hoops of doing
>>>
>>>   target_phys_addr_t ram_end = ram_size;
>>>
>>> just because there are some subtile semantic differences between the two
>>> variables? They're integers at the end of the day. Both of them.
>> Because handing a ram_addr_t to something that wants a target_phys_addr_t
>> is a red flag that something might be wrong. (As is handing a variable
>> called _size to something that wants an address.)
>>
>> Force in Newtons and Force in pound-feet are also both just integers
>> at the end of the day, but that doesn't make them interchangeable:
>> http://en.wikipedia.org/wiki/Mars_Climate_Orbiter#Communications_loss
> 
> Or actually rather this one:
> 
> 
> Alex
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index d936809..61b67e8 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -167,6 +167,7 @@ static void s390_init(ram_addr_t my_ram_size,
>      uint8_t *storage_keys;
>      void *virtio_region;
>      target_phys_addr_t virtio_region_len;
> +    target_phys_addr_t virtio_region_start;
>      int i;
> 
>      /* s390x ram size detection needs a 16bit multiplier + an
> increment. So
> @@ -188,7 +189,9 @@ static void s390_init(ram_addr_t my_ram_size,
> 
>      /* clear virtio region */
>      virtio_region_len = my_ram_size - ram_size;
> -    virtio_region = cpu_physical_memory_map(ram_size,
> &virtio_region_len, true);
> +    virtio_region_start = ram_size;
> +    virtio_region = cpu_physical_memory_map(virtio_region_start,
> + &virtio_region_len, true);

Yes, ..._start reads better than ..._size there. Thanks.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg