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

Re: [PATCH v3 3/7] xen/arm: create a cpuinfo structure for guest


  • To: Julien Grall <julien@xxxxxxx>
  • From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • Date: Thu, 10 Dec 2020 16:17:04 +0000
  • Accept-language: en-GB, en-US
  • 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=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=fHRqENzcbnT8NZL9P8ZfWgaZCvF+rDpKye35Gl8bMWk=; b=IJU8AwXlq9zzW1iDplB1W3CYyr3JdKsiivyVxZKHrQLIM7c1Zc8PQe5k6/a/HHWP2q6uKu8YKuvh/1eWw+84NOyOqoid/DZC44TAGanvKmGBF19aV10EgrKnXD1dk1mJ4Xgp1lRkeSE3xb12RBjGBKFKu7NYdxEkZp2127TdTVmsgmlkdW/aT2OaVIop4ZVNkBGruQEf2iM2ofK+ehHT8RwaKdnB7Oqkuep+mG/awuynncHipJdyKTzJmIXoceKB6yhkTuS3qW8WUbuOUsA3UEtTzb+a9KsazWkbtjMUfGY7Jd0nGDu8IUlKZI4ey0NcdK2Z2w1CXzT8qXynvvjceA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=cCrOEKjT4nhoWz+LqQCXmLHyZ0wGKGi4EGWRed5bYLIOfnfN6PlzDmAV6zxwp6Rii/RhN84sPvNHvms1l3kD6DfNHJ4O6CVsBU0B0ratXaM68LZLr+4G+uWKJQCZ4tycu2qJpTMgpyTYFEXtoP6158CUmlOiqlLe36Z5ztGrY9ndHNC22FL5a/kVMgaYnWk6PA1tYZWVP+sb98lLy9buMpz5yPL4VCdBUBGcQwjqFI6WxYn5kxDXMqsm/sEzKMC1U7eHD/bxYQyKi6rpPpKvTMWEHpWWk0l5HU/6j4xZz/ZQePPeQAT7XzsTNa7hghi/fr4DkyR1ao2wlFxJkrJYYQ==
  • Authentication-results-original: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=arm.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Thu, 10 Dec 2020 16:17:35 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHWzklW+4XymezDRUeCGhvhzW8UY6nvZCkAgAEXL4CAAAS6AIAAAyYA
  • Thread-topic: [PATCH v3 3/7] xen/arm: create a cpuinfo structure for guest

Hi Julien,

> On 10 Dec 2020, at 16:05, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 10/12/2020 15:48, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 9 Dec 2020, at 23:09, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> Hi Bertand,
>>> 
>>> On 09/12/2020 16:30, Bertrand Marquis wrote:
>>>> Create a cpuinfo structure for guest and mask into it the features that
>>>> we do not support in Xen or that we do not want to publish to guests.
>>>> Modify some values in the cpuinfo structure for guests to mask some
>>>> features which we do not want to allow to guests (like AMU) or we do not
>>>> support (like SVE).
>>>> The code is trying to group together registers modifications for the
>>>> same feature to be able in the long term to easily enable/disable a
>>>> feature depending on user parameters or add other registers modification
>>>> in the same place (like enabling/disabling HCR bits).
>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>>>> ---
>>>> Changes in V2: Rebase
>>>> Changes in V3:
>>>>   Use current_cpu_data info instead of recalling identify_cpu
>>>> ---
>>>>  xen/arch/arm/cpufeature.c        | 51 ++++++++++++++++++++++++++++++++
>>>>  xen/include/asm-arm/cpufeature.h |  2 ++
>>>>  2 files changed, 53 insertions(+)
>>>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
>>>> index bc7ee5ac95..7255383504 100644
>>>> --- a/xen/arch/arm/cpufeature.c
>>>> +++ b/xen/arch/arm/cpufeature.c
>>>> @@ -24,6 +24,8 @@
>>>>    DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS);
>>>>  +struct cpuinfo_arm __read_mostly guest_cpuinfo;
>>>> +
>>>>  void update_cpu_capabilities(const struct arm_cpu_capabilities *caps,
>>>>                               const char *info)
>>>>  {
>>>> @@ -157,6 +159,55 @@ void identify_cpu(struct cpuinfo_arm *c)
>>>>  #endif
>>>>  }
>>>>  +/*
>>>> + * This function is creating a cpuinfo structure with values modified to 
>>>> mask
>>>> + * all cpu features that should not be published to guest.
>>>> + * The created structure is then used to provide ID registers values to 
>>>> guests.
>>>> + */
>>>> +static int __init create_guest_cpuinfo(void)
>>>> +{
>>>> +    /*
>>>> +     * TODO: The code is currently using only the features detected on 
>>>> the boot
>>>> +     * core. In the long term we should try to compute values containing 
>>>> only
>>>> +     * features supported by all cores.
>>>> +     */
>>>> +    guest_cpuinfo = current_cpu_data;
>>> 
>>> It would be more logical to use boot_cpu_data as this would be easier to 
>>> match with your comment.
>> Agree, I will fix that in V4.
>>> 
>>>> +
>>>> +#ifdef CONFIG_ARM_64
>>>> +    /* Disable MPAM as xen does not support it */
>>>> +    guest_cpuinfo.pfr64.mpam = 0;
>>>> +    guest_cpuinfo.pfr64.mpam_frac = 0;
>>>> +
>>>> +    /* Disable SVE as Xen does not support it */
>>>> +    guest_cpuinfo.pfr64.sve = 0;
>>>> +    guest_cpuinfo.zfr64.bits[0] = 0;
>>>> +
>>>> +    /* Disable MTE as Xen does not support it */
>>>> +    guest_cpuinfo.pfr64.mte = 0;
>>>> +#endif
>>>> +
>>>> +    /* Disable AMU */
>>>> +#ifdef CONFIG_ARM_64
>>>> +    guest_cpuinfo.pfr64.amu = 0;
>>>> +#endif
>>>> +    guest_cpuinfo.pfr32.amu = 0;
>>>> +
>>>> +    /* Disable RAS as Xen does not support it */
>>>> +#ifdef CONFIG_ARM_64
>>>> +    guest_cpuinfo.pfr64.ras = 0;
>>>> +    guest_cpuinfo.pfr64.ras_frac = 0;
>>>> +#endif
>>>> +    guest_cpuinfo.pfr32.ras = 0;
>>>> +    guest_cpuinfo.pfr32.ras_frac = 0;
>>> 
>>> How about all the fields that are currently marked as RES0/RES1? Shouldn't 
>>> we make sure they will stay like that even if newer architecture use them?
>> Definitely we can do more then this here (including allowing to enable some 
>> things for dom0 or for test reasons).
>> This is a first try to solve current issues with MPAM and SVE and I plan to 
>> continue to enhance this in the future
>> to enable more customisation here.
>> I do think we could do a bit more here to have some features controlled by 
>> the user but this will need a bit of
>> discussion to agree on a strategy.
> 
> I think you misunderstood my comment. I am not asking whether we want to 
> customize the value per domain. Instead, I am raising questions for the 
> strategy taken in this patch.
> 
> I am going to leave the safety aside, because I think this is orthogonal to 
> this patch.
> 
> This patch is introducing a deny list. This means that all the features will 
> be exposed to a domain unless someone determine that this is not
> supported by Xen.
> 
> This means we will always try to catch up with what Arm decided to invent and 
> attempt to fix it before the silicon is out.
> 
> Instead, I think it would be better to use an allow list. We should only 
> expose features to the guest we know works (this could possibly be just the 
> Armv8.0 one).

I understood that and I fully agree that this is what we should do: only expose 
what we support and know and let everything else as “disabled”.
And I definitely want to do that and I think with this serie we have the 
required support to do that, we will need to rework how we initialise the
guest_cpuinfo structure.

I just want to leave this discussion for after so that we can at least right 
now have a current linux booting without the need to modify the linux
kernel binary to remove things like SVE.

Regards
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall


 


Rackspace

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