Message ID | 155793986451.464434.12887933000007255549.stgit@bahia.lan |
---|---|
State | New |
Headers | show |
Series | spapr/xive: Sanity checks of OV5 during CAS | expand |
On Wed, May 15, 2019 at 07:04:24PM +0200, Greg Kurz wrote: > If a machine is started with ic-mode=xive but the guest only knows > about XICS, eg. an RHEL 7.6 guest, the kernel panics. This is > expected but a bit unfortunate since the crash doesn't provide > much information for the end user to guess what's happening. > > Detect that during CAS and exit QEMU with a proper error message > instead, like it is already done for the MMU. > > Even if this is less likely to happen, the opposite case of a guest > that only knows about XIVE would certainly fail all the same if the > machine is started with ic-mode=xics. > > Also, the only valid values a guest can pass in byte 23 of OV5 during > CAS are 0b00 (XIVE legacy mode) and 0b01 (XIVE exploitation mode). Any > other value is a bug, at least with the current spec. Again, it does > not seem right to let the guest go on without a precise idea of the > interrupt mode it asked for. > > Handle these cases as well. > > Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com> > Signed-off-by: Greg Kurz <groug@kaod.org> Seems sensible to me, applied. > --- > hw/ppc/spapr_hcall.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 6c16d2b12040..63a55614b83d 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -1513,6 +1513,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > bool guest_radix; > Error *local_err = NULL; > bool raw_mode_supported = false; > + bool guest_xive; > > cas_pvr = cas_check_pvr(spapr, cpu, &addr, &raw_mode_supported, &local_err); > if (local_err) { > @@ -1545,10 +1546,17 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > error_report("guest requested hash and radix MMU, which is invalid."); > exit(EXIT_FAILURE); > } > + if (spapr_ovec_test(ov5_guest, OV5_XIVE_BOTH)) { > + error_report("guest requested an invalid interrupt mode"); > + exit(EXIT_FAILURE); > + } > + > /* The radix/hash bit in byte 24 requires special handling: */ > guest_radix = spapr_ovec_test(ov5_guest, OV5_MMU_RADIX_300); > spapr_ovec_clear(ov5_guest, OV5_MMU_RADIX_300); > > + guest_xive = spapr_ovec_test(ov5_guest, OV5_XIVE_EXPLOIT); > + > /* > * HPT resizing is a bit of a special case, because when enabled > * we assume an HPT guest will support it until it says it > @@ -1632,6 +1640,22 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > ov5_updates) != 0); > } > > + /* > + * Ensure the guest asks for an interrupt mode we support; otherwise > + * terminate the boot. > + */ > + if (guest_xive) { > + if (spapr->irq->ov5 == SPAPR_OV5_XIVE_LEGACY) { > + error_report("Guest requested unavailable interrupt mode (XIVE)"); > + exit(EXIT_FAILURE); > + } > + } else { > + if (spapr->irq->ov5 == SPAPR_OV5_XIVE_EXPLOIT) { > + error_report("Guest requested unavailable interrupt mode (XICS)"); > + exit(EXIT_FAILURE); > + } > + } > + > /* > * Generate a machine reset when we have an update of the > * interrupt mode. Only required when the machine supports both >
On 5/16/19 3:24 AM, David Gibson wrote: > On Wed, May 15, 2019 at 07:04:24PM +0200, Greg Kurz wrote: >> If a machine is started with ic-mode=xive but the guest only knows >> about XICS, eg. an RHEL 7.6 guest, the kernel panics. This is >> expected but a bit unfortunate since the crash doesn't provide >> much information for the end user to guess what's happening. >> >> Detect that during CAS and exit QEMU with a proper error message >> instead, like it is already done for the MMU. >> >> Even if this is less likely to happen, the opposite case of a guest >> that only knows about XIVE would certainly fail all the same if the >> machine is started with ic-mode=xics. >> >> Also, the only valid values a guest can pass in byte 23 of OV5 during >> CAS are 0b00 (XIVE legacy mode) and 0b01 (XIVE exploitation mode). Any >> other value is a bug, at least with the current spec. Again, it does >> not seem right to let the guest go on without a precise idea of the >> interrupt mode it asked for. >> >> Handle these cases as well. >> >> Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com> >> Signed-off-by: Greg Kurz <groug@kaod.org> > > Seems sensible to me, applied. yes. I would have splitted the patch though. But this is minor. Thanks, C. >> --- >> hw/ppc/spapr_hcall.c | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index 6c16d2b12040..63a55614b83d 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -1513,6 +1513,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, >> bool guest_radix; >> Error *local_err = NULL; >> bool raw_mode_supported = false; >> + bool guest_xive; >> >> cas_pvr = cas_check_pvr(spapr, cpu, &addr, &raw_mode_supported, &local_err); >> if (local_err) { >> @@ -1545,10 +1546,17 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, >> error_report("guest requested hash and radix MMU, which is invalid."); >> exit(EXIT_FAILURE); >> } >> + if (spapr_ovec_test(ov5_guest, OV5_XIVE_BOTH)) { >> + error_report("guest requested an invalid interrupt mode"); >> + exit(EXIT_FAILURE); >> + } >> + >> /* The radix/hash bit in byte 24 requires special handling: */ >> guest_radix = spapr_ovec_test(ov5_guest, OV5_MMU_RADIX_300); >> spapr_ovec_clear(ov5_guest, OV5_MMU_RADIX_300); >> >> + guest_xive = spapr_ovec_test(ov5_guest, OV5_XIVE_EXPLOIT); >> + >> /* >> * HPT resizing is a bit of a special case, because when enabled >> * we assume an HPT guest will support it until it says it >> @@ -1632,6 +1640,22 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, >> ov5_updates) != 0); >> } >> >> + /* >> + * Ensure the guest asks for an interrupt mode we support; otherwise >> + * terminate the boot. >> + */ >> + if (guest_xive) { >> + if (spapr->irq->ov5 == SPAPR_OV5_XIVE_LEGACY) { >> + error_report("Guest requested unavailable interrupt mode (XIVE)"); >> + exit(EXIT_FAILURE); >> + } >> + } else { >> + if (spapr->irq->ov5 == SPAPR_OV5_XIVE_EXPLOIT) { >> + error_report("Guest requested unavailable interrupt mode (XICS)"); >> + exit(EXIT_FAILURE); >> + } >> + } >> + >> /* >> * Generate a machine reset when we have an update of the >> * interrupt mode. Only required when the machine supports both >> >
On Wed, May 15, 2019 at 07:04:24PM +0200, Greg Kurz wrote: > If a machine is started with ic-mode=xive but the guest only knows > about XICS, eg. an RHEL 7.6 guest, the kernel panics. This is > expected but a bit unfortunate since the crash doesn't provide > much information for the end user to guess what's happening. > > Detect that during CAS and exit QEMU with a proper error message > instead, like it is already done for the MMU. > > Even if this is less likely to happen, the opposite case of a guest > that only knows about XIVE would certainly fail all the same if the > machine is started with ic-mode=xics. > > Also, the only valid values a guest can pass in byte 23 of OV5 during > CAS are 0b00 (XIVE legacy mode) and 0b01 (XIVE exploitation mode). Any > other value is a bug, at least with the current spec. Again, it does > not seem right to let the guest go on without a precise idea of the > interrupt mode it asked for. > > Handle these cases as well. > > Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com> > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/spapr_hcall.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 6c16d2b12040..63a55614b83d 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -1513,6 +1513,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > bool guest_radix; > Error *local_err = NULL; > bool raw_mode_supported = false; > + bool guest_xive; > > cas_pvr = cas_check_pvr(spapr, cpu, &addr, &raw_mode_supported, &local_err); > if (local_err) { > @@ -1545,10 +1546,17 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > error_report("guest requested hash and radix MMU, which is invalid."); > exit(EXIT_FAILURE); > } > + if (spapr_ovec_test(ov5_guest, OV5_XIVE_BOTH)) { > + error_report("guest requested an invalid interrupt mode"); > + exit(EXIT_FAILURE); > + } > + > /* The radix/hash bit in byte 24 requires special handling: */ > guest_radix = spapr_ovec_test(ov5_guest, OV5_MMU_RADIX_300); > spapr_ovec_clear(ov5_guest, OV5_MMU_RADIX_300); > > + guest_xive = spapr_ovec_test(ov5_guest, OV5_XIVE_EXPLOIT); > + > /* > * HPT resizing is a bit of a special case, because when enabled > * we assume an HPT guest will support it until it says it > @@ -1632,6 +1640,22 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > ov5_updates) != 0); > } > > + /* > + * Ensure the guest asks for an interrupt mode we support; otherwise > + * terminate the boot. > + */ > + if (guest_xive) { > + if (spapr->irq->ov5 == SPAPR_OV5_XIVE_LEGACY) { > + error_report("Guest requested unavailable interrupt mode (XIVE)"); > + exit(EXIT_FAILURE); > + } > + } else { > + if (spapr->irq->ov5 == SPAPR_OV5_XIVE_EXPLOIT) { > + error_report("Guest requested unavailable interrupt mode (XICS)"); Looks like there is a typo in the error msg reported, instead it should be something like below "Guest requested unavailable interrupt mode(XIVE), please use supported interrupt mode(ic-type=xics)" Same for the previous check aswell. Coz, I booted 3.10(kernel) guest with ic-type=xive and got a below error, which seems wrong 2019-05-16T06:24:58.713261Z qemu-system-ppc64: Guest requested unavailable interrupt mode (XICS) Regards, -Satheesh. > + exit(EXIT_FAILURE); > + } > + } > + > /* > * Generate a machine reset when we have an update of the > * interrupt mode. Only required when the machine supports both >
On Thu, 16 May 2019 12:09:57 +0530 Satheesh Rajendran <sathnaga@linux.vnet.ibm.com> wrote: > On Wed, May 15, 2019 at 07:04:24PM +0200, Greg Kurz wrote: > > If a machine is started with ic-mode=xive but the guest only knows > > about XICS, eg. an RHEL 7.6 guest, the kernel panics. This is > > expected but a bit unfortunate since the crash doesn't provide > > much information for the end user to guess what's happening. > > > > Detect that during CAS and exit QEMU with a proper error message > > instead, like it is already done for the MMU. > > > > Even if this is less likely to happen, the opposite case of a guest > > that only knows about XIVE would certainly fail all the same if the > > machine is started with ic-mode=xics. > > > > Also, the only valid values a guest can pass in byte 23 of OV5 during > > CAS are 0b00 (XIVE legacy mode) and 0b01 (XIVE exploitation mode). Any > > other value is a bug, at least with the current spec. Again, it does > > not seem right to let the guest go on without a precise idea of the > > interrupt mode it asked for. > > > > Handle these cases as well. > > > > Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com> > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/ppc/spapr_hcall.c | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > > index 6c16d2b12040..63a55614b83d 100644 > > --- a/hw/ppc/spapr_hcall.c > > +++ b/hw/ppc/spapr_hcall.c > > @@ -1513,6 +1513,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > > bool guest_radix; > > Error *local_err = NULL; > > bool raw_mode_supported = false; > > + bool guest_xive; > > > > cas_pvr = cas_check_pvr(spapr, cpu, &addr, &raw_mode_supported, &local_err); > > if (local_err) { > > @@ -1545,10 +1546,17 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > > error_report("guest requested hash and radix MMU, which is invalid."); > > exit(EXIT_FAILURE); > > } > > + if (spapr_ovec_test(ov5_guest, OV5_XIVE_BOTH)) { > > + error_report("guest requested an invalid interrupt mode"); > > + exit(EXIT_FAILURE); > > + } > > + > > /* The radix/hash bit in byte 24 requires special handling: */ > > guest_radix = spapr_ovec_test(ov5_guest, OV5_MMU_RADIX_300); > > spapr_ovec_clear(ov5_guest, OV5_MMU_RADIX_300); > > > > + guest_xive = spapr_ovec_test(ov5_guest, OV5_XIVE_EXPLOIT); > > + > > /* > > * HPT resizing is a bit of a special case, because when enabled > > * we assume an HPT guest will support it until it says it > > @@ -1632,6 +1640,22 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > > ov5_updates) != 0); > > } > > > > + /* > > + * Ensure the guest asks for an interrupt mode we support; otherwise > > + * terminate the boot. > > + */ > > + if (guest_xive) { > > + if (spapr->irq->ov5 == SPAPR_OV5_XIVE_LEGACY) { > > + error_report("Guest requested unavailable interrupt mode (XIVE)"); > > + exit(EXIT_FAILURE); > > + } > > + } else { > > + if (spapr->irq->ov5 == SPAPR_OV5_XIVE_EXPLOIT) { > > + error_report("Guest requested unavailable interrupt mode (XICS)"); > > Looks like there is a typo in the error msg reported, instead it should be something like below > > "Guest requested unavailable interrupt mode(XIVE), please use supported interrupt mode(ic-type=xics)" > Hmm... Agreed that it may be worth adding some hint for the user to proceed but... > Same for the previous check aswell. > > Coz, I booted 3.10(kernel) guest with ic-type=xive and got a below error, which seems wrong > > 2019-05-16T06:24:58.713261Z qemu-system-ppc64: Guest requested unavailable interrupt mode (XICS) > ... this message is ok: the 3.10 kernel based guest _does_ ask for XICS, which isn't available because of ic-mode=xive. Cheers, -- Greg > Regards, > -Satheesh. > > > + exit(EXIT_FAILURE); > > + } > > + } > > + > > /* > > * Generate a machine reset when we have an update of the > > * interrupt mode. Only required when the machine supports both > > >
On Thu, May 16, 2019 at 08:58:14AM +0200, Greg Kurz wrote: > On Thu, 16 May 2019 12:09:57 +0530 > Satheesh Rajendran <sathnaga@linux.vnet.ibm.com> wrote: > > > On Wed, May 15, 2019 at 07:04:24PM +0200, Greg Kurz wrote: > > > If a machine is started with ic-mode=xive but the guest only knows > > > about XICS, eg. an RHEL 7.6 guest, the kernel panics. This is > > > expected but a bit unfortunate since the crash doesn't provide > > > much information for the end user to guess what's happening. > > > > > > Detect that during CAS and exit QEMU with a proper error message > > > instead, like it is already done for the MMU. > > > > > > Even if this is less likely to happen, the opposite case of a guest > > > that only knows about XIVE would certainly fail all the same if the > > > machine is started with ic-mode=xics. > > > > > > Also, the only valid values a guest can pass in byte 23 of OV5 during > > > CAS are 0b00 (XIVE legacy mode) and 0b01 (XIVE exploitation mode). Any > > > other value is a bug, at least with the current spec. Again, it does > > > not seem right to let the guest go on without a precise idea of the > > > interrupt mode it asked for. > > > > > > Handle these cases as well. > > > > > > Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com> > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > hw/ppc/spapr_hcall.c | 24 ++++++++++++++++++++++++ > > > 1 file changed, 24 insertions(+) > > > > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > > > index 6c16d2b12040..63a55614b83d 100644 > > > --- a/hw/ppc/spapr_hcall.c > > > +++ b/hw/ppc/spapr_hcall.c > > > @@ -1513,6 +1513,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > > > bool guest_radix; > > > Error *local_err = NULL; > > > bool raw_mode_supported = false; > > > + bool guest_xive; > > > > > > cas_pvr = cas_check_pvr(spapr, cpu, &addr, &raw_mode_supported, &local_err); > > > if (local_err) { > > > @@ -1545,10 +1546,17 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > > > error_report("guest requested hash and radix MMU, which is invalid."); > > > exit(EXIT_FAILURE); > > > } > > > + if (spapr_ovec_test(ov5_guest, OV5_XIVE_BOTH)) { > > > + error_report("guest requested an invalid interrupt mode"); > > > + exit(EXIT_FAILURE); > > > + } > > > + > > > /* The radix/hash bit in byte 24 requires special handling: */ > > > guest_radix = spapr_ovec_test(ov5_guest, OV5_MMU_RADIX_300); > > > spapr_ovec_clear(ov5_guest, OV5_MMU_RADIX_300); > > > > > > + guest_xive = spapr_ovec_test(ov5_guest, OV5_XIVE_EXPLOIT); > > > + > > > /* > > > * HPT resizing is a bit of a special case, because when enabled > > > * we assume an HPT guest will support it until it says it > > > @@ -1632,6 +1640,22 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > > > ov5_updates) != 0); > > > } > > > > > > + /* > > > + * Ensure the guest asks for an interrupt mode we support; otherwise > > > + * terminate the boot. > > > + */ > > > + if (guest_xive) { > > > + if (spapr->irq->ov5 == SPAPR_OV5_XIVE_LEGACY) { > > > + error_report("Guest requested unavailable interrupt mode (XIVE)"); > > > + exit(EXIT_FAILURE); > > > + } > > > + } else { > > > + if (spapr->irq->ov5 == SPAPR_OV5_XIVE_EXPLOIT) { > > > + error_report("Guest requested unavailable interrupt mode (XICS)"); > > > > Looks like there is a typo in the error msg reported, instead it should be something like below > > > > "Guest requested unavailable interrupt mode(XIVE), please use supported interrupt mode(ic-type=xics)" > > > > Hmm... Agreed that it may be worth adding some hint for the user to proceed but... > > > Same for the previous check aswell. > > > > Coz, I booted 3.10(kernel) guest with ic-type=xive and got a below error, which seems wrong > > > > 2019-05-16T06:24:58.713261Z qemu-system-ppc64: Guest requested unavailable interrupt mode (XICS) > > > > ... this message is ok: the 3.10 kernel based guest _does_ ask for XICS, > which isn't available because of ic-mode=xive. I won't be sending a pull request for a little while yet, so if you want to send a followup improving the messages I can fold it into the original patch in my tree.
On Thu, 16 May 2019 21:46:01 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: [...] > > I won't be sending a pull request for a little while yet, so if you > want to send a followup improving the messages I can fold it into the > original patch in my tree. > Done. https://patchwork.ozlabs.org/patch/1100396/
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 6c16d2b12040..63a55614b83d 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -1513,6 +1513,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, bool guest_radix; Error *local_err = NULL; bool raw_mode_supported = false; + bool guest_xive; cas_pvr = cas_check_pvr(spapr, cpu, &addr, &raw_mode_supported, &local_err); if (local_err) { @@ -1545,10 +1546,17 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, error_report("guest requested hash and radix MMU, which is invalid."); exit(EXIT_FAILURE); } + if (spapr_ovec_test(ov5_guest, OV5_XIVE_BOTH)) { + error_report("guest requested an invalid interrupt mode"); + exit(EXIT_FAILURE); + } + /* The radix/hash bit in byte 24 requires special handling: */ guest_radix = spapr_ovec_test(ov5_guest, OV5_MMU_RADIX_300); spapr_ovec_clear(ov5_guest, OV5_MMU_RADIX_300); + guest_xive = spapr_ovec_test(ov5_guest, OV5_XIVE_EXPLOIT); + /* * HPT resizing is a bit of a special case, because when enabled * we assume an HPT guest will support it until it says it @@ -1632,6 +1640,22 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, ov5_updates) != 0); } + /* + * Ensure the guest asks for an interrupt mode we support; otherwise + * terminate the boot. + */ + if (guest_xive) { + if (spapr->irq->ov5 == SPAPR_OV5_XIVE_LEGACY) { + error_report("Guest requested unavailable interrupt mode (XIVE)"); + exit(EXIT_FAILURE); + } + } else { + if (spapr->irq->ov5 == SPAPR_OV5_XIVE_EXPLOIT) { + error_report("Guest requested unavailable interrupt mode (XICS)"); + exit(EXIT_FAILURE); + } + } + /* * Generate a machine reset when we have an update of the * interrupt mode. Only required when the machine supports both
If a machine is started with ic-mode=xive but the guest only knows about XICS, eg. an RHEL 7.6 guest, the kernel panics. This is expected but a bit unfortunate since the crash doesn't provide much information for the end user to guess what's happening. Detect that during CAS and exit QEMU with a proper error message instead, like it is already done for the MMU. Even if this is less likely to happen, the opposite case of a guest that only knows about XIVE would certainly fail all the same if the machine is started with ic-mode=xics. Also, the only valid values a guest can pass in byte 23 of OV5 during CAS are 0b00 (XIVE legacy mode) and 0b01 (XIVE exploitation mode). Any other value is a bug, at least with the current spec. Again, it does not seem right to let the guest go on without a precise idea of the interrupt mode it asked for. Handle these cases as well. Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com> Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr_hcall.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)