[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/2] xen/arm: Improve handling of nr_spis


  • To: "Orzel, Michal" <Michal.Orzel@xxxxxxx>
  • From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • Date: Tue, 11 Mar 2025 10:12:49 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=arm.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com])
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=fqXRkNJj/GExad2D0Ni1U+hHNoKohuJvuRLi5cvzO30=; b=dKsEp89JdsSM4YtDMwJ4S3OkLfoOeca/KZN4slW+gg9JfPrKtNVBGf81IQKOxLYuYvBzfGkK8jPPNRYTrg8XN4SEd4CUAUjGWM6A7bFxPz/Tbjuh7V8hThPtDhoL6h5Zg7y38Jc5QtVVjNew2mDpd9gG/gflQkLGWRRo50kxpz9iBHrPnBOpTTAqnAlyKyfFn7DgLK74YQxrOaNOPUSipRASmBq9dfA9/B4LtsfJe6Ac2BDNfNVdaym3rphMzzePOl53mmcr4exEVT0Tm+mIJSZqB4LfMMAB4SNiWqdeJ0bvbPJQ64zO+gFCi7ryhNK+y4wD61QbaKPNCCzCyfiUxQ==
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=fqXRkNJj/GExad2D0Ni1U+hHNoKohuJvuRLi5cvzO30=; b=WdDm4FY41BEHlfH8NQ2b4KTz5FqSj5zMKdO3ud8qNcj5+40Q8WkbTRLfOimfVhf4qb1YceS01Iy6YTpXE0ilIp1wp5/h2xERrpX1PbX5wJ1i7MeOyzEqOI2KIegSF1jCtogHR9cLaU5QBH9sNZHl2oJXJHfrOMaZoLFWZOMm+FC1A+C4s4Im4DR+C0B1b1SRMGu/qjuj9OO7JzVWilb6obV9JGn+tqwSIHGfveA4UUOCGoz27C5KcNOF9a9idTsNiTNAklWEXoasIIRmVJ46lJPAjQVRiW0Wn1ae0nmFcaHfRKeURYnVCSqhyTUSP5qToHk6lvYok4mtWDCaPXCXcQ==
  • Arc-seal: i=2; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=pass; b=LERqqdXFoB+LftNCf8wzRgbQBNikt9ZEy1Ebw7fWfneYcLznrQKq4rXETrQGfPKYgfQGi8Bg6oefrqXLlfYj5/4Q5k1BkFcs2JTnLn3ldZSMg/FuFZFiPcdtwHmhRc6g9jxmqHlPUdfVVHxinG1i6UF8i4q06YcqRKFwWjVO6pZ7YymnvZrJulHFdN/HsBmk3Y4d+TyisLT/yuOTQ17UXhFmRcsaarydKUcLNAu8TiSq7Dg+SjWOpxHZzOWpwApRFzIlhlosSRs/e9YIpJPZti+cr+PYaPjpnpgBzAxEkNGXohwVnUnwfDod/4/lOFyZFHZELwH6C04oLhQBbQTt7Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=fUTfmP0CtiaWkouTPXmShPrIz1xC10iZpew/MH3WyIiU7ovzR4KoQ7XS7aOg1fTURUdg8ksL6UhZbMKNN0mYpOo2xPWt22Zw0//D/FoY5Nwca4w0E+jKZA7+1sTBDbGttUCjZ0q/m4KfTkWqrznOYCl++zVOW9RRzi2r+X2gPSbGW/XiaAnuZEmDBpB5tz/XJcBFR3bLMVU+MP0q6KvRhUfTyfCnyIw2oY1T8G1+OYRUNAjagBBVuyYW4n/A5D8YI8rf4DoBLE3oBx2X6zw2uZ86PPMYa1/vy68a4igFZdIEhdqDu5t58ehpr/cx10wA0bl5swvH/dBkgu2oKeBS6w==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Tue, 11 Mar 2025 10:13:13 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHbkmScFxwSwnbWbUmo6sX8GQY3CLNtq7oAgAAILgCAAAOLgA==
  • Thread-topic: [PATCH 1/2] xen/arm: Improve handling of nr_spis


