Re: [Xen-devel] [PATCH v2 17/23] libacpi: Build DSDT for PVH guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.08.16 at 23:06, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -34,13 +34,15 @@ $(H_SRC): $(ACPI_BUILD_DIR)/%.h: %.asl iasl
>  $(MK_DSDT): mk_dsdt.c
>       $(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_xeninclude) -o $@ mk_dsdt.c
>  
> -$(ACPI_BUILD_DIR)/dsdt_anycpu_qemu_xen.asl: dsdt.asl $(MK_DSDT)
> +$(ACPI_BUILD_DIR)/dsdt_anycpu_qemu_xen.asl: dsdt.asl dsdt_acpi_info.asl 
> $(MK_DSDT)
>       awk 'NR > 1 {print s} {s=$$0}' $< > $@
> +     cat dsdt_acpi_info.asl >> $@
>       $(MK_DSDT) --debug=$(debug) --dm-version qemu-xen >> $@

I understand the problem is pre-existing, but unfortunately you make
it worse: Repeatedly redirecting into the destination file is prone to
problems when someone interrupts a build. There should be a
temporary file created, and as the last step that one should be
mv-ed to the destination.

> @@ -58,7 +71,7 @@ iasl:
>       @exit 1
>  
>  clean:
> -     rm -fr $(C_SRC) $(H_SRC) $(MK_DSDT) $(patsubst %.c,%.asl,$(C_SRC))
> +     rm -fr $(C_SRC) $(H_SRC) $(MK_DSDT) $(patsubst %.c,%.asl,$(C_SRC)) 
> $(ACPI_BUILD_DIR)/dsdt_pvh.c

Isn't this already being taken care of by $(C_SRC)?

> --- /dev/null
> +++ b/tools/libacpi/dsdt_acpi_info.asl
> @@ -0,0 +1,23 @@
> +
> +    Scope (\_SB)
> +    {
> +       /* ACPI_INFO_PHYSICAL_ADDRESS == 0xFC000000 */
> +       OperationRegion(BIOS, SystemMemory, 0xFC000000, 40)
> +       Field(BIOS, ByteAcc, NoLock, Preserve) {
> +           UAR1, 1,
> +           UAR2, 1,
> +           LTP1, 1,
> +           HPET, 1,
> +           Offset(2),
> +           NCPU, 16,
> +           PMIN, 32,
> +           PLEN, 32,
> +           MSUA, 32, /* MADT checksum address */
> +           MAPA, 32, /* MADT LAPIC0 address */
> +           VGIA, 32, /* VM generation id address */
> +           LMIN, 32,
> +           HMIN, 32,
> +           LLEN, 32,
> +           HLEN, 32
> +       }
> +    }

While moving it, could you please extend the comment to add an
xref note similar to the one ahead of the struct acpi_info declaration
(but pointing in the opposite direction)?

> --- a/tools/libacpi/mk_dsdt.c
> +++ b/tools/libacpi/mk_dsdt.c
> @@ -98,6 +98,7 @@ static struct option options[] = {
>      { "maxcpu", 1, 0, 'c' },
>      { "dm-version", 1, 0, 'q' },
>      { "debug", 1, 0, 'd' },
> +    { "no-dm", 0, 0, 'n' },

Please, instead of going this route, add support for --dm-version none,
...

> @@ -105,6 +106,7 @@ int main(int argc, char **argv)
>  {
>      unsigned int slot, dev, intx, link, cpu, max_cpus = HVM_MAX_VCPUS;
>      dm_version dm_version = QEMU_XEN_TRADITIONAL;
> +    bool no_dm = 0;

... at once extending the dm_version enum (and presumably that
enumerator would preferably become the first one, i.e. with value
zero, so you could - if you want - use an expression like
!dm_version).

Jan


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