[Qemu-devel] [qemu PATCH 1/5] gitignore: ignore generated qapi job files

[Qemu-devel] [qemu PATCH 1/5] gitignore: ignore generated qapi job files

[Ross Zwisler]

With a fully built QEMU I currently see the following with "git status":

  Untracked files:
    (use "git add <file>..." to include in what will be committed)

  	qapi/qapi-commands-job.c
  	qapi/qapi-commands-job.h
  	qapi/qapi-events-job.c
  	qapi/qapi-events-job.h
  	qapi/qapi-types-job.c
  	qapi/qapi-types-job.h
  	qapi/qapi-visit-job.c
  	qapi/qapi-visit-job.h

These are all generated files.  Ignore them.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Fixes: commit bf42508f24ee ("job: Introduce qapi/job.json")
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 81e1f2fb0f..2980090f0a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -100,6 +100,7 @@
 /qapi/qapi-visit-ui.[ch]
 /qapi/qapi-visit.[ch]
 /qapi/qapi-doc.texi
+/qapi/qapi-*-job.[ch]
 /qemu-doc.html
 /qemu-doc.info
 /qemu-doc.txt
-- 
2.14.4

[Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Ross Zwisler]

Currently if "make check" detects a mismatch in the ASL generated during
testing, we print an error such as:

  acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl,
  aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl,
  aml:tests/acpi-test-data/q35/SSDT.dimmpxm].

but the testing still exits with good shell status.  This is wrong, and
makes bisecting such a failure difficult.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 tests/bios-tables-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 256d463cb8..9b5db1ee8f 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -469,6 +469,7 @@ static void test_acpi_asl(test_data *data)
                     }
                 }
           }
+          g_test_fail();
         }
         g_string_free(asl, true);
         g_string_free(exp_asl, true);
-- 
2.14.4

Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Michael S. Tsirkin]

On Thu, Jun 07, 2018 at 04:31:08PM -0600, Ross Zwisler wrote:
> Currently if "make check" detects a mismatch in the ASL generated during
> testing, we print an error such as:
> 
>   acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl,
>   aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl,
>   aml:tests/acpi-test-data/q35/SSDT.dimmpxm].
> 
> but the testing still exits with good shell status.  This is wrong, and
> makes bisecting such a failure difficult.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>

Failing would also mean that any change must update the expected files
at the same time.  And that in turn is problematic because expected
files are binary and can't be merged.

In other words the way we devel ACPI right now means that bisect will
periodically produce a diff, it's not an error.


> ---
>  tests/bios-tables-test.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 256d463cb8..9b5db1ee8f 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -469,6 +469,7 @@ static void test_acpi_asl(test_data *data)
>                      }
>                  }
>            }
> +          g_test_fail();
>          }
>          g_string_free(asl, true);
>          g_string_free(exp_asl, true);
> -- 
> 2.14.4

Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Thomas Huth]

On 08.06.2018 01:09, Michael S. Tsirkin wrote:
> On Thu, Jun 07, 2018 at 04:31:08PM -0600, Ross Zwisler wrote:
>> Currently if "make check" detects a mismatch in the ASL generated during
>> testing, we print an error such as:
>>
>>   acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl,
>>   aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl,
>>   aml:tests/acpi-test-data/q35/SSDT.dimmpxm].
>>
>> but the testing still exits with good shell status.  This is wrong, and
>> makes bisecting such a failure difficult.
>>
>> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> 
> Failing would also mean that any change must update the expected files
> at the same time.  And that in turn is problematic because expected
> files are binary and can't be merged.
> 
> In other words the way we devel ACPI right now means that bisect will
> periodically produce a diff, it's not an error.

But apparently the current way also allows that real bug go unnoticed
for a while, until somebody accidentially spots the warning in the
output of "make check". Wouldn't it be better to fail at CI time
already? If a merge of the file is required, you can still resolve that
manually (i.e. by rebasing one of the pull requests).

 Thomas

Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Ross Zwisler]

On Fri, Jun 08, 2018 at 07:17:51AM +0200, Thomas Huth wrote:
> On 08.06.2018 01:09, Michael S. Tsirkin wrote:
> > On Thu, Jun 07, 2018 at 04:31:08PM -0600, Ross Zwisler wrote:
> >> Currently if "make check" detects a mismatch in the ASL generated during
> >> testing, we print an error such as:
> >>
> >>   acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl,
> >>   aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl,
> >>   aml:tests/acpi-test-data/q35/SSDT.dimmpxm].
> >>
> >> but the testing still exits with good shell status.  This is wrong, and
> >> makes bisecting such a failure difficult.
> >>
> >> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > 
> > Failing would also mean that any change must update the expected files
> > at the same time.  And that in turn is problematic because expected
> > files are binary and can't be merged.
> > 
> > In other words the way we devel ACPI right now means that bisect will
> > periodically produce a diff, it's not an error.
> 
> But apparently the current way also allows that real bug go unnoticed
> for a while, until somebody accidentially spots the warning in the
> output of "make check". Wouldn't it be better to fail at CI time
> already? If a merge of the file is required, you can still resolve that
> manually (i.e. by rebasing one of the pull requests).

I share this point of view.  The unit tests only add value if we keep them up
to date and passing as we modify the source.  The ACPI tables in this case
were broken in an innocuous way and just needed to be updated to match again,
but it means that the tests for them are now basically turned off.  Someone
else could come along and break the ACPI table in a real and harmful way, and
nobody would notice because the and result would still just be an ACPI table
mismatch that is non-fatal and ignored.

Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Michael S. Tsirkin]

On Fri, Jun 08, 2018 at 09:34:02AM -0600, Ross Zwisler wrote:
> On Fri, Jun 08, 2018 at 07:17:51AM +0200, Thomas Huth wrote:
> > On 08.06.2018 01:09, Michael S. Tsirkin wrote:
> > > On Thu, Jun 07, 2018 at 04:31:08PM -0600, Ross Zwisler wrote:
> > >> Currently if "make check" detects a mismatch in the ASL generated during
> > >> testing, we print an error such as:
> > >>
> > >>   acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl,
> > >>   aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl,
> > >>   aml:tests/acpi-test-data/q35/SSDT.dimmpxm].
> > >>
> > >> but the testing still exits with good shell status.  This is wrong, and
> > >> makes bisecting such a failure difficult.
> > >>
> > >> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > 
> > > Failing would also mean that any change must update the expected files
> > > at the same time.  And that in turn is problematic because expected
> > > files are binary and can't be merged.
> > > 
> > > In other words the way we devel ACPI right now means that bisect will
> > > periodically produce a diff, it's not an error.
> > 
> > But apparently the current way also allows that real bug go unnoticed
> > for a while, until somebody accidentially spots the warning in the
> > output of "make check". Wouldn't it be better to fail at CI time
> > already? If a merge of the file is required, you can still resolve that
> > manually (i.e. by rebasing one of the pull requests).
> 
> I share this point of view.  The unit tests only add value if we keep them up
> to date and passing as we modify the source.  The ACPI tables in this case
> were broken in an innocuous way and just needed to be updated to match again,
> but it means that the tests for them are now basically turned off.

The expected value tests are a debugging aid. They do not catch bugs and
aren't designed to. In particular the comparisons do not even run if
IASL isn't installed.


>  Someone
> else could come along and break the ACPI table in a real and harmful way, and
> nobody would notice because the and result would still just be an ACPI table
> mismatch that is non-fatal and ignored.

There are tests with windows and linux guests that will catch it.  It's
just not something we can handle reasonably in a unit test.

-- 
MST

Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Peter Maydell]

On 8 June 2018 at 16:59, Michael S. Tsirkin <mst@redhat.com> wrote:
> The expected value tests are a debugging aid. They do not catch bugs and
> aren't designed to. In particular the comparisons do not even run if
> IASL isn't installed.

If they're not actually tests to catch bugs, maybe we shouldn't
be running them in "make check" ?

thanks
-- PMM

Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Michael S. Tsirkin]

On Fri, Jun 08, 2018 at 05:14:09PM +0100, Peter Maydell wrote:
> On 8 June 2018 at 16:59, Michael S. Tsirkin <mst@redhat.com> wrote:
> > The expected value tests are a debugging aid. They do not catch bugs and
> > aren't designed to. In particular the comparisons do not even run if
> > IASL isn't installed.
> 
> If they're not actually tests to catch bugs, maybe we shouldn't
> be running them in "make check" ?
> 
> thanks
> -- PMM

We are running tests to do basic things like verifying the
checksum of the tables. Failure on these causes make check to fail.

As long as we have the tables anyway, and assuming IASL is installed
(which most people who do not work on ACPI would not have) we compare to
the expected set, that is often helpful for debugging.

-- 
MST

Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Michael S. Tsirkin]

On Fri, Jun 08, 2018 at 07:17:51AM +0200, Thomas Huth wrote:
> On 08.06.2018 01:09, Michael S. Tsirkin wrote:
> > On Thu, Jun 07, 2018 at 04:31:08PM -0600, Ross Zwisler wrote:
> >> Currently if "make check" detects a mismatch in the ASL generated during
> >> testing, we print an error such as:
> >>
> >>   acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl,
> >>   aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl,
> >>   aml:tests/acpi-test-data/q35/SSDT.dimmpxm].
> >>
> >> but the testing still exits with good shell status.  This is wrong, and
> >> makes bisecting such a failure difficult.
> >>
> >> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > 
> > Failing would also mean that any change must update the expected files
> > at the same time.  And that in turn is problematic because expected
> > files are binary and can't be merged.
> > 
> > In other words the way we devel ACPI right now means that bisect will
> > periodically produce a diff, it's not an error.
> 
> But apparently the current way also allows that real bug go unnoticed
> for a while, until somebody accidentially spots the warning in the
> output of "make check". Wouldn't it be better to fail at CI time
> already? If a merge of the file is required, you can still resolve that
> manually (i.e. by rebasing one of the pull requests).
> 
>  Thomas

Pull requests are somewhat different, they are usually tested for lack
of warnings. This change didn't arrive as a result of a pull request
maybe that's why it slipped through the cracks. Peter?

Maybe we need a "pedantic" flag to fail on any warnings, or just catch
output to stderr.

-- 
MST

Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Peter Maydell]

On 8 June 2018 at 17:03, Michael S. Tsirkin <mst@redhat.com> wrote:
> Pull requests are somewhat different, they are usually tested for lack
> of warnings. This change didn't arrive as a result of a pull request
> maybe that's why it slipped through the cracks. Peter?
>
> Maybe we need a "pedantic" flag to fail on any warnings, or just catch
> output to stderr.

If there's a situation that shouldn't exist in the tree (ie
a bug), then make check should catch it, and result in a
failure, not just printing random stuff to stderr. Otherwise
I'm not going to notice it, whether I'm applying a pull request
or an individual patch.

thanks
-- PMM

Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Michael S. Tsirkin]

On Fri, Jun 08, 2018 at 05:16:30PM +0100, Peter Maydell wrote:
> On 8 June 2018 at 17:03, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Pull requests are somewhat different, they are usually tested for lack
> > of warnings. This change didn't arrive as a result of a pull request
> > maybe that's why it slipped through the cracks. Peter?
> >
> > Maybe we need a "pedantic" flag to fail on any warnings, or just catch
> > output to stderr.
> 
> If there's a situation that shouldn't exist in the tree (ie
> a bug), then make check should catch it, and result in a
> failure, not just printing random stuff to stderr. Otherwise
> I'm not going to notice it, whether I'm applying a pull request
> or an individual patch.
> 
> thanks
> -- PMM

