In tools/ioemu/hw/iommu.c, I found #define PAGE_MASK (PAGE_SIZE - 1), I
think it''s not correct.
diff -r dfca1120813f tools/ioemu/hw/iommu.c
--- a/tools/ioemu/hw/iommu.c Sun Nov 11 18:28:57 2007 +0000
+++ b/tools/ioemu/hw/iommu.c Tue Nov 13 13:08:16 2007 +0800
@@ -88,7 +88,7 @@ do { printf("IOMMU: " fmt , ##args); } w
#define PAGE_SHIFT 14
#endif
#define PAGE_SIZE (1 << PAGE_SHIFT)
-#define PAGE_MASK (PAGE_SIZE - 1)
+#define PAGE_MASK (~(PAGE_SIZE - 1))
typedef struct IOMMUState {
uint32_t addr
-- Weidong
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Han, Weidong writes ("[Xen-devel] [QEMU] Correct PAGE_MASK
definition"):> In tools/ioemu/hw/iommu.c, I found #define PAGE_MASK (PAGE_SIZE - 1), I
> think it''s not correct.
Well, yes, but just changing the definition isn''t correct either.
This particular definition is used in only one place, in
iommu_translate_pa:
pa = ((pa & IOPTE_PAGE) << 4) + (addr & PAGE_MASK);
So here it is used as `the part of the address which specifies the
location within the page'' rather than `which specifies the
page''.
This is both confusing and inconsistent with most of the other things
called *PAGE_MASK.
grepping xen-unstable shows similar confusion here:
extras/mini-os/include/ia64/efi.h:#define EFI_PAGE_MASK 0xFFF
tools/libxc/xc_ptrace.h:#define BSD_PAGE_MASK (PAGE_SIZE-1)
This confusion seems to be present in linux-2.6.18-xen.hg too (and I
imagine these came from upstream):
arch/mips/boot/addinitrd.c:#define MIPS_PAGE_MASK (MIPS_PAGE_SIZE-1)
drivers/media/dvb/b2c2/flexcop-usb.h:#define V8_MEMORY_PAGE_MASK 0x7FFF
drivers/mtd/nand/nand_base.c:#define BBT_PAGE_MASK 0xffffff3f
drivers/w1/slaves/w1_ds2433.c:#define W1_PAGE_MASK 0x1F
Under these circumstances it may be best to just leave things be
rather than try to make this consistent. In any case if you change
the definition you must change the point of use too.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson wrote:> Han, Weidong writes ("[Xen-devel] [QEMU] Correct PAGE_MASK > definition"): >> In tools/ioemu/hw/iommu.c, I found #define PAGE_MASK (PAGE_SIZE - >> 1), I think it''s not correct. > > Well, yes, but just changing the definition isn''t correct either. > This particular definition is used in only one place, in > iommu_translate_pa: > > pa = ((pa & IOPTE_PAGE) << 4) + (addr & PAGE_MASK); > > So here it is used as `the part of the address which specifies the > location within the page'' rather than `which specifies the page''. > This is both confusing and inconsistent with most of the other things > called *PAGE_MASK. > > grepping xen-unstable shows similar confusion here: > extras/mini-os/include/ia64/efi.h:#define EFI_PAGE_MASK 0xFFF > tools/libxc/xc_ptrace.h:#define BSD_PAGE_MASK (PAGE_SIZE-1) > > This confusion seems to be present in linux-2.6.18-xen.hg too (and I > imagine these came from upstream): > arch/mips/boot/addinitrd.c:#define MIPS_PAGE_MASK > (MIPS_PAGE_SIZE-1) drivers/media/dvb/b2c2/flexcop-usb.h:#define > V8_MEMORY_PAGE_MASK 0x7FFF drivers/mtd/nand/nand_base.c:#define > BBT_PAGE_MASK 0xffffff3f drivers/w1/slaves/w1_ds2433.c:#define > W1_PAGE_MASK 0x1F > > Under these circumstances it may be best to just leave things be > rather than try to make this consistent. In any case if you change > the definition you must change the point of use too. >Agree, but inconsistent usage is not good after all. -- Weidong _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel