Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Tue, 2015-04-28 at 10:47 -0600, Toshi Kani wrote: > On Wed, 2015-04-22 at 13:00 -0700, Dan Williams wrote: > > On Wed, Apr 22, 2015 at 12:38 PM, Toshi Kani wrote: > > > On Wed, 2015-04-22 at 12:28 -0700, Dan Williams wrote: > > >> On Wed, Apr 22, 2015 at 11:23 AM, Toshi Kani wrote: > > >> > On Wed, 2015-04-22 at 11:20 -0700, Dan Williams wrote: > > >> >> On Wed, Apr 22, 2015 at 11:00 AM, Linda Knippers > > >> >> wrote: > > >> >> Wait, point of clarification, DCRs (dimm-control-regions) have RFICs, > > >> >> not MEMDEVs (memory-device-to-spa-mapping). Toshi's original report > > >> >> was that an NFIT with a SPA+MEMDEV was failing to enable a PMEM > > >> >> device. That specific problem can be fixed by either deleting the > > >> >> MEMDEV, or adding a DCR. > > >> > > > >> > By a DCR, do you mean a DCR structure or SPA with Control Region GUID? > > >> > > >> Hmm, I meant a DCR as defined below. I agree you would not need a > > >> "SPA-DCR". > > >> > > >> > Adding a DCR structure does not solve this issue since it requires SPA > > >> > with Control Region GUID, which battery-backed DIMMs do not have. > > >> > > >> I would not go that far, half of a DCR entry is relevant for any > > >> NVDIMM, and half is only relevant if a DIMM offers BLK access: > > >> > > >> struct acpi_nfit_dcr { > > >> u16 type; > > >> u16 length; > > >> u16 dcr_index; > > >> u16 vendor_id; > > >> u16 device_id; > > >> u16 revision_id; > > >> u16 sub_vendor_id; > > >> u16 sub_device_id; > > >> u16 sub_revision_id; > > >> u8 reserved[6]; > > >> u32 serial_number; > > >> u16 fic; > > >> < BLK relevant fields start here < > > >> u16 num_bcw; > > >> u64 bcw_size; > > >> u64 cmd_offset; > > >> u64 cmd_size; > > >> u64 status_offset; > > >> u64 status_size; > > >> u16 flags; > > >> u8 reserved2[6]; > > >> }; > > > > > > Yes, we do have a DCR entry. But we do not have a SPA-DCR. > > > > Got it. will fix. > > Attached is an example implementation of the NFIT table with 2 > battery-backed NVDIMM cards, which I have used for testing. I hope this > provides a good example of an NFIT table with SPA(PMEM), MEMDEV and DCR > entries, which allows optional _DSMs for battery-backed NVDIMMs as > necessary. > > HP is also defining _DSM method for battery-backed NVDIMMs, and will > share the spec when it is ready. Sorry, using ".txt" extension to a Linux text file caused my mailer to perform some unnecessary conversion... Attached is the same file without ".txt" this time. -Toshi hp_nfit Description: Binary data
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Wed, 2015-04-22 at 13:00 -0700, Dan Williams wrote: > On Wed, Apr 22, 2015 at 12:38 PM, Toshi Kani wrote: > > On Wed, 2015-04-22 at 12:28 -0700, Dan Williams wrote: > >> On Wed, Apr 22, 2015 at 11:23 AM, Toshi Kani wrote: > >> > On Wed, 2015-04-22 at 11:20 -0700, Dan Williams wrote: > >> >> On Wed, Apr 22, 2015 at 11:00 AM, Linda Knippers > >> >> wrote: > >> >> Wait, point of clarification, DCRs (dimm-control-regions) have RFICs, > >> >> not MEMDEVs (memory-device-to-spa-mapping). Toshi's original report > >> >> was that an NFIT with a SPA+MEMDEV was failing to enable a PMEM > >> >> device. That specific problem can be fixed by either deleting the > >> >> MEMDEV, or adding a DCR. > >> > > >> > By a DCR, do you mean a DCR structure or SPA with Control Region GUID? > >> > >> Hmm, I meant a DCR as defined below. I agree you would not need a > >> "SPA-DCR". > >> > >> > Adding a DCR structure does not solve this issue since it requires SPA > >> > with Control Region GUID, which battery-backed DIMMs do not have. > >> > >> I would not go that far, half of a DCR entry is relevant for any > >> NVDIMM, and half is only relevant if a DIMM offers BLK access: > >> > >> struct acpi_nfit_dcr { > >> u16 type; > >> u16 length; > >> u16 dcr_index; > >> u16 vendor_id; > >> u16 device_id; > >> u16 revision_id; > >> u16 sub_vendor_id; > >> u16 sub_device_id; > >> u16 sub_revision_id; > >> u8 reserved[6]; > >> u32 serial_number; > >> u16 fic; > >> < BLK relevant fields start here < > >> u16 num_bcw; > >> u64 bcw_size; > >> u64 cmd_offset; > >> u64 cmd_size; > >> u64 status_offset; > >> u64 status_size; > >> u16 flags; > >> u8 reserved2[6]; > >> }; > > > > Yes, we do have a DCR entry. But we do not have a SPA-DCR. > > Got it. will fix. Attached is an example implementation of the NFIT table with 2 battery-backed NVDIMM cards, which I have used for testing. I hope this provides a good example of an NFIT table with SPA(PMEM), MEMDEV and DCR entries, which allows optional _DSMs for battery-backed NVDIMMs as necessary. HP is also defining _DSM method for battery-backed NVDIMMs, and will share the spec when it is ready. Thanks, -Toshi Thanks, -Toshi ��* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * N V D I M M F i r m w a r e I n t e r f a c e T a b l e * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * N F I T a d d r e s s . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 0 x 0 0 0 0 0 0 0 0 7 B 7 E 7 0 0 0 T a b l e H e a d e r : S i g n a t u r e . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . ' N F I T ' L e n g t h . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 0 x 0 0 0 0 0 1 3 8 R e v i s i o n . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 0 x 0 1 C h e c k s u m . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 0 x 4 A O E M I D . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . ' H P ' O E M T a b l e I D . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . ' P r o L i a n t ' O E M R e v i s i o n . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 0 x 0 0 0 0 0 0 0 1 C r e a t o r I D . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . ' H P ' C r e a t o r R e v i s i o n . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 0 x 0 0 0 0 0 0 0 1 T a b l e C o n t e n t s : * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * N F I T S y s t e m P h y s i c a l A d d r e s s R a n g e S t r u c t u r e * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * R a n g e I n d e x . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 0 x 0 0 0 1 F l a g s . . . . . . . . . . . . . . . . . .
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Wed, Apr 22, 2015 at 12:38 PM, Toshi Kani wrote: > On Wed, 2015-04-22 at 12:28 -0700, Dan Williams wrote: >> On Wed, Apr 22, 2015 at 11:23 AM, Toshi Kani wrote: >> > On Wed, 2015-04-22 at 11:20 -0700, Dan Williams wrote: >> >> On Wed, Apr 22, 2015 at 11:00 AM, Linda Knippers >> >> wrote: >> >> Wait, point of clarification, DCRs (dimm-control-regions) have RFICs, >> >> not MEMDEVs (memory-device-to-spa-mapping). Toshi's original report >> >> was that an NFIT with a SPA+MEMDEV was failing to enable a PMEM >> >> device. That specific problem can be fixed by either deleting the >> >> MEMDEV, or adding a DCR. >> > >> > By a DCR, do you mean a DCR structure or SPA with Control Region GUID? >> >> Hmm, I meant a DCR as defined below. I agree you would not need a "SPA-DCR". >> >> > Adding a DCR structure does not solve this issue since it requires SPA >> > with Control Region GUID, which battery-backed DIMMs do not have. >> >> I would not go that far, half of a DCR entry is relevant for any >> NVDIMM, and half is only relevant if a DIMM offers BLK access: >> >> struct acpi_nfit_dcr { >> u16 type; >> u16 length; >> u16 dcr_index; >> u16 vendor_id; >> u16 device_id; >> u16 revision_id; >> u16 sub_vendor_id; >> u16 sub_device_id; >> u16 sub_revision_id; >> u8 reserved[6]; >> u32 serial_number; >> u16 fic; >> < BLK relevant fields start here < >> u16 num_bcw; >> u64 bcw_size; >> u64 cmd_offset; >> u64 cmd_size; >> u64 status_offset; >> u64 status_size; >> u16 flags; >> u8 reserved2[6]; >> }; > > Yes, we do have a DCR entry. But we do not have a SPA-DCR. Got it. will fix. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Wed, 2015-04-22 at 12:28 -0700, Dan Williams wrote: > On Wed, Apr 22, 2015 at 11:23 AM, Toshi Kani wrote: > > On Wed, 2015-04-22 at 11:20 -0700, Dan Williams wrote: > >> On Wed, Apr 22, 2015 at 11:00 AM, Linda Knippers > >> wrote: > >> Wait, point of clarification, DCRs (dimm-control-regions) have RFICs, > >> not MEMDEVs (memory-device-to-spa-mapping). Toshi's original report > >> was that an NFIT with a SPA+MEMDEV was failing to enable a PMEM > >> device. That specific problem can be fixed by either deleting the > >> MEMDEV, or adding a DCR. > > > > By a DCR, do you mean a DCR structure or SPA with Control Region GUID? > > Hmm, I meant a DCR as defined below. I agree you would not need a "SPA-DCR". > > > Adding a DCR structure does not solve this issue since it requires SPA > > with Control Region GUID, which battery-backed DIMMs do not have. > > I would not go that far, half of a DCR entry is relevant for any > NVDIMM, and half is only relevant if a DIMM offers BLK access: > > struct acpi_nfit_dcr { > u16 type; > u16 length; > u16 dcr_index; > u16 vendor_id; > u16 device_id; > u16 revision_id; > u16 sub_vendor_id; > u16 sub_device_id; > u16 sub_revision_id; > u8 reserved[6]; > u32 serial_number; > u16 fic; > < BLK relevant fields start here < > u16 num_bcw; > u64 bcw_size; > u64 cmd_offset; > u64 cmd_size; > u64 status_offset; > u64 status_size; > u16 flags; > u8 reserved2[6]; > }; Yes, we do have a DCR entry. But we do not have a SPA-DCR. The previous issue I reported to nd_mem_init() was caused by the fact that there was no "SPA-DCR". nd_mem_init() requires SPA-DCR to initialize nd_mem objects. > >> Of course, if you add a DCR with a different intended DSM layout than > >> the DSM-example-interface the driver will need to add support for > >> handling that case. > > > > Yes, we consider to add different _DSMs for management. We do not need > > the nd_acpi driver to support it now, but we need this framework to work > > without the DSM-example-interface present. > > > > One possible workaround is that I could ignore MEMDEV entries that do > not have a corresponding DCR. This would enable nd_namespace_io > devices to be surfaced for your use case. Would that work for you? > I.e. do you need the nfit_handle exposed? We have MEMDEV entries and their corresponding DCR entries. ACPI 6.0 states that NVDIMM control region structure index must contain a non-zero value in a MEMDEV entry, so I think they must correspond. Yes, we need this framework to enumerate all entries. Thanks, -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Wed, Apr 22, 2015 at 11:23 AM, Toshi Kani wrote: > On Wed, 2015-04-22 at 11:20 -0700, Dan Williams wrote: >> On Wed, Apr 22, 2015 at 11:00 AM, Linda Knippers >> wrote: >> Wait, point of clarification, DCRs (dimm-control-regions) have RFICs, >> not MEMDEVs (memory-device-to-spa-mapping). Toshi's original report >> was that an NFIT with a SPA+MEMDEV was failing to enable a PMEM >> device. That specific problem can be fixed by either deleting the >> MEMDEV, or adding a DCR. > > By a DCR, do you mean a DCR structure or SPA with Control Region GUID? Hmm, I meant a DCR as defined below. I agree you would not need a "SPA-DCR". > Adding a DCR structure does not solve this issue since it requires SPA > with Control Region GUID, which battery-backed DIMMs do not have. I would not go that far, half of a DCR entry is relevant for any NVDIMM, and half is only relevant if a DIMM offers BLK access: struct acpi_nfit_dcr { u16 type; u16 length; u16 dcr_index; u16 vendor_id; u16 device_id; u16 revision_id; u16 sub_vendor_id; u16 sub_device_id; u16 sub_revision_id; u8 reserved[6]; u32 serial_number; u16 fic; < BLK relevant fields start here < u16 num_bcw; u64 bcw_size; u64 cmd_offset; u64 cmd_size; u64 status_offset; u64 status_size; u16 flags; u8 reserved2[6]; }; >> Of course, if you add a DCR with a different intended DSM layout than >> the DSM-example-interface the driver will need to add support for >> handling that case. > > Yes, we consider to add different _DSMs for management. We do not need > the nd_acpi driver to support it now, but we need this framework to work > without the DSM-example-interface present. > One possible workaround is that I could ignore MEMDEV entries that do not have a corresponding DCR. This would enable nd_namespace_io devices to be surfaced for your use case. Would that work for you? I.e. do you need the nfit_handle exposed? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Wed, 2015-04-22 at 11:20 -0700, Dan Williams wrote: > On Wed, Apr 22, 2015 at 11:00 AM, Linda Knippers > wrote: > > On 4/22/2015 1:03 PM, Dan Williams wrote: > >> On Wed, Apr 22, 2015 at 9:39 AM, Toshi Kani wrote: > >>> On Tue, 2015-04-21 at 13:35 -0700, Dan Williams wrote: > On Tue, Apr 21, 2015 at 12:55 PM, Toshi Kani wrote: > > On Tue, 2015-04-21 at 12:58 -0700, Dan Williams wrote: > >> On Tue, Apr 21, 2015 at 12:35 PM, Toshi Kani wrote: > >>> On Fri, 2015-04-17 at 21:35 -0400, Dan Williams wrote: > >>> : > + > +static int nd_mem_init(struct nd_bus *nd_bus) > +{ > + struct nd_spa *nd_spa; > + > + /* > + * For each SPA-DCR address range find its corresponding > + * MEMDEV(s). From each MEMDEV find the corresponding DCR. > + * Then, try to find a SPA-BDW and a corresponding BDW that > + * references the DCR. Throw it all into an nd_mem object. > + * Note, that BDWs are optional. > + */ > + list_for_each_entry(nd_spa, &nd_bus->spas, list) { > + u16 spa_index = readw(&nd_spa->nfit_spa->spa_index); > + int type = nfit_spa_type(nd_spa->nfit_spa); > + struct nd_mem *nd_mem, *found; > + struct nd_memdev *nd_memdev; > + u16 dcr_index; > + > + if (type != NFIT_SPA_DCR) > + continue; > >>> > >>> This function requires NFIT_SPA_DCR, SPA Range Structure with NVDIMM > >>> Control Region GUID, for initializing an nd_mem object. However, > >>> battery-backed DIMMs do not have such control region SPA. IIUC, the > >>> NFIT spec does not require NFIT_SPA_DCR. > >>> > >>> Can you change this function to work with NFIT_SPA_PM as well? > >> > >> NFIT_SPA_PM ranges are handled separately from nd_mem_init(). See > >> nd_region_create() in patch 10. > > > > If nd_mem_init() does not initialize nd_mem objects, nd_bus_probe() in > > core.c fails in nd_bus_init_interleave_sets() and skips all subsequent > > nd_bus_xxx() calls. So, nd_region_create() won't be called. > > > > nd_bus_init_interleave_sets() fails because init_interleave_set() > > returns -ENODEV if (!nd_mem). > > Ah, ok your test case is specifying PMEM backed by memory device > info. We have a test case for simple ranges (nfit_test1_setup()), but > it doesn't hit this bug because it does not specify any memory-device > tables. > >>> > >>> Yes, we have NFIT table with SPA range (PM), memory device to SPA, and > >>> NVDIMM control region structures. With the memory device to SPA > >>> structure, this code requires full sets of information, including the > >>> namespace label data in _DSM [1], which is outside of ACPI 6.0 and is > >>> optional. Battery-backed DIMMs do not have such label data. > >> > >> This is what "nd_namespace_io" devices are for, they do not require labels. > >> > >> Question, if you don't have labels and you don't have DSMs then why > >> publish a MEMDEV table at all? Why not simply publish an anonymous > >> range? See nfit_test1_setup(). > > > > The MEMDEV table provides useful information, and there may be _DSMs, > > perhaps just not the same _DSM as some other devices. > > > >>> It needs > >>> to work with NFIT table with these structures without this _DSM or with > >>> a different type of _DSM which this code may or may not need to support. > >>> It should also check Region Format Interface Code (RFIC) in the NVDIMM > >>> control region structure before assuming this _DSM is present to > >>> implement RFIC 0x0201. > >> > >> Ok I can look into adding this check, but I don't think it is > >> necessary if you simply refrain from publishing a MEMDEV entry. > > > > But we need the MEMDEV. And as Toshi mentions, we could have other > > RFICs with other _DSMs than your example. That's why there is an RFIC. > > Wait, point of clarification, DCRs (dimm-control-regions) have RFICs, > not MEMDEVs (memory-device-to-spa-mapping). Toshi's original report > was that an NFIT with a SPA+MEMDEV was failing to enable a PMEM > device. That specific problem can be fixed by either deleting the > MEMDEV, or adding a DCR. By a DCR, do you mean a DCR structure or SPA with Control Region GUID? Adding a DCR structure does not solve this issue since it requires SPA with Control Region GUID, which battery-backed DIMMs do not have. > Of course, if you add a DCR with a different intended DSM layout than > the DSM-example-interface the driver will need to add support for > handling that case. Yes, we consider to add different _DSMs for management. We do not need the nd_acpi driver to support it now, but we need this framework to work without the DSM-example-interface present. Thanks, -Toshi -- To unsubscribe from
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Wed, Apr 22, 2015 at 11:00 AM, Linda Knippers wrote: > On 4/22/2015 1:03 PM, Dan Williams wrote: >> On Wed, Apr 22, 2015 at 9:39 AM, Toshi Kani wrote: >>> On Tue, 2015-04-21 at 13:35 -0700, Dan Williams wrote: On Tue, Apr 21, 2015 at 12:55 PM, Toshi Kani wrote: > On Tue, 2015-04-21 at 12:58 -0700, Dan Williams wrote: >> On Tue, Apr 21, 2015 at 12:35 PM, Toshi Kani wrote: >>> On Fri, 2015-04-17 at 21:35 -0400, Dan Williams wrote: >>> : + +static int nd_mem_init(struct nd_bus *nd_bus) +{ + struct nd_spa *nd_spa; + + /* + * For each SPA-DCR address range find its corresponding + * MEMDEV(s). From each MEMDEV find the corresponding DCR. + * Then, try to find a SPA-BDW and a corresponding BDW that + * references the DCR. Throw it all into an nd_mem object. + * Note, that BDWs are optional. + */ + list_for_each_entry(nd_spa, &nd_bus->spas, list) { + u16 spa_index = readw(&nd_spa->nfit_spa->spa_index); + int type = nfit_spa_type(nd_spa->nfit_spa); + struct nd_mem *nd_mem, *found; + struct nd_memdev *nd_memdev; + u16 dcr_index; + + if (type != NFIT_SPA_DCR) + continue; >>> >>> This function requires NFIT_SPA_DCR, SPA Range Structure with NVDIMM >>> Control Region GUID, for initializing an nd_mem object. However, >>> battery-backed DIMMs do not have such control region SPA. IIUC, the >>> NFIT spec does not require NFIT_SPA_DCR. >>> >>> Can you change this function to work with NFIT_SPA_PM as well? >> >> NFIT_SPA_PM ranges are handled separately from nd_mem_init(). See >> nd_region_create() in patch 10. > > If nd_mem_init() does not initialize nd_mem objects, nd_bus_probe() in > core.c fails in nd_bus_init_interleave_sets() and skips all subsequent > nd_bus_xxx() calls. So, nd_region_create() won't be called. > > nd_bus_init_interleave_sets() fails because init_interleave_set() > returns -ENODEV if (!nd_mem). Ah, ok your test case is specifying PMEM backed by memory device info. We have a test case for simple ranges (nfit_test1_setup()), but it doesn't hit this bug because it does not specify any memory-device tables. >>> >>> Yes, we have NFIT table with SPA range (PM), memory device to SPA, and >>> NVDIMM control region structures. With the memory device to SPA >>> structure, this code requires full sets of information, including the >>> namespace label data in _DSM [1], which is outside of ACPI 6.0 and is >>> optional. Battery-backed DIMMs do not have such label data. >> >> This is what "nd_namespace_io" devices are for, they do not require labels. >> >> Question, if you don't have labels and you don't have DSMs then why >> publish a MEMDEV table at all? Why not simply publish an anonymous >> range? See nfit_test1_setup(). > > The MEMDEV table provides useful information, and there may be _DSMs, > perhaps just not the same _DSM as some other devices. > >>> It needs >>> to work with NFIT table with these structures without this _DSM or with >>> a different type of _DSM which this code may or may not need to support. >>> It should also check Region Format Interface Code (RFIC) in the NVDIMM >>> control region structure before assuming this _DSM is present to >>> implement RFIC 0x0201. >> >> Ok I can look into adding this check, but I don't think it is >> necessary if you simply refrain from publishing a MEMDEV entry. > > But we need the MEMDEV. And as Toshi mentions, we could have other > RFICs with other _DSMs than your example. That's why there is an RFIC. Wait, point of clarification, DCRs (dimm-control-regions) have RFICs, not MEMDEVs (memory-device-to-spa-mapping). Toshi's original report was that an NFIT with a SPA+MEMDEV was failing to enable a PMEM device. That specific problem can be fixed by either deleting the MEMDEV, or adding a DCR. Of course, if you add a DCR with a different intended DSM layout than the DSM-example-interface the driver will need to add support for handling that case. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On 4/22/2015 1:03 PM, Dan Williams wrote: > On Wed, Apr 22, 2015 at 9:39 AM, Toshi Kani wrote: >> On Tue, 2015-04-21 at 13:35 -0700, Dan Williams wrote: >>> On Tue, Apr 21, 2015 at 12:55 PM, Toshi Kani wrote: On Tue, 2015-04-21 at 12:58 -0700, Dan Williams wrote: > On Tue, Apr 21, 2015 at 12:35 PM, Toshi Kani wrote: >> On Fri, 2015-04-17 at 21:35 -0400, Dan Williams wrote: >> : >>> + >>> +static int nd_mem_init(struct nd_bus *nd_bus) >>> +{ >>> + struct nd_spa *nd_spa; >>> + >>> + /* >>> + * For each SPA-DCR address range find its corresponding >>> + * MEMDEV(s). From each MEMDEV find the corresponding DCR. >>> + * Then, try to find a SPA-BDW and a corresponding BDW that >>> + * references the DCR. Throw it all into an nd_mem object. >>> + * Note, that BDWs are optional. >>> + */ >>> + list_for_each_entry(nd_spa, &nd_bus->spas, list) { >>> + u16 spa_index = readw(&nd_spa->nfit_spa->spa_index); >>> + int type = nfit_spa_type(nd_spa->nfit_spa); >>> + struct nd_mem *nd_mem, *found; >>> + struct nd_memdev *nd_memdev; >>> + u16 dcr_index; >>> + >>> + if (type != NFIT_SPA_DCR) >>> + continue; >> >> This function requires NFIT_SPA_DCR, SPA Range Structure with NVDIMM >> Control Region GUID, for initializing an nd_mem object. However, >> battery-backed DIMMs do not have such control region SPA. IIUC, the >> NFIT spec does not require NFIT_SPA_DCR. >> >> Can you change this function to work with NFIT_SPA_PM as well? > > NFIT_SPA_PM ranges are handled separately from nd_mem_init(). See > nd_region_create() in patch 10. If nd_mem_init() does not initialize nd_mem objects, nd_bus_probe() in core.c fails in nd_bus_init_interleave_sets() and skips all subsequent nd_bus_xxx() calls. So, nd_region_create() won't be called. nd_bus_init_interleave_sets() fails because init_interleave_set() returns -ENODEV if (!nd_mem). >>> >>> Ah, ok your test case is specifying PMEM backed by memory device >>> info. We have a test case for simple ranges (nfit_test1_setup()), but >>> it doesn't hit this bug because it does not specify any memory-device >>> tables. >> >> Yes, we have NFIT table with SPA range (PM), memory device to SPA, and >> NVDIMM control region structures. With the memory device to SPA >> structure, this code requires full sets of information, including the >> namespace label data in _DSM [1], which is outside of ACPI 6.0 and is >> optional. Battery-backed DIMMs do not have such label data. > > This is what "nd_namespace_io" devices are for, they do not require labels. > > Question, if you don't have labels and you don't have DSMs then why > publish a MEMDEV table at all? Why not simply publish an anonymous > range? See nfit_test1_setup(). The MEMDEV table provides useful information, and there may be _DSMs, perhaps just not the same _DSM as some other devices. >> It needs >> to work with NFIT table with these structures without this _DSM or with >> a different type of _DSM which this code may or may not need to support. >> It should also check Region Format Interface Code (RFIC) in the NVDIMM >> control region structure before assuming this _DSM is present to >> implement RFIC 0x0201. > > Ok I can look into adding this check, but I don't think it is > necessary if you simply refrain from publishing a MEMDEV entry. But we need the MEMDEV. And as Toshi mentions, we could have other RFICs with other _DSMs than your example. That's why there is an RFIC. -- ljk > ___ > Linux-nvdimm mailing list > linux-nvd...@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Wed, Apr 22, 2015 at 9:39 AM, Toshi Kani wrote: > On Tue, 2015-04-21 at 13:35 -0700, Dan Williams wrote: >> On Tue, Apr 21, 2015 at 12:55 PM, Toshi Kani wrote: >> > On Tue, 2015-04-21 at 12:58 -0700, Dan Williams wrote: >> >> On Tue, Apr 21, 2015 at 12:35 PM, Toshi Kani wrote: >> >> > On Fri, 2015-04-17 at 21:35 -0400, Dan Williams wrote: >> >> > : >> >> >> + >> >> >> +static int nd_mem_init(struct nd_bus *nd_bus) >> >> >> +{ >> >> >> + struct nd_spa *nd_spa; >> >> >> + >> >> >> + /* >> >> >> + * For each SPA-DCR address range find its corresponding >> >> >> + * MEMDEV(s). From each MEMDEV find the corresponding DCR. >> >> >> + * Then, try to find a SPA-BDW and a corresponding BDW that >> >> >> + * references the DCR. Throw it all into an nd_mem object. >> >> >> + * Note, that BDWs are optional. >> >> >> + */ >> >> >> + list_for_each_entry(nd_spa, &nd_bus->spas, list) { >> >> >> + u16 spa_index = readw(&nd_spa->nfit_spa->spa_index); >> >> >> + int type = nfit_spa_type(nd_spa->nfit_spa); >> >> >> + struct nd_mem *nd_mem, *found; >> >> >> + struct nd_memdev *nd_memdev; >> >> >> + u16 dcr_index; >> >> >> + >> >> >> + if (type != NFIT_SPA_DCR) >> >> >> + continue; >> >> > >> >> > This function requires NFIT_SPA_DCR, SPA Range Structure with NVDIMM >> >> > Control Region GUID, for initializing an nd_mem object. However, >> >> > battery-backed DIMMs do not have such control region SPA. IIUC, the >> >> > NFIT spec does not require NFIT_SPA_DCR. >> >> > >> >> > Can you change this function to work with NFIT_SPA_PM as well? >> >> >> >> NFIT_SPA_PM ranges are handled separately from nd_mem_init(). See >> >> nd_region_create() in patch 10. >> > >> > If nd_mem_init() does not initialize nd_mem objects, nd_bus_probe() in >> > core.c fails in nd_bus_init_interleave_sets() and skips all subsequent >> > nd_bus_xxx() calls. So, nd_region_create() won't be called. >> > >> > nd_bus_init_interleave_sets() fails because init_interleave_set() >> > returns -ENODEV if (!nd_mem). >> >> Ah, ok your test case is specifying PMEM backed by memory device >> info. We have a test case for simple ranges (nfit_test1_setup()), but >> it doesn't hit this bug because it does not specify any memory-device >> tables. > > Yes, we have NFIT table with SPA range (PM), memory device to SPA, and > NVDIMM control region structures. With the memory device to SPA > structure, this code requires full sets of information, including the > namespace label data in _DSM [1], which is outside of ACPI 6.0 and is > optional. Battery-backed DIMMs do not have such label data. This is what "nd_namespace_io" devices are for, they do not require labels. Question, if you don't have labels and you don't have DSMs then why publish a MEMDEV table at all? Why not simply publish an anonymous range? See nfit_test1_setup(). > It needs > to work with NFIT table with these structures without this _DSM or with > a different type of _DSM which this code may or may not need to support. > It should also check Region Format Interface Code (RFIC) in the NVDIMM > control region structure before assuming this _DSM is present to > implement RFIC 0x0201. Ok I can look into adding this check, but I don't think it is necessary if you simply refrain from publishing a MEMDEV entry. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Tue, 2015-04-21 at 13:35 -0700, Dan Williams wrote: > On Tue, Apr 21, 2015 at 12:55 PM, Toshi Kani wrote: > > On Tue, 2015-04-21 at 12:58 -0700, Dan Williams wrote: > >> On Tue, Apr 21, 2015 at 12:35 PM, Toshi Kani wrote: > >> > On Fri, 2015-04-17 at 21:35 -0400, Dan Williams wrote: > >> > : > >> >> + > >> >> +static int nd_mem_init(struct nd_bus *nd_bus) > >> >> +{ > >> >> + struct nd_spa *nd_spa; > >> >> + > >> >> + /* > >> >> + * For each SPA-DCR address range find its corresponding > >> >> + * MEMDEV(s). From each MEMDEV find the corresponding DCR. > >> >> + * Then, try to find a SPA-BDW and a corresponding BDW that > >> >> + * references the DCR. Throw it all into an nd_mem object. > >> >> + * Note, that BDWs are optional. > >> >> + */ > >> >> + list_for_each_entry(nd_spa, &nd_bus->spas, list) { > >> >> + u16 spa_index = readw(&nd_spa->nfit_spa->spa_index); > >> >> + int type = nfit_spa_type(nd_spa->nfit_spa); > >> >> + struct nd_mem *nd_mem, *found; > >> >> + struct nd_memdev *nd_memdev; > >> >> + u16 dcr_index; > >> >> + > >> >> + if (type != NFIT_SPA_DCR) > >> >> + continue; > >> > > >> > This function requires NFIT_SPA_DCR, SPA Range Structure with NVDIMM > >> > Control Region GUID, for initializing an nd_mem object. However, > >> > battery-backed DIMMs do not have such control region SPA. IIUC, the > >> > NFIT spec does not require NFIT_SPA_DCR. > >> > > >> > Can you change this function to work with NFIT_SPA_PM as well? > >> > >> NFIT_SPA_PM ranges are handled separately from nd_mem_init(). See > >> nd_region_create() in patch 10. > > > > If nd_mem_init() does not initialize nd_mem objects, nd_bus_probe() in > > core.c fails in nd_bus_init_interleave_sets() and skips all subsequent > > nd_bus_xxx() calls. So, nd_region_create() won't be called. > > > > nd_bus_init_interleave_sets() fails because init_interleave_set() > > returns -ENODEV if (!nd_mem). > > Ah, ok your test case is specifying PMEM backed by memory device > info. We have a test case for simple ranges (nfit_test1_setup()), but > it doesn't hit this bug because it does not specify any memory-device > tables. Yes, we have NFIT table with SPA range (PM), memory device to SPA, and NVDIMM control region structures. With the memory device to SPA structure, this code requires full sets of information, including the namespace label data in _DSM [1], which is outside of ACPI 6.0 and is optional. Battery-backed DIMMs do not have such label data. It needs to work with NFIT table with these structures without this _DSM or with a different type of _DSM which this code may or may not need to support. It should also check Region Format Interface Code (RFIC) in the NVDIMM control region structure before assuming this _DSM is present to implement RFIC 0x0201. [1] http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf Thanks, -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Tue, 2015-04-21 at 13:35 -0700, Dan Williams wrote: > On Tue, Apr 21, 2015 at 12:55 PM, Toshi Kani wrote: > > On Tue, 2015-04-21 at 12:58 -0700, Dan Williams wrote: > >> On Tue, Apr 21, 2015 at 12:35 PM, Toshi Kani wrote: > >> > On Fri, 2015-04-17 at 21:35 -0400, Dan Williams wrote: > >> > : > >> >> + > >> >> +static int nd_mem_init(struct nd_bus *nd_bus) > >> >> +{ > >> >> + struct nd_spa *nd_spa; > >> >> + > >> >> + /* > >> >> + * For each SPA-DCR address range find its corresponding > >> >> + * MEMDEV(s). From each MEMDEV find the corresponding DCR. > >> >> + * Then, try to find a SPA-BDW and a corresponding BDW that > >> >> + * references the DCR. Throw it all into an nd_mem object. > >> >> + * Note, that BDWs are optional. > >> >> + */ > >> >> + list_for_each_entry(nd_spa, &nd_bus->spas, list) { > >> >> + u16 spa_index = readw(&nd_spa->nfit_spa->spa_index); > >> >> + int type = nfit_spa_type(nd_spa->nfit_spa); > >> >> + struct nd_mem *nd_mem, *found; > >> >> + struct nd_memdev *nd_memdev; > >> >> + u16 dcr_index; > >> >> + > >> >> + if (type != NFIT_SPA_DCR) > >> >> + continue; > >> > > >> > This function requires NFIT_SPA_DCR, SPA Range Structure with NVDIMM > >> > Control Region GUID, for initializing an nd_mem object. However, > >> > battery-backed DIMMs do not have such control region SPA. IIUC, the > >> > NFIT spec does not require NFIT_SPA_DCR. > >> > > >> > Can you change this function to work with NFIT_SPA_PM as well? > >> > >> NFIT_SPA_PM ranges are handled separately from nd_mem_init(). See > >> nd_region_create() in patch 10. > > > > If nd_mem_init() does not initialize nd_mem objects, nd_bus_probe() in > > core.c fails in nd_bus_init_interleave_sets() and skips all subsequent > > nd_bus_xxx() calls. So, nd_region_create() won't be called. > > > > nd_bus_init_interleave_sets() fails because init_interleave_set() > > returns -ENODEV if (!nd_mem). > > Ah, ok your test case is specifying PMEM backed by memory device > info. We have a test case for simple ranges (nfit_test1_setup()), but > it doesn't hit this bug because it does not specify any memory-device > tables. > > Thanks, will fix this in v2 of the patch set. > > > BTW, there are two nd_bus_probe() in bus.c and core.c, which is > > confusing. > > Ok, will fix this as well in the v2 posting. Cool! Thanks Dan! -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Tue, Apr 21, 2015 at 12:55 PM, Toshi Kani wrote: > On Tue, 2015-04-21 at 12:58 -0700, Dan Williams wrote: >> On Tue, Apr 21, 2015 at 12:35 PM, Toshi Kani wrote: >> > On Fri, 2015-04-17 at 21:35 -0400, Dan Williams wrote: >> > : >> >> + >> >> +static int nd_mem_init(struct nd_bus *nd_bus) >> >> +{ >> >> + struct nd_spa *nd_spa; >> >> + >> >> + /* >> >> + * For each SPA-DCR address range find its corresponding >> >> + * MEMDEV(s). From each MEMDEV find the corresponding DCR. >> >> + * Then, try to find a SPA-BDW and a corresponding BDW that >> >> + * references the DCR. Throw it all into an nd_mem object. >> >> + * Note, that BDWs are optional. >> >> + */ >> >> + list_for_each_entry(nd_spa, &nd_bus->spas, list) { >> >> + u16 spa_index = readw(&nd_spa->nfit_spa->spa_index); >> >> + int type = nfit_spa_type(nd_spa->nfit_spa); >> >> + struct nd_mem *nd_mem, *found; >> >> + struct nd_memdev *nd_memdev; >> >> + u16 dcr_index; >> >> + >> >> + if (type != NFIT_SPA_DCR) >> >> + continue; >> > >> > This function requires NFIT_SPA_DCR, SPA Range Structure with NVDIMM >> > Control Region GUID, for initializing an nd_mem object. However, >> > battery-backed DIMMs do not have such control region SPA. IIUC, the >> > NFIT spec does not require NFIT_SPA_DCR. >> > >> > Can you change this function to work with NFIT_SPA_PM as well? >> >> NFIT_SPA_PM ranges are handled separately from nd_mem_init(). See >> nd_region_create() in patch 10. > > If nd_mem_init() does not initialize nd_mem objects, nd_bus_probe() in > core.c fails in nd_bus_init_interleave_sets() and skips all subsequent > nd_bus_xxx() calls. So, nd_region_create() won't be called. > > nd_bus_init_interleave_sets() fails because init_interleave_set() > returns -ENODEV if (!nd_mem). Ah, ok your test case is specifying PMEM backed by memory device info. We have a test case for simple ranges (nfit_test1_setup()), but it doesn't hit this bug because it does not specify any memory-device tables. Thanks, will fix this in v2 of the patch set. > BTW, there are two nd_bus_probe() in bus.c and core.c, which is > confusing. Ok, will fix this as well in the v2 posting. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Tue, 2015-04-21 at 12:58 -0700, Dan Williams wrote: > On Tue, Apr 21, 2015 at 12:35 PM, Toshi Kani wrote: > > On Fri, 2015-04-17 at 21:35 -0400, Dan Williams wrote: > > : > >> + > >> +static int nd_mem_init(struct nd_bus *nd_bus) > >> +{ > >> + struct nd_spa *nd_spa; > >> + > >> + /* > >> + * For each SPA-DCR address range find its corresponding > >> + * MEMDEV(s). From each MEMDEV find the corresponding DCR. > >> + * Then, try to find a SPA-BDW and a corresponding BDW that > >> + * references the DCR. Throw it all into an nd_mem object. > >> + * Note, that BDWs are optional. > >> + */ > >> + list_for_each_entry(nd_spa, &nd_bus->spas, list) { > >> + u16 spa_index = readw(&nd_spa->nfit_spa->spa_index); > >> + int type = nfit_spa_type(nd_spa->nfit_spa); > >> + struct nd_mem *nd_mem, *found; > >> + struct nd_memdev *nd_memdev; > >> + u16 dcr_index; > >> + > >> + if (type != NFIT_SPA_DCR) > >> + continue; > > > > This function requires NFIT_SPA_DCR, SPA Range Structure with NVDIMM > > Control Region GUID, for initializing an nd_mem object. However, > > battery-backed DIMMs do not have such control region SPA. IIUC, the > > NFIT spec does not require NFIT_SPA_DCR. > > > > Can you change this function to work with NFIT_SPA_PM as well? > > NFIT_SPA_PM ranges are handled separately from nd_mem_init(). See > nd_region_create() in patch 10. If nd_mem_init() does not initialize nd_mem objects, nd_bus_probe() in core.c fails in nd_bus_init_interleave_sets() and skips all subsequent nd_bus_xxx() calls. So, nd_region_create() won't be called. nd_bus_init_interleave_sets() fails because init_interleave_set() returns -ENODEV if (!nd_mem). BTW, there are two nd_bus_probe() in bus.c and core.c, which is confusing. Thanks, -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Tue, Apr 21, 2015 at 12:35 PM, Toshi Kani wrote: > On Fri, 2015-04-17 at 21:35 -0400, Dan Williams wrote: > : >> + >> +static int nd_mem_init(struct nd_bus *nd_bus) >> +{ >> + struct nd_spa *nd_spa; >> + >> + /* >> + * For each SPA-DCR address range find its corresponding >> + * MEMDEV(s). From each MEMDEV find the corresponding DCR. >> + * Then, try to find a SPA-BDW and a corresponding BDW that >> + * references the DCR. Throw it all into an nd_mem object. >> + * Note, that BDWs are optional. >> + */ >> + list_for_each_entry(nd_spa, &nd_bus->spas, list) { >> + u16 spa_index = readw(&nd_spa->nfit_spa->spa_index); >> + int type = nfit_spa_type(nd_spa->nfit_spa); >> + struct nd_mem *nd_mem, *found; >> + struct nd_memdev *nd_memdev; >> + u16 dcr_index; >> + >> + if (type != NFIT_SPA_DCR) >> + continue; > > This function requires NFIT_SPA_DCR, SPA Range Structure with NVDIMM > Control Region GUID, for initializing an nd_mem object. However, > battery-backed DIMMs do not have such control region SPA. IIUC, the > NFIT spec does not require NFIT_SPA_DCR. > > Can you change this function to work with NFIT_SPA_PM as well? NFIT_SPA_PM ranges are handled separately from nd_mem_init(). See nd_region_create() in patch 10. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 04/21] nd: create an 'nd_bus' from an 'nfit_desc'
On Fri, 2015-04-17 at 21:35 -0400, Dan Williams wrote: : > + > +static int nd_mem_init(struct nd_bus *nd_bus) > +{ > + struct nd_spa *nd_spa; > + > + /* > + * For each SPA-DCR address range find its corresponding > + * MEMDEV(s). From each MEMDEV find the corresponding DCR. > + * Then, try to find a SPA-BDW and a corresponding BDW that > + * references the DCR. Throw it all into an nd_mem object. > + * Note, that BDWs are optional. > + */ > + list_for_each_entry(nd_spa, &nd_bus->spas, list) { > + u16 spa_index = readw(&nd_spa->nfit_spa->spa_index); > + int type = nfit_spa_type(nd_spa->nfit_spa); > + struct nd_mem *nd_mem, *found; > + struct nd_memdev *nd_memdev; > + u16 dcr_index; > + > + if (type != NFIT_SPA_DCR) > + continue; This function requires NFIT_SPA_DCR, SPA Range Structure with NVDIMM Control Region GUID, for initializing an nd_mem object. However, battery-backed DIMMs do not have such control region SPA. IIUC, the NFIT spec does not require NFIT_SPA_DCR. Can you change this function to work with NFIT_SPA_PM as well? Thanks, -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/