Re: [Xen-devel] [PATCH 4 of 6] REDO: mem_access & mem_access 2: HVMOPs for setting mem access

[Joe Epstein <jepstein@xxxxxxxxxxxxxxxxxxxx>]

One comment in line.

Thanks!

On Wed, Jan 5, 2011 at 6:25 AM, Tim Deegan <Tim.Deegan@xxxxxxxxxx> wrote:

Hi,

The libxc parts of this will need an Ack from one of the tools
maintainers.   Apart from that:

At 22:07 +0000 on 04 Jan (1294178841), Joe Epstein wrote:
> +/* Notify that a region of memory is to have specific access types */
> +struct xen_hvm_set_mem_access {
> +    /* Domain to be updated. */
> +    domid_t domid;
> +    /* Memory type */
> +    hvmmem_access_t hvmmem_access;
> +    /* 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);

Please arrange that these structs have the same size and layout in
32-bit and 64-bit builds.  Otherwise you'll have to write compatibility
code for 32-bit tools running on 64-bit Xen, and nobody wants more of
that.  The easiest thing is to use explicitly-sized intXX_t throughout,
naturally aligned.

Sorry, I just copied set_mem_type, which also had an enum.  I'll force the bit size on the mem_access functions...would you want me to change set_mem_type (not my code) as well for consistency?
 

> +#define HVMOP_get_mem_access        13
> +/* Get the specific access type for that region of memory */
> +struct xen_hvm_get_mem_access {
> +    /* Domain to be queried. */
> +    domid_t domid;
> +    /* Memory type: OUT */
> +    hvmmem_access_t hvmmem_access;
> +    /* pfn, or ~0ull for default access for new pages.  IN */
> +    uint64_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);
>  #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */
> diff -r 85a7611248b8 -r 4ead881da87d tools/libxc/Makefile
> --- a/tools/libxc/Makefile      Tue Jan 04 12:16:42 2011 -0800
> +++ b/tools/libxc/Makefile      Tue Jan 04 12:28:36 2011 -0800
> @@ -28,6 +28,7 @@
>  CTRL_SRCS-y       += xc_tmem.c
>  CTRL_SRCS-y       += xc_mem_event.c
>  CTRL_SRCS-y       += xc_mem_paging.c
> +CTRL_SRCS-y       += xc_mem_access.c
>  CTRL_SRCS-y       += xc_memshr.c
>  CTRL_SRCS-y       += xc_hcall_buf.c
>  CTRL_SRCS-y       += xc_foreign_memory.c
> diff -r 85a7611248b8 -r 4ead881da87d tools/libxc/xc_mem_access.c
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/libxc/xc_mem_access.c       Tue Jan 04 12:28:36 2011 -0800
> @@ -0,0 +1,42 @@
> +/******************************************************************************
> + *
> + * tools/libxc/xc_mem_access.c
> + *
> + * Interface to low-level memory access mode functionality
> + *
> + * Copyright (c) 2009 by Citrix Systems, Inc.

Ahem.

Otherwise, this looks OK to me.

Tim.

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

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