[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0
From: |
Jan Beulich |
Subject: |
Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching |
Date: |
Tue, 22 Oct 2013 09:06:08 +0100 |
>>> On 22.10.13 at 06:08, "Gonglei (Arei)" <address@hidden> wrote:
> Hi, guys. The new patch has been modified based on the principles you
> suggested, thank you so much.
> Last time I test the patch based on the codes of 4.3.0.
> This time, I found that the system based on the codes of trunk causes the VM
> reboot again and again, which I have not found out the reason.
> So i can not test the patch based on the codes of trunk (details in
> EJ0_ACPI_PCI_Hotplug.patch)..
I'm afraid we will need you to figure out that problem first, and
then do the verification on -unstable. Even if the code shouldn't
be that different from 4.3, we still don't want to apply completely
untested stuff.
> --- a/tools/firmware/hvmloader/acpi/Makefile
> +++ b/tools/firmware/hvmloader/acpi/Makefile
> @@ -24,30 +24,46 @@ OBJS = $(patsubst %.c,%.o,$(C_SRC))
> CFLAGS += $(CFLAGS_xeninclude)
>
> vpath iasl $(PATH)
> +vpath python $(PATH)
Iirc this ought to be $(PYTHON) (also further down).
> +
> +.DELETE_ON_ERROR:$(filter dsdt_%.c,$(C_SRC))
Missing blank after ":".
> dsdt_anycpu_qemu_xen.asl: dsdt.asl mk_dsdt
> - awk 'NR > 1 {print s} {s=$$0}' $< > $@
> - ./mk_dsdt --dm-version qemu-xen >> $@
> -
> + awk 'NR > 1 {print s} {s=$$0}' $< > address@hidden
> + sed -i 's/AmlCode/dsdt_anycpu_qemu_xen/g' address@hidden
> + ./mk_dsdt --dm-version qemu-xen >> address@hidden
> + sed -i 's/aml_ej0_name/dsdt_anycpu_qemu_xen_aml_ej0_name/g'
> address@hidden
> + sed -i 's/aml_adr_dword/dsdt_anycpu_qemu_xen_aml_adr_dword/g'
> address@hidden
> + $(call move-if-changed,address@hidden $@)
> +
Line containing just a tab.
> # NB. awk invocation is a portable alternative to 'head -n -1'
> dsdt_%cpu.asl: dsdt.asl mk_dsdt
> - awk 'NR > 1 {print s} {s=$$0}' $< > $@
> - ./mk_dsdt --maxcpu $* >> $@
> + awk 'NR > 1 {print s} {s=$$0}' $< > address@hidden
> + sed -i 's/AmlCode/dsdt_$*cpu/g' address@hidden
> + ./mk_dsdt --maxcpu $* >> address@hidden
> + $(call move-if-changed,address@hidden $@)
>
> -$(filter dsdt_%.c,$(C_SRC)): %.c: iasl %.asl
> - iasl -vs -p $* -tc $*.asl
> - sed -e 's/AmlCode/$*/g' $*.hex >$@
> - echo "int $*_len=sizeof($*);" >>$@
> - rm -f $*.aml $*.hex
> +$(filter dsdt_%.c,$(C_SRC)): %.c: iasl python %.asl
As you need to touch this anyway, please adjust it to match
common style: The input file should come first in the list of
dependencies, allowing you ...
> + cpp -P $*.asl > $*.asl.i.orig
... use $< here (and further down where applicable) in favor of $*.
> + python ../tools/acpi_extract_preprocess.py $*.asl.i.orig > $*.asl.i
Please be consistent in whether you put a blank after ">" and ">>".
> + iasl -vs -l -tc -p $* $*.asl.i
> + python ../tools/acpi_extract.py $*.lst > address@hidden
> + echo "int $*_len=sizeof($*);" >>address@hidden
> + if grep -q "$*_aml_ej0_name" address@hidden; then echo "int
> $*_aml_ej0_name_len=sizeof($*_aml_ej0_name);" >>address@hidden; fi
> + if grep -q "$*_aml_adr_dword" address@hidden; then echo "int
> $*_aml_adr_dword_len=sizeof($*_aml_adr_dword);" >>address@hidden;fi
Missing blank before "fi".
> +python:
> + @echo
> + @echo "python is needed"
> + @echo
> + @exit 1
What is this good for? For one, the tools build already requires
Python. And then such a dependency would belong into the
configure scripts, not here.
> --- a/tools/firmware/hvmloader/acpi/build.c
> +++ b/tools/firmware/hvmloader/acpi/build.c
There are various coding style issues in the changes to this file.
Please make sure you match the style found elsewhere in here.
> @@ -30,6 +30,9 @@
> #define align16(sz) (((sz) + 15) & ~15)
> #define fixed_strcpy(d, s) strncpy((d), (s), sizeof(d))
>
> +
> +#define PCI_RMV_BASE 0xae0c
> +
> extern struct acpi_20_rsdp Rsdp;
> extern struct acpi_20_rsdt Rsdt;
> extern struct acpi_20_xsdt Xsdt;
> @@ -404,6 +407,7 @@ void acpi_build_tables(struct acpi_config *config,
> unsigned int physical)
> unsigned char *dsdt;
> unsigned long secondary_tables[ACPI_MAX_SECONDARY_TABLES];
> int nr_secondaries, i;
> + unsigned int rmvc_pcrm = 0;
Pointless initializer. You'd be better off moving the declaration to
the first use point anyway (and then it can have a proper initializer
right away).
> @@ -438,9 +442,27 @@ void acpi_build_tables(struct acpi_config *config,
> unsigned int physical)
> dsdt = mem_alloc(config->dsdt_anycpu_len, 16);
> if (!dsdt) goto oom;
> memcpy(dsdt, config->dsdt_anycpu, config->dsdt_anycpu_len);
> - nr_processor_objects = HVM_MAX_VCPUS;
The movement to this assignment is clearly wrong, as it now
overwrites the different value stored when using the <= 15 CPUs
path.
This also raises the question of why you do the adjustment below
only in the >= 16 CPUs code path.
I also don't see how this adjustment is in line with mk_dsdt.c's
handling of the qemu-traditional case. Or is that building on
SeaBIOS only ever being used with upstream qemu?
> + if(config->aml_adr_dword_len && config->aml_ej0_name_len &&
> + (config->aml_adr_dword_len/sizeof(config->aml_adr_dword[0]) ==
> config->aml_ej0_name_len/sizeof(config->aml_ej0_name[0])))
> + {
> + rmvc_pcrm = inl(PCI_RMV_BASE);
> + printf("rmvc_pcrm is %x\n", rmvc_pcrm);
> + for(i = 0; i <
> config->aml_adr_dword_len/sizeof(config->aml_adr_dword[0]); i ++)
> + {
> + /* Slot is in byte 2 in _ADR */
> + unsigned char slot = dsdt[config->aml_adr_dword[i] + 2] &
> 0x1F;
> + /* Sanity check */
> + if (memcmp(dsdt + config->aml_ej0_name[i], "_EJ0", 4)) {
> + goto oom;
A memcmp() failure should result in a meaningful error message;
definitely not "out of memory".
> + }
> + if (!(rmvc_pcrm & (0x1 << slot))) {
> + memcpy(dsdt + config->aml_ej0_name[i], "EJ0_", 4);
> + }
> + }
> + }
> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -79,7 +79,11 @@ static void ovmf_acpi_build_tables(void)
> .dsdt_anycpu = dsdt_anycpu,
> .dsdt_anycpu_len = dsdt_anycpu_len,
> .dsdt_15cpu = NULL,
> - .dsdt_15cpu_len = 0
> + .dsdt_15cpu_len = 0,
> + .aml_ej0_name = NULL,
> + .aml_adr_dword = NULL,
> + .aml_ej0_name_len = 0,
> + .aml_adr_dword_len = 0,
I don't see why you're adding these.
> --- a/tools/firmware/hvmloader/rombios.c
> +++ b/tools/firmware/hvmloader/rombios.c
> @@ -179,6 +179,10 @@ static void rombios_acpi_build_tables(void)
> .dsdt_anycpu_len = dsdt_anycpu_len,
> .dsdt_15cpu = dsdt_15cpu,
> .dsdt_15cpu_len = dsdt_15cpu_len,
> + .aml_ej0_name = NULL,
> + .aml_adr_dword = NULL,
> + .aml_ej0_name_len = 0,
> + .aml_adr_dword_len = 0,
Same here.
Jan
- [Qemu-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Gonglei (Arei), 2013/10/16
- Re: [Qemu-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Fabio Fantoni, 2013/10/16
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Ian Campbell, 2013/10/16
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Jan Beulich, 2013/10/16
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Ian Campbell, 2013/10/16
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Markus Armbruster, 2013/10/16
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Ian Campbell, 2013/10/16
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Markus Armbruster, 2013/10/16
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Jan Beulich, 2013/10/16
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Gonglei (Arei), 2013/10/22
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching,
Jan Beulich <=
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Gonglei (Arei), 2013/10/24
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Fabio Fantoni, 2013/10/24
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Gonglei (Arei), 2013/10/24
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Jan Beulich, 2013/10/28
- Re: [Qemu-devel] [Xen-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Gonglei (Arei), 2013/10/28