[PATCH v2 00/16] Add ACPI support for Xen Dom0 on ARM64

[PATCH v2 00/16] Add ACPI support for Xen Dom0 on ARM64

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

This patch set adds ACPI support for Xen Dom0 on ARM64. The relevant Xen
ACPI on ARM64 design document could be found from [1].

This patch set adds a new FDT node "uefi" under /hypervisor to pass UEFI
information. Introduce a bus notifier of AMBA and Platform bus to map
the new added device's MMIO space. Make Xen domain use
xlated_setup_gnttab_pages to setup grant table and a new hypercall to
get event-channel irq.

Regarding the initialization flow of Linux kernel, it needs to move
xen_early_init() before efi_init(). Then xen_early_init() will check
whether it runs on Xen through the /hypervisor node and efi_init() will
call a new function fdt_find_xen_uefi_params(), to parse those
xen,uefi-* parameters just like the existing efi_get_fdt_params().

And in arm64_enable_runtime_services() it will check whether it runs on
Xen and call another new function xen_efi_runtime_setup() to setup
runtime service instead of efi_native_runtime_setup(). The
xen_efi_runtime_setup() will assign the runtime function pointers with
the functions of driver/xen/efi.c.

And since we pass a /hypervisor node and a /chosen node to Dom0, it
needs to check whether the DTS only contains a /hypervisor node and a
/chosen node in acpi_boot_table_init().

Patches are tested on FVP base model. The corresponding Xen patches can
be fetched from [2].

Thanks,
Shannon

[1] http://lists.xen.org/archives/html/xen-devel/2015-11/msg00488.html
[2] http://git.linaro.org/people/shannon.zhao/xen.git  ACPI_XEN_ARM_V3


Changes since v1:
* Rebase on linux mainline and wallclock patch from Stefano
* Refactor AMBA and platform device MMIO map to one file
* Use EFI_PARAVIRT to check if it supports XEN EFI
* Refactor Xen EFI codes
* Address other comments

Shannon Zhao (16):
  Xen: ACPI: Hide UART used by Xen
  xen/grant-table: Move xlated_setup_gnttab_pages to common place
  Xen: xlate: Use page_to_xen_pfn instead of page_to_pfn
  arm/xen: Use xen_xlate_map_ballooned_pages to setup grant table
  xen: memory : Add new XENMAPSPACE type XENMAPSPACE_dev_mmio
  Xen: ARM: Add support for mapping platform device mmio
  Xen: ARM: Add support for mapping AMBA device mmio
  xen/hvm/params: Add a new delivery type for event-channel in
    HVM_PARAM_CALLBACK_IRQ
  arm/xen: Get event-channel irq through HVM_PARAM when booting with
    ACPI
  ARM: XEN: Move xen_early_init() before efi_init()
  ARM64: ACPI: Check if it runs on Xen to enable or disable ACPI
  ARM: Xen: Document UEFI support on Xen ARM virtual platforms
  ARM: XEN: Set EFI_PARAVIRT if Xen supports EFI
  Xen: EFI: Parse DT parameters for Xen specific UEFI
  XEN: EFI: Move x86 specific codes to architecture directory
  ARM64: XEN: Initialize Xen specific UEFI runtime services

 Documentation/devicetree/bindings/arm/xen.txt |  42 ++++++
 arch/arm/xen/enlighten.c                      | 121 +++++++++++++----
 arch/arm64/kernel/acpi.c                      |  12 +-
 arch/arm64/kernel/efi.c                       |   5 +
 arch/arm64/kernel/setup.c                     |   2 +-
 arch/arm64/xen/Makefile                       |   1 +
 arch/arm64/xen/efi.c                          |  36 +++++
 arch/x86/xen/efi.c                            | 112 ++++++++++++++++
 arch/x86/xen/grant-table.c                    |  57 +-------
 drivers/acpi/bus.c                            |  30 +++++
 drivers/firmware/efi/efi.c                    |  45 ++++++-
 drivers/xen/Kconfig                           |   2 +-
 drivers/xen/Makefile                          |   1 +
 drivers/xen/arm-device.c                      | 183 ++++++++++++++++++++++++++
 drivers/xen/efi.c                             | 174 +++++-------------------
 drivers/xen/xlate_mmu.c                       |  61 +++++++++
 include/xen/interface/hvm/params.h            |   5 +
 include/xen/interface/memory.h                |   1 +
 include/xen/xen-ops.h                         |  33 +++--
 19 files changed, 689 insertions(+), 234 deletions(-)
 create mode 100644 arch/arm64/xen/efi.c
 create mode 100644 drivers/xen/arm-device.c

-- 
2.0.4

[PATCH v2 01/16] Xen: ACPI: Hide UART used by Xen

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

ACPI 6.0 introduces a new table STAO to list the devices which are used
by Xen and can't be used by Dom0. On Xen virtual platforms, the physical
UART is used by Xen. So here it hides UART from Dom0.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
CC: "Rafael J. Wysocki" <rjw@rjwysocki.net> (supporter:ACPI)
CC: Len Brown <lenb@kernel.org> (supporter:ACPI)
CC: linux-acpi@vger.kernel.org (open list:ACPI)
---
 drivers/acpi/bus.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index a212cef..d7a559f 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -46,6 +46,7 @@ ACPI_MODULE_NAME("bus");
 struct acpi_device *acpi_root;
 struct proc_dir_entry *acpi_root_dir;
 EXPORT_SYMBOL(acpi_root_dir);
+static u64 spcr_uart_addr;
 
 #ifdef CONFIG_X86
 #ifdef CONFIG_ACPI_CUSTOM_DSDT
@@ -93,6 +94,17 @@ acpi_status acpi_bus_get_status_handle(acpi_handle handle,
 {
 	acpi_status status;
 
+	if (spcr_uart_addr != 0xffffffffffffffff) {
+		u64 addr;
+
+		status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL,
+					       &addr);
+		if (ACPI_SUCCESS(status) && (addr == spcr_uart_addr)) {
+			*sta = 0;
+			return AE_OK;
+		}
+	}
+
 	status = acpi_evaluate_integer(handle, "_STA", NULL, sta);
 	if (ACPI_SUCCESS(status))
 		return AE_OK;
@@ -1069,6 +1081,8 @@ EXPORT_SYMBOL_GPL(acpi_kobj);
 static int __init acpi_init(void)
 {
 	int result;
+	acpi_status status;
+	struct acpi_table_stao *stao_ptr;
 
 	if (acpi_disabled) {
 		printk(KERN_INFO PREFIX "Interpreter disabled.\n");
@@ -1081,6 +1095,22 @@ static int __init acpi_init(void)
 		acpi_kobj = NULL;
 	}
 
+	/* If there is STAO table, check whether it needs to ignore the UART
+	 * device in SPCR table.
+	 */
+	spcr_uart_addr = 0xffffffffffffffff;
+	status = acpi_get_table(ACPI_SIG_STAO, 0,
+				(struct acpi_table_header **)&stao_ptr);
+	if (ACPI_SUCCESS(status)) {
+		if (stao_ptr->ignore_uart) {
+			struct acpi_table_spcr *spcr_ptr;
+
+			acpi_get_table(ACPI_SIG_SPCR, 0,
+				       (struct acpi_table_header **)&spcr_ptr);
+			spcr_uart_addr = spcr_ptr->serial_port.address;
+		}
+	}
+
 	init_acpi_device_notify();
 	result = acpi_bus_init();
 	if (result) {
-- 
2.0.4

Re: [PATCH v2 01/16] Xen: ACPI: Hide UART used by Xen

[Stefano Stabellini]

On Fri, 15 Jan 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> ACPI 6.0 introduces a new table STAO to list the devices which are used
> by Xen and can't be used by Dom0. On Xen virtual platforms, the physical
> UART is used by Xen. So here it hides UART from Dom0.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> CC: "Rafael J. Wysocki" <rjw@rjwysocki.net> (supporter:ACPI)
> CC: Len Brown <lenb@kernel.org> (supporter:ACPI)
> CC: linux-acpi@vger.kernel.org (open list:ACPI)
> ---
>  drivers/acpi/bus.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index a212cef..d7a559f 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -46,6 +46,7 @@ ACPI_MODULE_NAME("bus");
>  struct acpi_device *acpi_root;
>  struct proc_dir_entry *acpi_root_dir;
>  EXPORT_SYMBOL(acpi_root_dir);
> +static u64 spcr_uart_addr;
>  
>  #ifdef CONFIG_X86
>  #ifdef CONFIG_ACPI_CUSTOM_DSDT
> @@ -93,6 +94,17 @@ acpi_status acpi_bus_get_status_handle(acpi_handle handle,
>  {
>  	acpi_status status;
>  
> +	if (spcr_uart_addr != 0xffffffffffffffff) {

Please #define something like EFI_INVALID_TABLE_ADDR and use it here
instead of 0xffffffffffffffff for readability. Also spcr_uart_addr could
be initialized where it is defined.

Maybe it could be better to move this check into a separate function and
only call acpi_set_device_status if spcr_uart_addr != addr?


> +		u64 addr;
> +
> +		status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL,
> +					       &addr);
> +		if (ACPI_SUCCESS(status) && (addr == spcr_uart_addr)) {
> +			*sta = 0;
> +			return AE_OK;
> +		}
> +	}
> +
>  	status = acpi_evaluate_integer(handle, "_STA", NULL, sta);
>  	if (ACPI_SUCCESS(status))
>  		return AE_OK;
> @@ -1069,6 +1081,8 @@ EXPORT_SYMBOL_GPL(acpi_kobj);
>  static int __init acpi_init(void)
>  {
>  	int result;
> +	acpi_status status;
> +	struct acpi_table_stao *stao_ptr;
>  
>  	if (acpi_disabled) {
>  		printk(KERN_INFO PREFIX "Interpreter disabled.\n");
> @@ -1081,6 +1095,22 @@ static int __init acpi_init(void)
>  		acpi_kobj = NULL;
>  	}
>  
> +	/* If there is STAO table, check whether it needs to ignore the UART
> +	 * device in SPCR table.
> +	 */
> +	spcr_uart_addr = 0xffffffffffffffff;
> +	status = acpi_get_table(ACPI_SIG_STAO, 0,
> +				(struct acpi_table_header **)&stao_ptr);
> +	if (ACPI_SUCCESS(status)) {
> +		if (stao_ptr->ignore_uart) {

if (ACPI_SUCCESS(status) && stao_ptr->ignore_uart) {


> +			struct acpi_table_spcr *spcr_ptr;
> +
> +			acpi_get_table(ACPI_SIG_SPCR, 0,
> +				       (struct acpi_table_header **)&spcr_ptr);

Maybe we should check that acpi_get_table succeeded, even though it
should at this point.


> +			spcr_uart_addr = spcr_ptr->serial_port.address;
> +		}
> +	}
> +
>  	init_acpi_device_notify();
>  	result = acpi_bus_init();
>  	if (result) {

Re: [PATCH v2 01/16] Xen: ACPI: Hide UART used by Xen

[Mark Rutland]

On Fri, Jan 15, 2016 at 02:55:14PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> ACPI 6.0 introduces a new table STAO to list the devices which are used
> by Xen and can't be used by Dom0. On Xen virtual platforms, the physical
> UART is used by Xen. So here it hides UART from Dom0.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> CC: "Rafael J. Wysocki" <rjw@rjwysocki.net> (supporter:ACPI)
> CC: Len Brown <lenb@kernel.org> (supporter:ACPI)
> CC: linux-acpi@vger.kernel.org (open list:ACPI)
> ---
>  drivers/acpi/bus.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index a212cef..d7a559f 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -46,6 +46,7 @@ ACPI_MODULE_NAME("bus");
>  struct acpi_device *acpi_root;
>  struct proc_dir_entry *acpi_root_dir;
>  EXPORT_SYMBOL(acpi_root_dir);
> +static u64 spcr_uart_addr;
>  
>  #ifdef CONFIG_X86
>  #ifdef CONFIG_ACPI_CUSTOM_DSDT
> @@ -93,6 +94,17 @@ acpi_status acpi_bus_get_status_handle(acpi_handle handle,
>  {
>  	acpi_status status;
>  
> +	if (spcr_uart_addr != 0xffffffffffffffff) {

The SPCR spec says that the Base Address fields being zero means that
console redirection is disabled (though I'm not clear on whether or not
that requires the whole acpi_generic_address to be zero).

Can we not use that here?

Mark.

> +		u64 addr;
> +
> +		status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL,
> +					       &addr);
> +		if (ACPI_SUCCESS(status) && (addr == spcr_uart_addr)) {
> +			*sta = 0;
> +			return AE_OK;
> +		}
> +	}
> +
>  	status = acpi_evaluate_integer(handle, "_STA", NULL, sta);
>  	if (ACPI_SUCCESS(status))
>  		return AE_OK;
> @@ -1069,6 +1081,8 @@ EXPORT_SYMBOL_GPL(acpi_kobj);
>  static int __init acpi_init(void)
>  {
>  	int result;
> +	acpi_status status;
> +	struct acpi_table_stao *stao_ptr;
>  
>  	if (acpi_disabled) {
>  		printk(KERN_INFO PREFIX "Interpreter disabled.\n");
> @@ -1081,6 +1095,22 @@ static int __init acpi_init(void)
>  		acpi_kobj = NULL;
>  	}
>  
> +	/* If there is STAO table, check whether it needs to ignore the UART
> +	 * device in SPCR table.
> +	 */
> +	spcr_uart_addr = 0xffffffffffffffff;
> +	status = acpi_get_table(ACPI_SIG_STAO, 0,
> +				(struct acpi_table_header **)&stao_ptr);
> +	if (ACPI_SUCCESS(status)) {
> +		if (stao_ptr->ignore_uart) {
> +			struct acpi_table_spcr *spcr_ptr;
> +
> +			acpi_get_table(ACPI_SIG_SPCR, 0,
> +				       (struct acpi_table_header **)&spcr_ptr);
> +			spcr_uart_addr = spcr_ptr->serial_port.address;
> +		}
> +	}
> +
>  	init_acpi_device_notify();
>  	result = acpi_bus_init();
>  	if (result) {
> -- 
> 2.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 01/16] Xen: ACPI: Hide UART used by Xen

[Stefano Stabellini]

On Mon, 18 Jan 2016, Mark Rutland wrote:
> On Fri, Jan 15, 2016 at 02:55:14PM +0800, Shannon Zhao wrote:
> > From: Shannon Zhao <shannon.zhao@linaro.org>
> > 
> > ACPI 6.0 introduces a new table STAO to list the devices which are used
> > by Xen and can't be used by Dom0. On Xen virtual platforms, the physical
> > UART is used by Xen. So here it hides UART from Dom0.
> > 
> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > ---
> > CC: "Rafael J. Wysocki" <rjw@rjwysocki.net> (supporter:ACPI)
> > CC: Len Brown <lenb@kernel.org> (supporter:ACPI)
> > CC: linux-acpi@vger.kernel.org (open list:ACPI)
> > ---
> >  drivers/acpi/bus.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index a212cef..d7a559f 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -46,6 +46,7 @@ ACPI_MODULE_NAME("bus");
> >  struct acpi_device *acpi_root;
> >  struct proc_dir_entry *acpi_root_dir;
> >  EXPORT_SYMBOL(acpi_root_dir);
> > +static u64 spcr_uart_addr;
> >  
> >  #ifdef CONFIG_X86
> >  #ifdef CONFIG_ACPI_CUSTOM_DSDT
> > @@ -93,6 +94,17 @@ acpi_status acpi_bus_get_status_handle(acpi_handle handle,
> >  {
> >  	acpi_status status;
> >  
> > +	if (spcr_uart_addr != 0xffffffffffffffff) {
> 
> The SPCR spec says that the Base Address fields being zero means that
> console redirection is disabled (though I'm not clear on whether or not
> that requires the whole acpi_generic_address to be zero).
> 
> Can we not use that here?

Shannon changed the code from 0 to 0xffffffffffffffff following my
suggestion to his previous series, but I didn't realize that the SPCR
spec says 0 means no console redirection. In that case using 0 is
better.

[PATCH v2 02/16] xen/grant-table: Move xlated_setup_gnttab_pages to common place

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Move xlated_setup_gnttab_pages to common place, so it can be reused by
ARM to setup grant table.

Rename it to xen_xlate_map_ballooned_pages.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 arch/x86/xen/grant-table.c | 57 +++++--------------------------------------
 drivers/xen/xlate_mmu.c    | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 include/xen/xen-ops.h      |  2 ++
 3 files changed, 69 insertions(+), 51 deletions(-)

diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
index e079500..de4144c 100644
--- a/arch/x86/xen/grant-table.c
+++ b/arch/x86/xen/grant-table.c
@@ -111,63 +111,18 @@ int arch_gnttab_init(unsigned long nr_shared)
 }
 
 #ifdef CONFIG_XEN_PVH
-#include <xen/balloon.h>
 #include <xen/events.h>
-#include <linux/slab.h>
-static int __init xlated_setup_gnttab_pages(void)
-{
-	struct page **pages;
-	xen_pfn_t *pfns;
-	void *vaddr;
-	int rc;
-	unsigned int i;
-	unsigned long nr_grant_frames = gnttab_max_grant_frames();
-
-	BUG_ON(nr_grant_frames == 0);
-	pages = kcalloc(nr_grant_frames, sizeof(pages[0]), GFP_KERNEL);
-	if (!pages)
-		return -ENOMEM;
-
-	pfns = kcalloc(nr_grant_frames, sizeof(pfns[0]), GFP_KERNEL);
-	if (!pfns) {
-		kfree(pages);
-		return -ENOMEM;
-	}
-	rc = alloc_xenballooned_pages(nr_grant_frames, pages);
-	if (rc) {
-		pr_warn("%s Couldn't balloon alloc %ld pfns rc:%d\n", __func__,
-			nr_grant_frames, rc);
-		kfree(pages);
-		kfree(pfns);
-		return rc;
-	}
-	for (i = 0; i < nr_grant_frames; i++)
-		pfns[i] = page_to_pfn(pages[i]);
-
-	vaddr = vmap(pages, nr_grant_frames, 0, PAGE_KERNEL);
-	if (!vaddr) {
-		pr_warn("%s Couldn't map %ld pfns rc:%d\n", __func__,
-			nr_grant_frames, rc);
-		free_xenballooned_pages(nr_grant_frames, pages);
-		kfree(pages);
-		kfree(pfns);
-		return -ENOMEM;
-	}
-	kfree(pages);
-
-	xen_auto_xlat_grant_frames.pfn = pfns;
-	xen_auto_xlat_grant_frames.count = nr_grant_frames;
-	xen_auto_xlat_grant_frames.vaddr = vaddr;
-
-	return 0;
-}
-
+#include <xen/xen-ops.h>
 static int __init xen_pvh_gnttab_setup(void)
 {
 	if (!xen_pvh_domain())
 		return -ENODEV;
 
-	return xlated_setup_gnttab_pages();
+	xen_auto_xlat_grant_frames.count = gnttab_max_grant_frames();
+
+	return xen_xlate_map_ballooned_pages(&xen_auto_xlat_grant_frames.pfn,
+					     &xen_auto_xlat_grant_frames.vaddr,
+					     xen_auto_xlat_grant_frames.count);
 }
 /* Call it _before_ __gnttab_init as we need to initialize the
  * xen_auto_xlat_grant_frames first. */
diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
index 5063c5e..9692656 100644
--- a/drivers/xen/xlate_mmu.c
+++ b/drivers/xen/xlate_mmu.c
@@ -29,6 +29,8 @@
  */
 #include <linux/kernel.h>
 #include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
 
 #include <asm/xen/hypercall.h>
 #include <asm/xen/hypervisor.h>
@@ -37,6 +39,7 @@
 #include <xen/page.h>
 #include <xen/interface/xen.h>
 #include <xen/interface/memory.h>
+#include <xen/balloon.h>
 
 typedef void (*xen_gfn_fn_t)(unsigned long gfn, void *data);
 
@@ -185,3 +188,61 @@ int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(xen_xlate_unmap_gfn_range);
+
+/**
+ * xen_xlate_map_ballooned_pages - map a new set of ballooned pages
+ * @gfns: returns the array of corresponding GFNs
+ * @virt: returns the virtual address of the mapped region
+ * @nr_grant_frames: number of GFNs
+ * @return 0 on success, error otherwise
+ *
+ * This allocates a set of ballooned pages and maps them into the
+ * kernel's address space.
+ */
+int __init xen_xlate_map_ballooned_pages(xen_pfn_t **gfns, void **virt,
+					 unsigned long nr_grant_frames)
+{
+	struct page **pages;
+	xen_pfn_t *pfns;
+	void *vaddr;
+	int rc;
+	unsigned int i;
+
+	BUG_ON(nr_grant_frames == 0);
+	pages = kcalloc(nr_grant_frames, sizeof(pages[0]), GFP_KERNEL);
+	if (!pages)
+		return -ENOMEM;
+
+	pfns = kcalloc(nr_grant_frames, sizeof(pfns[0]), GFP_KERNEL);
+	if (!pfns) {
+		kfree(pages);
+		return -ENOMEM;
+	}
+	rc = alloc_xenballooned_pages(nr_grant_frames, pages);
+	if (rc) {
+		pr_warn("%s Couldn't balloon alloc %ld pfns rc:%d\n", __func__,
+			nr_grant_frames, rc);
+		kfree(pages);
+		kfree(pfns);
+		return rc;
+	}
+	for (i = 0; i < nr_grant_frames; i++)
+		pfns[i] = page_to_pfn(pages[i]);
+
+	vaddr = vmap(pages, nr_grant_frames, 0, PAGE_KERNEL);
+	if (!vaddr) {
+		pr_warn("%s Couldn't map %ld pfns rc:%d\n", __func__,
+			nr_grant_frames, rc);
+		free_xenballooned_pages(nr_grant_frames, pages);
+		kfree(pages);
+		kfree(pfns);
+		return -ENOMEM;
+	}
+	kfree(pages);
+
+	*gfns = pfns;
+	*virt = vaddr;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xen_xlate_map_ballooned_pages);
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index e4e214a..42be31a 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -80,6 +80,8 @@ int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
 			      struct page **pages);
 int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
 			      int nr, struct page **pages);
+int xen_xlate_map_ballooned_pages(xen_pfn_t **pfns, void **vaddr,
+				  unsigned long nr_grant_frames);
 
 bool xen_running_on_version_or_later(unsigned int major, unsigned int minor);
 
-- 
2.0.4

Re: [Xen-devel] [PATCH v2 02/16] xen/grant-table: Move xlated_setup_gnttab_pages to common place

[Stefano Stabellini]

On Fri, 15 Jan 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Move xlated_setup_gnttab_pages to common place, so it can be reused by
> ARM to setup grant table.
> 
> Rename it to xen_xlate_map_ballooned_pages.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  arch/x86/xen/grant-table.c | 57 +++++--------------------------------------
>  drivers/xen/xlate_mmu.c    | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/xen/xen-ops.h      |  2 ++
>  3 files changed, 69 insertions(+), 51 deletions(-)
> 
> diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
> index e079500..de4144c 100644
> --- a/arch/x86/xen/grant-table.c
> +++ b/arch/x86/xen/grant-table.c
> @@ -111,63 +111,18 @@ int arch_gnttab_init(unsigned long nr_shared)
>  }
>  
>  #ifdef CONFIG_XEN_PVH
> -#include <xen/balloon.h>
>  #include <xen/events.h>
> -#include <linux/slab.h>
> -static int __init xlated_setup_gnttab_pages(void)
> -{
> -	struct page **pages;
> -	xen_pfn_t *pfns;
> -	void *vaddr;
> -	int rc;
> -	unsigned int i;
> -	unsigned long nr_grant_frames = gnttab_max_grant_frames();
> -
> -	BUG_ON(nr_grant_frames == 0);
> -	pages = kcalloc(nr_grant_frames, sizeof(pages[0]), GFP_KERNEL);
> -	if (!pages)
> -		return -ENOMEM;
> -
> -	pfns = kcalloc(nr_grant_frames, sizeof(pfns[0]), GFP_KERNEL);
> -	if (!pfns) {
> -		kfree(pages);
> -		return -ENOMEM;
> -	}
> -	rc = alloc_xenballooned_pages(nr_grant_frames, pages);
> -	if (rc) {
> -		pr_warn("%s Couldn't balloon alloc %ld pfns rc:%d\n", __func__,
> -			nr_grant_frames, rc);
> -		kfree(pages);
> -		kfree(pfns);
> -		return rc;
> -	}
> -	for (i = 0; i < nr_grant_frames; i++)
> -		pfns[i] = page_to_pfn(pages[i]);
> -
> -	vaddr = vmap(pages, nr_grant_frames, 0, PAGE_KERNEL);
> -	if (!vaddr) {
> -		pr_warn("%s Couldn't map %ld pfns rc:%d\n", __func__,
> -			nr_grant_frames, rc);
> -		free_xenballooned_pages(nr_grant_frames, pages);
> -		kfree(pages);
> -		kfree(pfns);
> -		return -ENOMEM;
> -	}
> -	kfree(pages);
> -
> -	xen_auto_xlat_grant_frames.pfn = pfns;
> -	xen_auto_xlat_grant_frames.count = nr_grant_frames;
> -	xen_auto_xlat_grant_frames.vaddr = vaddr;
> -
> -	return 0;
> -}
> -
> +#include <xen/xen-ops.h>
>  static int __init xen_pvh_gnttab_setup(void)
>  {
>  	if (!xen_pvh_domain())
>  		return -ENODEV;
>  
> -	return xlated_setup_gnttab_pages();
> +	xen_auto_xlat_grant_frames.count = gnttab_max_grant_frames();
> +
> +	return xen_xlate_map_ballooned_pages(&xen_auto_xlat_grant_frames.pfn,
> +					     &xen_auto_xlat_grant_frames.vaddr,
> +					     xen_auto_xlat_grant_frames.count);
>  }
>  /* Call it _before_ __gnttab_init as we need to initialize the
>   * xen_auto_xlat_grant_frames first. */
> diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
> index 5063c5e..9692656 100644
> --- a/drivers/xen/xlate_mmu.c
> +++ b/drivers/xen/xlate_mmu.c
> @@ -29,6 +29,8 @@
>   */
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
>  
>  #include <asm/xen/hypercall.h>
>  #include <asm/xen/hypervisor.h>
> @@ -37,6 +39,7 @@
>  #include <xen/page.h>
>  #include <xen/interface/xen.h>
>  #include <xen/interface/memory.h>
> +#include <xen/balloon.h>
>  
>  typedef void (*xen_gfn_fn_t)(unsigned long gfn, void *data);
>  
> @@ -185,3 +188,61 @@ int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(xen_xlate_unmap_gfn_range);
> +
> +/**
> + * xen_xlate_map_ballooned_pages - map a new set of ballooned pages
> + * @gfns: returns the array of corresponding GFNs
> + * @virt: returns the virtual address of the mapped region
> + * @nr_grant_frames: number of GFNs
> + * @return 0 on success, error otherwise
> + *
> + * This allocates a set of ballooned pages and maps them into the
> + * kernel's address space.
> + */
> +int __init xen_xlate_map_ballooned_pages(xen_pfn_t **gfns, void **virt,
> +					 unsigned long nr_grant_frames)
> +{
> +	struct page **pages;
> +	xen_pfn_t *pfns;
> +	void *vaddr;
> +	int rc;
> +	unsigned int i;
> +
> +	BUG_ON(nr_grant_frames == 0);
> +	pages = kcalloc(nr_grant_frames, sizeof(pages[0]), GFP_KERNEL);
> +	if (!pages)
> +		return -ENOMEM;
> +
> +	pfns = kcalloc(nr_grant_frames, sizeof(pfns[0]), GFP_KERNEL);
> +	if (!pfns) {
> +		kfree(pages);
> +		return -ENOMEM;
> +	}
> +	rc = alloc_xenballooned_pages(nr_grant_frames, pages);
> +	if (rc) {
> +		pr_warn("%s Couldn't balloon alloc %ld pfns rc:%d\n", __func__,
> +			nr_grant_frames, rc);
> +		kfree(pages);
> +		kfree(pfns);
> +		return rc;
> +	}
> +	for (i = 0; i < nr_grant_frames; i++)
> +		pfns[i] = page_to_pfn(pages[i]);
> +
> +	vaddr = vmap(pages, nr_grant_frames, 0, PAGE_KERNEL);
> +	if (!vaddr) {
> +		pr_warn("%s Couldn't map %ld pfns rc:%d\n", __func__,
> +			nr_grant_frames, rc);
> +		free_xenballooned_pages(nr_grant_frames, pages);
> +		kfree(pages);
> +		kfree(pfns);
> +		return -ENOMEM;
> +	}
> +	kfree(pages);
> +
> +	*gfns = pfns;
> +	*virt = vaddr;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(xen_xlate_map_ballooned_pages);
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index e4e214a..42be31a 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -80,6 +80,8 @@ int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
>  			      struct page **pages);
>  int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
>  			      int nr, struct page **pages);
> +int xen_xlate_map_ballooned_pages(xen_pfn_t **pfns, void **vaddr,
> +				  unsigned long nr_grant_frames);
>  
>  bool xen_running_on_version_or_later(unsigned int major, unsigned int minor);
>  
> -- 
> 2.0.4
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

[PATCH v2 03/16] Xen: xlate: Use page_to_xen_pfn instead of page_to_pfn

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Use page_to_xen_pfn in case of 64KB page.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 drivers/xen/xlate_mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
index 9692656..b9fcc2c 100644
--- a/drivers/xen/xlate_mmu.c
+++ b/drivers/xen/xlate_mmu.c
@@ -227,7 +227,7 @@ int __init xen_xlate_map_ballooned_pages(xen_pfn_t **gfns, void **virt,
 		return rc;
 	}
 	for (i = 0; i < nr_grant_frames; i++)
-		pfns[i] = page_to_pfn(pages[i]);
+		pfns[i] = page_to_xen_pfn(pages[i]);
 
 	vaddr = vmap(pages, nr_grant_frames, 0, PAGE_KERNEL);
 	if (!vaddr) {
-- 
2.0.4

Re: [Xen-devel] [PATCH v2 03/16] Xen: xlate: Use page_to_xen_pfn instead of page_to_pfn

[Stefano Stabellini]

On Fri, 15 Jan 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Use page_to_xen_pfn in case of 64KB page.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  drivers/xen/xlate_mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
> index 9692656..b9fcc2c 100644
> --- a/drivers/xen/xlate_mmu.c
> +++ b/drivers/xen/xlate_mmu.c
> @@ -227,7 +227,7 @@ int __init xen_xlate_map_ballooned_pages(xen_pfn_t **gfns, void **virt,
>  		return rc;
>  	}
>  	for (i = 0; i < nr_grant_frames; i++)
> -		pfns[i] = page_to_pfn(pages[i]);
> +		pfns[i] = page_to_xen_pfn(pages[i]);

Shannon, thanks for the patch.

Keeping in mind that in the 64K case, kernel pages are 64K but xen pages
are still 4K, I think you also need to allocate
(nr_grant_frames/XEN_PFN_PER_PAGE) kernel pages (assuming that they are
allocated contiguously): nr_grant_frames refers to 4K pages, while
xen_xlate_map_ballooned_pages is allocating pages on a 64K granularity
(sizeof(pages[0]) == 64K).

Be careful that alloc_xenballooned_pages deals with 64K pages (because
it deals with kernel pages), while xen_pfn_t is always 4K based (because
it deals with Xen pfns).

Please test it with and without CONFIG_ARM64_64K_PAGES. Thanks!



>  	vaddr = vmap(pages, nr_grant_frames, 0, PAGE_KERNEL);
>  	if (!vaddr) {
> -- 
> 2.0.4
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [Xen-devel] [PATCH v2 03/16] Xen: xlate: Use page_to_xen_pfn instead of page_to_pfn

[Shannon Zhao]

On 2016/1/16 1:08, Stefano Stabellini wrote:
> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Use page_to_xen_pfn in case of 64KB page.
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  drivers/xen/xlate_mmu.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
>> index 9692656..b9fcc2c 100644
>> --- a/drivers/xen/xlate_mmu.c
>> +++ b/drivers/xen/xlate_mmu.c
>> @@ -227,7 +227,7 @@ int __init xen_xlate_map_ballooned_pages(xen_pfn_t **gfns, void **virt,
>>  		return rc;
>>  	}
>>  	for (i = 0; i < nr_grant_frames; i++)
>> -		pfns[i] = page_to_pfn(pages[i]);
>> +		pfns[i] = page_to_xen_pfn(pages[i]);
> 
> Shannon, thanks for the patch.
> 
> Keeping in mind that in the 64K case, kernel pages are 64K but xen pages
> are still 4K, I think you also need to allocate
> (nr_grant_frames/XEN_PFN_PER_PAGE) kernel pages (assuming that they are
> allocated contiguously): nr_grant_frames refers to 4K pages, while
> xen_xlate_map_ballooned_pages is allocating pages on a 64K granularity
> (sizeof(pages[0]) == 64K).
> 
> Be careful that alloc_xenballooned_pages deals with 64K pages (because
> it deals with kernel pages), while xen_pfn_t is always 4K based (because
> it deals with Xen pfns).
> 
> Please test it with and without CONFIG_ARM64_64K_PAGES. Thanks!
> 
Stefano, thanks for your explanation. How about below patch?

diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
index 9692656..e1f7c95 100644
--- a/drivers/xen/xlate_mmu.c
+++ b/drivers/xen/xlate_mmu.c
@@ -207,9 +207,12 @@ int __init xen_xlate_map_ballooned_pages(xen_pfn_t
**gfns, void **virt,
        void *vaddr;
        int rc;
        unsigned int i;
+       unsigned long nr_pages;
+       xen_pfn_t xen_pfn = 0;

        BUG_ON(nr_grant_frames == 0);
-       pages = kcalloc(nr_grant_frames, sizeof(pages[0]), GFP_KERNEL);
+       nr_pages = DIV_ROUND_UP(nr_grant_frames, XEN_PFN_PER_PAGE);
+       pages = kcalloc(nr_pages, sizeof(pages[0]), GFP_KERNEL);
        if (!pages)
                return -ENOMEM;

@@ -218,22 +221,25 @@ int __init xen_xlate_map_ballooned_pages(xen_pfn_t
**gfns, void **virt,
                kfree(pages);
                return -ENOMEM;
        }
-       rc = alloc_xenballooned_pages(nr_grant_frames, pages);
+       rc = alloc_xenballooned_pages(nr_pages, pages);
        if (rc) {
-               pr_warn("%s Couldn't balloon alloc %ld pfns rc:%d\n",
__func__,
-                       nr_grant_frames, rc);
+               pr_warn("%s Couldn't balloon alloc %ld pages rc:%d\n",
__func__,
+                       nr_pages, rc);
                kfree(pages);
                kfree(pfns);
                return rc;
        }
-       for (i = 0; i < nr_grant_frames; i++)
-               pfns[i] = page_to_pfn(pages[i]);
+       for (i = 0; i < nr_grant_frames; i++) {
+               if ((i % XEN_PFN_PER_PAGE) == 0)
+                       xen_pfn = page_to_xen_pfn(pages[i /
XEN_PFN_PER_PAGE]);
+               pfns[i] = xen_pfn++;
+       }

-       vaddr = vmap(pages, nr_grant_frames, 0, PAGE_KERNEL);
+       vaddr = vmap(pages, nr_pages, 0, PAGE_KERNEL);
        if (!vaddr) {
-               pr_warn("%s Couldn't map %ld pfns rc:%d\n", __func__,
-                       nr_grant_frames, rc);
-               free_xenballooned_pages(nr_grant_frames, pages);
+               pr_warn("%s Couldn't map %ld pages rc:%d\n", __func__,
+                       nr_pages, rc);
+               free_xenballooned_pages(nr_pages, pages);
                kfree(pages);
                kfree(pfns);
                return -ENOMEM;

-- 
Shannon

Re: [Xen-devel] [PATCH v2 03/16] Xen: xlate: Use page_to_xen_pfn instead of page_to_pfn

[Stefano Stabellini]

On Mon, 18 Jan 2016, Shannon Zhao wrote:
> On 2016/1/16 1:08, Stefano Stabellini wrote:
> > On Fri, 15 Jan 2016, Shannon Zhao wrote:
> >> From: Shannon Zhao <shannon.zhao@linaro.org>
> >>
> >> Use page_to_xen_pfn in case of 64KB page.
> >>
> >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >> ---
> >>  drivers/xen/xlate_mmu.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
> >> index 9692656..b9fcc2c 100644
> >> --- a/drivers/xen/xlate_mmu.c
> >> +++ b/drivers/xen/xlate_mmu.c
> >> @@ -227,7 +227,7 @@ int __init xen_xlate_map_ballooned_pages(xen_pfn_t **gfns, void **virt,
> >>  		return rc;
> >>  	}
> >>  	for (i = 0; i < nr_grant_frames; i++)
> >> -		pfns[i] = page_to_pfn(pages[i]);
> >> +		pfns[i] = page_to_xen_pfn(pages[i]);
> > 
> > Shannon, thanks for the patch.
> > 
> > Keeping in mind that in the 64K case, kernel pages are 64K but xen pages
> > are still 4K, I think you also need to allocate
> > (nr_grant_frames/XEN_PFN_PER_PAGE) kernel pages (assuming that they are
> > allocated contiguously): nr_grant_frames refers to 4K pages, while
> > xen_xlate_map_ballooned_pages is allocating pages on a 64K granularity
> > (sizeof(pages[0]) == 64K).
> > 
> > Be careful that alloc_xenballooned_pages deals with 64K pages (because
> > it deals with kernel pages), while xen_pfn_t is always 4K based (because
> > it deals with Xen pfns).
> > 
> > Please test it with and without CONFIG_ARM64_64K_PAGES. Thanks!
> > 
> Stefano, thanks for your explanation. How about below patch?

Good work, it looks like you covered all bases, I think it should work,
but I haven't tested it myself. Just one note below.


> diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
> index 9692656..e1f7c95 100644
> --- a/drivers/xen/xlate_mmu.c
> +++ b/drivers/xen/xlate_mmu.c
> @@ -207,9 +207,12 @@ int __init xen_xlate_map_ballooned_pages(xen_pfn_t
> **gfns, void **virt,
>         void *vaddr;
>         int rc;
>         unsigned int i;
> +       unsigned long nr_pages;
> +       xen_pfn_t xen_pfn = 0;
> 
>         BUG_ON(nr_grant_frames == 0);
> -       pages = kcalloc(nr_grant_frames, sizeof(pages[0]), GFP_KERNEL);
> +       nr_pages = DIV_ROUND_UP(nr_grant_frames, XEN_PFN_PER_PAGE);
> +       pages = kcalloc(nr_pages, sizeof(pages[0]), GFP_KERNEL);
>         if (!pages)
>                 return -ENOMEM;
> 
> @@ -218,22 +221,25 @@ int __init xen_xlate_map_ballooned_pages(xen_pfn_t
> **gfns, void **virt,
>                 kfree(pages);
>                 return -ENOMEM;
>         }
> -       rc = alloc_xenballooned_pages(nr_grant_frames, pages);
> +       rc = alloc_xenballooned_pages(nr_pages, pages);
>         if (rc) {
> -               pr_warn("%s Couldn't balloon alloc %ld pfns rc:%d\n",
> __func__,
> -                       nr_grant_frames, rc);
> +               pr_warn("%s Couldn't balloon alloc %ld pages rc:%d\n",
> __func__,
> +                       nr_pages, rc);
>                 kfree(pages);
>                 kfree(pfns);
>                 return rc;
>         }
> -       for (i = 0; i < nr_grant_frames; i++)
> -               pfns[i] = page_to_pfn(pages[i]);
> +       for (i = 0; i < nr_grant_frames; i++) {
> +               if ((i % XEN_PFN_PER_PAGE) == 0)
> +                       xen_pfn = page_to_xen_pfn(pages[i /
> XEN_PFN_PER_PAGE]);
> +               pfns[i] = xen_pfn++;
> +       }

We might want to:

  pfns[i] = pfn_to_gfn(xen_pfn++);

for consistency, even though for autotranslate guests pfn_to_gfn always
returns pfn.


> -       vaddr = vmap(pages, nr_grant_frames, 0, PAGE_KERNEL);
> +       vaddr = vmap(pages, nr_pages, 0, PAGE_KERNEL);
>         if (!vaddr) {
> -               pr_warn("%s Couldn't map %ld pfns rc:%d\n", __func__,
> -                       nr_grant_frames, rc);
> -               free_xenballooned_pages(nr_grant_frames, pages);
> +               pr_warn("%s Couldn't map %ld pages rc:%d\n", __func__,
> +                       nr_pages, rc);
> +               free_xenballooned_pages(nr_pages, pages);
>                 kfree(pages);
>                 kfree(pfns);
>                 return -ENOMEM;
> 
> -- 
> Shannon
> 

Re: [Xen-devel] [PATCH v2 03/16] Xen: xlate: Use page_to_xen_pfn instead of page_to_pfn

[Shannon Zhao]

On 2016/1/19 22:59, Stefano Stabellini wrote:
> On Mon, 18 Jan 2016, Shannon Zhao wrote:
>> On 2016/1/16 1:08, Stefano Stabellini wrote:
>>> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>
>>>> Use page_to_xen_pfn in case of 64KB page.
>>>>
>>>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>>> ---
>>>>   drivers/xen/xlate_mmu.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
>>>> index 9692656..b9fcc2c 100644
>>>> --- a/drivers/xen/xlate_mmu.c
>>>> +++ b/drivers/xen/xlate_mmu.c
>>>> @@ -227,7 +227,7 @@ int __init xen_xlate_map_ballooned_pages(xen_pfn_t **gfns, void **virt,
>>>>   		return rc;
>>>>   	}
>>>>   	for (i = 0; i < nr_grant_frames; i++)
>>>> -		pfns[i] = page_to_pfn(pages[i]);
>>>> +		pfns[i] = page_to_xen_pfn(pages[i]);
>>>
>>> Shannon, thanks for the patch.
>>>
>>> Keeping in mind that in the 64K case, kernel pages are 64K but xen pages
>>> are still 4K, I think you also need to allocate
>>> (nr_grant_frames/XEN_PFN_PER_PAGE) kernel pages (assuming that they are
>>> allocated contiguously): nr_grant_frames refers to 4K pages, while
>>> xen_xlate_map_ballooned_pages is allocating pages on a 64K granularity
>>> (sizeof(pages[0]) == 64K).
>>>
>>> Be careful that alloc_xenballooned_pages deals with 64K pages (because
>>> it deals with kernel pages), while xen_pfn_t is always 4K based (because
>>> it deals with Xen pfns).
>>>
>>> Please test it with and without CONFIG_ARM64_64K_PAGES. Thanks!
>>>
>> Stefano, thanks for your explanation. How about below patch?
>
> Good work, it looks like you covered all bases, I think it should work,
> but I haven't tested it myself. Just one note below.
>
>
>> diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
>> index 9692656..e1f7c95 100644
>> --- a/drivers/xen/xlate_mmu.c
>> +++ b/drivers/xen/xlate_mmu.c
>> @@ -207,9 +207,12 @@ int __init xen_xlate_map_ballooned_pages(xen_pfn_t
>> **gfns, void **virt,
>>          void *vaddr;
>>          int rc;
>>          unsigned int i;
>> +       unsigned long nr_pages;
>> +       xen_pfn_t xen_pfn = 0;
>>
>>          BUG_ON(nr_grant_frames == 0);
>> -       pages = kcalloc(nr_grant_frames, sizeof(pages[0]), GFP_KERNEL);
>> +       nr_pages = DIV_ROUND_UP(nr_grant_frames, XEN_PFN_PER_PAGE);
>> +       pages = kcalloc(nr_pages, sizeof(pages[0]), GFP_KERNEL);
>>          if (!pages)
>>                  return -ENOMEM;
>>
>> @@ -218,22 +221,25 @@ int __init xen_xlate_map_ballooned_pages(xen_pfn_t
>> **gfns, void **virt,
>>                  kfree(pages);
>>                  return -ENOMEM;
>>          }
>> -       rc = alloc_xenballooned_pages(nr_grant_frames, pages);
>> +       rc = alloc_xenballooned_pages(nr_pages, pages);
>>          if (rc) {
>> -               pr_warn("%s Couldn't balloon alloc %ld pfns rc:%d\n",
>> __func__,
>> -                       nr_grant_frames, rc);
>> +               pr_warn("%s Couldn't balloon alloc %ld pages rc:%d\n",
>> __func__,
>> +                       nr_pages, rc);
>>                  kfree(pages);
>>                  kfree(pfns);
>>                  return rc;
>>          }
>> -       for (i = 0; i < nr_grant_frames; i++)
>> -               pfns[i] = page_to_pfn(pages[i]);
>> +       for (i = 0; i < nr_grant_frames; i++) {
>> +               if ((i % XEN_PFN_PER_PAGE) == 0)
>> +                       xen_pfn = page_to_xen_pfn(pages[i /
>> XEN_PFN_PER_PAGE]);
>> +               pfns[i] = xen_pfn++;
>> +       }
>
> We might want to:
>
>    pfns[i] = pfn_to_gfn(xen_pfn++);
>
> for consistency, even though for autotranslate guests pfn_to_gfn always
> returns pfn.
>
Ok, will add. Thanks.

>
>> -       vaddr = vmap(pages, nr_grant_frames, 0, PAGE_KERNEL);
>> +       vaddr = vmap(pages, nr_pages, 0, PAGE_KERNEL);
>>          if (!vaddr) {
>> -               pr_warn("%s Couldn't map %ld pfns rc:%d\n", __func__,
>> -                       nr_grant_frames, rc);
>> -               free_xenballooned_pages(nr_grant_frames, pages);
>> +               pr_warn("%s Couldn't map %ld pages rc:%d\n", __func__,
>> +                       nr_pages, rc);
>> +               free_xenballooned_pages(nr_pages, pages);
>>                  kfree(pages);
>>                  kfree(pfns);
>>                  return -ENOMEM;
>>
>> --
>> Shannon
>>

-- 
Shannon

[PATCH v2 04/16] arm/xen: Use xen_xlate_map_ballooned_pages to setup grant table

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Use xen_xlate_map_ballooned_pages to setup grant table. Then it doesn't
rely on DT or ACPI to pass the start address and size of grant table.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 arch/arm/xen/enlighten.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index afe6175..6d90a62 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -266,18 +266,10 @@ static int __init xen_guest_init(void)
 {
 	struct xen_add_to_physmap xatp;
 	struct shared_info *shared_info_page = NULL;
-	struct resource res;
-	phys_addr_t grant_frames;
 
 	if (!xen_domain())
 		return 0;
 
-	if (of_address_to_resource(xen_node, GRANT_TABLE_PHYSADDR, &res)) {
-		pr_err("Xen grant table base address not found\n");
-		return -ENODEV;
-	}
-	grant_frames = res.start;
-
 	xen_events_irq = irq_of_parse_and_map(xen_node, 0);
 	if (!xen_events_irq) {
 		pr_err("Xen event channel interrupt not found\n");
@@ -312,7 +304,10 @@ static int __init xen_guest_init(void)
 	if (xen_vcpu_info == NULL)
 		return -ENOMEM;
 
-	if (gnttab_setup_auto_xlat_frames(grant_frames)) {
+	xen_auto_xlat_grant_frames.count = gnttab_max_grant_frames();
+	if (xen_xlate_map_ballooned_pages(&xen_auto_xlat_grant_frames.pfn,
+					  &xen_auto_xlat_grant_frames.vaddr,
+					  xen_auto_xlat_grant_frames.count)) {
 		free_percpu(xen_vcpu_info);
 		return -ENOMEM;
 	}
-- 
2.0.4

Re: [PATCH v2 04/16] arm/xen: Use xen_xlate_map_ballooned_pages to setup grant table

[Stefano Stabellini]

On Fri, 15 Jan 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Use xen_xlate_map_ballooned_pages to setup grant table. Then it doesn't
> rely on DT or ACPI to pass the start address and size of grant table.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  arch/arm/xen/enlighten.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index afe6175..6d90a62 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -266,18 +266,10 @@ static int __init xen_guest_init(void)
>  {
>  	struct xen_add_to_physmap xatp;
>  	struct shared_info *shared_info_page = NULL;
> -	struct resource res;
> -	phys_addr_t grant_frames;
>  
>  	if (!xen_domain())
>  		return 0;
>  
> -	if (of_address_to_resource(xen_node, GRANT_TABLE_PHYSADDR, &res)) {
> -		pr_err("Xen grant table base address not found\n");
> -		return -ENODEV;
> -	}
> -	grant_frames = res.start;
> -
>  	xen_events_irq = irq_of_parse_and_map(xen_node, 0);
>  	if (!xen_events_irq) {
>  		pr_err("Xen event channel interrupt not found\n");
> @@ -312,7 +304,10 @@ static int __init xen_guest_init(void)
>  	if (xen_vcpu_info == NULL)
>  		return -ENOMEM;
>  
> -	if (gnttab_setup_auto_xlat_frames(grant_frames)) {
> +	xen_auto_xlat_grant_frames.count = gnttab_max_grant_frames();
> +	if (xen_xlate_map_ballooned_pages(&xen_auto_xlat_grant_frames.pfn,
> +					  &xen_auto_xlat_grant_frames.vaddr,
> +					  xen_auto_xlat_grant_frames.count)) {
>  		free_percpu(xen_vcpu_info);
>  		return -ENOMEM;
>  	}
> -- 
> 2.0.4
> 
> 

[PATCH v2 05/16] xen: memory : Add new XENMAPSPACE type XENMAPSPACE_dev_mmio

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Add a new type of Xen map space for Dom0 to map device's MMIO region.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 include/xen/interface/memory.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
index 2ecfe4f..9aa8988 100644
--- a/include/xen/interface/memory.h
+++ b/include/xen/interface/memory.h
@@ -160,6 +160,7 @@ DEFINE_GUEST_HANDLE_STRUCT(xen_machphys_mapping_t);
 #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom,
 				    * XENMEM_add_to_physmap_range only.
 				    */
+#define XENMAPSPACE_dev_mmio     5 /* device mmio region */
 
 /*
  * Sets the GPFN at which a particular page appears in the specified guest's
-- 
2.0.4

[PATCH v2 06/16] Xen: ARM: Add support for mapping platform device mmio

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Add a bus_notifier for platform bus device in order to map the device
mmio regions when DOM0 booting with ACPI.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 drivers/xen/Makefile     |   1 +
 drivers/xen/arm-device.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 141 insertions(+)
 create mode 100644 drivers/xen/arm-device.c

diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index aa8a7f7..0c087b1 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -9,6 +9,7 @@ CFLAGS_features.o			:= $(nostackp)
 
 CFLAGS_efi.o				+= -fshort-wchar
 
+dom0-$(CONFIG_ARM64) += arm-device.o
 dom0-$(CONFIG_PCI) += pci.o
 dom0-$(CONFIG_USB_SUPPORT) += dbgp.o
 dom0-$(CONFIG_XEN_ACPI) += acpi.o $(xen-pad-y)
diff --git a/drivers/xen/arm-device.c b/drivers/xen/arm-device.c
new file mode 100644
index 0000000..7d902ea
--- /dev/null
+++ b/drivers/xen/arm-device.c
@@ -0,0 +1,140 @@
+/*
+ * Copyright (c) 2015, Linaro Limited, Shannon Zhao
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/acpi.h>
+#include <xen/xen.h>
+#include <xen/page.h>
+#include <xen/interface/memory.h>
+#include <asm/xen/hypervisor.h>
+#include <asm/xen/hypercall.h>
+
+static int xen_unmap_device_mmio(struct resource *resource, unsigned int count)
+{
+	unsigned int i, j, nr;
+	int rc = 0;
+	struct resource *r;
+	struct xen_remove_from_physmap xrp;
+
+	for (i = 0; i < count; i++) {
+		r = &resource[i];
+		nr = DIV_ROUND_UP(resource_size(r), XEN_PAGE_SIZE);
+		if ((resource_type(r) != IORESOURCE_MEM) || (nr == 0))
+			continue;
+
+		for (j = 0; j < nr; j++) {
+			xrp.domid = DOMID_SELF;
+			xrp.gpfn = XEN_PFN_DOWN(r->start) + j;
+			rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap,
+						  &xrp);
+			if (rc)
+				return rc;
+		}
+	}
+
+	return rc;
+}
+
+static int xen_map_device_mmio(struct resource *resource, unsigned int count)
+{
+	unsigned int i, j, nr;
+	int rc = 0;
+	struct resource *r;
+	xen_pfn_t *gpfns;
+	xen_ulong_t *idxs;
+	int *errs;
+	struct xen_add_to_physmap_range xatp;
+
+	for (i = 0; i < count; i++) {
+		r = &resource[i];
+		nr = DIV_ROUND_UP(resource_size(r), XEN_PAGE_SIZE);
+		if ((resource_type(r) != IORESOURCE_MEM) || (nr == 0))
+			continue;
+
+		gpfns = kzalloc(sizeof(xen_pfn_t) * nr, GFP_KERNEL);
+		idxs = kzalloc(sizeof(xen_ulong_t) * nr, GFP_KERNEL);
+		errs = kzalloc(sizeof(int) * nr, GFP_KERNEL);
+		if (!gpfns || !idxs || !errs) {
+			kfree(gpfns);
+			kfree(idxs);
+			return -ENOMEM;
+		}
+
+		for (j = 0; j < nr; j++) {
+			gpfns[j] = XEN_PFN_DOWN(r->start) + j;
+			idxs[j] = XEN_PFN_DOWN(r->start) + j;
+		}
+
+		xatp.domid = DOMID_SELF;
+		xatp.size = nr;
+		xatp.space = XENMAPSPACE_dev_mmio;
+
+		set_xen_guest_handle(xatp.gpfns, gpfns);
+		set_xen_guest_handle(xatp.idxs, idxs);
+		set_xen_guest_handle(xatp.errs, errs);
+
+		rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
+		kfree(gpfns);
+		kfree(idxs);
+		kfree(errs);
+		if (rc)
+			return rc;
+	}
+
+	return rc;
+}
+
+static int xen_platform_notifier(struct notifier_block *nb,
+				 unsigned long action, void *data)
+{
+	struct platform_device *pdev = to_platform_device(data);
+	int r = 0;
+
+	if (pdev->num_resources == 0 || pdev->resource == NULL)
+		return NOTIFY_OK;
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		r = xen_map_device_mmio(pdev->resource, pdev->num_resources);
+		break;
+	case BUS_NOTIFY_DEL_DEVICE:
+		r = xen_unmap_device_mmio(pdev->resource, pdev->num_resources);
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+	if (r)
+		dev_err(&pdev->dev, "Platform: Failed to %s device %s MMIO!\n",
+			action == BUS_NOTIFY_ADD_DEVICE ? "map" :
+			(action == BUS_NOTIFY_DEL_DEVICE ? "unmap" : "?"),
+			pdev->name);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block platform_device_nb = {
+	.notifier_call = xen_platform_notifier,
+};
+
+static int __init register_xen_platform_notifier(void)
+{
+	if (!xen_initial_domain() || acpi_disabled)
+		return 0;
+
+	return bus_register_notifier(&platform_bus_type, &platform_device_nb);
+}
+
+arch_initcall(register_xen_platform_notifier);
-- 
2.0.4

Re: [Xen-devel] [PATCH v2 06/16] Xen: ARM: Add support for mapping platform device mmio

[Stefano Stabellini]

On Fri, 15 Jan 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Add a bus_notifier for platform bus device in order to map the device
> mmio regions when DOM0 booting with ACPI.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  drivers/xen/Makefile     |   1 +
>  drivers/xen/arm-device.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 141 insertions(+)
>  create mode 100644 drivers/xen/arm-device.c
> 
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index aa8a7f7..0c087b1 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -9,6 +9,7 @@ CFLAGS_features.o			:= $(nostackp)
>  
>  CFLAGS_efi.o				+= -fshort-wchar
>  
> +dom0-$(CONFIG_ARM64) += arm-device.o
>  dom0-$(CONFIG_PCI) += pci.o
>  dom0-$(CONFIG_USB_SUPPORT) += dbgp.o
>  dom0-$(CONFIG_XEN_ACPI) += acpi.o $(xen-pad-y)
> diff --git a/drivers/xen/arm-device.c b/drivers/xen/arm-device.c
> new file mode 100644
> index 0000000..7d902ea
> --- /dev/null
> +++ b/drivers/xen/arm-device.c
> @@ -0,0 +1,140 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited, Shannon Zhao
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/acpi.h>
> +#include <xen/xen.h>
> +#include <xen/page.h>
> +#include <xen/interface/memory.h>
> +#include <asm/xen/hypervisor.h>
> +#include <asm/xen/hypercall.h>
> +
> +static int xen_unmap_device_mmio(struct resource *resource, unsigned int count)
> +{
> +	unsigned int i, j, nr;
> +	int rc = 0;
> +	struct resource *r;
> +	struct xen_remove_from_physmap xrp;
> +
> +	for (i = 0; i < count; i++) {
> +		r = &resource[i];
> +		nr = DIV_ROUND_UP(resource_size(r), XEN_PAGE_SIZE);
> +		if ((resource_type(r) != IORESOURCE_MEM) || (nr == 0))
> +			continue;
> +
> +		for (j = 0; j < nr; j++) {
> +			xrp.domid = DOMID_SELF;
> +			xrp.gpfn = XEN_PFN_DOWN(r->start) + j;
> +			rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap,
> +						  &xrp);
> +			if (rc)
> +				return rc;
> +		}
> +	}
> +
> +	return rc;
> +}
> +
> +static int xen_map_device_mmio(struct resource *resource, unsigned int count)
> +{
> +	unsigned int i, j, nr;
> +	int rc = 0;
> +	struct resource *r;
> +	xen_pfn_t *gpfns;
> +	xen_ulong_t *idxs;
> +	int *errs;
> +	struct xen_add_to_physmap_range xatp;
> +
> +	for (i = 0; i < count; i++) {
> +		r = &resource[i];
> +		nr = DIV_ROUND_UP(resource_size(r), XEN_PAGE_SIZE);
> +		if ((resource_type(r) != IORESOURCE_MEM) || (nr == 0))
> +			continue;
> +
> +		gpfns = kzalloc(sizeof(xen_pfn_t) * nr, GFP_KERNEL);
> +		idxs = kzalloc(sizeof(xen_ulong_t) * nr, GFP_KERNEL);
> +		errs = kzalloc(sizeof(int) * nr, GFP_KERNEL);
> +		if (!gpfns || !idxs || !errs) {
> +			kfree(gpfns);
> +			kfree(idxs);

kfree(errs)?

Aside from this:

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


> +			return -ENOMEM;
> +		}
> +
> +		for (j = 0; j < nr; j++) {
> +			gpfns[j] = XEN_PFN_DOWN(r->start) + j;
> +			idxs[j] = XEN_PFN_DOWN(r->start) + j;
> +		}
> +
> +		xatp.domid = DOMID_SELF;
> +		xatp.size = nr;
> +		xatp.space = XENMAPSPACE_dev_mmio;
> +
> +		set_xen_guest_handle(xatp.gpfns, gpfns);
> +		set_xen_guest_handle(xatp.idxs, idxs);
> +		set_xen_guest_handle(xatp.errs, errs);
> +
> +		rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
> +		kfree(gpfns);
> +		kfree(idxs);
> +		kfree(errs);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return rc;
> +}
> +
> +static int xen_platform_notifier(struct notifier_block *nb,
> +				 unsigned long action, void *data)
> +{
> +	struct platform_device *pdev = to_platform_device(data);
> +	int r = 0;
> +
> +	if (pdev->num_resources == 0 || pdev->resource == NULL)
> +		return NOTIFY_OK;
> +
> +	switch (action) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		r = xen_map_device_mmio(pdev->resource, pdev->num_resources);
> +		break;
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		r = xen_unmap_device_mmio(pdev->resource, pdev->num_resources);
> +		break;
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +	if (r)
> +		dev_err(&pdev->dev, "Platform: Failed to %s device %s MMIO!\n",
> +			action == BUS_NOTIFY_ADD_DEVICE ? "map" :
> +			(action == BUS_NOTIFY_DEL_DEVICE ? "unmap" : "?"),
> +			pdev->name);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block platform_device_nb = {
> +	.notifier_call = xen_platform_notifier,
> +};
> +
> +static int __init register_xen_platform_notifier(void)
> +{
> +	if (!xen_initial_domain() || acpi_disabled)
> +		return 0;
> +
> +	return bus_register_notifier(&platform_bus_type, &platform_device_nb);
> +}
> +
> +arch_initcall(register_xen_platform_notifier);

[PATCH v2 07/16] Xen: ARM: Add support for mapping AMBA device mmio

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Add a bus_notifier for AMBA bus device in order to map the device
mmio regions when DOM0 booting with ACPI.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 drivers/xen/arm-device.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/xen/arm-device.c b/drivers/xen/arm-device.c
index 7d902ea..9978f8d 100644
--- a/drivers/xen/arm-device.c
+++ b/drivers/xen/arm-device.c
@@ -138,3 +138,46 @@ static int __init register_xen_platform_notifier(void)
 }
 
 arch_initcall(register_xen_platform_notifier);
+
+#ifdef CONFIG_ARM_AMBA
+#include <linux/amba/bus.h>
+
+static int xen_amba_notifier(struct notifier_block *nb,
+			     unsigned long action, void *data)
+{
+	struct amba_device *adev = to_amba_device(data);
+	int r = 0;
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		r = xen_map_device_mmio(&adev->res, 1);
+		break;
+	case BUS_NOTIFY_DEL_DEVICE:
+		r = xen_unmap_device_mmio(&adev->res, 1);
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+	if (r)
+		dev_err(&adev->dev, "AMBA: Failed to %s device %s MMIO!\n",
+			action == BUS_NOTIFY_ADD_DEVICE ? "map" :
+			(action == BUS_NOTIFY_DEL_DEVICE ? "unmap" : "?"),
+			adev->dev.init_name);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block amba_device_nb = {
+	.notifier_call = xen_amba_notifier,
+};
+
+static int __init register_xen_amba_notifier(void)
+{
+	if (!xen_initial_domain() || acpi_disabled)
+		return 0;
+
+	return bus_register_notifier(&amba_bustype, &amba_device_nb);
+}
+
+arch_initcall(register_xen_amba_notifier);
+#endif
-- 
2.0.4

Re: [Xen-devel] [PATCH v2 07/16] Xen: ARM: Add support for mapping AMBA device mmio

[Stefano Stabellini]

On Fri, 15 Jan 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Add a bus_notifier for AMBA bus device in order to map the device
> mmio regions when DOM0 booting with ACPI.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  drivers/xen/arm-device.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/xen/arm-device.c b/drivers/xen/arm-device.c
> index 7d902ea..9978f8d 100644
> --- a/drivers/xen/arm-device.c
> +++ b/drivers/xen/arm-device.c
> @@ -138,3 +138,46 @@ static int __init register_xen_platform_notifier(void)
>  }
>  
>  arch_initcall(register_xen_platform_notifier);
> +
> +#ifdef CONFIG_ARM_AMBA
> +#include <linux/amba/bus.h>
> +
> +static int xen_amba_notifier(struct notifier_block *nb,
> +			     unsigned long action, void *data)
> +{
> +	struct amba_device *adev = to_amba_device(data);
> +	int r = 0;
> +
> +	switch (action) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		r = xen_map_device_mmio(&adev->res, 1);
> +		break;
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		r = xen_unmap_device_mmio(&adev->res, 1);
> +		break;
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +	if (r)
> +		dev_err(&adev->dev, "AMBA: Failed to %s device %s MMIO!\n",
> +			action == BUS_NOTIFY_ADD_DEVICE ? "map" :
> +			(action == BUS_NOTIFY_DEL_DEVICE ? "unmap" : "?"),
> +			adev->dev.init_name);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block amba_device_nb = {
> +	.notifier_call = xen_amba_notifier,
> +};
> +
> +static int __init register_xen_amba_notifier(void)
> +{
> +	if (!xen_initial_domain() || acpi_disabled)
> +		return 0;
> +
> +	return bus_register_notifier(&amba_bustype, &amba_device_nb);
> +}
> +
> +arch_initcall(register_xen_amba_notifier);
> +#endif
> -- 
> 2.0.4
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

[PATCH v2 08/16] xen/hvm/params: Add a new delivery type for event-channel in HVM_PARAM_CALLBACK_IRQ

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Add a new delivery type:
val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI.
To the flag, bit 0 stands the interrupt mode is edge(1) or level(0) and
bit 1 stands the interrupt polarity is active low(1) or high(0).

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 include/xen/interface/hvm/params.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/xen/interface/hvm/params.h b/include/xen/interface/hvm/params.h
index a6c7991..550688a 100644
--- a/include/xen/interface/hvm/params.h
+++ b/include/xen/interface/hvm/params.h
@@ -34,6 +34,11 @@
  *                  Domain = val[47:32], Bus  = val[31:16],
  *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
  * val[63:56] == 2: val[7:0] is a vector number.
+ * val[63:56] == 3: val[15:8] is flag of event-channel interrupt:
+ *                      bit 0: interrupt is edge(1) or level(0) triggered
+ *                      bit 1: interrupt is active low(1) or high(0)
+ *                  val[7:0] is PPI number used by event-channel.
+ *                  This is only used by ARM/ARM64.
  * If val == 0 then CPU0 event-channel notifications are not delivered.
  */
 #define HVM_PARAM_CALLBACK_IRQ 0
-- 
2.0.4

Re: [Xen-devel] [PATCH v2 08/16] xen/hvm/params: Add a new delivery type for event-channel in HVM_PARAM_CALLBACK_IRQ

[Stefano Stabellini]

On Fri, 15 Jan 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Add a new delivery type:
> val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI.
> To the flag, bit 0 stands the interrupt mode is edge(1) or level(0) and
> bit 1 stands the interrupt polarity is active low(1) or high(0).
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  include/xen/interface/hvm/params.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/xen/interface/hvm/params.h b/include/xen/interface/hvm/params.h
> index a6c7991..550688a 100644
> --- a/include/xen/interface/hvm/params.h
> +++ b/include/xen/interface/hvm/params.h
> @@ -34,6 +34,11 @@
>   *                  Domain = val[47:32], Bus  = val[31:16],
>   *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
>   * val[63:56] == 2: val[7:0] is a vector number.
> + * val[63:56] == 3: val[15:8] is flag of event-channel interrupt:
> + *                      bit 0: interrupt is edge(1) or level(0) triggered
> + *                      bit 1: interrupt is active low(1) or high(0)
> + *                  val[7:0] is PPI number used by event-channel.
> + *                  This is only used by ARM/ARM64.
>   * If val == 0 then CPU0 event-channel notifications are not delivered.
>   */
>  #define HVM_PARAM_CALLBACK_IRQ 0

Andrew, I think that this patch is correct. Looking back at your
previous comment (http://marc.info/?l=devicetree&m=144804014214262&w=2),
is it possible that you were confused by enum callback_via_type, which
is internal to Xen and offset'ed by 1 compared to the described values
in xen/include/public/hvm/params.h?

If not, and indeed somebody introduced one more field but failed to
document it, then I suggest she sends a patch to fix the issue as soon
as possible.

Re: [Xen-devel] [PATCH v2 08/16] xen/hvm/params: Add a new delivery type for event-channel in HVM_PARAM_CALLBACK_IRQ

[Andrew Cooper]

On 18/01/16 12:38, Stefano Stabellini wrote:
> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Add a new delivery type:
>> val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI.
>> To the flag, bit 0 stands the interrupt mode is edge(1) or level(0) and
>> bit 1 stands the interrupt polarity is active low(1) or high(0).
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  include/xen/interface/hvm/params.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/include/xen/interface/hvm/params.h b/include/xen/interface/hvm/params.h
>> index a6c7991..550688a 100644
>> --- a/include/xen/interface/hvm/params.h
>> +++ b/include/xen/interface/hvm/params.h
>> @@ -34,6 +34,11 @@
>>   *                  Domain = val[47:32], Bus  = val[31:16],
>>   *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
>>   * val[63:56] == 2: val[7:0] is a vector number.
>> + * val[63:56] == 3: val[15:8] is flag of event-channel interrupt:
>> + *                      bit 0: interrupt is edge(1) or level(0) triggered
>> + *                      bit 1: interrupt is active low(1) or high(0)
>> + *                  val[7:0] is PPI number used by event-channel.
>> + *                  This is only used by ARM/ARM64.
>>   * If val == 0 then CPU0 event-channel notifications are not delivered.
>>   */
>>  #define HVM_PARAM_CALLBACK_IRQ 0
> Andrew, I think that this patch is correct. Looking back at your
> previous comment (http://marc.info/?l=devicetree&m=144804014214262&w=2),
> is it possible that you were confused by enum callback_via_type, which
> is internal to Xen and offset'ed by 1 compared to the described values
> in xen/include/public/hvm/params.h?
>
> If not, and indeed somebody introduced one more field but failed to
> document it, then I suggest she sends a patch to fix the issue as soon
> as possible.

I was indeed confused - the ABI is utterly mad.

However, this change does need rebasing over c/s ca5c54b, which was the
result of the original discussion.

~Andrew

Re: [Xen-devel] [PATCH v2 08/16] xen/hvm/params: Add a new delivery type for event-channel in HVM_PARAM_CALLBACK_IRQ

[Stefano Stabellini]

On Mon, 18 Jan 2016, Andrew Cooper wrote:
> On 18/01/16 12:38, Stefano Stabellini wrote:
> > On Fri, 15 Jan 2016, Shannon Zhao wrote:
> >> From: Shannon Zhao <shannon.zhao@linaro.org>
> >>
> >> Add a new delivery type:
> >> val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI.
> >> To the flag, bit 0 stands the interrupt mode is edge(1) or level(0) and
> >> bit 1 stands the interrupt polarity is active low(1) or high(0).
> >>
> >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >> ---
> >>  include/xen/interface/hvm/params.h | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/include/xen/interface/hvm/params.h b/include/xen/interface/hvm/params.h
> >> index a6c7991..550688a 100644
> >> --- a/include/xen/interface/hvm/params.h
> >> +++ b/include/xen/interface/hvm/params.h
> >> @@ -34,6 +34,11 @@
> >>   *                  Domain = val[47:32], Bus  = val[31:16],
> >>   *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
> >>   * val[63:56] == 2: val[7:0] is a vector number.
> >> + * val[63:56] == 3: val[15:8] is flag of event-channel interrupt:
> >> + *                      bit 0: interrupt is edge(1) or level(0) triggered
> >> + *                      bit 1: interrupt is active low(1) or high(0)
> >> + *                  val[7:0] is PPI number used by event-channel.
> >> + *                  This is only used by ARM/ARM64.
> >>   * If val == 0 then CPU0 event-channel notifications are not delivered.
> >>   */
> >>  #define HVM_PARAM_CALLBACK_IRQ 0
> > Andrew, I think that this patch is correct. Looking back at your
> > previous comment (http://marc.info/?l=devicetree&m=144804014214262&w=2),
> > is it possible that you were confused by enum callback_via_type, which
> > is internal to Xen and offset'ed by 1 compared to the described values
> > in xen/include/public/hvm/params.h?
> >
> > If not, and indeed somebody introduced one more field but failed to
> > document it, then I suggest she sends a patch to fix the issue as soon
> > as possible.
> 
> I was indeed confused - the ABI is utterly mad.

All right. In that case, Shannon, you can add my

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


> However, this change does need rebasing over c/s ca5c54b, which was the
> result of the original discussion.

c/s ca5c54b is for Xen, while this is a Linux patch (Linux has its own
set of Xen headers).

Re: [Xen-devel] [PATCH v2 08/16] xen/hvm/params: Add a new delivery type for event-channel in HVM_PARAM_CALLBACK_IRQ

[Andrew Cooper]

On 18/01/16 12:46, Stefano Stabellini wrote:
> On Mon, 18 Jan 2016, Andrew Cooper wrote:
>> On 18/01/16 12:38, Stefano Stabellini wrote:
>>> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>
>>>> Add a new delivery type:
>>>> val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI.
>>>> To the flag, bit 0 stands the interrupt mode is edge(1) or level(0) and
>>>> bit 1 stands the interrupt polarity is active low(1) or high(0).
>>>>
>>>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>>> ---
>>>>  include/xen/interface/hvm/params.h | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/include/xen/interface/hvm/params.h b/include/xen/interface/hvm/params.h
>>>> index a6c7991..550688a 100644
>>>> --- a/include/xen/interface/hvm/params.h
>>>> +++ b/include/xen/interface/hvm/params.h
>>>> @@ -34,6 +34,11 @@
>>>>   *                  Domain = val[47:32], Bus  = val[31:16],
>>>>   *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
>>>>   * val[63:56] == 2: val[7:0] is a vector number.
>>>> + * val[63:56] == 3: val[15:8] is flag of event-channel interrupt:
>>>> + *                      bit 0: interrupt is edge(1) or level(0) triggered
>>>> + *                      bit 1: interrupt is active low(1) or high(0)
>>>> + *                  val[7:0] is PPI number used by event-channel.
>>>> + *                  This is only used by ARM/ARM64.
>>>>   * If val == 0 then CPU0 event-channel notifications are not delivered.
>>>>   */
>>>>  #define HVM_PARAM_CALLBACK_IRQ 0
>>> Andrew, I think that this patch is correct. Looking back at your
>>> previous comment (http://marc.info/?l=devicetree&m=144804014214262&w=2),
>>> is it possible that you were confused by enum callback_via_type, which
>>> is internal to Xen and offset'ed by 1 compared to the described values
>>> in xen/include/public/hvm/params.h?
>>>
>>> If not, and indeed somebody introduced one more field but failed to
>>> document it, then I suggest she sends a patch to fix the issue as soon
>>> as possible.
>> I was indeed confused - the ABI is utterly mad.
> All right. In that case, Shannon, you can add my
>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
>
>> However, this change does need rebasing over c/s ca5c54b, which was the
>> result of the original discussion.
> c/s ca5c54b is for Xen, while this is a Linux patch (Linux has its own
> set of Xen headers).

All ABI changes need to happen in the Xen public headers first.  This
patch cannot be accepted yet.

~Andrew

Re: [Xen-devel] [PATCH v2 08/16] xen/hvm/params: Add a new delivery type for event-channel in HVM_PARAM_CALLBACK_IRQ

[Shannon Zhao]

On 2016/1/18 20:52, Andrew Cooper wrote:
> On 18/01/16 12:46, Stefano Stabellini wrote:
>> > On Mon, 18 Jan 2016, Andrew Cooper wrote:
>>> >> On 18/01/16 12:38, Stefano Stabellini wrote:
>>>> >>> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>>>>> >>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>> >>>>
>>>>> >>>> Add a new delivery type:
>>>>> >>>> val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI.
>>>>> >>>> To the flag, bit 0 stands the interrupt mode is edge(1) or level(0) and
>>>>> >>>> bit 1 stands the interrupt polarity is active low(1) or high(0).
>>>>> >>>>
>>>>> >>>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>>>> >>>> ---
>>>>> >>>>  include/xen/interface/hvm/params.h | 5 +++++
>>>>> >>>>  1 file changed, 5 insertions(+)
>>>>> >>>>
>>>>> >>>> diff --git a/include/xen/interface/hvm/params.h b/include/xen/interface/hvm/params.h
>>>>> >>>> index a6c7991..550688a 100644
>>>>> >>>> --- a/include/xen/interface/hvm/params.h
>>>>> >>>> +++ b/include/xen/interface/hvm/params.h
>>>>> >>>> @@ -34,6 +34,11 @@
>>>>> >>>>   *                  Domain = val[47:32], Bus  = val[31:16],
>>>>> >>>>   *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
>>>>> >>>>   * val[63:56] == 2: val[7:0] is a vector number.
>>>>> >>>> + * val[63:56] == 3: val[15:8] is flag of event-channel interrupt:
>>>>> >>>> + *                      bit 0: interrupt is edge(1) or level(0) triggered
>>>>> >>>> + *                      bit 1: interrupt is active low(1) or high(0)
>>>>> >>>> + *                  val[7:0] is PPI number used by event-channel.
>>>>> >>>> + *                  This is only used by ARM/ARM64.
>>>>> >>>>   * If val == 0 then CPU0 event-channel notifications are not delivered.
>>>>> >>>>   */
>>>>> >>>>  #define HVM_PARAM_CALLBACK_IRQ 0
>>>> >>> Andrew, I think that this patch is correct. Looking back at your
>>>> >>> previous comment (http://marc.info/?l=devicetree&m=144804014214262&w=2),
>>>> >>> is it possible that you were confused by enum callback_via_type, which
>>>> >>> is internal to Xen and offset'ed by 1 compared to the described values
>>>> >>> in xen/include/public/hvm/params.h?
>>>> >>>
>>>> >>> If not, and indeed somebody introduced one more field but failed to
>>>> >>> document it, then I suggest she sends a patch to fix the issue as soon
>>>> >>> as possible.
>>> >> I was indeed confused - the ABI is utterly mad.
>> > All right. In that case, Shannon, you can add my
>> >
>> > Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> >
>> >
>>> >> However, this change does need rebasing over c/s ca5c54b, which was the
>>> >> result of the original discussion.
>> > c/s ca5c54b is for Xen, while this is a Linux patch (Linux has its own
>> > set of Xen headers).
> All ABI changes need to happen in the Xen public headers first.  This
> patch cannot be accepted yet.
So you mean it should port c/s ca5c54b to Linux first? Then add this
change based on that?

Thanks,
-- 
Shannon

Re: [Xen-devel] [PATCH v2 08/16] xen/hvm/params: Add a new delivery type for event-channel in HVM_PARAM_CALLBACK_IRQ

[Andrew Cooper]

On 20/01/2016 06:33, Shannon Zhao wrote:
>
> On 2016/1/18 20:52, Andrew Cooper wrote:
>> On 18/01/16 12:46, Stefano Stabellini wrote:
>>>> On Mon, 18 Jan 2016, Andrew Cooper wrote:
>>>>>> On 18/01/16 12:38, Stefano Stabellini wrote:
>>>>>>>> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>>>>>>>>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>>>>>>>
>>>>>>>>>> Add a new delivery type:
>>>>>>>>>> val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI.
>>>>>>>>>> To the flag, bit 0 stands the interrupt mode is edge(1) or level(0) and
>>>>>>>>>> bit 1 stands the interrupt polarity is active low(1) or high(0).
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>>>>>>>>> ---
>>>>>>>>>>  include/xen/interface/hvm/params.h | 5 +++++
>>>>>>>>>>  1 file changed, 5 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/include/xen/interface/hvm/params.h b/include/xen/interface/hvm/params.h
>>>>>>>>>> index a6c7991..550688a 100644
>>>>>>>>>> --- a/include/xen/interface/hvm/params.h
>>>>>>>>>> +++ b/include/xen/interface/hvm/params.h
>>>>>>>>>> @@ -34,6 +34,11 @@
>>>>>>>>>>   *                  Domain = val[47:32], Bus  = val[31:16],
>>>>>>>>>>   *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
>>>>>>>>>>   * val[63:56] == 2: val[7:0] is a vector number.
>>>>>>>>>> + * val[63:56] == 3: val[15:8] is flag of event-channel interrupt:
>>>>>>>>>> + *                      bit 0: interrupt is edge(1) or level(0) triggered
>>>>>>>>>> + *                      bit 1: interrupt is active low(1) or high(0)
>>>>>>>>>> + *                  val[7:0] is PPI number used by event-channel.
>>>>>>>>>> + *                  This is only used by ARM/ARM64.
>>>>>>>>>>   * If val == 0 then CPU0 event-channel notifications are not delivered.
>>>>>>>>>>   */
>>>>>>>>>>  #define HVM_PARAM_CALLBACK_IRQ 0
>>>>>>>> Andrew, I think that this patch is correct. Looking back at your
>>>>>>>> previous comment (http://marc.info/?l=devicetree&m=144804014214262&w=2),
>>>>>>>> is it possible that you were confused by enum callback_via_type, which
>>>>>>>> is internal to Xen and offset'ed by 1 compared to the described values
>>>>>>>> in xen/include/public/hvm/params.h?
>>>>>>>>
>>>>>>>> If not, and indeed somebody introduced one more field but failed to
>>>>>>>> document it, then I suggest she sends a patch to fix the issue as soon
>>>>>>>> as possible.
>>>>>> I was indeed confused - the ABI is utterly mad.
>>>> All right. In that case, Shannon, you can add my
>>>>
>>>> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>>
>>>>
>>>>>> However, this change does need rebasing over c/s ca5c54b, which was the
>>>>>> result of the original discussion.
>>>> c/s ca5c54b is for Xen, while this is a Linux patch (Linux has its own
>>>> set of Xen headers).
>> All ABI changes need to happen in the Xen public headers first.  This
>> patch cannot be accepted yet.
> So you mean it should port c/s ca5c54b to Linux first? Then add this
> change based on that?

FIrst, you must get this content and submit a patch against Xen.  The
Xen repository is the authoritative version of the ABI.

Once that patch is accepted, you will need to port ca5c54b and the new
patch as part of this series.

~Andrew

Re: [Xen-devel] [PATCH v2 08/16] xen/hvm/params: Add a new delivery type for event-channel in HVM_PARAM_CALLBACK_IRQ

[Shannon Zhao]

On 2016/1/20 16:39, Andrew Cooper wrote:
> On 20/01/2016 06:33, Shannon Zhao wrote:
>> >
>> > On 2016/1/18 20:52, Andrew Cooper wrote:
>>> >> On 18/01/16 12:46, Stefano Stabellini wrote:
>>>>> >>>> On Mon, 18 Jan 2016, Andrew Cooper wrote:
>>>>>>> >>>>>> On 18/01/16 12:38, Stefano Stabellini wrote:
>>>>>>>>> >>>>>>>> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>>>>>>>>>>> >>>>>>>>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>>>>>>>> >>>>>>>>>>
>>>>>>>>>>> >>>>>>>>>> Add a new delivery type:
>>>>>>>>>>> >>>>>>>>>> val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI.
>>>>>>>>>>> >>>>>>>>>> To the flag, bit 0 stands the interrupt mode is edge(1) or level(0) and
>>>>>>>>>>> >>>>>>>>>> bit 1 stands the interrupt polarity is active low(1) or high(0).
>>>>>>>>>>> >>>>>>>>>>
>>>>>>>>>>> >>>>>>>>>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>>>>>>>>>> >>>>>>>>>> ---
>>>>>>>>>>> >>>>>>>>>>  include/xen/interface/hvm/params.h | 5 +++++
>>>>>>>>>>> >>>>>>>>>>  1 file changed, 5 insertions(+)
>>>>>>>>>>> >>>>>>>>>>
>>>>>>>>>>> >>>>>>>>>> diff --git a/include/xen/interface/hvm/params.h b/include/xen/interface/hvm/params.h
>>>>>>>>>>> >>>>>>>>>> index a6c7991..550688a 100644
>>>>>>>>>>> >>>>>>>>>> --- a/include/xen/interface/hvm/params.h
>>>>>>>>>>> >>>>>>>>>> +++ b/include/xen/interface/hvm/params.h
>>>>>>>>>>> >>>>>>>>>> @@ -34,6 +34,11 @@
>>>>>>>>>>> >>>>>>>>>>   *                  Domain = val[47:32], Bus  = val[31:16],
>>>>>>>>>>> >>>>>>>>>>   *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
>>>>>>>>>>> >>>>>>>>>>   * val[63:56] == 2: val[7:0] is a vector number.
>>>>>>>>>>> >>>>>>>>>> + * val[63:56] == 3: val[15:8] is flag of event-channel interrupt:
>>>>>>>>>>> >>>>>>>>>> + *                      bit 0: interrupt is edge(1) or level(0) triggered
>>>>>>>>>>> >>>>>>>>>> + *                      bit 1: interrupt is active low(1) or high(0)
>>>>>>>>>>> >>>>>>>>>> + *                  val[7:0] is PPI number used by event-channel.
>>>>>>>>>>> >>>>>>>>>> + *                  This is only used by ARM/ARM64.
>>>>>>>>>>> >>>>>>>>>>   * If val == 0 then CPU0 event-channel notifications are not delivered.
>>>>>>>>>>> >>>>>>>>>>   */
>>>>>>>>>>> >>>>>>>>>>  #define HVM_PARAM_CALLBACK_IRQ 0
>>>>>>>>> >>>>>>>> Andrew, I think that this patch is correct. Looking back at your
>>>>>>>>> >>>>>>>> previous comment (http://marc.info/?l=devicetree&m=144804014214262&w=2),
>>>>>>>>> >>>>>>>> is it possible that you were confused by enum callback_via_type, which
>>>>>>>>> >>>>>>>> is internal to Xen and offset'ed by 1 compared to the described values
>>>>>>>>> >>>>>>>> in xen/include/public/hvm/params.h?
>>>>>>>>> >>>>>>>>
>>>>>>>>> >>>>>>>> If not, and indeed somebody introduced one more field but failed to
>>>>>>>>> >>>>>>>> document it, then I suggest she sends a patch to fix the issue as soon
>>>>>>>>> >>>>>>>> as possible.
>>>>>>> >>>>>> I was indeed confused - the ABI is utterly mad.
>>>>> >>>> All right. In that case, Shannon, you can add my
>>>>> >>>>
>>>>> >>>> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>>> >>>>
>>>>> >>>>
>>>>>>> >>>>>> However, this change does need rebasing over c/s ca5c54b, which was the
>>>>>>> >>>>>> result of the original discussion.
>>>>> >>>> c/s ca5c54b is for Xen, while this is a Linux patch (Linux has its own
>>>>> >>>> set of Xen headers).
>>> >> All ABI changes need to happen in the Xen public headers first.  This
>>> >> patch cannot be accepted yet.
>> > So you mean it should port c/s ca5c54b to Linux first? Then add this
>> > change based on that?
> FIrst, you must get this content and submit a patch against Xen.  The
> Xen repository is the authoritative version of the ABI.
> 
I've sent a patch to Xen, but not applied yet.

> Once that patch is accepted, you will need to port ca5c54b and the new
> patch as part of this series.
Ok, thanks.

-- 
Shannon

[PATCH v2 09/16] arm/xen: Get event-channel irq through HVM_PARAM when booting with ACPI

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

When booting with ACPI, it could get the event-channel irq through
HVM_PARAM_CALLBACK_IRQ.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 arch/arm/xen/enlighten.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 6d90a62..4e78c49 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -29,6 +29,7 @@
 #include <linux/time64.h>
 #include <linux/timekeeping.h>
 #include <linux/timekeeper_internal.h>
+#include <linux/acpi.h>
 
 #include <linux/mm.h>
 
@@ -270,7 +271,34 @@ static int __init xen_guest_init(void)
 	if (!xen_domain())
 		return 0;
 
-	xen_events_irq = irq_of_parse_and_map(xen_node, 0);
+	if (!acpi_disabled) {
+#ifdef CONFIG_ACPI
+		struct xen_hvm_param a;
+		int interrupt, trigger, polarity;
+
+		a.domid = DOMID_SELF;
+		a.index = HVM_PARAM_CALLBACK_IRQ;
+		xen_events_irq = 0;
+		if (!HYPERVISOR_hvm_op(HVMOP_get_param, &a)) {
+			if ((a.value >> 56) == 3) {
+				interrupt = a.value & 0xff;
+				trigger = ((a.value >> 8) & 0x1)
+					  ? ACPI_EDGE_SENSITIVE
+					  : ACPI_LEVEL_SENSITIVE;
+				polarity = ((a.value >> 8) & 0x2)
+					  ? ACPI_ACTIVE_LOW
+					  : ACPI_ACTIVE_HIGH;
+				xen_events_irq = acpi_register_gsi(NULL,
+								   interrupt,
+								   trigger,
+								   polarity);
+			}
+		}
+#endif
+	} else {
+		xen_events_irq = irq_of_parse_and_map(xen_node, 0);
+	}
+
 	if (!xen_events_irq) {
 		pr_err("Xen event channel interrupt not found\n");
 		return -ENODEV;
-- 
2.0.4

Re: [Xen-devel] [PATCH v2 09/16] arm/xen: Get event-channel irq through HVM_PARAM when booting with ACPI

[Stefano Stabellini]

On Fri, 15 Jan 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> When booting with ACPI, it could get the event-channel irq through
> HVM_PARAM_CALLBACK_IRQ.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  arch/arm/xen/enlighten.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 6d90a62..4e78c49 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -29,6 +29,7 @@
>  #include <linux/time64.h>
>  #include <linux/timekeeping.h>
>  #include <linux/timekeeper_internal.h>
> +#include <linux/acpi.h>
>  
>  #include <linux/mm.h>
>  
> @@ -270,7 +271,34 @@ static int __init xen_guest_init(void)
>  	if (!xen_domain())
>  		return 0;
>  
> -	xen_events_irq = irq_of_parse_and_map(xen_node, 0);
> +	if (!acpi_disabled) {
> +#ifdef CONFIG_ACPI

I think it would be best to move the acpi specific and DT specific
initializations out of xen_guest_init. Then at the beginning of
xen_guest_init, call only the one that applies.  The acpi init function
could be ifdef'ed CONFIG_ACPI. Making sure that acpi_disabled is 1 when
!CONFIG_ACPI, we could even avoid all ifdef's from xen_guest_init.

The rest is good, thanks.


> +		struct xen_hvm_param a;
> +		int interrupt, trigger, polarity;
> +
> +		a.domid = DOMID_SELF;
> +		a.index = HVM_PARAM_CALLBACK_IRQ;
> +		xen_events_irq = 0;
> +		if (!HYPERVISOR_hvm_op(HVMOP_get_param, &a)) {
> +			if ((a.value >> 56) == 3) {
> +				interrupt = a.value & 0xff;
> +				trigger = ((a.value >> 8) & 0x1)
> +					  ? ACPI_EDGE_SENSITIVE
> +					  : ACPI_LEVEL_SENSITIVE;
> +				polarity = ((a.value >> 8) & 0x2)
> +					  ? ACPI_ACTIVE_LOW
> +					  : ACPI_ACTIVE_HIGH;
> +				xen_events_irq = acpi_register_gsi(NULL,
> +								   interrupt,
> +								   trigger,
> +								   polarity);
> +			}
> +		}
> +
> +#endif
> +	} else {
> +		xen_events_irq = irq_of_parse_and_map(xen_node, 0);
> +	}
> +
>  	if (!xen_events_irq) {
>  		pr_err("Xen event channel interrupt not found\n");
>  		return -ENODEV;

Re: [Xen-devel] [PATCH v2 09/16] arm/xen: Get event-channel irq through HVM_PARAM when booting with ACPI

[Shannon Zhao]

On 2016/1/18 22:38, Stefano Stabellini wrote:
> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> When booting with ACPI, it could get the event-channel irq through
>> HVM_PARAM_CALLBACK_IRQ.
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  arch/arm/xen/enlighten.c | 30 +++++++++++++++++++++++++++++-
>>  1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> index 6d90a62..4e78c49 100644
>> --- a/arch/arm/xen/enlighten.c
>> +++ b/arch/arm/xen/enlighten.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/time64.h>
>>  #include <linux/timekeeping.h>
>>  #include <linux/timekeeper_internal.h>
>> +#include <linux/acpi.h>
>>  
>>  #include <linux/mm.h>
>>  
>> @@ -270,7 +271,34 @@ static int __init xen_guest_init(void)
>>  	if (!xen_domain())
>>  		return 0;
>>  
>> -	xen_events_irq = irq_of_parse_and_map(xen_node, 0);
>> +	if (!acpi_disabled) {
>> +#ifdef CONFIG_ACPI
> 
> I think it would be best to move the acpi specific and DT specific
> initializations out of xen_guest_init. Then at the beginning of
> xen_guest_init, call only the one that applies.  The acpi init function
> could be ifdef'ed CONFIG_ACPI. Making sure that acpi_disabled is 1 when
> !CONFIG_ACPI, we could even avoid all ifdef's from xen_guest_init.
> 
Sure, will fix. Thanks.

> The rest is good, thanks.
> 
> 
>> +		struct xen_hvm_param a;
>> +		int interrupt, trigger, polarity;
>> +
>> +		a.domid = DOMID_SELF;
>> +		a.index = HVM_PARAM_CALLBACK_IRQ;
>> +		xen_events_irq = 0;
>> +		if (!HYPERVISOR_hvm_op(HVMOP_get_param, &a)) {
>> +			if ((a.value >> 56) == 3) {
>> +				interrupt = a.value & 0xff;
>> +				trigger = ((a.value >> 8) & 0x1)
>> +					  ? ACPI_EDGE_SENSITIVE
>> +					  : ACPI_LEVEL_SENSITIVE;
>> +				polarity = ((a.value >> 8) & 0x2)
>> +					  ? ACPI_ACTIVE_LOW
>> +					  : ACPI_ACTIVE_HIGH;
>> +				xen_events_irq = acpi_register_gsi(NULL,
>> +								   interrupt,
>> +								   trigger,
>> +								   polarity);
>> +			}
>> +		}
>> +
>> +#endif
>> +	} else {
>> +		xen_events_irq = irq_of_parse_and_map(xen_node, 0);
>> +	}
>> +
>>  	if (!xen_events_irq) {
>>  		pr_err("Xen event channel interrupt not found\n");
>>  		return -ENODEV;
> 
> .
> 

-- 
Shannon

[PATCH v2 10/16] ARM: XEN: Move xen_early_init() before efi_init()

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Move xen_early_init() before efi_init(), then when calling efi_init()
could initialize Xen specific UEFI.

Check if it runs on Xen hypervisor through the flat dts.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 arch/arm/xen/enlighten.c  | 56 ++++++++++++++++++++++++++++++++++-------------
 arch/arm64/kernel/setup.c |  2 +-
 2 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 4e78c49..5d0fe68 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -19,6 +19,7 @@
 #include <linux/irqreturn.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_fdt.h>
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
 #include <linux/cpuidle.h>
@@ -52,8 +53,6 @@ struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
 
 static __read_mostly unsigned int xen_events_irq;
 
-static __initdata struct device_node *xen_node;
-
 int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
 			       unsigned long addr,
 			       xen_pfn_t *gfn, int nr,
@@ -222,6 +221,33 @@ static irqreturn_t xen_arm_callback(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static __initdata struct {
+	const char *compat;
+	const char *prefix;
+	const char *version;
+	bool found;
+} xen_node = {"xen,xen", "xen,xen-", NULL, false};
+
+static int __init fdt_find_xen_node(unsigned long node, const char *uname,
+				    int depth, void *data)
+{
+	const void *s = NULL;
+	int len;
+
+	if (depth != 1 || strcmp(uname, "hypervisor") != 0)
+		return 0;
+
+	if (of_flat_dt_is_compatible(node, xen_node.compat))
+		xen_node.found = true;
+
+	s = of_get_flat_dt_prop(node, "compatible", &len);
+	if (strlen(xen_node.prefix) + 3  < len &&
+	    !strncmp(xen_node.prefix, s, strlen(xen_node.prefix)))
+		xen_node.version = s + strlen(xen_node.prefix);
+
+	return 0;
+}
+
 /*
  * see Documentation/devicetree/bindings/arm/xen.txt for the
  * documentation of the Xen Device Tree format.
@@ -229,26 +255,18 @@ static irqreturn_t xen_arm_callback(int irq, void *arg)
 #define GRANT_TABLE_PHYSADDR 0
 void __init xen_early_init(void)
 {
-	int len;
-	const char *s = NULL;
-	const char *version = NULL;
-	const char *xen_prefix = "xen,xen-";
-
-	xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
-	if (!xen_node) {
+	of_scan_flat_dt(fdt_find_xen_node, NULL);
+	if (!xen_node.found) {
 		pr_debug("No Xen support\n");
 		return;
 	}
-	s = of_get_property(xen_node, "compatible", &len);
-	if (strlen(xen_prefix) + 3  < len &&
-			!strncmp(xen_prefix, s, strlen(xen_prefix)))
-		version = s + strlen(xen_prefix);
-	if (version == NULL) {
+
+	if (xen_node.version == NULL) {
 		pr_debug("Xen version not found\n");
 		return;
 	}
 
-	pr_info("Xen %s support found\n", version);
+	pr_info("Xen %s support found\n", xen_node.version);
 
 	xen_domain_type = XEN_HVM_DOMAIN;
 
@@ -296,6 +314,14 @@ static int __init xen_guest_init(void)
 		}
 #endif
 	} else {
+		struct device_node *xen_node;
+
+		xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
+		if (!xen_node) {
+			pr_debug("No Xen support\n");
+			return -ENODEV;
+		}
+
 		xen_events_irq = irq_of_parse_and_map(xen_node, 0);
 	}
 
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 8119479..a4a2878 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -313,6 +313,7 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	local_async_enable();
 
+	xen_early_init();
 	efi_init();
 	arm64_memblock_init();
 
@@ -334,7 +335,6 @@ void __init setup_arch(char **cmdline_p)
 	} else {
 		psci_acpi_init();
 	}
-	xen_early_init();
 
 	cpu_read_bootcpu_ops();
 	smp_init_cpus();
-- 
2.0.4

Re: [Xen-devel] [PATCH v2 10/16] ARM: XEN: Move xen_early_init() before efi_init()

[Stefano Stabellini]

On Fri, 15 Jan 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Move xen_early_init() before efi_init(), then when calling efi_init()
> could initialize Xen specific UEFI.
> 
> Check if it runs on Xen hypervisor through the flat dts.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  arch/arm/xen/enlighten.c  | 56 ++++++++++++++++++++++++++++++++++-------------
>  arch/arm64/kernel/setup.c |  2 +-
>  2 files changed, 42 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 4e78c49..5d0fe68 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -19,6 +19,7 @@
>  #include <linux/irqreturn.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_fdt.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_address.h>
>  #include <linux/cpuidle.h>
> @@ -52,8 +53,6 @@ struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
>  
>  static __read_mostly unsigned int xen_events_irq;
>  
> -static __initdata struct device_node *xen_node;
> -
>  int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
>  			       unsigned long addr,
>  			       xen_pfn_t *gfn, int nr,
> @@ -222,6 +221,33 @@ static irqreturn_t xen_arm_callback(int irq, void *arg)
>  	return IRQ_HANDLED;
>  }
>  
> +static __initdata struct {
> +	const char *compat;
> +	const char *prefix;
> +	const char *version;
> +	bool found;
> +} xen_node = {"xen,xen", "xen,xen-", NULL, false};
> +
> +static int __init fdt_find_xen_node(unsigned long node, const char *uname,
> +				    int depth, void *data)
> +{
> +	const void *s = NULL;
> +	int len;
> +
> +	if (depth != 1 || strcmp(uname, "hypervisor") != 0)
> +		return 0;
> +
> +	if (of_flat_dt_is_compatible(node, xen_node.compat))
> +		xen_node.found = true;
> +
> +	s = of_get_flat_dt_prop(node, "compatible", &len);
> +	if (strlen(xen_node.prefix) + 3  < len &&
> +	    !strncmp(xen_node.prefix, s, strlen(xen_node.prefix)))
> +		xen_node.version = s + strlen(xen_node.prefix);
> +
> +	return 0;
> +}
> +
>  /*
>   * see Documentation/devicetree/bindings/arm/xen.txt for the
>   * documentation of the Xen Device Tree format.
> @@ -229,26 +255,18 @@ static irqreturn_t xen_arm_callback(int irq, void *arg)
>  #define GRANT_TABLE_PHYSADDR 0
>  void __init xen_early_init(void)
>  {
> -	int len;
> -	const char *s = NULL;
> -	const char *version = NULL;
> -	const char *xen_prefix = "xen,xen-";
> -
> -	xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
> -	if (!xen_node) {
> +	of_scan_flat_dt(fdt_find_xen_node, NULL);
> +	if (!xen_node.found) {
>  		pr_debug("No Xen support\n");
>  		return;
>  	}
> -	s = of_get_property(xen_node, "compatible", &len);
> -	if (strlen(xen_prefix) + 3  < len &&
> -			!strncmp(xen_prefix, s, strlen(xen_prefix)))
> -		version = s + strlen(xen_prefix);
> -	if (version == NULL) {
> +
> +	if (xen_node.version == NULL) {
>  		pr_debug("Xen version not found\n");
>  		return;
>  	}

This is going to access data within the fdt, after it has been
unflattened (by the unflatten_device_tree call). Is that allowed?

If that's not allowed, then you could copy the version string to
xen_node.version, rather than just copying the pointer. Otherwise you
could move the version check entirely within fdt_find_xen_node and
remove the version field from xen_node.


> -	pr_info("Xen %s support found\n", version);
> +	pr_info("Xen %s support found\n", xen_node.version);
>  
>  	xen_domain_type = XEN_HVM_DOMAIN;
>  
> @@ -296,6 +314,14 @@ static int __init xen_guest_init(void)
>  		}
>  #endif
>  	} else {
> +		struct device_node *xen_node;
> +
> +		xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
> +		if (!xen_node) {
> +			pr_debug("No Xen support\n");
> +			return -ENODEV;
> +		}

It makes sense to move this into a DT specific init function called from
xen_guest_init. Also, given that at this point we know that a Xen node
should be present, it makes sense to return EFAULT on error, instead of
ENODEV. The error message could be changed to something like: "Xen
support was detected before, but it has disappeared" and should be
pr_err.


>  		xen_events_irq = irq_of_parse_and_map(xen_node, 0);
>  	}
>  
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 8119479..a4a2878 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -313,6 +313,7 @@ void __init setup_arch(char **cmdline_p)
>  	 */
>  	local_async_enable();
>  
> +	xen_early_init();
>  	efi_init();
>  	arm64_memblock_init();
>  
> @@ -334,7 +335,6 @@ void __init setup_arch(char **cmdline_p)
>  	} else {
>  		psci_acpi_init();
>  	}
> -	xen_early_init();
>  
>  	cpu_read_bootcpu_ops();
>  	smp_init_cpus();

Re: [Xen-devel] [PATCH v2 10/16] ARM: XEN: Move xen_early_init() before efi_init()

[Shannon Zhao]

On 2016/1/18 22:59, Stefano Stabellini wrote:
> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Move xen_early_init() before efi_init(), then when calling efi_init()
>> could initialize Xen specific UEFI.
>>
>> Check if it runs on Xen hypervisor through the flat dts.
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  arch/arm/xen/enlighten.c  | 56 ++++++++++++++++++++++++++++++++++-------------
>>  arch/arm64/kernel/setup.c |  2 +-
>>  2 files changed, 42 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> index 4e78c49..5d0fe68 100644
>> --- a/arch/arm/xen/enlighten.c
>> +++ b/arch/arm/xen/enlighten.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/irqreturn.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>> +#include <linux/of_fdt.h>
>>  #include <linux/of_irq.h>
>>  #include <linux/of_address.h>
>>  #include <linux/cpuidle.h>
>> @@ -52,8 +53,6 @@ struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
>>  
>>  static __read_mostly unsigned int xen_events_irq;
>>  
>> -static __initdata struct device_node *xen_node;
>> -
>>  int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
>>  			       unsigned long addr,
>>  			       xen_pfn_t *gfn, int nr,
>> @@ -222,6 +221,33 @@ static irqreturn_t xen_arm_callback(int irq, void *arg)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static __initdata struct {
>> +	const char *compat;
>> +	const char *prefix;
>> +	const char *version;
>> +	bool found;
>> +} xen_node = {"xen,xen", "xen,xen-", NULL, false};
>> +
>> +static int __init fdt_find_xen_node(unsigned long node, const char *uname,
>> +				    int depth, void *data)
>> +{
>> +	const void *s = NULL;
>> +	int len;
>> +
>> +	if (depth != 1 || strcmp(uname, "hypervisor") != 0)
>> +		return 0;
>> +
>> +	if (of_flat_dt_is_compatible(node, xen_node.compat))
>> +		xen_node.found = true;
>> +
>> +	s = of_get_flat_dt_prop(node, "compatible", &len);
>> +	if (strlen(xen_node.prefix) + 3  < len &&
>> +	    !strncmp(xen_node.prefix, s, strlen(xen_node.prefix)))
>> +		xen_node.version = s + strlen(xen_node.prefix);
>> +
>> +	return 0;
>> +}
>> +
>>  /*
>>   * see Documentation/devicetree/bindings/arm/xen.txt for the
>>   * documentation of the Xen Device Tree format.
>> @@ -229,26 +255,18 @@ static irqreturn_t xen_arm_callback(int irq, void *arg)
>>  #define GRANT_TABLE_PHYSADDR 0
>>  void __init xen_early_init(void)
>>  {
>> -	int len;
>> -	const char *s = NULL;
>> -	const char *version = NULL;
>> -	const char *xen_prefix = "xen,xen-";
>> -
>> -	xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
>> -	if (!xen_node) {
>> +	of_scan_flat_dt(fdt_find_xen_node, NULL);
>> +	if (!xen_node.found) {
>>  		pr_debug("No Xen support\n");
>>  		return;
>>  	}
>> -	s = of_get_property(xen_node, "compatible", &len);
>> -	if (strlen(xen_prefix) + 3  < len &&
>> -			!strncmp(xen_prefix, s, strlen(xen_prefix)))
>> -		version = s + strlen(xen_prefix);
>> -	if (version == NULL) {
>> +
>> +	if (xen_node.version == NULL) {
>>  		pr_debug("Xen version not found\n");
>>  		return;
>>  	}
> 
> This is going to access data within the fdt, after it has been
> unflattened (by the unflatten_device_tree call). Is that allowed?
> 
> If that's not allowed, then you could copy the version string to
> xen_node.version, rather than just copying the pointer. Otherwise you
> could move the version check entirely within fdt_find_xen_node and
> remove the version field from xen_node.
> 
A little confused. Since here we move xen_early_init before efi_init and
before unflatten_device_tree as well, at this moment in xen_early_init
the fdt is flattened. The xen_node.version is only set in
fdt_find_xen_node and accessed in xen_early_init.

> 
>> -	pr_info("Xen %s support found\n", version);
>> +	pr_info("Xen %s support found\n", xen_node.version);
>>  
>>  	xen_domain_type = XEN_HVM_DOMAIN;
>>  
>> @@ -296,6 +314,14 @@ static int __init xen_guest_init(void)
>>  		}
>>  #endif
>>  	} else {
>> +		struct device_node *xen_node;
>> +
>> +		xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
>> +		if (!xen_node) {
>> +			pr_debug("No Xen support\n");
>> +			return -ENODEV;
>> +		}
> 
> It makes sense to move this into a DT specific init function called from
> xen_guest_init. Also, given that at this point we know that a Xen node
> should be present, it makes sense to return EFAULT on error, instead of
> ENODEV. The error message could be changed to something like: "Xen
> support was detected before, but it has disappeared" and should be
> pr_err.
> 
OK, thanks.
> 
>>  		xen_events_irq = irq_of_parse_and_map(xen_node, 0);
>>  	}
>>  
>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>> index 8119479..a4a2878 100644
>> --- a/arch/arm64/kernel/setup.c
>> +++ b/arch/arm64/kernel/setup.c
>> @@ -313,6 +313,7 @@ void __init setup_arch(char **cmdline_p)
>>  	 */
>>  	local_async_enable();
>>  
>> +	xen_early_init();
>>  	efi_init();
>>  	arm64_memblock_init();
>>  
>> @@ -334,7 +335,6 @@ void __init setup_arch(char **cmdline_p)
>>  	} else {
>>  		psci_acpi_init();
>>  	}
>> -	xen_early_init();
>>  
>>  	cpu_read_bootcpu_ops();
>>  	smp_init_cpus();
> 
> .
> 

-- 
Shannon

Re: [Xen-devel] [PATCH v2 10/16] ARM: XEN: Move xen_early_init() before efi_init()

[Stefano Stabellini]

On Tue, 19 Jan 2016, Shannon Zhao wrote:
> On 2016/1/18 22:59, Stefano Stabellini wrote:
> > On Fri, 15 Jan 2016, Shannon Zhao wrote:
> >> From: Shannon Zhao <shannon.zhao@linaro.org>
> >>
> >> Move xen_early_init() before efi_init(), then when calling efi_init()
> >> could initialize Xen specific UEFI.
> >>
> >> Check if it runs on Xen hypervisor through the flat dts.
> >>
> >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >> ---
> >>  arch/arm/xen/enlighten.c  | 56 ++++++++++++++++++++++++++++++++++-------------
> >>  arch/arm64/kernel/setup.c |  2 +-
> >>  2 files changed, 42 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> >> index 4e78c49..5d0fe68 100644
> >> --- a/arch/arm/xen/enlighten.c
> >> +++ b/arch/arm/xen/enlighten.c
> >> @@ -19,6 +19,7 @@
> >>  #include <linux/irqreturn.h>
> >>  #include <linux/module.h>
> >>  #include <linux/of.h>
> >> +#include <linux/of_fdt.h>
> >>  #include <linux/of_irq.h>
> >>  #include <linux/of_address.h>
> >>  #include <linux/cpuidle.h>
> >> @@ -52,8 +53,6 @@ struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
> >>  
> >>  static __read_mostly unsigned int xen_events_irq;
> >>  
> >> -static __initdata struct device_node *xen_node;
> >> -
> >>  int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
> >>  			       unsigned long addr,
> >>  			       xen_pfn_t *gfn, int nr,
> >> @@ -222,6 +221,33 @@ static irqreturn_t xen_arm_callback(int irq, void *arg)
> >>  	return IRQ_HANDLED;
> >>  }
> >>  
> >> +static __initdata struct {
> >> +	const char *compat;
> >> +	const char *prefix;
> >> +	const char *version;
> >> +	bool found;
> >> +} xen_node = {"xen,xen", "xen,xen-", NULL, false};
> >> +
> >> +static int __init fdt_find_xen_node(unsigned long node, const char *uname,
> >> +				    int depth, void *data)
> >> +{
> >> +	const void *s = NULL;
> >> +	int len;
> >> +
> >> +	if (depth != 1 || strcmp(uname, "hypervisor") != 0)
> >> +		return 0;
> >> +
> >> +	if (of_flat_dt_is_compatible(node, xen_node.compat))
> >> +		xen_node.found = true;
> >> +
> >> +	s = of_get_flat_dt_prop(node, "compatible", &len);
> >> +	if (strlen(xen_node.prefix) + 3  < len &&
> >> +	    !strncmp(xen_node.prefix, s, strlen(xen_node.prefix)))
> >> +		xen_node.version = s + strlen(xen_node.prefix);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  /*
> >>   * see Documentation/devicetree/bindings/arm/xen.txt for the
> >>   * documentation of the Xen Device Tree format.
> >> @@ -229,26 +255,18 @@ static irqreturn_t xen_arm_callback(int irq, void *arg)
> >>  #define GRANT_TABLE_PHYSADDR 0
> >>  void __init xen_early_init(void)
> >>  {
> >> -	int len;
> >> -	const char *s = NULL;
> >> -	const char *version = NULL;
> >> -	const char *xen_prefix = "xen,xen-";
> >> -
> >> -	xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
> >> -	if (!xen_node) {
> >> +	of_scan_flat_dt(fdt_find_xen_node, NULL);
> >> +	if (!xen_node.found) {
> >>  		pr_debug("No Xen support\n");
> >>  		return;
> >>  	}
> >> -	s = of_get_property(xen_node, "compatible", &len);
> >> -	if (strlen(xen_prefix) + 3  < len &&
> >> -			!strncmp(xen_prefix, s, strlen(xen_prefix)))
> >> -		version = s + strlen(xen_prefix);
> >> -	if (version == NULL) {
> >> +
> >> +	if (xen_node.version == NULL) {
> >>  		pr_debug("Xen version not found\n");
> >>  		return;
> >>  	}
> > 
> > This is going to access data within the fdt, after it has been
> > unflattened (by the unflatten_device_tree call). Is that allowed?
> > 
> > If that's not allowed, then you could copy the version string to
> > xen_node.version, rather than just copying the pointer. Otherwise you
> > could move the version check entirely within fdt_find_xen_node and
> > remove the version field from xen_node.
> > 
> A little confused. Since here we move xen_early_init before efi_init and
> before unflatten_device_tree as well, at this moment in xen_early_init
> the fdt is flattened. The xen_node.version is only set in
> fdt_find_xen_node and accessed in xen_early_init.

You are right, sorry, I must have been confused. It might be good to
rename the xen_node variable that this patch introduced in xen_guest_init.



> >> -	pr_info("Xen %s support found\n", version);
> >> +	pr_info("Xen %s support found\n", xen_node.version);
> >>  
> >>  	xen_domain_type = XEN_HVM_DOMAIN;
> >>  
> >> @@ -296,6 +314,14 @@ static int __init xen_guest_init(void)
> >>  		}
> >>  #endif
> >>  	} else {
> >> +		struct device_node *xen_node;
> >> +
> >> +		xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
> >> +		if (!xen_node) {
> >> +			pr_debug("No Xen support\n");
> >> +			return -ENODEV;
> >> +		}
> > 
> > It makes sense to move this into a DT specific init function called from
> > xen_guest_init. Also, given that at this point we know that a Xen node
> > should be present, it makes sense to return EFAULT on error, instead of
> > ENODEV. The error message could be changed to something like: "Xen
> > support was detected before, but it has disappeared" and should be
> > pr_err.
> > 
> OK, thanks.

[PATCH v2 11/16] ARM64: ACPI: Check if it runs on Xen to enable or disable ACPI

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
a /hypervisor node in DT. So check if it needs to enable ACPI.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 arch/arm64/kernel/acpi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index d1ce8e2..4e92be0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -67,10 +67,13 @@ static int __init dt_scan_depth1_nodes(unsigned long node,
 {
 	/*
 	 * Return 1 as soon as we encounter a node at depth 1 that is
-	 * not the /chosen node.
+	 * not the /chosen node, or /hypervisor node when running on Xen.
 	 */
-	if (depth == 1 && (strcmp(uname, "chosen") != 0))
-		return 1;
+	if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
+		if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
+			return 1;
+	}
+
 	return 0;
 }
 
@@ -184,7 +187,8 @@ void __init acpi_boot_table_init(void)
 	/*
 	 * Enable ACPI instead of device tree unless
 	 * - ACPI has been disabled explicitly (acpi=off), or
-	 * - the device tree is not empty (it has more than just a /chosen node)
+	 * - the device tree is not empty (it has more than just a /chosen node,
+	 *   and a /hypervisor node when running on Xen)
 	 *   and ACPI has not been force enabled (acpi=force)
 	 */
 	if (param_acpi_off ||
-- 
2.0.4

Re: [PATCH v2 11/16] ARM64: ACPI: Check if it runs on Xen to enable or disable ACPI

[Mark Rutland]

On Fri, Jan 15, 2016 at 02:55:24PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
> a /hypervisor node in DT. So check if it needs to enable ACPI.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  arch/arm64/kernel/acpi.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index d1ce8e2..4e92be0 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -67,10 +67,13 @@ static int __init dt_scan_depth1_nodes(unsigned long node,
>  {
>  	/*
>  	 * Return 1 as soon as we encounter a node at depth 1 that is
> -	 * not the /chosen node.
> +	 * not the /chosen node, or /hypervisor node when running on Xen.
>  	 */
> -	if (depth == 1 && (strcmp(uname, "chosen") != 0))
> -		return 1;
> +	if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
> +		if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
> +			return 1;
> +	}
> +
>  	return 0;
>  }

As this is changing the semantic of an "empty" DT, we should consider
now if there's anything else that might also need to exist in an "empty"
DT. We don't want to change this again in future if we don't have to,
given the compatiblity nightmare that's sure to result.

We should also consider if the "hypervisor" node name is sufficient (I
think it is, but let's not assume anything).

Mark.

>  
> @@ -184,7 +187,8 @@ void __init acpi_boot_table_init(void)
>  	/*
>  	 * Enable ACPI instead of device tree unless
>  	 * - ACPI has been disabled explicitly (acpi=off), or
> -	 * - the device tree is not empty (it has more than just a /chosen node)
> +	 * - the device tree is not empty (it has more than just a /chosen node,
> +	 *   and a /hypervisor node when running on Xen)
>  	 *   and ACPI has not been force enabled (acpi=force)
>  	 */
>  	if (param_acpi_off ||
> -- 
> 2.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 11/16] ARM64: ACPI: Check if it runs on Xen to enable or disable ACPI

[Stefano Stabellini]

On Mon, 18 Jan 2016, Mark Rutland wrote:
> On Fri, Jan 15, 2016 at 02:55:24PM +0800, Shannon Zhao wrote:
> > From: Shannon Zhao <shannon.zhao@linaro.org>
> > 
> > When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
> > a /hypervisor node in DT. So check if it needs to enable ACPI.
> > 
> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > ---
> >  arch/arm64/kernel/acpi.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > index d1ce8e2..4e92be0 100644
> > --- a/arch/arm64/kernel/acpi.c
> > +++ b/arch/arm64/kernel/acpi.c
> > @@ -67,10 +67,13 @@ static int __init dt_scan_depth1_nodes(unsigned long node,
> >  {
> >  	/*
> >  	 * Return 1 as soon as we encounter a node at depth 1 that is
> > -	 * not the /chosen node.
> > +	 * not the /chosen node, or /hypervisor node when running on Xen.
> >  	 */
> > -	if (depth == 1 && (strcmp(uname, "chosen") != 0))
> > -		return 1;
> > +	if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
> > +		if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
> > +			return 1;
> > +	}
> > +
> >  	return 0;
> >  }
> 
> As this is changing the semantic of an "empty" DT, we should consider
> now if there's anything else that might also need to exist in an "empty"
> DT. We don't want to change this again in future if we don't have to,
> given the compatiblity nightmare that's sure to result.
> 
> We should also consider if the "hypervisor" node name is sufficient (I
> think it is, but let's not assume anything).

>From Xen point of view I think it is enough: real hardware is described
in ACPI anyway and anything hypervisor related can be done via
hypercalls once Xen support is discovered, for which the hypervisor node
is sufficient.

Re: [PATCH v2 11/16] ARM64: ACPI: Check if it runs on Xen to enable or disable ACPI

[Shannon Zhao]

On 2016/1/18 23:07, Stefano Stabellini wrote:
> On Mon, 18 Jan 2016, Mark Rutland wrote:
>> > On Fri, Jan 15, 2016 at 02:55:24PM +0800, Shannon Zhao wrote:
>>> > > From: Shannon Zhao <shannon.zhao@linaro.org>
>>> > > 
>>> > > When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
>>> > > a /hypervisor node in DT. So check if it needs to enable ACPI.
>>> > > 
>>> > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>> > > ---
>>> > >  arch/arm64/kernel/acpi.c | 12 ++++++++----
>>> > >  1 file changed, 8 insertions(+), 4 deletions(-)
>>> > > 
>>> > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>>> > > index d1ce8e2..4e92be0 100644
>>> > > --- a/arch/arm64/kernel/acpi.c
>>> > > +++ b/arch/arm64/kernel/acpi.c
>>> > > @@ -67,10 +67,13 @@ static int __init dt_scan_depth1_nodes(unsigned long node,
>>> > >  {
>>> > >  	/*
>>> > >  	 * Return 1 as soon as we encounter a node at depth 1 that is
>>> > > -	 * not the /chosen node.
>>> > > +	 * not the /chosen node, or /hypervisor node when running on Xen.
>>> > >  	 */
>>> > > -	if (depth == 1 && (strcmp(uname, "chosen") != 0))
>>> > > -		return 1;
>>> > > +	if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
>>> > > +		if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
>>> > > +			return 1;
>>> > > +	}
>>> > > +
>>> > >  	return 0;
>>> > >  }
>> > 
>> > As this is changing the semantic of an "empty" DT, we should consider
>> > now if there's anything else that might also need to exist in an "empty"
>> > DT. We don't want to change this again in future if we don't have to,
>> > given the compatiblity nightmare that's sure to result.
>> > 
>> > We should also consider if the "hypervisor" node name is sufficient (I
>> > think it is, but let's not assume anything).
>>From Xen point of view I think it is enough: real hardware is described
> in ACPI anyway and anything hypervisor related can be done via
> hypercalls once Xen support is discovered, for which the hypervisor node
> is sufficient.

Yes, I think the hypervisor node is sufficient for current Xen ACPI support.

-- 
Shannon

[PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Add a "uefi" node under /hypervisor node in FDT, then Linux kernel could
scan this to get the UEFI information.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 Documentation/devicetree/bindings/arm/xen.txt | 42 +++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt
index 0f7b9c2..fbc17ae 100644
--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt
@@ -15,6 +15,36 @@ the following properties:
 - interrupts: the interrupt used by Xen to inject event notifications.
   A GIC node is also required.
 
+To support UEFI on Xen ARM virtual platforms, Xen pupulates the FDT "uefi" node
+under /hypervisor with following parameters:
+
+________________________________________________________________________________
+Name                      | Size   | Description
+================================================================================
+xen,uefi-system-table     | 64-bit | Guest physical address of the UEFI System
+			  |	   | Table.
+--------------------------------------------------------------------------------
+xen,uefi-mmap-start       | 64-bit | Guest physical address of the UEFI memory
+			  |	   | map.
+--------------------------------------------------------------------------------
+xen,uefi-mmap-size        | 32-bit | Size in bytes of the UEFI memory map
+                          |        | pointed to in previous entry.
+--------------------------------------------------------------------------------
+xen,uefi-mmap-desc-size   | 32-bit | Size in bytes of each entry in the UEFI
+                          |        | memory map.
+--------------------------------------------------------------------------------
+xen,uefi-mmap-desc-ver    | 32-bit | Version of the mmap descriptor format.
+--------------------------------------------------------------------------------
+
+Below is the format of the mmap descriptor.
+typedef struct {
+	u32 type;
+	u32 pad;
+	u64 phys_addr;
+	u64 virt_addr;
+	u64 num_pages;
+	u64 attribute;
+} efi_memory_desc_t;
 
 Example (assuming #address-cells = <2> and #size-cells = <2>):
 
@@ -22,4 +52,16 @@ hypervisor {
 	compatible = "xen,xen-4.3", "xen,xen";
 	reg = <0 0xb0000000 0 0x20000>;
 	interrupts = <1 15 0xf08>;
+	uefi {
+		xen,uefi-system-table = <0xXXXXXXXX>;
+		xen,uefi-mmap-start = <0xXXXXXXXX>;
+		xen,uefi-mmap-size = <0xXXXXXXXX>;
+		xen,uefi-mmap-desc-size = <0xXXXXXXXX>;
+		xen,uefi-mmap-desc-ver = <0xXXXXXXXX>;
+        };
 };
+
+These "xen,uefi-*" parameters are similar to those in Documentation/arm/uefi.txt
+which are used by normal UEFI. But to Xen ARM virtual platforms, it needs to
+introduce a Xen specific UEFI and it doesn't want to mix with normal UEFI.
+Therefore, it defines these parameters under /hypervisor node.
-- 
2.0.4

Re: [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Rob Herring]

On Fri, Jan 15, 2016 at 02:55:25PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Add a "uefi" node under /hypervisor node in FDT, then Linux kernel could
> scan this to get the UEFI information.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/xen.txt | 42 +++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Mark Rutland]

On Fri, Jan 15, 2016 at 02:55:25PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Add a "uefi" node under /hypervisor node in FDT, then Linux kernel could
> scan this to get the UEFI information.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/xen.txt | 42 +++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt
> index 0f7b9c2..fbc17ae 100644
> --- a/Documentation/devicetree/bindings/arm/xen.txt
> +++ b/Documentation/devicetree/bindings/arm/xen.txt
> @@ -15,6 +15,36 @@ the following properties:
>  - interrupts: the interrupt used by Xen to inject event notifications.
>    A GIC node is also required.
>  
> +To support UEFI on Xen ARM virtual platforms, Xen pupulates the FDT "uefi" node
> +under /hypervisor with following parameters:

s/pupulates/populates/

> +
> +________________________________________________________________________________
> +Name                      | Size   | Description
> +================================================================================
> +xen,uefi-system-table     | 64-bit | Guest physical address of the UEFI System
> +			  |	   | Table.
> +--------------------------------------------------------------------------------
> +xen,uefi-mmap-start       | 64-bit | Guest physical address of the UEFI memory
> +			  |	   | map.
> +--------------------------------------------------------------------------------
> +xen,uefi-mmap-size        | 32-bit | Size in bytes of the UEFI memory map
> +                          |        | pointed to in previous entry.
> +--------------------------------------------------------------------------------
> +xen,uefi-mmap-desc-size   | 32-bit | Size in bytes of each entry in the UEFI
> +                          |        | memory map.
> +--------------------------------------------------------------------------------
> +xen,uefi-mmap-desc-ver    | 32-bit | Version of the mmap descriptor format.
> +--------------------------------------------------------------------------------
> +
> +Below is the format of the mmap descriptor.
> +typedef struct {
> +	u32 type;
> +	u32 pad;
> +	u64 phys_addr;
> +	u64 virt_addr;
> +	u64 num_pages;
> +	u64 attribute;
> +} efi_memory_desc_t;

I don't think we should describe this here, as it duplicates the UEFI
spec, and is techincally incorrect the above is only guaranteed to be
the prefix of each memory descriptor -- that's why the
uefi-mmap-desc-size property exists.

We don't do this in Documentation/arm/uefi.txt, and I don't see why we
should do so here.

Does Xen handle arbitrary size memory map descriptors? I'm not sure what
new information might be passed in future additions to the descriptor
format, and I'm not sure what should happen in the Dom0 case.

>  Example (assuming #address-cells = <2> and #size-cells = <2>):
>  
> @@ -22,4 +52,16 @@ hypervisor {
>  	compatible = "xen,xen-4.3", "xen,xen";
>  	reg = <0 0xb0000000 0 0x20000>;
>  	interrupts = <1 15 0xf08>;
> +	uefi {
> +		xen,uefi-system-table = <0xXXXXXXXX>;
> +		xen,uefi-mmap-start = <0xXXXXXXXX>;
> +		xen,uefi-mmap-size = <0xXXXXXXXX>;
> +		xen,uefi-mmap-desc-size = <0xXXXXXXXX>;
> +		xen,uefi-mmap-desc-ver = <0xXXXXXXXX>;
> +        };
>  };
> +
> +These "xen,uefi-*" parameters are similar to those in Documentation/arm/uefi.txt
> +which are used by normal UEFI. But to Xen ARM virtual platforms, it needs to
> +introduce a Xen specific UEFI and it doesn't want to mix with normal UEFI.
> +Therefore, it defines these parameters under /hypervisor node.

Could we please describe what that actual difference is?

I know that the OS must handle a system table differently under Xen, but
this doesn't describe what it should do.

I assume that the OS can handle the memory map in an identical fashion
to when it is native. Is that true?

Mark.

Re: [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Stefano Stabellini]

On Mon, 18 Jan 2016, Mark Rutland wrote:
> On Fri, Jan 15, 2016 at 02:55:25PM +0800, Shannon Zhao wrote:
> > From: Shannon Zhao <shannon.zhao@linaro.org>
> > 
> > Add a "uefi" node under /hypervisor node in FDT, then Linux kernel could
> > scan this to get the UEFI information.
> > 
> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/arm/xen.txt | 42 +++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt
> > index 0f7b9c2..fbc17ae 100644
> > --- a/Documentation/devicetree/bindings/arm/xen.txt
> > +++ b/Documentation/devicetree/bindings/arm/xen.txt
> > @@ -15,6 +15,36 @@ the following properties:
> >  - interrupts: the interrupt used by Xen to inject event notifications.
> >    A GIC node is also required.
> >  
> > +To support UEFI on Xen ARM virtual platforms, Xen pupulates the FDT "uefi" node
> > +under /hypervisor with following parameters:
> 
> s/pupulates/populates/
> 
> > +
> > +________________________________________________________________________________
> > +Name                      | Size   | Description
> > +================================================================================
> > +xen,uefi-system-table     | 64-bit | Guest physical address of the UEFI System
> > +			  |	   | Table.
> > +--------------------------------------------------------------------------------
> > +xen,uefi-mmap-start       | 64-bit | Guest physical address of the UEFI memory
> > +			  |	   | map.
> > +--------------------------------------------------------------------------------
> > +xen,uefi-mmap-size        | 32-bit | Size in bytes of the UEFI memory map
> > +                          |        | pointed to in previous entry.
> > +--------------------------------------------------------------------------------
> > +xen,uefi-mmap-desc-size   | 32-bit | Size in bytes of each entry in the UEFI
> > +                          |        | memory map.
> > +--------------------------------------------------------------------------------
> > +xen,uefi-mmap-desc-ver    | 32-bit | Version of the mmap descriptor format.
> > +--------------------------------------------------------------------------------
> > +
> > +Below is the format of the mmap descriptor.
> > +typedef struct {
> > +	u32 type;
> > +	u32 pad;
> > +	u64 phys_addr;
> > +	u64 virt_addr;
> > +	u64 num_pages;
> > +	u64 attribute;
> > +} efi_memory_desc_t;
> 
> I don't think we should describe this here, as it duplicates the UEFI
> spec, and is techincally incorrect the above is only guaranteed to be
> the prefix of each memory descriptor -- that's why the
> uefi-mmap-desc-size property exists.
> 
> We don't do this in Documentation/arm/uefi.txt, and I don't see why we
> should do so here.
> 
> Does Xen handle arbitrary size memory map descriptors? I'm not sure what
> new information might be passed in future additions to the descriptor
> format, and I'm not sure what should happen in the Dom0 case.

Xen passes to Dom0 the memory map in the same format as the native
memory map.


> >  Example (assuming #address-cells = <2> and #size-cells = <2>):
> >  
> > @@ -22,4 +52,16 @@ hypervisor {
> >  	compatible = "xen,xen-4.3", "xen,xen";
> >  	reg = <0 0xb0000000 0 0x20000>;
> >  	interrupts = <1 15 0xf08>;
> > +	uefi {
> > +		xen,uefi-system-table = <0xXXXXXXXX>;
> > +		xen,uefi-mmap-start = <0xXXXXXXXX>;
> > +		xen,uefi-mmap-size = <0xXXXXXXXX>;
> > +		xen,uefi-mmap-desc-size = <0xXXXXXXXX>;
> > +		xen,uefi-mmap-desc-ver = <0xXXXXXXXX>;
> > +        };
> >  };
> > +
> > +These "xen,uefi-*" parameters are similar to those in Documentation/arm/uefi.txt
> > +which are used by normal UEFI. But to Xen ARM virtual platforms, it needs to
> > +introduce a Xen specific UEFI and it doesn't want to mix with normal UEFI.
> > +Therefore, it defines these parameters under /hypervisor node.
> 
> Could we please describe what that actual difference is?
> 
> I know that the OS must handle a system table differently under Xen, but
> this doesn't describe what it should do.

For a reference, the hypercall interface is described in not so many words here:

include/xen/interface/platform.h
http://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=xen/include/public/platform.h;hb=HEAD

However it is clear that platform.h also contains some x86 specific
calls, for example xenpf_set_processor_pminfo. It might be a good idea
to list the calls that are available on ARM64.


> I assume that the OS can handle the memory map in an identical fashion
> to when it is native. Is that true?
 
Yes, I think that's true.

Re: [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Shannon Zhao]

On 2016/1/19 1:34, Stefano Stabellini wrote:
> On Mon, 18 Jan 2016, Mark Rutland wrote:
>> On Fri, Jan 15, 2016 at 02:55:25PM +0800, Shannon Zhao wrote:
>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>
>>> Add a "uefi" node under /hypervisor node in FDT, then Linux kernel could
>>> scan this to get the UEFI information.
>>>
>>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>> ---
>>>  Documentation/devicetree/bindings/arm/xen.txt | 42 +++++++++++++++++++++++++++
>>>  1 file changed, 42 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt
>>> index 0f7b9c2..fbc17ae 100644
>>> --- a/Documentation/devicetree/bindings/arm/xen.txt
>>> +++ b/Documentation/devicetree/bindings/arm/xen.txt
>>> @@ -15,6 +15,36 @@ the following properties:
>>>  - interrupts: the interrupt used by Xen to inject event notifications.
>>>    A GIC node is also required.
>>>  
>>> +To support UEFI on Xen ARM virtual platforms, Xen pupulates the FDT "uefi" node
>>> +under /hypervisor with following parameters:
>>
>> s/pupulates/populates/
>>
>>> +
>>> +________________________________________________________________________________
>>> +Name                      | Size   | Description
>>> +================================================================================
>>> +xen,uefi-system-table     | 64-bit | Guest physical address of the UEFI System
>>> +			  |	   | Table.
>>> +--------------------------------------------------------------------------------
>>> +xen,uefi-mmap-start       | 64-bit | Guest physical address of the UEFI memory
>>> +			  |	   | map.
>>> +--------------------------------------------------------------------------------
>>> +xen,uefi-mmap-size        | 32-bit | Size in bytes of the UEFI memory map
>>> +                          |        | pointed to in previous entry.
>>> +--------------------------------------------------------------------------------
>>> +xen,uefi-mmap-desc-size   | 32-bit | Size in bytes of each entry in the UEFI
>>> +                          |        | memory map.
>>> +--------------------------------------------------------------------------------
>>> +xen,uefi-mmap-desc-ver    | 32-bit | Version of the mmap descriptor format.
>>> +--------------------------------------------------------------------------------
>>> +
>>> +Below is the format of the mmap descriptor.
>>> +typedef struct {
>>> +	u32 type;
>>> +	u32 pad;
>>> +	u64 phys_addr;
>>> +	u64 virt_addr;
>>> +	u64 num_pages;
>>> +	u64 attribute;
>>> +} efi_memory_desc_t;
>>
>> I don't think we should describe this here, as it duplicates the UEFI
>> spec, and is techincally incorrect the above is only guaranteed to be
>> the prefix of each memory descriptor -- that's why the
>> uefi-mmap-desc-size property exists.
>>
Oh, this format is suggested to describe here at previous patch set.

>> We don't do this in Documentation/arm/uefi.txt, and I don't see why we
>> should do so here.
>>
>> Does Xen handle arbitrary size memory map descriptors? I'm not sure what
>> new information might be passed in future additions to the descriptor
>> format, and I'm not sure what should happen in the Dom0 case.
> 
> Xen passes to Dom0 the memory map in the same format as the native
> memory map.
> 
> 
>>>  Example (assuming #address-cells = <2> and #size-cells = <2>):
>>>  
>>> @@ -22,4 +52,16 @@ hypervisor {
>>>  	compatible = "xen,xen-4.3", "xen,xen";
>>>  	reg = <0 0xb0000000 0 0x20000>;
>>>  	interrupts = <1 15 0xf08>;
>>> +	uefi {
>>> +		xen,uefi-system-table = <0xXXXXXXXX>;
>>> +		xen,uefi-mmap-start = <0xXXXXXXXX>;
>>> +		xen,uefi-mmap-size = <0xXXXXXXXX>;
>>> +		xen,uefi-mmap-desc-size = <0xXXXXXXXX>;
>>> +		xen,uefi-mmap-desc-ver = <0xXXXXXXXX>;
>>> +        };
>>>  };
>>> +
>>> +These "xen,uefi-*" parameters are similar to those in Documentation/arm/uefi.txt
>>> +which are used by normal UEFI. But to Xen ARM virtual platforms, it needs to
>>> +introduce a Xen specific UEFI and it doesn't want to mix with normal UEFI.
>>> +Therefore, it defines these parameters under /hypervisor node.
>>
>> Could we please describe what that actual difference is?
>>
>> I know that the OS must handle a system table differently under Xen, but
>> this doesn't describe what it should do.
> 
> For a reference, the hypercall interface is described in not so many words here:
> 
> include/xen/interface/platform.h
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=xen/include/public/platform.h;hb=HEAD
> 
> However it is clear that platform.h also contains some x86 specific
> calls, for example xenpf_set_processor_pminfo. It might be a good idea
> to list the calls that are available on ARM64.
> 
As Stefano said, I think the difference is that for Xen Dom0 the runtime
services are worked through hypercalls not the RuntimeServices under the
system table.

> 
>> I assume that the OS can handle the memory map in an identical fashion
>> to when it is native. Is that true?
>  
> Yes, I think that's true.
> 
> .
> 

-- 
Shannon

Re: [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Mark Rutland]

On Tue, Jan 19, 2016 at 06:25:25PM +0800, Shannon Zhao wrote:
> 
> 
> On 2016/1/19 1:34, Stefano Stabellini wrote:
> > On Mon, 18 Jan 2016, Mark Rutland wrote:
> >> On Fri, Jan 15, 2016 at 02:55:25PM +0800, Shannon Zhao wrote:
> >>> From: Shannon Zhao <shannon.zhao@linaro.org>
> >>>
> >>> Add a "uefi" node under /hypervisor node in FDT, then Linux kernel could
> >>> scan this to get the UEFI information.
> >>>
> >>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >>> ---
> >>>  Documentation/devicetree/bindings/arm/xen.txt | 42 +++++++++++++++++++++++++++
> >>>  1 file changed, 42 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt
> >>> index 0f7b9c2..fbc17ae 100644
> >>> --- a/Documentation/devicetree/bindings/arm/xen.txt
> >>> +++ b/Documentation/devicetree/bindings/arm/xen.txt
> >>> @@ -15,6 +15,36 @@ the following properties:
> >>>  - interrupts: the interrupt used by Xen to inject event notifications.
> >>>    A GIC node is also required.
> >>>  
> >>> +To support UEFI on Xen ARM virtual platforms, Xen pupulates the FDT "uefi" node
> >>> +under /hypervisor with following parameters:
> >>
> >> s/pupulates/populates/
> >>
> >>> +
> >>> +________________________________________________________________________________
> >>> +Name                      | Size   | Description
> >>> +================================================================================
> >>> +xen,uefi-system-table     | 64-bit | Guest physical address of the UEFI System
> >>> +			  |	   | Table.
> >>> +--------------------------------------------------------------------------------
> >>> +xen,uefi-mmap-start       | 64-bit | Guest physical address of the UEFI memory
> >>> +			  |	   | map.
> >>> +--------------------------------------------------------------------------------
> >>> +xen,uefi-mmap-size        | 32-bit | Size in bytes of the UEFI memory map
> >>> +                          |        | pointed to in previous entry.
> >>> +--------------------------------------------------------------------------------
> >>> +xen,uefi-mmap-desc-size   | 32-bit | Size in bytes of each entry in the UEFI
> >>> +                          |        | memory map.
> >>> +--------------------------------------------------------------------------------
> >>> +xen,uefi-mmap-desc-ver    | 32-bit | Version of the mmap descriptor format.
> >>> +--------------------------------------------------------------------------------
> >>> +
> >>> +Below is the format of the mmap descriptor.
> >>> +typedef struct {
> >>> +	u32 type;
> >>> +	u32 pad;
> >>> +	u64 phys_addr;
> >>> +	u64 virt_addr;
> >>> +	u64 num_pages;
> >>> +	u64 attribute;
> >>> +} efi_memory_desc_t;
> >>
> >> I don't think we should describe this here, as it duplicates the UEFI
> >> spec, and is techincally incorrect the above is only guaranteed to be
> >> the prefix of each memory descriptor -- that's why the
> >> uefi-mmap-desc-size property exists.
> >>
> Oh, this format is suggested to describe here at previous patch set.

We can describe it by referring to the definition in the UEFI
specification (i.e. state the properties represent the return values of
EFI_BOOT_SERVICES.GetMemoryMap()).

If that's necessary at all, fix that in the usual
Documentation/arm/uefi.txt, and state here that the format and meaning
of each property here follows its unprefixed cousin, with the caveat
that Xen-specific assumptions also apply (e.g. runtime services must be
indirected via hypercalls).

Anything else is redundant and risks being wrong.

> >> We don't do this in Documentation/arm/uefi.txt, and I don't see why we
> >> should do so here.
> >>
> >> Does Xen handle arbitrary size memory map descriptors? I'm not sure what
> >> new information might be passed in future additions to the descriptor
> >> format, and I'm not sure what should happen in the Dom0 case.
> > 
> > Xen passes to Dom0 the memory map in the same format as the native
> > memory map.

Does Xen parse or modify the EFI memory map in any way?

Does it pass the raw values returned by EFI_BOOT_SERVICES.GetMemoryMap()
through to the xen,uefi-* properties, or does is make any static
assumptions about what the values will be?

I'm trying to get a feeling for what the behaviour will be if/when a
version of the EFI spec expands the memory map format.

> >>>  Example (assuming #address-cells = <2> and #size-cells = <2>):
> >>>  
> >>> @@ -22,4 +52,16 @@ hypervisor {
> >>>  	compatible = "xen,xen-4.3", "xen,xen";
> >>>  	reg = <0 0xb0000000 0 0x20000>;
> >>>  	interrupts = <1 15 0xf08>;
> >>> +	uefi {
> >>> +		xen,uefi-system-table = <0xXXXXXXXX>;
> >>> +		xen,uefi-mmap-start = <0xXXXXXXXX>;
> >>> +		xen,uefi-mmap-size = <0xXXXXXXXX>;
> >>> +		xen,uefi-mmap-desc-size = <0xXXXXXXXX>;
> >>> +		xen,uefi-mmap-desc-ver = <0xXXXXXXXX>;
> >>> +        };
> >>>  };
> >>> +
> >>> +These "xen,uefi-*" parameters are similar to those in Documentation/arm/uefi.txt
> >>> +which are used by normal UEFI. But to Xen ARM virtual platforms, it needs to
> >>> +introduce a Xen specific UEFI and it doesn't want to mix with normal UEFI.
> >>> +Therefore, it defines these parameters under /hypervisor node.
> >>
> >> Could we please describe what that actual difference is?
> >>
> >> I know that the OS must handle a system table differently under Xen, but
> >> this doesn't describe what it should do.
> > 
> > For a reference, the hypercall interface is described in not so many words here:
> > 
> > include/xen/interface/platform.h
> > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=xen/include/public/platform.h;hb=HEAD
> > 
> > However it is clear that platform.h also contains some x86 specific
> > calls, for example xenpf_set_processor_pminfo. It might be a good idea
> > to list the calls that are available on ARM64.
> > 
> As Stefano said, I think the difference is that for Xen Dom0 the runtime
> services are worked through hypercalls not the RuntimeServices under the
> system table.

So just state that Xen-specific hypercalls are required for Dom0 to make
use of the runtime services.

That makes it clear what the major diffence is between the native case
and the Dom0 case, even if it's light on detail.

Thanks,
Mark.

Re: [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Stefano Stabellini]

On Tue, 19 Jan 2016, Mark Rutland wrote:
> On Tue, Jan 19, 2016 at 06:25:25PM +0800, Shannon Zhao wrote:
> > >> We don't do this in Documentation/arm/uefi.txt, and I don't see why we
> > >> should do so here.
> > >>
> > >> Does Xen handle arbitrary size memory map descriptors? I'm not sure what
> > >> new information might be passed in future additions to the descriptor
> > >> format, and I'm not sure what should happen in the Dom0 case.
> > > 
> > > Xen passes to Dom0 the memory map in the same format as the native
> > > memory map.
> 
> Does Xen parse or modify the EFI memory map in any way?

Xen:
- calls EFI_BOOT_SERVICES.GetMemoryMap()
- takes note of the memory regions for its own usage
- create the fdt notes, including efi-mmap-start, with a pointer to it


> Does it pass the raw values returned by EFI_BOOT_SERVICES.GetMemoryMap()
> through to the xen,uefi-* properties, or does is make any static
> assumptions about what the values will be?

It just passes the raw values.


> I'm trying to get a feeling for what the behaviour will be if/when a
> version of the EFI spec expands the memory map format.

Linux is likely to get the memory map in the same format as it's
provided on native.

Re: [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Mark Rutland]

On Tue, Jan 19, 2016 at 12:23:17PM +0000, Stefano Stabellini wrote:
> On Tue, 19 Jan 2016, Mark Rutland wrote:
> > On Tue, Jan 19, 2016 at 06:25:25PM +0800, Shannon Zhao wrote:
> > > >> We don't do this in Documentation/arm/uefi.txt, and I don't see why we
> > > >> should do so here.
> > > >>
> > > >> Does Xen handle arbitrary size memory map descriptors? I'm not sure what
> > > >> new information might be passed in future additions to the descriptor
> > > >> format, and I'm not sure what should happen in the Dom0 case.
> > > > 
> > > > Xen passes to Dom0 the memory map in the same format as the native
> > > > memory map.
> > 
> > Does Xen parse or modify the EFI memory map in any way?
> 
> Xen:
> - calls EFI_BOOT_SERVICES.GetMemoryMap()
> - takes note of the memory regions for its own usage
> - create the fdt notes, including efi-mmap-start, with a pointer to it
> 
> 
> > Does it pass the raw values returned by EFI_BOOT_SERVICES.GetMemoryMap()
> > through to the xen,uefi-* properties, or does is make any static
> > assumptions about what the values will be?
> 
> It just passes the raw values.

I take it that means that any memory carved out for Xen itself is
described/discovered via a separate mechanism? How does that work?

> > I'm trying to get a feeling for what the behaviour will be if/when a
> > version of the EFI spec expands the memory map format.
> 
> Linux is likely to get the memory map in the same format as it's
> provided on native.

Ok.

The only fragility I forsee there would be if some extended memory
descriptor format defined things the kernel understood but Xen did not.

Given that would require an update to the kernel, at that point code is
added for that we can figure out how to determine whether Xen handled
any of said information, and whether any further negotiation with Xen is
necessary. I assume Xen exposes some feature probing mechanism to guests
that can cater for that if necessary.

Ard, Leif, I trust you two can keep an eye out for anything of that
sort on the UEFI side. :)

Thanks,
Mark.

Re: [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Ard Biesheuvel]

On 19 January 2016 at 14:13, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jan 19, 2016 at 12:23:17PM +0000, Stefano Stabellini wrote:
>> On Tue, 19 Jan 2016, Mark Rutland wrote:
>> > On Tue, Jan 19, 2016 at 06:25:25PM +0800, Shannon Zhao wrote:
>> > > >> We don't do this in Documentation/arm/uefi.txt, and I don't see why we
>> > > >> should do so here.
>> > > >>
>> > > >> Does Xen handle arbitrary size memory map descriptors? I'm not sure what
>> > > >> new information might be passed in future additions to the descriptor
>> > > >> format, and I'm not sure what should happen in the Dom0 case.
>> > > >
>> > > > Xen passes to Dom0 the memory map in the same format as the native
>> > > > memory map.
>> >
>> > Does Xen parse or modify the EFI memory map in any way?
>>
>> Xen:
>> - calls EFI_BOOT_SERVICES.GetMemoryMap()
>> - takes note of the memory regions for its own usage
>> - create the fdt notes, including efi-mmap-start, with a pointer to it
>>
>>
>> > Does it pass the raw values returned by EFI_BOOT_SERVICES.GetMemoryMap()
>> > through to the xen,uefi-* properties, or does is make any static
>> > assumptions about what the values will be?
>>
>> It just passes the raw values.
>
> I take it that means that any memory carved out for Xen itself is
> described/discovered via a separate mechanism? How does that work?
>
>> > I'm trying to get a feeling for what the behaviour will be if/when a
>> > version of the EFI spec expands the memory map format.
>>
>> Linux is likely to get the memory map in the same format as it's
>> provided on native.
>
> Ok.
>
> The only fragility I forsee there would be if some extended memory
> descriptor format defined things the kernel understood but Xen did not.
>
> Given that would require an update to the kernel, at that point code is
> added for that we can figure out how to determine whether Xen handled
> any of said information, and whether any further negotiation with Xen is
> necessary. I assume Xen exposes some feature probing mechanism to guests
> that can cater for that if necessary.
>
> Ard, Leif, I trust you two can keep an eye out for anything of that
> sort on the UEFI side. :)
>

Yes. I think the likelihood that we will ever see an updated memory
descriptor format is negligible (we are still at version 1). I do know
that EDK2 deliberately uses a descriptor size that is larger than the
size of the struct in its implementation of GetMemoryMap(), so it
would appear that most existing OSes will be able to deal with it
(apart from being able to interpret the new fields) if it ever does
occur.

-- 
Ard.

Re: [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Shannon Zhao]

On 2016/1/19 21:13, Mark Rutland wrote:
> On Tue, Jan 19, 2016 at 12:23:17PM +0000, Stefano Stabellini wrote:
>> >On Tue, 19 Jan 2016, Mark Rutland wrote:
>>> > >On Tue, Jan 19, 2016 at 06:25:25PM +0800, Shannon Zhao wrote:
>>>>>> > > > >>We don't do this in Documentation/arm/uefi.txt, and I don't see why we
>>>>>> > > > >>should do so here.
>>>>>> > > > >>
>>>>>> > > > >>Does Xen handle arbitrary size memory map descriptors? I'm not sure what
>>>>>> > > > >>new information might be passed in future additions to the descriptor
>>>>>> > > > >>format, and I'm not sure what should happen in the Dom0 case.
>>>>> > > > >
>>>>> > > > >Xen passes to Dom0 the memory map in the same format as the native
>>>>> > > > >memory map.
>>> > >
>>> > >Does Xen parse or modify the EFI memory map in any way?
>> >
>> >Xen:
>> >- calls EFI_BOOT_SERVICES.GetMemoryMap()
>> >- takes note of the memory regions for its own usage
>> >- create the fdt notes, including efi-mmap-start, with a pointer to it
>> >
>> >
>>> > >Does it pass the raw values returned by EFI_BOOT_SERVICES.GetMemoryMap()
>>> > >through to the xen,uefi-* properties, or does is make any static
>>> > >assumptions about what the values will be?
>> >
>> >It just passes the raw values.
> I take it that means that any memory carved out for Xen itself is
> described/discovered via a separate mechanism? How does that work

For Xen hypervisor booting on UEFI, it get the EFI memory map through 
the similar way like Linux, e.g. call EFI_BOOT_SERVICES.GetMemoryMap().
For Dom0, Xen will create a new EFI memory map for Dom0.

See [PATCH v3 52/62] arm/acpi: Prepare EFI memory descriptor for Dom0
http://lists.xen.org/archives/html/xen-devel/2015-11/msg01884.html

Thanks,
-- 
Shannon

Re: [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Stefano Stabellini]

On Tue, 19 Jan 2016, Shannon Zhao wrote:
> On 2016/1/19 21:13, Mark Rutland wrote:
> > On Tue, Jan 19, 2016 at 12:23:17PM +0000, Stefano Stabellini wrote:
> > > >On Tue, 19 Jan 2016, Mark Rutland wrote:
> > > > > >On Tue, Jan 19, 2016 at 06:25:25PM +0800, Shannon Zhao wrote:
> > > > > > > > > > >>We don't do this in Documentation/arm/uefi.txt, and I
> > > > > > > don't see why we
> > > > > > > > > > >>should do so here.
> > > > > > > > > > >>
> > > > > > > > > > >>Does Xen handle arbitrary size memory map descriptors? I'm
> > > > > > > not sure what
> > > > > > > > > > >>new information might be passed in future additions to the
> > > > > > > descriptor
> > > > > > > > > > >>format, and I'm not sure what should happen in the Dom0
> > > > > > > case.
> > > > > > > > > >
> > > > > > > > > >Xen passes to Dom0 the memory map in the same format as the
> > > > > > native
> > > > > > > > > >memory map.
> > > > > >
> > > > > >Does Xen parse or modify the EFI memory map in any way?
> > > >
> > > >Xen:
> > > >- calls EFI_BOOT_SERVICES.GetMemoryMap()
> > > >- takes note of the memory regions for its own usage
> > > >- create the fdt notes, including efi-mmap-start, with a pointer to it
> > > >
> > > >
> > > > > >Does it pass the raw values returned by
> > > > EFI_BOOT_SERVICES.GetMemoryMap()
> > > > > >through to the xen,uefi-* properties, or does is make any static
> > > > > >assumptions about what the values will be?
> > > >
> > > >It just passes the raw values.
> > I take it that means that any memory carved out for Xen itself is
> > described/discovered via a separate mechanism? How does that work
> 
> For Xen hypervisor booting on UEFI, it get the EFI memory map through the
> similar way like Linux, e.g. call EFI_BOOT_SERVICES.GetMemoryMap().
> For Dom0, Xen will create a new EFI memory map for Dom0.
> 
> See [PATCH v3 52/62] arm/acpi: Prepare EFI memory descriptor for Dom0
> http://lists.xen.org/archives/html/xen-devel/2015-11/msg01884.html

Ah! That patch is not upstream yet, that's why I thought that something
was off. Listen to Shannon, don't listen to me :-)

Re: [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Mark Rutland]

On Tue, Jan 19, 2016 at 09:43:59PM +0800, Shannon Zhao wrote:
> 
> On 2016/1/19 21:13, Mark Rutland wrote:
> >On Tue, Jan 19, 2016 at 12:23:17PM +0000, Stefano Stabellini wrote:
> >>>On Tue, 19 Jan 2016, Mark Rutland wrote:
> >>>> >On Tue, Jan 19, 2016 at 06:25:25PM +0800, Shannon Zhao wrote:
> >>>>>>> > > >>We don't do this in Documentation/arm/uefi.txt, and I don't see why we
> >>>>>>> > > >>should do so here.
> >>>>>>> > > >>
> >>>>>>> > > >>Does Xen handle arbitrary size memory map descriptors? I'm not sure what
> >>>>>>> > > >>new information might be passed in future additions to the descriptor
> >>>>>>> > > >>format, and I'm not sure what should happen in the Dom0 case.
> >>>>>> > > >
> >>>>>> > > >Xen passes to Dom0 the memory map in the same format as the native
> >>>>>> > > >memory map.
> >>>> >
> >>>> >Does Xen parse or modify the EFI memory map in any way?
> >>>
> >>>Xen:
> >>>- calls EFI_BOOT_SERVICES.GetMemoryMap()
> >>>- takes note of the memory regions for its own usage
> >>>- create the fdt notes, including efi-mmap-start, with a pointer to it
> >>>
> >>>
> >>>> >Does it pass the raw values returned by EFI_BOOT_SERVICES.GetMemoryMap()
> >>>> >through to the xen,uefi-* properties, or does is make any static
> >>>> >assumptions about what the values will be?
> >>>
> >>>It just passes the raw values.
> >I take it that means that any memory carved out for Xen itself is
> >described/discovered via a separate mechanism? How does that work
> 
> For Xen hypervisor booting on UEFI, it get the EFI memory map
> through the similar way like Linux, e.g. call
> EFI_BOOT_SERVICES.GetMemoryMap().
> For Dom0, Xen will create a new EFI memory map for Dom0.
> 
> See [PATCH v3 52/62] arm/acpi: Prepare EFI memory descriptor for Dom0
> http://lists.xen.org/archives/html/xen-devel/2015-11/msg01884.html

Ok. So Xen will parse the EFI memory map, and will create a new memory
map to pass to the Dom0 kernel, presumably with some memory having been
carved out for Xen itself, and never described to Dom0.

So if there's any extension to that in future, Dom0 may see problems.
There's not much that can be done about that, however, and extensions to
the descriptors seem unlikely at present.

Thanks,
Mark.

[PATCH v2 13/16] ARM: XEN: Set EFI_PARAVIRT if Xen supports EFI

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Check if there is "uefi" node in the DT. If so, set EFI_PARAVIRT flag.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 arch/arm/xen/enlighten.c | 23 +++++++++++++++++++++++
 arch/arm64/kernel/efi.c  |  5 +++++
 2 files changed, 28 insertions(+)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 5d0fe68..485e117 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -31,6 +31,7 @@
 #include <linux/timekeeping.h>
 #include <linux/timekeeper_internal.h>
 #include <linux/acpi.h>
+#include <linux/efi.h>
 
 #include <linux/mm.h>
 
@@ -248,6 +249,19 @@ static int __init fdt_find_xen_node(unsigned long node, const char *uname,
 	return 0;
 }
 
+static int __init fdt_find_uefi_node(unsigned long node, const char *uname,
+				     int depth, void *data)
+{
+	bool *found = data;
+
+	if (depth != 2 || strcmp(uname, "uefi") != 0)
+		return 0;
+
+	*found = true;
+
+	return 1;
+}
+
 /*
  * see Documentation/devicetree/bindings/arm/xen.txt for the
  * documentation of the Xen Device Tree format.
@@ -255,6 +269,8 @@ static int __init fdt_find_xen_node(unsigned long node, const char *uname,
 #define GRANT_TABLE_PHYSADDR 0
 void __init xen_early_init(void)
 {
+	bool uefi_found = false;
+
 	of_scan_flat_dt(fdt_find_xen_node, NULL);
 	if (!xen_node.found) {
 		pr_debug("No Xen support\n");
@@ -279,6 +295,13 @@ void __init xen_early_init(void)
 
 	if (!console_set_on_cmdline && !xen_initial_domain())
 		add_preferred_console("hvc", 0, NULL);
+
+	if (IS_ENABLED(CONFIG_XEN_EFI)) {
+		/* Check if Xen support UEFI */
+		of_scan_flat_dt(fdt_find_uefi_node, &uefi_found);
+		if (uefi_found)
+			set_bit(EFI_PARAVIRT, &efi.flags);
+	}
 }
 
 static int __init xen_guest_init(void)
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 4eeb171..16c6b72 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -288,6 +288,11 @@ static int __init arm64_enable_runtime_services(void)
 		return 0;
 	}
 
+	if (efi_enabled(EFI_PARAVIRT)) {
+		pr_info("EFI runtime services access via paravirt.\n");
+		return -1;
+	}
+
 	pr_info("Remapping and enabling EFI services.\n");
 
 	mapsize = memmap.map_end - memmap.map;
-- 
2.0.4

Re: [PATCH v2 13/16] ARM: XEN: Set EFI_PARAVIRT if Xen supports EFI

[Mark Rutland]

On Fri, Jan 15, 2016 at 02:55:26PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Check if there is "uefi" node in the DT. If so, set EFI_PARAVIRT flag.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  arch/arm/xen/enlighten.c | 23 +++++++++++++++++++++++
>  arch/arm64/kernel/efi.c  |  5 +++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 5d0fe68..485e117 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -31,6 +31,7 @@
>  #include <linux/timekeeping.h>
>  #include <linux/timekeeper_internal.h>
>  #include <linux/acpi.h>
> +#include <linux/efi.h>
>  
>  #include <linux/mm.h>
>  
> @@ -248,6 +249,19 @@ static int __init fdt_find_xen_node(unsigned long node, const char *uname,
>  	return 0;
>  }
>  
> +static int __init fdt_find_uefi_node(unsigned long node, const char *uname,
> +				     int depth, void *data)
> +{
> +	bool *found = data;
> +
> +	if (depth != 2 || strcmp(uname, "uefi") != 0)
> +		return 0;
> +
> +	*found = true;
> +
> +	return 1;
> +}

I don't like this. What if we had to add a uefi node in the !Xen case
for some reason?

You want to look for /hypervisor/uefi, specifically when the hypervisor
compatible contains "xen,xen".

It would be better to find the "/hypervisor" node, checking for the
compatible string, then walk within that in the Xen-specific init
routine.

> +
>  /*
>   * see Documentation/devicetree/bindings/arm/xen.txt for the
>   * documentation of the Xen Device Tree format.
> @@ -255,6 +269,8 @@ static int __init fdt_find_xen_node(unsigned long node, const char *uname,
>  #define GRANT_TABLE_PHYSADDR 0
>  void __init xen_early_init(void)
>  {
> +	bool uefi_found = false;
> +
>  	of_scan_flat_dt(fdt_find_xen_node, NULL);
>  	if (!xen_node.found) {
>  		pr_debug("No Xen support\n");
> @@ -279,6 +295,13 @@ void __init xen_early_init(void)
>  
>  	if (!console_set_on_cmdline && !xen_initial_domain())
>  		add_preferred_console("hvc", 0, NULL);
> +
> +	if (IS_ENABLED(CONFIG_XEN_EFI)) {
> +		/* Check if Xen support UEFI */
> +		of_scan_flat_dt(fdt_find_uefi_node, &uefi_found);
> +		if (uefi_found)
> +			set_bit(EFI_PARAVIRT, &efi.flags);
> +	}
>  }

This alone is insufficient given that we haven't parsed the rest of the
/hypervisor/uefi properties. Is the kernel resilient such that this
patch alone will not result in a panic?

I think it would be best for this to be in the same patch as the rest of
the hypervisor UEFI property parsing, being unified with that.

Mark.

>  static int __init xen_guest_init(void)
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 4eeb171..16c6b72 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -288,6 +288,11 @@ static int __init arm64_enable_runtime_services(void)
>  		return 0;
>  	}
>  
> +	if (efi_enabled(EFI_PARAVIRT)) {
> +		pr_info("EFI runtime services access via paravirt.\n");
> +		return -1;
> +	}
> +
>  	pr_info("Remapping and enabling EFI services.\n");
>  
>  	mapsize = memmap.map_end - memmap.map;
> -- 
> 2.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v2 14/16] Xen: EFI: Parse DT parameters for Xen specific UEFI

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Add a new function to parse DT parameters for Xen specific UEFI just
like the way for normal UEFI. Then it could reuse the existing codes.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 drivers/firmware/efi/efi.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 027ca21..2dbc2ac 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -24,6 +24,7 @@
 #include <linux/of_fdt.h>
 #include <linux/io.h>
 #include <linux/platform_device.h>
+#include <xen/xen.h>
 
 struct efi __read_mostly efi = {
 	.mps			= EFI_INVALID_TABLE_ADDR,
@@ -498,12 +499,14 @@ device_initcall(efi_load_efivars);
 		FIELD_SIZEOF(struct efi_fdt_params, field) \
 	}
 
-static __initdata struct {
+struct params {
 	const char name[32];
 	const char propname[32];
 	int offset;
 	int size;
-} dt_params[] = {
+};
+
+static struct params fdt_params[] __initdata = {
 	UEFI_PARAM("System Table", "linux,uefi-system-table", system_table),
 	UEFI_PARAM("MemMap Address", "linux,uefi-mmap-start", mmap),
 	UEFI_PARAM("MemMap Size", "linux,uefi-mmap-size", mmap_size),
@@ -511,6 +514,14 @@ static __initdata struct {
 	UEFI_PARAM("MemMap Desc. Version", "linux,uefi-mmap-desc-ver", desc_ver)
 };
 
+static struct params xen_fdt_params[] __initdata = {
+	UEFI_PARAM("System Table", "xen,uefi-system-table", system_table),
+	UEFI_PARAM("MemMap Address", "xen,uefi-mmap-start", mmap),
+	UEFI_PARAM("MemMap Size", "xen,uefi-mmap-size", mmap_size),
+	UEFI_PARAM("MemMap Desc. Size", "xen,uefi-mmap-desc-size", desc_size),
+	UEFI_PARAM("MemMap Desc. Version", "xen,uefi-mmap-desc-ver", desc_ver)
+};
+
 struct param_info {
 	int found;
 	void *params;
@@ -520,15 +531,28 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
 				       int depth, void *data)
 {
 	struct param_info *info = data;
+	struct params *dt_params;
+	unsigned int size;
 	const void *prop;
 	void *dest;
 	u64 val;
 	int i, len;
 
-	if (depth != 1 || strcmp(uname, "chosen") != 0)
-		return 0;
+	if (efi_enabled(EFI_PARAVIRT)) {
+		if (depth != 2 || strcmp(uname, "uefi") != 0)
+			return 0;
 
-	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
+		dt_params = xen_fdt_params;
+		size = ARRAY_SIZE(xen_fdt_params);
+	} else {
+		if (depth != 1 || strcmp(uname, "chosen") != 0)
+			return 0;
+
+		dt_params = fdt_params;
+		size = ARRAY_SIZE(fdt_params);
+	}
+
+	for (i = 0; i < size; i++) {
 		prop = of_get_flat_dt_prop(node, dt_params[i].propname, &len);
 		if (!prop)
 			return 0;
@@ -552,6 +576,7 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
 int __init efi_get_fdt_params(struct efi_fdt_params *params)
 {
 	struct param_info info;
+	struct params *dt_params;
 	int ret;
 
 	pr_info("Getting EFI parameters from FDT:\n");
@@ -559,12 +584,18 @@ int __init efi_get_fdt_params(struct efi_fdt_params *params)
 	info.found = 0;
 	info.params = params;
 
+	if (efi_enabled(EFI_PARAVIRT))
+		dt_params = xen_fdt_params;
+	else
+		dt_params = fdt_params;
+
 	ret = of_scan_flat_dt(fdt_find_uefi_params, &info);
-	if (!info.found)
+	if (!info.found) {
 		pr_info("UEFI not found.\n");
-	else if (!ret)
+	} else if (!ret) {
 		pr_err("Can't find '%s' in device tree!\n",
 		       dt_params[info.found].name);
+	}
 
 	return ret;
 }
-- 
2.0.4

Re: [PATCH v2 14/16] Xen: EFI: Parse DT parameters for Xen specific UEFI

[Mark Rutland]

On Fri, Jan 15, 2016 at 02:55:27PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Add a new function to parse DT parameters for Xen specific UEFI just
> like the way for normal UEFI. Then it could reuse the existing codes.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  drivers/firmware/efi/efi.c | 45 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 7 deletions(-)

> @@ -520,15 +531,28 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
>  				       int depth, void *data)
>  {
>  	struct param_info *info = data;
> +	struct params *dt_params;
> +	unsigned int size;
>  	const void *prop;
>  	void *dest;
>  	u64 val;
>  	int i, len;
>  
> -	if (depth != 1 || strcmp(uname, "chosen") != 0)
> -		return 0;
> +	if (efi_enabled(EFI_PARAVIRT)) {
> +		if (depth != 2 || strcmp(uname, "uefi") != 0)
> +			return 0;

Please check the full path rather than the leaf node name alone.

Mark.

Re: [Xen-devel] [PATCH v2 14/16] Xen: EFI: Parse DT parameters for Xen specific UEFI

[Stefano Stabellini]

CC'ing Matt Fleming

On Fri, 15 Jan 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Add a new function to parse DT parameters for Xen specific UEFI just
> like the way for normal UEFI. Then it could reuse the existing codes.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>

Please CC the maintainers, use ./scripts/get_maintainer.pl to find out
who they are.


>  drivers/firmware/efi/efi.c | 45 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 027ca21..2dbc2ac 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -24,6 +24,7 @@
>  #include <linux/of_fdt.h>
>  #include <linux/io.h>
>  #include <linux/platform_device.h>
> +#include <xen/xen.h>
>  
>  struct efi __read_mostly efi = {
>  	.mps			= EFI_INVALID_TABLE_ADDR,
> @@ -498,12 +499,14 @@ device_initcall(efi_load_efivars);
>  		FIELD_SIZEOF(struct efi_fdt_params, field) \
>  	}
>  
> -static __initdata struct {
> +struct params {
>  	const char name[32];
>  	const char propname[32];
>  	int offset;
>  	int size;
> -} dt_params[] = {
> +};
> +
> +static struct params fdt_params[] __initdata = {
>  	UEFI_PARAM("System Table", "linux,uefi-system-table", system_table),
>  	UEFI_PARAM("MemMap Address", "linux,uefi-mmap-start", mmap),
>  	UEFI_PARAM("MemMap Size", "linux,uefi-mmap-size", mmap_size),
> @@ -511,6 +514,14 @@ static __initdata struct {
>  	UEFI_PARAM("MemMap Desc. Version", "linux,uefi-mmap-desc-ver", desc_ver)
>  };
>  
> +static struct params xen_fdt_params[] __initdata = {
> +	UEFI_PARAM("System Table", "xen,uefi-system-table", system_table),
> +	UEFI_PARAM("MemMap Address", "xen,uefi-mmap-start", mmap),
> +	UEFI_PARAM("MemMap Size", "xen,uefi-mmap-size", mmap_size),
> +	UEFI_PARAM("MemMap Desc. Size", "xen,uefi-mmap-desc-size", desc_size),
> +	UEFI_PARAM("MemMap Desc. Version", "xen,uefi-mmap-desc-ver", desc_ver)
> +};
> +
>  struct param_info {
>  	int found;
>  	void *params;
> @@ -520,15 +531,28 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
>  				       int depth, void *data)
>  {
>  	struct param_info *info = data;
> +	struct params *dt_params;
> +	unsigned int size;
>  	const void *prop;
>  	void *dest;
>  	u64 val;
>  	int i, len;
>  
> -	if (depth != 1 || strcmp(uname, "chosen") != 0)
> -		return 0;
> +	if (efi_enabled(EFI_PARAVIRT)) {
> +		if (depth != 2 || strcmp(uname, "uefi") != 0)

You are already introducing this check in the previous patch when
setting EFI_PARAVIRT, why do this again now?  But if we need to do this
check again, then, like Mark suggested, it should be done against the
full path.

Also you added below to efi_get_fdt_params:

	if (efi_enabled(EFI_PARAVIRT))
		dt_params = xen_fdt_params;
	else
		dt_params = fdt_params;

efi_get_fdt_params is the sole caller of fdt_find_uefi_params: it makes
sense to set dt_params only once, then pass it to fdt_find_uefi_params
as parameter.


> +			return 0;
>  
> -	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
> +		dt_params = xen_fdt_params;
> +		size = ARRAY_SIZE(xen_fdt_params);
> +	} else {
> +		if (depth != 1 || strcmp(uname, "chosen") != 0)
> +			return 0;
> +
> +		dt_params = fdt_params;
> +		size = ARRAY_SIZE(fdt_params);
> +	}
> +
> +	for (i = 0; i < size; i++) {
>  		prop = of_get_flat_dt_prop(node, dt_params[i].propname, &len);
>  		if (!prop)
>  			return 0;
> @@ -552,6 +576,7 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
>  int __init efi_get_fdt_params(struct efi_fdt_params *params)
>  {
>  	struct param_info info;
> +	struct params *dt_params;
>  	int ret;
>  
>  	pr_info("Getting EFI parameters from FDT:\n");
> @@ -559,12 +584,18 @@ int __init efi_get_fdt_params(struct efi_fdt_params *params)
>  	info.found = 0;
>  	info.params = params;
>  
> +	if (efi_enabled(EFI_PARAVIRT))
> +		dt_params = xen_fdt_params;
> +	else
> +		dt_params = fdt_params;
> +
>  	ret = of_scan_flat_dt(fdt_find_uefi_params, &info);
> -	if (!info.found)
> +	if (!info.found) {
>  		pr_info("UEFI not found.\n");
> -	else if (!ret)
> +	} else if (!ret) {
>  		pr_err("Can't find '%s' in device tree!\n",
>  		       dt_params[info.found].name);
> +	}
>  
>  	return ret;
>  }
> -- 
> 2.0.4
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [Xen-devel] [PATCH v2 14/16] Xen: EFI: Parse DT parameters for Xen specific UEFI

[Shannon Zhao]

On 2016/1/18 23:41, Stefano Stabellini wrote:
> CC'ing Matt Fleming
>
> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Add a new function to parse DT parameters for Xen specific UEFI just
>> like the way for normal UEFI. Then it could reuse the existing codes.
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>
> Please CC the maintainers, use ./scripts/get_maintainer.pl to find out
> who they are.
>
>
>>   drivers/firmware/efi/efi.c | 45 ++++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 38 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>> index 027ca21..2dbc2ac 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/of_fdt.h>
>>   #include <linux/io.h>
>>   #include <linux/platform_device.h>
>> +#include <xen/xen.h>
>>
>>   struct efi __read_mostly efi = {
>>   	.mps			= EFI_INVALID_TABLE_ADDR,
>> @@ -498,12 +499,14 @@ device_initcall(efi_load_efivars);
>>   		FIELD_SIZEOF(struct efi_fdt_params, field) \
>>   	}
>>
>> -static __initdata struct {
>> +struct params {
>>   	const char name[32];
>>   	const char propname[32];
>>   	int offset;
>>   	int size;
>> -} dt_params[] = {
>> +};
>> +
>> +static struct params fdt_params[] __initdata = {
>>   	UEFI_PARAM("System Table", "linux,uefi-system-table", system_table),
>>   	UEFI_PARAM("MemMap Address", "linux,uefi-mmap-start", mmap),
>>   	UEFI_PARAM("MemMap Size", "linux,uefi-mmap-size", mmap_size),
>> @@ -511,6 +514,14 @@ static __initdata struct {
>>   	UEFI_PARAM("MemMap Desc. Version", "linux,uefi-mmap-desc-ver", desc_ver)
>>   };
>>
>> +static struct params xen_fdt_params[] __initdata = {
>> +	UEFI_PARAM("System Table", "xen,uefi-system-table", system_table),
>> +	UEFI_PARAM("MemMap Address", "xen,uefi-mmap-start", mmap),
>> +	UEFI_PARAM("MemMap Size", "xen,uefi-mmap-size", mmap_size),
>> +	UEFI_PARAM("MemMap Desc. Size", "xen,uefi-mmap-desc-size", desc_size),
>> +	UEFI_PARAM("MemMap Desc. Version", "xen,uefi-mmap-desc-ver", desc_ver)
>> +};
>> +
>>   struct param_info {
>>   	int found;
>>   	void *params;
>> @@ -520,15 +531,28 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
>>   				       int depth, void *data)
>>   {
>>   	struct param_info *info = data;
>> +	struct params *dt_params;
>> +	unsigned int size;
>>   	const void *prop;
>>   	void *dest;
>>   	u64 val;
>>   	int i, len;
>>
>> -	if (depth != 1 || strcmp(uname, "chosen") != 0)
>> -		return 0;
>> +	if (efi_enabled(EFI_PARAVIRT)) {
>> +		if (depth != 2 || strcmp(uname, "uefi") != 0)
>
> You are already introducing this check in the previous patch when
> setting EFI_PARAVIRT, why do this again now?  But if we need to do this
> check again, then, like Mark suggested, it should be done against the
> full path.
>
This check just wants to confirm that the current node is the "uefi" 
node and we can parse it with xen_fdt_params now.

> Also you added below to efi_get_fdt_params:
>
> 	if (efi_enabled(EFI_PARAVIRT))
> 		dt_params = xen_fdt_params;
> 	else
> 		dt_params = fdt_params;
>
> efi_get_fdt_params is the sole caller of fdt_find_uefi_params: it makes
> sense to set dt_params only once, then pass it to fdt_find_uefi_params
> as parameter.
>
Sure.
>
>> +			return 0;
>>
>> -	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
>> +		dt_params = xen_fdt_params;
>> +		size = ARRAY_SIZE(xen_fdt_params);
>> +	} else {
>> +		if (depth != 1 || strcmp(uname, "chosen") != 0)
>> +			return 0;
>> +
>> +		dt_params = fdt_params;
>> +		size = ARRAY_SIZE(fdt_params);
>> +	}
>> +
>> +	for (i = 0; i < size; i++) {
>>   		prop = of_get_flat_dt_prop(node, dt_params[i].propname, &len);
>>   		if (!prop)
>>   			return 0;
>> @@ -552,6 +576,7 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
>>   int __init efi_get_fdt_params(struct efi_fdt_params *params)
>>   {
>>   	struct param_info info;
>> +	struct params *dt_params;
>>   	int ret;
>>
>>   	pr_info("Getting EFI parameters from FDT:\n");
>> @@ -559,12 +584,18 @@ int __init efi_get_fdt_params(struct efi_fdt_params *params)
>>   	info.found = 0;
>>   	info.params = params;
>>
>> +	if (efi_enabled(EFI_PARAVIRT))
>> +		dt_params = xen_fdt_params;
>> +	else
>> +		dt_params = fdt_params;
>> +
>>   	ret = of_scan_flat_dt(fdt_find_uefi_params, &info);
>> -	if (!info.found)
>> +	if (!info.found) {
>>   		pr_info("UEFI not found.\n");
>> -	else if (!ret)
>> +	} else if (!ret) {
>>   		pr_err("Can't find '%s' in device tree!\n",
>>   		       dt_params[info.found].name);
>> +	}
>>
>>   	return ret;
>>   }
>> --
>> 2.0.4
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>

-- 
Shannon

Re: [Xen-devel] [PATCH v2 14/16] Xen: EFI: Parse DT parameters for Xen specific UEFI

[Mark Rutland]

On Tue, Jan 19, 2016 at 09:19:05PM +0800, Shannon Zhao wrote:
> 
> 
> On 2016/1/18 23:41, Stefano Stabellini wrote:
> >CC'ing Matt Fleming
> >
> >On Fri, 15 Jan 2016, Shannon Zhao wrote:
> >>From: Shannon Zhao <shannon.zhao@linaro.org>
> >>@@ -520,15 +531,28 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
> >>  				       int depth, void *data)
> >>  {
> >>  	struct param_info *info = data;
> >>+	struct params *dt_params;
> >>+	unsigned int size;
> >>  	const void *prop;
> >>  	void *dest;
> >>  	u64 val;
> >>  	int i, len;
> >>
> >>-	if (depth != 1 || strcmp(uname, "chosen") != 0)
> >>-		return 0;
> >>+	if (efi_enabled(EFI_PARAVIRT)) {
> >>+		if (depth != 2 || strcmp(uname, "uefi") != 0)
> >
> >You are already introducing this check in the previous patch when
> >setting EFI_PARAVIRT, why do this again now?  But if we need to do this
> >check again, then, like Mark suggested, it should be done against the
> >full path.
> >
> This check just wants to confirm that the current node is the "uefi"
> node and we can parse it with xen_fdt_params now.

There is no single "uefi" node as many nodes can share that name. There
is at most a single, /hypervisor/uefi node, as that is qualified by a
full path.

Checking the leaf node name, as above, is insufficient. For example, the
below will be accepted:

* /chosen/uefi
* /foo/uefi
* /not-a-hypervisor/uefi

Any of these could exist in addition to a /hypervisor/uefi node, and
could appear ealier or later in the DTB.

There may be reasons to add such nodes in future, and regardless we
should not read properties from an invalid/wrong node.

Thanks,
Mark.

Re: [Xen-devel] [PATCH v2 14/16] Xen: EFI: Parse DT parameters for Xen specific UEFI

[Shannon Zhao]

On 2016/1/19 21:43, Mark Rutland wrote:
> On Tue, Jan 19, 2016 at 09:19:05PM +0800, Shannon Zhao wrote:
>>
>>
>> On 2016/1/18 23:41, Stefano Stabellini wrote:
>>> CC'ing Matt Fleming
>>>
>>> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>> @@ -520,15 +531,28 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
>>>>   				       int depth, void *data)
>>>>   {
>>>>   	struct param_info *info = data;
>>>> +	struct params *dt_params;
>>>> +	unsigned int size;
>>>>   	const void *prop;
>>>>   	void *dest;
>>>>   	u64 val;
>>>>   	int i, len;
>>>>
>>>> -	if (depth != 1 || strcmp(uname, "chosen") != 0)
>>>> -		return 0;
>>>> +	if (efi_enabled(EFI_PARAVIRT)) {
>>>> +		if (depth != 2 || strcmp(uname, "uefi") != 0)
>>>
>>> You are already introducing this check in the previous patch when
>>> setting EFI_PARAVIRT, why do this again now?  But if we need to do this
>>> check again, then, like Mark suggested, it should be done against the
>>> full path.
>>>
>> This check just wants to confirm that the current node is the "uefi"
>> node and we can parse it with xen_fdt_params now.
>
> There is no single "uefi" node as many nodes can share that name. There
> is at most a single, /hypervisor/uefi node, as that is qualified by a
> full path.
>
Sure, I will check it by full path.

> Checking the leaf node name, as above, is insufficient. For example, the
> below will be accepted:
>
> * /chosen/uefi
> * /foo/uefi
> * /not-a-hypervisor/uefi
>
> Any of these could exist in addition to a /hypervisor/uefi node, and
> could appear ealier or later in the DTB.
>
> There may be reasons to add such nodes in future, and regardless we
> should not read properties from an invalid/wrong node.
>
> Thanks,
> Mark.
>

-- 
Shannon

[PATCH v2 15/16] XEN: EFI: Move x86 specific codes to architecture directory

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Move x86 specific codes to architecture directory and export those EFI
runtime service functions. This will be useful for initializing runtime
service on ARM later.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 arch/x86/xen/efi.c    | 112 ++++++++++++++++++++++++++++++++
 drivers/xen/efi.c     | 174 ++++++++++----------------------------------------
 include/xen/xen-ops.h |  30 ++++++---
 3 files changed, 168 insertions(+), 148 deletions(-)

diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
index be14cc3..86527f1 100644
--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -20,10 +20,122 @@
 #include <linux/init.h>
 #include <linux/string.h>
 
+#include <xen/xen.h>
 #include <xen/xen-ops.h>
+#include <xen/interface/platform.h>
 
 #include <asm/page.h>
 #include <asm/setup.h>
+#include <asm/xen/hypercall.h>
+
+static efi_char16_t vendor[100] __initdata;
+
+static efi_system_table_t efi_systab_xen __initdata = {
+	.hdr = {
+		.signature	= EFI_SYSTEM_TABLE_SIGNATURE,
+		.revision	= 0, /* Initialized later. */
+		.headersize	= 0, /* Ignored by Linux Kernel. */
+		.crc32		= 0, /* Ignored by Linux Kernel. */
+		.reserved	= 0
+	},
+	.fw_vendor	= EFI_INVALID_TABLE_ADDR, /* Initialized later. */
+	.fw_revision	= 0,			  /* Initialized later. */
+	.con_in_handle	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.con_in		= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.con_out_handle	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.con_out	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.stderr_handle	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.stderr		= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.runtime	= (efi_runtime_services_t *)EFI_INVALID_TABLE_ADDR,
+						  /* Not used under Xen. */
+	.boottime	= (efi_boot_services_t *)EFI_INVALID_TABLE_ADDR,
+						  /* Not used under Xen. */
+	.nr_tables	= 0,			  /* Initialized later. */
+	.tables		= EFI_INVALID_TABLE_ADDR  /* Initialized later. */
+};
+
+static const struct efi efi_xen __initconst = {
+	.systab                   = NULL, /* Initialized later. */
+	.runtime_version	  = 0,    /* Initialized later. */
+	.mps                      = EFI_INVALID_TABLE_ADDR,
+	.acpi                     = EFI_INVALID_TABLE_ADDR,
+	.acpi20                   = EFI_INVALID_TABLE_ADDR,
+	.smbios                   = EFI_INVALID_TABLE_ADDR,
+	.smbios3                  = EFI_INVALID_TABLE_ADDR,
+	.sal_systab               = EFI_INVALID_TABLE_ADDR,
+	.boot_info                = EFI_INVALID_TABLE_ADDR,
+	.hcdp                     = EFI_INVALID_TABLE_ADDR,
+	.uga                      = EFI_INVALID_TABLE_ADDR,
+	.uv_systab                = EFI_INVALID_TABLE_ADDR,
+	.fw_vendor                = EFI_INVALID_TABLE_ADDR,
+	.runtime                  = EFI_INVALID_TABLE_ADDR,
+	.config_table             = EFI_INVALID_TABLE_ADDR,
+	.get_time                 = xen_efi_get_time,
+	.set_time                 = xen_efi_set_time,
+	.get_wakeup_time          = xen_efi_get_wakeup_time,
+	.set_wakeup_time          = xen_efi_set_wakeup_time,
+	.get_variable             = xen_efi_get_variable,
+	.get_next_variable        = xen_efi_get_next_variable,
+	.set_variable             = xen_efi_set_variable,
+	.query_variable_info      = xen_efi_query_variable_info,
+	.update_capsule           = xen_efi_update_capsule,
+	.query_capsule_caps       = xen_efi_query_capsule_caps,
+	.get_next_high_mono_count = xen_efi_get_next_high_mono_count,
+	.reset_system             = NULL, /* Functionality provided by Xen. */
+	.set_virtual_address_map  = NULL, /* Not used under Xen. */
+	.memmap                   = NULL, /* Not used under Xen. */
+	.flags			  = 0     /* Initialized later. */
+};
+
+static efi_system_table_t __init *xen_efi_probe(void)
+{
+	struct xen_platform_op op = {
+		.cmd = XENPF_firmware_info,
+		.u.firmware_info = {
+			.type = XEN_FW_EFI_INFO,
+			.index = XEN_FW_EFI_CONFIG_TABLE
+		}
+	};
+	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
+
+	if (!xen_initial_domain() || HYPERVISOR_platform_op(&op) < 0)
+		return NULL;
+
+	/* Here we know that Xen runs on EFI platform. */
+
+	efi = efi_xen;
+
+	efi_systab_xen.tables = info->cfg.addr;
+	efi_systab_xen.nr_tables = info->cfg.nent;
+
+	op.cmd = XENPF_firmware_info;
+	op.u.firmware_info.type = XEN_FW_EFI_INFO;
+	op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
+	info->vendor.bufsz = sizeof(vendor);
+	set_xen_guest_handle(info->vendor.name, vendor);
+
+	if (HYPERVISOR_platform_op(&op) == 0) {
+		efi_systab_xen.fw_vendor = __pa_symbol(vendor);
+		efi_systab_xen.fw_revision = info->vendor.revision;
+	} else
+		efi_systab_xen.fw_vendor = __pa_symbol(L"UNKNOWN");
+
+	op.cmd = XENPF_firmware_info;
+	op.u.firmware_info.type = XEN_FW_EFI_INFO;
+	op.u.firmware_info.index = XEN_FW_EFI_VERSION;
+
+	if (HYPERVISOR_platform_op(&op) == 0)
+		efi_systab_xen.hdr.revision = info->version;
+
+	op.cmd = XENPF_firmware_info;
+	op.u.firmware_info.type = XEN_FW_EFI_INFO;
+	op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
+
+	if (HYPERVISOR_platform_op(&op) == 0)
+		efi.runtime_version = info->version;
+
+	return &efi_systab_xen;
+}
 
 void __init xen_efi_init(void)
 {
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index be7e56a..22f71ff 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -38,7 +38,7 @@
 
 #define efi_data(op)	(op.u.efi_runtime_call)
 
-static efi_status_t xen_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
+efi_status_t xen_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 {
 	struct xen_platform_op op = INIT_EFI_OP(get_time);
 
@@ -59,8 +59,9 @@ static efi_status_t xen_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 
 	return efi_data(op).status;
 }
+EXPORT_SYMBOL_GPL(xen_efi_get_time);
 
-static efi_status_t xen_efi_set_time(efi_time_t *tm)
+efi_status_t xen_efi_set_time(efi_time_t *tm)
 {
 	struct xen_platform_op op = INIT_EFI_OP(set_time);
 
@@ -72,10 +73,10 @@ static efi_status_t xen_efi_set_time(efi_time_t *tm)
 
 	return efi_data(op).status;
 }
+EXPORT_SYMBOL_GPL(xen_efi_set_time);
 
-static efi_status_t xen_efi_get_wakeup_time(efi_bool_t *enabled,
-					    efi_bool_t *pending,
-					    efi_time_t *tm)
+efi_status_t xen_efi_get_wakeup_time(efi_bool_t *enabled, efi_bool_t *pending,
+				     efi_time_t *tm)
 {
 	struct xen_platform_op op = INIT_EFI_OP(get_wakeup_time);
 
@@ -95,8 +96,9 @@ static efi_status_t xen_efi_get_wakeup_time(efi_bool_t *enabled,
 
 	return efi_data(op).status;
 }
+EXPORT_SYMBOL_GPL(xen_efi_get_wakeup_time);
 
-static efi_status_t xen_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
+efi_status_t xen_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 {
 	struct xen_platform_op op = INIT_EFI_OP(set_wakeup_time);
 
@@ -113,12 +115,11 @@ static efi_status_t xen_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 
 	return efi_data(op).status;
 }
+EXPORT_SYMBOL_GPL(xen_efi_set_wakeup_time);
 
-static efi_status_t xen_efi_get_variable(efi_char16_t *name,
-					 efi_guid_t *vendor,
-					 u32 *attr,
-					 unsigned long *data_size,
-					 void *data)
+efi_status_t xen_efi_get_variable(efi_char16_t *name, efi_guid_t *vendor,
+				  u32 *attr, unsigned long *data_size,
+				  void *data)
 {
 	struct xen_platform_op op = INIT_EFI_OP(get_variable);
 
@@ -138,10 +139,11 @@ static efi_status_t xen_efi_get_variable(efi_char16_t *name,
 
 	return efi_data(op).status;
 }
+EXPORT_SYMBOL_GPL(xen_efi_get_variable);
 
-static efi_status_t xen_efi_get_next_variable(unsigned long *name_size,
-					      efi_char16_t *name,
-					      efi_guid_t *vendor)
+efi_status_t xen_efi_get_next_variable(unsigned long *name_size,
+				       efi_char16_t *name,
+				       efi_guid_t *vendor)
 {
 	struct xen_platform_op op = INIT_EFI_OP(get_next_variable_name);
 
@@ -161,12 +163,11 @@ static efi_status_t xen_efi_get_next_variable(unsigned long *name_size,
 
 	return efi_data(op).status;
 }
+EXPORT_SYMBOL_GPL(xen_efi_get_next_variable);
 
-static efi_status_t xen_efi_set_variable(efi_char16_t *name,
-					 efi_guid_t *vendor,
-					 u32 attr,
-					 unsigned long data_size,
-					 void *data)
+efi_status_t xen_efi_set_variable(efi_char16_t *name, efi_guid_t *vendor,
+				 u32 attr, unsigned long data_size,
+				 void *data)
 {
 	struct xen_platform_op op = INIT_EFI_OP(set_variable);
 
@@ -183,11 +184,11 @@ static efi_status_t xen_efi_set_variable(efi_char16_t *name,
 
 	return efi_data(op).status;
 }
+EXPORT_SYMBOL_GPL(xen_efi_set_variable);
 
-static efi_status_t xen_efi_query_variable_info(u32 attr,
-						u64 *storage_space,
-						u64 *remaining_space,
-						u64 *max_variable_size)
+efi_status_t xen_efi_query_variable_info(u32 attr, u64 *storage_space,
+					 u64 *remaining_space,
+					 u64 *max_variable_size)
 {
 	struct xen_platform_op op = INIT_EFI_OP(query_variable_info);
 
@@ -205,8 +206,9 @@ static efi_status_t xen_efi_query_variable_info(u32 attr,
 
 	return efi_data(op).status;
 }
+EXPORT_SYMBOL_GPL(xen_efi_query_variable_info);
 
-static efi_status_t xen_efi_get_next_high_mono_count(u32 *count)
+efi_status_t xen_efi_get_next_high_mono_count(u32 *count)
 {
 	struct xen_platform_op op = INIT_EFI_OP(get_next_high_monotonic_count);
 
@@ -217,10 +219,10 @@ static efi_status_t xen_efi_get_next_high_mono_count(u32 *count)
 
 	return efi_data(op).status;
 }
+EXPORT_SYMBOL_GPL(xen_efi_get_next_high_mono_count);
 
-static efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
-					   unsigned long count,
-					   unsigned long sg_list)
+efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
+				    unsigned long count, unsigned long sg_list)
 {
 	struct xen_platform_op op = INIT_EFI_OP(update_capsule);
 
@@ -237,11 +239,11 @@ static efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
 
 	return efi_data(op).status;
 }
+EXPORT_SYMBOL_GPL(xen_efi_update_capsule);
 
-static efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
-					       unsigned long count,
-					       u64 *max_size,
-					       int *reset_type)
+efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
+					unsigned long count, u64 *max_size,
+					int *reset_type)
 {
 	struct xen_platform_op op = INIT_EFI_OP(query_capsule_capabilities);
 
@@ -260,112 +262,4 @@ static efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 
 	return efi_data(op).status;
 }
-
-static efi_char16_t vendor[100] __initdata;
-
-static efi_system_table_t efi_systab_xen __initdata = {
-	.hdr = {
-		.signature	= EFI_SYSTEM_TABLE_SIGNATURE,
-		.revision	= 0, /* Initialized later. */
-		.headersize	= 0, /* Ignored by Linux Kernel. */
-		.crc32		= 0, /* Ignored by Linux Kernel. */
-		.reserved	= 0
-	},
-	.fw_vendor	= EFI_INVALID_TABLE_ADDR, /* Initialized later. */
-	.fw_revision	= 0,			  /* Initialized later. */
-	.con_in_handle	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
-	.con_in		= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
-	.con_out_handle	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
-	.con_out	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
-	.stderr_handle	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
-	.stderr		= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
-	.runtime	= (efi_runtime_services_t *)EFI_INVALID_TABLE_ADDR,
-						  /* Not used under Xen. */
-	.boottime	= (efi_boot_services_t *)EFI_INVALID_TABLE_ADDR,
-						  /* Not used under Xen. */
-	.nr_tables	= 0,			  /* Initialized later. */
-	.tables		= EFI_INVALID_TABLE_ADDR  /* Initialized later. */
-};
-
-static const struct efi efi_xen __initconst = {
-	.systab                   = NULL, /* Initialized later. */
-	.runtime_version	  = 0,    /* Initialized later. */
-	.mps                      = EFI_INVALID_TABLE_ADDR,
-	.acpi                     = EFI_INVALID_TABLE_ADDR,
-	.acpi20                   = EFI_INVALID_TABLE_ADDR,
-	.smbios                   = EFI_INVALID_TABLE_ADDR,
-	.smbios3                  = EFI_INVALID_TABLE_ADDR,
-	.sal_systab               = EFI_INVALID_TABLE_ADDR,
-	.boot_info                = EFI_INVALID_TABLE_ADDR,
-	.hcdp                     = EFI_INVALID_TABLE_ADDR,
-	.uga                      = EFI_INVALID_TABLE_ADDR,
-	.uv_systab                = EFI_INVALID_TABLE_ADDR,
-	.fw_vendor                = EFI_INVALID_TABLE_ADDR,
-	.runtime                  = EFI_INVALID_TABLE_ADDR,
-	.config_table             = EFI_INVALID_TABLE_ADDR,
-	.get_time                 = xen_efi_get_time,
-	.set_time                 = xen_efi_set_time,
-	.get_wakeup_time          = xen_efi_get_wakeup_time,
-	.set_wakeup_time          = xen_efi_set_wakeup_time,
-	.get_variable             = xen_efi_get_variable,
-	.get_next_variable        = xen_efi_get_next_variable,
-	.set_variable             = xen_efi_set_variable,
-	.query_variable_info      = xen_efi_query_variable_info,
-	.update_capsule           = xen_efi_update_capsule,
-	.query_capsule_caps       = xen_efi_query_capsule_caps,
-	.get_next_high_mono_count = xen_efi_get_next_high_mono_count,
-	.reset_system             = NULL, /* Functionality provided by Xen. */
-	.set_virtual_address_map  = NULL, /* Not used under Xen. */
-	.memmap                   = NULL, /* Not used under Xen. */
-	.flags			  = 0     /* Initialized later. */
-};
-
-efi_system_table_t __init *xen_efi_probe(void)
-{
-	struct xen_platform_op op = {
-		.cmd = XENPF_firmware_info,
-		.u.firmware_info = {
-			.type = XEN_FW_EFI_INFO,
-			.index = XEN_FW_EFI_CONFIG_TABLE
-		}
-	};
-	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
-
-	if (!xen_initial_domain() || HYPERVISOR_platform_op(&op) < 0)
-		return NULL;
-
-	/* Here we know that Xen runs on EFI platform. */
-
-	efi = efi_xen;
-
-	efi_systab_xen.tables = info->cfg.addr;
-	efi_systab_xen.nr_tables = info->cfg.nent;
-
-	op.cmd = XENPF_firmware_info;
-	op.u.firmware_info.type = XEN_FW_EFI_INFO;
-	op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
-	info->vendor.bufsz = sizeof(vendor);
-	set_xen_guest_handle(info->vendor.name, vendor);
-
-	if (HYPERVISOR_platform_op(&op) == 0) {
-		efi_systab_xen.fw_vendor = __pa_symbol(vendor);
-		efi_systab_xen.fw_revision = info->vendor.revision;
-	} else
-		efi_systab_xen.fw_vendor = __pa_symbol(L"UNKNOWN");
-
-	op.cmd = XENPF_firmware_info;
-	op.u.firmware_info.type = XEN_FW_EFI_INFO;
-	op.u.firmware_info.index = XEN_FW_EFI_VERSION;
-
-	if (HYPERVISOR_platform_op(&op) == 0)
-		efi_systab_xen.hdr.revision = info->version;
-
-	op.cmd = XENPF_firmware_info;
-	op.u.firmware_info.type = XEN_FW_EFI_INFO;
-	op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
-
-	if (HYPERVISOR_platform_op(&op) == 0)
-		efi.runtime_version = info->version;
-
-	return &efi_systab_xen;
-}
+EXPORT_SYMBOL_GPL(xen_efi_query_capsule_caps);
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 42be31a..c83a338 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -85,14 +85,28 @@ int xen_xlate_map_ballooned_pages(xen_pfn_t **pfns, void **vaddr,
 
 bool xen_running_on_version_or_later(unsigned int major, unsigned int minor);
 
-#ifdef CONFIG_XEN_EFI
-extern efi_system_table_t *xen_efi_probe(void);
-#else
-static inline efi_system_table_t __init *xen_efi_probe(void)
-{
-	return NULL;
-}
-#endif
+efi_status_t xen_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc);
+efi_status_t xen_efi_set_time(efi_time_t *tm);
+efi_status_t xen_efi_get_wakeup_time(efi_bool_t *enabled, efi_bool_t *pending,
+				     efi_time_t *tm);
+efi_status_t xen_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm);
+efi_status_t xen_efi_get_variable(efi_char16_t *name, efi_guid_t *vendor,
+				  u32 *attr, unsigned long *data_size,
+				  void *data);
+efi_status_t xen_efi_get_next_variable(unsigned long *name_size,
+				       efi_char16_t *name, efi_guid_t *vendor);
+efi_status_t xen_efi_set_variable(efi_char16_t *name, efi_guid_t *vendor,
+				  u32 attr, unsigned long data_size,
+				  void *data);
+efi_status_t xen_efi_query_variable_info(u32 attr, u64 *storage_space,
+					 u64 *remaining_space,
+					 u64 *max_variable_size);
+efi_status_t xen_efi_get_next_high_mono_count(u32 *count);
+efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
+				    unsigned long count, unsigned long sg_list);
+efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
+					unsigned long count, u64 *max_size,
+					int *reset_type);
 
 #ifdef CONFIG_PREEMPT
 
-- 
2.0.4

[PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

When running on Xen hypervisor, runtime services are supported through
hypercall. So call Xen specific function to initialize runtime services.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 arch/arm/xen/enlighten.c |  5 +++++
 arch/arm64/xen/Makefile  |  1 +
 arch/arm64/xen/efi.c     | 36 ++++++++++++++++++++++++++++++++++++
 drivers/xen/Kconfig      |  2 +-
 include/xen/xen-ops.h    |  1 +
 5 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/xen/efi.c

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 485e117..84f27ec 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -414,6 +414,11 @@ static int __init xen_guest_init(void)
 	if (xen_initial_domain())
 		pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
 
+	if (IS_ENABLED(CONFIG_XEN_EFI)) {
+		if (efi_enabled(EFI_PARAVIRT))
+			xen_efi_runtime_setup();
+	}
+
 	return 0;
 }
 early_initcall(xen_guest_init);
diff --git a/arch/arm64/xen/Makefile b/arch/arm64/xen/Makefile
index 74a8d87..62e6fe2 100644
--- a/arch/arm64/xen/Makefile
+++ b/arch/arm64/xen/Makefile
@@ -1,2 +1,3 @@
 xen-arm-y	+= $(addprefix ../../arm/xen/, enlighten.o grant-table.o p2m.o mm.o)
 obj-y		:= xen-arm.o hypercall.o
+obj-$(CONFIG_XEN_EFI) += efi.o
diff --git a/arch/arm64/xen/efi.c b/arch/arm64/xen/efi.c
new file mode 100644
index 0000000..33046b0
--- /dev/null
+++ b/arch/arm64/xen/efi.c
@@ -0,0 +1,36 @@
+/*
+ * Copyright (c) 2015, Linaro Limited, Shannon Zhao
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/efi.h>
+#include <xen/xen-ops.h>
+
+void __init xen_efi_runtime_setup(void)
+{
+	efi.get_time                 = xen_efi_get_time;
+	efi.set_time                 = xen_efi_set_time;
+	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
+	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
+	efi.get_variable             = xen_efi_get_variable;
+	efi.get_next_variable        = xen_efi_get_next_variable;
+	efi.set_variable             = xen_efi_set_variable;
+	efi.query_variable_info      = xen_efi_query_variable_info;
+	efi.update_capsule           = xen_efi_update_capsule;
+	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
+	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
+	efi.reset_system             = NULL;
+}
+EXPORT_SYMBOL_GPL(xen_efi_runtime_setup);
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 73708ac..27d216a 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -268,7 +268,7 @@ config XEN_HAVE_PVMMU
 
 config XEN_EFI
 	def_bool y
-	depends on X86_64 && EFI
+	depends on (ARM64 || X86_64) && EFI
 
 config XEN_AUTO_XLATE
 	def_bool y
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index c83a338..36ff8e4 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -107,6 +107,7 @@ efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
 efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 					unsigned long count, u64 *max_size,
 					int *reset_type);
+void xen_efi_runtime_setup(void);
 
 #ifdef CONFIG_PREEMPT
 
-- 
2.0.4

Re: [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Mark Rutland]

On Fri, Jan 15, 2016 at 02:55:29PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> When running on Xen hypervisor, runtime services are supported through
> hypercall. So call Xen specific function to initialize runtime services.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  arch/arm/xen/enlighten.c |  5 +++++
>  arch/arm64/xen/Makefile  |  1 +
>  arch/arm64/xen/efi.c     | 36 ++++++++++++++++++++++++++++++++++++
>  drivers/xen/Kconfig      |  2 +-
>  include/xen/xen-ops.h    |  1 +
>  5 files changed, 44 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/xen/efi.c
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 485e117..84f27ec 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -414,6 +414,11 @@ static int __init xen_guest_init(void)
>  	if (xen_initial_domain())
>  		pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
>  
> +	if (IS_ENABLED(CONFIG_XEN_EFI)) {
> +		if (efi_enabled(EFI_PARAVIRT))
> +			xen_efi_runtime_setup();
> +	}
> +
>  	return 0;
>  }
>  early_initcall(xen_guest_init);
> diff --git a/arch/arm64/xen/Makefile b/arch/arm64/xen/Makefile
> index 74a8d87..62e6fe2 100644
> --- a/arch/arm64/xen/Makefile
> +++ b/arch/arm64/xen/Makefile
> @@ -1,2 +1,3 @@
>  xen-arm-y	+= $(addprefix ../../arm/xen/, enlighten.o grant-table.o p2m.o mm.o)
>  obj-y		:= xen-arm.o hypercall.o
> +obj-$(CONFIG_XEN_EFI) += efi.o
> diff --git a/arch/arm64/xen/efi.c b/arch/arm64/xen/efi.c
> new file mode 100644
> index 0000000..33046b0
> --- /dev/null
> +++ b/arch/arm64/xen/efi.c
> @@ -0,0 +1,36 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited, Shannon Zhao
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/efi.h>
> +#include <xen/xen-ops.h>
> +
> +void __init xen_efi_runtime_setup(void)
> +{
> +	efi.get_time                 = xen_efi_get_time;
> +	efi.set_time                 = xen_efi_set_time;
> +	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
> +	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
> +	efi.get_variable             = xen_efi_get_variable;
> +	efi.get_next_variable        = xen_efi_get_next_variable;
> +	efi.set_variable             = xen_efi_set_variable;
> +	efi.query_variable_info      = xen_efi_query_variable_info;
> +	efi.update_capsule           = xen_efi_update_capsule;
> +	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
> +	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> +	efi.reset_system             = NULL;
> +}

How do capsules work in the absence of an EFI system reset?

Are there any other mandatory features that are missing in a
Xen-provided pseudo-EFI?

Mark.

> +EXPORT_SYMBOL_GPL(xen_efi_runtime_setup);
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index 73708ac..27d216a 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -268,7 +268,7 @@ config XEN_HAVE_PVMMU
>  
>  config XEN_EFI
>  	def_bool y
> -	depends on X86_64 && EFI
> +	depends on (ARM64 || X86_64) && EFI
>  
>  config XEN_AUTO_XLATE
>  	def_bool y
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index c83a338..36ff8e4 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -107,6 +107,7 @@ efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
>  efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
>  					unsigned long count, u64 *max_size,
>  					int *reset_type);
> +void xen_efi_runtime_setup(void);
>  
>  #ifdef CONFIG_PREEMPT
>  
> -- 
> 2.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Stefano Stabellini]

On Mon, 18 Jan 2016, Mark Rutland wrote:
> On Fri, Jan 15, 2016 at 02:55:29PM +0800, Shannon Zhao wrote:
> > From: Shannon Zhao <shannon.zhao@linaro.org>
> > 
> > When running on Xen hypervisor, runtime services are supported through
> > hypercall. So call Xen specific function to initialize runtime services.
> > 
> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > ---
> >  arch/arm/xen/enlighten.c |  5 +++++
> >  arch/arm64/xen/Makefile  |  1 +
> >  arch/arm64/xen/efi.c     | 36 ++++++++++++++++++++++++++++++++++++
> >  drivers/xen/Kconfig      |  2 +-
> >  include/xen/xen-ops.h    |  1 +
> >  5 files changed, 44 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/arm64/xen/efi.c
> > 
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index 485e117..84f27ec 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -414,6 +414,11 @@ static int __init xen_guest_init(void)
> >  	if (xen_initial_domain())
> >  		pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
> >  
> > +	if (IS_ENABLED(CONFIG_XEN_EFI)) {
> > +		if (efi_enabled(EFI_PARAVIRT))
> > +			xen_efi_runtime_setup();
> > +	}
> > +
> >  	return 0;
> >  }
> >  early_initcall(xen_guest_init);
> > diff --git a/arch/arm64/xen/Makefile b/arch/arm64/xen/Makefile
> > index 74a8d87..62e6fe2 100644
> > --- a/arch/arm64/xen/Makefile
> > +++ b/arch/arm64/xen/Makefile
> > @@ -1,2 +1,3 @@
> >  xen-arm-y	+= $(addprefix ../../arm/xen/, enlighten.o grant-table.o p2m.o mm.o)
> >  obj-y		:= xen-arm.o hypercall.o
> > +obj-$(CONFIG_XEN_EFI) += efi.o
> > diff --git a/arch/arm64/xen/efi.c b/arch/arm64/xen/efi.c
> > new file mode 100644
> > index 0000000..33046b0
> > --- /dev/null
> > +++ b/arch/arm64/xen/efi.c
> > @@ -0,0 +1,36 @@
> > +/*
> > + * Copyright (c) 2015, Linaro Limited, Shannon Zhao
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/efi.h>
> > +#include <xen/xen-ops.h>
> > +
> > +void __init xen_efi_runtime_setup(void)
> > +{
> > +	efi.get_time                 = xen_efi_get_time;
> > +	efi.set_time                 = xen_efi_set_time;
> > +	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
> > +	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
> > +	efi.get_variable             = xen_efi_get_variable;
> > +	efi.get_next_variable        = xen_efi_get_next_variable;
> > +	efi.set_variable             = xen_efi_set_variable;
> > +	efi.query_variable_info      = xen_efi_query_variable_info;
> > +	efi.update_capsule           = xen_efi_update_capsule;
> > +	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
> > +	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> > +	efi.reset_system             = NULL;
> > +}
> 
> How do capsules work in the absence of an EFI system reset?

Actually I don't think that capsules are available in Xen on ARM64 yet,
see "TODO - disabled until implemented on ARM" in
xen/common/efi/runtime.c.

FYI system reset is available, but it is provided via a different
mechanism (HYPERVISOR_sched_op(xen_restart...)


> Are there any other mandatory features that are missing in a
> Xen-provided pseudo-EFI?

Re: [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Mark Rutland]

On Mon, Jan 18, 2016 at 05:45:24PM +0000, Stefano Stabellini wrote:
> On Mon, 18 Jan 2016, Mark Rutland wrote:
> > On Fri, Jan 15, 2016 at 02:55:29PM +0800, Shannon Zhao wrote:
> > > +void __init xen_efi_runtime_setup(void)
> > > +{
> > > +	efi.get_time                 = xen_efi_get_time;
> > > +	efi.set_time                 = xen_efi_set_time;
> > > +	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
> > > +	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
> > > +	efi.get_variable             = xen_efi_get_variable;
> > > +	efi.get_next_variable        = xen_efi_get_next_variable;
> > > +	efi.set_variable             = xen_efi_set_variable;
> > > +	efi.query_variable_info      = xen_efi_query_variable_info;
> > > +	efi.update_capsule           = xen_efi_update_capsule;
> > > +	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
> > > +	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> > > +	efi.reset_system             = NULL;
> > > +}
> > 
> > How do capsules work in the absence of an EFI system reset?
> 
> Actually I don't think that capsules are available in Xen on ARM64 yet,
> see "TODO - disabled until implemented on ARM" in
> xen/common/efi/runtime.c.
> 
> FYI system reset is available, but it is provided via a different
> mechanism (HYPERVISOR_sched_op(xen_restart...)

Will that trigger Xen to do the right thing to trigger capsule updates
when implemented in Xen? Or do we need a xen_efi_reset_system?

Does that override PSCI?

In machine_restart we try efi_reboot first specifically to allow for
capsule updates. Similarly drivers/firmware/efi/reboot.c registers
efi_power_off late in order to override anything else, though that's
best-effort at present.

Thanks,
Mark.

Re: [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Stefano Stabellini]

On Mon, 18 Jan 2016, Mark Rutland wrote:
> On Mon, Jan 18, 2016 at 05:45:24PM +0000, Stefano Stabellini wrote:
> > On Mon, 18 Jan 2016, Mark Rutland wrote:
> > > On Fri, Jan 15, 2016 at 02:55:29PM +0800, Shannon Zhao wrote:
> > > > +void __init xen_efi_runtime_setup(void)
> > > > +{
> > > > +	efi.get_time                 = xen_efi_get_time;
> > > > +	efi.set_time                 = xen_efi_set_time;
> > > > +	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
> > > > +	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
> > > > +	efi.get_variable             = xen_efi_get_variable;
> > > > +	efi.get_next_variable        = xen_efi_get_next_variable;
> > > > +	efi.set_variable             = xen_efi_set_variable;
> > > > +	efi.query_variable_info      = xen_efi_query_variable_info;
> > > > +	efi.update_capsule           = xen_efi_update_capsule;
> > > > +	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
> > > > +	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> > > > +	efi.reset_system             = NULL;
> > > > +}
> > > 
> > > How do capsules work in the absence of an EFI system reset?
> > 
> > Actually I don't think that capsules are available in Xen on ARM64 yet,
> > see "TODO - disabled until implemented on ARM" in
> > xen/common/efi/runtime.c.
> > 
> > FYI system reset is available, but it is provided via a different
> > mechanism (HYPERVISOR_sched_op(xen_restart...)
> 
> Will that trigger Xen to do the right thing to trigger capsule updates
> when implemented in Xen? Or do we need a xen_efi_reset_system?

On ARM, to reboot the hardware, Xen calls the native PSCI system_reset
method. On x86, Xen calls efi_reset_system on EFI systems, and has
several fall backs if that doesn't work as expected (see
xen/arch/x86/shutdown.c:machine_restart).

But on a second look it doesn't look like that the capsule hypercalls
are implemented correctly even on x86 (there is an "XXX fall through for
now" comment in the code). I guess they are not available on Xen at all
unfortunately.


> Does that override PSCI?

It does not, HYPERVISOR_sched_op(xen_restart,) is in addition to it. It
ends up calling the same function within Xen as PSCI system_reset.


> In machine_restart we try efi_reboot first specifically to allow for
> capsule updates. Similarly drivers/firmware/efi/reboot.c registers
> efi_power_off late in order to override anything else, though that's
> best-effort at present.

That's very interesting. I think that Xen on ARM should follow what
Linux does and what Xen already does on x86 and try efi_reset_system
first on efi systems.

Re: [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Mark Rutland]

On Tue, Jan 19, 2016 at 12:03:59PM +0000, Stefano Stabellini wrote:
> On Mon, 18 Jan 2016, Mark Rutland wrote:
> > On Mon, Jan 18, 2016 at 05:45:24PM +0000, Stefano Stabellini wrote:
> > > On Mon, 18 Jan 2016, Mark Rutland wrote:
> > > > On Fri, Jan 15, 2016 at 02:55:29PM +0800, Shannon Zhao wrote:
> > > > > +void __init xen_efi_runtime_setup(void)
> > > > > +{
> > > > > +	efi.get_time                 = xen_efi_get_time;
> > > > > +	efi.set_time                 = xen_efi_set_time;
> > > > > +	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
> > > > > +	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
> > > > > +	efi.get_variable             = xen_efi_get_variable;
> > > > > +	efi.get_next_variable        = xen_efi_get_next_variable;
> > > > > +	efi.set_variable             = xen_efi_set_variable;
> > > > > +	efi.query_variable_info      = xen_efi_query_variable_info;
> > > > > +	efi.update_capsule           = xen_efi_update_capsule;
> > > > > +	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
> > > > > +	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> > > > > +	efi.reset_system             = NULL;
> > > > > +}
> > > > 
> > > > How do capsules work in the absence of an EFI system reset?
> > > 
> > > Actually I don't think that capsules are available in Xen on ARM64 yet,
> > > see "TODO - disabled until implemented on ARM" in
> > > xen/common/efi/runtime.c.
> > > 
> > > FYI system reset is available, but it is provided via a different
> > > mechanism (HYPERVISOR_sched_op(xen_restart...)
> > 
> > Will that trigger Xen to do the right thing to trigger capsule updates
> > when implemented in Xen? Or do we need a xen_efi_reset_system?
> 
> On ARM, to reboot the hardware, Xen calls the native PSCI system_reset
> method. On x86, Xen calls efi_reset_system on EFI systems, and has
> several fall backs if that doesn't work as expected (see
> xen/arch/x86/shutdown.c:machine_restart).
> 
> But on a second look it doesn't look like that the capsule hypercalls
> are implemented correctly even on x86 (there is an "XXX fall through for
> now" comment in the code). I guess they are not available on Xen at all
> unfortunately.

That is incredibly unfortunate. It effectively renders the firmware
non-updateable when using Xen.

> > Does that override PSCI?
> 
> It does not, HYPERVISOR_sched_op(xen_restart,) is in addition to it. It
> ends up calling the same function within Xen as PSCI system_reset.

I meant within Dom0.

Presumably Dom0 calls HYPERVISOR_sched_op(xen_restart,), and doesn't
ever call PSCI SYSTEM_RESET?

> > In machine_restart we try efi_reboot first specifically to allow for
> > capsule updates. Similarly drivers/firmware/efi/reboot.c registers
> > efi_power_off late in order to override anything else, though that's
> > best-effort at present.
> 
> That's very interesting. I think that Xen on ARM should follow what
> Linux does and what Xen already does on x86 and try efi_reset_system
> first on efi systems.

I would agree.

Thanks,
Mark.

Re: [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Shannon Zhao]

On 2016/1/19 21:03, Mark Rutland wrote:
> On Tue, Jan 19, 2016 at 12:03:59PM +0000, Stefano Stabellini wrote:
>> On Mon, 18 Jan 2016, Mark Rutland wrote:
>>> On Mon, Jan 18, 2016 at 05:45:24PM +0000, Stefano Stabellini wrote:
>>>> On Mon, 18 Jan 2016, Mark Rutland wrote:
>>>>> On Fri, Jan 15, 2016 at 02:55:29PM +0800, Shannon Zhao wrote:
>>>>>> +void __init xen_efi_runtime_setup(void)
>>>>>> +{
>>>>>> +	efi.get_time                 = xen_efi_get_time;
>>>>>> +	efi.set_time                 = xen_efi_set_time;
>>>>>> +	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
>>>>>> +	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
>>>>>> +	efi.get_variable             = xen_efi_get_variable;
>>>>>> +	efi.get_next_variable        = xen_efi_get_next_variable;
>>>>>> +	efi.set_variable             = xen_efi_set_variable;
>>>>>> +	efi.query_variable_info      = xen_efi_query_variable_info;
>>>>>> +	efi.update_capsule           = xen_efi_update_capsule;
>>>>>> +	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
>>>>>> +	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
>>>>>> +	efi.reset_system             = NULL;
>>>>>> +}
>>>>>
>>>>> How do capsules work in the absence of an EFI system reset?
>>>>
>>>> Actually I don't think that capsules are available in Xen on ARM64 yet,
>>>> see "TODO - disabled until implemented on ARM" in
>>>> xen/common/efi/runtime.c.
>>>>
>>>> FYI system reset is available, but it is provided via a different
>>>> mechanism (HYPERVISOR_sched_op(xen_restart...)
>>>
>>> Will that trigger Xen to do the right thing to trigger capsule updates
>>> when implemented in Xen? Or do we need a xen_efi_reset_system?
>>
>> On ARM, to reboot the hardware, Xen calls the native PSCI system_reset
>> method. On x86, Xen calls efi_reset_system on EFI systems, and has
>> several fall backs if that doesn't work as expected (see
>> xen/arch/x86/shutdown.c:machine_restart).
>>
>> But on a second look it doesn't look like that the capsule hypercalls
>> are implemented correctly even on x86 (there is an "XXX fall through for
>> now" comment in the code). I guess they are not available on Xen at all
>> unfortunately.
>
> That is incredibly unfortunate. It effectively renders the firmware
> non-updateable when using Xen.
>
>>> Does that override PSCI?
>>
>> It does not, HYPERVISOR_sched_op(xen_restart,) is in addition to it. It
>> ends up calling the same function within Xen as PSCI system_reset.
>
> I meant within Dom0.
>
> Presumably Dom0 calls HYPERVISOR_sched_op(xen_restart,), and doesn't
> ever call PSCI SYSTEM_RESET?
>
I think executing reset in Dom0 will reset not only Dom0 but also the 
Xen hypervisor, right?

>>> In machine_restart we try efi_reboot first specifically to allow for
>>> capsule updates. Similarly drivers/firmware/efi/reboot.c registers
>>> efi_power_off late in order to override anything else, though that's
>>> best-effort at present.
>>
>> That's very interesting. I think that Xen on ARM should follow what
>> Linux does and what Xen already does on x86 and try efi_reset_system
>> first on efi systems.
>
> I would agree.
>
> Thanks,
> Mark.
>

-- 
Shannon

Re: [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Stefano Stabellini]

On Tue, 19 Jan 2016, Shannon Zhao wrote:
> On 2016/1/19 21:03, Mark Rutland wrote:
> > On Tue, Jan 19, 2016 at 12:03:59PM +0000, Stefano Stabellini wrote:
> > > On Mon, 18 Jan 2016, Mark Rutland wrote:
> > > > On Mon, Jan 18, 2016 at 05:45:24PM +0000, Stefano Stabellini wrote:
> > > > > On Mon, 18 Jan 2016, Mark Rutland wrote:
> > > > > > On Fri, Jan 15, 2016 at 02:55:29PM +0800, Shannon Zhao wrote:
> > > > > > > +void __init xen_efi_runtime_setup(void)
> > > > > > > +{
> > > > > > > +	efi.get_time                 = xen_efi_get_time;
> > > > > > > +	efi.set_time                 = xen_efi_set_time;
> > > > > > > +	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
> > > > > > > +	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
> > > > > > > +	efi.get_variable             = xen_efi_get_variable;
> > > > > > > +	efi.get_next_variable        = xen_efi_get_next_variable;
> > > > > > > +	efi.set_variable             = xen_efi_set_variable;
> > > > > > > +	efi.query_variable_info      = xen_efi_query_variable_info;
> > > > > > > +	efi.update_capsule           = xen_efi_update_capsule;
> > > > > > > +	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
> > > > > > > +	efi.get_next_high_mono_count =
> > > > > > > xen_efi_get_next_high_mono_count;
> > > > > > > +	efi.reset_system             = NULL;
> > > > > > > +}
> > > > > > 
> > > > > > How do capsules work in the absence of an EFI system reset?
> > > > > 
> > > > > Actually I don't think that capsules are available in Xen on ARM64
> > > > > yet,
> > > > > see "TODO - disabled until implemented on ARM" in
> > > > > xen/common/efi/runtime.c.
> > > > > 
> > > > > FYI system reset is available, but it is provided via a different
> > > > > mechanism (HYPERVISOR_sched_op(xen_restart...)
> > > > 
> > > > Will that trigger Xen to do the right thing to trigger capsule updates
> > > > when implemented in Xen? Or do we need a xen_efi_reset_system?
> > > 
> > > On ARM, to reboot the hardware, Xen calls the native PSCI system_reset
> > > method. On x86, Xen calls efi_reset_system on EFI systems, and has
> > > several fall backs if that doesn't work as expected (see
> > > xen/arch/x86/shutdown.c:machine_restart).
> > > 
> > > But on a second look it doesn't look like that the capsule hypercalls
> > > are implemented correctly even on x86 (there is an "XXX fall through for
> > > now" comment in the code). I guess they are not available on Xen at all
> > > unfortunately.
> > 
> > That is incredibly unfortunate. It effectively renders the firmware
> > non-updateable when using Xen.
> > 
> > > > Does that override PSCI?
> > > 
> > > It does not, HYPERVISOR_sched_op(xen_restart,) is in addition to it. It
> > > ends up calling the same function within Xen as PSCI system_reset.
> > 
> > I meant within Dom0.
> > 
> > Presumably Dom0 calls HYPERVISOR_sched_op(xen_restart,), and doesn't
> > ever call PSCI SYSTEM_RESET?
> > 
> I think executing reset in Dom0 will reset not only Dom0 but also the Xen
> hypervisor, right?

Dom0 and DomUs call an HYPERVISOR_sched_op for machine reboot and
shutdown (by setting pm_power_off and arm_pm_restart), but a virtual
PSCI interface is also available and can be used. In the case of DomUs
the virtul machine is rebooted or shut down, in the case of Dom0, the
physical machine is rebooted or shut down.

The native PSCI methods are never exposed to Dom0 (or any DomUs).

Re: [Xen-devel] [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Stefano Stabellini]

On Fri, 15 Jan 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> When running on Xen hypervisor, runtime services are supported through
> hypercall. So call Xen specific function to initialize runtime services.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>

Thanks Shannon, much much better!  Just a couple of questions.


>  arch/arm/xen/enlighten.c |  5 +++++
>  arch/arm64/xen/Makefile  |  1 +
>  arch/arm64/xen/efi.c     | 36 ++++++++++++++++++++++++++++++++++++
>  drivers/xen/Kconfig      |  2 +-
>  include/xen/xen-ops.h    |  1 +
>  5 files changed, 44 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/xen/efi.c
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 485e117..84f27ec 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -414,6 +414,11 @@ static int __init xen_guest_init(void)
>  	if (xen_initial_domain())
>  		pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
>  
> +	if (IS_ENABLED(CONFIG_XEN_EFI)) {
> +		if (efi_enabled(EFI_PARAVIRT))
> +			xen_efi_runtime_setup();
> +	}
> +
>  	return 0;
>  }
>  early_initcall(xen_guest_init);
> diff --git a/arch/arm64/xen/Makefile b/arch/arm64/xen/Makefile
> index 74a8d87..62e6fe2 100644
> --- a/arch/arm64/xen/Makefile
> +++ b/arch/arm64/xen/Makefile
> @@ -1,2 +1,3 @@
>  xen-arm-y	+= $(addprefix ../../arm/xen/, enlighten.o grant-table.o p2m.o mm.o)
>  obj-y		:= xen-arm.o hypercall.o
> +obj-$(CONFIG_XEN_EFI) += efi.o
> diff --git a/arch/arm64/xen/efi.c b/arch/arm64/xen/efi.c
> new file mode 100644
> index 0000000..33046b0
> --- /dev/null
> +++ b/arch/arm64/xen/efi.c
> @@ -0,0 +1,36 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited, Shannon Zhao
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/efi.h>
> +#include <xen/xen-ops.h>
> +
> +void __init xen_efi_runtime_setup(void)
> +{
> +	efi.get_time                 = xen_efi_get_time;
> +	efi.set_time                 = xen_efi_set_time;
> +	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
> +	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
> +	efi.get_variable             = xen_efi_get_variable;
> +	efi.get_next_variable        = xen_efi_get_next_variable;
> +	efi.set_variable             = xen_efi_set_variable;
> +	efi.query_variable_info      = xen_efi_query_variable_info;
> +	efi.update_capsule           = xen_efi_update_capsule;
> +	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
> +	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> +	efi.reset_system             = NULL;
> +}
> +EXPORT_SYMBOL_GPL(xen_efi_runtime_setup);

This looks very similar to struct efi efi_xen previously in
drivers/xen/efi.c.  Maybe it makes sense to leave struct efi efi_xen in
drivers/xen/efi.c, export it in include/xen/xen-ops.h, then here just:

  efi = efi_xen;

Would that improve code readability?


Correct me if I am wrong, but on ARM64 (differently from x86) it is not
necessary to set efi.systab because it is not used, right? If so, it
would be best to add a comment here to remember.


> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index 73708ac..27d216a 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -268,7 +268,7 @@ config XEN_HAVE_PVMMU
>  
>  config XEN_EFI
>  	def_bool y
> -	depends on X86_64 && EFI
> +	depends on (ARM64 || X86_64) && EFI
>  
>  config XEN_AUTO_XLATE
>  	def_bool y
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index c83a338..36ff8e4 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -107,6 +107,7 @@ efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
>  efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
>  					unsigned long count, u64 *max_size,
>  					int *reset_type);
> +void xen_efi_runtime_setup(void);

xen_efi_runtime_setup is not defined on x86, but this header is not arch
specific.

Re: [Xen-devel] [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Shannon Zhao]

On 2016/1/19 1:03, Stefano Stabellini wrote:
> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> When running on Xen hypervisor, runtime services are supported through
>> hypercall. So call Xen specific function to initialize runtime services.
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>
> Thanks Shannon, much much better!  Just a couple of questions.
>
>
>>   arch/arm/xen/enlighten.c |  5 +++++
>>   arch/arm64/xen/Makefile  |  1 +
>>   arch/arm64/xen/efi.c     | 36 ++++++++++++++++++++++++++++++++++++
>>   drivers/xen/Kconfig      |  2 +-
>>   include/xen/xen-ops.h    |  1 +
>>   5 files changed, 44 insertions(+), 1 deletion(-)
>>   create mode 100644 arch/arm64/xen/efi.c
>>
>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> index 485e117..84f27ec 100644
>> --- a/arch/arm/xen/enlighten.c
>> +++ b/arch/arm/xen/enlighten.c
>> @@ -414,6 +414,11 @@ static int __init xen_guest_init(void)
>>   	if (xen_initial_domain())
>>   		pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
>>
>> +	if (IS_ENABLED(CONFIG_XEN_EFI)) {
>> +		if (efi_enabled(EFI_PARAVIRT))
>> +			xen_efi_runtime_setup();
>> +	}
>> +
>>   	return 0;
>>   }
>>   early_initcall(xen_guest_init);
>> diff --git a/arch/arm64/xen/Makefile b/arch/arm64/xen/Makefile
>> index 74a8d87..62e6fe2 100644
>> --- a/arch/arm64/xen/Makefile
>> +++ b/arch/arm64/xen/Makefile
>> @@ -1,2 +1,3 @@
>>   xen-arm-y	+= $(addprefix ../../arm/xen/, enlighten.o grant-table.o p2m.o mm.o)
>>   obj-y		:= xen-arm.o hypercall.o
>> +obj-$(CONFIG_XEN_EFI) += efi.o
>> diff --git a/arch/arm64/xen/efi.c b/arch/arm64/xen/efi.c
>> new file mode 100644
>> index 0000000..33046b0
>> --- /dev/null
>> +++ b/arch/arm64/xen/efi.c
>> @@ -0,0 +1,36 @@
>> +/*
>> + * Copyright (c) 2015, Linaro Limited, Shannon Zhao
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/efi.h>
>> +#include <xen/xen-ops.h>
>> +
>> +void __init xen_efi_runtime_setup(void)
>> +{
>> +	efi.get_time                 = xen_efi_get_time;
>> +	efi.set_time                 = xen_efi_set_time;
>> +	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
>> +	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
>> +	efi.get_variable             = xen_efi_get_variable;
>> +	efi.get_next_variable        = xen_efi_get_next_variable;
>> +	efi.set_variable             = xen_efi_set_variable;
>> +	efi.query_variable_info      = xen_efi_query_variable_info;
>> +	efi.update_capsule           = xen_efi_update_capsule;
>> +	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
>> +	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
>> +	efi.reset_system             = NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(xen_efi_runtime_setup);
>
> This looks very similar to struct efi efi_xen previously in
> drivers/xen/efi.c.  Maybe it makes sense to leave struct efi efi_xen in
> drivers/xen/efi.c, export it in include/xen/xen-ops.h, then here just:
>
>    efi = efi_xen;
>
> Would that improve code readability?
>
Ok.
>
> Correct me if I am wrong, but on ARM64 (differently from x86) it is not
> necessary to set efi.systab because it is not used, right? If so, it
> would be best to add a comment here to remember.
>
Not set efi.systab here because it gets the system table through fdt and 
set efi.systab there. See uefi_init() in arch/arm64/kernel.efi.c

	efi.systab = early_memremap(efi_system_table,
				    sizeof(efi_system_table_t));
>
>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>> index 73708ac..27d216a 100644
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -268,7 +268,7 @@ config XEN_HAVE_PVMMU
>>
>>   config XEN_EFI
>>   	def_bool y
>> -	depends on X86_64 && EFI
>> +	depends on (ARM64 || X86_64) && EFI
>>
>>   config XEN_AUTO_XLATE
>>   	def_bool y
>> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
>> index c83a338..36ff8e4 100644
>> --- a/include/xen/xen-ops.h
>> +++ b/include/xen/xen-ops.h
>> @@ -107,6 +107,7 @@ efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
>>   efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
>>   					unsigned long count, u64 *max_size,
>>   					int *reset_type);
>> +void xen_efi_runtime_setup(void);
>
> xen_efi_runtime_setup is not defined on x86, but this header is not arch
> specific.
>

-- 
Shannon

Re: [Xen-devel] [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Stefano Stabellini]

On Tue, 19 Jan 2016, Shannon Zhao wrote:
> On 2016/1/19 1:03, Stefano Stabellini wrote:
> > On Fri, 15 Jan 2016, Shannon Zhao wrote:
> > > From: Shannon Zhao <shannon.zhao@linaro.org>
> > > 
> > > When running on Xen hypervisor, runtime services are supported through
> > > hypercall. So call Xen specific function to initialize runtime services.
> > > 
> > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > 
> > Thanks Shannon, much much better!  Just a couple of questions.
> > 
> > 
> > >   arch/arm/xen/enlighten.c |  5 +++++
> > >   arch/arm64/xen/Makefile  |  1 +
> > >   arch/arm64/xen/efi.c     | 36 ++++++++++++++++++++++++++++++++++++
> > >   drivers/xen/Kconfig      |  2 +-
> > >   include/xen/xen-ops.h    |  1 +
> > >   5 files changed, 44 insertions(+), 1 deletion(-)
> > >   create mode 100644 arch/arm64/xen/efi.c
> > > 
> > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > > index 485e117..84f27ec 100644
> > > --- a/arch/arm/xen/enlighten.c
> > > +++ b/arch/arm/xen/enlighten.c
> > > @@ -414,6 +414,11 @@ static int __init xen_guest_init(void)
> > >   	if (xen_initial_domain())
> > >   		pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
> > > 
> > > +	if (IS_ENABLED(CONFIG_XEN_EFI)) {
> > > +		if (efi_enabled(EFI_PARAVIRT))
> > > +			xen_efi_runtime_setup();
> > > +	}
> > > +
> > >   	return 0;
> > >   }
> > >   early_initcall(xen_guest_init);
> > > diff --git a/arch/arm64/xen/Makefile b/arch/arm64/xen/Makefile
> > > index 74a8d87..62e6fe2 100644
> > > --- a/arch/arm64/xen/Makefile
> > > +++ b/arch/arm64/xen/Makefile
> > > @@ -1,2 +1,3 @@
> > >   xen-arm-y	+= $(addprefix ../../arm/xen/, enlighten.o grant-table.o p2m.o
> > > mm.o)
> > >   obj-y		:= xen-arm.o hypercall.o
> > > +obj-$(CONFIG_XEN_EFI) += efi.o
> > > diff --git a/arch/arm64/xen/efi.c b/arch/arm64/xen/efi.c
> > > new file mode 100644
> > > index 0000000..33046b0
> > > --- /dev/null
> > > +++ b/arch/arm64/xen/efi.c
> > > @@ -0,0 +1,36 @@
> > > +/*
> > > + * Copyright (c) 2015, Linaro Limited, Shannon Zhao
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> > > along
> > > + * with this program.  If not, see <http://www.gnu.org/licenses/>.
> > > + */
> > > +
> > > +#include <linux/efi.h>
> > > +#include <xen/xen-ops.h>
> > > +
> > > +void __init xen_efi_runtime_setup(void)
> > > +{
> > > +	efi.get_time                 = xen_efi_get_time;
> > > +	efi.set_time                 = xen_efi_set_time;
> > > +	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
> > > +	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
> > > +	efi.get_variable             = xen_efi_get_variable;
> > > +	efi.get_next_variable        = xen_efi_get_next_variable;
> > > +	efi.set_variable             = xen_efi_set_variable;
> > > +	efi.query_variable_info      = xen_efi_query_variable_info;
> > > +	efi.update_capsule           = xen_efi_update_capsule;
> > > +	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
> > > +	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> > > +	efi.reset_system             = NULL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(xen_efi_runtime_setup);
> > 
> > This looks very similar to struct efi efi_xen previously in
> > drivers/xen/efi.c.  Maybe it makes sense to leave struct efi efi_xen in
> > drivers/xen/efi.c, export it in include/xen/xen-ops.h, then here just:
> > 
> >    efi = efi_xen;
> > 
> > Would that improve code readability?
> > 
> Ok.
> > 
> > Correct me if I am wrong, but on ARM64 (differently from x86) it is not
> > necessary to set efi.systab because it is not used, right? If so, it
> > would be best to add a comment here to remember.
> > 
> Not set efi.systab here because it gets the system table through fdt and set
> efi.systab there. See uefi_init() in arch/arm64/kernel.efi.c
> 
> 	efi.systab = early_memremap(efi_system_table,
> 				    sizeof(efi_system_table_t));

I see now. Then it might be still good to add a comment about that.


> > > diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> > > index 73708ac..27d216a 100644
> > > --- a/drivers/xen/Kconfig
> > > +++ b/drivers/xen/Kconfig
> > > @@ -268,7 +268,7 @@ config XEN_HAVE_PVMMU
> > > 
> > >   config XEN_EFI
> > >   	def_bool y
> > > -	depends on X86_64 && EFI
> > > +	depends on (ARM64 || X86_64) && EFI
> > > 
> > >   config XEN_AUTO_XLATE
> > >   	def_bool y
> > > diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> > > index c83a338..36ff8e4 100644
> > > --- a/include/xen/xen-ops.h
> > > +++ b/include/xen/xen-ops.h
> > > @@ -107,6 +107,7 @@ efi_status_t
> > > xen_efi_update_capsule(efi_capsule_header_t **capsules,
> > >   efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
> > >   					unsigned long count, u64 *max_size,
> > >   					int *reset_type);
> > > +void xen_efi_runtime_setup(void);
> > 
> > xen_efi_runtime_setup is not defined on x86, but this header is not arch
> > specific.
> > 
> 
> -- 
> Shannon
> 

Re: [Xen-devel] [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Shannon Zhao]

On 2016/1/19 1:03, Stefano Stabellini wrote:
> On Fri, 15 Jan 2016, Shannon Zhao wrote:
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> > 
>> > When running on Xen hypervisor, runtime services are supported through
>> > hypercall. So call Xen specific function to initialize runtime services.
>> > 
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> Thanks Shannon, much much better!  Just a couple of questions.
> 
> 
>> >  arch/arm/xen/enlighten.c |  5 +++++
>> >  arch/arm64/xen/Makefile  |  1 +
>> >  arch/arm64/xen/efi.c     | 36 ++++++++++++++++++++++++++++++++++++
>> >  drivers/xen/Kconfig      |  2 +-
>> >  include/xen/xen-ops.h    |  1 +
>> >  5 files changed, 44 insertions(+), 1 deletion(-)
>> >  create mode 100644 arch/arm64/xen/efi.c
>> > 
>> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> > index 485e117..84f27ec 100644
>> > --- a/arch/arm/xen/enlighten.c
>> > +++ b/arch/arm/xen/enlighten.c
>> > @@ -414,6 +414,11 @@ static int __init xen_guest_init(void)
>> >  	if (xen_initial_domain())
>> >  		pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
>> >  
>> > +	if (IS_ENABLED(CONFIG_XEN_EFI)) {
>> > +		if (efi_enabled(EFI_PARAVIRT))
>> > +			xen_efi_runtime_setup();
>> > +	}
>> > +
>> >  	return 0;
>> >  }
>> >  early_initcall(xen_guest_init);
>> > diff --git a/arch/arm64/xen/Makefile b/arch/arm64/xen/Makefile
>> > index 74a8d87..62e6fe2 100644
>> > --- a/arch/arm64/xen/Makefile
>> > +++ b/arch/arm64/xen/Makefile
>> > @@ -1,2 +1,3 @@
>> >  xen-arm-y	+= $(addprefix ../../arm/xen/, enlighten.o grant-table.o p2m.o mm.o)
>> >  obj-y		:= xen-arm.o hypercall.o
>> > +obj-$(CONFIG_XEN_EFI) += efi.o
>> > diff --git a/arch/arm64/xen/efi.c b/arch/arm64/xen/efi.c
>> > new file mode 100644
>> > index 0000000..33046b0
>> > --- /dev/null
>> > +++ b/arch/arm64/xen/efi.c
>> > @@ -0,0 +1,36 @@
>> > +/*
>> > + * Copyright (c) 2015, Linaro Limited, Shannon Zhao
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License as published by
>> > + * the Free Software Foundation; either version 2 of the License, or
>> > + * (at your option) any later version.
>> > + *
>> > + * This program is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> > + * GNU General Public License for more details.
>> > + *
>> > + * You should have received a copy of the GNU General Public License along
>> > + * with this program.  If not, see <http://www.gnu.org/licenses/>.
>> > + */
>> > +
>> > +#include <linux/efi.h>
>> > +#include <xen/xen-ops.h>
>> > +
>> > +void __init xen_efi_runtime_setup(void)
>> > +{
>> > +	efi.get_time                 = xen_efi_get_time;
>> > +	efi.set_time                 = xen_efi_set_time;
>> > +	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
>> > +	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
>> > +	efi.get_variable             = xen_efi_get_variable;
>> > +	efi.get_next_variable        = xen_efi_get_next_variable;
>> > +	efi.set_variable             = xen_efi_set_variable;
>> > +	efi.query_variable_info      = xen_efi_query_variable_info;
>> > +	efi.update_capsule           = xen_efi_update_capsule;
>> > +	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
>> > +	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
>> > +	efi.reset_system             = NULL;
>> > +}
>> > +EXPORT_SYMBOL_GPL(xen_efi_runtime_setup);
> This looks very similar to struct efi efi_xen previously in
> drivers/xen/efi.c.  Maybe it makes sense to leave struct efi efi_xen in
> drivers/xen/efi.c, export it in include/xen/xen-ops.h, then here just:
> 
>   efi = efi_xen;
> 
> Would that improve code readability?

Rethink about this. It's a little different on ARM since we call
xen_efi_runtime_setup after parsing the FDT and setting some members of
efi already, e.g. efi.systab, efi.acpi20. So it necessary to have a
different way to initialize the struct efi.

Thanks,
-- 
Shannon

Re: [Xen-devel] [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Stefano Stabellini]

On Fri, 22 Jan 2016, Shannon Zhao wrote:
> On 2016/1/19 1:03, Stefano Stabellini wrote:
> > On Fri, 15 Jan 2016, Shannon Zhao wrote:
> >> > From: Shannon Zhao <shannon.zhao@linaro.org>
> >> > 
> >> > When running on Xen hypervisor, runtime services are supported through
> >> > hypercall. So call Xen specific function to initialize runtime services.
> >> > 
> >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > Thanks Shannon, much much better!  Just a couple of questions.
> > 
> > 
> >> >  arch/arm/xen/enlighten.c |  5 +++++
> >> >  arch/arm64/xen/Makefile  |  1 +
> >> >  arch/arm64/xen/efi.c     | 36 ++++++++++++++++++++++++++++++++++++
> >> >  drivers/xen/Kconfig      |  2 +-
> >> >  include/xen/xen-ops.h    |  1 +
> >> >  5 files changed, 44 insertions(+), 1 deletion(-)
> >> >  create mode 100644 arch/arm64/xen/efi.c
> >> > 
> >> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> >> > index 485e117..84f27ec 100644
> >> > --- a/arch/arm/xen/enlighten.c
> >> > +++ b/arch/arm/xen/enlighten.c
> >> > @@ -414,6 +414,11 @@ static int __init xen_guest_init(void)
> >> >  	if (xen_initial_domain())
> >> >  		pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
> >> >  
> >> > +	if (IS_ENABLED(CONFIG_XEN_EFI)) {
> >> > +		if (efi_enabled(EFI_PARAVIRT))
> >> > +			xen_efi_runtime_setup();
> >> > +	}
> >> > +
> >> >  	return 0;
> >> >  }
> >> >  early_initcall(xen_guest_init);
> >> > diff --git a/arch/arm64/xen/Makefile b/arch/arm64/xen/Makefile
> >> > index 74a8d87..62e6fe2 100644
> >> > --- a/arch/arm64/xen/Makefile
> >> > +++ b/arch/arm64/xen/Makefile
> >> > @@ -1,2 +1,3 @@
> >> >  xen-arm-y	+= $(addprefix ../../arm/xen/, enlighten.o grant-table.o p2m.o mm.o)
> >> >  obj-y		:= xen-arm.o hypercall.o
> >> > +obj-$(CONFIG_XEN_EFI) += efi.o
> >> > diff --git a/arch/arm64/xen/efi.c b/arch/arm64/xen/efi.c
> >> > new file mode 100644
> >> > index 0000000..33046b0
> >> > --- /dev/null
> >> > +++ b/arch/arm64/xen/efi.c
> >> > @@ -0,0 +1,36 @@
> >> > +/*
> >> > + * Copyright (c) 2015, Linaro Limited, Shannon Zhao
> >> > + *
> >> > + * This program is free software; you can redistribute it and/or modify
> >> > + * it under the terms of the GNU General Public License as published by
> >> > + * the Free Software Foundation; either version 2 of the License, or
> >> > + * (at your option) any later version.
> >> > + *
> >> > + * This program is distributed in the hope that it will be useful,
> >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> > + * GNU General Public License for more details.
> >> > + *
> >> > + * You should have received a copy of the GNU General Public License along
> >> > + * with this program.  If not, see <http://www.gnu.org/licenses/>.
> >> > + */
> >> > +
> >> > +#include <linux/efi.h>
> >> > +#include <xen/xen-ops.h>
> >> > +
> >> > +void __init xen_efi_runtime_setup(void)
> >> > +{
> >> > +	efi.get_time                 = xen_efi_get_time;
> >> > +	efi.set_time                 = xen_efi_set_time;
> >> > +	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
> >> > +	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
> >> > +	efi.get_variable             = xen_efi_get_variable;
> >> > +	efi.get_next_variable        = xen_efi_get_next_variable;
> >> > +	efi.set_variable             = xen_efi_set_variable;
> >> > +	efi.query_variable_info      = xen_efi_query_variable_info;
> >> > +	efi.update_capsule           = xen_efi_update_capsule;
> >> > +	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
> >> > +	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> >> > +	efi.reset_system             = NULL;
> >> > +}
> >> > +EXPORT_SYMBOL_GPL(xen_efi_runtime_setup);
> > This looks very similar to struct efi efi_xen previously in
> > drivers/xen/efi.c.  Maybe it makes sense to leave struct efi efi_xen in
> > drivers/xen/efi.c, export it in include/xen/xen-ops.h, then here just:
> > 
> >   efi = efi_xen;
> > 
> > Would that improve code readability?
> 
> Rethink about this. It's a little different on ARM since we call
> xen_efi_runtime_setup after parsing the FDT and setting some members of
> efi already, e.g. efi.systab, efi.acpi20. So it necessary to have a
> different way to initialize the struct efi.

OK, fair enough.