| Message ID | 20231006130059.97700-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | domain: followup for phys address mapping series | expand |
Hi Roger, > On Oct 6, 2023, at 21:00, Roger Pau Monne <roger.pau@citrix.com> wrote: > > unmap_domain_page_global() expects the provided address to be page aligned, or > else some of the called functions will trigger assertions, like > modify_xen_mappings() on x86 or destroy_xen_mappings() on Arm. > > The following assert has been reported by osstest arm 32bit tests: > > (XEN) Assertion 'IS_ALIGNED(s, PAGE_SIZE)' failed at arch/arm/mm.c:1243 > (XEN) ----[ Xen-4.18-rc arm32 debug=y Not tainted ]---- > (XEN) CPU: 0 > (XEN) PC: 00271a38 destroy_xen_mappings+0x50/0x5c > [...] > (XEN) Xen call trace: > (XEN) [<00271a38>] destroy_xen_mappings+0x50/0x5c (PC) > (XEN) [<00235aa8>] vunmap+0x30/0x1a0 (LR) > (XEN) [<0026ad88>] unmap_domain_page_global+0x10/0x20 > (XEN) [<00208e38>] unmap_guest_area+0x90/0xec > (XEN) [<00208f98>] domain_kill+0x104/0x180 > (XEN) [<00239e3c>] do_domctl+0x8ac/0x14fc > (XEN) [<0027ae34>] do_trap_guest_sync+0x570/0x66c > (XEN) [<002019f0>] arch/arm/arm32/entry.o#return_from_trap+0/0x4 > > Fixes: eadc288cbb0d ('domain: map/unmap GADDR based shared guest areas') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry > --- > Changes since v1: > - Also page-align the address in map_guest_area(). > --- > xen/common/domain.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/common/domain.c b/xen/common/domain.c > index b8281d7cff9d..1468638ade8b 100644 > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -1601,7 +1601,7 @@ int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size, > unmap: > if ( pg ) > { > - unmap_domain_page_global(map); > + unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK)); > put_page_and_type(pg); > } > > @@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct guest_area *area) > > if ( pg ) > { > - unmap_domain_page_global(map); > + unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK)); > put_page_and_type(pg); > } > } > -- > 2.42.0 >
Hi Roger, On 06/10/2023 14:00, Roger Pau Monne wrote: > unmap_domain_page_global() expects the provided address to be page aligned, or > else some of the called functions will trigger assertions, like > modify_xen_mappings() on x86 or destroy_xen_mappings() on Arm. > > The following assert has been reported by osstest arm 32bit tests: > > (XEN) Assertion 'IS_ALIGNED(s, PAGE_SIZE)' failed at arch/arm/mm.c:1243 > (XEN) ----[ Xen-4.18-rc arm32 debug=y Not tainted ]---- > (XEN) CPU: 0 > (XEN) PC: 00271a38 destroy_xen_mappings+0x50/0x5c > [...] > (XEN) Xen call trace: > (XEN) [<00271a38>] destroy_xen_mappings+0x50/0x5c (PC) > (XEN) [<00235aa8>] vunmap+0x30/0x1a0 (LR) > (XEN) [<0026ad88>] unmap_domain_page_global+0x10/0x20 > (XEN) [<00208e38>] unmap_guest_area+0x90/0xec > (XEN) [<00208f98>] domain_kill+0x104/0x180 > (XEN) [<00239e3c>] do_domctl+0x8ac/0x14fc > (XEN) [<0027ae34>] do_trap_guest_sync+0x570/0x66c > (XEN) [<002019f0>] arch/arm/arm32/entry.o#return_from_trap+0/0x4 > > Fixes: eadc288cbb0d ('domain: map/unmap GADDR based shared guest areas') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers,
On 06.10.2023 15:00, Roger Pau Monne wrote:> --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -1601,7 +1601,7 @@ int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size, > unmap: > if ( pg ) > { > - unmap_domain_page_global(map); > + unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK)); > put_page_and_type(pg); > } > > @@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct guest_area *area) > > if ( pg ) > { > - unmap_domain_page_global(map); > + unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK)); > put_page_and_type(pg); > } > } On v1 in a reply to Julien you talk of "limiting misuse" by not relaxing expecations in Arm's backing code, but I wonder what kind of misuse you think about. Aiui there's no strong need to insist on page aligned input, and relaxing things there may simplify code elsewhere as well. Jan
On Mon, Oct 16, 2023 at 02:30:12PM +0200, Jan Beulich wrote: > On 06.10.2023 15:00, Roger Pau Monne wrote:> --- a/xen/common/domain.c > > +++ b/xen/common/domain.c > > @@ -1601,7 +1601,7 @@ int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size, > > unmap: > > if ( pg ) > > { > > - unmap_domain_page_global(map); > > + unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK)); > > put_page_and_type(pg); > > } > > > > @@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct guest_area *area) > > > > if ( pg ) > > { > > - unmap_domain_page_global(map); > > + unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK)); > > put_page_and_type(pg); > > } > > } > > On v1 in a reply to Julien you talk of "limiting misuse" by not relaxing > expecations in Arm's backing code, but I wonder what kind of misuse you > think about. Aiui there's no strong need to insist on page aligned input, > and relaxing things there may simplify code elsewhere as well. destroy_xen_mappings() both on Arm and x86 will trigger asserts if the passed address is not page aligned. I do think it makes sense to call unmap_domain_page_global() with page-aligned addresses, as that could help detect bogus callers or corrupted data passed as input. IMO an assert for page aligned input address should be placed at vunmap() in order to not get differing expectations on input address being page aligned or not whether destroy_xen_mappings() or map_pages_to_xen() is used. map_pages_to_xen() doesn't require page-aligned virtual addresses as input. Roger.
diff --git a/xen/common/domain.c b/xen/common/domain.c index b8281d7cff9d..1468638ade8b 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -1601,7 +1601,7 @@ int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size, unmap: if ( pg ) { - unmap_domain_page_global(map); + unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK)); put_page_and_type(pg); } @@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct guest_area *area) if ( pg ) { - unmap_domain_page_global(map); + unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK)); put_page_and_type(pg); } }
unmap_domain_page_global() expects the provided address to be page aligned, or else some of the called functions will trigger assertions, like modify_xen_mappings() on x86 or destroy_xen_mappings() on Arm. The following assert has been reported by osstest arm 32bit tests: (XEN) Assertion 'IS_ALIGNED(s, PAGE_SIZE)' failed at arch/arm/mm.c:1243 (XEN) ----[ Xen-4.18-rc arm32 debug=y Not tainted ]---- (XEN) CPU: 0 (XEN) PC: 00271a38 destroy_xen_mappings+0x50/0x5c [...] (XEN) Xen call trace: (XEN) [<00271a38>] destroy_xen_mappings+0x50/0x5c (PC) (XEN) [<00235aa8>] vunmap+0x30/0x1a0 (LR) (XEN) [<0026ad88>] unmap_domain_page_global+0x10/0x20 (XEN) [<00208e38>] unmap_guest_area+0x90/0xec (XEN) [<00208f98>] domain_kill+0x104/0x180 (XEN) [<00239e3c>] do_domctl+0x8ac/0x14fc (XEN) [<0027ae34>] do_trap_guest_sync+0x570/0x66c (XEN) [<002019f0>] arch/arm/arm32/entry.o#return_from_trap+0/0x4 Fixes: eadc288cbb0d ('domain: map/unmap GADDR based shared guest areas') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Also page-align the address in map_guest_area(). --- xen/common/domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)