diff mbox

[Xen-devel,RFC,34/35] arm : acpi workarounds for firmware/linux dependencies

Message ID 1423058539-26403-35-git-send-email-parth.dixit@linaro.org
State New
Headers show

Commit Message

Parth Dixit Feb. 4, 2015, 2:02 p.m. UTC
From: Parth Dixit <parth.dixit@linaro.org>

Some bugs are identified in edk2 and some of the functionality is not
yet merged. This patch contains workarounds for them

Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
---
 xen/arch/arm/domain_build.c | 82 ++++++++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/vgic.c         | 16 +++++++++
 xen/drivers/acpi/osl.c      |  7 ++--
 3 files changed, 102 insertions(+), 3 deletions(-)

Comments

Julien Grall Feb. 5, 2015, 5:38 a.m. UTC | #1
Hi Parth,

On 04/02/2015 14:02, parth.dixit@linaro.org wrote:
> From: Parth Dixit <parth.dixit@linaro.org>
>
> Some bugs are identified in edk2 and some of the functionality is not
> yet merged. This patch contains workarounds for them

While I understand some workaround (based on your cover letter), some of 
them is unclear to me and need explanation.

>
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
> ---

[..]

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 97061ce..e74555d 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -254,6 +254,8 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>       }
>   }
>
> +#define VGIC_ICFG_MASK(intr) ( 1 << ( ( 2 * ( intr % 16 ) ) + 1 ) )
> +
>   void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>   {
>       struct domain *d = v->domain;
> @@ -266,6 +268,20 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>
>       while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>           irq = i + (32 * n);
> +#if defined(CONFIG_ARM_64) && defined(CONFIG_ACPI)
> +        if( ( n!=0 ) && is_hardware_domain(d) ){
> +            struct vgic_irq_rank *vr = vgic_get_rank(v, n);
> +            uint32_t tr;
> +            tr = vr->icfg[i >> 4] ;
> +
> +            if( ( tr & VGIC_ICFG_MASK(i) ) )
> +                acpi_set_irq(irq, DT_IRQ_TYPE_EDGE_BOTH);
> +            else
> +                acpi_set_irq(irq, DT_IRQ_TYPE_LEVEL_MASK);

What's the status of the dynamic IRQ configuration?

> +
> +            route_irq_to_guest(d,irq,NULL);

Hmmm, do you really plan to keep that here? What's your plan for this?

> +        }
> +#endif
>           v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
>           p = irq_to_pending(v_target, irq);
>           set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
> index 73da9d9..2d78ba0 100644
> --- a/xen/drivers/acpi/osl.c
> +++ b/xen/drivers/acpi/osl.c
> @@ -66,7 +66,7 @@ void __init acpi_os_vprintf(const char *fmt, va_list args)
>
>   acpi_physical_address __init acpi_os_get_root_pointer(void)
>   {
> -	if (efi_enabled) {
> +    if (efi_enabled) {

Spurious change

>   		if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
>   			return efi.acpi20;
>   		else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
> @@ -198,8 +198,11 @@ acpi_os_write_memory(acpi_physical_address phys_addr, u32 value, u32 width)
>
>   	return AE_OK;
>   }
> -
> +#ifdef CONFIG_X86
>   #define is_xmalloc_memory(ptr) ((unsigned long)(ptr) & (PAGE_SIZE - 1))
> +#else
> +#define is_xmalloc_memory(ptr) 1
> +#endif

Why? I though this was resolved?

>   void *__init acpi_os_alloc_memory(size_t sz)
>   {
>

Regards,
Parth Dixit Feb. 5, 2015, 10:30 a.m. UTC | #2
On 5 February 2015 at 11:08, Julien Grall <julien.grall@linaro.org> wrote:
> Hi Parth,
>
> On 04/02/2015 14:02, parth.dixit@linaro.org wrote:
>>
>> From: Parth Dixit <parth.dixit@linaro.org>
>>
>> Some bugs are identified in edk2 and some of the functionality is not
>> yet merged. This patch contains workarounds for them
>
>
> While I understand some workaround (based on your cover letter), some of
> them is unclear to me and need explanation.
Sure, ask them, i'll reply to it.
>>
>> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
>> ---
>
>
> [..]
>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 97061ce..e74555d 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -254,6 +254,8 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int
>> n)
>>       }
>>   }
>>
>> +#define VGIC_ICFG_MASK(intr) ( 1 << ( ( 2 * ( intr % 16 ) ) + 1 ) )
>> +
>>   void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>>   {
>>       struct domain *d = v->domain;
>> @@ -266,6 +268,20 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int
>> n)
>>
>>       while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>>           irq = i + (32 * n);
>> +#if defined(CONFIG_ARM_64) && defined(CONFIG_ACPI)
>> +        if( ( n!=0 ) && is_hardware_domain(d) ){
>> +            struct vgic_irq_rank *vr = vgic_get_rank(v, n);
>> +            uint32_t tr;
>> +            tr = vr->icfg[i >> 4] ;
>> +
>> +            if( ( tr & VGIC_ICFG_MASK(i) ) )
>> +                acpi_set_irq(irq, DT_IRQ_TYPE_EDGE_BOTH);
>> +            else
>> +                acpi_set_irq(irq, DT_IRQ_TYPE_LEVEL_MASK);
>
>
> What's the status of the dynamic IRQ configuration?
It would be on top of your patches but this is the place where i am
planning to trap it.
I have not rebased it on top of your patches, so that needs to be done.
>> +
>> +            route_irq_to_guest(d,irq,NULL);
>
>
> Hmmm, do you really plan to keep that here? What's your plan for this?
yes, but i am open to suggestions , if you think there is a better
place i'll move it there.
>> +        }
>> +#endif
>>           v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
>>           p = irq_to_pending(v_target, irq);
>>           set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
>> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
>> index 73da9d9..2d78ba0 100644
>> --- a/xen/drivers/acpi/osl.c
>> +++ b/xen/drivers/acpi/osl.c
>> @@ -66,7 +66,7 @@ void __init acpi_os_vprintf(const char *fmt, va_list
>> args)
>>
>>   acpi_physical_address __init acpi_os_get_root_pointer(void)
>>   {
>> -       if (efi_enabled) {
>> +    if (efi_enabled) {
>
>
> Spurious change
will take care in next patchset
>>                 if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
>>                         return efi.acpi20;
>>                 else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
>> @@ -198,8 +198,11 @@ acpi_os_write_memory(acpi_physical_address phys_addr,
>> u32 value, u32 width)
>>
>>         return AE_OK;
>>   }
>> -
>> +#ifdef CONFIG_X86
>>   #define is_xmalloc_memory(ptr) ((unsigned long)(ptr) & (PAGE_SIZE - 1))
>> +#else
>> +#define is_xmalloc_memory(ptr) 1
>> +#endif
>
>
> Why? I though this was resolved?
i am not aware what was the resolution on it?
>>   void *__init acpi_os_alloc_memory(size_t sz)
>>   {
>>
>
> Regards,
>
> --
> Julien Grall
Julien Grall Feb. 5, 2015, 2:59 p.m. UTC | #3
Hi Parth,

On 05/02/2015 18:30, Parth Dixit wrote:
> On 5 February 2015 at 11:08, Julien Grall <julien.grall@linaro.org> wrote:
>> Hi Parth,
>>
>> On 04/02/2015 14:02, parth.dixit@linaro.org wrote:
>>>
>>> From: Parth Dixit <parth.dixit@linaro.org>
>>>
>>> Some bugs are identified in edk2 and some of the functionality is not
>>> yet merged. This patch contains workarounds for them
>>
>>
>> While I understand some workaround (based on your cover letter), some of
>> them is unclear to me and need explanation.
> Sure, ask them, i'll reply to it.

I asked them on the previous mail. It seems you answered all of them, 
thanks :)

>>>
>>> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
>>> ---
>>
>>
>> [..]
>>
>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>>> index 97061ce..e74555d 100644
>>> --- a/xen/arch/arm/vgic.c
>>> +++ b/xen/arch/arm/vgic.c
>>> @@ -254,6 +254,8 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int
>>> n)
>>>        }
>>>    }
>>>
>>> +#define VGIC_ICFG_MASK(intr) ( 1 << ( ( 2 * ( intr % 16 ) ) + 1 ) )
>>> +
>>>    void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>>>    {
>>>        struct domain *d = v->domain;
>>> @@ -266,6 +268,20 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int
>>> n)
>>>
>>>        while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>>>            irq = i + (32 * n);
>>> +#if defined(CONFIG_ARM_64) && defined(CONFIG_ACPI)
>>> +        if( ( n!=0 ) && is_hardware_domain(d) ){
>>> +            struct vgic_irq_rank *vr = vgic_get_rank(v, n);
>>> +            uint32_t tr;
>>> +            tr = vr->icfg[i >> 4] ;
>>> +
>>> +            if( ( tr & VGIC_ICFG_MASK(i) ) )
>>> +                acpi_set_irq(irq, DT_IRQ_TYPE_EDGE_BOTH);
>>> +            else
>>> +                acpi_set_irq(irq, DT_IRQ_TYPE_LEVEL_MASK);
>>
>>
>> What's the status of the dynamic IRQ configuration?
> It would be on top of your patches but this is the place where i am
> planning to trap it.
> I have not rebased it on top of your patches, so that needs to be done.

Ok.

>>> +
>>> +            route_irq_to_guest(d,irq,NULL);
>>
>>
>> Hmmm, do you really plan to keep that here? What's your plan for this?
> yes, but i am open to suggestions , if you think there is a better
> place i'll move it there.

Ok, when we discussed about it in december that wasn't the plan.

While I'm okay for the IRQ configuration (setting level/edge), the 
routing should not be done here.

First at all, route_irq_to_guest could fail and enable doesn't handle 
this use.

Then, DOM0 (aka the hardware domain) may not be able to use all the 
interrupts (think about pass-through).

So, for IRQ routing there is 2 possibles solutions:
	1) Route all the SPIs to the guest in Xen
	2) Let DOM0 Linux requesting to route the IRQ via a PHYSDEV_op

IIRC, we deciced to choose the first solution for now. Even though none 
of the solutions are perfect.

>>> +        }
>>> +#endif
>>>            v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
>>>            p = irq_to_pending(v_target, irq);
>>>            set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
>>> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
>>> index 73da9d9..2d78ba0 100644
>>> --- a/xen/drivers/acpi/osl.c
>>> +++ b/xen/drivers/acpi/osl.c
>>> @@ -66,7 +66,7 @@ void __init acpi_os_vprintf(const char *fmt, va_list
>>> args)
>>>
>>>    acpi_physical_address __init acpi_os_get_root_pointer(void)
>>>    {
>>> -       if (efi_enabled) {
>>> +    if (efi_enabled) {
>>
>>
>> Spurious change
> will take care in next patchset
>>>                  if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
>>>                          return efi.acpi20;
>>>                  else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
>>> @@ -198,8 +198,11 @@ acpi_os_write_memory(acpi_physical_address phys_addr,
>>> u32 value, u32 width)
>>>
>>>          return AE_OK;
>>>    }
>>> -
>>> +#ifdef CONFIG_X86
>>>    #define is_xmalloc_memory(ptr) ((unsigned long)(ptr) & (PAGE_SIZE - 1))
>>> +#else
>>> +#define is_xmalloc_memory(ptr) 1
>>> +#endif
>>
>>
>> Why? I though this was resolved?
> i am not aware what was the resolution on it?

I didn't have access on my IRC log in the plane. So I wasn't able to 
find the discussion.

AFAIR, this issue was because we are allocating a chunk bigger than 
PAGE_SIZE in acpi_tb_resize_root_table_list.

x86 manage to never allocate a chunk bigger than PAGE_SIZE. So I'm not 
sure why ARM64 requires too. This would need more debug.

This macro is used to know if the memory has been allocated by the boot 
allocator of the memory pool.

So 2 solutions:
	1) We find a better way to use detect it
	2) We avoid to allocate big chunk

I'm not sure which one is the easiest/better one.

Regards,
Parth Dixit Feb. 5, 2015, 7:30 p.m. UTC | #4
On 5 February 2015 at 23:18, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Wed, 4 Feb 2015, parth.dixit@linaro.org wrote:
>> From: Parth Dixit <parth.dixit@linaro.org>
>>
>> Some bugs are identified in edk2 and some of the functionality is not
>> yet merged. This patch contains workarounds for them
>
> A patch with a few workarounds left is OK, but you should explain
> exactly what they are and why you weren't able to solve the relative
> issues properly.
>
>
>> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
>> ---
>>  xen/arch/arm/domain_build.c | 82 ++++++++++++++++++++++++++++++++++++++++++++-
>>  xen/arch/arm/vgic.c         | 16 +++++++++
>>  xen/drivers/acpi/osl.c      |  7 ++--
>>  3 files changed, 102 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index a504064..a425ef4 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1234,7 +1234,87 @@ static int make_chosen_node(const struct domain *d, const struct kernel_info *ki
>>
>>      return res;
>>  }
>> +#define ACPI_UEFI_MEM_STUB
>> +
>> +#ifdef ACPI_UEFI_MEM_STUB
>> +enum{
>> +    IO_NET0,
>> +    IO_SREG,
>> +    IO_VIRT,
>> +    IO_SCT0,
>> +    IO_AAC0,
>> +    IO_MMC0,
>> +    IO_KMI0,
>> +    IO_KMI1,
>> +    IO_SER1,
>> +    IO_SER2,
>> +    IO_SER3,
>> +    IO_WDT0,
>> +    IO_TIM0,
>> +    IO_TIM2,
>> +    IO_RTC0,
>> +    IO_TIM3,
>> +    IO_TIM4,
>> +    IO_MAX
>> +};
>> +#define INIT_IO( dev,addr,size ) [dev] = {addr,size}
>> +
>> +static const u64 acpi_mmio_region[][IO_MAX]=
>> +    {
>> +        INIT_IO(IO_NET0,0x1a000000,(PAGE_SIZE*16) ),
>> +        INIT_IO(IO_SREG,0x1c010000,PAGE_SIZE),
>> +        INIT_IO(IO_VIRT,0x1c130000,PAGE_SIZE),
>> +        INIT_IO(IO_SCT0,0x1c020000,PAGE_SIZE),
>> +        INIT_IO(IO_AAC0,0x1c040000,PAGE_SIZE),
>> +        INIT_IO(IO_MMC0,0x1c050000,PAGE_SIZE),
>> +        INIT_IO(IO_KMI0,0x1c060000,PAGE_SIZE),
>> +        INIT_IO(IO_KMI1,0x1c070000,PAGE_SIZE),
>> +        INIT_IO(IO_SER1,0x1c0a0000,PAGE_SIZE),
>> +        INIT_IO(IO_SER2,0x1c0b0000,PAGE_SIZE),
>> +        INIT_IO(IO_SER3,0x1c0c0000,PAGE_SIZE),
>> +        INIT_IO(IO_WDT0,0x1c0f0000,PAGE_SIZE),
>> +        INIT_IO(IO_TIM0,0x1c110000,PAGE_SIZE),
>> +        INIT_IO(IO_TIM2,0x1c120000,PAGE_SIZE),
>> +        INIT_IO(IO_RTC0,0x1c170000,PAGE_SIZE),
>> +        INIT_IO(IO_TIM3,0x2a810000,(PAGE_SIZE*16) ),
>> +        INIT_IO(IO_TIM4,0x2a830000,(PAGE_SIZE*16) ),
>> +    };
>
> What is this?
This is the mmio map of fvp model as these regions are not described
in uefi firmware right now,
bug is already filed for it.
>
>> +int acpi_map_mmio(struct domain *d)
>> +{
>> +    int i,res;
>> +    u64 addr,size;
>> +
>> +    for (i = 0; i < IO_MAX; i++)
>> +    {
>> +        addr = acpi_mmio_region[i][0];
>> +        size = acpi_mmio_region[i][1];
>>
>> +        res = iomem_permit_access(d, paddr_to_pfn(addr & PAGE_MASK),
>> +                                  paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
>> +        res = map_mmio_regions(d,
>> +                               paddr_to_pfn(addr & PAGE_MASK),
>> +                               DIV_ROUND_UP(size, PAGE_SIZE),
>> +                               paddr_to_pfn(addr & PAGE_MASK));
>> +
>> +
>> +    }
>> +#if 0
>> +     for( i=32 ; i < 255 ; i++ )
>> +     {
>> +        res = vgic_reserve_virq(d, i);
>> +        res = route_irq_to_guest(d, i, NULL);
>> +         if ( res )
>> +        {
>> +            printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
>> +                   i, d->domain_id);
>> +        }
>> +
>> +     }
>> +#endif
>> +     return 0;
>> +}
>> +#else
>>  static int acpi_map_mmio(struct domain *d)
>>  {
>>      int i,res;
>> @@ -1273,7 +1353,7 @@ static int acpi_map_mmio(struct domain *d)
>>
>>       return 0;
>>  }
>> -
>> +#endif
>
> This is pretty terrible. You are ifdef'ing out the entire function that
> you introduced earlier.
i will refactor it to make it more clear, its essentially same
function but is reading mmio regions locally instead of uefi tables.
>
>>  static int map_acpi_regions(struct domain *d)
>>  {
>>      int res;
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 97061ce..e74555d 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -254,6 +254,8 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>>      }
>>  }
>>
>> +#define VGIC_ICFG_MASK(intr) ( 1 << ( ( 2 * ( intr % 16 ) ) + 1 ) )
>> +
>>  void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>>  {
>>      struct domain *d = v->domain;
>> @@ -266,6 +268,20 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>>
>>      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>>          irq = i + (32 * n);
>> +#if defined(CONFIG_ARM_64) && defined(CONFIG_ACPI)
>> +        if( ( n!=0 ) && is_hardware_domain(d) ){
>> +            struct vgic_irq_rank *vr = vgic_get_rank(v, n);
>> +            uint32_t tr;
>> +            tr = vr->icfg[i >> 4] ;
>> +
>> +            if( ( tr & VGIC_ICFG_MASK(i) ) )
>> +                acpi_set_irq(irq, DT_IRQ_TYPE_EDGE_BOTH);
>> +            else
>> +                acpi_set_irq(irq, DT_IRQ_TYPE_LEVEL_MASK);
>> +
>> +            route_irq_to_guest(d,irq,NULL);
>> +        }
>> +#endif
>
> Is this the code that currently assigns all the irqs to dom0?
yes
>
>>          v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
>>          p = irq_to_pending(v_target, irq);
>>          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
>> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
>> index 73da9d9..2d78ba0 100644
>> --- a/xen/drivers/acpi/osl.c
>> +++ b/xen/drivers/acpi/osl.c
>> @@ -66,7 +66,7 @@ void __init acpi_os_vprintf(const char *fmt, va_list args)
>>
>>  acpi_physical_address __init acpi_os_get_root_pointer(void)
>>  {
>> -     if (efi_enabled) {
>> +    if (efi_enabled) {
>>               if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
>>                       return efi.acpi20;
>>               else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
>> @@ -198,8 +198,11 @@ acpi_os_write_memory(acpi_physical_address phys_addr, u32 value, u32 width)
>>
>>       return AE_OK;
>>  }
>> -
>> +#ifdef CONFIG_X86
>>  #define is_xmalloc_memory(ptr) ((unsigned long)(ptr) & (PAGE_SIZE - 1))
>> +#else
>> +#define is_xmalloc_memory(ptr) 1
>> +#endif
>
> Why?
this test is failing,leading to crash.
>
>>  void *__init acpi_os_alloc_memory(size_t sz)
>>  {
>> --
>> 1.9.1
>>
Julien Grall Feb. 10, 2015, 9:38 a.m. UTC | #5
Hi Parth,

On 05/02/2015 22:59, Julien Grall wrote:
>>>> +#ifdef CONFIG_X86
>>>>    #define is_xmalloc_memory(ptr) ((unsigned long)(ptr) & (PAGE_SIZE
>>>> - 1))
>>>> +#else
>>>> +#define is_xmalloc_memory(ptr) 1
>>>> +#endif
>>>
>>>
>>> Why? I though this was resolved?
>> i am not aware what was the resolution on it?
>
> I didn't have access on my IRC log in the plane. So I wasn't able to
> find the discussion.
>
> AFAIR, this issue was because we are allocating a chunk bigger than
> PAGE_SIZE in acpi_tb_resize_root_table_list.
>
> x86 manage to never allocate a chunk bigger than PAGE_SIZE. So I'm not
> sure why ARM64 requires too. This would need more debug.
>
> This macro is used to know if the memory has been allocated by the boot
> allocator of the memory pool.
>
> So 2 solutions:
>      1) We find a better way to use detect it
>      2) We avoid to allocate big chunk
>
> I'm not sure which one is the easiest/better one.

So, I took time to understand better the issue. This is happening at the 
first call of acpi_tb_resize_root_table_list (see stack trace at the end 
of the mail).

We are trying to allocate ACPI_MAX_TABLES * sizeof (struct acpi_table_desc).

The size of the structure is 32 bytes which give a total allocation of 
4096 bytes on both x86 and ARM64.

Why it's working on x86? This big allocation is done via the boot 
allocator memory (because the system state is early boot). Hopefully, we 
never have to resize it.

On ARM64, ACPI is initialized after the boot allocator has ended, so we 
have to use xmalloc which will return a page-align pointer.

As ACPI on ARM64 will never use the boot allocator, maybe the 
xmalloc_is_ptr is acceptable? It would require a big explanation which 
it's safe.

What do you think?

[<0000000000284bfc>] acpi_os_zalloc_memory+0x5c/0x8c (PC)
[<0000000000284be8>] acpi_os_zalloc_memory+0x48/0x8c (LR)
[<000000000028571c>] acpi_tb_resize_root_table_list+0x94/0x124
[<00000000002857d0>] acpi_allocate_root_table+0x24/0x2c
[<0000000000285800>] acpi_initialize_tables+0x28/0x98
[<000000000028663c>] acpi_table_init+0x1c/0x88
[<000000000028bcbc>] acpi_boot_table_init+0x1c/0x48
[<0000000000289d08>] start_xen+0x574/0xcbc
[<0000000000200610>] paging+0x88/0xc0

