Re: [PULL 17/96] ppc/pnv: Implement ADU access to LPC space

[Cédric Le Goater]

On 7/26/24 01:52, Nicholas Piggin wrote:
> One of the functions of the ADU is indirect memory access engines that
> send and receive data via ADU registers.
>
> This implements the ADU LPC memory access functionality sufficiently
> for IBM proprietary firmware to access the UART and print characters
> to the serial port as it does on real hardware.
>
> This requires a linkage between adu and lpc, which allows adu to
> perform memory access in the lpc space.
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/ppc/pnv.c             |  4 ++
>   hw/ppc/pnv_adu.c         | 95 ++++++++++++++++++++++++++++++++++++++++
>   hw/ppc/pnv_lpc.c         | 12 ++---
>   include/hw/ppc/pnv_adu.h |  7 +++
>   include/hw/ppc/pnv_lpc.h |  5 +++
>   5 files changed, 117 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 689197cbb7..91ff1be21f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1663,6 +1663,8 @@ static void pnv_chip_power9_realize(DeviceState *dev, 
> Error **errp)
>       }
>   
>       /* ADU */
> +    object_property_set_link(OBJECT(&chip9->adu), "lpc", OBJECT(&chip9->lpc),
> +                             &error_abort);
>       if (!qdev_realize(DEVICE(&chip9->adu), NULL, errp)) {
>           return;
>       }
> @@ -1929,6 +1931,8 @@ static void pnv_chip_power10_realize(DeviceState *dev, 
> Error **errp)
>       }
>   
>       /* ADU */
> +    object_property_set_link(OBJECT(&chip10->adu), "lpc", OBJECT(&chip10->lpc),
> +                             &error_abort);
>       if (!qdev_realize(DEVICE(&chip10->adu), NULL, errp)) {
>           return;
>       }
> diff --git a/hw/ppc/pnv_adu.c b/hw/ppc/pnv_adu.c
> index 8279bc8b26..81b7d6e526 100644
> --- a/hw/ppc/pnv_adu.c
> +++ b/hw/ppc/pnv_adu.c
> @@ -21,11 +21,18 @@
>   #include "hw/ppc/pnv.h"
>   #include "hw/ppc/pnv_adu.h"
>   #include "hw/ppc/pnv_chip.h"
> +#include "hw/ppc/pnv_lpc.h"
>   #include "hw/ppc/pnv_xscom.h"
>   #include "trace.h"
>   
> +#define ADU_LPC_BASE_REG     0x40
> +#define ADU_LPC_CMD_REG      0x41
> +#define ADU_LPC_DATA_REG     0x42
> +#define ADU_LPC_STATUS_REG   0x43
> +
>   static uint64_t pnv_adu_xscom_read(void *opaque, hwaddr addr, unsigned width)
>   {
> +    PnvADU *adu = PNV_ADU(opaque);
>       uint32_t offset = addr >> 3;
>       uint64_t val = 0;
>   
> @@ -34,6 +41,24 @@ static uint64_t pnv_adu_xscom_read(void *opaque, hwaddr addr, unsigned width)
>       case 0x12:     /* log register */
>       case 0x13:     /* error register */
>           break;
> +    case ADU_LPC_BASE_REG:
> +        /*
> +         * LPC Address Map in Pervasive ADU Workbook
> +         *
> +         * return PNV10_LPCM_BASE(chip) & PPC_BITMASK(8, 31);
> +         * XXX: implement as class property, or get from LPC?
> +         */
> +        qemu_log_mask(LOG_UNIMP, "ADU: LPC_BASE_REG is not implemented\n");
> +        break;
> +    case ADU_LPC_CMD_REG:
> +        val = adu->lpc_cmd_reg;
> +        break;
> +    case ADU_LPC_DATA_REG:
> +        val = adu->lpc_data_reg;
> +        break;
> +    case ADU_LPC_STATUS_REG:
> +        val = PPC_BIT(0); /* ack / done */
> +        break;
>   
>       default:
>           qemu_log_mask(LOG_UNIMP, "ADU Unimplemented read register: Ox%08x\n",
> @@ -45,9 +70,30 @@ static uint64_t pnv_adu_xscom_read(void *opaque, hwaddr 
> addr, unsigned width)
>       return val;
>   }
>   
> +static bool lpc_cmd_read(PnvADU *adu)
> +{
> +    return !!(adu->lpc_cmd_reg & PPC_BIT(0));
> +}
> +
> +static bool lpc_cmd_write(PnvADU *adu)
> +{
> +    return !lpc_cmd_read(adu);
> +}
> +
> +static uint32_t lpc_cmd_addr(PnvADU *adu)
> +{
> +    return (adu->lpc_cmd_reg & PPC_BITMASK(32, 63)) >> PPC_BIT_NR(63);
> +}
> +
> +static uint32_t lpc_cmd_size(PnvADU *adu)
> +{
> +    return (adu->lpc_cmd_reg & PPC_BITMASK(5, 11)) >> PPC_BIT_NR(11);
> +}
> +
>   static void pnv_adu_xscom_write(void *opaque, hwaddr addr, uint64_t val,
>                                   unsigned width)
>   {
> +    PnvADU *adu = PNV_ADU(opaque);
>       uint32_t offset = addr >> 3;
>   
>       trace_pnv_adu_xscom_write(addr, val);
> @@ -58,6 +104,47 @@ static void pnv_adu_xscom_write(void *opaque, hwaddr addr, 
> uint64_t val,
>       case 0x13:     /* error register */
>           break;
>   
> +    case ADU_LPC_BASE_REG:
> +        qemu_log_mask(LOG_UNIMP,
> +                      "ADU: Changing LPC_BASE_REG is not implemented\n");
> +        break;
> +
> +    case ADU_LPC_CMD_REG:
> +        adu->lpc_cmd_reg = val;
> +        if (lpc_cmd_read(adu)) {
> +            uint32_t lpc_addr = lpc_cmd_addr(adu);
> +            uint32_t lpc_size = lpc_cmd_size(adu);
> +            uint64_t data = 0;
> +
> +            pnv_lpc_opb_read(adu->lpc, lpc_addr, (void *)&data, lpc_size);


Coverity reports an issue :

> > >     CID 1558830:    (OVERRUN)
> > >     Overrunning buffer pointed to by "(void *)&data" of 8 bytes by passing it to a 
> > > function which accesses it at byte offset 126 using argument "lpc_size" (which evaluates to 
> > > 127).


I think we should change the type of variable 'data' to allow reading
a maximum of 128 bytes to cover all possible LPC access. Also, possible
sizes should be limited to power of 2.

Thanks,

C.




> +
> +            /*
> +             * ADU access is performed within 8-byte aligned sectors. Smaller
> +             * access sizes don't get formatted to the least significant byte,
> +             * but rather appear in the data reg at the same offset as the
> +             * address in memory. This shifts them into that position.
> +             */
> +            adu->lpc_data_reg = be64_to_cpu(data) >> ((lpc_addr & 7) * 8);
> +        }
> +        break;
> +
> +    case ADU_LPC_DATA_REG:
> +        adu->lpc_data_reg = val;
> +        if (lpc_cmd_write(adu)) {
> +            uint32_t lpc_addr = lpc_cmd_addr(adu);
> +            uint32_t lpc_size = lpc_cmd_size(adu);
> +            uint64_t data;
> +
> +            data = cpu_to_be64(val) >> ((lpc_addr & 7) * 8); /* See above */
> +            pnv_lpc_opb_write(adu->lpc, lpc_addr, (void *)&data, lpc_size);
> +        }
> +        break;
> +
> +    case ADU_LPC_STATUS_REG:
> +        qemu_log_mask(LOG_UNIMP,
> +                      "ADU: Changing LPC_STATUS_REG is not implemented\n");
> +        break;
> +
>       default:
>           qemu_log_mask(LOG_UNIMP, "ADU Unimplemented write register: Ox%08x\n",
>                                                                        offset);
> @@ -78,18 +165,26 @@ static void pnv_adu_realize(DeviceState *dev, Error **errp)
>   {
>       PnvADU *adu = PNV_ADU(dev);
>   
> +    assert(adu->lpc);
> +
>       /* XScom regions for ADU registers */
>       pnv_xscom_region_init(&adu->xscom_regs, OBJECT(dev),
>                             &pnv_adu_xscom_ops, adu, "xscom-adu",
>                             PNV9_XSCOM_ADU_SIZE);
>   }
>   
> +static Property pnv_adu_properties[] = {
> +    DEFINE_PROP_LINK("lpc", PnvADU, lpc, TYPE_PNV_LPC, PnvLpcController *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>   static void pnv_adu_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
>       dc->realize = pnv_adu_realize;
>       dc->desc = "PowerNV ADU";
> +    device_class_set_props(dc, pnv_adu_properties);
>       dc->user_creatable = false;
>   }
>   
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 0f14e180af..f8aad955b5 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -236,16 +236,16 @@ int pnv_dt_lpc(PnvChip *chip, void *fdt, int root_offset, 
> uint64_t lpcm_addr,
>    * TODO: rework to use address_space_stq() and address_space_ldq()
>    * instead.
>    */
> -static bool opb_read(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> -                     int sz)
> +bool pnv_lpc_opb_read(PnvLpcController *lpc, uint32_t addr,
> +                      uint8_t *data, int sz)
>   {
>       /* XXX Handle access size limits and FW read caching here */
>       return !address_space_read(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
>                                  data, sz);
>   }
>   
> -static bool opb_write(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> -                      int sz)
> +bool pnv_lpc_opb_write(PnvLpcController *lpc, uint32_t addr,
> +                       uint8_t *data, int sz)
>   {
>       /* XXX Handle access size limits here */
>       return !address_space_write(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> @@ -277,7 +277,7 @@ static void pnv_lpc_do_eccb(PnvLpcController *lpc, uint64_t 
> cmd)
>       }
>   
>       if (cmd & ECCB_CTL_READ) {
> -        success = opb_read(lpc, opb_addr, data, sz);
> +        success = pnv_lpc_opb_read(lpc, opb_addr, data, sz);
>           if (success) {
>               lpc->eccb_stat_reg = ECCB_STAT_OP_DONE |
>                       (((uint64_t)data[0]) << 24 |
> @@ -294,7 +294,7 @@ static void pnv_lpc_do_eccb(PnvLpcController *lpc, uint64_t 
> cmd)
>           data[2] = lpc->eccb_data_reg >>  8;
>           data[3] = lpc->eccb_data_reg;
>   
> -        success = opb_write(lpc, opb_addr, data, sz);
> +        success = pnv_lpc_opb_write(lpc, opb_addr, data, sz);
>           lpc->eccb_stat_reg = ECCB_STAT_OP_DONE;
>       }
>       /* XXX Which error bit (if any) to signal OPB error ? */
> diff --git a/include/hw/ppc/pnv_adu.h b/include/hw/ppc/pnv_adu.h
> index b5f308627b..f9dbd8c8b3 100644
> --- a/include/hw/ppc/pnv_adu.h
> +++ b/include/hw/ppc/pnv_adu.h
> @@ -10,6 +10,7 @@
>   #define PPC_PNV_ADU_H
>   
>   #include "hw/ppc/pnv.h"
> +#include "hw/ppc/pnv_lpc.h"
>   #include "hw/qdev-core.h"
>   
>   #define TYPE_PNV_ADU "pnv-adu"
> @@ -19,6 +20,12 @@ OBJECT_DECLARE_TYPE(PnvADU, PnvADUClass, PNV_ADU)
>   struct PnvADU {
>       DeviceState xd;
>   
> +    /* LPCMC (LPC Master Controller) access engine */
> +    PnvLpcController *lpc;
> +    uint64_t     lpc_base_reg;
> +    uint64_t     lpc_cmd_reg;
> +    uint64_t     lpc_data_reg;
> +
>       MemoryRegion xscom_regs;
>   };
>   
> diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
> index e0fd5e4130..174add4c53 100644
> --- a/include/hw/ppc/pnv_lpc.h
> +++ b/include/hw/ppc/pnv_lpc.h
> @@ -109,6 +109,11 @@ struct PnvLpcClass {
>       DeviceRealize parent_realize;
>   };
>   
> +bool pnv_lpc_opb_read(PnvLpcController *lpc, uint32_t addr,
> +                      uint8_t *data, int sz);
> +bool pnv_lpc_opb_write(PnvLpcController *lpc, uint32_t addr,
> +                       uint8_t *data, int sz);
> +
>   ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error 
> **errp);
>   int pnv_dt_lpc(PnvChip *chip, void *fdt, int root_offset,
>                  uint64_t lpcm_addr, uint64_t lpcm_size);