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

If this information is useful, please help other people find it:
Share via:
Jan Beulich
2013-Oct-22 08:06 UTC
Re: [Qemu-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching

>>> On 22.10.13 at 06:08, "Gonglei (Arei)"
<arei.gonglei@huawei.com> 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 - Oct 2013 - Re: [Qemu-devel] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching

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