Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On 2015/6/18 19:03, Andrew Jones wrote:
On Thu, Jun 18, 2015 at 06:28:36PM +0800, Shannon Zhao wrote:
On 2015/6/17 17:42, Andrew Jones wrote:
On Wed, Jun 17, 2015 at 09:06:47AM +0800, Shannon Zhao wrote:
On 2015/6/16 22:19, Michael S. Tsirkin wrote:
On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
On 2015/6/16 2:13, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
On 15 June 2015 at 17:32, Andrew Jones drjo...@redhat.com wrote:
On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0x anyway. I can change those two to make them more
explicit,
if that's preferred.
Yep, I just looked over the struct definition, so since this
has been reviewed I'll apply it to target-arm.next.
You could probably make it easier to review and write
code that has to do these endianness swaps with something
like
#define acpi_struct_assign(FIELD, VAL) \
((FIELD) = \
__builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
__builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
abort
(untested, but based on some code in linux-user/qemu.h).
Then it's always
acpi_struct_assign(spcr-field, value);
whether the field is 1, 2, 4 or 8 bytes.
Not my bit of the codebase though, so I'll leave it to the
ACPI maintainers to decide how much they like magic macros :-)
thanks
-- PMM
We don't much. One can use build_append_int_noprefix and just avoid
structs altogether.
But if we use build_append_int_noprefix, we have to bother about the
unused fields of the struct and have lots of
build_append_int_noprefix(table, 0, 1/2/4/8).
With a struct you have a bunch of reserved fields - is that very
different?
Not only about the reserved fields, but also the fields which ARM
doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct
AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use
build_append_int_noprefix, we should add lots of
build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we
just need to care them when we define it, rather than every time we use.
I've had a chat with Igor about this, and thought about it, and I'm now
in the build_append camp. It took me a while to see that point of view,
as it isn't a pattern I'm familiar with. However, the pattern, which is
* table generation code is a sequence of build_appends, commented with
strings matching strings in the spec
* table generation code has the minimal number of parameters necessary
to be useful for all users, all other table values have default values
filled (typically zero)
* the parameters to the table generation code can be packed in a param
struct that has descriptive member names, allowing the caller to
still use the struct initializing type pattern, and to more cleanly
manage the parameters
with the benefits of
* structs (the param structs) stay small
But the table generation codes will be huge and have lots of meaningless
build_append_int_noprefix(table, 0, 1/2/4/8) for the unused fields.
Maybe the readability is poor.
It should be about the same number of lines as the struct definition,
one line per field, and it'll only appear in one place, an API call in
aml-build.c. Looking at aml-build.c for some examples where this is
already done, e.g. aml_memory32_fixed and aml_interrupt, ones you added
These are operators which don't have too many unused fields for
different platforms. Here we are discussing about the generation of ACPI
table.
If you want to rework these, maybe you can have a try to rework FACP
table first and compare the lines and readability.
:-), I would agree that, at first look, it's not super easy to see what's
what. However the key to understanding it is to have the spec open.
Knowing that no field is skipped allows a 1:1 mapping of build_append
lines to spec table fields, and good comments help guide the eye for
that mapping as well, even though just counting lines should work.
Note, the build_append_int_noprefix(table, 0, 1/2/4/8) lines are no more
meaningless than the unused fields in a struct definition. The advantage
of dropping the struct in favor of the build_append lines is the
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On 2015/6/17 17:42, Andrew Jones wrote:
On Wed, Jun 17, 2015 at 09:06:47AM +0800, Shannon Zhao wrote:
On 2015/6/16 22:19, Michael S. Tsirkin wrote:
On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
On 2015/6/16 2:13, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
On 15 June 2015 at 17:32, Andrew Jones drjo...@redhat.com wrote:
On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0x anyway. I can change those two to make them more
explicit,
if that's preferred.
Yep, I just looked over the struct definition, so since this
has been reviewed I'll apply it to target-arm.next.
You could probably make it easier to review and write
code that has to do these endianness swaps with something
like
#define acpi_struct_assign(FIELD, VAL) \
((FIELD) = \
__builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
__builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
abort
(untested, but based on some code in linux-user/qemu.h).
Then it's always
acpi_struct_assign(spcr-field, value);
whether the field is 1, 2, 4 or 8 bytes.
Not my bit of the codebase though, so I'll leave it to the
ACPI maintainers to decide how much they like magic macros :-)
thanks
-- PMM
We don't much. One can use build_append_int_noprefix and just avoid
structs altogether.
But if we use build_append_int_noprefix, we have to bother about the
unused fields of the struct and have lots of
build_append_int_noprefix(table, 0, 1/2/4/8).
With a struct you have a bunch of reserved fields - is that very
different?
Not only about the reserved fields, but also the fields which ARM
doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct
AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use
build_append_int_noprefix, we should add lots of
build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we
just need to care them when we define it, rather than every time we use.
I've had a chat with Igor about this, and thought about it, and I'm now
in the build_append camp. It took me a while to see that point of view,
as it isn't a pattern I'm familiar with. However, the pattern, which is
* table generation code is a sequence of build_appends, commented with
strings matching strings in the spec
* table generation code has the minimal number of parameters necessary
to be useful for all users, all other table values have default values
filled (typically zero)
* the parameters to the table generation code can be packed in a param
struct that has descriptive member names, allowing the caller to
still use the struct initializing type pattern, and to more cleanly
manage the parameters
with the benefits of
* structs (the param structs) stay small
But the table generation codes will be huge and have lots of meaningless
build_append_int_noprefix(table, 0, 1/2/4/8) for the unused fields.
Maybe the readability is poor.
* callers don't have to worry about endianness
* maximum code sharing across users
* no need to try and reproduce the spec as a struct definition, which
can easily explode with comments/enums trying to describe each field
accurately
So, I think it would be nice to convert ARM's ACPI generation over to
this type of pattern, which may allow more merging of ARM and x86.
However, that said, it'd be good to get the endianness fix patch and
the gicv2m patch in sooner than later. Maybe we should start with those
patches (just using cpu_to_le*), and then consider a rework of the
struct based table generation, before continuing to extend it.
Thanks,
drew
.
--
Shannon
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On Thu, Jun 18, 2015 at 06:28:36PM +0800, Shannon Zhao wrote:
On 2015/6/17 17:42, Andrew Jones wrote:
On Wed, Jun 17, 2015 at 09:06:47AM +0800, Shannon Zhao wrote:
On 2015/6/16 22:19, Michael S. Tsirkin wrote:
On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
On 2015/6/16 2:13, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
On 15 June 2015 at 17:32, Andrew Jones drjo...@redhat.com wrote:
On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0x anyway. I can change those two to make them more
explicit,
if that's preferred.
Yep, I just looked over the struct definition, so since this
has been reviewed I'll apply it to target-arm.next.
You could probably make it easier to review and write
code that has to do these endianness swaps with something
like
#define acpi_struct_assign(FIELD, VAL) \
((FIELD) = \
__builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
__builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
abort
(untested, but based on some code in linux-user/qemu.h).
Then it's always
acpi_struct_assign(spcr-field, value);
whether the field is 1, 2, 4 or 8 bytes.
Not my bit of the codebase though, so I'll leave it to the
ACPI maintainers to decide how much they like magic macros :-)
thanks
-- PMM
We don't much. One can use build_append_int_noprefix and just avoid
structs altogether.
But if we use build_append_int_noprefix, we have to bother about the
unused fields of the struct and have lots of
build_append_int_noprefix(table, 0, 1/2/4/8).
With a struct you have a bunch of reserved fields - is that very
different?
Not only about the reserved fields, but also the fields which ARM
doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct
AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use
build_append_int_noprefix, we should add lots of
build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we
just need to care them when we define it, rather than every time we use.
I've had a chat with Igor about this, and thought about it, and I'm now
in the build_append camp. It took me a while to see that point of view,
as it isn't a pattern I'm familiar with. However, the pattern, which is
* table generation code is a sequence of build_appends, commented with
strings matching strings in the spec
* table generation code has the minimal number of parameters necessary
to be useful for all users, all other table values have default values
filled (typically zero)
* the parameters to the table generation code can be packed in a param
struct that has descriptive member names, allowing the caller to
still use the struct initializing type pattern, and to more cleanly
manage the parameters
with the benefits of
* structs (the param structs) stay small
But the table generation codes will be huge and have lots of meaningless
build_append_int_noprefix(table, 0, 1/2/4/8) for the unused fields.
Maybe the readability is poor.
It should be about the same number of lines as the struct definition,
one line per field, and it'll only appear in one place, an API call in
aml-build.c. Looking at aml-build.c for some examples where this is
already done, e.g. aml_memory32_fixed and aml_interrupt, ones you added
:-), I would agree that, at first look, it's not super easy to see what's
what. However the key to understanding it is to have the spec open.
Knowing that no field is skipped allows a 1:1 mapping of build_append
lines to spec table fields, and good comments help guide the eye for
that mapping as well, even though just counting lines should work.
Note, the build_append_int_noprefix(table, 0, 1/2/4/8) lines are no more
meaningless than the unused fields in a struct definition. The advantage
of dropping the struct in favor of the build_append lines is the benefit
of simplifying the use of the used fields, as there are no more concerns
with endianness, nor the details of a AcpiGenericAddress, etc.
drew
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On Wed, Jun 17, 2015 at 09:06:47AM +0800, Shannon Zhao wrote:
On 2015/6/16 22:19, Michael S. Tsirkin wrote:
On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
On 2015/6/16 2:13, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
On 15 June 2015 at 17:32, Andrew Jones drjo...@redhat.com wrote:
On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0x anyway. I can change those two to make them more
explicit,
if that's preferred.
Yep, I just looked over the struct definition, so since this
has been reviewed I'll apply it to target-arm.next.
You could probably make it easier to review and write
code that has to do these endianness swaps with something
like
#define acpi_struct_assign(FIELD, VAL) \
((FIELD) = \
__builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
__builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
abort
(untested, but based on some code in linux-user/qemu.h).
Then it's always
acpi_struct_assign(spcr-field, value);
whether the field is 1, 2, 4 or 8 bytes.
Not my bit of the codebase though, so I'll leave it to the
ACPI maintainers to decide how much they like magic macros :-)
thanks
-- PMM
We don't much. One can use build_append_int_noprefix and just avoid
structs altogether.
But if we use build_append_int_noprefix, we have to bother about the
unused fields of the struct and have lots of
build_append_int_noprefix(table, 0, 1/2/4/8).
With a struct you have a bunch of reserved fields - is that very
different?
Not only about the reserved fields, but also the fields which ARM
doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct
AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use
build_append_int_noprefix, we should add lots of
build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we
just need to care them when we define it, rather than every time we use.
So the advice above assumes that you have a wrapper function
for building each struct. Then you would just pass 0
as parameters as appropriate.
But I am not claiming we need to switch all code away
from structs. If you like it like this, keep it around.
We did this for some structures and I'm thinking it's a good direction
generally.
--
Shannon
--
Shannon
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On Wed, Jun 17, 2015 at 09:06:47AM +0800, Shannon Zhao wrote:
On 2015/6/16 22:19, Michael S. Tsirkin wrote:
On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
On 2015/6/16 2:13, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
On 15 June 2015 at 17:32, Andrew Jones drjo...@redhat.com wrote:
On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0x anyway. I can change those two to make them more
explicit,
if that's preferred.
Yep, I just looked over the struct definition, so since this
has been reviewed I'll apply it to target-arm.next.
You could probably make it easier to review and write
code that has to do these endianness swaps with something
like
#define acpi_struct_assign(FIELD, VAL) \
((FIELD) = \
__builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
__builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
abort
(untested, but based on some code in linux-user/qemu.h).
Then it's always
acpi_struct_assign(spcr-field, value);
whether the field is 1, 2, 4 or 8 bytes.
Not my bit of the codebase though, so I'll leave it to the
ACPI maintainers to decide how much they like magic macros :-)
thanks
-- PMM
We don't much. One can use build_append_int_noprefix and just avoid
structs altogether.
But if we use build_append_int_noprefix, we have to bother about the
unused fields of the struct and have lots of
build_append_int_noprefix(table, 0, 1/2/4/8).
With a struct you have a bunch of reserved fields - is that very
different?
Not only about the reserved fields, but also the fields which ARM
doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct
AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use
build_append_int_noprefix, we should add lots of
build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we
just need to care them when we define it, rather than every time we use.
I've had a chat with Igor about this, and thought about it, and I'm now
in the build_append camp. It took me a while to see that point of view,
as it isn't a pattern I'm familiar with. However, the pattern, which is
* table generation code is a sequence of build_appends, commented with
strings matching strings in the spec
* table generation code has the minimal number of parameters necessary
to be useful for all users, all other table values have default values
filled (typically zero)
* the parameters to the table generation code can be packed in a param
struct that has descriptive member names, allowing the caller to
still use the struct initializing type pattern, and to more cleanly
manage the parameters
with the benefits of
* structs (the param structs) stay small
* callers don't have to worry about endianness
* maximum code sharing across users
* no need to try and reproduce the spec as a struct definition, which
can easily explode with comments/enums trying to describe each field
accurately
So, I think it would be nice to convert ARM's ACPI generation over to
this type of pattern, which may allow more merging of ARM and x86.
However, that said, it'd be good to get the endianness fix patch and
the gicv2m patch in sooner than later. Maybe we should start with those
patches (just using cpu_to_le*), and then consider a rework of the
struct based table generation, before continuing to extend it.
Thanks,
drew
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
On 2015/6/16 2:13, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
On 15 June 2015 at 17:32, Andrew Jones drjo...@redhat.com wrote:
On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0x anyway. I can change those two to make them more explicit,
if that's preferred.
Yep, I just looked over the struct definition, so since this
has been reviewed I'll apply it to target-arm.next.
You could probably make it easier to review and write
code that has to do these endianness swaps with something
like
#define acpi_struct_assign(FIELD, VAL) \
((FIELD) = \
__builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
__builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
abort
(untested, but based on some code in linux-user/qemu.h).
Then it's always
acpi_struct_assign(spcr-field, value);
whether the field is 1, 2, 4 or 8 bytes.
Not my bit of the codebase though, so I'll leave it to the
ACPI maintainers to decide how much they like magic macros :-)
thanks
-- PMM
We don't much. One can use build_append_int_noprefix and just avoid
structs altogether.
But if we use build_append_int_noprefix, we have to bother about the
unused fields of the struct and have lots of
build_append_int_noprefix(table, 0, 1/2/4/8).
With a struct you have a bunch of reserved fields - is that very
different?
We did this for some structures and I'm thinking it's a good direction
generally.
--
Shannon
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On Tue, 16 Jun 2015 09:33:19 +0800
Shannon Zhao shannon.z...@linaro.org wrote:
On 2015/6/16 2:13, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
On 15 June 2015 at 17:32, Andrew Jones drjo...@redhat.com wrote:
On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0x anyway. I can change those two to make them more explicit,
if that's preferred.
Yep, I just looked over the struct definition, so since this
has been reviewed I'll apply it to target-arm.next.
You could probably make it easier to review and write
code that has to do these endianness swaps with something
like
#define acpi_struct_assign(FIELD, VAL) \
((FIELD) = \
__builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
__builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
abort
(untested, but based on some code in linux-user/qemu.h).
Then it's always
acpi_struct_assign(spcr-field, value);
whether the field is 1, 2, 4 or 8 bytes.
Not my bit of the codebase though, so I'll leave it to the
ACPI maintainers to decide how much they like magic macros :-)
thanks
-- PMM
We don't much. One can use build_append_int_noprefix and just avoid
structs altogether.
But if we use build_append_int_noprefix, we have to bother about the
unused fields of the struct and have lots of
build_append_int_noprefix(table, 0, 1/2/4/8).
that would be drop in replacement for struct
(i.e. you'll just use build_append_int_noprefix instead of struct)
It's easier to review either since it repeats table descriptions
from spec practically 1:1 and there is no need to invent names for
struct fields anymore.
this approach is used in aml_build.c and so far works well.
We did this for some structures and I'm thinking it's a good direction
generally.
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On 2015/6/16 22:19, Michael S. Tsirkin wrote:
On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
On 2015/6/16 2:13, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
On 15 June 2015 at 17:32, Andrew Jones drjo...@redhat.com wrote:
On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0x anyway. I can change those two to make them more explicit,
if that's preferred.
Yep, I just looked over the struct definition, so since this
has been reviewed I'll apply it to target-arm.next.
You could probably make it easier to review and write
code that has to do these endianness swaps with something
like
#define acpi_struct_assign(FIELD, VAL) \
((FIELD) = \
__builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
__builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
abort
(untested, but based on some code in linux-user/qemu.h).
Then it's always
acpi_struct_assign(spcr-field, value);
whether the field is 1, 2, 4 or 8 bytes.
Not my bit of the codebase though, so I'll leave it to the
ACPI maintainers to decide how much they like magic macros :-)
thanks
-- PMM
We don't much. One can use build_append_int_noprefix and just avoid
structs altogether.
But if we use build_append_int_noprefix, we have to bother about the
unused fields of the struct and have lots of
build_append_int_noprefix(table, 0, 1/2/4/8).
With a struct you have a bunch of reserved fields - is that very
different?
Not only about the reserved fields, but also the fields which ARM
doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct
AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use
build_append_int_noprefix, we should add lots of
build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we
just need to care them when we define it, rather than every time we use.
We did this for some structures and I'm thinking it's a good direction
generally.
--
Shannon
--
Shannon
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On 10 June 2015 at 10:52, Andrew Jones drjo...@redhat.com wrote:
Signed-off-by: Andrew Jones drjo...@redhat.com
Tested-by: Shannon Zhao shannon.z...@linaro.org
---
hw/arm/virt-acpi-build.c | 43 ++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a9373ccaca6cb..d5a8b9c0178ea 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -84,6 +84,12 @@ static void acpi_dsdt_add_uart(Aml *scope, const
MemMapEntry *uart_memmap,
aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
AML_EXCLUSIVE, uart_irq));
aml_append(dev, aml_name_decl(_CRS, crs));
+
+/* The _ADR entry is used to link this device to the UART described
+ * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
+ */
+aml_append(dev, aml_name_decl(_ADR, aml_int(uart_memmap-base)));
+
aml_append(scope, dev);
}
@@ -334,6 +340,38 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned
rsdt)
}
static void
+build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
+{
+AcpiSerialPortConsoleRedirection *spcr;
+const MemMapEntry *uart_memmap = guest_info-memmap[VIRT_UART];
+int irq = guest_info-irqmap[VIRT_UART] + ARM_SPI_BASE;
+
+spcr = acpi_data_push(table_data, sizeof(*spcr));
+
+spcr-interface_type = 0x3;/* ARM PL011 UART */
+
+spcr-base_address.space_id = AML_SYSTEM_MEMORY;
+spcr-base_address.bit_width = 8;
+spcr-base_address.bit_offset = 0;
+spcr-base_address.access_width = 1;
+spcr-base_address.address = cpu_to_le64(uart_memmap-base);
+
+spcr-interrupt_types = (1 3); /* Bit[3] ARMH GIC interrupt */
+spcr-gsi = cpu_to_le32(irq); /* Global System Interrupt */
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
thanks
-- PMM
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On 2015/6/16 2:13, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
On 15 June 2015 at 17:32, Andrew Jones drjo...@redhat.com wrote:
On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0x anyway. I can change those two to make them more explicit,
if that's preferred.
Yep, I just looked over the struct definition, so since this
has been reviewed I'll apply it to target-arm.next.
You could probably make it easier to review and write
code that has to do these endianness swaps with something
like
#define acpi_struct_assign(FIELD, VAL) \
((FIELD) = \
__builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
__builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
abort
(untested, but based on some code in linux-user/qemu.h).
Then it's always
acpi_struct_assign(spcr-field, value);
whether the field is 1, 2, 4 or 8 bytes.
Not my bit of the codebase though, so I'll leave it to the
ACPI maintainers to decide how much they like magic macros :-)
thanks
-- PMM
We don't much. One can use build_append_int_noprefix and just avoid
structs altogether.
But if we use build_append_int_noprefix, we have to bother about the
unused fields of the struct and have lots of
build_append_int_noprefix(table, 0, 1/2/4/8).
We did this for some structures and I'm thinking it's a good direction
generally.
--
Shannon
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
On 10 June 2015 at 10:52, Andrew Jones drjo...@redhat.com wrote:
Signed-off-by: Andrew Jones drjo...@redhat.com
Tested-by: Shannon Zhao shannon.z...@linaro.org
---
hw/arm/virt-acpi-build.c | 43 ++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a9373ccaca6cb..d5a8b9c0178ea 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -84,6 +84,12 @@ static void acpi_dsdt_add_uart(Aml *scope, const
MemMapEntry *uart_memmap,
aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
AML_EXCLUSIVE, uart_irq));
aml_append(dev, aml_name_decl(_CRS, crs));
+
+/* The _ADR entry is used to link this device to the UART described
+ * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
+ */
+aml_append(dev, aml_name_decl(_ADR, aml_int(uart_memmap-base)));
+
aml_append(scope, dev);
}
@@ -334,6 +340,38 @@ build_rsdp(GArray *rsdp_table, GArray *linker,
unsigned rsdt)
}
static void
+build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
+{
+AcpiSerialPortConsoleRedirection *spcr;
+const MemMapEntry *uart_memmap = guest_info-memmap[VIRT_UART];
+int irq = guest_info-irqmap[VIRT_UART] + ARM_SPI_BASE;
+
+spcr = acpi_data_push(table_data, sizeof(*spcr));
+
+spcr-interface_type = 0x3;/* ARM PL011 UART */
+
+spcr-base_address.space_id = AML_SYSTEM_MEMORY;
+spcr-base_address.bit_width = 8;
+spcr-base_address.bit_offset = 0;
+spcr-base_address.access_width = 1;
+spcr-base_address.address = cpu_to_le64(uart_memmap-base);
+
+spcr-interrupt_types = (1 3); /* Bit[3] ARMH GIC interrupt */
+spcr-gsi = cpu_to_le32(irq); /* Global System Interrupt */
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
thanks
-- PMM
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
--
MST
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
On 15 June 2015 at 17:32, Andrew Jones drjo...@redhat.com wrote:
On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0x anyway. I can change those two to make them more explicit,
if that's preferred.
Yep, I just looked over the struct definition, so since this
has been reviewed I'll apply it to target-arm.next.
You could probably make it easier to review and write
code that has to do these endianness swaps with something
like
#define acpi_struct_assign(FIELD, VAL) \
((FIELD) = \
__builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
__builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
abort
(untested, but based on some code in linux-user/qemu.h).
Then it's always
acpi_struct_assign(spcr-field, value);
whether the field is 1, 2, 4 or 8 bytes.
Not my bit of the codebase though, so I'll leave it to the
ACPI maintainers to decide how much they like magic macros :-)
thanks
-- PMM
We don't much. One can use build_append_int_noprefix and just avoid
structs altogether.
We did this for some structures and I'm thinking it's a good direction
generally.
--
MST
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On 15 June 2015 at 17:32, Andrew Jones drjo...@redhat.com wrote:
On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0x anyway. I can change those two to make them more explicit,
if that's preferred.
Yep, I just looked over the struct definition, so since this
has been reviewed I'll apply it to target-arm.next.
You could probably make it easier to review and write
code that has to do these endianness swaps with something
like
#define acpi_struct_assign(FIELD, VAL) \
((FIELD) = \
__builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
__builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
__builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
abort
(untested, but based on some code in linux-user/qemu.h).
Then it's always
acpi_struct_assign(spcr-field, value);
whether the field is 1, 2, 4 or 8 bytes.
Not my bit of the codebase though, so I'll leave it to the
ACPI maintainers to decide how much they like magic macros :-)
thanks
-- PMM
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
On 10 June 2015 at 10:52, Andrew Jones drjo...@redhat.com wrote:
Signed-off-by: Andrew Jones drjo...@redhat.com
Tested-by: Shannon Zhao shannon.z...@linaro.org
---
hw/arm/virt-acpi-build.c | 43 ++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a9373ccaca6cb..d5a8b9c0178ea 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -84,6 +84,12 @@ static void acpi_dsdt_add_uart(Aml *scope, const
MemMapEntry *uart_memmap,
aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
AML_EXCLUSIVE, uart_irq));
aml_append(dev, aml_name_decl(_CRS, crs));
+
+/* The _ADR entry is used to link this device to the UART described
+ * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
+ */
+aml_append(dev, aml_name_decl(_ADR, aml_int(uart_memmap-base)));
+
aml_append(scope, dev);
}
@@ -334,6 +340,38 @@ build_rsdp(GArray *rsdp_table, GArray *linker,
unsigned rsdt)
}
static void
+build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
+{
+AcpiSerialPortConsoleRedirection *spcr;
+const MemMapEntry *uart_memmap = guest_info-memmap[VIRT_UART];
+int irq = guest_info-irqmap[VIRT_UART] + ARM_SPI_BASE;
+
+spcr = acpi_data_push(table_data, sizeof(*spcr));
+
+spcr-interface_type = 0x3;/* ARM PL011 UART */
+
+spcr-base_address.space_id = AML_SYSTEM_MEMORY;
+spcr-base_address.bit_width = 8;
+spcr-base_address.bit_offset = 0;
+spcr-base_address.access_width = 1;
+spcr-base_address.address = cpu_to_le64(uart_memmap-base);
+
+spcr-interrupt_types = (1 3); /* Bit[3] ARMH GIC interrupt */
+spcr-gsi = cpu_to_le32(irq); /* Global System Interrupt */
I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?
thanks
-- PMM
Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.
Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0x anyway. I can change those two to make them more explicit,
if that's preferred.
Thanks,
drew
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On Wed, 10 Jun 2015 05:52:39 -0400
Andrew Jones drjo...@redhat.com wrote:
Signed-off-by: Andrew Jones drjo...@redhat.com
Tested-by: Shannon Zhao shannon.z...@linaro.org
Reviewed-by: Igor Mammedov imamm...@redhat.com
---
hw/arm/virt-acpi-build.c | 43 ++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a9373ccaca6cb..d5a8b9c0178ea 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -84,6 +84,12 @@ static void acpi_dsdt_add_uart(Aml *scope, const
MemMapEntry *uart_memmap,
aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
AML_EXCLUSIVE, uart_irq));
aml_append(dev, aml_name_decl(_CRS, crs));
+
+/* The _ADR entry is used to link this device to the UART described
+ * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
+ */
+aml_append(dev, aml_name_decl(_ADR, aml_int(uart_memmap-base)));
+
aml_append(scope, dev);
}
@@ -334,6 +340,38 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned
rsdt)
}
static void
+build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
+{
+AcpiSerialPortConsoleRedirection *spcr;
+const MemMapEntry *uart_memmap = guest_info-memmap[VIRT_UART];
+int irq = guest_info-irqmap[VIRT_UART] + ARM_SPI_BASE;
+
+spcr = acpi_data_push(table_data, sizeof(*spcr));
+
+spcr-interface_type = 0x3;/* ARM PL011 UART */
+
+spcr-base_address.space_id = AML_SYSTEM_MEMORY;
+spcr-base_address.bit_width = 8;
+spcr-base_address.bit_offset = 0;
+spcr-base_address.access_width = 1;
+spcr-base_address.address = cpu_to_le64(uart_memmap-base);
+
+spcr-interrupt_types = (1 3); /* Bit[3] ARMH GIC interrupt */
+spcr-gsi = cpu_to_le32(irq); /* Global System Interrupt */
+
+spcr-baud = 3;/* Baud Rate: 3 = 9600 */
+spcr-parity = 0; /* No Parity */
+spcr-stopbits = 1;/* 1 Stop bit */
+spcr-flowctrl = (1 1); /* Bit[1] = RTS/CTS hardware flow control
*/
+spcr-term_type = 0; /* Terminal Type: 0 = VT100 */
+
+spcr-pci_device_id = 0x; /* PCI Device ID: not a PCI device */
+spcr-pci_vendor_id = 0x; /* PCI Vendor ID: not a PCI device */
+
+build_header(linker, table_data, (void *)spcr, SPCR, sizeof(*spcr), 2);
+}
+
+static void
build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
{
AcpiTableMcfg *mcfg;
@@ -514,7 +552,7 @@ void virt_acpi_build(VirtGuestInfo *guest_info,
AcpiBuildTables *tables)
dsdt = tables_blob-len;
build_dsdt(tables_blob, tables-linker, guest_info);
-/* FADT MADT GTDT pointed to by RSDT */
+/* FADT MADT GTDT SPCR pointed to by RSDT */
acpi_add_table(table_offsets, tables_blob);
build_fadt(tables_blob, tables-linker, dsdt);
@@ -527,6 +565,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info,
AcpiBuildTables *tables)
acpi_add_table(table_offsets, tables_blob);
build_mcfg(tables_blob, tables-linker, guest_info);
+acpi_add_table(table_offsets, tables_blob);
+build_spcr(tables_blob, tables-linker, guest_info);
+
/* RSDT is pointed to by RSDP */
rsdt = tables_blob-len;
build_rsdt(tables_blob, tables-linker, table_offsets);
[Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
Signed-off-by: Andrew Jones drjo...@redhat.com
Tested-by: Shannon Zhao shannon.z...@linaro.org
---
hw/arm/virt-acpi-build.c | 43 ++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a9373ccaca6cb..d5a8b9c0178ea 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -84,6 +84,12 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry
*uart_memmap,
aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
AML_EXCLUSIVE, uart_irq));
aml_append(dev, aml_name_decl(_CRS, crs));
+
+/* The _ADR entry is used to link this device to the UART described
+ * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
+ */
+aml_append(dev, aml_name_decl(_ADR, aml_int(uart_memmap-base)));
+
aml_append(scope, dev);
}
@@ -334,6 +340,38 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned
rsdt)
}
static void
+build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
+{
+AcpiSerialPortConsoleRedirection *spcr;
+const MemMapEntry *uart_memmap = guest_info-memmap[VIRT_UART];
+int irq = guest_info-irqmap[VIRT_UART] + ARM_SPI_BASE;
+
+spcr = acpi_data_push(table_data, sizeof(*spcr));
+
+spcr-interface_type = 0x3;/* ARM PL011 UART */
+
+spcr-base_address.space_id = AML_SYSTEM_MEMORY;
+spcr-base_address.bit_width = 8;
+spcr-base_address.bit_offset = 0;
+spcr-base_address.access_width = 1;
+spcr-base_address.address = cpu_to_le64(uart_memmap-base);
+
+spcr-interrupt_types = (1 3); /* Bit[3] ARMH GIC interrupt */
+spcr-gsi = cpu_to_le32(irq); /* Global System Interrupt */
+
+spcr-baud = 3;/* Baud Rate: 3 = 9600 */
+spcr-parity = 0; /* No Parity */
+spcr-stopbits = 1;/* 1 Stop bit */
+spcr-flowctrl = (1 1); /* Bit[1] = RTS/CTS hardware flow control */
+spcr-term_type = 0; /* Terminal Type: 0 = VT100 */
+
+spcr-pci_device_id = 0x; /* PCI Device ID: not a PCI device */
+spcr-pci_vendor_id = 0x; /* PCI Vendor ID: not a PCI device */
+
+build_header(linker, table_data, (void *)spcr, SPCR, sizeof(*spcr), 2);
+}
+
+static void
build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
{
AcpiTableMcfg *mcfg;
@@ -514,7 +552,7 @@ void virt_acpi_build(VirtGuestInfo *guest_info,
AcpiBuildTables *tables)
dsdt = tables_blob-len;
build_dsdt(tables_blob, tables-linker, guest_info);
-/* FADT MADT GTDT pointed to by RSDT */
+/* FADT MADT GTDT SPCR pointed to by RSDT */
acpi_add_table(table_offsets, tables_blob);
build_fadt(tables_blob, tables-linker, dsdt);
@@ -527,6 +565,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info,
AcpiBuildTables *tables)
acpi_add_table(table_offsets, tables_blob);
build_mcfg(tables_blob, tables-linker, guest_info);
+acpi_add_table(table_offsets, tables_blob);
+build_spcr(tables_blob, tables-linker, guest_info);
+
/* RSDT is pointed to by RSDP */
rsdt = tables_blob-len;
build_rsdt(tables_blob, tables-linker, table_offsets);
--
1.8.3.1
Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add SPCR table
On Wed, Jun 10, 2015 at 05:52:39AM -0400, Andrew Jones wrote:
Signed-off-by: Andrew Jones drjo...@redhat.com
Tested-by: Shannon Zhao shannon.z...@linaro.org
Acked-by: Michael S. Tsirkin m...@redhat.com
---
hw/arm/virt-acpi-build.c | 43 ++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a9373ccaca6cb..d5a8b9c0178ea 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -84,6 +84,12 @@ static void acpi_dsdt_add_uart(Aml *scope, const
MemMapEntry *uart_memmap,
aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
AML_EXCLUSIVE, uart_irq));
aml_append(dev, aml_name_decl(_CRS, crs));
+
+/* The _ADR entry is used to link this device to the UART described
+ * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
+ */
+aml_append(dev, aml_name_decl(_ADR, aml_int(uart_memmap-base)));
+
aml_append(scope, dev);
}
@@ -334,6 +340,38 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned
rsdt)
}
static void
+build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
+{
+AcpiSerialPortConsoleRedirection *spcr;
+const MemMapEntry *uart_memmap = guest_info-memmap[VIRT_UART];
+int irq = guest_info-irqmap[VIRT_UART] + ARM_SPI_BASE;
+
+spcr = acpi_data_push(table_data, sizeof(*spcr));
+
+spcr-interface_type = 0x3;/* ARM PL011 UART */
+
+spcr-base_address.space_id = AML_SYSTEM_MEMORY;
+spcr-base_address.bit_width = 8;
+spcr-base_address.bit_offset = 0;
+spcr-base_address.access_width = 1;
+spcr-base_address.address = cpu_to_le64(uart_memmap-base);
+
+spcr-interrupt_types = (1 3); /* Bit[3] ARMH GIC interrupt */
+spcr-gsi = cpu_to_le32(irq); /* Global System Interrupt */
+
+spcr-baud = 3;/* Baud Rate: 3 = 9600 */
+spcr-parity = 0; /* No Parity */
+spcr-stopbits = 1;/* 1 Stop bit */
+spcr-flowctrl = (1 1); /* Bit[1] = RTS/CTS hardware flow control
*/
+spcr-term_type = 0; /* Terminal Type: 0 = VT100 */
+
+spcr-pci_device_id = 0x; /* PCI Device ID: not a PCI device */
+spcr-pci_vendor_id = 0x; /* PCI Vendor ID: not a PCI device */
+
+build_header(linker, table_data, (void *)spcr, SPCR, sizeof(*spcr), 2);
+}
+
+static void
build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
{
AcpiTableMcfg *mcfg;
@@ -514,7 +552,7 @@ void virt_acpi_build(VirtGuestInfo *guest_info,
AcpiBuildTables *tables)
dsdt = tables_blob-len;
build_dsdt(tables_blob, tables-linker, guest_info);
-/* FADT MADT GTDT pointed to by RSDT */
+/* FADT MADT GTDT SPCR pointed to by RSDT */
acpi_add_table(table_offsets, tables_blob);
build_fadt(tables_blob, tables-linker, dsdt);
@@ -527,6 +565,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info,
AcpiBuildTables *tables)
acpi_add_table(table_offsets, tables_blob);
build_mcfg(tables_blob, tables-linker, guest_info);
+acpi_add_table(table_offsets, tables_blob);
+build_spcr(tables_blob, tables-linker, guest_info);
+
/* RSDT is pointed to by RSDP */
rsdt = tables_blob-len;
build_rsdt(tables_blob, tables-linker, table_offsets);
--
1.8.3.1
