Re: [Xen-devel] [PATCH v2 16/29] Ovmf/Xen: fix pointer to int cast in XenBusDxe

[Laszlo Ersek <lersek@xxxxxxxxxx>]

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 <stefano.stabellini@xxxxxxxxxxxxx>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> ---
>  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 <lersek@xxxxxxxxxx>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel