Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
Hi, On 11/15/18 12:35 AM, Stefano Stabellini wrote: On Wed, 14 Nov 2018, Julien Grall wrote: On 14/11/2018 22:45, Stefano Stabellini wrote: On Wed, 14 Nov 2018, Julien Grall wrote: Hi, On 13/11/2018 20:44, Stefano Stabellini wrote: On Mon, 12 Nov 2018, Julien Grall wrote: (+ Andre) On 11/12/18 5:42 PM, Mirela Simonovic wrote: Hi Julien, On Mon, Nov 12, 2018 at 6:00 PM Julien Grall wrote: On 11/12/18 4:52 PM, Mirela Simonovic wrote: Hi Julien, Hi, Thanks for the feedback. On Mon, Nov 12, 2018 at 4:36 PM Julien Grall wrote: Hi Mirela, On 11/12/18 11:30 AM, Mirela Simonovic wrote: GIC and virtual timer context must be saved when the domain suspends. Please provide the rationale for this. Is it required by the spec? This is required for GIC because a guest leaves enabled interrupts in the GIC that could wake it up, and on resume it should be able to detect which interrupt woke it up. Without saving/restoring the state of GIC this would not be possible. I am confused. In patch #10, you save the GIC host because the GIC can be powered-down. Linux is also saving the GIC state. So how the interrupt can come up if the GIC is powered down? After Xen (or Linux in the config without Xen) hands over the control to EL3 on suspend (calls system suspend PSCI to EL3), it leaves enabled interrupts that are its wake-up sources. Saving a GIC state only means that its current configuration will be remembered somewhere in data structures, but the configuration is not changed on suspend. Everything that happens with interrupt configuration from this point on is platform specific. Now there are 2 options: 1) EL3 will never allow CPU0 to be powered down and the wake-up interrupt will indeed propagate via GIC; or 2) CPU0 will be powered down and the GIC may be powered down as well, so an external help is needed to deal with wake-up and resume (e.g. in order to react to a wake-up interrupt while the GIC is down, and power up CPU0). This external help is provided by some low-power processor outside of the Cortex-A cluster. So the platform firmware is responsible for properly configuring a wake-up path if GIC goes down. This is commonly handled by EL3 communicating with low-power processor. When the GIC power comes up, the interrupt triggered by a peripheral is still active and the software on Cortex-A cluster should be able to observe it once the GIC state is restored, i.e. interrupts get enabled at GIC. Thank you for the explanation. Now the question is why can't we reset at least the GIC CPU interface? AFAIU, the guest should restore them in any case. The only things we need to know is the interrupt was received for a given guest. We can then queue it and wake-up the domain. This seems to fit with the description on top of gic_dist_save() in Linux GICv2 driver. Can we rely on all PSCI compliant OSes to restore their own GIC again at resume? The PSCI spec is not very clear in that regard (at the the version I am looking at...) I am just asking so that we don't come up with a solution that only works with Linux. See PSCI 1.1 (DEN0022D) section 6.8. Each level should save its own context because the PSCI implementations is allowed to shutdown the GIC. Great, in that case we should be able to skip saving some of the GICD registers too. We do need to save GICD_ISACTIVER, and GICD_ICFGR, but we should be able to skip the others (GICD_ISENABLER, GICD_IPRIORITYR, GICD_ITARGETSR). If we do, we still need to re-initialize them as we do in gicv2_dist_init. You are assuming a domain will handle properly the suspend/resume. I don't think we can promise that as we call freeze/thaw. Dho! That would break every single guest that has been forcefully suspended :-/ Right, we can't do that (FYI I tested today the series with an unaware domU and it all resumed correctly.) But given that we only suspend/resume GICC_CTLR, GICC_PMR, GICC_BPR of the GICC interface, it should be fine to re-initialize that. We do need to be careful because the current implementation of gicv2_cpu_init touches a bunch of GICD registers that we'll have to save separately for suspend/resume. See my review on patch #10. I suggested to move out GICC_CTLR, GICC_PMR, GICC_BPR in a separate helpers that could be called by gicv2_cpu_init and the new function. A good name for that helper would be gicv2_cpu_if_up(). Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
On Wed, 14 Nov 2018, Julien Grall wrote: > On 14/11/2018 22:45, Stefano Stabellini wrote: > > On Wed, 14 Nov 2018, Julien Grall wrote: > >> Hi, > >> > >> On 13/11/2018 20:44, Stefano Stabellini wrote: > >>> On Mon, 12 Nov 2018, Julien Grall wrote: > (+ Andre) > > On 11/12/18 5:42 PM, Mirela Simonovic wrote: > > Hi Julien, > > > > On Mon, Nov 12, 2018 at 6:00 PM Julien Grall > > wrote: > >> > >> > >> > >> On 11/12/18 4:52 PM, Mirela Simonovic wrote: > >>> Hi Julien, > >> > >> Hi, > >> > >>> Thanks for the feedback. > >>> > >>> On Mon, Nov 12, 2018 at 4:36 PM Julien Grall > >>> wrote: > > Hi Mirela, > > On 11/12/18 11:30 AM, Mirela Simonovic wrote: > > GIC and virtual timer context must be saved when the domain > > suspends. > > Please provide the rationale for this. Is it required by the spec? > > >>> > >>> This is required for GIC because a guest leaves enabled interrupts > >>> in > >>> the GIC that could wake it up, and on resume it should be able to > >>> detect which interrupt woke it up. Without saving/restoring the > >>> state > >>> of GIC this would not be possible. > >> > >> I am confused. In patch #10, you save the GIC host because the GIC can > >> be powered-down. Linux is also saving the GIC state. So how the > >> interrupt can come up if the GIC is powered down? > >> > > > > After Xen (or Linux in the config without Xen) hands over the control > > to EL3 on suspend (calls system suspend PSCI to EL3), it leaves > > enabled interrupts that are its wake-up sources. Saving a GIC state > > only means that its current configuration will be remembered somewhere > > in data structures, but the configuration is not changed on suspend. > > Everything that happens with interrupt configuration from this point > > on is platform specific. Now there are 2 options: 1) EL3 will never > > allow CPU0 to be powered down and the wake-up interrupt will indeed > > propagate via GIC; > > or 2) CPU0 will be powered down and the GIC may be > > powered down as well, so an external help is needed to deal with > > wake-up and resume (e.g. in order to react to a wake-up interrupt > > while the GIC is down, and power up CPU0). This external help is > > provided by some low-power processor outside of the Cortex-A cluster. > > > > So the platform firmware is responsible for properly configuring a > > wake-up path if GIC goes down. This is commonly handled by EL3 > > communicating with low-power processor. When the GIC power comes up, > > the interrupt triggered by a peripheral is still active and the > > software on Cortex-A cluster should be able to observe it once the GIC > > state is restored, i.e. interrupts get enabled at GIC. > > Thank you for the explanation. Now the question is why can't we reset at > least the GIC CPU interface? > > AFAIU, the guest should restore them in any case. The only things we need > to > know is the interrupt was received for a given guest. We can then queue > it > and > wake-up the domain. > > This seems to fit with the description on top of gic_dist_save() in Linux > GICv2 driver. > >>> > >>> Can we rely on all PSCI compliant OSes to restore their own GIC again at > >>> resume? The PSCI spec is not very clear in that regard (at the the > >>> version I am looking at...) I am just asking so that we don't come up > >>> with a solution that only works with Linux. > >> See PSCI 1.1 (DEN0022D) section 6.8. Each level should save its own context > >> because the PSCI implementations is allowed to shutdown the GIC. > > > > Great, in that case we should be able to skip saving some of the GICD > > registers too. We do need to save GICD_ISACTIVER, and GICD_ICFGR, > > but we should be able to skip the others (GICD_ISENABLER, > > GICD_IPRIORITYR, GICD_ITARGETSR). If we do, we still need to > > re-initialize them as we do in gicv2_dist_init. > > You are assuming a domain will handle properly the suspend/resume. I > don't think we can promise that as we call freeze/thaw. Dho! That would break every single guest that has been forcefully suspended :-/ Right, we can't do that (FYI I tested today the series with an unaware domU and it all resumed correctly.) But given that we only suspend/resume GICC_CTLR, GICC_PMR, GICC_BPR of the GICC interface, it should be fine to re-initialize that. We do need to be careful because the current implementation of gicv2_cpu_init touches a bunch of GICD registers that we'll have to save separately for suspend/resume. > Furthermore, we still have to suspend/resume other drivers in Xen. I > think this is a bit painful to have to rely on every drivers to deal > with their interrupts.
Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
Sorry for the formatting. On Wed, 14 Nov 2018, 23:41 Julien Grall, wrote: > > > On 14/11/2018 22:45, Stefano Stabellini wrote: > > On Wed, 14 Nov 2018, Julien Grall wrote: > >> Hi, > >> > >> On 13/11/2018 20:44, Stefano Stabellini wrote: > >>> On Mon, 12 Nov 2018, Julien Grall wrote: > (+ Andre) > > On 11/12/18 5:42 PM, Mirela Simonovic wrote: > > Hi Julien, > > > > On Mon, Nov 12, 2018 at 6:00 PM Julien Grall > > wrote: > >> > >> > >> > >> On 11/12/18 4:52 PM, Mirela Simonovic wrote: > >>> Hi Julien, > >> > >> Hi, > >> > >>> Thanks for the feedback. > >>> > >>> On Mon, Nov 12, 2018 at 4:36 PM Julien Grall > > >>> wrote: > > Hi Mirela, > > On 11/12/18 11:30 AM, Mirela Simonovic wrote: > > GIC and virtual timer context must be saved when the domain > > suspends. > > Please provide the rationale for this. Is it required by the spec? > > >>> > >>> This is required for GIC because a guest leaves enabled interrupts > >>> in > >>> the GIC that could wake it up, and on resume it should be able to > >>> detect which interrupt woke it up. Without saving/restoring the > >>> state > >>> of GIC this would not be possible. > >> > >> I am confused. In patch #10, you save the GIC host because the GIC > can > >> be powered-down. Linux is also saving the GIC state. So how the > >> interrupt can come up if the GIC is powered down? > >> > > > > After Xen (or Linux in the config without Xen) hands over the control > > to EL3 on suspend (calls system suspend PSCI to EL3), it leaves > > enabled interrupts that are its wake-up sources. Saving a GIC state > > only means that its current configuration will be remembered > somewhere > > in data structures, but the configuration is not changed on suspend. > > Everything that happens with interrupt configuration from this point > > on is platform specific. Now there are 2 options: 1) EL3 will never > > allow CPU0 to be powered down and the wake-up interrupt will indeed > > propagate via GIC; > > or 2) CPU0 will be powered down and the GIC may be > > powered down as well, so an external help is needed to deal with > > wake-up and resume (e.g. in order to react to a wake-up interrupt > > while the GIC is down, and power up CPU0). This external help is > > provided by some low-power processor outside of the Cortex-A cluster. > > > > So the platform firmware is responsible for properly configuring a > > wake-up path if GIC goes down. This is commonly handled by EL3 > > communicating with low-power processor. When the GIC power comes up, > > the interrupt triggered by a peripheral is still active and the > > software on Cortex-A cluster should be able to observe it once the > GIC > > state is restored, i.e. interrupts get enabled at GIC. > > Thank you for the explanation. Now the question is why can't we > reset at > least the GIC CPU interface? > > AFAIU, the guest should restore them in any case. The only things we > need > to > know is the interrupt was received for a given guest. We can then > queue it > and > wake-up the domain. > > This seems to fit with the description on top of gic_dist_save() in > Linux > GICv2 driver. > >>> > >>> Can we rely on all PSCI compliant OSes to restore their own GIC again > at > >>> resume? The PSCI spec is not very clear in that regard (at the the > >>> version I am looking at...) I am just asking so that we don't come up > >>> with a solution that only works with Linux. > >> See PSCI 1.1 (DEN0022D) section 6.8. Each level should save its own > context > >> because the PSCI implementations is allowed to shutdown the GIC. > > > > Great, in that case we should be able to skip saving some of the GICD > > registers too. We do need to save GICD_ISACTIVER, and GICD_ICFGR, > > but we should be able to skip the others (GICD_ISENABLER, > > GICD_IPRIORITYR, GICD_ITARGETSR). If we do, we still need to > > re-initialize them as we do in gicv2_dist_init. > > You are assuming a domain will handle properly the suspend/resume. I > don't think we can promise that as we call freeze/thaw. > To clarify what I meant by "handle properly" is any domain that will not call SYSTEM_SUSPEND before the host is suspending. That may happen if the domain is not aware of suspend/resume. If you wonder, it is not Xen to decide whether we should stop suspending but the control domain to not issue the suspend. Cheers, > Furthermore, we still have to suspend/resume other drivers in Xen. I > think this is a bit painful to have to rely on every drivers to deal > with their interrupts. > > Cheers, > > -- > Julien Grall > ___ > Xen-devel mailing list > Xen-devel@lists.xenproject.org >
Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
On 14/11/2018 22:45, Stefano Stabellini wrote: > On Wed, 14 Nov 2018, Julien Grall wrote: >> Hi, >> >> On 13/11/2018 20:44, Stefano Stabellini wrote: >>> On Mon, 12 Nov 2018, Julien Grall wrote: (+ Andre) On 11/12/18 5:42 PM, Mirela Simonovic wrote: > Hi Julien, > > On Mon, Nov 12, 2018 at 6:00 PM Julien Grall > wrote: >> >> >> >> On 11/12/18 4:52 PM, Mirela Simonovic wrote: >>> Hi Julien, >> >> Hi, >> >>> Thanks for the feedback. >>> >>> On Mon, Nov 12, 2018 at 4:36 PM Julien Grall >>> wrote: Hi Mirela, On 11/12/18 11:30 AM, Mirela Simonovic wrote: > GIC and virtual timer context must be saved when the domain > suspends. Please provide the rationale for this. Is it required by the spec? >>> >>> This is required for GIC because a guest leaves enabled interrupts >>> in >>> the GIC that could wake it up, and on resume it should be able to >>> detect which interrupt woke it up. Without saving/restoring the >>> state >>> of GIC this would not be possible. >> >> I am confused. In patch #10, you save the GIC host because the GIC can >> be powered-down. Linux is also saving the GIC state. So how the >> interrupt can come up if the GIC is powered down? >> > > After Xen (or Linux in the config without Xen) hands over the control > to EL3 on suspend (calls system suspend PSCI to EL3), it leaves > enabled interrupts that are its wake-up sources. Saving a GIC state > only means that its current configuration will be remembered somewhere > in data structures, but the configuration is not changed on suspend. > Everything that happens with interrupt configuration from this point > on is platform specific. Now there are 2 options: 1) EL3 will never > allow CPU0 to be powered down and the wake-up interrupt will indeed > propagate via GIC; > or 2) CPU0 will be powered down and the GIC may be > powered down as well, so an external help is needed to deal with > wake-up and resume (e.g. in order to react to a wake-up interrupt > while the GIC is down, and power up CPU0). This external help is > provided by some low-power processor outside of the Cortex-A cluster. > > So the platform firmware is responsible for properly configuring a > wake-up path if GIC goes down. This is commonly handled by EL3 > communicating with low-power processor. When the GIC power comes up, > the interrupt triggered by a peripheral is still active and the > software on Cortex-A cluster should be able to observe it once the GIC > state is restored, i.e. interrupts get enabled at GIC. Thank you for the explanation. Now the question is why can't we reset at least the GIC CPU interface? AFAIU, the guest should restore them in any case. The only things we need to know is the interrupt was received for a given guest. We can then queue it and wake-up the domain. This seems to fit with the description on top of gic_dist_save() in Linux GICv2 driver. >>> >>> Can we rely on all PSCI compliant OSes to restore their own GIC again at >>> resume? The PSCI spec is not very clear in that regard (at the the >>> version I am looking at...) I am just asking so that we don't come up >>> with a solution that only works with Linux. >> See PSCI 1.1 (DEN0022D) section 6.8. Each level should save its own context >> because the PSCI implementations is allowed to shutdown the GIC. > > Great, in that case we should be able to skip saving some of the GICD > registers too. We do need to save GICD_ISACTIVER, and GICD_ICFGR, > but we should be able to skip the others (GICD_ISENABLER, > GICD_IPRIORITYR, GICD_ITARGETSR). If we do, we still need to > re-initialize them as we do in gicv2_dist_init. You are assuming a domain will handle properly the suspend/resume. I don't think we can promise that as we call freeze/thaw. Furthermore, we still have to suspend/resume other drivers in Xen. I think this is a bit painful to have to rely on every drivers to deal with their interrupts. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
On Wed, 14 Nov 2018, Julien Grall wrote: > Hi, > > On 13/11/2018 20:44, Stefano Stabellini wrote: > > On Mon, 12 Nov 2018, Julien Grall wrote: > > > (+ Andre) > > > > > > On 11/12/18 5:42 PM, Mirela Simonovic wrote: > > > > Hi Julien, > > > > > > > > On Mon, Nov 12, 2018 at 6:00 PM Julien Grall > > > > wrote: > > > > > > > > > > > > > > > > > > > > On 11/12/18 4:52 PM, Mirela Simonovic wrote: > > > > > > Hi Julien, > > > > > > > > > > Hi, > > > > > > > > > > > Thanks for the feedback. > > > > > > > > > > > > On Mon, Nov 12, 2018 at 4:36 PM Julien Grall > > > > > > wrote: > > > > > > > > > > > > > > Hi Mirela, > > > > > > > > > > > > > > On 11/12/18 11:30 AM, Mirela Simonovic wrote: > > > > > > > > GIC and virtual timer context must be saved when the domain > > > > > > > > suspends. > > > > > > > > > > > > > > Please provide the rationale for this. Is it required by the spec? > > > > > > > > > > > > > > > > > > > This is required for GIC because a guest leaves enabled interrupts > > > > > > in > > > > > > the GIC that could wake it up, and on resume it should be able to > > > > > > detect which interrupt woke it up. Without saving/restoring the > > > > > > state > > > > > > of GIC this would not be possible. > > > > > > > > > > I am confused. In patch #10, you save the GIC host because the GIC can > > > > > be powered-down. Linux is also saving the GIC state. So how the > > > > > interrupt can come up if the GIC is powered down? > > > > > > > > > > > > > After Xen (or Linux in the config without Xen) hands over the control > > > > to EL3 on suspend (calls system suspend PSCI to EL3), it leaves > > > > enabled interrupts that are its wake-up sources. Saving a GIC state > > > > only means that its current configuration will be remembered somewhere > > > > in data structures, but the configuration is not changed on suspend. > > > > Everything that happens with interrupt configuration from this point > > > > on is platform specific. Now there are 2 options: 1) EL3 will never > > > > allow CPU0 to be powered down and the wake-up interrupt will indeed > > > > propagate via GIC; > > > > or 2) CPU0 will be powered down and the GIC may be > > > > powered down as well, so an external help is needed to deal with > > > > wake-up and resume (e.g. in order to react to a wake-up interrupt > > > > while the GIC is down, and power up CPU0). This external help is > > > > provided by some low-power processor outside of the Cortex-A cluster. > > > > > > > > So the platform firmware is responsible for properly configuring a > > > > wake-up path if GIC goes down. This is commonly handled by EL3 > > > > communicating with low-power processor. When the GIC power comes up, > > > > the interrupt triggered by a peripheral is still active and the > > > > software on Cortex-A cluster should be able to observe it once the GIC > > > > state is restored, i.e. interrupts get enabled at GIC. > > > > > > Thank you for the explanation. Now the question is why can't we reset at > > > least the GIC CPU interface? > > > > > > AFAIU, the guest should restore them in any case. The only things we need > > > to > > > know is the interrupt was received for a given guest. We can then queue it > > > and > > > wake-up the domain. > > > > > > This seems to fit with the description on top of gic_dist_save() in Linux > > > GICv2 driver. > > > > Can we rely on all PSCI compliant OSes to restore their own GIC again at > > resume? The PSCI spec is not very clear in that regard (at the the > > version I am looking at...) I am just asking so that we don't come up > > with a solution that only works with Linux. > See PSCI 1.1 (DEN0022D) section 6.8. Each level should save its own context > because the PSCI implementations is allowed to shutdown the GIC. Great, in that case we should be able to skip saving some of the GICD registers too. We do need to save GICD_ISACTIVER, and GICD_ICFGR, but we should be able to skip the others (GICD_ISENABLER, GICD_IPRIORITYR, GICD_ITARGETSR). If we do, we still need to re-initialize them as we do in gicv2_dist_init. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
Hi, On 13/11/2018 20:44, Stefano Stabellini wrote: On Mon, 12 Nov 2018, Julien Grall wrote: (+ Andre) On 11/12/18 5:42 PM, Mirela Simonovic wrote: Hi Julien, On Mon, Nov 12, 2018 at 6:00 PM Julien Grall wrote: On 11/12/18 4:52 PM, Mirela Simonovic wrote: Hi Julien, Hi, Thanks for the feedback. On Mon, Nov 12, 2018 at 4:36 PM Julien Grall wrote: Hi Mirela, On 11/12/18 11:30 AM, Mirela Simonovic wrote: GIC and virtual timer context must be saved when the domain suspends. Please provide the rationale for this. Is it required by the spec? This is required for GIC because a guest leaves enabled interrupts in the GIC that could wake it up, and on resume it should be able to detect which interrupt woke it up. Without saving/restoring the state of GIC this would not be possible. I am confused. In patch #10, you save the GIC host because the GIC can be powered-down. Linux is also saving the GIC state. So how the interrupt can come up if the GIC is powered down? After Xen (or Linux in the config without Xen) hands over the control to EL3 on suspend (calls system suspend PSCI to EL3), it leaves enabled interrupts that are its wake-up sources. Saving a GIC state only means that its current configuration will be remembered somewhere in data structures, but the configuration is not changed on suspend. Everything that happens with interrupt configuration from this point on is platform specific. Now there are 2 options: 1) EL3 will never allow CPU0 to be powered down and the wake-up interrupt will indeed propagate via GIC; or 2) CPU0 will be powered down and the GIC may be powered down as well, so an external help is needed to deal with wake-up and resume (e.g. in order to react to a wake-up interrupt while the GIC is down, and power up CPU0). This external help is provided by some low-power processor outside of the Cortex-A cluster. So the platform firmware is responsible for properly configuring a wake-up path if GIC goes down. This is commonly handled by EL3 communicating with low-power processor. When the GIC power comes up, the interrupt triggered by a peripheral is still active and the software on Cortex-A cluster should be able to observe it once the GIC state is restored, i.e. interrupts get enabled at GIC. Thank you for the explanation. Now the question is why can't we reset at least the GIC CPU interface? AFAIU, the guest should restore them in any case. The only things we need to know is the interrupt was received for a given guest. We can then queue it and wake-up the domain. This seems to fit with the description on top of gic_dist_save() in Linux GICv2 driver. Can we rely on all PSCI compliant OSes to restore their own GIC again at resume? The PSCI spec is not very clear in that regard (at the the version I am looking at...) I am just asking so that we don't come up with a solution that only works with Linux. See PSCI 1.1 (DEN0022D) section 6.8. Each level should save its own context because the PSCI implementations is allowed to shutdown the GIC. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
On Mon, 12 Nov 2018, Julien Grall wrote: > (+ Andre) > > On 11/12/18 5:42 PM, Mirela Simonovic wrote: > > Hi Julien, > > > > On Mon, Nov 12, 2018 at 6:00 PM Julien Grall wrote: > > > > > > > > > > > > On 11/12/18 4:52 PM, Mirela Simonovic wrote: > > > > Hi Julien, > > > > > > Hi, > > > > > > > Thanks for the feedback. > > > > > > > > On Mon, Nov 12, 2018 at 4:36 PM Julien Grall > > > > wrote: > > > > > > > > > > Hi Mirela, > > > > > > > > > > On 11/12/18 11:30 AM, Mirela Simonovic wrote: > > > > > > GIC and virtual timer context must be saved when the domain > > > > > > suspends. > > > > > > > > > > Please provide the rationale for this. Is it required by the spec? > > > > > > > > > > > > > This is required for GIC because a guest leaves enabled interrupts in > > > > the GIC that could wake it up, and on resume it should be able to > > > > detect which interrupt woke it up. Without saving/restoring the state > > > > of GIC this would not be possible. > > > > > > I am confused. In patch #10, you save the GIC host because the GIC can > > > be powered-down. Linux is also saving the GIC state. So how the > > > interrupt can come up if the GIC is powered down? > > > > > > > After Xen (or Linux in the config without Xen) hands over the control > > to EL3 on suspend (calls system suspend PSCI to EL3), it leaves > > enabled interrupts that are its wake-up sources. Saving a GIC state > > only means that its current configuration will be remembered somewhere > > in data structures, but the configuration is not changed on suspend. > > Everything that happens with interrupt configuration from this point > > on is platform specific. Now there are 2 options: 1) EL3 will never > > allow CPU0 to be powered down and the wake-up interrupt will indeed > > propagate via GIC; > > or 2) CPU0 will be powered down and the GIC may be > > powered down as well, so an external help is needed to deal with > > wake-up and resume (e.g. in order to react to a wake-up interrupt > > while the GIC is down, and power up CPU0). This external help is > > provided by some low-power processor outside of the Cortex-A cluster. > > > > So the platform firmware is responsible for properly configuring a > > wake-up path if GIC goes down. This is commonly handled by EL3 > > communicating with low-power processor. When the GIC power comes up, > > the interrupt triggered by a peripheral is still active and the > > software on Cortex-A cluster should be able to observe it once the GIC > > state is restored, i.e. interrupts get enabled at GIC. > > Thank you for the explanation. Now the question is why can't we reset at > least the GIC CPU interface? > > AFAIU, the guest should restore them in any case. The only things we need to > know is the interrupt was received for a given guest. We can then queue it and > wake-up the domain. > > This seems to fit with the description on top of gic_dist_save() in Linux > GICv2 driver. Can we rely on all PSCI compliant OSes to restore their own GIC again at resume? The PSCI spec is not very clear in that regard (at the the version I am looking at...) I am just asking so that we don't come up with a solution that only works with Linux. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
(+ Andre) On 11/12/18 5:42 PM, Mirela Simonovic wrote: Hi Julien, On Mon, Nov 12, 2018 at 6:00 PM Julien Grall wrote: On 11/12/18 4:52 PM, Mirela Simonovic wrote: Hi Julien, Hi, Thanks for the feedback. On Mon, Nov 12, 2018 at 4:36 PM Julien Grall wrote: Hi Mirela, On 11/12/18 11:30 AM, Mirela Simonovic wrote: GIC and virtual timer context must be saved when the domain suspends. Please provide the rationale for this. Is it required by the spec? This is required for GIC because a guest leaves enabled interrupts in the GIC that could wake it up, and on resume it should be able to detect which interrupt woke it up. Without saving/restoring the state of GIC this would not be possible. I am confused. In patch #10, you save the GIC host because the GIC can be powered-down. Linux is also saving the GIC state. So how the interrupt can come up if the GIC is powered down? After Xen (or Linux in the config without Xen) hands over the control to EL3 on suspend (calls system suspend PSCI to EL3), it leaves enabled interrupts that are its wake-up sources. Saving a GIC state only means that its current configuration will be remembered somewhere in data structures, but the configuration is not changed on suspend. Everything that happens with interrupt configuration from this point on is platform specific. Now there are 2 options: 1) EL3 will never allow CPU0 to be powered down and the wake-up interrupt will indeed propagate via GIC; or 2) CPU0 will be powered down and the GIC may be powered down as well, so an external help is needed to deal with wake-up and resume (e.g. in order to react to a wake-up interrupt while the GIC is down, and power up CPU0). This external help is provided by some low-power processor outside of the Cortex-A cluster. So the platform firmware is responsible for properly configuring a wake-up path if GIC goes down. This is commonly handled by EL3 communicating with low-power processor. When the GIC power comes up, the interrupt triggered by a peripheral is still active and the software on Cortex-A cluster should be able to observe it once the GIC state is restored, i.e. interrupts get enabled at GIC. Thank you for the explanation. Now the question is why can't we reset at least the GIC CPU interface? AFAIU, the guest should restore them in any case. The only things we need to know is the interrupt was received for a given guest. We can then queue it and wake-up the domain. This seems to fit with the description on top of gic_dist_save() in Linux GICv2 driver. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
Hi Julien, On Mon, Nov 12, 2018 at 6:00 PM Julien Grall wrote: > > > > On 11/12/18 4:52 PM, Mirela Simonovic wrote: > > Hi Julien, > > Hi, > > > Thanks for the feedback. > > > > On Mon, Nov 12, 2018 at 4:36 PM Julien Grall wrote: > >> > >> Hi Mirela, > >> > >> On 11/12/18 11:30 AM, Mirela Simonovic wrote: > >>> GIC and virtual timer context must be saved when the domain suspends. > >> > >> Please provide the rationale for this. Is it required by the spec? > >> > > > > This is required for GIC because a guest leaves enabled interrupts in > > the GIC that could wake it up, and on resume it should be able to > > detect which interrupt woke it up. Without saving/restoring the state > > of GIC this would not be possible. > > I am confused. In patch #10, you save the GIC host because the GIC can > be powered-down. Linux is also saving the GIC state. So how the > interrupt can come up if the GIC is powered down? > After Xen (or Linux in the config without Xen) hands over the control to EL3 on suspend (calls system suspend PSCI to EL3), it leaves enabled interrupts that are its wake-up sources. Saving a GIC state only means that its current configuration will be remembered somewhere in data structures, but the configuration is not changed on suspend. Everything that happens with interrupt configuration from this point on is platform specific. Now there are 2 options: 1) EL3 will never allow CPU0 to be powered down and the wake-up interrupt will indeed propagate via GIC; or 2) CPU0 will be powered down and the GIC may be powered down as well, so an external help is needed to deal with wake-up and resume (e.g. in order to react to a wake-up interrupt while the GIC is down, and power up CPU0). This external help is provided by some low-power processor outside of the Cortex-A cluster. So the platform firmware is responsible for properly configuring a wake-up path if GIC goes down. This is commonly handled by EL3 communicating with low-power processor. When the GIC power comes up, the interrupt triggered by a peripheral is still active and the software on Cortex-A cluster should be able to observe it once the GIC state is restored, i.e. interrupts get enabled at GIC. > > For timer, my understanding is that vtimer accounts for how long the > > guest was executing and this tracking should not be affected by the > > suspend/resume sequence. Without saving/restoring the state of vtimer, > > the time tracking would be affected by the suspend/resume. > > Currently, vtimer does not take such things into account. It only setup > a timer in Xen to wake up the guest later on. > > > Hope this is ok, I'll add it to the commit message. > > > >>> This is done by moving the respective code in ctxt_switch_from() > >>> before the return that happens if the domain suspended. > >>> > >>> Signed-off-by: Mirela Simonovic > >>> Signed-off-by: Saeed Nowshadi > >>> --- > >>>xen/arch/arm/domain.c | 14 +++--- > >>>1 file changed, 7 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > >>> index 7f8105465c..bebe3238e8 100644 > >>> --- a/xen/arch/arm/domain.c > >>> +++ b/xen/arch/arm/domain.c > >>> @@ -97,6 +97,13 @@ static void ctxt_switch_from(struct vcpu *p) > >>>if ( is_idle_vcpu(p) ) > >>>return; > >>> > >>> +/* VGIC */ > >>> +gic_save_state(p); > >>> + > >>> +/* Arch timer */ > >>> +p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1); > >>> +virt_timer_save(p); > >>> + > >>>/* VCPU's context should not be saved if its domain is suspended */ > >> > >> The GIC and timer are part of the vCPU context. So this comment looks a > >> bit stale. > > > > It's not stale, but incorrect in that perspective. The comment should > > be updated to "VCPU architecture specific context should ..." > > But the timer is part of the architecture... > > Cheers, > > -- > Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
On 11/12/18 4:52 PM, Mirela Simonovic wrote: Hi Julien, Hi, Thanks for the feedback. On Mon, Nov 12, 2018 at 4:36 PM Julien Grall wrote: Hi Mirela, On 11/12/18 11:30 AM, Mirela Simonovic wrote: GIC and virtual timer context must be saved when the domain suspends. Please provide the rationale for this. Is it required by the spec? This is required for GIC because a guest leaves enabled interrupts in the GIC that could wake it up, and on resume it should be able to detect which interrupt woke it up. Without saving/restoring the state of GIC this would not be possible. I am confused. In patch #10, you save the GIC host because the GIC can be powered-down. Linux is also saving the GIC state. So how the interrupt can come up if the GIC is powered down? For timer, my understanding is that vtimer accounts for how long the guest was executing and this tracking should not be affected by the suspend/resume sequence. Without saving/restoring the state of vtimer, the time tracking would be affected by the suspend/resume. Currently, vtimer does not take such things into account. It only setup a timer in Xen to wake up the guest later on. Hope this is ok, I'll add it to the commit message. This is done by moving the respective code in ctxt_switch_from() before the return that happens if the domain suspended. Signed-off-by: Mirela Simonovic Signed-off-by: Saeed Nowshadi --- xen/arch/arm/domain.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 7f8105465c..bebe3238e8 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -97,6 +97,13 @@ static void ctxt_switch_from(struct vcpu *p) if ( is_idle_vcpu(p) ) return; +/* VGIC */ +gic_save_state(p); + +/* Arch timer */ +p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1); +virt_timer_save(p); + /* VCPU's context should not be saved if its domain is suspended */ The GIC and timer are part of the vCPU context. So this comment looks a bit stale. It's not stale, but incorrect in that perspective. The comment should be updated to "VCPU architecture specific context should ..." But the timer is part of the architecture... Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
Hi Julien,
Thanks for the feedback.
On Mon, Nov 12, 2018 at 4:36 PM Julien Grall wrote:
>
> Hi Mirela,
>
> On 11/12/18 11:30 AM, Mirela Simonovic wrote:
> > GIC and virtual timer context must be saved when the domain suspends.
>
> Please provide the rationale for this. Is it required by the spec?
>
This is required for GIC because a guest leaves enabled interrupts in
the GIC that could wake it up, and on resume it should be able to
detect which interrupt woke it up. Without saving/restoring the state
of GIC this would not be possible.
For timer, my understanding is that vtimer accounts for how long the
guest was executing and this tracking should not be affected by the
suspend/resume sequence. Without saving/restoring the state of vtimer,
the time tracking would be affected by the suspend/resume.
Hope this is ok, I'll add it to the commit message.
> > This is done by moving the respective code in ctxt_switch_from()
> > before the return that happens if the domain suspended.
> >
> > Signed-off-by: Mirela Simonovic
> > Signed-off-by: Saeed Nowshadi
> > ---
> > xen/arch/arm/domain.c | 14 +++---
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 7f8105465c..bebe3238e8 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -97,6 +97,13 @@ static void ctxt_switch_from(struct vcpu *p)
> > if ( is_idle_vcpu(p) )
> > return;
> >
> > +/* VGIC */
> > +gic_save_state(p);
> > +
> > +/* Arch timer */
> > +p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
> > +virt_timer_save(p);
> > +
> > /* VCPU's context should not be saved if its domain is suspended */
>
> The GIC and timer are part of the vCPU context. So this comment looks a
> bit stale.
It's not stale, but incorrect in that perspective. The comment should
be updated to "VCPU architecture specific context should ..."
>
> > if ( p->domain->is_shut_down &&
> > (p->domain->shutdown_code == SHUTDOWN_suspend) )
> > @@ -115,10 +122,6 @@ static void ctxt_switch_from(struct vcpu *p)
> > p->arch.tpidrro_el0 = READ_SYSREG(TPIDRRO_EL0);
> > p->arch.tpidr_el1 = READ_SYSREG(TPIDR_EL1);
> >
> > -/* Arch timer */
> > -p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
> > -virt_timer_save(p);
> > -
> > if ( is_32bit_domain(p->domain) && cpu_has_thumbee )
> > {
> > p->arch.teecr = READ_SYSREG32(TEECR32_EL1);
> > @@ -170,9 +173,6 @@ static void ctxt_switch_from(struct vcpu *p)
> > /* VFP */
> > vfp_save_state(p);
> >
> > -/* VGIC */
> > -gic_save_state(p);
> > -
> > isb();
> > }
> >
> >
>
> Cheers,
>
> --
> Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
Hi Mirela,
On 11/12/18 11:30 AM, Mirela Simonovic wrote:
GIC and virtual timer context must be saved when the domain suspends.
Please provide the rationale for this. Is it required by the spec?
This is done by moving the respective code in ctxt_switch_from()
before the return that happens if the domain suspended.
Signed-off-by: Mirela Simonovic
Signed-off-by: Saeed Nowshadi
---
xen/arch/arm/domain.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 7f8105465c..bebe3238e8 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,13 @@ static void ctxt_switch_from(struct vcpu *p)
if ( is_idle_vcpu(p) )
return;
+/* VGIC */
+gic_save_state(p);
+
+/* Arch timer */
+p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
+virt_timer_save(p);
+
/* VCPU's context should not be saved if its domain is suspended */
The GIC and timer are part of the vCPU context. So this comment looks a
bit stale.
if ( p->domain->is_shut_down &&
(p->domain->shutdown_code == SHUTDOWN_suspend) )
@@ -115,10 +122,6 @@ static void ctxt_switch_from(struct vcpu *p)
p->arch.tpidrro_el0 = READ_SYSREG(TPIDRRO_EL0);
p->arch.tpidr_el1 = READ_SYSREG(TPIDR_EL1);
-/* Arch timer */
-p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
-virt_timer_save(p);
-
if ( is_32bit_domain(p->domain) && cpu_has_thumbee )
{
p->arch.teecr = READ_SYSREG32(TEECR32_EL1);
@@ -170,9 +173,6 @@ static void ctxt_switch_from(struct vcpu *p)
/* VFP */
vfp_save_state(p);
-/* VGIC */
-gic_save_state(p);
-
isb();
}
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
GIC and virtual timer context must be saved when the domain suspends.
This is done by moving the respective code in ctxt_switch_from()
before the return that happens if the domain suspended.
Signed-off-by: Mirela Simonovic
Signed-off-by: Saeed Nowshadi
---
xen/arch/arm/domain.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 7f8105465c..bebe3238e8 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,13 @@ static void ctxt_switch_from(struct vcpu *p)
if ( is_idle_vcpu(p) )
return;
+/* VGIC */
+gic_save_state(p);
+
+/* Arch timer */
+p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
+virt_timer_save(p);
+
/* VCPU's context should not be saved if its domain is suspended */
if ( p->domain->is_shut_down &&
(p->domain->shutdown_code == SHUTDOWN_suspend) )
@@ -115,10 +122,6 @@ static void ctxt_switch_from(struct vcpu *p)
p->arch.tpidrro_el0 = READ_SYSREG(TPIDRRO_EL0);
p->arch.tpidr_el1 = READ_SYSREG(TPIDR_EL1);
-/* Arch timer */
-p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
-virt_timer_save(p);
-
if ( is_32bit_domain(p->domain) && cpu_has_thumbee )
{
p->arch.teecr = READ_SYSREG32(TEECR32_EL1);
@@ -170,9 +173,6 @@ static void ctxt_switch_from(struct vcpu *p)
/* VFP */
vfp_save_state(p);
-/* VGIC */
-gic_save_state(p);
-
isb();
}
--
2.13.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
