QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Igor Mammedov <imammedo@redhat.com>
To: gengdongjiu <gengdongjiu@huawei.com>
Cc: "peter.maydell@linaro.org" <peter.maydell@linaro.org>,
	"ehabkost@redhat.com" <ehabkost@redhat.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"mst@redhat.com" <mst@redhat.com>,
	"Wanghaibin \(D\)" <wanghaibin.wang@huawei.com>,
	"mtosatti@redhat.com" <mtosatti@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	Linuxarm <linuxarm@huawei.com>,
	"shannon.zhaosl@gmail.com" <shannon.zhaosl@gmail.com>,
	"zhengxiang \(A\)" <zhengxiang9@huawei.com>,
	"qemu-arm@nongnu.org" <qemu-arm@nongnu.org>,
	"james.morse@arm.com" <james.morse@arm.com>,
	"xuwei \(O\)" <xuwei5@huawei.com>,
	Jonathan Cameron <jonathan.cameron@huawei.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"lersek@redhat.com" <lersek@redhat.com>,
	"rth@twiddle.net" <rth@twiddle.net>
Subject: Re: [RESEND PATCH v21 3/6] ACPI: Add APEI GHES table generation support
Date: Fri, 15 Nov 2019 15:55:27 +0100
Message-ID: <20191115155527.287ee95b@redhat.com> (raw)
In-Reply-To: <19b1b4b9ceb24aad9f34ab4e58bccab3@huawei.com>

On Fri, 15 Nov 2019 14:32:47 +0000
gengdongjiu <gengdongjiu@huawei.com> wrote:

> > > + */
> > > +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

Using this file is ok, what I've meant for you to split function from
this patch into a separate smaller patch.

With the way code split now I have to review 2 big complex patches at
the same which makes reviewing hard and I have a hard time convincing
myself that code it correct.

Moving small self-contained chunks of code in to separate smaller patches
makes review easier.

> 
> [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/
> 
> 



  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-15 14:32 gengdongjiu
2019-11-15 14:55 ` Igor Mammedov [this message]
  -- strict thread matches above, loose matches on Subject: below --
2019-11-16  0:58 gengdongjiu
2019-11-11  1:40 [RESEND PATCH v21 0/6] Add ARMv8 RAS virtualization support in QEMU Xiang Zheng
2019-11-11  1:40 ` [RESEND PATCH v21 3/6] ACPI: Add APEI GHES table generation support Xiang Zheng
2019-11-15  9:38   ` Igor Mammedov
2019-11-18 12:49     ` gengdongjiu
2019-11-18 13:18       ` gengdongjiu
2019-11-18 13:21         ` Michael S. Tsirkin
2019-11-18 13:57           ` gengdongjiu
2019-11-25  9:48           ` Igor Mammedov
2019-11-27 11:16             ` gengdongjiu
2019-11-22 15:44       ` Beata Michalska
2019-11-22 15:42   ` Beata Michalska
2019-11-25  9:23     ` Igor Mammedov

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191115155527.287ee95b@redhat.com \
    --to=imammedo@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=gengdongjiu@huawei.com \
    --cc=james.morse@arm.com \
    --cc=jonathan.cameron@huawei.com \
    --cc=kvm@vger.kernel.org \
    --cc=lersek@redhat.com \
    --cc=linuxarm@huawei.com \
    --cc=mst@redhat.com \
    --cc=mtosatti@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=rth@twiddle.net \
    --cc=shannon.zhaosl@gmail.com \
    --cc=wanghaibin.wang@huawei.com \
    --cc=xuwei5@huawei.com \
    --cc=zhengxiang9@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git