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

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

[Anthony PERARD]

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

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

[Fabio Fantoni]

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;

Re: [PATCH for v1.6] pc: Fix initialization of the ram_memory variable.

[Fabio Fantoni]

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;

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

[Fabio Fantoni]

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;
>

Re: [PATCH for v1.6] pc: Fix initialization of the ram_memory variable.

[Fabio Fantoni]

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;
>

Re: [Qemu-devel] [PATCH for v1.6] pc: Fix initialization of the ram_memory variable.

[Peter Maydell]

On 16 August 2013 15:46, Anthony PERARD <anthony.perard@citrix.com> wrote:
> 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 ?

Yes, is this really the right fix? I raised this before 1.6 came
out (since clang complains about it):
http://lists.gnu.org/archive/html/qemu-devel/2013-07/msg05383.html
(though I see I got the sense of the conditional wrong in the
subject line of that mail)

and Stefano suggested the right fix was more complicated
than just passing NULL. Also IIRC if you follow the variable
down then it eventually gets actually used by code, so just
passing NULL sounds wrong.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for v1.6] pc: Fix initialization of the ram_memory variable.

[Peter Maydell]

On 16 August 2013 15:46, Anthony PERARD <anthony.perard@citrix.com> wrote:
> 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 ?

Yes, is this really the right fix? I raised this before 1.6 came
out (since clang complains about it):
http://lists.gnu.org/archive/html/qemu-devel/2013-07/msg05383.html
(though I see I got the sense of the conditional wrong in the
subject line of that mail)

and Stefano suggested the right fix was more complicated
than just passing NULL. Also IIRC if you follow the variable
down then it eventually gets actually used by code, so just
passing NULL sounds wrong.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for v1.6] pc: Fix initialization of the ram_memory variable.

[Paolo Bonzini]

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;
> 

Re: [PATCH for v1.6] pc: Fix initialization of the ram_memory variable.

[Paolo Bonzini]

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;
> 

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

[Fabio Fantoni]

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

Re: [PATCH for v1.6] pc: Fix initialization of the ram_memory variable.

[Fabio Fantoni]

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

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

[Stefano Stabellini]

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.

Re: [PATCH for v1.6] pc: Fix initialization of the ram_memory variable.

[Stefano Stabellini]

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.

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

[Stefano Stabellini]

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.

Re: [PATCH for v1.6] pc: Fix initialization of the ram_memory variable.

[Stefano Stabellini]

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.