Regards,
diff mbox

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index a504064..a425ef4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1234,7 +1234,87 @@  static int make_chosen_node(const struct domain *d, const struct kernel_info *ki
 
     return res;
 }
+#define ACPI_UEFI_MEM_STUB
+
+#ifdef ACPI_UEFI_MEM_STUB
+enum{
+    IO_NET0,
+    IO_SREG,
+    IO_VIRT,
+    IO_SCT0,
+    IO_AAC0,
+    IO_MMC0,
+    IO_KMI0,
+    IO_KMI1,
+    IO_SER1,
+    IO_SER2,
+    IO_SER3,
+    IO_WDT0,
+    IO_TIM0,
+    IO_TIM2,
+    IO_RTC0,
+    IO_TIM3,
+    IO_TIM4,
+    IO_MAX
+};
+#define INIT_IO( dev,addr,size ) [dev] = {addr,size}
+
+static const u64 acpi_mmio_region[][IO_MAX]=
+    {
+        INIT_IO(IO_NET0,0x1a000000,(PAGE_SIZE*16) ),
+        INIT_IO(IO_SREG,0x1c010000,PAGE_SIZE),
+        INIT_IO(IO_VIRT,0x1c130000,PAGE_SIZE),
+        INIT_IO(IO_SCT0,0x1c020000,PAGE_SIZE),
+        INIT_IO(IO_AAC0,0x1c040000,PAGE_SIZE),
+        INIT_IO(IO_MMC0,0x1c050000,PAGE_SIZE),
+        INIT_IO(IO_KMI0,0x1c060000,PAGE_SIZE),
+        INIT_IO(IO_KMI1,0x1c070000,PAGE_SIZE),
+        INIT_IO(IO_SER1,0x1c0a0000,PAGE_SIZE),
+        INIT_IO(IO_SER2,0x1c0b0000,PAGE_SIZE),
+        INIT_IO(IO_SER3,0x1c0c0000,PAGE_SIZE),
+        INIT_IO(IO_WDT0,0x1c0f0000,PAGE_SIZE),
+        INIT_IO(IO_TIM0,0x1c110000,PAGE_SIZE),
+        INIT_IO(IO_TIM2,0x1c120000,PAGE_SIZE),
+        INIT_IO(IO_RTC0,0x1c170000,PAGE_SIZE),
+        INIT_IO(IO_TIM3,0x2a810000,(PAGE_SIZE*16) ),
+        INIT_IO(IO_TIM4,0x2a830000,(PAGE_SIZE*16) ),
+    };
+
+int acpi_map_mmio(struct domain *d)
+{
+    int i,res;
+    u64 addr,size;
+
+    for (i = 0; i < IO_MAX; i++)
+    {
+        addr = acpi_mmio_region[i][0];
+        size = acpi_mmio_region[i][1];
 
+        res = iomem_permit_access(d, paddr_to_pfn(addr & PAGE_MASK),
+                                  paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
+        res = map_mmio_regions(d,
+                               paddr_to_pfn(addr & PAGE_MASK),
+                               DIV_ROUND_UP(size, PAGE_SIZE),
+                               paddr_to_pfn(addr & PAGE_MASK));
+
+
+    }
+#if 0
+     for( i=32 ; i < 255 ; i++ )
+     {
+        res = vgic_reserve_virq(d, i);
+        res = route_irq_to_guest(d, i, NULL);
+         if ( res )
+        {
+            printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
+                   i, d->domain_id);
+        }
+
+     }
+#endif
+     return 0;
+}
+#else
 static int acpi_map_mmio(struct domain *d)
 {
     int i,res;
@@ -1273,7 +1353,7 @@  static int acpi_map_mmio(struct domain *d)
 
      return 0;
 }
