Re: [PATCH v2 4/9] prep: add ppc-parity write method - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Li Qiang <liq3ea@gmail.com>, P J P <ppandit@redhat.com>
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
	"Prasad J Pandit" <pjp@fedoraproject.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Lei Sun" <slei.casper@gmail.com>,
	"Alex Williamson" <alex.williamson@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"David Gibson" <david@gibson.dropbear.id.au>
Subject: Re: [PATCH v2 4/9] prep: add ppc-parity write method
Date: Tue, 21 Jul 2020 10:24:10 +0200	[thread overview]
Message-ID: <c2b078bd-1625-7f6f-1457-20e684104789@redhat.com> (raw)
In-Reply-To: <CAKXe6SJy0Pocgd2EuF6jfdOn8AN4xFmzepgJMgKK=x-ouA5JQA@mail.gmail.com>

On 6/29/20 1:22 PM, Li Qiang wrote:
> P J P <ppandit@redhat.com> 于2020年6月25日周四 上午3:00写道：
>>
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> Add ppc-parity mmio write method to avoid NULL pointer dereference
>> issue.
>>
>> Reported-by: Lei Sun <slei.casper@gmail.com>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
>>  hw/ppc/prep_systemio.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> Update v2: use LOG_GUEST_ERROR
>>   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg04975.html
>>
>> diff --git a/hw/ppc/prep_systemio.c b/hw/ppc/prep_systemio.c
>> index bbc51b6e9a..03a2f8af6e 100644
>> --- a/hw/ppc/prep_systemio.c
>> +++ b/hw/ppc/prep_systemio.c
>> @@ -23,6 +23,7 @@
>>   */
>>
>>  #include "qemu/osdep.h"
>> +#include "qemu/log.h"
>>  #include "hw/irq.h"
>>  #include "hw/isa/isa.h"
>>  #include "hw/qdev-properties.h"
>> @@ -235,8 +236,15 @@ static uint64_t ppc_parity_error_readl(void *opaque, hwaddr addr,
>>      return val;
>>  }
>>
>> +static void ppc_parity_error_writel(void *opaque, hwaddr addr,
>> +                                    uint64_t data, unsigned size)
>> +{
>> +    qemu_log_mask(LOG_GUEST_ERROR, "%s not implemented\n", __func__);
>> +}
> 
> I'm not familiar with this platform-related stuff. I think a
> 'qemu_log_mask(LOG_UNIMP, xxx)' is ok.

No. The message is indeed confusing, as Prasad mixed GuestError
VS UnimplementedFeature.

Guest should not write the the error parity registers (I suppose
they are read-only). If it does, the hardware won't behave
incorrectly, it will just ignore the invalid accesses. We want
to report an incorrect guest behavior (why is the guest code
trying to do that?). Also, we will never implement this.
We usually use something like:

  qemu_log_mask(LOG_GUEST_ERROR,
                "%s: Illegal write to read-only register "
                "(size %u, offset 0x%"HWADDR_PRIx","
                " value 0x04%"PRIx64")\n",
                __func__, size, addr, data);

> 
> Thanks,
> Li Qiang
> 
>> +
>>  static const MemoryRegionOps ppc_parity_error_ops = {
>>      .read = ppc_parity_error_readl,
>> +    .write = ppc_parity_error_writel,
>>      .valid = {
>>          .min_access_size = 4,
>>          .max_access_size = 4,
>> --
>> 2.26.2
>>
> 