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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.10.13 at 06:08, "Gonglei (Arei)" <arei.gonglei@xxxxxxxxxx> 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}' $< > $@.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 $@)
> +     

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}' $< > $@.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

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 > $@.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

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

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