| Message ID | 20200415010255.10081-10-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/12] xen: introduce xen_dom_flags | expand |
Hi Stefano, On 15/04/2020 02:02, Stefano Stabellini wrote: > We always use a fix address to map the vPL011 to domains. The address > could be a problem for domains that are directly mapped. > > Instead, for domains that are directly mapped, reuse the address of the > physical UART on the platform to avoid potential clashes. How do you know the physical UART MMIO region is big enough to fit the PL011? What if the user want to assign the physical UART to the domain + the vpl011? Cheers,
On Wed, 15 Apr 2020, Julien Grall wrote: > Hi Stefano, > > On 15/04/2020 02:02, Stefano Stabellini wrote: > > We always use a fix address to map the vPL011 to domains. The address > > could be a problem for domains that are directly mapped. > > > > Instead, for domains that are directly mapped, reuse the address of the > > physical UART on the platform to avoid potential clashes. > > How do you know the physical UART MMIO region is big enough to fit the PL011? That cannot be because the vPL011 MMIO size is 1 page, which is the minimum right? > What if the user want to assign the physical UART to the domain + the vpl011? A user can assign a UART to the domain, but it cannot assign the UART used by Xen (DTUART), which is the one we are using here to get the physical information. (If there is no UART used by Xen then we'll fall back to the virtual addresses. If they conflict we return error and let the user fix the configuration.)
On 01/05/2020 02:26, Stefano Stabellini wrote: > On Wed, 15 Apr 2020, Julien Grall wrote: >> Hi Stefano, >> >> On 15/04/2020 02:02, Stefano Stabellini wrote: >>> We always use a fix address to map the vPL011 to domains. The address >>> could be a problem for domains that are directly mapped. >>> >>> Instead, for domains that are directly mapped, reuse the address of the >>> physical UART on the platform to avoid potential clashes. >> >> How do you know the physical UART MMIO region is big enough to fit the PL011? > > That cannot be because the vPL011 MMIO size is 1 page, which is the > minimum right? No, there are platforms out with multiple UARTs in the same page (see sunxi for instance). > > >> What if the user want to assign the physical UART to the domain + the vpl011? > > A user can assign a UART to the domain, but it cannot assign the UART > used by Xen (DTUART), which is the one we are using here to get the > physical information. > > > (If there is no UART used by Xen then we'll fall back to the virtual > addresses. If they conflict we return error and let the user fix the > configuration.) If there is no UART in Xen, how the user will know the addresses conflicted? Earlyprintk? Cheers,
On Fri, 1 May 2020, Julien Grall wrote: > On 01/05/2020 02:26, Stefano Stabellini wrote: > > On Wed, 15 Apr 2020, Julien Grall wrote: > > > Hi Stefano, > > > > > > On 15/04/2020 02:02, Stefano Stabellini wrote: > > > > We always use a fix address to map the vPL011 to domains. The address > > > > could be a problem for domains that are directly mapped. > > > > > > > > Instead, for domains that are directly mapped, reuse the address of the > > > > physical UART on the platform to avoid potential clashes. > > > > > > How do you know the physical UART MMIO region is big enough to fit the > > > PL011? > > > > That cannot be because the vPL011 MMIO size is 1 page, which is the > > minimum right? > > No, there are platforms out with multiple UARTs in the same page (see sunxi > for instance). But if there are multiple UARTs sharing the same page, and the first one is used by Xen, there is no way to assign one of the secondary UARTs to a domU. So there would be no problem choosing the physical UART address for the virtual PL011. > > > What if the user want to assign the physical UART to the domain + the > > > vpl011? > > > > A user can assign a UART to the domain, but it cannot assign the UART > > used by Xen (DTUART), which is the one we are using here to get the > > physical information. > > > > > > (If there is no UART used by Xen then we'll fall back to the virtual > > addresses. If they conflict we return error and let the user fix the > > configuration.) > > If there is no UART in Xen, how the user will know the addresses conflicted? > Earlyprintk? That's a good question. Yes, I think earlyprintk would be the only way.
Hi Stefano, On 09/05/2020 01:07, Stefano Stabellini wrote: > On Fri, 1 May 2020, Julien Grall wrote: >> On 01/05/2020 02:26, Stefano Stabellini wrote: >>> On Wed, 15 Apr 2020, Julien Grall wrote: >>>> Hi Stefano, >>>> >>>> On 15/04/2020 02:02, Stefano Stabellini wrote: >>>>> We always use a fix address to map the vPL011 to domains. The address >>>>> could be a problem for domains that are directly mapped. >>>>> >>>>> Instead, for domains that are directly mapped, reuse the address of the >>>>> physical UART on the platform to avoid potential clashes. >>>> >>>> How do you know the physical UART MMIO region is big enough to fit the >>>> PL011? >>> >>> That cannot be because the vPL011 MMIO size is 1 page, which is the >>> minimum right? >> >> No, there are platforms out with multiple UARTs in the same page (see sunxi >> for instance). > > But if there are multiple UARTs sharing the same page, and the first one > is used by Xen, there is no way to assign one of the secondary UARTs to > a domU. So there would be no problem choosing the physical UART address > for the virtual PL011. AFAICT, nothing prevents a user to assign such UART to a dom0less guest today. It would not be safe, but it should work. If you want to make it safe, then you would need to trap the MMIO access so they can be sanitized. For a UART device, I don't think the overhead would be too bad. Anyway, the only thing I request is to add sanity check in the code to help the user diagnostics any potential clash. Cheers,
On Sat, 9 May 2020, Julien Grall wrote: > Hi Stefano, > > On 09/05/2020 01:07, Stefano Stabellini wrote: > > On Fri, 1 May 2020, Julien Grall wrote: > > > On 01/05/2020 02:26, Stefano Stabellini wrote: > > > > On Wed, 15 Apr 2020, Julien Grall wrote: > > > > > Hi Stefano, > > > > > > > > > > On 15/04/2020 02:02, Stefano Stabellini wrote: > > > > > > We always use a fix address to map the vPL011 to domains. The > > > > > > address > > > > > > could be a problem for domains that are directly mapped. > > > > > > > > > > > > Instead, for domains that are directly mapped, reuse the address of > > > > > > the > > > > > > physical UART on the platform to avoid potential clashes. > > > > > > > > > > How do you know the physical UART MMIO region is big enough to fit the > > > > > PL011? > > > > > > > > That cannot be because the vPL011 MMIO size is 1 page, which is the > > > > minimum right? > > > > > > No, there are platforms out with multiple UARTs in the same page (see > > > sunxi > > > for instance). > > > > But if there are multiple UARTs sharing the same page, and the first one > > is used by Xen, there is no way to assign one of the secondary UARTs to > > a domU. So there would be no problem choosing the physical UART address > > for the virtual PL011. > > AFAICT, nothing prevents a user to assign such UART to a dom0less guest today. > It would not be safe, but it should work. > > If you want to make it safe, then you would need to trap the MMIO access so > they can be sanitized. For a UART device, I don't think the overhead would be > too bad. > > Anyway, the only thing I request is to add sanity check in the code to help > the user diagnostics any potential clash. OK thanks for clarifying, I'll do that.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index beec0a144c..9bc0b810a7 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1768,8 +1768,11 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo) gic_interrupt_t intr; __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; __be32 *cells; + struct domain *d = kinfo->d; + char buf[27]; - res = fdt_begin_node(fdt, "sbsa-uart@"__stringify(GUEST_PL011_BASE)); + snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d->arch.vpl011_addr); + res = fdt_begin_node(fdt, buf); if ( res ) return res; @@ -1779,7 +1782,7 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo) cells = ®[0]; dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, - GUEST_ROOT_SIZE_CELLS, GUEST_PL011_BASE, + GUEST_ROOT_SIZE_CELLS, d->arch.vpl011_addr, GUEST_PL011_SIZE); res = fdt_property(fdt, "reg", reg, sizeof(reg)); @@ -2524,6 +2527,9 @@ static int __init construct_domU(struct domain *d, reserve_memory_11(d, &kinfo, &banks[0], i); } + if ( kinfo.vpl011 ) + rc = domain_vpl011_init(d, NULL); + rc = prepare_dtb_domU(d, &kinfo); if ( rc < 0 ) return rc; @@ -2532,9 +2538,6 @@ static int __init construct_domU(struct domain *d, if ( rc < 0 ) return rc; - if ( kinfo.vpl011 ) - rc = domain_vpl011_init(d, NULL); - return rc; } diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index 895f436cc4..44173a76fd 100644 --- a/xen/arch/arm/vpl011.c +++ b/xen/arch/arm/vpl011.c @@ -347,7 +347,7 @@ static int vpl011_mmio_read(struct vcpu *v, void *priv) { struct hsr_dabt dabt = info->dabt; - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE); + uint32_t vpl011_reg = (uint32_t)(info->gpa - v->domain->arch.vpl011_addr); struct vpl011 *vpl011 = &v->domain->arch.vpl011; struct domain *d = v->domain; unsigned long flags; @@ -430,7 +430,7 @@ static int vpl011_mmio_write(struct vcpu *v, void *priv) { struct hsr_dabt dabt = info->dabt; - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE); + uint32_t vpl011_reg = (uint32_t)(info->gpa - v->domain->arch.vpl011_addr); struct vpl011 *vpl011 = &v->domain->arch.vpl011; struct domain *d = v->domain; unsigned long flags; @@ -622,10 +622,16 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info) { int rc; struct vpl011 *vpl011 = &d->arch.vpl011; + const struct vuart_info *uart = serial_vuart_info(SERHND_DTUART); if ( vpl011->backend.dom.ring_buf ) return -EINVAL; + if ( is_domain_direct_mapped(d) && uart != NULL ) + d->arch.vpl011_addr = uart->base_addr; + else + d->arch.vpl011_addr = GUEST_PL011_BASE; + /* * info is NULL when the backend is in Xen. * info is != NULL when the backend is in a domain. @@ -673,7 +679,7 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info) spin_lock_init(&vpl011->lock); register_mmio_handler(d, &vpl011_mmio_handler, - GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL); + d->arch.vpl011_addr, GUEST_PL011_SIZE, NULL); return 0; diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 7a498921bf..52741895c8 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -100,6 +100,7 @@ struct arch_domain #endif bool direct_map; + paddr_t vpl011_addr; } __cacheline_aligned; struct arch_xen_dom_flags
We always use a fix address to map the vPL011 to domains. The address could be a problem for domains that are directly mapped. Instead, for domains that are directly mapped, reuse the address of the physical UART on the platform to avoid potential clashes. Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> --- xen/arch/arm/domain_build.c | 13 ++++++++----- xen/arch/arm/vpl011.c | 12 +++++++++--- xen/include/asm-arm/domain.h | 1 + 3 files changed, 18 insertions(+), 8 deletions(-)