Re: [Qemu-arm] [PATCH v5 2/3] ACPI: Add APEI GHES Table Generation support

[Michael S. Tsirkin]

On Thu, Jul 13, 2017 at 09:41:03PM +0200, Laszlo Ersek wrote:
> >> +
> >> +        error_source++;
> >> +    }
> >> +
> >> +    for (i = 0; i < GHES_ACPI_HEST_NOTIFY_RESERVED; i++) {
> >> +        bios_linker_loader_add_pointer(linker,
> >> +            GHES_ERRORS_FW_CFG_FILE, sizeof(uint64_t) * i, 
> >> sizeof(uint64_t),
> >> +            GHES_ERRORS_FW_CFG_FILE, GHES_ACPI_HEST_NOTIFY_RESERVED *
> >> +            sizeof(uint64_t) + i * GHES_MAX_RAW_DATA_LENGTH);
> >> +    }
> 
> So basically all this math exists to set up the pointers that are shown
> in the diagram in the commit message. It is a bit tricky because most of
> those pointer fields (all 8-bytes wide) are individually embedded into
> their own containing structures. In the previous version of this patch
> set, I painstakingly verified the math, and pointed out wherever I
> thought updates were necessary.
> 
> I agree the math is hard to read, the code is very "dense". My
> suggestion (supporting yours) would be to calculate the fw_cfg blob
> offsets that should be patched in more fine-grained steps, possibly with
> multiple separate increments, using:
> - structure type names,
> - sizeof operators,
> - offsetof macros,
> - and possibly a separate comment for each offset increment.
> 
> Thanks,
> Laszlo

Right. That's not what rest of ACPI does though. What we do there
is one of:

1. Use GArray to gradually build up the structure.

Struct *s = acpi_data_push(table, sizeof (*s));
s->foo = bar;

2. Even simpler: use build_append_int_noprefix:

build_append_int_noprefix(table, 2, bar); /* Foo field */

this removes the need to declare Struct in a header,
just add comments to match the spec exactly.

We might gradually move more code to (2) but (1) is an
OK style too, it makes it obvious the sizes are fine.


-- 
MST