Re: [Xen-devel] [PATCH v3] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching

["Gonglei (Arei)" <arei.gonglei@xxxxxxxxxx>]

Hi,

> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
> Sent: Thursday, May 08, 2014 7:22 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxx; JBeulich@xxxxxxxx;
> stefano.stabellini@xxxxxxxxxxxxx; fabio.fantoni@xxxxxxx;
> anthony.perard@xxxxxxxxxx; 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, 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?
> 
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 <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.
> 
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 >$@.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?
> 
Agreed.

> > +   $(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...
> 
Ok, we change _aml_adr_dword_len as array length.

> > +   $(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)
> 
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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel