| Message ID | 20191120152442.26657-30-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand |
On 11/20/19 4:24 PM, Marc-André Lureau wrote: > There is no "saar" property. Note: I haven't been able to test this > code. Help welcome. > > May fix commit 043715d1e0fbb3e3411be3f898c5b77b7f90327a ("target/mips: > Update ITU to utilize SAARI and SAAR CP0 registers") > > Cc: Aleksandar Markovic <amarkovic@wavecomp.com> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/mips/cps.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/mips/cps.c b/hw/mips/cps.c > index 1660f86908..c49868d5da 100644 > --- a/hw/mips/cps.c > +++ b/hw/mips/cps.c > @@ -106,7 +106,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) > object_property_set_bool(OBJECT(&s->itu), saar_present, "saar-present", > &err); > if (saar_present) { > - qdev_prop_set_ptr(DEVICE(&s->itu), "saar", (void *)&env->CP0_SAAR); > + s->itu.saar = &env->CP0_SAAR; Hmmm this could be what the author of 043715d1e wanted to do indeed. Since this feature is incomplete (see comments in previous versions of this series: $ git grep saarp hw/mips/cps.c:98: saar_present = (bool)env->saarp; target/mips/cpu.h:1103: int saarp; and I don't know how to test this, I'll defer to Aleksandar regarding this patch. > } > object_property_set_bool(OBJECT(&s->itu), true, "realized", &err); > if (err != NULL) { >
> From: Philippe Mathieu-Daudé <philmd@redhat.com> > Sent: Sunday, December 15, 2019 6:56 AM > > On 11/20/19 4:24 PM, Marc-André Lureau wrote: > > There is no "saar" property. Note: I haven't been able to test this > > code. Help welcome. > > > > May fix commit 043715d1e0fbb3e3411be3f898c5b77b7f90327a ("target/mips: > > Update ITU to utilize SAARI and SAAR CP0 registers") > > > > Cc: Aleksandar Markovic <amarkovic@wavecomp.com> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > hw/mips/cps.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/mips/cps.c b/hw/mips/cps.c > > index 1660f86908..c49868d5da 100644 > > --- a/hw/mips/cps.c > > +++ b/hw/mips/cps.c > > @@ -106,7 +106,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) > > object_property_set_bool(OBJECT(&s->itu), saar_present, "saar-present", > > &err); > > if (saar_present) { > > - qdev_prop_set_ptr(DEVICE(&s->itu), "saar", (void *)&env->CP0_SAAR); > > + s->itu.saar = &env->CP0_SAAR; > > Hmmm this could be what the author of 043715d1e wanted to do indeed. > > Since this feature is incomplete (see comments in previous versions of > this series: > $ git grep saarp > hw/mips/cps.c:98: saar_present = (bool)env->saarp; > target/mips/cpu.h:1103: int saarp; > and I don't know how to test this, I'll defer to Aleksandar regarding > this patch. Hello, Philippe, Good that you brought this to my attention - but Marc-André and I already had a discussion about this in this series' cover letter responses (unfortunately not followed up by me as a response to this patch, sorry about that): https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg00056.html In essence, our conclusion was that the real fix would be certainly outside of the scope of this series, since it requires some really (non-straightforward) mips-specific code changes, so we agreed that Marc-André submit this patch as-is, and later on (certainly within timeframe of 5.0) I will submit the fix addressing the root cause (absence of "saar" property, and incomplete handling of SAAR and SAARI config registers). In other words, this patch is fine at this moment, and formally: Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com> (Marc-André, just please remove "RFC" from the title, and remove the sentence "Note: I haven't been able to test this code. Help welcome." from the commit message, since we have the deal on how and who will fix the underlining problem.) Thanks to both of you! Aleksandar
On 12/16/19 8:36 PM, Aleksandar Markovic wrote: >> From: Philippe Mathieu-Daudé <philmd@redhat.com> >> Sent: Sunday, December 15, 2019 6:56 AM >>> On 11/20/19 4:24 PM, Marc-André Lureau wrote: >>> There is no "saar" property. Note: I haven't been able to test this >>> code. Help welcome. >>> >>> May fix commit 043715d1e0fbb3e3411be3f898c5b77b7f90327a ("target/mips: >>> Update ITU to utilize SAARI and SAAR CP0 registers") >>> >>> Cc: Aleksandar Markovic <amarkovic@wavecomp.com> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> --- >>> hw/mips/cps.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/mips/cps.c b/hw/mips/cps.c >>> index 1660f86908..c49868d5da 100644 >>> --- a/hw/mips/cps.c >>> +++ b/hw/mips/cps.c >>> @@ -106,7 +106,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) >>> object_property_set_bool(OBJECT(&s->itu), saar_present, "saar-present", >>> &err); >>> if (saar_present) { >>> - qdev_prop_set_ptr(DEVICE(&s->itu), "saar", (void *)&env->CP0_SAAR); >>> + s->itu.saar = &env->CP0_SAAR; >> >> Hmmm this could be what the author of 043715d1e wanted to do indeed. >> >> Since this feature is incomplete (see comments in previous versions of >> this series: >> $ git grep saarp >> hw/mips/cps.c:98: saar_present = (bool)env->saarp; >> target/mips/cpu.h:1103: int saarp; >> and I don't know how to test this, I'll defer to Aleksandar regarding >> this patch. > > Hello, Philippe, > > Good that you brought this to my attention - but Marc-André and > I already had a discussion about this in this series' cover letter > responses (unfortunately not followed up by me as a response to > this patch, sorry about that): > > https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg00056.html > > In essence, our conclusion was that the real fix would be certainly > outside of the scope of this series, since it requires some really > (non-straightforward) mips-specific code changes, so we agreed > that Marc-André submit this patch as-is, and later on (certainly > within timeframe of 5.0) I will submit the fix addressing the root > cause (absence of "saar" property, and incomplete handling of > SAAR and SAARI config registers). OK, thanks for clarifying again! > In other words, this patch is fine at this moment, and formally: > > Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com> > > (Marc-André, just please remove "RFC" from the title, and remove > the sentence "Note: I haven't been able to test this code. Help > welcome." from the commit message, since we have the deal on > how and who will fix the underlining problem.) > > Thanks to both of you! > > Aleksandar >
diff --git a/hw/mips/cps.c b/hw/mips/cps.c index 1660f86908..c49868d5da 100644 --- a/hw/mips/cps.c +++ b/hw/mips/cps.c @@ -106,7 +106,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) object_property_set_bool(OBJECT(&s->itu), saar_present, "saar-present", &err); if (saar_present) { - qdev_prop_set_ptr(DEVICE(&s->itu), "saar", (void *)&env->CP0_SAAR); + s->itu.saar = &env->CP0_SAAR; } object_property_set_bool(OBJECT(&s->itu), true, "realized", &err); if (err != NULL) {
There is no "saar" property. Note: I haven't been able to test this code. Help welcome. May fix commit 043715d1e0fbb3e3411be3f898c5b77b7f90327a ("target/mips: Update ITU to utilize SAARI and SAAR CP0 registers") Cc: Aleksandar Markovic <amarkovic@wavecomp.com> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- hw/mips/cps.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)