|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v7 2/2] xen/arm: consolidate make_timer_node and make_timer_domU_node
Hi Julien,
On Wed, Aug 7, 2019 at 4:05 PM Julien Grall <julien.grall@xxxxxxx> wrote:
>
>
> Hi,
> On 07/08/2019 11:10, Viktor Mitin wrote:
> > Functions make_timer_node and make_timer_domU_node are quite similar.
> > So it is better to consolidate them to avoid discrepancy.
> > The main difference between the functions is the timer interrupts used.
> >
> > Keep the domU version for the compatible as it is simpler:
> > do not copy platform's 'compatible' property into hwdom device tree,
> > instead set either arm,armv7-timer or arm,armv8-timer,
> > depending on the platform type.
>
> I am afraid this comment does not match the code below. The compatible is set
> based on the domain created not the platform.
>
> Also, I think it would be worth mentioning what's the implication of for dom0
> as
> this change behavior.
>
> >
> > Keep the hw version for the clock as it is relevant for the both cases.
>
> I guess you mean "dom0"/"hwdom" version? But then it is not clear what "clock"
> mean here. Did you intend to refer to "timer frequency"?
>
> The code below looks good to me and v8 (actually v9 now...) for such series a
> bit too much... So let's discuss about the commit message here.
>
> I would suggest the following commit message:
>
> "
> xen/arm: domain_build: Consolidate make_timer_node() and
> make_timer_domU_node()
>
> At the moment, the hwdom and domUs are creating the timer node differently.
>
> Technically the timer exposed the same way for any domain, the only difference
> should be the interrupts used. The two current other differences are:
> - compatible: The hwdom DT will use the same as the one provided by the
> host
> provided. The domUs DT will use "arm,armv7-timer" for 32-bit domain and
> "arm,armv8-timer" for 64-bit domain. The latter matches the behavior of libxl
> when guests are created from userspace.
>
> - clock-frequency: The property is used on platform with broken firmware
> to
> indicate the clock frequency. This should be used by all the domains, however
> this is not yet the case for domUs created by Xen.
>
> To avoid more discrepancy the two functions are now consolidated into one
> place
> make_timer_node().
>
> For simplicity, the compatible will now be based on the bitness even for the
> hwdom. This means the compatible exposed for the hwdom may differ. This should
> only have an impact on 32-bit hwdom booting on Armv8 hardware.
> "
Fine with me.
Thanks
> >
> > Suggested-by: Julien Grall <julien.grall@xxxxxxx>
> > Signed-off-by: Viktor Mitin <viktor_mitin@xxxxxxxx>
> > ---
> > v4 updates:
> > updated "Keep the domU version for the compatible as it is simpler"
> >
> > v5 updates:
> > - changed 'kept' to 'keep', etc.
> > - removed empty line
> > - updated indentation of parameters in functions calls
> > - fixed NITs
> > - updated commit message
> >
> > v6 updates:
> > - move if out of outer "if"
> > - add full stop at the end of the last sentence
> > - minor rephrase of commit message
> >
> > v7 updates:
> > - removed extra braces according to Coding style rule:
> > Braces should be omitted for blocks with a single statement.
> > ---
> > xen/arch/arm/domain_build.c | 96 ++++++++++++-------------------------
> > 1 file changed, 31 insertions(+), 65 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 26cd0ae12c..4e51e22927 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct
> > kernel_info *kinfo)
> > { /* sentinel */ },
> > };
> > struct dt_device_node *dev;
> > - u32 len;
> > - const void *compatible;
> > int res;
> > - unsigned int irq;
> > + unsigned int irq[MAX_TIMER_PPI];
> > gic_interrupt_t intrs[3];
> > u32 clock_frequency;
> > bool clock_valid;
> > @@ -990,35 +988,43 @@ static int __init make_timer_node(const struct
> > kernel_info *kinfo)
> > return -FDT_ERR_XEN(ENOENT);
> > }
> >
> > - compatible = dt_get_property(dev, "compatible", &len);
> > - if ( !compatible )
> > - {
> > - dprintk(XENLOG_ERR, "Can't find compatible property for timer
> > node\n");
> > - return -FDT_ERR_XEN(ENOENT);
> > - }
> > -
> > res = fdt_begin_node(fdt, "timer");
> > if ( res )
> > return res;
> >
> > - res = fdt_property(fdt, "compatible", compatible, len);
> > + if ( !is_64bit_domain(kinfo->d) )
> > + res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
> > + else
> > + res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
> > if ( res )
> > return res;
> >
> > - /* The timer IRQ is emulated by Xen. It always exposes an active-low
> > - * level-sensitive interrupt */
> > -
> > - irq = timer_get_irq(TIMER_PHYS_SECURE_PPI);
> > - dt_dprintk(" Secure interrupt %u\n", irq);
> > - set_interrupt(intrs[0], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > -
> > - irq = timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
> > - dt_dprintk(" Non secure interrupt %u\n", irq);
> > - set_interrupt(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > + /*
> > + * The timer IRQ is emulated by Xen.
> > + * It always exposes an active-low level-sensitive interrupt.
> > + */
> >
> > - irq = timer_get_irq(TIMER_VIRT_PPI);
> > - dt_dprintk(" Virt interrupt %u\n", irq);
> > - set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > + if ( is_hardware_domain(kinfo->d) )
> > + {
> > + irq[TIMER_PHYS_SECURE_PPI] = timer_get_irq(TIMER_PHYS_SECURE_PPI);
> > + irq[TIMER_PHYS_NONSECURE_PPI] =
> > +
> > timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
> > + irq[TIMER_VIRT_PPI] = timer_get_irq(TIMER_VIRT_PPI);
> > + }
> > + else
> > + {
> > + irq[TIMER_PHYS_SECURE_PPI] = GUEST_TIMER_PHYS_S_PPI;
> > + irq[TIMER_PHYS_NONSECURE_PPI] = GUEST_TIMER_PHYS_NS_PPI;
> > + irq[TIMER_VIRT_PPI] = GUEST_TIMER_VIRT_PPI;
> > + }
> > + dt_dprintk(" Secure interrupt %u\n", irq[TIMER_PHYS_SECURE_PPI]);
> > + set_interrupt(intrs[0], irq[TIMER_PHYS_SECURE_PPI],
> > + 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > + dt_dprintk(" Non secure interrupt %u\n",
> > irq[TIMER_PHYS_NONSECURE_PPI]);
> > + set_interrupt(intrs[1], irq[TIMER_PHYS_NONSECURE_PPI],
> > + 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > + dt_dprintk(" Virt interrupt %u\n", irq[TIMER_VIRT_PPI]);
> > + set_interrupt(intrs[2], irq[TIMER_VIRT_PPI], 0xf,
> > DT_IRQ_TYPE_LEVEL_LOW);
> >
> > res = fdt_property_interrupts(kinfo, intrs, 3);
> > if ( res )
> > @@ -1603,46 +1609,6 @@ static int __init make_gic_domU_node(const struct
> > domain *d, void *fdt)
> > }
> > }
> >
> > -static int __init make_timer_domU_node(const struct domain *d, void *fdt)
> > -{
> > - int res;
> > - gic_interrupt_t intrs[3];
> > -
> > - res = fdt_begin_node(fdt, "timer");
> > - if ( res )
> > - return res;
> > -
> > - if ( !is_64bit_domain(d) )
> > - {
> > - res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
> > - if ( res )
> > - return res;
> > - }
> > - else
> > - {
> > - res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
> > - if ( res )
> > - return res;
> > - }
> > -
> > - set_interrupt(intrs[0], GUEST_TIMER_PHYS_S_PPI, 0xf,
> > DT_IRQ_TYPE_LEVEL_LOW);
> > - set_interrupt(intrs[1], GUEST_TIMER_PHYS_NS_PPI, 0xf,
> > DT_IRQ_TYPE_LEVEL_LOW);
> > - set_interrupt(intrs[2], GUEST_TIMER_VIRT_PPI, 0xf,
> > DT_IRQ_TYPE_LEVEL_LOW);
> > -
> > - res = fdt_property(fdt, "interrupts", intrs, sizeof (intrs[0]) * 3);
> > - if ( res )
> > - return res;
> > -
> > - res = fdt_property_cell(fdt, "interrupt-parent",
> > - GUEST_PHANDLE_GIC);
> > - if (res)
> > - return res;
> > -
> > - res = fdt_end_node(fdt);
> > -
> > - return res;
> > -}
> > -
> > #ifdef CONFIG_SBSA_VUART_CONSOLE
> > static int __init make_vpl011_uart_node(const struct domain *d, void *fdt)
> > {
> > @@ -1748,7 +1714,7 @@ static int __init prepare_dtb_domU(struct domain *d,
> > struct kernel_info *kinfo)
> > if ( ret )
> > goto err;
> >
> > - ret = make_timer_domU_node(d, kinfo->fdt);
> > + ret = make_timer_node(kinfo);
> > if ( ret )
> > goto err;
> >
> >
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |