| Message ID | 1376664409-2695-1-git-send-email-anthony.perard@citrix.com |
|---|---|
| State | New |
| Headers | show |
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; >
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
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.
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;
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(-)