On 01/26/15 20:03, Ard Biesheuvel wrote: > On ARM, xen_pfn_t is 64 bits but the size of a pointer is only > 32 bits, so casting between them needs to go via (UINTN). Also > move the xen_pfn_t cast outside the shift so that we can avoid > shifting 64-bit quantities on 32-bit architectures, which may > require runtime library support. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Reviewed-by: Stefano Stabellini <[email protected]> > Signed-off-by: Ard Biesheuvel <[email protected]> > --- > OvmfPkg/XenBusDxe/GrantTable.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/OvmfPkg/XenBusDxe/GrantTable.c b/OvmfPkg/XenBusDxe/GrantTable.c > index 37d3bf786c64..8405edc51bc4 100644 > --- a/OvmfPkg/XenBusDxe/GrantTable.c > +++ b/OvmfPkg/XenBusDxe/GrantTable.c > @@ -160,7 +160,7 @@ XenGrantTableInit ( > Parameters.domid = DOMID_SELF; > Parameters.idx = Index; > Parameters.space = XENMAPSPACE_grant_table; > - Parameters.gpfn = (((xen_pfn_t) GrantTable) >> EFI_PAGE_SHIFT) + Index; > + Parameters.gpfn = (xen_pfn_t) ((UINTN) GrantTable >> EFI_PAGE_SHIFT) + > Index; > ReturnCode = XenHypercallMemoryOp (Dev, XENMEM_add_to_physmap, > &Parameters); > if (ReturnCode != 0) { > DEBUG ((EFI_D_ERROR, "Xen GrantTable, add_to_physmap hypercall error: > %d\n", ReturnCode)); > @@ -182,7 +182,7 @@ XenGrantTableDeinit ( > > for (Index = NR_GRANT_FRAMES - 1; Index >= 0; Index--) { > Parameters.domid = DOMID_SELF; > - Parameters.gpfn = (((xen_pfn_t) GrantTable) >> EFI_PAGE_SHIFT) + Index; > + Parameters.gpfn = (xen_pfn_t) ((UINTN) GrantTable >> EFI_PAGE_SHIFT) + > Index; > DEBUG ((EFI_D_INFO, "Xen GrantTable, removing %X\n", Parameters.gpfn)); > ReturnCode = XenHypercallMemoryOp (Dev, XENMEM_remove_from_physmap, > &Parameters); > if (ReturnCode != 0) { >
One edk2 coding style guideline that I consistently reject & never apply is the first space in (type) expr1 <op> expr2 We had an obscure bug due to this guideline earlier, which I luckily managed to catch through review, before it could do damage. The problem with the first space is that it actively obscures the fact that the cast operator has one of the strongest operator bindings. Most of the time, the above means ((type)expr1) <op> expr2 and not (type)(expr1 <op> expr2) as the space would incorrectly suggest. Your patch is fine, but I had to think thrice. :) Reviewed-by: Laszlo Ersek <[email protected]> ------------------------------------------------------------------------------ Dive into the World of Parallel Programming. The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
