Re: [PATCH v4 0/4] softmmu/memory_mapping: optimize dump/tpm for virtio-mem

[Paolo Bonzini]

On 27/07/21 10:25, David Hildenbrand wrote:

Minor fixes and cleanups, followed by an optimization for virtio-mem
regarding guest dumps and tpm.

virtio-mem logically plugs/unplugs memory within a sparse memory region
and notifies via the RamDiscardMgr interface when parts become
plugged (populated) or unplugged (discarded).

Currently, guest_phys_blocks_append() appends the whole (sparse)
virtio-mem managed region and therefore tpm code might zero the hole
region and dump code will dump the whole region. Let's only add logically
plugged (populated) parts of that region, skipping over logically
unplugged (discarded) parts by reusing the RamDiscardMgr infrastructure
introduced to handle virtio-mem + VFIO properly.


Queued, thanks.

Paolo


v3 -> v4:
- "tpm: mark correct memory region range dirty when clearing RAM"
-- Finally get it right :) I tried triggering that code without luck. So
I ended up forcing that call path, verifying that the offset into
memory regions is now correct.

v2 -> v3:
- "tpm: mark correct memory region range dirty when clearing RAM"
-- Fix calculation of offset into memory region (thanks Peter!)
- "softmmu/memory_mapping: reuse qemu_get_guest_simple_memory_mapping()"
-- Dropped

v1 -> v2:
- "softmmu/memory_mapping: factor out adding physical memory ranges"
-- Simplify based on RamDiscardManager changes: add using a
MemoryRegionSection
- "softmmu/memory_mapping: optimize for RamDiscardManager sections"
-- Simplify based on RamDiscardManager changes

David Hildenbrand (4):
   tpm: mark correct memory region range dirty when clearing RAM
   softmmu/memory_mapping: never merge ranges accross memory regions
   softmmu/memory_mapping: factor out adding physical memory ranges
   softmmu/memory_mapping: optimize for RamDiscardManager sections

  hw/tpm/tpm_ppi.c |  5 +++-
  softmmu/memory_mapping.c | 64 ++--
  2 files changed, 46 insertions(+), 23 deletions(-)

[PULL 02/13] qapi/gen: use dict.items() to iterate over _modules

[Markus Armbruster]

From: John Snow 

New pylint warning. I could silence it, but this is the only occurrence
in the entire tree, including everything in iotests/ and python/. Easier
to just change this one instance.

(The warning is emitted in cases where you are fetching the values
anyway, so you may as well just take advantage of the iterator to avoid
redundant lookups.)

Signed-off-by: John Snow 
Message-Id: <20210930205716.1148693-3-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 scripts/qapi/gen.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index ab26d5c937..2ec1e7b3b6 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -296,10 +296,9 @@ def _temp_module(self, name: str) -> Iterator[None]:
 self._current_module = old_module
 
 def write(self, output_dir: str, opt_builtins: bool = False) -> None:
-for name in self._module:
+for name, (genc, genh) in self._module.items():
 if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
 continue
-(genc, genh) = self._module[name]
 genc.write(output_dir)
 genh.write(output_dir)
 
-- 
2.31.1

[PULL 07/13] qapi/parser: Introduce NullSection

[Markus Armbruster]

From: John Snow 

Here's the weird bit. QAPIDoc generally expects -- virtually everywhere
-- that it will always have a current section. The sole exception to
this is in the case that end_comment() is called, which leaves us with
*no* section. However, in this case, we also don't expect to actually
ever mutate the comment contents ever again.

NullSection is just a Null-object that allows us to maintain the
invariant that we *always* have a current section, enforced by static
typing -- allowing us to type that field as QAPIDoc.Section instead of
the more ambiguous Optional[QAPIDoc.Section].

end_section is renamed to switch_section and now accepts as an argument
the new section to activate, clarifying that no callers ever just
unilaterally end a section; they only do so when starting a new section.

Signed-off-by: John Snow 

Message-Id: <20210930205716.1148693-8-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 scripts/qapi/parser.py | 27 ---
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 82f1d952b1..40c5da4b17 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -478,6 +478,13 @@ def __init__(self, parser, name, indent=0):
 def connect(self, member):
 self.member = member
 
+class NullSection(Section):
+"""
+Immutable dummy section for use at the end of a doc block.
+"""
+def append(self, line):
+assert False, "Text appended after end_comment() called."
+
 def __init__(self, parser, info):
 # self._parser is used to report errors with QAPIParseError.  The
 # resulting error position depends on the state of the parser.
@@ -525,7 +532,7 @@ def append(self, line):
 self._append_line(line)
 
 def end_comment(self):
-self._end_section()
+self._switch_section(QAPIDoc.NullSection(self._parser))
 
 @staticmethod
 def _is_section_tag(name):
@@ -699,9 +706,9 @@ def _start_symbol_section(self, symbols_dict, name, indent):
 raise QAPIParseError(self._parser,
  "'%s' parameter name duplicated" % name)
 assert not self.sections
-self._end_section()
-self._section = QAPIDoc.ArgSection(self._parser, name, indent)
-symbols_dict[name] = self._section
+new_section = QAPIDoc.ArgSection(self._parser, name, indent)
+self._switch_section(new_section)
+symbols_dict[name] = new_section
 
 def _start_args_section(self, name, indent):
 self._start_symbol_section(self.args, name, indent)
@@ -713,13 +720,11 @@ def _start_section(self, name=None, indent=0):
 if name in ('Returns', 'Since') and self.has_section(name):
 raise QAPIParseError(self._parser,
  "duplicated '%s' section" % name)
-self._end_section()
-self._section = QAPIDoc.Section(self._parser, name, indent)
-self.sections.append(self._section)
-
-def _end_section(self):
-assert self._section is not None
+new_section = QAPIDoc.Section(self._parser, name, indent)
+self._switch_section(new_section)
+self.sections.append(new_section)
 
+def _switch_section(self, new_section):
 text = self._section.text = self._section.text.strip()
 
 # Only the 'body' section is allowed to have an empty body.
@@ -732,7 +737,7 @@ def _end_section(self):
 self._parser,
 "empty doc section '%s'" % self._section.name)
 
-self._section = None
+self._section = new_section
 
 def _append_freeform(self, line):
 match = re.match(r'(@\S+:)', line)
-- 
2.31.1

Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device

[Paolo Bonzini]

On 22/09/21 14:57, David Hildenbrand wrote:

We want to rate-limit MEMORY_DEVICE_SIZE_CHANGE events per device,
otherwise we can lose some events for devices. As we might not always have
a device id, add the qom-path to the event and use that to rate-limit
per device.

This was noticed by starting a VM with two virtio-mem devices that each
have a requested size > 0. The Linux guest will initialize both devices
in parallel, resulting in losing MEMORY_DEVICE_SIZE_CHANGE events for
one of the devices.

Fixes: 722a3c783ef4 ("virtio-pci: Send qapi events when the virtio-mem size 
changes")
Suggested-by: Markus Armbruster 
Cc: "Dr. David Alan Gilbert" 
Cc: Markus Armbruster 
Cc: Michael S. Tsirkin 
Cc: Eric Blake 
Cc: Igor Mammedov 
Cc: Michal Privoznik 
Signed-off-by: David Hildenbrand 
---


I cannot find v3 in my inbox, but I queued it from patchew.

Thanks,

Paolo



Follow up of:
 https://lkml.kernel.org/r/20210921102434.24273-1-da...@redhat.com

v1 -> v2:
- Add the qom-path and use that identifier to rate-limit per device
- Rephrase subject/description

---
  hw/virtio/virtio-mem-pci.c | 3 ++-
  monitor/monitor.c  | 9 +
  qapi/machine.json  | 5 -
  3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-mem-pci.c b/hw/virtio/virtio-mem-pci.c
index fa5395cd88..dd5085497f 100644
--- a/hw/virtio/virtio-mem-pci.c
+++ b/hw/virtio/virtio-mem-pci.c
@@ -87,6 +87,7 @@ static void virtio_mem_pci_size_change_notify(Notifier 
*notifier, void *data)
  VirtIOMEMPCI *pci_mem = container_of(notifier, VirtIOMEMPCI,
   size_change_notifier);
  DeviceState *dev = DEVICE(pci_mem);
+const char * qom_path = object_get_canonical_path(OBJECT(dev));
  const uint64_t * const size_p = data;
  const char *id = NULL;
  
@@ -94,7 +95,7 @@ static void virtio_mem_pci_size_change_notify(Notifier *notifier, void *data)

  id = g_strdup(dev->id);
  }
  
-qapi_event_send_memory_device_size_change(!!id, id, *size_p);

+qapi_event_send_memory_device_size_change(!!id, id, *size_p, qom_path);
  }
  
  static void virtio_mem_pci_class_init(ObjectClass *klass, void *data)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 46a171bca6..21c7a68758 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -474,6 +474,10 @@ static unsigned int qapi_event_throttle_hash(const void 
*key)
  hash += g_str_hash(qdict_get_str(evstate->data, "node-name"));
  }
  
+if (evstate->event == QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE) {

+hash += g_str_hash(qdict_get_str(evstate->data, "qom-path"));
+}
+
  return hash;
  }
  
@@ -496,6 +500,11 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b)

 qdict_get_str(evb->data, "node-name"));
  }
  
+if (eva->event == QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE) {

+return !strcmp(qdict_get_str(eva->data, "qom-path"),
+   qdict_get_str(evb->data, "qom-path"));
+}
+
  return TRUE;
  }
  
diff --git a/qapi/machine.json b/qapi/machine.json

index 157712f006..2487c92f18 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1245,8 +1245,11 @@
  # action).
  #
  # @id: device's ID
+#
  # @size: the new size of memory that the device provides
  #
+# @qom-path: path to the device object in the QOM tree (since 6.2)
+#
  # Note: this event is rate-limited.
  #
  # Since: 5.1
@@ -1259,7 +1262,7 @@
  #
  ##
  { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
-  'data': { '*id': 'str', 'size': 'size' } }
+  'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
  
  
  ##

[PULL 13/13] qapi/parser: enable pylint checks

[Markus Armbruster]

From: John Snow 

Signed-off-by: John Snow 

Message-Id: <20210930205716.1148693-14-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 scripts/qapi/pylintrc | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index 5b7dbc58ad..b259531a72 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -2,8 +2,7 @@
 
 # Add files or directories matching the regex patterns to the ignore list.
 # The regex matches against base names, not paths.
-ignore-patterns=parser.py,
-schema.py,
+ignore-patterns=schema.py,
 
 
 [MESSAGES CONTROL]
-- 
2.31.1

[PULL 04/13] qapi: Add spaces after symbol declaration for consistency

[Markus Armbruster]

From: John Snow 

Several QGA definitions omit a blank line after the symbol
declaration. This works OK currently, but it's the only place where we
do this. Adjust it for consistency.

Future commits may wind up enforcing this formatting.

Signed-off-by: John Snow 

Message-Id: <20210930205716.1148693-5-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 qapi/block-core.json| 1 +
 qga/qapi-schema.json| 3 +++
 tests/qapi-schema/doc-good.json | 8 
 3 files changed, 12 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 623a4f4a3f..6d3217abb6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3132,6 +3132,7 @@
 
 ##
 # @BlockdevQcow2EncryptionFormat:
+#
 # @aes: AES-CBC with plain64 initialization vectors
 #
 # Since: 2.10
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index c60f5e669d..94e4aacdcc 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1140,6 +1140,7 @@
 
 ##
 # @GuestExec:
+#
 # @pid: pid of child process in guest OS
 #
 # Since: 2.5
@@ -1171,6 +1172,7 @@
 
 ##
 # @GuestHostName:
+#
 # @host-name: Fully qualified domain name of the guest OS
 #
 # Since: 2.10
@@ -1197,6 +1199,7 @@
 
 ##
 # @GuestUser:
+#
 # @user: Username
 # @domain: Logon domain (windows only)
 # @login-time: Time of login of this user on the computer. If multiple
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index a20acffd8b..86dc25d2bd 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -53,6 +53,7 @@
 
 ##
 # @Enum:
+#
 # @one: The _one_ {and only}
 #
 # Features:
@@ -67,6 +68,7 @@
 
 ##
 # @Base:
+#
 # @base1:
 # the first member
 ##
@@ -75,6 +77,7 @@
 
 ##
 # @Variant1:
+#
 # A paragraph
 #
 # Another paragraph (but no @var: line)
@@ -91,11 +94,13 @@
 
 ##
 # @Variant2:
+#
 ##
 { 'struct': 'Variant2', 'data': {} }
 
 ##
 # @Object:
+#
 # Features:
 # @union-feat1: a feature
 ##
@@ -109,6 +114,7 @@
 
 ##
 # @Alternate:
+#
 # @i: an integer
 # @b is undocumented
 #
@@ -126,6 +132,7 @@
 
 ##
 # @cmd:
+#
 # @arg1: the first argument
 #
 # @arg2: the second
@@ -175,6 +182,7 @@
 
 ##
 # @EVT_BOXED:
+#
 # Features:
 # @feat3: a feature
 ##
-- 
2.31.1

[PULL 00/13] QAPI patches patches for 2021-10-02

[Markus Armbruster]

The following changes since commit 5f992102383ed8ed97076548e1c897c7034ed8a4:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging 
(2021-10-01 13:44:36 -0400)

are available in the Git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2021-10-02

for you to fetch changes up to d183e0481b1510b253ac94e702c76115f3bb6450:

  qapi/parser: enable pylint checks (2021-10-02 07:33:42 +0200)


QAPI patches patches for 2021-10-02


John Snow (13):
  qapi/pylintrc: ignore 'consider-using-f-string' warning
  qapi/gen: use dict.items() to iterate over _modules
  qapi/parser: fix unused check_args_section arguments
  qapi: Add spaces after symbol declaration for consistency
  qapi/parser: remove FIXME comment from _append_body_line
  qapi/parser: clarify _end_section() logic
  qapi/parser: Introduce NullSection
  qapi/parser: add import cycle workaround
  qapi/parser: add type hint annotations (QAPIDoc)
  qapi/parser: Add FIXME for consolidating JSON-related types
  qapi/parser: enable mypy checks
  qapi/parser: Silence too-few-public-methods warning
  qapi/parser: enable pylint checks

 qapi/block-core.json   |   1 +
 qga/qapi-schema.json   |   3 +
 scripts/qapi/gen.py|   3 +-
 scripts/qapi/mypy.ini  |   5 --
 scripts/qapi/parser.py | 148 +
 scripts/qapi/pylintrc  |   4 +-
 tests/qapi-schema/doc-bad-feature.err  |   2 +-
 tests/qapi-schema/doc-empty-symbol.err |   2 +-
 tests/qapi-schema/doc-good.json|   8 ++
 9 files changed, 112 insertions(+), 64 deletions(-)

-- 
2.31.1

[PULL 11/13] qapi/parser: enable mypy checks

[Markus Armbruster]

From: John Snow 

Signed-off-by: John Snow 

Message-Id: <20210930205716.1148693-12-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 scripts/qapi/mypy.ini | 5 -
 1 file changed, 5 deletions(-)

diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 54ca4483d6..6625356429 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -3,11 +3,6 @@ strict = True
 disallow_untyped_calls = False
 python_version = 3.6
 
-[mypy-qapi.parser]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-
 [mypy-qapi.schema]
 disallow_untyped_defs = False
 disallow_incomplete_defs = False
-- 
2.31.1

[PULL 12/13] qapi/parser: Silence too-few-public-methods warning

[Markus Armbruster]

From: John Snow 

Eh. Not worth the fuss today. There are bigger fish to fry.

Signed-off-by: John Snow 
Message-Id: <20210930205716.1148693-13-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 scripts/qapi/parser.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 0265b47b95..1b006cdc13 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -461,8 +461,10 @@ class QAPIDoc:
 """
 
 class Section:
+# pylint: disable=too-few-public-methods
 def __init__(self, parser: QAPISchemaParser,
  name: Optional[str] = None, indent: int = 0):
+
 # parser, for error messages about indentation
 self._parser = parser
 # optional section name (argument/member or section name)
@@ -498,6 +500,7 @@ class NullSection(Section):
 """
 Immutable dummy section for use at the end of a doc block.
 """
+# pylint: disable=too-few-public-methods
 def append(self, line: str) -> None:
 assert False, "Text appended after end_comment() called."
 
-- 
2.31.1

[PULL 06/13] qapi/parser: clarify _end_section() logic

[Markus Armbruster]

From: John Snow 

The "if self._section" clause in end_section is mysterious: In which
circumstances might we end a section when we don't have one?

QAPIDoc always expects there to be a "current section", only except
after a call to end_comment(). This actually *shouldn't* ever be 'None',
so let's remove that logic so I don't wonder why it's like this again in
three months.

Signed-off-by: John Snow 
Message-Id: <20210930205716.1148693-7-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 scripts/qapi/parser.py | 22 +++---
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 23898ab1dc..82f1d952b1 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -718,13 +718,21 @@ def _start_section(self, name=None, indent=0):
 self.sections.append(self._section)
 
 def _end_section(self):
-if self._section:
-text = self._section.text = self._section.text.strip()
-if self._section.name and (not text or text.isspace()):
-raise QAPIParseError(
-self._parser,
-"empty doc section '%s'" % self._section.name)
-self._section = None
+assert self._section is not None
+
+text = self._section.text = self._section.text.strip()
+
+# Only the 'body' section is allowed to have an empty body.
+# All other sections, including anonymous ones, must have text.
+if self._section != self.body and not text:
+# We do not create anonymous sections unless there is
+# something to put in them; this is a parser bug.
+assert self._section.name
+raise QAPIParseError(
+self._parser,
+"empty doc section '%s'" % self._section.name)
+
+self._section = None
 
 def _append_freeform(self, line):
 match = re.match(r'(@\S+:)', line)
-- 
2.31.1

[PULL 01/13] qapi/pylintrc: ignore 'consider-using-f-string' warning

[Markus Armbruster]

From: John Snow 

Pylint 2.11.x adds this warning. We're not yet ready to pursue that
conversion, so silence it for now.

Signed-off-by: John Snow 
Message-Id: <20210930205716.1148693-2-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 scripts/qapi/pylintrc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index c5275d5f59..5b7dbc58ad 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -23,6 +23,7 @@ disable=fixme,
 too-many-branches,
 too-many-statements,
 too-many-instance-attributes,
+consider-using-f-string,
 
 [REPORTS]
 
-- 
2.31.1

[PULL 10/13] qapi/parser: Add FIXME for consolidating JSON-related types

[Markus Armbruster]

From: John Snow 

The fix for this comment is forthcoming in a future commit, but this
will keep me honest. The linting configuration in ./python/setup.cfg
prohibits 'FIXME' comments. A goal of this long-running series is to
move ./scripts/qapi to ./python/qemu/qapi so that the QAPI generator is
regularly type-checked by GitLab CI.

This comment is a time-bomb to force me to address this issue prior to
that step.

Signed-off-by: John Snow 
Message-Id: <20210930205716.1148693-11-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 scripts/qapi/parser.py | 4 
 1 file changed, 4 insertions(+)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 73c1c4ef59..0265b47b95 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -43,6 +43,10 @@
 # Return value alias for get_expr().
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
+# FIXME: Consolidate and centralize definitions for TopLevelExpr,
+# _ExprValue, _JSONValue, and _JSONObject; currently scattered across
+# several modules.
+
 
 class QAPIParseError(QAPISourceError):
 """Error class for all QAPI schema parsing errors."""
-- 
2.31.1

[PULL 05/13] qapi/parser: remove FIXME comment from _append_body_line

[Markus Armbruster]

From: John Snow 

True, we do not check the validity of this symbol -- but we don't check
the validity of definition names during parse, either -- that happens
later, during the expr check. I don't want to introduce a dependency on
expr.py:check_name_str here and introduce a cycle.

Instead, rest assured that a documentation block is required for each
definition. This requirement uses the names of each section to ensure
that we fulfilled this requirement.

e.g., let's say that block-core.json has a comment block for
"Snapshot!Info" by accident. We'll see this error message:

In file included from ../../qapi/block.json:8:
../../qapi/block-core.json: In struct 'SnapshotInfo':
../../qapi/block-core.json:38: documentation comment is for 'Snapshot!Info'

That's a pretty decent error message.

Now, let's say that we actually mangle it twice, identically:

../../qapi/block-core.json: In struct 'Snapshot!Info':
../../qapi/block-core.json:38: struct has an invalid name

That's also pretty decent. If we forget to fix it in both places, we'll
just be back to the first error.

Therefore, let's just drop this FIXME and adjust the error message to
not imply a more thorough check than is actually performed.

Signed-off-by: John Snow 
Message-Id: <20210930205716.1148693-6-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 scripts/qapi/parser.py | 6 --
 tests/qapi-schema/doc-empty-symbol.err | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index bfd2dbfd9a..23898ab1dc 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -556,9 +556,11 @@ def _append_body_line(self, line):
 if not line.endswith(':'):
 raise QAPIParseError(self._parser, "line should end with ':'")
 self.symbol = line[1:-1]
-# FIXME invalid names other than the empty string aren't flagged
+# Invalid names are not checked here, but the name provided MUST
+# match the following definition, which *is* validated in expr.py.
 if not self.symbol:
-raise QAPIParseError(self._parser, "invalid name")
+raise QAPIParseError(
+self._parser, "name required after '@'")
 elif self.symbol:
 # This is a definition documentation block
 if name.startswith('@') and name.endswith(':'):
diff --git a/tests/qapi-schema/doc-empty-symbol.err 
b/tests/qapi-schema/doc-empty-symbol.err
index 81b90e882a..aa51be41b2 100644
--- a/tests/qapi-schema/doc-empty-symbol.err
+++ b/tests/qapi-schema/doc-empty-symbol.err
@@ -1 +1 @@
-doc-empty-symbol.json:4:1: invalid name
+doc-empty-symbol.json:4:1: name required after '@'
-- 
2.31.1

[PULL 08/13] qapi/parser: add import cycle workaround

[Markus Armbruster]

From: John Snow 

Adding static types causes a cycle in the QAPI generator:
[schema -> expr -> parser -> schema]. It exists because the QAPIDoc
class needs the names of types defined by the schema module, but the
schema module needs to import both expr.py/parser.py to do its actual
parsing.

Ultimately, the layering violation is that parser.py should not have any
knowledge of specifics of the Schema. QAPIDoc performs double-duty here
both as a parser *and* as a finalized object that is part of the schema.

In this patch, add the offending type hints alongside the workaround to
avoid the cycle becoming a problem at runtime. See
https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles
for more information on this workaround technique.

I see three ultimate resolutions here:

(1) Just keep this patch and use the TYPE_CHECKING trick to eliminate
the cycle which is only present during static analysis.

(2) Don't bother to annotate connect_member() et al, give them 'object'
or 'Any'. I don't particularly like this, because it diminishes the
usefulness of type hints for documentation purposes. Still, it's an
extremely quick fix.

(3) Reimplement doc <--> definition correlation directly in schema.py,
integrating doc fields directly into QAPISchemaMember and relieving
the QAPIDoc class of the responsibility. Users of the information
would instead visit the members first and retrieve their
documentation instead of the inverse operation -- visiting the
documentation and retrieving their members.

My preference is (3), but in the short-term (1) is the easiest way to
have my cake (strong type hints) and eat it too (Not have import
cycles). Do (1) for now, but plan for (3).

Signed-off-by: John Snow 
Message-Id: <20210930205716.1148693-9-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 scripts/qapi/parser.py | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 40c5da4b17..75582ddb00 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -18,6 +18,7 @@
 import os
 import re
 from typing import (
+TYPE_CHECKING,
 Dict,
 List,
 Optional,
@@ -30,6 +31,12 @@
 from .source import QAPISourceInfo
 
 
+if TYPE_CHECKING:
+# pylint: disable=cyclic-import
+# TODO: Remove cycle. [schema -> expr -> parser -> schema]
+from .schema import QAPISchemaFeature, QAPISchemaMember
+
+
 # Return value alias for get_expr().
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
@@ -473,9 +480,9 @@ def append(self, line):
 class ArgSection(Section):
 def __init__(self, parser, name, indent=0):
 super().__init__(parser, name, indent)
-self.member = None
+self.member: Optional['QAPISchemaMember'] = None
 
-def connect(self, member):
+def connect(self, member: 'QAPISchemaMember') -> None:
 self.member = member
 
 class NullSection(Section):
@@ -747,14 +754,14 @@ def _append_freeform(self, line):
  % match.group(1))
 self._section.append(line)
 
-def connect_member(self, member):
+def connect_member(self, member: 'QAPISchemaMember') -> None:
 if member.name not in self.args:
 # Undocumented TODO outlaw
 self.args[member.name] = QAPIDoc.ArgSection(self._parser,
 member.name)
 self.args[member.name].connect(member)
 
-def connect_feature(self, feature):
+def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
 if feature.name not in self.features:
 raise QAPISemError(feature.info,
"feature '%s' lacks documentation"
-- 
2.31.1

[PULL 09/13] qapi/parser: add type hint annotations (QAPIDoc)

[Markus Armbruster]

From: John Snow 

Annotations do not change runtime behavior.
This commit consists of only annotations.

Signed-off-by: John Snow 
Message-Id: <20210930205716.1148693-10-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 scripts/qapi/parser.py | 67 --
 1 file changed, 39 insertions(+), 28 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 75582ddb00..73c1c4ef59 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -37,6 +37,9 @@
 from .schema import QAPISchemaFeature, QAPISchemaMember
 
 
+#: Represents a single Top Level QAPI schema expression.
+TopLevelExpr = Dict[str, object]
+
 # Return value alias for get_expr().
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
@@ -454,7 +457,8 @@ class QAPIDoc:
 """
 
 class Section:
-def __init__(self, parser, name=None, indent=0):
+def __init__(self, parser: QAPISchemaParser,
+ name: Optional[str] = None, indent: int = 0):
 # parser, for error messages about indentation
 self._parser = parser
 # optional section name (argument/member or section name)
@@ -463,7 +467,7 @@ def __init__(self, parser, name=None, indent=0):
 # the expected indent level of the text of this section
 self._indent = indent
 
-def append(self, line):
+def append(self, line: str) -> None:
 # Strip leading spaces corresponding to the expected indent level
 # Blank lines are always OK.
 if line:
@@ -478,7 +482,8 @@ def append(self, line):
 self.text += line.rstrip() + '\n'
 
 class ArgSection(Section):
-def __init__(self, parser, name, indent=0):
+def __init__(self, parser: QAPISchemaParser,
+ name: str, indent: int = 0):
 super().__init__(parser, name, indent)
 self.member: Optional['QAPISchemaMember'] = None
 
@@ -489,35 +494,34 @@ class NullSection(Section):
 """
 Immutable dummy section for use at the end of a doc block.
 """
-def append(self, line):
+def append(self, line: str) -> None:
 assert False, "Text appended after end_comment() called."
 
-def __init__(self, parser, info):
+def __init__(self, parser: QAPISchemaParser, info: QAPISourceInfo):
 # self._parser is used to report errors with QAPIParseError.  The
 # resulting error position depends on the state of the parser.
 # It happens to be the beginning of the comment.  More or less
 # servicable, but action at a distance.
 self._parser = parser
 self.info = info
-self.symbol = None
+self.symbol: Optional[str] = None
 self.body = QAPIDoc.Section(parser)
-# dict mapping parameter name to ArgSection
-self.args = OrderedDict()
-self.features = OrderedDict()
-# a list of Section
-self.sections = []
+# dicts mapping parameter/feature names to their ArgSection
+self.args: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
+self.features: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
+self.sections: List[QAPIDoc.Section] = []
 # the current section
 self._section = self.body
 self._append_line = self._append_body_line
 
-def has_section(self, name):
+def has_section(self, name: str) -> bool:
 """Return True if we have a section with this name."""
 for i in self.sections:
 if i.name == name:
 return True
 return False
 
-def append(self, line):
+def append(self, line: str) -> None:
 """
 Parse a comment line and add it to the documentation.
 
@@ -538,18 +542,18 @@ def append(self, line):
 line = line[1:]
 self._append_line(line)
 
-def end_comment(self):
+def end_comment(self) -> None:
 self._switch_section(QAPIDoc.NullSection(self._parser))
 
 @staticmethod
-def _is_section_tag(name):
+def _is_section_tag(name: str) -> bool:
 return name in ('Returns:', 'Since:',
 # those are often singular or plural
 'Note:', 'Notes:',
 'Example:', 'Examples:',
 'TODO:')
 
-def _append_body_line(self, line):
+def _append_body_line(self, line: str) -> None:
 """
 Process a line of documentation text in the body section.
 
@@ -591,7 +595,7 @@ def _append_body_line(self, line):
 # This is a free-form documentation block
 self._append_freeform(line)
 
