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

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


  • To: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • From: "Orzel, Michal" <michal.orzel@xxxxxxx>
  • Date: Tue, 11 Mar 2025 14:33:21 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none
  • 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=XDHAVDN9PSSrx3B66DflA08/kQ1c9upqiB4ay8GMFIc=; b=jPZovbdDUXYVfNNsCJzimgCSq+pa6WzqQWDnHpZk9AdB3S5/DyZ8gse1Fj5lDYNI5mjw7CEsnnI66lwBu6M9keAA31SgFg4gqVmn09fuVxfsuYnWj/KDaEazpB6idnEH1hyvR2RlxWe1H23CTHmM6CEgjVlQ3RgwGPEhNqMsg39neYIPJ6yWd8i5BVHSZbm2DZBoNrILQa+In8bQ9V0drvZDM7zps+Haxr5T+ueepXg0LPkOIV9l5M0ByL7Sa/1Vwx5fuRPZvW6+aFih94kfucCK2vC1XYT3B9N7+xDyIPDxSu+YamRmr1bOlz8ib6cWpQu/voqy8MVwfDSKprx9Kw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=NCKHTEe4y/QJQbJQ1lpSg/J7opKOkoWPutFr9wNtQ9kA/Z1XHfyWgnig3Ph+ZSA0GugN1OeVc2v11Fal9WgXWS41+EfXTZmhYRXRT5fj67ju4l/WfNIN8rRjEyfjkgSvjDN8WkAL3ZqJgddmWhnl2B/oJA0TDH0U9fUR52+KfGOh5bsCQR5h9QKf9qiIzi58izL/TRP9OqZ5bpckLn+vuP0gqeTNW/V9CUn7G1iTyWk91GE+tcVkiF4GolqTc41Y/1MnHaXVna8OEBhUoTXdevvayJSy6lS2xdv5IH6FID1uriIkLebnpXzbvAqp9A8cSbdsqvIGjRyeLTZosGM4kw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.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 13:33:39 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>


On 11/03/2025 14:26, Bertrand Marquis wrote:
> 
> 
> Hi Michal,
> 
>> On 11 Mar 2025, at 12:06, Orzel, Michal <michal.orzel@xxxxxxx> wrote:
>>
>>
>>
>> On 11/03/2025 11:12, Bertrand Marquis wrote:
>>>
>>>
>>>> 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.
>> Actually, I took a look at the code and I don't think we should panic (i.e. 
>> we
>> should keep things as they are today). In your example, if something uses 
>> IRQ >
>> VGIC_MAX_IRQS that is bigger than gic_number_lines(), then we will receive 
>> error
>> when mapping this IRQ to guest (but you don't have to use such device and in 
>> the
>> future we may enable IRQ re-mapping). That's why in all the places related to
>> domains, we use vgic_num_irqs() and not gic_number_lines(). The latter is 
>> only
>> used for IRQs routed to Xen.
> 
> So you will get an error later such an IRQ is mapped to a guest. Tracking why 
> this is might not
> be easy.
I did check and we get a nice error that I find good enough, e.g.:
(XEN) the vIRQ number 260 is too high for domain 0 (max = 256)
(XEN) Unable to map IRQ260 to d0

~Michal




 


Rackspace

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