Re: [Xen-devel] [PATCH v2 28/29] ArmVirtualizationPkg/VirtFdtDxe: wire up XenBusDxe to "xen, xen" DT node

[Laszlo Ersek <lersek@xxxxxxxxxx>]

On 01/26/15 20:03, Ard Biesheuvel wrote:
> This patchs adds support to VirtFdtDxe for the Xen DT node which
> contains the base address of the Grant Table. This data is communicated
> to XenBusDxe using a XENIO_PROTOCOL instance.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> ---
>  ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c   | 23 
> +++++++++++++++++++++++
>  ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf |  1 +
>  2 files changed, 24 insertions(+)
> 
> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c 
> b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> index 34fac40fa803..1ceb85146430 100644
> --- a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> @@ -26,6 +26,7 @@
>  #include <Library/DxeServicesLib.h>
>  #include <Library/HobLib.h>
>  #include <libfdt.h>
> +#include <Library/XenIoMmioLib.h>
>  
>  #include <Guid/Fdt.h>
>  #include <Guid/VirtioMmioTransport.h>
> @@ -49,6 +50,7 @@ typedef enum {
>    PropertyTypePsci,
>    PropertyTypeFwCfg,
>    PropertyTypeGicV3,
> +  PropertyTypeXen,
>  } PROPERTY_TYPE;
>  
>  typedef struct {
> @@ -66,6 +68,7 @@ STATIC CONST PROPERTY CompatibleProperties[] = {
>    { PropertyTypePsci,    "arm,psci-0.2"        },
>    { PropertyTypeFwCfg,   "qemu,fw-cfg-mmio"    },
>    { PropertyTypeGicV3,   "arm,gic-v3"          },
> +  { PropertyTypeXen,     "xen,xen"             },
>    { PropertyTypeUnknown, ""                    }
>  };
>  
> @@ -332,6 +335,26 @@ InitializeVirtFdtDxe (
>        }
>        break;
>  
> +    case PropertyTypeXen:
> +      ASSERT (Len == 16);
> +
> +      //
> +      // Retrieve the reg base from this node and add it to a
> +      // XENIO_PROTOCOL instance installed on a new handle.
> +      //
> +      RegBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
> +      Handle = NULL;
> +      Status = XenIoMmioInstall (&Handle, RegBase);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((EFI_D_ERROR, "%a: Failed to install XENIO_PROTOCOL on a new 
> handle "
> +          "(Status == %r)\n", __FUNCTION__, Status));

(1) I don't think it's necessary to mention XENIO_PROTOCOL here.
XenIoMmioInstall() does more, and can fail for more reasons. I think it
would suffice to mention XenIoMmioInstall() and the status it returns.
(XenIoMmioInstall() logs errors internally anyway.) I don't insist though.

> +        break;
> +      }
> +
> +      DEBUG ((EFI_D_INFO, "Found Xen node with Grant table @ 0x%p\n", 
> RegBase));

(2) 0x%p is incorrect here, please say 0x%Lx. RegBase is not a pointer
but a UINT64.

> +
> +      break;
> +
>      default:
>        break;
>      }
> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf 
> b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf
> index 1392c7c3fa45..f8a58238c37b 100644
> --- a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf
> @@ -41,6 +41,7 @@
>    FdtLib
>    VirtioMmioDeviceLib
>    HobLib
> +  XenIoMmioLib
>  
>  [Guids]
>    gFdtTableGuid
> 

With those changes:

Reviewed-by: Laszlo Ersek <lersek@xxxxxxxxxx>

Thanks
Laszlo

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