It's ok if it happens, but it just makes debugging and reviewing
ACPI patches a little bit harder until it's fixed.

-- 
MST

Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Thomas Huth]

On 08.06.2018 18:24, Michael S. Tsirkin wrote:
> On Fri, Jun 08, 2018 at 05:16:30PM +0100, Peter Maydell wrote:
>> On 8 June 2018 at 17:03, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> Pull requests are somewhat different, they are usually tested for lack
>>> of warnings. This change didn't arrive as a result of a pull request
>>> maybe that's why it slipped through the cracks. Peter?
>>>
>>> Maybe we need a "pedantic" flag to fail on any warnings, or just catch
>>> output to stderr.
>>
>> If there's a situation that shouldn't exist in the tree (ie
>> a bug), then make check should catch it, and result in a
>> failure, not just printing random stuff to stderr. Otherwise
>> I'm not going to notice it, whether I'm applying a pull request
>> or an individual patch.
>>
>> thanks
>> -- PMM
> 
> It's ok if it happens, but it just makes debugging and reviewing
> ACPI patches a little bit harder until it's fixed.

It's maybe ok for *you*, but this certainly confuses everybody else. If
I want to check my patches and suddenly some strange warnings are
popping up, I first assume that there is something wrong in my patches
(since I assume that the git repository is clean by default). So I've
got to waste my time debugging issues that are not my own. Thanks for
that :-/

 Thomas

Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Michael S. Tsirkin]

On Fri, Jun 08, 2018 at 07:23:06PM +0200, Thomas Huth wrote:
> On 08.06.2018 18:24, Michael S. Tsirkin wrote:
> > On Fri, Jun 08, 2018 at 05:16:30PM +0100, Peter Maydell wrote:
> >> On 8 June 2018 at 17:03, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>> Pull requests are somewhat different, they are usually tested for lack
> >>> of warnings. This change didn't arrive as a result of a pull request
> >>> maybe that's why it slipped through the cracks. Peter?
> >>>
> >>> Maybe we need a "pedantic" flag to fail on any warnings, or just catch
> >>> output to stderr.
> >>
> >> If there's a situation that shouldn't exist in the tree (ie
> >> a bug), then make check should catch it, and result in a
> >> failure, not just printing random stuff to stderr. Otherwise
> >> I'm not going to notice it, whether I'm applying a pull request
> >> or an individual patch.
> >>
> >> thanks
> >> -- PMM
> > 
> > It's ok if it happens, but it just makes debugging and reviewing
> > ACPI patches a little bit harder until it's fixed.
> 
> It's maybe ok for *you*, but this certainly confuses everybody else. If
> I want to check my patches and suddenly some strange warnings are
> popping up, I first assume that there is something wrong in my patches
> (since I assume that the git repository is clean by default). So I've
> got to waste my time debugging issues that are not my own. Thanks for
> that :-/
> 
>  Thomas

Right so normally these do not pop out at all as I fix expected
with a patch on top.

-- 
MST

Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Thomas Huth]

On 08.06.2018 20:41, Michael S. Tsirkin wrote:
> On Fri, Jun 08, 2018 at 07:23:06PM +0200, Thomas Huth wrote:
>> On 08.06.2018 18:24, Michael S. Tsirkin wrote:
>>> On Fri, Jun 08, 2018 at 05:16:30PM +0100, Peter Maydell wrote:
[...]
>>>> If there's a situation that shouldn't exist in the tree (ie
>>>> a bug), then make check should catch it, and result in a
>>>> failure, not just printing random stuff to stderr. Otherwise
>>>> I'm not going to notice it, whether I'm applying a pull request
>>>> or an individual patch.
>>>>
>>> It's ok if it happens, but it just makes debugging and reviewing
>>> ACPI patches a little bit harder until it's fixed.
>>
>> It's maybe ok for *you*, but this certainly confuses everybody else. If
>> I want to check my patches and suddenly some strange warnings are
>> popping up, I first assume that there is something wrong in my patches
>> (since I assume that the git repository is clean by default). So I've
>> got to waste my time debugging issues that are not my own. Thanks for
>> that :-/
> 
> Right so normally these do not pop out at all as I fix expected
> with a patch on top.

Apparently other people can also introduce changes that cause these
warnings. Anyway, I now "fixed" it here by uninstalling iasl, so never mind.

 Thomas

[Qemu-devel] [qemu PATCH 3/5] hw/i386: Update SSDT table used by "make check"

[Ross Zwisler]

This commit:

commit aa78a16d8645 ("hw/i386: Rename 2.13 machine types to 3.0")

updated the name used to create the q35 machine, which in turn changed the
SSDT table which is generated when we run "make check":

  acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl,
  aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl,
  aml:tests/acpi-test-data/q35/SSDT.dimmpxm].

Here's the only difference, aside from the checksum:

  <     Name (MEMA, 0x07FFF000)
  ---
  >     Name (MEMA, 0x07FFE000)

Update the binary table that we compare against so it reflects this name
change.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Fixes: commit aa78a16d8645 ("hw/i386: Rename 2.13 machine types to 3.0")
---
 tests/acpi-test-data/q35/SSDT.dimmpxm | Bin 685 -> 685 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)

diff --git a/tests/acpi-test-data/q35/SSDT.dimmpxm b/tests/acpi-test-data/q35/SSDT.dimmpxm
index 8ba0e67cb72daa81a65da4906d37a5e0f4af1fd4..2d5b721bcf9c398feb6d005761f898015042e8a4 100644
GIT binary patch
delta 23
fcmZ3>x|WqIIM^j*EfWI+qr*n71x(Bz{<8xBPCEwk

delta 23
fcmZ3>x|WqIIM^j*EfWI+W57nP1x(Bj{<8xBPMZev

-- 
2.14.4

Re: [Qemu-devel] [qemu PATCH 3/5] hw/i386: Update SSDT table used by "make check"

[Michael S. Tsirkin]

On Thu, Jun 07, 2018 at 04:31:09PM -0600, Ross Zwisler wrote:
> This commit:
> 
> commit aa78a16d8645 ("hw/i386: Rename 2.13 machine types to 3.0")
> 
> updated the name used to create the q35 machine, which in turn changed the
> SSDT table which is generated when we run "make check":
> 
>   acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl,
>   aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl,
>   aml:tests/acpi-test-data/q35/SSDT.dimmpxm].
> 
> Here's the only difference, aside from the checksum:
> 
>   <     Name (MEMA, 0x07FFF000)
>   ---
>   >     Name (MEMA, 0x07FFE000)

Weird. How come the phys address changes just because of machine name?

> 
> Update the binary table that we compare against so it reflects this name
> change.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Fixes: commit aa78a16d8645 ("hw/i386: Rename 2.13 machine types to 3.0")
> ---
>  tests/acpi-test-data/q35/SSDT.dimmpxm | Bin 685 -> 685 bytes
>  1 file changed, 0 insertions(+), 0 deletions(-)
> 
> diff --git a/tests/acpi-test-data/q35/SSDT.dimmpxm b/tests/acpi-test-data/q35/SSDT.dimmpxm
> index 8ba0e67cb72daa81a65da4906d37a5e0f4af1fd4..2d5b721bcf9c398feb6d005761f898015042e8a4 100644
> GIT binary patch
> delta 23
> fcmZ3>x|WqIIM^j*EfWI+qr*n71x(Bz{<8xBPCEwk
> 
> delta 23
> fcmZ3>x|WqIIM^j*EfWI+W57nP1x(Bj{<8xBPMZev
> 
> -- 
> 2.14.4

Re: [Qemu-devel] [qemu PATCH 3/5] hw/i386: Update SSDT table used by "make check"

[Eric Blake]

On 06/07/2018 06:14 PM, Michael S. Tsirkin wrote:
> On Thu, Jun 07, 2018 at 04:31:09PM -0600, Ross Zwisler wrote:
>> This commit:
>>
>> commit aa78a16d8645 ("hw/i386: Rename 2.13 machine types to 3.0")
>>
>> updated the name used to create the q35 machine, which in turn changed the
>> SSDT table which is generated when we run "make check":
>>
>>    acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl,
>>    aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl,
>>    aml:tests/acpi-test-data/q35/SSDT.dimmpxm].
>>
>> Here's the only difference, aside from the checksum:
>>
>>    <     Name (MEMA, 0x07FFF000)
>>    ---
>>    >     Name (MEMA, 0x07FFE000)
> 
> Weird. How come the phys address changes just because of machine name?

"2.13" is a different length than "3.0"; depending on whatever other 
alignment coincidences or sharing of similar substrings are in place, 
this obviously shuffled enough data that the one byte change then 
reflects into an entire page boundary difference.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [qemu PATCH 3/5] hw/i386: Update SSDT table used by "make check"

[Michael S. Tsirkin]

On Fri, Jun 08, 2018 at 09:24:49AM -0500, Eric Blake wrote:
> On 06/07/2018 06:14 PM, Michael S. Tsirkin wrote:
> > On Thu, Jun 07, 2018 at 04:31:09PM -0600, Ross Zwisler wrote:
> > > This commit:
> > > 
> > > commit aa78a16d8645 ("hw/i386: Rename 2.13 machine types to 3.0")
> > > 
> > > updated the name used to create the q35 machine, which in turn changed the
> > > SSDT table which is generated when we run "make check":
> > > 
> > >    acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl,
> > >    aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl,
> > >    aml:tests/acpi-test-data/q35/SSDT.dimmpxm].
> > > 
> > > Here's the only difference, aside from the checksum:
> > > 
> > >    <     Name (MEMA, 0x07FFF000)
> > >    ---
> > >    >     Name (MEMA, 0x07FFE000)
> > 
> > Weird. How come the phys address changes just because of machine name?
> 
> "2.13" is a different length than "3.0"; depending on whatever other
> alignment coincidences or sharing of similar substrings are in place, this
> obviously shuffled enough data that the one byte change then reflects into
> an entire page boundary difference.

I don't think we expose the version number to guests though - do we?

> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [qemu PATCH 3/5] hw/i386: Update SSDT table used by "make check"

[Thomas Huth]

On 08.06.2018 00:31, Ross Zwisler wrote:
> This commit:
> 
> commit aa78a16d8645 ("hw/i386: Rename 2.13 machine types to 3.0")
> 
> updated the name used to create the q35 machine, which in turn changed the
> SSDT table which is generated when we run "make check":
> 
>   acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl,
>   aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl,
>   aml:tests/acpi-test-data/q35/SSDT.dimmpxm].
> 
> Here's the only difference, aside from the checksum:
> 
>   <     Name (MEMA, 0x07FFF000)
>   ---
>   >     Name (MEMA, 0x07FFE000)
> 
> Update the binary table that we compare against so it reflects this name
> change.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Fixes: commit aa78a16d8645 ("hw/i386: Rename 2.13 machine types to 3.0")
> ---
>  tests/acpi-test-data/q35/SSDT.dimmpxm | Bin 685 -> 685 bytes
>  1 file changed, 0 insertions(+), 0 deletions(-)
> 
> diff --git a/tests/acpi-test-data/q35/SSDT.dimmpxm b/tests/acpi-test-data/q35/SSDT.dimmpxm
> index 8ba0e67cb72daa81a65da4906d37a5e0f4af1fd4..2d5b721bcf9c398feb6d005761f898015042e8a4 100644
> GIT binary patch
> delta 23
> fcmZ3>x|WqIIM^j*EfWI+qr*n71x(Bz{<8xBPCEwk
> 
> delta 23
> fcmZ3>x|WqIIM^j*EfWI+W57nP1x(Bj{<8xBPMZev
> 

Thanks, that fixes the warning for me.

Tested-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [qemu PATCH 4/5] machine: fix some misspelled words

[Ross Zwisler]

Normally this might not be worth fixing, but several of these are strings
which are displayed to users.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 hw/core/machine.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 617e5f8d75..a21269fa39 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -609,7 +609,7 @@ static void machine_class_init(ObjectClass *oc, void *data)
         machine_get_igd_gfx_passthru, machine_set_igd_gfx_passthru,
         &error_abort);
     object_class_property_set_description(oc, "igd-passthru",
-        "Set on/off to enable/disable igd passthrou", &error_abort);
+        "Set on/off to enable/disable igd passthru", &error_abort);
 
     object_class_property_add_str(oc, "firmware",
         machine_get_firmware, machine_set_firmware,
@@ -633,7 +633,7 @@ static void machine_class_init(ObjectClass *oc, void *data)
         machine_get_memory_encryption, machine_set_memory_encryption,
         &error_abort);
     object_class_property_set_description(oc, "memory-encryption",
-        "Set memory encyption object to use", &error_abort);
+        "Set memory encryption object to use", &error_abort);
 }
 
 static void machine_class_base_init(ObjectClass *oc, void *data)
@@ -806,7 +806,7 @@ void machine_run_board_init(MachineState *machine)
         for (i = 0; machine_class->valid_cpu_types[i]; i++) {
             if (object_class_dynamic_cast(class,
                                           machine_class->valid_cpu_types[i])) {
-                /* The user specificed CPU is in the valid field, we are
+                /* The user specified CPU is in the valid field, we are
                  * good to go.
                  */
                 break;
-- 
2.14.4

Re: [Qemu-devel] [qemu PATCH 4/5] machine: fix some misspelled words

[Thomas Huth]

On 08.06.2018 00:31, Ross Zwisler wrote:
> Normally this might not be worth fixing, but several of these are strings
> which are displayed to users.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  hw/core/machine.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 617e5f8d75..a21269fa39 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -609,7 +609,7 @@ static void machine_class_init(ObjectClass *oc, void *data)
>          machine_get_igd_gfx_passthru, machine_set_igd_gfx_passthru,
>          &error_abort);
>      object_class_property_set_description(oc, "igd-passthru",
> -        "Set on/off to enable/disable igd passthrou", &error_abort);
> +        "Set on/off to enable/disable igd passthru", &error_abort);

Shouldn't that rather be "passthrough" instead?

 Thomas

Re: [Qemu-devel] [qemu PATCH 4/5] machine: fix some misspelled words

[Ross Zwisler]

On Fri, Jun 08, 2018 at 07:38:17AM +0200, Thomas Huth wrote:
> On 08.06.2018 00:31, Ross Zwisler wrote:
> > Normally this might not be worth fixing, but several of these are strings
> > which are displayed to users.
> > 
> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > ---
> >  hw/core/machine.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 617e5f8d75..a21269fa39 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -609,7 +609,7 @@ static void machine_class_init(ObjectClass *oc, void *data)
> >          machine_get_igd_gfx_passthru, machine_set_igd_gfx_passthru,
> >          &error_abort);
> >      object_class_property_set_description(oc, "igd-passthru",
> > -        "Set on/off to enable/disable igd passthrou", &error_abort);
> > +        "Set on/off to enable/disable igd passthru", &error_abort);
> 
> Shouldn't that rather be "passthrough" instead?

Either works, I think.  "thru" and "passthru" are short informal versions of
"through" and "passthrough", but both the long and short versions of both
words are used all over the QEMU source.  "passthrou" is clearly wrong.  If
the longer version is preferred in this case please feel free to fix up when
you apply.

Re: [Qemu-devel] [qemu PATCH 4/5] machine: fix some misspelled words

[Eric Blake]

On 06/08/2018 12:41 PM, Ross Zwisler wrote:

>>> -        "Set on/off to enable/disable igd passthrou", &error_abort);
>>> +        "Set on/off to enable/disable igd passthru", &error_abort);
>>
>> Shouldn't that rather be "passthrough" instead?
> 
> Either works, I think.  "thru" and "passthru" are short informal versions of
> "through" and "passthrough", but both the long and short versions of both
> words are used all over the QEMU source.  "passthrou" is clearly wrong.  If
> the longer version is preferred in this case please feel free to fix up when
> you apply.

"passthru" is fine as an abbreviation in source code (and gcc 
understands it).  But in English text presented to the end user, as is 
the case here, you should use the correct spelling "passthrough", and 
not an abbreviation.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-devel] [qemu PATCH 5/5] nvdimm: make persistence option symbolic

[Ross Zwisler]

Replace the "nvdimm-cap" option which took numeric arguments such as "2"
with a more user friendly "nvdimm-persistence" option which takes symbolic
arguments "cpu" or "mem-ctrl".

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
---
 docs/nvdimm.txt          | 31 ++++++++++++-------------------
 hw/acpi/nvdimm.c         |  4 ++--
 hw/i386/pc.c             | 35 +++++++++++++++++------------------
 include/hw/i386/pc.h     |  2 +-
 include/hw/mem/nvdimm.h  |  3 ++-
 tests/bios-tables-test.c |  2 +-
 6 files changed, 35 insertions(+), 42 deletions(-)

diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
index 8b48fb4633..24b443b655 100644
--- a/docs/nvdimm.txt
+++ b/docs/nvdimm.txt
@@ -154,29 +154,22 @@ guest software that this vNVDIMM device contains a region that cannot
 accept persistent writes. In result, for example, the guest Linux
 NVDIMM driver, marks such vNVDIMM device as read-only.
 
-Platform Capabilities
----------------------
+NVDIMM Persistence
+------------------
 
 ACPI 6.2 Errata A added support for a new Platform Capabilities Structure
 which allows the platform to communicate what features it supports related to
