Re: [PATCH v4 5/8] arm/vm_event: get/set registers

["Jan Beulich" <JBeulich@suse.com>]

>>> On 01.06.16 at 21:34, <rcojocaru@bitdefender.com> wrote:
> On 06/01/16 21:21, Tamas K Lengyel wrote:
>> On Wed, Jun 1, 2016 at 5:24 AM, Julien Grall <julien.grall@arm.com> wrote:
>>> On 01/06/16 09:41, Jan Beulich wrote:
>>>> Once an ABI is set in stone, and if that ABI allows for optimizations
>>>> (by consumers) like the one mentioned, I don't think this would be
>>>> careless use. The resulting code is very clearly much more efficient
>>>> than e.g. a switch() statement with a case label for each and every
>>>> register. Well, yes, I already hear the "memory is cheap and hence
>>>> code size doesn't matter" argument, but as said elsewhere quite
>>>> recently I don't buy this.
>>>
>>>
>>> I agree with Jan here.
>>>
>>> ARM64 has 31 general purposes registers (x0-x30). The switch to find a
>>> register based on the index will be quite big.
>>>
>>> If you order the register and provide all the general purposes registers (x0
>>> - x30), you will be able to replace by a single line (for instance see
>>> select_user_reg in arch/arm/traps.c).
>> 
>> The issue is that the x86 vm_event struct right now has 32*uint64_t
>> size. So if we would want to transmit all ARM64 GPRs + cpsr, PC and
>> TTBR0/1 we would end up growing this structure beyond what it is
>> currently. I want to avoid that as it affects both ARM32 and x86
>> introspection applications as well as now we can hold fewer events on
>> the ring. I would say it is better to avoid that then potentially
>> saving some on a switch in ARM64 introspection applications.
>> 
>>>
>>> This will also likely speed up your introspection application.
>> 
>> Win some, lose some. I would prefer if these changes had no
>> cross-architectural effect unless absolutely required. Razvan, what do
>> you think?

Perhaps that cross architectural effect then hints at some design
shortcoming? I.e. things should have been arranged for changes
to one arch'es register set to _never_ affect others.

> I think that if we keep a single page sized event ring buffer it would
> be best to only send what consumers need right now.
> 
> The only purpose of having that information in the request is to quickly
> get things that are immediately necessary - otherwise the full context
> can be obtained with xc_domain_hvm_getcontext_partial().
> 
> As long as there's a comment present that says that this is all the
> information needed at this time, and the struct can grow if needs
> change, it might be best not to add unnecessary data just in case
> somebody will need it later.

Well, that's not quite enough I would say (and this is as a general
remark, as I've peeked ahead and hence did see that Tamas agreed
to include all GPRs): The criteria for inclusion or exclusion should
follow a predictable model. I.e. if someone comes along and says
"I need register Y", then there should be rules that (s)he can apply
up front to determine what (at least in the vast majority of cases)
the response is going to be. You saying "I need only this arbitrary
subset of registers" is completely in-transparent, as it leaves open
why this is so, and why this would also be so for others. I.e. you'd
at least need to answer the question why (as an example) you
need x5 included, but not x25, despite both registers being equal
from an architecture pov. An answer like "My application gets away
with it" is not acceptable here.

Jan


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