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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 0B605C36010 for ; Fri, 4 Apr 2025 08:43:22 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.937941.1338787 (Exim 4.92) (envelope-from ) id 1u0cdo-000426-TN; Fri, 04 Apr 2025 08:43:08 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 937941.1338787; Fri, 04 Apr 2025 08:43:08 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1u0cdo-00041z-Qo; Fri, 04 Apr 2025 08:43:08 +0000 Received: by outflank-mailman (input) for mailman id 937941; Fri, 04 Apr 2025 08:43:07 +0000 Received: from se1-gles-sth1-in.inumbo.com ([159.253.27.254] helo=se1-gles-sth1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1u0cdn-00041t-3l for xen-devel@lists.xenproject.org; Fri, 04 Apr 2025 08:43:07 +0000 Received: from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com [2a00:1450:4864:20::52e]) by se1-gles-sth1.inumbo.com (Halon) with ESMTPS id d3b67d83-1130-11f0-9eaa-5ba50f476ded; Fri, 04 Apr 2025 10:43:05 +0200 (CEST) Received: by mail-ed1-x52e.google.com with SMTP id 4fb4d7f45d1cf-5e6167d0536so3191620a12.1 for ; Fri, 04 Apr 2025 01:43:05 -0700 (PDT) Received: from [192.168.1.5] (user-109-243-64-225.play-internet.pl. [109.243.64.225]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5f0880a45bdsm1942187a12.71.2025.04.04.01.43.03 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 04 Apr 2025 01:43:04 -0700 (PDT) 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" X-Inumbo-ID: d3b67d83-1130-11f0-9eaa-5ba50f476ded DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1743756185; x=1744360985; darn=lists.xenproject.org; h=in-reply-to:from:content-language:references:cc:to:subject :user-agent:mime-version:date:message-id:from:to:cc:subject:date :message-id:reply-to; bh=nMdlDnFeE/ugQuOKWKolBfE2c+xXSu7MDrl9WQXcshg=; b=H0R2BVeTL4Ep3WWQDlLm8RVPEKjK4PoMZgo8V6U+/VaBheHflCXGtWzH3T+NT5D4yx 84QTen+300uaIpxtgJR6Cu1/954irJO5/bhmHmFm+KYvsf0kn9ooVp0DRdHFQUiJfYw9 alyrrm/HHrhaw3zUbtB1HFD45z0nQSrFS2r4NKVqeiYI07WcZALA33Cj6LTNP0OQttqf EdPw/C3r2fpFRQbSYgxuMMwqNvwYJwJyG1gFWz/HtSosESsD788xi6P5i7Owzu4olB0L 1hk4VYCy4B3Tf5gD/tRFDuh9V0sK69lj1MbGLGe2VrwYhE6izzDF2L0gTFNngD9Pf+mb VCxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1743756185; x=1744360985; h=in-reply-to:from:content-language:references:cc:to:subject :user-agent:mime-version:date:message-id:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=nMdlDnFeE/ugQuOKWKolBfE2c+xXSu7MDrl9WQXcshg=; b=RrWJurI8lWYh1Kqg/8yL5EKGmpde7+g6YFhP0uAMyr1JRAUzBzK+B7xbsFu7q6B3xb SlV+Uz8K8vD4bhkAZF1DBI8JYh8aEcHG3mwFgWyWtinPNUs8aSu5JAfD3fmmJvAgI96i cO+T8PnnSZLWyMA4VbknTjGoR5R/wtcyFOZA9RnUkl7W9uuxf/dODaMLSPJkXDqXm4qc 6H3m/sRGZF1VGPR3u4FGKJI2KPsmWYR0ljnNFo/66J7rc3c3i4YJanfjtHubbAGmNXl+ YyZO6dR1ZitjzwI649e5rnWSS20MfZF2MJv5kObAPaC+JA4axKNH+wy3+khyBSNt94W2 5t0A== X-Forwarded-Encrypted: i=1; AJvYcCUahxY62Zk+q2Y/IVQ9mgFBboupYx67VIxjgIfYDWK88wk2n5MBxJRc0Ad1R5BKTEV/nzBh/dU/TVI=@lists.xenproject.org X-Gm-Message-State: AOJu0YxUeg/dmMf5t2zmDaPrXIKzJkwSXOXiEfrUHGrurJKfndc7nPNE 6PU7OHKfksRnJEM1am/TsjGLE3ds+lrji2XyaM9/LPcQgLKRbEal X-Gm-Gg: ASbGncugY6+QVsCMjjETiCEvjySpTe3mX+ASGB4xzXXPOt6BfGGlfk8N+6b8nFmD4gO HTmA40HiKTm53Z9z18wmltorZBQBbQjRenQk1oEz2k5eoIEHuHlYMzAzbzQCL7SvvAk0CLp5ryo bdov4KGHyqCE7+csuZgNOG3WdiFWSwybWvtgl/PbxGq0eNR5x1YpR1FCd1tCfLORck6h/W6BntA hyPUndbalxAxuKD3rGdsUEozek3wXjwTeBTL7CNJm6kDkxxGp7/oPdf5jufTIF5a42Uqlr2cHrV XU0DoqXufyy1wy8xxtcciJoIhQDTDULV2TgIhOcLRdYisfZZUwQFZrWGLopPn5B+uW+J4PCuumX D0RNgIaqX1HcD1R5QuRVeCls5KwkuWZs= X-Google-Smtp-Source: AGHT+IEHTyJ5tHtqj733uXfBNO2G4B2FU3pL1edfgEnysxMnpkFXh3GXyjk14+1hOV+RpizrQDrWKQ== X-Received: by 2002:a05:6402:2708:b0:5f0:9eb3:8e71 with SMTP id 4fb4d7f45d1cf-5f0b3e34eafmr1943326a12.27.1743756184949; Fri, 04 Apr 2025 01:43:04 -0700 (PDT) Content-Type: multipart/alternative; boundary="------------3yWG1vJgau96FWZoQYQnAYCa" Message-ID: Date: Fri, 4 Apr 2025 10:43:02 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1] xen/riscv: Increase XEN_VIRT_SIZE To: Jan Beulich Cc: Alistair Francis , Bob Eshleman , Connor Davis , Andrew Cooper , Anthony PERARD , Michal Orzel , Julien Grall , =?UTF-8?Q?Roger_Pau_Monn=C3=A9?= , Stefano Stabellini , xen-devel@lists.xenproject.org References: <54ebdcb7-071f-411f-803a-930dc330a497@suse.com> <32264ccb-e566-41e0-973f-5bc7d874f970@gmail.com> <9d7e1553-3af8-4fc3-a400-8714d9b68411@suse.com> <30d8e316-aff5-498a-b2bd-448e0b2518ae@gmail.com> <3c2127ec-63fb-457b-8229-fc8a2b9fbf00@suse.com> <14ac3e72-d21d-4b45-a434-d123152c0113@suse.com> Content-Language: en-US From: Oleksii Kurochko In-Reply-To: <14ac3e72-d21d-4b45-a434-d123152c0113@suse.com> This is a multi-part message in MIME format. --------------3yWG1vJgau96FWZoQYQnAYCa Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 4/4/25 9:52 AM, Jan Beulich wrote: > On 04.04.2025 09:31, Oleksii Kurochko wrote: >> On 4/4/25 8:56 AM, Jan Beulich wrote: >>> On 03.04.2025 18:20, Oleksii Kurochko wrote: >>>> On 4/1/25 6:04 PM, Jan Beulich wrote: >>>>> On 01.04.2025 17:58, Oleksii Kurochko wrote: >>>>>> On 3/31/25 6:14 PM, Jan Beulich wrote: >>>>>>> On 31.03.2025 17:20, Oleksii Kurochko wrote: >>>>>>>> + _AC(XEN_VIRT_START, UL) >> vpn1_shift; >>>>>>>> + const unsigned long xen_virt_end_vpn = >>>>>>>> + xen_virt_starn_vpn + ((XEN_VIRT_SIZE >> vpn1_shift) - 1); >>>>>>>> + >>>>>>>> if ((va >= DIRECTMAP_VIRT_START) && >>>>>>>> (va <= DIRECTMAP_VIRT_END)) >>>>>>>> return directmapoff_to_maddr(va - directmap_virt_start); >>>>>>>> >>>>>>>> - BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2)); >>>>>>>> - ASSERT((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) == >>>>>>>> - (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER + PAGE_SHIFT))); >>>>>>>> + BUILD_BUG_ON(XEN_VIRT_SIZE != MB(8)); >>>>>>> Is it necessary to be != ? Won't > suffice? >>>>>> It could be just > MB(2). Or perphaps >=. >>>>>> = would make the build fail, wouldn't it? >>>> I just realized that BUILD_BUG_ON() condition is compared to zero so actually everything what >>>> will make the condition true will cause a build fail as inside it used !(condition). >>> ??? >> |BUILD_BUG_ON()| forces a compilation error if the given condition is true. Therefore, if the condition >> |XEN_VIRT_SIZE != MB(2)| is changed to|XEN_VIRT_SIZE > MB(2)|, the condition will always evaluate to true >> (assuming|XEN_VIRT_SIZE| is greater than 2 MB), which will result in a compilation error. > Well, it was you who used MB(2) in a reply, when previously talk was of MB(8), > and that to grow to MB(16). The BUILD_BUG_ON() is - aiui - about you having set > aside enough page table space. Quite possibly the need for this BUILD_BUG_ON() > then disappears altogether when XEN_VIRT_SIZE is properly taken into account > for the number-of-page-tables calculation. In no event do I see why the MB(2) > boundary would be relevant for anything going forward. Also, doesn’t|BUILD_BUG_ON()| affect how the|ASSERT()| that follows it is written? The changes, at the moment, look like: + const unsigned int vpn1_shift = PAGETABLE_ORDER + PAGE_SHIFT; + const unsigned long va_vpn = va >> vpn1_shift; + const unsigned long xen_virt_start_vpn = + _AC(XEN_VIRT_START, UL) >> vpn1_shift; + const unsigned long xen_virt_end_vpn = + xen_virt_start_vpn + ((XEN_VIRT_SIZE >> vpn1_shift) - 1); + if ((va >= DIRECTMAP_VIRT_START) && (va <= DIRECTMAP_VIRT_END)) return directmapoff_to_maddr(va - directmap_virt_start); - BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2)); - ASSERT((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) == - (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER + PAGE_SHIFT))); + BUILD_BUG_ON(XEN_VIRT_SIZE != MB(16)); + ASSERT((va_vpn >= xen_virt_start_vpn) && (va_vpn <= xen_virt_end_vpn)); If|XEN_VIRT_SIZE| is greater than|GB(1)|, then|xen_virt_end_vpn| may be calculated incorrectly. For example, if|XEN_VIRT_START| is|0xFFFFFFFF80000000| and|XEN_VIRT_SIZE| is|0x40200000|, then|(XEN_VIRT_SIZE >> vpn1_shift)| equals 513, whereas|va_vpn| is always in the range [0, 511], but xen_virt_end_vpn will be greater then 511. So shouldn't it be checked before ASSERT() that XEN_VIRT_SIZE is <= GB(1): BUILD_BUG_ON(XEN_VIRT_SIZE <= GB(1))? ~ Oleksii --------------3yWG1vJgau96FWZoQYQnAYCa Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit


On 4/4/25 9:52 AM, Jan Beulich wrote:
On 04.04.2025 09:31, Oleksii Kurochko wrote:

On 4/4/25 8:56 AM, Jan Beulich wrote:

On 03.04.2025 18:20, Oleksii Kurochko wrote:

On 4/1/25 6:04 PM, Jan Beulich wrote:

On 01.04.2025 17:58, Oleksii Kurochko wrote:

On 3/31/25 6:14 PM, Jan Beulich wrote:

On 31.03.2025 17:20, Oleksii Kurochko wrote:

+        _AC(XEN_VIRT_START, UL) >> vpn1_shift;
+    const unsigned long xen_virt_end_vpn =
+        xen_virt_starn_vpn + ((XEN_VIRT_SIZE >> vpn1_shift) - 1);
+
        if ((va >= DIRECTMAP_VIRT_START) &&
            (va <= DIRECTMAP_VIRT_END))
            return directmapoff_to_maddr(va - directmap_virt_start);
    
-    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2));
-    ASSERT((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) ==
-           (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER + PAGE_SHIFT)));
+    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(8));

