Hi Julien,
> On 10 Dec 2020, at 16:05, Julien Grall <jul...@xen.org> wrote:
>
>
>
> On 10/12/2020 15:48, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 9 Dec 2020, at 23:09, Julien Grall <jul...@xen.org> 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.marq...@arm.com>
>>>> ---
>>>> 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
