| Message ID | 1435669649-3035-4-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 30, 2015 at 6:07 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > If our builtin kernel bootloader is directly booting a kernel in > the NonSecure world, then we must configure the GIC to put all > the IRQs into the NonSecure group. (By default all interrupts > are configured to be Secure on reset, which means that a NonSecure > guest kernel cannot use any of them.) This job would usually > be done by the Secure boot firmware, but our builtin bootloader > is doing the job of firmware. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/arm/boot.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > index 1e7fd28..3974499 100644 > --- a/hw/arm/boot.c > +++ b/hw/arm/boot.c > @@ -13,6 +13,7 @@ > #include "sysemu/sysemu.h" > #include "hw/boards.h" > #include "hw/loader.h" > +#include "hw/intc/arm_gic_common.h" > #include "elf.h" > #include "sysemu/device_tree.h" > #include "qemu/config-file.h" > @@ -557,6 +558,33 @@ static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key, > fw_cfg_add_bytes(fw_cfg, data_key, data, size); > } > > +static int find_gics(Object *obj, void *opaque) > +{ > + GICState *gic = (GICState *)object_dynamic_cast(obj, TYPE_ARM_GIC_COMMON); > + bool has_sec_extns; > + > + if (!gic) { > + /* Might be a container, traverse it for children */ > + return object_child_foreach(obj, find_gics, opaque); > + } This need to traverse the whole tree has come up more than once for me, so I think this should be a core feature of QOM. > + > + has_sec_extns = object_property_get_bool(obj, "has-security-extensions", > + &error_abort); > + if (has_sec_extns) { > + object_property_set_bool(obj, true, "irqs-reset-nonsecure", > + &error_abort); > + } > + return 0; > +} > + > +static void reconfigure_gics_nonsecure(void) > +{ > + /* Find every GIC in the system and tell it to reconfigure > + * itself with interrupts as NonSecure. > + */ > + object_child_foreach(qdev_get_machine(), find_gics, NULL); > +} > + > static void arm_load_kernel_notify(Notifier *notifier, void *data) > { > CPUState *cs; > @@ -767,6 +795,17 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data) > } > info->is_linux = is_linux; > > + if (is_linux && arm_feature(&cpu->env, ARM_FEATURE_EL3) && Do we need to conditional on ARM_FEATURE_EL3 or can we make GIC logic independent of the primary CPU? > + !info->secure_boot) { > + /* We're directly booting a kernel into NonSecure. If the system > + * has a GIC which implements the security extensions then we must > + * configure it to have all the interrupts be NonSecure (this is > + * a job that is done by the Secure boot firmware, and boot.c is > + * a minimalist firmware-and-boot-loader equivalent). > + */ So I actually had my own patches for this one that went in a different direction. The reason is, there are also other devs out there which need post-firmware state setup. The one I an thinking of mainly is the Zynq SLCR setup for non-firmware boots, I.E. the Linux kernel expects firmware to setup devices to some sort of initialized state (mainly turning clocks on). I think this GIC setup falls in the same category. The third use case is the GIC_CPU_IF stuff currently managed by machine code in boot.c that could be outsourced to GIC (in either a similar way to what is done here, or more fully outsourced using my new API). So with these three use cases of initing devices in post-firmware state, I created a little interface to allow devices to generically specify their post linux state. Patches on list overnight. I'll write more in the cover. Regards, Peter > + reconfigure_gics_nonsecure(); > + } > + > for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) { > ARM_CPU(cs)->env.boot_info = info; > } > -- > 1.9.1 > >
On 30 June 2015 at 20:01, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Tue, Jun 30, 2015 at 6:07 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> If our builtin kernel bootloader is directly booting a kernel in >> the NonSecure world, then we must configure the GIC to put all >> the IRQs into the NonSecure group. (By default all interrupts >> are configured to be Secure on reset, which means that a NonSecure >> guest kernel cannot use any of them.) This job would usually >> be done by the Secure boot firmware, but our builtin bootloader >> is doing the job of firmware. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> hw/arm/boot.c | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c >> index 1e7fd28..3974499 100644 >> --- a/hw/arm/boot.c >> +++ b/hw/arm/boot.c >> @@ -13,6 +13,7 @@ >> #include "sysemu/sysemu.h" >> #include "hw/boards.h" >> #include "hw/loader.h" >> +#include "hw/intc/arm_gic_common.h" >> #include "elf.h" >> #include "sysemu/device_tree.h" >> #include "qemu/config-file.h" >> @@ -557,6 +558,33 @@ static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key, >> fw_cfg_add_bytes(fw_cfg, data_key, data, size); >> } >> >> +static int find_gics(Object *obj, void *opaque) >> +{ >> + GICState *gic = (GICState *)object_dynamic_cast(obj, TYPE_ARM_GIC_COMMON); >> + bool has_sec_extns; >> + >> + if (!gic) { >> + /* Might be a container, traverse it for children */ >> + return object_child_foreach(obj, find_gics, opaque); >> + } > > This need to traverse the whole tree has come up more than once for > me, so I think this should be a core feature of QOM. I did think about that, yeah. I guess something like object_descendants_foreach(), same signature as object_child_foreach(), same semantics except it also iterates through the whole tree (and calls the callback for the nodes-with-children as well as the leaves) ? >> + >> + has_sec_extns = object_property_get_bool(obj, "has-security-extensions", >> + &error_abort); >> + if (has_sec_extns) { >> + object_property_set_bool(obj, true, "irqs-reset-nonsecure", >> + &error_abort); >> + } >> + return 0; >> +} >> + >> +static void reconfigure_gics_nonsecure(void) >> +{ >> + /* Find every GIC in the system and tell it to reconfigure >> + * itself with interrupts as NonSecure. >> + */ >> + object_child_foreach(qdev_get_machine(), find_gics, NULL); >> +} >> + >> static void arm_load_kernel_notify(Notifier *notifier, void *data) >> { >> CPUState *cs; >> @@ -767,6 +795,17 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data) >> } >> info->is_linux = is_linux; >> >> + if (is_linux && arm_feature(&cpu->env, ARM_FEATURE_EL3) && > > Do we need to conditional on ARM_FEATURE_EL3 or can we make GIC logic > independent of the primary CPU? The point here is that "do we need to do this" is exactly dependent on what we're doing with the CPU. Only if we want to put the guest into NS do we do this, and the condition for "are we going to put the guest into NS" is "is this a Linux boot on a CPU with EL3 but where the board says don't boot in S". It matches what the existing logic does for when it sets the SCR_NS bit in do_cpu_reset() in this file. >> + !info->secure_boot) { >> + /* We're directly booting a kernel into NonSecure. If the system >> + * has a GIC which implements the security extensions then we must >> + * configure it to have all the interrupts be NonSecure (this is >> + * a job that is done by the Secure boot firmware, and boot.c is >> + * a minimalist firmware-and-boot-loader equivalent). >> + */ > > So I actually had my own patches for this one that went in a different > direction. The reason is, there are also other devs out there which > need post-firmware state setup. The one I an thinking of mainly is the > Zynq SLCR setup for non-firmware boots, I.E. the Linux kernel expects > firmware to setup devices to some sort of initialized state (mainly > turning clocks on). I think this GIC setup falls in the same category. > The third use case is the GIC_CPU_IF stuff currently managed by > machine code in boot.c that could be outsourced to GIC (in either a > similar way to what is done here, or more fully outsourced using my > new API). I'm a bit torn here -- I don't want to make it *too* easy for people to add linux-booting specific code to random devices, as this will lead to the bootloader code having its tentacles everywhere within the tree... thanks -- PMM
On Tue, Jun 30, 2015 at 12:42 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 June 2015 at 20:01, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Tue, Jun 30, 2015 at 6:07 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> If our builtin kernel bootloader is directly booting a kernel in >>> the NonSecure world, then we must configure the GIC to put all >>> the IRQs into the NonSecure group. (By default all interrupts >>> are configured to be Secure on reset, which means that a NonSecure >>> guest kernel cannot use any of them.) This job would usually >>> be done by the Secure boot firmware, but our builtin bootloader >>> is doing the job of firmware. >>> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>> --- >>> hw/arm/boot.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >>> >>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c >>> index 1e7fd28..3974499 100644 >>> --- a/hw/arm/boot.c >>> +++ b/hw/arm/boot.c >>> @@ -13,6 +13,7 @@ >>> #include "sysemu/sysemu.h" >>> #include "hw/boards.h" >>> #include "hw/loader.h" >>> +#include "hw/intc/arm_gic_common.h" >>> #include "elf.h" >>> #include "sysemu/device_tree.h" >>> #include "qemu/config-file.h" >>> @@ -557,6 +558,33 @@ static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key, >>> fw_cfg_add_bytes(fw_cfg, data_key, data, size); >>> } >>> >>> +static int find_gics(Object *obj, void *opaque) >>> +{ >>> + GICState *gic = (GICState *)object_dynamic_cast(obj, TYPE_ARM_GIC_COMMON); >>> + bool has_sec_extns; >>> + >>> + if (!gic) { >>> + /* Might be a container, traverse it for children */ >>> + return object_child_foreach(obj, find_gics, opaque); >>> + } >> >> This need to traverse the whole tree has come up more than once for >> me, so I think this should be a core feature of QOM. > > I did think about that, yeah. I guess something like > object_descendants_foreach(), same signature as > object_child_foreach(), same semantics except it also > iterates through the whole tree (and calls the callback > for the nodes-with-children as well as the leaves) ? > >>> + >>> + has_sec_extns = object_property_get_bool(obj, "has-security-extensions", >>> + &error_abort); >>> + if (has_sec_extns) { >>> + object_property_set_bool(obj, true, "irqs-reset-nonsecure", >>> + &error_abort); >>> + } >>> + return 0; >>> +} >>> + >>> +static void reconfigure_gics_nonsecure(void) >>> +{ >>> + /* Find every GIC in the system and tell it to reconfigure >>> + * itself with interrupts as NonSecure. >>> + */ >>> + object_child_foreach(qdev_get_machine(), find_gics, NULL); >>> +} >>> + >>> static void arm_load_kernel_notify(Notifier *notifier, void *data) >>> { >>> CPUState *cs; >>> @@ -767,6 +795,17 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data) >>> } >>> info->is_linux = is_linux; >>> >>> + if (is_linux && arm_feature(&cpu->env, ARM_FEATURE_EL3) && >> >> Do we need to conditional on ARM_FEATURE_EL3 or can we make GIC logic >> independent of the primary CPU? > > The point here is that "do we need to do this" is exactly > dependent on what we're doing with the CPU. Only if we > want to put the guest into NS do we do this, and the > condition for "are we going to put the guest into NS" > is "is this a Linux boot on a CPU with EL3 but where > the board says don't boot in S". It matches what the > existing logic does for when it sets the SCR_NS bit in > do_cpu_reset() in this file. > Then maybe this belongs on the lowest common denominator for GIC and CPU - the SoC level. SoCs can register a linux_init function that checks the CPU and GIC NS support and does the switchup. Can make it a common function that multiple socs/machines can call (virt, vexpress and zynq-mp so far I think). For cases where the linux_init in genuinely self contained (like my zynq SLCR case), the device can register it. >>> + !info->secure_boot) { >>> + /* We're directly booting a kernel into NonSecure. If the system >>> + * has a GIC which implements the security extensions then we must >>> + * configure it to have all the interrupts be NonSecure (this is >>> + * a job that is done by the Secure boot firmware, and boot.c is >>> + * a minimalist firmware-and-boot-loader equivalent). >>> + */ >> >> So I actually had my own patches for this one that went in a different >> direction. The reason is, there are also other devs out there which >> need post-firmware state setup. The one I an thinking of mainly is the >> Zynq SLCR setup for non-firmware boots, I.E. the Linux kernel expects >> firmware to setup devices to some sort of initialized state (mainly >> turning clocks on). I think this GIC setup falls in the same category. >> The third use case is the GIC_CPU_IF stuff currently managed by >> machine code in boot.c that could be outsourced to GIC (in either a >> similar way to what is done here, or more fully outsourced using my >> new API). > > I'm a bit torn here -- I don't want to make it *too* easy for > people to add linux-booting specific code to random devices, > as this will lead to the bootloader code having its tentacles > everywhere within the tree... > Maybe the compromise to to restrict this API to SoCs and machines and I can handle my SLCR case with a single "post-firmware" boolean property? Regards, Peter > thanks > -- PMM >
On 30 June 2015 at 21:10, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Tue, Jun 30, 2015 at 12:42 PM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> The point here is that "do we need to do this" is exactly >> dependent on what we're doing with the CPU. Only if we >> want to put the guest into NS do we do this, and the >> condition for "are we going to put the guest into NS" >> is "is this a Linux boot on a CPU with EL3 but where >> the board says don't boot in S". It matches what the >> existing logic does for when it sets the SCR_NS bit in >> do_cpu_reset() in this file. >> > > Then maybe this belongs on the lowest common denominator for GIC and > CPU - the SoC level. SoCs can register a linux_init function that > checks the CPU and GIC NS support and does the switchup. The one board we have so far that needs this code doesn't have an SoC level object -- virt just creates the CPU and GIC itself... -- PMM
On Tue, Jun 30, 2015 at 1:16 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 June 2015 at 21:10, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Tue, Jun 30, 2015 at 12:42 PM, Peter Maydell >> <peter.maydell@linaro.org> wrote: >>> The point here is that "do we need to do this" is exactly >>> dependent on what we're doing with the CPU. Only if we >>> want to put the guest into NS do we do this, and the >>> condition for "are we going to put the guest into NS" >>> is "is this a Linux boot on a CPU with EL3 but where >>> the board says don't boot in S". It matches what the >>> existing logic does for when it sets the SCR_NS bit in >>> do_cpu_reset() in this file. >>> >> >> Then maybe this belongs on the lowest common denominator for GIC and >> CPU - the SoC level. SoCs can register a linux_init function that >> checks the CPU and GIC NS support and does the switchup. > > The one board we have so far that needs this code doesn't > have an SoC level object -- virt just creates the CPU and > GIC itself... Right, but machines are QOMified objects now, so they should be able to implement this interface. zynqmp SoC needs this feature for when we switch on EL3. Regards, Peter > > -- PMM >
On 30 June 2015 at 21:10, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Tue, Jun 30, 2015 at 12:42 PM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> On 30 June 2015 at 20:01, Peter Crosthwaite >> <peter.crosthwaite@xilinx.com> wrote: >>> So I actually had my own patches for this one that went in a different >>> direction. The reason is, there are also other devs out there which >>> need post-firmware state setup. The one I an thinking of mainly is the >>> Zynq SLCR setup for non-firmware boots, I.E. the Linux kernel expects >>> firmware to setup devices to some sort of initialized state (mainly >>> turning clocks on). I think this GIC setup falls in the same category. >>> The third use case is the GIC_CPU_IF stuff currently managed by >>> machine code in boot.c that could be outsourced to GIC (in either a >>> similar way to what is done here, or more fully outsourced using my >>> new API). >> >> I'm a bit torn here -- I don't want to make it *too* easy for >> people to add linux-booting specific code to random devices, >> as this will lead to the bootloader code having its tentacles >> everywhere within the tree... So I thought about this a bit, and I guess that having a general interface is probably better than specifically setting a GIC property. It does need to be called once at arm_kernel_load_notify time, not as a reset hook. I also think it should pass in the arm_boot_info* as a parameter. This would let the GIC do the right thing based on whether we're booting S or NS. Are you planning to respin this patchset, or should I just pull the relevant bits into my series? thanks -- PMM
On Thu, Jul 2, 2015 at 5:41 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 June 2015 at 21:10, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Tue, Jun 30, 2015 at 12:42 PM, Peter Maydell >> <peter.maydell@linaro.org> wrote: >>> On 30 June 2015 at 20:01, Peter Crosthwaite >>> <peter.crosthwaite@xilinx.com> wrote: >>>> So I actually had my own patches for this one that went in a different >>>> direction. The reason is, there are also other devs out there which >>>> need post-firmware state setup. The one I an thinking of mainly is the >>>> Zynq SLCR setup for non-firmware boots, I.E. the Linux kernel expects >>>> firmware to setup devices to some sort of initialized state (mainly >>>> turning clocks on). I think this GIC setup falls in the same category. >>>> The third use case is the GIC_CPU_IF stuff currently managed by >>>> machine code in boot.c that could be outsourced to GIC (in either a >>>> similar way to what is done here, or more fully outsourced using my >>>> new API). >>> >>> I'm a bit torn here -- I don't want to make it *too* easy for >>> people to add linux-booting specific code to random devices, >>> as this will lead to the bootloader code having its tentacles >>> everywhere within the tree... > > So I thought about this a bit, and I guess that having a general > interface is probably better than specifically setting a GIC > property. It does need to be called once at arm_kernel_load_notify > time, not as a reset hook. I also think it should pass in the > arm_boot_info* as a parameter. This would let the GIC do the > right thing based on whether we're booting S or NS. > > Are you planning to respin this patchset, or should I just pull > the relevant bits into my series? > Just pull relevants I think. Should be just patches 1 and 2, the boot.c one needs rewrite and the gic one is a write-off as yours is much more functionally correct. Regards, Peter > thanks > -- PMM >
diff --git a/hw/arm/boot.c b/hw/arm/boot.c index 1e7fd28..3974499 100644 --- a/hw/arm/boot.c +++ b/hw/arm/boot.c @@ -13,6 +13,7 @@ #include "sysemu/sysemu.h" #include "hw/boards.h" #include "hw/loader.h" +#include "hw/intc/arm_gic_common.h" #include "elf.h" #include "sysemu/device_tree.h" #include "qemu/config-file.h" @@ -557,6 +558,33 @@ static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key, fw_cfg_add_bytes(fw_cfg, data_key, data, size); } +static int find_gics(Object *obj, void *opaque) +{ + GICState *gic = (GICState *)object_dynamic_cast(obj, TYPE_ARM_GIC_COMMON); + bool has_sec_extns; + + if (!gic) { + /* Might be a container, traverse it for children */ + return object_child_foreach(obj, find_gics, opaque); + } + + has_sec_extns = object_property_get_bool(obj, "has-security-extensions", + &error_abort); + if (has_sec_extns) { + object_property_set_bool(obj, true, "irqs-reset-nonsecure", + &error_abort); + } + return 0; +} + +static void reconfigure_gics_nonsecure(void) +{ + /* Find every GIC in the system and tell it to reconfigure + * itself with interrupts as NonSecure. + */ + object_child_foreach(qdev_get_machine(), find_gics, NULL); +} + static void arm_load_kernel_notify(Notifier *notifier, void *data) { CPUState *cs; @@ -767,6 +795,17 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data) } info->is_linux = is_linux; + if (is_linux && arm_feature(&cpu->env, ARM_FEATURE_EL3) && + !info->secure_boot) { + /* We're directly booting a kernel into NonSecure. If the system + * has a GIC which implements the security extensions then we must + * configure it to have all the interrupts be NonSecure (this is + * a job that is done by the Secure boot firmware, and boot.c is + * a minimalist firmware-and-boot-loader equivalent). + */ + reconfigure_gics_nonsecure(); + } + for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) { ARM_CPU(cs)->env.boot_info = info; }
If our builtin kernel bootloader is directly booting a kernel in the NonSecure world, then we must configure the GIC to put all the IRQs into the NonSecure group. (By default all interrupts are configured to be Secure on reset, which means that a NonSecure guest kernel cannot use any of them.) This job would usually be done by the Secure boot firmware, but our builtin bootloader is doing the job of firmware. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/boot.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)