Xen devel - Feb 2011 - [PATCH] xen: Pack some hvmop memory structures better

Some of the hvmop memory structures have a shocking amount of unnecesssary
padding in them.  Elements which can have only 3 values are given 64 bits of
memory, and then aligned (so that there is padding behind them).

This patch resizes and reorganizes in the following way, (hopefully) without
introducing any differences between the layout for 32- and 64-bit.

xen_hvm_set_mem_type:
 hvmmem_type -> 16 bits
 nr -> 32 bits (limiting us to setting 16TB at a time)

xen_hvm_set_mem_access:
 hvmmem_access -> 16 bits
 nr -> 32 bits

xen_hvm_get_mem_access:
 hvmmem_access -> 16 bits

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 4bdb78db22b6 -r dffa1a0edc8c xen/include/public/hvm/hvm_op.h
--- a/xen/include/public/hvm/hvm_op.h	Wed Feb 02 17:06:36 2011 +0000
+++ b/xen/include/public/hvm/hvm_op.h	Fri Feb 04 15:11:52 2011 +0000
@@ -119,11 +119,11 @@
     /* Domain to be updated. */
     domid_t domid;
     /* Memory type */
-    uint64_aligned_t hvmmem_type;
+    uint16_t hvmmem_type;
+    /* Number of pages. */
+    uint32_t nr;
     /* First pfn. */
     uint64_aligned_t first_pfn;
-    /* Number of pages. */
-    uint64_aligned_t nr;
 };
 typedef struct xen_hvm_set_mem_type xen_hvm_set_mem_type_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_mem_type_t);
@@ -176,13 +176,12 @@
 struct xen_hvm_set_mem_access {
     /* Domain to be updated. */
     domid_t domid;
-    uint16_t pad[3]; /* align next field on 8-byte boundary */
     /* Memory type */
-    uint64_t hvmmem_access; /* hvm_access_t */
+    uint16_t hvmmem_access; /* hvm_access_t */
+    /* Number of pages, ignored on setting default access */
+    uint32_t nr;
     /* First pfn, or ~0ull to set the default access for new pages */
     uint64_t first_pfn;
-    /* Number of pages, ignored on setting default access */
-    uint64_t nr;
 };
 typedef struct xen_hvm_set_mem_access xen_hvm_set_mem_access_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_mem_access_t);
@@ -192,11 +191,10 @@
 struct xen_hvm_get_mem_access {
     /* Domain to be queried. */
     domid_t domid;
-    uint16_t pad[3]; /* align next field on 8-byte boundary */
     /* Memory type: OUT */
-    uint64_t hvmmem_access; /* hvm_access_t */
+    uint16_t hvmmem_access; /* hvm_access_t */
     /* pfn, or ~0ull for default access for new pages.  IN */
-    uint64_t pfn;
+    uint64_aligned_t pfn;
 };
 typedef struct xen_hvm_get_mem_access xen_hvm_get_mem_access_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_get_mem_access_t);

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

On 02/04/2011 04:28 PM, George Dunlap wrote:
> Some of the hvmop memory structures have a shocking amount of unnecesssary
> padding in them.  Elements which can have only 3 values are given 64 bits
of
> memory, and then aligned (so that there is padding behind them).
>
> This patch resizes and reorganizes in the following way, (hopefully)
without
> introducing any differences between the layout for 32- and 64-bit.
Am I missing something glaring, or this is breaking the ABI between 
hypervisor and kernels?

Paolo

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

At 16:24 +0000 on 04 Feb (1296836660), Paolo Bonzini
wrote:
> Am I missing something glaring, or this is breaking the ABI between 
> hypervisor and kernels?
It only breaks the ABI between xen and tools, which are supposed always
to be version-matched.  I''m not sure that it''s particularly
worthwhile
though - at first glance it''s saving about ten bytes of argument space.
Unless these operations are happening in large batches?

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

At 16:35 +0000 on 04 Feb (1296837332), George Dunlap
wrote:
> On Fri, Feb 4, 2011 at 4:30 PM, Tim Deegan <Tim.Deegan@citrix.com>
wrote:
> > It only breaks the ABI between xen and tools, which are supposed
always
> > to be version-matched.  I''m not sure that it''s
particularly worthwhile
> > though - at first glance it''s saving about ten bytes of
argument space.
> > Unless these operations are happening in large batches?
> 
> It''s mostly about just making things cleaner.  It does, however,
have
> the advantage that the hvmop structure then fits within 7 32-bit
> words; and thus can be simply copied directly into a xentrace record,
> rather than having to be marshalled on a per-hvmop basis.
That''s good enough for me.  Also, I see Keir''s applied it
already. :)

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

>>> On 04.02.11 at 17:30, Tim Deegan <Tim.Deegan@citrix.com>
wrote:
> At 16:24 +0000 on 04 Feb (1296836660), Paolo Bonzini wrote:
>> Am I missing something glaring, or this is breaking the ABI between 
>> hypervisor and kernels?
> 
> It only breaks the ABI between xen and tools, which are supposed always
> to be version-matched.  I''m not sure that it''s
particularly worthwhile
According to x86''s do_hvm_op() I would say a HVM guest can issue
HVMOP_set_mem_type for itself. Is that perhaps a mistake?

Similarly, HVMOP_[gs]et_mem_access bail on
current->domain->domain_id == a.domid, but
rcu_lock_target_domain_by_id() happily accepts DOMID_SELF
without any further checks. At least for the "set" variant this
very much looks like a mistake to me.
> though - at first glance it''s saving about ten bytes of argument
space.
> Unless these operations are happening in large batches?
Jan


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