Re: [PATCH v3 08/12] x86/emulator: Refactor FXSAVE_AREA to use wrappers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.01.2025 16:42, Alejandro Vallejo wrote:
> On Mon Jan 27, 2025 at 10:52 AM GMT, Jan Beulich wrote:
>> On 10.01.2025 14:28, Alejandro Vallejo wrote:
>>> Adds an UNMAP primitive to make use of vcpu_unmap_xsave_area() when
>>> linked into xen. unmap is a no-op during tests.
>>>
>>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
>>
>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Thanks,
> 
>> despite ...
>>
>>> --- a/xen/arch/x86/x86_emulate/blk.c
>>> +++ b/xen/arch/x86/x86_emulate/blk.c
>>> @@ -11,9 +11,12 @@
>>>      !defined(X86EMUL_NO_SIMD)
>>>  # ifdef __XEN__
>>>  #  include <asm/xstate.h>
>>> -#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
>>> +/* Has a fastpath for `current`, so there's no actual map */
>>> +#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
>>> +#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(current, x)
>>
>> ... the comment here kind of suggesting that ...
>>
>>>  # else
>>>  #  define FXSAVE_AREA get_fpu_save_area()
>>> +#  define UNMAP_FXSAVE_AREA(x) ((void)(x))
>>
>> ... use of this new construct is solely decoration, and could hence as
>> well be omitted.
> 
> It seems like a dangerous proposition to abuse knowledge of an implementation
> in order to skip parts of its interface. The fact that no such map is required
> at this point in x86_emulate does not mean it never will be. Predicting the
> future is hard, but being consistent today is less so (imo).

Entirely true. How likely do you consider it though that with a future
change altering that property, the comment above would be left in place
(and hence then be stale)? My take: Very likely, as long as the two
"current" uses wouldn't need altering.

Yet FTAOD: I'm not asking for any adjustment here, I'm merely mentioning
an observation.

Jan