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

RE: [PATCH] xen/arm: Missing N1/A76/A75 FP registers in vCPU context switch


  • To: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Andre Przywara <Andre.Przywara@xxxxxxx>
  • From: Wei Chen <Wei.Chen@xxxxxxx>
  • Date: Fri, 21 Aug 2020 06:26:42 +0000
  • Accept-language: 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=vtQa3YTJeTJRi77dDJuwHG/TemKX8hji/cKKwlq5D9g=; b=V0g8wGu/IqiqGacYtGw9gAIvWtZGg69tQCRSdnRl7Ov0G6AvD2AKID/EPt6Jq9A1q8XijSGxt7MIjZPTXQjP7FavE5XUfyBS2UY78NNPFAPhNijXuVKw18ThBxWU+D+Fz23DBUDSOP/hvnq/y7TFzLyuSOcmUXFhMdXdfufAtIkyW+qnAqJRvsFLncK+XmQYXqFadwZpXwtpzrScmvG6VgnCS0lH4goYHgZVb9+PVfupjtUWDnDcms+wSCFvZsTxlzKzY/asUtAAys+k3spglG/KImM/hewyKuRBn/AzqD6/he09gqhIgSNv4zPoI5N+9nK1x/cAxGAflhjumGGYtg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=P0WVdefVDAWJlsrX3oxAdOAMHfeHcN6U3dR/xKD3W5moNNcUE+y8z/LWpBqA2egKibHR4KM7j0nSueTmgsu4tqhIcn7CFDDTJOawZGrkInpLKZOUL4M1xuLTeYtUMrekpJ/DjXdB7hY0pCWecMg1XFUJ+y1249KJIirNbgl0ytQpPYGht84c+3lkh9IPFlDkLC/1NnR4WBCsTlpLpYwz6vn+Gqs09fHDITsJEsOS25iWrEdfE7arrR1ddssfxJHupZ/Qt+k4VAWObtaRFYT8H5DVhCmSkU8J+ulNtbPC6Swwv/gHVG3RQ8yFo8lGYTyNSN0wu5O8XrmSvq+tTsk4ww==
  • Authentication-results-original: arm.com; dkim=none (message not signed) header.d=none;arm.com; dmarc=none action=none header.from=arm.com;
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Steve Capper <Steve.Capper@xxxxxxx>, Kaly Xin <Kaly.Xin@xxxxxxx>, nd <nd@xxxxxxx>
  • Delivery-date: Fri, 21 Aug 2020 06:27:00 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: arm.com; dkim=none (message not signed) header.d=none;arm.com; dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHWdQ1DU6QzTrGCRkmVmocCo7wV0qk9lfsAgAAC9YCAAAS2AIAACKqAgAATZwCAACb7gIAEMhbQ
  • Thread-topic: [PATCH] xen/arm: Missing N1/A76/A75 FP registers in vCPU context switch

Hi Julien, Andre, Bertrand,

Thanks for your comments. It took me some time to remove the disclaimer 
message. 
And please see my comments below : )