Is it necessary to be != ? Won't > suffice?

It could be just > MB(2). Or perphaps >=.
= would make the build fail, wouldn't it?

I just realized that BUILD_BUG_ON() condition is compared to zero so actually everything what
will make the condition true will cause a build fail as inside it used !(condition).

???

|BUILD_BUG_ON()| forces a compilation error if the given condition is true. Therefore, if the condition
|XEN_VIRT_SIZE != MB(2)| is changed to|XEN_VIRT_SIZE > MB(2)|, the condition will always evaluate to true
(assuming|XEN_VIRT_SIZE| is greater than 2 MB), which will result in a compilation error.

Well, it was you who used MB(2) in a reply, when previously talk was of MB(8),
and that to grow to MB(16). The BUILD_BUG_ON() is - aiui - about you having set
aside enough page table space. Quite possibly the need for this BUILD_BUG_ON()
then disappears altogether when XEN_VIRT_SIZE is properly taken into account
for the number-of-page-tables calculation. In no event do I see why the MB(2)
boundary would be relevant for anything going forward.
Also, doesn’t BUILD_BUG_ON() affect how the ASSERT() that follows it is written?

The changes, at the moment, look like:
+    const unsigned int vpn1_shift = PAGETABLE_ORDER + PAGE_SHIFT;
+    const unsigned long va_vpn = va >> vpn1_shift;
+    const unsigned long xen_virt_start_vpn =
+        _AC(XEN_VIRT_START, UL) >> vpn1_shift;
+    const unsigned long xen_virt_end_vpn =
+        xen_virt_start_vpn + ((XEN_VIRT_SIZE >> vpn1_shift) - 1);
+
     if ((va >= DIRECTMAP_VIRT_START) &&
         (va <= DIRECTMAP_VIRT_END))
         return directmapoff_to_maddr(va - directmap_virt_start);
 
-    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2));
-    ASSERT((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) ==
-           (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER + PAGE_SHIFT)));
+    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(16));
+    ASSERT((va_vpn >= xen_virt_start_vpn) && (va_vpn <= xen_virt_end_vpn));


If XEN_VIRT_SIZE is greater than GB(1), then xen_virt_end_vpn may be calculated
incorrectly.

For example, if XEN_VIRT_START is 0xFFFFFFFF80000000 and XEN_VIRT_SIZE is 0x40200000,
then (XEN_VIRT_SIZE >> vpn1_shift) equals 513, whereas va_vpn is always in the range [0, 511],
but xen_virt_end_vpn will be greater then 511.

So shouldn't it  be checked before ASSERT() that XEN_VIRT_SIZE is <= GB(1):
  BUILD_BUG_ON(XEN_VIRT_SIZE <= GB(1))?

~ Oleksii
--------------3yWG1vJgau96FWZoQYQnAYCa--