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

[David Vrabel]

On 18/11/15 04:32, Shannon Zhao wrote:
> On 2015/11/18 0:02, David Vrabel wrote:
>> On 17/11/15 09:57, shannon.z...@linaro.org wrote:
>>> From: Shannon Zhao 
>>>
>>> Move xlated_setup_gnttab_pages to common place, so it can be reused by
>>> ARM to setup grant table when booting with ACPI.
>> [...]
>>> --- a/drivers/xen/grant-table.c
>>> +++ b/drivers/xen/grant-table.c
>>> @@ -664,6 +664,55 @@ int gnttab_setup_auto_xlat_frames(phys_addr_t addr)
>>>  }
>>>  EXPORT_SYMBOL_GPL(gnttab_setup_auto_xlat_frames);
>>>  
>>> +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, 0 /* lowmem */);
>>> +   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);
>>
>> Can you move most of this function into drivers/xen/xlate_mmu.c in a
>> function like:
>>
>> /**
>>  * xen_xlate_map_ballooned_pages - map a new set of ballooned pages
>>  * count: number of GFNs.
>>  * pages: returns the array of (now) ballooned pages.
>>  * gfns: returns the array of corresponding GFNs.
>>  * virt: returns the virtual address of the mapped region.
>>  *
>>  * This allocate a set of ballooned pages and maps them into the
>>  * kernel's address space.
>>  */
>> void *xen_xlate_map_ballooned_pages(
>> unsigned int count,
>> struct page **pages,
>> xen_pfn_t **gfns);
>>
> Sure, but the parameter "pages" is not necessary I think, since the
> pages has been freed in the function and it doesn't need in
> xen_auto_xlat_grant_frames either. Right?

Yes.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH 06/13] Xen: ARM: Add support for mapping amba device mmio

[David Vrabel]

On 17/11/15 09:57, shannon.z...@linaro.org wrote:
> From: Shannon Zhao 
> 
> Add a bus_notifier for AMBA bus device in order to map the device
> mmio regions when DOM0 booting with ACPI.
[...]
> +static int xen_map_amba_device_mmio(struct amba_device *adev)
> +{
> + int rc = 0;
> + struct resource *r = &adev->res;
> +
> + if (resource_type(r) == IORESOURCE_MEM)
> + {

Haven't I seen something like this before...?

This is (almost) identical to the code added for platform devices.  This
needs to be generic.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH 07/13] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[David Vrabel]

On 17/11/15 09:57, shannon.z...@linaro.org wrote:
> From: Shannon Zhao 
> 
> Add a "uefi" node under /hypervisor node in FDT, then Linux kernel could
> scan this to get the UEFI information.
[...]
> --- a/Documentation/devicetree/bindings/arm/xen.txt
> +++ b/Documentation/devicetree/bindings/arm/xen.txt
> @@ -15,6 +15,24 @@ 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:

It's not really clear why this is under /hypervisor and why xen needs to
define its own nodes.

The handling of the existing linux standard nodes can be made
Xen-specific by the presence of the /hypervisor/compatible == "xen" node
(or similar), right?

> +
> +Name  | Size   | Description
> +
> +xen,uefi-system-table | 64-bit | Physical address of the UEFI System 
> Table.
> +
> +xen,uefi-mmap-start   | 64-bit | Physical address of the UEFI memory map.
> +

I would probably say "Guest physical address..." just to be clear.

> +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.
> +

Need a reference to the mmap descriptor format here.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH 05/13] Xen: ARM: Add support for mapping platform device mmio

[David Vrabel]

On 17/11/15 09:57, shannon.z...@linaro.org wrote:
> From: Shannon Zhao 
> 
> Add a bus_notifier for platform bus device in order to map the device
> mmio regions when DOM0 booting with ACPI.
[...]
> --- /dev/null
> +++ b/drivers/xen/platform.c
> @@ -0,0 +1,104 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited.
> + *
> + * 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, write to the Free Software Foundation, Inc., 59 
> Temple
> + * Place - Suite 330, Boston, MA 02111-1307 USA.

"You should have received a copy of the GNU General Public License
along with this program.  If not, see ."

> + *
> + * Author: Shannon Zhao 

Omit this since it just becomes stale and git will know the correct
authorship.

> +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 (!acpi_disabled && (action == BUS_NOTIFY_ADD_DEVICE))
> + r = xen_map_platform_device_mmio(pdev);

Why even register the notifier if acpi_disabled?

> +
> + if (r)
> + {
> + dev_err(&pdev->dev, "Failed to add_to_physmap device (%s) 
> mmio!\n",
> + pdev->name);
> + return NOTIFY_OK;

Return NOTIFY_BAD here?  The device will be unusable.

> + }
> +
> + return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block platform_device_nb = {
> + .notifier_call = xen_platform_notifier,
> +};
> +
> +static int __init register_xen_platform_notifier(void)
> +{
> + if (!xen_initial_domain())
> + return 0;
> +
> + return bus_register_notifier(&platform_bus_type, &platform_device_nb);

Why only platform busses?  What about PCI devices?  Where will they get
added to dom0's physmap?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Vrabel]

On 17/11/15 09:57, shannon.z...@linaro.org wrote:
> From: Shannon Zhao 
> 
> Move xlated_setup_gnttab_pages to common place, so it can be reused by
> ARM to setup grant table when booting with ACPI.
[...]
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -664,6 +664,55 @@ int gnttab_setup_auto_xlat_frames(phys_addr_t addr)
>  }
>  EXPORT_SYMBOL_GPL(gnttab_setup_auto_xlat_frames);
>  
> +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, 0 /* lowmem */);
> + 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);

Can you move most of this function into drivers/xen/xlate_mmu.c in a
function like:

/**
 * xen_xlate_map_ballooned_pages - map a new set of ballooned pages
 * count: number of GFNs.
 * pages: returns the array of (now) ballooned pages.
 * gfns: returns the array of corresponding GFNs.
 * virt: returns the virtual address of the mapped region.
 *
 * This allocate a set of ballooned pages and maps them into the
 * kernel's address space.
 */
void *xen_xlate_map_ballooned_pages(
unsigned int count,
struct page **pages,
xen_pfn_t **gfns);

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/10] xen: Add EFI support

[David Vrabel]

On 30/06/14 18:52, Daniel Kiper wrote:
> Hey,
> 
> This patch series adds EFI support for Xen dom0 guests.
> It is based on Jan Beulich and Tang Liang work. I was
> trying to take into account all previous comments,
> however, if I missed something sorry for that.

What's changed in this version?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH v6 7/9] xen: Put EFI machinery in place

[David Vrabel]

On 20/06/14 22:29, Daniel Kiper wrote:
> This patch enables EFI usage under Xen dom0. Standard EFI Linux
> Kernel infrastructure cannot be used because it requires direct
> access to EFI data and code. However, in dom0 case it is not possible
> because above mentioned EFI stuff is fully owned and controlled
> by Xen hypervisor. In this case all calls from dom0 to EFI must
> be requested via special hypercall which in turn executes relevant
> EFI code in behalf of dom0.
> 
> When dom0 kernel boots it checks for EFI availability on a machine.
> If it is detected then artificial EFI system table is filled.
> Native EFI callas are replaced by functions which mimics them
> by calling relevant hypercall. Later pointer to EFI system table
> is passed to standard EFI machinery and it continues EFI subsystem
> initialization taking into account that there is no direct access
> to EFI boot services, runtime, tables, structures, etc. After that
> system runs as usual.

Reviewed-by: David Vrabel 

(With or without the change suggested by Stefano).

Thanks.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/9] arch/x86: Do not access EFI memory map if it is not available

[David Vrabel]

