'seq_buf' provides a very useful abstraction for writing to a string buffer without needing to worry about it over-flowing. However even though the API has been stable for couple of years now its stills not exported to external modules limiting its usage. Hence this patch proposes update to 'seq_buf.c' to mark seq_buf_printf() which is part of the seq_buf API to be exported to external GPL modules. This symbol will be used in later parts of this patchset to simplify content creation for a sysfs attribute. Cc: Steven Rostedt <rostedt(a)goodmis.org&gt; Cc: Piotr Maziarz <piotrx.maziarz(a)linux.intel.com&gt; Cc: Cezary Rojewski <cezary.rojewski(a)intel.com&gt; Cc: Borislav Petkov <bp(a)alien8.de&gt; Acked-by: Steven Rostedt (VMware) <rostedt(a)goodmis.org&gt; Signed-off-by: Vaibhav Jain <vaibhav(a)linux.ibm.com&gt; --- Changelog: Resend: * Added ack from Steven Rostedt v6..v7: * New patch in the series --- lib/seq_buf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/seq_buf.c b/lib/seq_buf.c index 4e865d42ab03..707453f5d58e 100644 --- a/lib/seq_buf.c +++ b/lib/seq_buf.c @@ -91,6 +91,7 @@ int seq_buf_printf(struct seq_buf *s, const char *fmt, ...) return ret; } +EXPORT_SYMBOL_GPL(seq_buf_printf); #ifdef CONFIG_BINARY_PRINTF /** -- 2.26.2

