Re: [Xen-devel] [PATCH v2 2/2] xen/arm32: implement VFP context switch

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 05/31/2013 04:54 PM, Ian Campbell wrote:

> On Fri, 2013-05-31 at 16:27 +0100, Julien Grall wrote:
>> On 05/31/2013 03:12 PM, Ian Campbell wrote:
>>
>>> On Thu, 2013-05-30 at 17:01 +0100, Julien Grall wrote:
>>>> +void vfp_save_state(struct vcpu *v)
>>>> +{
>>>> +    uint32_t tmp;
>>>> +
>>>> +    v->arch.vfp.fpexc = READ_CP32(FPEXC);
>>>
>>> The docs seem to call for reading this via an explicit VMRS
>>> instruction. 
>>>
>>> Looking at the ARM ARM this seems to be an alias for the encoding of an
>>> MRC instruction corresponding to reading FPEXC as you have done. Did you
>>> have a reference for that aliasing? (I'm not finding it in the ARM ARM).
>>
>>
>> I didn't find anything on the aliasing. I looked at the linux VFP
>> include (arch/arm/include/asm/vfp.h).
> 
> I wonder if it would be better to do this via VFP specific macros?


Except in this code we don't need the VFP macros. I don't see specific
reason to use it here.

> Probably not.
> 
>>> Are you avoiding the mnemonic to avoid issues with binutils providing
>>> the instruction?
>>
>> This mnemonic can only be used if VFP is enabled by gcc. I think it's
>> safer to use mrc if we don't want to use VFP on the other part of XEN.
> 
> Agreed.
> 
>>> Do you know what happens to the values of d0..d31  and FPSCR etc when
>>
>>> FPEXC.EN is disabled? I'm wondering if we can avoid saving the registers
>>> in that case, we'd still need to restore to prevent leaking the last
>>> guests state if the VCPU enabled exceptions. I suppose it's not worth it
>>> unless we implement a more full featured lazy saving regime.
>>
>>
>> I didn't find something about the state of the registers when VFP is
>> disabled. I think the registers is not clobbered.
>> Linux seems to save vfp registers at each context switch only if VFP is
>> enabled. But we cannot rely on that for the other distributions.
> 
> Linux likely traps if the process then goes on to use VFP, at which
> point it can clear/restore the registers as necessary. We could do
> something similar using HCPTR -- that's what I meant by lazy saving (and
> restore).

I will not have time before 4.3 to implement lazy context switch. I
think we can postpone this part and see later.

>>
>>>> +}
>>>> +
>>>> +void vfp_restore_state(struct vcpu *v)
>>>> +{
>>>
>>> Some of the same comments (tmps, barriers etc) apply to restore as to
>>> save.
>>>
>>> Is there any chance that any of these loads could cause an FP fault?
>>> e.g. if the guest had an FP exception pending when we saved it, could
>>> reloading the register retrigger it?
>>
>> I don't know.
> 
> Hrm :-/
> 
> If there are no invalid encodings for d0..d31 then it should just be a
> case of checking the ARM ARM for FPINST* and FPSCR.


For FPINST*:
"Any value read from a Floating-Point Instruction Register can be
written back to the same register. This means
context switch and debugger software can save and restore Floating-Point
Instruction Register values."

For FPSCR:
"Writes to the FPSCR can have side-effects on various aspects of
processor operation. All of these side-effects are
synchronous to the FPSCR write. This means they are guaranteed not to be
visible to earlier instructions in the
execution stream, and they are guaranteed to be visible to later
instructions in the execution stream."

Except if I missed something in FPSCR encoding, we are safe during the
restore step.

>>>> @@ -106,6 +114,7 @@
>>>>  #define NSACR           p15,0,c1,c1,2   /* Non-Secure Access Control 
>>>> Register */
>>>>  #define HSCTLR          p15,4,c1,c0,0   /* Hyp. System Control Register */
>>>>  #define HCR             p15,4,c1,c1,0   /* Hyp. Configuration Register */
>>>> +#define HCPTR           p15,4,c1,c1,2   /* Hyp. Coprocessor Trap Register 
>>>> */
>>>
>>> Is this intentionally unused? I don't mind adding unused register
>>> definitions, just wanted to check there wasn't a hunk missing or
>>> anything.
>>
>>
>> Another lost when I have created the patch. I was using it to trap VFP
>> when I tested the context switch.
> 
> OK.
> 
>>
>>> The register resets to 0x0 which is OK but it might be wise to trap all
>>> accesses the coprocessors which we haven't implemented ctx switching
>>> for? So at least we find out about missing ones instead of silently
>>> leaking information between guests and/or corrupting their state.
>>
>>
>> What about sending an undefined instruction to the guest if the
>> coprocessor is not implemented?
>> It seems that some software (sshd, ntpdate) which are using libcrypto,
>> are trying to access to the cryptographic coprocessor even if it doesn't
>> exist.
> 
> Urk. We should either ctx switch it properly or we should hide it
> properly (from all the feature registers) and inject undef.


I will try to write a patch for that when I will have time.

-- 
Julien

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel