[PATCH 0/7] ppc/pnv: Misc cleanups

[PATCH 0/7] ppc/pnv: Misc cleanups

[Cédric Le Goater]

Hello,

Most important changes in this series are a fix for the support for
external BMCs, when a QEMU Aspeed machine is used as a BMC instead of
the simulator, and a cleanup of the LPC model which was handling the
PNOR mapping.

The PNOR mapping is still a problem when using an external BMC and
this would require some kind of framework to do memory ops on a remote
memory region (LPC FW address space). Multi process might be a start
for that using the proxy object. Something to study.

Thanks,

C.

Cédric Le Goater (7):
  ppc/pnv: Add trace events for PCI event notification
  ppc/xive: Add firmware bit when dumping the ENDs
  ppc/pnv: Use skiboot addresses to load kernel and ramfs
  ppc/pnv: Simplify pnv_bmc_create()
  ppc/pnv: Discard internal BMC initialization when BMC is external
  ppc/pnv: Remove default disablement of the PNOR contents
  ppc/pnv: Introduce a LPC FW memory region attribute to map the PNOR

 include/hw/ppc/pnv.h       |  1 +
 include/hw/ppc/xive_regs.h |  2 ++
 hw/intc/pnv_xive.c         |  3 +++
 hw/intc/xive.c             |  3 ++-
 hw/pci-host/pnv_phb4.c     |  3 +++
 hw/ppc/pnv.c               | 17 ++++++++++++++---
 hw/ppc/pnv_bmc.c           | 22 +++++++++++++++-------
 hw/ppc/pnv_lpc.c           | 15 ---------------
 hw/intc/trace-events       |  3 +++
 hw/pci-host/trace-events   |  3 +++
 10 files changed, 46 insertions(+), 26 deletions(-)

-- 
2.26.2

[PATCH 1/7] ppc/pnv: Add trace events for PCI event notification

[Cédric Le Goater]

On POWER9 systems, PHB controllers signal the XIVE interrupt controller
of a source interrupt notification using a store on a MMIO region. Add
traces for such events.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/intc/pnv_xive.c       | 3 +++
 hw/pci-host/pnv_phb4.c   | 3 +++
 hw/intc/trace-events     | 3 +++
 hw/pci-host/trace-events | 3 +++
 4 files changed, 12 insertions(+)

diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
index 5f69626b3a8d..ad43483612e5 100644
--- a/hw/intc/pnv_xive.c
+++ b/hw/intc/pnv_xive.c
@@ -24,6 +24,7 @@
 #include "hw/ppc/xive_regs.h"
 #include "hw/qdev-properties.h"
 #include "hw/ppc/ppc.h"
+#include "trace.h"
 
 #include <libfdt.h>
 
@@ -1319,6 +1320,8 @@ static void pnv_xive_ic_hw_trigger(PnvXive *xive, hwaddr addr, uint64_t val)
     uint8_t blk;
     uint32_t idx;
 
+    trace_pnv_xive_ic_hw_trigger(addr, val);
+
     if (val & XIVE_TRIGGER_END) {
         xive_error(xive, "IC: END trigger at @0x%"HWADDR_PRIx" data 0x%"PRIx64,
                    addr, val);
diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 6328e985f81c..54f57c660a94 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -22,6 +22,7 @@
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "qom/object.h"
+#include "trace.h"
 
 #define phb_error(phb, fmt, ...)                                        \
     qemu_log_mask(LOG_GUEST_ERROR, "phb4[%d:%d]: " fmt "\n",            \
@@ -1257,6 +1258,8 @@ static void pnv_phb4_xive_notify(XiveNotifier *xf, uint32_t srcno)
     uint64_t data = XIVE_TRIGGER_PQ | offset | srcno;
     MemTxResult result;
 
+    trace_pnv_phb4_xive_notify(notif_port, data);
+
     address_space_stq_be(&address_space_memory, notif_port, data,
                          MEMTXATTRS_UNSPECIFIED, &result);
     if (result != MEMTX_OK) {
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 8ed397a0d587..45ddaf48df8e 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -236,3 +236,6 @@ xive_tctx_tm_write(uint64_t offset, unsigned int size, uint64_t value) "@0x0x%"P
 xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value) "@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
 xive_presenter_notify(uint8_t nvt_blk, uint32_t nvt_idx, uint8_t ring) "found NVT 0x%x/0x%x ring=0x%x"
 xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) "END 0x%x/0x%x @0x0x%"PRIx64
+
+# pnv_xive.c
+pnv_xive_ic_hw_trigger(uint64_t addr, uint64_t val) "@0x%"PRIx64" val=0x%"PRIx64
diff --git a/hw/pci-host/trace-events b/hw/pci-host/trace-events
index d19ca9aef6f7..7d8063ac4212 100644
--- a/hw/pci-host/trace-events
+++ b/hw/pci-host/trace-events
@@ -20,3 +20,6 @@ unin_data_write(uint64_t addr, unsigned len, uint64_t val) "write addr 0x%"PRIx6
 unin_data_read(uint64_t addr, unsigned len, uint64_t val) "read addr 0x%"PRIx64 " len %d val 0x%"PRIx64
 unin_write(uint64_t addr, uint64_t value) "addr=0x%" PRIx64 " val=0x%"PRIx64
 unin_read(uint64_t addr, uint64_t value) "addr=0x%" PRIx64 " val=0x%"PRIx64
+
+# pnv_phb4.c
+pnv_phb4_xive_notify(uint64_t notif_port, uint64_t data) "notif=@0x%"PRIx64" data=0x%"PRIx64
-- 
2.26.2

Re: [PATCH 1/7] ppc/pnv: Add trace events for PCI event notification

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3658 bytes --]

On Tue, Jan 26, 2021 at 06:10:53PM +0100, Cédric Le Goater wrote:
> On POWER9 systems, PHB controllers signal the XIVE interrupt controller
> of a source interrupt notification using a store on a MMIO region. Add
> traces for such events.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-6.0, thanks.

> ---
>  hw/intc/pnv_xive.c       | 3 +++
>  hw/pci-host/pnv_phb4.c   | 3 +++
>  hw/intc/trace-events     | 3 +++
>  hw/pci-host/trace-events | 3 +++
>  4 files changed, 12 insertions(+)
> 
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index 5f69626b3a8d..ad43483612e5 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -24,6 +24,7 @@
>  #include "hw/ppc/xive_regs.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/ppc/ppc.h"
> +#include "trace.h"
>  
>  #include <libfdt.h>
>  
> @@ -1319,6 +1320,8 @@ static void pnv_xive_ic_hw_trigger(PnvXive *xive, hwaddr addr, uint64_t val)
>      uint8_t blk;
>      uint32_t idx;
>  
> +    trace_pnv_xive_ic_hw_trigger(addr, val);
> +
>      if (val & XIVE_TRIGGER_END) {
>          xive_error(xive, "IC: END trigger at @0x%"HWADDR_PRIx" data 0x%"PRIx64,
>                     addr, val);
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 6328e985f81c..54f57c660a94 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -22,6 +22,7 @@
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
>  #include "qom/object.h"
> +#include "trace.h"
>  
>  #define phb_error(phb, fmt, ...)                                        \
>      qemu_log_mask(LOG_GUEST_ERROR, "phb4[%d:%d]: " fmt "\n",            \
> @@ -1257,6 +1258,8 @@ static void pnv_phb4_xive_notify(XiveNotifier *xf, uint32_t srcno)
>      uint64_t data = XIVE_TRIGGER_PQ | offset | srcno;
>      MemTxResult result;
>  
> +    trace_pnv_phb4_xive_notify(notif_port, data);
> +
>      address_space_stq_be(&address_space_memory, notif_port, data,
>                           MEMTXATTRS_UNSPECIFIED, &result);
>      if (result != MEMTX_OK) {
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 8ed397a0d587..45ddaf48df8e 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -236,3 +236,6 @@ xive_tctx_tm_write(uint64_t offset, unsigned int size, uint64_t value) "@0x0x%"P
>  xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value) "@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
>  xive_presenter_notify(uint8_t nvt_blk, uint32_t nvt_idx, uint8_t ring) "found NVT 0x%x/0x%x ring=0x%x"
>  xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) "END 0x%x/0x%x @0x0x%"PRIx64
> +
> +# pnv_xive.c
> +pnv_xive_ic_hw_trigger(uint64_t addr, uint64_t val) "@0x%"PRIx64" val=0x%"PRIx64
> diff --git a/hw/pci-host/trace-events b/hw/pci-host/trace-events
> index d19ca9aef6f7..7d8063ac4212 100644
> --- a/hw/pci-host/trace-events
> +++ b/hw/pci-host/trace-events
> @@ -20,3 +20,6 @@ unin_data_write(uint64_t addr, unsigned len, uint64_t val) "write addr 0x%"PRIx6
>  unin_data_read(uint64_t addr, unsigned len, uint64_t val) "read addr 0x%"PRIx64 " len %d val 0x%"PRIx64
>  unin_write(uint64_t addr, uint64_t value) "addr=0x%" PRIx64 " val=0x%"PRIx64
>  unin_read(uint64_t addr, uint64_t value) "addr=0x%" PRIx64 " val=0x%"PRIx64
> +
> +# pnv_phb4.c
> +pnv_phb4_xive_notify(uint64_t notif_port, uint64_t data) "notif=@0x%"PRIx64" data=0x%"PRIx64

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/7] ppc/xive: Add firmware bit when dumping the ENDs

[Cédric Le Goater]

ENDs allocated by OPAL for the HW thread VPs are tagged as owned by FW.
Dump the state in 'info pic'.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/xive_regs.h | 2 ++
 hw/intc/xive.c             | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h
index 787969282593..b7fde2354e31 100644
--- a/include/hw/ppc/xive_regs.h
+++ b/include/hw/ppc/xive_regs.h
@@ -236,6 +236,8 @@ typedef struct XiveEND {
     (be32_to_cpu((end)->w0) & END_W0_UNCOND_ESCALATE)
 #define xive_end_is_silent_escalation(end)              \
     (be32_to_cpu((end)->w0) & END_W0_SILENT_ESCALATE)
+#define xive_end_is_firmware(end)              \
+    (be32_to_cpu((end)->w0) & END_W0_FIRMWARE)
 
 static inline uint64_t xive_end_qaddr(XiveEND *end)
 {
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index fa8c3d82877f..eeb4e62ba954 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -1294,7 +1294,7 @@ void xive_end_pic_print_info(XiveEND *end, uint32_t end_idx, Monitor *mon)
 
     pq = xive_get_field32(END_W1_ESn, end->w1);
 
-    monitor_printf(mon, "  %08x %c%c %c%c%c%c%c%c%c prio:%d nvt:%02x/%04x",
+    monitor_printf(mon, "  %08x %c%c %c%c%c%c%c%c%c%c prio:%d nvt:%02x/%04x",
                    end_idx,
                    pq & XIVE_ESB_VAL_P ? 'P' : '-',
                    pq & XIVE_ESB_VAL_Q ? 'Q' : '-',
@@ -1305,6 +1305,7 @@ void xive_end_pic_print_info(XiveEND *end, uint32_t end_idx, Monitor *mon)
                    xive_end_is_escalate(end) ? 'e' : '-',
                    xive_end_is_uncond_escalation(end)   ? 'u' : '-',
                    xive_end_is_silent_escalation(end)   ? 's' : '-',
+                   xive_end_is_firmware(end)   ? 'f' : '-',
                    priority, nvt_blk, nvt_idx);
 
     if (qaddr_base) {
-- 
2.26.2

Re: [PATCH 2/7] ppc/xive: Add firmware bit when dumping the ENDs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2248 bytes --]

On Tue, Jan 26, 2021 at 06:10:54PM +0100, Cédric Le Goater wrote:
> ENDs allocated by OPAL for the HW thread VPs are tagged as owned by FW.
> Dump the state in 'info pic'.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-6.0, thanks.

> ---
>  include/hw/ppc/xive_regs.h | 2 ++
>  hw/intc/xive.c             | 3 ++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h
> index 787969282593..b7fde2354e31 100644
> --- a/include/hw/ppc/xive_regs.h
> +++ b/include/hw/ppc/xive_regs.h
> @@ -236,6 +236,8 @@ typedef struct XiveEND {
>      (be32_to_cpu((end)->w0) & END_W0_UNCOND_ESCALATE)
>  #define xive_end_is_silent_escalation(end)              \
>      (be32_to_cpu((end)->w0) & END_W0_SILENT_ESCALATE)
> +#define xive_end_is_firmware(end)              \
> +    (be32_to_cpu((end)->w0) & END_W0_FIRMWARE)
>  
>  static inline uint64_t xive_end_qaddr(XiveEND *end)
>  {
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index fa8c3d82877f..eeb4e62ba954 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -1294,7 +1294,7 @@ void xive_end_pic_print_info(XiveEND *end, uint32_t end_idx, Monitor *mon)
>  
>      pq = xive_get_field32(END_W1_ESn, end->w1);
>  
> -    monitor_printf(mon, "  %08x %c%c %c%c%c%c%c%c%c prio:%d nvt:%02x/%04x",
> +    monitor_printf(mon, "  %08x %c%c %c%c%c%c%c%c%c%c prio:%d nvt:%02x/%04x",
>                     end_idx,
>                     pq & XIVE_ESB_VAL_P ? 'P' : '-',
>                     pq & XIVE_ESB_VAL_Q ? 'Q' : '-',
> @@ -1305,6 +1305,7 @@ void xive_end_pic_print_info(XiveEND *end, uint32_t end_idx, Monitor *mon)
>                     xive_end_is_escalate(end) ? 'e' : '-',
>                     xive_end_is_uncond_escalation(end)   ? 'u' : '-',
>                     xive_end_is_silent_escalation(end)   ? 's' : '-',
> +                   xive_end_is_firmware(end)   ? 'f' : '-',
>                     priority, nvt_blk, nvt_idx);
>  
>      if (qaddr_base) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/7] ppc/pnv: Use skiboot addresses to load kernel and ramfs

[Cédric Le Goater]

The current settings are useful to load large kernels (with debug) but
it moves the initrd image in a memory region not protected by
skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will
corrupt the initrd.

Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 If we want to increase the kernel size limit as commit b45b56baeecd
 ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I
 think we should add a machine option.

 hw/ppc/pnv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 14fc9758a973..e500c2e2437e 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -65,9 +65,9 @@
 #define FW_MAX_SIZE             (16 * MiB)
 
 #define KERNEL_LOAD_ADDR        0x20000000
-#define KERNEL_MAX_SIZE         (256 * MiB)
-#define INITRD_LOAD_ADDR        0x60000000
-#define INITRD_MAX_SIZE         (256 * MiB)
+#define KERNEL_MAX_SIZE         (128 * MiB)
+#define INITRD_LOAD_ADDR        0x28000000
+#define INITRD_MAX_SIZE         (128 * MiB)
 
 static const char *pnv_chip_core_typename(const PnvChip *o)
 {
-- 
2.26.2

Re: [PATCH 3/7] ppc/pnv: Use skiboot addresses to load kernel and ramfs

[Murilo Opsfelder Araújo]

Bonjour, Cédric.

On Tuesday, January 26, 2021 2:10:55 PM -03 Cédric Le Goater wrote:
> The current settings are useful to load large kernels (with debug) but
> it moves the initrd image in a memory region not protected by
> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will
> corrupt the initrd.
> 
> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  If we want to increase the kernel size limit as commit b45b56baeecd
>  ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I
>  think we should add a machine option.

Is this a problem on bare-metal as well?

I'm wondering if we should address this the other way around by increasing
KERNEL_LOAD_SIZE and INITRAMFS_LOAD_SIZE in skiboot to accomodate large kernel
and initramfs images.

I think Linux debuginfo images won't get smaller with time and, assuming this
also happens on bare-metal (I haven't verified), updating skiboot looks more
appropriate.

Bear in mind that I'm not an skiboot expert, I'm just considering the
possibilities.

> 
>  hw/ppc/pnv.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 14fc9758a973..e500c2e2437e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -65,9 +65,9 @@
>  #define FW_MAX_SIZE             (16 * MiB)
> 
>  #define KERNEL_LOAD_ADDR        0x20000000
> -#define KERNEL_MAX_SIZE         (256 * MiB)
> -#define INITRD_LOAD_ADDR        0x60000000
> -#define INITRD_MAX_SIZE         (256 * MiB)
> +#define KERNEL_MAX_SIZE         (128 * MiB)
> +#define INITRD_LOAD_ADDR        0x28000000
> +#define INITRD_MAX_SIZE         (128 * MiB)
> 
>  static const char *pnv_chip_core_typename(const PnvChip *o)
>  {

Cheers!

-- 
Murilo

Re: [PATCH 3/7] ppc/pnv: Use skiboot addresses to load kernel and ramfs

[Cédric Le Goater]

On 1/27/21 2:27 AM, Murilo Opsfelder Araújo wrote:
> Bonjour, Cédric.
> 
> On Tuesday, January 26, 2021 2:10:55 PM -03 Cédric Le Goater wrote:
>> The current settings are useful to load large kernels (with debug) but
>> it moves the initrd image in a memory region not protected by
>> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will
>> corrupt the initrd.
>>
>> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  If we want to increase the kernel size limit as commit b45b56baeecd
>>  ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I
>>  think we should add a machine option.
> 
> Is this a problem on bare-metal as well?
> 
> I'm wondering if we should address this the other way around by increasing
> KERNEL_LOAD_SIZE and INITRAMFS_LOAD_SIZE in skiboot to accomodate large kernel
> and initramfs images.

The different memory areas are all strictly defined here : 

  https://github.com/open-power/skiboot/blob/master/include/mem-map.h

C. 

> I think Linux debuginfo images won't get smaller with time and, assuming this
> also happens on bare-metal (I haven't verified), updating skiboot looks more
> appropriate.
> 
> Bear in mind that I'm not an skiboot expert, I'm just considering the
> possibilities.
> 
>>
>>  hw/ppc/pnv.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 14fc9758a973..e500c2e2437e 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -65,9 +65,9 @@
>>  #define FW_MAX_SIZE             (16 * MiB)
>>
>>  #define KERNEL_LOAD_ADDR        0x20000000
>> -#define KERNEL_MAX_SIZE         (256 * MiB)
>> -#define INITRD_LOAD_ADDR        0x60000000
>> -#define INITRD_MAX_SIZE         (256 * MiB)
>> +#define KERNEL_MAX_SIZE         (128 * MiB)
>> +#define INITRD_LOAD_ADDR        0x28000000
>> +#define INITRD_MAX_SIZE         (128 * MiB)
>>
>>  static const char *pnv_chip_core_typename(const PnvChip *o)
>>  {
> 
> Cheers!
> 

Re: [PATCH 3/7] ppc/pnv: Use skiboot addresses to load kernel and ramfs

[Murilo Opsfelder Araújo]

On Tuesday, January 26, 2021 2:10:55 PM -03 Cédric Le Goater wrote:
> The current settings are useful to load large kernels (with debug) but
> it moves the initrd image in a memory region not protected by
> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will
> corrupt the initrd.
>
> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>

> ---
>
>  If we want to increase the kernel size limit as commit b45b56baeecd
>  ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I
>  think we should add a machine option.
>
>  hw/ppc/pnv.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 14fc9758a973..e500c2e2437e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -65,9 +65,9 @@
>  #define FW_MAX_SIZE             (16 * MiB)
>
>  #define KERNEL_LOAD_ADDR        0x20000000
> -#define KERNEL_MAX_SIZE         (256 * MiB)
> -#define INITRD_LOAD_ADDR        0x60000000
> -#define INITRD_MAX_SIZE         (256 * MiB)
> +#define KERNEL_MAX_SIZE         (128 * MiB)
> +#define INITRD_LOAD_ADDR        0x28000000
> +#define INITRD_MAX_SIZE         (128 * MiB)
>
>  static const char *pnv_chip_core_typename(const PnvChip *o)
>  {


--
Murilo

Re: [PATCH 3/7] ppc/pnv: Use skiboot addresses to load kernel and ramfs

[Joel Stanley]

On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater <clg@kaod.org> wrote:
>
> The current settings are useful to load large kernels (with debug) but
> it moves the initrd image in a memory region not protected by
> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will
> corrupt the initrd.
>
> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

Note that the machine's default ram size will change with this patch:

 mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;

So we will go from 1.75GB to 768MB. Does anything break when the
machine has less than 1GB of ram?

> ---
>
>  If we want to increase the kernel size limit as commit b45b56baeecd
>  ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I
>  think we should add a machine option.
>
>  hw/ppc/pnv.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 14fc9758a973..e500c2e2437e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -65,9 +65,9 @@
>  #define FW_MAX_SIZE             (16 * MiB)
>
>  #define KERNEL_LOAD_ADDR        0x20000000
> -#define KERNEL_MAX_SIZE         (256 * MiB)
> -#define INITRD_LOAD_ADDR        0x60000000
> -#define INITRD_MAX_SIZE         (256 * MiB)
> +#define KERNEL_MAX_SIZE         (128 * MiB)
> +#define INITRD_LOAD_ADDR        0x28000000
> +#define INITRD_MAX_SIZE         (128 * MiB)
>
>  static const char *pnv_chip_core_typename(const PnvChip *o)
>  {
> --
> 2.26.2
>
>

Re: [PATCH 3/7] ppc/pnv: Use skiboot addresses to load kernel and ramfs

[Cédric Le Goater]

On 1/28/21 1:45 AM, Joel Stanley wrote:
> On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> The current settings are useful to load large kernels (with debug) but
>> it moves the initrd image in a memory region not protected by
>> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will
>> corrupt the initrd.
>>
>> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
> Note that the machine's default ram size will change with this patch:
> 
>  mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;

Ah yes. I missed that.

> So we will go from 1.75GB to 768MB. Does anything break when the
> machine has less than 1GB of ram?

There is a warning if the machine has less than 1GB but we should
also change the default RAM size to 1G to be on the safe side.

Thanks,

C. 

> 
>> ---
>>
>>  If we want to increase the kernel size limit as commit b45b56baeecd
>>  ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I
>>  think we should add a machine option.
>>
>>  hw/ppc/pnv.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 14fc9758a973..e500c2e2437e 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -65,9 +65,9 @@
>>  #define FW_MAX_SIZE             (16 * MiB)
>>
>>  #define KERNEL_LOAD_ADDR        0x20000000
>> -#define KERNEL_MAX_SIZE         (256 * MiB)
>> -#define INITRD_LOAD_ADDR        0x60000000
>> -#define INITRD_MAX_SIZE         (256 * MiB)
>> +#define KERNEL_MAX_SIZE         (128 * MiB)
>> +#define INITRD_LOAD_ADDR        0x28000000
>> +#define INITRD_MAX_SIZE         (128 * MiB)
>>
>>  static const char *pnv_chip_core_typename(const PnvChip *o)
>>  {
>> --
>> 2.26.2
>>
>>

Re: [PATCH 3/7] ppc/pnv: Use skiboot addresses to load kernel and ramfs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2374 bytes --]

On Thu, Jan 28, 2021 at 08:02:41AM +0100, Cédric Le Goater wrote:
> On 1/28/21 1:45 AM, Joel Stanley wrote:
> > On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >> The current settings are useful to load large kernels (with debug) but
> >> it moves the initrd image in a memory region not protected by
> >> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will
> >> corrupt the initrd.
> >>
> >> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > 
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
> > 
> > Note that the machine's default ram size will change with this patch:
> > 
> >  mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
> 
> Ah yes. I missed that.
> 
> > So we will go from 1.75GB to 768MB. Does anything break when the
> > machine has less than 1GB of ram?
> 
> There is a warning if the machine has less than 1GB but we should
> also change the default RAM size to 1G to be on the safe side.

I've merged the patch, but I'm happy to replace it with an updated
version, or fold in a change, if that helps.

> 
> Thanks,
> 
> C. 
> 
> > 
> >> ---
> >>
> >>  If we want to increase the kernel size limit as commit b45b56baeecd
> >>  ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I
> >>  think we should add a machine option.
> >>
> >>  hw/ppc/pnv.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index 14fc9758a973..e500c2e2437e 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -65,9 +65,9 @@
> >>  #define FW_MAX_SIZE             (16 * MiB)
> >>
> >>  #define KERNEL_LOAD_ADDR        0x20000000
> >> -#define KERNEL_MAX_SIZE         (256 * MiB)
> >> -#define INITRD_LOAD_ADDR        0x60000000
> >> -#define INITRD_MAX_SIZE         (256 * MiB)
> >> +#define KERNEL_MAX_SIZE         (128 * MiB)
> >> +#define INITRD_LOAD_ADDR        0x28000000
> >> +#define INITRD_MAX_SIZE         (128 * MiB)
> >>
> >>  static const char *pnv_chip_core_typename(const PnvChip *o)
> >>  {
> >>
> >>
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/7] ppc/pnv: Use skiboot addresses to load kernel and ramfs

[Cédric Le Goater]

On 1/28/21 11:36 PM, David Gibson wrote:
> On Thu, Jan 28, 2021 at 08:02:41AM +0100, Cédric Le Goater wrote:
>> On 1/28/21 1:45 AM, Joel Stanley wrote:
>>> On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater <clg@kaod.org> wrote:
>>>>
>>>> The current settings are useful to load large kernels (with debug) but
>>>> it moves the initrd image in a memory region not protected by
>>>> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will
>>>> corrupt the initrd.
>>>>
>>>> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>
>>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>>>
>>> Note that the machine's default ram size will change with this patch:
>>>
>>>  mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
>>
>> Ah yes. I missed that.
>>
>>> So we will go from 1.75GB to 768MB. Does anything break when the
>>> machine has less than 1GB of ram?
>>
>> There is a warning if the machine has less than 1GB but we should
>> also change the default RAM size to 1G to be on the safe side.
> 
> I've merged the patch, but I'm happy to replace it with an updated
> version, or fold in a change, if that helps.

I will send a little fix to set the default RAM size of the machine.

Thanks,

C. 

Re: [PATCH 3/7] ppc/pnv: Use skiboot addresses to load kernel and ramfs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

On Tue, Jan 26, 2021 at 06:10:55PM +0100, Cédric Le Goater wrote:
> The current settings are useful to load large kernels (with debug) but
> it moves the initrd image in a memory region not protected by
> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will
> corrupt the initrd.
> 
> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-6.0, thanks.

> ---
> 
>  If we want to increase the kernel size limit as commit b45b56baeecd
>  ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I
>  think we should add a machine option.
> 
>  hw/ppc/pnv.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 14fc9758a973..e500c2e2437e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -65,9 +65,9 @@
>  #define FW_MAX_SIZE             (16 * MiB)
>  
>  #define KERNEL_LOAD_ADDR        0x20000000
> -#define KERNEL_MAX_SIZE         (256 * MiB)
> -#define INITRD_LOAD_ADDR        0x60000000
> -#define INITRD_MAX_SIZE         (256 * MiB)
> +#define KERNEL_MAX_SIZE         (128 * MiB)
> +#define INITRD_LOAD_ADDR        0x28000000
> +#define INITRD_MAX_SIZE         (128 * MiB)
>  
>  static const char *pnv_chip_core_typename(const PnvChip *o)
>  {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()

[Cédric Le Goater]

and reuse pnv_bmc_set_pnor() to share the setting of the PNOR.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv_bmc.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
index 67ebb16c4d5f..86d16b493539 100644
--- a/hw/ppc/pnv_bmc.c
+++ b/hw/ppc/pnv_bmc.c
@@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
     Object *obj;
 
     obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
-    object_ref(OBJECT(pnor));
-    object_property_add_const_link(obj, "pnor", OBJECT(pnor));
     qdev_realize(DEVICE(obj), NULL, &error_fatal);
-
-    /* Install the HIOMAP protocol handlers to access the PNOR */
-    ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM,
-                            &hiomap_netfn);
+    pnv_bmc_set_pnor(IPMI_BMC(obj), pnor);
 
     return IPMI_BMC(obj);
 }
-- 
2.26.2

Re: [PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()

[Joel Stanley]

On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote:
>
> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ppc/pnv_bmc.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
> index 67ebb16c4d5f..86d16b493539 100644
> --- a/hw/ppc/pnv_bmc.c
> +++ b/hw/ppc/pnv_bmc.c
> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
>      Object *obj;
>
>      obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
> -    object_ref(OBJECT(pnor));
> -    object_property_add_const_link(obj, "pnor", OBJECT(pnor));

I assume it's ok to move the link set to after the realise of the BMC object?

>      qdev_realize(DEVICE(obj), NULL, &error_fatal);
> -
> -    /* Install the HIOMAP protocol handlers to access the PNOR */
> -    ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM,
> -                            &hiomap_netfn);
> +    pnv_bmc_set_pnor(IPMI_BMC(obj), pnor);
>
>      return IPMI_BMC(obj);
>  }
> --
> 2.26.2
>
>

Re: [PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()

[Cédric Le Goater]

On 1/28/21 1:46 AM, Joel Stanley wrote:
> On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/ppc/pnv_bmc.c | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
>> index 67ebb16c4d5f..86d16b493539 100644
>> --- a/hw/ppc/pnv_bmc.c
>> +++ b/hw/ppc/pnv_bmc.c
>> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
>>      Object *obj;
>>
>>      obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
>> -    object_ref(OBJECT(pnor));
>> -    object_property_add_const_link(obj, "pnor", OBJECT(pnor));
> 
> I assume it's ok to move the link set to after the realise of the BMC object?
 

When 2 objects need to be linked, one has to be realized first. 
I suppose this is why it is allowed but I am not expert in that area. 

Greg  ?

That was the case already when defining a "ipmi-bmc-sim" device on the 
command line. 

C. 


>>      qdev_realize(DEVICE(obj), NULL, &error_fatal);
>> -
>> -    /* Install the HIOMAP protocol handlers to access the PNOR */
>> -    ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM,
>> -                            &hiomap_netfn);
>> +    pnv_bmc_set_pnor(IPMI_BMC(obj), pnor);
>>
>>      return IPMI_BMC(obj);
>>  }
>> --
>> 2.26.2
>>
>>

Re: [PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()

[Greg Kurz]

On Thu, 28 Jan 2021 08:46:01 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 1/28/21 1:46 AM, Joel Stanley wrote:
> > On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  hw/ppc/pnv_bmc.c | 7 +------
> >>  1 file changed, 1 insertion(+), 6 deletions(-)
> >>
> >> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
> >> index 67ebb16c4d5f..86d16b493539 100644
> >> --- a/hw/ppc/pnv_bmc.c
> >> +++ b/hw/ppc/pnv_bmc.c
> >> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
> >>      Object *obj;
> >>
> >>      obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
> >> -    object_ref(OBJECT(pnor));
> >> -    object_property_add_const_link(obj, "pnor", OBJECT(pnor));
> > 
> > I assume it's ok to move the link set to after the realise of the BMC object?
>  
> 
> When 2 objects need to be linked, one has to be realized first. 

Realize isn't a QOM concept in the first place...

> I suppose this is why it is allowed but I am not expert in that area. 
> 

... so no surprise object_property_add_const_link() doesn't care
about it.

> Greg  ?
> 

What is important though is that a property with a given name
can only be added *once* to an object during its lifetime.
Doing the contrary is a bug and QEMU aborts. So, with this
in mind, it seems to me that adding QOM properties to a
device object should only be done from some init path
that is only called once.

> That was the case already when defining a "ipmi-bmc-sim" device on the 
> command line. 
> 

Yeah and the property is added during machine reset... which
is typically a path that can be taken several times during
the machine lifetime. The potential crash is avoided because
pnv_reset() doesn't call pnv_bmc_set_pnor() if pnv->bmc is
already set, but this is a fragile workaround...

A QOM link doesn't look like the correct way to model the BMC
accesses to the PNOR. Since the only user is hiomap_cmd(),
it seems you could reach the same result with a pointer to
the PNOR object being passed to ipmi_sim_register_netfn()
and later passed to hiomap_cmd().

> C. 
> 
> 
> >>      qdev_realize(DEVICE(obj), NULL, &error_fatal);
> >> -
> >> -    /* Install the HIOMAP protocol handlers to access the PNOR */
> >> -    ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM,
> >> -                            &hiomap_netfn);
> >> +    pnv_bmc_set_pnor(IPMI_BMC(obj), pnor);
> >>
> >>      return IPMI_BMC(obj);
> >>  }
> >> --
> >> 2.26.2
> >>
> >>
> 

Re: [PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3204 bytes --]

On Thu, Jan 28, 2021 at 01:04:28PM +0100, Greg Kurz wrote:
> On Thu, 28 Jan 2021 08:46:01 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > On 1/28/21 1:46 AM, Joel Stanley wrote:
> > > On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote:
> > >>
> > >> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR.
> > >>
> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > >> ---
> > >>  hw/ppc/pnv_bmc.c | 7 +------
> > >>  1 file changed, 1 insertion(+), 6 deletions(-)
> > >>
> > >> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
> > >> index 67ebb16c4d5f..86d16b493539 100644
> > >> --- a/hw/ppc/pnv_bmc.c
> > >> +++ b/hw/ppc/pnv_bmc.c
> > >> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
> > >>      Object *obj;
> > >>
> > >>      obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
> > >> -    object_ref(OBJECT(pnor));
> > >> -    object_property_add_const_link(obj, "pnor", OBJECT(pnor));
> > > 
> > > I assume it's ok to move the link set to after the realise of the BMC object?
> >  
> > 
> > When 2 objects need to be linked, one has to be realized first. 
> 
> Realize isn't a QOM concept in the first place...
> 
> > I suppose this is why it is allowed but I am not expert in that area. 
> > 
> 
> ... so no surprise object_property_add_const_link() doesn't care
> about it.
> 
> > Greg  ?
> > 
> 
> What is important though is that a property with a given name
> can only be added *once* to an object during its lifetime.
> Doing the contrary is a bug and QEMU aborts. So, with this
> in mind, it seems to me that adding QOM properties to a
> device object should only be done from some init path
> that is only called once.
> 
> > That was the case already when defining a "ipmi-bmc-sim" device on the 
> > command line. 
> > 
> 
> Yeah and the property is added during machine reset... which
> is typically a path that can be taken several times during
> the machine lifetime. The potential crash is avoided because
> pnv_reset() doesn't call pnv_bmc_set_pnor() if pnv->bmc is
> already set, but this is a fragile workaround...

Oof, yeah.  In general we should avoid creating or realizing objects
at machine reset time.  There might be a few exceptions, but they're
rare.

> A QOM link doesn't look like the correct way to model the BMC
> accesses to the PNOR. Since the only user is hiomap_cmd(),
> it seems you could reach the same result with a pointer to
> the PNOR object being passed to ipmi_sim_register_netfn()
> and later passed to hiomap_cmd().
> 
> > C. 
> > 
> > 
> > >>      qdev_realize(DEVICE(obj), NULL, &error_fatal);
> > >> -
> > >> -    /* Install the HIOMAP protocol handlers to access the PNOR */
> > >> -    ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM,
> > >> -                            &hiomap_netfn);
> > >> +    pnv_bmc_set_pnor(IPMI_BMC(obj), pnor);
> > >>
> > >>      return IPMI_BMC(obj);
> > >>  }
> > >>
> > >>
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1986 bytes --]

On Thu, Jan 28, 2021 at 08:46:01AM +0100, Cédric Le Goater wrote:
> On 1/28/21 1:46 AM, Joel Stanley wrote:
> > On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  hw/ppc/pnv_bmc.c | 7 +------
> >>  1 file changed, 1 insertion(+), 6 deletions(-)
> >>
> >> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
> >> index 67ebb16c4d5f..86d16b493539 100644
> >> --- a/hw/ppc/pnv_bmc.c
> >> +++ b/hw/ppc/pnv_bmc.c
> >> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
> >>      Object *obj;
> >>
> >>      obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
> >> -    object_ref(OBJECT(pnor));
> >> -    object_property_add_const_link(obj, "pnor", OBJECT(pnor));
> > 
> > I assume it's ok to move the link set to after the realise of the BMC object?
>  
> 
> When 2 objects need to be linked, one has to be realized first. 
> I suppose this is why it is allowed but I am not expert in that area. 
> 
> Greg  ?
> 
> That was the case already when defining a "ipmi-bmc-sim" device on the 
> command line.

Well, the other thing here is that the IPMI_BMC_SIMULATOR isn't a
POWER specific object, and doesn't actually know anything about pnor,
so it never looks at that property.  Do we even need it?

> 
> C. 
> 
> 
> >>      qdev_realize(DEVICE(obj), NULL, &error_fatal);
> >> -
> >> -    /* Install the HIOMAP protocol handlers to access the PNOR */
> >> -    ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM,
> >> -                            &hiomap_netfn);
> >> +    pnv_bmc_set_pnor(IPMI_BMC(obj), pnor);
> >>
> >>      return IPMI_BMC(obj);
> >>  }
> >>
> >>
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()

[Cédric Le Goater]

On 1/28/21 11:40 PM, David Gibson wrote:
> On Thu, Jan 28, 2021 at 08:46:01AM +0100, Cédric Le Goater wrote:
>> On 1/28/21 1:46 AM, Joel Stanley wrote:
>>> On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote:
>>>>
>>>> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>  hw/ppc/pnv_bmc.c | 7 +------
>>>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
>>>> index 67ebb16c4d5f..86d16b493539 100644
>>>> --- a/hw/ppc/pnv_bmc.c
>>>> +++ b/hw/ppc/pnv_bmc.c
>>>> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
>>>>      Object *obj;
>>>>
>>>>      obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
>>>> -    object_ref(OBJECT(pnor));
>>>> -    object_property_add_const_link(obj, "pnor", OBJECT(pnor));
>>>
>>> I assume it's ok to move the link set to after the realise of the BMC object?
>>  
>>
>> When 2 objects need to be linked, one has to be realized first. 
>> I suppose this is why it is allowed but I am not expert in that area. 
>>
>> Greg  ?
>>
>> That was the case already when defining a "ipmi-bmc-sim" device on the 
>> command line.
> 
> Well, the other thing here is that the IPMI_BMC_SIMULATOR isn't a
> POWER specific object, and doesn't actually know anything about pnor,
> so it never looks at that property.  Do we even need it?

It does through hiomap_cmd() which handles HIOMAP commands related 
to the PNOR. The link was introduced to remove a reference to the 
global machine (qdev_get_machine()). The PNOR device is instantiated 
at the machine level but conceptually, this is incorrect. 

The PNOR is a device controlled by the BMC and accessed by the host 
through a mapping on the LPC FW address space. It used to be controlled 
from the host also, through the iLPC2AHB device and mboxes, but these 
"doors" were closed sometime ago.

I am thinking of moving the PNOR at the BMC level. It won't change 
the default device settings but '-nodefaults' will result in no PNOR, 
same impact if the BMC device is an external one, but that's a more 
complex matter. We would need a way to model memory operations on a 
LPC bus shared by two QEMU machines.

We are doing something similar with the Aspeed iBT device but it's
very specific to this device. I hope the QEMU multi-process patchset
offers some framework on which we can build upon.

C.

Re: [PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()

[Andrew Jeffery]

On Fri, 29 Jan 2021, at 19:09, Cédric Le Goater wrote:
> On 1/28/21 11:40 PM, David Gibson wrote:
> > On Thu, Jan 28, 2021 at 08:46:01AM +0100, Cédric Le Goater wrote:
> >> On 1/28/21 1:46 AM, Joel Stanley wrote:
> >>> On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote:
> >>>>
> >>>> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR.
> >>>>
> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>>> ---
> >>>>  hw/ppc/pnv_bmc.c | 7 +------
> >>>>  1 file changed, 1 insertion(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
> >>>> index 67ebb16c4d5f..86d16b493539 100644
> >>>> --- a/hw/ppc/pnv_bmc.c
> >>>> +++ b/hw/ppc/pnv_bmc.c
> >>>> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
> >>>>      Object *obj;
> >>>>
> >>>>      obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
> >>>> -    object_ref(OBJECT(pnor));
> >>>> -    object_property_add_const_link(obj, "pnor", OBJECT(pnor));
> >>>
> >>> I assume it's ok to move the link set to after the realise of the BMC object?
> >>  
> >>
> >> When 2 objects need to be linked, one has to be realized first. 
> >> I suppose this is why it is allowed but I am not expert in that area. 
> >>
> >> Greg  ?
> >>
> >> That was the case already when defining a "ipmi-bmc-sim" device on the 
> >> command line.
> > 
> > Well, the other thing here is that the IPMI_BMC_SIMULATOR isn't a
> > POWER specific object, and doesn't actually know anything about pnor,
> > so it never looks at that property.  Do we even need it?
> 
> It does through hiomap_cmd() which handles HIOMAP commands related 
> to the PNOR. The link was introduced to remove a reference to the 
> global machine (qdev_get_machine()). The PNOR device is instantiated 
> at the machine level but conceptually, this is incorrect. 
> 
> The PNOR is a device controlled by the BMC and accessed by the host 
> through a mapping on the LPC FW address space. It used to be controlled 
> from the host also, through the iLPC2AHB device and mboxes, but these 
> "doors" were closed sometime ago.

Right, so rehashing what Cédric said about the context for the PNOR and IPMI:

On PowerNV platforms, the host firmware accesses the data in the PNOR by 
sending commands over IPMI to the BMC to change the mappings in the LPC FW 
space. HIOMAP (Host I/O Map)[1] is the name of the little spec/protocol that 
defines the layout of data in the FW space and the commands and ABI for 
manipulating the mappings.

[1] https://github.com/openbmc/hiomapd/blob/master/Documentation/protocol.md

It's all terrible, but IPMI was the most well-trodden path I had at my disposal 
for improving the security of the (Aspeed) BMCs' hardware configuration for 
Power platform designs. From BMC reset the host has unrestricted access to the 
BMC's AHB via bridges on the LPC and PCIe buses until the BMC firmware disables 
them. Leaving the bridges enabled is not great for security or stability of the 
BMC firmware, so this meant cooking up some magic to allow the host to write 
back to the PNOR (owned by the BMC as Cédric mentions above) without exposing 
the BMC in unacceptable ways.

Andrew

Re: [PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]

On Tue, Jan 26, 2021 at 06:10:56PM +0100, Cédric Le Goater wrote:
> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/pnv_bmc.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
> index 67ebb16c4d5f..86d16b493539 100644
> --- a/hw/ppc/pnv_bmc.c
> +++ b/hw/ppc/pnv_bmc.c
> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
>      Object *obj;
>  
>      obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
> -    object_ref(OBJECT(pnor));
> -    object_property_add_const_link(obj, "pnor", OBJECT(pnor));
>      qdev_realize(DEVICE(obj), NULL, &error_fatal);
> -
> -    /* Install the HIOMAP protocol handlers to access the PNOR */
> -    ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM,
> -                            &hiomap_netfn);
> +    pnv_bmc_set_pnor(IPMI_BMC(obj), pnor);
>  
>      return IPMI_BMC(obj);
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 5/7] ppc/pnv: Discard internal BMC initialization when BMC is external

[Cédric Le Goater]

The PowerNV machine can be run with an external IPMI BMC device
connected to a remote QEMU machine acting as BMC, using these options :

  -chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
  -device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
  -device isa-ipmi-bt,bmc=bmc0,irq=10 \
  -nodefaults

In that case, some aspects of the BMC initialization should be
skipped, since they rely on the simulator interface.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv_bmc.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
index 86d16b493539..b9bf5735ea0f 100644
--- a/hw/ppc/pnv_bmc.c
+++ b/hw/ppc/pnv_bmc.c
@@ -51,6 +51,11 @@ typedef struct OemSel {
 #define SOFT_OFF        0x00
 #define SOFT_REBOOT     0x01
 
+static bool pnv_bmc_is_simulator(IPMIBmc *bmc)
+{
+    return object_dynamic_cast(OBJECT(bmc), TYPE_IPMI_BMC_SIMULATOR);
+}
+
 static void pnv_gen_oem_sel(IPMIBmc *bmc, uint8_t reboot)
 {
     /* IPMI SEL Event are 16 bytes long */
@@ -79,6 +84,10 @@ void pnv_dt_bmc_sensors(IPMIBmc *bmc, void *fdt)
     const struct ipmi_sdr_compact *sdr;
     uint16_t nextrec;
 
+    if (!pnv_bmc_is_simulator(bmc)) {
+        return;
+    }
+
     offset = fdt_add_subnode(fdt, 0, "bmc");
     _FDT(offset);
 
@@ -243,6 +252,10 @@ static const IPMINetfn hiomap_netfn = {
 
 void pnv_bmc_set_pnor(IPMIBmc *bmc, PnvPnor *pnor)
 {
+    if (!pnv_bmc_is_simulator(bmc)) {
+        return;
+    }
+
     object_ref(OBJECT(pnor));
     object_property_add_const_link(OBJECT(bmc), "pnor", OBJECT(pnor));
 
@@ -286,7 +299,7 @@ static int bmc_find(Object *child, void *opaque)
 
 IPMIBmc *pnv_bmc_find(Error **errp)
 {
-    ForeachArgs args = { TYPE_IPMI_BMC_SIMULATOR, NULL };
+    ForeachArgs args = { TYPE_IPMI_BMC, NULL };
     int ret;
 
     ret = object_child_foreach_recursive(object_get_root(), bmc_find, &args);
-- 
2.26.2

Re: [PATCH 5/7] ppc/pnv: Discard internal BMC initialization when BMC is external

[Joel Stanley]

On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater <clg@kaod.org> wrote:
>
> The PowerNV machine can be run with an external IPMI BMC device
> connected to a remote QEMU machine acting as BMC, using these options :
>
>   -chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
>   -device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
>   -device isa-ipmi-bt,bmc=bmc0,irq=10 \
>   -nodefaults

Should this information also go in docs/system/ppc similar to the
descriptions we have in docs/system/arm?

>
> In that case, some aspects of the BMC initialization should be
> skipped, since they rely on the simulator interface.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>


> ---
>  hw/ppc/pnv_bmc.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
> index 86d16b493539..b9bf5735ea0f 100644
> --- a/hw/ppc/pnv_bmc.c
> +++ b/hw/ppc/pnv_bmc.c
> @@ -51,6 +51,11 @@ typedef struct OemSel {
>  #define SOFT_OFF        0x00
>  #define SOFT_REBOOT     0x01
>
> +static bool pnv_bmc_is_simulator(IPMIBmc *bmc)
> +{
> +    return object_dynamic_cast(OBJECT(bmc), TYPE_IPMI_BMC_SIMULATOR);
> +}
> +
>  static void pnv_gen_oem_sel(IPMIBmc *bmc, uint8_t reboot)
>  {
>      /* IPMI SEL Event are 16 bytes long */
> @@ -79,6 +84,10 @@ void pnv_dt_bmc_sensors(IPMIBmc *bmc, void *fdt)
>      const struct ipmi_sdr_compact *sdr;
>      uint16_t nextrec;
>
> +    if (!pnv_bmc_is_simulator(bmc)) {
> +        return;
> +    }
> +
>      offset = fdt_add_subnode(fdt, 0, "bmc");
>      _FDT(offset);
>
> @@ -243,6 +252,10 @@ static const IPMINetfn hiomap_netfn = {
>
>  void pnv_bmc_set_pnor(IPMIBmc *bmc, PnvPnor *pnor)
>  {
> +    if (!pnv_bmc_is_simulator(bmc)) {
> +        return;
> +    }
> +
>      object_ref(OBJECT(pnor));
>      object_property_add_const_link(OBJECT(bmc), "pnor", OBJECT(pnor));
>
> @@ -286,7 +299,7 @@ static int bmc_find(Object *child, void *opaque)
>
>  IPMIBmc *pnv_bmc_find(Error **errp)
>  {
> -    ForeachArgs args = { TYPE_IPMI_BMC_SIMULATOR, NULL };
> +    ForeachArgs args = { TYPE_IPMI_BMC, NULL };
>      int ret;
>
>      ret = object_child_foreach_recursive(object_get_root(), bmc_find, &args);
> --
> 2.26.2
>
>

Re: [PATCH 5/7] ppc/pnv: Discard internal BMC initialization when BMC is external

[Cédric Le Goater]

On 1/28/21 1:48 AM, Joel Stanley wrote:
> On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> The PowerNV machine can be run with an external IPMI BMC device
>> connected to a remote QEMU machine acting as BMC, using these options :
>>
>>   -chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
>>   -device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
>>   -device isa-ipmi-bt,bmc=bmc0,irq=10 \
>>   -nodefaults
> 
> Should this information also go in docs/system/ppc similar to the
> descriptions we have in docs/system/arm?

yes. 

Do you think we could reference :

    https://openpower.xyz/job/openpower/job/openpower-op-build/

?

Thanks,

C. 
 
>>
>> In that case, some aspects of the BMC initialization should be
>> skipped, since they rely on the simulator interface.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
> 
>> ---
>>  hw/ppc/pnv_bmc.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
>> index 86d16b493539..b9bf5735ea0f 100644
>> --- a/hw/ppc/pnv_bmc.c
>> +++ b/hw/ppc/pnv_bmc.c
>> @@ -51,6 +51,11 @@ typedef struct OemSel {
>>  #define SOFT_OFF        0x00
>>  #define SOFT_REBOOT     0x01
>>
>> +static bool pnv_bmc_is_simulator(IPMIBmc *bmc)
>> +{
>> +    return object_dynamic_cast(OBJECT(bmc), TYPE_IPMI_BMC_SIMULATOR);
>> +}
>> +
>>  static void pnv_gen_oem_sel(IPMIBmc *bmc, uint8_t reboot)
>>  {
>>      /* IPMI SEL Event are 16 bytes long */
>> @@ -79,6 +84,10 @@ void pnv_dt_bmc_sensors(IPMIBmc *bmc, void *fdt)
>>      const struct ipmi_sdr_compact *sdr;
>>      uint16_t nextrec;
>>
>> +    if (!pnv_bmc_is_simulator(bmc)) {
>> +        return;
>> +    }
>> +
>>      offset = fdt_add_subnode(fdt, 0, "bmc");
>>      _FDT(offset);
>>
>> @@ -243,6 +252,10 @@ static const IPMINetfn hiomap_netfn = {
>>
>>  void pnv_bmc_set_pnor(IPMIBmc *bmc, PnvPnor *pnor)
>>  {
>> +    if (!pnv_bmc_is_simulator(bmc)) {
>> +        return;
>> +    }
>> +
>>      object_ref(OBJECT(pnor));
>>      object_property_add_const_link(OBJECT(bmc), "pnor", OBJECT(pnor));
>>
>> @@ -286,7 +299,7 @@ static int bmc_find(Object *child, void *opaque)
>>
>>  IPMIBmc *pnv_bmc_find(Error **errp)
>>  {
>> -    ForeachArgs args = { TYPE_IPMI_BMC_SIMULATOR, NULL };
>> +    ForeachArgs args = { TYPE_IPMI_BMC, NULL };
>>      int ret;
>>
>>      ret = object_child_foreach_recursive(object_get_root(), bmc_find, &args);
>> --
>> 2.26.2
>>
>>

Re: [PATCH 5/7] ppc/pnv: Discard internal BMC initialization when BMC is external

[Joel Stanley]

On Thu, 28 Jan 2021 at 07:13, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 1/28/21 1:48 AM, Joel Stanley wrote:
> > On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >> The PowerNV machine can be run with an external IPMI BMC device
> >> connected to a remote QEMU machine acting as BMC, using these options :
> >>
> >>   -chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
> >>   -device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
> >>   -device isa-ipmi-bt,bmc=bmc0,irq=10 \
> >>   -nodefaults
> >
> > Should this information also go in docs/system/ppc similar to the
> > descriptions we have in docs/system/arm?
>
> yes.
>
> Do you think we could reference :
>
>     https://openpower.xyz/job/openpower/job/openpower-op-build/

I'm not sure how long into the future it will stay up. Hosting some
images somewhere else would be ideal.

Perhaps include that URL for now, and we can update it once we have a
better location.

Cheers,

Joel

Re: [PATCH 5/7] ppc/pnv: Discard internal BMC initialization when BMC is external

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2396 bytes --]

On Tue, Jan 26, 2021 at 06:10:57PM +0100, Cédric Le Goater wrote:
> The PowerNV machine can be run with an external IPMI BMC device
> connected to a remote QEMU machine acting as BMC, using these options :
> 
>   -chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
>   -device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
>   -device isa-ipmi-bt,bmc=bmc0,irq=10 \
>   -nodefaults
> 
> In that case, some aspects of the BMC initialization should be
> skipped, since they rely on the simulator interface.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/pnv_bmc.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
> index 86d16b493539..b9bf5735ea0f 100644
> --- a/hw/ppc/pnv_bmc.c
> +++ b/hw/ppc/pnv_bmc.c
> @@ -51,6 +51,11 @@ typedef struct OemSel {
>  #define SOFT_OFF        0x00
>  #define SOFT_REBOOT     0x01
>  
> +static bool pnv_bmc_is_simulator(IPMIBmc *bmc)
> +{
> +    return object_dynamic_cast(OBJECT(bmc), TYPE_IPMI_BMC_SIMULATOR);
> +}
> +
>  static void pnv_gen_oem_sel(IPMIBmc *bmc, uint8_t reboot)
>  {
>      /* IPMI SEL Event are 16 bytes long */
> @@ -79,6 +84,10 @@ void pnv_dt_bmc_sensors(IPMIBmc *bmc, void *fdt)
>      const struct ipmi_sdr_compact *sdr;
>      uint16_t nextrec;
>  
> +    if (!pnv_bmc_is_simulator(bmc)) {
> +        return;
> +    }
> +
>      offset = fdt_add_subnode(fdt, 0, "bmc");
>      _FDT(offset);
>  
> @@ -243,6 +252,10 @@ static const IPMINetfn hiomap_netfn = {
>  
>  void pnv_bmc_set_pnor(IPMIBmc *bmc, PnvPnor *pnor)
>  {
> +    if (!pnv_bmc_is_simulator(bmc)) {
> +        return;
> +    }
> +
>      object_ref(OBJECT(pnor));
>      object_property_add_const_link(OBJECT(bmc), "pnor", OBJECT(pnor));
>  
> @@ -286,7 +299,7 @@ static int bmc_find(Object *child, void *opaque)
>  
>  IPMIBmc *pnv_bmc_find(Error **errp)
>  {
> -    ForeachArgs args = { TYPE_IPMI_BMC_SIMULATOR, NULL };
> +    ForeachArgs args = { TYPE_IPMI_BMC, NULL };
>      int ret;
>  
>      ret = object_child_foreach_recursive(object_get_root(), bmc_find, &args);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 6/7] ppc/pnv: Remove default disablement of the PNOR contents

[Cédric Le Goater]

On PowerNV systems, the BMC is in charge of mapping the PNOR contents
on the LPC FW address space using the HIOMAP protocol. Under QEMU, we
emulate this behavior and we also add an extra control on the flash
accesses by letting the HIOMAP command handler decide whether the
memory region is accessible or not depending on the firmware requests.

However, this behavior is not compatible with hostboot like firmwares
which need this mapping to be always available. For this reason, the
PNOR memory region is initially disabled for skiboot mode only.

This is badly placed under the LPC model and requires the use of the
machine. Since it doesn't add much, simply remove the initial setting.
The extra control in the HIOMAP command handler will still be performed.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv_lpc.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index 590359022084..11739e397b27 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -825,7 +825,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp)
     qemu_irq *irqs;
     qemu_irq_handler handler;
     PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
-    bool hostboot_mode = !!pnv->fw_load_addr;
 
     /* let isa_bus_new() create its own bridge on SysBus otherwise
      * devices specified on the command line won't find the bus and
@@ -856,13 +855,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp)
      */
     memory_region_add_subregion(&lpc->isa_fw, PNOR_SPI_OFFSET,
                                 &pnv->pnor->mmio);
-    /*
-     * Start disabled. The HIOMAP protocol will activate the mapping
-     * with HIOMAP_C_CREATE_WRITE_WINDOW
-     */
-    if (!hostboot_mode) {
-        memory_region_set_enabled(&pnv->pnor->mmio, false);
-    }
 
     return isa_bus;
 }
-- 
2.26.2

Re: [PATCH 6/7] ppc/pnv: Remove default disablement of the PNOR contents

[Joel Stanley]

On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater <clg@kaod.org> wrote:
>
> On PowerNV systems, the BMC is in charge of mapping the PNOR contents
> on the LPC FW address space using the HIOMAP protocol. Under QEMU, we
> emulate this behavior and we also add an extra control on the flash
> accesses by letting the HIOMAP command handler decide whether the
> memory region is accessible or not depending on the firmware requests.
>
> However, this behavior is not compatible with hostboot like firmwares
> which need this mapping to be always available. For this reason, the
> PNOR memory region is initially disabled for skiboot mode only.
>
> This is badly placed under the LPC model and requires the use of the
> machine. Since it doesn't add much, simply remove the initial setting.
> The extra control in the HIOMAP command handler will still be performed.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/ppc/pnv_lpc.c | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 590359022084..11739e397b27 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -825,7 +825,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp)
>      qemu_irq *irqs;
>      qemu_irq_handler handler;
>      PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> -    bool hostboot_mode = !!pnv->fw_load_addr;
>
>      /* let isa_bus_new() create its own bridge on SysBus otherwise
>       * devices specified on the command line won't find the bus and
> @@ -856,13 +855,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp)
>       */
>      memory_region_add_subregion(&lpc->isa_fw, PNOR_SPI_OFFSET,
>                                  &pnv->pnor->mmio);
> -    /*
> -     * Start disabled. The HIOMAP protocol will activate the mapping
> -     * with HIOMAP_C_CREATE_WRITE_WINDOW
> -     */
> -    if (!hostboot_mode) {
> -        memory_region_set_enabled(&pnv->pnor->mmio, false);
> -    }
>
>      return isa_bus;
>  }
> --
> 2.26.2
>
>

Re: [PATCH 6/7] ppc/pnv: Remove default disablement of the PNOR contents

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]

On Tue, Jan 26, 2021 at 06:10:58PM +0100, Cédric Le Goater wrote:
> On PowerNV systems, the BMC is in charge of mapping the PNOR contents
> on the LPC FW address space using the HIOMAP protocol. Under QEMU, we
> emulate this behavior and we also add an extra control on the flash
> accesses by letting the HIOMAP command handler decide whether the
> memory region is accessible or not depending on the firmware requests.
> 
> However, this behavior is not compatible with hostboot like firmwares
> which need this mapping to be always available. For this reason, the
> PNOR memory region is initially disabled for skiboot mode only.
> 
> This is badly placed under the LPC model and requires the use of the
> machine. Since it doesn't add much, simply remove the initial setting.
> The extra control in the HIOMAP command handler will still be performed.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/pnv_lpc.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 590359022084..11739e397b27 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -825,7 +825,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp)
>      qemu_irq *irqs;
>      qemu_irq_handler handler;
>      PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> -    bool hostboot_mode = !!pnv->fw_load_addr;
>  
>      /* let isa_bus_new() create its own bridge on SysBus otherwise
>       * devices specified on the command line won't find the bus and
> @@ -856,13 +855,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp)
>       */
>      memory_region_add_subregion(&lpc->isa_fw, PNOR_SPI_OFFSET,
>                                  &pnv->pnor->mmio);
> -    /*
> -     * Start disabled. The HIOMAP protocol will activate the mapping
> -     * with HIOMAP_C_CREATE_WRITE_WINDOW
> -     */
> -    if (!hostboot_mode) {
> -        memory_region_set_enabled(&pnv->pnor->mmio, false);
> -    }
>  
>      return isa_bus;
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 7/7] ppc/pnv: Introduce a LPC FW memory region attribute to map the PNOR

[Cédric Le Goater]

This to map the PNOR from the machine init handler directly and finish
the cleanup of the LPC model.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/pnv.h |  1 +
 hw/ppc/pnv.c         | 11 +++++++++++
 hw/ppc/pnv_lpc.c     |  7 -------
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index ee7eda3e0102..d69cee17b232 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -58,6 +58,7 @@ struct PnvChip {
     MemoryRegion xscom;
     AddressSpace xscom_as;
 
+    MemoryRegion *fw_mr;
     gchar        *dt_isa_nodename;
 };
 
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index e500c2e2437e..50810df83815 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -871,6 +871,14 @@ static void pnv_init(MachineState *machine)
         pnv_ipmi_bt_init(pnv->isa_bus, pnv->bmc, 10);
     }
 
+    /*
+     * The PNOR is mapped on the LPC FW address space by the BMC.
+     * Since we can not reach the remote BMC machine with LPC memops,
+     * map it always for now.
+     */
+    memory_region_add_subregion(pnv->chips[0]->fw_mr, PNOR_SPI_OFFSET,
+                                &pnv->pnor->mmio);
+
     /*
      * OpenPOWER systems use a IPMI SEL Event message to notify the
      * host to powerdown
@@ -1150,6 +1158,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
     qdev_realize(DEVICE(&chip8->lpc), NULL, &error_fatal);
     pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip8->lpc.xscom_regs);
 
+    chip->fw_mr = &chip8->lpc.isa_fw;
     chip->dt_isa_nodename = g_strdup_printf("/xscom@%" PRIx64 "/isa@%x",
                                             (uint64_t) PNV_XSCOM_BASE(chip),
                                             PNV_XSCOM_LPC_BASE);
@@ -1479,6 +1488,7 @@ static void pnv_chip_power9_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(get_system_memory(), PNV9_LPCM_BASE(chip),
                                 &chip9->lpc.xscom_regs);
 
+    chip->fw_mr = &chip9->lpc.isa_fw;
     chip->dt_isa_nodename = g_strdup_printf("/lpcm-opb@%" PRIx64 "/lpc@0",
                                             (uint64_t) PNV9_LPCM_BASE(chip));
 
@@ -1592,6 +1602,7 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(get_system_memory(), PNV10_LPCM_BASE(chip),
                                 &chip10->lpc.xscom_regs);
 
+    chip->fw_mr = &chip10->lpc.isa_fw;
     chip->dt_isa_nodename = g_strdup_printf("/lpcm-opb@%" PRIx64 "/lpc@0",
                                             (uint64_t) PNV10_LPCM_BASE(chip));
 }
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index 11739e397b27..bcbca3db9743 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -824,7 +824,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp)
     ISABus *isa_bus;
     qemu_irq *irqs;
     qemu_irq_handler handler;
-    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
 
     /* let isa_bus_new() create its own bridge on SysBus otherwise
      * devices specified on the command line won't find the bus and
@@ -850,11 +849,5 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp)
 
     isa_bus_irqs(isa_bus, irqs);
 
-    /*
-     * TODO: Map PNOR on the LPC FW address space on demand ?
-     */
-    memory_region_add_subregion(&lpc->isa_fw, PNOR_SPI_OFFSET,
-                                &pnv->pnor->mmio);
-
     return isa_bus;
 }
-- 
2.26.2

Re: [PATCH 7/7] ppc/pnv: Introduce a LPC FW memory region attribute to map the PNOR

[Joel Stanley]

On Tue, 26 Jan 2021 at 17:19, Cédric Le Goater <clg@kaod.org> wrote:
>
> This to map the PNOR from the machine init handler directly and finish
> the cleanup of the LPC model.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  include/hw/ppc/pnv.h |  1 +
>  hw/ppc/pnv.c         | 11 +++++++++++
>  hw/ppc/pnv_lpc.c     |  7 -------
>  3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index ee7eda3e0102..d69cee17b232 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -58,6 +58,7 @@ struct PnvChip {
>      MemoryRegion xscom;
>      AddressSpace xscom_as;
>
> +    MemoryRegion *fw_mr;
>      gchar        *dt_isa_nodename;
>  };
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index e500c2e2437e..50810df83815 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -871,6 +871,14 @@ static void pnv_init(MachineState *machine)
>          pnv_ipmi_bt_init(pnv->isa_bus, pnv->bmc, 10);
>      }
>
> +    /*
> +     * The PNOR is mapped on the LPC FW address space by the BMC.
> +     * Since we can not reach the remote BMC machine with LPC memops,
> +     * map it always for now.
> +     */
> +    memory_region_add_subregion(pnv->chips[0]->fw_mr, PNOR_SPI_OFFSET,
> +                                &pnv->pnor->mmio);
> +
>      /*
>       * OpenPOWER systems use a IPMI SEL Event message to notify the
>       * host to powerdown
> @@ -1150,6 +1158,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>      qdev_realize(DEVICE(&chip8->lpc), NULL, &error_fatal);
>      pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip8->lpc.xscom_regs);
>
> +    chip->fw_mr = &chip8->lpc.isa_fw;
>      chip->dt_isa_nodename = g_strdup_printf("/xscom@%" PRIx64 "/isa@%x",
>                                              (uint64_t) PNV_XSCOM_BASE(chip),
>                                              PNV_XSCOM_LPC_BASE);
> @@ -1479,6 +1488,7 @@ static void pnv_chip_power9_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(get_system_memory(), PNV9_LPCM_BASE(chip),
>                                  &chip9->lpc.xscom_regs);
>
> +    chip->fw_mr = &chip9->lpc.isa_fw;
>      chip->dt_isa_nodename = g_strdup_printf("/lpcm-opb@%" PRIx64 "/lpc@0",
>                                              (uint64_t) PNV9_LPCM_BASE(chip));
>
> @@ -1592,6 +1602,7 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(get_system_memory(), PNV10_LPCM_BASE(chip),
>                                  &chip10->lpc.xscom_regs);
>
> +    chip->fw_mr = &chip10->lpc.isa_fw;
>      chip->dt_isa_nodename = g_strdup_printf("/lpcm-opb@%" PRIx64 "/lpc@0",
>                                              (uint64_t) PNV10_LPCM_BASE(chip));
>  }
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 11739e397b27..bcbca3db9743 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -824,7 +824,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp)
>      ISABus *isa_bus;
>      qemu_irq *irqs;
>      qemu_irq_handler handler;
> -    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>
>      /* let isa_bus_new() create its own bridge on SysBus otherwise
>       * devices specified on the command line won't find the bus and
> @@ -850,11 +849,5 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp)
>
>      isa_bus_irqs(isa_bus, irqs);
>
> -    /*
> -     * TODO: Map PNOR on the LPC FW address space on demand ?
> -     */
> -    memory_region_add_subregion(&lpc->isa_fw, PNOR_SPI_OFFSET,
> -                                &pnv->pnor->mmio);
> -
>      return isa_bus;
>  }
> --
> 2.26.2
>
>

Re: [PATCH 7/7] ppc/pnv: Introduce a LPC FW memory region attribute to map the PNOR

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4054 bytes --]

On Tue, Jan 26, 2021 at 06:10:59PM +0100, Cédric Le Goater wrote:
> This to map the PNOR from the machine init handler directly and finish
> the cleanup of the LPC model.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-6.0, thanks.

> ---
>  include/hw/ppc/pnv.h |  1 +
>  hw/ppc/pnv.c         | 11 +++++++++++
>  hw/ppc/pnv_lpc.c     |  7 -------
>  3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index ee7eda3e0102..d69cee17b232 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -58,6 +58,7 @@ struct PnvChip {
>      MemoryRegion xscom;
>      AddressSpace xscom_as;
>  
> +    MemoryRegion *fw_mr;
>      gchar        *dt_isa_nodename;
>  };
>  
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index e500c2e2437e..50810df83815 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -871,6 +871,14 @@ static void pnv_init(MachineState *machine)
>          pnv_ipmi_bt_init(pnv->isa_bus, pnv->bmc, 10);
>      }
>  
> +    /*
> +     * The PNOR is mapped on the LPC FW address space by the BMC.
> +     * Since we can not reach the remote BMC machine with LPC memops,
> +     * map it always for now.
> +     */
> +    memory_region_add_subregion(pnv->chips[0]->fw_mr, PNOR_SPI_OFFSET,
> +                                &pnv->pnor->mmio);
> +
>      /*
>       * OpenPOWER systems use a IPMI SEL Event message to notify the
>       * host to powerdown
> @@ -1150,6 +1158,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>      qdev_realize(DEVICE(&chip8->lpc), NULL, &error_fatal);
>      pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip8->lpc.xscom_regs);
>  
> +    chip->fw_mr = &chip8->lpc.isa_fw;
>      chip->dt_isa_nodename = g_strdup_printf("/xscom@%" PRIx64 "/isa@%x",
>                                              (uint64_t) PNV_XSCOM_BASE(chip),
>                                              PNV_XSCOM_LPC_BASE);
> @@ -1479,6 +1488,7 @@ static void pnv_chip_power9_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(get_system_memory(), PNV9_LPCM_BASE(chip),
>                                  &chip9->lpc.xscom_regs);
>  
> +    chip->fw_mr = &chip9->lpc.isa_fw;
>      chip->dt_isa_nodename = g_strdup_printf("/lpcm-opb@%" PRIx64 "/lpc@0",
>                                              (uint64_t) PNV9_LPCM_BASE(chip));
>  
> @@ -1592,6 +1602,7 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(get_system_memory(), PNV10_LPCM_BASE(chip),
>                                  &chip10->lpc.xscom_regs);
>  
> +    chip->fw_mr = &chip10->lpc.isa_fw;
>      chip->dt_isa_nodename = g_strdup_printf("/lpcm-opb@%" PRIx64 "/lpc@0",
>                                              (uint64_t) PNV10_LPCM_BASE(chip));
>  }
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 11739e397b27..bcbca3db9743 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -824,7 +824,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp)
>      ISABus *isa_bus;
>      qemu_irq *irqs;
>      qemu_irq_handler handler;
> -    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>  
>      /* let isa_bus_new() create its own bridge on SysBus otherwise
>       * devices specified on the command line won't find the bus and
> @@ -850,11 +849,5 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp)
>  
>      isa_bus_irqs(isa_bus, irqs);
>  
> -    /*
> -     * TODO: Map PNOR on the LPC FW address space on demand ?
> -     */
> -    memory_region_add_subregion(&lpc->isa_fw, PNOR_SPI_OFFSET,
> -                                &pnv->pnor->mmio);
> -
>      return isa_bus;
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]