Re: [Xen-devel] [PATCH v4 10/21] acpi/hvmloader: Link ACPI object files directly

[Boris Ostrovsky]

On 09/21/2016 06:52 AM, Jan Beulich wrote:
 On 20.09.16 at 02:19,  wrote:
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -127,13 +127,13 @@ tools/firmware/*bios/*bios*.txt
>>  tools/firmware/etherboot/gpxe/*
>>  tools/firmware/extboot/extboot.img
>>  tools/firmware/extboot/signrom
>> -tools/firmware/hvmloader/acpi/mk_dsdt
>> -tools/firmware/hvmloader/acpi/dsdt*.c
>> -tools/firmware/hvmloader/acpi/dsdt_*cpu*.asl
>> -tools/firmware/hvmloader/acpi/ssdt_*.h
>> +tools/firmware/hvmloader/dsdt*.c
>> +tools/firmware/hvmloader/dsdt_*.asl
> Aren't you wrongly dropping the *cpu part here?

(Missed this)

No: *cpu* was added earlier to keep hvmloader/acpi/dsdt_acpi_info.asl
tracked by git but ignore generated files like
hvmloader/acpi/dsdt_anycpu.asl. With this patch, those generated files
will be created in hvmloader/ directory while dsdt_acpi_info.asl stays
in hvmloader/acpi/ (and will eventually be moved to tools/libacpi).

Come think of it, hvmloader/dsdt* and hvmloader/ssdt* should all be ignored.

-boris



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

Re: [Xen-devel] [PATCH v4 10/21] acpi/hvmloader: Link ACPI object files directly

[Ian Jackson]

Boris Ostrovsky writes ("Re: [PATCH v4 10/21] acpi/hvmloader: Link ACPI object 
files directly"):
> On 09/21/2016 11:05 AM, Ian Jackson wrote:
> > Wait, the libxl makefiles are going to end up running iasl ?
> 
> Not directly. There is a new target in libxl's Makefile:
> 
> acpi:
> $(MAKE) -C $(ACPI_PATH) ACPI_BUILD_DIR=$(CURDIR)
> 
> (ACPI_PATH is tools/libacpi)

Right.

> > If you end up making these files appear elsewhere then I think ".tmp"
> > is fine if the first part of the filename is unique enough.
> 
> They are built in libxl directory, this is by design. If we were to
> build them in tools/libacpi then we may collide with other components
> (such as hvmloader) doing the build at the same time.

I see.  Yes, this is a good approach.

> Keeping them in /tmp/`mktemp -d`, for example, does not quite work since
> we can't easily clean this up in case of an interrupt. (I tried this in
> an earlier version and it doesn't look good).

What you have now is better.

> But the names are pretty unique (dsdt_ or ).

That's good.

Thanks,
Ian.

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

Re: [Xen-devel] [PATCH v4 10/21] acpi/hvmloader: Link ACPI object files directly

[Boris Ostrovsky]

On 09/21/2016 11:05 AM, Ian Jackson wrote:
> Boris Ostrovsky writes ("Re: [PATCH v4 10/21] acpi/hvmloader: Link ACPI 
> object files directly"):
>> 2. ".tmp__" vs ".tmp": Because the temporary files are generated not in
>> tools/libacpi but in the directory of the libacpi user (such as libxl)
>> it is possible that a Makefile there might use ".tmp' for its own
>> purposes so I am trying here to minimize chances of a conflict. Maybe
>> even ".tmp_acpi"?
> Wait, the libxl makefiles are going to end up running iasl ?

Not directly. There is a new target in libxl's Makefile:

acpi:
$(MAKE) -C $(ACPI_PATH) ACPI_BUILD_DIR=$(CURDIR)

(ACPI_PATH is tools/libacpi)

>
> If you end up making these files appear elsewhere then I think ".tmp"
> is fine if the first part of the filename is unique enough.

They are built in libxl directory, this is by design. If we were to
build them in tools/libacpi then we may collide with other components
(such as hvmloader) doing the build at the same time.

Keeping them in /tmp/`mktemp -d`, for example, does not quite work since
we can't easily clean this up in case of an interrupt. (I tried this in
an earlier version and it doesn't look good).

But the names are pretty unique (dsdt_ or ).

-boris


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

Re: [Xen-devel] [PATCH v4 10/21] acpi/hvmloader: Link ACPI object files directly

[Ian Jackson]

Boris Ostrovsky writes ("Re: [PATCH v4 10/21] acpi/hvmloader: Link ACPI object 
files directly"):
> 2. ".tmp__" vs ".tmp": Because the temporary files are generated not in
> tools/libacpi but in the directory of the libacpi user (such as libxl)
> it is possible that a Makefile there might use ".tmp' for its own
> purposes so I am trying here to minimize chances of a conflict. Maybe
> even ".tmp_acpi"?

Wait, the libxl makefiles are going to end up running iasl ?

If you end up making these files appear elsewhere then I think ".tmp"
is fine if the first part of the filename is unique enough.

Ian.

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

Re: [Xen-devel] [PATCH v4 10/21] acpi/hvmloader: Link ACPI object files directly

[Boris Ostrovsky]

On 09/21/2016 07:40 AM, Jan Beulich wrote:
 On 21.09.16 at 13:38,  wrote:
>> Jan Beulich writes ("Re: [PATCH v4 10/21] acpi/hvmloader: Link ACPI object 
>> files directly"):
>>> On 21.09.16 at 13:29,  wrote:
 I think `.dummy' would be a better string, if indeed it's a string
 which we expect always to be stripped, and not to appear in any
 filenames.
>>> Another (currently later, but I'd prefer it to be moved ahead)
>>> patch uses this for actual temporary files.
>> OK, then I don't understand what's wrong with ".tmp" ...
> So did I think when looking at the patch, but then I also thought
> (at least until I saw that actual files get created with that suffix)
> that it doesn't really matter. Boris?

I think there are two questions about using TMP_SUFFIX=tmp__

1. It is indeed used for two purposes --- one is to work around the bug
and the other is to append to intermediate build files (that are later
removed). There are two instances where I need to handle the bug and
thus I use a variable and not a literal ".dummy". And since I already
have (or, in this iteration of the series, will have in the later patch)
TMP_SUFFIX I figured I'd use it here as well.

2. ".tmp__" vs ".tmp": Because the temporary files are generated not in
tools/libacpi but in the directory of the libacpi user (such as libxl)
it is possible that a Makefile there might use ".tmp' for its own
purposes so I am trying here to minimize chances of a conflict. Maybe
even ".tmp_acpi"?

-boris


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

Re: [Xen-devel] [PATCH v4 10/21] acpi/hvmloader: Link ACPI object files directly

[Jan Beulich]

>>> On 21.09.16 at 13:38,  wrote:
> Jan Beulich writes ("Re: [PATCH v4 10/21] acpi/hvmloader: Link ACPI object 
> files directly"):
>> On 21.09.16 at 13:29,  wrote:
>> > I think `.dummy' would be a better string, if indeed it's a string
>> > which we expect always to be stripped, and not to appear in any
>> > filenames.
>> 
>> Another (currently later, but I'd prefer it to be moved ahead)
>> patch uses this for actual temporary files.
> 
> OK, then I don't understand what's wrong with ".tmp" ...

So did I think when looking at the patch, but then I also thought
(at least until I saw that actual files get created with that suffix)
that it doesn't really matter. Boris?

Jan


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

Re: [Xen-devel] [PATCH v4 10/21] acpi/hvmloader: Link ACPI object files directly

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v4 10/21] acpi/hvmloader: Link ACPI object 
files directly"):
> On 21.09.16 at 13:29,  wrote:
> > I think `.dummy' would be a better string, if indeed it's a string
> > which we expect always to be stripped, and not to appear in any
> > filenames.
> 
> Another (currently later, but I'd prefer it to be moved ahead)
> patch uses this for actual temporary files.

OK, then I don't understand what's wrong with ".tmp" ...

Ian.

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

Re: [Xen-devel] [PATCH v4 10/21] acpi/hvmloader: Link ACPI object files directly

[Jan Beulich]

>>> On 21.09.16 at 13:29,  wrote:
> Jan Beulich writes ("Re: [PATCH v4 10/21] acpi/hvmloader: Link ACPI object 
> files directly"):
>> On 20.09.16 at 02:19,  wrote:
>> > --- a/tools/firmware/hvmloader/acpi/Makefile
>> > +++ b/tools/firmware/hvmloader/acpi/Makefile
>> > @@ -15,41 +15,45 @@
>> >  XEN_ROOT = $(CURDIR)/../../../..
>> >  include $(XEN_ROOT)/tools/firmware/Rules.mk
>> >  
>> > -C_SRC-$(GPL)  = build.c dsdt_anycpu.c dsdt_15cpu.c 
>> > dsdt_anycpu_qemu_xen.c
>> > -C_SRC = build.c static_tables.c $(C_SRC-y)
>> > -OBJS  = $(patsubst %.c,%.o,$(C_SRC))
>> > +# Used as a workaround for a bug in some older iasl versions where
>> > +# the tool will ignore everything after last '.' in the path ('-p' 
>> > argument)
>> > +TMP_SUFFIX= tmp__
>> 
>> Hmm, the comment leaves open what newer iasl does? I suppose it
>> strips everything after the last . too, but only if that ones comes
>> after the last path separator?
> 
> I think Boris's approach ensures that the last . always comes after
> the last path separator.
> 
>> It took me a moment to understand
>> that no file with this suffix will ever be created, if my above summary
>> is right. Please clarify this in the comment.
> 
> I'm not sure it's really helpful to put this TMP_SUFFIX thing in a
> make variable but maybe Jan prefers it that way.

At least I didn't ask for it, and I'd be fine without.

> In any case I think the choice of `tmp__' is rather odd.  AFAICT the
> resulting iasl command lines look like this:
> 
>   iasl -vs -p /path/to/here/blah.tmp__ -tc /path/to/here/blah.asl
> 
> I think `.dummy' would be a better string, if indeed it's a string
> which we expect always to be stripped, and not to appear in any
> filenames.

Another (currently later, but I'd prefer it to be moved ahead)
patch uses this for actual temporary files.

Jan


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

Re: [Xen-devel] [PATCH v4 10/21] acpi/hvmloader: Link ACPI object files directly

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v4 10/21] acpi/hvmloader: Link ACPI object 
files directly"):
> On 20.09.16 at 02:19,  wrote:
> > --- a/tools/firmware/hvmloader/acpi/Makefile
> > +++ b/tools/firmware/hvmloader/acpi/Makefile
> > @@ -15,41 +15,45 @@
> >  XEN_ROOT = $(CURDIR)/../../../..
> >  include $(XEN_ROOT)/tools/firmware/Rules.mk
> >  
> > -C_SRC-$(GPL)   = build.c dsdt_anycpu.c dsdt_15cpu.c 
> > dsdt_anycpu_qemu_xen.c
> > -C_SRC  = build.c static_tables.c $(C_SRC-y)
> > -OBJS  = $(patsubst %.c,%.o,$(C_SRC))
> > +# Used as a workaround for a bug in some older iasl versions where
> > +# the tool will ignore everything after last '.' in the path ('-p' 
> > argument)
> > +TMP_SUFFIX = tmp__
> 
> Hmm, the comment leaves open what newer iasl does? I suppose it
> strips everything after the last . too, but only if that ones comes
> after the last path separator?

I think Boris's approach ensures that the last . always comes after
the last path separator.

> It took me a moment to understand
> that no file with this suffix will ever be created, if my above summary
> is right. Please clarify this in the comment.

I'm not sure it's really helpful to put this TMP_SUFFIX thing in a
make variable but maybe Jan prefers it that way.

In any case I think the choice of `tmp__' is rather odd.  AFAICT the
resulting iasl command lines look like this:

  iasl -vs -p /path/to/here/blah.tmp__ -tc /path/to/here/blah.asl

I think `.dummy' would be a better string, if indeed it's a string
which we expect always to be stripped, and not to appear in any
filenames.

ian.

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

Re: [Xen-devel] [PATCH v4 10/21] acpi/hvmloader: Link ACPI object files directly

[Jan Beulich]

>>> On 20.09.16 at 02:19,  wrote:
> --- a/.gitignore
> +++ b/.gitignore
> @@ -127,13 +127,13 @@ tools/firmware/*bios/*bios*.txt
>  tools/firmware/etherboot/gpxe/*
>  tools/firmware/extboot/extboot.img
>  tools/firmware/extboot/signrom
> -tools/firmware/hvmloader/acpi/mk_dsdt
> -tools/firmware/hvmloader/acpi/dsdt*.c
> -tools/firmware/hvmloader/acpi/dsdt_*cpu*.asl
> -tools/firmware/hvmloader/acpi/ssdt_*.h
> +tools/firmware/hvmloader/dsdt*.c
> +tools/firmware/hvmloader/dsdt_*.asl

Aren't you wrongly dropping the *cpu part here?

> --- a/tools/firmware/hvmloader/acpi/Makefile
> +++ b/tools/firmware/hvmloader/acpi/Makefile
> @@ -15,41 +15,45 @@
>  XEN_ROOT = $(CURDIR)/../../../..
>  include $(XEN_ROOT)/tools/firmware/Rules.mk
>  
> -C_SRC-$(GPL) = build.c dsdt_anycpu.c dsdt_15cpu.c 
> dsdt_anycpu_qemu_xen.c
> -C_SRC= build.c static_tables.c $(C_SRC-y)
> -OBJS  = $(patsubst %.c,%.o,$(C_SRC))
> +# Used as a workaround for a bug in some older iasl versions where
> +# the tool will ignore everything after last '.' in the path ('-p' argument)
> +TMP_SUFFIX   = tmp__

Hmm, the comment leaves open what newer iasl does? I suppose it
strips everything after the last . too, but only if that ones comes
after the last path separator? It took me a moment to understand
that no file with this suffix will ever be created, if my above summary
is right. Please clarify this in the comment.

With these two minor issues addressed,
Acked-by: Jan Beulich 

Jan


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