-def _append_args_line(self, line):
+def _append_args_line(self, line: str) -> None:
 """
 Process a line of documentation text in an argument section.
 
@@ -637,7 +641,7 @@ def _append_args_line(self, line):
 
 self._ap

[PULL 03/13] qapi/parser: fix unused check_args_section arguments

[Markus Armbruster]

From: John Snow 

Pylint informs us we're not using these arguments. Oops, it's
right. Correct the error message and remove the remaining unused
parameter.

Fix test output now that the error message is improved.

Fixes: e151941d1b
Signed-off-by: John Snow 
Message-Id: <20210930205716.1148693-4-js...@redhat.com>
[Commit message formatting tweaked]
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 scripts/qapi/parser.py| 16 +---
 tests/qapi-schema/doc-bad-feature.err |  2 +-
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index f03ba2cfec..bfd2dbfd9a 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -753,16 +753,18 @@ def check_expr(self, expr):
 
 def check(self):
 
-def check_args_section(args, info, what):
+def check_args_section(args, what):
 bogus = [name for name, section in args.items()
  if not section.member]
 if bogus:
 raise QAPISemError(
 self.info,
-"documented member%s '%s' %s not exist"
-% ("s" if len(bogus) > 1 else "",
-   "', '".join(bogus),
-   "do" if len(bogus) > 1 else "does"))
+"documented %s%s '%s' %s not exist" % (
+what,
+"s" if len(bogus) > 1 else "",
+"', '".join(bogus),
+"do" if len(bogus) > 1 else "does"
+))
 
-check_args_section(self.args, self.info, 'members')
-check_args_section(self.features, self.info, 'features')
+check_args_section(self.args, 'member')
+check_args_section(self.features, 'feature')
diff --git a/tests/qapi-schema/doc-bad-feature.err 
b/tests/qapi-schema/doc-bad-feature.err
index e4c62adfa3..49d1746c3d 100644
--- a/tests/qapi-schema/doc-bad-feature.err
+++ b/tests/qapi-schema/doc-bad-feature.err
@@ -1 +1 @@
-doc-bad-feature.json:3: documented member 'a' does not exist
+doc-bad-feature.json:3: documented feature 'a' does not exist
-- 
2.31.1

Re: [PULL v2 00/33] x86 and misc changes for 2021-09-28

[Peter Maydell]

On Sat, 2 Oct 2021 at 01:41, Richard Henderson
 wrote:
>
> On 9/30/21 10:57 AM, Paolo Bonzini wrote:
> > The following changes since commit ba0fa56bc06e563de68d2a2bf3ddb0cfea1be4f9:
> >
> >Merge remote-tracking branch 
> > 'remotes/vivier/tags/q800-for-6.2-pull-request' into staging (2021-09-29 
> > 21:20:49 +0100)
> >
> > are available in the Git repository at:
> >
> >https://gitlab.com/bonzini/qemu.git  tags/for-upstream
> >
> > for you to fetch changes up to c1de5858bd39b299d3d8baec38b0376bed7f19e8:
> >
> >meson_options.txt: Switch the default value for the vnc option to 'auto' 
> > (2021-09-30 15:30:25 +0200)
>
> Applied, thanks

Uh, I'd already done this one :-)

-- PMM

Re: [PATCH v3 0/9] hw/nvram: hw/arm: Introduce Xilinx eFUSE and BBRAM

[Peter Maydell]

On Fri, 17 Sept 2021 at 06:24, Tong Ho  wrote:
>
> This series implements the Xilinx eFUSE and BBRAM devices for
> the Versal and ZynqMP product families.
>
> Furthermore, both new devices are connected to the xlnx-versal-virt
> board and the xlnx-zcu102 board.
>
> See changes in docs/system/arm/xlnx-versal-virt.rst for detail.

Hi -- now this has landed upstream, Coverity points out a
lot of memory leaks on error or logging paths, where
the code does things like:

*** CID 1464071:  Resource leaks  (RESOURCE_LEAK)
/qemu/hw/nvram/xlnx-versal-efuse-ctrl.c: 628 in efuse_ctrl_reg_write()
622 dev = reg_array->mem.owner;
623 assert(dev);
624
625 s = XLNX_VERSAL_EFUSE_CTRL(dev);
626
627 if (addr != A_WR_LOCK && s->regs[R_WR_LOCK]) {
>>> CID 1464071:  Resource leaks  (RESOURCE_LEAK)
>>> Failing to save or free storage allocated by 
>>> "object_get_canonical_path((Object *)s)" leaks it.
628 qemu_log_mask(LOG_GUEST_ERROR,
629   "%s[reg_0x%02lx]: Attempt to write
locked register.\n",
630   object_get_canonical_path(OBJECT(s)), (long)addr);
631 } else {
632 register_write_memory(opaque, addr, data, size);
633 }

You need to free the memory here. A good pattern is how it's
done in xlnx-zynqmp-can.c with g_autofree:

if (ARRAY_FIELD_EX32(s->regs, SOFTWARE_RESET_REGISTER, SRST)) {
g_autofree char *path = object_get_canonical_path(OBJECT(s));

qemu_log_mask(LOG_GUEST_ERROR, "%s: Attempting to transfer data while"
  " data while controller is in reset mode.\n",
  path);
return false;
}

Could somebody send some followup patches that fix all of these,
please? (There are 10 coverity issues, covering probably all of
the places where object_get_canonical_path() is used in this series.)

thanks
-- PMM

Re: [PULL 11/20] nubus-device: add romfile property for loading declaration ROMs

[Peter Maydell]

On Wed, 29 Sept 2021 at 10:53, Laurent Vivier  wrote:
>
> From: Mark Cave-Ayland 
>
> The declaration ROM is located at the top-most address of the standard slot
> space.
>
> Signed-off-by: Mark Cave-Ayland 
> Reviewed-by: Laurent Vivier 
> Message-Id: <20210924073808.1041-12-mark.cave-ayl...@ilande.co.uk>
> Signed-off-by: Laurent Vivier 

Coverity spots a memory leak here: CID 1464062

> +name = g_strdup_printf("nubus-slot-%x-declaration-rom", nd->slot);
> +memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, size,
> +   &error_abort);
> +ret = load_image_mr(path, &nd->decl_rom);
> +g_free(path);
> +if (ret < 0) {
> +error_setg(errp, "could not load romfile \"%s\"", nd->romfile);
> +return;
> +}
> +memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - size,
> +&nd->decl_rom);

'name' is allocated, but never freed.

-- PMM

Re: [PULL 03/20] nubus-device: expose separate super slot memory region

[Peter Maydell]

On Wed, 29 Sept 2021 at 10:49, Laurent Vivier  wrote:
>
> From: Mark Cave-Ayland 
>
> According to "Designing Cards and Drivers for the Macintosh Family" each 
> physical
> nubus slot can access 2 separate address ranges: a super slot memory region 
> which
> is 256MB and a standard slot memory region which is 16MB.
>
> Currently a Nubus device uses the physical slot number to determine whether 
> it is
> using a standard slot memory region or a super slot memory region rather than
> exposing both memory regions for use as required.


> +/* Super */
> +slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;

Hi; Coverity thinks this multiply might overflow, because
we're calculating a hw_addr (64-bits) but the multiply is only
done at 32-bits. Adding an explicit cast or using 'ULL' in the
constant #define rather than just 'U' would fix this.
This is CID 1464070.

> +
> +name = g_strdup_printf("nubus-super-slot-%x", nd->slot);
> +memory_region_init(&nd->super_slot_mem, OBJECT(dev), name,
> +   NUBUS_SUPER_SLOT_SIZE);
> +memory_region_add_subregion(&nubus->super_slot_io, slot_offset,
> +&nd->super_slot_mem);
> +g_free(name);
> +
> +/* Normal */
> +slot_offset = nd->slot * NUBUS_SLOT_SIZE;

Same with this one.

thanks
-- PMM

Re: [PULL 33/44] target/ppc: Fix 64-bit decrementer

[Peter Maydell]

On Thu, 30 Sept 2021 at 06:44, David Gibson  wrote:
>
> From: Cédric Le Goater 
>
> The current way the mask is built can overflow with a 64-bit decrementer.
> Use sextract64() to extract the signed values and remove the logic to
> handle negative values which has become useless.
>
> Cc: Luis Fernando Fujita Pires 
> Fixes: a8dafa525181 ("target/ppc: Implement large decrementer support for 
> TCG")
> Signed-off-by: Cédric Le Goater 
> Message-Id: <20210920061203.989563-5-...@kaod.org>
> Reviewed-by: Luis Pires 
> Signed-off-by: David Gibson 

Hi; Coverity complains about dead code here (CID 1464061):



>   * On MSB edge based DEC implementations the MSB going from 0 -> 1 
> triggers
>   * an edge interrupt, so raise it here too.
>   */
> -if ((value < 3) ||
> -((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && negative) ||
> -((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && negative
> -  && !(decr & (1ULL << (nr_bits - 1) {
> +if ((signed_value < 3) ||
> +((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && signed_value < 0) ||
> +((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && signed_value < 0
> +  && signed_decr >= 0)) {
>  (*raise_excp)(cpu);
>  return;
>  }

If signed_value < 3 then the first clause of the || evaluates as true,
and so we will definitely take the if() clause. So if we're evaluating
the second operand to the || then we know that signed_value > 3,
which means that 'signed_value < 0' is always false and in turn that
neither of the other two '||' options can be true. The whole expression
is equivalent to just "if (signed_value < 3)".

What was intended here? If this was supposed to be a no-behaviour-change
commit (apart from fixing the 64-bit case) then the condition should
have stayed as "(value < 3)", I think.

thanks
-- PMM

[PATCH 00/12] macfb: fixes for booting MacOS

[Mark Cave-Ayland]

This is the next set of patches to allow users to boot MacOS in QEMU's
q800 machine.

Patches 1 to 3 are fixes for existing bugs that I discovered whilst
developing the remainder of the patchset whilst patch 4 simplifies the
registration of the framebuffer RAM.

Patch 5 adds trace events to the framebuffer register accesses. The
framebuffer registers are not officially documented, so the macfb
device changes here are based upon reading of Linux/NetBSD source code,
using gdbstub during the MacOS toolbox ROM initialisation, and changing
the framebuffer size/depth within MacOS itself with these trace events
enabled.

Patches 6 and 7 implement the mode sense logic documented in Apple
Technical Note HW26 "Macintosh Quadra Built-In Video" and configure the
default display type to be VGA.

Patch 8 implements the common monitor modes used for VGA at 640x480 and
800x600 for 1, 2, 4, 8 and 24-bit depths and also the Apple 21" color
monitor at 1152x870 with 8-bit depth.

Patches 9 and 10 fix up errors in the 1-bit and 24-bit pixel encodings
discovered when testing these color depths in MacOS.

Patch 11 adds a timer to implement the 60.15Hz VBL interrupt which is
required for MacOS to process mouse movements, whilst patch 12 wires the
same interrupt to a dedicated pin on VIA2 reserved for the video
interrupt on the Quadra 800.

Signed-off-by: Mark Cave-Ayland 


Mark Cave-Ayland (12):
  macfb: handle errors that occur during realize
  macfb: fix invalid object reference in macfb_common_realize()
  macfb: fix overflow of color_palette array
  macfb: use memory_region_init_ram() in macfb_common_realize() for the
framebuffer
  macfb: add trace events for reading and writing the control registers
  macfb: implement mode sense to allow display type to be detected
  macfb: add qdev property to specify display type
  macfb: add common monitor modes supported by the MacOS toolbox ROM
  macfb: fix up 1-bit pixel encoding
  macfb: fix 24-bit RGB pixel encoding
  macfb: add vertical blank interrupt
  q800: wire macfb IRQ to separate video interrupt on VIA2

 hw/display/macfb.c | 348 +++--
 hw/display/trace-events|   7 +
 hw/m68k/q800.c |  23 ++-
 include/hw/display/macfb.h |  43 +
 4 files changed, 397 insertions(+), 24 deletions(-)

-- 
2.20.1

[PATCH 02/12] macfb: fix invalid object reference in macfb_common_realize()

[Mark Cave-Ayland]

During realize memory_region_init_ram_nomigrate() is used to initialise the RAM
memory region used for the framebuffer but the owner object reference is
incorrect since MacFbState is a typedef and not a QOM type.

Change the memory region owner to be the corresponding DeviceState to fix the
issue and prevent random crashes during macfb_common_realize().

Signed-off-by: Mark Cave-Ayland 
---
 hw/display/macfb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 2b747a8de8..815870f2fe 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -365,7 +365,7 @@ static void macfb_common_realize(DeviceState *dev, 
MacfbState *s, Error **errp)
 memory_region_init_io(&s->mem_ctrl, OBJECT(dev), &macfb_ctrl_ops, s,
   "macfb-ctrl", 0x1000);
 
-memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(s), "macfb-vram",
+memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(dev), "macfb-vram",
  MACFB_VRAM_SIZE, errp);
 s->vram = memory_region_get_ram_ptr(&s->mem_vram);
 s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
-- 
2.20.1

[PATCH 01/12] macfb: handle errors that occur during realize

[Mark Cave-Ayland]

Make sure any errors that occur within the macfb realize chain are detected
and handled correctly to prevent crashes and to ensure that error messages are
reported back to the user.

Signed-off-by: Mark Cave-Ayland 
---
 hw/display/macfb.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 76808b69cc..2b747a8de8 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error 
**errp)
 MacfbState *ms = &s->macfb;
 
 macfb_common_realize(dev, ms, errp);
+if (*errp) {
+return;
+}
+
 sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_ctrl);
 sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_vram);
 }
@@ -391,8 +395,15 @@ static void macfb_nubus_realize(DeviceState *dev, Error 
**errp)
 MacfbState *ms = &s->macfb;
 
 ndc->parent_realize(dev, errp);
+if (*errp) {
+return;
+}
 
 macfb_common_realize(dev, ms, errp);
+if (*errp) {
+return;
+}
+
 memory_region_add_subregion(&nd->slot_mem, DAFB_BASE, &ms->mem_ctrl);
 memory_region_add_subregion(&nd->slot_mem, VIDEO_BASE, &ms->mem_vram);
 }
-- 
2.20.1

[PATCH 05/12] macfb: add trace events for reading and writing the control registers

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland 
---
 hw/display/macfb.c  | 8 +++-
 hw/display/trace-events | 4 
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index e86f64..62c2727a5b 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -20,6 +20,7 @@
 #include "qapi/error.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
+#include "trace.h"
 
 #define VIDEO_BASE 0x1000
 #define DAFB_BASE  0x0080
@@ -289,7 +290,10 @@ static uint64_t macfb_ctrl_read(void *opaque,
 hwaddr addr,
 unsigned int size)
 {
-return 0;
+uint64_t val = 0;
+
+trace_macfb_ctrl_read(addr, val, size);
+return val;
 }
 
 static void macfb_ctrl_write(void *opaque,
@@ -312,6 +316,8 @@ static void macfb_ctrl_write(void *opaque,
 }
 break;
 }
+
+trace_macfb_ctrl_write(addr, val, size);
 }
 
 static const MemoryRegionOps macfb_ctrl_ops = {
diff --git a/hw/display/trace-events b/hw/display/trace-events
index f03f6655bc..be1353e8e7 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -167,3 +167,7 @@ sm501_disp_ctrl_read(uint32_t addr, uint32_t val) 
"addr=0x%x, val=0x%x"
 sm501_disp_ctrl_write(uint32_t addr, uint32_t val) "addr=0x%x, val=0x%x"
 sm501_2d_engine_read(uint32_t addr, uint32_t val) "addr=0x%x, val=0x%x"
 sm501_2d_engine_write(uint32_t addr, uint32_t val) "addr=0x%x, val=0x%x"
+
+# macfb.c
+macfb_ctrl_read(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " 
value 0x%"PRIx64 " size %d"
+macfb_ctrl_write(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " 
value 0x%"PRIx64 " size %d"
-- 
2.20.1

[PATCH 08/12] macfb: add common monitor modes supported by the MacOS toolbox ROM

[Mark Cave-Ayland]

The monitor modes table is found by experimenting with the Monitors Control
Panel in MacOS and analysing the reads/writes. From this it can be found that
the mode is controlled by writes to the DAFB_MODE_CTRL1 and DAFB_MODE_CTRL2
registers.

Implement the first block of DAFB registers as a register array including the
existing sense register, the newly discovered control registers above, and also
the DAFB_MODE_VADDR1 and DAFB_MODE_VADDR2 registers which are used by NetBSD to
determine the current video mode.

These experiments also show that the offset of the start of video RAM and the
stride can change depending upon the monitor mode, so update 
macfb_draw_graphic()
and both the BI_MAC_VADDR and BI_MAC_VROW bootinfo for the q800 machine
accordingly.

Finally update macfb_common_realize() so that only the resolution and depth
supported by the display type can be specified on the command line.

Signed-off-by: Mark Cave-Ayland 
---
 hw/display/macfb.c | 125 -
 hw/display/trace-events|   1 +
 hw/m68k/q800.c |  11 ++--
 include/hw/display/macfb.h |  16 -
 4 files changed, 132 insertions(+), 21 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 023d1f0cd1..6a69334565 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -22,12 +22,16 @@
 #include "migration/vmstate.h"
 #include "trace.h"
 
-#define VIDEO_BASE 0x1000
+#define VIDEO_BASE 0x0
 #define DAFB_BASE  0x0080
 
 #define MACFB_PAGE_SIZE 4096
 #define MACFB_VRAM_SIZE (4 * MiB)
 
+#define DAFB_MODE_VADDR10x0
+#define DAFB_MODE_VADDR20x4
+#define DAFB_MODE_CTRL1 0x8
+#define DAFB_MODE_CTRL2 0xc
 #define DAFB_MODE_SENSE 0x1c
 #define DAFB_RESET  0x200
 #define DAFB_LUT0x213
@@ -89,6 +93,22 @@ static MacFbSense macfb_sense_table[] = {
 { MACFB_DISPLAY_SVGA, 0x7, 0x5 },
 };
 
+static MacFbMode macfb_mode_table[] = {
+{ MACFB_DISPLAY_VGA, 1, 0x100, 0x71e, 640, 480, 0x400, 0x1000 },
+{ MACFB_DISPLAY_VGA, 2, 0x100, 0x70e, 640, 480, 0x400, 0x1000 },
+{ MACFB_DISPLAY_VGA, 4, 0x100, 0x706, 640, 480, 0x400, 0x1000 },
+{ MACFB_DISPLAY_VGA, 8, 0x100, 0x702, 640, 480, 0x400, 0x1000 },
+{ MACFB_DISPLAY_VGA, 24, 0x100, 0x7ff, 640, 480, 0x1000, 0x1000 },
+{ MACFB_DISPLAY_VGA, 1, 0xd0 , 0x70e, 800, 600, 0x340, 0xe00 },
+{ MACFB_DISPLAY_VGA, 2, 0xd0 , 0x706, 800, 600, 0x340, 0xe00 },
+{ MACFB_DISPLAY_VGA, 4, 0xd0 , 0x702, 800, 600, 0x340, 0xe00 },
+{ MACFB_DISPLAY_VGA, 8, 0xd0,  0x700, 800, 600, 0x340, 0xe00 },
+{ MACFB_DISPLAY_VGA, 24, 0x340, 0x100, 800, 600, 0xd00, 0xe00 },
+{ MACFB_DISPLAY_APPLE_21_COLOR, 1, 0x90, 0x506, 1152, 870, 0x240, 0x80 },
+{ MACFB_DISPLAY_APPLE_21_COLOR, 2, 0x90, 0x502, 1152, 870, 0x240, 0x80 },
+{ MACFB_DISPLAY_APPLE_21_COLOR, 4, 0x90, 0x500, 1152, 870, 0x240, 0x80 },
+{ MACFB_DISPLAY_APPLE_21_COLOR, 8, 0x120, 0x5ff, 1152, 870, 0x480, 0x80 },
+};
 
 typedef void macfb_draw_line_func(MacfbState *s, uint8_t *d, uint32_t addr,
   int width);
@@ -246,7 +266,7 @@ static void macfb_draw_graphic(MacfbState *s)
 ram_addr_t page;
 uint32_t v = 0;
 int y, ymin;
-int macfb_stride = (s->depth * s->width + 7) / 8;
+int macfb_stride = s->mode->stride;
 macfb_draw_line_func *macfb_draw_line;
 
 switch (s->depth) {
@@ -278,7 +298,7 @@ static void macfb_draw_graphic(MacfbState *s)
  DIRTY_MEMORY_VGA);
 
 ymin = -1;
-page = 0;
+page = s->mode->offset;
 for (y = 0; y < s->height; y++, page += macfb_stride) {
 if (macfb_check_dirty(s, snap, page, macfb_stride)) {
 uint8_t *data_display;
@@ -322,25 +342,26 @@ static uint32_t macfb_sense_read(MacfbState *s)
 sense = 0;
 if (!(macfb_sense->ext_sense & 1)) {
 /* Pins 7-4 together */
-if (~s->sense & 3) {
-sense = (~s->sense & 7) | 3;
+if (~s->regs[DAFB_MODE_SENSE >> 2] & 3) {
+sense = (~s->regs[DAFB_MODE_SENSE >> 2] & 7) | 3;
 }
 }
 if (!(macfb_sense->ext_sense & 2)) {
 /* Pins 10-7 together */
-if (~s->sense & 6) {
-sense = (~s->sense & 7) | 6;
+if (~s->regs[DAFB_MODE_SENSE >> 2] & 6) {
+sense = (~s->regs[DAFB_MODE_SENSE >> 2] & 7) | 6;
 }
 }
 if (!(macfb_sense->ext_sense & 4)) {
 /* Pins 4-10 together */
-if (~s->sense & 5) {
-sense = (~s->sense & 7) | 5;
+if (~s->regs[DAFB_MODE_SENSE >> 2] & 5) {
+sense = (~s->regs[DAFB_MODE_SENSE >> 2] & 7) | 5;
 }
 }
 } else {
 /* Normal sense */
-sense = (~macfb_sense->sense & 7) | (~s->sense & 7);
+sense = (~macfb_sense->sense & 7) |
+(~s->regs[DAFB_MODE_SENSE >> 2] & 7);
 }
 
 trace_macfb_sense_read(sense

[PATCH 04/12] macfb: use memory_region_init_ram() in macfb_common_realize() for the framebuffer

[Mark Cave-Ayland]

Currently macfb_common_realize() defines the framebuffer RAM memory region as
being non-migrateable but then immediately registers it for migration. Replace
memory_region_init_ram_nomigrate() with memory_region_init_ram() which is 
clearer
and does exactly the same thing.

Signed-off-by: Mark Cave-Ayland 
---
 hw/display/macfb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index f4e789d0d7..e86f64 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -368,11 +368,10 @@ static void macfb_common_realize(DeviceState *dev, 
MacfbState *s, Error **errp)
 memory_region_init_io(&s->mem_ctrl, OBJECT(dev), &macfb_ctrl_ops, s,
   "macfb-ctrl", 0x1000);
 
-memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(dev), "macfb-vram",
- MACFB_VRAM_SIZE, errp);
+memory_region_init_ram(&s->mem_vram, OBJECT(dev), "macfb-vram",
+   MACFB_VRAM_SIZE, errp);
 s->vram = memory_region_get_ram_ptr(&s->mem_vram);
 s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
-vmstate_register_ram(&s->mem_vram, dev);
 memory_region_set_coalescing(&s->mem_vram);
 }
 
-- 
2.20.1

[PATCH 09/12] macfb: fix up 1-bit pixel encoding

[Mark Cave-Ayland]

The MacOS driver expects the RGB values for the pixel to be in entries 0 and 1
of the colour palette.

Signed-off-by: Mark Cave-Ayland 
---
 hw/display/macfb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 6a69334565..0c9e181b9b 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -128,7 +128,9 @@ static void macfb_draw_line1(MacfbState *s, uint8_t *d, 
uint32_t addr,
 for (x = 0; x < width; x++) {
 int bit = x & 7;
 int idx = (macfb_read_byte(s, addr) >> (7 - bit)) & 1;
-r = g = b  = ((1 - idx) << 7);
+r = s->color_palette[idx * 3];
+g = s->color_palette[idx * 3 + 1];
+b = s->color_palette[idx * 3 + 2];
 addr += (bit == 7);
 
 *(uint32_t *)d = rgb_to_pixel32(r, g, b);
-- 
2.20.1

[PATCH 03/12] macfb: fix overflow of color_palette array

[Mark Cave-Ayland]

The palette_current index counter has a maximum size of 256 * 3 to cover a full
color palette of 256 RGB entries. Linux assumes that the palette_current index
wraps back around to zero after writing 256 RGB entries so ensure that
palette_current is reset at this point to prevent data corruption within
MacfbState.

Signed-off-by: Mark Cave-Ayland 
---
 hw/display/macfb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 815870f2fe..f4e789d0d7 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -307,6 +307,9 @@ static void macfb_ctrl_write(void *opaque,
 if (s->palette_current % 3) {
 macfb_invalidate_display(s);
 }
+if (s->palette_current >= sizeof(s->color_palette)) {
+s->palette_current = 0;
+}
 break;
 }
 }
-- 
2.20.1

[PATCH 07/12] macfb: add qdev property to specify display type

[Mark Cave-Ayland]

Since the available resolutions and colour depths are determined by the attached
display type, add a qdev property to allow the display type to be specified.

The main resolutions of interest are high resolution 1152x870 with 8-bit colour
and SVGA resolution up to 800x600 with 32-bit colour so update the q800 machine
to allow high resolution mode if specified and otherwise fall back to SVGA.

Signed-off-by: Mark Cave-Ayland 
---
 hw/display/macfb.c | 6 +-
 hw/m68k/q800.c | 5 +
 include/hw/display/macfb.h | 1 +
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 5c95aa4a11..023d1f0cd1 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -316,7 +316,7 @@ static uint32_t macfb_sense_read(MacfbState *s)
 MacFbSense *macfb_sense;
 uint8_t sense;
 
-macfb_sense = &macfb_sense_table[MACFB_DISPLAY_VGA];
+macfb_sense = &macfb_sense_table[s->type];
 if (macfb_sense->sense == 0x7) {
 /* Extended sense */
 sense = 0;
@@ -545,6 +545,8 @@ static Property macfb_sysbus_properties[] = {
 DEFINE_PROP_UINT32("width", MacfbSysBusState, macfb.width, 640),
 DEFINE_PROP_UINT32("height", MacfbSysBusState, macfb.height, 480),
 DEFINE_PROP_UINT8("depth", MacfbSysBusState, macfb.depth, 8),
+DEFINE_PROP_UINT8("display", MacfbSysBusState, macfb.type,
+  MACFB_DISPLAY_VGA),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -552,6 +554,8 @@ static Property macfb_nubus_properties[] = {
 DEFINE_PROP_UINT32("width", MacfbNubusState, macfb.width, 640),
 DEFINE_PROP_UINT32("height", MacfbNubusState, macfb.height, 480),
 DEFINE_PROP_UINT8("depth", MacfbNubusState, macfb.depth, 8),
+DEFINE_PROP_UINT8("display", MacfbNubusState, macfb.type,
+  MACFB_DISPLAY_VGA),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 09b3366024..5223b880bc 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -421,6 +421,11 @@ static void q800_init(MachineState *machine)
 qdev_prop_set_uint32(dev, "width", graphic_width);
 qdev_prop_set_uint32(dev, "height", graphic_height);
 qdev_prop_set_uint8(dev, "depth", graphic_depth);
+if (graphic_width == 1152 && graphic_height == 870 && graphic_depth == 8) {
+qdev_prop_set_uint8(dev, "display", MACFB_DISPLAY_APPLE_21_COLOR);
+} else {
+qdev_prop_set_uint8(dev, "display", MACFB_DISPLAY_VGA);
+}
 qdev_realize_and_unref(dev, BUS(nubus), &error_fatal);
 
 cs = CPU(cpu);
diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
index febf4ce0e8..e95a97ebdc 100644
--- a/include/hw/display/macfb.h
+++ b/include/hw/display/macfb.h
@@ -46,6 +46,7 @@ typedef struct MacfbState {
 uint8_t color_palette[256 * 3];
 uint32_t width, height; /* in pixels */
 uint8_t depth;
+uint8_t type;
 
 uint32_t sense;
 } MacfbState;
-- 
2.20.1

[PATCH 11/12] macfb: add vertical blank interrupt

[Mark Cave-Ayland]

The MacOS driver expects a 60.15Hz vertical blank interrupt to be generated by
the framebuffer which in turn schedules the mouse driver via the Vertical 
Retrace
Manager.

Signed-off-by: Mark Cave-Ayland 
---
 hw/display/macfb.c | 81 ++
 include/hw/display/macfb.h |  8 
 2 files changed, 89 insertions(+)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 29f6ad8eba..60a203e67b 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -33,9 +33,16 @@
 #define DAFB_MODE_CTRL1 0x8
 #define DAFB_MODE_CTRL2 0xc
 #define DAFB_MODE_SENSE 0x1c
+#define DAFB_INTR_MASK  0x104
+#define DAFB_INTR_STAT  0x108
+#define DAFB_INTR_CLEAR 0x10c
 #define DAFB_RESET  0x200
 #define DAFB_LUT0x213
 
+#define DAFB_INTR_VBL   0x4
+
+/* Vertical Blank period (60.15Hz) */
+#define DAFB_INTR_VBL_PERIOD_NS 16625800
 
 /*
  * Quadra sense codes taken from Apple Technical Note HW26:
@@ -449,6 +456,32 @@ static void macfb_update_display(void *opaque)
 macfb_draw_graphic(s);
 }
 
+static void macfb_update_irq(MacfbState *s)
+{
+uint32_t irq_state = s->irq_state & s->irq_mask;
+
+if (irq_state) {
+qemu_irq_raise(s->irq);
+} else {
+qemu_irq_lower(s->irq);
+}
+}
+
+static void macfb_vbl_timer(void *opaque)
+{
+MacfbState *s = opaque;
+int64_t next_vbl;
+
+s->irq_state |= DAFB_INTR_VBL;
+macfb_update_irq(s);
+
+/* 60 Hz irq */
+next_vbl = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+DAFB_INTR_VBL_PERIOD_NS) /
+DAFB_INTR_VBL_PERIOD_NS * DAFB_INTR_VBL_PERIOD_NS;
+timer_mod(s->vbl_timer, next_vbl);
+}
+
 static void macfb_reset(MacfbState *s)
 {
 int i;
@@ -477,6 +510,9 @@ static uint64_t macfb_ctrl_read(void *opaque,
 case DAFB_MODE_CTRL2:
 val = s->regs[addr >> 2];
 break;
+case DAFB_INTR_STAT:
+val = s->irq_state;
+break;
 case DAFB_MODE_SENSE:
 val = macfb_sense_read(s);
 break;
@@ -492,6 +528,8 @@ static void macfb_ctrl_write(void *opaque,
  unsigned int size)
 {
 MacfbState *s = opaque;
+int64_t next_vbl;
+
 switch (addr) {
 case DAFB_MODE_VADDR1:
 case DAFB_MODE_VADDR2:
@@ -507,8 +545,25 @@ static void macfb_ctrl_write(void *opaque,
 case DAFB_MODE_SENSE:
 macfb_sense_write(s, val);
 break;
+case DAFB_INTR_MASK:
+s->irq_mask = val;
+if (val & DAFB_INTR_VBL) {
+next_vbl = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+DAFB_INTR_VBL_PERIOD_NS) /
+DAFB_INTR_VBL_PERIOD_NS * DAFB_INTR_VBL_PERIOD_NS;
+timer_mod(s->vbl_timer, next_vbl);
+} else {
+timer_del(s->vbl_timer);
+}
+break;
+case DAFB_INTR_CLEAR:
+s->irq_state &= ~DAFB_INTR_VBL;
+macfb_update_irq(s);
+break;
 case DAFB_RESET:
 s->palette_current = 0;
+s->irq_state &= ~DAFB_INTR_VBL;
+macfb_update_irq(s);
 break;
 case DAFB_LUT:
 s->color_palette[s->palette_current++] = val;
@@ -586,6 +641,7 @@ static void macfb_common_realize(DeviceState *dev, 
MacfbState *s, Error **errp)
 s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
 memory_region_set_coalescing(&s->mem_vram);
 
+s->vbl_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, macfb_vbl_timer, s);
 macfb_update_mode(s);
 }
 
@@ -601,6 +657,16 @@ static void macfb_sysbus_realize(DeviceState *dev, Error 
**errp)
 
 sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_ctrl);
 sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_vram);
+
+qdev_init_gpio_out(dev, &ms->irq, 1);
+}
+
+static void macfb_nubus_set_irq(void *opaque, int n, int level)
+{
+MacfbNubusState *s = NUBUS_MACFB(opaque);
+NubusDevice *nd = NUBUS_DEVICE(s);
+
+nubus_set_irq(nd, level);
 }
 
 static void macfb_nubus_realize(DeviceState *dev, Error **errp)
@@ -622,6 +688,19 @@ static void macfb_nubus_realize(DeviceState *dev, Error 
**errp)
 
 memory_region_add_subregion(&nd->slot_mem, DAFB_BASE, &ms->mem_ctrl);
 memory_region_add_subregion(&nd->slot_mem, VIDEO_BASE, &ms->mem_vram);
+
+ms->irq = qemu_allocate_irq(macfb_nubus_set_irq, s, 0);
+}
+
+static void macfb_nubus_unrealize(DeviceState *dev)
+{
+MacfbNubusState *s = NUBUS_MACFB(dev);
+MacfbNubusDeviceClass *ndc = NUBUS_MACFB_GET_CLASS(dev);
+MacfbState *ms = &s->macfb;
+
+ndc->parent_unrealize(dev);
+
+qemu_free_irq(ms->irq);
 }
 
 static void macfb_sysbus_reset(DeviceState *d)
@@ -672,6 +751,8 @@ static void macfb_nubus_class_init(ObjectClass *klass, void 
*data)
 
 device_class_set_parent_realize(dc, macfb_nubus_realize,
 &ndc->parent_realize);
+device_class_set_parent_unrealize(dc, macfb_nubus_unrealize,
+  &ndc->parent_unrealize);
 dc->desc = "Nubus Macintosh framebuffer";
 

[PATCH 12/12] q800: wire macfb IRQ to separate video interrupt on VIA2

[Mark Cave-Ayland]

Whilst the in-built Quadra 800 framebuffer exists within the Nubus address
space for slot 9, it has its own dedicated interrupt on VIA2. Force the
macfb device to occupy slot 9 in the q800 machine and wire its IRQ to the
separate video interrupt since this is what is expected by the MacOS
interrupt handler.

Signed-off-by: Mark Cave-Ayland 
---
 hw/m68k/q800.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index df3fd3711e..fd4855047e 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -407,8 +407,10 @@ static void q800_init(MachineState *machine)
 MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE);
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
 MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE);
-
-for (i = 0; i < VIA2_NUBUS_IRQ_NB; i++) {
+qdev_connect_gpio_out(dev, 9,
+  qdev_get_gpio_in_named(via2_dev, "nubus-irq",
+  VIA2_NUBUS_IRQ_INTVIDEO));
+for (i = 1; i < VIA2_NUBUS_IRQ_NB; i++) {
 qdev_connect_gpio_out(dev, 9 + i,
   qdev_get_gpio_in_named(via2_dev, "nubus-irq",
  VIA2_NUBUS_IRQ_9 + i));
@@ -419,6 +421,7 @@ static void q800_init(MachineState *machine)
 /* framebuffer in nubus slot #9 */
 
 dev = qdev_new(TYPE_NUBUS_MACFB);
+qdev_prop_set_uint32(dev, "slot", 9);
 qdev_prop_set_uint32(dev, "width", graphic_width);
 qdev_prop_set_uint32(dev, "height", graphic_height);
 qdev_prop_set_uint8(dev, "depth", graphic_depth);
-- 
2.20.1

[PATCH 06/12] macfb: implement mode sense to allow display type to be detected

[Mark Cave-Ayland]

The MacOS toolbox ROM uses the monitor sense to detect the display type and then
offer a fixed set of resolutions and colour depths accordingly. Implement the
monitor sense using information found in Apple Technical Note HW26: "Macintosh
Quadra Built-In Video" along with some local experiments.

Since the default configuration is 640 x 480 with 8-bit colour then hardcode
the sense register to return MACFB_DISPLAY_VGA for now.

Signed-off-by: Mark Cave-Ayland 
---
 hw/display/macfb.c | 117 -
 hw/display/trace-events|   2 +
 include/hw/display/macfb.h |  20 +++
 3 files changed, 137 insertions(+), 2 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 62c2727a5b..5c95aa4a11 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -28,8 +28,66 @@
 #define MACFB_PAGE_SIZE 4096
 #define MACFB_VRAM_SIZE (4 * MiB)
 
-#define DAFB_RESET  0x200
-#define DAFB_LUT0x213
+#define DAFB_MODE_SENSE 0x1c
+#define DAFB_RESET  0x200
+#define DAFB_LUT0x213
+
+
+/*
+ * Quadra sense codes taken from Apple Technical Note HW26:
+ * "Macintosh Quadra Built-In Video". The sense codes and
+ * extended sense codes have different meanings:
+ *
+ * Sense:
+ *bit 2: SENSE2 (pin 10)
+ *bit 1: SENSE1 (pin 7)
+ *bit 0: SENSE0 (pin 4)
+ *
+ * 0 = pin tied to ground
+ * 1 = pin unconnected
+ *
+ * Extended Sense:
+ *bit 2: pins 4-10
+ *bit 1: pins 10-7
+ *bit 0: pins 7-4
+ *
+ * 0 = pins tied together
+ * 1 = pins unconnected
+ *
+ * Reads from the sense register appear to be active low, i.e. a 1 indicates
+ * that the pin is tied to ground, a 0 indicates the pin is disconnected.
+ *
+ * Writes to the sense register appear to activate pulldowns i.e. a 1 enables
+ * a pulldown on a particular pin.
+ *
+ * The MacOS toolbox appears to use a series of reads and writes to first
+ * determine if extended sense is to be used, and then check which pins are
+ * tied together in order to determine the display type.
+ */
+
+typedef struct MacFbSense {
+uint8_t type;
+uint8_t sense;
+uint8_t ext_sense;
+} MacFbSense;
+
+static MacFbSense macfb_sense_table[] = {
+{ MACFB_DISPLAY_APPLE_21_COLOR, 0x0, 0 },
+{ MACFB_DISPLAY_APPLE_PORTRAIT, 0x1, 0 },
+{ MACFB_DISPLAY_APPLE_12_RGB, 0x2, 0 },
+{ MACFB_DISPLAY_APPLE_2PAGE_MONO, 0x3, 0 },
+{ MACFB_DISPLAY_NTSC_UNDERSCAN, 0x4, 0 },
+{ MACFB_DISPLAY_NTSC_OVERSCAN, 0x4, 0 },
+{ MACFB_DISPLAY_APPLE_12_MONO, 0x6, 0 },
+{ MACFB_DISPLAY_APPLE_13_RGB, 0x6, 0 },
+{ MACFB_DISPLAY_16_COLOR, 0x7, 0x3 },
+{ MACFB_DISPLAY_PAL1_UNDERSCAN, 0x7, 0x0 },
+{ MACFB_DISPLAY_PAL1_OVERSCAN, 0x7, 0x0 },
+{ MACFB_DISPLAY_PAL2_UNDERSCAN, 0x7, 0x6 },
+{ MACFB_DISPLAY_PAL2_OVERSCAN, 0x7, 0x6 },
+{ MACFB_DISPLAY_VGA, 0x7, 0x5 },
+{ MACFB_DISPLAY_SVGA, 0x7, 0x5 },
+};
 
 
 typedef void macfb_draw_line_func(MacfbState *s, uint8_t *d, uint32_t addr,
@@ -253,6 +311,50 @@ static void macfb_invalidate_display(void *opaque)
 memory_region_set_dirty(&s->mem_vram, 0, MACFB_VRAM_SIZE);
 }
 
+static uint32_t macfb_sense_read(MacfbState *s)
+{
+MacFbSense *macfb_sense;
+uint8_t sense;
+
+macfb_sense = &macfb_sense_table[MACFB_DISPLAY_VGA];
+if (macfb_sense->sense == 0x7) {
+/* Extended sense */
+sense = 0;
+if (!(macfb_sense->ext_sense & 1)) {
+/* Pins 7-4 together */
+if (~s->sense & 3) {
+sense = (~s->sense & 7) | 3;
+}
+}
+if (!(macfb_sense->ext_sense & 2)) {
+/* Pins 10-7 together */
+if (~s->sense & 6) {
+sense = (~s->sense & 7) | 6;
+}
+}
+if (!(macfb_sense->ext_sense & 4)) {
+/* Pins 4-10 together */
+if (~s->sense & 5) {
+sense = (~s->sense & 7) | 5;
+}
+}
+} else {
+/* Normal sense */
+sense = (~macfb_sense->sense & 7) | (~s->sense & 7);
+}
+
+trace_macfb_sense_read(sense);
+return sense;
+}
+
+static void macfb_sense_write(MacfbState *s, uint32_t val)
+{
+s->sense = val;
+
+trace_macfb_sense_write(val);
+return;
+}
+
 static void macfb_update_display(void *opaque)
 {
 MacfbState *s = opaque;
@@ -290,8 +392,15 @@ static uint64_t macfb_ctrl_read(void *opaque,
 hwaddr addr,
 unsigned int size)
 {
+MacfbState *s = opaque;
 uint64_t val = 0;
 
+switch (addr) {
+case DAFB_MODE_SENSE:
+val = macfb_sense_read(s);
+break;
+}
+
 trace_macfb_ctrl_read(addr, val, size);
 return val;
 }
@@ -303,6 +412,9 @@ static void macfb_ctrl_write(void *opaque,
 {
 MacfbState *s = opaque;
 switch (addr) {
+case DAFB_MODE_SENSE:
+macfb_sense_write(s, val);
+break;
 case DAFB_RESET:
 s->palette_current = 0;
 break;
@@ -343,6 +455,7

[PATCH 10/12] macfb: fix 24-bit RGB pixel encoding

[Mark Cave-Ayland]

According to Apple Technical Note HW26: "Macintosh Quadra Built-In Video" the
in-built framebuffer encodes each 24-bit pixel into 4 bytes. Adjust the 24-bit
RGB pixel encoding accordingly which agrees with the encoding expected by MacOS
when changing into 24-bit colour mode.

Signed-off-by: Mark Cave-Ayland 
---
 hw/display/macfb.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 0c9e181b9b..29f6ad8eba 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -224,10 +224,10 @@ static void macfb_draw_line24(MacfbState *s, uint8_t *d, 
uint32_t addr,
 int x;
 
 for (x = 0; x < width; x++) {
-r = macfb_read_byte(s, addr);
-g = macfb_read_byte(s, addr + 1);
-b = macfb_read_byte(s, addr + 2);
-addr += 3;
+r = macfb_read_byte(s, addr + 1);
+g = macfb_read_byte(s, addr + 2);
+b = macfb_read_byte(s, addr + 3);
+addr += 4;
 
 *(uint32_t *)d = rgb_to_pixel32(r, g, b);
 d += 4;
-- 
2.20.1

Re: [PATCH v12 16/16] machine: Make smp_parse return a boolean

[Markus Armbruster]

Paolo Bonzini  writes:

> On 01/10/21 19:15, Daniel P. Berrangé wrote:
>> On Fri, Oct 01, 2021 at 07:08:51PM +0200, Paolo Bonzini wrote:
>>> On 29/09/21 04:58, Yanan Wang wrote:
 @@ -933,8 +935,7 @@ static void machine_set_smp(Object *obj, Visitor *v, 
 const char *name,
return;
}
 -smp_parse(ms, config, errp);
 -if (*errp) {
 +if (!smp_parse(ms, config, errp)) {
qapi_free_SMPConfiguration(config);
}
}

>>>
>>> This is actually a leak, so I'm replacing this patch with
>> 
>> This patch isn't adding a leak, as there's no change in
>> control flow / exit paths.  AFAICT, the leak was introduced
>> in patch 15 instead, so the code below shoudl be squashed
>> into that, and this patch left as-is.
>
> Yes, even better make it a separate patch and fix the conflict in patch
> 15.  But I'm still not sure of the wisdom of this patch.
>
> At this point smp_parse has exactly one caller and it doesn't care about
> the return value.  The "return a boolean" rule adds some complexity (and
> a possibility for things to be wrong/inconsistent) to the function for
> the benefit of the callers.

Yes, but returning something is only a minor burden.  It also makes
success vs. failure obvious at a glance.

I'm not worrying about inconsistency anymore.  In a way, void functions
are an exception.  Many non-void functions return a distinct error value
on failure, like NULL.  The only kind of inconsistency I can remember
seeing in these functions is forgetting to set an error.  Can be screwed
up in a void function just as easily.

>  If there is only one caller, as is the case
> here or for virtual functions, the benefit can well be zero (this case)
> or negative (virtual functions).

Two small benefits here:

1. No need for ERRP_GUARD().

2. Conforms to the rules.  Rules are not laws, but let's stick to them
when it's as easy as it is here.

For what it's worth, GLib always advised users of GError to return a
value.  We chose to deviate for our Error, then spent nine years
learning how that leads to cumbersome code, leading us to:

commit e3fe3988d7851cac30abffae06d2f555ff7bee62
Author: Markus Armbruster 
Date:   Tue Jul 7 18:05:31 2020 +0200

error: Document Error API usage rules

This merely codifies existing practice, with one exception: the rule
advising against returning void, where existing practice is mixed.

When the Error API was created, we adopted the (unwritten) rule to
return void when the function returns no useful value on success,
unlike GError, which recommends to return true on success and false on
error then.

When a function returns a distinct error value, say false, a checked
call that passes the error up looks like

if (!frobnicate(..., errp)) {
handle the error...
}

When it returns void, we need

Error *err = NULL;

frobnicate(..., &err);
if (err) {
handle the error...
error_propagate(errp, err);
}

Not only is this more verbose, it also creates an Error object even
when @errp is null, &error_abort or &error_fatal.

People got tired of the additional boilerplate, and started to ignore
the unwritten rule.  The result is confusion among developers about
the preferred usage.

Make the rule advising against returning void official by putting it
in writing.  This will hopefully reduce confusion.

Update the examples accordingly.

The remainder of this series will update a substantial amount of code
to honor the rule.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Greg Kurz 
Message-Id: <20200707160613.848843-4-arm...@redhat.com>
[Tweak prose as per advice from Eric]

Re: [PATCH 01/12] macfb: handle errors that occur during realize

[BALATON Zoltan]

On Sat, 2 Oct 2021, Mark Cave-Ayland wrote:

Make sure any errors that occur within the macfb realize chain are detected
and handled correctly to prevent crashes and to ensure that error messages are
reported back to the user.

Signed-off-by: Mark Cave-Ayland 
---
hw/display/macfb.c | 11 +++
1 file changed, 11 insertions(+)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 76808b69cc..2b747a8de8 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c


There's one more in macfb_common_realize() after:

memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(s), "macfb-vram", 
MACFB_VRAM_SIZE, errp);

otherwise

Reviewed-by: BALATON Zoltan 



@@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error 
**errp)
MacfbState *ms = &s->macfb;

macfb_common_realize(dev, ms, errp);
+if (*errp) {
+return;
+}
+
sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_ctrl);
sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_vram);
}
@@ -391,8 +395,15 @@ static void macfb_nubus_realize(DeviceState *dev, Error 
**errp)
MacfbState *ms = &s->macfb;

ndc->parent_realize(dev, errp);
+if (*errp) {
+return;
+}

macfb_common_realize(dev, ms, errp);
+if (*errp) {
+return;
+}
+
memory_region_add_subregion(&nd->slot_mem, DAFB_BASE, &ms->mem_ctrl);
memory_region_add_subregion(&nd->slot_mem, VIDEO_BASE, &ms->mem_vram);
}

Re: [PATCH 02/12] macfb: fix invalid object reference in macfb_common_realize()

[BALATON Zoltan]

On Sat, 2 Oct 2021, Mark Cave-Ayland wrote:

During realize memory_region_init_ram_nomigrate() is used to initialise the RAM
memory region used for the framebuffer but the owner object reference is
incorrect since MacFbState is a typedef and not a QOM type.

Change the memory region owner to be the corresponding DeviceState to fix the
issue and prevent random crashes during macfb_common_realize().

Signed-off-by: Mark Cave-Ayland 


Reviewed-by: BALATON Zoltan 


---
hw/display/macfb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 2b747a8de8..815870f2fe 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -365,7 +365,7 @@ static void macfb_common_realize(DeviceState *dev, 
MacfbState *s, Error **errp)
memory_region_init_io(&s->mem_ctrl, OBJECT(dev), &macfb_ctrl_ops, s,
  "macfb-ctrl", 0x1000);

-memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(s), "macfb-vram",
+memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(dev), "macfb-vram",
 MACFB_VRAM_SIZE, errp);
s->vram = memory_region_get_ram_ptr(&s->mem_vram);
s->vram_bit_mask = MACFB_VRAM_SIZE - 1;

Re: [PATCH 04/12] macfb: use memory_region_init_ram() in macfb_common_realize() for the framebuffer

[BALATON Zoltan]

On Sat, 2 Oct 2021, Mark Cave-Ayland wrote:

Currently macfb_common_realize() defines the framebuffer RAM memory region as
being non-migrateable but then immediately registers it for migration. Replace
memory_region_init_ram_nomigrate() with memory_region_init_ram() which is 
clearer
and does exactly the same thing.

Signed-off-by: Mark Cave-Ayland 


Reviewed-by: BALATON Zoltan 


---
hw/display/macfb.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index f4e789d0d7..e86f64 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -368,11 +368,10 @@ static void macfb_common_realize(DeviceState *dev, 
MacfbState *s, Error **errp)
memory_region_init_io(&s->mem_ctrl, OBJECT(dev), &macfb_ctrl_ops, s,
  "macfb-ctrl", 0x1000);

-memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(dev), "macfb-vram",
- MACFB_VRAM_SIZE, errp);
+memory_region_init_ram(&s->mem_vram, OBJECT(dev), "macfb-vram",
+   MACFB_VRAM_SIZE, errp);
s->vram = memory_region_get_ram_ptr(&s->mem_vram);
s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
-vmstate_register_ram(&s->mem_vram, dev);
memory_region_set_coalescing(&s->mem_vram);
}

[PATCH 0/2] nubus: a couple of Coverity fixes

[Mark Cave-Ayland]

These patches fix a couple of issues found by Coverity in the recent nubus
patchset as reported by Peter.

Signed-off-by: Mark Cave-Ayland 


Mark Cave-Ayland (2):
  nubus.h: add ULL suffix to NUBUS_SUPER_SLOT_SIZE and
NUBUS_SUPER_SLOT_SIZE
  nubus-device: ensure that name is freed after use in
nubus_device_realize()

 hw/nubus/nubus-device.c  | 1 +
 include/hw/nubus/nubus.h | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH 2/2] nubus-device: ensure that name is freed after use in nubus_device_realize()

[Mark Cave-Ayland]

Coverity points out that there is memory leak because name is never freed after
use in nubus_device_realize().

Fixes: Coverity CID 1464062
Signed-off-by: Mark Cave-Ayland 
---
 hw/nubus/nubus-device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 0f1852f671..64f837e44d 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -78,6 +78,7 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 name = g_strdup_printf("nubus-slot-%x-declaration-rom", nd->slot);
 memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, size,
&error_abort);
+g_free(name);
 ret = load_image_mr(path, &nd->decl_rom);
 g_free(path);
 if (ret < 0) {
-- 
2.20.1

[PATCH 1/2] nubus.h: add ULL suffix to NUBUS_SUPER_SLOT_SIZE and NUBUS_SUPER_SLOT_SIZE

[Mark Cave-Ayland]

Coverity thinks that the slot_offset multiplications in nubus_device_realize()
might overflow because the resulting hwaddr is 64-bit whilst the multiplication
is only done at 32-bits.

Add an explicit ULL suffix to NUBUS_SUPER_SLOT_SIZE and NUBUS_SUPER_SLOT_SIZE
to ensure that the multiplication is also done at 64-bits.

Fixes: Coverity CID 1464070
Signed-off-by: Mark Cave-Ayland 
---
 include/hw/nubus/nubus.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index b3b4d2eadb..677dd6e5a2 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -15,13 +15,13 @@
 #include "qom/object.h"
 #include "qemu/units.h"
 
-#define NUBUS_SUPER_SLOT_SIZE 0x1000U
+#define NUBUS_SUPER_SLOT_SIZE 0x1000ULL
 #define NUBUS_SUPER_SLOT_NB   0xe
 
 #define NUBUS_SLOT_BASE   (NUBUS_SUPER_SLOT_SIZE * \
(NUBUS_SUPER_SLOT_NB + 1))
 
-#define NUBUS_SLOT_SIZE   0x0100
+#define NUBUS_SLOT_SIZE   0x0100ULL
 #define NUBUS_FIRST_SLOT  0x0
 #define NUBUS_LAST_SLOT   0xf
 #define NUBUS_SLOT_NB (NUBUS_LAST_SLOT - NUBUS_FIRST_SLOT + 1)
-- 
2.20.1

[PATCH v3 00/22] target/i386/sev: Housekeeping SEV + measured Linux SEV guest

[Philippe Mathieu-Daudé]

Hi,

While testing James & Dov patch:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg810571.html
I wasted some time trying to figure out how OVMF was supposed to
behave until realizing the binary I was using was built without SEV
support... Then wrote this series to help other developers to not
hit the same problem.

Since v2:
- Rebased on top of SGX
- Addressed review comments from Markus / David
- Included/rebased 'Measured Linux SEV guest' from Dov [1]
- Added orphean MAINTAINERS section

[1] 
https://lore.kernel.org/qemu-devel/20210825073538.959525-1-dovmu...@linux.ibm.com/

Supersedes: <20210616204328.2611406-1-phi...@redhat.com>

Dov Murik (2):
  sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux
boot
  x86/sev: generate SEV kernel loader hashes in x86_load_linux

Dr. David Alan Gilbert (1):
  target/i386/sev: sev_get_attestation_report use g_autofree

Philippe Mathieu-Daudé (19):
  qapi/misc-target: Wrap long 'SEV Attestation Report' long lines
  qapi/misc-target: Group SEV QAPI definitions
  target/i386/kvm: Introduce i386_softmmu_kvm Meson source set
  target/i386/kvm: Restrict SEV stubs to x86 architecture
  target/i386/monitor: Return QMP error when SEV is disabled in build
  target/i386/cpu: Add missing 'qapi/error.h' header
  target/i386/sev_i386.h: Remove unused headers
  target/i386/sev: Remove sev_get_me_mask()
  target/i386/sev: Mark unreachable code with g_assert_not_reached()
  target/i386/sev: Restrict SEV to system emulation
  target/i386/sev: Declare system-specific functions in 'sev_i386.h'
  target/i386/sev: Remove stubs by using code elision
  target/i386/sev: Move qmp_query_sev_attestation_report() to sev.c
  target/i386/sev: Move qmp_sev_inject_launch_secret() to sev.c
  target/i386/sev: Move qmp_query_sev_capabilities() to sev.c
  target/i386/sev: Move qmp_query_sev_launch_measure() to sev.c
  target/i386/sev: Move qmp_query_sev() & hmp_info_sev() to sev.c
  monitor: Restrict 'info sev' to x86 targets
  MAINTAINERS: Cover AMD SEV files

 qapi/misc-target.json |  77 
 include/monitor/hmp-target.h  |   1 +
 include/monitor/hmp.h |   1 -
 include/sysemu/sev.h  |  20 +-
 target/i386/sev_i386.h|  32 +--
 hw/i386/pc_sysfw.c|   2 +-
 hw/i386/x86.c |  25 ++-
 target/i386/cpu.c |  17 +-
 {accel => target/i386}/kvm/sev-stub.c |   0
 target/i386/monitor.c |  92 +
 target/i386/sev-stub.c|  83 
 target/i386/sev-sysemu-stub.c |  70 +++
 target/i386/sev.c | 268 +++---
 MAINTAINERS   |   7 +
 accel/kvm/meson.build |   1 -
 target/i386/kvm/meson.build   |   8 +-
 target/i386/meson.build   |   4 +-
 17 files changed, 438 insertions(+), 270 deletions(-)
 rename {accel => target/i386}/kvm/sev-stub.c (100%)
 delete mode 100644 target/i386/sev-stub.c
 create mode 100644 target/i386/sev-sysemu-stub.c

-- 
2.31.1

[PATCH v3 05/22] target/i386/monitor: Return QMP error when SEV is disabled in build

[Philippe Mathieu-Daudé]

If the management layer tries to inject a secret, it gets an empty
response in case the binary built without SEV:

  { "execute": "sev-inject-launch-secret",
"arguments": { "packet-header": "mypkt", "secret": "mypass", "gpa": 
4294959104 }
  }
  {
  "return": {
  }
  }

Make it clearer by returning an error, mentioning the feature is
disabled:

  { "execute": "sev-inject-launch-secret",
"arguments": { "packet-header": "mypkt", "secret": "mypass", "gpa": 
4294959104 }
  }
  {
  "error": {
  "class": "GenericError",
  "desc": "this feature or command is not currently supported"
  }
  }

Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Connor Kuehl 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/monitor.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 196c1c9e77f..a9f85acd473 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -28,6 +28,7 @@
 #include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qerror.h"
 #include "sysemu/kvm.h"
 #include "sysemu/sev.h"
 #include "qapi/error.h"
@@ -743,6 +744,10 @@ void qmp_sev_inject_launch_secret(const char *packet_hdr,
   bool has_gpa, uint64_t gpa,
   Error **errp)
 {
+if (!sev_enabled()) {
+error_setg(errp, QERR_UNSUPPORTED);
+return;
+}
 if (!has_gpa) {
 uint8_t *data;
 struct sev_secret_area *area;
-- 
2.31.1

[PATCH v3 12/22] target/i386/sev: Declare system-specific functions in 'sev_i386.h'

[Philippe Mathieu-Daudé]

While prefixed with sysemu, 'sysemu/sev.h' contains the architecture
specific declarations. The system specific parts are declared in
'sev_i386.h'.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/sysemu/sev.h   | 6 --
 target/i386/sev_i386.h | 7 +++
 hw/i386/pc_sysfw.c | 2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 94d821d737c..a329ed75c1c 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -18,11 +18,5 @@
 
 bool sev_enabled(void);
 int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
-int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
-int sev_inject_launch_secret(const char *hdr, const char *secret,
- uint64_t gpa, Error **errp);
-
-int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
-void sev_es_set_reset_vector(CPUState *cpu);
 
 #endif
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index afa19a0a161..0798ab3519a 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -33,4 +33,11 @@ extern SevCapability *sev_get_capabilities(Error **errp);
 extern SevAttestationReport *
 sev_get_attestation_report(const char *mnonce, Error **errp);
 
+int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
+int sev_inject_launch_secret(const char *hdr, const char *secret,
+ uint64_t gpa, Error **errp);
+
+int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
+void sev_es_set_reset_vector(CPUState *cpu);
+
 #endif
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 68d6b1f783e..0b202138b66 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -37,7 +37,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/block/flash.h"
 #include "sysemu/kvm.h"
-#include "sysemu/sev.h"
+#include "sev_i386.h"
 
 #define FLASH_SECTOR_SIZE 4096
 
-- 
2.31.1

[PATCH v3 03/22] target/i386/kvm: Introduce i386_softmmu_kvm Meson source set

[Philippe Mathieu-Daudé]

Introduce the i386_softmmu_kvm Meson source set to be able to
add features dependent on CONFIG_KVM.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/kvm/meson.build | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
index 0a533411cab..b1c76957c76 100644
--- a/target/i386/kvm/meson.build
+++ b/target/i386/kvm/meson.build
@@ -1,8 +1,12 @@
 i386_ss.add(when: 'CONFIG_KVM', if_false: files('kvm-stub.c'))
 
-i386_softmmu_ss.add(when: 'CONFIG_KVM', if_true: files(
+i386_softmmu_kvm_ss = ss.source_set()
+
+i386_softmmu_kvm_ss.add(files(
   'kvm.c',
   'kvm-cpu.c',
 ))
 
 i386_softmmu_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), 
if_false: files('hyperv-stub.c'))
+
+i386_softmmu_ss.add_all(when: 'CONFIG_KVM', if_true: i386_softmmu_kvm_ss)
-- 
2.31.1

[PATCH v3 08/22] target/i386/sev: Remove sev_get_me_mask()

[Philippe Mathieu-Daudé]

Unused dead code makes review harder, so remove it.

Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Connor Kuehl 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/sev_i386.h | 1 -
 target/i386/sev-stub.c | 5 -
 target/i386/sev.c  | 9 -
 3 files changed, 15 deletions(-)

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index f4223f1febf..afa19a0a161 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -25,7 +25,6 @@
 #define SEV_POLICY_SEV  0x20
 
 extern bool sev_es_enabled(void);
-extern uint64_t sev_get_me_mask(void);
 extern SevInfo *sev_get_info(void);
 extern uint32_t sev_get_cbit_position(void);
 extern uint32_t sev_get_reduced_phys_bits(void);
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index d91c2ece784..eb0c89bf2be 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -25,11 +25,6 @@ bool sev_enabled(void)
 return false;
 }
 
-uint64_t sev_get_me_mask(void)
-{
-return ~0;
-}
-
 uint32_t sev_get_cbit_position(void)
 {
 return 0;
diff --git a/target/i386/sev.c b/target/i386/sev.c
index fa7210473a6..c88cd808410 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -64,7 +64,6 @@ struct SevGuestState {
 uint8_t api_major;
 uint8_t api_minor;
 uint8_t build_id;
-uint64_t me_mask;
 int sev_fd;
 SevState state;
 gchar *measurement;
@@ -362,12 +361,6 @@ sev_es_enabled(void)
 return sev_enabled() && (sev_guest->policy & SEV_POLICY_ES);
 }
 
-uint64_t
-sev_get_me_mask(void)
-{
-return sev_guest ? sev_guest->me_mask : ~0;
-}
-
 uint32_t
 sev_get_cbit_position(void)
 {
@@ -804,8 +797,6 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
**errp)
 goto err;
 }
 
-sev->me_mask = ~(1UL << sev->cbitpos);
-
 devname = object_property_get_str(OBJECT(sev), "sev-device", NULL);
 sev->sev_fd = open(devname, O_RDWR);
 if (sev->sev_fd < 0) {
-- 
2.31.1

[PATCH v3 01/22] qapi/misc-target: Wrap long 'SEV Attestation Report' long lines

[Philippe Mathieu-Daudé]

Wrap long lines before 70 characters for legibility.

Suggested-by: Markus Armbruster 
Reviewed-by: Markus Armbruster 
Signed-off-by: Philippe Mathieu-Daudé 
---
 qapi/misc-target.json | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 594fbd1577f..ae5577e0390 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -300,8 +300,8 @@
 ##
 # @SevAttestationReport:
 #
-# The struct describes attestation report for a Secure Encrypted Virtualization
-# feature.
+# The struct describes attestation report for a Secure Encrypted
+# Virtualization feature.
 #
 # @data:  guest attestation report (base64 encoded)
 #
@@ -315,10 +315,11 @@
 ##
 # @query-sev-attestation-report:
 #
-# This command is used to get the SEV attestation report, and is supported on 
AMD
-# X86 platforms only.
+# This command is used to get the SEV attestation report, and is
+# supported on AMD X86 platforms only.
 #
-# @mnonce: a random 16 bytes value encoded in base64 (it will be included in 
report)
+# @mnonce: a random 16 bytes value encoded in base64 (it will be
+#  included in report)
 #
 # Returns: SevAttestationReport objects.
 #
@@ -326,11 +327,13 @@
 #
 # Example:
 #
-# -> { "execute" : "query-sev-attestation-report", "arguments": { "mnonce": 
"aaa" } }
+# -> { "execute" : "query-sev-attestation-report",
+#  "arguments": { "mnonce": "aaa" } }
 # <- { "return" : { "data": "bbbd"} }
 #
 ##
-{ 'command': 'query-sev-attestation-report', 'data': { 'mnonce': 'str' },
+{ 'command': 'query-sev-attestation-report',
+  'data': { 'mnonce': 'str' },
   'returns': 'SevAttestationReport',
   'if': 'TARGET_I386' }
 
-- 
2.31.1

[PATCH v3 19/22] monitor: Restrict 'info sev' to x86 targets

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/monitor/hmp-target.h  | 1 +
 include/monitor/hmp.h | 1 -
 target/i386/sev-sysemu-stub.c | 2 +-
 target/i386/sev.c | 2 +-
 4 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index dc53add7eef..96956d0fc41 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -49,6 +49,7 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict);
 void hmp_mce(Monitor *mon, const QDict *qdict);
 void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
 void hmp_info_io_apic(Monitor *mon, const QDict *qdict);
+void hmp_info_sev(Monitor *mon, const QDict *qdict);
 void hmp_info_sgx(Monitor *mon, const QDict *qdict);
 
 #endif /* MONITOR_HMP_TARGET_H */
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 3baa1058e2c..6bc27639e01 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -124,7 +124,6 @@ void hmp_info_ramblock(Monitor *mon, const QDict *qdict);
 void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
 void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict);
 void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict);
-void hmp_info_sev(Monitor *mon, const QDict *qdict);
 void hmp_info_replay(Monitor *mon, const QDict *qdict);
 void hmp_replay_break(Monitor *mon, const QDict *qdict);
 void hmp_replay_delete_break(Monitor *mon, const QDict *qdict);
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index 1836b32e4fc..b2a4033a030 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -13,7 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "monitor/monitor.h"
-#include "monitor/hmp.h"
+#include "monitor/hmp-target.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/error.h"
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 7caaa117ff7..c6d8fc52eb2 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -32,7 +32,7 @@
 #include "migration/blocker.h"
 #include "qom/object.h"
 #include "monitor/monitor.h"
-#include "monitor/hmp.h"
+#include "monitor/hmp-target.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qmp/qerror.h"
 #include "exec/confidential-guest-support.h"
-- 
2.31.1

[PATCH v3 02/22] qapi/misc-target: Group SEV QAPI definitions

[Philippe Mathieu-Daudé]

There is already a section with various SEV commands / types,
so move the SEV guest attestation together.

Signed-off-by: Philippe Mathieu-Daudé 
---
 qapi/misc-target.json | 80 +--
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index ae5577e0390..5aa2b95b7d4 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -229,6 +229,46 @@
   'data': { 'packet-header': 'str', 'secret': 'str', '*gpa': 'uint64' },
   'if': 'TARGET_I386' }
 
+##
+# @SevAttestationReport:
+#
+# The struct describes attestation report for a Secure Encrypted
+# Virtualization feature.
+#
+# @data:  guest attestation report (base64 encoded)
+#
+#
+# Since: 6.1
+##
+{ 'struct': 'SevAttestationReport',
+  'data': { 'data': 'str'},
+  'if': 'TARGET_I386' }
+
+##
+# @query-sev-attestation-report:
+#
+# This command is used to get the SEV attestation report, and is
+# supported on AMD X86 platforms only.
+#
+# @mnonce: a random 16 bytes value encoded in base64 (it will be
+#  included in report)
+#
+# Returns: SevAttestationReport objects.
+#
+# Since: 6.1
+#
+# Example:
+#
+# -> { "execute" : "query-sev-attestation-report",
+#  "arguments": { "mnonce": "aaa" } }
+# <- { "return" : { "data": "bbbd"} }
+#
+##
+{ 'command': 'query-sev-attestation-report',
+  'data': { 'mnonce': 'str' },
+  'returns': 'SevAttestationReport',
+  'if': 'TARGET_I386' }
+
 ##
 # @dump-skeys:
 #
@@ -297,46 +337,6 @@
   'if': 'TARGET_ARM' }
 
 
-##
-# @SevAttestationReport:
-#
-# The struct describes attestation report for a Secure Encrypted
-# Virtualization feature.
-#
-# @data:  guest attestation report (base64 encoded)
-#
-#
-# Since: 6.1
-##
-{ 'struct': 'SevAttestationReport',
-  'data': { 'data': 'str'},
-  'if': 'TARGET_I386' }
-
-##
-# @query-sev-attestation-report:
-#
-# This command is used to get the SEV attestation report, and is
-# supported on AMD X86 platforms only.
-#
-# @mnonce: a random 16 bytes value encoded in base64 (it will be
-#  included in report)
-#
-# Returns: SevAttestationReport objects.
-#
-# Since: 6.1
-#
-# Example:
-#
-# -> { "execute" : "query-sev-attestation-report",
-#  "arguments": { "mnonce": "aaa" } }
-# <- { "return" : { "data": "bbbd"} }
-#
-##
-{ 'command': 'query-sev-attestation-report',
-  'data': { 'mnonce': 'str' },
-  'returns': 'SevAttestationReport',
-  'if': 'TARGET_I386' }
-
 ##
 # @SGXInfo:
 #
-- 
2.31.1

[PATCH v3 07/22] target/i386/sev_i386.h: Remove unused headers

[Philippe Mathieu-Daudé]

Declarations don't require these headers, remove them.

Reviewed-by: Connor Kuehl 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/sev_i386.h | 4 
 target/i386/sev-stub.c | 1 +
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index ae6d8404787..f4223f1febf 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -14,11 +14,7 @@
 #ifndef QEMU_SEV_I386_H
 #define QEMU_SEV_I386_H
 
-#include "qom/object.h"
-#include "qapi/error.h"
-#include "sysemu/kvm.h"
 #include "sysemu/sev.h"
-#include "qemu/error-report.h"
 #include "qapi/qapi-types-misc-target.h"
 
 #define SEV_POLICY_NODBG0x1
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 0227cb51778..d91c2ece784 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "sev_i386.h"
 
 SevInfo *sev_get_info(void)
-- 
2.31.1

[PATCH v3 10/22] target/i386/sev: sev_get_attestation_report use g_autofree

[Philippe Mathieu-Daudé]

From: "Dr. David Alan Gilbert" 

Removes a whole bunch of g_free's and a goto.

Signed-off-by: Dr. David Alan Gilbert 
Reviewed-by: Connor Kuehl 
Reviewed-by: Brijesh Singh 
Message-Id: <20210603113017.34922-1-dgilb...@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/sev.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index c88cd808410..aefbef4bb63 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -493,8 +493,8 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
 struct kvm_sev_attestation_report input = {};
 SevAttestationReport *report = NULL;
 SevGuestState *sev = sev_guest;
-guchar *data;
-guchar *buf;
+g_autofree guchar *data = NULL;
+g_autofree guchar *buf = NULL;
 gsize len;
 int err = 0, ret;
 
@@ -514,7 +514,6 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
 if (len != sizeof(input.mnonce)) {
 error_setg(errp, "SEV: mnonce must be %zu bytes (got %" G_GSIZE_FORMAT 
")",
 sizeof(input.mnonce), len);
-g_free(buf);
 return NULL;
 }
 
@@ -525,7 +524,6 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
 if (err != SEV_RET_INVALID_LEN) {
 error_setg(errp, "failed to query the attestation report length "
 "ret=%d fw_err=%d (%s)", ret, err, fw_error_to_str(err));
-g_free(buf);
 return NULL;
 }
 }
@@ -540,7 +538,7 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
 if (ret) {
 error_setg_errno(errp, errno, "Failed to get attestation report"
 " ret=%d fw_err=%d (%s)", ret, err, fw_error_to_str(err));
-goto e_free_data;
+return NULL;
 }
 
 report = g_new0(SevAttestationReport, 1);
@@ -548,9 +546,6 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
 
 trace_kvm_sev_attestation_report(mnonce, report->data);
 
-e_free_data:
-g_free(data);
-g_free(buf);
 return report;
 }
 
-- 
2.31.1

Re: [PULL v2 00/33] x86 and misc changes for 2021-09-28

[Richard Henderson]

On 10/2/21 6:09 AM, Peter Maydell wrote:

On Sat, 2 Oct 2021 at 01:41, Richard Henderson
 wrote:


On 9/30/21 10:57 AM, Paolo Bonzini wrote:

The following changes since commit ba0fa56bc06e563de68d2a2bf3ddb0cfea1be4f9:

Merge remote-tracking branch 
'remotes/vivier/tags/q800-for-6.2-pull-request' into staging (2021-09-29 
21:20:49 +0100)

are available in the Git repository at:

https://gitlab.com/bonzini/qemu.git  tags/for-upstream

for you to fetch changes up to c1de5858bd39b299d3d8baec38b0376bed7f19e8:

meson_options.txt: Switch the default value for the vnc option to 'auto' 
(2021-09-30 15:30:25 +0200)


Applied, thanks


Uh, I'd already done this one :-)


I seem to have replied to the wrong cover.  How odd.

r~

[PATCH v3 04/22] target/i386/kvm: Restrict SEV stubs to x86 architecture

[Philippe Mathieu-Daudé]

SEV is x86-specific, no need to add its stub to other
architectures. Move the stub file to target/i386/kvm/.

Signed-off-by: Philippe Mathieu-Daudé 
---
 {accel => target/i386}/kvm/sev-stub.c | 0
 accel/kvm/meson.build | 1 -
 target/i386/kvm/meson.build   | 2 ++
 3 files changed, 2 insertions(+), 1 deletion(-)
 rename {accel => target/i386}/kvm/sev-stub.c (100%)

diff --git a/accel/kvm/sev-stub.c b/target/i386/kvm/sev-stub.c
similarity index 100%
rename from accel/kvm/sev-stub.c
rename to target/i386/kvm/sev-stub.c
diff --git a/accel/kvm/meson.build b/accel/kvm/meson.build
index 8d219bea507..397a1fe1fd1 100644
--- a/accel/kvm/meson.build
+++ b/accel/kvm/meson.build
@@ -3,6 +3,5 @@
   'kvm-all.c',
   'kvm-accel-ops.c',
 ))
-kvm_ss.add(when: 'CONFIG_SEV', if_false: files('sev-stub.c'))
 
 specific_ss.add_all(when: 'CONFIG_KVM', if_true: kvm_ss)
diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
index b1c76957c76..736df8b72e3 100644
--- a/target/i386/kvm/meson.build
+++ b/target/i386/kvm/meson.build
@@ -7,6 +7,8 @@
   'kvm-cpu.c',
 ))
 
+i386_softmmu_kvm_ss.add(when: 'CONFIG_SEV', if_false: files('sev-stub.c'))
+
 i386_softmmu_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), 
if_false: files('hyperv-stub.c'))
 
 i386_softmmu_ss.add_all(when: 'CONFIG_KVM', if_true: i386_softmmu_kvm_ss)
-- 
2.31.1

[PATCH v3 14/22] target/i386/sev: Move qmp_query_sev_attestation_report() to sev.c

[Philippe Mathieu-Daudé]

Move qmp_query_sev_attestation_report() from monitor.c to sev.c
and make sev_get_attestation_report() static. We don't need the
stub anymore, remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/sev_i386.h|  2 --
 target/i386/monitor.c |  6 --
 target/i386/sev-sysemu-stub.c |  7 ---
 target/i386/sev.c | 12 ++--
 4 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 2d9a1a0112e..5f367f78eb7 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -27,8 +27,6 @@
 extern SevInfo *sev_get_info(void);
 extern char *sev_get_launch_measurement(void);
 extern SevCapability *sev_get_capabilities(Error **errp);
-extern SevAttestationReport *
-sev_get_attestation_report(const char *mnonce, Error **errp);
 
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index a9f85acd473..c05d70252a2 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -764,12 +764,6 @@ void qmp_sev_inject_launch_secret(const char *packet_hdr,
 sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
 }
 
-SevAttestationReport *
-qmp_query_sev_attestation_report(const char *mnonce, Error **errp)
-{
-return sev_get_attestation_report(mnonce, errp);
-}
-
 SGXInfo *qmp_query_sgx(Error **errp)
 {
 return sgx_get_info(errp);
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index d556b4f091f..813b9a6a03b 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/qapi-commands-misc-target.h"
+#include "qapi/qmp/qerror.h"
 #include "qapi/error.h"
 #include "sev_i386.h"
 
@@ -52,9 +53,9 @@ int sev_es_save_reset_vector(void *flash_ptr, uint64_t 
flash_size)
 g_assert_not_reached();
 }
 
-SevAttestationReport *sev_get_attestation_report(const char *mnonce,
- Error **errp)
+SevAttestationReport *qmp_query_sev_attestation_report(const char *mnonce,
+   Error **errp)
 {
-error_setg(errp, "SEV is not available in this QEMU");
+error_setg(errp, QERR_UNSUPPORTED);
 return NULL;
 }
diff --git a/target/i386/sev.c b/target/i386/sev.c
index aefbef4bb63..91a217bbb85 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -31,6 +31,8 @@
 #include "migration/blocker.h"
 #include "qom/object.h"
 #include "monitor/monitor.h"
+#include "qapi/qapi-commands-misc-target.h"
+#include "qapi/qmp/qerror.h"
 #include "exec/confidential-guest-support.h"
 #include "hw/i386/pc.h"
 
@@ -487,8 +489,8 @@ out:
 return cap;
 }
 
-SevAttestationReport *
-sev_get_attestation_report(const char *mnonce, Error **errp)
+static SevAttestationReport *sev_get_attestation_report(const char *mnonce,
+Error **errp)
 {
 struct kvm_sev_attestation_report input = {};
 SevAttestationReport *report = NULL;
@@ -549,6 +551,12 @@ sev_get_attestation_report(const char *mnonce, Error 
**errp)
 return report;
 }
 
+SevAttestationReport *qmp_query_sev_attestation_report(const char *mnonce,
+   Error **errp)
+{
+return sev_get_attestation_report(mnonce, errp);
+}
+
 static int
 sev_read_file_base64(const char *filename, guchar **data, gsize *len)
 {
-- 
2.31.1

[PATCH v3 09/22] target/i386/sev: Mark unreachable code with g_assert_not_reached()

[Philippe Mathieu-Daudé]

The unique sev_encrypt_flash() invocation (in pc_system_flash_map)
is protected by the "if (sev_enabled())" check, so is not
reacheable.
Replace the abort() call in sev_es_save_reset_vector() by
g_assert_not_reached() which meaning is clearer.

Reviewed-by: Connor Kuehl 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/sev-stub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index eb0c89bf2be..4668365fd3e 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -54,7 +54,7 @@ int sev_inject_launch_secret(const char *hdr, const char 
*secret,
 
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
 {
-return 0;
+g_assert_not_reached();
 }
 
 bool sev_es_enabled(void)
@@ -68,7 +68,7 @@ void sev_es_set_reset_vector(CPUState *cpu)
 
 int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
 {
-abort();
+g_assert_not_reached();
 }
 
 SevAttestationReport *
-- 
2.31.1

[PATCH v3 06/22] target/i386/cpu: Add missing 'qapi/error.h' header

[Philippe Mathieu-Daudé]

Commit 00b81053244 ("target-i386: Remove assert_no_error usage")
forgot to add the "qapi/error.h" for &error_abort, add it now.

Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Connor Kuehl 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index cacec605bf1..e169a01713d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -27,6 +27,7 @@
 #include "sysemu/hvf.h"
 #include "kvm/kvm_i386.h"
 #include "sev_i386.h"
+#include "qapi/error.h"
 #include "qapi/qapi-visit-machine.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qapi-commands-machine-target.h"
-- 
2.31.1

[PATCH v3 16/22] target/i386/sev: Move qmp_query_sev_capabilities() to sev.c

[Philippe Mathieu-Daudé]

Move qmp_query_sev_capabilities() from monitor.c to sev.c
and make sev_get_capabilities() static. We don't need the
stub anymore, remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/sev_i386.h| 1 -
 target/i386/monitor.c | 5 -
 target/i386/sev-sysemu-stub.c | 4 ++--
 target/i386/sev.c | 8 ++--
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 5f367f78eb7..8d9388d8c5c 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -26,7 +26,6 @@
 
 extern SevInfo *sev_get_info(void);
 extern char *sev_get_launch_measurement(void);
-extern SevCapability *sev_get_capabilities(Error **errp);
 
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 188203da6f2..da36522fa15 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -728,11 +728,6 @@ SevLaunchMeasureInfo *qmp_query_sev_launch_measure(Error 
**errp)
 return info;
 }
 
-SevCapability *qmp_query_sev_capabilities(Error **errp)
-{
-return sev_get_capabilities(errp);
-}
-
 SGXInfo *qmp_query_sgx(Error **errp)
 {
 return sgx_get_info(errp);
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index 66b69540aa5..cc486a1afbe 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -27,9 +27,9 @@ char *sev_get_launch_measurement(void)
 return NULL;
 }
 
-SevCapability *sev_get_capabilities(Error **errp)
+SevCapability *qmp_query_sev_capabilities(Error **errp)
 {
-error_setg(errp, "SEV is not available in this QEMU");
+error_setg(errp, QERR_UNSUPPORTED);
 return NULL;
 }
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 2198d550be2..fce007d6749 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -438,8 +438,7 @@ e_free:
 return 1;
 }
 
-SevCapability *
-sev_get_capabilities(Error **errp)
+static SevCapability *sev_get_capabilities(Error **errp)
 {
 SevCapability *cap = NULL;
 guchar *pdh_data = NULL;
@@ -489,6 +488,11 @@ out:
 return cap;
 }
 
+SevCapability *qmp_query_sev_capabilities(Error **errp)
+{
+return sev_get_capabilities(errp);
+}
+
 static SevAttestationReport *sev_get_attestation_report(const char *mnonce,
 Error **errp)
 {
-- 
2.31.1

[PATCH v3 17/22] target/i386/sev: Move qmp_query_sev_launch_measure() to sev.c

[Philippe Mathieu-Daudé]

Move qmp_query_sev_launch_measure() from monitor.c to sev.c
and make sev_get_launch_measurement() static. We don't need the
stub anymore, remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/sev_i386.h|  1 -
 target/i386/monitor.c | 17 -
 target/i386/sev-sysemu-stub.c |  3 ++-
 target/i386/sev.c | 20 ++--
 4 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 8d9388d8c5c..1699376ad87 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -25,7 +25,6 @@
 #define SEV_POLICY_SEV  0x20
 
 extern SevInfo *sev_get_info(void);
-extern char *sev_get_launch_measurement(void);
 
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index da36522fa15..0b38e970c73 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -711,23 +711,6 @@ void hmp_info_sev(Monitor *mon, const QDict *qdict)
 qapi_free_SevInfo(info);
 }
 
-SevLaunchMeasureInfo *qmp_query_sev_launch_measure(Error **errp)
-{
-char *data;
-SevLaunchMeasureInfo *info;
-
-data = sev_get_launch_measurement();
-if (!data) {
-error_setg(errp, "Measurement is not available");
-return NULL;
-}
-
-info = g_malloc0(sizeof(*info));
-info->data = data;
-
-return info;
-}
-
 SGXInfo *qmp_query_sgx(Error **errp)
 {
 return sgx_get_info(errp);
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index cc486a1afbe..355391c16c4 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -22,8 +22,9 @@ SevInfo *sev_get_info(void)
 return NULL;
 }
 
-char *sev_get_launch_measurement(void)
+SevLaunchMeasureInfo *qmp_query_sev_launch_measure(Error **errp)
 {
+error_setg(errp, QERR_UNSUPPORTED);
 return NULL;
 }
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index fce007d6749..8e9cce62196 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -718,8 +718,7 @@ free_measurement:
 g_free(measurement);
 }
 
-char *
-sev_get_launch_measurement(void)
+static char *sev_get_launch_measurement(void)
 {
 if (sev_guest &&
 sev_guest->state >= SEV_STATE_LAUNCH_SECRET) {
@@ -729,6 +728,23 @@ sev_get_launch_measurement(void)
 return NULL;
 }
 
+SevLaunchMeasureInfo *qmp_query_sev_launch_measure(Error **errp)
+{
+char *data;
+SevLaunchMeasureInfo *info;
+
+data = sev_get_launch_measurement();
+if (!data) {
+error_setg(errp, "Measurement is not available");
+return NULL;
+}
+
+info = g_malloc0(sizeof(*info));
+info->data = data;
+
+return info;
+}
+
 static Notifier sev_machine_done_notify = {
 .notify = sev_launch_get_measure,
 };
-- 
2.31.1

[PATCH v3 11/22] target/i386/sev: Restrict SEV to system emulation

[Philippe Mathieu-Daudé]

SEV is irrelevant on user emulation, so restrict it to sysemu.
Some stubs are still required because used in cpu.c by
x86_register_cpudef_types(), so move the sysemu specific stubs
to sev-sysemu-stub.c instead. This will allow us to simplify
monitor.c (which is not available in user emulation) in the
next commit.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/sev-stub.c| 43 -
 target/i386/sev-sysemu-stub.c | 60 +++
 target/i386/meson.build   |  4 ++-
 3 files changed, 63 insertions(+), 44 deletions(-)
 create mode 100644 target/i386/sev-sysemu-stub.c

diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 4668365fd3e..8eae5d2fa8d 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -15,11 +15,6 @@
 #include "qapi/error.h"
 #include "sev_i386.h"
 
-SevInfo *sev_get_info(void)
-{
-return NULL;
-}
-
 bool sev_enabled(void)
 {
 return false;
@@ -35,45 +30,7 @@ uint32_t sev_get_reduced_phys_bits(void)
 return 0;
 }
 
-char *sev_get_launch_measurement(void)
-{
-return NULL;
-}
-
-SevCapability *sev_get_capabilities(Error **errp)
-{
-error_setg(errp, "SEV is not available in this QEMU");
-return NULL;
-}
-
-int sev_inject_launch_secret(const char *hdr, const char *secret,
- uint64_t gpa, Error **errp)
-{
-return 1;
-}
-
-int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
-{
-g_assert_not_reached();
-}
-
 bool sev_es_enabled(void)
 {
 return false;
 }
-
-void sev_es_set_reset_vector(CPUState *cpu)
-{
-}
-
-int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
-{
-g_assert_not_reached();
-}
-
-SevAttestationReport *
-sev_get_attestation_report(const char *mnonce, Error **errp)
-{
-error_setg(errp, "SEV is not available in this QEMU");
-return NULL;
-}
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
new file mode 100644
index 000..d556b4f091f
--- /dev/null
+++ b/target/i386/sev-sysemu-stub.c
@@ -0,0 +1,60 @@
+/*
+ * QEMU SEV system stub
+ *
+ * Copyright Advanced Micro Devices 2018
+ *
+ * Authors:
+ *  Brijesh Singh 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-commands-misc-target.h"
+#include "qapi/error.h"
+#include "sev_i386.h"
+
+SevInfo *sev_get_info(void)
+{
+return NULL;
+}
+
+char *sev_get_launch_measurement(void)
+{
+return NULL;
+}
+
+SevCapability *sev_get_capabilities(Error **errp)
+{
+error_setg(errp, "SEV is not available in this QEMU");
+return NULL;
+}
+
+int sev_inject_launch_secret(const char *hdr, const char *secret,
+ uint64_t gpa, Error **errp)
+{
+return 1;
+}
+
+int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
+{
+g_assert_not_reached();
+}
+
+void sev_es_set_reset_vector(CPUState *cpu)
+{
+}
+
+int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
+{
+g_assert_not_reached();
+}
+
+SevAttestationReport *sev_get_attestation_report(const char *mnonce,
+ Error **errp)
+{
+error_setg(errp, "SEV is not available in this QEMU");
+return NULL;
+}
diff --git a/target/i386/meson.build b/target/i386/meson.build
index dac19ec00d4..a4f45c3ec1d 100644
--- a/target/i386/meson.build
+++ b/target/i386/meson.build
@@ -6,7 +6,7 @@
   'xsave_helper.c',
   'cpu-dump.c',
 ))
-i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c', 'sev.c'), 
if_false: files('sev-stub.c'))
+i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c'), if_false: 
files('sev-stub.c'))
 
 # x86 cpu type
 i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
@@ -20,6 +20,8 @@
   'monitor.c',
   'cpu-sysemu.c',
 ))
+i386_softmmu_ss.add(when: 'CONFIG_SEV', if_true: files('sev.c'), if_false: 
files('sev-sysemu-stub.c'))
+
 i386_user_ss = ss.source_set()
 
 subdir('kvm')
-- 
2.31.1

[PATCH v3 21/22] x86/sev: generate SEV kernel loader hashes in x86_load_linux

[Philippe Mathieu-Daudé]

From: Dov Murik 

If SEV is enabled and a kernel is passed via -kernel, pass the hashes of
kernel/initrd/cmdline in an encrypted guest page to OVMF for SEV
measured boot.

Co-developed-by: James Bottomley 
Signed-off-by: James Bottomley 
Signed-off-by: Dov Murik 
Reviewed-by: Daniel P. Berrangé 
Message-Id: <20210930054915.13252-3-dovmu...@linux.ibm.com>
[PMD: Rebased on top of 0021c4765a6]
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/i386/x86.c | 25 -
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 41ef9a84a9f..0c7c054e3a0 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -47,6 +47,7 @@
 #include "hw/i386/fw_cfg.h"
 #include "hw/intc/i8259.h"
 #include "hw/rtc/mc146818rtc.h"
+#include "target/i386/sev_i386.h"
 
 #include "hw/acpi/cpu_hotplug.h"
 #include "hw/irq.h"
@@ -780,6 +781,7 @@ void x86_load_linux(X86MachineState *x86ms,
 const char *initrd_filename = machine->initrd_filename;
 const char *dtb_filename = machine->dtb;
 const char *kernel_cmdline = machine->kernel_cmdline;
+SevKernelLoaderContext sev_load_ctx = {};
 
 /* Align to 16 bytes as a paranoia measure */
 cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
@@ -926,6 +928,8 @@ void x86_load_linux(X86MachineState *x86ms,
 fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr);
 fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1);
 fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
+sev_load_ctx.cmdline_data = (char *)kernel_cmdline;
+sev_load_ctx.cmdline_size = strlen(kernel_cmdline) + 1;
 
 if (protocol >= 0x202) {
 stl_p(header + 0x228, cmdline_addr);
@@ -1007,6 +1011,8 @@ void x86_load_linux(X86MachineState *x86ms,
 fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, initrd_addr);
 fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
 fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, initrd_data, initrd_size);
+sev_load_ctx.initrd_data = initrd_data;
+sev_load_ctx.initrd_size = initrd_size;
 
 stl_p(header + 0x218, initrd_addr);
 stl_p(header + 0x21c, initrd_size);
@@ -1065,15 +1071,32 @@ void x86_load_linux(X86MachineState *x86ms,
 load_image_size(dtb_filename, setup_data->data, dtb_size);
 }
 
-memcpy(setup, header, MIN(sizeof(header), setup_size));
+/*
+ * If we're starting an encrypted VM, it will be OVMF based, which uses the
+ * efi stub for booting and doesn't require any values to be placed in the
+ * kernel header.  We therefore don't update the header so the hash of the
+ * kernel on the other side of the fw_cfg interface matches the hash of the
+ * file the user passed in.
+ */
+if (!sev_enabled()) {
+memcpy(setup, header, MIN(sizeof(header), setup_size));
+}
 
 fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr);
 fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
 fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_size);
+sev_load_ctx.kernel_data = (char *)kernel;
+sev_load_ctx.kernel_size = kernel_size;
 
 fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_ADDR, real_addr);
 fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
 fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size);
+sev_load_ctx.setup_data = (char *)setup;
+sev_load_ctx.setup_size = setup_size;
+
+if (sev_enabled()) {
+sev_add_kernel_loader_hashes(&sev_load_ctx, &error_fatal);
+}
 
 option_rom[nb_option_roms].bootindex = 0;
 option_rom[nb_option_roms].name = "linuxboot.bin";
-- 
2.31.1

[PATCH v3 13/22] target/i386/sev: Remove stubs by using code elision

[Philippe Mathieu-Daudé]

Only declare sev_enabled() and sev_es_enabled() when CONFIG_SEV is
set, to allow the compiler to elide unused code. Remove unnecessary
stubs.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/sysemu/sev.h| 14 +-
 target/i386/sev_i386.h  |  3 ---
 target/i386/cpu.c   | 16 +---
 target/i386/sev-stub.c  | 36 
 target/i386/meson.build |  2 +-
 5 files changed, 23 insertions(+), 48 deletions(-)
 delete mode 100644 target/i386/sev-stub.c

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index a329ed75c1c..f5c625bb3b3 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -14,9 +14,21 @@
 #ifndef QEMU_SEV_H
 #define QEMU_SEV_H
 
-#include "sysemu/kvm.h"
+#ifndef CONFIG_USER_ONLY
+#include CONFIG_DEVICES /* CONFIG_SEV */
+#endif
 
+#ifdef CONFIG_SEV
 bool sev_enabled(void);
+bool sev_es_enabled(void);
+#else
+#define sev_enabled() 0
+#define sev_es_enabled() 0
+#endif
+
+uint32_t sev_get_cbit_position(void);
+uint32_t sev_get_reduced_phys_bits(void);
+
 int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
 
 #endif
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 0798ab3519a..2d9a1a0112e 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -24,10 +24,7 @@
 #define SEV_POLICY_DOMAIN   0x10
 #define SEV_POLICY_SEV  0x20
 
-extern bool sev_es_enabled(void);
 extern SevInfo *sev_get_info(void);
-extern uint32_t sev_get_cbit_position(void);
-extern uint32_t sev_get_reduced_phys_bits(void);
 extern char *sev_get_launch_measurement(void);
 extern SevCapability *sev_get_capabilities(Error **errp);
 extern SevAttestationReport *
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index e169a01713d..27992bdc9f8 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -25,8 +25,8 @@
 #include "tcg/helper-tcg.h"
 #include "sysemu/reset.h"
 #include "sysemu/hvf.h"
+#include "sysemu/sev.h"
 #include "kvm/kvm_i386.h"
-#include "sev_i386.h"
 #include "qapi/error.h"
 #include "qapi/qapi-visit-machine.h"
 #include "qapi/qmp/qerror.h"
@@ -38,6 +38,7 @@
 #include "exec/address-spaces.h"
 #include "hw/boards.h"
 #include "hw/i386/sgx-epc.h"
+#include "sev_i386.h"
 #endif
 
 #include "disas/capstone.h"
@@ -5764,12 +5765,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
uint32_t count,
 *edx = 0;
 break;
 case 0x801F:
-*eax = sev_enabled() ? 0x2 : 0;
-*eax |= sev_es_enabled() ? 0x8 : 0;
-*ebx = sev_get_cbit_position();
-*ebx |= sev_get_reduced_phys_bits() << 6;
-*ecx = 0;
-*edx = 0;
+*eax = *ebx = *ecx = *edx = 0;
+if (sev_enabled()) {
+*eax = 0x2;
+*eax |= sev_es_enabled() ? 0x8 : 0;
+*ebx = sev_get_cbit_position();
+*ebx |= sev_get_reduced_phys_bits() << 6;
+}
 break;
 default:
 /* reserved values: zero */
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
deleted file mode 100644
index 8eae5d2fa8d..000
--- a/target/i386/sev-stub.c
+++ /dev/null
@@ -1,36 +0,0 @@
-/*
- * QEMU SEV stub
- *
- * Copyright Advanced Micro Devices 2018
- *
- * Authors:
- *  Brijesh Singh 
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-
-#include "qemu/osdep.h"
-#include "qapi/error.h"
-#include "sev_i386.h"
-
-bool sev_enabled(void)
-{
-return false;
-}
-
-uint32_t sev_get_cbit_position(void)
-{
-return 0;
-}
-
-uint32_t sev_get_reduced_phys_bits(void)
-{
-return 0;
-}
-
-bool sev_es_enabled(void)
-{
-return false;
-}
diff --git a/target/i386/meson.build b/target/i386/meson.build
index a4f45c3ec1d..ae38dc95635 100644
--- a/target/i386/meson.build
+++ b/target/i386/meson.build
@@ -6,7 +6,7 @@
   'xsave_helper.c',
   'cpu-dump.c',
 ))
-i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c'), if_false: 
files('sev-stub.c'))
+i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c'))
 
 # x86 cpu type
 i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
-- 
2.31.1

[PATCH v3 22/22] MAINTAINERS: Cover AMD SEV files

[Philippe Mathieu-Daudé]

Add an entry to list SEV-related files.

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 50435b8d2f5..733a5201e76 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3038,6 +3038,13 @@ F: hw/core/clock-vmstate.c
 F: hw/core/qdev-clock.c
 F: docs/devel/clocks.rst
 
+AMD Secure Encrypted Virtualization (SEV)
+S: Orphan
+F: docs/amd-memory-encryption.txt
+F: target/i386/sev*
+F: target/i386/kvm/sev-stub.c
+F: include/sysemu/sev.h
+
 Usermode Emulation
 --
 Overall usermode emulation
-- 
2.31.1

[PATCH v3 15/22] target/i386/sev: Move qmp_sev_inject_launch_secret() to sev.c

[Philippe Mathieu-Daudé]

Move qmp_sev_inject_launch_secret() from monitor.c to sev.c
and make sev_inject_launch_secret() static. We don't need the
stub anymore, remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/monitor.c | 31 ---
 target/i386/sev-sysemu-stub.c |  6 +++---
 target/i386/sev.c | 31 +++
 3 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index c05d70252a2..188203da6f2 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -733,37 +733,6 @@ SevCapability *qmp_query_sev_capabilities(Error **errp)
 return sev_get_capabilities(errp);
 }
 
-#define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294"
-struct sev_secret_area {
-uint32_t base;
-uint32_t size;
-};
-
-void qmp_sev_inject_launch_secret(const char *packet_hdr,
-  const char *secret,
-  bool has_gpa, uint64_t gpa,
-  Error **errp)
-{
-if (!sev_enabled()) {
-error_setg(errp, QERR_UNSUPPORTED);
-return;
-}
-if (!has_gpa) {
-uint8_t *data;
-struct sev_secret_area *area;
-
-if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) {
-error_setg(errp, "SEV: no secret area found in OVMF,"
-   " gpa must be specified.");
-return;
-}
-area = (struct sev_secret_area *)data;
-gpa = area->base;
-}
-
-sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
-}
-
 SGXInfo *qmp_query_sgx(Error **errp)
 {
 return sgx_get_info(errp);
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index 813b9a6a03b..66b69540aa5 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -33,10 +33,10 @@ SevCapability *sev_get_capabilities(Error **errp)
 return NULL;
 }
 
-int sev_inject_launch_secret(const char *hdr, const char *secret,
- uint64_t gpa, Error **errp)
+void qmp_sev_inject_launch_secret(const char *packet_header, const char 
*secret,
+  bool has_gpa, uint64_t gpa, Error **errp)
 {
-return 1;
+error_setg(errp, QERR_UNSUPPORTED);
 }
 
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 91a217bbb85..2198d550be2 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -949,6 +949,37 @@ int sev_inject_launch_secret(const char *packet_hdr, const 
char *secret,
 return 0;
 }
 
+#define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294"
+struct sev_secret_area {
+uint32_t base;
+uint32_t size;
+};
+
+void qmp_sev_inject_launch_secret(const char *packet_hdr,
+  const char *secret,
+  bool has_gpa, uint64_t gpa,
+  Error **errp)
+{
+if (!sev_enabled()) {
+error_setg(errp, QERR_UNSUPPORTED);
+return;
+}
+if (!has_gpa) {
+uint8_t *data;
+struct sev_secret_area *area;
+
+if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) {
+error_setg(errp, "SEV: no secret area found in OVMF,"
+   " gpa must be specified.");
+return;
+}
+area = (struct sev_secret_area *)data;
+gpa = area->base;
+}
+
+sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
+}
+
 static int
 sev_es_parse_reset_block(SevInfoBlock *info, uint32_t *addr)
 {
-- 
2.31.1

[PATCH v3 18/22] target/i386/sev: Move qmp_query_sev() & hmp_info_sev() to sev.c

[Philippe Mathieu-Daudé]

Move qmp_query_sev() & hmp_info_sev()() from monitor.c to sev.c
and make sev_get_info() static. We don't need the stub anymore,
remove it. Add a stub for hmp_info_sev().

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/sev_i386.h|  3 ---
 target/i386/monitor.c | 38 +-
 target/i386/sev-sysemu-stub.c | 10 -
 target/i386/sev.c | 39 +--
 4 files changed, 47 insertions(+), 43 deletions(-)

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 1699376ad87..15a959d6174 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -15,7 +15,6 @@
 #define QEMU_SEV_I386_H
 
 #include "sysemu/sev.h"
-#include "qapi/qapi-types-misc-target.h"
 
 #define SEV_POLICY_NODBG0x1
 #define SEV_POLICY_NOKS 0x2
@@ -24,8 +23,6 @@
 #define SEV_POLICY_DOMAIN   0x10
 #define SEV_POLICY_SEV  0x20
 
-extern SevInfo *sev_get_info(void);
-
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
  uint64_t gpa, Error **errp);
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 0b38e970c73..890870b252d 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -28,11 +28,9 @@
 #include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
 #include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qerror.h"
+//#include "qapi/qmp/qerror.h"
 #include "sysemu/kvm.h"
-#include "sysemu/sev.h"
 #include "qapi/error.h"
-#include "sev_i386.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qapi-commands-misc.h"
 #include "hw/i386/pc.h"
@@ -677,40 +675,6 @@ void hmp_info_io_apic(Monitor *mon, const QDict *qdict)
"removed soon. Please use 'info pic' instead.\n");
 }
 
-SevInfo *qmp_query_sev(Error **errp)
-{
-SevInfo *info;
-
-info = sev_get_info();
-if (!info) {
-error_setg(errp, "SEV feature is not available");
-return NULL;
-}
-
-return info;
-}
-
-void hmp_info_sev(Monitor *mon, const QDict *qdict)
-{
-SevInfo *info = sev_get_info();
-
-if (info && info->enabled) {
-monitor_printf(mon, "handle: %d\n", info->handle);
-monitor_printf(mon, "state: %s\n", SevState_str(info->state));
-monitor_printf(mon, "build: %d\n", info->build_id);
-monitor_printf(mon, "api version: %d.%d\n",
-   info->api_major, info->api_minor);
-monitor_printf(mon, "debug: %s\n",
-   info->policy & SEV_POLICY_NODBG ? "off" : "on");
-monitor_printf(mon, "key-sharing: %s\n",
-   info->policy & SEV_POLICY_NOKS ? "off" : "on");
-} else {
-monitor_printf(mon, "SEV is not enabled\n");
-}
-
-qapi_free_SevInfo(info);
-}
-
 SGXInfo *qmp_query_sgx(Error **errp)
 {
 return sgx_get_info(errp);
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index 355391c16c4..1836b32e4fc 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -12,13 +12,16 @@
  */
 
 #include "qemu/osdep.h"
+#include "monitor/monitor.h"
+#include "monitor/hmp.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/error.h"
 #include "sev_i386.h"
 
-SevInfo *sev_get_info(void)
+SevInfo *qmp_query_sev(Error **errp)
 {
+error_setg(errp, QERR_UNSUPPORTED);
 return NULL;
 }
 
@@ -60,3 +63,8 @@ SevAttestationReport *qmp_query_sev_attestation_report(const 
char *mnonce,
 error_setg(errp, QERR_UNSUPPORTED);
 return NULL;
 }
+
+void hmp_info_sev(Monitor *mon, const QDict *qdict)
+{
+monitor_printf(mon, "SEV is not available in this QEMU\n");
+}
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 8e9cce62196..7caaa117ff7 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -27,10 +27,12 @@
 #include "sev_i386.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/runstate.h"
+#include "sysemu/sev.h"
 #include "trace.h"
 #include "migration/blocker.h"
 #include "qom/object.h"
 #include "monitor/monitor.h"
+#include "monitor/hmp.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qmp/qerror.h"
 #include "exec/confidential-guest-support.h"
@@ -375,8 +377,7 @@ sev_get_reduced_phys_bits(void)
 return sev_guest ? sev_guest->reduced_phys_bits : 0;
 }
 
-SevInfo *
-sev_get_info(void)
+static SevInfo *sev_get_info(void)
 {
 SevInfo *info;
 
@@ -395,6 +396,40 @@ sev_get_info(void)
 return info;
 }
 
+SevInfo *qmp_query_sev(Error **errp)
+{
+SevInfo *info;
+
+info = sev_get_info();
+if (!info) {
+error_setg(errp, "SEV feature is not available");
+return NULL;
+}
+
+return info;
+}
+
+void hmp_info_sev(Monitor *mon, const QDict *qdict)
+{
+SevInfo *info = sev_get_info();
+
+if (info && info->enabled) {
+monitor_printf(mon, "handle: %d\n", info->handle);
+monitor_printf(mon, "state: %

[PATCH v3 20/22] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Philippe Mathieu-Daudé]

From: Dov Murik 

Add the sev_add_kernel_loader_hashes function to calculate the hashes of
the kernel/initrd/cmdline and fill a designated OVMF encrypted hash
table area.  For this to work, OVMF must support an encrypted area to
place the data which is advertised via a special GUID in the OVMF reset
table.

The hashes of each of the files is calculated (or the string in the case
of the cmdline with trailing '\0' included).  Each entry in the hashes
table is GUID identified and since they're passed through the
sev_encrypt_flash interface, the hashes will be accumulated by the AMD
PSP measurement (SEV_LAUNCH_MEASURE).

Co-developed-by: James Bottomley 
Signed-off-by: James Bottomley 
Signed-off-by: Dov Murik 
Reviewed-by: Daniel P. Berrangé 
Message-Id: <20210930054915.13252-2-dovmu...@linux.ibm.com>
[PMD: Rebased on top of 0021c4765a6]
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/sev_i386.h |  12 
 target/i386/sev.c  | 138 +
 2 files changed, 150 insertions(+)

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 15a959d6174..17031cddd37 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -23,9 +23,21 @@
 #define SEV_POLICY_DOMAIN   0x10
 #define SEV_POLICY_SEV  0x20
 
+typedef struct SevKernelLoaderContext {
+char *setup_data;
+size_t setup_size;
+char *kernel_data;
+size_t kernel_size;
+char *initrd_data;
+size_t initrd_size;
+char *cmdline_data;
+size_t cmdline_size;
+} SevKernelLoaderContext;
+
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
  uint64_t gpa, Error **errp);
+bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp);
 
 int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
 void sev_es_set_reset_vector(CPUState *cpu);
diff --git a/target/i386/sev.c b/target/i386/sev.c
index c6d8fc52eb2..91fdf0d4503 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -23,6 +23,7 @@
 #include "qemu/base64.h"
 #include "qemu/module.h"
 #include "qemu/uuid.h"
+#include "crypto/hash.h"
 #include "sysemu/kvm.h"
 #include "sev_i386.h"
 #include "sysemu/sysemu.h"
@@ -86,6 +87,32 @@ typedef struct __attribute__((__packed__)) SevInfoBlock {
 uint32_t reset_addr;
 } SevInfoBlock;
 
+#define SEV_HASH_TABLE_RV_GUID  "7255371f-3a3b-4b04-927b-1da6efa8d454"
+typedef struct QEMU_PACKED SevHashTableDescriptor {
+/* SEV hash table area guest address */
+uint32_t base;
+/* SEV hash table area size (in bytes) */
+uint32_t size;
+} SevHashTableDescriptor;
+
+/* hard code sha256 digest size */
+#define HASH_SIZE 32
+
+typedef struct QEMU_PACKED SevHashTableEntry {
+QemuUUID guid;
+uint16_t len;
+uint8_t hash[HASH_SIZE];
+} SevHashTableEntry;
+
+typedef struct QEMU_PACKED SevHashTable {
+QemuUUID guid;
+uint16_t len;
+SevHashTableEntry cmdline;
+SevHashTableEntry initrd;
+SevHashTableEntry kernel;
+uint8_t padding[];
+} SevHashTable;
+
 static SevGuestState *sev_guest;
 static Error *sev_mig_blocker;
 
@@ -1151,6 +1178,117 @@ int sev_es_save_reset_vector(void *flash_ptr, uint64_t 
flash_size)
 return 0;
 }
 
+static const QemuUUID sev_hash_table_header_guid = {
+.data = UUID_LE(0x9438d606, 0x4f22, 0x4cc9, 0xb4, 0x79, 0xa7, 0x93,
+0xd4, 0x11, 0xfd, 0x21)
+};
+
+static const QemuUUID sev_kernel_entry_guid = {
+.data = UUID_LE(0x4de79437, 0xabd2, 0x427f, 0xb8, 0x35, 0xd5, 0xb1,
+0x72, 0xd2, 0x04, 0x5b)
+};
+static const QemuUUID sev_initrd_entry_guid = {
+.data = UUID_LE(0x44baf731, 0x3a2f, 0x4bd7, 0x9a, 0xf1, 0x41, 0xe2,
+0x91, 0x69, 0x78, 0x1d)
+};
+static const QemuUUID sev_cmdline_entry_guid = {
+.data = UUID_LE(0x97d02dd8, 0xbd20, 0x4c94, 0xaa, 0x78, 0xe7, 0x71,
+0x4d, 0x36, 0xab, 0x2a)
+};
+
+/*
+ * Add the hashes of the linux kernel/initrd/cmdline to an encrypted guest page
+ * which is included in SEV's initial memory measurement.
+ */
+bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
+{
+uint8_t *data;
+SevHashTableDescriptor *area;
+SevHashTable *ht;
+uint8_t cmdline_hash[HASH_SIZE];
+uint8_t initrd_hash[HASH_SIZE];
+uint8_t kernel_hash[HASH_SIZE];
+uint8_t *hashp;
+size_t hash_len = HASH_SIZE;
+int aligned_len;
+
+if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
+error_setg(errp,
+   "SEV: kernel specified but OVMF has no hash table guid");
+return false;
+}
+area = (SevHashTableDescriptor *)data;
+
+/*
+ * Calculate hash of kernel command-line with the terminating null byte. If
+ * the user doesn't supply a command-line via -append, the 1-byte "\0" will
+ * be used.
+ */
+hashp = cmdline_hash;
+if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG

Re: hexagon container update

[Richard Henderson]

On 10/1/21 1:59 PM, Brian Cain wrote:

If there's any trust concerns we can verify the download in the dockerfile 
using the hash file for the tarball and/or the gpg signature.


That's true, I should have thought of that.


r~

Re: [PATCH v3 6/6] tests/qapi-schema: Test cases for aliases

[Markus Armbruster]

Kevin Wolf  writes:

> Am 17.09.2021 um 10:26 hat Markus Armbruster geschrieben:

[...]

>> >> We actually require much less: for QMP chardev-add, we need
>> >> 'data.addr.data.FOO' and nothing else, and for CLI -chardev, we need
>> >> 'FOO' and nothing else (I think).  The unneeded ones become accidental
>> >> parts of the external interface.  If experience is any guide, we'll have
>> >> plenty of opportunity to regret such accidents :)
>> >> 
>> >> Can we somehow restrict external interfaces to what we actually need?
>> >
>> > Not reasonably, I would say. Of course, you could try to cover all
>> > paths with aliases in the top level, but the top level really shouldn't
>> > have to know about the details of types deep inside some union variants.
>> >
>> > The solution for reducing the allowed options here is to work on
>> > introspection, mark 'data' deprecated everywhere and get rid of the
>> > simple union nonsense.
>> 
>> Accidental extension of QMP to enable QAPIfication elsewhere would be a
>> mistake.  Elsewhere right now: -chardev.
>>
>> The knee-jerk short-term solution for QMP is to ignore aliases there
>> completely.  Without introspection, they can't be used seriously anyway.
>
> I would say it's intentional enough. If we can flatten simple unions for
> the CLI, why not accept them in QMP, too? (And management tools will
> only be happier if they can use the same representation for QMP and
> CLI.) I hope that we can get introspection done for 6.2, but even if we
> can't, making the case already work shouldn't hurt anyone.
>
> Now you could argue that some aliases to be introduced for -chardev have
> no place in QMP because they have no practical use there. But isn't a
> consistent QAPI structure on all external interfaces more valuable than
> keeping the interface in QMP minimal, but inconsistent with the CLI?
>
> The problem I would generally see with accidental extension of QMP is
> that it may restrict future changes for no reason. But if we already
> get the restriction because we must stay compatible with the CLI, too,
> then this doesn't apply any more.

I agree consistency matters.  But what exactly should be consistent?

I believe we all agree that a CLI option's *JSON* argument should be
consistent with the corresponding QMP command.  This is easy: since both
JSON arguments are fed to the same QAPI machinery, consistency is the
default, and inconsistency would take extra effort.

But what about the dotted keys argument?

One point of view is the difference between the JSON and the dotted keys
argument should be concrete syntax only.  Fine print: there may be
arguments dotted keys can't express, but let's ignore that here.

Another point of view is that dotted keys arguments are to JSON
arguments what HMP is to QMP: a (hopefully) human-friendly layer on top,
where we have a certain degree of freedom to improve on the
machine-friendly interface for human use.

Let me try to explain why I believe the latter one makes more sense.

When QAPIfying existing CLI options, the new dotted keys option argument
must be backward compatible to the old option argument, and the QMP
command must also remain backward compatible.

If their abstract syntax is already perfectly consistent, we can simply
use qobject_input_visitor_new_str().  All we have to worry then is the
differences between dotted keys syntax and whatever it replaces (almost
always QemuOpts), which is of no concern to us right now.

What if their abstract syntax differs, as it does for -chardev and
QMP chardev-add?

One solution is to use the *union* of the two languages both for CLI and
QMP.  Complicates both.  I don't like this.  We can try to mitigate by
deprecating unwanted parts of the abstract syntax.  More on that below.

If we treat dotted keys as separate, human-friendly layer on top, we can
instead *translate* from one language to the other.  This is actually
what -chardev, HMP chardev-add, and all the other HMP commands wrapping
around QMP do today, or close to it.


Different tack.  I've been struggling with distilling my intuitions
about the proposed QAPI aliases feature into thought (one reason for me
taking so painfully long to reply).  I believe my difficulties are in
part due to a disconnect from use cases: there's a lot aliases could
enable us to do, and I keep getting lost in possibilities.  So let's try
to ground this by looking at QAPIfication of -chardev.  This way, we
discuss how to solve a specific problem in its entirety instead of
trying to come to a conclusion on a building block that may be useful
(but isn't sufficient) to solve an unclear set of problems (unclear to
*me*).

The equivalent QMP command is chardev-add.  The definition of its
argument is spread over several QAPI type definitions.  It boils down
to:

id: str
backend: ChardevBackend
type: ChardevBackendKindtag
data: ChardevFile   when file
*logfile: str
*logappend: bool

Re: [RFC PATCH: v3 1/2] add mi device in qemu

[Padmakar Kalghatgi]

On Wed, Sep 29, 2021 at 06:37:54AM +0200, Klaus Jensen wrote:

On Aug  3 12:54, Padmakar Kalghatgi wrote:

From: padmakar 

This patch contains the implementation of certain commands
of nvme-mi specification.The MI commands are useful to
manage/configure/monitor the device.Eventhough the MI commands
can be sent via the inband NVMe-MI send/recieve commands, the idea
here is to emulate the sideband interface for MI.

The changes here includes the interface for i2c/smbus
for nvme-mi protocol. We have used i2c address of 0x15
using which the guest VM can send and recieve the nvme-mi
commands. Since the nvme-mi device uses the I2C_SLAVE as
parent, we have used the send and recieve callbacks by
which the nvme-mi device will get the required notification.
With the callback approach, we have eliminated the use of
threads.

One needs to specify the following command in the launch to
specify the nvme-mi device, link to nvme and assign the i2c address.
<-device nvme-mi,nvme=nvme0,address=0x15>

This module makes use of the NvmeCtrl structure of the nvme module,
to fetch relevant information of the nvme device which are used in
some of the mi commands. Eventhough certain commands might require
modification to the nvme module, currently we have currently refrained
from making changes to the nvme module.

cmd-name  cmd-type
*
read nvme-mi dsnvme-mi
configuration set  nvme-mi
configuration get  nvme-mi
vpd read   nvme-mi
vpd write  nvme-mi
controller Health Staus Poll   nvme-mi
nvme subsystem health status poll  nvme-mi
identify   nvme-admin
get log page   nvme-admin
get features   nvme-admin

Signed-off-by: Padmakar Kalghatgi 
Reviewed-by: Klaus Birkelund Jensen 
Reviewed-by: Jaegyu Choi 

v3
-- rebased on master

---
 hw/nvme/meson.build |   2 +-
 hw/nvme/nvme-mi.c   | 650 
 hw/nvme/nvme-mi.h   | 297 
 3 files changed, 948 insertions(+), 1 deletion(-)
 create mode 100644 hw/nvme/nvme-mi.c
 create mode 100644 hw/nvme/nvme-mi.h



Some general comments.

* Please be consistent about MI vs Mi in naming. I have no preference,
 but NvmeMi is probably most aligned with existing style.

* hw/nvme generally does not mix declarations and code. Please move
 variables declarations to the top of their scope. And please get rid
 of the hungarian notation ({b,l,u,...}VarName naming) ;)

* I'd like that the header was cleaned up to not include stuff that
 isn't used.

* I understand that you'd like to not impact the hw/nvme/ctrl.c device
 too much, but the Identify, Get Log Page and Get Feature "passthru"
 commands could really benefit from using the same code as in
 hw/nvme/ctrl.c - this requires a bit of refactoring such that the
 data can be returned explicitly instead of being directly DMA'ed to
 the host.

* This is not compliant with the MCTP I2C/SMBus binding spec. The spec
 states that all transactions are based on the SMBus Block Write bus
 protocol. This means that responses require that the device (which is
 defined as an I2C slave must itself master the bus and do a Block
 Write to the message originator (the address of which is contained in
 the fourth byte of the packet).


diff --git a/hw/nvme/meson.build b/hw/nvme/meson.build
index 3cf4004..8dac50e 100644
--- a/hw/nvme/meson.build
+++ b/hw/nvme/meson.build
@@ -1 +1 @@
-softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c', 'dif.c', 
'ns.c', 'subsys.c'))
+softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c', 'dif.c', 
'ns.c', 'subsys.c', 'nvme-mi.c'))
diff --git a/hw/nvme/nvme-mi.c b/hw/nvme/nvme-mi.c
new file mode 100644
index 000..a90ce90
--- /dev/null
+++ b/hw/nvme/nvme-mi.c
@@ -0,0 +1,650 @@
+/*
+ * QEMU NVMe-MI Controller
+ *
+ * Copyright (c) 2021, Samsung Electronics co Ltd.
+ *
+ * Written by Padmakar Kalghatgi 
+ *Arun Kumar Agasar 
+ *Saurav Kumar 
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ */
+
+/**
+ * Reference Specs: 
https://protect2.fireeye.com/v1/url?k=b5aa98b4-d54805e9-b5ab13fb-000babd9f1ba-0224a06b55b4fe71&q=1&e=f0445d9e-6d0d-4fac-8d43-1785c5d98f8e&u=http%3A%2F%2Fwww.nvmexpress.org%2F,
+ *
+ *
+ * Usage
+ * -
+ * The nvme-mi device has to be used along with nvme device only
+ *
+ * Add options:
+ *-device  nvme-mi,nvme=,address=0x15",


Considering that NVMe-MI can run with various MCTP bindings, I think
this particular instance of it should be called 'nvme-mi-i2c'.
Supporting VDM on a PCIe binding is probably not really a goal at all,
but it's better to be explicit about this I think.


+ *
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-core.h"
+#include "hw/block/block.h"
+#include "hw/pci/msix.h"
+#include "nvme.h

[PATCH] target/mips: remove gen_mfc0_load64() and use tcg_gen_ld32s_tl()

[Philippe Mathieu-Daudé]

From: Leon Alrae 

Remove misleading gen_mfc0_load64() which actually loads 32 or 64 bits
depending whether MIPS32 or MIPS64 and also replace the pair of
tcg_gen_ld_tl() + tcg_gen_ext32s_tl() with single tcg_gen_ld32s_tl().

Patch partly generated using the following spatch script:

@@
expression reg, env, ofs;
@@
-tcg_gen_ld_tl(reg, env, ofs);
-tcg_gen_ext32s_tl(reg, reg);
+tcg_gen_ld32s_tl(reg, env, ofs);

Signed-off-by: Leon Alrae 
[PMD: Rebased and used Coccinelle spatch to complete]
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/tcg/translate.c | 68 -
 1 file changed, 29 insertions(+), 39 deletions(-)

diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index 148afec9dc0..40b350d6e17 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -5382,12 +5382,6 @@ static inline void gen_mfc0_load32(TCGv arg, 
target_ulong off)
 tcg_temp_free_i32(t0);
 }
 
-static inline void gen_mfc0_load64(TCGv arg, target_ulong off)
-{
-tcg_gen_ld_tl(arg, cpu_env, off);
-tcg_gen_ext32s_tl(arg, arg);
-}
-
 static inline void gen_mtc0_store32(TCGv arg, target_ulong off)
 {
 TCGv_i32 t0 = tcg_temp_new_i32();
@@ -5679,17 +5673,19 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
 break;
 case CP0_REG01__YQMASK:
 CP0_CHECK(ctx->insn_flags & ASE_MT);
-gen_mfc0_load64(arg, offsetof(CPUMIPSState, CP0_YQMask));
+tcg_gen_ld32s_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_YQMask));
 register_name = "YQMask";
 break;
 case CP0_REG01__VPESCHEDULE:
 CP0_CHECK(ctx->insn_flags & ASE_MT);
-gen_mfc0_load64(arg, offsetof(CPUMIPSState, CP0_VPESchedule));
+tcg_gen_ld32s_tl(arg, cpu_env,
+ offsetof(CPUMIPSState, CP0_VPESchedule));
 register_name = "VPESchedule";
 break;
 case CP0_REG01__VPESCHEFBACK:
 CP0_CHECK(ctx->insn_flags & ASE_MT);
-gen_mfc0_load64(arg, offsetof(CPUMIPSState, CP0_VPEScheFBack));
+tcg_gen_ld32s_tl(arg, cpu_env,
+ offsetof(CPUMIPSState, CP0_VPEScheFBack));
 register_name = "VPEScheFBack";
 break;
 case CP0_REG01__VPEOPT:
@@ -5790,8 +5786,7 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
 case CP0_REGISTER_04:
 switch (sel) {
 case CP0_REG04__CONTEXT:
-tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_Context));
-tcg_gen_ext32s_tl(arg, arg);
+tcg_gen_ld32s_tl(arg, cpu_env, offsetof(CPUMIPSState, 
CP0_Context));
 register_name = "Context";
 break;
 case CP0_REG04__CONTEXTCONFIG:
@@ -5801,9 +5796,8 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
 goto cp0_unimplemented;
 case CP0_REG04__USERLOCAL:
 CP0_CHECK(ctx->ulri);
-tcg_gen_ld_tl(arg, cpu_env,
-  offsetof(CPUMIPSState, active_tc.CP0_UserLocal));
-tcg_gen_ext32s_tl(arg, arg);
+tcg_gen_ld32s_tl(arg, cpu_env,
+ offsetof(CPUMIPSState, active_tc.CP0_UserLocal));
 register_name = "UserLocal";
 break;
 case CP0_REG04__MMID:
@@ -5828,20 +5822,20 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
 break;
 case CP0_REG05__SEGCTL0:
 CP0_CHECK(ctx->sc);
-tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_SegCtl0));
-tcg_gen_ext32s_tl(arg, arg);
+tcg_gen_ld32s_tl(arg, cpu_env,
+ offsetof(CPUMIPSState, CP0_SegCtl0));
 register_name = "SegCtl0";
 break;
 case CP0_REG05__SEGCTL1:
 CP0_CHECK(ctx->sc);
-tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_SegCtl1));
-tcg_gen_ext32s_tl(arg, arg);
+tcg_gen_ld32s_tl(arg, cpu_env,
+ offsetof(CPUMIPSState, CP0_SegCtl1));
 register_name = "SegCtl1";
 break;
 case CP0_REG05__SEGCTL2:
 CP0_CHECK(ctx->sc);
-tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_SegCtl2));
-tcg_gen_ext32s_tl(arg, arg);
+tcg_gen_ld32s_tl(arg, cpu_env,
+ offsetof(CPUMIPSState, CP0_SegCtl2));
 register_name = "SegCtl2";
 break;
 case CP0_REG05__PWBASE:
@@ -5917,8 +5911,8 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
 case CP0_REGISTER_08:
 switch (sel) {
 case CP0_REG08__BADVADDR:
-tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_BadVAddr));
-tcg_gen_ext32s_tl(arg, arg);
+tcg_gen_ld32s_tl(arg, cpu_env,
+

Re: [PATCH 1/2] nubus.h: add ULL suffix to NUBUS_SUPER_SLOT_SIZE and NUBUS_SUPER_SLOT_SIZE

[Philippe Mathieu-Daudé]

On 10/2/21 14:31, Mark Cave-Ayland wrote:
> Coverity thinks that the slot_offset multiplications in nubus_device_realize()
> might overflow because the resulting hwaddr is 64-bit whilst the 
> multiplication
> is only done at 32-bits.
> 
> Add an explicit ULL suffix to NUBUS_SUPER_SLOT_SIZE and NUBUS_SUPER_SLOT_SIZE
> to ensure that the multiplication is also done at 64-bits.
> 
> Fixes: Coverity CID 1464070
> Signed-off-by: Mark Cave-Ayland 
> ---
>  include/hw/nubus/nubus.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 2/2] nubus-device: ensure that name is freed after use in nubus_device_realize()

[Philippe Mathieu-Daudé]

On 10/2/21 14:31, Mark Cave-Ayland wrote:
> Coverity points out that there is memory leak because name is never freed 
> after
> use in nubus_device_realize().
> 
> Fixes: Coverity CID 1464062
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/nubus/nubus-device.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 01/12] macfb: handle errors that occur during realize

[Philippe Mathieu-Daudé]

On 10/2/21 12:59, Mark Cave-Ayland wrote:
> Make sure any errors that occur within the macfb realize chain are detected
> and handled correctly to prevent crashes and to ensure that error messages are
> reported back to the user.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/display/macfb.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 76808b69cc..2b747a8de8 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error 
> **errp)
>  MacfbState *ms = &s->macfb;
>  
>  macfb_common_realize(dev, ms, errp);
> +if (*errp) {
> +return;
> +}

See a related discussion:
https://lore.kernel.org/qemu-devel/87bl47ll9l@dusky.pond.sub.org/

Re: [PATCH 02/12] macfb: fix invalid object reference in macfb_common_realize()

[Philippe Mathieu-Daudé]

On 10/2/21 12:59, Mark Cave-Ayland wrote:
> During realize memory_region_init_ram_nomigrate() is used to initialise the 
> RAM
> memory region used for the framebuffer but the owner object reference is
> incorrect since MacFbState is a typedef and not a QOM type.
> 
> Change the memory region owner to be the corresponding DeviceState to fix the
> issue and prevent random crashes during macfb_common_realize().
> 

Fixes: 8ac919a0654 ("hw/m68k: add Nubus macfb video card")
Reviewed-by: Philippe Mathieu-Daudé 

> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/display/macfb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Re: [PATCH 04/12] macfb: use memory_region_init_ram() in macfb_common_realize() for the framebuffer

[Philippe Mathieu-Daudé]

On 10/2/21 12:59, Mark Cave-Ayland wrote:
> Currently macfb_common_realize() defines the framebuffer RAM memory region as
> being non-migrateable but then immediately registers it for migration. Replace
> memory_region_init_ram_nomigrate() with memory_region_init_ram() which is 
> clearer
> and does exactly the same thing.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/display/macfb.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 05/12] macfb: add trace events for reading and writing the control registers

[Philippe Mathieu-Daudé]

On 10/2/21 13:00, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/display/macfb.c  | 8 +++-
>  hw/display/trace-events | 4 
>  2 files changed, 11 insertions(+), 1 deletion(-)

> @@ -289,7 +290,10 @@ static uint64_t macfb_ctrl_read(void *opaque,
>  hwaddr addr,
>  unsigned int size)
>  {

> +# macfb.c
> +macfb_ctrl_read(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " 
> value 0x%"PRIx64 " size %d"
> +macfb_ctrl_write(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " 
> value 0x%"PRIx64 " size %d"
> 

'size' is unsigned, otherwise:
Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 07/12] macfb: add qdev property to specify display type

[Philippe Mathieu-Daudé]

On 10/2/21 13:00, Mark Cave-Ayland wrote:
> Since the available resolutions and colour depths are determined by the 
> attached
> display type, add a qdev property to allow the display type to be specified.
> 
> The main resolutions of interest are high resolution 1152x870 with 8-bit 
> colour
> and SVGA resolution up to 800x600 with 32-bit colour so update the q800 
> machine
> to allow high resolution mode if specified and otherwise fall back to SVGA.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/display/macfb.c | 6 +-
>  hw/m68k/q800.c | 5 +
>  include/hw/display/macfb.h | 1 +
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 5c95aa4a11..023d1f0cd1 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -316,7 +316,7 @@ static uint32_t macfb_sense_read(MacfbState *s)
>  MacFbSense *macfb_sense;
>  uint8_t sense;
>  

What about:

   assert(s->type < ARRAY_SIZE(macfb_sense_table));

> -macfb_sense = &macfb_sense_table[MACFB_DISPLAY_VGA];
> +macfb_sense = &macfb_sense_table[s->type];

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 12/12] q800: wire macfb IRQ to separate video interrupt on VIA2

[Philippe Mathieu-Daudé]

On 10/2/21 13:00, Mark Cave-Ayland wrote:
> Whilst the in-built Quadra 800 framebuffer exists within the Nubus address
> space for slot 9, it has its own dedicated interrupt on VIA2. Force the
> macfb device to occupy slot 9 in the q800 machine and wire its IRQ to the
> separate video interrupt since this is what is expected by the MacOS
> interrupt handler.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/m68k/q800.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [RFC PATCH 0/4] Misc OHCI patches

[BALATON Zoltan]

On Sat, 2 Oct 2021, Howard Spoelstra wrote:

Hi all,

I've booted Fedora12 and MacOS 9.2/OSX10.4 ppc with these patches applied.
All boot OK.
(Fedora12 requires -mac99,via=pmu. Here there seems to be some interference
with the kbd, due to via=pmu presenting usb mouse and kdb to the guest. So
I could not test this further.).
All tests done in Fedora 34 host with current master and patched build.

./qemu-system-ppc-ohcipatch02102021 \
-M mac99 \
-L pc-bios \
-display gtk \
-m 512 \
-boot c \
-hda /home/hsp/Mac-disks/10.4.qcow2 \
-device
usb-host,loglevel=4,vendorid=0x046d,productid=0x0a37,pcap=ohci-usb1headset-usb1port-ohcipatchV1-macos104.pcap
\
-trace "usb_*" -D ohci-usb1headset-usb1port-ohcipatchV1-macos104.txt

Endpoint 4 is the interrupt-based hid device, endpoint 1 would be the
isochronous audio stream if it showed up.
Each test consisted of booting to the desktop, opening the system
profiler/system report to check of the presence of the usb device, push the
various buttons on the headset, attempt to play a sound, unplug-replug and
attempt to play a sound again. Then shut down.

Both succesfully open the usb device:
usb_ohci_init_time usb_bit_time=100 usb_frame_time=83
usb_port_claim bus 0, port 1
usb_host_auto_scan_enabled
usb_ohci_reset pci-ohci
usb_ohci_stop pci-ohci: USB Suspended
usb_ohci_stop pci-ohci: USB Suspended
usb_host_open_started dev 3:29
usb_host_detach_kernel dev 3:29, if 0
usb_host_detach_kernel dev 3:29, if 1
usb_host_detach_kernel dev 3:29, if 2
usb_host_detach_kernel dev 3:29, if 3
usb_host_parse_config dev 3:29, value 1, active 1
usb_host_parse_interface dev 3:29, num 0, alt 0, active 1
usb_host_parse_interface dev 3:29, num 1, alt 0, active 1
usb_host_parse_interface dev 3:29, num 2, alt 0, active 1
usb_host_parse_interface dev 3:29, num 3, alt 0, active 1
usb_host_parse_endpoint dev 3:29, ep 4, in, int, active 1
usb_port_attach bus 0, port 1, devspeed full, portspeed full
usb_ohci_port_attach port #0
usb_host_open_success dev 3:29

Master with Mac OS 9.2:
usb_ohci_ed_pkt ED @ 0x00152920 h=0 c=0
 head=0x00164000 tailp=0x00164030 next=0x00152020
usb_ohci_ed_pkt_flags fa=37 en=4 d=2 s=0 k=0 f=0 mps=37
usb_ohci_td_skip_async


Maybe we need to look a bit before this when the async packet still 
waiting here is submitted and see what is that and why it's not 
completing. At this point we only have everything waiting for that.



usb_ohci_ed_pkt ED @ 0x00152900 h=0 c=0
 head=0x001645a0 tailp=0x00164660 next=0x001528a0
usb_ohci_ed_pkt_flags fa=37 en=0 d=0 s=0 k=0 f=0 mps=64
usb_ohci_td_pkt_hdr  TD @ 0x001645a0 8 of 8 bytes setup r=1 cbp=0x01661b90
be=0x01661b97
usb_ohci_td_pkt_full OUT data:  01 0b 00 00 01 00 00 00
usb_ohci_td_too_many_pending
usb_ohci_ed_pkt ED @ 0x00152920 h=0 c=0
 head=0x00164000 tailp=0x00164030 next=0x00152020
usb_ohci_ed_pkt_flags fa=37 en=4 d=2 s=0 k=0 f=0 mps=37
usb_ohci_td_skip_async
usb_ohci_ed_pkt ED @ 0x00152900 h=0 c=0
 head=0x001645a0 tailp=0x00164660 next=0x001528a0
usb_ohci_ed_pkt_flags fa=37 en=0 d=0 s=0 k=0 f=0 mps=64
usb_ohci_td_pkt_hdr  TD @ 0x001645a0 8 of 8 bytes setup r=1 cbp=0x01661b90
be=0x01661b97
usb_ohci_td_pkt_full OUT data:  01 0b 00 00 01 00 00 00
usb_ohci_td_too_many_pending
usb_ohci_ed_pkt ED @ 0x00152920 h=0 c=0
 head=0x00164000 tailp=0x00164030 next=0x00152020
usb_ohci_ed_pkt_flags fa=37 en=4 d=2 s=0 k=0 f=0 mps=37
usb_ohci_td_skip_async
usb_ohci_ed_pkt ED @ 0x00152900 h=0 c=0
 head=0x001645a0 tailp=0x00164660 next=0x001528a0
usb_ohci_ed_pkt_flags fa=37 en=0 d=0 s=0 k=0 f=0 mps=64
usb_ohci_td_pkt_hdr  TD @ 0x001645a0 8 of 8 bytes setup r=1 cbp=0x01661b90
be=0x01661b97
usb_ohci_td_pkt_full OUT data:  01 0b 00 00 01 00 00 00
usb_ohci_td_too_many_pending
usb_ohci_ed_pkt ED @ 0x00152920 h=0 c=0
 head=0x00164000 tailp=0x00164030 next=0x00152020
usb_ohci_ed_pkt_flags fa=37 en=4 d=2 s=0 k=0 f=0 mps=37
usb_ohci_td_skip_async
usb_ohci_ed_pkt ED @ 0x00152900 h=0 c=0
 head=0x001645a0 tailp=0x00164660 next=0x001528a0
usb_ohci_ed_pkt_flags fa=37 en=0 d=0 s=0 k=0 f=0 mps=64
usb_ohci_td_pkt_hdr  TD @ 0x001645a0 8 of 8 bytes setup r=1 cbp=0x01661b90
be=0x01661b97
usb_ohci_td_pkt_full OUT data:  01 0b 00 00 01 00 00 00
usb_ohci_td_too_many_pending
usb_ohci_ed_pkt ED @ 0x00152920 h=0 c=0
 head=0x00164000 tailp=0x00164030 next=0x00152020
usb_ohci_ed_pkt_flags fa=37 en=4 d=2 s=0 k=0 f=0 mps=37
usb_ohci_td_skip_async

OHCI patch with MacOS 9.2:
usb_ohci_ed_pkt ED @ 0x001528e0 h=0 c=0
 head=0x001609c0 tailp=0x001609f0 next=0x00152020
usb_ohci_ed_pkt_flags fa=41 en=4 d=2 s=0 k=0 f=0 mps=37
usb_ohci_td_skip_async
usb_ohci_ed_pkt ED @ 0x001528c0 h=0 c=0
 head=0x00161030 tailp=0x001610c0 next=0x001528a0
usb_ohci_ed_pkt_flags fa=41 en=0 d=0 s=0 k=0 f=0 mps=64
usb_ohci_td_pkt_hdr  TD @ 0x00161030 8 of 8 bytes setup r=1 cbp=0x01587b08
be=0x01587b0f
usb_ohci_td_pkt_full OUT data:  80 06 00 02 00 00 0a 00
usb_ohci_td_too_many_pending


This does not seem to have changed with the patch so maybe the iso 
transfer is not the cause as what the patch does is making

Re: [PATCH v3 10/41] linux-user/host/sparc: Populate host_signal.h

[Philippe Mathieu-Daudé]

On 10/1/21 19:11, Richard Henderson wrote:
> Split host_signal_pc and host_signal_write out of user-exec.c.
> Drop the *BSD code, to be re-created under bsd-user/ later.
> Drop the Solais code as completely unused.

Typo 'Solaris'.

> 
> Signed-off-by: Richard Henderson 
> ---
>  linux-user/host/sparc/host-signal.h   | 54 ++-
>  linux-user/host/sparc64/host-signal.h |  2 +-
>  accel/tcg/user-exec.c | 62 +--
>  3 files changed, 55 insertions(+), 63 deletions(-)

Re: [PATCH v3 11/41] linux-user/host/arm: Populate host_signal.h

[Philippe Mathieu-Daudé]

On 10/1/21 19:11, Richard Henderson wrote:
> Split host_signal_pc and host_signal_write out of user-exec.c.
> Drop the *BSD code, to be re-created under bsd-user/ later.
> 
> Signed-off-by: Richard Henderson 
> ---
>  linux-user/host/arm/host-signal.h | 30 -
>  accel/tcg/user-exec.c | 45 +--
>  2 files changed, 30 insertions(+), 45 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 21/41] target/alpha: Make alpha_cpu_tlb_fill sysemu only

[Philippe Mathieu-Daudé]

On 10/1/21 19:11, Richard Henderson wrote:
> The fallback code in raise_sigsegv is sufficient for alpha-linux-user.

Renamed to TCGCPUOps.record_sigsegv in patch #19 of this series.

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé 

> Remove the code from cpu_loop that handled EXCP_MMFAULT.
> 
> Signed-off-by: Richard Henderson 
> ---
>  target/alpha/cpu.h  |  7 ---
>  linux-user/alpha/cpu_loop.c |  8 
>  target/alpha/cpu.c  |  2 +-
>  target/alpha/helper.c   | 13 +
>  4 files changed, 6 insertions(+), 24 deletions(-)

Re: [PATCH v3 22/41] target/arm: Use cpu_loop_exit_segv for mte tag lookup

[Philippe Mathieu-Daudé]

On 10/1/21 19:11, Richard Henderson wrote:
> Use the new os interface for raising the exception,
> rather than calling arm_cpu_tlb_fill directly.
> 
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/mte_helper.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 28/41] target/m68k: Make m68k_cpu_tlb_fill sysemu only

[Philippe Mathieu-Daudé]

On 10/1/21 19:11, Richard Henderson wrote:
> The fallback code in raise_sigsegv is sufficient for m68k-linux-user.
> Remove the code from cpu_loop that handled EXCP_ACCESS.
> 
> Signed-off-by: Richard Henderson 
> ---
>  linux-user/m68k/cpu_loop.c | 10 --
>  target/m68k/cpu.c  |  2 +-
>  target/m68k/helper.c   |  6 +-
>  3 files changed, 2 insertions(+), 16 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 30/41] target/mips: Make mips_cpu_tlb_fill sysemu only

[Philippe Mathieu-Daudé]

On 10/1/21 19:11, Richard Henderson wrote:
> The fallback code in raise_sigsegv is sufficient for mips linux-user.
> This means we can remove tcg/user/tlb_helper.c entirely.
> Remove the code from cpu_loop that raised SIGSEGV.
> 
> Signed-off-by: Richard Henderson 
> ---
>  target/mips/tcg/tcg-internal.h|  7 ++--
>  linux-user/mips/cpu_loop.c| 11 --
>  target/mips/cpu.c |  2 +-
>  target/mips/tcg/user/tlb_helper.c | 59 ---
>  target/mips/tcg/meson.build   |  3 --
>  target/mips/tcg/user/meson.build  |  3 --
>  6 files changed, 5 insertions(+), 80 deletions(-)
>  delete mode 100644 target/mips/tcg/user/tlb_helper.c
>  delete mode 100644 target/mips/tcg/user/meson.build

Yay!

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 31/41] target/nios2: Implement nios2_cpu_record_sigsegv

[Philippe Mathieu-Daudé]

On 10/1/21 19:11, Richard Henderson wrote:
> Because the linux-user kuser page handling is currently implemented
> by detecting magic addresses in the unnamed 0xaa trap, we cannot
> simply remove nios2_cpu_tlb_fill and rely on the fallback code.
> 
> Signed-off-by: Richard Henderson 
> ---
>  target/nios2/cpu.h| 6 ++
>  target/nios2/cpu.c| 6 --
>  target/nios2/helper.c | 7 ---
>  3 files changed, 14 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 33/41] target/openrisc: Make openrisc_cpu_tlb_fill sysemu only

[Philippe Mathieu-Daudé]

On 10/1/21 19:11, Richard Henderson wrote:
> The fallback code in raise_sigsegv is sufficient for openrisc.
> This makes all of the code in mmu.c sysemu only, so remove
> the ifdefs and move the file to openrisc_softmmu_ss.
> Remove the code from cpu_loop that handled EXCP_DPF.
> 
> Cc: Stafford Horne 
> Signed-off-by: Richard Henderson 
> ---
>  target/openrisc/cpu.h  | 7 ---
>  linux-user/openrisc/cpu_loop.c | 8 
>  target/openrisc/cpu.c  | 2 +-
>  target/openrisc/mmu.c  | 8 
>  target/openrisc/meson.build| 2 +-
>  5 files changed, 6 insertions(+), 21 deletions(-)

>  static void raise_mmu_exception(OpenRISCCPU *cpu, target_ulong address,
>  int exception)
> @@ -113,7 +109,6 @@ bool openrisc_cpu_tlb_fill(CPUState *cs, vaddr addr, int 
> size,
>  OpenRISCCPU *cpu = OPENRISC_CPU(cs);
>  int excp = EXCP_DPF;
>  

^ Can we remove this extra line? Otherwise:
Reviewed-by: Philippe Mathieu-Daudé 

> -#ifndef CONFIG_USER_ONLY
>  int prot;
>  hwaddr phys_addr;
>  
> @@ -138,13 +133,11 @@ bool openrisc_cpu_tlb_fill(CPUState *cs, vaddr addr, 
> int size,
>  if (probe) {
>  return false;
>  }
> -#endif

Re: [PATCH v3 35/41] target/riscv: Make riscv_cpu_tlb_fill sysemu only

[Philippe Mathieu-Daudé]

On 10/1/21 19:11, Richard Henderson wrote:
> The fallback code in raise_sigsegv is sufficient for riscv.
> Remove the code from cpu_loop that raised SIGSEGV.
> 
> Cc: qemu-ri...@nongnu.org
> Signed-off-by: Richard Henderson 
> ---
>  linux-user/riscv/cpu_loop.c |  7 ---
>  target/riscv/cpu.c  |  2 +-
>  target/riscv/cpu_helper.c   | 21 +
>  3 files changed, 2 insertions(+), 28 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 40/41] target/xtensa: Make xtensa_cpu_tlb_fill sysemu only

[Philippe Mathieu-Daudé]

On 10/1/21 19:11, Richard Henderson wrote:
> The fallback code in raise_sigsegv is sufficient for xtensa.
> Remove the code from cpu_loop that raised SIGSEGV.
> 
> Cc: Max Filippov 
> Signed-off-by: Richard Henderson 
> ---
>  target/xtensa/cpu.h  |  2 +-
>  linux-user/xtensa/cpu_loop.c |  9 -
>  target/xtensa/cpu.c  |  2 +-
>  target/xtensa/helper.c   | 22 +-
>  4 files changed, 3 insertions(+), 32 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 41/41] accel/tcg: Restrict TCGCPUOps::tlb_fill() to sysemu

[Philippe Mathieu-Daudé]

On 10/1/21 19:11, Richard Henderson wrote:
> We have replaced tlb_fill with record_sigsegv for user mod.

Typo "user mode"?

> Move the declaration to restrict it to system emulation.
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Richard Henderson 
> ---
>  include/hw/core/tcg-cpu-ops.h | 22 ++
>  linux-user/signal.c   |  3 ---
>  2 files changed, 10 insertions(+), 15 deletions(-)

Re: [PATCH v3 39/41] target/sparc: Make sparc_cpu_tlb_fill sysemu only

[Philippe Mathieu-Daudé]

On 10/1/21 19:11, Richard Henderson wrote:
> The fallback code in raise_sigsegv is sufficient for sparc.
> 
> This makes all of the code in mmu_helper.c sysemu only, so remove
> the ifdefs and move the file to sparc_softmmu_ss.  Remove the code
> from cpu_loop that handled TT_DFAULT and TT_TFAULT.
> 
> Cc: Mark Cave-Ayland 
> Signed-off-by: Richard Henderson 
> ---
>  linux-user/sparc/cpu_loop.c | 25 -
>  target/sparc/cpu.c  |  2 +-
>  target/sparc/mmu_helper.c   | 25 -
>  target/sparc/meson.build|  2 +-
>  4 files changed, 2 insertions(+), 52 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [RFC PATCH 0/4] Misc OHCI patches

[BALATON Zoltan]

On Fri, 1 Oct 2021, BALATON Zoltan wrote:

Posted as RFC because it's unfinfished and untested as there seems to
be some regression with mac99 so it does not boot for me for some
reason I haven't debugged yet. Hope Howard can test it and see if it
changes any of the traces seen before.


I've now tried it with MorphOS on mac99 (needs a new patched OpenBIOS as 
the previous one stopped working with recent escc reset changes, see 
https://mail.coreboot.org/hyperkitty/list/openb...@openbios.org/thread/PE6B3HSQB6XIIELYJP6GJMR3L4RIUANR/ 
) but I don't have a usb audio device to pass through so tried with the 
emulated -device usb-audio instead. It's recognised both before and after 
this patch but does not make any sound. While info about the device shows 
up it seems to get packet NAKs when it's accessed and packet data is all 
zeros so maybe there's some problem decoding OHCI structures from the 
guest? It also has two audio endpoints and the first one is disabled, 
MorphOS seems to try to use the second one, Not sure where these are 
connected or is the first one an input for recording?


Interestingly on pegasos2 which has UHCI instead of OHCI MorphOS does not 
even correctly detect the emulated usb-audio device and trying to get info 
makes it freeze so looks like the UHCI model may also have some problems 
of its own.


On sam460ex that has EHCI it does the same as on mac99, detects usb-audio, 
does not freeze but does not make sound either. I think this would need 
someone with more understanding of the wotkings of usb-audio to debug 
this.


Regards,
BALATON Zoltan

Re: [PATCH] docs/devel: memory: Document MemoryRegionOps requirement

[Bin Meng]

On Thu, Sep 9, 2021 at 4:17 AM Philippe Mathieu-Daudé  wrote:
>
> On 9/8/21 8:50 PM, Peter Xu wrote:
> > On Mon, Sep 06, 2021 at 03:01:54PM +0200, Philippe Mathieu-Daudé wrote:
> >> On 9/6/21 2:20 PM, Bin Meng wrote:
> >>> It's been a requirement that at least one function pointer for read
> >>> and one for write are provided ever since the MemoryRegion APIs were
> >>> introduced in 2012.
> >>>
> >>> Signed-off-by: Bin Meng 
> >>> ---
> >>>
> >>>  docs/devel/memory.rst | 5 +
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
> >>> index 5dc8a12682..7b589b21d2 100644
> >>> --- a/docs/devel/memory.rst
> >>> +++ b/docs/devel/memory.rst
> >>> @@ -344,6 +344,11 @@ based on the attributes used for the memory 
> >>> transaction, or need
> >>>  to be able to respond that the access should provoke a bus error
> >>>  rather than completing successfully; those devices can use the
> >>>  ->read_with_attrs() and ->write_with_attrs() callbacks instead.
> >>> +The requirement for a device's MemoryRegionOps is that at least
> >>> +one callback for read and one for write are provided. If both
> >>> +->read() and ->read_with_attrs() are provided, the plain ->read()
> >>> +version takes precedence over the with_attrs() version. So does
> >>> +the write callback.
> >>
> >> What about also adding a runtime check?
> >>
> >> -- >8 --
> >> diff --git a/softmmu/memory.c b/softmmu/memory.c
> >> index bfedaf9c4df..8ab602d3379 100644
> >> --- a/softmmu/memory.c
> >> +++ b/softmmu/memory.c
> >> @@ -1516,6 +1516,17 @@ MemTxResult
> >> memory_region_dispatch_write(MemoryRegion *mr,
> >>  }
> >>  }
> >>
> >> +static void memory_region_set_ops(MemoryRegion *mr, const
> >> MemoryRegionOps *ops)
> >> +{
> >> +if (ops) {
> >> +assert(ops->valid.accepts || (ops->read || ops->read_with_attrs));
> >> +assert(ops->valid.accepts || (ops->write ||
> >> ops->write_with_attrs));
> >
> > Curious why accepts() matters.. Say, if there's only accepts() provided and 
> > it
> > returned true, then I think we still can't avoid the coredump when 
> > read/write?
>
> Good point :(
>
> > I'm also curious what's the issue that Paolo mentioned here:
> >
> > https://lore.kernel.org/qemu-devel/8da074de-7dff-6505-5180-720cf2f47...@redhat.com/
> >
> > I believe Paolo was referring to this series from Prasad:
> >
> > https://lore.kernel.org/qemu-devel/2020084133.672647-10-ppan...@redhat.com/
> >
> > We may need to solve that issue then maybe we can consider revive Prasad's
> > patchset?

It looks this patch is not applied. Given it's a doc improvement for
current implementation, I think we should apply this, and future
enhancement should be done in separate series?

Regards,
Bin

Re: [RFC PATCH 0/4] Misc OHCI patches

[BALATON Zoltan]

On Sat, 2 Oct 2021, Howard Spoelstra wrote:

Both have issues communicating with endpoint 4 (the hid controls volume
up/down and mute).
Endpoint 1 should receive the isochronous audio stream, but never does.

After some experimentation with unplugging/plugging the headset and probing
the usb stack (using the usb prober from the mac usb ddk for Mac OS 9.2) at
some point endpoint 4 communication works for both guests tested. Only once
was I able to get sound out and in working in Mac OS 9.2. For OSX I could
only once get audio in working.


The async packets are on endpoint 0. I'm not sure the HID endpoint 4 is 
normally involved at all unless you push some buttons but it should work 
without that so maybe look at the 0 and the audio endpoints instead of 
HID that should have no traffic unless you press buttons.



Pcap and text logs for both MacOS 9.2 and OSX 10.4 tests included...


These are way too big to be possible to find anything in them. Maybe you 
should do something simpler like boot the guest without the device 
attached and discard all logs up to that point. Then start collecting logs 
and attach device and play a short sound. Stop collecting log and try to 
make sense of where that fails. That's still a lot of traces but should 
only contain the relevant info of detecing the device and playing a sound 
not a lot of others that makes it difficult to find what's relevant.


I'm also not sure where's the problem (maybe we have multiple problems). 
It also does not work with an emulated usb-audio device but that also 
doesn't work with EHCI so it may have a problem by itself (UHCI also seems 
to be broken on its own so it does not even work as much as OHCI and 
EHCI). You've also said it worked with pass through with a different 
device so maybe this only happens with some devices or some sequence of 
things these devices are doing. Only allowing one async packet in OHCI 
instead of allowing one per endpoint is just a guess that could cause 
delays in processing other packets but eventually it should go through 
unless one async packet never completes and blocks all later ones or the 
delays introduced by this limitaion causes things to go in a way that 
guests don't expect and thus fail.


Regards,
BALATON Zoltan

[PATCH v3 3/3] hw/mips/boston: Add FDT generator

[Jiaxun Yang]

Generate FDT on our own if no dtb argument supplied.
Avoid introducing unused device in FDT with user supplied dtb.

Signed-off-by: Jiaxun Yang 
--
v2: Address f4bug cmments (Thanks!)
---
 hw/mips/boston.c | 234 +--
 1 file changed, 226 insertions(+), 8 deletions(-)

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index e6d5cc2d22..d0935dbc79 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -49,6 +49,15 @@ typedef struct BostonState BostonState;
 DECLARE_INSTANCE_CHECKER(BostonState, BOSTON,
  TYPE_BOSTON)
 
+#define FDT_IRQ_TYPE_NONE   0
+#define FDT_IRQ_TYPE_LEVEL_HIGH 4
+#define FDT_GIC_SHARED  0
+#define FDT_GIC_LOCAL   1
+#define FDT_BOSTON_CLK_SYS  1
+#define FDT_BOSTON_CLK_CPU  2
+#define FDT_PCI_IRQ_MAP_PINS4
+#define FDT_PCI_IRQ_MAP_DESCS   6
+
 struct BostonState {
 SysBusDevice parent_obj;
 
@@ -435,6 +444,213 @@ xilinx_pcie_init(MemoryRegion *sys_mem, uint32_t bus_nr,
 return XILINX_PCIE_HOST(dev);
 }
 
+
+static void fdt_create_pcie(void *fdt, int gic_ph, int irq, hwaddr reg_base,
+hwaddr reg_size, hwaddr mmio_base, hwaddr 
mmio_size)
+{
+int i;
+char *name, *intc_name;
+uint32_t intc_ph;
+uint32_t interrupt_map[FDT_PCI_IRQ_MAP_PINS][FDT_PCI_IRQ_MAP_DESCS];
+
+intc_ph = qemu_fdt_alloc_phandle(fdt);
+name = g_strdup_printf("/soc/pci@%" HWADDR_PRIx, reg_base);
+qemu_fdt_add_subnode(fdt, name);
+qemu_fdt_setprop_string(fdt, name, "compatible", 
"xlnx,axi-pcie-host-1.00.a");
+qemu_fdt_setprop_string(fdt, name, "device_type", "pci");
+qemu_fdt_setprop_cells(fdt, name, "reg", reg_base, reg_size);
+
+qemu_fdt_setprop_cell(fdt, name, "#address-cells", 3);
+qemu_fdt_setprop_cell(fdt, name, "#size-cells", 2);
+qemu_fdt_setprop_cell(fdt, name, "#interrupt-cells", 1);
+
+qemu_fdt_setprop_cell(fdt, name, "interrupt-parent", gic_ph);
+qemu_fdt_setprop_cells(fdt, name, "interrupts", FDT_GIC_SHARED, irq,
+FDT_IRQ_TYPE_LEVEL_HIGH);
+
+qemu_fdt_setprop_cells(fdt, name, "ranges", 0x0200, 0, mmio_base,
+mmio_base, 0, mmio_size);
+qemu_fdt_setprop_cells(fdt, name, "bus-range", 0x00, 0xff);
+
+
+
+intc_name = g_strdup_printf("%s/interrupt-controller", name);
+qemu_fdt_add_subnode(fdt, intc_name);
+qemu_fdt_setprop(fdt, intc_name, "interrupt-controller", NULL, 0);
+qemu_fdt_setprop_cell(fdt, intc_name, "#address-cells", 0);
+qemu_fdt_setprop_cell(fdt, intc_name, "#interrupt-cells", 1);
+qemu_fdt_setprop_cell(fdt, intc_name, "phandle", intc_ph);
+
+qemu_fdt_setprop_cells(fdt, name, "interrupt-map-mask", 0, 0, 0, 7);
+for (i = 0; i < FDT_PCI_IRQ_MAP_PINS; i++) {
+uint32_t *irqmap = interrupt_map[i];
+
+irqmap[0] = cpu_to_be32(0);
+irqmap[1] = cpu_to_be32(0);
+irqmap[2] = cpu_to_be32(0);
+irqmap[3] = cpu_to_be32(i + 1);
+irqmap[4] = cpu_to_be32(intc_ph);
+irqmap[5] = cpu_to_be32(i + 1);
+}
+qemu_fdt_setprop(fdt, name, "interrupt-map", &interrupt_map, 
sizeof(interrupt_map));
+
+g_free(intc_name);
+g_free(name);
+}
+
+static const void *create_fdt(BostonState *s, const MemMapEntry *memmap, int 
*dt_size)
+{
+void *fdt;
+int cpu;
+MachineState *mc = s->mach;
+uint32_t platreg_ph, gic_ph, clk_ph;
+char *name, *gic_name, *platreg_name, *stdout_name;
+static const char * const syscon_compat[2] = {"img,boston-platform-regs", 
"syscon"};
+
+fdt = create_device_tree(dt_size);
+if (!fdt) {
+error_report("create_device_tree() failed");
+exit(1);
+}
+
+platreg_ph = qemu_fdt_alloc_phandle(fdt);
+gic_ph = qemu_fdt_alloc_phandle(fdt);
+clk_ph = qemu_fdt_alloc_phandle(fdt);
+
+qemu_fdt_setprop_string(fdt, "/", "model", "img,boston");
+qemu_fdt_setprop_string(fdt, "/", "compatible", "img,boston");
+qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x1);
+qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x1);
+
+
+qemu_fdt_add_subnode(fdt, "/cpus");
+qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
+qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
+
+for (cpu = 0; cpu < mc->smp.cpus; cpu++) {
+name = g_strdup_printf("/cpus/cpu@%d", cpu);
+qemu_fdt_add_subnode(fdt, name);
+qemu_fdt_setprop_string(fdt, name, "compatible", "img,mips");
+qemu_fdt_setprop_string(fdt, name, "status", "okay");
+qemu_fdt_setprop_cell(fdt, name, "reg", cpu);
+qemu_fdt_setprop_string(fdt, name, "device_type", "cpu");
+qemu_fdt_setprop_cells(fdt, name, "clocks", clk_ph, 
FDT_BOSTON_CLK_CPU);
+g_free(name);
+}
+
+qemu_fdt_add_subnode(fdt, "/soc");
+qemu_fdt_setprop(fdt, "/soc", "ranges", NULL, 0);
+qemu_fdt_setprop_string(fdt, "/soc", "compatible", "simple-bus");
+qemu_fdt_setprop_cell(fdt, "/soc", "#s

[PATCH v3 0/3] hw/mips/boston: ELF kernel support

[Jiaxun Yang]

Jiaxun Yang (3):
  hw/mips/boston: Massage memory map information
  hw/mips/boston: Allow loading elf kernel and dtb
  hw/mips/boston: Add FDT generator

 hw/mips/boston.c | 348 +++
 1 file changed, 320 insertions(+), 28 deletions(-)

-- 
2.30.2

[PATCH v3 1/3] hw/mips/boston: Massage memory map information

[Jiaxun Yang]

Use memmap array to uinfy address of memory map.
That would allow us reuse address information for FDT generation.

Signed-off-by: Jiaxun Yang 
Reviewed-by: Philippe Mathieu-Daudé 
--
v2: Fix minor style issue, fix uart map size
---
 hw/mips/boston.c | 95 
 1 file changed, 71 insertions(+), 24 deletions(-)

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 20b06865b2..5c720440fb 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -64,6 +64,44 @@ struct BostonState {
 hwaddr fdt_base;
 };
 
+enum {
+BOSTON_LOWDDR,
+BOSTON_PCIE0,
+BOSTON_PCIE1,
+BOSTON_PCIE2,
+BOSTON_PCIE2_MMIO,
+BOSTON_CM,
+BOSTON_GIC,
+BOSTON_CDMM,
+BOSTON_CPC,
+BOSTON_PLATREG,
+BOSTON_UART,
+BOSTON_LCD,
+BOSTON_FLASH,
+BOSTON_PCIE1_MMIO,
+BOSTON_PCIE0_MMIO,
+BOSTON_HIGHDDR,
+};
+
+static const MemMapEntry boston_memmap[] = {
+[BOSTON_LOWDDR] = {0x0,0x1000 },
+[BOSTON_PCIE0] =  { 0x1000, 0x200 },
+[BOSTON_PCIE1] =  { 0x1200, 0x200 },
+[BOSTON_PCIE2] =  { 0x1400, 0x200 },
+[BOSTON_PCIE2_MMIO] = { 0x1600,  0x10 },
+[BOSTON_CM] = { 0x1610,   0x2 },
+[BOSTON_GIC] ={ 0x1612,   0x2 },
+[BOSTON_CDMM] =   { 0x1614,0x8000 },
+[BOSTON_CPC] ={ 0x1620,0x8000 },
+[BOSTON_PLATREG] ={ 0x17ffd000,0x1000 },
+[BOSTON_UART] =   { 0x17ffe000,  0x20 },
+[BOSTON_LCD] ={ 0x17fff000,   0x8 },
+[BOSTON_FLASH] =  { 0x1800, 0x800 },
+[BOSTON_PCIE1_MMIO] = { 0x2000,0x2000 },
+[BOSTON_PCIE0_MMIO] = { 0x4000,0x4000 },
+[BOSTON_HIGHDDR] ={ 0x8000,   0x0 },
+};
+
 enum boston_plat_reg {
 PLAT_FPGA_BUILD = 0x00,
 PLAT_CORE_CL= 0x04,
@@ -275,24 +313,22 @@ type_init(boston_register_types)
 
 static void gen_firmware(uint32_t *p, hwaddr kernel_entry, hwaddr fdt_addr)
 {
-const uint32_t cm_base = 0x1610;
-const uint32_t gic_base = 0x1612;
-const uint32_t cpc_base = 0x1620;
-
 /* Move CM GCRs */
 bl_gen_write_ulong(&p,
cpu_mips_phys_to_kseg1(NULL, GCR_BASE_ADDR + 
GCR_BASE_OFS),
-   cm_base);
+   boston_memmap[BOSTON_CM].base);
 
 /* Move & enable GIC GCRs */
 bl_gen_write_ulong(&p,
-   cpu_mips_phys_to_kseg1(NULL, cm_base + 
GCR_GIC_BASE_OFS),
-   gic_base | GCR_GIC_BASE_GICEN_MSK);
+   cpu_mips_phys_to_kseg1(NULL,
+boston_memmap[BOSTON_CM].base + GCR_GIC_BASE_OFS),
+   boston_memmap[BOSTON_GIC].base | 
GCR_GIC_BASE_GICEN_MSK);
 
 /* Move & enable CPC GCRs */
 bl_gen_write_ulong(&p,
-   cpu_mips_phys_to_kseg1(NULL, cm_base + 
GCR_CPC_BASE_OFS),
-   cpc_base | GCR_CPC_BASE_CPCEN_MSK);
+   cpu_mips_phys_to_kseg1(NULL,
+boston_memmap[BOSTON_CM].base + GCR_CPC_BASE_OFS),
+   boston_memmap[BOSTON_CPC].base | 
GCR_CPC_BASE_CPCEN_MSK);
 
 /*
  * Setup argument registers to follow the UHI boot protocol:
@@ -333,8 +369,9 @@ static const void *boston_fdt_filter(void *opaque, const 
void *fdt_orig,
 ram_low_sz = MIN(256 * MiB, machine->ram_size);
 ram_high_sz = machine->ram_size - ram_low_sz;
 qemu_fdt_setprop_sized_cells(fdt, "/memory@0", "reg",
- 1, 0x, 1, ram_low_sz,
- 1, 0x9000, 1, ram_high_sz);
+ 1, boston_memmap[BOSTON_LOWDDR].base, 1, 
ram_low_sz,
+ 1, boston_memmap[BOSTON_HIGHDDR].base + 
ram_low_sz,
+ 1, ram_high_sz);
 
 fdt = g_realloc(fdt, fdt_totalsize(fdt));
 qemu_fdt_dumpdtb(fdt, fdt_sz);
@@ -438,11 +475,13 @@ static void boston_mach_init(MachineState *machine)
 sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
 
 flash =  g_new(MemoryRegion, 1);
-memory_region_init_rom(flash, NULL, "boston.flash", 128 * MiB,
+memory_region_init_rom(flash, NULL, "boston.flash", 
boston_memmap[BOSTON_FLASH].size,
&error_fatal);
-memory_region_add_subregion_overlap(sys_mem, 0x1800, flash, 0);
+memory_region_add_subregion_overlap(sys_mem, 
boston_memmap[BOSTON_FLASH].base,
+flash, 0);
 
-memory_region_add_subregion_overlap(sys_mem, 0x8000, machine->ram, 0);
+memory_region_add_subregion_overlap(sys_mem, 
boston_memmap[BOSTON_HIGHDDR].base,
+machine->ram, 0);
 
 ddr_low_alias = g_new(MemoryRegion, 1);
 memory_region_init_alias(ddr_low_alias, NULL, "boston_low.ddr

[PATCH v3 2/3] hw/mips/boston: Allow loading elf kernel and dtb

[Jiaxun Yang]

ELF kernel allows us debugging much easier with DWARF symbols.

Signed-off-by: Jiaxun Yang 
Reviewed-by: Philippe Mathieu-Daudé 
--
v2: Use g_autofree
v3: Remove unused kernel_low and uint64_t cast (BALATON)
---
 hw/mips/boston.c | 35 +++
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 5c720440fb..e6d5cc2d22 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -20,6 +20,7 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 
+#include "elf.h"
 #include "hw/boards.h"
 #include "hw/char/serial.h"
 #include "hw/ide/pci.h"
@@ -546,10 +547,36 @@ static void boston_mach_init(MachineState *machine)
 exit(1);
 }
 } else if (machine->kernel_filename) {
-fit_err = load_fit(&boston_fit_loader, machine->kernel_filename, s);
-if (fit_err) {
-error_report("unable to load FIT image");
-exit(1);
+uint64_t kernel_entry, kernel_high, kernel_size;
+
+kernel_size = load_elf(machine->kernel_filename, NULL,
+   cpu_mips_kseg0_to_phys, NULL,
+   &kernel_entry, NULL, &kernel_high,
+   NULL, 0, EM_MIPS, 1, 0);
+
+if (kernel_size) {
+hwaddr dtb_paddr = QEMU_ALIGN_UP(kernel_high, 64 * KiB);
+hwaddr dtb_vaddr = cpu_mips_phys_to_kseg0(NULL, dtb_paddr);
+
+s->kernel_entry = kernel_entry;
+if (machine->dtb) {
+int dt_size;
+g_autofree const void *dtb_file_data, *dtb_load_data;
+
+dtb_file_data = load_device_tree(machine->dtb, &dt_size);
+dtb_load_data = boston_fdt_filter(s, dtb_file_data, NULL, 
&dtb_vaddr);
+
+/* Calculate real fdt size after filter */
+dt_size = fdt_totalsize(dtb_load_data);
+rom_add_blob_fixed("dtb", dtb_load_data, dt_size, dtb_paddr);
+}
+} else {
+/* Try to load file as FIT */
+fit_err = load_fit(&boston_fit_loader, machine->kernel_filename, 
s);
+if (fit_err) {
+error_report("unable to load kernel image");
+exit(1);
+}
 }
 
 gen_firmware(memory_region_get_ram_ptr(flash) + 0x7c0,
-- 
2.30.2

Re: [PATCH] target/mips: remove gen_mfc0_load64() and use tcg_gen_ld32s_tl()

[Jiaxun Yang]

在 2021/10/2 14:37, Philippe Mathieu-Daudé 写道:

From: Leon Alrae 

Remove misleading gen_mfc0_load64() which actually loads 32 or 64 bits
depending whether MIPS32 or MIPS64 and also replace the pair of
tcg_gen_ld_tl() + tcg_gen_ext32s_tl() with single tcg_gen_ld32s_tl().

Patch partly generated using the following spatch script:

 @@
 expression reg, env, ofs;
 @@
 -tcg_gen_ld_tl(reg, env, ofs);
 -tcg_gen_ext32s_tl(reg, reg);
 +tcg_gen_ld32s_tl(reg, env, ofs);

Signed-off-by: Leon Alrae 
[PMD: Rebased and used Coccinelle spatch to complete]
Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Jiaxun Yang 

That seems much more clear!
Thanks.

- Jiaxun

---
  target/mips/tcg/translate.c | 68 -
  1 file changed, 29 insertions(+), 39 deletions(-)

diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index 148afec9dc0..40b350d6e17 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -5382,12 +5382,6 @@ static inline void gen_mfc0_load32(TCGv arg, 
target_ulong off)
  tcg_temp_free_i32(t0);
  }
  
-static inline void gen_mfc0_load64(TCGv arg, target_ulong off)

-{
-tcg_gen_ld_tl(arg, cpu_env, off);
-tcg_gen_ext32s_tl(arg, arg);
-}
-
  static inline void gen_mtc0_store32(TCGv arg, target_ulong off)
  {
  TCGv_i32 t0 = tcg_temp_new_i32();
@@ -5679,17 +5673,19 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
  break;
  case CP0_REG01__YQMASK:
  CP0_CHECK(ctx->insn_flags & ASE_MT);
-gen_mfc0_load64(arg, offsetof(CPUMIPSState, CP0_YQMask));
+tcg_gen_ld32s_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_YQMask));
  register_name = "YQMask";
  break;
  case CP0_REG01__VPESCHEDULE:
  CP0_CHECK(ctx->insn_flags & ASE_MT);
-gen_mfc0_load64(arg, offsetof(CPUMIPSState, CP0_VPESchedule));
+tcg_gen_ld32s_tl(arg, cpu_env,
+ offsetof(CPUMIPSState, CP0_VPESchedule));
  register_name = "VPESchedule";
  break;
  case CP0_REG01__VPESCHEFBACK:
  CP0_CHECK(ctx->insn_flags & ASE_MT);
-gen_mfc0_load64(arg, offsetof(CPUMIPSState, CP0_VPEScheFBack));
+tcg_gen_ld32s_tl(arg, cpu_env,
+ offsetof(CPUMIPSState, CP0_VPEScheFBack));
  register_name = "VPEScheFBack";
  break;
  case CP0_REG01__VPEOPT:
@@ -5790,8 +5786,7 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
  case CP0_REGISTER_04:
  switch (sel) {
  case CP0_REG04__CONTEXT:
-tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_Context));
-tcg_gen_ext32s_tl(arg, arg);
+tcg_gen_ld32s_tl(arg, cpu_env, offsetof(CPUMIPSState, 
CP0_Context));
  register_name = "Context";
  break;
  case CP0_REG04__CONTEXTCONFIG:
@@ -5801,9 +5796,8 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
  goto cp0_unimplemented;
  case CP0_REG04__USERLOCAL:
  CP0_CHECK(ctx->ulri);
-tcg_gen_ld_tl(arg, cpu_env,
-  offsetof(CPUMIPSState, active_tc.CP0_UserLocal));
-tcg_gen_ext32s_tl(arg, arg);
+tcg_gen_ld32s_tl(arg, cpu_env,
+ offsetof(CPUMIPSState, active_tc.CP0_UserLocal));
  register_name = "UserLocal";
  break;
  case CP0_REG04__MMID:
@@ -5828,20 +5822,20 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
  break;
  case CP0_REG05__SEGCTL0:
  CP0_CHECK(ctx->sc);
-tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_SegCtl0));
-tcg_gen_ext32s_tl(arg, arg);
+tcg_gen_ld32s_tl(arg, cpu_env,
+ offsetof(CPUMIPSState, CP0_SegCtl0));
  register_name = "SegCtl0";
  break;
  case CP0_REG05__SEGCTL1:
  CP0_CHECK(ctx->sc);
-tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_SegCtl1));
-tcg_gen_ext32s_tl(arg, arg);
+tcg_gen_ld32s_tl(arg, cpu_env,
+ offsetof(CPUMIPSState, CP0_SegCtl1));
  register_name = "SegCtl1";
  break;
  case CP0_REG05__SEGCTL2:
  CP0_CHECK(ctx->sc);
-tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_SegCtl2));
-tcg_gen_ext32s_tl(arg, arg);
+tcg_gen_ld32s_tl(arg, cpu_env,
+ offsetof(CPUMIPSState, CP0_SegCtl2));
  register_name = "SegCtl2";
  break;
  case CP0_REG05__PWBASE:
@@ -5917,8 +5911,8 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
  case CP0_REGISTER_08:
  switch (sel) {
  case CP0_REG08__B

Re: [PULL 00/13] QAPI patches patches for 2021-10-02

[Richard Henderson]

On 10/2/21 5:56 AM, Markus Armbruster wrote:

The following changes since commit 5f992102383ed8ed97076548e1c897c7034ed8a4:

   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into 
staging (2021-10-01 13:44:36 -0400)

are available in the Git repository at:

   git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2021-10-02

for you to fetch changes up to d183e0481b1510b253ac94e702c76115f3bb6450:

   qapi/parser: enable pylint checks (2021-10-02 07:33:42 +0200)


QAPI patches patches for 2021-10-02


John Snow (13):
   qapi/pylintrc: ignore 'consider-using-f-string' warning
   qapi/gen: use dict.items() to iterate over _modules
   qapi/parser: fix unused check_args_section arguments
   qapi: Add spaces after symbol declaration for consistency
   qapi/parser: remove FIXME comment from _append_body_line
   qapi/parser: clarify _end_section() logic
   qapi/parser: Introduce NullSection
   qapi/parser: add import cycle workaround
   qapi/parser: add type hint annotations (QAPIDoc)
   qapi/parser: Add FIXME for consolidating JSON-related types
   qapi/parser: enable mypy checks
   qapi/parser: Silence too-few-public-methods warning
   qapi/parser: enable pylint checks


Thanks, applied.

r~

Re: [PATCH] target/mips: remove gen_mfc0_load64() and use tcg_gen_ld32s_tl()

[Richard Henderson]

On 10/2/21 9:37 AM, Philippe Mathieu-Daudé wrote:

From: Leon Alrae

Remove misleading gen_mfc0_load64() which actually loads 32 or 64 bits
depending whether MIPS32 or MIPS64 and also replace the pair of
tcg_gen_ld_tl() + tcg_gen_ext32s_tl() with single tcg_gen_ld32s_tl().

Patch partly generated using the following spatch script:

 @@
 expression reg, env, ofs;
 @@
 -tcg_gen_ld_tl(reg, env, ofs);
 -tcg_gen_ext32s_tl(reg, reg);
 +tcg_gen_ld32s_tl(reg, env, ofs);

Signed-off-by: Leon Alrae
[PMD: Rebased and used Coccinelle spatch to complete]
Signed-off-by: Philippe Mathieu-Daudé
---
  target/mips/tcg/translate.c | 68 -
  1 file changed, 29 insertions(+), 39 deletions(-)


Reviewed-by: Richard Henderson 

r~