-NVDIMM data durability.  Users can provide a capabilities value to a guest via
-the optional "nvdimm-cap" machine command line option:
+NVDIMM data persistence.  Users can provide a persistence value to a guest via
+the optional "nvdimm-persistence" machine command line option:
 
-    -machine pc,accel=kvm,nvdimm,nvdimm-cap=2
+    -machine pc,accel=kvm,nvdimm,nvdimm-persistence=cpu
 
-This "nvdimm-cap" field is an integer, and is the combined value of the
-various capability bits defined in table 5-137 of the ACPI 6.2 Errata A spec.
+There are currently two valid values for this option:
 
-Here is a quick summary of the three bits that are defined as of that spec:
+"mem-ctrl" - The platform supports flushing dirty data from the memory
+             controller to the NVDIMMs in the event of power loss.
 
-Bit[0] - CPU Cache Flush to NVDIMM Durability on Power Loss Capable.
-Bit[1] - Memory Controller Flush to NVDIMM Durability on Power Loss Capable.
-         Note: If bit 0 is set to 1 then this bit shall be set to 1 as well.
-Bit[2] - Byte Addressable Persistent Memory Hardware Mirroring Capable.
-
-So, a "nvdimm-cap" value of 2 would mean that the platform supports Memory
-Controller Flush on Power Loss, a value of 3 would mean that the platform
-supports CPU Cache Flush and Memory Controller Flush on Power Loss, etc.
-
-For a complete list of the flags available and for more detailed descriptions,
-please consult the ACPI spec.
+"cpu"      - The platform supports flushing dirty data from the CPU cache to
+             the NVDIMMs in the event of power loss.  This implies that the
+             platform also supports flushing dirty data through the memory
+             controller on power loss.
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 87e4280c71..27eeb6609f 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -404,8 +404,8 @@ static GArray *nvdimm_build_device_structure(AcpiNVDIMMState *state)
     }
     g_slist_free(device_list);
 
-    if (state->capabilities) {
-        nvdimm_build_structure_caps(structures, state->capabilities);
+    if (state->persistence) {
+        nvdimm_build_structure_caps(structures, state->persistence);
     }
 
     return structures;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f3befe6721..5bba9dcf5a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2181,31 +2181,30 @@ static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
     pcms->acpi_nvdimm_state.is_enabled = value;
 }
 
-static void pc_machine_get_nvdimm_capabilities(Object *obj, Visitor *v,
-                                               const char *name, void *opaque,
-                                               Error **errp)
+static char *pc_machine_get_nvdimm_persistence(Object *obj, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
-    uint32_t value = pcms->acpi_nvdimm_state.capabilities;
 
-    visit_type_uint32(v, name, &value, errp);
+    return g_strdup(pcms->acpi_nvdimm_state.persistence_string);
 }
 
-static void pc_machine_set_nvdimm_capabilities(Object *obj, Visitor *v,
-                                               const char *name, void *opaque,
+static void pc_machine_set_nvdimm_persistence(Object *obj, const char *value,
                                                Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
-    Error *error = NULL;
-    uint32_t value;
-
-    visit_type_uint32(v, name, &value, &error);
-    if (error) {
-        error_propagate(errp, error);
-        return;
+    AcpiNVDIMMState *nvdimm_state = &pcms->acpi_nvdimm_state;
+
+    if (strcmp(value, "cpu") == 0)
+        nvdimm_state->persistence = 3;
+    else if (strcmp(value, "mem-ctrl") == 0)
+        nvdimm_state->persistence = 2;
+    else {
+        error_report("-machine nvdimm-persistence=%s: unsupported option", value);
+        exit(EXIT_FAILURE);
     }
 
-    pcms->acpi_nvdimm_state.capabilities = value;
+    g_free(nvdimm_state->persistence_string);
+    nvdimm_state->persistence_string = g_strdup(value);
 }
 
 static bool pc_machine_get_smbus(Object *obj, Error **errp)
@@ -2421,9 +2420,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
         pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
 
-    object_class_property_add(oc, PC_MACHINE_NVDIMM_CAP, "uint32",
-        pc_machine_get_nvdimm_capabilities,
-        pc_machine_set_nvdimm_capabilities, NULL, NULL, &error_abort);
+    object_class_property_add_str(oc, PC_MACHINE_NVDIMM_PERSIST,
+        pc_machine_get_nvdimm_persistence,
+        pc_machine_set_nvdimm_persistence, &error_abort);
 
     object_class_property_add_bool(oc, PC_MACHINE_SMBUS,
         pc_machine_get_smbus, pc_machine_set_smbus, &error_abort);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 04d1f8c6c3..fc8dedca12 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -76,7 +76,7 @@ struct PCMachineState {
 #define PC_MACHINE_VMPORT           "vmport"
 #define PC_MACHINE_SMM              "smm"
 #define PC_MACHINE_NVDIMM           "nvdimm"
-#define PC_MACHINE_NVDIMM_CAP       "nvdimm-cap"
+#define PC_MACHINE_NVDIMM_PERSIST   "nvdimm-persistence"
 #define PC_MACHINE_SMBUS            "smbus"
 #define PC_MACHINE_SATA             "sata"
 #define PC_MACHINE_PIT              "pit"
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 3c82751bab..9340631cfc 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -138,7 +138,8 @@ struct AcpiNVDIMMState {
     /*
      * Platform capabilities, section 5.2.25.9 of ACPI 6.2 Errata A
      */
-    int32_t capabilities;
+    int32_t persistence;
+    char    *persistence_string;
 };
 typedef struct AcpiNVDIMMState AcpiNVDIMMState;
 
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 9b5db1ee8f..b95d56aa4e 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -831,7 +831,7 @@ static void test_acpi_tcg_dimm_pxm(const char *machine)
     memset(&data, 0, sizeof(data));
     data.machine = machine;
     data.variant = ".dimmpxm";
-    test_acpi_one(" -machine nvdimm=on,nvdimm-cap=3"
+    test_acpi_one(" -machine nvdimm=on,nvdimm-persistence=cpu"
                   " -smp 4,sockets=4"
                   " -m 128M,slots=3,maxmem=1G"
                   " -numa node,mem=32M,nodeid=0"
-- 
2.14.4

Re: [Qemu-devel] [qemu PATCH 5/5] nvdimm: make persistence option symbolic

[Michael S. Tsirkin]

On Thu, Jun 07, 2018 at 04:31:11PM -0600, Ross Zwisler wrote:
> Replace the "nvdimm-cap" option which took numeric arguments such as "2"
> with a more user friendly "nvdimm-persistence" option which takes symbolic
> arguments "cpu" or "mem-ctrl".
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  docs/nvdimm.txt          | 31 ++++++++++++-------------------
>  hw/acpi/nvdimm.c         |  4 ++--
>  hw/i386/pc.c             | 35 +++++++++++++++++------------------
>  include/hw/i386/pc.h     |  2 +-
>  include/hw/mem/nvdimm.h  |  3 ++-
>  tests/bios-tables-test.c |  2 +-
>  6 files changed, 35 insertions(+), 42 deletions(-)
> 
> diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
> index 8b48fb4633..24b443b655 100644
> --- a/docs/nvdimm.txt
> +++ b/docs/nvdimm.txt
> @@ -154,29 +154,22 @@ guest software that this vNVDIMM device contains a region that cannot
>  accept persistent writes. In result, for example, the guest Linux
>  NVDIMM driver, marks such vNVDIMM device as read-only.
>  
> -Platform Capabilities
> ----------------------
> +NVDIMM Persistence
> +------------------
>  
>  ACPI 6.2 Errata A added support for a new Platform Capabilities Structure
>  which allows the platform to communicate what features it supports related to
> -NVDIMM data durability.  Users can provide a capabilities value to a guest via
> -the optional "nvdimm-cap" machine command line option:
> +NVDIMM data persistence.  Users can provide a persistence value to a guest via
> +the optional "nvdimm-persistence" machine command line option:
>  
> -    -machine pc,accel=kvm,nvdimm,nvdimm-cap=2
> +    -machine pc,accel=kvm,nvdimm,nvdimm-persistence=cpu
>  
> -This "nvdimm-cap" field is an integer, and is the combined value of the
> -various capability bits defined in table 5-137 of the ACPI 6.2 Errata A spec.
> +There are currently two valid values for this option:
>  
> -Here is a quick summary of the three bits that are defined as of that spec:
> +"mem-ctrl" - The platform supports flushing dirty data from the memory
> +             controller to the NVDIMMs in the event of power loss.
>  
> -Bit[0] - CPU Cache Flush to NVDIMM Durability on Power Loss Capable.
> -Bit[1] - Memory Controller Flush to NVDIMM Durability on Power Loss Capable.
> -         Note: If bit 0 is set to 1 then this bit shall be set to 1 as well.
> -Bit[2] - Byte Addressable Persistent Memory Hardware Mirroring Capable.
> -
> -So, a "nvdimm-cap" value of 2 would mean that the platform supports Memory
> -Controller Flush on Power Loss, a value of 3 would mean that the platform
> -supports CPU Cache Flush and Memory Controller Flush on Power Loss, etc.
> -
> -For a complete list of the flags available and for more detailed descriptions,
> -please consult the ACPI spec.
> +"cpu"      - The platform supports flushing dirty data from the CPU cache to
> +             the NVDIMMs in the event of power loss.  This implies that the
> +             platform also supports flushing dirty data through the memory
> +             controller on power loss.
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 87e4280c71..27eeb6609f 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -404,8 +404,8 @@ static GArray *nvdimm_build_device_structure(AcpiNVDIMMState *state)
>      }
>      g_slist_free(device_list);
>  
> -    if (state->capabilities) {
> -        nvdimm_build_structure_caps(structures, state->capabilities);
> +    if (state->persistence) {
> +        nvdimm_build_structure_caps(structures, state->persistence);
>      }
>  
>      return structures;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f3befe6721..5bba9dcf5a 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2181,31 +2181,30 @@ static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
>      pcms->acpi_nvdimm_state.is_enabled = value;
>  }
>  
> -static void pc_machine_get_nvdimm_capabilities(Object *obj, Visitor *v,
> -                                               const char *name, void *opaque,
> -                                               Error **errp)
> +static char *pc_machine_get_nvdimm_persistence(Object *obj, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
> -    uint32_t value = pcms->acpi_nvdimm_state.capabilities;
>  
> -    visit_type_uint32(v, name, &value, errp);
> +    return g_strdup(pcms->acpi_nvdimm_state.persistence_string);
>  }
>  
> -static void pc_machine_set_nvdimm_capabilities(Object *obj, Visitor *v,
> -                                               const char *name, void *opaque,
> +static void pc_machine_set_nvdimm_persistence(Object *obj, const char *value,
>                                                 Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
> -    Error *error = NULL;
> -    uint32_t value;
> -
> -    visit_type_uint32(v, name, &value, &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        return;
> +    AcpiNVDIMMState *nvdimm_state = &pcms->acpi_nvdimm_state;
> +
> +    if (strcmp(value, "cpu") == 0)
> +        nvdimm_state->persistence = 3;
> +    else if (strcmp(value, "mem-ctrl") == 0)
> +        nvdimm_state->persistence = 2;
> +    else {
> +        error_report("-machine nvdimm-persistence=%s: unsupported option", value);
> +        exit(EXIT_FAILURE);
>      }
>  
> -    pcms->acpi_nvdimm_state.capabilities = value;
> +    g_free(nvdimm_state->persistence_string);
> +    nvdimm_state->persistence_string = g_strdup(value);
>  }
>  
>  static bool pc_machine_get_smbus(Object *obj, Error **errp)
> @@ -2421,9 +2420,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
>          pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
>  
> -    object_class_property_add(oc, PC_MACHINE_NVDIMM_CAP, "uint32",
> -        pc_machine_get_nvdimm_capabilities,
> -        pc_machine_set_nvdimm_capabilities, NULL, NULL, &error_abort);
> +    object_class_property_add_str(oc, PC_MACHINE_NVDIMM_PERSIST,
> +        pc_machine_get_nvdimm_persistence,
> +        pc_machine_set_nvdimm_persistence, &error_abort);
>  
>      object_class_property_add_bool(oc, PC_MACHINE_SMBUS,
>          pc_machine_get_smbus, pc_machine_set_smbus, &error_abort);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 04d1f8c6c3..fc8dedca12 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -76,7 +76,7 @@ struct PCMachineState {
>  #define PC_MACHINE_VMPORT           "vmport"
>  #define PC_MACHINE_SMM              "smm"
>  #define PC_MACHINE_NVDIMM           "nvdimm"
> -#define PC_MACHINE_NVDIMM_CAP       "nvdimm-cap"
> +#define PC_MACHINE_NVDIMM_PERSIST   "nvdimm-persistence"
>  #define PC_MACHINE_SMBUS            "smbus"
>  #define PC_MACHINE_SATA             "sata"
>  #define PC_MACHINE_PIT              "pit"
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 3c82751bab..9340631cfc 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -138,7 +138,8 @@ struct AcpiNVDIMMState {
>      /*
>       * Platform capabilities, section 5.2.25.9 of ACPI 6.2 Errata A
>       */
> -    int32_t capabilities;
> +    int32_t persistence;
> +    char    *persistence_string;
>  };
>  typedef struct AcpiNVDIMMState AcpiNVDIMMState;
>  
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 9b5db1ee8f..b95d56aa4e 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -831,7 +831,7 @@ static void test_acpi_tcg_dimm_pxm(const char *machine)
>      memset(&data, 0, sizeof(data));
>      data.machine = machine;
>      data.variant = ".dimmpxm";
> -    test_acpi_one(" -machine nvdimm=on,nvdimm-cap=3"
> +    test_acpi_one(" -machine nvdimm=on,nvdimm-persistence=cpu"
>                    " -smp 4,sockets=4"
>                    " -m 128M,slots=3,maxmem=1G"
>                    " -numa node,mem=32M,nodeid=0"
> -- 
> 2.14.4

Re: [Qemu-devel] [qemu PATCH 1/5] gitignore: ignore generated qapi job files

[Eric Blake]

On 06/07/2018 05:31 PM, Ross Zwisler wrote:
> With a fully built QEMU I currently see the following with "git status":
> 
>    Untracked files:
>      (use "git add <file>..." to include in what will be committed)
> 
>    	qapi/qapi-commands-job.c
>    	qapi/qapi-commands-job.h
>    	qapi/qapi-events-job.c
>    	qapi/qapi-events-job.h
>    	qapi/qapi-types-job.c
>    	qapi/qapi-types-job.h
>    	qapi/qapi-visit-job.c
>    	qapi/qapi-visit-job.h
> 
> These are all generated files.  Ignore them.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Fixes: commit bf42508f24ee ("job: Introduce qapi/job.json")

You're the third to post this:

https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg07280.html
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg01624.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [qemu PATCH 1/5] gitignore: ignore generated qapi job files

[Eric Blake]

On 06/08/2018 09:59 AM, Eric Blake wrote:
> On 06/07/2018 05:31 PM, Ross Zwisler wrote:
>> With a fully built QEMU I currently see the following with "git status":
>>
>>    Untracked files:
>>      (use "git add <file>..." to include in what will be committed)
>>
>>        qapi/qapi-commands-job.c
>>        qapi/qapi-commands-job.h
>>        qapi/qapi-events-job.c
>>        qapi/qapi-events-job.h
>>        qapi/qapi-types-job.c
>>        qapi/qapi-types-job.h
>>        qapi/qapi-visit-job.c
>>        qapi/qapi-visit-job.h
>>
>> These are all generated files.  Ignore them.
>>
>> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Fixes: commit bf42508f24ee ("job: Introduce qapi/job.json")
> 
> You're the third to post this:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg07280.html
> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg01624.html

Also, when sending a series, it's best to include a 0/5 cover letter 
(the automated CI tooling handles that better, among other reasons). 
You can use 'git config' to automatically send a cover letter for any 
patch series > 1.  For more tips, 
https://wiki.qemu.org/Contribute/SubmitAPatch

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [qemu PATCH 1/5] gitignore: ignore generated qapi job files

[Ross Zwisler]

On Fri, Jun 08, 2018 at 09:59:00AM -0500, Eric Blake wrote:
> On 06/07/2018 05:31 PM, Ross Zwisler wrote:
> > With a fully built QEMU I currently see the following with "git status":
> > 
> >    Untracked files:
> >      (use "git add <file>..." to include in what will be committed)
> > 
> >    	qapi/qapi-commands-job.c
> >    	qapi/qapi-commands-job.h
> >    	qapi/qapi-events-job.c
> >    	qapi/qapi-events-job.h
> >    	qapi/qapi-types-job.c
> >    	qapi/qapi-types-job.h
> >    	qapi/qapi-visit-job.c
> >    	qapi/qapi-visit-job.h
> > 
> > These are all generated files.  Ignore them.
> > 
> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: Eric Blake <eblake@redhat.com>
> > Fixes: commit bf42508f24ee ("job: Introduce qapi/job.json")
> 
> You're the third to post this:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg07280.html
> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg01624.html

I pulled your master branch yesterday before posting the series.  If multiple
people are sending fixes to the same issue and the fixes are correct, the
easiest way to stop getting those fixes is probably to merge one of the
patches.