| Message ID | 20201127073212.219148-1-clg@kaod.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xive/p9: Remove assert from xive_eq_for_target() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (89a32b4930be829f37e6967354a759e38048d01f) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On Fri, 27 Nov 2020 08:32:12 +0100 Cédric Le Goater <clg@kaod.org> wrote: > XIVE VPs are structures describing the vCPUs of guests. When starting > a guest, these are allocated and enabled and some checks are done on > the location of the associated ENDs, which describe the event > queues. If the block of the VP and the block of the ENDs do not match, > the XIVE driver asserts. > > Unfortunately, there is no way to check that a VP identifier is part > of a VP block that was previously allocated and it is relatively easy > to crash the host with a bogus VP id. That can be done with a QEMU > hack on a machine using vsmt. > Reported-by: Greg Kurz <groug@kaod.org> :) > Simply remove the assert, the OS should gracefully handle the error. > This seems to be reasonable when xive_eq_for_target() is called from an OPAL call since they'd all return OPAL_PARAMETER to the OS in this case. Some other paths maybe need more care though, eg: xive_ipi_init() __xive_set_irq_config() xive_set_irq_targetting() xive_eq_for_target() If xive_eq_for_target() fails to map the target to a valid EQ, this ends up being ignored in xive_ipi_init() with this patch. Is it okay ? > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/xive.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/xive.c b/hw/xive.c > index 7d4e029f19cb..c442ea5e30ed 100644 > --- a/hw/xive.c > +++ b/hw/xive.c > @@ -2152,7 +2152,7 @@ static inline bool xive_eq_for_target(uint32_t target, uint8_t prio, > if (eq_blk != vp_blk) { > xive_err(x, "eq_blk != vp_blk (%d vs. %d) for target 0x%08x/%d\n", > eq_blk, vp_blk, target, prio); > - assert(false); > + return false; > } > > if (out_eq_blk)
On 11/27/20 9:08 AM, Greg Kurz wrote: > On Fri, 27 Nov 2020 08:32:12 +0100 > Cédric Le Goater <clg@kaod.org> wrote: > >> XIVE VPs are structures describing the vCPUs of guests. When starting >> a guest, these are allocated and enabled and some checks are done on >> the location of the associated ENDs, which describe the event >> queues. If the block of the VP and the block of the ENDs do not match, >> the XIVE driver asserts. >> >> Unfortunately, there is no way to check that a VP identifier is part >> of a VP block that was previously allocated and it is relatively easy >> to crash the host with a bogus VP id. That can be done with a QEMU >> hack on a machine using vsmt. >> > > Reported-by: Greg Kurz <groug@kaod.org> > > :) > >> Simply remove the assert, the OS should gracefully handle the error. >> > > This seems to be reasonable when xive_eq_for_target() is called from > an OPAL call since they'd all return OPAL_PARAMETER to the OS in this > case. Linux behaves correctly. There are errors but the rollback is correct. > Some other paths maybe need more care though, eg: > > xive_ipi_init() > __xive_set_irq_config() > xive_set_irq_targetting() > xive_eq_for_target() > > If xive_eq_for_target() fails to map the target to a valid EQ, > this ends up being ignored in xive_ipi_init() with this patch. > Is it okay ? This is the P8 emulation layer fully handled by OPAL. It the VP is bogus, then it means that OPAL just screwed up. And this seems to be the case because it's using cpu->pir and not a vp identifier ... If this is working, it's pure luck. Another good reason to drop support for this emulation which is a source of complexity. It's dropped for P10. See : patchwork.ozlabs.org/patch/1157193 xive/p9: remove XICS emulation patchwork.ozlabs.org/patch/1157182 xive/p9: remove percpu logging patchwork.ozlabs.org/patch/1157172 xive/p9: remove XICS emulation OPAL calls I would be very happy to push these again and get rid of 1100 lines of useless bringup complexity. C. > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/xive.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/xive.c b/hw/xive.c >> index 7d4e029f19cb..c442ea5e30ed 100644 >> --- a/hw/xive.c >> +++ b/hw/xive.c >> @@ -2152,7 +2152,7 @@ static inline bool xive_eq_for_target(uint32_t target, uint8_t prio, >> if (eq_blk != vp_blk) { >> xive_err(x, "eq_blk != vp_blk (%d vs. %d) for target 0x%08x/%d\n", >> eq_blk, vp_blk, target, prio); >> - assert(false); >> + return false; >> } >> >> if (out_eq_blk) >
On Fri, 27 Nov 2020 09:32:43 +0100 Cédric Le Goater <clg@kaod.org> wrote: > On 11/27/20 9:08 AM, Greg Kurz wrote: > > On Fri, 27 Nov 2020 08:32:12 +0100 > > Cédric Le Goater <clg@kaod.org> wrote: > > > >> XIVE VPs are structures describing the vCPUs of guests. When starting > >> a guest, these are allocated and enabled and some checks are done on > >> the location of the associated ENDs, which describe the event > >> queues. If the block of the VP and the block of the ENDs do not match, > >> the XIVE driver asserts. > >> > >> Unfortunately, there is no way to check that a VP identifier is part > >> of a VP block that was previously allocated and it is relatively easy > >> to crash the host with a bogus VP id. That can be done with a QEMU > >> hack on a machine using vsmt. > >> > > > > Reported-by: Greg Kurz <groug@kaod.org> > > > > :) > > > >> Simply remove the assert, the OS should gracefully handle the error. > >> > > > > This seems to be reasonable when xive_eq_for_target() is called from > > an OPAL call since they'd all return OPAL_PARAMETER to the OS in this > > case. > > Linux behaves correctly. There are errors but the rollback is correct. > Cool. > > > Some other paths maybe need more care though, eg: > > > > xive_ipi_init() > > __xive_set_irq_config() > > xive_set_irq_targetting() > > xive_eq_for_target() > > > > If xive_eq_for_target() fails to map the target to a valid EQ, > > this ends up being ignored in xive_ipi_init() with this patch. > > Is it okay ? > > This is the P8 emulation layer fully handled by OPAL. It the VP > is bogus, then it means that OPAL just screwed up. > > And this seems to be the case because it's using cpu->pir and not > a vp identifier ... If this is working, it's pure luck. Another > good reason to drop support for this emulation which is a source > of complexity. It's dropped for P10. > > See : > > patchwork.ozlabs.org/patch/1157193 xive/p9: remove XICS emulation > patchwork.ozlabs.org/patch/1157182 xive/p9: remove percpu logging > patchwork.ozlabs.org/patch/1157172 xive/p9: remove XICS emulation OPAL calls > > I would be very happy to push these again and get rid of 1100 > lines of useless bringup complexity. > Hmm... some existing setups might still be using power8 compat mode for legitimate reasons, eg. workload only certified on POWER8. I'm not sure you can get rid of the XICS emulation that easily. > C. > > > > > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >> --- > >> hw/xive.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/hw/xive.c b/hw/xive.c > >> index 7d4e029f19cb..c442ea5e30ed 100644 > >> --- a/hw/xive.c > >> +++ b/hw/xive.c > >> @@ -2152,7 +2152,7 @@ static inline bool xive_eq_for_target(uint32_t target, uint8_t prio, > >> if (eq_blk != vp_blk) { > >> xive_err(x, "eq_blk != vp_blk (%d vs. %d) for target 0x%08x/%d\n", > >> eq_blk, vp_blk, target, prio); > >> - assert(false); > >> + return false; > >> } > >> > >> if (out_eq_blk) > > >
On 11/27/20 10:23 AM, Greg Kurz wrote: > On Fri, 27 Nov 2020 09:32:43 +0100 > Cédric Le Goater <clg@kaod.org> wrote: > >> On 11/27/20 9:08 AM, Greg Kurz wrote: >>> On Fri, 27 Nov 2020 08:32:12 +0100 >>> Cédric Le Goater <clg@kaod.org> wrote: >>> >>>> XIVE VPs are structures describing the vCPUs of guests. When starting >>>> a guest, these are allocated and enabled and some checks are done on >>>> the location of the associated ENDs, which describe the event >>>> queues. If the block of the VP and the block of the ENDs do not match, >>>> the XIVE driver asserts. >>>> >>>> Unfortunately, there is no way to check that a VP identifier is part >>>> of a VP block that was previously allocated and it is relatively easy >>>> to crash the host with a bogus VP id. That can be done with a QEMU >>>> hack on a machine using vsmt. >>>> >>> >>> Reported-by: Greg Kurz <groug@kaod.org> >>> >>> :) >>> >>>> Simply remove the assert, the OS should gracefully handle the error. >>>> >>> >>> This seems to be reasonable when xive_eq_for_target() is called from >>> an OPAL call since they'd all return OPAL_PARAMETER to the OS in this >>> case. >> >> Linux behaves correctly. There are errors but the rollback is correct. >> > > Cool. > >> >>> Some other paths maybe need more care though, eg: >>> >>> xive_ipi_init() >>> __xive_set_irq_config() >>> xive_set_irq_targetting() >>> xive_eq_for_target() >>> >>> If xive_eq_for_target() fails to map the target to a valid EQ, >>> this ends up being ignored in xive_ipi_init() with this patch. >>> Is it okay ? >> >> This is the P8 emulation layer fully handled by OPAL. It the VP >> is bogus, then it means that OPAL just screwed up. >> >> And this seems to be the case because it's using cpu->pir and not >> a vp identifier ... If this is working, it's pure luck. Another >> good reason to drop support for this emulation which is a source >> of complexity. It's dropped for P10. >> >> See : >> >> patchwork.ozlabs.org/patch/1157193 xive/p9: remove XICS emulation >> patchwork.ozlabs.org/patch/1157182 xive/p9: remove percpu logging >> patchwork.ozlabs.org/patch/1157172 xive/p9: remove XICS emulation OPAL calls >> >> I would be very happy to push these again and get rid of 1100 >> lines of useless bringup complexity. >> > > Hmm... some existing setups might still be using power8 compat mode > for legitimate reasons, eg. workload only certified on POWER8. To be clear, I am talking of XICS emulation in OPAL/BML and not in KVM or PowerVM. If I am correct, the certification of an application is tied to an OS which is tied to a set of HW. RH7 is certified to run on POWER8 but not on POWER9. So, if it might work to run a RH7 on a POWER9 but I doubt Redhat would guarantee any POWER8 workload there. C.
On Fri, 27 Nov 2020 10:44:01 +0100 Cédric Le Goater <clg@kaod.org> wrote: [...] > > > > Hmm... some existing setups might still be using power8 compat mode > > for legitimate reasons, eg. workload only certified on POWER8. > > To be clear, I am talking of XICS emulation in OPAL/BML and not in KVM > or PowerVM. > Ah, I got confused, sorry. Thanks for the clarification. > If I am correct, the certification of an application is tied to an OS > which is tied to a set of HW. RH7 is certified to run on POWER8 but > not on POWER9. So, if it might work to run a RH7 on a POWER9 but > I doubt Redhat would guarantee any POWER8 workload there. > RH guarantees that a RHEL7 guest runs in POWER8 compat mode on a POWER9 host AFAIK. > C.
On 11/27/20 12:36 PM, Greg Kurz wrote: > On Fri, 27 Nov 2020 10:44:01 +0100 > Cédric Le Goater <clg@kaod.org> wrote: > > [...] >>> >>> Hmm... some existing setups might still be using power8 compat mode >>> for legitimate reasons, eg. workload only certified on POWER8. >> >> To be clear, I am talking of XICS emulation in OPAL/BML and not in KVM >> or PowerVM. >> > > Ah, I got confused, sorry. Thanks for the clarification. > >> If I am correct, the certification of an application is tied to an OS >> which is tied to a set of HW. RH7 is certified to run on POWER8 but >> not on POWER9. So, if it might work to run a RH7 on a POWER9 but >> I doubt Redhat would guarantee any POWER8 workload there. >> > > RH guarantees that a RHEL7 guest runs in POWER8 compat mode on a > POWER9 host AFAIK. Yes. That's make sense to me. It's the whole purpose of the compat mode for KVM guests or LPARs if PowerVM. On OPAL/BML, we are more in the experimental domain or HW bringup. C.
On 11/27/20 1:02 PM, Cédric Le Goater wrote: > XIVE VPs are structures describing the vCPUs of guests. When starting > a guest, these are allocated and enabled and some checks are done on > the location of the associated ENDs, which describe the event > queues. If the block of the VP and the block of the ENDs do not match, > the XIVE driver asserts. > > Unfortunately, there is no way to check that a VP identifier is part > of a VP block that was previously allocated and it is relatively easy > to crash the host with a bogus VP id. That can be done with a QEMU > hack on a machine using vsmt. > > Simply remove the assert, the OS should gracefully handle the error. Thanks! Merged to master as of f07ea956. Cedric, I see that you have marked this to stable tree. Do we need this in all supported stable branches or only in latest stable i.e. v6.7.x? -Vasant
On 12/16/20 9:03 AM, Vasant Hegde wrote: > On 11/27/20 1:02 PM, Cédric Le Goater wrote: >> XIVE VPs are structures describing the vCPUs of guests. When starting >> a guest, these are allocated and enabled and some checks are done on >> the location of the associated ENDs, which describe the event >> queues. If the block of the VP and the block of the ENDs do not match, >> the XIVE driver asserts. >> >> Unfortunately, there is no way to check that a VP identifier is part >> of a VP block that was previously allocated and it is relatively easy >> to crash the host with a bogus VP id. That can be done with a QEMU >> hack on a machine using vsmt. >> >> Simply remove the assert, the OS should gracefully handle the error. > > Thanks! Merged to master as of f07ea956. > > Cedric, > > I see that you have marked this to stable tree. Do we need this in > all supported stable branches or only in latest stable i.e. v6.7.x? Since KVM XIVE is supported, so that's v6.3+ Thanks, C.
diff --git a/hw/xive.c b/hw/xive.c index 7d4e029f19cb..c442ea5e30ed 100644 --- a/hw/xive.c +++ b/hw/xive.c @@ -2152,7 +2152,7 @@ static inline bool xive_eq_for_target(uint32_t target, uint8_t prio, if (eq_blk != vp_blk) { xive_err(x, "eq_blk != vp_blk (%d vs. %d) for target 0x%08x/%d\n", eq_blk, vp_blk, target, prio); - assert(false); + return false; } if (out_eq_blk)
XIVE VPs are structures describing the vCPUs of guests. When starting a guest, these are allocated and enabled and some checks are done on the location of the associated ENDs, which describe the event queues. If the block of the VP and the block of the ENDs do not match, the XIVE driver asserts. Unfortunately, there is no way to check that a VP identifier is part of a VP block that was previously allocated and it is relatively easy to crash the host with a bogus VP id. That can be done with a QEMU hack on a machine using vsmt. Simply remove the assert, the OS should gracefully handle the error. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/xive.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)