Re: [Xen-devel] Getting the XSAVE size from userspace

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/05/2015 04:05 PM, Andrew Cooper wrote:
> On 05/11/15 12:26, Razvan Cojocaru wrote:
>> On 11/05/2015 01:44 PM, Andrew Cooper wrote:
>>> On 05/11/15 11:35, Andrei LUTAS wrote:
>>>> The use-case is the following: whenever an EPT violation is triggered
>>>> inside a monitored VM, the introspection logic needs to know how many
>>>> bytes were accessed (read/written). This is done by inspecting the
>>>> faulting instruction and directly inferring the size, which is not
>>>> straight-forward for XSAVE/XRSTOR family. Using the maximum possible
>>>> size is wrong, as in any given moment the OS may or may not desire to
>>>> XSAVE/XRSTOR the entire state (and thinking that the instruction tries
>>>> to access more than it actually does may yield undesired effects).
>>>> Therefore, the size needed for the currently enabled features of the
>>>> monitored guest is required instead. Normally, it could be done by
>>>> running CPUID with eax = 0xD and ecx = i, where i >= 2 and XCR0[i] is
>>>> 1 (XCR0 belongs to the monitored guest), but I am unsure if using
>>>> CPUID this way would be safe/desired: will Xen expose the same CPUID
>>>> features, for XSAVE related functionality, on all VMs? (using XCPUID
>>>> with eax = 0xD and ecx = 0 would give us the needed size for the SVA,
>>>> and like I said, using the maximum size would not be safe, even if
>>>> it's the same across all VMs on a given host). Also, I'm unsure how
>>>> this would get along with migration...
>>> Hmm yes - there is no way to do this currently.
>>>
>>> Xen's CPUID handling for xsave related things is broken in levelling and
>>> migration scenarios, which is why it is *still* disabled by default in
>>> XenServer.
>>>
>>> I am working on fixing it, and will take this usecase into account
>>> (although I think I had already included enough for this usecase to work).
>>>
>>> At the point of the xsave/xrestor trap, you need to know xcr0 and be
>>> able to perfom a cpuid instruction in the context of a target domain, to
>>> make use of 0xD[0].ebx to get the "current size based on xcr0".
>> So then the closest thing to what we need would be to add a size field
>> to struct hvm_hw_cpu_xsave, and just assign the size variable to it in
>> hvm_save_cpu_xsave_states (migration aside)?
>>
>> 2130 static int hvm_save_cpu_xsave_states(struct domain *d,
>> hvm_domain_context_t *h)
>> 2131 {
>> 2132     struct vcpu *v;
>> 2133     struct hvm_hw_cpu_xsave *ctxt;
>> 2134
>> 2135     if ( !cpu_has_xsave )
>> 2136         return 0;   /* do nothing */
>> 2137
>> 2138     for_each_vcpu ( d, v )
>> 2139     {
>> 2140         unsigned int size = HVM_CPU_XSAVE_SIZE(v->arch.xcr0_accum);
>> 2141
>> 2142         if ( !xsave_enabled(v) )
>> 2143             continue;
>> 2144         if ( _hvm_init_entry(h, CPU_XSAVE_CODE, v->vcpu_id, size) )
>> 2145             return 1;
>> 2146         ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur];
>> 2147         h->cur += size;
>> 2148
>> 2149         ctxt->xfeature_mask = xfeature_mask;
>> 2150         ctxt->xcr0 = v->arch.xcr0;
>> 2151         ctxt->xcr0_accum = v->arch.xcr0_accum;
>> 2152         memcpy(&ctxt->save_area, v->arch.xsave_area,
>> 2153                size - offsetof(struct hvm_hw_cpu_xsave, save_area));
>> 2154     }
>> 2155
>> 2156     return 0;
>> 2157 }
> 
> I don't see any difference between this pasted code and the current
> hvm_save_cpu_xsave_states().  What have you changed?

I haven't changed anything, I was just pointing out what code I'm
referring to (which size variable I'm talking about), sorry for not
being as clear as possible.

> You can't use this size value, and it is the accumulated xcr0 over the
> life of the VM, not the xcr0 in use at the time of the intercepted
> instruction.

OK.

> You also can't blindly modify the ctxt structure, or you will break
> migration.

Well, yes, not blindly, that assumes that something like a patch for
mainline is agreed upon, or that migration is disabled for guests that
need this, and so on.

> The xcr0 -> size mapping is static, and won't change going forwards. 
> Your best bet is just to query each one and stash all the results.

OK.


Thanks,
Razvan

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