| Message ID | 20200624185523.762240-8-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory: assert and define MemoryRegionOps callbacks | expand |
On 6/24/20 8:55 PM, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > Add tz-ppc-dummy mmio read/write methods to avoid assert failure > during initialisation. > > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/misc/tz-ppc.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/hw/misc/tz-ppc.c b/hw/misc/tz-ppc.c > index 6431257b52..dc8892f61f 100644 > --- a/hw/misc/tz-ppc.c > +++ b/hw/misc/tz-ppc.c > @@ -196,7 +196,22 @@ static bool tz_ppc_dummy_accepts(void *opaque, hwaddr addr, > g_assert_not_reached(); This is a shame we now have to fill the read/write handlers for unreachable code :( > } > > +static uint64_t tz_ppc_dummy_read(void *opaque, hwaddr addr, unsigned size) > +{ I'd instead use a clearer: g_assert_not_reached(); > + qemu_log_mask(LOG_GUEST_ERROR, "%s not implemented\n", __func__); > + return 0; > +} > + > +static void tz_ppc_dummy_write(void *opaque, hwaddr addr, > + uint64_t data, unsigned size) > +{ Ditto: g_assert_not_reached(); > + qemu_log_mask(LOG_GUEST_ERROR, "%s not implemented\n", __func__); > +} > + > static const MemoryRegionOps tz_ppc_dummy_ops = { > + /* define r/w methods to avoid assert failure in memory_region_init_io */ > + .read = tz_ppc_dummy_read, > + .write = tz_ppc_dummy_write, > .valid.accepts = tz_ppc_dummy_accepts, > }; > >
+-- On Thu, 25 Jun 2020, Philippe Mathieu-Daudé wrote --+ | > @@ -196,7 +196,22 @@ static bool tz_ppc_dummy_accepts(void *opaque, hwaddr addr, | > g_assert_not_reached(); | | This is a shame we now have to fill the read/write handlers for | unreachable code :( | | > +static uint64_t tz_ppc_dummy_read(void *opaque, hwaddr addr, unsigned size) | | I'd instead use a clearer: | g_assert_not_reached(); | | > +static void tz_ppc_dummy_write(void *opaque, hwaddr addr, | | Ditto: | g_assert_not_reached(); This will likely be called in tz_ppc_dummy_accepts() above. Do we still want to revise this patch? considering read/write callbacks are unreachable. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
On 6/25/20 11:18 AM, P J P wrote: > +-- On Thu, 25 Jun 2020, Philippe Mathieu-Daudé wrote --+ > | > @@ -196,7 +196,22 @@ static bool tz_ppc_dummy_accepts(void *opaque, hwaddr addr, > | > g_assert_not_reached(); > | > | This is a shame we now have to fill the read/write handlers for > | unreachable code :( > | > | > +static uint64_t tz_ppc_dummy_read(void *opaque, hwaddr addr, unsigned size) > | > | I'd instead use a clearer: > | g_assert_not_reached(); > | > | > +static void tz_ppc_dummy_write(void *opaque, hwaddr addr, > | > | Ditto: > | g_assert_not_reached(); > > This will likely be called in tz_ppc_dummy_accepts() above. Do we still want > to revise this patch? considering read/write callbacks are unreachable. So a simple comment in each read/write might be sufficient (removing the qemu_log_mask calls).
+-- On Thu, 25 Jun 2020, Philippe Mathieu-Daudé wrote --+ | On 6/25/20 11:18 AM, P J P wrote: | > | g_assert_not_reached(); | > | > This will likely be called in tz_ppc_dummy_accepts() above. Do we still | > want to revise this patch? considering read/write callbacks are | > unreachable. | | So a simple comment in each read/write might be sufficient (removing the | qemu_log_mask calls). Okay. Will wait for other reviews, before sending a revised series v3. Hope that's okay. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
On Thu, 25 Jun 2020 at 10:18, P J P <ppandit@redhat.com> wrote: > > +-- On Thu, 25 Jun 2020, Philippe Mathieu-Daudé wrote --+ > | > @@ -196,7 +196,22 @@ static bool tz_ppc_dummy_accepts(void *opaque, hwaddr addr, > | > g_assert_not_reached(); > | > | This is a shame we now have to fill the read/write handlers for > | unreachable code :( > | > | > +static uint64_t tz_ppc_dummy_read(void *opaque, hwaddr addr, unsigned size) > | > | I'd instead use a clearer: > | g_assert_not_reached(); > | > | > +static void tz_ppc_dummy_write(void *opaque, hwaddr addr, > | > | Ditto: > | g_assert_not_reached(); > > This will likely be called in tz_ppc_dummy_accepts() above. Do we still want > to revise this patch? considering read/write callbacks are unreachable. The point of g_assert_not_reached() is that it documents and asserts that the code is not reachable. If the read and write callbacks are unreachable (which they are) then having their bodies just be a call to g_assert_not_reached() is the best way to document that. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> 于2020年6月25日周四 下午8:22写道： > > On Thu, 25 Jun 2020 at 10:18, P J P <ppandit@redhat.com> wrote: > > > > +-- On Thu, 25 Jun 2020, Philippe Mathieu-Daudé wrote --+ > > | > @@ -196,7 +196,22 @@ static bool tz_ppc_dummy_accepts(void *opaque, hwaddr addr, > > | > g_assert_not_reached(); > > | > > | This is a shame we now have to fill the read/write handlers for > > | unreachable code :( > > | > > | > +static uint64_t tz_ppc_dummy_read(void *opaque, hwaddr addr, unsigned size) > > | > > | I'd instead use a clearer: > > | g_assert_not_reached(); > > | > > | > +static void tz_ppc_dummy_write(void *opaque, hwaddr addr, > > | > > | Ditto: > > | g_assert_not_reached(); > > > > This will likely be called in tz_ppc_dummy_accepts() above. Do we still want > > to revise this patch? considering read/write callbacks are unreachable. > > The point of g_assert_not_reached() is that it documents and > asserts that the code is not reachable. If the read and write > callbacks are unreachable (which they are) then having their > bodies just be a call to g_assert_not_reached() is the best > way to document that. I agree with this. Thanks, Li Qiang > > thanks > -- PMM
+-- On Mon, 29 Jun 2020, Li Qiang wrote --+ | Peter Maydell <peter.maydell@linaro.org> 于2020年6月25日周四 下午8:22写道： | > The point of g_assert_not_reached() is that it documents and asserts that | > the code is not reachable. If the read and write callbacks are unreachable | > (which they are) then having their bodies just be a call to | > g_assert_not_reached() is the best way to document that. | | I agree with this. Okay, will do. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
On 25/06/20 12:21, Philippe Mathieu-Daudé wrote: >> | > g_assert_not_reached(); >> | >> | This is a shame we now have to fill the read/write handlers for >> | unreachable code :( >> | >> | > +static uint64_t tz_ppc_dummy_read(void *opaque, hwaddr addr, unsigned size) >> | >> | I'd instead use a clearer: >> | g_assert_not_reached(); >> | >> | > +static void tz_ppc_dummy_write(void *opaque, hwaddr addr, >> | >> | Ditto: >> | g_assert_not_reached(); >> >> This will likely be called in tz_ppc_dummy_accepts() above. Do we still want >> to revise this patch? considering read/write callbacks are unreachable. What would be called in tz_ppc_dummy_accepts()? Generally g_assert_not_reached is better (even better than abort :)). Paolo
diff --git a/hw/misc/tz-ppc.c b/hw/misc/tz-ppc.c index 6431257b52..dc8892f61f 100644 --- a/hw/misc/tz-ppc.c +++ b/hw/misc/tz-ppc.c @@ -196,7 +196,22 @@ static bool tz_ppc_dummy_accepts(void *opaque, hwaddr addr, g_assert_not_reached(); } +static uint64_t tz_ppc_dummy_read(void *opaque, hwaddr addr, unsigned size) +{ + qemu_log_mask(LOG_GUEST_ERROR, "%s not implemented\n", __func__); + return 0; +} + +static void tz_ppc_dummy_write(void *opaque, hwaddr addr, + uint64_t data, unsigned size) +{ + qemu_log_mask(LOG_GUEST_ERROR, "%s not implemented\n", __func__); +} + static const MemoryRegionOps tz_ppc_dummy_ops = { + /* define r/w methods to avoid assert failure in memory_region_init_io */ + .read = tz_ppc_dummy_read, + .write = tz_ppc_dummy_write, .valid.accepts = tz_ppc_dummy_accepts, };