Re: [PATCH] platform/x86: amd-pmc: Add support for AMD Spill to DRAM STB feature

["Limonciello, Mario" <mario.limonciello@amd.com>]

On 2/2/2022 00:03, Shyam Sundar S K wrote:
> 
> 
> On 2/2/2022 10:14 AM, Limonciello, Mario wrote:
>> On 1/27/2022 04:09, Sanket Goswami wrote:
>>> Spill to DRAM functionality is a feature that allows STB (Smart Trace
>>> Buffer) to spill data from SRAM into DRAM on some future AMD ASICs. The
>>> size allocated for STB is more than the earlier SoC's which helps to
>>> collect more tracing and telemetry data.
>>>
>>> Co-developed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>>> ---
>>>    drivers/platform/x86/amd-pmc.c | 132 +++++++++++++++++++++++++++++----
>>>    1 file changed, 118 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/amd-pmc.c
>>> b/drivers/platform/x86/amd-pmc.c
>>> index 4c72ba68b315..3040f50627c6 100644
>>> --- a/drivers/platform/x86/amd-pmc.c
>>> +++ b/drivers/platform/x86/amd-pmc.c
>>> @@ -41,6 +41,16 @@
>>>    #define AMD_PMC_STB_PMI_0        0x03E30600
>>>    #define AMD_PMC_STB_PREDEF        0xC6000001
>>>    +/* STB S2D(Spill to DRAM) has different message port offset */
>>> +#define STB_SPILL_TO_DRAM        0xBE
>>> +#define AMD_S2D_REGISTER_MESSAGE    0xA20
>>> +#define AMD_S2D_REGISTER_RESPONSE    0xA80
>>> +#define AMD_S2D_REGISTER_ARGUMENT    0xA88
>>> +
>>> +/* STB Spill to DRAM Parameters */
>>> +#define S2D_TELEMETRY_BYTES_MAX        0x100000
>>> +#define S2D_TELEMETRY_DRAMBYTES_MAX    0x1000000
>>
>> Why have two separate definitions for the same value?  Is there a case
>> that these will ever be different?
> 
> Values are different right? _BYTES_MAX is 1M and _DRAM_MAX is 16M

Sorry, my mistake when I looked - yes.

