diff mbox series

[v2,4/9] prep: add ppc-parity write method

Message ID 20200624185523.762240-5-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show
Series memory: assert and define MemoryRegionOps callbacks | expand

Commit Message

Prasad Pandit June 24, 2020, 6:55 p.m. UTC 
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

Comments

David Gibson June 25, 2020, 1:40 a.m. UTC | #1 
On Thu, Jun 25, 2020 at 12:25:18AM +0530, P J P wrote:
> 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>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  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__);
> +}
> +
>  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,
Philippe Mathieu-Daudé June 25, 2020, 6:24 a.m. UTC | #2 
On 6/24/20 8:55 PM, P J P wrote:
> 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__);

What I meant was an error message about illegal/invalid write access.

> +}
> +
>  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,
>
Li Qiang June 29, 2020, 11:22 a.m. UTC | #3 
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.

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
>
Philippe Mathieu-Daudé July 21, 2020, 8:24 a.m. UTC | #4 
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
>>
>
diff mbox series

Patch

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__);
+}
+
 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,