Re: [PATCH v1 4/7] x86/vmx: add do_vmtrace_op

[Tamas K Lengyel <tamas.k.lengyel@gmail.com>]

On Thu, Jun 18, 2020 at 9:41 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 18.06.2020 17:25, Michał Leszczyński wrote:
> > ----- 16 cze 2020 o 19:23, Roger Pau Monné roger.pau@citrix.com napisał(a):
> >> On Tue, Jun 16, 2020 at 05:22:06PM +0200, Michał Leszczyński wrote:
> >>> --- a/xen/include/public/hvm/hvm_op.h
> >>> +++ b/xen/include/public/hvm/hvm_op.h
> >>> @@ -382,6 +382,33 @@ struct xen_hvm_altp2m_op {
> >>>  typedef struct xen_hvm_altp2m_op xen_hvm_altp2m_op_t;
> >>>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_op_t);
> >>>
> >>> +/* HVMOP_vmtrace: Perform VM tracing related operation */
> >>> +#define HVMOP_vmtrace 26
> >>> +
> >>> +#define HVMOP_VMTRACE_INTERFACE_VERSION 0x00000001
> >>> +
> >>> +struct xen_hvm_vmtrace_op {
> >>> +    /* IN variable */
> >>> +    uint32_t version;   /* HVMOP_VMTRACE_INTERFACE_VERSION */
> >>> +    uint32_t cmd;
> >>> +/* Enable/disable external vmtrace for given domain */
> >>> +#define HVMOP_vmtrace_ipt_enable      1
> >>> +#define HVMOP_vmtrace_ipt_disable     2
> >>> +#define HVMOP_vmtrace_ipt_get_buf     3
> >>> +#define HVMOP_vmtrace_ipt_get_offset  4
> >>> +    domid_t domain;
> >>
> >> You are missing a padding field here AFAICT.
> >
> >
> > Could you point out what is the purpose of this padding field and what should be the size in this case? It's pretty unclear to me.
>
> In the public interface we aim at making all padding explicit.

Just to expand a bit on this: this is an ABI meaning the hypervisor
and the tool sending this structure communicate via memory only. Since
the hypervisor and the compiler can be compiled using different
compilers, using stuff that's not explicit in the C standard needs to
be avoided. For example using standard types like "int" or "long" is
no good since it's really up to the compiler to decide how much memory
those need. The C specification is great like that.. Same stands for
padding. Different compilers can decide to align things differently,
pack things or not pack things, so we have to manually add the padding
to take that choice away from the compiler.

As discussed many times on the list, using C struct as the base of the
ABI was a bad design decision to start with, but we are now stuck with
it. It would now make more sense to use something like JSON to pass
information like this between the hypervisor and the toolstack which
is what we opted to do in new hypervisors like Bareflank/Boxy.

Tamas