acpi_tables: fix the access_size of GenericAddress #38
Conversation
Lencerf
requested review from
jiangliu,
liuw,
rbradford and
sboeuf
as code owners
src/sdt.rs
Outdated
| @@ -25,7 +25,7 @@ impl GenericAddress { | |||
| address_space_id: 1, | |||
| register_bit_width: 8 * core::mem::size_of::<T>() as u8, | |||
| register_bit_offset: 0, | |||
| access_size: core::mem::size_of::<T>() as u8, | |||
| access_size: core::mem::size_of::<T>().trailing_zeros() as u8 + 1, | |||
There was a problem hiding this comment.
I'm not sure I would have done it like this instead i'd have had an enum for the access sizes and then mapped the byte length into that and then made it #[repr(u8)] and used that. But this trailing_zeros solution does match QEMU.
There was a problem hiding this comment.
How about the following?
let access_size = match core::mem::size_of::<T>() {
1 => 1,
2 => 2,
4 => 3,
8 => 4,
_ => 0,
};If it looks fine, the next question is, since this is a public API and there is no way to return an error, in the _ case, should we silently set it to 0 (Undefined (legacy reasons)), or just panic?
There was a problem hiding this comment.
How about the following?
let access_size = match core::mem::size_of::<T>() { 1 => 1, 2 => 2, 4 => 3, 8 => 4, _ => 0, };
If it looks fine, the next question is, since this is a public API and there is no way to return an error, in the
_case, should we silently set it to 0 (Undefined (legacy reasons)), or just panic?
Sounds good - you can put that as a method on GenericAddress. Use unreachable!() to panic for unsupported types.
For now I don't think this needs to be public so it can be a private method.
There was a problem hiding this comment.
Done. Please review the new commit.
Without the fix, the code gives
1 for Byte access
2 for Word access
4 for Dword access
8 for QWord access.
However, As per ACPI 6.4 spec Sec.5.2.3.2 [1], the access_size should
be
1 for Byte access
2 for Word access
3 for Dword access
4 for QWord access.
[1] https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#generic-address-structure-gas
Signed-off-by: Changyuan Lyu <changyuanl@google.com>
Summary of the PR
Without the fix, the code gives
1 for Byte access
2 for Word access
4 for Dword access
8 for QWord access.
However, As per ACPI 6.4 spec Sec.5.2.3.2 [1], the access_size should be
1 for Byte access
2 for Word access
3 for Dword access
4 for QWord access.
The fix takes QEMU's implementation[2] as a reference. A related patch[3] has been merged into crosvm.
[1] https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#generic-address-structure-gas
[2] https://github.com/qemu/qemu/blob/6a4180af9686830d88c387baab6d79563ce42a15/hw/acpi/erst.c#L218
[3] https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5394229
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.