On 20/06/14 22:29, Daniel Kiper wrote:
> Do not access EFI memory map if it is not available. At least
> Xen dom0 EFI implementation does not have an access to it.

Could it make one based on the XENMEM_memory_map or
XENMEM_machine_memory_map hypercall?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/9] xen: Define EFI related stuff

[David Vrabel]

On 20/06/14 22:29, Daniel Kiper wrote:
> Define constants and structures which are needed to properly
> execute EFI related hypercall in Xen dom0.

Reviewed-by: David Vrabel 

Thanks.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/7] efi: Use early_mem*() instead of early_io*()

[David Vrabel]

On 18/06/14 13:17, Matt Fleming wrote:
> (Pulling in Mark Salter for early_ioremap() knowledge)
> 
> On Fri, 13 Jun, at 07:00:17PM, Daniel Kiper wrote:
>> Use early_mem*() instead of early_io*() because all mapped EFI regions
>> are true RAM not I/O regions. Additionally, I/O family calls do not work
>> correctly under Xen in our case. AIUI, early_io*() maps/unmaps real machine
>> addresses. However, all artificial EFI structures created under Xen live
>> in dom0 memory and should be mapped/unmapped using early_mem*() family
>> calls which map domain memory.
>>
>> Signed-off-by: Daniel Kiper 
>> ---
>>  arch/x86/platform/efi/efi.c |   42 
>> +-
>>  drivers/firmware/efi/efi.c  |4 ++--
>>  2 files changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> index 87fc96b..dd1e351 100644
>> --- a/arch/x86/platform/efi/efi.c
>> +++ b/arch/x86/platform/efi/efi.c
>> @@ -427,7 +427,7 @@ void __init efi_unmap_memmap(void)
>>  {
>>  clear_bit(EFI_MEMMAP, &efi.flags);
>>  if (memmap.map) {
>> -early_iounmap(memmap.map, memmap.nr_map * memmap.desc_size);
>> +early_memunmap(memmap.map, memmap.nr_map * memmap.desc_size);
>>  memmap.map = NULL;
>>  }
>>  }
>> @@ -467,12 +467,12 @@ static int __init efi_systab_init(void *phys)
>>  if (!data)
>>  return -ENOMEM;
>>  }
>> -systab64 = early_ioremap((unsigned long)phys,
>> - sizeof(*systab64));
>> +systab64 = early_memremap((unsigned long)phys,
>> +sizeof(*systab64));
> 
> Please don't misalign the arguments :-( This makes the diff harder to
> read when all you're doing is changing the function call. You're
> indentation isn't an improvement.
>  
> As Matthew pointed out we may also need to access EFI mapped flash
> devices.
> 
> Now, the only difference between early_memremap() and early_ioremap(),
> at least on x86, is PAGE_KERNEL vs. PAGE_KERNEL_IO, where PAGE_KERNEL_IO
> has the additional _PAGE_BIT_IOMAP bit set in the pte. But that's a
> software bit for x86.

_PAGE_BIT_IOMAP is supposed to be going away...

> So, even though this change should be harmless, it's not clear to me why
> this change is needed. You say "I/O family calls do not work correctly
> under Xen in our case", but could you provide some more explanation as
> to why they don't work correctly?

_PAGE_BIT_IOMAP causes a Xen PV guest to skip the PFN to MFN conversion
when building the PTE.  Using it for RAM will attempt to map the wrong
machine frame.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 4/7] xen: Put EFI machinery in place

[David Vrabel]

On 13/06/14 18:00, Daniel Kiper wrote:
> 
> v5 - suggestions/fixes:

Put after a --- marker.

> +static efi_char16_t vendor[100] __initdata;

Why 100?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH v5 3/7] xen: Define EFI related stuff

[David Vrabel]

On 13/06/14 18:00, Daniel Kiper wrote:
> Define constants and structures which are needed to properly
> execute EFI related hypercall in Xen dom0.
> 
> This patch is based on Jan Beulich and Tang Liang work.
> 
> v5 - suggestions/fixes:

