[PATCH] libnvdimm, nfit: treat volatile virtual CD region as read-only pmem

[PATCH] libnvdimm, nfit: treat volatile virtual CD region as read-only pmem

[Lee, Chun-Yi]

This patch adds codes to treat a volatile virtual CD region as a
read-only pmem region, then read-only /dev/pmem* device can be mounted
with iso9660.

It's useful to work with the httpboot in EFI firmware to pull a remote
ISO file to the local memory region for booting and installation.

Wiki page of UEFI HTTPBoot with OVMF:
	https://en.opensuse.org/UEFI_HTTPBoot_with_OVMF

Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
Cc: Gary Lin <GLin@suse.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 drivers/acpi/nfit.c          |  8 +++++++-
 drivers/nvdimm/region_devs.c | 26 +++++++++++++++++++++++++-
 include/linux/libnvdimm.h    |  2 ++
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index 2215fc8..b100a17 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -1949,6 +1949,7 @@ static int acpi_nfit_init_mapping(struct acpi_nfit_desc *acpi_desc,
 	switch (nfit_spa_type(spa)) {
 	case NFIT_SPA_PM:
 	case NFIT_SPA_VOLATILE:
+	case NFIT_SPA_VCD:
 		nd_mapping->start = memdev->address;
 		nd_mapping->size = memdev->region_size;
 		break;
@@ -1995,7 +1996,7 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
 	if (nfit_spa->nd_region)
 		return 0;
 
-	if (spa->range_index == 0) {
+	if (spa->range_index == 0 && nfit_spa_type(spa) != NFIT_SPA_VCD) {
 		dev_dbg(acpi_desc->dev, "%s: detected invalid spa index\n",
 				__func__);
 		return 0;
@@ -2059,6 +2060,11 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
 				ndr_desc);
 		if (!nfit_spa->nd_region)
 			rc = -ENOMEM;
+	} else if (nfit_spa_type(spa) == NFIT_SPA_VCD) {
+		nfit_spa->nd_region = nvdimm_vcd_region_create(nvdimm_bus,
+				ndr_desc);
+		if (!nfit_spa->nd_region)
+			rc = -ENOMEM;
 	}
 
  out:
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 40fcfea..f155941 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -56,9 +56,19 @@ static struct device_type nd_volatile_device_type = {
 	.release = nd_region_release,
 };
 
+static struct device_type nd_vcd_device_type = {
+	.name = "nd_vcd",
+	.release = nd_region_release,
+};
+
+bool is_nd_vcd(struct device *dev)
+{
+	return dev ? dev->type == &nd_vcd_device_type : false;
+}
+
 bool is_nd_pmem(struct device *dev)
 {
-	return dev ? dev->type == &nd_pmem_device_type : false;
+	return dev ? dev->type == &nd_pmem_device_type || is_nd_vcd(dev) : false;
 }
 
 bool is_nd_blk(struct device *dev)
@@ -338,6 +348,9 @@ static ssize_t read_only_store(struct device *dev,
 	int rc = strtobool(buf, &ro);
 	struct nd_region *nd_region = to_nd_region(dev);
 
+	if (is_nd_vcd(dev))
+		return -ENXIO;
+
 	if (rc)
 		return rc;
 
@@ -687,6 +700,9 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus,
 			ro = 1;
 	}
 
+	if (dev_type == &nd_vcd_device_type)
+		ro = 1;
+
 	if (dev_type == &nd_blk_device_type) {
 		struct nd_blk_region_desc *ndbr_desc;
 		struct nd_blk_region *ndbr;
@@ -774,6 +790,14 @@ struct nd_region *nvdimm_pmem_region_create(struct nvdimm_bus *nvdimm_bus,
 }
 EXPORT_SYMBOL_GPL(nvdimm_pmem_region_create);
 
+struct nd_region *nvdimm_vcd_region_create(struct nvdimm_bus *nvdimm_bus,
+		struct nd_region_desc *ndr_desc)
+{
+	return nd_region_create(nvdimm_bus, ndr_desc, &nd_vcd_device_type,
+			__func__);
+}
+EXPORT_SYMBOL_GPL(nvdimm_vcd_region_create);
+
 struct nd_region *nvdimm_blk_region_create(struct nvdimm_bus *nvdimm_bus,
 		struct nd_region_desc *ndr_desc)
 {
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 0c3c30c..0a1f949 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -145,6 +145,8 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd,
 int nvdimm_bus_check_dimm_count(struct nvdimm_bus *nvdimm_bus, int dimm_count);
 struct nd_region *nvdimm_pmem_region_create(struct nvdimm_bus *nvdimm_bus,
 		struct nd_region_desc *ndr_desc);
+struct nd_region *nvdimm_vcd_region_create(struct nvdimm_bus *nvdimm_bus,
+		struct nd_region_desc *ndr_desc);
 struct nd_region *nvdimm_blk_region_create(struct nvdimm_bus *nvdimm_bus,
 		struct nd_region_desc *ndr_desc);
 struct nd_region *nvdimm_volatile_region_create(struct nvdimm_bus *nvdimm_bus,
-- 
2.1.4

Re: [PATCH] libnvdimm, nfit: treat volatile virtual CD region as read-only pmem

[Dan Williams]

On Fri, Jun 3, 2016 at 12:13 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> This patch adds codes to treat a volatile virtual CD region as a
> read-only pmem region, then read-only /dev/pmem* device can be mounted
> with iso9660.
>
> It's useful to work with the httpboot in EFI firmware to pull a remote
> ISO file to the local memory region for booting and installation.
>
> Wiki page of UEFI HTTPBoot with OVMF:
>         https://en.opensuse.org/UEFI_HTTPBoot_with_OVMF
>
> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> Cc: Gary Lin <GLin@suse.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> ---
>  drivers/acpi/nfit.c          |  8 +++++++-
>  drivers/nvdimm/region_devs.c | 26 +++++++++++++++++++++++++-
>  include/linux/libnvdimm.h    |  2 ++
>  3 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index 2215fc8..b100a17 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -1949,6 +1949,7 @@ static int acpi_nfit_init_mapping(struct acpi_nfit_desc *acpi_desc,
>         switch (nfit_spa_type(spa)) {
>         case NFIT_SPA_PM:
>         case NFIT_SPA_VOLATILE:
> +       case NFIT_SPA_VCD:
>                 nd_mapping->start = memdev->address;
>                 nd_mapping->size = memdev->region_size;
>                 break;

Why do we need to distinguish NFIT_SPA_VOLATILE vs NFIT_SPA_VCD, i.e.
what happens if something writes to a VCD device?

Re: [PATCH] libnvdimm, nfit: treat volatile virtual CD region as read-only pmem

[joeyli]

Hi Dan, 

Thanks for your review.

On Fri, Jun 03, 2016 at 12:27:34PM -0700, Dan Williams wrote:
> On Fri, Jun 3, 2016 at 12:13 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > This patch adds codes to treat a volatile virtual CD region as a
> > read-only pmem region, then read-only /dev/pmem* device can be mounted
> > with iso9660.
> >
> > It's useful to work with the httpboot in EFI firmware to pull a remote
> > ISO file to the local memory region for booting and installation.
> >
> > Wiki page of UEFI HTTPBoot with OVMF:
> >         https://en.opensuse.org/UEFI_HTTPBoot_with_OVMF
> >
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > Cc: Gary Lin <GLin@suse.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > ---
> >  drivers/acpi/nfit.c          |  8 +++++++-
> >  drivers/nvdimm/region_devs.c | 26 +++++++++++++++++++++++++-
> >  include/linux/libnvdimm.h    |  2 ++
> >  3 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index 2215fc8..b100a17 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -1949,6 +1949,7 @@ static int acpi_nfit_init_mapping(struct acpi_nfit_desc *acpi_desc,
> >         switch (nfit_spa_type(spa)) {
> >         case NFIT_SPA_PM:
> >         case NFIT_SPA_VOLATILE:
> > +       case NFIT_SPA_VCD:
> >                 nd_mapping->start = memdev->address;
> >                 nd_mapping->size = memdev->region_size;
> >                 break;
> 
> Why do we need to distinguish NFIT_SPA_VOLATILE vs NFIT_SPA_VCD, i.e.
> what happens if something writes to a VCD device?

Actually I didn't try to write SPA-VCD device before. Every time I mount it
that the system responses read-only:

# mount /dev/pmem0 /mnt/
mount: /dev/pmem0 is write-protected, mounting read-only

If it can be written, then I think there have no difference between
NFIT_SPA_VOLATILE with NFIT_SPA_VCD region.

I implemented this patch to treat VCD region as read-only pmem because the
pmem region generates /dev/pmem* device that it can be mounted.

Maybe I missed. Does NFIT_SPA_VOLATILE region also generate a device in /dev
that it can be mounted with filesystem? Then I think treat the VCD region as
a read-only VOLATILE region that's also a solution.


Thanks a lot!
Joey Lee

Re: [PATCH] libnvdimm, nfit: treat volatile virtual CD region as read-only pmem

[Dan Williams]

On Sat, Jun 4, 2016 at 4:01 AM, joeyli <jlee@suse.com> wrote:
> Hi Dan,
>
> Thanks for your review.
>
> On Fri, Jun 03, 2016 at 12:27:34PM -0700, Dan Williams wrote:
>> On Fri, Jun 3, 2016 at 12:13 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
>> > This patch adds codes to treat a volatile virtual CD region as a
>> > read-only pmem region, then read-only /dev/pmem* device can be mounted
>> > with iso9660.
>> >
>> > It's useful to work with the httpboot in EFI firmware to pull a remote
>> > ISO file to the local memory region for booting and installation.
>> >
>> > Wiki page of UEFI HTTPBoot with OVMF:
>> >         https://en.opensuse.org/UEFI_HTTPBoot_with_OVMF
>> >
>> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
>> > Cc: Gary Lin <GLin@suse.com>
>> > Cc: Dan Williams <dan.j.williams@intel.com>
>> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> > ---
>> >  drivers/acpi/nfit.c          |  8 +++++++-
>> >  drivers/nvdimm/region_devs.c | 26 +++++++++++++++++++++++++-
>> >  include/linux/libnvdimm.h    |  2 ++
>> >  3 files changed, 34 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
>> > index 2215fc8..b100a17 100644
>> > --- a/drivers/acpi/nfit.c
>> > +++ b/drivers/acpi/nfit.c
>> > @@ -1949,6 +1949,7 @@ static int acpi_nfit_init_mapping(struct acpi_nfit_desc *acpi_desc,
>> >         switch (nfit_spa_type(spa)) {
>> >         case NFIT_SPA_PM:
>> >         case NFIT_SPA_VOLATILE:
>> > +       case NFIT_SPA_VCD:
>> >                 nd_mapping->start = memdev->address;
>> >                 nd_mapping->size = memdev->region_size;
>> >                 break;
>>
>> Why do we need to distinguish NFIT_SPA_VOLATILE vs NFIT_SPA_VCD, i.e.
>> what happens if something writes to a VCD device?
>
> Actually I didn't try to write SPA-VCD device before. Every time I mount it
> that the system responses read-only:
>
> # mount /dev/pmem0 /mnt/
> mount: /dev/pmem0 is write-protected, mounting read-only
>
> If it can be written, then I think there have no difference between
> NFIT_SPA_VOLATILE with NFIT_SPA_VCD region.
>
> I implemented this patch to treat VCD region as read-only pmem because the
> pmem region generates /dev/pmem* device that it can be mounted.
>
> Maybe I missed. Does NFIT_SPA_VOLATILE region also generate a device in /dev
> that it can be mounted with filesystem? Then I think treat the VCD region as
> a read-only VOLATILE region that's also a solution.

My question is why does it need to be read-only?  If it's a volatile
region, does it matter if we allow writes at the block device level?
Especially if it is formatted as iso9660, it won't be writable through
the filesystem anyway.

Re: [PATCH] libnvdimm, nfit: treat volatile virtual CD region as read-only pmem

[joeyli]

On Sat, Jun 04, 2016 at 09:24:54AM -0700, Dan Williams wrote:
> On Sat, Jun 4, 2016 at 4:01 AM, joeyli <jlee@suse.com> wrote:
> > Hi Dan,
> >
> > Thanks for your review.
> >
> > On Fri, Jun 03, 2016 at 12:27:34PM -0700, Dan Williams wrote:
> >> On Fri, Jun 3, 2016 at 12:13 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> >> > This patch adds codes to treat a volatile virtual CD region as a
> >> > read-only pmem region, then read-only /dev/pmem* device can be mounted
> >> > with iso9660.
> >> >
> >> > It's useful to work with the httpboot in EFI firmware to pull a remote
> >> > ISO file to the local memory region for booting and installation.
> >> >
> >> > Wiki page of UEFI HTTPBoot with OVMF:
> >> >         https://en.opensuse.org/UEFI_HTTPBoot_with_OVMF
> >> >
> >> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> >> > Cc: Gary Lin <GLin@suse.com>
> >> > Cc: Dan Williams <dan.j.williams@intel.com>
> >> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> >> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >> > ---
> >> >  drivers/acpi/nfit.c          |  8 +++++++-
> >> >  drivers/nvdimm/region_devs.c | 26 +++++++++++++++++++++++++-
> >> >  include/linux/libnvdimm.h    |  2 ++
> >> >  3 files changed, 34 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> >> > index 2215fc8..b100a17 100644
> >> > --- a/drivers/acpi/nfit.c
> >> > +++ b/drivers/acpi/nfit.c
> >> > @@ -1949,6 +1949,7 @@ static int acpi_nfit_init_mapping(struct acpi_nfit_desc *acpi_desc,
> >> >         switch (nfit_spa_type(spa)) {
> >> >         case NFIT_SPA_PM:
> >> >         case NFIT_SPA_VOLATILE:
> >> > +       case NFIT_SPA_VCD:
> >> >                 nd_mapping->start = memdev->address;
> >> >                 nd_mapping->size = memdev->region_size;
> >> >                 break;
> >>
> >> Why do we need to distinguish NFIT_SPA_VOLATILE vs NFIT_SPA_VCD, i.e.
> >> what happens if something writes to a VCD device?
> >
> > Actually I didn't try to write SPA-VCD device before. Every time I mount it
> > that the system responses read-only:
> >
> > # mount /dev/pmem0 /mnt/
> > mount: /dev/pmem0 is write-protected, mounting read-only
> >
> > If it can be written, then I think there have no difference between
> > NFIT_SPA_VOLATILE with NFIT_SPA_VCD region.
> >
> > I implemented this patch to treat VCD region as read-only pmem because the
> > pmem region generates /dev/pmem* device that it can be mounted.
> >
> > Maybe I missed. Does NFIT_SPA_VOLATILE region also generate a device in /dev
> > that it can be mounted with filesystem? Then I think treat the VCD region as
> > a read-only VOLATILE region that's also a solution.
> 
> My question is why does it need to be read-only?  If it's a volatile
> region, does it matter if we allow writes at the block device level?
> Especially if it is formatted as iso9660, it won't be writable through
> the filesystem anyway.

The httpboot function of EFI firmware downloads iso image from http server
, then it allocates a big enough continuity memory region and extract iso to
the region.

Actually you are right, it does not need to be read-only. It's similar with
a volatile region, just the type is NFIT_SPA_VCD and it contains the data
that extracted from a iso file.

Do you have better approach to treat the SPA_VCD region as a block device
that it can be mounted by iso9660? Maybe I missed something... Can I mount a
volatile region with iso9660? If yes, maybe just treat VCD SPA as a VOLATILE
region?


Thanks a lot!
Joey Lee

Re: [PATCH] libnvdimm, nfit: treat volatile virtual CD region as read-only pmem

[joeyli]

On Sat, Jun 04, 2016 at 09:24:54AM -0700, Dan Williams wrote:
> On Sat, Jun 4, 2016 at 4:01 AM, joeyli <jlee@suse.com> wrote:
> > Hi Dan,
> >
> > Thanks for your review.
> >
> > On Fri, Jun 03, 2016 at 12:27:34PM -0700, Dan Williams wrote:
> >> On Fri, Jun 3, 2016 at 12:13 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> >> > This patch adds codes to treat a volatile virtual CD region as a
> >> > read-only pmem region, then read-only /dev/pmem* device can be mounted
> >> > with iso9660.
> >> >
> >> > It's useful to work with the httpboot in EFI firmware to pull a remote
> >> > ISO file to the local memory region for booting and installation.
> >> >
> >> > Wiki page of UEFI HTTPBoot with OVMF:
> >> >         https://en.opensuse.org/UEFI_HTTPBoot_with_OVMF
> >> >
> >> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> >> > Cc: Gary Lin <GLin@suse.com>
> >> > Cc: Dan Williams <dan.j.williams@intel.com>
> >> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> >> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >> > ---
> >> >  drivers/acpi/nfit.c          |  8 +++++++-
> >> >  drivers/nvdimm/region_devs.c | 26 +++++++++++++++++++++++++-
> >> >  include/linux/libnvdimm.h    |  2 ++
> >> >  3 files changed, 34 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> >> > index 2215fc8..b100a17 100644
> >> > --- a/drivers/acpi/nfit.c
> >> > +++ b/drivers/acpi/nfit.c
> >> > @@ -1949,6 +1949,7 @@ static int acpi_nfit_init_mapping(struct acpi_nfit_desc *acpi_desc,
> >> >         switch (nfit_spa_type(spa)) {
> >> >         case NFIT_SPA_PM:
> >> >         case NFIT_SPA_VOLATILE:
> >> > +       case NFIT_SPA_VCD:
> >> >                 nd_mapping->start = memdev->address;
> >> >                 nd_mapping->size = memdev->region_size;
> >> >                 break;
> >>
> >> Why do we need to distinguish NFIT_SPA_VOLATILE vs NFIT_SPA_VCD, i.e.
> >> what happens if something writes to a VCD device?
> >
> > Actually I didn't try to write SPA-VCD device before. Every time I mount it
> > that the system responses read-only:
> >
> > # mount /dev/pmem0 /mnt/
> > mount: /dev/pmem0 is write-protected, mounting read-only
> >
> > If it can be written, then I think there have no difference between
> > NFIT_SPA_VOLATILE with NFIT_SPA_VCD region.
> >
> > I implemented this patch to treat VCD region as read-only pmem because the
> > pmem region generates /dev/pmem* device that it can be mounted.
> >
> > Maybe I missed. Does NFIT_SPA_VOLATILE region also generate a device in /dev
> > that it can be mounted with filesystem? Then I think treat the VCD region as
> > a read-only VOLATILE region that's also a solution.
> 
> My question is why does it need to be read-only?  If it's a volatile
> region, does it matter if we allow writes at the block device level?
> Especially if it is formatted as iso9660, it won't be writable through
> the filesystem anyway.

The httpboot function of EFI firmware downloads iso image from http server
, then it allocates a big enough continuity memory region and extract iso to
the region.

Actually you are right, it does not need to be read-only. It's similar with
a volatile region, just the type is NFIT_SPA_VCD and it contains the data
that extracted from a iso file.

Do you have better approach to treat the SPA_VCD region as a block device
that it can be mounted by iso9660? Maybe I missed something... Can I mount a
volatile region with iso9660? If yes, maybe just treat VCD SPA as a VOLATILE
region?


Thanks a lot!
Joey Lee

Re: [PATCH] libnvdimm, nfit: treat volatile virtual CD region as read-only pmem

[Linda Knippers]

On 6/4/2016 7:01 AM, joeyli wrote:
> Hi Dan, 
> 
> Thanks for your review.
> 
> On Fri, Jun 03, 2016 at 12:27:34PM -0700, Dan Williams wrote:
>> On Fri, Jun 3, 2016 at 12:13 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
>>> This patch adds codes to treat a volatile virtual CD region as a
>>> read-only pmem region, then read-only /dev/pmem* device can be mounted
>>> with iso9660.
>>>
>>> It's useful to work with the httpboot in EFI firmware to pull a remote
>>> ISO file to the local memory region for booting and installation.
>>>
>>> Wiki page of UEFI HTTPBoot with OVMF:
>>>         https://en.opensuse.org/UEFI_HTTPBoot_with_OVMF
>>>
>>> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
>>> Cc: Gary Lin <GLin@suse.com>
>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>> ---
>>>  drivers/acpi/nfit.c          |  8 +++++++-
>>>  drivers/nvdimm/region_devs.c | 26 +++++++++++++++++++++++++-
>>>  include/linux/libnvdimm.h    |  2 ++
>>>  3 files changed, 34 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
>>> index 2215fc8..b100a17 100644
>>> --- a/drivers/acpi/nfit.c
>>> +++ b/drivers/acpi/nfit.c
>>> @@ -1949,6 +1949,7 @@ static int acpi_nfit_init_mapping(struct acpi_nfit_desc *acpi_desc,
>>>         switch (nfit_spa_type(spa)) {
>>>         case NFIT_SPA_PM:
>>>         case NFIT_SPA_VOLATILE:
>>> +       case NFIT_SPA_VCD:
>>>                 nd_mapping->start = memdev->address;
>>>                 nd_mapping->size = memdev->region_size;
>>>                 break;
>>
>> Why do we need to distinguish NFIT_SPA_VOLATILE vs NFIT_SPA_VCD, i.e.
>> what happens if something writes to a VCD device?
> 
> Actually I didn't try to write SPA-VCD device before. Every time I mount it
> that the system responses read-only:
> 
> # mount /dev/pmem0 /mnt/
> mount: /dev/pmem0 is write-protected, mounting read-only
> 
> If it can be written, then I think there have no difference between
> NFIT_SPA_VOLATILE with NFIT_SPA_VCD region.

It's not clear to me what the expectations for this type of device are, or
whether they should be read-only.  The ACPI spec is not helpful here.
The other Disk Region and CD Region types are also unclear.  Anyone
care to define them?

> I implemented this patch to treat VCD region as read-only pmem because the
> pmem region generates /dev/pmem* device that it can be mounted.

I'm a bit worried about this type of device showing up as a "pmem" device.
I realize they're described in the NFIT (not sure why but they are) but do
any of the operations that we support for other pmem devices work on these?
Do root device DSMs make any sense?  Are there other DSMs?  What will happen
if someone uses ndctl to reconfigure the device?

I'm especially concerned on systems that might have one of these devices
and also have NVDIMMs.  Do all the pmem devices get numbered different if
this device comes and goes across reboots?  (I know, we shouldn't rely on
those names but it will still confuse people.)  Can they be some other name
that better represents what they're trying to be?

> Maybe I missed. Does NFIT_SPA_VOLATILE region also generate a device in /dev
> that it can be mounted with filesystem? 

Yes.

-- ljk

> Then I think treat the VCD region as
> a read-only VOLATILE region that's also a solution.
> 
> 
> Thanks a lot!
> Joey Lee
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
> 

Re: [PATCH] libnvdimm, nfit: treat volatile virtual CD region as read-only pmem

[Dan Williams]

On Thu, Jun 9, 2016 at 3:08 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
> On 6/4/2016 7:01 AM, joeyli wrote:
>> Hi Dan,
>>
>> Thanks for your review.
>>
>> On Fri, Jun 03, 2016 at 12:27:34PM -0700, Dan Williams wrote:
>>> On Fri, Jun 3, 2016 at 12:13 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
>>>> This patch adds codes to treat a volatile virtual CD region as a
>>>> read-only pmem region, then read-only /dev/pmem* device can be mounted
>>>> with iso9660.
>>>>
>>>> It's useful to work with the httpboot in EFI firmware to pull a remote
>>>> ISO file to the local memory region for booting and installation.
>>>>
>>>> Wiki page of UEFI HTTPBoot with OVMF:
>>>>         https://en.opensuse.org/UEFI_HTTPBoot_with_OVMF
>>>>
>>>> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
>>>> Cc: Gary Lin <GLin@suse.com>
>>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>>> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>>> ---
>>>>  drivers/acpi/nfit.c          |  8 +++++++-
>>>>  drivers/nvdimm/region_devs.c | 26 +++++++++++++++++++++++++-
>>>>  include/linux/libnvdimm.h    |  2 ++
>>>>  3 files changed, 34 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
>>>> index 2215fc8..b100a17 100644
>>>> --- a/drivers/acpi/nfit.c
>>>> +++ b/drivers/acpi/nfit.c
>>>> @@ -1949,6 +1949,7 @@ static int acpi_nfit_init_mapping(struct acpi_nfit_desc *acpi_desc,
>>>>         switch (nfit_spa_type(spa)) {
>>>>         case NFIT_SPA_PM:
>>>>         case NFIT_SPA_VOLATILE:
>>>> +       case NFIT_SPA_VCD:
>>>>                 nd_mapping->start = memdev->address;
>>>>                 nd_mapping->size = memdev->region_size;
>>>>                 break;
>>>
>>> Why do we need to distinguish NFIT_SPA_VOLATILE vs NFIT_SPA_VCD, i.e.
>>> what happens if something writes to a VCD device?
>>
>> Actually I didn't try to write SPA-VCD device before. Every time I mount it
>> that the system responses read-only:
>>
>> # mount /dev/pmem0 /mnt/
>> mount: /dev/pmem0 is write-protected, mounting read-only
>>
>> If it can be written, then I think there have no difference between
>> NFIT_SPA_VOLATILE with NFIT_SPA_VCD region.
>
> It's not clear to me what the expectations for this type of device are, or
> whether they should be read-only.  The ACPI spec is not helpful here.
> The other Disk Region and CD Region types are also unclear.  Anyone
> care to define them?
>
>> I implemented this patch to treat VCD region as read-only pmem because the
>> pmem region generates /dev/pmem* device that it can be mounted.
>
> I'm a bit worried about this type of device showing up as a "pmem" device.
> I realize they're described in the NFIT (not sure why but they are) but do
> any of the operations that we support for other pmem devices work on these?

It would be just another pmem device, so it would support all of them.

> Do root device DSMs make any sense?

Root device DSMs take a physical address so they could apply, but I
assume a firmware could simply refuse to run any operations against
them.

> Are there other DSMs?

Not that I can think of...

> What will happen if someone uses ndctl to reconfigure the device?

It won't know or care about the difference.  Unless there's a negative
side effect of allowing writes to a "volatile cd" it would be yet
another pmem device.

> I'm especially concerned on systems that might have one of these devices
> and also have NVDIMMs.  Do all the pmem devices get numbered different if
> this device comes and goes across reboots?  (I know, we shouldn't rely on
> those names but it will still confuse people.)  Can they be some other name
> that better represents what they're trying to be?

If they all go through the same driver they should have the same
naming scheme.  Software should be identifying the block device via
blkid, not kernel device name.

Re: [PATCH] libnvdimm, nfit: treat volatile virtual CD region as read-only pmem

[joeyli]

Hi Linda, 

Thanks for your review and comments.

On Thu, Jun 09, 2016 at 06:08:17PM -0400, Linda Knippers wrote:
> On 6/4/2016 7:01 AM, joeyli wrote:
> > Hi Dan, 
> > 
> > Thanks for your review.
> > 
> > On Fri, Jun 03, 2016 at 12:27:34PM -0700, Dan Williams wrote:
> >> On Fri, Jun 3, 2016 at 12:13 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> >>> This patch adds codes to treat a volatile virtual CD region as a
> >>> read-only pmem region, then read-only /dev/pmem* device can be mounted
> >>> with iso9660.
> >>>
> >>> It's useful to work with the httpboot in EFI firmware to pull a remote
> >>> ISO file to the local memory region for booting and installation.
> >>>
> >>> Wiki page of UEFI HTTPBoot with OVMF:
> >>>         https://en.opensuse.org/UEFI_HTTPBoot_with_OVMF
> >>>
> >>> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> >>> Cc: Gary Lin <GLin@suse.com>
> >>> Cc: Dan Williams <dan.j.williams@intel.com>
> >>> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> >>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >>> ---
> >>>  drivers/acpi/nfit.c          |  8 +++++++-
> >>>  drivers/nvdimm/region_devs.c | 26 +++++++++++++++++++++++++-
> >>>  include/linux/libnvdimm.h    |  2 ++
> >>>  3 files changed, 34 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> >>> index 2215fc8..b100a17 100644
> >>> --- a/drivers/acpi/nfit.c
> >>> +++ b/drivers/acpi/nfit.c
> >>> @@ -1949,6 +1949,7 @@ static int acpi_nfit_init_mapping(struct acpi_nfit_desc *acpi_desc,
> >>>         switch (nfit_spa_type(spa)) {
> >>>         case NFIT_SPA_PM:
> >>>         case NFIT_SPA_VOLATILE:
> >>> +       case NFIT_SPA_VCD:
> >>>                 nd_mapping->start = memdev->address;
> >>>                 nd_mapping->size = memdev->region_size;
> >>>                 break;
> >>
> >> Why do we need to distinguish NFIT_SPA_VOLATILE vs NFIT_SPA_VCD, i.e.
> >> what happens if something writes to a VCD device?
> > 
> > Actually I didn't try to write SPA-VCD device before. Every time I mount it
> > that the system responses read-only:
> > 
> > # mount /dev/pmem0 /mnt/
> > mount: /dev/pmem0 is write-protected, mounting read-only
> > 
> > If it can be written, then I think there have no difference between
> > NFIT_SPA_VOLATILE with NFIT_SPA_VCD region.
> 
> It's not clear to me what the expectations for this type of device are, or
> whether they should be read-only.  The ACPI spec is not helpful here.
> The other Disk Region and CD Region types are also unclear.  Anyone
> care to define them?
>

In ACPI spec 6.1, it said "a volatile memory region that contains an ISO image":

This GUID defines a RAM Disk supporting a Virtual CD Region – Volatile (a volatile memory
region that contains an ISO image):
{ 0x3D5ABD30,0x4175,0x87CE,0x6D,0x64,0xD2,0xAD,0xE5,0x23,0xC4,0xBB }

I think the behavior that is the same with a volatile memory region but it
contains ISO.

I agree doesn't have any spec that it mentions the type should be read-only, I
will remove the code in patch.

I also agree that it doesn't have detail description of those ram disk
types. Do you have any idea or comment that you want to add to spec? Either
for ACPI or UEFI?
 
> > I implemented this patch to treat VCD region as read-only pmem because the
> > pmem region generates /dev/pmem* device that it can be mounted.
> 
> I'm a bit worried about this type of device showing up as a "pmem" device.
> I realize they're described in the NFIT (not sure why but they are) but do
> any of the operations that we support for other pmem devices work on these?
> Do root device DSMs make any sense?  Are there other DSMs?  What will happen
> if someone uses ndctl to reconfigure the device?
>

By using the Ramdisk function in EDK2/OVMF, it only generates a ACPI0012 root
device that it contains a empty _STA but no _DSM support:

DefinitionBlock ("ssdt2.aml", "SSDT", 2, "INTEL ", "RamDisk ", 0x00001000)
{
    Scope (\_SB)
    {   
        Device (NVDR)
        {   
            Name (_HID, "ACPI0012")  // _HID: Hardware ID
            Name (_STR, Unicode ("NVDIMM Root Device"))  // _STR: Description String
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {   
                Return (0x0F)
            }
        }
    }
}

So there have no way to control this root device through _DSM. The ACPI0012
root device is used to trigger the nfit driver in acpi.

I will put the above parser result to the patch description in next version.


On the other hand, here is the NFIT that it is generated by OVMF:

[000h 0000   4]                    Signature : "NFIT"    [NVDIMM Firmware Interface Table]
[004h 0004   4]                 Table Length : 00000060
[008h 0008   1]                     Revision : 01
[009h 0009   1]                     Checksum : 0C
[00Ah 0010   6]                       Oem ID : "INTEL "
[010h 0016   8]                 Oem Table ID : "EDK2    "
[018h 0024   4]                 Oem Revision : 00000002
[01Ch 0028   4]              Asl Compiler ID : "    "
[020h 0032   4]        Asl Compiler Revision : 01000013

[024h 0036   4]                     Reserved : 00000000

[028h 0040   2]                Subtable Type : 0000 [System Physical Address Range]
[02Ah 0042   2]                       Length : 0038

[02Ch 0044   2]                  Range Index : 0000
[02Eh 0046   2]        Flags (decoded below) : 0000
                   Add/Online Operation Only : 0
                      Proximity Domain Valid : 0
[030h 0048   4]                     Reserved : 00000000
[034h 0052   4]             Proximity Domain : 00000000
[038h 0056  16]           Address Range GUID : 77AB535A-45FC-624B-5560-F7B281D1F96E
[048h 0072   8]           Address Range Base : 00000000B6ABD018
[050h 0080   8]         Address Range Length : 0000000005500000
[058h 0088   8]         Memory Map Attribute : 0000000000000000

The "Address Range GUID" is a "Virtual Disk Region – Volatile" but not a VCD
because I used the Ramdisk function in EDK2 to generate the NFIT. When using
http boot with a iso file, this GUID will be changed to VCD type.

As the above NFIT, there have "Address Range Base" and "Address Range Length"
but the "Range Index" will be zero. So my patch ignores the "Range Index"
checking on VCD type.
 
> I'm especially concerned on systems that might have one of these devices
> and also have NVDIMMs.  Do all the pmem devices get numbered different if
> this device comes and goes across reboots?  (I know, we shouldn't rely on
> those names but it will still confuse people.)  Can they be some other name
> that better represents what they're trying to be?
> 
> > Maybe I missed. Does NFIT_SPA_VOLATILE region also generate a device in /dev
> > that it can be mounted with filesystem? 
> 
> Yes.
> 
> -- ljk
>

I checked the NVDIMM driver, there have 3 drivers call register_blkdev() that
may generates "nd_blk", "btt" and "pmem" block devices. But I didn't find the
block device that it reflects to volatile memory SPA region. How can I mount
a volatile memory region with a filesystem?

I have tried to call nvdimm_volatile_region_create() in my patch when VCD
region shows up. But I didn't find any new block device in /dev/devices.


Thanks a lot!
Joey Lee 

Re: [PATCH] libnvdimm, nfit: treat volatile virtual CD region as read-only pmem

[joeyli]

On Thu, Jun 09, 2016 at 03:34:52PM -0700, Dan Williams wrote:
> On Thu, Jun 9, 2016 at 3:08 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
> > On 6/4/2016 7:01 AM, joeyli wrote:
> >> Hi Dan,
> >>
> >> Thanks for your review.
> >>
> >> On Fri, Jun 03, 2016 at 12:27:34PM -0700, Dan Williams wrote:
> >>> On Fri, Jun 3, 2016 at 12:13 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> >>>> This patch adds codes to treat a volatile virtual CD region as a
> >>>> read-only pmem region, then read-only /dev/pmem* device can be mounted
> >>>> with iso9660.
> >>>>
> >>>> It's useful to work with the httpboot in EFI firmware to pull a remote
> >>>> ISO file to the local memory region for booting and installation.
> >>>>
> >>>> Wiki page of UEFI HTTPBoot with OVMF:
> >>>>         https://en.opensuse.org/UEFI_HTTPBoot_with_OVMF
> >>>>
> >>>> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> >>>> Cc: Gary Lin <GLin@suse.com>
> >>>> Cc: Dan Williams <dan.j.williams@intel.com>
> >>>> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> >>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >>>> ---
> >>>>  drivers/acpi/nfit.c          |  8 +++++++-
> >>>>  drivers/nvdimm/region_devs.c | 26 +++++++++++++++++++++++++-
> >>>>  include/linux/libnvdimm.h    |  2 ++
> >>>>  3 files changed, 34 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> >>>> index 2215fc8..b100a17 100644
> >>>> --- a/drivers/acpi/nfit.c
> >>>> +++ b/drivers/acpi/nfit.c
> >>>> @@ -1949,6 +1949,7 @@ static int acpi_nfit_init_mapping(struct acpi_nfit_desc *acpi_desc,
> >>>>         switch (nfit_spa_type(spa)) {
> >>>>         case NFIT_SPA_PM:
> >>>>         case NFIT_SPA_VOLATILE:
> >>>> +       case NFIT_SPA_VCD:
> >>>>                 nd_mapping->start = memdev->address;
> >>>>                 nd_mapping->size = memdev->region_size;
> >>>>                 break;
> >>>
> >>> Why do we need to distinguish NFIT_SPA_VOLATILE vs NFIT_SPA_VCD, i.e.
> >>> what happens if something writes to a VCD device?
> >>
> >> Actually I didn't try to write SPA-VCD device before. Every time I mount it
> >> that the system responses read-only:
> >>
> >> # mount /dev/pmem0 /mnt/
> >> mount: /dev/pmem0 is write-protected, mounting read-only
> >>
> >> If it can be written, then I think there have no difference between
> >> NFIT_SPA_VOLATILE with NFIT_SPA_VCD region.
> >
> > It's not clear to me what the expectations for this type of device are, or
> > whether they should be read-only.  The ACPI spec is not helpful here.
> > The other Disk Region and CD Region types are also unclear.  Anyone
> > care to define them?
> >
> >> I implemented this patch to treat VCD region as read-only pmem because the
> >> pmem region generates /dev/pmem* device that it can be mounted.
> >
> > I'm a bit worried about this type of device showing up as a "pmem" device.
> > I realize they're described in the NFIT (not sure why but they are) but do
> > any of the operations that we support for other pmem devices work on these?
> 
> It would be just another pmem device, so it would support all of them.
> 
> > Do root device DSMs make any sense?
> 
> Root device DSMs take a physical address so they could apply, but I
> assume a firmware could simply refuse to run any operations against
> them.
> 

Yes, I pasted the ACPI0012 root device in another mail that it doesn't
have _DSM method.

> > Are there other DSMs?
> 
> Not that I can think of...
> 
> > What will happen if someone uses ndctl to reconfigure the device?
> 
> It won't know or care about the difference.  Unless there's a negative
> side effect of allowing writes to a "volatile cd" it would be yet
> another pmem device.
>

I didn't try to using ndctl because the ACPI device doesn't have _DSM method.
 
> > I'm especially concerned on systems that might have one of these devices
> > and also have NVDIMMs.  Do all the pmem devices get numbered different if
> > this device comes and goes across reboots?  (I know, we shouldn't rely on
> > those names but it will still confuse people.)  Can they be some other name
> > that better represents what they're trying to be?
> 
> If they all go through the same driver they should have the same
> naming scheme.  Software should be identifying the block device via
> blkid, not kernel device name.

Honestly I didn't have machine to test this function with a real NVDIMM memory.

This function may also applies to the machines that do not have NVDIMM memory
but it provides httpboot function with ISO file for booting or installation. So
it only generates a ACPI0012 root device and a simple NFIT that it only contains
one SPA range structure.

If we don't want to treat VCD type as PMEM device (I will removed the read-only
parts in patch), then I think that we need another NVDIMM driver to generate
block device for VCD type.


Thanks a lot!
Joey Lee