Re: [PATCH v5 1/2] tools/console: Add escape argument to configure escape character
On 2023/7/12 18:29, Peter Hoyes wrote:
From: Peter Hoyes
Dom0 may be accessed via telnet, meaning the default escape character
(which is the same as telnet's) cannot be directly used to exit the
console. It would be helpful to make the escape character customizable
in such use cases.
Add --escape argument to console tool for this purpose.
Add argument to getopt options, parse and validate the escape character
and pass value to console_loop.
If --escape is not specified, it falls back to the existing behavior
using DEFAULT_ESCAPE_SEQUENCE.
Signed-off-by: Peter Hoyes
---
Changes in v5:
- Add this changelog
Changes in v4:
- Improve validation of the escape_character optarg
Changes in v3:
- Re-add the Reviewed-By tag accidentally removed in v2
Changes in v2:
- Drop the tags intended only for internal use at Arm
tools/console/client/main.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index 6775006488..d2dcc3ddca 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -42,7 +42,7 @@
#include
#include "xenctrl.h"
-#define ESCAPE_CHARACTER 0x1d
+#define DEFAULT_ESCAPE_CHARACTER 0x1d
static volatile sig_atomic_t received_signal = 0;
static char lockfile[sizeof (XEN_LOCK_DIR "/xenconsole.") + 8] = { 0 };
@@ -77,6 +77,7 @@ static void usage(const char *program) {
" -n, --num N use console number N\n"
" --type TYPE console type. must be 'pv', 'serial' or
'vuart'\n"
" --start-notify-fd N file descriptor used to notify parent\n"
+ " --escape E escape sequence to exit console\n"
, program);
}
@@ -174,7 +175,7 @@ static void restore_term(int fd, struct termios *old)
}
static int console_loop(int fd, struct xs_handle *xs, char *pty_path,
- bool interactive)
+ bool interactive, char escape_character)
{
int ret, xs_fd = xs_fileno(xs), max_fd = -1;
@@ -215,7 +216,7 @@ static int console_loop(int fd, struct xs_handle *xs, char *pty_path,
char msg[60];
len = read(STDIN_FILENO, msg, sizeof(msg));
- if (len == 1 && msg[0] == ESCAPE_CHARACTER) {
+ if (len == 1 && msg[0] == escape_character) {
return 0;
}
@@ -335,6 +336,7 @@ int main(int argc, char **argv)
{ "help",0, 0, 'h' },
{ "start-notify-fd", 1, 0, 's' },
{ "interactive", 0, 0, 'i' },
+ { "escape", 1, 0, 'e' },
{ 0 },
};
@@ -345,6 +347,7 @@ int main(int argc, char **argv)
console_type type = CONSOLE_INVAL;
bool interactive = 0;
const char *console_names = "serial, pv, vuart";
+ char escape_character = DEFAULT_ESCAPE_CHARACTER;
while((ch = getopt_long(argc, argv, sopt, lopt, _ind)) != -1) {
switch(ch) {
@@ -375,6 +378,16 @@ int main(int argc, char **argv)
case 'i':
interactive = 1;
break;
+ case 'e':
+ if (optarg[0] == '^' && optarg[1] && optarg[2] == '\0')
+ escape_character = optarg[1] & 0x1f;
+ else if (optarg[0] && optarg[1] == '\0')
+ escape_character = optarg[0];
+ else {
+ fprintf(stderr, "Invalid escape argument\n");
+ exit(EINVAL);
+ }
+ break;
default:
fprintf(stderr, "Invalid argument\n");
fprintf(stderr, "Try `%s --help' for more
information.\n",
@@ -493,7 +506,7 @@ int main(int argc, char **argv)
close(start_notify_fd);
}
- console_loop(spty, xs, path, interactive);
+ console_loop(spty, xs, path, interactive, escape_character);
free(path);
free(dom_path);
Nice work~
Reviewed-by: Hongda Deng
Re: [PATCH] xen/arm: vgic: Add missing 'U' in VGIC_ICFG_MASK for shifted constant
On 2023/6/30 07:03, Stefano Stabellini wrote:
On Fri, 30 Jun 2023, Henry Wang wrote:
With UBSAN on some arm64 platforms, e.g. FVP_Base_RevC-2xAEMvA, the
following splat will be printed while Dom0 is booting:
```
(XEN) ==
(XEN) UBSAN: Undefined behaviour in arch/arm/vgic.c:372:15
(XEN) left shift of 1 by 31 places cannot be represented in type 'int'
(XEN) Xen WARN at common/ubsan/ubsan.c:172
(XEN) [ Xen-4.18-unstable arm64 debug=y ubsan=y Not tainted ]
```
This is because there is a device node in the device tree with 0xf
as the interrupts property. Example of the device tree node is shown
below:
```
ethernet@20200 {
compatible = "smsc,lan91c111";
reg = <0x2 0x200 0x1>;
interrupts = <0xf>;
};
```
and this value is passed to vgic_get_virq_type() as "index" then "intr"
in VGIC_ICFG_MASK.
Add the missing 'U' in VGIC_ICFG_MASK as a fix, and this should also
addressing MISRA Rule 7.2:
A "u" or "U" suffix shall be applied to all integer constants that
are represented in an unsigned type
Signed-off-by: Henry Wang
Reviewed-by: Stefano Stabellini
---
This patch should be based on top of Julien's series
"xen/arm: Enable UBSAN support" to test.
---
xen/arch/arm/vgic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index c61c68870c..97d6f61066 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -358,7 +358,7 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
}
}
-#define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1))
+#define VGIC_ICFG_MASK(intr) (1U << ((2 * ((intr) % 16)) + 1))
/* The function should be called with the rank lock taken */
static inline unsigned int vgic_get_virq_type(struct vcpu *v, int n, int
index)
--
2.25.1
Reviewed-by: Hongda Deng
[PATCH v2] arm/vgic-v3: fix virq offset in the rank when storing irouter
When vGIC performs irouter registers emulation, to get the target vCPU
via virq conveniently, Xen doesn't store the irouter value directly,
instead it will use the value (affinities) in irouter to calculate the
target vCPU, and then save the target vCPU in irq rank->vcpu[offset].
When vGIC tries to get the target vCPU, it first calculates the target
vCPU index via
int target = read_atomic(>vcpu[virq & INTERRUPT_RANK_MASK]);
and then it gets the target vCPU via
v->domain->vcpu[target];
When vGIC tries to store irouter for one virq, the target vCPU index
in the rank is computed as
offset &= virq & INTERRUPT_RANK_MASK;
finally it gets the target vCPU via
d->vcpu[read_atomic(>vcpu[offset])];
There is a difference between them while getting the target vCPU index
in the rank. Actually (virq & INTERRUPT_RANK_MASK) would already get
the target vCPU index in the rank, it's wrong to add '&' before '=' when
calculate the offset.
For example, the target vCPU index in the rank should be 6 for virq 38,
but vGIC will get offset=0 when vGIC stores the irouter for this virq,
and finally vGIC will access the wrong target vCPU index in the rank
when updating the irouter.
Fixes: 5d495f4349b5 ("xen/arm: vgic: Optimize the way to store the target vCPU
in the rank")
Signed-off-by: Hongda Deng
---
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 e4ba9a6476..7fb99a9ff2 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -135,7 +135,7 @@ static void vgic_store_irouter(struct domain *d, struct
vgic_irq_rank *rank,
ASSERT(virq >= 32);
/* Get the index in the rank */
-offset &= virq & INTERRUPT_RANK_MASK;
+offset = virq & INTERRUPT_RANK_MASK;
new_vcpu = vgic_v3_irouter_to_vcpu(d, irouter);
old_vcpu = d->vcpu[read_atomic(>vcpu[offset])];
--
2.25.1
[PATCH] arm/vgic-v3: fix virq offset in the rank
When vgic performs irouter registers emulation, to get the target vCPU
via virq conveniently, Xen doesn't store the irouter value directly,
instead it will use the value (affinities) in irouter to calculate the
target vCPU, and then save the target vCPU in irq rank->vCPU[offset].
But there seems to be a bug in the way the offset is calculated when
vgic tries to store irouter.
When vgic tries to get the target vcpu, it first calculates the target
vcpu index via
int target = read_atomic(>vcpu[virq & INTERRUPT_RANK_MASK]);
and then get the target vcpu via
v->domain->vcpu[target];
When vgic tries to store irouter for one virq, the target vcpu index
in the rank is got via
offset &= virq & INTERRUPT_RANK_MASK;
finally it gets the target vcpu via
d->vcpu[read_atomic(>vcpu[offset])];
There is a difference between them when gets the target vcpu index in
the rank.
Here (virq & INTERRUPT_RANK_MASK) would already get the target vcpu
index in the rank, so we don't need the '&' before '=' when calculate
the offset.
For example, the target vcpu index in the rank should be 6 for virq 38,
but vgic will get offset=0 when vgic stores the irouter for this virq,
and finally vgic will access wrong target vcpu index in the rank when
it updates the irouter.
Fixes: 5d495f4349b5 ("xen/arm: vgic: Optimize the way to store the target vCPU
in the rank")
Signed-off-by: Hongda Deng
---
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 e4ba9a6476..7fb99a9ff2 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -135,7 +135,7 @@ static void vgic_store_irouter(struct domain *d, struct
vgic_irq_rank *rank,
ASSERT(virq >= 32);
/* Get the index in the rank */
-offset &= virq & INTERRUPT_RANK_MASK;
+offset = virq & INTERRUPT_RANK_MASK;
new_vcpu = vgic_v3_irouter_to_vcpu(d, irouter);
old_vcpu = d->vcpu[read_atomic(>vcpu[offset])];
--
2.25.1
RE: [PATCH v3 05/19] xen/arm: mm: Add support for the contiguous bit
Hi Julien,
> -Original Message-
> From: Xen-devel On Behalf Of Julien
> Grall
> Sent: 2022年2月27日 3:30
> To: xen-devel@lists.xenproject.org
> Cc: Julien Grall ; Stefano Stabellini
> ; Bertrand Marquis ;
> Volodymyr Babchuk
> Subject: Re: [PATCH v3 05/19] xen/arm: mm: Add support for the contiguous bit
>
> Hi,
>
> On 21/02/2022 10:22, Julien Grall wrote:
> > @@ -1333,21 +1386,34 @@ static int xen_pt_update(unsigned long virt,
> > while ( left )
> > {
> > unsigned int order, level;
> > +unsigned int nr_contig;
> > +unsigned int new_flags;
> >
> > level = xen_pt_mapping_level(vfn, mfn, left, flags);
> > order = XEN_PT_LEVEL_ORDER(level);
> >
> > ASSERT(left >= BIT(order, UL));
> >
> > -rc = xen_pt_update_entry(root, pfn_to_paddr(vfn), mfn, level,
> > flags);
> > -if ( rc )
> > -break;
> > +/*
> > + * Check if we can set the contiguous mapping and update the
> > + * flags accordingly.
> > + */
> > +nr_contig = xen_pt_check_contig(vfn, mfn, level, left, flags);
> > +new_flags = flags | ((nr_contig > 1) ? _PAGE_CONTIG : 0);
> >
> > -vfn += 1U << order;
> > -if ( !mfn_eq(mfn, INVALID_MFN) )
> > -mfn = mfn_add(mfn, 1U << order);
> > +for ( ; nr_contig > 0; nr_contig-- )
> > +{
> > +rc = xen_pt_update_entry(root, pfn_to_paddr(vfn), mfn, level,
> > + new_flags);
> > +if ( rc )
> > +break;
> >
> > -left -= (1U << order);
> > +vfn += 1U << order;
> > +if ( !mfn_eq(mfn, INVALID_MFN) )
> > +mfn = mfn_add(mfn, 1U << order);
> > +
> > +left -= (1U << order);
> > +}
>
> I forgot to add:
>
> if ( rc )
>break;
>
> Without it, the outer loop will never exit in case of an error.
>
> Cheers,
>
> --
> Julien Grall
Yep, I am happy with that.
Reviewed-by: Hongda Deng
Cheers,
---
Hongda
RE: [PATCH v3 09/19] xen/arm32: mm: Check if the virtual address is shared before updating it
Hi Julien,
> -Original Message-
> From: Xen-devel On Behalf Of Julien
> Grall
> Sent: 2022年2月21日 18:22
> To: xen-devel@lists.xenproject.org
> Cc: jul...@xen.org; Julien Grall ; Stefano Stabellini
> ; Bertrand Marquis ;
> Volodymyr Babchuk
> Subject: [PATCH v3 09/19] xen/arm32: mm: Check if the virtual address is
> shared
> before updating it
>
> From: Julien Grall
>
> Only the first 2GB of the virtual address space is shared between all
> the page-tables on Arm32.
>
> There is a long outstanding TODO in xen_pt_update() stating that the
> function can only work with shared mapping. Nobody has ever called
> the function with private mapping, however as we add more callers
> there is a risk to mess things up.
>
> Introduce a new define to mark the end of the shared mappings and use
> it in xen_pt_update() to verify if the address is correct.
>
> Note that on Arm64, all the mappings are shared. Some compiler may
> complain about an always true check, so the new define is not introduced
> for arm64 and the code is protected with an #ifdef.
>
> Signed-off-by: Julien Grall
>
> ---
> Changes in v2:
> - New patch
> ---
> xen/arch/arm/include/asm/config.h | 4
> xen/arch/arm/mm.c | 11 +--
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/config.h
> b/xen/arch/arm/include/asm/config.h
> index c7b77912013e..85d4a510ce8a 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -137,6 +137,10 @@
>
> #define XENHEAP_VIRT_START _AT(vaddr_t,0x4000)
> #define XENHEAP_VIRT_END _AT(vaddr_t,0x7fff)
> +
> +/* The first 2GB is always shared between all the page-tables. */
> +#define SHARED_VIRT_END_AT(vaddr_t, 0x7fff)
> +
> #define DOMHEAP_VIRT_START _AT(vaddr_t,0x8000)
> #define DOMHEAP_VIRT_END _AT(vaddr_t,0x)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 24de8dcb9042..f18f65745595 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1365,11 +1365,18 @@ static int xen_pt_update(unsigned long virt,
> * For arm32, page-tables are different on each CPUs. Yet, they share
> * some common mappings. It is assumed that only common mappings
> * will be modified with this function.
> - *
> - * XXX: Add a check.
> */
> const mfn_t root = virt_to_mfn(THIS_CPU_PGTABLE);
>
> +#ifdef SHARED_VIRT_END
> +if ( virt > SHARED_VIRT_END ||
> + (SHARED_VIRT_END - virt) < nr_mfns )
Why not convert (SHARED_VIRT_END - virt) to page number before comparation?
I think nr_mfns is something related to page numbers, so maybe something like
PAGE_SHIFT or round_pgdown is needed.
I am just wondering, and forgive me if I am wrong.
> +{
> +mm_printk("Trying to map outside of the shared area.\n");
> +return -EINVAL;
> +}
> +#endif
> +
> /*
> * The hardware was configured to forbid mapping both writeable and
> * executable.
> --
> 2.32.0
>
Cheers,
---
Hongda
RE: [PATCH v3 08/19] xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()
Hi Julien,
> -Original Message-
> From: Xen-devel On Behalf Of Julien
> Grall
> Sent: 2022年2月21日 18:22
> To: xen-devel@lists.xenproject.org
> Cc: jul...@xen.org; Julien Grall ; Stefano Stabellini
> ; Bertrand Marquis ;
> Volodymyr Babchuk ; Julien Grall
>
> Subject: [PATCH v3 08/19] xen/arm: mm: Re-implement early_fdt_map() using
> map_pages_to_xen()
>
> From: Julien Grall
>
> Now that map_pages_to_xen() has been extended to support 2MB mappings,
> we can replace the create_mappings() calls by map_pages_to_xen() calls.
>
> The mapping can also be marked read-only has Xen as no business to
In my opinion I think it may should be:
... read-only as Xen has no business ...
instead of:
... read-only has Xen as no business ...
For this and other patches before this:
Reviewed-by: Hongda Deng
> modify the host Device Tree.
>
> Signed-off-by: Julien Grall
> Signed-off-by: Julien Grall
>
> ---
> Changes in v2:
> - Add my AWS signed-off-by
> - Fix typo in the commit message
> ---
> xen/arch/arm/mm.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index f088a4b2de96..24de8dcb9042 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -559,6 +559,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> paddr_t offset;
> void *fdt_virt;
> uint32_t size;
> +int rc;
>
> /*
> * Check whether the physical FDT address is set and meets the minimum
> @@ -574,8 +575,12 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> /* The FDT is mapped using 2MB superpage */
> BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);
>
> -create_mappings(xen_second, BOOT_FDT_VIRT_START,
> paddr_to_pfn(base_paddr),
> -SZ_2M >> PAGE_SHIFT, SZ_2M);
> +rc = map_pages_to_xen(BOOT_FDT_VIRT_START,
> maddr_to_mfn(base_paddr),
> + SZ_2M >> PAGE_SHIFT,
> + PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
> +if ( rc )
> +panic("Unable to map the device-tree.\n");
> +
>
> offset = fdt_paddr % SECOND_SIZE;
> fdt_virt = (void *)BOOT_FDT_VIRT_START + offset;
> @@ -589,9 +594,12 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>
> if ( (offset + size) > SZ_2M )
> {
> -create_mappings(xen_second, BOOT_FDT_VIRT_START + SZ_2M,
> -paddr_to_pfn(base_paddr + SZ_2M),
> -SZ_2M >> PAGE_SHIFT, SZ_2M);
> +rc = map_pages_to_xen(BOOT_FDT_VIRT_START + SZ_2M,
> + maddr_to_mfn(base_paddr + SZ_2M),
> + SZ_2M >> PAGE_SHIFT,
> + PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
> +if ( rc )
> +panic("Unable to map the device-tree\n");
> }
>
> return fdt_virt;
> --
> 2.32.0
>
Cheers,
---
Hongda
RE: [PATCH v3 19/19] xen/arm: mm: Re-implement setup_frame_table_mappings() with map_pages_to_xen()
Hi Julien,
> -Original Message-
> From: Xen-devel On Behalf Of Julien
> Grall
> Sent: 2022年2月21日 18:22
> To: xen-devel@lists.xenproject.org
> Cc: jul...@xen.org; Julien Grall ; Stefano Stabellini
> ; Bertrand Marquis ;
> Volodymyr Babchuk ; Julien Grall
>
> Subject: [PATCH v3 19/19] xen/arm: mm: Re-implement
> setup_frame_table_mappings() with map_pages_to_xen()
>
> From: Julien Grall
>
> Now that map_pages_to_xen() has been extended to support 2MB mappings,
> we can replace the create_mappings() call by map_pages_to_xen() call.
>
> This has the advantage to remove the differences between 32-bit and
> 64-bit code.
>
> Lastly remove create_mappings() as there is no more callers.
>
> Signed-off-by: Julien Grall
> Signed-off-by: Julien Grall
>
> ---
> Changes in v3:
> - Fix typo in the commit message
> - Remove the TODO regarding contiguous bit
>
> Changes in v2:
> - New patch
For the all 19 patches:
Tested-by: Hongda Deng
> ---
> xen/arch/arm/mm.c | 63 ---
> 1 file changed, 5 insertions(+), 58 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 4af59375d998..d73f49d5b6fc 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -354,40 +354,6 @@ void clear_fixmap(unsigned map)
> BUG_ON(res != 0);
> }
>
> -/* Create Xen's mappings of memory.
> - * Mapping_size must be either 2MB or 32MB.
> - * Base and virt must be mapping_size aligned.
> - * Size must be a multiple of mapping_size.
> - * second must be a contiguous set of second level page tables
> - * covering the region starting at virt_offset. */
> -static void __init create_mappings(lpae_t *second,
> - unsigned long virt_offset,
> - unsigned long base_mfn,
> - unsigned long nr_mfns,
> - unsigned int mapping_size)
> -{
> -unsigned long i, count;
> -const unsigned long granularity = mapping_size >> PAGE_SHIFT;
> -lpae_t pte, *p;
> -
> -ASSERT((mapping_size == MB(2)) || (mapping_size == MB(32)));
> -ASSERT(!((virt_offset >> PAGE_SHIFT) % granularity));
> -ASSERT(!(base_mfn % granularity));
> -ASSERT(!(nr_mfns % granularity));
> -
> -count = nr_mfns / XEN_PT_LPAE_ENTRIES;
> -p = second + second_linear_offset(virt_offset);
> -pte = mfn_to_xen_entry(_mfn(base_mfn), MT_NORMAL);
> -if ( granularity == 16 * XEN_PT_LPAE_ENTRIES )
> -pte.pt.contig = 1; /* These maps are in 16-entry contiguous chunks.
> */
> -for ( i = 0; i < count; i++ )
> -{
> -write_pte(p + i, pte);
> -pte.pt.base += 1 << XEN_PT_LPAE_SHIFT;
> -}
> -flush_xen_tlb_local();
> -}
> -
> #ifdef CONFIG_DOMAIN_PAGE
> void *map_domain_page_global(mfn_t mfn)
> {
> @@ -846,36 +812,17 @@ void __init setup_frametable_mappings(paddr_t ps,
> paddr_t pe)
> unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
> mfn_t base_mfn;
> const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) :
> MB(32);
> -#ifdef CONFIG_ARM_64
> -lpae_t *second, pte;
> -unsigned long nr_second;
> -mfn_t second_base;
> -int i;
> -#endif
> +int rc;
>
> frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps));
> /* Round up to 2M or 32M boundary, as appropriate. */
> frametable_size = ROUNDUP(frametable_size, mapping_size);
> base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
>
> -#ifdef CONFIG_ARM_64
> -/* Compute the number of second level pages. */
> -nr_second = ROUNDUP(frametable_size, FIRST_SIZE) >> FIRST_SHIFT;
> -second_base = alloc_boot_pages(nr_second, 1);
> -second = mfn_to_virt(second_base);
> -for ( i = 0; i < nr_second; i++ )
> -{
> -clear_page(mfn_to_virt(mfn_add(second_base, i)));
> -pte = mfn_to_xen_entry(mfn_add(second_base, i), MT_NORMAL);
> -pte.pt.table = 1;
> -write_pte(_first[first_table_offset(FRAMETABLE_VIRT_START)+i],
> pte);
> -}
> -create_mappings(second, 0, mfn_x(base_mfn), frametable_size >>
> PAGE_SHIFT,
> -mapping_size);
> -#else
> -create_mappings(xen_second, FRAMETABLE_VIRT_START, mfn_x(base_mfn),
> -frametable_size >> PAGE_SHIFT, mapping_size);
> -#endif
> +rc = map_pages_to_xen(FRAMETABLE_VIRT_START, base_mfn,
> + frametable_size >> PAGE_SHIFT, PAGE_HYPERVISOR_RW);
> +if ( rc )
> +panic("Unable to setup the frametable mappings.\n");
>
> memset(_table[0], 0, nr_pdxs * sizeof(struct page_info));
> memset(_table[nr_pdxs], -1,
> --
> 2.32.0
>
RE: [for-4.16] Re: [PATCH v4] xen/arm: vgic: Ignore write access to ICPENDR*
Hi, > -Original Message- > From: Julien Grall > Sent: 2021年10月22日 1:16 > To: Hongda Deng ; xen-devel@lists.xenproject.org; > sstabell...@kernel.org > Cc: Bertrand Marquis ; Wei Chen > ; Ian Jackson > Subject: Re: [for-4.16] Re: [PATCH v4] xen/arm: vgic: Ignore write access to > ICPENDR* > > > > On 21/10/2021 16:14, Julien Grall wrote: > > On the previous version, we discussed to include the patch for 4.16. So > > please tag it with for-4.16 and CC the Release Manager (Ian). This will > > help him to track what's outstanding for the release. > > > > On 21/10/2021 13:03, Hongda Deng wrote: > >> Currently, Xen will return IO unhandled when guests write ICPENDR* > >> virtual registers, which will raise a data abort inside the guest. > >> For Linux guest, these virtual registers will not be accessed. But > >> for Zephyr, these virtual registers will be accessed during the > >> initialization. Zephyr guest will get an IO data abort and crash. > >> Emulating ICPENDR is not easy with the existing vGIC, this patch > >> reworks the emulation to ignore write access to ICPENDR* virtual > >> registers and print a message about whether they are already pending > >> instead of returning unhandled. > >> More details can be found at [1]. > >> > >> [1] https://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e > >> cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274 > >> > >> Signed-off-by: Hongda Deng > > > > While I agree the Reviewed-by from Bertrand should be dropped, the > > Release-acked-by from Ian is simply a way to say he is happy to include > > the patch for 4.16. So this should have been retain. > > > > The patch looks good to me, so I can add Ian's tag on commit: > > > > Reviewed-by: Julien Grall > > Committed. > > Cheers, > > -- > Julien Grall Thank you ! I just learned that I should add "Reviewed-by" and " Release-acked-by" tags based on previous patches, sorry for that, I will keep it in mind. Cheers, --- Hongda
[PATCH v4] xen/arm: vgic: Ignore write access to ICPENDR*
Currently, Xen will return IO unhandled when guests write ICPENDR*
virtual registers, which will raise a data abort inside the guest.
For Linux guest, these virtual registers will not be accessed. But
for Zephyr, these virtual registers will be accessed during the
initialization. Zephyr guest will get an IO data abort and crash.
Emulating ICPENDR is not easy with the existing vGIC, this patch
reworks the emulation to ignore write access to ICPENDR* virtual
registers and print a message about whether they are already pending
instead of returning unhandled.
More details can be found at [1].
[1] https://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274
Signed-off-by: Hongda Deng
---
Changes since v3:
* Commit message modification
* Change "goto write_ignore_32" to "goto write_ignore" to avoid double
checking dabt.size
* Delete data.size check in vgic_v3_rdistr_sgi_mmio_write to avoid
double check in __vgic_v3_distr_common_mmio_write for SGI
* Declare flags, p, v_target within the loop to reduce their scope
* Use the same vgic_get_target_vcpu(v, irq) to get v_target for SPI,
PPI and SGI
* Code principle modification
Changes since v2:
* Avoid to print messages when there is no pending interrupt
* Add helper vgic_check_inflight_irqs_pending to check pending status
* Print a message for each interrupt separately
Changes since v1:
* Check pending states by going through vcpu->arch.vgic.inflight_irqs
instead of checking hardware registers
---
xen/arch/arm/vgic-v2.c | 10 ++
xen/arch/arm/vgic-v3.c | 17 -
xen/arch/arm/vgic.c| 28
xen/include/asm-arm/vgic.h | 2 ++
4 files changed, 44 insertions(+), 13 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b2da886adc..589c033eda 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -481,10 +481,12 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
mmio_info_t *info,
case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
if ( dabt.size != DABT_WORD ) goto bad_width;
-printk(XENLOG_G_ERR
- "%pv: vGICD: unhandled word write %#"PRIregister" to
ICPENDR%d\n",
- v, r, gicd_reg - GICD_ICPENDR);
-return 0;
+rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
+if ( rank == NULL ) goto write_ignore;
+
+vgic_check_inflight_irqs_pending(v->domain, v, rank->index, r);
+
+goto write_ignore;
case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
if ( dabt.size != DABT_WORD ) goto bad_width;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index cb5a70c42e..65bb7991a6 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -817,10 +817,12 @@ static int __vgic_v3_distr_common_mmio_write(const char
*name, struct vcpu *v,
case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
if ( dabt.size != DABT_WORD ) goto bad_width;
-printk(XENLOG_G_ERR
- "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d\n",
- v, name, r, reg - GICD_ICPENDR);
-return 0;
+rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
+if ( rank == NULL ) goto write_ignore;
+
+vgic_check_inflight_irqs_pending(v->domain, v, rank->index, r);
+
+goto write_ignore;
case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -986,11 +988,8 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v,
mmio_info_t *info,
info, gicr_reg, r);
case VREG32(GICR_ICPENDR0):
-if ( dabt.size != DABT_WORD ) goto bad_width;
-printk(XENLOG_G_ERR
- "%pv: vGICR: SGI: unhandled word write %#"PRIregister" to
ICPENDR0\n",
- v, r);
-return 0;
+return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
+ info, gicr_reg, r);
case VREG32(GICR_IGRPMODR0):
/* We do not implement security extensions for guests, write ignore */
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8f9400a519..83386cf3d5 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -726,6 +726,34 @@ unsigned int vgic_max_vcpus(unsigned int
domctl_vgic_version)
}
}
+void vgic_check_inflight_irqs_pending(struct domain *d, struct vcpu *v,
+ unsigned int rank, uint32_t r)
+{
+const unsigned long mask = r;
+unsigned int i;
+
+for_each_set_bit( i, , 32 )
+{
+struct pending_irq *p;
+struct vcpu *v_target;
+unsigned long flags;
+unsigned int irq = i + 32 * rank;
+
+v_target =
RE: [PATCH v3] xen/arm: vgic to ignore GICD ICPENDRn registers access
> -Original Message- > From: Julien Grall > Sent: 2021年10月21日 1:45 > To: Hongda Deng ; xen- > de...@lists.xenproject.org; sstabell...@kernel.org > Cc: Bertrand Marquis ; Wei Chen > > Subject: Re: [PATCH v3] xen/arm: vgic to ignore GICD ICPENDRn registers > access > > Hi Hongda, > > Title: I would suggest the following title: > > xen/arm: vgic: Ignore write access to ICPENDR* > Agreed. > On 20/10/2021 11:10, Hongda Deng wrote: > > Currently, Xen will return IO unhandled when guests access GICD ICPENRn > > registers. This will raise a data abort inside guest. For Linux Guest, > > these virtual registers will not be accessed. But for Zephyr, in its > > GIC initialization code, these virtual registers will be accessed. And > > zephyr guest will get an IO data abort in initilization stage and enter > Typo: s/initilization/initialization/ > > I would also s/in/during the/ > > > fatal error. Emulating ICPENDR is not easy with the existing vGIC, so > > How about s/enter fatal error/crash/? > Agreed. > > we currently ignore these virtual registers access and print a message > > To me 'currently' refers to the existing code base (i.e. without your > patch). In fact, this seems to be how you use 'currently' in the first > paragraph. So how about replace "so we currently" with "rework the > emulation to ignore...". > > This seems to suggest the patch will modify both read and write access. > However, AFAICT, only the write emulation is modified. Can this be > clarified in the commit message? > Ack. > > about whether they are already pending instead of returning unhandled. > > More details can be found at [1]. > > > > [1] https://github.com/zephyrproject- > rtos/zephyr/blob/eaf6cf745df3807e6e > > cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274 > > > > Signed-off-by: Hongda Deng > > --- > > Changes since v2: > > * Avoid to print messages when there is no pending interrupt > > * Add helper vgic_check_inflight_irqs_pending to check pending status > > * Print a message for each interrupt separately > > Changes since v1: > > * Check pending states by going through vcpu->arch.vgic.inflight_irqs > > instead of checking hardware registers > > --- > > xen/arch/arm/vgic-v2.c | 10 ++ > > xen/arch/arm/vgic-v3.c | 16 > > xen/arch/arm/vgic.c| 36 > > > xen/include/asm-arm/vgic.h | 3 ++- > > 4 files changed, 52 insertions(+), 13 deletions(-) > > > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c > > index b2da886adc..7c30da327c 100644 > > --- a/xen/arch/arm/vgic-v2.c > > +++ b/xen/arch/arm/vgic-v2.c > > @@ -481,10 +481,12 @@ static int vgic_v2_distr_mmio_write(struct vcpu > *v, mmio_info_t *info, > > > > case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN): > > if ( dabt.size != DABT_WORD ) goto bad_width; > > -printk(XENLOG_G_ERR > > - "%pv: vGICD: unhandled word write %#"PRIregister" to > ICPENDR%d\n", > > - v, r, gicd_reg - GICD_ICPENDR); > > -return 0; > > +rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD); > > +if ( rank == NULL ) goto write_ignore; > > + > > +vgic_check_inflight_irqs_pending(v->domain, v, rank->index, r); > + > > +goto write_ignore_32; > > NIT: We already check the access above. So I would simply use > "write_ignore" here. > Ack. > > > > case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN): > > if ( dabt.size != DABT_WORD ) goto bad_width; > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > > index cb5a70c42e..4913301d22 100644 > > --- a/xen/arch/arm/vgic-v3.c > > +++ b/xen/arch/arm/vgic-v3.c > > @@ -817,10 +817,12 @@ static int > __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v, > > > > case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN): > > if ( dabt.size != DABT_WORD ) goto bad_width; > > -printk(XENLOG_G_ERR > > - "%pv: %s: unhandled word write %#"PRIregister" to > ICPENDR%d\n", > > - v, name, r, reg - GICD_ICPENDR); > > -return 0; > > +rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD); > > +if ( rank == NULL ) goto write_ignore; > > + > > +vgic_check_inflight_irqs_pending(v->domain, v, rank->index, r
[PATCH v3] xen/arm: vgic to ignore GICD ICPENDRn registers access
Currently, Xen will return IO unhandled when guests access GICD ICPENRn
registers. This will raise a data abort inside guest. For Linux Guest,
these virtual registers will not be accessed. But for Zephyr, in its
GIC initialization code, these virtual registers will be accessed. And
zephyr guest will get an IO data abort in initilization stage and enter
fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
we currently ignore these virtual registers access and print a message
about whether they are already pending instead of returning unhandled.
More details can be found at [1].
[1] https://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274
Signed-off-by: Hongda Deng
---
Changes since v2:
* Avoid to print messages when there is no pending interrupt
* Add helper vgic_check_inflight_irqs_pending to check pending status
* Print a message for each interrupt separately
Changes since v1:
* Check pending states by going through vcpu->arch.vgic.inflight_irqs
instead of checking hardware registers
---
xen/arch/arm/vgic-v2.c | 10 ++
xen/arch/arm/vgic-v3.c | 16
xen/arch/arm/vgic.c| 36
xen/include/asm-arm/vgic.h | 3 ++-
4 files changed, 52 insertions(+), 13 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b2da886adc..7c30da327c 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -481,10 +481,12 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
mmio_info_t *info,
case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
if ( dabt.size != DABT_WORD ) goto bad_width;
-printk(XENLOG_G_ERR
- "%pv: vGICD: unhandled word write %#"PRIregister" to
ICPENDR%d\n",
- v, r, gicd_reg - GICD_ICPENDR);
-return 0;
+rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
+if ( rank == NULL ) goto write_ignore;
+
+vgic_check_inflight_irqs_pending(v->domain, v, rank->index, r);
+
+goto write_ignore_32;
case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
if ( dabt.size != DABT_WORD ) goto bad_width;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index cb5a70c42e..4913301d22 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -817,10 +817,12 @@ static int __vgic_v3_distr_common_mmio_write(const char
*name, struct vcpu *v,
case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
if ( dabt.size != DABT_WORD ) goto bad_width;
-printk(XENLOG_G_ERR
- "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d\n",
- v, name, r, reg - GICD_ICPENDR);
-return 0;
+rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
+if ( rank == NULL ) goto write_ignore;
+
+vgic_check_inflight_irqs_pending(v->domain, v, rank->index, r);
+
+goto write_ignore_32;
case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -987,10 +989,8 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v,
mmio_info_t *info,
case VREG32(GICR_ICPENDR0):
if ( dabt.size != DABT_WORD ) goto bad_width;
-printk(XENLOG_G_ERR
- "%pv: vGICR: SGI: unhandled word write %#"PRIregister" to
ICPENDR0\n",
- v, r);
-return 0;
+return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
+ info, gicr_reg, r);
case VREG32(GICR_IGRPMODR0):
/* We do not implement security extensions for guests, write ignore */
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8f9400a519..0565557814 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -726,6 +726,42 @@ unsigned int vgic_max_vcpus(unsigned int
domctl_vgic_version)
}
}
+void vgic_check_inflight_irqs_pending(struct domain *d, struct vcpu *v,
+ unsigned int rank, uint32_t r)
+{
+const unsigned long mask = r;
+unsigned int i;
+unsigned long flags;
+struct pending_irq *p;
+bool private = rank == 0;
+struct vcpu *v_target;
+
+for_each_set_bit( i, , 32 )
+{
+unsigned int irq = i + 32 * rank;
+
+if ( private )
+v_target = vgic_get_target_vcpu(v, irq);
+else
+v_target = vgic_get_target_vcpu(d->vcpu[0], irq);
+
+spin_lock_irqsave(_target->arch.vgic.lock, flags);
+
+p = irq_to_pending(v_target, irq);
+
+if ( unlikely(!p) )
+{
+spin_unlock_irqrestore(_target->arch.vgic.lock, flags);
+continue;
+}
+
+if ( !list_empty(>inflight) )
+printk("%pv trying to clear pending interrupt %u.\n", v, i
RE: [PATCH v2] xen/arm: vgic to ignore GICD ICPENRn registers access
Hi,
> -Original Message-
> From: Julien Grall
> Sent: 2021年10月14日 17:30
> To: Hongda Deng ; xen-
> de...@lists.xenproject.org; sstabell...@kernel.org
> Cc: Bertrand Marquis ; Wei Chen
>
> Subject: Re: [PATCH v2] xen/arm: vgic to ignore GICD ICPENRn registers
> access
>
> On 14/10/2021 07:55, Hongda Deng wrote:
> > Hi,
> Hi,
>
> >> -Original Message-
> >> From: Julien Grall
> >> Sent: 2021年10月13日 5:58
> >> To: Hongda Deng ; xen-
> de...@lists.xenproject.org;
> >> sstabell...@kernel.org
> >> Cc: Bertrand Marquis ; Wei Chen
> >>
> >> Subject: Re: [PATCH v2] xen/arm: vgic to ignore GICD ICPENRn registers
> access
> >>
> >> Hi,
> >>
> >> On 12/10/2021 07:24, Hongda Deng wrote:
> >>> Currently, Xen will return IO unhandled when guests access GICD
> ICPENRn
> >>> registers. This will raise a data abort inside guest. For Linux Guest,
> >>> these virtual registers will not be accessed. But for Zephyr, in its
> >>> GIC initialization code, these virtual registers will be accessed. And
> >>> zephyr guest will get an IO data abort in initilization stage and enter
> >>> fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
> >>> we currently ignore these virtual registers access and print a message
> >>> about whether they are already pending instead of returning unhandled.
> >>> More details can be found at [1].
> >>
> >> The link you provide only states that I am happy with the warning. This
> >> doesn't seem relevant for a future reader. Did you intend to point to
> >> something different?
> >>
> >
> > Yes, I would attach this link [1] then, which explains how zephyr accesses
> > ICPENDR at its initialization. ( Though I still don't understand why zephyr
> > would clear this register at initialization while linux wouldn't )
>
> I am confused as well. From my understanding, clearing ICPENDR at
> initialization is pointless for level interrupts if you didn't quiesce
> the device beforehand.
>
> The git history doesn't seem to give much details on the reason. But
> looking at the code, I am wondering if the intention was to clear the
> active bit rather than the pending bit.
>
I will try to find someone works on zephyr to see it if he/she knows that.
> >
> >>>
> >>> [1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/
> >>> msg00744.html
> >>>
> >>> Signed-off-by: Hongda Deng
> >>> ---
> >>>xen/arch/arm/vgic-v2.c | 26 +-
> >>>xen/arch/arm/vgic-v3.c | 40 +++--
> ---
> >>>2 files changed, 56 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> >>> index b2da886adc..d7ffaeeb65 100644
> >>> --- a/xen/arch/arm/vgic-v2.c
> >>> +++ b/xen/arch/arm/vgic-v2.c
> >>> @@ -480,11 +480,35 @@ static int vgic_v2_distr_mmio_write(struct
> vcpu *v,
> >> mmio_info_t *info,
> >>>return 1;
> >>>
> >>>case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
> >>> +{
> >>> +struct pending_irq *iter;
> >>> +unsigned int irq_start;
> >>> +unsigned int irq_end;
> >>> +uint32_t irq_pending = 0;
> >>> +
> >>>if ( dabt.size != DABT_WORD ) goto bad_width;
> >>>printk(XENLOG_G_ERR
> >>> "%pv: vGICD: unhandled word write %#"PRIregister" to
> >> ICPENDR%d\n",
> >>> v, r, gicd_reg - GICD_ICPENDR);
> >>
> >> As I wrote in v1, we should avoid to print a message when we know there
> >> is no pending interrupts.
> >>
> >
> > These are not the modifications made in this patch, and same codes also
> exist
> > for ICACTIVER. I didn't delete them for that I think they are used to give
> some
> > useful and coherent messages to user for reference. Should we delete it
> for both
> > or only for ICPENDR?
>
> Looking at the implementation ICACTIVER, we simply ignore the write so
> it makes sense to print a message everytime.
>
> This is quite different to the implementation of ICPENDR as we will
> partially emulate it. We technically emulated the register correctly
> when there is no pending interrupts, so I think it is wrong to print a
> message st
RE: [PATCH v2] xen/arm: vgic to ignore GICD ICPENRn registers access
Hi,
> -Original Message-
> From: Julien Grall
> Sent: 2021年10月13日 5:58
> To: Hongda Deng ; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org
> Cc: Bertrand Marquis ; Wei Chen
>
> Subject: Re: [PATCH v2] xen/arm: vgic to ignore GICD ICPENRn registers access
>
> Hi,
>
> On 12/10/2021 07:24, Hongda Deng wrote:
> > Currently, Xen will return IO unhandled when guests access GICD ICPENRn
> > registers. This will raise a data abort inside guest. For Linux Guest,
> > these virtual registers will not be accessed. But for Zephyr, in its
> > GIC initialization code, these virtual registers will be accessed. And
> > zephyr guest will get an IO data abort in initilization stage and enter
> > fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
> > we currently ignore these virtual registers access and print a message
> > about whether they are already pending instead of returning unhandled.
> > More details can be found at [1].
>
> The link you provide only states that I am happy with the warning. This
> doesn't seem relevant for a future reader. Did you intend to point to
> something different?
>
Yes, I would attach this link [1] then, which explains how zephyr accesses
ICPENDR at its initialization. ( Though I still don't understand why zephyr
would clear this register at initialization while linux wouldn't )
> >
> > [1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/
> > msg00744.html
> >
> > Signed-off-by: Hongda Deng
> > ---
> > xen/arch/arm/vgic-v2.c | 26 +-
> > xen/arch/arm/vgic-v3.c | 40 +++-
> > 2 files changed, 56 insertions(+), 10 deletions(-)
> >
> > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > index b2da886adc..d7ffaeeb65 100644
> > --- a/xen/arch/arm/vgic-v2.c
> > +++ b/xen/arch/arm/vgic-v2.c
> > @@ -480,11 +480,35 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
> mmio_info_t *info,
> > return 1;
> >
> > case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
> > +{
> > +struct pending_irq *iter;
> > +unsigned int irq_start;
> > +unsigned int irq_end;
> > +uint32_t irq_pending = 0;
> > +
> > if ( dabt.size != DABT_WORD ) goto bad_width;
> > printk(XENLOG_G_ERR
> > "%pv: vGICD: unhandled word write %#"PRIregister" to
> ICPENDR%d\n",
> > v, r, gicd_reg - GICD_ICPENDR);
>
> As I wrote in v1, we should avoid to print a message when we know there
> is no pending interrupts.
>
These are not the modifications made in this patch, and same codes also exist
for ICACTIVER. I didn't delete them for that I think they are used to give some
useful and coherent messages to user for reference. Should we delete it for both
or only for ICPENDR?
> > -return 0;
> > +
> > +irq_start = (gicd_reg - GICD_ICPENDR) * 32;
> > +irq_end = irq_start + 31;
> > +/* go through inflight_irqs and print specified pending irqs */
> > +list_for_each_entry(iter, >arch.vgic.inflight_irqs, inflight)
> You need to hold v->arch.vgic.lock (with interrupt disabled) to go
> through the list of inflight irqs. Otherwise, the list may be modified
> while you are walking it.
>
Ack.
> However, I am a little bit concerned with this approached (I noticed
> Stefano suggested). The list may in theory contains a few hundreds
> interrupts (a malicious OS May decide to never read IAR). So we are
> potentially doing more work than necessary (we need to think about the
> worse case scenario).
>
> Instead, I think it would be better to go through the 32 interrupts and
> for each of them:
>1) find the pending_irq() using irq_to_pending()
>2) check if the IRQ in the inflight list with list_empty(>inflight)
>
> In addition to that, you want to check that the rank exists so we don't
> do any extra work if the guest is trying to clear an interrupts above
> the number of interrupts we support.
>
Agreed, and that's quite helpful.
> > +{
> > +if ( iter->irq < irq_start || irq_end < iter->irq )
> > +continue;
> > +
> > +if ( test_bit(GIC_IRQ_GUEST_QUEUED, >status) )
> > +irq_pending = irq_pending | (1 << (iter->irq - irq_start));
> > +}
> > +
> > +if ( irq_pending != 0 )
> > +printk(XENLOG_G_ERR
> > + "%pv: vGICD: ICPENDR%d=0x%08x\n",
> > +
[PATCH v2] xen/arm: vgic to ignore GICD ICPENRn registers access
Currently, Xen will return IO unhandled when guests access GICD ICPENRn
registers. This will raise a data abort inside guest. For Linux Guest,
these virtual registers will not be accessed. But for Zephyr, in its
GIC initialization code, these virtual registers will be accessed. And
zephyr guest will get an IO data abort in initilization stage and enter
fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
we currently ignore these virtual registers access and print a message
about whether they are already pending instead of returning unhandled.
More details can be found at [1].
[1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/
msg00744.html
Signed-off-by: Hongda Deng
---
xen/arch/arm/vgic-v2.c | 26 +-
xen/arch/arm/vgic-v3.c | 40 +++-
2 files changed, 56 insertions(+), 10 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b2da886adc..d7ffaeeb65 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -480,11 +480,35 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
mmio_info_t *info,
return 1;
case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
+{
+struct pending_irq *iter;
+unsigned int irq_start;
+unsigned int irq_end;
+uint32_t irq_pending = 0;
+
if ( dabt.size != DABT_WORD ) goto bad_width;
printk(XENLOG_G_ERR
"%pv: vGICD: unhandled word write %#"PRIregister" to
ICPENDR%d\n",
v, r, gicd_reg - GICD_ICPENDR);
-return 0;
+
+irq_start = (gicd_reg - GICD_ICPENDR) * 32;
+irq_end = irq_start + 31;
+/* go through inflight_irqs and print specified pending irqs */
+list_for_each_entry(iter, >arch.vgic.inflight_irqs, inflight)
+{
+if ( iter->irq < irq_start || irq_end < iter->irq )
+continue;
+
+if ( test_bit(GIC_IRQ_GUEST_QUEUED, >status) )
+irq_pending = irq_pending | (1 << (iter->irq - irq_start));
+}
+
+if ( irq_pending != 0 )
+printk(XENLOG_G_ERR
+ "%pv: vGICD: ICPENDR%d=0x%08x\n",
+ v, gicd_reg - GICD_ICPENDR, irq_pending);
+goto write_ignore_32;
+}
case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
if ( dabt.size != DABT_WORD ) goto bad_width;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index cb5a70c42e..243b24e496 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -816,11 +816,35 @@ static int __vgic_v3_distr_common_mmio_write(const char
*name, struct vcpu *v,
return 1;
case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
+{
+struct pending_irq *iter;
+unsigned int irq_start;
+unsigned int irq_end;
+uint32_t irq_pending = 0;
+
if ( dabt.size != DABT_WORD ) goto bad_width;
printk(XENLOG_G_ERR
"%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d\n",
v, name, r, reg - GICD_ICPENDR);
-return 0;
+
+irq_start = (reg - GICD_ICPENDR) * 32;
+irq_end = irq_start + 31;
+/* go through inflight_irqs and print specified pending irqs */
+list_for_each_entry(iter, >arch.vgic.inflight_irqs, inflight)
+{
+if ( iter->irq < irq_start || irq_end < iter->irq )
+continue;
+
+if ( test_bit(GIC_IRQ_GUEST_QUEUED, >status) )
+irq_pending = irq_pending | (1 << (iter->irq - irq_start));
+}
+
+if ( irq_pending != 0 )
+printk(XENLOG_G_ERR
+ "%pv: %s: ICPENDR%d=0x%08x\n",
+ v, name, reg - GICD_ICPENDR, irq_pending);
+goto write_ignore_32;
+}
case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -978,19 +1002,17 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v,
mmio_info_t *info,
case VREG32(GICR_ICFGR1):
case VRANGE32(GICR_IPRIORITYR0, GICR_IPRIORITYR7):
case VREG32(GICR_ISPENDR0):
- /*
- * Above registers offset are common with GICD.
- * So handle common with GICD handling
- */
+/*
+* Above registers offset are common with GICD.
+* So handle common with GICD handling
+*/
return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
info, gicr_reg, r);
case VREG32(GICR_ICPENDR0):
if ( dabt.size != DABT_WORD ) goto bad_width;
-printk(XENLOG_G_ERR
- "%pv: vGICR: SGI: unhandled word write %#"PRIregister" to
ICPENDR0\n",
- v, r);
-return 0;
+return __vgic_v3_distr_common_mmio_write("vGIC
RE: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access
Hi, Thanks for your great and detailed advice, I did some investigations about vgic especially inflight_irqs in the last few days. > -Original Message- > From: Stefano Stabellini > Sent: 2021年9月24日 4:54 > To: Julien Grall > Cc: Hongda Deng ; xen-devel@lists.xenproject.org; > sstabell...@kernel.org; Bertrand Marquis ; Wei > Chen > Subject: Re: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access > > On Thu, 23 Sep 2021, Julien Grall wrote: > > Hi, > > > > On 23/09/2021 11:14, Hongda Deng wrote: > > > Currently, Xen will return IO unhandled when guests access GICD ICPENRn > > > registers. This will raise a data abort inside guest. For Linux Guest, > > > these virtual registers will not be accessed. But for Zephyr, in its > > > GIC initilization code, these virtual registers will be accessed. And > > > zephyr guest will get an IO dataabort in initilization stage and enter > > > > s/dataabort/data abort/ > > s/initilization/initialization/ > > Ack. > > > fatal error. Emulating ICPENDR is not easy with the existing vGIC, so > > > we currently ignore these virtual registers access and print a message > > > about whether they are already pending instead of returning unhandled. > > > More details can be found at [1]. > > > > > > [1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/ > > > msg00744.html > > > > > > Signed-off-by: Hongda Deng > > > --- > > > xen/arch/arm/vgic-v2.c | 10 +++--- > > > xen/arch/arm/vgic-v3.c | 29 + > > > xen/arch/arm/vgic.c| 37 + > > > xen/include/asm-arm/vgic.h | 2 ++ > > > 4 files changed, 63 insertions(+), 15 deletions(-) > > > > > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c > > > index b2da886adc..644c62757c 100644 > > > --- a/xen/arch/arm/vgic-v2.c > > > +++ b/xen/arch/arm/vgic-v2.c > > > @@ -481,10 +481,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, > > > mmio_info_t *info, > > > case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN): > > > if ( dabt.size != DABT_WORD ) goto bad_width; > > > +rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, > > > DABT_WORD); > > > +if ( rank == NULL ) goto write_ignore; > > > > > > > > > + > > > printk(XENLOG_G_ERR > > > - "%pv: vGICD: unhandled word write %#"PRIregister" to > > > ICPENDR%d\n", > > > - v, r, gicd_reg - GICD_ICPENDR); > > > -return 0; > > > + "%pv: vGICD: unhandled word write %#"PRIregister" to > > > ICPENDR%d, and current pending state is: %s\n", > > > + v, r, gicd_reg - GICD_ICPENDR, > > > + vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off"); > > > > Each register contain the information for multiple pending interrupts. So it > > is a bit confusing to say whether the state is on/off. Instead, it would be > > better to state which interrupt is pending. > > > > Also, I would rather avoid printing a message if there are no interrupts > > pending because there are no issues if this is happening. I will fix it in the next version patch. > > > > > +goto write_ignore_32; > > > case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN): > > > if ( dabt.size != DABT_WORD ) goto bad_width; > > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > > > index cb5a70c42e..c94e33ff4f 100644 > > > --- a/xen/arch/arm/vgic-v3.c > > > +++ b/xen/arch/arm/vgic-v3.c > > > @@ -817,10 +817,14 @@ static int > __vgic_v3_distr_common_mmio_write(const > > > char *name, struct vcpu *v, > > > case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN): > > > if ( dabt.size != DABT_WORD ) goto bad_width; > > > +rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD); > > > +if ( rank == NULL ) goto write_ignore; > > > + > > > printk(XENLOG_G_ERR > > > - "%pv: %s: unhandled word write %#"PRIregister" to > > > ICPENDR%d\n", > > > - v, name, r, reg - GICD_ICPENDR); > > > -return 0; > > > + "%pv: %s: unhandled word write %#"PRIregister" to > > > ICPENDR%d, > > > and cur
[PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access
Currently, Xen will return IO unhandled when guests access GICD ICPENRn
registers. This will raise a data abort inside guest. For Linux Guest,
these virtual registers will not be accessed. But for Zephyr, in its
GIC initilization code, these virtual registers will be accessed. And
zephyr guest will get an IO dataabort in initilization stage and enter
fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
we currently ignore these virtual registers access and print a message
about whether they are already pending instead of returning unhandled.
More details can be found at [1].
[1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/
msg00744.html
Signed-off-by: Hongda Deng
---
xen/arch/arm/vgic-v2.c | 10 +++---
xen/arch/arm/vgic-v3.c | 29 +
xen/arch/arm/vgic.c| 37 +
xen/include/asm-arm/vgic.h | 2 ++
4 files changed, 63 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b2da886adc..644c62757c 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -481,10 +481,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
mmio_info_t *info,
case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
if ( dabt.size != DABT_WORD ) goto bad_width;
+rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
+if ( rank == NULL ) goto write_ignore;
+
printk(XENLOG_G_ERR
- "%pv: vGICD: unhandled word write %#"PRIregister" to
ICPENDR%d\n",
- v, r, gicd_reg - GICD_ICPENDR);
-return 0;
+ "%pv: vGICD: unhandled word write %#"PRIregister" to ICPENDR%d,
and current pending state is: %s\n",
+ v, r, gicd_reg - GICD_ICPENDR,
+ vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
+goto write_ignore_32;
case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
if ( dabt.size != DABT_WORD ) goto bad_width;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index cb5a70c42e..c94e33ff4f 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -817,10 +817,14 @@ static int __vgic_v3_distr_common_mmio_write(const char
*name, struct vcpu *v,
case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
if ( dabt.size != DABT_WORD ) goto bad_width;
+rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
+if ( rank == NULL ) goto write_ignore;
+
printk(XENLOG_G_ERR
- "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d\n",
- v, name, r, reg - GICD_ICPENDR);
-return 0;
+ "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d,
and current pending state is: %s\n",
+ v, name, r, reg - GICD_ICPENDR,
+ vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
+goto write_ignore_32;
case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -978,19 +982,20 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v,
mmio_info_t *info,
case VREG32(GICR_ICFGR1):
case VRANGE32(GICR_IPRIORITYR0, GICR_IPRIORITYR7):
case VREG32(GICR_ISPENDR0):
- /*
- * Above registers offset are common with GICD.
- * So handle common with GICD handling
- */
+/*
+ * Above registers offset are common with GICD.
+ * So handle common with GICD handling
+ */
return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
info, gicr_reg, r);
case VREG32(GICR_ICPENDR0):
-if ( dabt.size != DABT_WORD ) goto bad_width;
-printk(XENLOG_G_ERR
- "%pv: vGICR: SGI: unhandled word write %#"PRIregister" to
ICPENDR0\n",
- v, r);
-return 0;
+/*
+ * Above registers offset are common with GICD.
+ * So handle common with GICD handling
+ */
+return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
+ info, gicr_reg, r);
case VREG32(GICR_IGRPMODR0):
/* We do not implement security extensions for guests, write ignore */
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8f9400a519..29a1aa5056 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -470,6 +470,43 @@ void vgic_set_irqs_pending(struct vcpu *v, uint32_t r,
unsigned int rank)
}
}
+bool vgic_get_irqs_pending(struct vcpu *v, uint32_t r, unsigned int rank)
+{
+const unsigned long mask = r;
+unsigned int i;
+/* The first rank is always per-vCPU */
+bool private = rank == 0;
+bool is_pending = false;
+
+/* LPIs status
RE: About Arm guests accessing to GICD_ICPENR
Hi Julien,
> On 06/09/2021 10:04, Hongda Deng wrote:
> > Hi Julien,
>
> Hi Hongda,
>
> > Xen provides vGIC to support Xen guests, and currently xen will return IO
> > unhandled when guests access GICD ICPENRn registers. This works fine with
> Linux
> > guests, for Linux won't access these registers. But for Zephyr, this
> > mechanism
> > will cause IO dataabort on Zephyr's initialization which makes Zephyr get in
> > fatal error.
> >
> > One method to ease this is to let vGIC ignore GICD ICPENRn registers
> > access. I
> > tested it with Linux guests and Zephyr guests, and both works fine. And I
> > found
> > in this patch[1] that it would be more complex to touch the read part for
> > I{S,C}PENDR and the write part for ICPENDR,
>
> Read to I{S,C}PENDR should already return. AFAIK, what's left
> unimplemented is write to ICPENDR.
>
> > so could we ignore GICD ICPENDRn
> > registers access to ease Zephyr's initialization problem?
> Would you be able to provide more information on how Zephyr is using it?
Zephyr will try to clear pending state at its initialization, and the code can
be
found here[1].
>
> >
> > If Xen wants a complete GICD ICPENDRn emulation to fix it, do you have any
> > suggestions.
>
> Emulating ICPENDR is not easy with the existing vGIC. It would be great
> to finally have a vGIC spec compliant, but I also appreciate this is
> going to take some time.
Yes, I agree with that.
>
> Ignoring the access is probably the best we can do. However, this is
> also a risky approach because Zephyr (or another OS) may genuinely want
> to clear an already pending interrupt. So I would suggest to walk the
> interrupts that are meant to be modified and check whether they are
> already pending. If they are then print a message.
OK, I would add this check and warning message to my current patch to fix it
in current stage. Is it OK?
>
> This would make clear to the developper that something may go wrong
> later on.
>
> Cheers,
>
> --
> Julien Grall
[1] https://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274
Cheers
Hongda
About Arm guests accessing to GICD_ICPENR
Hi Julien,
Xen provides vGIC to support Xen guests, and currently xen will return IO
unhandled when guests access GICD ICPENRn registers. This works fine with Linux
guests, for Linux won't access these registers. But for Zephyr, this mechanism
will cause IO dataabort on Zephyr's initialization which makes Zephyr get in
fatal error.
One method to ease this is to let vGIC ignore GICD ICPENRn registers access. I
tested it with Linux guests and Zephyr guests, and both works fine. And I found
in this patch[1] that it would be more complex to touch the read part for
I{S,C}PENDR and the write part for ICPENDR, so could we ignore GICD ICPENDRn
registers access to ease Zephyr's initialization problem?
If Xen wants a complete GICD ICPENDRn emulation to fix it, do you have any
suggestions?
[1] https://www.mail-archive.com/search?l=xen-devel@lists.xenproject.org=
subject:%22Re%5C%3A+%5C%5BPATCH+for%5C-4.15%5C%5D+xen%5C%2Fvgic%5C%3A
+Implement+write+to+ISPENDR+in+vGICv%5C%7B2%2C+3%5C%7D%22=newest=1
Cheers
Hongda
