[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [PATCH v5 1/3] ACPI: Add new ACPI structures
From: |
gengdongjiu |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH v5 1/3] ACPI: Add new ACPI structures and macros |
Date: |
Fri, 14 Jul 2017 16:25:02 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 |
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.
>
>
>
> .
>
Re: [Qemu-arm] [PATCH v5 1/3] ACPI: Add new ACPI structures and macros, Michael S. Tsirkin, 2017/07/13
Re: [Qemu-arm] [PATCH v5 1/3] ACPI: Add new ACPI structures and macros, Michael S. Tsirkin, 2017/07/13
[Qemu-arm] [PATCH v5 3/3] ACPI: build and enable APEI GHES in the Makefile and configuration, Dongjiu Geng, 2017/07/11
[Qemu-arm] [PATCH v5 2/3] ACPI: Add APEI GHES Table Generation support, Dongjiu Geng, 2017/07/11