Introduce support for Papr nvDimm Specific Methods (PDSM) in papr_scm modules and add the command family to the white list of NVDIMM command sets. Also advertise support for ND_CMD_CALL for the dimm command mask and implement necessary scaffolding in the module to handle ND_CMD_CALL ioctl and PDSM requests that we receive. The layout of the PDSM request as we expect from libnvdimm/libndctl is described in newly introduced uapi header 'papr_scm_pdsm.h' which defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used to communicate the PDSM request via member 'nd_pkg_papr_scm->nd_command' and size of payload that need to be sent/received for servicing the PDSM. A new function is_cmd_valid() is implemented that reads the args to papr_scm_ndctl() and performs sanity tests on them. A new function papr_scm_service_pdsm() is introduced and is called from papr_scm_ndctl() in case of a PDSM request is received via ND_CMD_CALL command from libnvdimm. Cc: Dan Williams <dan.j.williams(a)intel.com&gt; Cc: Michael Ellerman <mpe(a)ellerman.id.au&gt; Cc: "Aneesh Kumar K . V" <aneesh.kumar(a)linux.ibm.com&gt; Signed-off-by: Vaibhav Jain <vaibhav(a)linux.ibm.com&gt; --- Changelog: Resend: * None v6..v7 : * Removed the re-definitions of __packed macro from papr_scm_pdsm.h [Mpe]. * Removed the usage of __KERNEL__ macros in papr_scm_pdsm.h [Mpe]. * Removed macros that were unused in papr_scm.c from papr_scm_pdsm.h [Mpe]. * Made functions defined in papr_scm_pdsm.h as static inline. [Mpe] v5..v6 : * Changed the usage of the term DSM to PDSM to distinguish it from the ACPI term [ Dan Williams ] * Renamed papr_scm_dsm.h to papr_scm_pdsm.h and updated various struct to reflect the new terminology. * Updated the patch description and title to reflect the new terminology. * Squashed patch to introduce new command family in 'ndctl.h' with this patch [ Dan Williams ] * Updated the papr_scm_pdsm method starting index from 0x10000 to 0x0 [ Dan Williams ] * Removed redundant license text from the papr_scm_psdm.h file. [ Dan Williams ] * s/envelop/envelope/ at various places [ Dan Williams ] * Added '__packed' attribute to command package header to gaurd against different compiler adding paddings between the fields. [ Dan Williams] * Converted various pr_debug to dev_debug [ Dan Williams ] v4..v5 : * None v3..v4 : * None v2..v3 : * Updated the patch prefix to 'ndctl/uapi' [Aneesh] v1..v2 : * None --- arch/powerpc/include/uapi/asm/papr_scm_pdsm.h | 134 ++++++++++++++++++ arch/powerpc/platforms/pseries/papr_scm.c | 101 ++++++++++++- include/uapi/linux/ndctl.h | 1 + 3 files changed, 230 insertions(+), 6 deletions(-) create mode 100644 arch/powerpc/include/uapi/asm/papr_scm_pdsm.h diff --git a/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h b/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h new file mode 100644 index 000000000000..671693439c1c --- /dev/null +++ b/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h @@ -0,0 +1,134 @@ +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */ +/* + * PAPR-SCM Dimm specific methods (PDSM) and structs for libndctl + * + * (C) Copyright IBM 2020 + * + * Author: Vaibhav Jain <vaibhav at linux.ibm.com> + */ + +#ifndef _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_ +#define _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_ + +#include <linux/types.h> + +/* + * PDSM Envelope: + * + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via + * 'envelopes' which consists of a header and user-defined payload sections. + * The header is described by 'struct nd_pdsm_cmd_pkg' which expects a + * payload following it and offset of which relative to the struct is provided + * by 'nd_pdsm_cmd_pkg.payload_offset'. * + * + * +-------------+---------------------+---------------------------+ + * | 64-Bytes | 8-Bytes | Max 184-Bytes | + * +-------------+---------------------+---------------------------+ + * | nd_pdsm_cmd_pkg | | + * |-------------+ | | + * | nd_cmd_pkg | | | + * +-------------+---------------------+---------------------------+ + * | nd_family | | | + * | nd_size_out | cmd_status | | + * | nd_size_in | payload_version | PAYLOAD | + * | nd_command | payload_offset -----> | + * | nd_fw_size | | | + * +-------------+---------------------+---------------------------+ + * + * PDSM Header: + * + * The header is defined as 'struct nd_pdsm_cmd_pkg' which embeds a + * 'struct nd_cmd_pkg' instance. The PDSM command is assigned to member + * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelope which is + * contained in 'struct nd_cmd_pkg', the header also has members following + * members: + * + * 'cmd_status' : (Out) Errors if any encountered while servicing PDSM. + * 'payload_version' : (In/Out) Version number associated with the payload. + * 'payload_offset' : (In)Relative offset of payload from start of envelope. + * + * PDSM Payload: + * + * The layout of the PDSM Payload is defined by various structs shared between + * papr_scm and libndctl so that contents of payload can be interpreted. During + * servicing of a PDSM the papr_scm module will read input args from the payload + * field by casting its contents to an appropriate struct pointer based on the + * PDSM command. Similarly the output of servicing the PDSM command will be + * copied to the payload field using the same struct. + * + * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which + * leaves around 184 bytes for the envelope payload (ignoring any padding that + * the compiler may silently introduce). + * + * Payload Version: + * + * A 'payload_version' field is present in PDSM header that indicates a specific + * version of the structure present in PDSM Payload for a given PDSM command. + * This provides backward compatibility in case the PDSM Payload structure + * evolves and different structures are supported by 'papr_scm' and 'libndctl'. + * + * When sending a PDSM Payload to 'papr_scm', 'libndctl' should send the version + * of the payload struct it supports via 'payload_version' field. The 'papr_scm' + * module when servicing the PDSM envelope checks the 'payload_version' and then + * uses 'payload struct version' == MIN('payload_version field', + * 'max payload-struct-version supported by papr_scm') to service the PDSM. + * After servicing the PDSM, 'papr_scm' put the negotiated version of payload + * struct in returned 'payload_version' field. + * + * Libndctl on receiving the envelope back from papr_scm again checks the + * 'payload_version' field and based on it use the appropriate version dsm + * struct to parse the results. + * + * Backward Compatibility: + * + * Above scheme of exchanging different versioned PDSM struct between libndctl + * and papr_scm should provide backward compatibility until following two + * assumptions/conditions when defining new PDSM structs hold: + * + * Let T(X) = { set of attributes in PDSM struct 'T' versioned X } + * + * 1. T(X) is a proper subset of T(Y) if X > Y. + * i.e Each new version of PDSM struct should retain existing struct + * attributes from previous version + * + * 2. If an entity (libndctl or papr_scm) supports a PDSM struct T(X) then + * it should also support T(1), T(2)...T(X - 1). + * i.e When adding support for new version of a PDSM struct, libndctl + * and papr_scm should retain support of the existing PDSM struct + * version they support. + */ + +/* Papr-scm-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */ +struct nd_pdsm_cmd_pkg { + struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */ + __s32 cmd_status; /* Out: Sub-cmd status returned back */ + __u16 payload_offset; /* In: offset from start of struct */ + __u16 payload_version; /* In/Out: version of the payload */ + __u8 payload[]; /* In/Out: Sub-cmd data buffer */ +} __packed; + +/* + * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel + * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct + */ +enum papr_scm_pdsm { + PAPR_SCM_PDSM_MIN = 0x0, + PAPR_SCM_PDSM_MAX, +}; + +/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */ +static inline struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd) +{ + return (struct nd_pdsm_cmd_pkg *) cmd; +} + +/* Return the payload pointer for a given pcmd */ +static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd) +{ + if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0) + return NULL; + else + return (void *)((__u8 *) pcmd + pcmd->payload_offset); +} + +#endif /* _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_ */ diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c index 142636e1a59f..ed4b49a6f1e1 100644 --- a/arch/powerpc/platforms/pseries/papr_scm.c +++ b/arch/powerpc/platforms/pseries/papr_scm.c @@ -15,13 +15,15 @@ #include <linux/seq_buf.h> #include <asm/plpar_wrappers.h> +#include <asm/papr_scm_pdsm.h> #define BIND_ANY_ADDR (~0ul) #define PAPR_SCM_DIMM_CMD_MASK \ ((1ul << ND_CMD_GET_CONFIG_SIZE) | \ (1ul << ND_CMD_GET_CONFIG_DATA) | \ - (1ul << ND_CMD_SET_CONFIG_DATA)) + (1ul << ND_CMD_SET_CONFIG_DATA) | \ + (1ul << ND_CMD_CALL)) /* DIMM health bitmap bitmap indicators */ /* SCM device is unable to persist memory contents */ @@ -350,16 +352,97 @@ static int papr_scm_meta_set(struct papr_scm_priv *p, return 0; } +/* + * Validate the inputs args to dimm-control function and return '0' if valid. + * This also does initial sanity validation to ND_CMD_CALL sub-command packages. + */ +static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf, + unsigned int buf_len) +{ + unsigned long cmd_mask = PAPR_SCM_DIMM_CMD_MASK; + struct nd_pdsm_cmd_pkg *pkg = nd_to_pdsm_cmd_pkg(buf); + struct papr_scm_priv *p; + + /* Only dimm-specific calls are supported atm */ + if (!nvdimm) + return -EINVAL; + + /* get the provider date from struct nvdimm */ + p = nvdimm_provider_data(nvdimm); + + if (!test_bit(cmd, &cmd_mask)) { + dev_dbg(&p->pdev->dev, "Unsupported cmd=%u\n", cmd); + return -EINVAL; + } else if (cmd == ND_CMD_CALL) { + + /* Verify the envelope package */ + if (!buf || buf_len < sizeof(struct nd_pdsm_cmd_pkg)) { + dev_dbg(&p->pdev->dev, "Invalid pkg size=%u\n", + buf_len); + return -EINVAL; + } + + /* Verify that the PDSM family is valid */ + if (pkg->hdr.nd_family != NVDIMM_FAMILY_PAPR_SCM) { + dev_dbg(&p->pdev->dev, "Invalid pkg family=0x%llx\n", + pkg->hdr.nd_family); + return -EINVAL; + + } + + /* We except a payload with all PDSM commands */ + if (pdsm_cmd_to_payload(pkg) == NULL) { + dev_dbg(&p->pdev->dev, + "Empty payload for sub-command=0x%llx\n", + pkg->hdr.nd_command); + return -EINVAL; + } + } + + /* Command looks valid */ + return 0; +} + +static int papr_scm_service_pdsm(struct papr_scm_priv *p, + struct nd_pdsm_cmd_pkg *call_pkg) +{ + /* unknown subcommands return error in packages */ + if (call_pkg->hdr.nd_command <= PAPR_SCM_PDSM_MIN || + call_pkg->hdr.nd_command >= PAPR_SCM_PDSM_MAX) { + dev_dbg(&p->pdev->dev, "Invalid PDSM request 0x%llx\n", + call_pkg->hdr.nd_command); + call_pkg->cmd_status = -EINVAL; + return 0; + } + + /* Depending on the DSM command call appropriate service routine */ + switch (call_pkg->hdr.nd_command) { + default: + dev_dbg(&p->pdev->dev, "Unsupported PDSM request 0x%llx\n", + call_pkg->hdr.nd_command); + call_pkg->cmd_status = -ENOENT; + return 0; + } +} + static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc) { struct nd_cmd_get_config_size *get_size_hdr; struct papr_scm_priv *p; + struct nd_pdsm_cmd_pkg *call_pkg = NULL; + int rc; - /* Only dimm-specific calls are supported atm */ - if (!nvdimm) - return -EINVAL; + /* Use a local variable in case cmd_rc pointer is NULL */ + if (cmd_rc == NULL) + cmd_rc = &rc; + + *cmd_rc = is_cmd_valid(nvdimm, cmd, buf, buf_len); + if (*cmd_rc) { + pr_debug("Invalid cmd=0x%x. Err=%d\n", cmd, *cmd_rc); + return *cmd_rc; + } p = nvdimm_provider_data(nvdimm); @@ -381,13 +464,19 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, *cmd_rc = papr_scm_meta_set(p, buf); break; + case ND_CMD_CALL: + call_pkg = nd_to_pdsm_cmd_pkg(buf); + *cmd_rc = papr_scm_service_pdsm(p, call_pkg); + break; + default: - return -EINVAL; + dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd); + *cmd_rc = -EINVAL; } dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc); - return 0; + return *cmd_rc; } static ssize_t flags_show(struct device *dev, diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h index de5d90212409..99fb60600ef8 100644 --- a/include/uapi/linux/ndctl.h +++ b/include/uapi/linux/ndctl.h @@ -244,6 +244,7 @@ struct nd_cmd_pkg { #define NVDIMM_FAMILY_HPE2 2 #define NVDIMM_FAMILY_MSFT 3 #define NVDIMM_FAMILY_HYPERV 4 +#define NVDIMM_FAMILY_PAPR_SCM 5 #define ND_IOCTL_CALL _IOWR(ND_IOCTL, ND_CMD_CALL,\ struct nd_cmd_pkg) -- 2.26.2

On Wed, May 20, 2020 at 12:30:56AM +0530, Vaibhav Jain wrote: I wonder if this should be typed big endian as you mention that it is in the commit message? Is this exclusive to 64bit? Why not u64? plpar_hcall() returns long and this function returns int and rc is declared s64? Why not have them all be long to follow plpar_hcall? > + > + /* issue the hcall */ > + rc = plpar_hcall(H_SCM_HEALTH, ret, p->drc_index); > + if (rc != H_SUCCESS) { > + dev_err(&p->pdev->dev, > + "Failed to query health information, Err:%lld\n", rc); > + rc = -ENXIO; > + goto out; > + } > + > + p->lasthealth_jiffies = jiffies; > + p->health_bitmap = ret[0] & ret[1]; > + > + dev_dbg(&p->pdev->dev, > + "Queried dimm health info. Bitmap:0x%016lx Mask:0x%016lx\n", > + ret[0], ret[1]); > +out: > + return rc; > +} > + > +/* Min interval in seconds for assuming stable dimm health */ > +#define MIN_HEALTH_QUERY_INTERVAL 60 > + > +/* Query cached health info and if needed call drc_pmem_query_health */ > +static int drc_pmem_query_health(struct papr_scm_priv *p) > +{ > + unsigned long cache_timeout; > + > + /* Protect concurrent modifications to papr_scm_priv */ > + rc = mutex_lock_interruptible(&p->health_mutex); > + if (rc) > + return rc; > + > + /* Jiffies offset for which the health data is assumed to be same */ > + cache_timeout = p->lasthealth_jiffies + > + msecs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL * 1000); > + > + /* Fetch new health info is its older than MIN_HEALTH_QUERY_INTERVAL */ > + if (time_after(jiffies, cache_timeout)) > + rc = __drc_pmem_query_health(p); And back to s64 after returning int? and back to int... Just make them long all through... Ira > + if (rc) > + return rc; > + > + /* Copy health_bitmap locally, check masks & update out buffer */ > + health = READ_ONCE(p->health_bitmap); > + > + seq_buf_init(&s, buf, PAGE_SIZE); > + if (health & PAPR_SCM_DIMM_UNARMED_MASK) > + seq_buf_printf(&s, "not_armed "); > + > + if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK) > + seq_buf_printf(&s, "flush_fail "); > + > + if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK) > + seq_buf_printf(&s, "restore_fail "); > + > + if (health & PAPR_SCM_DIMM_ENCRYPTED) > + seq_buf_printf(&s, "encrypted "); > + > + if (health & PAPR_SCM_DIMM_SMART_EVENT_MASK) > + seq_buf_printf(&s, "smart_notify "); > + > + if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED) > + seq_buf_printf(&s, "scrubbed locked "); > + > + if (seq_buf_used(&s)) > + seq_buf_printf(&s, "\n"); > + > + return seq_buf_used(&s); > +} > +DEVICE_ATTR_RO(flags); > + > +/* papr_scm specific dimm attributes */ > +static struct attribute *papr_scm_nd_attributes[] = { > + &dev_attr_flags.attr, > + NULL, > +}; > + > +static struct attribute_group papr_scm_nd_attribute_group = { > + .name = "papr", > + .attrs = papr_scm_nd_attributes, > +}; > + > +static const struct attribute_group *papr_scm_dimm_attr_groups[] = { > + &papr_scm_nd_attribute_group, > + NULL, > +}; > + > static int papr_scm_nvdimm_init(struct papr_scm_priv *p) > { > struct device *dev = &p->pdev->dev; > @@ -312,8 +474,8 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p) > dimm_flags = 0; > set_bit(NDD_LABELING, &dimm_flags); > > - p->nvdimm = nvdimm_create(p->bus, p, NULL, dimm_flags, > - PAPR_SCM_DIMM_CMD_MASK, 0, NULL); > + p->nvdimm = nvdimm_create(p->bus, p, papr_scm_dimm_attr_groups, > + dimm_flags, PAPR_SCM_DIMM_CMD_MASK, 0, NULL); > if (!p->nvdimm) { > dev_err(dev, "Error creating DIMM object for %pOF\n", p->dn); > goto err; > @@ -399,6 +561,9 @@ static int papr_scm_probe(struct platform_device *pdev) > if (!p) > return -ENOMEM; > > + /* Initialize the dimm mutex */ > + mutex_init(&p->health_mutex); > + > /* optional DT properties */ > of_property_read_u32(dn, "ibm,metadata-size", &metadata_size); > > -- > 2.26.2 > _______________________________________________ > Linux-nvdimm mailing list -- linux-nvdimm(a)lists.01.org > To unsubscribe send an email to linux-nvdimm-leave(a)lists.01.org

Thanks for reviewing this patch Ira. My responses below: Ira Weiny <ira.weiny(a)intel.com&gt; writes: Taking constrains 1 & 2 mentioned below in section "Backward Compatibility" into account, there should be a 1:1 mapping between set of valid 'payload_versions' and set of corrosponding in/out sizes. However its much more intutive and convenient to figure out the struct type to use from version number rather than sizes. For example: struct v1 { u64 a; }; sizeof(v1) == 8 struct v2 { u64 a, b; }; sizeof(v2) == 16 With version numbers its easy to figure out which type to use. However with in/out size an extra lookup is needed to identify the type to be used. Good catch. This should be Y > X. Will get this fixed. -- Cheers ~ Vaibhav

Vaibhav Jain <vaibhav(a)linux.ibm.com&gt; writes: ... This has already caused a lot of confusion, so let me try and clear it up. I will probably fail :) The value is not big endian. It's returned in a GPR (a register), from the hypervisor. The ordering of bytes in a register is not dependent on what endian we're executing in. It's true that the hypervisor will have been running big endian, and when it returns to us we will now be running little endian. But the value is unchanged, it was 0xC00000000000000 in the GPR while the HV was running and it's still 0xC00000000000000 when we return to Linux. You can see this in mambo, see below for an example. _However_, the specification of the bits in the bitmap value uses MSB 0 ordering, as is traditional for IBM documentation. That means the most significant bit, aka. the left most bit, is called "bit 0". See: https://en.wikipedia.org/wiki/Bit_numbering#MSB_0_bit_numbering That is the opposite numbering from what most people use, and in particular what most code in Linux uses, which is that bit 0 is the least significant bit. Which is where the confusion comes in. It's not that the bytes are returned in a different order, it's that the bits are numbered differently in the IBM documentation. The way to fix this kind of thing is to read the docs, and convert all the bits into correct numbering (LSB=0), and then throw away the docs ;) cheers In mambo we can set a breakpoint just before the kernel enters skiboot, towards the end of __opal_call. The kernel is running LE and skiboot runs BE. systemsim-p9 [~/skiboot/skiboot/external/mambo] b 0xc0000000000c1744 breakpoint set at [0:0:0]: 0xc0000000000c1744 (0x00000000000C1744) Enc:0x2402004C : hrfid Then run: systemsim-p9 [~/skiboot/skiboot/external/mambo] c [0:0:0]: 0xC0000000000C1744 (0x00000000000C1744) Enc:0x2402004C : hrfid INFO: 121671618: (121671618): ** Execution stopped: user (tcl), ** 121671618: ** finished running 121671618 instructions ** And we stop there, on an hrfid that we haven't executed yet. We can print r0, to see the OPAL token: systemsim-p9 [~/skiboot/skiboot/external/mambo] p r0 0x0000000000000019 ie. we're calling OPAL_CONSOLE_WRITE_BUFFER_SPACE (25). And we can print the MSR: systemsim-p9 [~/skiboot/skiboot/external/mambo] p msr 0x9000000002001033 64-bit mode (SF): 0x1 [64-bit mode] Hypervisor State (HV): 0x1 Vector Available (VEC): 0x1 Machine Check Interrupt Enable (ME): 0x1 Instruction Relocate (IR): 0x1 Data Relocate (DR): 0x1 Recoverable Interrupt (RI): 0x1 Little-Endian Mode (LE): 0x1 [little-endian] ie. we're little endian. We then step one instruction: systemsim-p9 [~/skiboot/skiboot/external/mambo] s [0:0:0]: 0x0000000030002BF0 (0x0000000030002BF0) Enc:0x7D9FFAA6 : mfspr r12,PIR Now we're in skiboot. Print the MSR again: systemsim-p9 [~/skiboot/skiboot/external/mambo] p msr 0x9000000002001002 64-bit mode (SF): 0x1 [64-bit mode] Hypervisor State (HV): 0x1 Vector Available (VEC): 0x1 Machine Check Interrupt Enable (ME): 0x1 Recoverable Interrupt (RI): 0x1 We're big endian. Print r0: systemsim-p9 [~/skiboot/skiboot/external/mambo] p r0 0x0000000000000019 r0 is unchanged!

On Wed, May 20, 2020 at 10:45:58PM +0530, Vaibhav Jain wrote: ... ... ok. Honestly masking the error return of plpar_hcall() seems a problem as well... but ok. Ira > > > > >> + > >> + /* issue the hcall */ > >> + rc = plpar_hcall(H_SCM_HEALTH, ret, p->drc_index); > >> + if (rc != H_SUCCESS) { > >> + dev_err(&p->pdev->dev, > >> + "Failed to query health information, Err:%lld\n", rc); > >> + rc = -ENXIO; > >> + goto out; > >> + } > >> + > >> + p->lasthealth_jiffies = jiffies; > >> + p->health_bitmap = ret[0] & ret[1]; > >> + > >> + dev_dbg(&p->pdev->dev, > >> + "Queried dimm health info. Bitmap:0x%016lx Mask:0x%016lx\n", > >> + ret[0], ret[1]); > >> +out: > >> + return rc; > >> +} > >> + > >> +/* Min interval in seconds for assuming stable dimm health */ > >> +#define MIN_HEALTH_QUERY_INTERVAL 60 > >> + > >> +/* Query cached health info and if needed call drc_pmem_query_health */ > >> +static int drc_pmem_query_health(struct papr_scm_priv *p) > >> +{ > >> + unsigned long cache_timeout; > >> + s64 rc; > >> + > >> + /* Protect concurrent modifications to papr_scm_priv */ > >> + rc = mutex_lock_interruptible(&p->health_mutex); > >> + if (rc) > >> + return rc; > >> + > >> + /* Jiffies offset for which the health data is assumed to be same */ > >> + cache_timeout = p->lasthealth_jiffies + > >> + msecs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL * 1000); > >> + > >> + /* Fetch new health info is its older than MIN_HEALTH_QUERY_INTERVAL */ > >> + if (time_after(jiffies, cache_timeout)) > >> + rc = __drc_pmem_query_health(p); > > > > And back to s64 after returning int? > Agree, will change 's64 rc' to 'int rc'. > > > > >> + else > >> + /* Assume cached health data is valid */ > >> + rc = 0; > >> + > >> + mutex_unlock(&p->health_mutex); > >> + return rc; > >> +} > >> > >> static int papr_scm_meta_get(struct papr_scm_priv *p, > >> struct nd_cmd_get_config_data_hdr *hdr) > >> @@ -286,6 +390,64 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, > >> return 0; > >> } > >> > >> +static ssize_t flags_show(struct device *dev, > >> + struct device_attribute *attr, char *buf) > >> +{ > >> + struct nvdimm *dimm = to_nvdimm(dev); > >> + struct papr_scm_priv *p = nvdimm_provider_data(dimm); > >> + struct seq_buf s; > >> + u64 health; > >> + int rc; > >> + > >> + rc = drc_pmem_query_health(p); > > > > and back to int... > > > drc_pmem_query_health() returns an 'int' so the type of variable 'rc' > looks correct to me. > > > Just make them long all through... > I think the return type for above all functions is 'int' with > an issue in drc_pmem_query_health() that you pointed out. > > With that fixed the usage of 'int' return type for functions will become > consistent. > > > > > Ira > > > >> + if (rc) > >> + return rc; > >> + > >> + /* Copy health_bitmap locally, check masks & update out buffer */ > >> + health = READ_ONCE(p->health_bitmap); > >> + > >> + seq_buf_init(&s, buf, PAGE_SIZE); > >> + if (health & PAPR_SCM_DIMM_UNARMED_MASK) > >> + seq_buf_printf(&s, "not_armed "); > >> + > >> + if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK) > >> + seq_buf_printf(&s, "flush_fail "); > >> + > >> + if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK) > >> + seq_buf_printf(&s, "restore_fail "); > >> + > >> + if (health & PAPR_SCM_DIMM_ENCRYPTED) > >> + seq_buf_printf(&s, "encrypted "); > >> + > >> + if (health & PAPR_SCM_DIMM_SMART_EVENT_MASK) > >> + seq_buf_printf(&s, "smart_notify "); > >> + > >> + if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED) > >> + seq_buf_printf(&s, "scrubbed locked "); > >> + > >> + if (seq_buf_used(&s)) > >> + seq_buf_printf(&s, "\n"); > >> + > >> + return seq_buf_used(&s); > >> +} > >> +DEVICE_ATTR_RO(flags); > >> + > >> +/* papr_scm specific dimm attributes */ > >> +static struct attribute *papr_scm_nd_attributes[] = { > >> + &dev_attr_flags.attr, > >> + NULL, > >> +}; > >> + > >> +static struct attribute_group papr_scm_nd_attribute_group = { > >> + .name = "papr", > >> + .attrs = papr_scm_nd_attributes, > >> +}; > >> + > >> +static const struct attribute_group *papr_scm_dimm_attr_groups[] = { > >> + &papr_scm_nd_attribute_group, > >> + NULL, > >> +}; > >> + > >> static int papr_scm_nvdimm_init(struct papr_scm_priv *p) > >> { > >> struct device *dev = &p->pdev->dev; > >> @@ -312,8 +474,8 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p) > >> dimm_flags = 0; > >> set_bit(NDD_LABELING, &dimm_flags); > >> > >> - p->nvdimm = nvdimm_create(p->bus, p, NULL, dimm_flags, > >> - PAPR_SCM_DIMM_CMD_MASK, 0, NULL); > >> + p->nvdimm = nvdimm_create(p->bus, p, papr_scm_dimm_attr_groups, > >> + dimm_flags, PAPR_SCM_DIMM_CMD_MASK, 0, NULL); > >> if (!p->nvdimm) { > >> dev_err(dev, "Error creating DIMM object for %pOF\n", p->dn); > >> goto err; > >> @@ -399,6 +561,9 @@ static int papr_scm_probe(struct platform_device *pdev) > >> if (!p) > >> return -ENOMEM; > >> > >> + /* Initialize the dimm mutex */ > >> + mutex_init(&p->health_mutex); > >> + > >> /* optional DT properties */ > >> of_property_read_u32(dn, "ibm,metadata-size", &metadata_size); > >> > >> -- > >> 2.26.2 > >> _______________________________________________ > >> Linux-nvdimm mailing list -- linux-nvdimm(a)lists.01.org > >> To unsubscribe send an email to linux-nvdimm-leave(a)lists.01.org > > -- > Cheers > ~ Vaibhav