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@xxxxxxxxx> 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@xxxxxxxx>
> >>> Cc: Gary Lin <GLin@xxxxxxxx>
> >>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> >>> Cc: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>
> >>> Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> >>> ---
> >>>  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