This version information should be after a '---' marker.

> + struct {
> + GUEST_HANDLE(void) name;  /* UCS-2/UTF-16 string */
> + xen_ulong_t size;
> + GUEST_HANDLE(void) data;
> + struct xenpf_efi_guid {
> + uint32_t data1;
> + uint16_t data2;
> + uint16_t data3;
> + uint8_t data4[8];
> + } vendor_guid;

Indentation looks all messed up here and in many other places.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH v5 2/7] efi: Introduce EFI_NO_DIRECT flag

[David Vrabel]

On 13/06/14 18:00, Daniel Kiper wrote:
> Introduce EFI_NO_DIRECT flag.

EFI_PARAVIRT would be a clearer name I think.

> +#define EFI_NO_DIRECT6   /* Can we access EFI directly? 
> */

#define EFI_PARAVIRT 6 /* Access is via a paravirt interface */

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/5] xen: Put EFI machinery in place

[David Vrabel]

On 20/05/14 12:29, Daniel Kiper wrote:
> 
>>> +   if (!xen_initial_domain() || HYPERVISOR_dom0_op(&op))
>>> +   return NULL;
>>
>>  if (!xen_initial_domain())
>>  return NULL;
>>
>>  if (HYPERVISOR_dom0_op(&op) < 0)
>>  return NULL;
> 
> What is wrong with my if?

Style.  The function returns -ve on error not a boolean success/fail.

>>> +
>>> +   /* Here we know that Xen runs on EFI platform. */
>>> +
>>> +   efi = efi_xen;
>>> +
>>> +   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_dom0_op(&op)) {
>>
>> if (HYPERVISOR_dom0_op(&op) == 0)
> 
> Ditto?

Again, style.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/5] xen: Put EFI machinery in place

[David Vrabel]

On 16/05/14 21:41, Daniel Kiper wrote:
> Put EFI machinery for Xen in place.

Put what machinery to do what?

> @@ -1714,6 +1725,21 @@ asmlinkage __visible void __init xen_start_kernel(void)
>  
>   xen_setup_runstate_info(0);
>  
> + efi_systab_xen = xen_efi_probe();
> +
> + if (efi_systab_xen) {
> + strncpy((char *)&boot_params.efi_info.efi_loader_signature, 
> "Xen",
> + 
> sizeof(boot_params.efi_info.efi_loader_signature));
> + boot_params.efi_info.efi_systab = 
> (__u32)((__u64)efi_systab_xen);
> + boot_params.efi_info.efi_systab_hi = 
> (__u32)((__u64)efi_systab_xen >> 32);
> +
> + x86_platform.get_wallclock = efi_get_time;

x86_platform.get_wallclock should always be xen_get_wallclock().

> + x86_platform.set_wallclock = efi_set_rtc_mmss;
> +
> + set_bit(EFI_BOOT, &efi.flags);
> + set_bit(EFI_64BIT, &efi.flags);
> + }
> +
>   /* Start the world */
>  #ifdef CONFIG_X86_32
>   i386_start_kernel();
> --- /dev/null
> +++ b/drivers/xen/efi.c
> @@ -0,0 +1,374 @@
> + * Copyright (c) 2014 Daniel Kiper, Oracle Corporation

Is this really copyright by you personally and not Oracle?


> +#define call (op.u.efi_runtime_call)
> +#define DECLARE_CALL(what) \
> + struct xen_platform_op op; \
> + op.cmd = XENPF_efi_runtime_call; \
> + call.function = XEN_EFI_##what; \
> + call.misc = 0

Macros that declare local variables are awful.

Use what Andrew suggested and something like

struct xen_blah *call = &op.u.efi_runtime_call;


> +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,
> + .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. */

Xen provides functionality to reset (just maybe not via an EFI call).
Should an implementation be provided that does this?

