| Message ID | 1421938231-25698-7-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 22, 2015 at 02:49:50PM +0000, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/acpi/acpi-build-utils.c | 24 ++++++++++++++++++++++++ > include/hw/acpi/acpi-build-utils.h | 3 +++ > 2 files changed, 27 insertions(+) > > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c > index 40a1769..1bda2ec 100644 > --- a/hw/acpi/acpi-build-utils.c > +++ b/hw/acpi/acpi-build-utils.c > @@ -314,6 +314,30 @@ static AcpiAml aml_allocate_internal(uint8_t op, AcpiBlockFlags flags) > return var; > } > > +/* > + * help to construct NameString, which return AcpiAml object > + * for using with other aml_append or other acpi_* terms Here and elsewhere: I can't parse this header text. I'm guessing you just mean "construct NameString", and that's it? Also, most other places use build_append_namestring - so when should acpi_name be used instead? This should be made clear here in the comment. > + */ > +AcpiAml GCC_FMT_ATTR(1, 2) acpi_name(const char *name_format, ...) > +{ This isn't really a name. It just appends a string. So rename this acpi_string and then the below one adding a name can be named acpi_name? Also, in many places one must use only one nameseg. I think a separate api that actually validates that it's one segment is better than silently failing. Do we ever use it for more than 1 segment? If not, maybe the right thing to do is to use build_append_nameseg and call this one acpi_nameseg. > + va_list ap; > + AcpiAml var = aml_allocate_internal(0, NON_BLOCK); 0 hard coded? What does it mean? Same elsewhere. > + va_start(ap, name_format); > + build_append_namestringv(var.buf, name_format, ap); > + va_end(ap); > + return var; > + > +/* ACPI 5.0: 20.2.5.1 Namespace Modifier Objects Encoding: DefName */ Let's quote the earliest spec which documents each object: one year from now 5.0 will not be the latest. Applies here and elsewhere. In most places this will be 1.0b. Where the construct is newer, this will automatically document which guests support it. > +AcpiAml acpi_name_decl(const char *name, AcpiAml val) > +{ > + AcpiAml var = aml_allocate_internal(0, NON_BLOCK); > + build_append_byte(var.buf, 0x08); Pls add comment documenting what 0x08 is here. > + build_append_namestring(var.buf, "%s", name); > + aml_append(&var, val); > + return var; > +} > + > /* ACPI 5.0: 20.2.5.3 Type 1 Opcodes Encoding: DefIfElse */ > AcpiAml acpi_if(AcpiAml predicate) > { > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h > index 177f9ed..868cfa5 100644 > --- a/include/hw/acpi/acpi-build-utils.h > +++ b/include/hw/acpi/acpi-build-utils.h > @@ -21,6 +21,9 @@ typedef struct AcpiAml { > > void aml_append(AcpiAml *parent_ctx, AcpiAml child); > > +/* non block ASL object primitives */ what does it mean that it's a "non block primitive"? I didn't find this concept in the spec. > +AcpiAml GCC_FMT_ATTR(1, 2) acpi_name(const char *name_format, ...); > +AcpiAml acpi_name_decl(const char *name, AcpiAml val); > /* Block ASL object primitives */ > AcpiAml acpi_if(AcpiAml predicate); > AcpiAml acpi_method(const char *name, int arg_count); > -- > 1.8.3.1
On Fri, 23 Jan 2015 10:59:48 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Jan 22, 2015 at 02:49:50PM +0000, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/acpi/acpi-build-utils.c | 24 ++++++++++++++++++++++++ > > include/hw/acpi/acpi-build-utils.h | 3 +++ > > 2 files changed, 27 insertions(+) > > > > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c > > index 40a1769..1bda2ec 100644 > > --- a/hw/acpi/acpi-build-utils.c > > +++ b/hw/acpi/acpi-build-utils.c > > @@ -314,6 +314,30 @@ static AcpiAml aml_allocate_internal(uint8_t op, AcpiBlockFlags flags) > > return var; > > } > > > > +/* > > + * help to construct NameString, which return AcpiAml object > > + * for using with other aml_append or other acpi_* terms > > Here and elsewhere: I can't parse this header text. > I'm guessing you just mean "construct NameString", > and that's it? yes > > Also, most other places use build_append_namestring - > so when should acpi_name be used instead? > This should be made clear here in the comment. acpi_name() is a replacement/wrapper around build_append_namestring() which returns AcpiAml object. build_append_namestring() is a nonpublic lowlevel helper that deals with GArray, while acpi_name() follows semantic of AML API. > > > + */ > > +AcpiAml GCC_FMT_ATTR(1, 2) acpi_name(const char *name_format, ...) > > +{ > > This isn't really a name. It just appends a string. So rename this > acpi_string and then the below one adding a name can be named acpi_name? acpi_string is introduced in 27/47, which is a prefixed string as described in spec. > Also, in many places one must use only one nameseg. Where is it exactly? Perhaps we could build in acpi_name() a check if we know in what context enforce it. Better to have single/uniform API for names than a several which is confusing. > I think a separate api that actually validates > that it's one segment is better than silently failing. > Do we ever use it for more than 1 segment? Yes we use names with more than one segment. > If not, maybe the right thing to do is > to use build_append_nameseg and call this one acpi_nameseg. acpi_name() is used only for passing name as arguments to methods, in spec there isn't a limitation to only one segment when it comes to names, in ASL part of it. namesegment however only AML construct which helps to build name, I prefer not expose lowlevel AML unless we have to. > > > > + va_list ap; > > + AcpiAml var = aml_allocate_internal(0, NON_BLOCK); > > 0 hard coded? What does it mean? 1st arg for NON_BLOCK context doesn't mean anything/ignored. alternatively I can make aml_allocate_nonblock() wrapper around generic allocator. > Same elsewhere. > > > + va_start(ap, name_format); > > + build_append_namestringv(var.buf, name_format, ap); > > + va_end(ap); > > + return var; > > + > > +/* ACPI 5.0: 20.2.5.1 Namespace Modifier Objects Encoding: DefName */ > > Let's quote the earliest spec which documents each object: > one year from now 5.0 will not be the latest. > Applies here and elsewhere. > In most places this will be 1.0b. > Where the construct is newer, this will automatically > document which guests support it. I'll try to do it. > > > +AcpiAml acpi_name_decl(const char *name, AcpiAml val) > > +{ > > + AcpiAml var = aml_allocate_internal(0, NON_BLOCK); > > + build_append_byte(var.buf, 0x08); > > Pls add comment documenting what 0x08 is here. sure > > > + build_append_namestring(var.buf, "%s", name); > > + aml_append(&var, val); > > + return var; > > +} > > + > > /* ACPI 5.0: 20.2.5.3 Type 1 Opcodes Encoding: DefIfElse */ > > AcpiAml acpi_if(AcpiAml predicate) > > { > > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h > > index 177f9ed..868cfa5 100644 > > --- a/include/hw/acpi/acpi-build-utils.h > > +++ b/include/hw/acpi/acpi-build-utils.h > > @@ -21,6 +21,9 @@ typedef struct AcpiAml { > > > > void aml_append(AcpiAml *parent_ctx, AcpiAml child); > > > > +/* non block ASL object primitives */ > > what does it mean that it's a "non block primitive"? > I didn't find this concept in the spec. As for a question what is NON_BLOCK, it's for simple inline ASL construct that doesn't have to be packaged in special way examles: Store(A,B) Name(FOO, VAL) IO(...) while there are different block elements differing in how they are created see 1/47 aml_append(): ResourceTemplate { /* block of other ASL items */ } Package() { /* block of other ASL items */ } if ... else ... Scope() { /* block of other ASL items */ } and so on. > > > +AcpiAml GCC_FMT_ATTR(1, 2) acpi_name(const char *name_format, ...); > > +AcpiAml acpi_name_decl(const char *name, AcpiAml val); > > /* Block ASL object primitives */ > > AcpiAml acpi_if(AcpiAml predicate); > > AcpiAml acpi_method(const char *name, int arg_count); > > -- > > 1.8.3.1
On Fri, Jan 23, 2015 at 02:32:45PM +0100, Igor Mammedov wrote: > On Fri, 23 Jan 2015 10:59:48 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Thu, Jan 22, 2015 at 02:49:50PM +0000, Igor Mammedov wrote: > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > hw/acpi/acpi-build-utils.c | 24 ++++++++++++++++++++++++ > > > include/hw/acpi/acpi-build-utils.h | 3 +++ > > > 2 files changed, 27 insertions(+) > > > > > > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c > > > index 40a1769..1bda2ec 100644 > > > --- a/hw/acpi/acpi-build-utils.c > > > +++ b/hw/acpi/acpi-build-utils.c > > > @@ -314,6 +314,30 @@ static AcpiAml aml_allocate_internal(uint8_t op, AcpiBlockFlags flags) > > > return var; > > > } > > > > > > +/* > > > + * help to construct NameString, which return AcpiAml object > > > + * for using with other aml_append or other acpi_* terms > > > > Here and elsewhere: I can't parse this header text. > > I'm guessing you just mean "construct NameString", > > and that's it? > yes > > > > > Also, most other places use build_append_namestring - > > so when should acpi_name be used instead? > > This should be made clear here in the comment. > acpi_name() is a replacement/wrapper around build_append_namestring() > which returns AcpiAml object. build_append_namestring() is a nonpublic > lowlevel helper that deals with GArray, > while acpi_name() follows semantic of AML API. > > > > > > + */ > > > +AcpiAml GCC_FMT_ATTR(1, 2) acpi_name(const char *name_format, ...) > > > +{ > > > > This isn't really a name. It just appends a string. So rename this > > acpi_string and then the below one adding a name can be named acpi_name? > acpi_string is introduced in 27/47, which is a prefixed string > as described in spec. > > > Also, in many places one must use only one nameseg. > Where is it exactly? > Perhaps we could build in acpi_name() a check if we know in > what context enforce it. Better to have single/uniform API > for names than a several which is confusing. I agree here. > > I think a separate api that actually validates > > that it's one segment is better than silently failing. > > Do we ever use it for more than 1 segment? > Yes we use names with more than one segment. Interesting. where exactly? > > If not, maybe the right thing to do is > > to use build_append_nameseg and call this one acpi_nameseg. > acpi_name() is used only for passing name as arguments to methods, > in spec there isn't a limitation to only one segment when it comes > to names, in ASL part of it. namesegment however only AML construct > which helps to build name, I prefer not expose lowlevel AML > unless we have to. OK, I agree. > > > > > > > + va_list ap; > > > + AcpiAml var = aml_allocate_internal(0, NON_BLOCK); > > > > 0 hard coded? What does it mean? > 1st arg for NON_BLOCK context doesn't mean anything/ignored. > alternatively I can make aml_allocate_nonblock() wrapper > around generic allocator. 0 isn't a valid opcode either, it can really be anything. > > Same elsewhere. > > > > > + va_start(ap, name_format); > > > + build_append_namestringv(var.buf, name_format, ap); > > > + va_end(ap); > > > + return var; > > > + > > > +/* ACPI 5.0: 20.2.5.1 Namespace Modifier Objects Encoding: DefName */ > > > > Let's quote the earliest spec which documents each object: > > one year from now 5.0 will not be the latest. > > Applies here and elsewhere. > > In most places this will be 1.0b. > > Where the construct is newer, this will automatically > > document which guests support it. > I'll try to do it. > > > > > > +AcpiAml acpi_name_decl(const char *name, AcpiAml val) > > > +{ > > > + AcpiAml var = aml_allocate_internal(0, NON_BLOCK); > > > + build_append_byte(var.buf, 0x08); > > > > Pls add comment documenting what 0x08 is here. > sure > > > > > > + build_append_namestring(var.buf, "%s", name); > > > + aml_append(&var, val); > > > + return var; > > > +} > > > + > > > /* ACPI 5.0: 20.2.5.3 Type 1 Opcodes Encoding: DefIfElse */ > > > AcpiAml acpi_if(AcpiAml predicate) > > > { > > > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h > > > index 177f9ed..868cfa5 100644 > > > --- a/include/hw/acpi/acpi-build-utils.h > > > +++ b/include/hw/acpi/acpi-build-utils.h > > > @@ -21,6 +21,9 @@ typedef struct AcpiAml { > > > > > > void aml_append(AcpiAml *parent_ctx, AcpiAml child); > > > > > > +/* non block ASL object primitives */ > > > > what does it mean that it's a "non block primitive"? > > I didn't find this concept in the spec. > As for a question what is NON_BLOCK, it's for simple inline ASL > construct that doesn't have to be packaged in special way > examles: > Store(A,B) > Name(FOO, VAL) > IO(...) > while there are different block elements differing in how > they are created see 1/47 aml_append(): > > ResourceTemplate { > /* block of other ASL items */ > } > > Package() { > /* block of other ASL items */ > } > > if ... else ... > > Scope() { > /* block of other ASL items */ > } > > and so on. "special way" is kind of vague. Maybe add a comment explaining when it's used. Is it when length isn't used as a prefix? AML_NO_PREFIX? > > > > > +AcpiAml GCC_FMT_ATTR(1, 2) acpi_name(const char *name_format, ...); > > > +AcpiAml acpi_name_decl(const char *name, AcpiAml val); > > > /* Block ASL object primitives */ > > > AcpiAml acpi_if(AcpiAml predicate); > > > AcpiAml acpi_method(const char *name, int arg_count); > > > -- > > > 1.8.3.1
On Fri, 23 Jan 2015 15:42:30 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Jan 23, 2015 at 02:32:45PM +0100, Igor Mammedov wrote: > > On Fri, 23 Jan 2015 10:59:48 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Thu, Jan 22, 2015 at 02:49:50PM +0000, Igor Mammedov wrote: > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > hw/acpi/acpi-build-utils.c | 24 ++++++++++++++++++++++++ > > > > include/hw/acpi/acpi-build-utils.h | 3 +++ > > > > 2 files changed, 27 insertions(+) > > > > > > > > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c > > > > index 40a1769..1bda2ec 100644 > > > > --- a/hw/acpi/acpi-build-utils.c > > > > +++ b/hw/acpi/acpi-build-utils.c > > > > @@ -314,6 +314,30 @@ static AcpiAml aml_allocate_internal(uint8_t op, AcpiBlockFlags flags) > > > > return var; > > > > } > > > > > > > > +/* > > > > + * help to construct NameString, which return AcpiAml object > > > > + * for using with other aml_append or other acpi_* terms > > > > > > Here and elsewhere: I can't parse this header text. > > > I'm guessing you just mean "construct NameString", > > > and that's it? > > yes > > > > > > > > Also, most other places use build_append_namestring - > > > so when should acpi_name be used instead? > > > This should be made clear here in the comment. > > acpi_name() is a replacement/wrapper around build_append_namestring() > > which returns AcpiAml object. build_append_namestring() is a nonpublic > > lowlevel helper that deals with GArray, > > while acpi_name() follows semantic of AML API. > > > > > > > > > + */ > > > > +AcpiAml GCC_FMT_ATTR(1, 2) acpi_name(const char *name_format, ...) > > > > +{ > > > > > > This isn't really a name. It just appends a string. So rename this > > > acpi_string and then the below one adding a name can be named acpi_name? > > acpi_string is introduced in 27/47, which is a prefixed string > > as described in spec. > > > > > Also, in many places one must use only one nameseg. > > Where is it exactly? > > Perhaps we could build in acpi_name() a check if we know in > > what context enforce it. Better to have single/uniform API > > for names than a several which is confusing. > > I agree here. > > > > I think a separate api that actually validates > > > that it's one segment is better than silently failing. > > > Do we ever use it for more than 1 segment? > > Yes we use names with more than one segment. > > Interesting. where exactly? For example: build_append_namestring(method, "^S%.02X.PCNT" > > > > If not, maybe the right thing to do is > > > to use build_append_nameseg and call this one acpi_nameseg. > > acpi_name() is used only for passing name as arguments to methods, > > in spec there isn't a limitation to only one segment when it comes > > to names, in ASL part of it. namesegment however only AML construct > > which helps to build name, I prefer not expose lowlevel AML > > unless we have to. > > OK, I agree. > > > > > > > > > > > + va_list ap; > > > > + AcpiAml var = aml_allocate_internal(0, NON_BLOCK); > > > > > > 0 hard coded? What does it mean? > > 1st arg for NON_BLOCK context doesn't mean anything/ignored. > > alternatively I can make aml_allocate_nonblock() wrapper > > around generic allocator. > > 0 isn't a valid opcode either, it can really be anything. yes, it could. So I leave it as it's or is there any other way that you'd prefer here? > > > > > > Same elsewhere. > > > > > > > + va_start(ap, name_format); > > > > + build_append_namestringv(var.buf, name_format, ap); > > > > + va_end(ap); > > > > + return var; > > > > + > > > > +/* ACPI 5.0: 20.2.5.1 Namespace Modifier Objects Encoding: DefName */ > > > > > > Let's quote the earliest spec which documents each object: > > > one year from now 5.0 will not be the latest. > > > Applies here and elsewhere. > > > In most places this will be 1.0b. > > > Where the construct is newer, this will automatically > > > document which guests support it. > > I'll try to do it. > > > > > > > > > +AcpiAml acpi_name_decl(const char *name, AcpiAml val) > > > > +{ > > > > + AcpiAml var = aml_allocate_internal(0, NON_BLOCK); > > > > + build_append_byte(var.buf, 0x08); > > > > > > Pls add comment documenting what 0x08 is here. > > sure > > > > > > > > > + build_append_namestring(var.buf, "%s", name); > > > > + aml_append(&var, val); > > > > + return var; > > > > +} > > > > + > > > > /* ACPI 5.0: 20.2.5.3 Type 1 Opcodes Encoding: DefIfElse */ > > > > AcpiAml acpi_if(AcpiAml predicate) > > > > { > > > > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h > > > > index 177f9ed..868cfa5 100644 > > > > --- a/include/hw/acpi/acpi-build-utils.h > > > > +++ b/include/hw/acpi/acpi-build-utils.h > > > > @@ -21,6 +21,9 @@ typedef struct AcpiAml { > > > > > > > > void aml_append(AcpiAml *parent_ctx, AcpiAml child); > > > > > > > > +/* non block ASL object primitives */ > > > > > > what does it mean that it's a "non block primitive"? > > > I didn't find this concept in the spec. > > As for a question what is NON_BLOCK, it's for simple inline ASL > > construct that doesn't have to be packaged in special way > > examles: > > Store(A,B) > > Name(FOO, VAL) > > IO(...) > > while there are different block elements differing in how > > they are created see 1/47 aml_append(): > > > > ResourceTemplate { > > /* block of other ASL items */ > > } > > > > Package() { > > /* block of other ASL items */ > > } > > > > if ... else ... > > > > Scope() { > > /* block of other ASL items */ > > } > > > > and so on. > > "special way" is kind of vague. > Maybe add a comment explaining when it's used. > Is it when length isn't used as a prefix? > AML_NO_PREFIX? There are terms that have 'prefix' in definition but are still inline elements, like: DWordConst := DWordPrefix DWordData String := StringPrefix AsciiCharList NullChar using prefix would be confusing. Having(/not having) prefix doesn't make objects into block one, that can embed other objects. And although length-less is the closest definition for them but it also doesn't match all objects, for example: NamedField := NameSeg PkgLength uses PkgLength but still isn't a block element that includes other objects. That's why I've used NON_BLOCK as a neutral name but still describing what's going on. > > > > > > > > > +AcpiAml GCC_FMT_ATTR(1, 2) acpi_name(const char *name_format, ...); > > > > +AcpiAml acpi_name_decl(const char *name, AcpiAml val); > > > > /* Block ASL object primitives */ > > > > AcpiAml acpi_if(AcpiAml predicate); > > > > AcpiAml acpi_method(const char *name, int arg_count); > > > > -- > > > > 1.8.3.1
diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c index 40a1769..1bda2ec 100644 --- a/hw/acpi/acpi-build-utils.c +++ b/hw/acpi/acpi-build-utils.c @@ -314,6 +314,30 @@ static AcpiAml aml_allocate_internal(uint8_t op, AcpiBlockFlags flags) return var; } +/* + * help to construct NameString, which return AcpiAml object + * for using with other aml_append or other acpi_* terms + */ +AcpiAml GCC_FMT_ATTR(1, 2) acpi_name(const char *name_format, ...) +{ + va_list ap; + AcpiAml var = aml_allocate_internal(0, NON_BLOCK); + va_start(ap, name_format); + build_append_namestringv(var.buf, name_format, ap); + va_end(ap); + return var; +} + +/* ACPI 5.0: 20.2.5.1 Namespace Modifier Objects Encoding: DefName */ +AcpiAml acpi_name_decl(const char *name, AcpiAml val) +{ + AcpiAml var = aml_allocate_internal(0, NON_BLOCK); + build_append_byte(var.buf, 0x08); + build_append_namestring(var.buf, "%s", name); + aml_append(&var, val); + return var; +} + /* ACPI 5.0: 20.2.5.3 Type 1 Opcodes Encoding: DefIfElse */ AcpiAml acpi_if(AcpiAml predicate) { diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h index 177f9ed..868cfa5 100644 --- a/include/hw/acpi/acpi-build-utils.h +++ b/include/hw/acpi/acpi-build-utils.h @@ -21,6 +21,9 @@ typedef struct AcpiAml { void aml_append(AcpiAml *parent_ctx, AcpiAml child); +/* non block ASL object primitives */ +AcpiAml GCC_FMT_ATTR(1, 2) acpi_name(const char *name_format, ...); +AcpiAml acpi_name_decl(const char *name, AcpiAml val); /* Block ASL object primitives */ AcpiAml acpi_if(AcpiAml predicate); AcpiAml acpi_method(const char *name, int arg_count);
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/acpi/acpi-build-utils.c | 24 ++++++++++++++++++++++++ include/hw/acpi/acpi-build-utils.h | 3 +++ 2 files changed, 27 insertions(+)