> 
>>
>>> +
>>>    /* Base address of SMU for mapping physical address to virtual
>>> address */
>>>    #define AMD_PMC_SMU_INDEX_ADDRESS    0xB8
>>>    #define AMD_PMC_SMU_INDEX_DATA        0xBC
>>> @@ -95,6 +105,13 @@ enum amd_pmc_def {
>>>        MSG_OS_HINT_RN,
>>>    };
>>>    +enum s2d_arg {
>>> +    S2D_TELEMETRY_SIZE = 0x01,
>>> +    S2D_PHYS_ADDR_LOW,
>>> +    S2D_PHYS_ADDR_HIGH,
>>> +    S2D_NUM_SAMPLES,
>>> +};
>>> +
>>>    struct amd_pmc_bit_map {
>>>        const char *name;
>>>        u32 bit_mask;
>>> @@ -119,7 +136,9 @@ static const struct amd_pmc_bit_map soc15_ip_blk[]
>>> = {
>>>    struct amd_pmc_dev {
>>>        void __iomem *regbase;
>>>        void __iomem *smu_virt_addr;
>>> +    void __iomem *stb_virt_addr;
>>>        void __iomem *fch_virt_addr;
>>> +    bool msg_port;
>>>        u32 base_addr;
>>>        u32 cpu_id;
>>>        u32 active_ips;
>>> @@ -236,6 +255,65 @@ static const struct file_operations
>>> amd_pmc_stb_debugfs_fops = {
>>>        .release = amd_pmc_stb_debugfs_release,
>>>    };
>>>    +static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct
>>> file *filp)
>>> +{
>>> +    struct amd_pmc_dev *dev = filp->f_inode->i_private;
>>> +    u32 *buf, phys_addr_low, phys_addr_hi, size, samples;
>>> +    u64 stb_phys_addr;
>>> +
>>> +    /* Spill to DRAM feature uses separate SMU message port */
>>> +    dev->msg_port = 1;
>>
>> This message port is only used for STD right?  Existing messages for
>> version, OS_HINT and the like should go through the old port.
> 
> msg_port is a flag to decide whether to use the regular PMC port or the
> new STB port defined for Spill-to-DRAM feature.
> 
> Both are complementary. Spill-to-DRAM will only be enabled in Yellow
> Carp and future ASICs and lower than that the new message port will not
> be enabled.
> 
> The decision of whether to use v2 or v1 versions of debugfs is being
> taken in the debugfs creation with a cpuid check and hence I believe the
> versions/OS_HINT will not collide.
> 
> Also, this change has to tested several iterations on older and newer
> platforms before it was posted here. So, based that this we should
> ignore any such concerns around the OS_HINT.
>  >>
>> You don't seem to ever unset this, so I think if the STB was opened
>> before S0i3 was invoked I would think it would cause the OS_HINT message
>> to be sent to the wrong port wouldn't it?
> 
> I tried to cover this in the above details. No, it would not cause HINT
> to be sent to wrong message port.

I don't think you followed my thought process here.

If `amd_pmc_stb_debugfs_open_v2` is called then dev->msg_port is set to 1.

So that means that any future call to `amd_pmc_send_cmd` (such as is 
done for OS_HINT) will use the STD registers.  So doesn't that mean 
OS_HINT is sent to STD registers?

If STD registers support all the existing commands as the PMC registers 
this is fine, otherwise I think there needs to be more logic to check 
whether the command is PMC or STD related.

> 
>>
>> It might be safer to modify amd_pmc_send_cmd to have an extra argument
>> of which port to use in each invocation.  Then only in debugfs_v2
>> operations you can use this port.
>>
>>> +
>>> +    amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size,
>>> STB_SPILL_TO_DRAM, 1);
>>> +    if (size != S2D_TELEMETRY_BYTES_MAX)
>>> +        return -EIO;
>>
>> What if the firmware was configured to use less than 16MB?  Shouldn't
>> you be able to respect that?
> 
> That is why there is a seperate call to fetch the S2D(Spill-to-DRAM)
> size which the FW has configured it for notifying it to the driver.

But you only allow size to be S2D_TELEMETRY_BYTES_MAX, shouldn't you be 
flexible to a range of what the firmware returns rather than the single 
value?

> 
> Btw, though 16M is the max DRAM carved out space for 16M, but currently
> only 1M is allocated to get the full STB dumps from DRAM space and this
> by design. We have the same change on the windows side too.
> 
>>
>> I would think you instead want to have a boundary check on the "max" to
>> support.
> 
> Above explanation should hopefully address this.
> 
>>
>>> +
>>> +    /* Get STB DRAM address */
>>> +    amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low,
>>> STB_SPILL_TO_DRAM, 1);
>>> +    amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi,
>>> STB_SPILL_TO_DRAM, 1);
>>> +
>>> +    /* Get the number of samples present in the STB buffer */
>>> +    amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &samples,
>>> STB_SPILL_TO_DRAM, 1);
>>> +    stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>>> +
>>> +    dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr,
>>> S2D_TELEMETRY_DRAMBYTES_MAX);
>>> +    if (!dev->stb_virt_addr)
>>> +        return -ENOMEM;
>>> +
>>> +    buf = kzalloc(S2D_TELEMETRY_BYTES_MAX, GFP_KERNEL);
>>> +    if (!buf)
>>> +        return -ENOMEM;
>>
>> Shouldn't you clear the iomapping for dev->stb_virt_addr on this failure?
> 
> I believe, that is why we use managed devm_*() APIs which is supposed to
>   take care of reducing the reference counts ?

I think those only take account when unloading the driver or unbinding 
the device though, don't they?  So if devm_ioremap worked but kzalloc 
failed then you call `amd_pmc_stb_debugfs_open_v2` again without unbind 
or unload driver devm_ioremap will be called again too.

> 
> Thanks,
> Shyam
> 
>>
>>> +
>>> +    memcpy_fromio(buf, dev->stb_virt_addr, S2D_TELEMETRY_BYTES_MAX);
>>> +
>>> +    filp->private_data = buf;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char
>>> __user *buf, size_t size,
>>> +                       loff_t *pos)
>>> +{
>>> +    if (!filp->private_data)
>>> +        return -EINVAL;
>>> +
>>> +    return simple_read_from_buffer(buf, size, pos, filp->private_data,
>>> +                    S2D_TELEMETRY_BYTES_MAX);
>>> +}
>>> +
>>> +static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct
>>> file *filp)
>>> +{
>>> +    kfree(filp->private_data);
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
>>> +    .owner = THIS_MODULE,
>>> +    .open = amd_pmc_stb_debugfs_open_v2,
>>> +    .read = amd_pmc_stb_debugfs_read_v2,
>>> +    .release = amd_pmc_stb_debugfs_release_v2,
>>> +};
>>> +
>>>    static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct
>>> device *dev,
>>>                     struct seq_file *s)
>>>    {
>>> @@ -350,9 +428,14 @@ static void amd_pmc_dbgfs_register(struct
>>> amd_pmc_dev *dev)
>>>        debugfs_create_file("amd_pmc_idlemask", 0644, dev->dbgfs_dir, dev,
>>>                    &amd_pmc_idlemask_fops);
>>>        /* Enable STB only when the module_param is set */
>>> -    if (enable_stb)
>>> -        debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
>>> -                    &amd_pmc_stb_debugfs_fops);
>>> +    if (enable_stb) {
>>> +        if (dev->cpu_id == AMD_CPU_ID_YC)
>>> +            debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
>>> +                        &amd_pmc_stb_debugfs_fops_v2);
>>> +        else
>>> +            debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
>>> +                        &amd_pmc_stb_debugfs_fops);
>>> +    }
>>>    }
>>>    #else
>>>    static inline void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>>> @@ -392,26 +475,47 @@ static int amd_pmc_setup_smu_logging(struct
>>> amd_pmc_dev *dev)
>>>      static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
>>>    {
>>> -    u32 value;
>>> +    u32 value, message, argument, response;
>>> +
>>> +    if (dev->msg_port) {
>>> +        message = AMD_S2D_REGISTER_MESSAGE;
>>> +        argument = AMD_S2D_REGISTER_ARGUMENT;
>>> +        response = AMD_S2D_REGISTER_RESPONSE;
>>> +    } else {
>>> +        message = AMD_PMC_REGISTER_MESSAGE;
>>> +        argument = AMD_PMC_REGISTER_ARGUMENT;
>>> +        response = AMD_PMC_REGISTER_RESPONSE;
>>> +    }
>>>    -    value = amd_pmc_reg_read(dev, AMD_PMC_REGISTER_RESPONSE);
>>> +    value = amd_pmc_reg_read(dev, response);
>>>        dev_dbg(dev->dev, "AMD_PMC_REGISTER_RESPONSE:%x\n", value);
>>>    -    value = amd_pmc_reg_read(dev, AMD_PMC_REGISTER_ARGUMENT);
>>> +    value = amd_pmc_reg_read(dev, argument);
>>>        dev_dbg(dev->dev, "AMD_PMC_REGISTER_ARGUMENT:%x\n", value);
>>>    -    value = amd_pmc_reg_read(dev, AMD_PMC_REGISTER_MESSAGE);
>>> +    value = amd_pmc_reg_read(dev, message);
>>>        dev_dbg(dev->dev, "AMD_PMC_REGISTER_MESSAGE:%x\n", value);
>>>    }
>>>      static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32
>>> *data, u8 msg, bool ret)
>>>    {
>>>        int rc;
>>> -    u32 val;
>>> +    u32 val, message, argument, response;
>>>          mutex_lock(&dev->lock);
>>> +
>>> +    if (dev->msg_port) {
>>> +        message = AMD_S2D_REGISTER_MESSAGE;
>>> +        argument = AMD_S2D_REGISTER_ARGUMENT;
>>> +        response = AMD_S2D_REGISTER_RESPONSE;
>>> +    } else {
>>> +        message = AMD_PMC_REGISTER_MESSAGE;
>>> +        argument = AMD_PMC_REGISTER_ARGUMENT;
>>> +        response = AMD_PMC_REGISTER_RESPONSE;
>>> +    }
>>> +
>>>        /* Wait until we get a valid response */
>>> -    rc = readx_poll_timeout(ioread32, dev->regbase +
>>> AMD_PMC_REGISTER_RESPONSE,
>>> +    rc = readx_poll_timeout(ioread32, dev->regbase + response,
>>>                    val, val != 0, PMC_MSG_DELAY_MIN_US,
>>>                    PMC_MSG_DELAY_MIN_US * RESPONSE_REGISTER_LOOP_MAX);
>>>        if (rc) {
>>> @@ -420,16 +524,16 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev
>>> *dev, u32 arg, u32 *data, u8 msg,
>>>        }
>>>          /* Write zero to response register */
>>> -    amd_pmc_reg_write(dev, AMD_PMC_REGISTER_RESPONSE, 0);
>>> +    amd_pmc_reg_write(dev, response, 0);
>>>          /* Write argument into response register */
>>> -    amd_pmc_reg_write(dev, AMD_PMC_REGISTER_ARGUMENT, arg);
>>> +    amd_pmc_reg_write(dev, argument, arg);
>>>          /* Write message ID to message ID register */
>>> -    amd_pmc_reg_write(dev, AMD_PMC_REGISTER_MESSAGE, msg);
>>> +    amd_pmc_reg_write(dev, message, msg);
>>>          /* Wait until we get a valid response */
>>> -    rc = readx_poll_timeout(ioread32, dev->regbase +
>>> AMD_PMC_REGISTER_RESPONSE,
>>> +    rc = readx_poll_timeout(ioread32, dev->regbase + response,
>>>                    val, val != 0, PMC_MSG_DELAY_MIN_US,
>>>                    PMC_MSG_DELAY_MIN_US * RESPONSE_REGISTER_LOOP_MAX);
>>>        if (rc) {
>>> @@ -442,7 +546,7 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev
>>> *dev, u32 arg, u32 *data, u8 msg,
>>>            if (ret) {
>>>                /* PMFW may take longer time to return back the data */
>>>                usleep_range(DELAY_MIN_US, 10 * DELAY_MAX_US);
>>> -            *data = amd_pmc_reg_read(dev, AMD_PMC_REGISTER_ARGUMENT);
>>> +            *data = amd_pmc_reg_read(dev, argument);
>>>            }
>>>            break;
>>>        case AMD_PMC_RESULT_CMD_REJECT_BUSY:
>>