| Message ID | 20190103093704.32635-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sun4u: add power_mem_read routine | expand |
On 03/01/2019 09:37, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > Define skeleton 'power_mem_read' routine. Avoid NULL dereference. > > Reported-by: Fakhri Zulkifli <mohdfakhrizulkifli@gmail.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/sparc64/sun4u.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c > index f76b19e4e9..cbdb2bb411 100644 > --- a/hw/sparc64/sun4u.c > +++ b/hw/sparc64/sun4u.c > @@ -214,6 +214,11 @@ typedef struct PowerDevice { > } PowerDevice; > > /* Power */ > +static uint64_t power_mem_read(void *opaque, hwaddr addr, unsigned size) > +{ > + return 0xffffffff; > +} > + > static void power_mem_write(void *opaque, hwaddr addr, > uint64_t val, unsigned size) > { > @@ -224,6 +229,7 @@ static void power_mem_write(void *opaque, hwaddr addr, > } > > static const MemoryRegionOps power_mem_ops = { > + .read = power_mem_read, > .write = power_mem_write, > .endianness = DEVICE_NATIVE_ENDIAN, > .valid = { Certainly the addition of the missing read function is valid, although I see that you've defaulted reads to returning 0xffffffff - can you point me towards the source that indicates that this behaviour matches that of real Ultra-5 hardware? ATB, Mark.
Hello Mark,
+-- On Thu, 3 Jan 2019, Mark Cave-Ayland wrote --+
| > /* Power */
| > +static uint64_t power_mem_read(void *opaque, hwaddr addr, unsigned size)
| > +{
| > + return 0xffffffff;
| > +}
| > +
| >
| > static const MemoryRegionOps power_mem_ops = {
| > + .read = power_mem_read,
| > .write = power_mem_write,
| > .endianness = DEVICE_NATIVE_ENDIAN,
| > .valid = {
|
| Certainly the addition of the missing read function is valid, although I see that
| you've defaulted reads to returning 0xffffffff - can you point me towards the source
| that indicates that this behaviour matches that of real Ultra-5 hardware?
-> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02787.html
I haven't checked Ultra-5 h/w spec, return value was chosen similar to above
one, thinking unimplemented .read need not return success.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On 04/01/2019 05:47, P J P wrote: > Hello Mark, > > +-- On Thu, 3 Jan 2019, Mark Cave-Ayland wrote --+ > | > /* Power */ > | > +static uint64_t power_mem_read(void *opaque, hwaddr addr, unsigned size) > | > +{ > | > + return 0xffffffff; > | > +} > | > + > | > > | > static const MemoryRegionOps power_mem_ops = { > | > + .read = power_mem_read, > | > .write = power_mem_write, > | > .endianness = DEVICE_NATIVE_ENDIAN, > | > .valid = { > | > | Certainly the addition of the missing read function is valid, although I see that > | you've defaulted reads to returning 0xffffffff - can you point me towards the source > | that indicates that this behaviour matches that of real Ultra-5 hardware? > > -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02787.html > > I haven't checked Ultra-5 h/w spec, return value was chosen similar to above > one, thinking unimplemented .read need not return success. > > Thank you. I asked someone with real Ultra-5 hardware to check this for me, and they've sent me back the following output: ok .properties address fffb8000 button interrupts 00000001 reg 00000014 00724000 00000004 name power ok fffb8000 l@ u. 0 Looks like the read value should be all 0s rather than all 1s. Can you resubmit the patch with this change? ATB, Mark.
+-- On Fri, 4 Jan 2019, Mark Cave-Ayland wrote --+ | I asked someone with real Ultra-5 hardware to check this for me, and they've | sent me back the following output: | | ok .properties | address fffb8000 | button | interrupts 00000001 | reg 00000014 00724000 00000004 | name power | ok fffb8000 l@ u. | 0 | | Looks like the read value should be all 0s rather than all 1s. Can you resubmit the | patch with this change? Done. Sent patch v1. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Le ven. 4 janv. 2019 10:55, P J P <ppandit@redhat.com> a écrit : > +-- On Fri, 4 Jan 2019, Mark Cave-Ayland wrote --+ > | I asked someone with real Ultra-5 hardware to check this for me, and > they've > | sent me back the following output: > | > | ok .properties > | address fffb8000 > | button > | interrupts 00000001 > | reg 00000014 00724000 00000004 > | name power > | ok fffb8000 l@ u. > | 0 > | > | Looks like the read value should be all 0s rather than all 1s. Can you > resubmit the > | patch with this change? > > Done. Sent patch v1. > The first patch is assumed to be v1, the second v2 and so on ;)
On 04/01/2019 09:53, P J P wrote: > +-- On Fri, 4 Jan 2019, Mark Cave-Ayland wrote --+ > | I asked someone with real Ultra-5 hardware to check this for me, and they've > | sent me back the following output: > | > | ok .properties > | address fffb8000 > | button > | interrupts 00000001 > | reg 00000014 00724000 00000004 > | name power > | ok fffb8000 l@ u. > | 0 > | > | Looks like the read value should be all 0s rather than all 1s. Can you resubmit the > | patch with this change? > > Done. Sent patch v1. Thanks. I've applied this to my qemu-sparc branch. ATB, Mark.
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c index f76b19e4e9..cbdb2bb411 100644 --- a/hw/sparc64/sun4u.c +++ b/hw/sparc64/sun4u.c @@ -214,6 +214,11 @@ typedef struct PowerDevice { } PowerDevice; /* Power */ +static uint64_t power_mem_read(void *opaque, hwaddr addr, unsigned size) +{ + return 0xffffffff; +} + static void power_mem_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) { @@ -224,6 +229,7 @@ static void power_mem_write(void *opaque, hwaddr addr, } static const MemoryRegionOps power_mem_ops = { + .read = power_mem_read, .write = power_mem_write, .endianness = DEVICE_NATIVE_ENDIAN, .valid = {