Re: [PATCH v2 6/7] platform/x86: Convert to ACPI based probing

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Wed, 20 Dec 2023, Suma Hegde wrote:

> ACPI table provides mailbox base address and register offset
> information. The base address is provided as part of CRS method
> and mailbox offsets are provided through DSD table.
> Sockets are differentiated by UIDs.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> 
> Changes since v1:
> 1. Define amd_hsmp_acpi_rdwr() for doing mailbox memory mapped io
> 2. Add a check to see if mailbox register offsets are set in
>    hsmp_read_acpi_dsd()
> 3. Add a check to see if sock->mbinfo.base_addr sockck->mbinfo.size are
>    set in hsmp_read_acpi_crs()
> 4. Change order of the statements in switch case ACPI_RESOURCE_TYPE_FIXED_MEMORY32
>    in hsmp_resource()
> 5. Add hsmp_test() after hsmp_parse_acpi_table() call
> 6. Add r.end < r.start check in hsmp_resource()
> 7. Add !dsd error check in hsmp_read_acpi_dsd
> 
> ---
>  drivers/platform/x86/amd/hsmp.c | 323 +++++++++++++++++++++++++++++---
>  1 file changed, 296 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index 3508399c6aa9..726ba73e23dd 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c
> @@ -18,9 +18,11 @@
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <linux/semaphore.h>
> +#include <linux/acpi.h>
>  
>  #define DRIVER_NAME		"amd_hsmp"
> -#define DRIVER_VERSION		"2.0"
> +#define DRIVER_VERSION		"2.2"
> +#define ACPI_HSMP_DEVICE_HID	"AMDI0097"
>  
>  /* HSMP Status / Error codes */
>  #define HSMP_STATUS_NOT_READY	0x00
> @@ -54,6 +56,11 @@
>  
>  #define HSMP_ATTR_GRP_NAME_SIZE	10
>  
> +/* These are the strings specified in ACPI table */
> +#define MSG_IDOFF_STR		"MsgIdOffset"
> +#define MSG_ARGOFF_STR		"MsgArgOffset"
> +#define MSG_RESPOFF_STR		"MsgRspOffset"
> +
>  struct hsmp_mbaddr_info {
>  	u32 base_addr;
>  	u32 msg_id_off;
> @@ -66,6 +73,7 @@ struct hsmp_socket {
>  	struct bin_attribute hsmp_attr;
>  	struct hsmp_mbaddr_info mbinfo;
>  	void __iomem *metric_tbl_addr;
> +	void __iomem *virt_base_addr;
>  	struct semaphore hsmp_sem;
>  	char name[HSMP_ATTR_GRP_NAME_SIZE];
>  	struct pci_dev *root;
> @@ -78,12 +86,14 @@ struct hsmp_plat_device {
>  	struct hsmp_socket *sock;
>  	u32 proto_ver;
>  	u16 num_sockets;
> +	bool is_acpi_device;
> +	bool is_probed;
>  };
>  
>  static struct hsmp_plat_device plat_dev;
>  
> -static int amd_hsmp_rdwr(struct hsmp_socket *sock, u32 offset,
> -			 u32 *value, bool write)
> +static int amd_hsmp_pci_rdwr(struct hsmp_socket *sock, u32 offset,
> +			     u32 *value, bool write)
>  {
>  	int ret;
>  
> @@ -101,8 +111,29 @@ static int amd_hsmp_rdwr(struct hsmp_socket *sock, u32 offset,
>  	return ret;
>  }
>  
> +static void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
> +			       u32 *value, bool write)
> +{
> +	if (write)
> +		iowrite32(*value, sock->virt_base_addr + offset);
> +	else
> +		*value = ioread32(sock->virt_base_addr + offset);
> +}
> +
> +static int amd_hsmp_rdwr(struct hsmp_socket *sock, u32 offset,
> +			 u32 *value, bool write)
> +{
> +	if (plat_dev.is_acpi_device)
> +		amd_hsmp_acpi_rdwr(sock, offset, value, write);
> +	else
> +		return amd_hsmp_pci_rdwr(sock, offset, value, write);
> +
> +	return 0;
> +}
> +
>  /*
> - * Send a message to the HSMP port via PCI-e config space registers.
> + * Send a message to the HSMP port via PCI-e config space registers
> + * or by writing to MMIO space.
>   *
>   * The caller is expected to zero out any unused arguments.
>   * If a response is expected, the number of response words should be greater than 0.
> @@ -450,6 +481,9 @@ static int hsmp_create_sysfs_interface(void)
>  	int ret;
>  	u16 i;
>  
> +	if (plat_dev.is_acpi_device)
> +		return 0;
> +
>  	/* String formatting is currently limited to u8 sockets */
>  	if (WARN_ON(plat_dev.num_sockets > U8_MAX))
>  		return -ERANGE;
> @@ -487,13 +521,188 @@ static int hsmp_create_sysfs_interface(void)
>  	return devm_device_add_groups(plat_dev.sock[0].dev, hsmp_attr_grps);
>  }
>  
> -static int hsmp_cache_proto_ver(void)
> +/* This is the UUID used for HSMP */
> +static const guid_t acpi_hsmp_uuid = GUID_INIT(0xb74d619d, 0x5707, 0x48bd,
> +						0xa6, 0x9f, 0x4e, 0xa2,
> +						0x87, 0x1f, 0xc2, 0xf6);
> +
> +static inline bool is_acpi_hsmp_uuid(union acpi_object *obj)
> +{
> +	if (obj->type == ACPI_TYPE_BUFFER && obj->buffer.length == 16)
> +		return guid_equal((guid_t *)obj->buffer.pointer, &acpi_hsmp_uuid);
> +
> +	return false;
> +}
> +
> +static inline int hsmp_get_uid(struct device *dev, u16 *sock_ind)
> +{
> +	char *uid;
> +
> +	/*
> +	 * UID (ID00, ID01..IDXX) is used for differentiating sockets,
> +	 * read it and strip the "ID" part of it and convert the remaining
> +	 * bytes to integer.
> +	 */
> +	uid = acpi_device_uid(ACPI_COMPANION(dev));
> +
> +	return kstrtou16((uid + 2), 10, sock_ind);
> +}
> +
> +static acpi_status hsmp_resource(struct acpi_resource *res, void *data)
> +{
> +	struct hsmp_socket *sock = data;
> +	struct resource r;
> +
> +	switch (res->type) {
> +	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> +		if (!acpi_dev_resource_memory(res, &r))
> +			return AE_ERROR;
> +		if (!r.start || r.end < r.start || !(r.flags & IORESOURCE_MEM_WRITEABLE))
> +			return AE_ERROR;
> +		sock->mbinfo.base_addr = r.start;
> +		sock->mbinfo.size = r.end - r.start + 1;

resource_size(r)

> +		break;
> +	case ACPI_RESOURCE_TYPE_END_TAG:
> +		break;
> +	default:
> +		return AE_ERROR;
> +	}
> +
> +	return AE_OK;
> +}
> +
> +static int hsmp_read_acpi_dsd(struct hsmp_socket *sock)
> +{
> +	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *guid, *mailbox_package;
> +	union acpi_object *dsd;
> +	acpi_status status;
> +	int ret = 0;
> +	int j;
> +
> +	status = acpi_evaluate_object_typed(ACPI_HANDLE(sock->dev), "_DSD", NULL,
> +					    &buf, ACPI_TYPE_PACKAGE);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(sock->dev, "Failed to read mailbox reg offsets from DSD table, err: %s\n",
> +			acpi_format_exception(status));
> +		return -EINVAL;
> +	}
> +
> +	dsd = buf.pointer;
> +
> +	/* HSMP _DSD property should contain 2 objects.
> +	 * 1. guid which is an acpi object of type ACPI_TYPE_BUFFER
> +	 * 2. mailbox which is an acpi object of type ACPI_TYPE_PACKAGE
> +	 *    This mailbox object contains 3 more acpi objects of type
> +	 *    ACPI_TYPE_PACKAGE for holding msgid, msgresp, msgarg offsets
> +	 *    these packages inturn contain 2 acpi objects of type
> +	 *    ACPI_TYPE_STRING and ACPI_TYPE_INTEGER
> +	 */
> +	if (!dsd || dsd->type != ACPI_TYPE_PACKAGE || dsd->package.count != 2) {
> +		ret = -EINVAL;
> +		goto free_buf;
> +	}
> +
> +	guid = &dsd->package.elements[0];
> +	mailbox_package = &dsd->package.elements[1];
> +	if (!is_acpi_hsmp_uuid(guid) || mailbox_package->type != ACPI_TYPE_PACKAGE) {
> +		dev_err(sock->dev, "Invalid hsmp _DSD table data\n");
> +		ret = -EINVAL;
> +		goto free_buf;
> +	}
> +
> +	for (j = 0; j < mailbox_package->package.count; j++) {
> +		union acpi_object *msgobj, *msgstr, *msgint;
> +
> +		msgobj	= &mailbox_package->package.elements[j];
> +		msgstr	= &msgobj->package.elements[0];
> +		msgint	= &msgobj->package.elements[1];
> +
> +		/* package should have 1 string and 1 integer object */
> +		if (msgobj->type != ACPI_TYPE_PACKAGE ||
> +		    msgstr->type != ACPI_TYPE_STRING ||
> +		    msgint->type != ACPI_TYPE_INTEGER) {
> +			ret = -EINVAL;
> +			goto free_buf;
> +		}
> +
> +		if (!strncmp(msgstr->string.pointer, MSG_IDOFF_STR,
> +			     msgstr->string.length)) {
> +			sock->mbinfo.msg_id_off = msgint->integer.value;
> +		} else if (!strncmp(msgstr->string.pointer, MSG_RESPOFF_STR,
> +				    msgstr->string.length)) {
> +			sock->mbinfo.msg_resp_off =  msgint->integer.value;
> +		} else if (!strncmp(msgstr->string.pointer, MSG_ARGOFF_STR,
> +				    msgstr->string.length)) {
> +			sock->mbinfo.msg_arg_off = msgint->integer.value;
> +		} else {
> +			ret = -EINVAL;
> +			goto free_buf;
> +		}
> +	}
> +
> +	if (!sock->mbinfo.msg_id_off || !sock->mbinfo.msg_resp_off ||
> +	    !sock->mbinfo.msg_arg_off)
> +		ret = -EINVAL;

These smell like return -ENODEV or -ENOENT but I don't have an idea what 
ACPI related funcs typically return.

> +free_buf:
> +	ACPI_FREE(buf.pointer);
> +	return ret;
> +}
> +
> +static int hsmp_read_acpi_crs(struct hsmp_socket *sock)
> +{
> +	acpi_status status;
> +
> +	status = acpi_walk_resources(ACPI_HANDLE(sock->dev), METHOD_NAME__CRS,
> +				     hsmp_resource, sock);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(sock->dev, "Failed to look up MP1 base address from CRS method, err: %s\n",
> +			acpi_format_exception(status));
> +		return -EINVAL;
> +	}
> +	if (!sock->mbinfo.base_addr || !sock->mbinfo.size)
> +		return -EINVAL;
> +
> +	/* The mapped region should be un cached */
> +	sock->virt_base_addr = devm_ioremap_uc(sock->dev, sock->mbinfo.base_addr,
> +					       sock->mbinfo.size);
> +	if (!sock->virt_base_addr) {
> +		dev_err(sock->dev, "Failed to ioremap MP1 base address\n");
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +/* Parse the ACPI table to read the data */
> +static int hsmp_parse_acpi_table(struct device *dev, u16 sock_ind)
> +{
> +	struct hsmp_socket *sock = &plat_dev.sock[sock_ind];
> +	int ret;
> +
> +	sock->sock_ind		= sock_ind;
> +	sock->dev		= dev;
> +	plat_dev.is_acpi_device	= true;
> +
> +	sema_init(&sock->hsmp_sem, 1);
> +
> +	/* Read MP1 base address from CRS method */
> +	ret = hsmp_read_acpi_crs(sock);
> +	if (ret)
> +		return ret;
> +
> +	/* Read mailbox offsets from DSD table */
> +	return hsmp_read_acpi_dsd(sock);
> +}
> +
> +static int hsmp_cache_proto_ver(u16 sock_ind)
>  {
>  	struct hsmp_message msg = { 0 };
>  	int ret;
>  
>  	msg.msg_id	= HSMP_GET_PROTO_VER;
> -	msg.sock_ind	= 0;
> +	msg.sock_ind	= sock_ind;
>  	msg.response_sz = hsmp_msg_desc_table[HSMP_GET_PROTO_VER].response_sz;
>  
>  	ret = hsmp_send_message(&msg);
> @@ -532,28 +741,62 @@ static int initialize_platdev(struct device *dev)
>  	return 0;
>  }
>  
> +static const struct acpi_device_id amd_hsmp_acpi_ids[] = {
> +	{ACPI_HSMP_DEVICE_HID, 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, amd_hsmp_acpi_ids);
> +
>  static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  {
> +	struct acpi_device *adev;
> +	u16 sock_ind = 0;
>  	int ret;
>  
> -	plat_dev.sock = devm_kzalloc(&pdev->dev,
> -				     (plat_dev.num_sockets * sizeof(struct hsmp_socket)),
> -				     GFP_KERNEL);
> -	if (!plat_dev.sock)
> -		return -ENOMEM;
> -
> -	ret = initialize_platdev(&pdev->dev);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * On ACPI supported BIOS, there is an ACPI HSMP device added for
> +	 * each socket, so the per socket probing, but the memory allocated for
> +	 * sockets should be contiguous to access it as an array,
> +	 * Hence allocate memory for all the sockets at once instead of allocating
> +	 * on each probe.
> +	 */
> +	if (!plat_dev.is_probed) {
> +		plat_dev.sock = devm_kzalloc(&pdev->dev,
> +					     (plat_dev.num_sockets * sizeof(struct hsmp_socket)),
> +					     GFP_KERNEL);
> +		if (!plat_dev.sock)
> +			return -ENOMEM;
> +	}
>  
> -	plat_dev.hsmp_device.name	= HSMP_CDEV_NAME;
> -	plat_dev.hsmp_device.minor	= MISC_DYNAMIC_MINOR;
> -	plat_dev.hsmp_device.fops	= &hsmp_fops;
> -	plat_dev.hsmp_device.parent	= &pdev->dev;
> -	plat_dev.hsmp_device.nodename	= HSMP_DEVNODE_NAME;
> -	plat_dev.hsmp_device.mode	= 0644;
> +	adev = ACPI_COMPANION(&pdev->dev);
> +	if (adev && !acpi_match_device_ids(adev, amd_hsmp_acpi_ids)) {
> +		ret = hsmp_get_uid(&pdev->dev, &sock_ind);
> +		if (ret)
> +			return ret;
> +		if (sock_ind >= plat_dev.num_sockets)
> +			return -EINVAL;

Again, caller didn't provide invalid value here either.

> +		ret = hsmp_parse_acpi_table(&pdev->dev, sock_ind);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to parse ACPI table\n");
> +			return ret;
> +		}
> +		/* Test the hsmp interface */
> +		ret = hsmp_test(sock_ind, 0xDEADBEEF);
> +		if (ret) {
> +			dev_err(&pdev->dev, "HSMP test message failed on Fam:%x model:%x\n",
> +				boot_cpu_data.x86, boot_cpu_data.x86_model);
> +			dev_err(&pdev->dev, "Is HSMP disabled in BIOS ?\n");
> +			return ret;
> +		}
> +	} else {
> +		ret = initialize_platdev(&pdev->dev);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to init HSMP mailbox\n");
> +			return ret;
> +		}
> +	}
>  
> -	ret = hsmp_cache_proto_ver();
> +	ret = hsmp_cache_proto_ver(sock_ind);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to read HSMP protocol version\n");
>  		return ret;
> @@ -563,12 +806,35 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  	if (ret)
>  		dev_err(&pdev->dev, "Failed to create HSMP sysfs interface\n");
>  
> -	return misc_register(&plat_dev.hsmp_device);
> +	if (!plat_dev.is_probed) {
> +		plat_dev.hsmp_device.name	= HSMP_CDEV_NAME;
> +		plat_dev.hsmp_device.minor	= MISC_DYNAMIC_MINOR;
> +		plat_dev.hsmp_device.fops	= &hsmp_fops;
> +		plat_dev.hsmp_device.parent	= &pdev->dev;
> +		plat_dev.hsmp_device.nodename	= HSMP_DEVNODE_NAME;
> +		plat_dev.hsmp_device.mode	= 0644;
> +
> +		ret = misc_register(&plat_dev.hsmp_device);
> +		if (ret)
> +			return ret;
> +
> +		plat_dev.is_probed = true;
> +	}
> +
> +	return 0;
> +
>  }
>  
>  static void hsmp_pltdrv_remove(struct platform_device *pdev)
>  {
> -	misc_deregister(&plat_dev.hsmp_device);
> +	/*
> +	 * We register only one misc_device even on multi socket system.
> +	 * So, deregister should happen only once.
> +	 */
> +	if (plat_dev.is_probed) {
> +		misc_deregister(&plat_dev.hsmp_device);
> +		plat_dev.is_probed = false;
> +	}
>  }
>  
>  static struct platform_driver amd_hsmp_driver = {
> @@ -576,6 +842,7 @@ static struct platform_driver amd_hsmp_driver = {
>  	.remove_new	= hsmp_pltdrv_remove,
>  	.driver		= {
>  		.name	= DRIVER_NAME,
> +		.acpi_match_table = amd_hsmp_acpi_ids,
>  	},
>  };
>  
> @@ -618,9 +885,11 @@ static int __init hsmp_plt_init(void)
>  	if (ret)
>  		return ret;
>  
> -	ret = hsmp_plat_dev_register();
> -	if (ret)
> -		platform_driver_unregister(&amd_hsmp_driver);
> +	if (!plat_dev.is_acpi_device) {
> +		ret = hsmp_plat_dev_register();
> +		if (ret)
> +			platform_driver_unregister(&amd_hsmp_driver);
> +	}
>  
>  	return ret;
>  }
> 

-- 
 i.