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=-12.4 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham 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 251DCC433E9 for ; Thu, 4 Feb 2021 16:30:07 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (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 CCFBB64F5E for ; Thu, 4 Feb 2021 16:30:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CCFBB64F5E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=xen.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.81368.150284 (Exim 4.92) (envelope-from ) id 1l7hVs-0005U8-8Z; Thu, 04 Feb 2021 16:29:48 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 81368.150284; Thu, 04 Feb 2021 16:29:48 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1l7hVs-0005U1-4N; Thu, 04 Feb 2021 16:29:48 +0000 Received: by outflank-mailman (input) for mailman id 81368; Thu, 04 Feb 2021 16:29:46 +0000 Received: from mail.xenproject.org ([104.130.215.37]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1l7hVq-0005Tv-0k for xen-devel@lists.xenproject.org; Thu, 04 Feb 2021 16:29:46 +0000 Received: from xenbits.xenproject.org ([104.239.192.120]) by mail.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1l7hVn-0007Bf-6p; Thu, 04 Feb 2021 16:29:43 +0000 Received: from 54-240-197-234.amazon.com ([54.240.197.234] helo=a483e7b01a66.ant.amazon.com) by xenbits.xenproject.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1l7hVm-0006Oc-Ru; Thu, 04 Feb 2021 16:29:43 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=xen.org; s=20200302mail; h=Content-Transfer-Encoding:Content-Type:In-Reply-To: MIME-Version:Date:Message-ID:From:References:Cc:To:Subject; bh=ijO9rP/V6JlFPhd7aySuFgmF/jjyZb2ilQfvIGxL4j8=; b=ICJ06rhrpoNLSxjDTjhRLLu72G FSns4KuoQyH8cQ08yE/WdKEYGlqwzU3B9jkWkelpHlB6+6XHrL8A5VB2ClfV5rdxmP65AWOs3qPJJ A56m/Cz9ZMYzt1V5NgK5cYpnh9zAzaZVDqKhgPPow/0G5pjWeXzwC2zDipZLL1OAe5XY=; Subject: Re: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses To: Stefano Stabellini , Julien Grall Cc: Elliott Mitchell , xen-devel , Volodymyr Babchuk References: <06d6b9ec-0db9-d6da-e30b-df9f9381157d@xen.org> From: Julien Grall Message-ID: <9b97789b-5560-0186-642a-0501789830e5@xen.org> Date: Thu, 4 Feb 2021 16:29:41 +0000 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit On 04/02/2021 00:13, Stefano Stabellini wrote: > On Wed, 3 Feb 2021, Julien Grall wrote: >> On Wed, 3 Feb 2021 at 22:18, Stefano Stabellini wrote: >>>>> But aside from PCIe, let's say that we know of a few nodes for which >>>>> "reg" needs a special treatment. I am not sure it makes sense to proceed >>>>> with parsing those nodes without knowing how to deal with that. >>>> >>>> I believe that most of the time the "special" treatment would be to ignore the >>>> property "regs" as it will not be an CPU memory address. >>>> >>>>> So maybe >>>>> we should add those nodes to skip_matches until we know what to do with >>>>> them. At that point, I would imagine we would introduce a special >>>>> handle_device function that knows what to do. In the case of PCIe, >>>>> something like "handle_device_pcie". >>>> Could you outline how "handle_device_pcie()" will differ with handle_node()? >>>> >>>> In fact, the problem is not the PCIe node directly. Instead, it is the second >>>> level of nodes below it (i.e usb@...). >>>> >>>> The current implementation of dt_number_of_address() only look at the bus type >>>> of the parent. As the parent has no bus type and "ranges" then it thinks this >>>> is something we can translate to a CPU address. >>>> >>>> However, this is below a PCI bus so the meaning of "reg" is completely >>>> different. In this case, we only need to ignore "reg". >>> >>> I see what you are saying and I agree: if we had to introduce a special >>> case for PCI, then dt_number_of_address() seems to be a good place. In >>> fact, we already have special PCI handling, see our >>> __dt_translate_address function and xen/common/device_tree.c:dt_busses. >>> >>> Which brings the question: why is this actually failing? >> >> I already hinted at the reason in my previous e-mail :). Let me expand >> a bit more. >> >>> >>> pcie0 { >>> ranges = <0x02000000 0x0 0xc0000000 0x6 0x00000000 0x0 0x40000000>; >>> >>> Which means that PCI addresses 0xc0000000-0x100000000 become 0x600000000-0x700000000. >>> >>> The offending DT is: >>> >>> &pcie0 { >>> pci@1,0 { >>> #address-cells = <3>; >>> #size-cells = <2>; >>> ranges; >>> >>> reg = <0 0 0 0 0>; >>> >>> usb@1,0 { >>> reg = <0x10000 0 0 0 0>; >>> resets = <&reset RASPBERRYPI_FIRMWARE_RESET_ID_USB>; >>> }; >>> }; >>> }; >>> >>> >>> reg = <0x10000 0 0 0 0> means that usb@1,0 is PCI device 01:00.0. >>> However, the rest of the regs cells are left as zero. It shouldn't be an >>> issue because usb@1,0 is a child of pci@1,0 but pci@1,0 is not a bus. >> >> The property "ranges" is used to define a mapping or translation >> between the address space of the "bus" (here pci@1,0) and the address >> space of the bus node's parent (&pcie0). >> IOW, it means "reg" in usb@1,0 is an address on the PCI bus (i.e. BDF). >> >> The problem is dt_number_of_address() will only look at the "bus" type >> of the parent using dt_match_bus(). This will return the default bus >> (see dt_bus_default_match()), because this is a property "ranges" in >> the parent node (i.e. pci@1,0). Therefore... >> >>> So >>> in theory dt_number_of_address() should already return 0 for it. >> >> ... dt_number_of_address() will return 1 even if the address is not a >> CPU address. So when Xen will try to translate it, it will fail. >> >>> >>> Maybe reg = <0 0 0 0 0> is the problem. In that case, we could simply >>> add a check to skip 0 size ranges. Just a hack to explain what I mean: >> >> The parent of pci@1,0 is a PCI bridge (see the property type), so the >> CPU addresses are found not via "regs" but "assigned-addresses". >> >> In this situation, "regs" will have a different meaning and therefore >> there is no promise that the size will be 0. > > I copy/pasted the following: > > pci@1,0 { > #address-cells = <3>; > #size-cells = <2>; > ranges; > > reg = <0 0 0 0 0>; > > usb@1,0 { > reg = <0x10000 0 0 0 0>; > resets = <&reset > RASPBERRYPI_FIRMWARE_RESET_ID_USB>; > }; > }; > > under pcie0 in my DTS to see what happens (the node is not there in the > device tree for the rpi-5.9.y kernel.) It results in the expected error: > > (XEN) Unable to retrieve address 0 for /scb/pcie@7d500000/pci@1,0/usb@1,0 > (XEN) Device tree generation failed (-22). > > I could verify that pci@1,0 is seen as "default" bus due to the range > property, thus dt_number_of_address() returns 1. > > > I can see that reg = <0 0 0 0 0> is not a problem because it is ignored > given that the parent is a PCI bus. assigned-addresses is the one that > is read. > > > But from a device tree perspective I am actually confused by the > presence of the "ranges" property under pci@1,0. Is that correct? It is > stating that addresses of children devices will be translated to the > address space of the parent (pcie0) using the parent translation rules. > I mean -- it looks like Xen is right in trying to translate reg = > <0x10000 0 0 0 0> using ranges = <0x02000000 0x0 0xc0000000 0x6 > 0x00000000 0x0 0x40000000>. > > Or maybe since pcie0 is a PCI bus all the children addresses, even > grand-children, are expected to be specified using "assigned-addresses"? > > > Looking at other examples [1][2] maybe the mistake is that pci@1,0 is > missing device_type = "pci"? Of course, if I add that, the error > disappear. I am afraid, I don't know the answer. I think it would be best to ask the Linux DT folks about it. > > [1] Documentation/devicetree/bindings/pci/mvebu-pci.txt > [2] Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt > > For the sake of making Xen more resilient to possible DTSes, maybe we > should try to extend the dt_bus_pci_match check? See for instance the > change below, but we might be able to come up with better ideas. > > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index 18825e333e..24d998f725 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -565,12 +565,21 @@ static unsigned int dt_bus_default_get_flags(const __be32 *addr) > > static bool_t dt_bus_pci_match(const struct dt_device_node *np) > { > + bool ret = false; > + > /* > * "pciex" is PCI Express "vci" is for the /chaos bridge on 1st-gen PCI > * powermacs "ht" is hypertransport > */ > - return !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") || > + ret = !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") || > !strcmp(np->type, "vci") || !strcmp(np->type, "ht"); > + > + if ( ret ) return ret; > + > + if ( !strcmp(np->name, "pci") ) > + ret = dt_bus_pci_match(dt_get_parent(np)); It is probably safe to assume that a PCI device (not hostbridge) will start with "pci". Although, I don't much like the idea because the name is not meant to be stable. AFAICT, we can only rely on "compatible" and "type". Cheers, -- Julien Grall