> + .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_dom0_op(&op))
> + return NULL;

if (!xen_initial_domain())
return NULL;

if (HYPERVISOR_dom0_op(&op) < 0)
return NULL;

> +
> + /* Here we know that Xen runs on EFI platform. */
> +
> + efi = efi_xen;
> +
> + 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_dom0_op(&op)) {

if (HYPERVISOR_dom0_op(&op) == 0)

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/5] xen: Define EFI related stuff

[David Vrabel]

On 16/05/14 21:41, Daniel Kiper wrote:
> Define EFI related stuff for Xen.

Define "stuff"?  Please write a more descriptive commit message.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag

[David Vrabel]

On 16/05/14 21:41, Daniel Kiper wrote:
> Introduce EFI_DIRECT flag. If it is set this means that Linux
> Kernel has direct access to EFI infrastructure. If not then
> kernel runs on EFI platform but it has not direct control
> on EFI stuff. This functionality is used in Xen dom0.

This is backwards.  It should flag should indicate the weird platform
not the standard, default one.

You also need to explain why this is needed rather than presenting a
more complete EFI interface to PV guests.  And you should explain why
each use is necessary.

> +static void __init __iomem *efi_early_ioremap(resource_size_t phys_addr,
> + unsigned long size)
> +{
> + if (efi_enabled(EFI_DIRECT))
> + return early_ioremap(phys_addr, size);
> +
> + return (__force void __iomem *)phys_addr;
> +}

Er...  Perhaps you mean BUG_ON(!efi_enabled(EFI_DIRECT))? Or return NULL?

> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -893,12 +893,13 @@ extern int __init efi_setup_pcdp_console(char *);
>   * possible, remove EFI-related code altogether.
>   */
>  #define EFI_BOOT 0   /* Were we booted from EFI? */
> -#define EFI_SYSTEM_TABLES1   /* Can we use EFI system tables? */
> -#define EFI_CONFIG_TABLES2   /* Can we use EFI config tables? */
> -#define EFI_RUNTIME_SERVICES 3   /* Can we use runtime services? */
> -#define EFI_MEMMAP   4   /* Can we use EFI memory map? */
> -#define EFI_64BIT5   /* Is the firmware 64-bit? */
> -#define EFI_ARCH_1   6   /* First arch-specific bit */
> +#define EFI_DIRECT   1   /* Can we access EFI directly? */
> +#define EFI_SYSTEM_TABLES2   /* Can we use EFI system tables? */
> +#define EFI_CONFIG_TABLES3   /* Can we use EFI config tables? */
> +#define EFI_RUNTIME_SERVICES 4   /* Can we use runtime services? */
> +#define EFI_MEMMAP   5   /* Can we use EFI memory map? */
> +#define EFI_64BIT6   /* Is the firmware 64-bit? */
> +#define EFI_ARCH_1   7   /* First arch-specific bit */

Unnecessary shuffling of these values.  Why didn't you stick it after
EFI_64BIT?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/5] xen: Add EFI support

[David Vrabel]

On 16/05/14 21:41, Daniel Kiper wrote:
> Hey,
> 
> This patch series adds EFI support for Xen dom0 guests.
> It is based on Jan Beulich and Tang Liang work. I was
> trying to take into account all previous comments,
> however, if I missed something sorry for that.

There needs to be a better description of this series.  In particular, a
description of why Xen PV guests are different and the overall design used.

> I am still not sure what to do with /sys/firmware/efi/config_table,
> /sys/firmware/efi/{fw_vendor,runtime,systab} files. On bare metal
> they contain physical addresses of relevant structures. However,
> in Xen case they does not make sens. So maybe they should contain
> invalid values (e.g. 0) or should not appear at all on Xen (I prefer
> last one). What do you think about that?

Generally, dom0 has provided access to BIOS provided data structures.  I
think you should do the same here.  They may be useful for diagnostics
if nothing else.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html