From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A152CC33C8C for ; Mon, 6 Jan 2020 17:19:41 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 67E6220715 for ; Mon, 6 Jan 2020 17:19:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="cK69Lcl9" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 67E6220715 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=hnYHIBiddfvZLmJro3t79IECr5XDjUtacgAsAEJTzLM=; b=cK69Lcl9VHKNvpg0JDAr/My0L 7b2jxCjsD2tqap4xiMrj8To7R/xgdPC+GE+j14RGfcgbmIF+D0kQvMOGXf87ymr0uHoZrzqaFIGTL Jbuw4dYi4llDUIohCOsC2s7ZK2GWWW9AGxgajNY+hB63qqugUtn9L7pWaiJZuUFY5uSEegVfWwN7U HdOOK8K2K4sptcS7xpyu4O5zEn6WB/PYi1IQfk9BQlCP4mEgdyuieVDr1fMGaZBL+4e8B7cD4HV0c QLellvjr57QPMCNZxr4o0TAqtS4/EE+T3QtTzEV96NsnR/2H2LxypvoEsx2zxTwhoSxX4bu3yAcoy 9CmmTEQYQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1ioW2W-0008Ep-Tb; Mon, 06 Jan 2020 17:19:40 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1ioW2S-0008Df-VQ for linux-arm-kernel@lists.infradead.org; Mon, 06 Jan 2020 17:19:39 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 37E6A31B; Mon, 6 Jan 2020 09:19:35 -0800 (PST) Received: from [10.1.196.37] (e121345-lin.cambridge.arm.com [10.1.196.37]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0F84C3F6C4; Mon, 6 Jan 2020 09:19:33 -0800 (PST) Subject: Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue To: Hanjun Guo , Lorenzo Pieralisi , Sudeep Holla , "Rafael J. Wysocki" , Pankaj Bansal References: <1577708824-4873-1-git-send-email-guohanjun@huawei.com> From: Robin Murphy Message-ID: <2ce224b2-d926-67b0-f9dd-85ac53d967c5@arm.com> Date: Mon, 6 Jan 2020 17:19:32 +0000 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <1577708824-4873-1-git-send-email-guohanjun@huawei.com> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200106_091937_110594_A0C09781 X-CRM114-Status: GOOD ( 27.33 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-acpi@vger.kernel.org, John Garry , linuxarm@huawei.com, linux-arm-kernel@lists.infradead.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 30/12/2019 12:27 pm, Hanjun Guo wrote: > The IORT spec [0] says Number of IDs = The number of IDs in the range minus > one, it is confusing but it was written down in the first version of the > IORT spec. But the IORT ID mapping function iort_id_map() did something > wrong from the start, which bails out if: > > the request ID >= the input base + number of IDs > > This is wrong because it ignored the "minus one", and breaks some valid > usecases such as ID mapping to contain single device mapping without > single mapping flag set. > > Pankaj Bansal proposed a solution to fix the issue [1], which bails > out if: > > the request ID > the input base + number of IDs > > This works as the spec defined, unfortunately some firmware didn't > minus one for the number of IDs in the range, and the propoased > solution will break those systems in this way: > > PCI hostbridge mapping entry 1: > Input base: 0x1000 > ID Count: 0x100 > Output base: 0x1000 > Output reference: 0xC4 //ITS reference > > PCI hostbridge mapping entry 2: > Input base: 0x1100 > ID Count: 0x100 > Output base: 0x2000 > Output reference: 0xD4 //ITS reference > > Two mapping entries which the second entry's Input base = the first > entry's Input base + ID count, so for requester ID 0x1100 will map > to ITS 0xC4 not 0xD4 if we update '>=' to '>'. > > So introduce a workaround to match the IORT's OEM information for > the broken firmware, also update the logic of the ID mapping for > firmwares report the number of IDs as the IORT spec defined, to > make the code compatible for both kinds of system. > > I checked the ACPI tables in the tianocore/edk2-platforms [2], only > HiSilicon HIP07/08 did wrong, so just add HIP07/08 to the workaround > info table, if we break other platforms, we can add that later. > > [0]: http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf > [1]: https://patchwork.kernel.org/patch/11292823/ > [2]: https://github.com/tianocore/edk2-platforms > > Cc: Pankaj Bansal > Cc: Lorenzo Pieralisi > Signed-off-by: Hanjun Guo > --- > > RFC->v1: > - Print warning when matched the workaround info, suggested by Pankaj. > > drivers/acpi/arm64/iort.c | 55 ++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 52 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 33f7198..60eb10d 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -298,6 +298,42 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node, > return status; > } > > +struct iort_workaround_oem_info { > + char oem_id[ACPI_OEM_ID_SIZE + 1]; > + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1]; > + u32 oem_revision; > +}; > + > +static bool apply_id_count_workaround; > + > +static struct iort_workaround_oem_info wa_info[] __initdata = { > + { > + .oem_id = "HISI ", > + .oem_table_id = "HIP07 ", > + .oem_revision = 0, > + }, { > + .oem_id = "HISI ", > + .oem_table_id = "HIP08 ", > + .oem_revision = 0, > + } > +}; > + > +static void __init > +iort_check_id_count_workaround(struct acpi_table_header *tbl) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(wa_info); i++) { > + if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) && > + !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) && > + wa_info[i].oem_revision == tbl->oem_revision) { > + apply_id_count_workaround = true; > + pr_warn(FW_BUG "ID count for ID mapping entry is wrong, applying workaround\n"); > + break; > + } > + } > +} > + > static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in, > u32 *rid_out) > { > @@ -314,9 +350,21 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in, > return -ENXIO; > } > > - if (rid_in < map->input_base || > - (rid_in >= map->input_base + map->id_count)) > - return -ENXIO; > + /* > + * IORT spec says Number of IDs = The number of IDs in the range minus > + * one, but the IORT code ingored the "minus one", and some firmware > + * did that too, so apply a workaround here to keep compatible with > + * both new and old versions of the firmware. > + */ > + if (apply_id_count_workaround) { > + if (rid_in < map->input_base || > + (rid_in >= map->input_base + map->id_count)) > + return -ENXIO; > + } else { > + if (rid_in < map->input_base || > + (rid_in > map->input_base + map->id_count)) > + return -ENXIO; > + } This seems needlessly repetitive and convoluted... how about refactoring to something like: map_max = map->input_base + map->id_count; if (apply_id_count_workaround) map_max--; ? Robin. > *rid_out = map->output_base + (rid_in - map->input_base); > return 0; > @@ -1631,5 +1679,6 @@ void __init acpi_iort_init(void) > return; > } > > + iort_check_id_count_workaround(iort_table); > iort_init_platform_devices(); > } > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel