On Fri, 1 Sep 2017 17:58:55 +0800
gengdongjiu [off-list ref] wrote:

Hi Igor,

On 2017/8/29 18:20, Igor Mammedov wrote:

quoted

On Fri, 18 Aug 2017 22:23:43 +0800
Dongjiu Geng [off-list ref] wrote:

[...]

quoted

quoted

+void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
+                                            BIOSLinker *linker)
+{
+    uint32_t ghes_start = table_data->len;
+    uint32_t address_size, error_status_address_offset;
+    uint32_t read_ack_register_offset, i;
+
+    address_size = sizeof(struct AcpiGenericAddress) -
+        offsetof(struct AcpiGenericAddress, address);  

it's confusing name for var,
AcpiGenericAddress::address is fixed unsigned 64 bit integer per spec
also, I'm not sure why it's needed at all.  

 it is because other people have concern about where does the "unsigned 64 bit integer"
 come from, they are confused about the "unsigned 64 bit integer"
 so they suggested use sizeof. anyway I will directly use unsigned 64 bit integer.

Maybe properly named macro instead of sizeof(foo) would do the job


[...]

quoted

quoted

+
+        /* Build error status address*/
+        build_address(table_data, linker, error_status_address_offset + i *
+            sizeof(AcpiGenericHardwareErrorSourceV2), i * address_size,
+            AML_SYSTEM_MEMORY, 0x40, 0, 4 /* QWord access */);  

just do something like this instead of build_address():
build_append_gas()
bios_linker_loader_add_pointer()  

Thanks for the suggestion.

quoted

also register width 0x40 looks suspicious, where does it come from?
While at it do you have a real hardware which has HEST table that you re trying to model after?
I'd like to see HEST and other related tables from it.  

Igor, what is your suspicious point? The register width 0x40 come from our host BIOS record to the System Memory space.

maybe s/0x40/ERROR_STATUS_BLOCK_POINTER_SIZE/

[...]