| Message ID | 20220630194249.886747-3-danielhb413@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | cleanup error handling in kvmppc_read_int_cpu_dt() | expand |
On 6/30/22 21:42, Daniel Henrique Barboza wrote: > The function can't just return 0 whether an error happened and call it a > day. We must provide a way of letting callers know if the zero return is > legitimate or due to an error. > > Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled > with an appropriate error, if one occurs. Callers are then free to pass > an Error pointer and handle it. > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > target/ppc/kvm.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index 109823136d..bc17437097 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) > > /* > * Read a CPU node property from the host device tree that's a single > - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong > - * (can't find or open the property, or doesn't understand the format) > + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes > + * wrong (can't find or open the property, or doesn't understand the > + * format) > */ > -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) > +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) > { > char buf[PATH_MAX], *tmp; > uint64_t val; > > if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { > + error_setg(errp, "Failed to read CPU property %s", propname); > return 0; > } > > @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) > > uint64_t kvmppc_get_clockfreq(void) > { > - return kvmppc_read_int_cpu_dt("clock-frequency"); > + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); This should be fatal. no ? C. > } > > static int kvmppc_get_dec_bits(void) > { > - int nr_bits = kvmppc_read_int_cpu_dt("ibm,dec-bits"); > + int nr_bits = kvmppc_read_int_cpu_dt("ibm,dec-bits", NULL); > > if (nr_bits > 0) { > return nr_bits; > @@ -2336,8 +2338,8 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on) > static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data) > { > PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); > - uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size"); > - uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size"); > + uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size", NULL); > + uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size", NULL); > > /* Now fix up the class with information we can query from the host */ > pcc->pvr = mfpvr();
On 7/2/22 03:24, Cédric Le Goater wrote: > On 6/30/22 21:42, Daniel Henrique Barboza wrote: >> The function can't just return 0 whether an error happened and call it a >> day. We must provide a way of letting callers know if the zero return is >> legitimate or due to an error. >> >> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled >> with an appropriate error, if one occurs. Callers are then free to pass >> an Error pointer and handle it. >> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> target/ppc/kvm.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >> index 109823136d..bc17437097 100644 >> --- a/target/ppc/kvm.c >> +++ b/target/ppc/kvm.c >> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) >> /* >> * Read a CPU node property from the host device tree that's a single >> - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong >> - * (can't find or open the property, or doesn't understand the format) >> + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes >> + * wrong (can't find or open the property, or doesn't understand the >> + * format) >> */ >> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) >> { >> char buf[PATH_MAX], *tmp; >> uint64_t val; >> if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { >> + error_setg(errp, "Failed to read CPU property %s", propname); >> return 0; >> } >> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >> uint64_t kvmppc_get_clockfreq(void) >> { >> - return kvmppc_read_int_cpu_dt("clock-frequency"); >> + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); > > > This should be fatal. no ? I'm not sure. I went under the assumption that there might be some weird condition where 'clock-frequency' might be missing from the DT, and this is why we're not exiting out immediately. That said, the advantage of turning this into fatal is that we won't need all the other patches that handles error on this function. We're going to assume that if 'clock-frequency' is missing then we can't boot. I'm okay with that. Thanks, Daniel > > C. > > >> } >> static int kvmppc_get_dec_bits(void) >> { >> - int nr_bits = kvmppc_read_int_cpu_dt("ibm,dec-bits"); >> + int nr_bits = kvmppc_read_int_cpu_dt("ibm,dec-bits", NULL); >> if (nr_bits > 0) { >> return nr_bits; >> @@ -2336,8 +2338,8 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on) >> static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data) >> { >> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); >> - uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size"); >> - uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size"); >> + uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size", NULL); >> + uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size", NULL); >> /* Now fix up the class with information we can query from the host */ >> pcc->pvr = mfpvr(); >
On 7/2/22 15:34, Daniel Henrique Barboza wrote: > > > On 7/2/22 03:24, Cédric Le Goater wrote: >> On 6/30/22 21:42, Daniel Henrique Barboza wrote: >>> The function can't just return 0 whether an error happened and call it a >>> day. We must provide a way of letting callers know if the zero return is >>> legitimate or due to an error. >>> >>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled >>> with an appropriate error, if one occurs. Callers are then free to pass >>> an Error pointer and handle it. >>> >>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>> --- >>> target/ppc/kvm.c | 16 +++++++++------- >>> 1 file changed, 9 insertions(+), 7 deletions(-) >>> >>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>> index 109823136d..bc17437097 100644 >>> --- a/target/ppc/kvm.c >>> +++ b/target/ppc/kvm.c >>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) >>> /* >>> * Read a CPU node property from the host device tree that's a single >>> - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong >>> - * (can't find or open the property, or doesn't understand the format) >>> + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes >>> + * wrong (can't find or open the property, or doesn't understand the >>> + * format) >>> */ >>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) >>> { >>> char buf[PATH_MAX], *tmp; >>> uint64_t val; >>> if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { >>> + error_setg(errp, "Failed to read CPU property %s", propname); >>> return 0; >>> } >>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>> uint64_t kvmppc_get_clockfreq(void) >>> { >>> - return kvmppc_read_int_cpu_dt("clock-frequency"); >>> + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); >> >> >> This should be fatal. no ? > > > I'm not sure. I went under the assumption that there might be some weird > condition where 'clock-frequency' might be missing from the DT, and this > is why we're not exiting out immediately. > > That said, the advantage of turning this into fatal is that we won't need > all the other patches that handles error on this function. We're going to > assume that if 'clock-frequency' is missing then we can't boot. I'm okay > with that. I think so. Some machines behave badly when 'clock-frequency' is bogus, division by zero, no console, etc. We could check easily with pseries which is the only KVM PPC platform. C.
On 7/4/22 14:34, Cédric Le Goater wrote: > On 7/2/22 15:34, Daniel Henrique Barboza wrote: >> >> >> On 7/2/22 03:24, Cédric Le Goater wrote: >>> On 6/30/22 21:42, Daniel Henrique Barboza wrote: >>>> The function can't just return 0 whether an error happened and call it a >>>> day. We must provide a way of letting callers know if the zero return is >>>> legitimate or due to an error. >>>> >>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled >>>> with an appropriate error, if one occurs. Callers are then free to pass >>>> an Error pointer and handle it. >>>> >>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>> --- >>>> target/ppc/kvm.c | 16 +++++++++------- >>>> 1 file changed, 9 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>>> index 109823136d..bc17437097 100644 >>>> --- a/target/ppc/kvm.c >>>> +++ b/target/ppc/kvm.c >>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) >>>> /* >>>> * Read a CPU node property from the host device tree that's a single >>>> - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong >>>> - * (can't find or open the property, or doesn't understand the format) >>>> + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes >>>> + * wrong (can't find or open the property, or doesn't understand the >>>> + * format) >>>> */ >>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) >>>> { >>>> char buf[PATH_MAX], *tmp; >>>> uint64_t val; >>>> if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { >>>> + error_setg(errp, "Failed to read CPU property %s", propname); >>>> return 0; >>>> } >>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>> uint64_t kvmppc_get_clockfreq(void) >>>> { >>>> - return kvmppc_read_int_cpu_dt("clock-frequency"); >>>> + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); >>> >>> >>> This should be fatal. no ? >> >> >> I'm not sure. I went under the assumption that there might be some weird >> condition where 'clock-frequency' might be missing from the DT, and this >> is why we're not exiting out immediately. >> >> That said, the advantage of turning this into fatal is that we won't need >> all the other patches that handles error on this function. We're going to >> assume that if 'clock-frequency' is missing then we can't boot. I'm okay >> with that. > > I think so. Some machines behave badly when 'clock-frequency' is bogus, > division by zero, no console, etc. We could check easily with pseries > which is the only KVM PPC platform. I can assert that with pSeries we can safely error_fatal if the DT doesn't contain 'clock-frequency' since it's on PAPR under this section: "C.6.2 OF Root Note This section defines additional properties and methods associated with PAPR platforms that OSs expect to find in the root node." I interpret this as "if there's no clock-frequency just bail out". Thanks, Daniel > > C. >
On 04/07/2022 18:34, Cédric Le Goater wrote: > On 7/2/22 15:34, Daniel Henrique Barboza wrote: >> >> >> On 7/2/22 03:24, Cédric Le Goater wrote: >>> On 6/30/22 21:42, Daniel Henrique Barboza wrote: >>>> The function can't just return 0 whether an error happened and call it a >>>> day. We must provide a way of letting callers know if the zero return is >>>> legitimate or due to an error. >>>> >>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled >>>> with an appropriate error, if one occurs. Callers are then free to pass >>>> an Error pointer and handle it. >>>> >>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>> --- >>>> target/ppc/kvm.c | 16 +++++++++------- >>>> 1 file changed, 9 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>>> index 109823136d..bc17437097 100644 >>>> --- a/target/ppc/kvm.c >>>> +++ b/target/ppc/kvm.c >>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) >>>> /* >>>> * Read a CPU node property from the host device tree that's a single >>>> - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong >>>> - * (can't find or open the property, or doesn't understand the format) >>>> + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes >>>> + * wrong (can't find or open the property, or doesn't understand the >>>> + * format) >>>> */ >>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) >>>> { >>>> char buf[PATH_MAX], *tmp; >>>> uint64_t val; >>>> if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { >>>> + error_setg(errp, "Failed to read CPU property %s", propname); >>>> return 0; >>>> } >>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>> uint64_t kvmppc_get_clockfreq(void) >>>> { >>>> - return kvmppc_read_int_cpu_dt("clock-frequency"); >>>> + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); >>> >>> >>> This should be fatal. no ? >> >> >> I'm not sure. I went under the assumption that there might be some weird >> condition where 'clock-frequency' might be missing from the DT, and this >> is why we're not exiting out immediately. >> >> That said, the advantage of turning this into fatal is that we won't need >> all the other patches that handles error on this function. We're going to >> assume that if 'clock-frequency' is missing then we can't boot. I'm okay >> with that. > > I think so. Some machines behave badly when 'clock-frequency' is bogus, > division by zero, no console, etc. We could check easily with pseries > which is the only KVM PPC platform. Well not quite true ;) I haven't tested it during the last release cycle, but the Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini last time I checked. ATB, Mark.
On 7/5/22 08:51, Mark Cave-Ayland wrote: > On 04/07/2022 18:34, Cédric Le Goater wrote: > >> On 7/2/22 15:34, Daniel Henrique Barboza wrote: >>> >>> >>> On 7/2/22 03:24, Cédric Le Goater wrote: >>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote: >>>>> The function can't just return 0 whether an error happened and call it a >>>>> day. We must provide a way of letting callers know if the zero return is >>>>> legitimate or due to an error. >>>>> >>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled >>>>> with an appropriate error, if one occurs. Callers are then free to pass >>>>> an Error pointer and handle it. >>>>> >>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>>> --- >>>>> target/ppc/kvm.c | 16 +++++++++------- >>>>> 1 file changed, 9 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>>>> index 109823136d..bc17437097 100644 >>>>> --- a/target/ppc/kvm.c >>>>> +++ b/target/ppc/kvm.c >>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) >>>>> /* >>>>> * Read a CPU node property from the host device tree that's a single >>>>> - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong >>>>> - * (can't find or open the property, or doesn't understand the format) >>>>> + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes >>>>> + * wrong (can't find or open the property, or doesn't understand the >>>>> + * format) >>>>> */ >>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) >>>>> { >>>>> char buf[PATH_MAX], *tmp; >>>>> uint64_t val; >>>>> if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { >>>>> + error_setg(errp, "Failed to read CPU property %s", propname); >>>>> return 0; >>>>> } >>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>> uint64_t kvmppc_get_clockfreq(void) >>>>> { >>>>> - return kvmppc_read_int_cpu_dt("clock-frequency"); >>>>> + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); >>>> >>>> >>>> This should be fatal. no ? >>> >>> >>> I'm not sure. I went under the assumption that there might be some weird >>> condition where 'clock-frequency' might be missing from the DT, and this >>> is why we're not exiting out immediately. >>> >>> That said, the advantage of turning this into fatal is that we won't need >>> all the other patches that handles error on this function. We're going to >>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay >>> with that. >> >> I think so. Some machines behave badly when 'clock-frequency' is bogus, >> division by zero, no console, etc. We could check easily with pseries >> which is the only KVM PPC platform. > > Well not quite true ;) I haven't tested it during the last release cycle, > but the Mac machines were still working fine to boot OS X with KVM-PR on > my G4 Mac Mini last time I checked. Oh. Sorry. and I still have access to a real G5 running the latest debian. I should give it a try one day. Thanks C.
On 7/5/22 03:51, Mark Cave-Ayland wrote: > On 04/07/2022 18:34, Cédric Le Goater wrote: > >> On 7/2/22 15:34, Daniel Henrique Barboza wrote: >>> >>> >>> On 7/2/22 03:24, Cédric Le Goater wrote: >>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote: >>>>> The function can't just return 0 whether an error happened and call it a >>>>> day. We must provide a way of letting callers know if the zero return is >>>>> legitimate or due to an error. >>>>> >>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled >>>>> with an appropriate error, if one occurs. Callers are then free to pass >>>>> an Error pointer and handle it. >>>>> >>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>>> --- >>>>> target/ppc/kvm.c | 16 +++++++++------- >>>>> 1 file changed, 9 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>>>> index 109823136d..bc17437097 100644 >>>>> --- a/target/ppc/kvm.c >>>>> +++ b/target/ppc/kvm.c >>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) >>>>> /* >>>>> * Read a CPU node property from the host device tree that's a single >>>>> - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong >>>>> - * (can't find or open the property, or doesn't understand the format) >>>>> + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes >>>>> + * wrong (can't find or open the property, or doesn't understand the >>>>> + * format) >>>>> */ >>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) >>>>> { >>>>> char buf[PATH_MAX], *tmp; >>>>> uint64_t val; >>>>> if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { >>>>> + error_setg(errp, "Failed to read CPU property %s", propname); >>>>> return 0; >>>>> } >>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>> uint64_t kvmppc_get_clockfreq(void) >>>>> { >>>>> - return kvmppc_read_int_cpu_dt("clock-frequency"); >>>>> + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); >>>> >>>> >>>> This should be fatal. no ? >>> >>> >>> I'm not sure. I went under the assumption that there might be some weird >>> condition where 'clock-frequency' might be missing from the DT, and this >>> is why we're not exiting out immediately. >>> >>> That said, the advantage of turning this into fatal is that we won't need >>> all the other patches that handles error on this function. We're going to >>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay >>> with that. >> >> I think so. Some machines behave badly when 'clock-frequency' is bogus, >> division by zero, no console, etc. We could check easily with pseries >> which is the only KVM PPC platform. > > Well not quite true ;) I haven't tested it during the last release cycle, but the Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini last time I checked. We can't just error_fatal by default then. Each machine should be able to determine how to handle this missing DT element. If I want to error_fatal for pseries then I can do so in patch 9/9, but other than that I'll keep the existing behavior. Thanks, Daniel > > > ATB, > > Mark.
On 05/07/2022 10:39, Daniel Henrique Barboza wrote: > On 7/5/22 03:51, Mark Cave-Ayland wrote: >> On 04/07/2022 18:34, Cédric Le Goater wrote: >> >>> On 7/2/22 15:34, Daniel Henrique Barboza wrote: >>>> >>>> >>>> On 7/2/22 03:24, Cédric Le Goater wrote: >>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote: >>>>>> The function can't just return 0 whether an error happened and call it a >>>>>> day. We must provide a way of letting callers know if the zero return is >>>>>> legitimate or due to an error. >>>>>> >>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled >>>>>> with an appropriate error, if one occurs. Callers are then free to pass >>>>>> an Error pointer and handle it. >>>>>> >>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>>>> --- >>>>>> target/ppc/kvm.c | 16 +++++++++------- >>>>>> 1 file changed, 9 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>>>>> index 109823136d..bc17437097 100644 >>>>>> --- a/target/ppc/kvm.c >>>>>> +++ b/target/ppc/kvm.c >>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) >>>>>> /* >>>>>> * Read a CPU node property from the host device tree that's a single >>>>>> - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong >>>>>> - * (can't find or open the property, or doesn't understand the format) >>>>>> + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes >>>>>> + * wrong (can't find or open the property, or doesn't understand the >>>>>> + * format) >>>>>> */ >>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) >>>>>> { >>>>>> char buf[PATH_MAX], *tmp; >>>>>> uint64_t val; >>>>>> if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { >>>>>> + error_setg(errp, "Failed to read CPU property %s", propname); >>>>>> return 0; >>>>>> } >>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char >>>>>> *propname) >>>>>> uint64_t kvmppc_get_clockfreq(void) >>>>>> { >>>>>> - return kvmppc_read_int_cpu_dt("clock-frequency"); >>>>>> + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); >>>>> >>>>> >>>>> This should be fatal. no ? >>>> >>>> >>>> I'm not sure. I went under the assumption that there might be some weird >>>> condition where 'clock-frequency' might be missing from the DT, and this >>>> is why we're not exiting out immediately. >>>> >>>> That said, the advantage of turning this into fatal is that we won't need >>>> all the other patches that handles error on this function. We're going to >>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay >>>> with that. >>> >>> I think so. Some machines behave badly when 'clock-frequency' is bogus, >>> division by zero, no console, etc. We could check easily with pseries >>> which is the only KVM PPC platform. >> >> Well not quite true ;) I haven't tested it during the last release cycle, but the >> Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini >> last time I checked. > > > We can't just error_fatal by default then. > > Each machine should be able to determine how to handle this missing DT > element. If I want to error_fatal for pseries then I can do so in patch > 9/9, but other than that I'll keep the existing behavior. This wouldn't be a problem for the Mac machines since they pass the clock frequency from QEMU to OpenBIOS using the fw_cfg interface which builds the tree itself rather than using FDT. So I think using error_fatal should still be fine. ATB, Mark.
On 7/5/22 11:39, Daniel Henrique Barboza wrote: > > > On 7/5/22 03:51, Mark Cave-Ayland wrote: >> On 04/07/2022 18:34, Cédric Le Goater wrote: >> >>> On 7/2/22 15:34, Daniel Henrique Barboza wrote: >>>> >>>> >>>> On 7/2/22 03:24, Cédric Le Goater wrote: >>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote: >>>>>> The function can't just return 0 whether an error happened and call it a >>>>>> day. We must provide a way of letting callers know if the zero return is >>>>>> legitimate or due to an error. >>>>>> >>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled >>>>>> with an appropriate error, if one occurs. Callers are then free to pass >>>>>> an Error pointer and handle it. >>>>>> >>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>>>> --- >>>>>> target/ppc/kvm.c | 16 +++++++++------- >>>>>> 1 file changed, 9 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>>>>> index 109823136d..bc17437097 100644 >>>>>> --- a/target/ppc/kvm.c >>>>>> +++ b/target/ppc/kvm.c >>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) >>>>>> /* >>>>>> * Read a CPU node property from the host device tree that's a single >>>>>> - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong >>>>>> - * (can't find or open the property, or doesn't understand the format) >>>>>> + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes >>>>>> + * wrong (can't find or open the property, or doesn't understand the >>>>>> + * format) >>>>>> */ >>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) >>>>>> { >>>>>> char buf[PATH_MAX], *tmp; >>>>>> uint64_t val; >>>>>> if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { >>>>>> + error_setg(errp, "Failed to read CPU property %s", propname); >>>>>> return 0; >>>>>> } >>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>>> uint64_t kvmppc_get_clockfreq(void) >>>>>> { >>>>>> - return kvmppc_read_int_cpu_dt("clock-frequency"); >>>>>> + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); >>>>> >>>>> >>>>> This should be fatal. no ? >>>> >>>> >>>> I'm not sure. I went under the assumption that there might be some weird >>>> condition where 'clock-frequency' might be missing from the DT, and this >>>> is why we're not exiting out immediately. >>>> >>>> That said, the advantage of turning this into fatal is that we won't need >>>> all the other patches that handles error on this function. We're going to >>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay >>>> with that. >>> >>> I think so. Some machines behave badly when 'clock-frequency' is bogus, >>> division by zero, no console, etc. We could check easily with pseries >>> which is the only KVM PPC platform. >> >> Well not quite true ;) I haven't tested it during the last release cycle, but the Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini last time I checked. > > > We can't just error_fatal by default then. > > Each machine should be able to determine how to handle this missing DT > element. If I want to error_fatal for pseries then I can do so in patch > 9/9, but other than that I'll keep the existing behavior. Or add an errp here : hw/ppc/e500.c: clock_freq = kvmppc_get_clockfreq(); hw/ppc/sam460ex.c: clock_freq = kvmppc_get_clockfreq(); hw/ppc/ppc440_bamboo.c: clock_freq = kvmppc_get_clockfreq(); hw/ppc/spapr.c: uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000; C.
On 7/5/22 06:44, Mark Cave-Ayland wrote: > On 05/07/2022 10:39, Daniel Henrique Barboza wrote: > >> On 7/5/22 03:51, Mark Cave-Ayland wrote: >>> On 04/07/2022 18:34, Cédric Le Goater wrote: >>> >>>> On 7/2/22 15:34, Daniel Henrique Barboza wrote: >>>>> >>>>> >>>>> On 7/2/22 03:24, Cédric Le Goater wrote: >>>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote: >>>>>>> The function can't just return 0 whether an error happened and call it a >>>>>>> day. We must provide a way of letting callers know if the zero return is >>>>>>> legitimate or due to an error. >>>>>>> >>>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled >>>>>>> with an appropriate error, if one occurs. Callers are then free to pass >>>>>>> an Error pointer and handle it. >>>>>>> >>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>>>>> --- >>>>>>> target/ppc/kvm.c | 16 +++++++++------- >>>>>>> 1 file changed, 9 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>>>>>> index 109823136d..bc17437097 100644 >>>>>>> --- a/target/ppc/kvm.c >>>>>>> +++ b/target/ppc/kvm.c >>>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) >>>>>>> /* >>>>>>> * Read a CPU node property from the host device tree that's a single >>>>>>> - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong >>>>>>> - * (can't find or open the property, or doesn't understand the format) >>>>>>> + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes >>>>>>> + * wrong (can't find or open the property, or doesn't understand the >>>>>>> + * format) >>>>>>> */ >>>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) >>>>>>> { >>>>>>> char buf[PATH_MAX], *tmp; >>>>>>> uint64_t val; >>>>>>> if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { >>>>>>> + error_setg(errp, "Failed to read CPU property %s", propname); >>>>>>> return 0; >>>>>>> } >>>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>>>> uint64_t kvmppc_get_clockfreq(void) >>>>>>> { >>>>>>> - return kvmppc_read_int_cpu_dt("clock-frequency"); >>>>>>> + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); >>>>>> >>>>>> >>>>>> This should be fatal. no ? >>>>> >>>>> >>>>> I'm not sure. I went under the assumption that there might be some weird >>>>> condition where 'clock-frequency' might be missing from the DT, and this >>>>> is why we're not exiting out immediately. >>>>> >>>>> That said, the advantage of turning this into fatal is that we won't need >>>>> all the other patches that handles error on this function. We're going to >>>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay >>>>> with that. >>>> >>>> I think so. Some machines behave badly when 'clock-frequency' is bogus, >>>> division by zero, no console, etc. We could check easily with pseries >>>> which is the only KVM PPC platform. >>> >>> Well not quite true ;) I haven't tested it during the last release cycle, but the Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini last time I checked. >> >> >> We can't just error_fatal by default then. >> >> Each machine should be able to determine how to handle this missing DT >> element. If I want to error_fatal for pseries then I can do so in patch >> 9/9, but other than that I'll keep the existing behavior. > > This wouldn't be a problem for the Mac machines since they pass the clock frequency from QEMU to OpenBIOS using the fw_cfg interface which builds the tree itself rather than using FDT. So I think using error_fatal should still be fine. One less board that would break if we error_fatal in that condition then :) In theory, for boards that doesn't care/doesn't support KVM, this function shouldn't be even called. I guess I'll move the series into this direction instead. Daniel > > > ATB, > > Mark.
On 7/5/22 08:57, Cédric Le Goater wrote: > On 7/5/22 08:51, Mark Cave-Ayland wrote: >> On 04/07/2022 18:34, Cédric Le Goater wrote: >> >>> On 7/2/22 15:34, Daniel Henrique Barboza wrote: >>>> >>>> >>>> On 7/2/22 03:24, Cédric Le Goater wrote: >>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote: >>>>>> The function can't just return 0 whether an error happened and call it a >>>>>> day. We must provide a way of letting callers know if the zero return is >>>>>> legitimate or due to an error. >>>>>> >>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled >>>>>> with an appropriate error, if one occurs. Callers are then free to pass >>>>>> an Error pointer and handle it. >>>>>> >>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>>>> --- >>>>>> target/ppc/kvm.c | 16 +++++++++------- >>>>>> 1 file changed, 9 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>>>>> index 109823136d..bc17437097 100644 >>>>>> --- a/target/ppc/kvm.c >>>>>> +++ b/target/ppc/kvm.c >>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) >>>>>> /* >>>>>> * Read a CPU node property from the host device tree that's a single >>>>>> - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong >>>>>> - * (can't find or open the property, or doesn't understand the format) >>>>>> + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes >>>>>> + * wrong (can't find or open the property, or doesn't understand the >>>>>> + * format) >>>>>> */ >>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) >>>>>> { >>>>>> char buf[PATH_MAX], *tmp; >>>>>> uint64_t val; >>>>>> if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { >>>>>> + error_setg(errp, "Failed to read CPU property %s", propname); >>>>>> return 0; >>>>>> } >>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>>> uint64_t kvmppc_get_clockfreq(void) >>>>>> { >>>>>> - return kvmppc_read_int_cpu_dt("clock-frequency"); >>>>>> + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); >>>>> >>>>> >>>>> This should be fatal. no ? >>>> >>>> >>>> I'm not sure. I went under the assumption that there might be some weird >>>> condition where 'clock-frequency' might be missing from the DT, and this >>>> is why we're not exiting out immediately. >>>> >>>> That said, the advantage of turning this into fatal is that we won't need >>>> all the other patches that handles error on this function. We're going to >>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay >>>> with that. >>> >>> I think so. Some machines behave badly when 'clock-frequency' is bogus, >>> division by zero, no console, etc. We could check easily with pseries >>> which is the only KVM PPC platform. >> >> Well not quite true ;) I haven't tested it during the last release cycle, but the Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini last time I checked. > > Oh. Sorry. and I still have access to a real G5 running the latest debian. > I should give it a try one day. I gave KVM a try on a : cpu : PPC970MP, altivec supported clock : 2000.000000MHz revision : 1.0 (pvr 0044 0100) processor : 1 cpu : PPC970MP, altivec supported clock : 2000.000000MHz revision : 1.0 (pvr 0044 0100) timebase : 33333333 platform : PowerMac model : PowerMac11,2 machine : PowerMac11,2 motherboard : PowerMac11,2 MacRISC4 Power Macintosh detected as : 337 (PowerMac G5 Dual Core) pmac flags : 00000000 L2 cache : 1024K unified pmac-generation : NewWorld running debian with kernel 5.18.0-2-powerpc64. With the installed QEMU 7.0.0, qemu-system-ppc64 -M mac99 -cpu host -accel kvm ... doesn't go very far. Program exception is quickly reached and host says: [56450.118422] Couldn't emulate instruction 0x00000000 (op 0 xop 0) [56450.119060] kvmppc_exit_pr_progint: emulation at 100 failed (00000000) Anything special I should know ? Thanks, C.
On 06/07/2022 08:45, Cédric Le Goater wrote: > On 7/5/22 08:57, Cédric Le Goater wrote: >> On 7/5/22 08:51, Mark Cave-Ayland wrote: >>> On 04/07/2022 18:34, Cédric Le Goater wrote: >>> >>>> On 7/2/22 15:34, Daniel Henrique Barboza wrote: >>>>> >>>>> >>>>> On 7/2/22 03:24, Cédric Le Goater wrote: >>>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote: >>>>>>> The function can't just return 0 whether an error happened and call it a >>>>>>> day. We must provide a way of letting callers know if the zero return is >>>>>>> legitimate or due to an error. >>>>>>> >>>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled >>>>>>> with an appropriate error, if one occurs. Callers are then free to pass >>>>>>> an Error pointer and handle it. >>>>>>> >>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>>>>> --- >>>>>>> target/ppc/kvm.c | 16 +++++++++------- >>>>>>> 1 file changed, 9 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>>>>>> index 109823136d..bc17437097 100644 >>>>>>> --- a/target/ppc/kvm.c >>>>>>> +++ b/target/ppc/kvm.c >>>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) >>>>>>> /* >>>>>>> * Read a CPU node property from the host device tree that's a single >>>>>>> - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong >>>>>>> - * (can't find or open the property, or doesn't understand the format) >>>>>>> + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes >>>>>>> + * wrong (can't find or open the property, or doesn't understand the >>>>>>> + * format) >>>>>>> */ >>>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) >>>>>>> { >>>>>>> char buf[PATH_MAX], *tmp; >>>>>>> uint64_t val; >>>>>>> if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { >>>>>>> + error_setg(errp, "Failed to read CPU property %s", propname); >>>>>>> return 0; >>>>>>> } >>>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char >>>>>>> *propname) >>>>>>> uint64_t kvmppc_get_clockfreq(void) >>>>>>> { >>>>>>> - return kvmppc_read_int_cpu_dt("clock-frequency"); >>>>>>> + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); >>>>>> >>>>>> >>>>>> This should be fatal. no ? >>>>> >>>>> >>>>> I'm not sure. I went under the assumption that there might be some weird >>>>> condition where 'clock-frequency' might be missing from the DT, and this >>>>> is why we're not exiting out immediately. >>>>> >>>>> That said, the advantage of turning this into fatal is that we won't need >>>>> all the other patches that handles error on this function. We're going to >>>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay >>>>> with that. >>>> >>>> I think so. Some machines behave badly when 'clock-frequency' is bogus, >>>> division by zero, no console, etc. We could check easily with pseries >>>> which is the only KVM PPC platform. >>> >>> Well not quite true ;) I haven't tested it during the last release cycle, but the >>> Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini >>> last time I checked. >> >> Oh. Sorry. and I still have access to a real G5 running the latest debian. >> I should give it a try one day. > > I gave KVM a try on a : > > cpu : PPC970MP, altivec supported > clock : 2000.000000MHz > revision : 1.0 (pvr 0044 0100) > processor : 1 > cpu : PPC970MP, altivec supported > clock : 2000.000000MHz > revision : 1.0 (pvr 0044 0100) > timebase : 33333333 > platform : PowerMac > model : PowerMac11,2 > machine : PowerMac11,2 > motherboard : PowerMac11,2 MacRISC4 Power Macintosh > detected as : 337 (PowerMac G5 Dual Core) > pmac flags : 00000000 > L2 cache : 1024K unified > pmac-generation : NewWorld > > running debian with kernel 5.18.0-2-powerpc64. With the installed QEMU 7.0.0, > > qemu-system-ppc64 -M mac99 -cpu host -accel kvm ... > > doesn't go very far. Program exception is quickly reached and host says: > > [56450.118422] Couldn't emulate instruction 0x00000000 (op 0 xop 0) > [56450.119060] kvmppc_exit_pr_progint: emulation at 100 failed (00000000) > > Anything special I should know ? As I don't have access to a G5 I've never tried that, however the qemu-system-ppc64 mac99 is wired differently to the qemu-system-ppc mac99 machine so I wouldn't be surprised if something is broken there. My normal test for MacOS is something like: qemu-system-ppc -M mac99 -accel kvm -hda macos104.img Can you try qemu-system-ppc and see if it is any better? If not then I can fire up the G4 and get the git hashes for my last known working configuration. ATB, Mark.
On 7/11/22 09:37, Mark Cave-Ayland wrote: > On 06/07/2022 08:45, Cédric Le Goater wrote: > >> On 7/5/22 08:57, Cédric Le Goater wrote: >>> On 7/5/22 08:51, Mark Cave-Ayland wrote: >>>> On 04/07/2022 18:34, Cédric Le Goater wrote: >>>> >>>>> On 7/2/22 15:34, Daniel Henrique Barboza wrote: >>>>>> >>>>>> >>>>>> On 7/2/22 03:24, Cédric Le Goater wrote: >>>>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote: >>>>>>>> The function can't just return 0 whether an error happened and call it a >>>>>>>> day. We must provide a way of letting callers know if the zero return is >>>>>>>> legitimate or due to an error. >>>>>>>> >>>>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled >>>>>>>> with an appropriate error, if one occurs. Callers are then free to pass >>>>>>>> an Error pointer and handle it. >>>>>>>> >>>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>>>>>> --- >>>>>>>> target/ppc/kvm.c | 16 +++++++++------- >>>>>>>> 1 file changed, 9 insertions(+), 7 deletions(-) >>>>>>>> >>>>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>>>>>>> index 109823136d..bc17437097 100644 >>>>>>>> --- a/target/ppc/kvm.c >>>>>>>> +++ b/target/ppc/kvm.c >>>>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) >>>>>>>> /* >>>>>>>> * Read a CPU node property from the host device tree that's a single >>>>>>>> - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong >>>>>>>> - * (can't find or open the property, or doesn't understand the format) >>>>>>>> + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes >>>>>>>> + * wrong (can't find or open the property, or doesn't understand the >>>>>>>> + * format) >>>>>>>> */ >>>>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) >>>>>>>> { >>>>>>>> char buf[PATH_MAX], *tmp; >>>>>>>> uint64_t val; >>>>>>>> if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { >>>>>>>> + error_setg(errp, "Failed to read CPU property %s", propname); >>>>>>>> return 0; >>>>>>>> } >>>>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) >>>>>>>> uint64_t kvmppc_get_clockfreq(void) >>>>>>>> { >>>>>>>> - return kvmppc_read_int_cpu_dt("clock-frequency"); >>>>>>>> + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); >>>>>>> >>>>>>> >>>>>>> This should be fatal. no ? >>>>>> >>>>>> >>>>>> I'm not sure. I went under the assumption that there might be some weird >>>>>> condition where 'clock-frequency' might be missing from the DT, and this >>>>>> is why we're not exiting out immediately. >>>>>> >>>>>> That said, the advantage of turning this into fatal is that we won't need >>>>>> all the other patches that handles error on this function. We're going to >>>>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay >>>>>> with that. >>>>> >>>>> I think so. Some machines behave badly when 'clock-frequency' is bogus, >>>>> division by zero, no console, etc. We could check easily with pseries >>>>> which is the only KVM PPC platform. >>>> >>>> Well not quite true ;) I haven't tested it during the last release cycle, but the Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini last time I checked. >>> >>> Oh. Sorry. and I still have access to a real G5 running the latest debian. >>> I should give it a try one day. >> >> I gave KVM a try on a : >> >> cpu : PPC970MP, altivec supported >> clock : 2000.000000MHz >> revision : 1.0 (pvr 0044 0100) >> processor : 1 >> cpu : PPC970MP, altivec supported >> clock : 2000.000000MHz >> revision : 1.0 (pvr 0044 0100) >> timebase : 33333333 >> platform : PowerMac >> model : PowerMac11,2 >> machine : PowerMac11,2 >> motherboard : PowerMac11,2 MacRISC4 Power Macintosh >> detected as : 337 (PowerMac G5 Dual Core) >> pmac flags : 00000000 >> L2 cache : 1024K unified >> pmac-generation : NewWorld >> >> running debian with kernel 5.18.0-2-powerpc64. With the installed QEMU 7.0.0, >> >> qemu-system-ppc64 -M mac99 -cpu host -accel kvm ... >> >> doesn't go very far. Program exception is quickly reached and host says: >> >> [56450.118422] Couldn't emulate instruction 0x00000000 (op 0 xop 0) >> [56450.119060] kvmppc_exit_pr_progint: emulation at 100 failed (00000000) >> >> Anything special I should know ? > > As I don't have access to a G5 I've never tried that, however the qemu-system-ppc64 mac99 is wired differently to the qemu-system-ppc mac99 machine so I wouldn't be surprised if something is broken there. > > My normal test for MacOS is something like: > > qemu-system-ppc -M mac99 -accel kvm -hda macos104.img > > Can you try qemu-system-ppc and see if it is any better? If not then I can fire up the G4 and get the git hashes for my last known working configuration. Same issue with 32bit. Thanks, C.
On Mon, 11 Jul 2022, Mark Cave-Ayland wrote: > On 06/07/2022 08:45, Cédric Le Goater wrote: >> I gave KVM a try on a : >> >> cpu : PPC970MP, altivec supported >> clock : 2000.000000MHz >> revision : 1.0 (pvr 0044 0100) >> processor : 1 >> cpu : PPC970MP, altivec supported >> clock : 2000.000000MHz >> revision : 1.0 (pvr 0044 0100) >> timebase : 33333333 >> platform : PowerMac >> model : PowerMac11,2 >> machine : PowerMac11,2 >> motherboard : PowerMac11,2 MacRISC4 Power Macintosh >> detected as : 337 (PowerMac G5 Dual Core) >> pmac flags : 00000000 >> L2 cache : 1024K unified >> pmac-generation : NewWorld >> >> running debian with kernel 5.18.0-2-powerpc64. With the installed QEMU >> 7.0.0, >> >> qemu-system-ppc64 -M mac99 -cpu host -accel kvm ... >> >> doesn't go very far. Program exception is quickly reached and host says: >> >> [56450.118422] Couldn't emulate instruction 0x00000000 (op 0 xop 0) >> [56450.119060] kvmppc_exit_pr_progint: emulation at 100 failed >> (00000000) Maybe try with -d unimp,guest_errors at least or some more debug options to find why it gets a 0 opcode. It probably takes a wrong exception somewhere? But with KVM maybe this does not give more info and you need to enable KVM tracing or run in a debgger instead? In the past I've managed to run Linux on qemu-system-ppc64 -M mac99 with TCG and trace KVM-PR guest within that but I forgot the details. If I remember correctly I've found there's some problem with emulated KVM and nobody replied on the list so I could not go further. It's also quite slow that way so not the best way to test. >> Anything special I should know ? > > As I don't have access to a G5 I've never tried that, however the > qemu-system-ppc64 mac99 is wired differently to the qemu-system-ppc mac99 > machine so I wouldn't be surprised if something is broken there. I think you can get the 32 bit version with qemu-system-ppc64 by adding -cpu G4 as it decides based on CPU type what to emulate. By default -M mac99 with qemu-system-ppc64 is a G5 Mac but OpenBIOS still gives it the same device-tree as the G4 one so guests might be confused by this. Linux did not seem to care that much though. > My normal test for MacOS is something like: > > qemu-system-ppc -M mac99 -accel kvm -hda macos104.img This should really be -M mac99,via=pmu as that is the closest to real hardware currently. This mac99 machine is quite confusing and maybe the only reason we need separate ppc and ppc64 qemu executables. What do you think about deprecating it and splitting it into something like powermac3_1 for mac99,via=pmu with G4 CPU, powermac7_2 for the G5 one and maybe some powerbookX_Y for the mac99 with CUDA instead of PMU but I don't know if that actually exists in real hardware? Also rename g3beige to the actual model name at the same time. This would clear this up and avoid the confusing options that we have now because of everything emulated by the single mac99 machine. Regards, BALATON Zoltan
On 11/07/2022 08:42, Cédric Le Goater wrote: >>> Anything special I should know ? >> >> As I don't have access to a G5 I've never tried that, however the qemu-system-ppc64 >> mac99 is wired differently to the qemu-system-ppc mac99 machine so I wouldn't be >> surprised if something is broken there. >> >> My normal test for MacOS is something like: >> >> qemu-system-ppc -M mac99 -accel kvm -hda macos104.img >> >> Can you try qemu-system-ppc and see if it is any better? If not then I can fire up >> the G4 and get the git hashes for my last known working configuration. > > Same issue with 32bit. I've just fired up my G4 to test this again, pulled the latest QEMU git master and confirmed that I have a working setup with the details below: Host kernel: (5.1.0-rc2+) commit a3ac7917b73070010c05b4485b8582a6c9cd69b6 Author: Linus Torvalds <torvalds@linux-foundation.org> Date: Mon Mar 25 14:49:00 2019 -0700 Guest kernel: (4.14.0-3-powerpc) using Debian ports debian-9.0-powerpc-NETINST-1.iso Command line: ./qemu-system-ppc [-M mac99] -accel kvm -cdrom /home/mca/images/debian-9.0-powerpc-NETINST-1.iso -boot d -nographic However if I switch to using the latest Debian ports debian-10.0.0-powerpc-NETINST-1.iso then I get a failure: [ 0.198565] BUG: Unable to handle kernel data access on read at 0xbb0030d4 [ 0.205152] Faulting instruction address: 0x0001b0c4 [ 0.210175] Oops: Kernel access of bad area, sig: 11 [#1] [ 0.214933] BE PAGE_SIZE=4K MMU=Hash PowerMac [ 0.218226] Modules linked in: [ 0.220746] CPU: 0 PID: 0 Comm: swapper Not tainted 5.6.0-2-powerpc #1 Debian 5.6.14-1 [ 0.226967] NIP: 0001b0c4 LR: 000030d4 CTR: 00000000 [ 0.230869] REGS: c7fb5908 TRAP: 0300 Not tainted (5.6.0-2-powerpc Debian 5.6.14-1) [ 0.236844] MSR: 00001012 <ME,DR,RI> CR: 24002820 XER: 20000000 [ 0.242096] DAR: bb0030d4 DSISR: 40000000 [ 0.242096] GPR00: c0044e70 c7fb59c0 c0b26510 c7fb5f48 bb0030d4 40000000 00000000 00000001 [ 0.242096] GPR08: ff340038 bb0030d4 00001032 c7fb59c0 00000000 00000000 00000000 00000004 [ 0.242096] GPR16: 029c61f0 029c5d68 07c5cd08 00000001 029dd844 fffffffd fff55d10 42000000 [ 0.242096] GPR24: c0af6704 c0b20d94 00000000 00000000 c0bd862c 00000000 00000000 0000000d [ 0.271138] NIP [0001b0c4] 0x1b0c4 [ 0.273978] LR [000030d4] 0x30d4 [ 0.276410] Call Trace: [ 0.278812] Instruction dump: [ 0.281219] 55290206 XXXXXXXX XXXXXXXX XXXXXXXX 4c00012c XXXXXXXX XXXXXXXX XXXXXXXX [ 0.287561] 419f0028 XXXXXXXX XXXXXXXX XXXXXXXX 81690000 XXXXXXXX XXXXXXXX XXXXXXXX [ 0.293922] ---[ end trace 3a9d775bab6f3340 ]--- [ 0.297491] [ 1.284408] Kernel panic - not syncing: Attempted to kill the idle task! [ 1.290027] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]--- Decompressing the initrd takes quite a long time, but I think this is the issue that was recently discussed on the mailing list? I think the next step should be to move my host kernel forward to a more current version with the working debian-9.0-powerpc-NETINST-1.iso and see if it is able to boot without any problems. ATB, Mark.
On Tue, 12 Jul 2022, Mark Cave-Ayland wrote: > On 11/07/2022 08:42, Cédric Le Goater wrote: >>>> Anything special I should know ? >>> >>> As I don't have access to a G5 I've never tried that, however the >>> qemu-system-ppc64 mac99 is wired differently to the qemu-system-ppc mac99 >>> machine so I wouldn't be surprised if something is broken there. >>> >>> My normal test for MacOS is something like: >>> >>> qemu-system-ppc -M mac99 -accel kvm -hda macos104.img >>> >>> Can you try qemu-system-ppc and see if it is any better? If not then I can >>> fire up the G4 and get the git hashes for my last known working >>> configuration. >> >> Same issue with 32bit. > > I've just fired up my G4 to test this again, pulled the latest QEMU git > master and confirmed that I have a working setup with the details below: > > Host kernel: (5.1.0-rc2+) > commit a3ac7917b73070010c05b4485b8582a6c9cd69b6 > Author: Linus Torvalds <torvalds@linux-foundation.org> > Date: Mon Mar 25 14:49:00 2019 -0700 > > Guest kernel: (4.14.0-3-powerpc) > using Debian ports debian-9.0-powerpc-NETINST-1.iso > > Command line: > ./qemu-system-ppc [-M mac99] -accel kvm -cdrom > /home/mca/images/debian-9.0-powerpc-NETINST-1.iso -boot d -nographic > > However if I switch to using the latest Debian ports > debian-10.0.0-powerpc-NETINST-1.iso then I get a failure: > > [ 0.198565] BUG: Unable to handle kernel data access on read at 0xbb0030d4 What is or should be at this address and why does the kernel access it? By default I see nothing mapped there. Do you need more RAM? Maybe the default 128 MB is not enough for newer kernels? I've seen such problem with other OSes before. Regards, BALATON Zoltan > [ 0.205152] Faulting instruction address: 0x0001b0c4 > [ 0.210175] Oops: Kernel access of bad area, sig: 11 [#1] > [ 0.214933] BE PAGE_SIZE=4K MMU=Hash PowerMac > [ 0.218226] Modules linked in: > [ 0.220746] CPU: 0 PID: 0 Comm: swapper Not tainted 5.6.0-2-powerpc #1 > Debian 5.6.14-1 > [ 0.226967] NIP: 0001b0c4 LR: 000030d4 CTR: 00000000 > [ 0.230869] REGS: c7fb5908 TRAP: 0300 Not tainted (5.6.0-2-powerpc > Debian 5.6.14-1) > [ 0.236844] MSR: 00001012 <ME,DR,RI> CR: 24002820 XER: 20000000 > [ 0.242096] DAR: bb0030d4 DSISR: 40000000 > [ 0.242096] GPR00: c0044e70 c7fb59c0 c0b26510 c7fb5f48 bb0030d4 40000000 > 00000000 00000001 > [ 0.242096] GPR08: ff340038 bb0030d4 00001032 c7fb59c0 00000000 00000000 > 00000000 00000004 > [ 0.242096] GPR16: 029c61f0 029c5d68 07c5cd08 00000001 029dd844 fffffffd > fff55d10 42000000 > [ 0.242096] GPR24: c0af6704 c0b20d94 00000000 00000000 c0bd862c 00000000 > 00000000 0000000d > [ 0.271138] NIP [0001b0c4] 0x1b0c4 > [ 0.273978] LR [000030d4] 0x30d4 > [ 0.276410] Call Trace: > [ 0.278812] Instruction dump: > [ 0.281219] 55290206 XXXXXXXX XXXXXXXX XXXXXXXX 4c00012c XXXXXXXX XXXXXXXX > XXXXXXXX > [ 0.287561] 419f0028 XXXXXXXX XXXXXXXX XXXXXXXX 81690000 XXXXXXXX XXXXXXXX > XXXXXXXX > [ 0.293922] ---[ end trace 3a9d775bab6f3340 ]--- > [ 0.297491] > [ 1.284408] Kernel panic - not syncing: Attempted to kill the idle task! > [ 1.290027] ---[ end Kernel panic - not syncing: Attempted to kill the > idle task! ]--- > > > Decompressing the initrd takes quite a long time, but I think this is the > issue that was recently discussed on the mailing list? > > I think the next step should be to move my host kernel forward to a more > current version with the working debian-9.0-powerpc-NETINST-1.iso and see if > it is able to boot without any problems. > > > ATB, > > Mark. > >
On 12/07/2022 15:54, BALATON Zoltan wrote: > On Tue, 12 Jul 2022, Mark Cave-Ayland wrote: >> On 11/07/2022 08:42, Cédric Le Goater wrote: >>>>> Anything special I should know ? >>>> >>>> As I don't have access to a G5 I've never tried that, however the >>>> qemu-system-ppc64 mac99 is wired differently to the qemu-system-ppc mac99 machine >>>> so I wouldn't be surprised if something is broken there. >>>> >>>> My normal test for MacOS is something like: >>>> >>>> qemu-system-ppc -M mac99 -accel kvm -hda macos104.img >>>> >>>> Can you try qemu-system-ppc and see if it is any better? If not then I can fire >>>> up the G4 and get the git hashes for my last known working configuration. >>> >>> Same issue with 32bit. >> >> I've just fired up my G4 to test this again, pulled the latest QEMU git master and >> confirmed that I have a working setup with the details below: >> >> Host kernel: (5.1.0-rc2+) >> commit a3ac7917b73070010c05b4485b8582a6c9cd69b6 >> Author: Linus Torvalds <torvalds@linux-foundation.org> >> Date: Mon Mar 25 14:49:00 2019 -0700 >> >> Guest kernel: (4.14.0-3-powerpc) >> using Debian ports debian-9.0-powerpc-NETINST-1.iso >> >> Command line: >> ./qemu-system-ppc [-M mac99] -accel kvm -cdrom >> /home/mca/images/debian-9.0-powerpc-NETINST-1.iso -boot d -nographic >> >> However if I switch to using the latest Debian ports >> debian-10.0.0-powerpc-NETINST-1.iso then I get a failure: >> >> [ 0.198565] BUG: Unable to handle kernel data access on read at 0xbb0030d4 > > What is or should be at this address and why does the kernel access it? By default I > see nothing mapped there. Do you need more RAM? Maybe the default 128 MB is not > enough for newer kernels? I've seen such problem with other OSes before. Yeah I've already tried increasing the RAM and it makes no difference. It wouldn't surprise me if it's a kernel issue since Christophe has done a lot of low-level work for 32-bit PPC including moving routines from asm to C and KASAN. I'm away for a few days but I will do a bisect when I get back, unless anyone beats me to it... ATB, Mark.
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c index 109823136d..bc17437097 100644 --- a/target/ppc/kvm.c +++ b/target/ppc/kvm.c @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename) /* * Read a CPU node property from the host device tree that's a single - * integer (32-bit or 64-bit). Returns 0 if anything goes wrong - * (can't find or open the property, or doesn't understand the format) + * integer (32-bit or 64-bit). Returns 0 and set errp if anything goes + * wrong (can't find or open the property, or doesn't understand the + * format) */ -static uint64_t kvmppc_read_int_cpu_dt(const char *propname) +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp) { char buf[PATH_MAX], *tmp; uint64_t val; if (kvmppc_find_cpu_dt(buf, sizeof(buf))) { + error_setg(errp, "Failed to read CPU property %s", propname); return 0; } @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) uint64_t kvmppc_get_clockfreq(void) { - return kvmppc_read_int_cpu_dt("clock-frequency"); + return kvmppc_read_int_cpu_dt("clock-frequency", NULL); } static int kvmppc_get_dec_bits(void) { - int nr_bits = kvmppc_read_int_cpu_dt("ibm,dec-bits"); + int nr_bits = kvmppc_read_int_cpu_dt("ibm,dec-bits", NULL); if (nr_bits > 0) { return nr_bits; @@ -2336,8 +2338,8 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on) static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data) { PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); - uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size"); - uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size"); + uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size", NULL); + uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size", NULL); /* Now fix up the class with information we can query from the host */ pcc->pvr = mfpvr();
The function can't just return 0 whether an error happened and call it a day. We must provide a way of letting callers know if the zero return is legitimate or due to an error. Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled with an appropriate error, if one occurs. Callers are then free to pass an Error pointer and handle it. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- target/ppc/kvm.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)