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 120FFF8A160 for ; Thu, 16 Apr 2026 12:22:02 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.1283311.1565579 (Exim 4.92) (envelope-from ) id 1wDLj5-0005eb-G8; Thu, 16 Apr 2026 12:21:43 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 1283311.1565579; Thu, 16 Apr 2026 12:21:43 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1wDLj5-0005eU-CH; Thu, 16 Apr 2026 12:21:43 +0000 Received: by outflank-mailman (input) for mailman id 1283311; Thu, 16 Apr 2026 12:21:41 +0000 Received: from mx.expurgate.net ([195.190.135.10]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1wDLj3-0005eO-KE for xen-devel@lists.xenproject.org; Thu, 16 Apr 2026 12:21:41 +0000 Received: from mx.expurgate.net (helo=localhost) by mx.expurgate.net with esmtp id 1wDLj3-00Dsfg-16 for xen-devel@lists.xenproject.org; Thu, 16 Apr 2026 14:21:41 +0200 Received: from [10.42.69.11] (helo=localhost) by localhost with ESMTP (eXpurgate MTA 0.9.1) (envelope-from ) id 69e0d43e-bab6-0a2a0a5309dd-0a2a450bdada-36 for ; Thu, 16 Apr 2026 14:21:40 +0200 Received: from [209.85.221.53] (helo=mail-wr1-f53.google.com) by tlsNG-42698a.mxtls.expurgate.net with ESMTPS (eXpurgate 4.56.1) (envelope-from ) id 69e0d454-212f-0a2a450b0019-d155dd35b54a-3 for ; Thu, 16 Apr 2026 14:21:40 +0200 Received: by mail-wr1-f53.google.com with SMTP id ffacd0b85a97d-43d73352cf2so4078066f8f.1 for ; Thu, 16 Apr 2026 05:21:40 -0700 (PDT) Received: from [10.156.60.236] (ip-037-024-206-209.um08.pools.vodafone-ip.de. [37.24.206.209]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-43ead3d5f11sm14373744f8f.18.2026.04.16.05.21.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 16 Apr 2026 05:21:39 -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" Authentication-Results: eu.smtp.expurgate.cloud; dkim=pass header.s=google header.d=suse.com header.i="@suse.com" header.h="Content-Transfer-Encoding:In-Reply-To:Autocrypt:From:Content-Language:References:Cc:To:Subject:User-Agent:MIME-Version:Date:Message-ID" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1776342100; x=1776946900; darn=lists.xenproject.org; h=content-transfer-encoding:in-reply-to:autocrypt:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=Jzka/KTnQChT2mJEPOM+6py4Rbv17O9g9qM1K9yBQXA=; b=E3LPcAKhSTTGFg//wiAGh//XJGcvzu821PvcYVtRMWpwPVjcSWtpW1H3vUavnL7GCf Pn/FQl1nTnX20gbCI1Sf7n6pJgEBAtG3TSfD0fbftVUrV+7SsUmPtOMbIM+JbPsXcRHu eiSBy4AWm2Tzuw/YTFZ5XQ8maDTFNxuFLbA7i6vW3TVQ/peoabtUoZPDKu/hg+CGbmVM 0IXU23j4DBNSEttVdCOPEeQvGq1qKeJxdWrpUHCTpyndPAdoVhE5noSz7MiUFhuTstBP xvlFO+QDXiWRHO05L07yYwIo/TtbF3RLnrNJSmp6SUpiamk4/bbjwbv8DMuC2DhyPEl7 6dig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1776342100; x=1776946900; h=content-transfer-encoding:in-reply-to:autocrypt:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=Jzka/KTnQChT2mJEPOM+6py4Rbv17O9g9qM1K9yBQXA=; b=Ia4KhPCTXoSESAVfRFip++jrigjMtqUWtiyMjljTfs6Si/ocol969XR+8OSuv9ebmK D2kutZR7TM/VKGlqFHgilWEF27el7vMdutnOAsi/H1zXZ4AlwNdaM1Io+zlPoQJJllVX 3p0+Cn8TAwisi8Czxm1/p2Md3t4uS7hwG2LDGnfzvBOVodlw/yeXUn7phZ/VLQp2Polt Y02TbttllUJflR72zHnwtLObrjj64WMCWqIDVu7vFQkgzIpLQAF6/lsGZSQ7Pp+Hg7xP 5FsGjBW3uDSMWLrNvdfXjRHOr7TXCnn0k6czNSHuo+49J+1DAy+PBY/xuz+z9Bz4Sluq wFyQ== X-Forwarded-Encrypted: i=1; AFNElJ/PbaTR4zVEVQFnB+gzth/nDCJusWaXRS2FmrIUUZdX+08iY635Z3tkZXpcUjB/dl3mMvEYlcs9Oxk=@lists.xenproject.org X-Gm-Message-State: AOJu0YyXTrgx7C1rQGePsoxxuLZ/NGMvxkOHrbO/uTL+0jQqFBNAcCLq 2YXOljENdK5iyER2Leg+KUxXHiL5b+LcWkaEpydWn3ISirnHJXBL6fZvhYEvjODtJA== X-Gm-Gg: AeBDievXY8i8KFkKhNsVg/VMYme2tEQlsWsfLyIlOQN8H/SlZudr9SQFW/4k1Y1kqXT KyAP3n4kltjb9YknEhE33kN54h8dIuIjMHs67oZG4PGt6sBCCflwSUbZo4JuJNbskoNLE9uo459 vWm6iGm76ZNdfaKjHx+f/xbb/HAONNYg4yi60d42mnd8OlyYKbfsimFhzeZD1SYL27TrU+s5t8D /QvX8xfikRW6RF4EWVov2/vM80upz4fqrJ19nEdy+pUx/6tv2bQKN0D12lKYh69T7MGtBO/vkHV aXXBquf4b077TIHm0yhSLs4dWNWkhbDBae9VTDyEsLG9obMyeBZSuBjqqQaYXOjAYcia7tIkfMM RrnbP9gR4PYqR8ZTKIsBk5UFxUN+aPqJDctO55SpM1W6lxDqznB0YHVUouAVghg6SGGJiS4GaG0 hA8hyDqmA/LmQObcQNFZUMcg8GaEvxdWgO1c0QMmsntL0tAy0PGp5jgOtlzHXCVftKe/HjZ3xRA A5UcB21RNXKM8SrDKtG5QNgUg== X-Received: by 2002:a05:6000:18a8:b0:43d:71b:204b with SMTP id ffacd0b85a97d-43d642cd3a0mr37033996f8f.39.1776342100111; Thu, 16 Apr 2026 05:21:40 -0700 (PDT) Message-ID: <314c064b-88eb-497a-9022-2013125d63a9@suse.com> Date: Thu, 16 Apr 2026 14:21:42 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1 13/27] xen/riscv: add basic VGEIN management for AIA guests To: Oleksii Kurochko Cc: Romain Caritey , Alistair Francis , 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: <09fed304-685c-46a9-9159-72baa1721224@suse.com> <4b977410-8d24-41c3-9c83-7d95637ddea3@gmail.com> Content-Language: en-US From: Jan Beulich Autocrypt: addr=jbeulich@suse.com; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL In-Reply-To: <4b977410-8d24-41c3-9c83-7d95637ddea3@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-purgate-ID: tlsNG-42698a/1776342100-07175F3B-2B6E47E3/0/0 X-purgate-type: clean X-purgate-size: 5759 On 13.04.2026 16:42, Oleksii Kurochko wrote: > On 4/2/26 12:03 PM, Jan Beulich wrote: >> On 10.03.2026 18:08, Oleksii Kurochko wrote: >>> AIA provides a hardware-accelerated mechanism for delivering external >>> interrupts to domains via "guest interrupt files" located in IMSIC. >>> A single physical hart can implement multiple such files (up to GEILEN), >>> allowing several virtual harts to receive interrupts directly from hardware >> >> Isn't use of such an optimization coming prematurely? Shouldn't this series >> focus on getting basic functionality in place? > > At the moment, we don't support only APLIC for guest interrupts as it > will require trap-and-emulation approach, so just from the start it was > decided to go with APLIC+IMSIC (IMSIC here as it only one interrupt > controller which exist and support VGEIN stuff at the momemnt) approach > and then when it will be needed back to only the case when APLIC is > supported. > > Maybe, it was better to introduce in patch series where a lauching of > domain actually happens. > > Considering that you've already made a review, I prefer then to have > this patch part of this patch series. That's okay, but then please take my remark as an indication that once again the patch description makes implications on what the reader knows (without it being written down anywhere I'm aware of). >>> @@ -14,12 +27,109 @@ bool aia_available(void) >>> return is_aia_available; >>> } >>> >>> +int __init vgein_init(unsigned int cpu) >> >> If this needs invoking once per CPU being brought up, it can't be __init. > > Yes, it is going to be called inside the secondary CPU bring-up function. > > __init sections are removed much later, after all CPUs are brought up, > so it looks like that at the moment when secondary CPUs are being > brought up, __init still exists and can be called. No. And I said so before. See XEN_SYSCTL_cpu_hotplug (and ignore it having "hotplug" in the name, but merely consider that you can take CPUs offline at runtime, and later bring them online again). >> Also - static? > > It isn't static because it will be called inside the secondary CPU > bring-up function. As it doesn't need calling from the outside for the boot CPU, it's not obvious why it would need calling from the outside for secondary ones. >>> +{ >>> + struct vgein_bmp *vgein = &per_cpu(vgein_bmp, cpu); >>> + >>> + csr_write(CSR_HGEIE, -1UL); >>> + vgein->geilen = flsl(csr_read(CSR_HGEIE)); >>> + csr_write(CSR_HGEIE, 0); >>> + if ( vgein->geilen ) >>> + vgein->geilen--; >> >> I don't understand this. The "len" in "geilen" stands for "length", I suppose, >> i.e. the number of bits. Hmm, the spec itself is inconsistent: "The number of >> bits implemented in hgeip and hgeie for guest external interrupts is UNSPECIFIED >> and may be zero. This number is known as GEILEN." This may or may not include >> bit 0 (which is implemented, but r/o zero). Then saying "Hence, if GEILEN is >> nonzero, bits GEILEN:1 shall be writable in ..." suggests 0 isn't included, but >> that's not unambiguous. > > But they explicitly wrote that: The least-significant bits are > implemented first, apart from bit 0. So bit 0 is explicitly excluded. Fine, but not including it in anything named *LEN feels - as said - ambiguous. >>> + /* >>> + * All vCPU guest interrupt files are used and we don't support a case >>> + * when number of vCPU on 1 pCPU is bigger then geilen. >>> + */ >> >> This wants checking in vgein_init() then. CPUs (beyond the boot one) >> violating this should not be brought online. > > It'll be nice. But we can't know how many vCPUs will be ran on pCPU when > vgein_init() is executed. I don't understand: How does it matter how many vCPU-s will exist later on (and will run wherever)? >>> + ASSERT(vgein_id < vgein_bmp->geilen); >> >> What if not bit is available? By asserting, you assume the caller will not >> call here when no ID is available. > > It is just a temporary ASSERT() (as we don't support software guest > interrupt files) because in general it is fine if there is no bit > available, it will just mean that that no physical hardware guest > interrupt file is assigned to the virtual hart, and software-based > emulation (a "software file") must be used to handle guest external > interrupts. > > Will it be better to return 0 now here and just don't create a vCPU > on ... > > Yet there is no caller of this function, >> so how can one verify whether this assertion is appropriate? > > ... the caller side when an assignment is expected to be happen? Yes, you want to return errors for cases you can't handle. You wan to assert on internal state only, not on anything controlled by a caller outside of the hypervisor. >>> + bitmap_set(bmp, vgein_id, 1); >> >> __set_bit()? > > I thought that it will be fine to use for bmp, bitmap_* functions(). > __set_bit is what is called inside bitmap_set(). Sure, but why make things look complicated? We use __set_bit() and friends all over the place on bitmaps. bitmap_*() exist for the less trivial cases. >>> + spin_unlock_irqrestore(&vgein_bmp->lock, flags); >>> + >>> +#ifdef VGEIN_DEBUG >>> + printk("%s: vgein_id(%u), xen_cpu%d_bmp=%#lx\n", >>> + __func__, vgen_id, v->processor, *vgein_bmp->bmp); >> >> I can't spot a difference from the message in vgein_assign(). How is one >> to distinguish the two in a log? > > By function name which is the first argument (__func__). My general take is that in gprintk() use of __func__ may be okay, but hardly anywhere else. Jan