Hi Julien > -----Original Message----- > From: Julien Grall <jul...@xen.org> > Sent: Thursday, October 14, 2021 2:01 AM > To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org; > sstabell...@kernel.org > Cc: Bertrand Marquis <bertrand.marq...@arm.com>; Wei Chen > <wei.c...@arm.com> > Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART > address and IRQ number for vPL011 > > On 12/10/2021 03:42, Penny Zheng wrote: > > Hi Julien > > Hi Penny, > > >> -----Original Message----- > >> From: Julien Grall <jul...@xen.org> > >> Sent: Monday, October 11, 2021 6:49 PM > >> To: Penny Zheng <penny.zh...@arm.com>; > >> xen-devel@lists.xenproject.org; sstabell...@kernel.org > >> Cc: Bertrand Marquis <bertrand.marq...@arm.com>; Wei Chen > >> <wei.c...@arm.com> > >> Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use > >> native UART address and IRQ number for vPL011 > >> > >> On 09/10/2021 09:47, Penny Zheng wrote: > >>> Hi Julien > >> > >> Hi Penny, > >> > >>>> -----Original Message----- > >>>> From: Julien Grall <jul...@xen.org> > >>>> Sent: Thursday, September 23, 2021 7:14 PM > >>>> To: Penny Zheng <penny.zh...@arm.com>; > >>>> xen-devel@lists.xenproject.org; sstabell...@kernel.org > >>>> Cc: Bertrand Marquis <bertrand.marq...@arm.com>; Wei Chen > >>>> <wei.c...@arm.com> > >>>> Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use > >>>> native UART address and IRQ number for vPL011 > >>>> > >>>> > >>>> > >>>> On 23/09/2021 08:11, Penny Zheng wrote: > >>>>> From: Stefano Stabellini <stefano.stabell...@xilinx.com> > >>>>> > >>>>> We always use a fix address to map the vPL011 to domains. The > >>>>> address could be a problem for domains that are directly mapped. > >>>>> > >>>>> So, for domains that are directly mapped, reuse the address of the > >>>>> physical UART on the platform to avoid potential clashes. > >>>>> > >>>>> Do the same for the virtual IRQ number: instead of always using > >>>>> GUEST_VPL011_SPI, try to reuse the physical SPI number if possible. > >>>>> > >>>>> Signed-off-by: Stefano Stabellini <stefano.stabell...@xilinx.com> > >>>>> Signed-off-by: Penny Zheng <penny.zh...@arm.com> > >>>>> --- > >>>>> xen/arch/arm/domain_build.c | 34 > >>>>> +++++++++++++++++++++++++++----- > >> -- > >>>>> xen/arch/arm/vpl011.c | 34 +++++++++++++++++++++++++++------- > >>>>> xen/include/asm-arm/vpl011.h | 2 ++ > >>>>> 3 files changed, 56 insertions(+), 14 deletions(-) > >>>>> > >>>>> diff --git a/xen/arch/arm/domain_build.c > >>>>> b/xen/arch/arm/domain_build.c index 120f1ae575..c92e510ae7 100644 > >>>>> --- a/xen/arch/arm/domain_build.c > >>>>> +++ b/xen/arch/arm/domain_build.c > >>>>> @@ -30,6 +30,7 @@ > >>>>> > >>>>> #include <xen/irq.h> > >>>>> #include <xen/grant_table.h> > >>>>> +#include <xen/serial.h> > >>>>> > >>>>> static unsigned int __initdata opt_dom0_max_vcpus; > >>>>> integer_param("dom0_max_vcpus", opt_dom0_max_vcpus); @@ - > >> 1942,8 > >>>>> +1943,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.base_addr); > >>>>> + res = fdt_begin_node(fdt, buf); > >>>>> if ( res ) > >>>>> return res; > >>>>> > >>>>> @@ -1953,14 +1957,14 @@ 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.base_addr, > >>>>> GUEST_PL011_SIZE); > >>>>> > >>>>> res = fdt_property(fdt, "reg", reg, sizeof(reg)); > >>>>> if ( res ) > >>>>> return res; > >>>>> > >>>>> - set_interrupt(intr, GUEST_VPL011_SPI, 0xf, > DT_IRQ_TYPE_LEVEL_HIGH); > >>>>> + set_interrupt(intr, d->arch.vpl011.virq, 0xf, > >>>>> + DT_IRQ_TYPE_LEVEL_HIGH); > >>>>> > >>>>> res = fdt_property(fdt, "interrupts", intr, sizeof (intr)); > >>>>> if ( res ) > >>>>> @@ -2670,6 +2674,13 @@ static int __init construct_domU(struct > >>>>> domain > >>>> *d, > >>>>> else > >>>>> allocate_static_memory(d, &kinfo, node); > >>>>> > >>>>> + /* > >>>>> + * Initialization before creating its device > >>>>> + * tree node in prepare_dtb_domU. > >>>>> + */ > >>>> > >>>> I think it would be better to explain *why* this needs to be done before. > >>>> > >>>>> + if ( kinfo.vpl011 ) > >>>>> + rc = domain_vpl011_init(d, NULL); > >>>>> + > >>>>> rc = prepare_dtb_domU(d, &kinfo); > >>>>> if ( rc < 0 ) > >>>>> return rc; > >>>>> @@ -2678,9 +2689,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; > >>>>> } > >>>>> > >>>>> @@ -2723,15 +2731,27 @@ void __init create_domUs(void) > >>>>> > >>>>> if ( !dt_property_read_u32(node, "nr_spis", > >>>>> &d_cfg.arch.nr_spis) ) > >>>>> { > >>>>> + unsigned int vpl011_virq = GUEST_VPL011_SPI; > >>>> > >>>> Coding style: Add a newline here. > >>>> > >>>>> d_cfg.arch.nr_spis = gic_number_lines() - 32; > >>>>> > >>>>> + /* > >>>>> + * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map > >>>>> in > >>>>> + * set, in which case we'll try to match the hardware. > >>>>> + * > >>>>> + * Typically, d->arch.vpl011.virq has the vpl011 irq number > >>>>> + * but at this point of the boot sequence it is not > >>>>> + * initialized yet. > >>>>> + */ > >>>>> + if ( direct_map && serial_irq(SERHND_DTUART) > 0 ) > >>>>> + vpl011_virq = serial_irq(SERHND_DTUART); > >>>> > >>>> I think we should not continue if the domain is direct-mapped *and* > >>>> the IRQ is not found. Otherwise, this will may just result to > >>>> potential breakage if GUEST_VPL011_SPI happens to be used for an HW > >> device. > >>>> > >>>>> + > >>>>> /* > >>>>> * vpl011 uses one emulated SPI. If vpl011 is > >>>>> requested, make > >>>>> * sure that we allocate enough SPIs for it. > >>>>> */ > >>>>> if ( dt_property_read_bool(node, "vpl011") ) > >>>>> d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis, > >>>>> - GUEST_VPL011_SPI - 32 + 1); > >>>>> + vpl011_virq - 32 + 1); > >>>>> } > >>>>> > >>>>> /* > >>>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index > >>>>> 895f436cc4..10df25f098 100644 > >>>>> --- a/xen/arch/arm/vpl011.c > >>>>> +++ b/xen/arch/arm/vpl011.c > >>>>> @@ -29,6 +29,7 @@ > >>>>> #include <xen/mm.h> > >>>>> #include <xen/sched.h> > >>>>> #include <xen/console.h> > >>>>> +#include <xen/serial.h> > >>>>> #include <public/domctl.h> > >>>>> #include <public/io/console.h> > >>>>> #include <asm/pl011-uart.h> > >>>>> @@ -71,11 +72,11 @@ static void > >>>>> vpl011_update_interrupt_status(struct > >>>> domain *d) > >>>>> * status bit has been set since the last time. > >>>>> */ > >>>>> if ( uartmis & ~vpl011->shadow_uartmis ) > >>>>> - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, true); > >>>>> + vgic_inject_irq(d, NULL, vpl011->virq, true); > >>>>> > >>>>> vpl011->shadow_uartmis = uartmis; > >>>>> #else > >>>>> - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, uartmis); > >>>>> + vgic_inject_irq(d, NULL, vpl011->virq, uartmis); > >>>>> #endif > >>>>> } > >>>>> > >>>>> @@ -347,7 +348,8 @@ 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.base_addr); > >>>>> struct vpl011 *vpl011 = &v->domain->arch.vpl011; > >>>>> struct domain *d = v->domain; > >>>>> unsigned long flags; > >>>>> @@ -430,7 +432,8 @@ 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.base_addr); > >>>>> struct vpl011 *vpl011 = &v->domain->arch.vpl011; > >>>>> struct domain *d = v->domain; > >>>>> unsigned long flags; > >>>>> @@ -622,10 +625,27 @@ 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; > >>>>> > >>>>> + vpl011->base_addr = GUEST_PL011_BASE; > >>>>> + vpl011->virq = GUEST_VPL011_SPI; > >>>>> + if ( is_domain_direct_mapped(d) ) > >>>>> + { > >>>>> + if ( uart != NULL && serial_irq(SERHND_DTUART) > 0 ) > >>>>> + { > >>>>> + vpl011->base_addr = uart->base_addr; > >>>>> + vpl011->virq = serial_irq(SERHND_DTUART); > >>>> > >>>> This seems a bit pointless to call serial_irq() twice. How about > >>>> add a field in vuart_info to return the interrupt number? > >>>> > >>>>> + } > >>>>> + else > >>>>> + printk(XENLOG_ERR > >>>>> + "Unable to reuse physical UART address and irq for > vPL011.\n" > >>>>> + "Defaulting to addr %#"PRIpaddr" and IRQ %u\n", > >>>>> + vpl011->base_addr, vpl011->virq); > >>>>> + } > >>>>> + > >>>>> /* > >>>>> * info is NULL when the backend is in Xen. > >>>>> * info is != NULL when the backend is in a domain. > >>>>> @@ -661,7 +681,7 @@ int domain_vpl011_init(struct domain *d, > >>>>> struct > >>>> vpl011_init_info *info) > >>>>> } > >>>>> } > >>>>> > >>>>> - rc = vgic_reserve_virq(d, GUEST_VPL011_SPI); > >>>>> + rc = vgic_reserve_virq(d, vpl011->virq); > >>>>> if ( !rc ) > >>>>> { > >>>>> rc = -EINVAL; > >>>>> @@ -673,12 +693,12 @@ 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); > >>>>> + vpl011->base_addr, GUEST_PL011_SIZE, > >>>>> + NULL); > >>>> > >>>> So you are making the assumpption that the UART region will be > >>>> equal to (or > >>>> bigger) than GUEST_PL011_SIZE. There are definitely UART out where > >>>> the MMIO region is smaller than 4K. > >>>> > >>> > >>> Sorry. I got a few confused here, since I am not very familiar with > >>> pl011/UART > >> knowledge. > >>> > >>> Problems will occur when UART region is bigger than > >>> GUEST_PL011_SIZE, since we are only considering MMIO region of > >>> [vpl011->base_addr, vpl011- base_addr + GUEST_PL011_SIZE], right? > >> > >> It is in fact the other way around. The problem will appear if the > >> host UART MMIO region is smaller than the one we will emulate for the > guest PL011. > >> > > > > Sorry to keep bothering. > > Is it that because when the UART MMIO region is smaller than the one > > we emulated, register(DR, RSR, FR, ...) will not be at the place where we > emulated? > > Let me give an example to clarify my point. On some Hardware (IIRC pine64), > the UART MMIO region is less than 4KB. In fact, there are multiple device > within the same 4KB region. > > At the moment, we are removing those devices because we can't assign to a > domain a region that is not page aligned (4KB today). But I can see some > benefits to be able to assign such devices to different domain/xen. > To support them, we would need to trap the region and then forward only > access to address the domain can access. > > The PL011 we emulate for the guest require a 4KB region. So this would > overlap with other device in the same region we may want to trap. > > For is not an issue for the reasons I mentionned above. However, I think it > is a > good idea to harden the code and add a check/comment when we know > potential pitfalls. >
Understood, a thousand thanks for the detailed explanation! ;) > Cheers, > > -- > Julien Grall