The only non-init item was the space reserved for the initial tables, but we can as well dynamically allocate that array. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/acpi/Makefile +++ b/xen/drivers/acpi/Makefile @@ -2,7 +2,7 @@ subdir-y += tables subdir-y += utilities subdir-$(x86) += apei -obj-y += tables.o +obj-bin-y += tables.init.o obj-y += numa.o obj-y += osl.o obj-y += pmstat.o --- a/xen/drivers/acpi/tables.c +++ b/xen/drivers/acpi/tables.c @@ -25,6 +25,8 @@ #include <xen/init.h> #include <xen/kernel.h> +#include <xen/mm.h> +#include <xen/pfn.h> #include <xen/smp.h> #include <xen/string.h> #include <xen/types.h> @@ -41,8 +43,6 @@ mps_inti_flags_polarity[] = { "dfl", "hi static const char *__initdata mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" }; -static struct acpi_table_desc initial_tables[ACPI_MAX_TABLES]; - static int acpi_apic_instance __initdata; void __init acpi_table_print_madt_entry(struct acpi_subtable_header *header) @@ -324,6 +324,11 @@ static void __init check_multiple_madt(v int __init acpi_table_init(void) { + struct acpi_table_desc *initial_tables + mfn_to_virt(alloc_boot_pages(PFN_UP(ACPI_MAX_TABLES * + sizeof(*initial_tables)), + 1)); + acpi_initialize_tables(initial_tables, ACPI_MAX_TABLES, 0); check_multiple_madt(); return 0; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On 18/09/2012 16:26, "Jan Beulich" <JBeulich@suse.com> wrote:> The only non-init item was the space reserved for the initial tables, > but we can as well dynamically allocate that array. > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Really worthwhile, versus having to round up the initial_tables[] allocation to a whole number of pages?> --- a/xen/drivers/acpi/Makefile > +++ b/xen/drivers/acpi/Makefile > @@ -2,7 +2,7 @@ subdir-y += tables > subdir-y += utilities > subdir-$(x86) += apei > > -obj-y += tables.o > +obj-bin-y += tables.init.o > obj-y += numa.o > obj-y += osl.o > obj-y += pmstat.o > --- a/xen/drivers/acpi/tables.c > +++ b/xen/drivers/acpi/tables.c > @@ -25,6 +25,8 @@ > > #include <xen/init.h> > #include <xen/kernel.h> > +#include <xen/mm.h> > +#include <xen/pfn.h> > #include <xen/smp.h> > #include <xen/string.h> > #include <xen/types.h> > @@ -41,8 +43,6 @@ mps_inti_flags_polarity[] = { "dfl", "hi > static const char *__initdata > mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" }; > > -static struct acpi_table_desc initial_tables[ACPI_MAX_TABLES]; > - > static int acpi_apic_instance __initdata; > > void __init acpi_table_print_madt_entry(struct acpi_subtable_header *header) > @@ -324,6 +324,11 @@ static void __init check_multiple_madt(v > > int __init acpi_table_init(void) > { > + struct acpi_table_desc *initial_tables > + mfn_to_virt(alloc_boot_pages(PFN_UP(ACPI_MAX_TABLES * > + sizeof(*initial_tables)), > + 1)); > + > acpi_initialize_tables(initial_tables, ACPI_MAX_TABLES, 0); > check_multiple_madt(); > return 0; > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
>>> On 18.09.12 at 17:42, Keir Fraser <keir.xen@gmail.com> wrote: > On 18/09/2012 16:26, "Jan Beulich" <JBeulich@suse.com> wrote: > >> The only non-init item was the space reserved for the initial tables, >> but we can as well dynamically allocate that array. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Really worthwhile, versus having to round up the initial_tables[] allocation > to a whole number of pages?It was exactly one page in size already. Jan
On 19/09/2012 08:29, "Jan Beulich" <JBeulich@suse.com> wrote:>>>> On 18.09.12 at 17:42, Keir Fraser <keir.xen@gmail.com> wrote: >> On 18/09/2012 16:26, "Jan Beulich" <JBeulich@suse.com> wrote: >> >>> The only non-init item was the space reserved for the initial tables, >>> but we can as well dynamically allocate that array. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> Really worthwhile, versus having to round up the initial_tables[] allocation >> to a whole number of pages? > > It was exactly one page in size already.Well, still I''m not sure about the style of converting static data to heap data, just to be able to mark a whole unit init-only. If everything else is already marked init inside that file already, is there a win? -- Keir> Jan >
>>> On 19.09.12 at 09:56, Keir Fraser <keir.xen@gmail.com> wrote: > On 19/09/2012 08:29, "Jan Beulich" <JBeulich@suse.com> wrote: > >>>>> On 18.09.12 at 17:42, Keir Fraser <keir.xen@gmail.com> wrote: >>> On 18/09/2012 16:26, "Jan Beulich" <JBeulich@suse.com> wrote: >>> >>>> The only non-init item was the space reserved for the initial tables, >>>> but we can as well dynamically allocate that array. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> Really worthwhile, versus having to round up the initial_tables[] allocation >>> to a whole number of pages? >> >> It was exactly one page in size already. > > Well, still I''m not sure about the style of converting static data to heap > data, just to be able to mark a whole unit init-only. If everything else is > already marked init inside that file already, is there a win?Yes - there a little of 1k of string literals, and there''s no way (other than making them variables to be able to apply section attributes) to move them into .init.* - that''s the purpose of the other instances where the same post processing is being applied. Of course, if you''re only against the dynamic allocation, moving the array elsewhere would be another option (but would require making the symbol global, whereas here the symbol goes away altogether from the symbol tables). Yet another option would be to do the dynamic allocation where it was actually intended to be done, passing NULL here. The resizing there isn''t being made use of anyway (and wouldn''t work afaict), so we could as well do away with it and replace it by the simple allocation needed here (or simply fix it, considering that we might need it at some point). Jan
On 19/09/2012 09:58, "Jan Beulich" <JBeulich@suse.com> wrote:> Of course, if you''re only against the dynamic allocation, moving > the array elsewhere would be another option (but would require > making the symbol global, whereas here the symbol goes away > altogether from the symbol tables). > > Yet another option would be to do the dynamic allocation where > it was actually intended to be done, passing NULL here. The > resizing there isn''t being made use of anyway (and wouldn''t > work afaict), so we could as well do away with it and replace it > by the simple allocation needed here (or simply fix it, > considering that we might need it at some point).I''m not wild about any of the options really. Perhaps passing NULL would be best. Still, overall, I''m not *that* bothered. You can have my Ack for the original patch: Acked-by: Keir Fraser <keir@xen.org> -- Keir
>>> On 19.09.12 at 13:40, Keir Fraser <keir.xen@gmail.com> wrote: > On 19/09/2012 09:58, "Jan Beulich" <JBeulich@suse.com> wrote: > >> Of course, if you''re only against the dynamic allocation, moving >> the array elsewhere would be another option (but would require >> making the symbol global, whereas here the symbol goes away >> altogether from the symbol tables). >> >> Yet another option would be to do the dynamic allocation where >> it was actually intended to be done, passing NULL here. The >> resizing there isn''t being made use of anyway (and wouldn''t >> work afaict), so we could as well do away with it and replace it >> by the simple allocation needed here (or simply fix it, >> considering that we might need it at some point). > > I''m not wild about any of the options really. Perhaps passing NULL would be > best. Still, overall, I''m not *that* bothered. You can have my Ack for the > original patch: > Acked-by: Keir Fraser <keir@xen.org>As you were not really happy with it, and as I (also already in the past) wondered about when the broken re-allocation there would hit us, I decided to produce a v2, fixing and using the re-allocation mechanism instead. Jan
On 19/09/2012 13:57, "Jan Beulich" <JBeulich@suse.com> wrote:>>>> On 19.09.12 at 13:40, Keir Fraser <keir.xen@gmail.com> wrote: >> On 19/09/2012 09:58, "Jan Beulich" <JBeulich@suse.com> wrote: >> >>> Of course, if you''re only against the dynamic allocation, moving >>> the array elsewhere would be another option (but would require >>> making the symbol global, whereas here the symbol goes away >>> altogether from the symbol tables). >>> >>> Yet another option would be to do the dynamic allocation where >>> it was actually intended to be done, passing NULL here. The >>> resizing there isn''t being made use of anyway (and wouldn''t >>> work afaict), so we could as well do away with it and replace it >>> by the simple allocation needed here (or simply fix it, >>> considering that we might need it at some point). >> >> I''m not wild about any of the options really. Perhaps passing NULL would be >> best. Still, overall, I''m not *that* bothered. You can have my Ack for the >> original patch: >> Acked-by: Keir Fraser <keir@xen.org> > > As you were not really happy with it, and as I (also already in > the past) wondered about when the broken re-allocation there > would hit us, I decided to produce a v2, fixing and using the > re-allocation mechanism instead.I like that more, yes. Thanks!> Jan >