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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 6 Feb 2015, Parth Dixit wrote:
> On 5 February 2015 at 23:18, Stefano Stabellini
> <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > On Wed, 4 Feb 2015, parth.dixit@xxxxxxxxxx wrote:
> >> From: Parth Dixit <parth.dixit@xxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxx>
> >> ---
> >>  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.

OK


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

thanks


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

It is not terrible, but it might be best if you just add a call there to
a new function, you can call it acpi_enable_dom0_irq.


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

Actually I don't understand this check even on x86.

Jan, why are we assuming that xmalloc cannot return align pointers on
x86?

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