Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Igor Mammedov 
On Fri, 4 Nov 2016 01:39:31 +0800
Xiao Guangrong  wrote:

> 
> 
> On 11/04/2016 01:29 AM, Igor Mammedov wrote:
> > On Fri, 4 Nov 2016 00:53:06 +0800
> > Xiao Guangrong  wrote:
> >
> >>
> >>
> >> On 11/04/2016 12:49 AM, Igor Mammedov wrote:
> >>> On Fri, 4 Nov 2016 00:17:00 +0800
> >>> Xiao Guangrong  wrote:
> >>>
> 
> 
>  On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> > On Thu, 3 Nov 2016 22:53:43 +0800
> > Xiao Guangrong  wrote:
> >
> >>
> >>
> >> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> >>> On Thu, 3 Nov 2016 21:02:22 +0800
> >>> Xiao Guangrong  wrote:
> >>>
> 
> 
>  On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> 
> 
> 
> 
> >>> just drop this and describe properly 'len' in spec section
> >>> i.e. len: length of entire returned data (including the
> >>> header)
> >>
> >> Okay, i will change the spec like this:
> >>
> >> QEMU Writes Output Data (based on the offset in the
> >> page): [0x0 - 0x3]: 4 bytes, length of entire returned data
> >> (including the header)
> >>
> >> And drop the length field in Read_Fit return buffer, doc
> >> the fit buffer like this:
> >>
> >> 
> >> +--+++---+
> >> |  Field   | Length | Offset |
> >> Description   |
> >> +--+++---+
> > you need to add length here, otherwise this table is not
> > correct
> 
>  Ah, so i am confused.
> 
>  struct NvdimmFuncReadFITOut definition is based on the layout
>  of Read_FI output. You suggested to drop the length filed in
>  NvdimmFuncReadFITOut but keep it in the layout, it is not
>  consistent.
> 
>  I missed something?
> >>>
> >>> +struct NvdimmFuncReadFITOut {
> >>> +/* the size of buffer filled by QEMU. */
> >>> +uint32_t len;
> >>> +uint32_t func_ret_status; /* return status code. */
> >>> +uint8_t fit[0]; /* the FIT data. */
> >>> +} QEMU_PACKED;
> >>>
> >>> 
> >>> | field   | len | off | desc...
> >>> 
> >>> | length  |  4  |  0  | 
> >>> 
> >>> | status  |  4  |  4  | 
> >>> 
> >>> | fit data| 
> >>>
> >>> i.e. you were forgetting to add length in spec so offsets were
> >>> wrong even for described fields.
> >>
> >>
> >> We can not do this.
> >>
> >> @len is used by QEMU emulation to count the size of the buffer
> >> that _DSM should return. It's only used in NVDIMM_COMMON_DSM
> >> method which is shared by the DSM method from VM and Read_Fit.
> > spec describes buffer layout independently from AML that uses it,
> > so it should describe whole data structure.
> >
> > Then it's upto guest how to read this data, it could be QEMU
> > generated AML (as it's here) or some other driver or even BIOS.
> 
>  However, what we are talking about is Read_FIT method, so this is
>  the layout of Read_FIT output rather than all memory in the dsm
>  page.
> 
>  Actually, in the spec we already have documented the common len
>  field:
> 
>  QEMU Writes Output Data (based on the offset in the page):
>  [0x0 - 0x3]: 4 bytes, the length of result
>  [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
> 
>  Also, i really do not hope to use this field to count the buffer
>  size returned by Read_FIT, we'd try the best to reuse existing DSM
>  method (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM
>  method.
> >>> buffer layout describes interface between QEMU and firmware (AML)
> >>> and it should describe it completely every time to avoid confusion.
> >>>
> >>> See for example ACPI spec, NFIT table, SRAT table, ...
> >>> all table descriptions start with complete header.
> >>
> >> Okay. Understood. :)
> >>
> >>>
> >>> If you skip length it rises question how much fit data are there,
> >>> meaning interface description isn't complete.
> >>
> >> So how about introduce a length field in the output buffer just
> >> as this patch did? I understand we are able to count the size
> >> from dsm len, however, it can simplify the code a lot...
> > it's redundant as there already is length for whole buffer,
> > interface should be kept as simple as possible and not include
> > redundant data for convenience.
> 
> Okay.
> 
> So the doc

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Xiao Guangrong 



On 11/04/2016 01:29 AM, Igor Mammedov wrote:

On Fri, 4 Nov 2016 00:53:06 +0800
Xiao Guangrong  wrote:




On 11/04/2016 12:49 AM, Igor Mammedov wrote:

On Fri, 4 Nov 2016 00:17:00 +0800
Xiao Guangrong  wrote:




On 11/04/2016 12:13 AM, Igor Mammedov wrote:

On Thu, 3 Nov 2016 22:53:43 +0800
Xiao Guangrong  wrote:




On 11/03/2016 10:49 PM, Igor Mammedov wrote:

On Thu, 3 Nov 2016 21:02:22 +0800
Xiao Guangrong  wrote:




On 11/03/2016 09:00 PM, Igor Mammedov wrote:





just drop this and describe properly 'len' in spec section
i.e. len: length of entire returned data (including the
header)


Okay, i will change the spec like this:

QEMU Writes Output Data (based on the offset in the
page): [0x0 - 0x3]: 4 bytes, length of entire returned data
(including the header)

And drop the length field in Read_Fit return buffer, doc
the fit buffer like this:

+--+++---+
|  Field   | Length | Offset |
Description   |
+--+++---+

you need to add length here, otherwise this table is not
correct


Ah, so i am confused.

struct NvdimmFuncReadFITOut definition is based on the layout
of Read_FI output. You suggested to drop the length filed in
NvdimmFuncReadFITOut but keep it in the layout, it is not
consistent.

I missed something?


+struct NvdimmFuncReadFITOut {
+/* the size of buffer filled by QEMU. */
+uint32_t len;
+uint32_t func_ret_status; /* return status code. */
+uint8_t fit[0]; /* the FIT data. */
+} QEMU_PACKED;


| field   | len | off | desc...

| length  |  4  |  0  | 

| status  |  4  |  4  | 

| fit data| 

i.e. you were forgetting to add length in spec so offsets were
wrong even for described fields.



We can not do this.

@len is used by QEMU emulation to count the size of the buffer
that _DSM should return. It's only used in NVDIMM_COMMON_DSM
method which is shared by the DSM method from VM and Read_Fit.

spec describes buffer layout independently from AML that uses it,
so it should describe whole data structure.

Then it's upto guest how to read this data, it could be QEMU
generated AML (as it's here) or some other driver or even BIOS.


However, what we are talking about is Read_FIT method, so this is
the layout of Read_FIT output rather than all memory in the dsm
page.

Actually, in the spec we already have documented the common len
field:

QEMU Writes Output Data (based on the offset in the page):
[0x0 - 0x3]: 4 bytes, the length of result
[0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU

Also, i really do not hope to use this field to count the buffer
size returned by Read_FIT, we'd try the best to reuse existing DSM
method (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM
method.

buffer layout describes interface between QEMU and firmware (AML)
and it should describe it completely every time to avoid confusion.

See for example ACPI spec, NFIT table, SRAT table, ...
all table descriptions start with complete header.


Okay. Understood. :)



If you skip length it rises question how much fit data are there,
meaning interface description isn't complete.


So how about introduce a length field in the output buffer just
as this patch did? I understand we are able to count the size
from dsm len, however, it can simplify the code a lot...

it's redundant as there already is length for whole buffer,
interface should be kept as simple as possible and not include
redundant data for convenience.


Okay.

So the doc should be changed to:

   Output layout in the dsm memory page:

   +--+++---+
   |  Field   | Length | Offset | Description   |
   +--+++---+
   | length   |   4|   4| length of entire returned data|
   |  ||| (including the header)|
   +--+-+---+
   |  ||| return status codes   |
   |  ||| 0x100 - error caused by NFIT update while |
   | status   |   4|   4| read by _FIT wasn't completed, other  |
   |  ||| codes follow Chapter 3 in DSM Spec Rev1   |
   +--+++---+
   | fit data | Varies |   8| FIT data, the remaining size is used by   |
   |  ||| fit data if status is success;|
   |  ||| otherwise it

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Igor Mammedov 
On Fri, 4 Nov 2016 00:53:06 +0800
Xiao Guangrong  wrote:

> 
> 
> On 11/04/2016 12:49 AM, Igor Mammedov wrote:
> > On Fri, 4 Nov 2016 00:17:00 +0800
> > Xiao Guangrong  wrote:
> >
> >>
> >>
> >> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> >>> On Thu, 3 Nov 2016 22:53:43 +0800
> >>> Xiao Guangrong  wrote:
> >>>
> 
> 
>  On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> > On Thu, 3 Nov 2016 21:02:22 +0800
> > Xiao Guangrong  wrote:
> >
> >>
> >>
> >> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> >>
> >>
> >>
> >>
> > just drop this and describe properly 'len' in spec section
> > i.e. len: length of entire returned data (including the
> > header)
> 
>  Okay, i will change the spec like this:
> 
>  QEMU Writes Output Data (based on the offset in the
>  page): [0x0 - 0x3]: 4 bytes, length of entire returned data
>  (including the header)
> 
>  And drop the length field in Read_Fit return buffer, doc
>  the fit buffer like this:
> 
>  
>  +--+++---+
>  |  Field   | Length | Offset |
>  Description   |
>  +--+++---+
> >>> you need to add length here, otherwise this table is not
> >>> correct
> >>
> >> Ah, so i am confused.
> >>
> >> struct NvdimmFuncReadFITOut definition is based on the layout
> >> of Read_FI output. You suggested to drop the length filed in
> >> NvdimmFuncReadFITOut but keep it in the layout, it is not
> >> consistent.
> >>
> >> I missed something?
> >
> > +struct NvdimmFuncReadFITOut {
> > +/* the size of buffer filled by QEMU. */
> > +uint32_t len;
> > +uint32_t func_ret_status; /* return status code. */
> > +uint8_t fit[0]; /* the FIT data. */
> > +} QEMU_PACKED;
> >
> > 
> > | field   | len | off | desc...
> > 
> > | length  |  4  |  0  | 
> > 
> > | status  |  4  |  4  | 
> > 
> > | fit data| 
> >
> > i.e. you were forgetting to add length in spec so offsets were
> > wrong even for described fields.
> 
> 
>  We can not do this.
> 
>  @len is used by QEMU emulation to count the size of the buffer
>  that _DSM should return. It's only used in NVDIMM_COMMON_DSM
>  method which is shared by the DSM method from VM and Read_Fit.
> >>> spec describes buffer layout independently from AML that uses it,
> >>> so it should describe whole data structure.
> >>>
> >>> Then it's upto guest how to read this data, it could be QEMU
> >>> generated AML (as it's here) or some other driver or even BIOS.
> >>
> >> However, what we are talking about is Read_FIT method, so this is
> >> the layout of Read_FIT output rather than all memory in the dsm
> >> page.
> >>
> >> Actually, in the spec we already have documented the common len
> >> field:
> >>
> >> QEMU Writes Output Data (based on the offset in the page):
> >> [0x0 - 0x3]: 4 bytes, the length of result
> >> [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
> >>
> >> Also, i really do not hope to use this field to count the buffer
> >> size returned by Read_FIT, we'd try the best to reuse existing DSM
> >> method (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM
> >> method.
> > buffer layout describes interface between QEMU and firmware (AML)
> > and it should describe it completely every time to avoid confusion.
> >
> > See for example ACPI spec, NFIT table, SRAT table, ...
> > all table descriptions start with complete header.
> 
> Okay. Understood. :)
> 
> >
> > If you skip length it rises question how much fit data are there,
> > meaning interface description isn't complete.
> 
> So how about introduce a length field in the output buffer just
> as this patch did? I understand we are able to count the size
> from dsm len, however, it can simplify the code a lot...
it's redundant as there already is length for whole buffer,
interface should be kept as simple as possible and not include
redundant data for convenience.

> 
> >
> > if you want to describe AML there you can do it after interface
> > description saying that common NCAL method extracts status and fit
> > data form dsm page and returns that as buffer object, but it's AML
> > impl. specific. I could write an alternative AML code that would
> > deal with dms page in its own way as long as I would know what
> > write/read at what offset.
> 
> Yes, i agree with you.
>

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Xiao Guangrong 



On 11/04/2016 12:49 AM, Igor Mammedov wrote:

On Fri, 4 Nov 2016 00:17:00 +0800
Xiao Guangrong  wrote:




On 11/04/2016 12:13 AM, Igor Mammedov wrote:

On Thu, 3 Nov 2016 22:53:43 +0800
Xiao Guangrong  wrote:




On 11/03/2016 10:49 PM, Igor Mammedov wrote:

On Thu, 3 Nov 2016 21:02:22 +0800
Xiao Guangrong  wrote:




On 11/03/2016 09:00 PM, Igor Mammedov wrote:





just drop this and describe properly 'len' in spec section
i.e. len: length of entire returned data (including the
header)


Okay, i will change the spec like this:

QEMU Writes Output Data (based on the offset in the page):
[0x0 - 0x3]: 4 bytes, length of entire returned data
(including the header)

And drop the length field in Read_Fit return buffer, doc
the fit buffer like this:

+--+++---+
|  Field   | Length | Offset |
Description   |
+--+++---+

you need to add length here, otherwise this table is not correct


Ah, so i am confused.

struct NvdimmFuncReadFITOut definition is based on the layout of
Read_FI output. You suggested to drop the length filed in
NvdimmFuncReadFITOut but keep it in the layout, it is not
consistent.

I missed something?


+struct NvdimmFuncReadFITOut {
+/* the size of buffer filled by QEMU. */
+uint32_t len;
+uint32_t func_ret_status; /* return status code. */
+uint8_t fit[0]; /* the FIT data. */
+} QEMU_PACKED;


| field   | len | off | desc...

| length  |  4  |  0  | 

| status  |  4  |  4  | 

| fit data| 

i.e. you were forgetting to add length in spec so offsets were
wrong even for described fields.



We can not do this.

@len is used by QEMU emulation to count the size of the buffer that
_DSM should return. It's only used in NVDIMM_COMMON_DSM method
which is shared by the DSM method from VM and Read_Fit.

spec describes buffer layout independently from AML that uses it,
so it should describe whole data structure.

Then it's upto guest how to read this data, it could be QEMU
generated AML (as it's here) or some other driver or even BIOS.


However, what we are talking about is Read_FIT method, so this is
the layout of Read_FIT output rather than all memory in the dsm page.

Actually, in the spec we already have documented the common len field:

QEMU Writes Output Data (based on the offset in the page):
[0x0 - 0x3]: 4 bytes, the length of result
[0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU

Also, i really do not hope to use this field to count the buffer size
returned by Read_FIT, we'd try the best to reuse existing DSM method
(NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM method.

buffer layout describes interface between QEMU and firmware (AML)
and it should describe it completely every time to avoid confusion.

See for example ACPI spec, NFIT table, SRAT table, ...
all table descriptions start with complete header.


Okay. Understood. :)



If you skip length it rises question how much fit data are there,
meaning interface description isn't complete.


So how about introduce a length field in the output buffer just
as this patch did? I understand we are able to count the size
from dsm len, however, it can simplify the code a lot...



if you want to describe AML there you can do it after interface
description saying that common NCAL method extracts status and fit
data form dsm page and returns that as buffer object, but it's AML
impl. specific. I could write an alternative AML code that would deal
with dms page in its own way as long as I would know what write/read at
what offset.


Yes, i agree with you.

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Igor Mammedov 
On Fri, 4 Nov 2016 00:17:00 +0800
Xiao Guangrong  wrote:

> 
> 
> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> > On Thu, 3 Nov 2016 22:53:43 +0800
> > Xiao Guangrong  wrote:
> >
> >>
> >>
> >> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> >>> On Thu, 3 Nov 2016 21:02:22 +0800
> >>> Xiao Guangrong  wrote:
> >>>
> 
> 
>  On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> 
> 
> 
> 
> >>> just drop this and describe properly 'len' in spec section
> >>> i.e. len: length of entire returned data (including the
> >>> header)
> >>
> >> Okay, i will change the spec like this:
> >>
> >> QEMU Writes Output Data (based on the offset in the page):
> >> [0x0 - 0x3]: 4 bytes, length of entire returned data
> >> (including the header)
> >>
> >> And drop the length field in Read_Fit return buffer, doc
> >> the fit buffer like this:
> >>
> >> 
> >> +--+++---+
> >> |  Field   | Length | Offset |
> >> Description   |
> >> +--+++---+
> > you need to add length here, otherwise this table is not correct
> 
>  Ah, so i am confused.
> 
>  struct NvdimmFuncReadFITOut definition is based on the layout of
>  Read_FI output. You suggested to drop the length filed in
>  NvdimmFuncReadFITOut but keep it in the layout, it is not
>  consistent.
> 
>  I missed something?
> >>>
> >>> +struct NvdimmFuncReadFITOut {
> >>> +/* the size of buffer filled by QEMU. */
> >>> +uint32_t len;
> >>> +uint32_t func_ret_status; /* return status code. */
> >>> +uint8_t fit[0]; /* the FIT data. */
> >>> +} QEMU_PACKED;
> >>>
> >>> 
> >>> | field   | len | off | desc...
> >>> 
> >>> | length  |  4  |  0  | 
> >>> 
> >>> | status  |  4  |  4  | 
> >>> 
> >>> | fit data| 
> >>>
> >>> i.e. you were forgetting to add length in spec so offsets were
> >>> wrong even for described fields.
> >>
> >>
> >> We can not do this.
> >>
> >> @len is used by QEMU emulation to count the size of the buffer that
> >> _DSM should return. It's only used in NVDIMM_COMMON_DSM method
> >> which is shared by the DSM method from VM and Read_Fit.
> > spec describes buffer layout independently from AML that uses it,
> > so it should describe whole data structure.
> >
> > Then it's upto guest how to read this data, it could be QEMU
> > generated AML (as it's here) or some other driver or even BIOS.
> 
> However, what we are talking about is Read_FIT method, so this is
> the layout of Read_FIT output rather than all memory in the dsm page.
> 
> Actually, in the spec we already have documented the common len field:
> 
> QEMU Writes Output Data (based on the offset in the page):
> [0x0 - 0x3]: 4 bytes, the length of result
> [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
> 
> Also, i really do not hope to use this field to count the buffer size
> returned by Read_FIT, we'd try the best to reuse existing DSM method
> (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM method.
buffer layout describes interface between QEMU and firmware (AML)
and it should describe it completely every time to avoid confusion.

See for example ACPI spec, NFIT table, SRAT table, ...
all table descriptions start with complete header.

If you skip length it rises question how much fit data are there,
meaning interface description isn't complete.

if you want to describe AML there you can do it after interface
description saying that common NCAL method extracts status and fit
data form dsm page and returns that as buffer object, but it's AML
impl. specific. I could write an alternative AML code that would deal
with dms page in its own way as long as I would know what write/read at
what offset.


> 
> 
> 
> 
>

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Xiao Guangrong 



On 11/04/2016 12:13 AM, Igor Mammedov wrote:

On Thu, 3 Nov 2016 22:53:43 +0800
Xiao Guangrong  wrote:




On 11/03/2016 10:49 PM, Igor Mammedov wrote:

On Thu, 3 Nov 2016 21:02:22 +0800
Xiao Guangrong  wrote:




On 11/03/2016 09:00 PM, Igor Mammedov wrote:





just drop this and describe properly 'len' in spec section
i.e. len: length of entire returned data (including the header)


Okay, i will change the spec like this:

QEMU Writes Output Data (based on the offset in the page):
[0x0 - 0x3]: 4 bytes, length of entire returned data
(including the header)

And drop the length field in Read_Fit return buffer, doc
the fit buffer like this:

+--+++---+
|  Field   | Length | Offset |
Description   |
+--+++---+

you need to add length here, otherwise this table is not correct


Ah, so i am confused.

struct NvdimmFuncReadFITOut definition is based on the layout of
Read_FI output. You suggested to drop the length filed in
NvdimmFuncReadFITOut but keep it in the layout, it is not
consistent.

I missed something?


+struct NvdimmFuncReadFITOut {
+/* the size of buffer filled by QEMU. */
+uint32_t len;
+uint32_t func_ret_status; /* return status code. */
+uint8_t fit[0]; /* the FIT data. */
+} QEMU_PACKED;


| field   | len | off | desc...

| length  |  4  |  0  | 

| status  |  4  |  4  | 

| fit data| 

i.e. you were forgetting to add length in spec so offsets were wrong
even for described fields.



We can not do this.

@len is used by QEMU emulation to count the size of the buffer that
_DSM should return. It's only used in NVDIMM_COMMON_DSM method which
is shared by the DSM method from VM and Read_Fit.

spec describes buffer layout independently from AML that uses it,
so it should describe whole data structure.

Then it's upto guest how to read this data, it could be QEMU generated
AML (as it's here) or some other driver or even BIOS.


However, what we are talking about is Read_FIT method, so this is
the layout of Read_FIT output rather than all memory in the dsm page.

Actually, in the spec we already have documented the common len field:

   QEMU Writes Output Data (based on the offset in the page):
   [0x0 - 0x3]: 4 bytes, the length of result
   [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU

Also, i really do not hope to use this field to count the buffer size
returned by Read_FIT, we'd try the best to reuse existing DSM method
(NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM method.

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Igor Mammedov 
On Thu, 3 Nov 2016 22:53:43 +0800
Xiao Guangrong  wrote:

> 
> 
> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> > On Thu, 3 Nov 2016 21:02:22 +0800
> > Xiao Guangrong  wrote:
> >
> >>
> >>
> >> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> >>
> >>
> >>
> >>
> > just drop this and describe properly 'len' in spec section
> > i.e. len: length of entire returned data (including the header)
> 
>  Okay, i will change the spec like this:
> 
>  QEMU Writes Output Data (based on the offset in the page):
>  [0x0 - 0x3]: 4 bytes, length of entire returned data
>  (including the header)
> 
>  And drop the length field in Read_Fit return buffer, doc
>  the fit buffer like this:
> 
>  
>  +--+++---+
>  |  Field   | Length | Offset |
>  Description   |
>  +--+++---+
> >>> you need to add length here, otherwise this table is not correct
> >>
> >> Ah, so i am confused.
> >>
> >> struct NvdimmFuncReadFITOut definition is based on the layout of
> >> Read_FI output. You suggested to drop the length filed in
> >> NvdimmFuncReadFITOut but keep it in the layout, it is not
> >> consistent.
> >>
> >> I missed something?
> >
> > +struct NvdimmFuncReadFITOut {
> > +/* the size of buffer filled by QEMU. */
> > +uint32_t len;
> > +uint32_t func_ret_status; /* return status code. */
> > +uint8_t fit[0]; /* the FIT data. */
> > +} QEMU_PACKED;
> >
> > 
> > | field   | len | off | desc...
> > 
> > | length  |  4  |  0  | 
> > 
> > | status  |  4  |  4  | 
> > 
> > | fit data| 
> >
> > i.e. you were forgetting to add length in spec so offsets were wrong
> > even for described fields.
> 
> 
> We can not do this.
> 
> @len is used by QEMU emulation to count the size of the buffer that
> _DSM should return. It's only used in NVDIMM_COMMON_DSM method which
> is shared by the DSM method from VM and Read_Fit.
spec describes buffer layout independently from AML that uses it,
so it should describe whole data structure.

Then it's upto guest how to read this data, it could be QEMU generated
AML (as it's here) or some other driver or even BIOS.

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Xiao Guangrong 



On 11/03/2016 10:49 PM, Igor Mammedov wrote:

On Thu, 3 Nov 2016 21:02:22 +0800
Xiao Guangrong  wrote:




On 11/03/2016 09:00 PM, Igor Mammedov wrote:





just drop this and describe properly 'len' in spec section
i.e. len: length of entire returned data (including the header)


Okay, i will change the spec like this:

QEMU Writes Output Data (based on the offset in the page):
[0x0 - 0x3]: 4 bytes, length of entire returned data
(including the header)

And drop the length field in Read_Fit return buffer, doc
the fit buffer like this:

+--+++---+
|  Field   | Length | Offset |
Description   |
+--+++---+

you need to add length here, otherwise this table is not correct


Ah, so i am confused.

struct NvdimmFuncReadFITOut definition is based on the layout of
Read_FI output. You suggested to drop the length filed in
NvdimmFuncReadFITOut but keep it in the layout, it is not consistent.

I missed something?


+struct NvdimmFuncReadFITOut {
+/* the size of buffer filled by QEMU. */
+uint32_t len;
+uint32_t func_ret_status; /* return status code. */
+uint8_t fit[0]; /* the FIT data. */
+} QEMU_PACKED;


| field   | len | off | desc...

| length  |  4  |  0  | 

| status  |  4  |  4  | 

| fit data| 

i.e. you were forgetting to add length in spec so offsets were wrong
even for described fields.



We can not do this.

@len is used by QEMU emulation to count the size of the buffer that
_DSM should return. It's only used in NVDIMM_COMMON_DSM method which
is shared by the DSM method from VM and Read_Fit.

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Igor Mammedov 
On Thu, 3 Nov 2016 21:02:22 +0800
Xiao Guangrong  wrote:

> 
> 
> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> 
> 
> 
> 
> >>> just drop this and describe properly 'len' in spec section
> >>> i.e. len: length of entire returned data (including the header)
> >>
> >> Okay, i will change the spec like this:
> >>
> >> QEMU Writes Output Data (based on the offset in the page):
> >> [0x0 - 0x3]: 4 bytes, length of entire returned data
> >> (including the header)
> >>
> >> And drop the length field in Read_Fit return buffer, doc
> >> the fit buffer like this:
> >>
> >> 
> >> +--+++---+
> >> |  Field   | Length | Offset |
> >> Description   |
> >> +--+++---+
> > you need to add length here, otherwise this table is not correct
> 
> Ah, so i am confused.
> 
> struct NvdimmFuncReadFITOut definition is based on the layout of
> Read_FI output. You suggested to drop the length filed in
> NvdimmFuncReadFITOut but keep it in the layout, it is not consistent.
> 
> I missed something?

+struct NvdimmFuncReadFITOut {
+/* the size of buffer filled by QEMU. */
+uint32_t len;
+uint32_t func_ret_status; /* return status code. */
+uint8_t fit[0]; /* the FIT data. */
+} QEMU_PACKED;


| field   | len | off | desc...

| length  |  4  |  0  | 

| status  |  4  |  4  | 

| fit data| 

i.e. you were forgetting to add length in spec so offsets were wrong
even for described fields.

> 
> >
> >
> >> |  ||| return status
> >> codes   | |  ||| 0x100
> >> - error caused by NFIT update while | | status   |   4|   0
> >> | read by _FIT wasn't completed, other  | |  |
> >> || codes follow Chapter 3 in DSM Spec Rev1   |
> >> +--+++---+
> >> | fit data | Varies |   8| FIT data, The remaining size in
> >> the   | |  ||| returned buffer is used
> >> by FIT|
> >> +--+++---+
> >>
> >>
> >>
>  +}
>  +
>  +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state,
>  NvdimmDsmIn *in,
>  + hwaddr dsm_mem_addr)
> >>> function name doesn't make any sense to me
> >>
> >> As i explained above, handle 0x1 indicates the reserved _DSM
> >> method is called on the root device...
> >>
> >> It makes sense now? :)
> > function name should reflect what it does,
> > i.e use verb and I see only nouns here.
> 
> Got it, will change it to: nvdimm_handle_reserved_root_method().
>

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Xiao Guangrong 



On 11/03/2016 09:00 PM, Igor Mammedov wrote:





just drop this and describe properly 'len' in spec section
i.e. len: length of entire returned data (including the header)


Okay, i will change the spec like this:

QEMU Writes Output Data (based on the offset in the page):
[0x0 - 0x3]: 4 bytes, length of entire returned data (including the header)

And drop the length field in Read_Fit return buffer, doc
the fit buffer like this:

+--+++---+
|  Field   | Length | Offset | Description   |
+--+++---+

you need to add length here, otherwise this table is not correct


Ah, so i am confused.

struct NvdimmFuncReadFITOut definition is based on the layout of
Read_FI output. You suggested to drop the length filed in NvdimmFuncReadFITOut
but keep it in the layout, it is not consistent.

I missed something?





|  ||| return status codes   |
|  ||| 0x100 - error caused by NFIT update while |
| status   |   4|   0| read by _FIT wasn't completed, other  |
|  ||| codes follow Chapter 3 in DSM Spec Rev1   |
+--+++---+
| fit data | Varies |   8| FIT data, The remaining size in the   |
|  ||| returned buffer is used by FIT|
+--+++---+




+}
+
+static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
+ hwaddr dsm_mem_addr)

function name doesn't make any sense to me


As i explained above, handle 0x1 indicates the reserved _DSM method is
called on the root device...

It makes sense now? :)

function name should reflect what it does,
i.e use verb and I see only nouns here.


Got it, will change it to: nvdimm_handle_reserved_root_method().

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Igor Mammedov 
On Thu, 3 Nov 2016 20:21:05 +0800
Xiao Guangrong  wrote:

> On 11/03/2016 07:58 PM, Igor Mammedov wrote:
> > On Thu,  3 Nov 2016 11:51:29 +0800
> > Xiao Guangrong  wrote:
> >  
> >> _FIT is required for hotplug support, guest will inquire the updated
> >> device info from it if a hotplug event is received  
> > s/_FIT/_FIT method/
> >
> > the same applies to subj. line  
> 
> Okay.
> 
> >  
> >>
> >> As FIT buffer is not completely mapped into guest address space, so a
> >> new function, Read FIT whose UUID is UUID
> >> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x1, function index
> >> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> >> is concatenated before _FIT return  
> >
> > Commit message hard to read/parse, it might be better if I'd use simple
> > short sentences.  
> 
> Okay, will change it to:
> 
> As FIT buffer is not completely mapped into guest address space, Read_FIT
> method is introduced to read NFIT structures blob from QEMU, The buffer
> is concatenated before _FIT return
> 
> >
> >  
> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design
> >>
> >> Signed-off-by: Xiao Guangrong 
> >> ---
> >>  docs/specs/acpi_nvdimm.txt |  63 -
> >>  hw/acpi/nvdimm.c   | 225 
> >> ++---
> >>  2 files changed, 271 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> >> index 0fdd251..364e832 100644
> >> --- a/docs/specs/acpi_nvdimm.txt
> >> +++ b/docs/specs/acpi_nvdimm.txt
> >> @@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
> >> The detailed definition of the structure can be found at ACPI 6.0: 
> >> 5.2.25
> >> NVDIMM Firmware Interface Table (NFIT).
> >>
> >> -QEMU NVDIMM Implemention
> >> -
> >> +QEMU NVDIMM Implementation
> >> +==
> >>  QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
> >>  for NVDIMM ACPI.
> >>
> >> @@ -82,6 +82,16 @@ Memory:
> >> ACPI writes _DSM Input Data (based on the offset in the page):
> >> [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
> >>  Root device.
> >> +
> >> +The handle is completely QEMU internal thing, the values 
> >> in
> >> +range [0, 0x] indicate nvdimm device (O means nvdimm  
> > [1, 0x] indicate nvdimm device
> > and move 0 to reserved section  
> 
> Okay.
> 
> >  
> >> +root device named NVDR), other values are reserved by 
> >> other  
> > s/by/for/  
> 
> okay.
> 
> >  
> >> +purpose.  
> > s/purpose/purposes/  
> 
> okay.
> 
> >  
> >> +Current reserved handle:  
> > s/Current reserved handle/Reserved handles/
> >  
> >> +0x1 is reserved for QEMU internal DSM function called 
> >> on
> >> +the root device.  
> > description is too obscure, I wonder if it could be more specific  
> 
> I would like to make these reserved values more generic in order to
> support more QEMU reserved _DSM methods in the further. So, i planed:
> UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is dedicated for QEMU reserved
> methods. Handle 0x1 indicates the method is for the root device.
> 0x10001 ~ 0x1 indicates the method is for the nvdimm devices.
> 
> As currently we do not have reserved method on nvdimm device, so i
> only document 0x1000 for the root device.
> 
> >
> >  
> >> [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
> >> [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
> >> [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
> >> @@ -127,6 +137,49 @@ _DSM process diagram:
> >>   | result from the page |  |  |
> >>   +--+  +--+
> >>
> >> - _FIT implementation
> >> - ---
> >> - TODO (will fill it when nvdimm hotplug is introduced)
> >> +QEMU internal use only _DSM function
> >> +
> >> +There is the function introduced by QEMU and only used by QEMU internal.  
> > drop it
> >  
> >> +1) Read FIT
> >> +   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
> >> +   function (private QEMU function)  
> > not necessary, drop it. Maybe extend UUID description in
> > "Input parameters:" section
> >  
> 
> okay.
> 
> >  
> >> +   _FIT method uses Read_FIT function to fetch NFIT structures blob from  
> > s/Read_FIT function/_DSM method/  
> 
> okay.
> 
> >  
> >> +   QEMU in 1 page sized increments which are then concatenated and 
> >> returned
> >> +   as _FIT method result.
> >> +
> >> +   Input parameters:
> >> +   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> >> +   Arg1 – Revision ID (set to 1)
> >> +   Arg2 - Function Index, 0x1
> >> +   Arg3 - A package containing a

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Igor Mammedov 
On Thu, 3 Nov 2016 18:08:04 +0800
Xiao Guangrong  wrote:

> On 11/03/2016 05:53 PM, Stefan Hajnoczi wrote:
> > On Thu, Nov 03, 2016 at 11:51:29AM +0800, Xiao Guangrong wrote:  
> >> @@ -504,6 +521,77 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, 
> >> hwaddr dsm_mem_addr)
> >>  cpu_physical_memory_write(dsm_mem_addr, , sizeof(out));
> >>  }
> >>
> >> +#define NVDIMM_DSM_RET_STATUS_SUCCESS0 /* Success */
> >> +#define NVDIMM_DSM_RET_STATUS_UNSUPPORT  1 /* Not Supported */
> >> +#define NVDIMM_DSM_RET_STATUS_INVALID3 /* Invalid Input 
> >> Parameters */  
> >
> > Not worth changing but please make each logical change a separate patch
> > in the future.  This patch is cluttered with NVDIMM_DSM_RET_STATUS_
> > constant renaming.  It's easier to review, bisect, and backport when
> > structured as separate patches.
> >  
> 
> Yes, indeed. Thanks for your suggestion, will pay more attention. :P
just do renaming first as separate patch
and then hotplug patches on top

> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Xiao Guangrong 



On 11/03/2016 07:58 PM, Igor Mammedov wrote:

On Thu,  3 Nov 2016 11:51:29 +0800
Xiao Guangrong  wrote:


_FIT is required for hotplug support, guest will inquire the updated
device info from it if a hotplug event is received

s/_FIT/_FIT method/

the same applies to subj. line


Okay.





As FIT buffer is not completely mapped into guest address space, so a
new function, Read FIT whose UUID is UUID
648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x1, function index
is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
is concatenated before _FIT return


Commit message hard to read/parse, it might be better if I'd use simple
short sentences.


Okay, will change it to:

As FIT buffer is not completely mapped into guest address space, Read_FIT
method is introduced to read NFIT structures blob from QEMU, The buffer
is concatenated before _FIT return





Refer to docs/specs/acpi-nvdimm.txt for detailed design

Signed-off-by: Xiao Guangrong 
---
 docs/specs/acpi_nvdimm.txt |  63 -
 hw/acpi/nvdimm.c   | 225 ++---
 2 files changed, 271 insertions(+), 17 deletions(-)

diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
index 0fdd251..364e832 100644
--- a/docs/specs/acpi_nvdimm.txt
+++ b/docs/specs/acpi_nvdimm.txt
@@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
NVDIMM Firmware Interface Table (NFIT).

-QEMU NVDIMM Implemention
-
+QEMU NVDIMM Implementation
+==
 QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
 for NVDIMM ACPI.

@@ -82,6 +82,16 @@ Memory:
ACPI writes _DSM Input Data (based on the offset in the page):
[0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
 Root device.
+
+The handle is completely QEMU internal thing, the values in
+range [0, 0x] indicate nvdimm device (O means nvdimm

[1, 0x] indicate nvdimm device
and move 0 to reserved section


Okay.




+root device named NVDR), other values are reserved by other

s/by/for/


okay.




+purpose.

s/purpose/purposes/


okay.




+Current reserved handle:

s/Current reserved handle/Reserved handles/


+0x1 is reserved for QEMU internal DSM function called on
+the root device.

description is too obscure, I wonder if it could be more specific


I would like to make these reserved values more generic in order to
support more QEMU reserved _DSM methods in the further. So, i planed:
UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is dedicated for QEMU reserved
methods. Handle 0x1 indicates the method is for the root device.
0x10001 ~ 0x1 indicates the method is for the nvdimm devices.

As currently we do not have reserved method on nvdimm device, so i
only document 0x1000 for the root device.





[0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
[0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
[0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
@@ -127,6 +137,49 @@ _DSM process diagram:
  | result from the page |  |  |
  +--+  +--+

- _FIT implementation
- ---
- TODO (will fill it when nvdimm hotplug is introduced)
+QEMU internal use only _DSM function
+
+There is the function introduced by QEMU and only used by QEMU internal.

drop it


+1) Read FIT
+   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
+   function (private QEMU function)

not necessary, drop it. Maybe extend UUID description in
"Input parameters:" section



okay.




+   _FIT method uses Read_FIT function to fetch NFIT structures blob from

s/Read_FIT function/_DSM method/


okay.




+   QEMU in 1 page sized increments which are then concatenated and returned
+   as _FIT method result.
+
+   Input parameters:
+   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
+   Arg1 – Revision ID (set to 1)
+   Arg2 - Function Index, 0x1
+   Arg3 - A package containing a buffer whose layout is as follows:
+
+   +--+++---+
+   |  Field   | Length | Offset | Description   |
+   +--+++---+
+   | offset   |   4|   0| offset in QEMU's NFIT structures blob to  |
+   |  ||| read from |
+   +--+++---+
+
+   Output:
+   +--+++---+
+   |  Field   | Length | Offset | Description

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Igor Mammedov 
On Thu,  3 Nov 2016 11:51:29 +0800
Xiao Guangrong  wrote:

> _FIT is required for hotplug support, guest will inquire the updated
> device info from it if a hotplug event is received
s/_FIT/_FIT method/

the same applies to subj. line

> 
> As FIT buffer is not completely mapped into guest address space, so a
> new function, Read FIT whose UUID is UUID
> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x1, function index
> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> is concatenated before _FIT return

Commit message hard to read/parse, it might be better if I'd use simple
short sentences.


> Refer to docs/specs/acpi-nvdimm.txt for detailed design
> 
> Signed-off-by: Xiao Guangrong 
> ---
>  docs/specs/acpi_nvdimm.txt |  63 -
>  hw/acpi/nvdimm.c   | 225 
> ++---
>  2 files changed, 271 insertions(+), 17 deletions(-)
> 
> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> index 0fdd251..364e832 100644
> --- a/docs/specs/acpi_nvdimm.txt
> +++ b/docs/specs/acpi_nvdimm.txt
> @@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
> The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
> NVDIMM Firmware Interface Table (NFIT).
>  
> -QEMU NVDIMM Implemention
> -
> +QEMU NVDIMM Implementation
> +==
>  QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
>  for NVDIMM ACPI.
>  
> @@ -82,6 +82,16 @@ Memory:
> ACPI writes _DSM Input Data (based on the offset in the page):
> [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
>  Root device.
> +
> +The handle is completely QEMU internal thing, the values in
> +range [0, 0x] indicate nvdimm device (O means nvdimm
[1, 0x] indicate nvdimm device
and move 0 to reserved section

> +root device named NVDR), other values are reserved by other
s/by/for/

> +purpose.
s/purpose/purposes/

> +Current reserved handle:
s/Current reserved handle/Reserved handles/

> +0x1 is reserved for QEMU internal DSM function called on
> +the root device.
description is too obscure, I wonder if it could be more specific


> [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
> [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
> [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
> @@ -127,6 +137,49 @@ _DSM process diagram:
>   | result from the page |  |  |
>   +--+  +--+
>  
> - _FIT implementation
> - ---
> - TODO (will fill it when nvdimm hotplug is introduced)
> +QEMU internal use only _DSM function
> +
> +There is the function introduced by QEMU and only used by QEMU internal.
drop it

> +1) Read FIT
> +   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
> +   function (private QEMU function)
not necessary, drop it. Maybe extend UUID description in
"Input parameters:" section


> +   _FIT method uses Read_FIT function to fetch NFIT structures blob from
s/Read_FIT function/_DSM method/

> +   QEMU in 1 page sized increments which are then concatenated and returned
> +   as _FIT method result.
> +
> +   Input parameters:
> +   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> +   Arg1 – Revision ID (set to 1)
> +   Arg2 - Function Index, 0x1
> +   Arg3 - A package containing a buffer whose layout is as follows:
> +
> +   +--+++---+
> +   |  Field   | Length | Offset | Description   |
> +   +--+++---+
> +   | offset   |   4|   0| offset in QEMU's NFIT structures blob to  |
> +   |  ||| read from |
> +   +--+++---+
> +
> +   Output:
> +   +--+++---+
> +   |  Field   | Length | Offset | Description   |
> +   +--+++---+
> +   |  ||| return status codes   |
> +   |  ||| 0x100 - error caused by NFIT update while |
> +   | status   |   4|   0| read by _FIT wasn't completed, other  |
> +   |  ||| codes follow Chapter 3 in DSM Spec Rev1   |
> +   +--+++---+
> +   | length   |   4|   4| The fit size  
> |   
> +

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Xiao Guangrong 



On 11/03/2016 05:53 PM, Stefan Hajnoczi wrote:

On Thu, Nov 03, 2016 at 11:51:29AM +0800, Xiao Guangrong wrote:

@@ -504,6 +521,77 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr 
dsm_mem_addr)
 cpu_physical_memory_write(dsm_mem_addr, , sizeof(out));
 }

+#define NVDIMM_DSM_RET_STATUS_SUCCESS0 /* Success */
+#define NVDIMM_DSM_RET_STATUS_UNSUPPORT  1 /* Not Supported */
+#define NVDIMM_DSM_RET_STATUS_INVALID3 /* Invalid Input Parameters */


Not worth changing but please make each logical change a separate patch
in the future.  This patch is cluttered with NVDIMM_DSM_RET_STATUS_
constant renaming.  It's easier to review, bisect, and backport when
structured as separate patches.



Yes, indeed. Thanks for your suggestion, will pay more attention. :P

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-03 Thread Stefan Hajnoczi 
On Thu, Nov 03, 2016 at 11:51:29AM +0800, Xiao Guangrong wrote:
> @@ -504,6 +521,77 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr 
> dsm_mem_addr)
>  cpu_physical_memory_write(dsm_mem_addr, , sizeof(out));
>  }
>  
> +#define NVDIMM_DSM_RET_STATUS_SUCCESS0 /* Success */
> +#define NVDIMM_DSM_RET_STATUS_UNSUPPORT  1 /* Not Supported */
> +#define NVDIMM_DSM_RET_STATUS_INVALID3 /* Invalid Input Parameters */

Not worth changing but please make each logical change a separate patch
in the future.  This patch is cluttered with NVDIMM_DSM_RET_STATUS_
constant renaming.  It's easier to review, bisect, and backport when
structured as separate patches.


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

2016-11-02 Thread Xiao Guangrong 
_FIT is required for hotplug support, guest will inquire the updated
device info from it if a hotplug event is received

As FIT buffer is not completely mapped into guest address space, so a
new function, Read FIT whose UUID is UUID
648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x1, function index
is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
is concatenated before _FIT return

Refer to docs/specs/acpi-nvdimm.txt for detailed design

Signed-off-by: Xiao Guangrong 
---
 docs/specs/acpi_nvdimm.txt |  63 -
 hw/acpi/nvdimm.c   | 225 ++---
 2 files changed, 271 insertions(+), 17 deletions(-)

diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
index 0fdd251..364e832 100644
--- a/docs/specs/acpi_nvdimm.txt
+++ b/docs/specs/acpi_nvdimm.txt
@@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
NVDIMM Firmware Interface Table (NFIT).
 
-QEMU NVDIMM Implemention
-
+QEMU NVDIMM Implementation
+==
 QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
 for NVDIMM ACPI.
 
@@ -82,6 +82,16 @@ Memory:
ACPI writes _DSM Input Data (based on the offset in the page):
[0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
 Root device.
+
+The handle is completely QEMU internal thing, the values in
+range [0, 0x] indicate nvdimm device (O means nvdimm
+root device named NVDR), other values are reserved by other
+purpose.
+
+Current reserved handle:
+0x1 is reserved for QEMU internal DSM function called on
+the root device.
+
[0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
[0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
[0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
@@ -127,6 +137,49 @@ _DSM process diagram:
  | result from the page |  |  |
  +--+  +--+
 
- _FIT implementation
- ---
- TODO (will fill it when nvdimm hotplug is introduced)
+QEMU internal use only _DSM function
+
+There is the function introduced by QEMU and only used by QEMU internal.
+
+1) Read FIT
+   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
+   function (private QEMU function)
+
+   _FIT method uses Read_FIT function to fetch NFIT structures blob from
+   QEMU in 1 page sized increments which are then concatenated and returned
+   as _FIT method result.
+
+   Input parameters:
+   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
+   Arg1 – Revision ID (set to 1)
+   Arg2 - Function Index, 0x1
+   Arg3 - A package containing a buffer whose layout is as follows:
+
+   +--+++---+
+   |  Field   | Length | Offset | Description   |
+   +--+++---+
+   | offset   |   4|   0| offset in QEMU's NFIT structures blob to  |
+   |  ||| read from |
+   +--+++---+
+
+   Output:
+   +--+++---+
+   |  Field   | Length | Offset | Description   |
+   +--+++---+
+   |  ||| return status codes   |
+   |  ||| 0x100 - error caused by NFIT update while |
+   | status   |   4|   0| read by _FIT wasn't completed, other  |
+   |  ||| codes follow Chapter 3 in DSM Spec Rev1   |
+   +--+++---+
+   | length   |   4|   4| The fit size  |  
 
+   +--+-+---+
+   | fit data | Varies |   8| FIT data, its size is indicated by length |
+   |  ||| filed above   |
+   +--+++---+
+
+   The FIT offset is maintained by the OSPM itself, current offset plus
+   the length returned by the function is the next offset we should read.
+   When all the FIT data has been read out, zero length is returned.
+
+   If it returns 0x100, OSPM should restart to read FIT (read from offset 0
+   again).
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 9fee077..593ac0d 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -484,6 +484,23 @@ typedef