| Message ID | 1577498922-192711-1-git-send-email-xuwei5@hisilicon.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Xen-devel] xen/arm: vgic-v3: Fix the typo of GICD IRQ active status range | expand |
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. @Stefano, you mention you will look at implementing ACTIVER. What's the state? > > Fixes: a2b83f95bfad ("xen/arm: vgic: Properly emulate the full register") > > Signed-off-by: Wei Xu <xuwei5@hisilicon.com> > --- > xen/arch/arm/vgic-v3.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index 422b94f..e802f20 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -706,7 +706,7 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v, > goto read_as_zero; > > /* Read the active status of an IRQ via GICD/GICR is not supported */ > - case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVER): > + case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN): > case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN): > goto read_as_zero; > > Cheers,
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! Best Regards, Wei > @Stefano, you mention you will look at implementing ACTIVER. What's > the state? > > > >> Fixes: a2b83f95bfad ("xen/arm: vgic: Properly emulate the full >> register") >> >> Signed-off-by: Wei Xu <xuwei5@hisilicon.com> >> --- >> xen/arch/arm/vgic-v3.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >> index 422b94f..e802f20 100644 >> --- a/xen/arch/arm/vgic-v3.c >> +++ b/xen/arch/arm/vgic-v3.c >> @@ -706,7 +706,7 @@ static int __vgic_v3_distr_common_mmio_read(const >> char *name, struct vcpu *v, >> goto read_as_zero; >> /* Read the active status of an IRQ via GICD/GICR is not >> supported */ >> - case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVER): >> + case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN): >> case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN): >> goto read_as_zero; >> > > Cheers, >
Hi Julien, On 2020/1/7 23:12, Julien Grall wrote: > > > On 07/01/2020 12:55, Wei Xu wrote: >> Hi Julien, >>> As only one entity should manage the UART (i.e Xen or Dom0), we >>> today assume this will be managed by Xen. Xen should expose a >>> partial virtual UART (only a few registers are emulating) to dom0 in >>> replacement. >>> >>> This is usually done by the UART driver. Looking at the log you >>> pasted in a separate e-mail: >>> >>> (XEN) Platform: Generic System >>> (XEN) Unable to initialize acpi uart: -9 >>> (XEN) Bad console= option 'dtuart' >>> >>> So Xen didn't manage to initialize the uart. The -9 suggests, Xen >>> didn't find a driver for your UART. At the moment, Xen is only able >>> to detect pl011, sbsa, sbsa32 UART for ACPI. What is the type of the >>> UART used on your platform? >>> >> >> Thanks! >> Got it. >> Our UART is 8250. > > You would need to teach the 8250 driver how to initialize the UART > with ACPI. It is not very difficult to do it, have a look at the pl011 > version. > >> >> Thanks! >> It is not working even I changed the condition to " if ( >> acpi_disabled ) ". > > Doh, thank you for spotting the extra !. > >> My grub 2.04 configuration is as below: >> >> xen_hypervisor /xen dom0_mem=4G acpi=force loglvl=all >> guest_loglvl=all >> xen_module /Image rdinit=/init acpi=force noinitrd >> root=/dev/sdb1 rw >> >> The log with the condition " if ( acpi_disabled ) " is as following: >> >> (XEN) Adding cpu 126 to runqueue 0 >> (XEN) Adding cpu 127 to runqueue 0 >> (XEN) alternatives: Patching with alt table 00000000002d4f48 -> >> 00000000002d5764 >> (XEN) *** LOADING DOMAIN 0 *** >> (XEN) Loading d0 kernel from boot module @ 0000000016257000 >> (XEN) Allocating 1:1 mappings totalling 4096MB for dom0: >> (XEN) BANK[0] 0x00000008000000-0x00000010000000 (128MB) >> (XEN) BANK[1] 0x00000020000000-0x00000038000000 (384MB) >> (XEN) BANK[2] 0x00000050000000-0x00000080000000 (768MB) >> (XEN) BANK[3] 0x00202000000000-0x00202080000000 (2048MB) >> (XEN) BANK[4] 0x002020b0000000-0x002020c0000000 (256MB) >> (XEN) BANK[5] 0x00202600000000-0x00202620000000 (512MB) >> (XEN) Grant table range: 0x000000181c7000-0x00000018207000 >> (XEN) Allocating PPI 16 for event channel interrupt >> (XEN) Loading zImage from 0000000016257000 to >> 0000000008080000-0000000009981200 >> (XEN) Loading d0 DTB to 0x000000000fe00000-0x000000000fe0025b >> (XEN) Initial low memory virq threshold set at 0x4000 pages. >> (XEN) Scrubbing Free RAM in background >> (XEN) Std. Loglevel: All >> (XEN) Guest Loglevel: All >> (XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to >> switch input) >> (XEN) Data Abort Trap. Syndrome=0x6 >> (XEN) Walking Hypervisor VA 0x10 on CPU0 via TTBR >> 0x00000000182ff000 >> (XEN) 0TH[0x0] = 0x0000000018302f7f >> (XEN) 1ST[0x0] = 0x0000000018300f7f >> (XEN) 2ND[0x0] = 0x0000000000000000 >> (XEN) CPU0: Unexpected Trap: Data Abort >> (XEN) ----[ Xen-4.13.0-rc arm64 debug=y Not tainted ]---- >> (XEN) CPU: 0 >> (XEN) PC: 00000000002b65c8 00000000002b65c8 > > Can you look with addr2line what this PC refers to? > Sorry for the late reply! The PC refers to fdt_num_mem_rsv during init_done. But the device_tree_flattened is NULL that the data abort happened. So I added below changes and the XEN dom0 can be booted. diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 1e83351..1ab80a1 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -392,7 +392,8 @@ void __init discard_initial_modules(void) !mfn_valid(maddr_to_mfn(e)) ) continue; - dt_unreserved_regions(s, e, init_domheap_pages, 0); + if( acpi_disabled ) + dt_unreserved_regions(s, e, init_domheap_pages, 0); } Thank you so much! Best Regards, Wei > Cheers, >
On 2020/1/18 4:41, Julien Grall wrote: > (Renaming the title to avoid confusion) > > On 17/01/2020 09:06, Wei Xu wrote: >> Hi Julien, > > Hi Wei, > >> On 2020/1/7 23:12, Julien Grall wrote: >> Sorry for the late reply! > > Don't worry, thank you for looking into the bug! > >> The PC refers to fdt_num_mem_rsv during init_done. >> But the device_tree_flattened is NULL that the data abort happened. > > Ah, I didn't realize that device_tree_flattened where still used > afterwards. Sorry for the breakage. I really need to setup a devbox > with ACPI so I can test changes properly. > >> So I added below changes and the XEN dom0 can be booted. >> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index 1e83351..1ab80a1 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -392,7 +392,8 @@ void __init discard_initial_modules(void) >> !mfn_valid(maddr_to_mfn(e)) ) >> continue; >> >> - dt_unreserved_regions(s, e, init_domheap_pages, 0); >> + if( acpi_disabled ) >> + dt_unreserved_regions(s, e, init_domheap_pages, 0); > > While I understand how this is fixing your problem, this unfortunately > means the memory ranges used by the inital modules (e.g Kernel, > Initrd) will not be re-used by Xen. So this is a "slight" waste of > memory. > > There are a few other places where dt_unreserved_regions() is called > (see setup_mm()). However, in the case of setup_mm() we have a pointer > to DT as we don't know yet we are running on ACPI systems. > > But it feels wrong to try to find out the reserved memory range > through the DT when ACPI will be used. The DT is either going to be > nearly empty, or contain the full description of the platform. I don't > know enough to be able to say if something is going to go wrong. > > I am thinking to suggest to create an helper that will do the job for > you. Something like: > > void fwtable_unreserved_regions(paddr_t s, paddr_t e, > void (*cb)(paddr_t, paddr_t), int first) > { > if ( acpi_disabled ) > dt_unreserved_regions(s, e, cb, first); > else > cb(s, e); > } > > Regarding the else part, this may need some adjustment if we need to > skip some reserved region for ACPI. On Xen 4.13, we should only have > usuable RAM in hand (the EFI stub is doing to sorting for us). Do you > know whether ACPI describes something similar to reserved-memory in DT > (i.e RAM regions reserved for cma...)? > Hi Julien, I think UEFI describes it via ARM_MEMORY_REGION_DESCRIPTOR[1] and XEN parses it at efi_init_memory but I did not find where to reserve the memory. 1: https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoMem.c Thanks! Best Regards, Wei > Cheers, >
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 422b94f..e802f20 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -706,7 +706,7 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v, goto read_as_zero; /* Read the active status of an IRQ via GICD/GICR is not supported */ - case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVER): + case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN): case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN): goto read_as_zero;
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. Fixes: a2b83f95bfad ("xen/arm: vgic: Properly emulate the full register") Signed-off-by: Wei Xu <xuwei5@hisilicon.com> --- xen/arch/arm/vgic-v3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)