|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching
0On Sun, 2014-05-04 at 17:25 +0800, arei.gonglei@xxxxxxxxxx wrote:
> From: Gaowei <gao.gaowei@xxxxxxxxxx>
>
> In Xen platform, after using upstream qemu, the all of pci devices will show
> hotplug in the windows guest. In this situation, the windows guest may occur
> blue screen when VM' user click the icon of VGA card for trying unplug VGA
> card.
> However, we don't hope VM's user can do such dangerous operation, and showing
> all pci devices inside the guest OS is unfriendly.
>
> This is done by runtime patching:
Which appears to involve an awful lot of jumping through hoops... Please
can you explain why it is necessary, as opposed to e.g. using a dynamic
set of SSDTs?
> - Rename _EJ0 methods for PCI slots in DSDT to EJ0_:note that this has the
> same checksum, but is ignored by OSPM.
> - At compile time, look for these methods in ASL source,find the matching
> AML,
> and store the offsets of these methods in a table named aml_ej0_data.
> - At run time, go over aml_ej0_data, check which slots not support hotplug
> and
> patch the ACPI table, replacing _EJ0 with EJ0_.
>
> Signed-off-by: Gaowei <gao.gaowei@xxxxxxxxxx>
> Signed-off-by: Gonglei <arei.gonglei@xxxxxxxxxx>
> ---
> v3->v2:
> reformat on the latest master
> v2->v1:
> rework by Jan Beulich's suggestion:
> - adjust the code style
>
> tools/firmware/hvmloader/acpi/Makefile | 44 ++-
> tools/firmware/hvmloader/acpi/acpi2_0.h | 4 +
> tools/firmware/hvmloader/acpi/build.c | 22 ++
> tools/firmware/hvmloader/acpi/dsdt.asl | 1 +
> tools/firmware/hvmloader/acpi/mk_dsdt.c | 2 +
> tools/firmware/hvmloader/ovmf.c | 6 +-
> tools/firmware/hvmloader/rombios.c | 4 +
> tools/firmware/hvmloader/seabios.c | 8 +
> tools/firmware/hvmloader/tools/acpi_extract.py | 308
> +++++++++++++++++++++
> .../hvmloader/tools/acpi_extract_preprocess.py | 41 +++
> 10 files changed, 428 insertions(+), 12 deletions(-)
> create mode 100644 tools/firmware/hvmloader/tools/acpi_extract.py
> create mode 100644 tools/firmware/hvmloader/tools/acpi_extract_preprocess.py
>
> diff --git a/tools/firmware/hvmloader/acpi/Makefile
> b/tools/firmware/hvmloader/acpi/Makefile
> index 2c50851..f79ecc3 100644
> --- 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)
I suppose this is trying to make things get rebuilt when the python
binary is upgraded, but since almost all the functionality is in library
and modules this doesn't seem like it will be very effective. (I suppose
it is for iasl which is a single binary, although even then its a bit
odd to special case iasl in this way out of all the tools used during
build)
Also please use $(PYTHON) to invoke the scripts.
> +.DELETE_ON_ERROR:$(filter dsdt_%.c,$(C_SRC))
Space after the : please.
> +
> all: acpi.a
>
> ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h: %.h: %.asl iasl
> iasl -vs -p $* -tc $<
> - sed -e 's/AmlCode/$*/g' $*.hex >$@
> + sed -e 's/AmlCode/$*/g' $*.hex >$@.tmp
> + $(call move-if-changed,$@.tmp $@)
> rm -f $*.hex $*.aml
>
> mk_dsdt: mk_dsdt.c
> $(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_xeninclude) -o $@ mk_dsdt.c
>
> 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}' $< > $@.tmp
> + sed -i 's/AmlCode/dsdt_anycpu_qemu_xen/g' $@.tmp
What is this sed doing?
> + ./mk_dsdt --dm-version qemu-xen >> $@.tmp
> + sed -i 's/aml_ej0_name/dsdt_anycpu_qemu_xen_aml_ej0_name/g' $@.tmp
> + sed -i 's/aml_adr_dword/dsdt_anycpu_qemu_xen_aml_adr_dword/g' $@.tmp
And these two?
Since $@.tmp is programatically generated by mk_dsdt can it not just Do
The Right Thing in the first place for all of these?
> + $(call move-if-changed,$@.tmp $@)
>
> # 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}' $< > $@.tmp
> + sed -i 's/AmlCode/dsdt_$*cpu/g' $@.tmp
> + ./mk_dsdt --maxcpu $* >> $@.tmp
> + $(call move-if-changed,$@.tmp $@)
>
> -$(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
> + cpp -P $*.asl > $*.asl.i.orig
> + python ../tools/acpi_extract_preprocess.py $*.asl.i.orig > $*.asl.i
> + iasl -vs -l -tc -p $* $*.asl.i
> + python ../tools/acpi_extract.py $*.lst > $@.tmp
> + echo "int $*_len=sizeof($*);" >>$@.tmp
> + if grep -q "$*_aml_ej0_name" $@.tmp; then echo "int
> $*_aml_ej0_name_len=sizeof($*_aml_ej0_name);" >>$@.tmp; fi
> + if grep -q "$*_aml_adr_dword" $@.tmp; then echo "int
> $*_aml_adr_dword_len=sizeof($*_aml_adr_dword);" >>$@.tmp;fi
Why can't these all just be generated at the same time as the array? Or
better yet, use an ARRAY_SIZE construct in the code. In fact the code
does "config->aml_ej0_name_len/sizeof(config->aml_ej0_name[0]);" which
is halfway between the two...
> + $(call move-if-changed,$@.tmp $@)
> + rm -f $*.aml $*.hex $*.asl.i.orig $*.asl.i $*.lst
>
> iasl:
> @echo
> @@ -57,6 +73,12 @@ iasl:
> @echo
> @exit 1
>
> +python:
> + @echo
> + @echo "python is needed"
> + @echo
> + @exit 1
This is checked elsewhere in the build system for sure. (The existing
iasl check is similarly pointless)
> diff --git a/tools/firmware/hvmloader/acpi/build.c
> b/tools/firmware/hvmloader/acpi/build.c
> index f1dd3f0..ccce420 100644
> --- a/tools/firmware/hvmloader/acpi/build.c
> +++ b/tools/firmware/hvmloader/acpi/build.c
> @@ -30,6 +30,8 @@
> #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 +406,8 @@ 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;
> + unsigned int len_aml_addr = 0, len_aml_ej0 = 0;
Please observe the existing indentation style.
> /* Allocate and initialise the acpi info area. */
> mem_hole_populate_ram(ACPI_INFO_PHYSICAL_ADDRESS >> PAGE_SHIFT, 1);
> @@ -440,6 +444,24 @@ void acpi_build_tables(struct acpi_config *config,
> unsigned int physical)
> memcpy(dsdt, config->dsdt_anycpu, config->dsdt_anycpu_len);
> nr_processor_objects = HVM_MAX_VCPUS;
> }
> + len_aml_addr =
> config->aml_adr_dword_len/sizeof(config->aml_adr_dword[0]);
> + len_aml_ej0 = config->aml_ej0_name_len/sizeof(config->aml_ej0_name[0]);
Can't config->aml_ej0_name_len just have the right units (array length,
not bytes)?
> + if (config->aml_adr_dword_len && config->aml_ej0_name_len &&
> (len_aml_addr == len_aml_ej0))
> + {
> + rmvc_pcrm = inl(PCI_RMV_BASE);
This is some qemu magic I suppose?
> + printf("rmvc_pcrm is %x\n", rmvc_pcrm);
Leftover debug?
> + for (i = 0; i < len_aml_addr; i++ ) {
CODING_STYLE calls for { on the next line and more spaces inside the
for()
> + /* Slot is in byte 2 in _ADR */
Indentation is wrong.
> + 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;
There is no out of memory here, so going to oom (which prints "out of
memory") is inappropriate.
CODING_STYLE does not require {}'s around single line statements.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |