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

[Tamas K Lengyel <tamas@tklengyel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3598 bytes --]

On May 31, 2016 01:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 30.05.16 at 21:47, <tamas@tklengyel.com> wrote:
> > On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
> >>> +struct vm_event_regs_arm32 {
> >>> +    uint32_t r0_usr;
> >>> +    uint32_t r1_usr;
> >>> +    uint32_t r2_usr;
> >>> +    uint32_t r3_usr;
> >>> +    uint32_t r4_usr;
> >>> +    uint32_t r5_usr;
> >>> +    uint32_t r6_usr;
> >>> +    uint32_t r7_usr;
> >>> +    uint32_t r8_usr;
> >>> +    uint32_t r9_usr;
> >>> +    uint32_t r10_usr;
> >>> +    uint32_t r12_usr;
> >>> +    uint32_t lr_usr;
> >>> +    uint32_t fp;
> >>> +    uint32_t pc;
> >>> +    uint32_t sp_usr;
> >>> +    uint32_t sp_svc;
> >>> +    uint32_t spsr_svc;
> >>> +};
> >>
> >> It would seem more natural for the "ordinary" registers to be
> >> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.
> >
> > Not sure I follow.
>
> For one it is quite natural for someone looking at a sequence of
> register values to assume / expect them to be in natural order.
> And then, having them in natural (numeric) order allows e.g.
> extracting the register identifying bits from an instruction to use
> them as an array index into (part of) this structure.
>
> (For some background: I've been bitten a number of times by
> people sorting x86 registers alphabetically instead or naturally,
> i.e. EAX, EBX, ECX, EDX instead of EAX, ECX, EDX, EBX).

I see, however I believe that would be a very careless use of this struct
from the user as the register sizes are not even necessarily match the
architecture. For example we only define the 64bit x86 registers, so trying
to access it as an array of 32bit registers wouldn't work at all. Plus we
are not doing a full set of registers, and I rather not imply that every
element in the "natural sequence" is present. It may be, it may be not.

>
> >>> +struct vm_event_regs_arm64 {
> >>> +    uint64_t x0;
> >>> +    uint64_t x1;
> >>> +    uint64_t x2;
> >>> +    uint64_t x3;
> >>> +    uint64_t x4;
> >>> +    uint64_t x5;
> >>> +    uint64_t x6;
> >>> +    uint64_t x7;
> >>> +    uint64_t x8;
> >>> +    uint64_t x9;
> >>> +    uint64_t x10;
> >>> +    uint64_t x16;
> >>> +    uint64_t lr;
> >>> +    uint64_t fp;
> >>> +    uint64_t pc;
> >>> +    uint64_t sp_el0;
> >>> +    uint64_t sp_el1;
> >>> +    uint32_t spsr_el1;
> >>> +    uint32_t _pad;
> >>> +};
> >>
> >> My ARM knowledge is certainly quite limited, but the incomplete set
> >> of GPRs here is quite obvious: Is there a reason to not make all of
> >> them available here? And if there is, could the criteria of which
> >> registers got put here please be documented in a comment (so that
> >> the next person noticing the incomplete set won't ask again)?
> >
> > There already is a comment in place that explains why we are not
> > sending the full set of registers here.
>
> I've just gone through the entire patch again, and I didn't find any.
> Are you perhaps referring to "Using custom vCPU structs (i.e. not
> hvm_hw_cpu) for both x86 and ARM so as to not fill the vm_event
> ring buffer too quickly"? If so, that's irrelevant here: It explains why
> e.g. floating point registers don't get sent, but it doesn't explain in
> any way why some GPRs are more important than others.
>
> >> I'm also puzzled by fp and lr - I'm not aware of registers of those
> >> names (and gas also doesn't accept them afaict).
> >
> > Not sure why but Xen names x29 fp and x30 lr. See
> > include/asm-arm/arm64/processor.h.
>
> See Julien's reply.
>
> Jan
>

[-- Attachment #1.2: Type: text/html, Size: 5193 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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