| Message ID | 1477850917-1214-31-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Sun, 30 Oct 2016 23:24:46 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > From: Xiao Guangrong <guangrong.xiao@linux.intel.com> > > According to ACPI 6.0 spec, "Memory Device Physical Address > Region Base" in memdev is defined as "This field provides the > Device Physical Address base of the region". This field should > be zero in our case I'm not sure that it should be a zero, care to point source which tells that it should be zero? > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/acpi/nvdimm.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > index bbb2cfd..c2f5caa 100644 > --- a/hw/acpi/nvdimm.c > +++ b/hw/acpi/nvdimm.c > @@ -289,8 +289,6 @@ static void > nvdimm_build_structure_memdev(GArray *structures, DeviceState *dev) > { > NvdimmNfitMemDev *nfit_memdev; > - uint64_t addr = object_property_get_int(OBJECT(dev), PC_DIMM_ADDR_PROP, > - NULL); > uint64_t size = object_property_get_int(OBJECT(dev), PC_DIMM_SIZE_PROP, > NULL); > int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, > @@ -314,7 +312,8 @@ nvdimm_build_structure_memdev(GArray *structures, DeviceState *dev) > > /* The memory region on the device. */ > nfit_memdev->region_len = cpu_to_le64(size); > - nfit_memdev->region_dpa = cpu_to_le64(addr); > + /* The device address starts from 0. */ > + nfit_memdev->region_dpa = cpu_to_le64(0); > > /* Only one interleave for PMEM. */ > nfit_memdev->interleave_ways = cpu_to_le16(1);
On 10/31/2016 05:20 PM, Igor Mammedov wrote: > On Sun, 30 Oct 2016 23:24:46 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> From: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> >> According to ACPI 6.0 spec, "Memory Device Physical Address >> Region Base" in memdev is defined as "This field provides the >> Device Physical Address base of the region". This field should >> be zero in our case > I'm not sure that it should be a zero, > care to point source which tells that it should be zero? The spec says that this is the Device Physical Address, so that it is the device internal address, it should be zero as we do not reserve any thing in device internal and we do not have no memory interleave. Actually, this bug was exported when we were enabling nvdimm in windows guest.
On Mon, 31 Oct 2016 17:23:31 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > On 10/31/2016 05:20 PM, Igor Mammedov wrote: > > On Sun, 30 Oct 2016 23:24:46 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > >> From: Xiao Guangrong <guangrong.xiao@linux.intel.com> > >> > >> According to ACPI 6.0 spec, "Memory Device Physical Address > >> Region Base" in memdev is defined as "This field provides the > >> Device Physical Address base of the region". This field should > >> be zero in our case > > I'm not sure that it should be a zero, > > care to point source which tells that it should be zero? > > The spec says that this is the Device Physical Address, so that > it is the device internal address, it should be zero as we do not > reserve any thing in device internal and we do not have no memory > interleave. spec says (ACPI 6.1: 5.2.25.3 NVDIMM Region Mapping Structure): "NVDIMM Physical Address Region Base": "The base physical address within the NVDIMM of the NVDIMM region." and nothing more than that so it's hard to come to conclusion that it's internal address nor it is offset as you treat it here (structure has 'Region Offset' for that). However there is "NVDIMM Region Size" field "In bytes. The size of the NVDIMM region. If SPA Range Structure Index and Interleave Ways are both non-zero, this field shall match System Physical Address Range Length divided by Interleave Ways." matches SPA structure, which makes me think that "NVDIMM Physical Address Region Base" similarly should match "System Physical Address Range Base" from SPA. > Actually, this bug was exported when we were enabling nvdimm in > windows guest. since it's rather new technology there isn't guaranty that Windows got it right yet. If spec is not clear we should ask for clarification and in mean time look at existing hardware for example.
On 10/31/2016 06:56 PM, Igor Mammedov wrote: > On Mon, 31 Oct 2016 17:23:31 +0800 > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > >> On 10/31/2016 05:20 PM, Igor Mammedov wrote: >>> On Sun, 30 Oct 2016 23:24:46 +0200 >>> "Michael S. Tsirkin" <mst@redhat.com> wrote: >>> >>>> From: Xiao Guangrong <guangrong.xiao@linux.intel.com> >>>> >>>> According to ACPI 6.0 spec, "Memory Device Physical Address >>>> Region Base" in memdev is defined as "This field provides the >>>> Device Physical Address base of the region". This field should >>>> be zero in our case >>> I'm not sure that it should be a zero, >>> care to point source which tells that it should be zero? >> >> The spec says that this is the Device Physical Address, so that >> it is the device internal address, it should be zero as we do not >> reserve any thing in device internal and we do not have no memory >> interleave. > spec says (ACPI 6.1: 5.2.25.3 NVDIMM Region Mapping Structure): > "NVDIMM Physical Address Region Base": > "The base physical address within the NVDIMM of the NVDIMM region." > > and nothing more than that so it's hard to come to conclusion that > it's internal address nor it is offset as you treat it here > (structure has 'Region Offset' for that). I think it is clear as the spec says "_within_ the NVDIMM", it is even more clear than ACPI 6.0 (5.2.25.2 Memory Device to System Physical Address Range Mapping Structure), in that, it says: "In bytes. This field provides the Device Physical Address base of the region." In the namespace spec (http://pmem.io/documents/NVDIMM_Namespace_Spec.pdf), the explanation to DPA in page 9: "DIMM Physical Address: A memory address from a DIMM’s perspective, that is, the offset into the DIMM’s memory, starting with DPA zero as the lowest addressable byte of the DIMM.
On Mon, 31 Oct 2016 19:09:07 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > On 10/31/2016 06:56 PM, Igor Mammedov wrote: > > On Mon, 31 Oct 2016 17:23:31 +0800 > > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > > > >> On 10/31/2016 05:20 PM, Igor Mammedov wrote: > >>> On Sun, 30 Oct 2016 23:24:46 +0200 > >>> "Michael S. Tsirkin" <mst@redhat.com> wrote: > >>> > >>>> From: Xiao Guangrong <guangrong.xiao@linux.intel.com> > >>>> > >>>> According to ACPI 6.0 spec, "Memory Device Physical Address > >>>> Region Base" in memdev is defined as "This field provides the > >>>> Device Physical Address base of the region". This field should > >>>> be zero in our case > >>> I'm not sure that it should be a zero, > >>> care to point source which tells that it should be zero? > >> > >> The spec says that this is the Device Physical Address, so that > >> it is the device internal address, it should be zero as we do not > >> reserve any thing in device internal and we do not have no memory > >> interleave. > > spec says (ACPI 6.1: 5.2.25.3 NVDIMM Region Mapping Structure): > > "NVDIMM Physical Address Region Base": > > "The base physical address within the NVDIMM of the NVDIMM region." > > > > and nothing more than that so it's hard to come to conclusion that > > it's internal address nor it is offset as you treat it here > > (structure has 'Region Offset' for that). > > I think it is clear as the spec says "_within_ the NVDIMM", it is > even more clear than ACPI 6.0 (5.2.25.2 Memory Device to System Physical > Address Range Mapping Structure), in that, it says: > "In bytes. This field provides the Device Physical Address > base of the region." It would be less confusing if spec would say "starting offset of NVDIMM region within the NVDIMM" (at least that's how I read it now after looking at 6.0 and NVDIMM_Namespace_Spec.pdf) as "base physical address" is not defined/described in ACPI spec. > In the namespace spec (http://pmem.io/documents/NVDIMM_Namespace_Spec.pdf), > the explanation to DPA in page 9: > "DIMM Physical Address: A memory address from a DIMM’s perspective, > that is, the offset into the DIMM’s memory, starting > with DPA zero > as the lowest addressable byte of the DIMM.
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index bbb2cfd..c2f5caa 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -289,8 +289,6 @@ static void nvdimm_build_structure_memdev(GArray *structures, DeviceState *dev) { NvdimmNfitMemDev *nfit_memdev; - uint64_t addr = object_property_get_int(OBJECT(dev), PC_DIMM_ADDR_PROP, - NULL); uint64_t size = object_property_get_int(OBJECT(dev), PC_DIMM_SIZE_PROP, NULL); int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, @@ -314,7 +312,8 @@ nvdimm_build_structure_memdev(GArray *structures, DeviceState *dev) /* The memory region on the device. */ nfit_memdev->region_len = cpu_to_le64(size); - nfit_memdev->region_dpa = cpu_to_le64(addr); + /* The device address starts from 0. */ + nfit_memdev->region_dpa = cpu_to_le64(0); /* Only one interleave for PMEM. */ nfit_memdev->interleave_ways = cpu_to_le16(1);