> -----Original Message-----
> From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Sent: 2020年8月18日 21:42
> To: Andre Przywara <Andre.Przywara@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> Julien Grall <julien@xxxxxxx>; Steve Capper <Steve.Capper@xxxxxxx>; Kaly
> Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [PATCH] xen/arm: Missing N1/A76/A75 FP registers in vCPU
> context switch
> 
> 
> 
> > On 18 Aug 2020, at 12:22, André Przywara <andre.przywara@xxxxxxx>
> wrote:
> >
> > On 18/08/2020 11:13, Bertrand Marquis wrote:
> >
> > Hi,
> >
> >>> On 18 Aug 2020, at 10:42, André Przywara <andre.przywara@xxxxxxx>
> wrote:
> >>>
> >>> On 18/08/2020 10:25, Bertrand Marquis wrote:
> >>>
> >>> Hi,
> >>>
> >>>>> On 18 Aug 2020, at 10:14, André Przywara <andre.przywara@xxxxxxx>
> wrote:
> >>>>>
> >>>>> On 18/08/2020 04:11, Wei Chen wrote:
> >>>>>
> >>>>> Hi Wei,
> >>>>>
> >>>>>> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU
> supports
> >>>>>> FP/SIMD or not. But currently, this two MACROs only consider value
> 0
> >>>>>> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for
> CPUs
> >>>>>> that support FP/SIMD and half-precision floating-point features, the
> >>>>>> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat
> them as
> >>>>>> no FP/SIMD support. In this case, the vfp_save/restore_state will
> not
> >>>>>> take effect.
> >>>>>>
> >>>>>> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD
> and
> >>>>>> half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
> >>>>>> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
> >>>>>> platforms, Xen will always miss the float pointer registers
> save/restore.
> >>>>>> If different vCPUs are running on the same pCPU, the float pointer
> >>>>>> registers will be corrupted randomly.
> >>>>>
> >>>>> That's a good catch, thanks for working this out!
> >>>>>
> >>>>> One thing below...
> >>>>>
> >>>>>> This patch fixes Xen on these new cores.
> >>>>>>
> >>>>>> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> >>>>>> ---
> >>>>>> xen/include/asm-arm/cpufeature.h | 4 ++--
> >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-
> arm/cpufeature.h
> >>>>>> index 674beb0353..588089e5ae 100644
> >>>>>> --- a/xen/include/asm-arm/cpufeature.h
> >>>>>> +++ b/xen/include/asm-arm/cpufeature.h
> >>>>>> @@ -13,8 +13,8 @@
> >>>>>> #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
> >>>>>> #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
> >>>>>> #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
> >>>>>> -#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
> >>>>>> -#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
> >>>>>> +#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
> >>>>>> +#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)
> >>>>>
> >>>>> But this is only good until the next feature bump. I think we should be
> >>>>> more future-proof here. The architecture describes those two fields
> as
> >>>>> "signed"[1], and guarantees that "if value >= 0" is a valid test for the
> >>>>> feature. Which means we are good as long as the sign bit (bit 3) is
> >>>>> clear, which translates into:
> >>>>> #define cpu_has_fp        (boot_cpu_feature64(fp) < 8)
> >>>>> Same for simd.
> >>>>>
> >>>>
> >>>> We cannot really be sure that a new version introduced will require the
> >>>> same context save/restore so it might dangerous to claim we support
> >>>> something we have no idea about.
> >>>
> >>> I am pretty sure we can, because this is what the FP feature describes.
> >>> If a feature bump would introduce a larger state to be saved and
> >>> restored, that would be covered by a new field, look at AdvSIMD and
> SVE
> >>> for examples.
> >>> The feature number would only be bumped if it's compatible:
> >>> ====================
> >>> · The field holds a signed value.
> >>> · The field value 0xF indicates that the feature is not implemented.
> >>> · The field value 0x0 indicates that the feature is implemented.
> >>> · Software that depends on the feature can use the test:
> >>>     if value >= 0 {  // Software features that depend on the presence
> >>> of the hardware feature }
> >>> ====================
> >>> (ARMv8 ARM D13.1.3)
> >>>
> >>> And this is how Linux handles this.
> >>
> >> Then changing the code to use <8 should be ok.
> >
> > Thanks. Another angle to look at this:
> > Using "< 8" will never be worse than "<= 1", since we only derive the
> > existence of the floating point registers from it. The moment we see a 2
> > in this register field, the "<= 1" would definitely fail to save/restore
> > the FP registers again. But the ARM ARM guarantees that those registers
> > are still around (since "value >= 0" hits, so the feature is present, as
> > shown above).
> > The theoretical worst case with "< 8" would be that it would not cover
> > *enough* state, but as described above this will never happen, with this
> > particular FP/SIMD field.
> 
> We could also issue a warning for a “non supported FP/SIMD” if something
> else then 0 or 1 shows up so that at least it does not passthrough without
> being noticed.
> 

I think we have made up the agreement to use "< 8" in these MACROs : )
The reset is that shall we need to throw any information to notice/warn
user that we detected a feature we haven't met before. In my opinion,
I agree with Bertrand, we shall give some message.

Quote from Arm ARM:
For a field describing some class of functionality:
• The value 0x1 was defined as indicating that item A is present.
• The value 0x2 was defined as indicating that items B and C are present, in 
addition to item A

If there is a value 0x3 bumped in the future, indicates D is present. Because 
of "< 8", what xen
Has done for A/B/C can also take effect for 0x3. But what Xen done for A/B/C 
may not always
cover D required. In this case, I think it valuable to pop some warning message 
for Xen known
FP/SIMD values. 

> Cheers
> Bertrand
> 
> >
> > Cheers,
> > Andre
> >
> >>>> I agree though about the analysis on the fact that values under 8 should
> >>>> be valid but only 0 and 1 currently exist [1], other values are reserved.
> >>>>
> >>>> So I would vote to keep the 1 for now there.
> >>>>
> >>>> Cheers
> >>>> Bertrand
> >>>>
> >>>> [1] https://developer.arm.com/docs/ddi0595/h/aarch64-system-
> registers/id_aa64pfr0_el1
> 


 


Rackspace

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