-
+#endif
 static int map_acpi_regions(struct domain *d)
 {
     int res;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 97061ce..e74555d 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -254,6 +254,8 @@  void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
     }
 }
 
+#define VGIC_ICFG_MASK(intr) ( 1 << ( ( 2 * ( intr % 16 ) ) + 1 ) )
+
 void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
 {
     struct domain *d = v->domain;
@@ -266,6 +268,20 @@  void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
 
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
+#if defined(CONFIG_ARM_64) && defined(CONFIG_ACPI)
+        if( ( n!=0 ) && is_hardware_domain(d) ){
+            struct vgic_irq_rank *vr = vgic_get_rank(v, n);
+            uint32_t tr;
+            tr = vr->icfg[i >> 4] ;
+
+            if( ( tr & VGIC_ICFG_MASK(i) ) )
+                acpi_set_irq(irq, DT_IRQ_TYPE_EDGE_BOTH);
+            else
+                acpi_set_irq(irq, DT_IRQ_TYPE_LEVEL_MASK);
+
+            route_irq_to_guest(d,irq,NULL);
+        }
+#endif
         v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
         p = irq_to_pending(v_target, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
index 73da9d9..2d78ba0 100644
--- a/xen/drivers/acpi/osl.c
+++ b/xen/drivers/acpi/osl.c
@@ -66,7 +66,7 @@  void __init acpi_os_vprintf(const char *fmt, va_list args)
 
 acpi_physical_address __init acpi_os_get_root_pointer(void)
 {
-	if (efi_enabled) {
+    if (efi_enabled) {
 		if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
 			return efi.acpi20;
 		else if (efi.acpi != EFI_INVALID_TABLE_ADDR)
@@ -198,8 +198,11 @@  acpi_os_write_memory(acpi_physical_address phys_addr, u32 value, u32 width)
 
 	return AE_OK;
 }
-
+#ifdef CONFIG_X86
 #define is_xmalloc_memory(ptr) ((unsigned long)(ptr) & (PAGE_SIZE - 1))
+#else
+#define is_xmalloc_memory(ptr) 1
+#endif
 
 void *__init acpi_os_alloc_memory(size_t sz)
 {