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

["Michael S. Tsirkin" <mst@xxxxxxxxxx>]

On Thu, May 08, 2014 at 07:29:15AM +0000, Gonglei (Arei) wrote:
> Hi, all
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:mst@xxxxxxxxxx]
> > Sent: Thursday, May 08, 2014 4:15 AM
> > To: Konrad Rzeszutek Wilk
> > Cc: Gonglei (Arei); qemu-devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> > Huangweidong (C); Ian.Campbell@xxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> > Hanweidong (Randy); fabio.fantoni@xxxxxxx; JBeulich@xxxxxxxx;
> > anthony.perard@xxxxxxxxxx; Gaowei (UVP)
> > Subject: Re: [Xen-devel] [PATCH v3] Hvmloader: Modify ACPI to only supply 
> > _EJ0
> > methods for PCIslots that support hotplug by runtime patching
> > 
> > On Wed, May 07, 2014 at 02:48:48PM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Sun, May 04, 2014 at 05:25:15PM +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:
> > > >   - 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_.
> > >
> > > How does this work with traditional QEMU ? Is this only when running with
> > SeaBIOS and upstream QEMU?
> > >
> The traditional QEMU is unaffected and works fine as normal. Yes, it is only 
> useful
> With seabios and upstream QEMU.
> 
> > > >
> > > > 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)
> > > > +
> > > > +.DELETE_ON_ERROR:$(filter dsdt_%.c,$(C_SRC))
> > > > +
> > > >  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
> > > > +       ./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
> > > > +       $(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
> > > > +       $(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
> > > > +
> > > >  build.o: ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h
> > > >
> > > >  acpi.a: $(OBJS)
> > > > @@ -64,7 +86,7 @@ acpi.a: $(OBJS)
> > > >
> > > >  clean:
> > > >         rm -rf *.a *.o $(IASL_VER) $(IASL_VER).tar.gz $(DEPS)
> > > > -       rm -rf ssdt_*.h dsdt*.c *~ *.aml *.hex mk_dsdt dsdt_*.asl
> > > > +       rm -rf ssdt_*.h dsdt*.c *~ *.aml *.hex mk_dsdt dsdt_*.asl 
> > > > *.asl.i
> > *.asl.i.orig *.lst *.tmp
> > > >
> > > >  install: all
> > > >
> > > > diff --git a/tools/firmware/hvmloader/acpi/acpi2_0.h
> > b/tools/firmware/hvmloader/acpi/acpi2_0.h
> > > > index 7b22d80..4ba3957 100644
> > > > --- a/tools/firmware/hvmloader/acpi/acpi2_0.h
> > > > +++ b/tools/firmware/hvmloader/acpi/acpi2_0.h
> > > > @@ -396,6 +396,10 @@ struct acpi_config {
> > > >      int dsdt_anycpu_len;
> > > >      unsigned char *dsdt_15cpu;
> > > >      int dsdt_15cpu_len;
> > > > +    unsigned short *aml_ej0_name;
> > > > +    unsigned short *aml_adr_dword;
> > > > +    int aml_ej0_name_len;
> > > > +    int aml_adr_dword_len;
> > > >  };
> > > >
> > > >  void acpi_build_tables(struct acpi_config *config, unsigned int 
> > > > physical);
> > > > 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;
> > > >
> > > >      /* 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]);
> > > > +    if (config->aml_adr_dword_len && config->aml_ej0_name_len &&
> > (len_aml_addr == len_aml_ej0))
> > > > +    {
> > > > +        rmvc_pcrm = inl(PCI_RMV_BASE);
> > >
> > >
> > > So .. what is at this special 0xae0c address?
> > >
> > > Is there some code in QEMU that needs this treatment as well?
> > >
> Yep, the 0xae0c is defined in upstream QEMU, which show the hotplugging 
> ability 
> of PCI devices. 
> 
> Including other special address about acpi hotplug defined in hw/acpi/pcihp.c:
> 
> #define ACPI_PCI_HOTPLUG_STATUS 2
> #define ACPI_PCIHP_ADDR 0xae00
> #define ACPI_PCIHP_SIZE 0x0014
> #define ACPI_PCIHP_LEGACY_SIZE 0x000f
> #define PCI_UP_BASE 0x0000
> #define PCI_DOWN_BASE 0x0004
> #define PCI_EJ_BASE 0x0008
> #define PCI_RMV_BASE 0x000c
> #define PCI_SEL_BASE 0x0010
> 
> > > > +        printf("rmvc_pcrm is %x\n", rmvc_pcrm);
> > > > +        for (i = 0;  i < len_aml_addr; 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;
> > > > +            }
> > > > +            if (!(rmvc_pcrm & (0x1 << slot))) {
> > > > +                memcpy(dsdt + config->aml_ej0_name[i], "EJ0_", 4);
> > > > +            }
> > > > +        }
> > > > +    }
> > > >
> > > >      /*
> > > >       * N.B. ACPI 1.0 operating systems may not handle FADT with
> > revision 2
> > > > diff --git a/tools/firmware/hvmloader/acpi/dsdt.asl
> > b/tools/firmware/hvmloader/acpi/dsdt.asl
> > > > index 247a8ad..1e7695b 100644
> > > > --- a/tools/firmware/hvmloader/acpi/dsdt.asl
> > > > +++ b/tools/firmware/hvmloader/acpi/dsdt.asl
> > > > @@ -16,6 +16,7 @@
> > > >   * this program; if not, write to the Free Software Foundation, Inc., 
> > > > 59
> > Temple
> > > >   * Place - Suite 330, Boston, MA 02111-1307 USA.
> > > >   */
> > > > +ACPI_EXTRACT_ALL_CODE AmlCode
> > > >
> > > >  DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", "HVM", 0)
> > > >  {
> > > > diff --git a/tools/firmware/hvmloader/acpi/mk_dsdt.c
> > b/tools/firmware/hvmloader/acpi/mk_dsdt.c
> > > > index a4b693b..555e062 100644
> > > > --- a/tools/firmware/hvmloader/acpi/mk_dsdt.c
> > > > +++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c
> > > > @@ -372,7 +372,9 @@ int main(int argc, char **argv)
> > > >          /* hotplug_slot */
> > > >          for (slot = 1; slot <= 31; slot++) {
> > > >              push_block("Device", "S%i", slot); {
> > > > +                printf("ACPI_EXTRACT_NAME_DWORD_CONST
> > aml_adr_dword\n");
> > > >                  stmt("Name", "_ADR, %#06x0000", slot);
> > > > +                printf("ACPI_EXTRACT_METHOD_STRING
> > aml_ej0_name\n");
> > > >                  push_block("Method", "_EJ0,1"); {
> > > >                      stmt("Store", "ShiftLeft(1, %#06x), B0EJ", slot);
> > > >                      stmt("Return", "0x0");
> > > > diff --git a/tools/firmware/hvmloader/ovmf.c
> > b/tools/firmware/hvmloader/ovmf.c
> > > > index 28dd7bc..ea11564 100644
> > > > --- a/tools/firmware/hvmloader/ovmf.c
> > > > +++ b/tools/firmware/hvmloader/ovmf.c
> > > > @@ -123,7 +123,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,
> > > >      };
> > > >
> > > >      acpi_build_tables(&config, ACPI_PHYSICAL_ADDRESS);
> > > > diff --git a/tools/firmware/hvmloader/rombios.c
> > b/tools/firmware/hvmloader/rombios.c
> > > > index 810bd24..803c9fa 100644
> > > > --- 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,
> > > >      };
> > > >
> > > >      acpi_build_tables(&config, ACPI_PHYSICAL_ADDRESS);
> > > > diff --git a/tools/firmware/hvmloader/seabios.c
> > b/tools/firmware/hvmloader/seabios.c
> > > > index dd7dfbe..ca01d27 100644
> > > > --- a/tools/firmware/hvmloader/seabios.c
> > > > +++ b/tools/firmware/hvmloader/seabios.c
> > > > @@ -33,6 +33,10 @@
> > > >
> > > >  extern unsigned char dsdt_anycpu_qemu_xen[];
> > > >  extern int dsdt_anycpu_qemu_xen_len;
> > > > +extern unsigned short dsdt_anycpu_qemu_xen_aml_ej0_name[];
> > > > +extern unsigned short dsdt_anycpu_qemu_xen_aml_adr_dword[];
> > > > +extern int dsdt_anycpu_qemu_xen_aml_ej0_name_len;
> > > > +extern int dsdt_anycpu_qemu_xen_aml_adr_dword_len;
> > > >
> > > >  struct seabios_info {
> > > >      char signature[14]; /* XenHVMSeaBIOS\0 */
> > > > @@ -99,6 +103,10 @@ static void seabios_acpi_build_tables(void)
> > > >          .dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len,
> > > >          .dsdt_15cpu = NULL,
> > > >          .dsdt_15cpu_len = 0,
> > > > +        .aml_ej0_name = dsdt_anycpu_qemu_xen_aml_ej0_name,
> > > > +        .aml_adr_dword = dsdt_anycpu_qemu_xen_aml_adr_dword,
> > > > +        .aml_ej0_name_len =
> > dsdt_anycpu_qemu_xen_aml_ej0_name_len,
> > > > +        .aml_adr_dword_len =
> > dsdt_anycpu_qemu_xen_aml_adr_dword_len,
> > > >      };
> > > >
> > > >      acpi_build_tables(&config, rsdp);
> > > > diff --git a/tools/firmware/hvmloader/tools/acpi_extract.py
> > b/tools/firmware/hvmloader/tools/acpi_extract.py
> > > > new file mode 100644
> > > > index 0000000..ccec089
> > > > --- /dev/null
> > > > +++ b/tools/firmware/hvmloader/tools/acpi_extract.py
> > > > @@ -0,0 +1,308 @@
> > > > +#!/usr/bin/python
> > > > +# Copyright (C) 2011 Red Hat, Inc., Michael S. Tsirkin <mst@xxxxxxxxxx>
> > > > +#
> > > > +# This file may be distributed under the terms of the GNU GPLv3 
> > > > license.
> > >
> > > Well, Xen code is under GPLv2 so I don't think you can do this?
> > >
> > > Unless Michael is willing to release it under a different licence?
> > 
> > I'd be fine with "GPLv2 or later" so code can flow back to seabios
> > as well - but you will have to track down everyone else who
> > changed this file since I wrote it and get their permission.
> > 
> Thanks, Michael. I search the git log of above two python files, and
> find two person has changed those two files, Johannes and Kevin.
> 
> CC'ing Johannes and Kevin for their permission, which using those 
> two files in Xen code. Thanks in advance!
> 
> CC'ing seabios maillist too.
> 
> 
> Best regards,
> -Gonglei

As an alternative, you can get a copy from qemu source which
has already been relicensed to GPLv2+.

Also I wonder: would it be possible for you to load the acpi
tables from qemu instead of generating your own?
This is what kvm with seabios do now, and
that would help make sure you get all the features automatically.

-- 
MST

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