| Message ID | 1320887981-2099-1-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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
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);
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(-)