[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] Hvmloader: Modify ACPI to only supply _EJ0 m
From: |
Gonglei (Arei) |
Subject: |
Re: [Qemu-devel] [PATCH v3] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching |
Date: |
Fri, 9 May 2014 08:38:48 +0000 |
Hi,
> -----Original Message-----
> From: Ian Campbell [mailto:address@hidden
> Sent: Thursday, May 08, 2014 7:22 PM
> To: Gonglei (Arei)
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; Huangweidong (C); Hanweidong (Randy); Gaowei
> (UVP)
> Subject: Re: [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, address@hidden wrote:
> > From: Gaowei <address@hidden>
> >
> > 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?
>
Ok, we will delete some pointless description.
> > - 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 <address@hidden>
> > Signed-off-by: Gonglei <address@hidden>
> > ---
> > 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.
>
Accept.
> > +.DELETE_ON_ERROR:$(filter dsdt_%.c,$(C_SRC))
>
> Space after the : please.
>
Agreed.
> > +
> > 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 >address@hidden
> > + $(call move-if-changed,address@hidden $@)
> > 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}' $< > address@hidden
> > + sed -i 's/AmlCode/dsdt_anycpu_qemu_xen/g' address@hidden
>
> What is this sed doing?
>
> > + ./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
>
> And these two?
>
> Since address@hidden is programatically generated by mk_dsdt can it not just
> Do
> The Right Thing in the first place for all of these?
>
Agreed.
> > + $(call move-if-changed,address@hidden $@)
> >
> > # 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
> > + 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 > 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
>
> 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...
>
Ok, we change _aml_adr_dword_len as array length.
> > + $(call move-if-changed,address@hidden $@)
> > + 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)
>
Agreed.
> > 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?
>
Yep.
> > + printf("rmvc_pcrm is %x\n", rmvc_pcrm);
>
> Leftover debug?
>
Deleted it.
> > + 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.
>
Yep.
> > + 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.
>
Accepted.
> CODING_STYLE does not require {}'s around single line statements.
>
Ok.
Best regards,
-Gonglei
- Re: [Qemu-devel] [Xen-devel] [PATCH v3] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, (continued)
Re: [Qemu-devel] [PATCH v3] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching, Ian Campbell, 2014/05/08
- Re: [Qemu-devel] [PATCH v3] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching,
Gonglei (Arei) <=