Re: [Xen-devel] [PATCH RFC 26/35] arm : acpi read mmio tables from uefi

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Parth,

On 04/02/2015 14:02, parth.dixit@xxxxxxxxxx wrote:
> From: Parth Dixit <parth.dixit@xxxxxxxxxx>
>
> For ACPI on arm device initialization is done by dom0 after parsing DSDT.
For ACPI on ARM, device ...

> xen requires mmio region information described in uefi tables
> for mapping it to dom0.
>
> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
> ---
>   xen/arch/arm/efi/efi-boot.h | 16 ++++++++++++++++
>   xen/arch/arm/setup.c        |  1 +
>   xen/include/asm-arm/setup.h |  1 +
>   3 files changed, 18 insertions(+)
>
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 639942d..535f484 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -127,6 +127,8 @@ static EFI_STATUS __init 
> efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
>   {
>       int Index;
>       int i = 0;
> +    int j = 0;
> +
>       EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
>
>       for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
> @@ -145,10 +147,24 @@ static EFI_STATUS __init 
> efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
>                   break;
>               }
>           }
> +        else if ( desc_ptr->Type == EfiMemoryMappedIO
> +                  || desc_ptr->Type == EfiMemoryMappedIOPortSpace )
I would turn the function into a switch case. if ... else if ... is 
really difficult to read and ...
> +        {
> +            acpi_mmio.bank[j].start = desc_ptr->PhysicalStart;
> +            acpi_mmio.bank[j].size  = desc_ptr->NumberOfPages * EFI_PAGE_SIZE;
> +            if ( ++j >= NR_MEM_BANKS )
> +            {
> +                PrintStr(L"Warning: All ");
> +                DisplayUint(NR_MEM_BANKS, -1);
> +                PrintStr(L" acpi meminfo mem banks exhausted.\r\n");
> +                break;
... make this such problem appears.

While the number of MMIO banks has expired, we should continue to loop 
to get the RAM regions. Leaving the loop will make less obvious the 
further issue (such as no memory bank found).
> +            }
> +        }
>           desc_ptr = NextMemoryDescriptor(desc_ptr, desc_size);
>       }
>
>       bootinfo.mem.nr_banks = i;
> +    acpi_mmio.nr_banks = j;
>       return EFI_SUCCESS;
>   }
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 93c8a8a..930746b 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -50,6 +50,7 @@
>   #include <asm-arm/cputype.h>
>
>   struct bootinfo __initdata bootinfo;
#ifdef CONFIG_ACPI?

> +struct meminfo __initdata acpi_mmio;
>
>   struct cpuinfo_arm __read_mostly boot_cpu_data;
>
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index ba5a67d..5ea9ed6 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -46,6 +46,7 @@ struct bootinfo {
>   };
>
>   extern struct bootinfo bootinfo;
> +extern struct meminfo acpi_mmio;
I'm not sure that it's the right things to use meminfo. It only has 64 
banks...
Could not we re-read at runtime the UEFI memory map?

Regards,

--
Julien Grall

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