Re: [Xen-devel] [PATCH] xen/arm: vgic-v3: Fix the typo of GICD IRQ active status range

[Julien Grall <julien@xxxxxxx>]

On 07/01/2020 08:39, Wei Xu wrote:
> Hi Stefano,
>
> On 2020/1/7 6:01, Stefano Stabellini wrote:
> > On Sat, 28 Dec 2019, Wei Xu wrote:
> > > Hi Julien,
> > >
> > > On 2019/12/28 16:09, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > On 28/12/2019 03:08, Wei Xu wrote:
> > > > > This patch fixes the typo about the active status range of an IRQ
> > > > > via GICD. Otherwise it will be failed to handle the mmio access and
> > > > > inject a data abort.
> > > > I have seen a patch similar from NXP a month ago and I disagreed on the
> > > > approach.
> > > >
> > > > If you look at the context you modifed, it says that reading ACTIVER 
> > > > is not
> > > > supported. While I agree the behavior is not consistent accross 
> > > > ACTIVER,
> > > > injecting a data abort is a perfectly fine behavior to me (though 
> > > > not spec
> > > > compliant) as we don't implement the registers correctly.
> > > >
> > > > I guess you are sending this patch, because you tried Linux 5.4 (or 
> > > > later)
> > > > on Xen, right? Linux has recently began to read ACTIVER to check 
> > > > whether an
> > > > IRQ is active at the HW level during the synchronizing of the IRQS. 
> > > > From my
> > > > understanding, this is used because there is a window where the 
> > > > interrupt is
> > > > active at the HW level but the Linux IRQ subsystem is not aware of it.
> > > >
> > > > While the patch below will allow Linux 5.4 to not crash, it is not 
> > > > going to
> > > > make it fly very far because of the above. So I am rather not happy 
> > > > with
> > > > persuing with returning 0.
> > > Yes, I am using Linux 5.5-rc2 :)
> > > Got it and thanks for the explanation.
> > > I am not insistent on this and OK to wait for the update.
> > > Thanks and have a very happy new year!
> > Hi Wei,
> >
> > what do you do to reproduce the issue? Are you just booting Linux
> > 5.5-rc2 as dom0 and seeing the issue during boot, or are you doing
> > something specific?
> >
> > .
> I directly tested the mainline kernel with defconfig.
> And the 5.5-rc5 kernel booting log is as below:
>
>      root@ubuntu:~# dmesg | more
>      [    0.000000] Booting Linux on physical CPU 0x0000000000 [0x481fd010]
>      [    0.000000] Linux version 5.5.0-rc5 (joyx@Turing-Arch-b) (gcc 
> version 4.9.1 2
>      0140505 (prerelease) (crosstool-NG linaro-1.13.1-4.9-2014.05 - 
> Linaro GCC 4.9-20
>      14.05)) #132 SMP PREEMPT Tue Jan 7 15:43:06 CST 2020
>      [    0.000000] Xen XEN_VERSION.XEN_SUBVERSION support found
>      [    0.000000] efi: Getting EFI parameters from FDT:
>      [    0.000000] efi: EFI v2.50 by Xen
>      [    0.000000] efi:  ACPI 2.0=0x181d0e70
>      [    0.000000] cma: Reserved 32 MiB at 0x000000007e000000
>      [    0.000000] ACPI: Early table checksum verification disabled
>      [    0.000000] ACPI: RSDP 0x00000000181D0E70 000024 (v02 HISI  )
>      [    0.000000] ACPI: XSDT 0x00000000181D0DB0 0000BC (v01 HISI 
> HIP08    0000000
>      0      01000013)
Is that the full log from Linux? If not, can you post it in full?

> But to boot with ACPI on our hardware, except above change I have also 
> done some hacking based on
> XEN 4.13 as below:
I haven't booted Xen on any ACPI systems recently so there might be bugs 
in the code. Your changes below is definitely a call to look more into 
details what's wrong.
>      diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>      index d028ec9..215a291 100644
>      --- a/xen/arch/arm/traps.c
>      +++ b/xen/arch/arm/traps.c
>      @@ -1856,8 +1856,8 @@ static bool try_map_mmio(gfn_t gfn)
>               return false;
>
>           /* The hardware domain can only map permitted MMIO regions */
>      -    if ( !iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + 1) )
>      -        return false;
>      +    /* if ( !iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + 1) 
> ) */
>      +    /*     return false; */
Dom0 should be able to map nearly all the address space through this 
function. The only thing not allowed is the GIC and UART (see 
acpi_iomem_deny_access).
So why do you want this change? What sort of address Dom0 is trying to 
map and fail?
>           return !map_regions_p2mt(d, gfn, 1, mfn, p2m_mmio_direct_c);
>       }
>
>
>      diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>      index 4d6c971..c626f9e 100644
>      --- a/xen/arch/arm/mm.c
>      +++ b/xen/arch/arm/mm.c
>      @@ -1095,7 +1095,7 @@ static bool xen_pt_check_entry(lpae_t entry, 
> mfn_t mfn, unsigned int flags)
>               {
>                   mm_printk("Changing MFN for a valid entry is not 
> allowed (%#"PRI_mfn" -> %#"PRI_mfn").\n",
>                             mfn_x(lpae_get_mfn(entry)), mfn_x(mfn));
>      -            return false;
>      +            return true;
There is a pretty good reason to prevent modifying the MFN on a valid 
entry. Indeed, the PT code is not handling of the Break-Before-Make 
sequence (requires when updating certain entry) as this is a can of worms.
However, during my testing I never found a place where an valid entry is 
modified (other than the permissions part). So can you give more details 
how you came up here?
>               }
>           }
>           /* Sanity check when removing a page. */
>
>
>      diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>      index 3c899cd..1e83351 100644
>      --- a/xen/arch/arm/setup.c
>      +++ b/xen/arch/arm/setup.c
>      @@ -852,7 +852,8 @@ void __init start_xen(unsigned long 
> boot_phys_offset,
>           else
>           {
>               printk("Booting using ACPI\n");
>      -        device_tree_flattened = NULL;
>      +        device_tree_flattened = relocate_fdt(fdt_paddr, fdt_size);
>      +        dt_unflatten_host_device_tree();
When using ACPI, the DT should not be used. So why do you need this?

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel