Xen devel - Aug 2013 - [PATCH for v1.6] pc: Fix initialization of the ram_memory variable.

In some cases (Xen), it will not be initialized before to be used. This
leads to segv.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

---

It works with this simple initialization to NULL, but would it be
necessary (or better) to assign a proper value to this variables ?
---
 hw/i386/pc_piix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6e1e654..596d433 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -86,7 +86,7 @@ static void pc_init1(MemoryRegion *system_memory,
     BusState *idebus[MAX_IDE_BUS];
     ISADevice *rtc_state;
     ISADevice *floppy;
-    MemoryRegion *ram_memory;
+    MemoryRegion *ram_memory = NULL;
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
     DeviceState *icc_bridge;
-- 
Anthony PERARD

Il 16/08/2013 16:46, Anthony PERARD ha scritto:
> In some cases (Xen), it will not be initialized before to be used. This
> leads to segv.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Tested-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
This patch have solved the critical regression of all hvm domUs that 
prevented domUs starting with qemu 1.6.
>
> ---
>
> It works with this simple initialization to NULL, but would it be
> necessary (or better) to assign a proper value to this variables ?
> ---
>   hw/i386/pc_piix.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 6e1e654..596d433 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -86,7 +86,7 @@ static void pc_init1(MemoryRegion *system_memory,
>       BusState *idebus[MAX_IDE_BUS];
>       ISADevice *rtc_state;
>       ISADevice *floppy;
> -    MemoryRegion *ram_memory;
> +    MemoryRegion *ram_memory = NULL;
>       MemoryRegion *pci_memory;
>       MemoryRegion *rom_memory;
>       DeviceState *icc_bridge;

Il 26/08/2013 17:00, Fabio Fantoni ha scritto:
> Il 16/08/2013 16:46, Anthony PERARD ha scritto:
>> In some cases (Xen), it will not be initialized before to be used. This
>> leads to segv.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>
> Tested-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> This patch have solved the critical regression of all hvm domUs that 
> prevented domUs starting with qemu 1.6.
>
I not see this patch on git for now, I think is urgent apply this patch 
or solve the problem in other way because qemu 1.6 is unusable at all 
for hvm domUs on xen and 1.5.x doesn''t works with hvm domUs with ram
>
2gb, with qemu 1.6 the regression of > 2 gb ram is solved but without 
this patch doesn''t works at all.
Thanks for any reply and sorry for my bad english.
>>
>> ---
>>
>> It works with this simple initialization to NULL, but would it be
>> necessary (or better) to assign a proper value to this variables ?
>> ---
>>   hw/i386/pc_piix.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 6e1e654..596d433 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -86,7 +86,7 @@ static void pc_init1(MemoryRegion *system_memory,
>>       BusState *idebus[MAX_IDE_BUS];
>>       ISADevice *rtc_state;
>>       ISADevice *floppy;
>> -    MemoryRegion *ram_memory;
>> +    MemoryRegion *ram_memory = NULL;
>>       MemoryRegion *pci_memory;
>>       MemoryRegion *rom_memory;
>>       DeviceState *icc_bridge;
>

Il 16/08/2013 16:46, Anthony PERARD ha scritto:
> In some cases (Xen), it will not be initialized before to be used. This
> leads to segv.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> ---
> 
> It works with this simple initialization to NULL, but would it be
> necessary (or better) to assign a proper value to this variables ?
Yeah, the right value of this variable comes from xen_ram_init (called
by xen_hvm_init).

Paolo
> ---
>  hw/i386/pc_piix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 6e1e654..596d433 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -86,7 +86,7 @@ static void pc_init1(MemoryRegion *system_memory,
>      BusState *idebus[MAX_IDE_BUS];
>      ISADevice *rtc_state;
>      ISADevice *floppy;
> -    MemoryRegion *ram_memory;
> +    MemoryRegion *ram_memory = NULL;
>      MemoryRegion *pci_memory;
>      MemoryRegion *rom_memory;
>      DeviceState *icc_bridge;
>

Il 29/08/2013 11:00, Paolo Bonzini ha scritto:
> Il 16/08/2013 16:46, Anthony PERARD ha scritto:
>> In some cases (Xen), it will not be initialized before to be used. This
>> leads to segv.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>
>> ---
>>
>> It works with this simple initialization to NULL, but would it be
>> necessary (or better) to assign a proper value to this variables ?
> Yeah, the right value of this variable comes from xen_ram_init (called
> by xen_hvm_init).
>
> Paolo
I done a fast search of patch between 1.5 and 1.6 about memory on xen 
files and I found this patch:

Commit:2c9b15cab12c21e32dffb67c5e18f3dc407ca224
* memory: add owner argument to initialization functions

I not understand if can be this that have introduced an unexpected event 
with xen and how to solve correctly.

Thanks for any reply and sorry for my bad english.
>
>> ---
>>   hw/i386/pc_piix.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 6e1e654..596d433 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -86,7 +86,7 @@ static void pc_init1(MemoryRegion *system_memory,
>>       BusState *idebus[MAX_IDE_BUS];
>>       ISADevice *rtc_state;
>>       ISADevice *floppy;
>> -    MemoryRegion *ram_memory;
>> +    MemoryRegion *ram_memory = NULL;
>>       MemoryRegion *pci_memory;
>>       MemoryRegion *rom_memory;
>>       DeviceState *icc_bridge;
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Thu, 29 Aug 2013, Fabio Fantoni wrote:
> Il 26/08/2013 17:00, Fabio Fantoni ha scritto:
> > Il 16/08/2013 16:46, Anthony PERARD ha scritto:
> > > In some cases (Xen), it will not be initialized before to be
used. This
> > > leads to segv.
> > > 
> > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > 
> > Tested-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> > This patch have solved the critical regression of all hvm domUs that
> > prevented domUs starting with qemu 1.6.
> > 
> 
> I not see this patch on git for now, I think is urgent apply this patch or
> solve the problem in other way because qemu 1.6 is unusable at all for hvm
> domUs on xen and 1.5.x doesn''t works with hvm domUs with ram >
2gb, with qemu
> 1.6 the regression of > 2 gb ram is solved but without this patch
doesn''t
> works at all.
> Thanks for any reply and sorry for my bad english.
Sorry, you are right. I know about this patch, it''s in my queue. I
haven''t had the time to send it upstream yet.

On Thu, 29 Aug 2013, Stefano Stabellini wrote:
> On Thu, 29 Aug 2013, Fabio Fantoni wrote:
> > Il 26/08/2013 17:00, Fabio Fantoni ha scritto:
> > > Il 16/08/2013 16:46, Anthony PERARD ha scritto:
> > > > In some cases (Xen), it will not be initialized before to be
used. This
> > > > leads to segv.
> > > > 
> > > > Signed-off-by: Anthony PERARD
<anthony.perard@citrix.com>
> > > 
> > > Tested-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> > > This patch have solved the critical regression of all hvm domUs
that
> > > prevented domUs starting with qemu 1.6.
> > > 
> > 
> > I not see this patch on git for now, I think is urgent apply this
patch or
> > solve the problem in other way because qemu 1.6 is unusable at all for
hvm
> > domUs on xen and 1.5.x doesn''t works with hvm domUs with ram
> 2gb, with qemu
> > 1.6 the regression of > 2 gb ram is solved but without this patch
doesn''t
> > works at all.
> > Thanks for any reply and sorry for my bad english.
> 
> Sorry, you are right. I know about this patch, it''s in my queue. I
> haven''t had the time to send it upstream yet.
And by that, I mean I haven''t looked at it yet, even though I know it
solves an outstanding issue. It might not be the right fix, as Peter
suggested.