On Fri, Feb 22, 2008 at 02:10:21PM +0900, Isaku Yamahata wrote:
I have no idea what the intent is for this patch, but I guess the end
result is merely some typedefs. It does appear to need some cleanups
> diff --git a/include/asm-ia64/xen/interface.h b/include/asm-ia64/xen/interface.h
> new file mode 100644
> index 0000000..69418f6
> --- /dev/null
> +++ b/include/asm-ia64/xen/interface.h
...
> +#ifndef __HYPERVISOR_IF_IA64_H__
> +#define __HYPERVISOR_IF_IA64_H__
Same old comment.
> +
> +/*
> + * TODO:
> + * linux's interface header removed XEN prefix
> + * Now just work around
> + */
> +#define DEFINE_GUEST_HANDLE_STRUCT(name) \
> + __DEFINE_XEN_GUEST_HANDLE(name, struct name)
> +#define DEFINE_GUEST_HANDLE(name) DEFINE_XEN_GUEST_HANDLE(name)
> +#define GUEST_HANDLE(name) XEN_GUEST_HANDLE(name)
> +
> +/* Structural guest handles introduced in 0x00030201. */
> +#if __XEN_INTERFACE_VERSION__ >= 0x00030201
Will this ever be false for the stuff in the kernel tree? if not, we
should clean this up before committing it.
> +#define ___DEFINE_XEN_GUEST_HANDLE(name, type) \
> + typedef struct { type *p; } __guest_handle_ ## name
> +#else
> +#define ___DEFINE_XEN_GUEST_HANDLE(name, type) \
> + typedef type * __guest_handle_ ## name
> +#endif
> +
> +#define __DEFINE_XEN_GUEST_HANDLE(name, type) \
> + ___DEFINE_XEN_GUEST_HANDLE(name, type)
Assuming the above cleanup is valid, why have the __ and ___ variants?
> +#define DEFINE_XEN_GUEST_HANDLE(name) __DEFINE_XEN_GUEST_HANDLE(name, name)
> +#define XEN_GUEST_HANDLE(name) __guest_handle_ ## name
> +#define XEN_GUEST_HANDLE_64(name) XEN_GUEST_HANDLE(name)
By this point in the review, I am beginning to get the feeling that you
want any random group of characters that contain XEN, or GUEST to be a
valid group of characters. I am not asking you to change anything,
merely consider whether all of the defines are really needed. I don't
know and don't really want to know, but I do hope you take the time to
rethink the shear number of #defines that all come back to
___DEFINE_XEN_GUEST_HANDLE.
> +#define uint64_aligned_t uint64_t
> +#define set_xen_guest_handle(hnd, val) do { (hnd).p = val; } while (0)
> +#ifdef __XEN_TOOLS__
> +#define get_xen_guest_handle(val, hnd) do { val = (hnd).p; } while (0)
> +#endif
> +
> +#ifndef __ASSEMBLY__
> +/* Guest handles for primitive C types. */
> +__DEFINE_XEN_GUEST_HANDLE(uchar, unsigned char);
> +__DEFINE_XEN_GUEST_HANDLE(uint, unsigned int);
> +__DEFINE_XEN_GUEST_HANDLE(ulong, unsigned long);
> +__DEFINE_XEN_GUEST_HANDLE(u64, unsigned long);
> +DEFINE_XEN_GUEST_HANDLE(char);
> +DEFINE_XEN_GUEST_HANDLE(int);
> +DEFINE_XEN_GUEST_HANDLE(long);
> +DEFINE_XEN_GUEST_HANDLE(void);
> +
> +typedef unsigned long xen_pfn_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_pfn_t);
> +#define PRI_xen_pfn "lx"
> +#endif
> +
> +/* Arch specific VIRQs definition */
> +#define VIRQ_ITC VIRQ_ARCH_0 /* V. Virtual itc timer */
> +#define VIRQ_MCA_CMC VIRQ_ARCH_1 /* MCA cmc interrupt */
> +#define VIRQ_MCA_CPE VIRQ_ARCH_2 /* MCA cpe interrupt */
> +
> +/* Maximum number of virtual CPUs in multi-processor guests. */
> +/* WARNING: before changing this, check that shared_info fits on a page */
Just asking again, but can the above be turned into either a #error,
#warn, or a BUG_ON()?
> +#define MAX_VIRT_CPUS 64
> +
> +#ifndef __ASSEMBLY__
> +
> +typedef unsigned long xen_ulong_t;
> +
> +#ifdef __XEN_TOOLS__
> +#define XEN_PAGE_SIZE XC_PAGE_SIZE
> +#else
> +#define XEN_PAGE_SIZE PAGE_SIZE
> +#endif
> +
> +#define INVALID_MFN (~0UL)
> +
> +#define MEM_G (1UL << 30)
> +#define MEM_M (1UL << 20)
> +#define MEM_K (1UL << 10)
> +
> +/* Guest physical address of IO ports space. */
> +#define IO_PORTS_PADDR 0x00000ffffc000000UL
> +#define IO_PORTS_SIZE 0x0000000004000000UL
> +
> +#define MMIO_START (3 * MEM_G)
> +#define MMIO_SIZE (512 * MEM_M)
> +
> +#define VGA_IO_START 0xA0000UL
> +#define VGA_IO_SIZE 0x20000
> +
> +#define LEGACY_IO_START (MMIO_START + MMIO_SIZE)
> +#define LEGACY_IO_SIZE (64 * MEM_M)
> +
> +#define IO_PAGE_START (LEGACY_IO_START + LEGACY_IO_SIZE)
> +#define IO_PAGE_SIZE XEN_PAGE_SIZE
> +
> +#define STORE_PAGE_START (IO_PAGE_START + IO_PAGE_SIZE)
> +#define STORE_PAGE_SIZE XEN_PAGE_SIZE
> +
> +#define BUFFER_IO_PAGE_START (STORE_PAGE_START + STORE_PAGE_SIZE)
> +#define BUFFER_IO_PAGE_SIZE XEN_PAGE_SIZE
> +
> +#define BUFFER_PIO_PAGE_START (BUFFER_IO_PAGE_START + BUFFER_IO_PAGE_SIZE)
> +#define BUFFER_PIO_PAGE_SIZE XEN_PAGE_SIZE
> +
> +#define IO_SAPIC_START 0xfec00000UL
> +#define IO_SAPIC_SIZE 0x100000
> +
> +#define PIB_START 0xfee00000UL
> +#define PIB_SIZE 0x200000
> +
> +#define GFW_START (4 * MEM_G - 16 * MEM_M)
> +#define GFW_SIZE (16 * MEM_M)
> +
> +/* Nvram belongs to GFW memory space */
> +#define NVRAM_SIZE (MEM_K * 64)
> +#define NVRAM_START (GFW_START + 10 * MEM_M)
> +
> +#define NVRAM_VALID_SIG 0x4650494e45584948 /* "HIXENIPF" */
> +struct nvram_save_addr {
> + unsigned long addr;
> + unsigned long signature;
> +};
> +
> +struct pt_fpreg {
> + union {
> + unsigned long bits[2];
> + long double __dummy; /* force 16-byte alignment */
> + } u;
> +};
> +
> +union vac {
> + unsigned long value;
> + struct {
> + int a_int:1;
> + int a_from_int_cr:1;
> + int a_to_int_cr:1;
> + int a_from_psr:1;
> + int a_from_cpuid:1;
> + int a_cover:1;
> + int a_bsw:1;
> + long reserved:57;
> + };
> +};
> +typedef union vac vac_t;
I thought typedefs were frowned upon.
I only skimmed the remainder of the file. More of the same comments you
have seen before.
> +/* xen perfmon */
> +#ifdef XEN
> +#ifndef __ASSEMBLY__
> +#ifndef _ASM_IA64_PERFMON_H
> +
> +#include <xen/list.h> /* asm/perfmon.h requires struct list_head */
> +#include <asm/perfmon.h>
Why not just always include these two. They should be doing their own
#ifndef's to prevent reinclusion. If not, correct them.
Thanks,
Robin
-
To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
|