Re: [Xen-devel] [PATCH] x86/vm_event: add gdtr_base to the vm_event structure

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, May 2, 2019 at 2:57 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 02.05.19 at 10:25, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> > On 5/2/19 2:57 AM, Tamas K Lengyel wrote:
> >> Receiving this register is useful for introspecting 32-bit Windows when the
> >> event being trapped happened while in ring3.
> >>
> >> Signed-off-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> >> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> >> Cc: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> >> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> >> Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> >> ---
> >>   xen/arch/x86/vm_event.c       | 5 +++++
> >>   xen/include/public/vm_event.h | 3 ++-
> >>   2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> >> index 51c3493b1d..873788e32f 100644
> >> --- a/xen/arch/x86/vm_event.c
> >> +++ b/xen/arch/x86/vm_event.c
> >> @@ -179,6 +179,10 @@ static void vm_event_pack_segment_register(enum
> > x86_segment segment,
> >>           reg->es_sel = seg.sel;
> >>           break;
> >>
> >> +    case x86_seg_gdtr:
> >> +        reg->gdtr_base = seg.base;
> >> +        break;
> >
> > Please also add limit, ar, sel, like the others do.
>
> There's no ar or sel for GDT (and IDT). Instead, because of ...
>
> > In addition to
> > making this modification look less strange (since, in contrast to the
> > function name, nothing is packed for gdtr_base), it will also save
> > future work for applications wanting to use gdtr which also require
> > backwards compatibility with previous vm_event versions.
> >
> > As you know, for each such modification we need to have a separate
> > vm_event_vX header in our applications so that we're ready to create a
> > ring buffer using requests and replies of the right size, and also to be
> > able to properly interpret the ring buffer data, so the least frequent
> > changes to the vm_event struct, the better.
>
> ... this I wonder whether the IDT details shouldn't get added at
> the same time.

The churn of the header is not that big of a deal - I do the same in
LibVMI and it's a largely copy-paste job that takes perhaps ~5m to add
(see 
https://github.com/tklengyel/libvmi/commit/7509ce56d0408dbec4e374b8780da69a7bd212e8).
So that should not be a factor in deciding whether we add a needed
extra piece or not. Since the vm_event ABI is changing for Xen 4.13
now the ABI version will only be bumped once for 4.13. So we should be
able to add/remove/shuffle whatever we want while 4.13 merge window is
open.

That said I don't have a use for idt and gdtr_limit that warrants
having to receive it via the vm_event structure - those pieces are
always just a hypercall away if needed. So in the vm_event structure I
tend to just put the registers needed often enough to warrant avoiding
that extra hypercall. But if Razvan says he has a use for them, I can
add them here.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel