| Message ID | 20210618163129.1912453-1-ira.weiny@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V3] cxl/mem: Account for partitionable space in ram/pmem ranges | expand |
On 21-06-18 09:31:29, ira.weiny@intel.com wrote: > From: Ira Weiny <ira.weiny@intel.com> > > Memory devices may specify volatile only, persistent only, and > partitionable space which when added together result in a total capacity. > > If Identify Memory Device.Partition Alignment != 0 the device supports > partitionable space. This partitionable space can be split between > volatile and persistent space. The total volatile and persistent sizes > are reported in Get Partition Info. ie > > active volatile memory = volatile only + partitionable volatile > active persistent memory = persistent only + partitionable persistent > > Define cxl_mem_get_partition(), check for partitionable support, and use > cxl_mem_get_partition() if applicable. This doesn't look right to me, but I'll happily stand corrected. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > Changes for V3: > Johnathan > Stop over documenting 'in bytes'... :-D > > Changes for V2: > Jonathan > Account for Get Partition Info being optional > Fix documentation > Dan > Store the active capacities in struct cxl_mem > Make cxl_mem_get_partition() static until there is a > need to export it. > --- > drivers/cxl/mem.h | 5 +++ > drivers/cxl/pci.c | 96 ++++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 96 insertions(+), 5 deletions(-) > > diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h > index 9dc34418db36..8d7bd966a298 100644 > --- a/drivers/cxl/mem.h > +++ b/drivers/cxl/mem.h > @@ -79,5 +79,10 @@ struct cxl_mem { > u64 volatile_only_bytes; > u64 persistent_only_bytes; > u64 partition_align_bytes; > + > + u64 active_volatile_bytes; > + u64 active_persistent_bytes; > + u64 next_volatile_bytes; > + u64 next_persistent_bytes; > }; > #endif /* __CXL_MEM_H__ */ > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 94b7ee08ef67..3c8b7aa8c273 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -1455,6 +1455,53 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm) > return ret; > } > > +/** > + * cxl_mem_get_partition_info - Get partition info > + * @cxlm: The device to act on > + * @active_volatile_bytes: returned active volatile capacity > + * @active_persistent_bytes: returned active persistent capacity > + * @next_volatile_bytes: return next volatile capacity > + * @next_persistent_bytes: return next persistent capacity > + * > + * Retrieve the current partition info for the device specified. If not 0, the > + * 'next' values are pending and take affect on next cold reset. > + * > + * Return: 0 if no error: or the result of the mailbox command. > + * > + * See CXL @8.2.9.5.2.1 Get Partition Info > + */ > +static int cxl_mem_get_partition_info(struct cxl_mem *cxlm, > + u64 *active_volatile_bytes, > + u64 *active_persistent_bytes, > + u64 *next_volatile_bytes, > + u64 *next_persistent_bytes) > +{ > + struct cxl_mbox_get_partition_info { > + u64 active_volatile_cap; > + u64 active_persistent_cap; > + u64 next_volatile_cap; > + u64 next_persistent_cap; > + } __packed pi; > + int rc; > + > + rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO, > + NULL, 0, &pi, sizeof(pi)); > + if (rc) > + return rc; > + > + *active_volatile_bytes = le64_to_cpu(pi.active_volatile_cap); > + *active_persistent_bytes = le64_to_cpu(pi.active_persistent_cap); > + *next_volatile_bytes = le64_to_cpu(pi.next_volatile_cap); > + *next_persistent_bytes = le64_to_cpu(pi.next_volatile_cap); > + > + *active_volatile_bytes *= CXL_CAPACITY_MULTIPLIER; > + *active_persistent_bytes *= CXL_CAPACITY_MULTIPLIER; > + *next_volatile_bytes *= CXL_CAPACITY_MULTIPLIER; > + *next_persistent_bytes *= CXL_CAPACITY_MULTIPLIER; > + > + return 0; > +} > + > /** > * cxl_mem_enumerate_cmds() - Enumerate commands for a device. > * @cxlm: The device. > @@ -1573,18 +1620,53 @@ static int cxl_mem_identify(struct cxl_mem *cxlm) > cxlm->persistent_only_bytes, > cxlm->partition_align_bytes); > > + cxlm->lsa_size = le32_to_cpu(id.lsa_size); > + memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision)); > + > + return 0; > +} > + > +static int cxl_mem_create_range_info(struct cxl_mem *cxlm) > +{ > + int rc; > + > + if (cxlm->partition_align_bytes == 0) { > + cxlm->ram_range.start = 0; > + cxlm->ram_range.end = cxlm->volatile_only_bytes - 1; > + cxlm->pmem_range.start = 0; > + cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1; > + return 0; > + } > + > + rc = cxl_mem_get_partition_info(cxlm, > + &cxlm->active_volatile_bytes, > + &cxlm->active_persistent_bytes, > + &cxlm->next_volatile_bytes, > + &cxlm->next_persistent_bytes); > + if (rc < 0) { > + dev_err(&cxlm->pdev->dev, "Failed to query partition information\n"); > + return rc; > + } If the command is not supported, you're going to get back -ENXIO here and fail. I think what you want to do also is use IDENTIFY if PARTITION_INFO doesn't exist. > + > + dev_dbg(&cxlm->pdev->dev, "Get Partition Info\n" > + " active_volatile_bytes = %#llx\n" > + " active_persistent_bytes = %#llx\n" > + " next_volatile_bytes = %#llx\n" > + " next_persistent_bytes = %#llx\n", > + cxlm->active_volatile_bytes, > + cxlm->active_persistent_bytes, > + cxlm->next_volatile_bytes, > + cxlm->next_persistent_bytes); > + > /* > * TODO: enumerate DPA map, as 'ram' and 'pmem' do not alias. > * For now, only the capacity is exported in sysfs > */ > cxlm->ram_range.start = 0; > - cxlm->ram_range.end = cxlm->volatile_only_bytes - 1; > + cxlm->ram_range.end = cxlm->active_volatile_bytes - 1; > > cxlm->pmem_range.start = 0; > - cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1; > - > - cxlm->lsa_size = le32_to_cpu(id.lsa_size); > - memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision)); > + cxlm->pmem_range.end = cxlm->active_persistent_bytes - 1; > > return 0; > } > @@ -1618,6 +1700,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (rc) > return rc; > > + rc = cxl_mem_create_range_info(cxlm); > + if (rc) > + return rc; > + Assuming my assessment is correct, I think it's time to make cxl_mem_mbox_send_cmd grow a little bit better error handling. I'd recommend something like this and then checking for -ENOTSUPP. diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index b1a5a18dba92..3b7d8a905393 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -92,6 +92,7 @@ struct mbox_cmd { size_t size_out; u16 return_code; #define CXL_MBOX_SUCCESS 0 +#define CXL_MBOX_UNSUPPORTED 0x15 }; static int cxl_mem_major; @@ -876,9 +877,15 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode, if (rc) return rc; - /* TODO: Map return code to proper kernel style errno */ - if (mbox_cmd.return_code != CXL_MBOX_SUCCESS) + switch (mbox_cmd.return_code) { + case CXL_MBOX_SUCCESS: + break; + case CXL_MBOX_UNSUPPORTED: + return -ENOTSUPP; + default: + /* TODO: Map return code to proper kernel style errno */ return -ENXIO; + } /* * Variable sized commands can't be validated and so it's up to the > return cxl_mem_add_memdev(cxlm); > } > > -- > 2.28.0.rc0.12.gb6a658bd00c9 >
On Fri, Jun 18, 2021 at 09:58:33AM -0700, Widawsky, Ben wrote: > On 21-06-18 09:31:29, ira.weiny@intel.com wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > > > Memory devices may specify volatile only, persistent only, and > > partitionable space which when added together result in a total capacity. > > > > If Identify Memory Device.Partition Alignment != 0 the device supports > > partitionable space. This partitionable space can be split between > > volatile and persistent space. The total volatile and persistent sizes > > are reported in Get Partition Info. ie > > > > active volatile memory = volatile only + partitionable volatile > > active persistent memory = persistent only + partitionable persistent > > > > Define cxl_mem_get_partition(), check for partitionable support, and use > > cxl_mem_get_partition() if applicable. > > This doesn't look right to me, but I'll happily stand corrected. > [snip] > > + > > +static int cxl_mem_create_range_info(struct cxl_mem *cxlm) > > +{ > > + int rc; > > + > > + if (cxlm->partition_align_bytes == 0) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This... > > + cxlm->ram_range.start = 0; > > + cxlm->ram_range.end = cxlm->volatile_only_bytes - 1; > > + cxlm->pmem_range.start = 0; > > + cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1; > > + return 0; > > + } > > + > > + rc = cxl_mem_get_partition_info(cxlm, > > + &cxlm->active_volatile_bytes, > > + &cxlm->active_persistent_bytes, > > + &cxlm->next_volatile_bytes, > > + &cxlm->next_persistent_bytes); > > + if (rc < 0) { > > + dev_err(&cxlm->pdev->dev, "Failed to query partition information\n"); > > + return rc; > > + } > > If the command is not supported, you're going to get back -ENXIO here and fail. > I think what you want to do also is use IDENTIFY if PARTITION_INFO doesn't > exist. ... should handle the situation where Get Partition Info is not supported. I did not see it explicitly stated but I believe if Partition Align Bytes is non-0 then partitioning is supported and Get/Set Partition Info should work. > > Assuming my assessment is correct, I think it's time to make > cxl_mem_mbox_send_cmd grow a little bit better error handling. I'd recommend > something like this and then checking for -ENOTSUPP. > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index b1a5a18dba92..3b7d8a905393 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -92,6 +92,7 @@ struct mbox_cmd { > size_t size_out; > u16 return_code; > #define CXL_MBOX_SUCCESS 0 > +#define CXL_MBOX_UNSUPPORTED 0x15 > }; > > static int cxl_mem_major; > @@ -876,9 +877,15 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode, > if (rc) > return rc; > > - /* TODO: Map return code to proper kernel style errno */ > - if (mbox_cmd.return_code != CXL_MBOX_SUCCESS) > + switch (mbox_cmd.return_code) { > + case CXL_MBOX_SUCCESS: > + break; > + case CXL_MBOX_UNSUPPORTED: > + return -ENOTSUPP; > + default: > + /* TODO: Map return code to proper kernel style errno */ > return -ENXIO; > + } I'm not opposed to adding this though. Ira
On 21-06-18 11:48:32, Ira Weiny wrote: > On Fri, Jun 18, 2021 at 09:58:33AM -0700, Widawsky, Ben wrote: > > On 21-06-18 09:31:29, ira.weiny@intel.com wrote: > > > From: Ira Weiny <ira.weiny@intel.com> > > > > > > Memory devices may specify volatile only, persistent only, and > > > partitionable space which when added together result in a total capacity. > > > > > > If Identify Memory Device.Partition Alignment != 0 the device supports > > > partitionable space. This partitionable space can be split between > > > volatile and persistent space. The total volatile and persistent sizes > > > are reported in Get Partition Info. ie > > > > > > active volatile memory = volatile only + partitionable volatile > > > active persistent memory = persistent only + partitionable persistent > > > > > > Define cxl_mem_get_partition(), check for partitionable support, and use > > > cxl_mem_get_partition() if applicable. > > > > This doesn't look right to me, but I'll happily stand corrected. > > > > [snip] > > > > + > > > +static int cxl_mem_create_range_info(struct cxl_mem *cxlm) > > > +{ > > > + int rc; > > > + > > > + if (cxlm->partition_align_bytes == 0) { > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > This... > > > > + cxlm->ram_range.start = 0; > > > + cxlm->ram_range.end = cxlm->volatile_only_bytes - 1; > > > + cxlm->pmem_range.start = 0; > > > + cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1; > > > + return 0; > > > + } > > > + > > > + rc = cxl_mem_get_partition_info(cxlm, > > > + &cxlm->active_volatile_bytes, > > > + &cxlm->active_persistent_bytes, > > > + &cxlm->next_volatile_bytes, > > > + &cxlm->next_persistent_bytes); > > > + if (rc < 0) { > > > + dev_err(&cxlm->pdev->dev, "Failed to query partition information\n"); > > > + return rc; > > > + } > > > > If the command is not supported, you're going to get back -ENXIO here and fail. > > I think what you want to do also is use IDENTIFY if PARTITION_INFO doesn't > > exist. > > ... should handle the situation where Get Partition Info is not supported. > > I did not see it explicitly stated but I believe if Partition Align Bytes is > non-0 then partitioning is supported and Get/Set Partition Info should work. Ah. I didn't notice that. Perhaps a comment with a spec reference might help the next person who comes along to look at that. My reading of it is the same (though not explicit). I wonder why they chose to put that in IDENTIFY... > > > > > Assuming my assessment is correct, I think it's time to make > > cxl_mem_mbox_send_cmd grow a little bit better error handling. I'd recommend > > something like this and then checking for -ENOTSUPP. > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index b1a5a18dba92..3b7d8a905393 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -92,6 +92,7 @@ struct mbox_cmd { > > size_t size_out; > > u16 return_code; > > #define CXL_MBOX_SUCCESS 0 > > +#define CXL_MBOX_UNSUPPORTED 0x15 > > }; > > > > static int cxl_mem_major; > > @@ -876,9 +877,15 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode, > > if (rc) > > return rc; > > > > - /* TODO: Map return code to proper kernel style errno */ > > - if (mbox_cmd.return_code != CXL_MBOX_SUCCESS) > > + switch (mbox_cmd.return_code) { > > + case CXL_MBOX_SUCCESS: > > + break; > > + case CXL_MBOX_UNSUPPORTED: > > + return -ENOTSUPP; > > + default: > > + /* TODO: Map return code to proper kernel style errno */ > > return -ENXIO; > > + } > > > I'm not opposed to adding this though. You've resolved my concern, so it's up to you. The other option would be to check the commands if the command got enabled in cxlm->enabled_cmds. > > Ira
diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h index 9dc34418db36..8d7bd966a298 100644 --- a/drivers/cxl/mem.h +++ b/drivers/cxl/mem.h @@ -79,5 +79,10 @@ struct cxl_mem { u64 volatile_only_bytes; u64 persistent_only_bytes; u64 partition_align_bytes; + + u64 active_volatile_bytes; + u64 active_persistent_bytes; + u64 next_volatile_bytes; + u64 next_persistent_bytes; }; #endif /* __CXL_MEM_H__ */ diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 94b7ee08ef67..3c8b7aa8c273 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -1455,6 +1455,53 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm) return ret; } +/** + * cxl_mem_get_partition_info - Get partition info + * @cxlm: The device to act on + * @active_volatile_bytes: returned active volatile capacity + * @active_persistent_bytes: returned active persistent capacity + * @next_volatile_bytes: return next volatile capacity + * @next_persistent_bytes: return next persistent capacity + * + * Retrieve the current partition info for the device specified. If not 0, the + * 'next' values are pending and take affect on next cold reset. + * + * Return: 0 if no error: or the result of the mailbox command. + * + * See CXL @8.2.9.5.2.1 Get Partition Info + */ +static int cxl_mem_get_partition_info(struct cxl_mem *cxlm, + u64 *active_volatile_bytes, + u64 *active_persistent_bytes, + u64 *next_volatile_bytes, + u64 *next_persistent_bytes) +{ + struct cxl_mbox_get_partition_info { + u64 active_volatile_cap; + u64 active_persistent_cap; + u64 next_volatile_cap; + u64 next_persistent_cap; + } __packed pi; + int rc; + + rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO, + NULL, 0, &pi, sizeof(pi)); + if (rc) + return rc; + + *active_volatile_bytes = le64_to_cpu(pi.active_volatile_cap); + *active_persistent_bytes = le64_to_cpu(pi.active_persistent_cap); + *next_volatile_bytes = le64_to_cpu(pi.next_volatile_cap); + *next_persistent_bytes = le64_to_cpu(pi.next_volatile_cap); + + *active_volatile_bytes *= CXL_CAPACITY_MULTIPLIER; + *active_persistent_bytes *= CXL_CAPACITY_MULTIPLIER; + *next_volatile_bytes *= CXL_CAPACITY_MULTIPLIER; + *next_persistent_bytes *= CXL_CAPACITY_MULTIPLIER; + + return 0; +} + /** * cxl_mem_enumerate_cmds() - Enumerate commands for a device. * @cxlm: The device. @@ -1573,18 +1620,53 @@ static int cxl_mem_identify(struct cxl_mem *cxlm) cxlm->persistent_only_bytes, cxlm->partition_align_bytes); + cxlm->lsa_size = le32_to_cpu(id.lsa_size); + memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision)); + + return 0; +} + +static int cxl_mem_create_range_info(struct cxl_mem *cxlm) +{ + int rc; + + if (cxlm->partition_align_bytes == 0) { + cxlm->ram_range.start = 0; + cxlm->ram_range.end = cxlm->volatile_only_bytes - 1; + cxlm->pmem_range.start = 0; + cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1; + return 0; + } + + rc = cxl_mem_get_partition_info(cxlm, + &cxlm->active_volatile_bytes, + &cxlm->active_persistent_bytes, + &cxlm->next_volatile_bytes, + &cxlm->next_persistent_bytes); + if (rc < 0) { + dev_err(&cxlm->pdev->dev, "Failed to query partition information\n"); + return rc; + } + + dev_dbg(&cxlm->pdev->dev, "Get Partition Info\n" + " active_volatile_bytes = %#llx\n" + " active_persistent_bytes = %#llx\n" + " next_volatile_bytes = %#llx\n" + " next_persistent_bytes = %#llx\n", + cxlm->active_volatile_bytes, + cxlm->active_persistent_bytes, + cxlm->next_volatile_bytes, + cxlm->next_persistent_bytes); + /* * TODO: enumerate DPA map, as 'ram' and 'pmem' do not alias. * For now, only the capacity is exported in sysfs */ cxlm->ram_range.start = 0; - cxlm->ram_range.end = cxlm->volatile_only_bytes - 1; + cxlm->ram_range.end = cxlm->active_volatile_bytes - 1; cxlm->pmem_range.start = 0; - cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1; - - cxlm->lsa_size = le32_to_cpu(id.lsa_size); - memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision)); + cxlm->pmem_range.end = cxlm->active_persistent_bytes - 1; return 0; } @@ -1618,6 +1700,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (rc) return rc; + rc = cxl_mem_create_range_info(cxlm); + if (rc) + return rc; + return cxl_mem_add_memdev(cxlm); }