From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 63B031741F0 for ; Thu, 26 Sep 2024 12:01:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.176.79.56 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727352063; cv=none; b=Yarlm7mmR86oC05CwnX20Cvk9OZbSa+JkBF1kh5Xr/CUvUGomA0fstwIXUI+3IYfo1lbwtFRX+aGSjskKNdduFF0CW039nVcRmyxmXonaheBvtutYiWJ3vymbzNqIC4Ho1zob/dWP4yDcU04KKExcGa9pUqPd660GXk7tn+u1Oo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727352063; c=relaxed/simple; bh=Qo4RxcudXypmYwICcrVSlrgLaG8mPa++K/0cPQwmWFA=; h=Date:From:To:CC:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=JNsNi5mEPAV5DdIhMyKeosxHFSyP7yxMAcSxSZnjgL3Mvyc+eAWX925CY2RfvUvf94JvNNezFSXcmz4WkJ4OUG73myaQKqjOdkird3D/th90fUN46OO7AnihHsqFLsMxDRLVCtpa+YxhgxptcbTMAZ6p5s0Wijk7djcYGbN75U4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=Huawei.com; spf=pass smtp.mailfrom=huawei.com; arc=none smtp.client-ip=185.176.79.56 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=Huawei.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=huawei.com Received: from mail.maildlp.com (unknown [172.18.186.31]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4XDsXB37Rpz6LDcy; Thu, 26 Sep 2024 19:56:58 +0800 (CST) Received: from frapeml500008.china.huawei.com (unknown [7.182.85.71]) by mail.maildlp.com (Postfix) with ESMTPS id D3B771401F3; Thu, 26 Sep 2024 20:00:57 +0800 (CST) Received: from localhost (10.203.177.66) by frapeml500008.china.huawei.com (7.182.85.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Thu, 26 Sep 2024 14:00:57 +0200 Date: Thu, 26 Sep 2024 13:00:56 +0100 From: Jonathan Cameron To: Mauro Carvalho Chehab CC: Igor Mammedov , Shiju Jose , "Michael S. Tsirkin" , Ani Sinha , Dongjiu Geng , , , Subject: Re: [PATCH 09/15] acpi/ghes: make the GHES record generation more generic Message-ID: <20240926130056.00001e12@Huawei.com> In-Reply-To: <9b256923695e2202f549cbbb11dc22982cb22abf.1727236561.git.mchehab+huawei@kernel.org> References: <9b256923695e2202f549cbbb11dc22982cb22abf.1727236561.git.mchehab+huawei@kernel.org> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-ClientProxiedBy: lhrpeml500004.china.huawei.com (7.191.163.9) To frapeml500008.china.huawei.com (7.182.85.71) On Wed, 25 Sep 2024 06:04:14 +0200 Mauro Carvalho Chehab wrote: > Split the code into separate functions to allow using the > common CPER filling code by different error sources. > > The generic code was moved to ghes_record_cper_errors(), > and ghes_gen_err_data_uncorrectable_recoverable() now contains > only a logic to fill GEGB part of the record. > > The remaining code to generate a memory error now belongs to > acpi_ghes_record_errors() function. > > A further patch will give it a better name. That bit is fine, I'm less sure about ghes_gen_err_data_uncorrectable_recoverable() Maybe you refactor that later, but I'd suggest doing so in this patch to make it ghes_gen_data() with the uncorrectable and recoverable bits passed in as parameters. Jonathan > > Signed-off-by: Mauro Carvalho Chehab > --- > hw/acpi/ghes.c | 118 +++++++++++++++++++++++++---------------- > include/hw/acpi/ghes.h | 3 ++ > 2 files changed, 74 insertions(+), 47 deletions(-) > > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c > index 340a0263faf8..307b5a41d539 100644 > --- a/hw/acpi/ghes.c > +++ b/hw/acpi/ghes.c > @@ -181,51 +181,30 @@ static void acpi_ghes_build_append_mem_cper(GArray *table, > build_append_int_noprefix(table, 0, 7); > } > > -static int acpi_ghes_record_mem_error(uint64_t error_block_address, > - uint64_t error_physical_addr) > +static void > +ghes_gen_err_data_uncorrectable_recoverable(GArray *block, > + const uint8_t *section_type, > + int data_length) > { That's an ugly name . Suggestion below on instead passing parameters for the uncorrectable and recoverable parts and amking this ghes_gen_err_data() > - GArray *block; > - > - /* Memory Error Section Type */ > - const uint8_t uefi_cper_mem_sec[] = > - UUID_LE(0xA5BC1114, 0x6F64, 0x4EDE, 0xB8, 0x63, 0x3E, 0x83, \ > - 0xED, 0x7C, 0x83, 0xB1); > - > /* invalid fru id: ACPI 4.0: 17.3.2.6.1 Generic Error Data, > * Table 17-13 Generic Error Data Entry > */ > QemuUUID fru_id = {}; > - uint32_t data_length; > > - block = g_array_new(false, true /* clear */, 1); > - > - /* This is the length if adding a new generic error data entry*/ > - data_length = ACPI_GHES_DATA_LENGTH + ACPI_GHES_MEM_CPER_LENGTH; > /* > - * It should not run out of the preallocated memory if adding a new generic > - * error data entry > + * Calculate the size with this block. No need to check for > + * too big CPER, as CPER size is checked at ghes_record_cper_errors() > */ > - assert((data_length + ACPI_GHES_GESB_SIZE) <= > - ACPI_GHES_MAX_RAW_DATA_LENGTH); > + data_length += ACPI_GHES_GESB_SIZE; > > /* Build the new generic error status block header */ > acpi_ghes_generic_error_status(block, ACPI_GEBS_UNCORRECTABLE, > 0, 0, data_length, ACPI_CPER_SEV_RECOVERABLE); > > /* Build this new generic error data entry header */ > - acpi_ghes_generic_error_data(block, uefi_cper_mem_sec, > + acpi_ghes_generic_error_data(block, section_type, > ACPI_CPER_SEV_RECOVERABLE, 0, 0, > ACPI_GHES_MEM_CPER_LENGTH, fru_id, 0); Maybe should just pass in ACPI_CPER_SEV_RECOVERABLE and ACPI_GEBS_UNCORRECTABLE in parameters. Main advantage being that should allow reuse for other combinations and it gets rid of the nasty function name! > - > - /* Build the memory section CPER for above new generic error data entry */ > - acpi_ghes_build_append_mem_cper(block, error_physical_addr); > - > - /* Write the generic error data entry into guest memory */ > - cpu_physical_memory_write(error_block_address, block->data, block->len); > - > - g_array_free(block, true); > - > - return 0; > } > > /* > @@ -399,14 +378,19 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s, > ags->present = true; > } > > -int acpi_ghes_record_errors(uint16_t source_id, uint64_t physical_address) > +void ghes_record_cper_errors(const void *cper, size_t len, > + uint16_t source_id, Error **errp) > { > - return ret; > + read_ack_register = cpu_to_le64(0); > + /* > + * Clear the Read Ack Register, OSPM will write it to 1 when > + * it acknowledges this error. > + */ Indent issue. > + cpu_physical_memory_write(read_ack_register_addr, > + &read_ack_register, sizeof(uint64_t)); > + > + /* Write the generic error data entry into guest memory */ > + cpu_physical_memory_write(error_block_addr, cper, len); > + > + return; > +}