Dear Michael, On 2017/7/14 1:13, Michael S. Tsirkin wrote: > On Thu, Jul 13, 2017 at 05:33:30PM +0200, Laszlo Ersek wrote: >> On 07/13/17 14:00, gengdongjiu wrote: >>> Laszlo, >>> Thank you for your review and comments. >>> >>> >>> On 2017/7/13 18:33, Laszlo Ersek wrote: >>>> On 07/12/17 04:08, Dongjiu Geng wrote: >> >> [snip] >> >>>>> --- a/include/qemu/uuid.h >>>>> +++ b/include/qemu/uuid.h >>>>> @@ -44,6 +44,17 @@ typedef struct { >>>>> >>>>> #define UUID_NONE "00000000-0000-0000-0000-000000000000" >>>>> >>>>> +#define UUID_BE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \ >>>>> +{{{ ((a) >> 24) & 0xff, ((a) >> 16) & 0xff, ((a) >> 8) & 0xff, (a) & >>>>> 0xff, \ >>>>> + ((b) >> 8) & 0xff, (b) & 0xff, \ >>>>> + ((c) >> 8) & 0xff, (c) & 0xff, \ >>>>> + (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) } } } >>>>> + >>>>> +/* Platform Memory, this is from UEFI 2.6 N.2.2 Section Descriptor */ >>>>> +#define UEFI_CPER_SEC_PLATFORM_MEM \ >>>>> + UUID_BE(0xA5BC1114, 0x6F64, 0x4EDE, 0xB8, 0x63, 0x3E, 0x83, \ >>>>> + 0xED, 0x7C, 0x83, 0xB1) >>>>> + >>>>> void qemu_uuid_generate(QemuUUID *out); >>>>> >>>>> int qemu_uuid_is_null(const QemuUUID *uu); >>>>> >>>> >>>> (e) I think the addition of UUID_BE should be split out to a separate >>>> patch; it adds a general facility. It should likely be the very first >>>> patch in the series. >>> Ok. >>> >>>> >>>> (f) While I think it is justified to have UUID_BE() in "qemu/uuid.h", I >>>> think UEFI_CPER_SEC_PLATFORM_MEM is too specific to have here. >>>> >>>> If UEFI_CPER_SEC_PLATFORM_MEM were *not* a standardized UUID, I would >>>> suggest moving it to the implementation ("include/hw/acpi/hest_ghes.h" >>>> -- which in turn should be moved to patch #2, see my remark (d)), *plus* >>>> I would suggest eliminating the new #include from "acpi-defs.h", see my >>>> remark (b). >>> understand your idea. >>> >>>> >>>> However, given that this UUID *is* standard, I suggest keeping the (b) >>>> #include as you currently propose, and to move the definition of >>>> UEFI_CPER_SEC_PLATFORM_MEM to "acpi-defs.h". >>> I agree with you. >>> >>>> >>>> I vaguely recall that Michael commented on this previously, but I don't >>>> remember what he said. Michael, are you OK with my suggestion? >>> Laszlo, I pasted Michael's comments here, as shown below. Michael said >>> the definition >>> should use build_append_int_noprefix to add data. but I think it may not >>> good, becuase >>> the section "UEFI_CPER_SEC_PLATFORM_MEM" is runtime recorded as CPER, not a >>> ACPI/HEST >>> table member, so it is not generated when system boot up. >> >> I agree: the UUID in question is not placed into the ACPI payload / >> fw_cfg blobs, it is written into guest memory at runtime, into the >> firmware-allocated area, if and when there is a hardware error to report. >> >> Thanks >> Laszlo > > The main point is that wrapping it up in a macro with an > unreadable name is not really helpful when it's only > used in a single place.
As I said in another mail, I will move it to the place where it is used with the comment. thanks > > >>> On the other hand,UEFI_CPER_SEC_PLATFORM_MEM >>> definition is from UEFI spec 2.6, N.2.2 Section Descriptor: {0xA5BC1114, >>> 0x6F64, 0x4EDE, {0xB8, 0x63, 0x3E, 0x83, 0xED, 0x7C, 0x83, 0xB1}}. >>> if use build_append_int_noprefix to add, may confuse others. >>> >>> ----------------------------------------------------------------- >>> ----------------------------------------------------------------- >>> There's no reason to define these messy one-time use macros. >>> They just make it hard to look things up in the spec. >>> >>> >>> You can use build_append_int_noprefix to add data of >>> any length in LE format. >>> ----------------------------------------------------------------- >>> ----------------------------------------------------------------- >>> >>> Hi Michael, >>> what is your suggestion about it? do you agree with Laszlo? > > My main point is that the macros do not seem helpful. > > > > . >