Re: [RESEND PATCH v21 3/6] ACPI: Add APEI GHES table generation support

From: gengdongjiu <gengdongjiu@huawei.com>
To: Igor Mammedov <imammedo@redhat.com>,
	"zhengxiang (A)" <zhengxiang9@huawei.com>
Cc: "pbonzini@redhat.com" <pbonzini@redhat.com>,
	"mst@redhat.com" <mst@redhat.com>,
	"shannon.zhaosl@gmail.com" <shannon.zhaosl@gmail.com>,
	"peter.maydell@linaro.org" <peter.maydell@linaro.org>,
	"lersek@redhat.com" <lersek@redhat.com>,
	"james.morse@arm.com" <james.morse@arm.com>,
	"mtosatti@redhat.com" <mtosatti@redhat.com>,
	"rth@twiddle.net" <rth@twiddle.net>,
	"ehabkost@redhat.com" <ehabkost@redhat.com>,
	"Jonathan Cameron" <jonathan.cameron@huawei.com>,
	"xuwei (O)" <xuwei5@huawei.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"qemu-arm@nongnu.org" <qemu-arm@nongnu.org>,
	Linuxarm <linuxarm@huawei.com>,
	"Wanghaibin (D)" <wanghaibin.wang@huawei.com>
Subject: Re: [RESEND PATCH v21 3/6] ACPI: Add APEI GHES table generation support
Date: Fri, 15 Nov 2019 14:32:47 +0000	[thread overview]
Message-ID: <19b1b4b9ceb24aad9f34ab4e58bccab3@huawei.com> (raw)

> > + */
> > +static void acpi_ghes_build_notify(GArray *table, const uint8_t type)
> 
> typically format should be build_WHAT(), so
>  build_ghes_hw_error_notification()
> 
> And I'd move this out into its own patch.
> this applies to other trivial in-depended sub-tables, that take all data needed to construct them from supplied arguments.

I very used your suggested method in previous series[1], but other maintainer suggested to move this function to this file, because he think only GHES used it

[1]:
https://patchwork.ozlabs.org/cover/1099428/

> 
> > +{
> > +        /* Type */
> > +        build_append_int_noprefix(table, type, 1);
> > +        /*
> > +         * Length:
> > +         * Total length of the structure in bytes
> > +         */
> > +        build_append_int_noprefix(table, 28, 1);
> > +        /* Configuration Write Enable */
> > +        build_append_int_noprefix(table, 0, 2);
> > +        /* Poll Interval */
> > +        build_append_int_noprefix(table, 0, 4);
> > +        /* Vector */
> > +        build_append_int_noprefix(table, 0, 4);
> > +        /* Switch To Polling Threshold Value */
> > +        build_append_int_noprefix(table, 0, 4);
> > +        /* Switch To Polling Threshold Window */
> > +        build_append_int_noprefix(table, 0, 4);
> > +        /* Error Threshold Value */
> > +        build_append_int_noprefix(table, 0, 4);
> > +        /* Error Threshold Window */
> > +        build_append_int_noprefix(table, 0, 4); }
> > +
> 
> /*
>   Initialize "etc/hardware_errors" and "etc/hardware_errors_addr" fwcfg blobs.
>   See docs/specs/acpi_hest_ghes.rst for blobs format */
> > +void acpi_ghes_build_error_table(GArray *hardware_errors, BIOSLinker
> > +*linker)
> build_ghes_error_table()
> 
> also I'd move this function into its own patch along with other related code that initializes and wires it into virt board.

I ever use your suggested method[1], but other maintainer, it seems Michael, suggested to move these functions to this file that used it, because he think only GHES used it.

[1]:
https://patchwork.ozlabs.org/patch/1099424/
https://patchwork.ozlabs.org/patch/1099425/
https://patchwork.ozlabs.org/patch/1099430/