RE: [PATCH v2 2/2] xen/arm: Throw messages for unknown FP/SIMD implement ID

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien, Bertrand,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2020年8月25日 1:23
> To: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>; sstabellini@xxxxxxxxxx; Andre Przywara
> <Andre.Przywara@xxxxxxx>; Penny Zheng <Penny.Zheng@xxxxxxx>; Kaly
> Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [PATCH v2 2/2] xen/arm: Throw messages for unknown
> FP/SIMD implement ID
> 
> 
> 
> On 24/08/2020 17:57, Bertrand Marquis wrote:
> > Hi Julien,
> 
> Hi,
> 
> >
> >> On 24 Aug 2020, at 14:34, Julien Grall <julien@xxxxxxx> wrote:
> >>
> >> Hi Wei,
> >>
> >> On 24/08/2020 04:28, Wei Chen wrote:
> >>> Arm ID_AA64PFR0_EL1 register provides two fields to describe CPU
> >>> FP/SIMD implementations. Currently, we exactly know the meaning of
> >>> 0x0, 0x1 and 0xf of these fields. Xen treats value < 8 as FP/SIMD
> >>> features presented. If there is a value 0x2 bumped in the future,
> >>> Xen behaviors for value <= 0x1 can also take effect. But what Xen
> >>> done for value <= 0x1 may not always cover new value 0x2 required.
> >>
> >> Right, but this will also happen with all the other features. This may
> actually confuse the users as they may think the rest of the features are 
> fully
> supported which is not correct. For instance, dom0 will crash if you boot Xen
> on a SVE-capable hardware.
> >
> > I would see this as an improvement already.
> 
> TBH, I only view this patch as a band-aid. I am OK with them but they
> need to be useful.
> 
> When I read "unknown value...", I have no clue whether the message is
> good or bad. It would be better to extend the message with what could go
> wrong. E.g "Unknown value X, this may result to corruption on the platform".
> 

> I would also consider to taint Xen as it may become unstable with this
> option set.
>  

Hmm, yes, it seems current messages are pale. The user could not understand
what we want the message expressed when he got one such message. I will improve
them to give user a clear meaning. And I think a XENLOG_WARNING would be better
to notice user. I will do them in v3.

> > More could be done for SVE (and other bits) but this should be in another
> patch set.
> >
> >>
> >>> We throw these messages to break the silence when Xen detected
> >>> unknown FP/SIMD IDs to notice user to check.
> >>
> >> It feels a bit odd to me to print unknown for the FP/SIMD feature but not
> for all the rest.
> >>
> >> IMHO, the right approach is to sanitize ID registers exposed to domains
> and only expose features we know are correctly handled.
> >
> > I actually started to look into this last week because I came to an issue
> comparable to SVE with pointer authentication.
> > Maybe we should discuss this subject separately as clearing TID3 bit in HCR
> and emulating all ID registers is possible
> > but I want to check first if this could have big impacts on performances
> 
> I don't expect any performance drop. In Linux, they will cache all the
> values and use alternative to avoid runtime check.
> 
> I also don't expect any OS to use them at runtime in hotpath.
> 

I think this would not be a problem. Most ID check operations would be done
in system/application initialization stage.

> > and see discuss how to design this as there
> > could be a huge amount of cases for example if we want to allow different
> parameters for different domains.
> 
> I would suggest to first consider a simple approach where we expose the
> same view of the ID registers to all the guests but it is sanitized.
> 

Can I ask the reason why we don't expose native ID registers to guest? Is it 
because
we want to avoid guest using some unaware features of Xen?

> We can then discuss whether we want to have a per-guest view of the ID
> registers and how to configure it.
> 
> Regarding the implementation, I personally quite like the Linux
> cpufeature framework. It is quite powerful and I believe would suit
> quite well Xen.
> 

I agree with you. But I think is quite huge, and out of this patch set's scope.
We need a separate thread to discuss the design and implement.


> Cheers,
> 
> --
> Julien Grall