> On 11 Mar 2025, at 10:59, Orzel, Michal <Michal.Orzel@xxxxxxx> wrote:
> 
> 
> 
> On 11/03/2025 10:30, Bertrand Marquis wrote:
>> 
>> 
>> Hi Michal,
>> 
>>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@xxxxxxx> wrote:
>>> 
>>> At the moment, we print a warning about max number of IRQs supported by
>>> GIC bigger than vGIC only for hardware domain. This check is not hwdom
>>> special, and should be made common. Also, in case of user not specifying
>>> nr_spis for dom0less domUs, we should take into account max number of
>>> IRQs supported by vGIC if it's smaller than for GIC.
>>> 
>>> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value.
>>> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max
>>> IRQs comparison common.
>>> 
>>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>>> ---
>>> xen/arch/arm/dom0less-build.c   | 2 +-
>>> xen/arch/arm/domain_build.c     | 9 ++-------
>>> xen/arch/arm/gic.c              | 3 +++
>>> xen/arch/arm/include/asm/vgic.h | 3 +++
>>> 4 files changed, 9 insertions(+), 8 deletions(-)
>>> 
>>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>>> index 31f31c38da3f..9a84fee94119 100644
>>> --- a/xen/arch/arm/dom0less-build.c
>>> +++ b/xen/arch/arm/dom0less-build.c
>>> @@ -1018,7 +1018,7 @@ void __init create_domUs(void)
>>>        {
>>>            int vpl011_virq = GUEST_VPL011_SPI;
>>> 
>>> -            d_cfg.arch.nr_spis = gic_number_lines() - 32;
>>> +            d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 
>>> 32;
>> 
>> I would suggest to introduce a static inline gic_nr_spis in a gic header ...
> Why GIC and not vGIC? This is domain's nr_spis, so vGIC.

yes vGIC sorry.

> But then, why static inline if the value does not change and is domain 
> agnostic?
> I struggle to find a good name for this macro. Maybe (in vgic.h):
> #define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32)
> to denote default nr_spis if not set by the user?

Yes that would work. My point is to prevent to have 2 definitions in 2 different
source file and a risk to forget to update one and not the other (let say if 
some
day we change 32 in 64).

> 
>> 
>>> 
>>>            /*
>>>             * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 7cc141ef75e9..b99c4e3a69bf 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -2371,13 +2371,8 @@ void __init create_dom0(void)
>>> 
>>>    /* The vGIC for DOM0 is exactly emulating the hardware GIC */
>>>    dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
>>> -    /*
>>> -     * Xen vGIC supports a maximum of 992 interrupt lines.
>>> -     * 32 are substracted to cover local IRQs.
>>> -     */
>>> -    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 
>>> 32;
>>> -    if ( gic_number_lines() > 992 )
>>> -        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
>>> +    /* 32 are substracted to cover local IRQs */
>>> +    dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
>> 
>> and reuse it here to make sure the value used is always the same.
>> 
>>>    dom0_cfg.arch.tee_type = tee_get_type();
>>>    dom0_cfg.max_vcpus = dom0_max_vcpus();
>>> 
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index acf61a4de373..e80fe0ca2421 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -251,6 +251,9 @@ void __init gic_init(void)
>>>        panic("Failed to initialize the GIC drivers\n");
>>>    /* Clear LR mask for cpu0 */
>>>    clear_cpu_lr_mask();
>>> +
>>> +    if ( gic_number_lines() > VGIC_MAX_IRQS )
>>> +        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n");
>> 
>> I am a bit unsure with this one.
>> If this is the case, it means any gicv2 or gicv3 init detected an impossible 
>> value and
>> any usage of gic_number_lines would be using an impossible value.
> Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 
> 992
> IRQs.

Maybe unsupported is a better wording, my point is that it could lead to non 
working system
if say something uses irq 1000.

> 
>> 
>> Shouldn't this somehow result in a panic as the gic detection was wrong ?
>> Do you think we can continue to work safely if this value is over 
>> VGIC_MAX_IRQS.
>> There are other places using gic_number_lines like in irq.c.
> I can add a panic, sure. This would be a change in behavior because we used 
> this
> check for hwdom unconditionally. I'd need others opinion for this one.

ok.

Cheers
Bertrand

> 
> ~Michal





 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.