Re: [PATCH RFC 34/35] arm : acpi workarounds for firmware/linux dependencies

[Julien Grall <julien.grall@linaro.org>]

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,

-- 
Julien Grall