Re: [PATCH] qemu: don't add --mac-addr option to passt commandline

[Laszlo Ersek]

On 7/13/23 18:30, Laine Stump wrote:
> When I implemented passt support in libvirt, I saw the --mac-addr
> option on the passt commandline, immediately assumed that this was
> used for setting the guest interface's mac address somewhere within
> passt, and read no further. As a result, "--mac-addr" is always added
> to the passt commandline, specifying the setting from
>  in the guest's interface config.
> 
> But as pointed out in this bugzilla comment:
> 
> https://bugzilla.redhat.com/2184967#c8
> 
> That is *not at all* what passt's --mac-addr option does. Instead, it
> is used to force the *remote* mac address for incoming traffic to a
> specific value. So setting --mac-addr results in all traffic on the
> interface having the same (the guest's) mac address for both source
> and destination in all traffic. Surprisingly, this still works, so
> nobody noticed it during testing.
> 
> The proper thing is to not specify any mac address to passt - the
> remote MAC addresses can and should remain untouched, and the local
> MAC address will end up being known to passt and beyond just by the
> guest sending out packets with that MAC address.
> 
> Reported-by: Laszlo Ersek 
> Signed-off-by: Laine Stump 
> ---
>  src/qemu/qemu_passt.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index 3679bf75fc..d36856e92e 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -176,7 +176,6 @@ qemuPasstStart(virDomainObj *vm,
>  g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net);
>  g_autoptr(virCommand) cmd = NULL;
>  g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> -char macaddr[VIR_MAC_STRING_BUFLEN];
>  bool needUnlink = false;
>  size_t i;
>  
> @@ -187,7 +186,6 @@ qemuPasstStart(virDomainObj *vm,
>  virCommandAddArgList(cmd,
>   "--one-off",
>   "--socket", passtSocketName,
> - "--mac-addr", virMacAddrFormat(>mac, macaddr),
>   "--pid", pidfile,
>   NULL);
>  

I've been thinking more about this. It is certainly an improvement. Per
my reading of the passt manual, "--mac-addr" and "--gateway" should
either be specified together, or none of them should be specified; and
this change satisfies that condition.

Reviewed-by: Laszlo Ersek 

Re: [PATCH] qemu: don't add --mac-addr option to passt commandline

[Laszlo Ersek]

On 7/13/23 18:30, Laine Stump wrote:
> When I implemented passt support in libvirt, I saw the --mac-addr
> option on the passt commandline, immediately assumed that this was
> used for setting the guest interface's mac address somewhere within
> passt, and read no further. As a result, "--mac-addr" is always added
> to the passt commandline, specifying the setting from
>  in the guest's interface config.
> 
> But as pointed out in this bugzilla comment:
> 
> https://bugzilla.redhat.com/2184967#c8
> 
> That is *not at all* what passt's --mac-addr option does. Instead, it
> is used to force the *remote* mac address for incoming traffic to a
> specific value. So setting --mac-addr results in all traffic on the
> interface having the same (the guest's) mac address for both source
> and destination in all traffic. Surprisingly, this still works, so
> nobody noticed it during testing.
> 
> The proper thing is to not specify any mac address to passt - the
> remote MAC addresses can and should remain untouched, and the local
> MAC address will end up being known to passt and beyond just by the
> guest sending out packets with that MAC address.
> 
> Reported-by: Laszlo Ersek 
> Signed-off-by: Laine Stump 
> ---
>  src/qemu/qemu_passt.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index 3679bf75fc..d36856e92e 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -176,7 +176,6 @@ qemuPasstStart(virDomainObj *vm,
>  g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net);
>  g_autoptr(virCommand) cmd = NULL;
>  g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> -char macaddr[VIR_MAC_STRING_BUFLEN];
>  bool needUnlink = false;
>  size_t i;
>  
> @@ -187,7 +186,6 @@ qemuPasstStart(virDomainObj *vm,
>  virCommandAddArgList(cmd,
>   "--one-off",
>   "--socket", passtSocketName,
> - "--mac-addr", virMacAddrFormat(>mac, macaddr),
>   "--pid", pidfile,
>   NULL);
>  

I *think* we might want to keep "--mac-addr", just with the MAC address
that SLIRP assigns to the host (as seen from the guest) -- namely
"52:56:00:00:00:02".

Passt's mission is to mimic the host environment as closely as possible
in the guest env. The docs on this option includes, "Default is to use
the MAC address of the interface with the first IPv4 default route on
the host". I think that might not be entirely consistent with us setting
"--address".

I'm not sure what the best solution would be; *one* solution I can think
of is just imitating the SLIRP environment: pass all of --address,
--netmask, --gateway and --mac-addr. The first two should control the
guest's details (according to the domain XML), and the latter two should
stick with the SLIRP defaults -- the "xx.xx.2.2" host IP address, and
the 52:56:00:00:00:02 host MAC address.

Laszlo

Re: [PATCH 2/2] docs: make isa-debugcon example more useful / directly applicable

[Laszlo Ersek]

On 5/18/23 16:34, Andrea Bolognani wrote:
> On Thu, May 18, 2023 at 02:59:39PM +0200, Laszlo Ersek wrote:
>> The type='pty' attribute in the  element causes a Pseudo TTY to be
>> allocated on the host side via "/dev/ptmx", which is meant to be
>> interacted with via "virsh console" or similar.
>>
>> That's not how a firmware log is typically viewed or saved. Replace
>> type='pty' with type='file', and also provide an example  element
>> (with the pathname of the logfile), similarly to how the  example
>> just above provides a  element too.
>>
>> Cc: "Daniel P. Berrangé" 
>> Cc: Andrea Bolognani 
>> Updates: 654968381df0256c047d2ecd4542ccc90dc57ad0
>> Signed-off-by: Laszlo Ersek 
>> ---
>>
>> Notes:
>> context:-U6
>>
>>  docs/formatdomain.rst | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> I see you've already pushed the patches

I for sure have not, I don't have the right (that I know of)!

Hm... we have Jano to blame ^W^W to thank to, for the push :)

> but, just for completeness' sake:
> 
> Reviewed-by: Andrea Bolognani 

Thank you both!
Laszlo

[PATCH 2/2] docs: make isa-debugcon example more useful / directly applicable

[Laszlo Ersek]

The type='pty' attribute in the  element causes a Pseudo TTY to be
allocated on the host side via "/dev/ptmx", which is meant to be
interacted with via "virsh console" or similar.

That's not how a firmware log is typically viewed or saved. Replace
type='pty' with type='file', and also provide an example  element
(with the pathname of the logfile), similarly to how the  example
just above provides a  element too.

Cc: "Daniel P. Berrangé" 
Cc: Andrea Bolognani 
Updates: 654968381df0256c047d2ecd4542ccc90dc57ad0
Signed-off-by: Laszlo Ersek 
---

Notes:
context:-U6

 docs/formatdomain.rst | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 6b8b60804e62..b46eefb0f675 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -6628,15 +6628,16 @@ Serial port
  
  


  
  
- 
+ 


+   
  
 

...
 
 ::

[PATCH 0/2] docs: improve isa-debugcon example

[Laszlo Ersek]

The feature from libvirt 8.1 that the OVMF debug log can be captured
with native domain XML elements is super useful, but the example snippet
in the documentation is more difficult to use than necessary. Improve
it.

Cc: "Daniel P. Berrangé" 
Cc: Andrea Bolognani 

Thanks!
Laszlo

Laszlo Ersek (2):
  docs: fix typo in isa-debugcon example
  docs: make isa-debugcon example more useful / directly applicable

 docs/formatdomain.rst | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


base-commit: 5ee27c37e6d0d081714d1e1d25a0a4e472665120

[PATCH 1/2] docs: fix typo in isa-debugcon example

[Laszlo Ersek]

The  opening tag is paired with the  closing tag; that's
a mismatch. The question is then whether to modify the former to
, or the latter to .

Per section "Relationship between serial ports and consoles",  is
used for emulated (not paravirt) consoles, and it's the type that's
suitable for early debug output (such as from firmware). Thus, change
 to .

Cc: "Daniel P. Berrangé" 
Cc: Andrea Bolognani 
Fixes: 654968381df0256c047d2ecd4542ccc90dc57ad0
Signed-off-by: Laszlo Ersek 
---
 docs/formatdomain.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 1f52f58d37e1..6b8b60804e62 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -6634,7 +6634,7 @@ Serial port
  


- 
+ 
 

...

Re: libvirt-guests configurability regression

[Laszlo Ersek]

On 11/10/22 11:32, Andrea Bolognani wrote:
> On Thu, Nov 10, 2022 at 08:57:25AM +, Daniel P. Berrangé wrote:
>> On Wed, Nov 09, 2022 at 09:17:08PM +0100, Olaf Hering wrote:
>>> Wed, 9 Nov 2022 09:04:12 -0800 Andrea Bolognani :
 Olaf, can you please remind me why the files we dropped were
 problematic but these ones apparently aren't?
>>>
>>> These are equally problematic because they are owned by the admin as well.
>>>
>>> They are essentially empty, their content is a duplication from doc/ (I 
>>> hope).
>>> They should not be part of libvirt.git.
>>
>> We are not going to remove these files from git or from the RPM, it would
>> be incredibly user hostile. I didn't mind removing the sysconfig files
>> because they were legacy configs from initscripts era which are obsolete
>> with systemd and so should not really be used except for existing
>> deployments.

Hmm. That's a good distinction actually. So I guess my original
complaint should be rephrased as one against systemd style service
configuration -- then again, that ship has sailed ;)

Thanks for the enlightenment!

> Should we reconsider the fate of libvirt-guests's sysconfig file
> then?
> 
> The other sysconfig files basically only influenced the way each
> service is spawned (VIRT*_ARGS, mostly used for --timeout) but in the
> case of libvirt-guests the sysconfig file is used to configure the
> behavior in ways that don't really fall under the purview of systemd.

I'm not sure if I know enough to claim this myself -- the libvirt-guests
service's configuration may actually fall in systemd territory. I just
find the override files

  /etc/systemd/system/NAME.service.d/override.conf

from "systemctl edit" much less intuitive than the previous way.

BTW, now that I've retried "systemctl edit", it seems that the override
file is "primed" from the central service file
(/usr/lib/systemd/system/NAME.service) -- the latter is included,
commented out, when $EDITOR starts up. So if all the documentation and
the default knob assignments were included in
"/usr/lib/systemd/system/libvirt-guests.service", then "systemctl edit"
would indeed be a mostly complete replacement for the sysconfig file.

In fact, commit 8eb4461645c5 does indicate *just this* method -- but as
I pointed out up-thread, the central "libvirt-guests.service" file does
not include the actual knobs and their docs; those are (at best) in
"tools/libvirt-guests.sh.in".

Instead, the central service file currently contains (excerpt):

> [Service]
> EnvironmentFile=-/etc/sysconfig/libvirt-guests
> # Hack just call traditional service until we factor
> # out the code
> ExecStart=/usr/libexec/libvirt-guests.sh start
> ExecStop=/usr/libexec/libvirt-guests.sh stop

I guess *that* is the actual problem -- it's technical debt. This
"factoring out" (= the migration of the knobs and the docs to the
service file) should have *preceded* commit 8eb4461645c5. Commit
8eb4461645c5 alluded to the proper (modern) way for configuring
services, and with that argument removed the sysconfig file for
"libvirt-guests" -- but the libvirt-guests service had not converted
sufficiently for that.

Therefore I do claim that commit 8eb4461645c5 is buggy -- it was too early.

Olaf said up-thread, "the script needs to be adjusted to recognize
existing environment variables", and technically, that may indeed be the
solution; I'm just saying it was actualy a *pre-requisite* (missed or
ignored) for commit 8eb4461645c5.

> Maybe a proper /etc/libvirt/libvirt-guests.conf would make sense, for
> consistency with other services? But then, you don't want to be
> parsing libvirt's config format from a shell script. Could we rewrite
> libvirt-guests in C, possibly improving on the hairier parts of it in
> the process? Perhaps even take the opportunity to reconsider the way
> some of its settings interact with each other.
> 
> I might be getting a bit ahead of myself here :)
> 

I think the logic being implemented as a shell script is fine, but its
configuration (env vars), and the related documentation, needs to be
moved / added to the central service file, so that "systemctl edit" can
present them as a crutch to the user, formatted in comments.

The manual page is *not* a substitute for this.

Laszlo

Re: libvirt-guests configurability regression

[Laszlo Ersek]

On 11/08/22 19:07, Andrea Bolognani wrote:
> On Tue, Nov 08, 2022 at 04:58:37PM +0100, Olaf Hering wrote:
>> Tue, 8 Nov 2022 08:46:34 +0100 Laszlo Ersek :
>>> Can you at least include the previously shipped *documented* config file
>>> as a template in a contrib or docs directory or something?
>>
>> This is up to the maintainers of libvirt.git to decide.
>>
>> If I were in charge, nothing would be installed into /etc.
>> What needs to go there, in case it is required, is already reasonably well 
>> documented.
> 
> Laszlo,
> 
> I feel your frustration.
> 
> I don't think a revert to the previous situation is going to happen,
> because there are some real advantages resulting from not shipping
> admin-owned files in our packages.
> 
> That said, the current situation is clearly not ideal either, so
> let's try to find a way to make things at least a bit better :)
> 
> I think a reasonable compromise would be to add
> 
>   Environment=URIS="default"
>   Environment=ON_BOOT="start"
>   ...
> 
> to libvirt-guests.service. Maybe instead of having the extended
> documentation that was originally in the defaults file we could have
> a shorter, one-line version? Maybe a pointer to the manual page?
> 
> Whatever comments we put there will show up when running 'systemctl
> edit libvirt-guests', which makes them fairly discoverable IMO.
> That's really the key point, because even today you can change the
> behavior both with a defaults file and a systemd unit override.
> 
> What do you think? Would that work for you?

I'd prefer an (otherwise initially inactive) file somewhere in the
distribution that followed the style of (say) "/etc/libvirt/qemu.conf".
I could copy that in-place, and then edit it.

If this still counts as duplication, then let's stick with the status quo.

Thanks,
Laszlo

Re: libvirt-guests configurability regression

[Laszlo Ersek]

On 11/08/22 18:36, Daniel P. Berrangé wrote:
> On Tue, Nov 08, 2022 at 08:46:34AM +0100, Laszlo Ersek wrote:
>> On 11/07/22 18:01, Olaf Hering wrote:
>>> Mon, 7 Nov 2022 14:17:00 +0100 Laszlo Ersek :
>>>
>>>> The (a) well-documented and (b) easily editable config file
>>>> "/etc/sysconfig/libvirt-guests" is now gone.
>>>
>>> Right, these admin-owned files are not installed anymore.
>>> The runtime code still uses the files, in case they are created by the 
>>> admin.
>>>
>>>> The message on commit 8eb4461645c5 says,
>>>> Remove the sysconfig file and place the current desired default into
>>>> the service file.
>>>
>>> Yes, and right in the next paragraph the commit messages states the files
>>> are (still) recognized.
>>>
>>> As of f8b6c7e5, libvirt-guests.sh initializes some internal defaults, then
>>> it loads the admin-owned sysconfig file, in case it was created.
>>>
>>> The documentation about this specific tool (libvirt-guests(1)) states what
>>> values from  /etc/sysconfig/libvirt-guests will be recognized.
>>>
>>>
>>> While browsing various user mailing lists in the past years I often saw
>>> messages like "system was upgraded to new version, and as a result an
>>> expected file somewhere in /etc does not exist anymore. What now?".
>>> Apparently some folks expect files to be present before they can be edited.
>>> In my experience it is always possible to create the required files and
>>> fill them with the desired content, based on available documentation.
>>
>> It's obviously possible; the question is how comfortable it is, how much
>> time the user now needs to spend on something that used to be much
>> easier / faster before.
>>
>> Can you at least include the previously shipped *documented* config file
>> as a template in a contrib or docs directory or something?
> 
> That would just be duplicating information already provideed in the
> manpage, where there is much more detailed description, so I don't
> think that would be useful.

The following files:

/etc/libvirt/libvirt-admin.conf
/etc/libvirt/libvirt.conf
/etc/libvirt/libvirtd.conf
/etc/libvirt/libxl.conf
/etc/libvirt/libxl-lockd.conf
/etc/libvirt/lxc.conf
/etc/libvirt/qemu.conf
/etc/libvirt/qemu-lockd.conf
/etc/libvirt/virtinterfaced.conf
/etc/libvirt/virtlockd.conf
/etc/libvirt/virtlogd.conf
/etc/libvirt/virtlxcd.conf
/etc/libvirt/virtnetworkd.conf
/etc/libvirt/virtnodedevd.conf
/etc/libvirt/virtnwfilterd.conf
/etc/libvirt/virtproxyd.conf
/etc/libvirt/virtqemud.conf
/etc/libvirt/virtsecretd.conf
/etc/libvirt/virtstoraged.conf
/etc/libvirt/virtvboxd.conf
/etc/libvirt/virtxend.conf

all exist out of the box too, and follow the style that I'm requesting
(= they are heavily documented in-line).

Laszlo

Re: libvirt-guests configurability regression

[Laszlo Ersek]

On 11/07/22 18:01, Olaf Hering wrote:
> Mon, 7 Nov 2022 14:17:00 +0100 Laszlo Ersek :
> 
>> The (a) well-documented and (b) easily editable config file
>> "/etc/sysconfig/libvirt-guests" is now gone.
> 
> Right, these admin-owned files are not installed anymore.
> The runtime code still uses the files, in case they are created by the admin.
> 
>> The message on commit 8eb4461645c5 says,
>> Remove the sysconfig file and place the current desired default into
>> the service file.
> 
> Yes, and right in the next paragraph the commit messages states the files
> are (still) recognized.
> 
> As of f8b6c7e5, libvirt-guests.sh initializes some internal defaults, then
> it loads the admin-owned sysconfig file, in case it was created.
> 
> The documentation about this specific tool (libvirt-guests(1)) states what
> values from  /etc/sysconfig/libvirt-guests will be recognized.
> 
> 
> While browsing various user mailing lists in the past years I often saw
> messages like "system was upgraded to new version, and as a result an
> expected file somewhere in /etc does not exist anymore. What now?".
> Apparently some folks expect files to be present before they can be edited.
> In my experience it is always possible to create the required files and
> fill them with the desired content, based on available documentation.

It's obviously possible; the question is how comfortable it is, how much
time the user now needs to spend on something that used to be much
easier / faster before.

Can you at least include the previously shipped *documented* config file
as a template in a contrib or docs directory or something?

Thanks
Laszlo

libvirt-guests configurability regression

[Laszlo Ersek]

Hi,

I'm really unhappy about commit 8eb4461645c5 ("remove sysconfig files",
2022-01-17), first included in release v8.1.0.

The (a) well-documented and (b) easily editable config file
"/etc/sysconfig/libvirt-guests" is now gone. So if I want to do now on
Fedora 36 the same thing that I used to do on up to and including Fedora
35, I now need to consult a new manual page (from grandparent commit
161727417a91, "docs: Add man page for libvirt-guests", 2022-01-17), and
collect a bunch of options manually.

The message on commit 8eb4461645c5 says,

Remove the sysconfig file and place the current desired default into
the service file.

which I briefly considered a consolation, figuring I'd just copy the
collected bunch of options (and hopefully their comments!) to the same
place as before, from the "service file" -- "libvirt-guests.service".

However, the actual commit does not live up to its promise; for example,
the important ON_SHUTDOWN knob is only *removed* from the codebase by
the commit; it is not reintroduced anywhere (certainly not in
"libvirt-guests.service"). Well, the manual page, two commits up the
branch, documents it, but that's totally no viable replacement.

As of f8ebb5816350:

> $ git grep -w ON_SHUTDOWN
>
> docs/manpages/libvirt-guests.rst:- ON_SHUTDOWN=suspend
> docs/manpages/libvirt-guests.rst:time to shutdown. When setting 
> ON_SHUTDOWN=shutdown, you must also set
> docs/manpages/libvirt-guests.rst:  "ON_SHUTDOWN" is set to "shutdown". If Set 
> to 0, guests will be shutdown one
> tools/libvirt-guests.sh.in:ON_SHUTDOWN="suspend"
> tools/libvirt-guests.sh.in:if [ "x$ON_SHUTDOWN" = xshutdown ]; then
> tools/libvirt-guests.sh.in:ON_SHUTDOWN="shutdown"

... It seems that "tools/libvirt-guests.sh.in" does have some built-in
defaults (going back as far as to 66823690e469, "Init script for
handling guests on shutdown/boot", 2010-05-21), which I could copy and
modify presumably; however, those defaults still lack the previously
directly adjacent documentation.

Please consider remedying this. Readily editable config files with
documentation and defaults included are very powerful. They are not
suitable for all config formats of course (especially hierarchical ones:
consider the domain XML for example), but for flat or otherwise simply
structured config files, offering that productivity boost to end-users
is a no-brainer, IMO. Please restore it if you can.

Thanks
Laszlo

Re: [PATCH] rpc: Pass OPENSSL_CONF through to ssh invocations

[Laszlo Ersek]

On 07/25/22 15:09, Richard W.M. Jones wrote:
> It's no longer possible for libvirt to connect over the ssh transport
> from RHEL 9 to RHEL 5.  This is because SHA1 signatures have been
> effectively banned in RHEL 9 at the openssl level.  They are required
> to check the RHEL 5 host key.  Note this is a separate issue from
> openssh requiring additional configuration in order to connect to
> older servers.
> 
> Connecting from a RHEL 9 client to RHEL 5 server:
> 
> $ cat ~/.ssh/config
> Host 192.168.0.91
>   KexAlgorithms+diffie-hellman-group14-sha1
>   MACs +hmac-sha1
>   HostKeyAlgorithms+ssh-rsa
>   PubkeyAcceptedKeyTypes   +ssh-rsa
>   PubkeyAcceptedAlgorithms +ssh-rsa
> 
> $ virsh -c 'qemu+ssh://root@192.168.0.91/system' list
> error: failed to connect to the hypervisor
> error: Cannot recv data: ssh_dispatch_run_fatal: Connection to 192.168.0.91 
> port 22: error in libcrypto: Connection reset by peer
> 
> "error in libcrypto: Connection reset by peer" is the characteristic
> error of openssl having been modified to disable SHA1 by default.
> (You will not see this on non-RHEL-derived distros.)
> 
> You could enable the legacy crypto policy which downgrades security on
> the entire host, but a more fine-grained way to do this is to create
> an alternate openssl configuration file that enables the "forbidden"
> signatures.  However this requires passing the OPENSSL_CONF
> environment variable through to ssh to specify the alternate
> configuration.  Libvirt filters out this environment variable, but
> this commit allows it through.  With this commit:
> 
> $ cat /var/tmp/openssl.cnf
> .include /etc/ssl/openssl.cnf
> [openssl_init]
> alg_section = evp_properties
> [evp_properties]
> rh-allow-sha1-signatures = yes
> 
> $ OPENSSL_CONF=/var/tmp/openssl.cnf ./run virsh -c 
> 'qemu+ssh://root@192.168.0.91/system' list
> root@192.168.0.91's password:
>  Id   Name   State
> 
> 
> Essentially my argument here is that OPENSSL_CONF is sufficiently
> similar in nature to KRB5CCNAME, SSH* and XAUTHORITY that we should
> permit it to be passed through.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2062360
> Signed-off-by: Richard W.M. Jones 
> ---
>  src/rpc/virnetsocket.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index 32f506d2d4..8280bda007 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -855,6 +855,7 @@ int virNetSocketNewConnectSSH(const char *nodename,
>  virCommandAddEnvPass(cmd, "KRB5CCNAME");
>  virCommandAddEnvPass(cmd, "SSH_AUTH_SOCK");
>      virCommandAddEnvPass(cmd, "SSH_ASKPASS");
> +virCommandAddEnvPass(cmd, "OPENSSL_CONF");
>  virCommandAddEnvPass(cmd, "DISPLAY");
>  virCommandAddEnvPass(cmd, "XAUTHORITY");
>  virCommandClearCaps(cmd);
> 

Acked-by: Laszlo Ersek 

Re: [PATCH 0/1] vmx: Fix mapping

[Laszlo Ersek]

On 10/04/21 11:59, Richard W.M. Jones wrote:
> It turns out that changing the qemu implementation is painful,
> particularly if we wish to maintain backwards compatibility of the
> command line and live migration.
>
> Instead I opted to document comprehensively what all the
> different hypervisors do:
>
>   
> https://github.com/libguestfs/virt-v2v/blob/master/docs/vm-generation-id-across-hypervisors.txt

> Unfortunately QEMU made a significant mistake when implementing this
> feature.  Because the string is 128 bits wrong, they decided it must
  ^^

Haha, that's a great typo :)

> be a UUID (as you can see above there is no evidence that Microsoft
> who wrote the original spec thought it was).  Following from this
> incorrect assumption, they stated that the "UUID" must be supplied to
> qemu in big endian format and must be byteswapped when writing it to
> guest memory.  Their documentation says that they only do this for
> little endian guests, but this is not true of their implementation
> which byte swaps it for all guests.

I don't think this is what section "Endian-ness Considerations" in
"docs/specs/vmgenid.txt" says. That text says that the *device* uses
little-endian format. That's independent of the endianness of *CPU* of
the particular guest architecture.

So the byte-swapping in the QEMU code occurs unconditionally because
QEMU's UUID type is inherently big endian, and the device model in
question is fixed little endian. The guest CPU's endianness is
irrelevant as far as the device is concerned.

If a BE guest CPU intends to read the generation ID and to interpret it
as a set of integers, then the guest CPU is supposed to byte-swap the
appropriate fields itself.

> References

I suggest adding two links in this section, namely to:
- docs/specs/vmgenid.txt
- hw/acpi/vmgenid.c

Thanks,
Laszlo

translating CD-ROM device paths from i440fx to Q35 in virt-v2v (was: test-v2v-cdrom: update the CD-ROM's bus to SATA in the converted domain)

[Laszlo Ersek]

(+libvirt-devel)

On 09/29/21 21:22, Richard W.M. Jones wrote:

> We currently partially install the virtio block drivers in the Windows
> guest (just enough to get the guest to boot on the target), and
> Windows itself re-installs the virtio block driver and other drivers
> it needs, and that's enough to get it to see C:
>
> As for other hard disk partitions, Windows does indeed contain a
> mapping to other drives in the Registry but IIRC it's not sensitive to
> the device driver (unlike Linux /dev/vdX vs /dev/sdX).  If you're
> interested in that, see libguestfs.git/daemon/inspect_fs_windows.ml:
> get_drive_mappings.  We never bothered with attempting to handle
> conversion of floppy drives or CD-ROMs for Windows.

OK. So AIUI, that means no work is needed here for Windows.

> On Linux we do better: We iterate over all the configuration files in
> /etc and change device paths.  The significance of this bug is we need
> to change (eg) /dev/hdc to /dev/.  The difficulty is
> working out where the device will appear on the target and not having
> it conflict with any hard disk, something we partly control (see
> virt-v2v.git/convert/target_bus_assignment.ml*)

AIUI the conflict avoidance logic ("no overlapping disks") is already in
place. The question is how to translate device paths in /etc/fstab and
similar.

Please correct me if I'm wrong: at the moment, I believe virt-v2v parses
and manipulates the following elements and attributes in the domain XML:

  
  
  
  
  ^^^   ^^^

My understanding is however that the target/@dev attribute is mostly
irrelevant:

https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms

The dev attribute indicates the "logical" device name. The actual
device name specified is not guaranteed to map to the device name in
the guest OS. Treat it as a device ordering hint. [...]

What actually matters is the target/@bus attribute, in combination with
the sibling element . Such as:

  
 ^^^
  
^   ^   ^

  
 ^^^
  
^   ^   ^

  
 ^^^
  
^   ^   ^

  
 ^^^
  
^   ^   ^

So, target/@dev should be mostly ignored; what matters is the following
tuple:

  (target/@bus, address/@controller, address/@bus, address/@unit)

Extracting just the tuples:

  (ide, 0, 0, 0)
  (ide, 0, 0, 1)
  (ide, 0, 1, 0)
  (ide, 0, 1, 1)

The first two components of each tuple -- i.e., (ide, 0) -- refer to the
following IDE controller:


^^ ^
  


and then the rest of the components, such as (0, 0), (0, 1), (1, 0), (1,
1), identify the disk on that IDE controller.

(Side comment: the PCI location of the (first) IDE controller is fixed
in QEMU; if one tries to change it, libvirt complains: "Primary IDE
controller must have PCI address 0:0:1.1".)

(Side comment: on the QEMU command line, this maps to

  -device ide-cd,bus=ide.0,unit=0,... \
  -device ide-cd,bus=ide.0,unit=1,... \
  -device ide-cd,bus=ide.1,unit=0,... \
  -device ide-cd,bus=ide.1,unit=1,... \
)

Inside the guest, /dev/hd* nodes don't even exist, so it's unlikely that
/etc/fstab would refer to them. /etc/fstab can however refer to symlinks
under "/dev/disk/by-id" (for example):

  lrwxrwxrwx. 1 root root  9 Sep 30 11:54 ata-QEMU_DVD-ROM_QM1 -> ../../sr0
  lrwxrwxrwx. 1 root root  9 Sep 30 11:54 ata-QEMU_DVD-ROM_QM2 -> ../../sr1
  lrwxrwxrwx. 1 root root  9 Sep 30 11:54 ata-QEMU_DVD-ROM_QM3 -> ../../sr2
  lrwxrwxrwx. 1 root root  9 Sep 30 11:54 ata-QEMU_DVD-ROM_QM4 -> ../../sr3

Furthermore, we have pseudo-files (directories) such as:

  /sys/devices/pci:00/:00:01.1/ata1/host0/target0:0:0/0:0:0:0/block/sr0
  /sys/devices/pci:00/:00:01.1/ata1/host0/target0:0:1/0:0:1:0/block/sr1
  /sys/devices/pci:00/:00:01.1/ata2/host1/target1:0:0/1:0:0:0/block/sr2
  /sys/devices/pci:00/:00:01.1/ata2/host1/target1:0:1/1:0:1:0/block/sr3
  ^   ^

So in order to map a device path from the original guest's "/etc/fstab",
such as "/dev/disk/by-id/ata-QEMU_DVD-ROM_QM3", to the original
domain XML's  element, we have to do the following in the "source"
appliance:

NODE=$(realpath /dev/disk/by-id/ata-QEMU_DVD-ROM_QM3)
# -> /dev/sr2
NODE=${NODE#/dev/}
# -> sr2
DEVPATH=$(ls -d 
/sys/devices/pci:00/:00:01.1/ata?/host?/target?:0:?/?:0:?:0/block/$NODE)
# -> 
/sys/devices/pci:00/:00:01.1/ata2/host1/target1:0:0/1:0:0:0/block/sr2

And then map the "1:0:0:0" pathname component from $DEVPATH to:

  
  
   ^^^
 

Re: [PATCH 0/1] vmx: Fix mapping

[Laszlo Ersek]

On 09/30/21 09:33, Richard W.M. Jones wrote:
> 
> More data: I found a colleague who has a Hyper-V instance with a
> Windows guest and he helped me to understand how Hyper-V represents
> generation IDs.  Hyper-V has had support for generation IDs since long
> before Microsoft proposed the feature for standardization.  Originally
> (I think pre-2013) Hyper-V used an XML description which included:
> 
> 4a07df4361fdf4c883f97bc30e524b9d
> 
> Note this is a pure hex string, not a GUID.
> 
> In current Hyper-V, the same representation is used but it's embedded
> in a binary file.
> 
> My colleague ran two Windows VMs on Hyper-V and examined the
> generation_id on the hypervisor and compared it to the output of
> VMGENID.EXE inside the guest.
> 
> The first guest had this in the binary hypervisor metadata:
> 
> 56b0  00 00 0e 67 65 6e 65 72  61 74 69 6f 6e 5f 69 64  |...generation_id|
> 56c0  00 40 00 00 00 38 00 30  00 35 00 61 00 32 00 38  |.@...8.0.5.a.2.8|
> 56d0  00 37 00 61 00 32 00 35  00 30 00 39 00 38 00 39  |.7.a.2.5.0.9.8.9|
> 56e0  00 65 00 34 00 66 00 65  00 36 00 66 00 36 00 39  |.e.4.f.e.6.f.6.9|
> 56f0  00 39 00 32 00 62 00 65  00 33 00 33 00 34 00 61  |.9.2.b.e.3.3.4.a|
> 5700  00 34 00 33 00 00 00 00  00 00 00 00 00 00 00 00  |.4.3|
> 
> and reported the output of VMGENID in this guest as:
> 
> VmCounterValue: fe6f6992be334a43:805a287a250989e4
> 
> The second guest had:
> 
> 5360  c5 06 00 00 00 0e 67 65  6e 65 72 61 74 69 6f 6e  |..generation|
> 5370  5f 69 64 00 40 00 00 00  65 00 62 00 66 00 62 00  |_id.@...e.b.f.b.|
> 5380  62 00 37 00 39 00 37 00  33 00 36 00 35 00 37 00  |b.7.9.7.3.6.5.7.|
> 5390  32 00 38 00 65 00 37 00  30 00 62 00 33 00 66 00  |2.8.e.7.0.b.3.f.|
> 53a0  64 00 33 00 63 00 37 00  31 00 33 00 65 00 63 00  |d.3.c.7.1.3.e.c.|
> 53b0  65 00 38 00 34 00 32 00  00 00 00 00 00 00 00 00  |e.8.4.2.|
> 
> and VMGENID was:
> 
> VmCounterValue: b3fd3c713ece842:ebfbb797365728e7
> 
> Note:
> 
>  - In both cases, the generation ID is clearly not a GUID.
> 
>  - The two 64 bit words are swapped over somewhere, but individual
>bytes are not byte-swapped, and there is again no evidence of
>UUID-aware byte swapping.
> 
> --
> 
> So how do we get from where we are to a more sane vmgenid in qemu?
> 
> I propose we deprecate the guid parameter in:
> 
>   -device vmgenid,guid=8987940a-0951-2cc5-e815-10634ff550b9,id=vmgenid0
> 
> Instead it will be replaced by bytes= which will simply write
> the bytes, in the order they appear, into guest memory with no
> attempt to interpret or byte-swap.  Something like:
> 
>   -device vmgenid,bytes=112233445566778899aabbccddeeff00,id=vmgenid0
> 
> (guid although deprecated will need to be kept around for a while,
> along with its weird byte-swapping behaviour).
> 
> We will then have a plain and simple method to emulate the behaviour
> of other hypervisors.  We will look at exactly what bytes they write
> to guest memory and copy that behaviour when v2v converting from those
> hypervisors.

I don't have anything against this, I just don't know who's going to
implement the QEMU change.

Thanks
Laszlo

Re: [libvirt PATCH 0/7] Add boot order to virtiofs

[Laszlo Ersek]

On 01/28/21 16:15, Ján Tomko wrote:
> Sadly, the replies changes for older QEMUs are synthetic.
> Separated for easier review.
> 
> Also available on gitlab:
> git fetch https://gitlab.com/janotomko/libvirt/ virtiofs-bootindex
> https://gitlab.com/janotomko/libvirt/-/tree/virtiofs-bootindex
> 
> And a broken pipeline:
> https://gitlab.com/janotomko/libvirt/-/pipelines/248162273
> 
> Ján Tomko (7):
>   tests: switch vhost-user-fs-hugepages to use boot order
>   conf: add boot order to filesystem
>   qemu: add QEMU_CAPS_VHOST_USER_FS_BOOTINDEX
>   fixup: vhost-user-fs-device properties
>   fixup: renumber
>   Add validation for virtiofs boot order setting
>   qemu: format bootindex for vhost-user-fs
> 
>  docs/schemas/domaincommon.rng |   3 +
>  src/conf/domain_conf.c|   5 +-
>  src/conf/domain_validate.c|  17 ++-
>  src/qemu/qemu_capabilities.c  |   8 +
>  src/qemu/qemu_capabilities.h  |   1 +
>  src/qemu/qemu_command.c   |   3 +
>  src/qemu/qemu_validate.c  |   6 +
>  .../caps_4.2.0.aarch64.replies| 131 
>  .../caps_4.2.0.s390x.replies  | 119 ---
>  .../caps_4.2.0.x86_64.replies | 131 
>  .../caps_5.0.0.aarch64.replies| 136 +
>  .../caps_5.0.0.ppc64.replies  | 124 +---
>  .../caps_5.0.0.riscv64.replies| 120 ---
>  .../caps_5.0.0.x86_64.replies | 136 +
>  .../caps_5.1.0.x86_64.replies | 136 +
>  .../caps_5.2.0.aarch64.replies| 136 +
>  .../caps_5.2.0.ppc64.replies  | 124 +---
>  .../caps_5.2.0.riscv64.replies| 120 ---
>  .../caps_5.2.0.s390x.replies  | 124 +---
>  .../caps_5.2.0.x86_64.replies | 136 +
>  .../caps_6.0.0.x86_64.replies | 140 ++
>  .../caps_6.0.0.x86_64.xml |   1 +
>  ...vhost-user-fs-hugepages.x86_64-latest.args |   3 +-
>  .../vhost-user-fs-hugepages.xml   |   3 +-
>  24 files changed, 1534 insertions(+), 329 deletions(-)
> 

I've applied this series locally on top of e59bb226b7d9 ("docs: link to
PCI docs from the kbase page", 2021-01-28), and tested it as follows:

- Added  to the virtio-fs element I already had; virsh
edit completed fine

- Booted the OVMF guest once with N=1 and then separately with N=3,
while the SCSI system disk of the guest had  in both
cases. Checked the firmware log to verify the behavior -- it was OK in
both cases.

So please add the following to whichever patch it applies to:

Tested-by: Laszlo Ersek 

(I didn't explicitly run any test suite, nor did I attempt to verify
behavior with an older QEMU, so I figure my T-b does not apply to every
patch in the series.)

Thank you Jano for implementing this feature.

--*--

Some general notes/questions on testing:

I used the documentation at <https://libvirt.org/compiling.html#building>.

- I think the example pathname "/home/to/your/checkout/src/libvirtd"
should include the "build" directory name now, after the meson conversion.

- I had to stop / start "virtlogd" separately, too; noting that in the
docs could help.

- I had to set SELinux to Permissive temporarily, otherwise QEMU
wouldn't start. A note on SELinux could be helpful.

- I manually "power cycled" the virtual networks on my laptop as well,
because the dnsmasq command lines refer to the "lease helper" binaries,
and the latter are also specific to the libvirtd build. I'm not sure
this was really necessary, but better safe than sorry?...

- After completing the tests, I shut down the one-off virtlogd and
libvirtd processes (Ctrl-C), and started the system-wide binaries again,
with systemctl. Systemctl reports both of those as "running OK" now;
however, when I try to net-start the virtual networks again, with
"virsh", I get the following error:

error: failed to connect to the hypervisor
error: Failed to connect socket to '/var/run/libvirt/libvirt-sock': No
such file or directory

I don't know what that's about. I guess I'll just reboot my laptop now :)

Thanks!
Laszlo

Re: [PATCH] docs: Discourage users from using fwcfg

[Laszlo Ersek]

On 09/08/20 19:26, Daniel P. Berrangé wrote:
> On Tue, Sep 08, 2020 at 10:17:08AM +0200, Laszlo Ersek wrote:
>> On 09/07/20 16:38, Daniel P. Berrangé wrote:
>>> On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
>>>> On 9/7/20 3:57 PM, Martin Kletzander wrote:
>>>>> On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>>>>>> Even though this was brought up in upstream discussion [1] it
>>>>>> missed my patches: users should prefer  over fwcfg.
>>>>>> The reason is that fwcfg is considered somewhat internal to QEMU
>>>>>> and it has limited number of slots and neither of these applies
>>>>>> to .
>>>>>>
>>>>>> While I'm at it, I'm fixing the example too (because it contains
>>>>>> incorrect element name) and clarifying sysfs/ exposure.
>>>>>>
>>>>>> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>>>>>
>>>>>> Reported-by: Laszlo Ersek 
>>>>>> Signed-off-by: Michal Privoznik 
>>>>>> ---
>>>>>> docs/formatdomain.rst | 14 +-
>>>>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>>>>
>>>>
>>>>
>>>>> It's nice that you say that, but people who would like to use fw_cfg for
>>>>> passing
>>>>> in a huge blob, which is saved in a file, will read this, go to
>>>>> 
>>>>> and see that there is no way to pass a file as an input.  Should that be
>>>>> dealt
>>>>> with somehow?  Or would that be discouraged as well?
>>>>
>>>> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at least
>>>> quick glance over hw/smbios/smbios.c doesn't show any signs it's allowed).
>>>
>>> Indeed, that is an RFE I've never got around to
>>
>> Yes, I remember that -- I remember that we discussed this, and also that
>> you didn't have time for it. Which is entirely justifiable; other stuff
>> has been (and continues to be) more important.
>>
>> For reference, here's the launchpad ticket that tracks the RFE for QEMU:
>>
>>   https://bugs.launchpad.net/qemu/+bug/1826200
> 
> I spent some time today (more than expected in fact!) implementing
> this
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg03023.html

That was quick! Thank you!

> I hit a possible EDK2 bug when SMBIOS 3.0 where it hangs when the
> size exceeds about 128 KB, but I don't intend to investigate it
> further.

I'll look into it.

Thanks!
Laszlo

Re: [PATCH] docs: Discourage users from using fwcfg

[Laszlo Ersek]

On 09/08/20 14:28, Michal Privoznik wrote:
> On 9/8/20 1:45 PM, Laszlo Ersek wrote:
>> Reviewed-by: Laszlo Ersek
> 
> Thank you, pushed.

Thank *you* for the patch. :)

Laszlo

Re: [PATCH] docs: Discourage users from using fwcfg

[Laszlo Ersek]

On 09/08/20 12:05, Michal Privoznik wrote:
> On 9/8/20 11:02 AM, Laszlo Ersek wrote:
>> On 09/07/20 15:48, Michal Privoznik wrote:
>>> Even though this was brought up in upstream discussion [1] it
>>> missed my patches: users should prefer  over fwcfg.
>>> The reason is that fwcfg is considered somewhat internal to QEMU
>>> and it has limited number of slots and neither of these applies
>>> to .
>>>
>>> While I'm at it, I'm fixing the example too (because it contains
>>> incorrect element name) and clarifying sysfs/ exposure.
>>>
>>> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>>
>>> Reported-by: Laszlo Ersek 
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>   docs/formatdomain.rst | 14 +-
>>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>> index 1979dfb8d3..821ffe8d60 100644
>>> --- a/docs/formatdomain.rst
>>> +++ b/docs/formatdomain.rst
>>> @@ -509,18 +509,22 @@ layout of sub-elements, with supported values of:
>>>  Some hypervisors provide unified way to tweak how firmware
>>> configures itself,
>>>  or may contain tables to be installed for the guest OS, for
>>> instance boot
>>>  order, ACPI, SMBIOS, etc. It even allows users to define their
>>> own config
>>> -   blobs. In case of QEMU, these then appear under domain's sysfs,
>>> under
>>> +   blobs. In case of QEMU, these then appear under domain's sysfs
>>> (if the guest
>>> +   kernel has FW_CFG_SYSFS config option enabled), under
>>>  ``/sys/firmware/qemu_fw_cfg``. Note, that these values apply
>>> regardless the
>>>   mode under . :since:`Since 6.5.0`
>>>
>>> +   **Please note that because of limited number of data slots use of
>>> fwcfg is
>>> +   strongly discouraged and  should be used instead**.
>>
>> please replace:
>>
>>    strongly discouraged
>>
>> with:
>>
>>    strongly discouraged for configuring any guest-side component other
>>    than the firmware
> 
> Actually, we have a check that forbids anything else than "opt/" prefix
> and also "opt/ovmf/" and "opt/org.qemu/" are forbidden:
> 
> https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_validate.c#L780
> 
> 
> Looks like "opt/org.tianocore" slipped through. I think the reasoning is
> that this is too low level and allows changing values of knobs that are
> exposed elsewhere (e.g. boot order). It really should be just to append
> new values, not change existing ones.
> 
>>
>> (
>>
>> Consider for example the following feature:
>>
>>    https://bugzilla.tianocore.org/show_bug.cgi?id=2681
>>
>> Namely, the following QEMU switches:
>>
>>    -fw_cfg name=opt/org.tianocore/IPv4PXESupport,string=[yn]
>>    -fw_cfg name=opt/org.tianocore/IPv6PXESupport,string=[yn]
>>
>> alter the behavior of OVMF and ArmVirtQemu. These flags are meant to be
>> stable. They do not need dedicated QEMU or libvirtd enablement. They
>> influence firmware behavior. So  is perfectly fine
>> (even ideal!) for tweaking them, through the domain XML. What's not fine
>> is configuring any random guest payload via .
>>
>> The point is that people who parse new fw_cfg files in edk2 such as
>> "opt/org.tianocore/IPv6PXESupport" are conscious of the slot count in
>> QEMU. They *can* bump the "x-file-slots" property in QEMU, for new
>> machine types, they just need to be aware of the property.
>>
>> )
>>
>>> +
>>>  ::
>>>
>>> -    
>>> +    
>>>     example value
>>
>> I suggest (according to the above):
>>
>> - name: opt/org.tianocore/IPv4PXESupport
>> - value: n
>>
>>> -  >> file='/tmp/provision.ign'/>
>>> -    
>>> +  >> file='/tmp/provision.ign'/>
>>
>> We have a functional -- working, stable -- example for name+file as
>> well:
>>
>> - name: etc/edk2/https/cacerts
>> - file: /etc/pki/ca-trust/extracted/edk2/cacerts.bin
>>
>> (This is documented in "OvmfPkg/README" in edk2, but it's not really
>> relevant here.)
>>
>>> +    
>>>
>>> -   The ``smbios`` element can have multiple ``entry`` child
>>> elements. Each
>>> +   The

Re: [PATCH] docs: Discourage users from using fwcfg

[Laszlo Ersek]

On 09/08/20 11:12, Daniel P. Berrangé wrote:
> On Tue, Sep 08, 2020 at 11:02:10AM +0200, Laszlo Ersek wrote:
>> On 09/07/20 15:48, Michal Privoznik wrote:
>>> Even though this was brought up in upstream discussion [1] it
>>> missed my patches: users should prefer  over fwcfg.
>>> The reason is that fwcfg is considered somewhat internal to QEMU
>>> and it has limited number of slots and neither of these applies
>>> to .
>>>
>>> While I'm at it, I'm fixing the example too (because it contains
>>> incorrect element name) and clarifying sysfs/ exposure.
>>>
>>> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>>
>>> Reported-by: Laszlo Ersek 
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  docs/formatdomain.rst | 14 +-
>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>> index 1979dfb8d3..821ffe8d60 100644
>>> --- a/docs/formatdomain.rst
>>> +++ b/docs/formatdomain.rst
>>> @@ -509,18 +509,22 @@ layout of sub-elements, with supported values of:
>>> Some hypervisors provide unified way to tweak how firmware configures 
>>> itself,
>>> or may contain tables to be installed for the guest OS, for instance 
>>> boot
>>> order, ACPI, SMBIOS, etc. It even allows users to define their own 
>>> config
>>> -   blobs. In case of QEMU, these then appear under domain's sysfs, under
>>> +   blobs. In case of QEMU, these then appear under domain's sysfs (if the 
>>> guest
>>> +   kernel has FW_CFG_SYSFS config option enabled), under
>>> ``/sys/firmware/qemu_fw_cfg``. Note, that these values apply regardless 
>>> the
>>>  mode under . :since:`Since 6.5.0`
>>>
>>> +   **Please note that because of limited number of data slots use of fwcfg 
>>> is
>>> +   strongly discouraged and  should be used instead**.
>>
>> please replace:
>>
>>   strongly discouraged
>>
>> with:
>>
>>   strongly discouraged for configuring any guest-side component other
>>   than the firmware
>>
>> (
>>
>> Consider for example the following feature:
>>
>>   https://bugzilla.tianocore.org/show_bug.cgi?id=2681
>>
>> Namely, the following QEMU switches:
>>
>>   -fw_cfg name=opt/org.tianocore/IPv4PXESupport,string=[yn]
>>   -fw_cfg name=opt/org.tianocore/IPv6PXESupport,string=[yn]
>>
>> alter the behavior of OVMF and ArmVirtQemu. These flags are meant to be
>> stable. They do not need dedicated QEMU or libvirtd enablement. They
>> influence firmware behavior. So  is perfectly fine
>> (even ideal!) for tweaking them, through the domain XML. What's not fine
>> is configuring any random guest payload via .
>>
>> The point is that people who parse new fw_cfg files in edk2 such as
>> "opt/org.tianocore/IPv6PXESupport" are conscious of the slot count in
>> QEMU. They *can* bump the "x-file-slots" property in QEMU, for new
>> machine types, they just need to be aware of the property.
> 
> I'd suggest that QEMU always sets x-file-slots to be 10 entries larger
> than the number of officially known slots. That leads some scope for
> app usage without risk of hitting the limits.

Right, at least I believe we've tried maintaining a "safety margin". The
"officially known slots" is sort of a fuzzy count one can deduce from
grepping the QEMU, SeaBIOS and edk2 source trees. So when we'd like to
introduce a new file, we redo those greps, and compare with the current
x-file-slots value (for the latest machine types, that is).

> 
> 
>>> -  
>>> -
>>> +  
>>
>> We have a functional -- working, stable -- example for name+file as
>> well:
>>
>> - name: etc/edk2/https/cacerts
>> - file: /etc/pki/ca-trust/extracted/edk2/cacerts.bin
> 
> I don't think we should document that in libvirt, since it is something
> libvirt will set silently behind the scenes. The ignition example is a
> acceptable real world example of expected usage in libvirt.

OK. I wasn't sure if specific libvirt enablement was going to occur for
the CA certs passing.

Thanks,
Laszlo

Re: [PATCH] docs: Discourage users from using fwcfg

[Laszlo Ersek]

On 09/08/20 11:05, Daniel P. Berrangé wrote:
> On Tue, Sep 08, 2020 at 10:22:34AM +0200, Laszlo Ersek wrote:
>> On 09/08/20 08:37, Martin Kletzander wrote:
>>> On Mon, Sep 07, 2020 at 03:38:23PM +0100, Daniel P. Berrangé wrote:
>>>> On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
>>>>> On 9/7/20 3:57 PM, Martin Kletzander wrote:
>>>>>> On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>>>>>>> Even though this was brought up in upstream discussion [1] it
>>>>>>> missed my patches: users should prefer  over fwcfg.
>>>>>>> The reason is that fwcfg is considered somewhat internal to QEMU
>>>>>>> and it has limited number of slots and neither of these applies
>>>>>>> to .
>>>>>>>
>>>>>>> While I'm at it, I'm fixing the example too (because it contains
>>>>>>> incorrect element name) and clarifying sysfs/ exposure.
>>>>>>>
>>>>>>> 1:
>>>>> https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>>>>>>
>>>>>>> Reported-by: Laszlo Ersek 
>>>>>>> Signed-off-by: Michal Privoznik 
>>>>>>> ---
>>>>>>> docs/formatdomain.rst | 14 +-
>>>>>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>
>>>>>
>>>>>> It's nice that you say that, but people who would like to use
>>>>> fw_cfg for
>>>>>> passing
>>>>>> in a huge blob, which is saved in a file, will read this, go to
>>>>>> 
>>>>>> and see that there is no way to pass a file as an input.  Should
>>>>> that be
>>>>>> dealt
>>>>>> with somehow?  Or would that be discouraged as well?
>>>>>
>>>>> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at
>>>>> least
>>>>> quick glance over hw/smbios/smbios.c doesn't show any signs it's
>>>>> allowed).
>>>>
>>>> Indeed, that is an RFE I've never got around to
>>>>
>>>
>>> Do you mean RFE for QEMU or libvirt?
>>>
>>> Are there any limitations for the oemStrings?  Libvirt could read the
>>> file and
>>> pass the data to QEMU as it is not something that is supposed to be
>>> writeable or
>>> shareable.  I understand it could be the first time we do something like
>>> this,
>>> but it might not be that bad of a precedence.
>>>
>>> Just so we are on the same page, I'm not against this patch, I just want
>>> to save
>>> our users the frustration that I, myself, experienced so many times. 
>>> There's
>>> something deep hurting when you finally find exactly what you're looking
>>> for,
>>> only to learn that it is deprecated with another option which does not
>>> provide
>>> the same functionality.
>>
>> This description is not entirely accurate. I would put it like this: you
>> stumble upon a mis-use of a feature that was originally meant for
>> something else. It seems that the original consumers of the feature have
>> always been unhappy about the mis-use (it presents a risk for the
>> subsystem they are responsible for), in spite of the mis-use possibly
>> scratching your itch too.
>>
>> That shouldn't hurt your feelings -- the point is that it's not
>> "deprecation"; the original thing has never been condoned for this
>> particular creative kind of abuse. Instead, the feature that could
>> scratch your itch has never existed.
> 
> I understand why you wish that was the case, but I don't think that
> is the reality today.
> 
> The QEMU manual page has explicitly documented -fw_cfg, even going
> as far as illustrating its usage for passing application specific
> data to the guest:
> 
> [quote]
>  Add named fw_cfg entry with contents from string str.
>  
>  The terminating NUL character of the contents of str will
>  not be included as part of the fw_cfg item data. To insert
>  contents with embedded NUL characters, you have to use the
>  file parameter.
> 
>  The fw_cfg entries are passed by QEMU through to the guest.
> 
>  Example:
>  -fw_cfg name=opt/com.mycompany/blob,file=./my_blob.bin
> 
>  creates an fw_cfg entry named opt/com.mycompany/blob with
>  contents from ./my_blob.bin.
> [/quote]
> 
> Similarly the -help output merely lists -fw_cfg as a "debug/expert"
> option, and doesn't give any indication it is not for application
> usage.
> 
> IOW, saying that this is not for general application usage is
> directly contradicting QEMU's own documentation that exists today.
> 
> If you want to change such that this option is marked not for
> application usage, then that is certainly a deprecation of an
> existing documented feature in QEMU.

... fair enough I guess.

Laszlo

Re: [PATCH] docs: Discourage users from using fwcfg

[Laszlo Ersek]

On 09/07/20 15:48, Michal Privoznik wrote:
> Even though this was brought up in upstream discussion [1] it
> missed my patches: users should prefer  over fwcfg.
> The reason is that fwcfg is considered somewhat internal to QEMU
> and it has limited number of slots and neither of these applies
> to .
>
> While I'm at it, I'm fixing the example too (because it contains
> incorrect element name) and clarifying sysfs/ exposure.
>
> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>
> Reported-by: Laszlo Ersek 
> Signed-off-by: Michal Privoznik 
> ---
>  docs/formatdomain.rst | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 1979dfb8d3..821ffe8d60 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -509,18 +509,22 @@ layout of sub-elements, with supported values of:
> Some hypervisors provide unified way to tweak how firmware configures 
> itself,
> or may contain tables to be installed for the guest OS, for instance boot
> order, ACPI, SMBIOS, etc. It even allows users to define their own config
> -   blobs. In case of QEMU, these then appear under domain's sysfs, under
> +   blobs. In case of QEMU, these then appear under domain's sysfs (if the 
> guest
> +   kernel has FW_CFG_SYSFS config option enabled), under
> ``/sys/firmware/qemu_fw_cfg``. Note, that these values apply regardless 
> the
>  mode under . :since:`Since 6.5.0`
>
> +   **Please note that because of limited number of data slots use of fwcfg is
> +   strongly discouraged and  should be used instead**.

please replace:

  strongly discouraged

with:

  strongly discouraged for configuring any guest-side component other
  than the firmware

(

Consider for example the following feature:

  https://bugzilla.tianocore.org/show_bug.cgi?id=2681

Namely, the following QEMU switches:

  -fw_cfg name=opt/org.tianocore/IPv4PXESupport,string=[yn]
  -fw_cfg name=opt/org.tianocore/IPv6PXESupport,string=[yn]

alter the behavior of OVMF and ArmVirtQemu. These flags are meant to be
stable. They do not need dedicated QEMU or libvirtd enablement. They
influence firmware behavior. So  is perfectly fine
(even ideal!) for tweaking them, through the domain XML. What's not fine
is configuring any random guest payload via .

The point is that people who parse new fw_cfg files in edk2 such as
"opt/org.tianocore/IPv6PXESupport" are conscious of the slot count in
QEMU. They *can* bump the "x-file-slots" property in QEMU, for new
machine types, they just need to be aware of the property.

)

> +
> ::
>
> -
> +
>example value

I suggest (according to the above):

- name: opt/org.tianocore/IPv4PXESupport
- value: n

> -  
> -
> +  

We have a functional -- working, stable -- example for name+file as
well:

- name: etc/edk2/https/cacerts
- file: /etc/pki/ca-trust/extracted/edk2/cacerts.bin

(This is documented in "OvmfPkg/README" in edk2, but it's not really
relevant here.)

> +
>
> -   The ``smbios`` element can have multiple ``entry`` child elements. Each
> +   The ``sysinfo`` element can have multiple ``entry`` child elements. Each
> element then has mandatory ``name`` attribute, which defines the name of 
> the
> blob and must begin with ``"opt/"`` and to avoid clashing with other 
> names is
> advised to be in form ``"opt/$RFQDN/$name"`` where ``$RFQDN`` is a reverse
>

It's hard to express this cleanly.

- The opt/RFQDN notation is a mitigation for users that are hell-bent on
  using fw-cfg files of their own purposes (not heeding our advice about
  not using fw-cfg for such purposes at all). So the idea is, "if you
  ignore our request, then (a) be prepared to run out of slots, and (b)
  *at least* use a name pattern (opt/RFQDN) that minimizes conflicts
  with other, similar-minded users / projects"

- For "officially supported" knobs that the firmware looks at, it's fine
  to use any names -- they avoid conflicts with the above "rogue" files.
  Examples:

  - opt/ovmf/  -- reserved for historical reasons

  - opt/org.tianocore/ -- should never conflict due to RFQDN

  - etc/edk2/https/... -- should never conflict due to being outside of
  opt/

So I guess the short rule is, "Feel free to refer to any fw_cfg file
name that your firmware officially supports. When defining other fw_cfg
file names (i.e., for your own purposes), then prepare for breakage in
the long-term, and then at least use the opt/RFQDN/ name pattern".

Thank you,
Laszlo

Re: [PATCH] docs: Discourage users from using fwcfg

[Laszlo Ersek]

On 09/08/20 08:37, Martin Kletzander wrote:
> On Mon, Sep 07, 2020 at 03:38:23PM +0100, Daniel P. Berrangé wrote:
>> On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
>>> On 9/7/20 3:57 PM, Martin Kletzander wrote:
>>> > On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>>> > > Even though this was brought up in upstream discussion [1] it
>>> > > missed my patches: users should prefer  over fwcfg.
>>> > > The reason is that fwcfg is considered somewhat internal to QEMU
>>> > > and it has limited number of slots and neither of these applies
>>> > > to .
>>> > >
>>> > > While I'm at it, I'm fixing the example too (because it contains
>>> > > incorrect element name) and clarifying sysfs/ exposure.
>>> > >
>>> > > 1:
>>> https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>> > >
>>> > > Reported-by: Laszlo Ersek 
>>> > > Signed-off-by: Michal Privoznik 
>>> > > ---
>>> > > docs/formatdomain.rst | 14 +-
>>> > > 1 file changed, 9 insertions(+), 5 deletions(-)
>>> > >
>>>
>>>
>>> > It's nice that you say that, but people who would like to use
>>> fw_cfg for
>>> > passing
>>> > in a huge blob, which is saved in a file, will read this, go to
>>> > 
>>> > and see that there is no way to pass a file as an input.  Should
>>> that be
>>> > dealt
>>> > with somehow?  Or would that be discouraged as well?
>>>
>>> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at
>>> least
>>> quick glance over hw/smbios/smbios.c doesn't show any signs it's
>>> allowed).
>>
>> Indeed, that is an RFE I've never got around to
>>
> 
> Do you mean RFE for QEMU or libvirt?
> 
> Are there any limitations for the oemStrings?  Libvirt could read the
> file and
> pass the data to QEMU as it is not something that is supposed to be
> writeable or
> shareable.  I understand it could be the first time we do something like
> this,
> but it might not be that bad of a precedence.
> 
> Just so we are on the same page, I'm not against this patch, I just want
> to save
> our users the frustration that I, myself, experienced so many times. 
> There's
> something deep hurting when you finally find exactly what you're looking
> for,
> only to learn that it is deprecated with another option which does not
> provide
> the same functionality.

This description is not entirely accurate. I would put it like this: you
stumble upon a mis-use of a feature that was originally meant for
something else. It seems that the original consumers of the feature have
always been unhappy about the mis-use (it presents a risk for the
subsystem they are responsible for), in spite of the mis-use possibly
scratching your itch too.

That shouldn't hurt your feelings -- the point is that it's not
"deprecation"; the original thing has never been condoned for this
particular creative kind of abuse. Instead, the feature that could
scratch your itch has never existed.

Thanks,
Laszlo


> Sure, you can have a start hook that reads a
> file and
> changes the data, etc.  But you get the point.  At least we could
> mention that?
> 
>> Regards,
>> Daniel
>> -- 
>> |: https://berrange.com      -o-   
>> https://www.flickr.com/photos/dberrange :|
>> |: https://libvirt.org         -o-           
>> https://fstop138.berrange.com :|
>> |: https://entangle-photo.org    -o-   
>> https://www.instagram.com/dberrange :|

Re: [PATCH] docs: Discourage users from using fwcfg

[Laszlo Ersek]

On 09/07/20 16:38, Daniel P. Berrangé wrote:
> On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
>> On 9/7/20 3:57 PM, Martin Kletzander wrote:
>>> On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>>>> Even though this was brought up in upstream discussion [1] it
>>>> missed my patches: users should prefer  over fwcfg.
>>>> The reason is that fwcfg is considered somewhat internal to QEMU
>>>> and it has limited number of slots and neither of these applies
>>>> to .
>>>>
>>>> While I'm at it, I'm fixing the example too (because it contains
>>>> incorrect element name) and clarifying sysfs/ exposure.
>>>>
>>>> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>>>
>>>> Reported-by: Laszlo Ersek 
>>>> Signed-off-by: Michal Privoznik 
>>>> ---
>>>> docs/formatdomain.rst | 14 +-
>>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>>
>>
>>
>>> It's nice that you say that, but people who would like to use fw_cfg for
>>> passing
>>> in a huge blob, which is saved in a file, will read this, go to
>>> 
>>> and see that there is no way to pass a file as an input.  Should that be
>>> dealt
>>> with somehow?  Or would that be discouraged as well?
>>
>> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at least
>> quick glance over hw/smbios/smbios.c doesn't show any signs it's allowed).
> 
> Indeed, that is an RFE I've never got around to

Yes, I remember that -- I remember that we discussed this, and also that
you didn't have time for it. Which is entirely justifiable; other stuff
has been (and continues to be) more important.

For reference, here's the launchpad ticket that tracks the RFE for QEMU:

  https://bugs.launchpad.net/qemu/+bug/1826200

Thanks,
Laszlo

Re: [libvirt PATCH v2] kbase: sev: Provide more details on virtio-net configuration

[Laszlo Ersek]

On 08/11/20 14:12, Erik Skultety wrote:
> With virtio-net we also need to disable the iPXE option ROM otherwise
> a SEV-enabled guest would not boot. While at it, fix the full machine
> XML examples accordingly.
> 
> Reported-by: Dr. David Alan Gilbert 
> Signed-off-by: Erik Skultety 
> ---
> since v1:
> - ditched any mentions of vhost, since we can assume all the supported
>   distros to have the latest QEMU-2.12 build containing the bugfix to make
>   vhost work with SEV
> 
> 
>  docs/kbase/launch_security_sev.rst | 28 ++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/kbase/launch_security_sev.rst 
> b/docs/kbase/launch_security_sev.rst
> index cfdc2a6120..4a37c0c379 100644
> --- a/docs/kbase/launch_security_sev.rst
> +++ b/docs/kbase/launch_security_sev.rst
> @@ -291,8 +291,9 @@ can still perform DoS on each other.
>  Virtio
>  --
> 
> -In order to make virtio devices work, we need to enable emulated IOMMU
> -on the devices so that virtual DMA can work.
> +In order to make virtio devices work, we need to use
> + inside the given device XML element in order
> +to enable DMA API in the virtio driver.
> 
>  ::
> 
> @@ -337,6 +338,26 @@ model, which means that virtio GPU cannot be used.
>   ...
> 
> 
> +Virtio-net
> +~~
> +With virtio-net it's also necessary to disable the iPXE option ROM as
> +iPXE is not aware of SEV (at the time of this writing). This translates to 
> the
> +following XML:
> +
> +::
> +
> +   
> + ...
> + 
> +...
> +   
> +   
> +   
> + 
> + ...
> +   
> +
> +
>  Checking SEV from within the guest
>  ==
> 
> @@ -424,6 +445,7 @@ Q35 machine
>   
>   
>   
> + 
> 
>     
>   
> @@ -496,6 +518,8 @@ PC-i440fx machine
>   
>   
>   
> + 
> + 
> 
> 
>   
> --
> 2.26.2
> 

Reviewed-by: Laszlo Ersek 

Re: [libvirt PATCH] kbase: sev: Provide more details on virtio-net configuration

[Laszlo Ersek]

On 08/07/20 13:21, Erik Skultety wrote:
> With virtio-net further configuration settings are required, so document
> them and while at it, fix the Q35 machine XML example which wouldn't
> work with SEV because of not disabling vhost and the option boot ROM.

(1) Please drop:

  not disabling vhost and

(2) please replace

  the option boot ROM

with

  the iPXE option ROM

(more details below)

> 
> Reported-by: Dr. David Alan Gilbert 
> Signed-off-by: Erik Skultety 
> ---
>  docs/kbase/launch_security_sev.rst | 28 +---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/kbase/launch_security_sev.rst 
> b/docs/kbase/launch_security_sev.rst
> index cfdc2a6120..9df4178aac 100644
> --- a/docs/kbase/launch_security_sev.rst
> +++ b/docs/kbase/launch_security_sev.rst
> @@ -291,8 +291,9 @@ can still perform DoS on each other.
>  Virtio
>  --
>  
> -In order to make virtio devices work, we need to enable emulated IOMMU
> -on the devices so that virtual DMA can work.
> +In order to make virtio devices work, we need to use
> + inside the given device XML element in order
> +to enable DMA API in the virtio driver.
>  
>  ::
>  

This hunk looks good.

> @@ -337,6 +338,26 @@ model, which means that virtio GPU cannot be used.
>   ...
> 
>  
> +Virtio-net
> +~~
> +With virtio-net it's also necessary to disable the iPXE option ROM on the
> +device as well as disable the vhost protocol

(3) Please break these items into separate sentences.

(4) Please restrict the latter (the vhost disablement) to QEMU version
v2.12.0 exactly.

(Per another part in this document, SEV appeared in QEMU v2.12.0, so we
need not consider anything earlier. And the vhost disablement is
unneeded with both v3.0.0 and v2.12.1, due to QEMU commits d542800d1edc
and 2f2b18923502, respectively. So the only QEMU version that needs the
vhost disablement is v2.12.0.)

> as SEV doesn't support either
> +(at the time of this writing).

(5) This statement is not correct:

First, vhost does support SEV, only QEMU had a small bug (see the
above-named commits) that prevented vhost from working with SEV. It's
not a "total lack of support".

Second, regarding iPXE, it's not that SEV doesn't support iPXE; it's
iPXE that is unaware of SEV, at the time of this writing.

> This translates to the following XML:
> +
> +::
> +
> +   
> + ...
> + 
> +...
> +   
> +   
> +   
> + 
> + ...
> +   
> +
> +
>  Checking SEV from within the guest
>  ==
>  

(6) So the @name='qemu' attribute for the  element should be
removed, assuming we intend to provide an example XML fragment only for
the latest QEMU version (at the time of this writing).

> @@ -423,7 +444,8 @@ Q35 machine
>   
>   
>   
> - 
> + 
> + 
> 
> 
>   
> 

(7) Same as (6).


... Ultimately, if any distro uses a v2.12-based QEMU, perhaps we can
expect that distro to use the latest stable release in the v2.12
"release stream". If we do have that expectation of distros, then we
should simply drop all mentions of "vhost".

Thanks!
Laszlo

Re: [libvirt] [PATCH v2 0/4] domain capabilities: Expose firmware auto selection feature

[Laszlo Ersek]

Hi,

On 04/09/19 16:52, Michal Privoznik wrote:
> v2 of:
> 
> https://www.redhat.com/archives/libvir-list/2019-April/msg00460.html
> 
> diff to v1:
> - Expose 'secure' too
> - Switch to uint64_t for qemuFirmwareGetSupported()
> 
> Michal Prívozník (4):
>   qemu_firmware: Separate firmware loading into a function
>   qemu_firmware: Separate machine and arch matching into a function
>   qemu_firmware: Introduce qemuFirmwareGetSupported
>   domain capabilities: Expose firmware auto selection feature
> 
>  docs/formatdomaincaps.html.in |  23 +++
>  docs/schemas/domaincaps.rng   |   1 +
>  src/conf/domain_capabilities.c|   3 +
>  src/conf/domain_capabilities.h|   2 +
>  src/qemu/qemu_capabilities.c  |  35 +++-
>  src/qemu/qemu_capabilities.h  |   1 +
>  src/qemu/qemu_driver.c|   1 +
>  src/qemu/qemu_firmware.c  | 169 ++
>  src/qemu/qemu_firmware.h  |  10 ++
>  tests/Makefile.am |   4 +-
>  .../qemu_1.7.0.x86_64.xml |   7 +
>  .../qemu_2.12.0-virt.aarch64.xml  |   6 +
>  .../qemu_2.12.0.ppc64.xml |   4 +
>  .../qemu_2.12.0.s390x.xml |   4 +
>  .../qemu_2.12.0.x86_64.xml|   7 +
>  .../qemu_2.6.0-virt.aarch64.xml   |   6 +
>  .../qemu_2.6.0.aarch64.xml|   4 +
>  .../domaincapsschemadata/qemu_2.6.0.ppc64.xml |   4 +
>  .../qemu_2.6.0.x86_64.xml |   7 +
>  .../domaincapsschemadata/qemu_2.7.0.s390x.xml |   4 +
>  .../qemu_2.8.0-tcg.x86_64.xml |   7 +
>  .../domaincapsschemadata/qemu_2.8.0.s390x.xml |   4 +
>  .../qemu_2.8.0.x86_64.xml |   7 +
>  .../qemu_2.9.0-q35.x86_64.xml |   8 +
>  .../qemu_2.9.0-tcg.x86_64.xml |   7 +
>  .../qemu_2.9.0.x86_64.xml |   7 +
>  .../domaincapsschemadata/qemu_3.0.0.s390x.xml |   4 +
>  .../qemu_3.1.0.x86_64.xml |   7 +
>  .../qemu_4.0.0.x86_64.xml |   7 +
>  tests/domaincapstest.c|  16 ++
>  tests/qemufirmwaretest.c  |  72 
>  31 files changed, 412 insertions(+), 36 deletions(-)
> 

you didn't push these patch sets to your personal repo, and also didn't
mention the fork-off commits on master. This matters because neither v1
nor v2 applies on top of master now (i.e., on a5e16020907e). So I tried
to correlate the posting timestamps of the cover letters with the commit
dates (not authorship dates) of the recent commits in the git history.
Ultimately I applied your
- v1 on top of fb0d6049cccf ("docs: Remove search.php and all
references", 2019-04-04), and
- v2 on top of c3e1275b6020 ("rpc: Refactor cleanup paths in
virNetLibsshAuthenticatePassword", 2019-04-09).

Then (because I have very little time for reviewing this,
unfortunately), I ran

$ git range-diff master michal_v1 michal_v2


... From that, I have two comments for the testSupportedFW() function:


(1) You still have one instance of:

++expectedInterfaces |= 1 << data->interfaces[i];

Please update the integer constant 1 to 1ULL here as well.


(2) You have an error message in

++if (actualSecure != data->secure) {
++fprintf(stderr,
++"Mismatch in supported secure boot. "
++"Expected %d got %d\n",
++data->secure, actualSecure);
 +return -1;
 +}

Please replace

  "Mismatch in supported secure boot. "

with

  "Mismatch in SMM requirement/support. "

(The commit message has been updated correctly already: it says "list of
supported interfaces and SMM feature", so that's OK.)

With (1) and (2) addressed:

Acked-by: Laszlo Ersek 


(If there are no other updates, I'm fine if you don't post v3 just for
these.)

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 4/4] domain capabilities: Expose firmware auto selection feature

[Laszlo Ersek]

On 04/05/19 10:44, Michal Privoznik wrote:
> On 4/5/19 10:31 AM, Daniel P. Berrangé wrote:
>> On Fri, Apr 05, 2019 at 09:19:48AM +0200, Michal Privoznik wrote:
>>> If a management application wants to use firmware auto selection
>>> feature it can't currently know if the libvirtd it's talking to
>>> support is or not. Moreover, it doesn't know which values that
>>> are accepted for the @firmware attribute of  when parsing
>>> will allow successful start of the domain later, i.e. if the mgmt
>>> application wants to use 'bios' whether there exists a FW
>>> descriptor in the system that describes bios.
>>>
>>> This commit then adds 'firmware' enum to  element in
>>>  XML like this:
>>>
>>>
>>>  bios
>>>  efi
>>>
>>>
>>> We can see both 'bios' and 'efi' listed which means that there
>>> are descriptors for both found in the system (matched with the
>>> machine type and architecture reported in the domain capabilities
>>> earlier and not shown here).
>>
>> I wonder if we should also have a enum for the "secure" attribute
>> of  to deal with SecureBoot
>>
>> 
>>   yes
>>   no
>> 
>>
>> Always report "no", only report "yes" if there is at least one
>> firmware file we see that can do SecureBoot.
>>
>> Yes, in theory that is a matrix against the firmware attribute
>> value, but we have many such dependancies between attributes
>> and it is not practical to fully express all valid combinations
>> in the capabilities, so taking the simple approach is valid
>> IMHO.
>
> Makes sense, I'll put it on my TODO list for v2.

With the above, the series looks good to me as well (mostly ready to
ACK).

I have a low-level question though. In patch #3:

  verify(sizeof(unsigned int) >= ((1ULL << VIR_DOMAIN_OS_DEF_FIRMWARE_LAST) >> 
2));

are you trying to express that all non-LAST enum values are <= 31?

I'm probably missing something, but on the LHS, you have a number of
bytes, while on the RHS, you have a bitmask with the "LAST" bit set,
divided by 4 (not 8).


A related question for patch #4:

  1 << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS

Apparently, we'd like to store such bitmasks in "unsigned int" objects
however (not in "int"s), so all similar expressions should be written
like

  1u << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS

Because otherwise, if (1 << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS) is
unrepresentable as a signed int (e.g. int is int32_t and
VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS has value 31), then the behavior is
undefined.


... Honestly I'd just go with "uint64_t" -- it's an optional type, per
standard C, but libvirt already uses that type elsewhere,
unconditionally. Then you could use:

  verify(VIR_DOMAIN_OS_DEF_FIRMWARE_LAST <= 64);

(assuming you never want to set the "LAST" bit in the mask)

and

  UINT64_C(1) << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS

for creating the single-bit masks.

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [PULL v3 00/54] Kconfig conversion, excluding ARM and MIPS

[Laszlo Ersek]

On 03/21/19 18:58, Peter Maydell wrote:
> On Thu, 21 Mar 2019 at 17:35, Laszlo Ersek  wrote:
>> It simply keeps the status quo from before the patchset.
>>
>> The specific emulation targets that virtio-vga should not be enabled for
>> (regardless of other targets / machine types) are arm/aarch64. IOW, in
>> the longer term, it's not that virtio-vga is suitable only for PC ||
>> DINO || PSERIES, but that it's *not* suitable for arm/aarch64.
> 
> Relying on the device not being present for aarch64 seems
> fragile to me. It will break if we add a different
> aarch64 machine other than "virt" and want that other
> machine to have virtio-vga. It will also break if we
> ever manage to get full heterogenous-CPU support and
> end up with a single qemu-system binary that can emulate
> both AArch64 virt and x86-64 pc machines :-)
> 
> If what you want is "don't use virtio-vga when using KVM
> on aarch64" then why not have logic that implements that?

That's actually the logic that I implemented originally (or something very 
close to it -- 
<https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=706b5b627719e95a33606c463bc83c841c7b5b0e>),
 but since then, this code has been modified in libvirt multiple times:

* [libvirt] [PATCH v2 00/17] cleanups and improvements for video device code
  https://www.redhat.com/archives/libvir-list/2016-October/msg00533.html
  
https://libvirt.org/git/?p=libvirt.git;a=shortlog;hp=a62655f9c33a8f7c6257779ddbdbf1352d53a526;h=fb8f3b1c22c8f272bb9a47e8f8915acc3cfb47f1

* [libvirt] [PATCH v3 0/6] Add support for video and input devices on S390
  https://www.redhat.com/archives/libvir-list/2018-March/msg01519.html
  
https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=4bbf7f8cb5bf979ea2f5c247b2ddb884a07ac33f

I'm not implying that those patches must have introduced a libvirt regression, 
I just agree that the code deserves investigation. This is why I added 
libvir-list to the CC list when reporting the problem.

> For 4.0 we should definitely just retain what we had
> before the introduction of KConfig, but as a general
> thing we should look to not have to have this odd
> wrinkle.
> 
> (AFAIK virtio-vga is no different in this regard to the other
> vga PCI devices, which also won't work in KVM setups.)

I agree -- please refer to the "domcapabilities" part (1) of my report:

https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg06060.html

> @@ -92,6 +92,8 @@
>  
>
>  vga
> +cirrus
> +vmvga
>  virtio
>
>  

"cirrus" and "vmvga" look wrong for aarch64.

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [PULL v3 00/54] Kconfig conversion, excluding ARM and MIPS

[Laszlo Ersek]

On 03/21/19 15:21, Philippe Mathieu-Daudé wrote:
> Le jeu. 21 mars 2019 13:39, Laszlo Ersek  a écrit :
> 
>> On 03/21/19 01:53, Laszlo Ersek wrote:
>>> Hi Paolo,
>>>
>>> (+libvir-list)
>>>
>>> I'd like to report a regression introduced by this patch set:
>>>
>>> On 03/07/19 21:58, Paolo Bonzini wrote:
>>>> The following changes since commit
>>>> 6cb4f6db4f4367faa33da85b15f75bbbd2bed2a6:
>>>>
>>>>   Merge remote-tracking branch
>>>>   'remotes/cleber/tags/python-next-pull-request' into staging
>>>>   (2019-03-07 16:16:02 +)
>>>>
>>>> are available in the Git repository at:
>>>>
>>>>   git://github.com/bonzini/qemu.git tags/for-upstream-kconfig
>>>>
>>>> for you to fetch changes up to
>>>> 576c3f2f16e7392e28cc9fe10d9a920d67d3645b:
>>>>
>>>>   kconfig: add documentation (2019-03-07 21:54:22 +0100)
>>>>
>>>> 
>>>> Initial Kconfig work, excluding ARM and MIPS
>>>>
>>>> 
>>>
>>
>> In brief, the regression is that the aarch64 system emulator now lists
>> the "virtio-vga" device for the "virt" machine type:
>>
>> $ qemu-system-aarch64 -M virt -device \? | grep virtio-vga
>> name "virtio-vga", bus PCI
>>
>> Here's where I think the issue was introduced:
>>
>> (1) In commit 82f5181777eb ("kconfig: introduce kconfig files",
>> 2019-03-07), VIRTIO_VGA was introduced simply as a bool (with no other
>> clauses) in "hw/display/Kconfig".
>>
>> (2) In commit 7c28b925b7e1 ("build: convert pci.mak to Kconfig",
>> 2019-03-07), VIRTIO_VGA received the following clauses:
>>
>> default y if PCI_DEVICES
>> depends on VIRTIO_PCI
>> select VGA
>>
>> This is too lax, because it permits virtio-vga for aarch64, while that
>> shouldn't happen (and it doesn't matches the previous state of the tree).
>>
> 
> Similar case I noticed in another thread. The Virt machine uses a chipset
> that provides PCI. Does this machine expose PCI slots?

It provides a PCI Express hierarchy, so "yes". (If you mean traditional
PCI, that is also available through the pcie-pci bridge device model,
which you can plug into the PCIE hierarchy.)

> If yes, this config
> is correct, the machine can get any PCI daughter card.

Haha, right, in theory :) On the same note, the ARMv8 architecture
should also, "in theory", have allowed the host side page mappings
unconditionally override the guest side mappings, like it works on x86.
Because in that case, QemuVideoDxe / the framebuffer would have just
worked in aarch64 guests too, I wouldn't have had to write VirtioGpuDxe
in the first place, and everybody would have been happy with just
virtio-vga.

But ARMv8 is not like that, so things don't work like that.

> Else we shouldn't
> use PCI_DEVICES that way.
> 
> Why Virt machine now default to virtio-vga?

See above, plus my answer to Peter.

Thanks
Laszlo

>> (3) In commit b42075bb7767 ("virtio: express virtio dependencies with
>> Kconfig", 2019-03-07), the determination of virtio-vga was switched to
>> the Kconfig system. Importantly, in this patch, the line
>>
>>   CONFIG_VIRTIO_VGA=y
>>
>> was removed *only* from the following file:
>>
>>   default-configs/i386-softmmu.mak
>>
>> (4) Both of the remaining instances of
>>
>>   CONFIG_VIRTIO_VGA=y
>>
>> were then removed in commit 9483cf27dd36 ("hppa-softmmu.mak: express
>> dependencies with Kconfig", 2019-03-07) and commit 87f9108bad0c ("ppc64:
>> Express dependencies of 'pseries' and 'powernv' machines with kconfig",
>> 2019-03-07), from files
>>
>>   default-configs/hppa-softmmu.mak
>>   default-configs/ppc64-softmmu.mak
>>
>> respectively.
>>
>>
>> Therefore I think commit 7c28b925b7e1 has the bug. The VIRTIO_VGA
>> restriction
>>
>>   depends on VIRTIO_PCI
>>
>> should now be replaced with
>>
>>   depends on VIRTIO_PCI && (PC || DINO || PSERIES)
>>
>> Thanks
>> Laszlo
>>
>>
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [PULL v3 00/54] Kconfig conversion, excluding ARM and MIPS

[Laszlo Ersek]

On 03/21/19 14:04, Peter Maydell wrote:
> On Thu, 21 Mar 2019 at 12:40, Laszlo Ersek  wrote:
>> In brief, the regression is that the aarch64 system emulator now lists
>> the "virtio-vga" device for the "virt" machine type:
>>
>> $ qemu-system-aarch64 -M virt -device \? | grep virtio-vga
>> name "virtio-vga", bus PCI
>>
>> Here's where I think the issue was introduced:
>>
>> (1) In commit 82f5181777eb ("kconfig: introduce kconfig files",
>> 2019-03-07), VIRTIO_VGA was introduced simply as a bool (with no other
>> clauses) in "hw/display/Kconfig".
>>
>> (2) In commit 7c28b925b7e1 ("build: convert pci.mak to Kconfig",
>> 2019-03-07), VIRTIO_VGA received the following clauses:
>>
>> default y if PCI_DEVICES
>> depends on VIRTIO_PCI
>> select VGA
>>
>> This is too lax, because it permits virtio-vga for aarch64, while that
>> shouldn't happen (and it doesn't matches the previous state of the tree).
>>
>> (3) In commit b42075bb7767 ("virtio: express virtio dependencies with
>> Kconfig", 2019-03-07), the determination of virtio-vga was switched to
>> the Kconfig system. Importantly, in this patch, the line
>>
>>   CONFIG_VIRTIO_VGA=y
>>
>> was removed *only* from the following file:
>>
>>   default-configs/i386-softmmu.mak
>>
>> (4) Both of the remaining instances of
>>
>>   CONFIG_VIRTIO_VGA=y
>>
>> were then removed in commit 9483cf27dd36 ("hppa-softmmu.mak: express
>> dependencies with Kconfig", 2019-03-07) and commit 87f9108bad0c ("ppc64:
>> Express dependencies of 'pseries' and 'powernv' machines with kconfig",
>> 2019-03-07), from files
>>
>>   default-configs/hppa-softmmu.mak
>>   default-configs/ppc64-softmmu.mak
>>
>> respectively.
>>
>>
>> Therefore I think commit 7c28b925b7e1 has the bug. The VIRTIO_VGA
>> restriction
>>
>>   depends on VIRTIO_PCI
>>
>> should now be replaced with
>>
>>   depends on VIRTIO_PCI && (PC || DINO || PSERIES)
> 
> Should it? What makes the virtio-vga device only
> suitable for those three machines?

It simply keeps the status quo from before the patchset.

The specific emulation targets that virtio-vga should not be enabled for
(regardless of other targets / machine types) are arm/aarch64. IOW, in
the longer term, it's not that virtio-vga is suitable only for PC ||
DINO || PSERIES, but that it's *not* suitable for arm/aarch64.

My original libvirt commit explains it:

https://libvirt.org/git/?p=libvirt.git;a=commit;h=706b5b627719e95a33606c463bc83c841c7b5b0e


Regarding the guest firmware: in edk2 there are two relevant drivers,
QemuVideoDxe and VirtioGpuDxe. The former is framebuffer-based, the
latter is a genuine virtio drier (no framebuffer). The former doesn't
work on arm/aarch64 KVM, the latter does.

The "virtio-vga" device presents a conflict for these drivers because it
could be bound by both drivers, at the same time. (The device presents
both interfaces.) Arbitration between UEFI drivers is very tricky. To
keep things platform independent and sane in both drivers, VirtioGpuDxe
never binds virtio-vga, only virtio-gpu-pci. Therefore, in OVMF, where
both drivers are included, virtio-vga is deterministically bound by
QemuVideoDxe, a framebuffer is exposed, and Windows is happy. You can
still use virtio-gpu-pci, and that will be bound by VirtioGpuDxe
(windows won't be happy, but you asked for it).

Whereas in ArmVirtQemu, only VirtioGpuDxe is included, as QemuVideoDxe
has no chance of working (on KVM). If you then boot with virtio-vga,
VirtioGpuDxe will still yield... but there won't be another driver to
bind the device instead. So, there you *must* use virtio-gpu-pci, for
VirtioGpuDxe to like the device.

Again, the goal is to keep VirtioGpuDxe and QemuVideoDxe
platform-independent; i.e. refrain from aarch64-specific and
x64-specific quirks in either. They bind or yield purely based on the
device model they see.

So, if you allow libvirt to see virtio-vga on aarch64, it will pick
virtio-vga for the guest, and then VirtioGpuDxe will not bind it, and
there's nothing else to bind it.

Thanks,
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [PULL v3 00/54] Kconfig conversion, excluding ARM and MIPS

[Laszlo Ersek]

On 03/21/19 01:53, Laszlo Ersek wrote:
> Hi Paolo,
> 
> (+libvir-list)
> 
> I'd like to report a regression introduced by this patch set:
> 
> On 03/07/19 21:58, Paolo Bonzini wrote:
>> The following changes since commit
>> 6cb4f6db4f4367faa33da85b15f75bbbd2bed2a6:
>>
>>   Merge remote-tracking branch
>>   'remotes/cleber/tags/python-next-pull-request' into staging
>>   (2019-03-07 16:16:02 +)
>>
>> are available in the Git repository at:
>>
>>   git://github.com/bonzini/qemu.git tags/for-upstream-kconfig
>>
>> for you to fetch changes up to
>> 576c3f2f16e7392e28cc9fe10d9a920d67d3645b:
>>
>>   kconfig: add documentation (2019-03-07 21:54:22 +0100)
>>
>> 
>> Initial Kconfig work, excluding ARM and MIPS
>>
>> 
> 

In brief, the regression is that the aarch64 system emulator now lists
the "virtio-vga" device for the "virt" machine type:

$ qemu-system-aarch64 -M virt -device \? | grep virtio-vga
name "virtio-vga", bus PCI

Here's where I think the issue was introduced:

(1) In commit 82f5181777eb ("kconfig: introduce kconfig files",
2019-03-07), VIRTIO_VGA was introduced simply as a bool (with no other
clauses) in "hw/display/Kconfig".

(2) In commit 7c28b925b7e1 ("build: convert pci.mak to Kconfig",
2019-03-07), VIRTIO_VGA received the following clauses:

default y if PCI_DEVICES
depends on VIRTIO_PCI
select VGA

This is too lax, because it permits virtio-vga for aarch64, while that
shouldn't happen (and it doesn't matches the previous state of the tree).

(3) In commit b42075bb7767 ("virtio: express virtio dependencies with
Kconfig", 2019-03-07), the determination of virtio-vga was switched to
the Kconfig system. Importantly, in this patch, the line

  CONFIG_VIRTIO_VGA=y

was removed *only* from the following file:

  default-configs/i386-softmmu.mak

(4) Both of the remaining instances of

  CONFIG_VIRTIO_VGA=y

were then removed in commit 9483cf27dd36 ("hppa-softmmu.mak: express
dependencies with Kconfig", 2019-03-07) and commit 87f9108bad0c ("ppc64:
Express dependencies of 'pseries' and 'powernv' machines with kconfig",
2019-03-07), from files

  default-configs/hppa-softmmu.mak
  default-configs/ppc64-softmmu.mak

respectively.


Therefore I think commit 7c28b925b7e1 has the bug. The VIRTIO_VGA
restriction

  depends on VIRTIO_PCI

should now be replaced with

  depends on VIRTIO_PCI && (PC || DINO || PSERIES)

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [PULL v3 00/54] Kconfig conversion, excluding ARM and MIPS

[Laszlo Ersek]

Hi Paolo,

(+libvir-list)

I'd like to report a regression introduced by this patch set:

On 03/07/19 21:58, Paolo Bonzini wrote:
> The following changes since commit
> 6cb4f6db4f4367faa33da85b15f75bbbd2bed2a6:
>
>   Merge remote-tracking branch
>   'remotes/cleber/tags/python-next-pull-request' into staging
>   (2019-03-07 16:16:02 +)
>
> are available in the Git repository at:
>
>   git://github.com/bonzini/qemu.git tags/for-upstream-kconfig
>
> for you to fetch changes up to
> 576c3f2f16e7392e28cc9fe10d9a920d67d3645b:
>
>   kconfig: add documentation (2019-03-07 21:54:22 +0100)
>
> 
> Initial Kconfig work, excluding ARM and MIPS
>
> 

I use "libvirt-daemon-4.5.0-10.el7.x86_64" at the moment. Here are the
symptoms:

(1) the output of the following command changes across this series:

  virsh domcapabilities qemu 
/opt/qemu-installed-optimized/bin/qemu-system-aarch64 aarch64 virt-4.0

as shown by the diff

> @@ -92,6 +92,8 @@
>  
>
>  vga
> +cirrus
> +vmvga
>  virtio
>
>  
> @@ -111,10 +113,7 @@
>  scsi
>
>
> -  
> -default
> -vfio
> -  
> +  
>  
>
>

(2) given the following domain XML snippet, in a qemu (not KVM) aarch64
guest that I have,

> 
>   
>function='0x0'/>
> 

the QEMU command line generated by libvirt changes:

> -  -device virtio-gpu-pci,id=video0,max_outputs=1,bus=pci.4,addr=0x0 \
> +  -device virtio-vga,id=video0,max_outputs=1,bus=pci.4,addr=0x0 \

Perhaps this series causes (1), which in turn causes (2), or else this
series causes both (1) and (2) independently of each other. I'm not sure
about that. Either way, (2) is a problem, because virtio-vga is
inappropriate for aarch64 guests (and the ArmVirtQemu platform firmware
in edk2 won't even try to drive it -- a blank screen is the result).


Now, I can't pinpoint an exact commit, because (after a *brutal*
bisection) I get:

> There are only 'skip'ped commits left to test.
> The first bad commit could be any of:
> a02c0edb35662de38d400f42b68845540669ca3d
> 03b348bdcbd1eda4ce097b2b84527dec774d80c4
> d6e9c470fc91f75db1785f17a9d3567d5a27953d
> a7e23159074c9d774fb1e88357b778994a0c9365
> bcb129b3154ba743f8e52c21c331a0dfcaee7c38
> 02017ee385ef574133c4a978d368640772978178
> 7c28b925b7e176b4e44ed05d23cf883561000546
> 1550b0e6bfe3ab6985e5ad75df1c528a0ca39468
> e9947d18df97e6c6584f020cf9cc995404cc8061
> 8f01b41e1098d8cb9491fa3ea7bd59cf187a5bd7
> 9533dcdd416530a0d72140c122bf90517b6c81eb
> 32690c8bed5762518272876dcb6dd39a54f54fd1
> c3a98aa54c734dcb7a36d193c6330d8f04d4bf8e
> ccf222a816d59af1318a7efb59c6b9c5578d1abf
> f349474920d80838ecea3d421531fdb0660b8740
> 2ac041c2c3d89691cda1657981c41fe4bc20244b
> e0e312f3525ad6ac18ba6633af29190dd9620cbc
> b42075bb77672616127c9452c0f836e757e9ce1a
> We cannot bisect more!

Here's how I built each step of the bisection:

- "git clean -ffdx" and "git submodule deinit --all --force" on the
  source directory

- create a new, pristine build directory

- configure QEMU for out-of-tree build, in that directory

- select the i386, x86_64, arm, and aarch64 targets (all softmmu)

- report "skip" to git-bisect iff the build breaks, otherwise test the
  aarch64 emulator through libvirt, and report good/bad dependent on the
  test result.

The bisection log is attached -- "git bisect replay" and "git bisect
visualize" should tell the story better than the above dump of commit
hashes. Basically:

- The tree builds up to and including commit 82f5181777eb ("kconfig:
  introduce kconfig files", 2019-03-07), and that commit works fine as
  well.

- Then the build breaks (starting with commit e0e312f3525a, "build:
  switch to Kconfig", 2019-03-07).

- The tree becomes buildable again at commit b42075bb7767 ("virtio:
  express virtio dependencies with Kconfig", 2019-03-07), but at that
  point, the functionality has been regressed.

Thanks,
Laszlo
git bisect start
# good: [1c5d9d8f111b9cfc0722c7edcdc3b090736972e5] Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20190218' into staging
git bisect good 1c5d9d8f111b9cfc0722c7edcdc3b090736972e5
# bad: [62a172e6a77d9072bb1a18f295ce0fcf4b90a4f2] Update version for v4.0.0-rc0 release
git bisect bad 62a172e6a77d9072bb1a18f295ce0fcf4b90a4f2
# good: [469bb49b3e131b5f641939d4fa6a4b09e6da47f8] qos-test: megasas test node
git bisect good 469bb49b3e131b5f641939d4fa6a4b09e6da47f8
# bad: [eda1df0345f5a1e337e30367124dcb0e802bdfde] Merge remote-tracking branch 'remotes/armbru/tags/pull-pflash-2019-03-11' into staging
git bisect bad eda1df0345f5a1e337e30367124dcb0e802bdfde
# bad: [79d8b1dc5b44d806d9700f2e8a8028075a8ff2b2] Merge remote-tracking branch 'remotes/kraxel/tags/ui-20190311-v2-pull-request' into staging
git bisect bad 79d8b1dc5b44d806d9700f2e8a8028075a8ff2b2
# bad: [e56d931a9ddee7230f647a25ada33ee19d26aa6d] Merge 

Re: [libvirt] [PATCH v2 15/15] qemuxml2argvtest: Test os.firmware autoselection

[Laszlo Ersek]

On 03/12/19 16:15, Michal Privoznik wrote:
> On 3/11/19 4:27 PM, Daniel P. Berrangé wrote:
>> On Thu, Mar 07, 2019 at 10:29:25AM +0100, Michal Privoznik wrote:
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>   tests/Makefile.am |  4 +-
>>>   ...arch64-os-firmware-efi.aarch64-latest.args | 37 ++
>>>   .../aarch64-os-firmware-efi.xml   | 30 
>>>   .../os-firmware-bios.x86_64-latest.args   | 39 +++
>>>   tests/qemuxml2argvdata/os-firmware-bios.xml   | 68 +++
>>>   ...os-firmware-efi-secboot.x86_64-latest.args | 43 
>>>   .../os-firmware-efi-secboot.xml   | 68 +++
>>>   .../os-firmware-efi.x86_64-latest.args    | 42 
>>>   tests/qemuxml2argvdata/os-firmware-efi.xml    | 68 +++
>>>   tests/qemuxml2argvtest.c  | 17 +
>>>   .../aarch64-os-firmware-efi.xml   |  1 +
>>>   tests/qemuxml2xmloutdata/os-firmware-bios.xml |  1 +
>>>   .../os-firmware-efi-secboot.xml   |  1 +
>>>   tests/qemuxml2xmloutdata/os-firmware-efi.xml  |  1 +
>>>   tests/qemuxml2xmltest.c   | 27 
>>>   15 files changed, 446 insertions(+), 1 deletion(-)
>>>   create mode 100644
>>> tests/qemuxml2argvdata/aarch64-os-firmware-efi.aarch64-latest.args
>>>   create mode 100644 tests/qemuxml2argvdata/aarch64-os-firmware-efi.xml
>>>   create mode 100644
>>> tests/qemuxml2argvdata/os-firmware-bios.x86_64-latest.args
>>>   create mode 100644 tests/qemuxml2argvdata/os-firmware-bios.xml
>>>   create mode 100644
>>> tests/qemuxml2argvdata/os-firmware-efi-secboot.x86_64-latest.args
>>>   create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-secboot.xml
>>>   create mode 100644
>>> tests/qemuxml2argvdata/os-firmware-efi.x86_64-latest.args
>>>   create mode 100644 tests/qemuxml2argvdata/os-firmware-efi.xml
>>>   create mode 12
>>> tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml
>>>   create mode 12 tests/qemuxml2xmloutdata/os-firmware-bios.xml
>>>   create mode 12
>>> tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml
>>>   create mode 12 tests/qemuxml2xmloutdata/os-firmware-efi.xml
>>
>> Reviewed-by: Daniel P. Berrangé 
>>
> 
> Thank you guys both. I've merged these. I'm pleasantly surprised it took
> only two iterations :-)

Thank *you*!
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 10/15] qemufirmwaretest: Test qemuFirmwareFetchConfigs()

[Laszlo Ersek]

On 03/12/19 12:20, Daniel P. Berrangé wrote:
> On Tue, Mar 12, 2019 at 12:13:46PM +0100, Laszlo Ersek wrote:
>> (adding Dan)
>>
>> On 03/07/19 10:29, Michal Privoznik wrote:
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  tests/Makefile.am |  1 +
>>>  .../etc/qemu/firmware/40-ovmf-sb-keys.json|  1 +
>>>  .../etc/qemu/firmware/60-ovmf-sb.json |  0
>>>  .../user/.config/qemu/firmware/10-bios.json   |  0
>>>  .../share/qemu/firmware/40-bios.json} |  0
>>>  .../share/qemu/firmware/50-ovmf-sb-keys.json} |  0
>>>  .../share/qemu/firmware/60-ovmf-sb.json}  |  0
>>>  .../share/qemu/firmware/61-ovmf.json} |  0
>>>  .../share/qemu/firmware/70-aavmf.json}|  0
>>>  tests/qemufirmwaretest.c  | 72 +--
>>>  10 files changed, 68 insertions(+), 6 deletions(-)
>>>  create mode 12 
>>> tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json
>>>  create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json
>>>  create mode 100644 
>>> tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json
>>>  rename tests/qemufirmwaredata/{bios.json => 
>>> usr/share/qemu/firmware/40-bios.json} (100%)
>>>  rename tests/qemufirmwaredata/{ovmf-sb-keys.json => 
>>> usr/share/qemu/firmware/50-ovmf-sb-keys.json} (100%)
>>>  rename tests/qemufirmwaredata/{ovmf-sb.json => 
>>> usr/share/qemu/firmware/60-ovmf-sb.json} (100%)
>>>  rename tests/qemufirmwaredata/{ovmf.json => 
>>> usr/share/qemu/firmware/61-ovmf.json} (100%)
>>>  rename tests/qemufirmwaredata/{aavmf.json => 
>>> usr/share/qemu/firmware/70-aavmf.json} (100%)
>>>
>>> diff --git a/tests/Makefile.am b/tests/Makefile.am
>>> index b3449fa96b..b18b9e67ae 100644
>>> --- a/tests/Makefile.am
>>> +++ b/tests/Makefile.am
>>> @@ -705,6 +705,7 @@ qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS)
>>>  qemufirmwaretest_SOURCES = \
>>> qemufirmwaretest.c \
>>> testutils.h testutils.c \
>>> +   virfilewrapper.c virfilewrapper.h \
>>> $(NULL)
>>>  qemufirmwaretest_LDADD = $(qemu_LDADDS) $(LDADDS)
>>>  
>>> diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json 
>>> b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json
>>> new file mode 12
>>> index 00..68e8cbbc2a
>>> --- /dev/null
>>> +++ b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json
>>> @@ -0,0 +1 @@
>>> +../../../usr/share/qemu/firmware/50-ovmf-sb-keys.json
>>> \ No newline at end of file
>>> diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json 
>>> b/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json
>>> new file mode 100644
>>> index 00..e69de29bb2
>>> diff --git 
>>> a/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json 
>>> b/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json
>>> new file mode 100644
>>> index 00..e69de29bb2
>>> diff --git a/tests/qemufirmwaredata/bios.json 
>>> b/tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json
>>> similarity index 100%
>>> rename from tests/qemufirmwaredata/bios.json
>>> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json
>>> diff --git a/tests/qemufirmwaredata/ovmf-sb-keys.json 
>>> b/tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json
>>> similarity index 100%
>>> rename from tests/qemufirmwaredata/ovmf-sb-keys.json
>>> rename to 
>>> tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json
>>> diff --git a/tests/qemufirmwaredata/ovmf-sb.json 
>>> b/tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json
>>> similarity index 100%
>>> rename from tests/qemufirmwaredata/ovmf-sb.json
>>> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json
>>> diff --git a/tests/qemufirmwaredata/ovmf.json 
>>> b/tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json
>>> similarity index 100%
>>> rename from tests/qemufirmwaredata/ovmf.json
>>> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json
>>> diff --git a/tests/qemufirmwaredata/aavmf.json 
>>> b/tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json
>>> similarity index 100%
>>> rename from tests/qemufi

Re: [libvirt] [PATCH v2 13/15] qemuDomainDefValidate: Don't require SMM if automatic firmware selection enabled

[Laszlo Ersek]

On 03/07/19 10:29, Michal Privoznik wrote:
> The firmware selection code will enable the feature if needed.
> There's no need to require SMM to be enabled in that case.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index e9b2b8453b..32025ea010 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4155,7 +4155,9 @@ qemuDomainDefValidate(const virDomainDef *def,
>  goto cleanup;
>  }
>  
> -if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) 
> {
> +/* SMM will be enabled by qemuFirmwareFillDomain() if needed. */
> +if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE &&
> +def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) 
> {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Secure boot requires SMM feature enabled"));
>  goto cleanup;
> 

OK. This makes sense. It restricts the check to the case when the new
feature is not active.

And the new feature does take care of it, in qemuFirmwareFillDomain() ->
qemuFirmwareEnableFeatures().

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 11/15] qemu_firmware: Introduce qemuFirmwareFillDomain()

[Laszlo Ersek]

 +return -1;
> +
> +if (STRNEQ(flash->nvram_template.format, "raw")) {
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +   _("unsupported nvram template format '%s'"),
> +   flash->nvram_template.format);
> +return -1;
> +}
> +
> +VIR_FREE(def->os.loader->templt);
> +if (VIR_STRDUP(def->os.loader->templt,
> +   flash->nvram_template.filename) < 0)
> +return -1;
> +
> +if (qemuDomainNVRAMPathGenerate(cfg, def) < 0)
> +return -1;
> +
> +VIR_DEBUG("decided on firmware '%s' varstore template '%s'",
> +  def->os.loader->path,
> +  def->os.loader->templt);
> +break;
> +
> +case QEMU_FIRMWARE_DEVICE_KERNEL:
> +VIR_FREE(def->os.kernel);
> +if (VIR_STRDUP(def->os.kernel, kernel->filename) < 0)
> +return -1;
> +
> +VIR_DEBUG("decided on kernel '%s'",
> +  def->os.kernel);
> +break;
> +
> +case QEMU_FIRMWARE_DEVICE_MEMORY:
> +if (!def->os.loader &&
> +VIR_ALLOC(def->os.loader) < 0)
> +return -1;
> +
> +def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
> +if (VIR_STRDUP(def->os.loader->path, memory->filename) < 0)
> +return -1;
> +
> +VIR_DEBUG("decided on loader '%s'",
> +  def->os.loader->path);
> +break;
> +
> +case QEMU_FIRMWARE_DEVICE_NONE:
> +case QEMU_FIRMWARE_DEVICE_LAST:
> +break;
> +}
> +
> +for (i = 0; i < fw->nfeatures; i++) {
> +switch (fw->features[i]) {
> +case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM:
> +switch (def->features[VIR_DOMAIN_FEATURE_SMM]) {
> +case VIR_TRISTATE_SWITCH_OFF:
> +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +   _("domain has SMM turned off "
> + "but chosen firmware requires it"));
> +return -1;
> +break;
> +case VIR_TRISTATE_SWITCH_ABSENT:
> +VIR_DEBUG("Enabling SMM feature");
> +def->features[VIR_DOMAIN_FEATURE_SMM] = 
> VIR_TRISTATE_SWITCH_ON;
> +break;
> +
> +case VIR_TRISTATE_SWITCH_ON:
> +case VIR_TRISTATE_SWITCH_LAST:
> +break;
> +}
> +break;

Right, this is good!

> +
> +case QEMU_FIRMWARE_FEATURE_NONE:
> +case QEMU_FIRMWARE_FEATURE_ACPI_S3:
> +case QEMU_FIRMWARE_FEATURE_ACPI_S4:
> +case QEMU_FIRMWARE_FEATURE_AMD_SEV:
> +case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS:
> +case QEMU_FIRMWARE_FEATURE_SECURE_BOOT:
> +case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC:
> +case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC:
> +case QEMU_FIRMWARE_FEATURE_LAST:
> +break;
> +}
> +}
> +
> +return 0;
> +}
> +
> +
> +static void
> +qemuFirmwareSanityCheck(const qemuFirmware *fw,
> +const char *filename)
> +{
> +    size_t i;
> +bool requiresSMM = false;
> +bool supportsSecureBoot = false;
> +
> +for (i = 0; i < fw->nfeatures; i++) {
> +switch (fw->features[i]) {
> +case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM:
> +requiresSMM = true;
> +break;
> +case QEMU_FIRMWARE_FEATURE_SECURE_BOOT:
> +supportsSecureBoot = true;
> +break;
> +case QEMU_FIRMWARE_FEATURE_NONE:
> +case QEMU_FIRMWARE_FEATURE_ACPI_S3:
> +case QEMU_FIRMWARE_FEATURE_ACPI_S4:
> +case QEMU_FIRMWARE_FEATURE_AMD_SEV:
> +case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS:
> +case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC:
> +case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC:
> +case QEMU_FIRMWARE_FEATURE_LAST:
> +break;
> +}
> +}
> +
> +if (supportsSecureBoot != requiresSMM) {
> +VIR_WARN("Firmware description '%s' has invalid set of features: "
> + "%s = %d, %s = %d",
> + filename,
> + 
> qemuFirmwareFeatureTypeToString(QEMU_FIRMWARE_FEATURE_REQUIRES_SMM),
> + requiresSMM,
> + 
> qemuFirmwareFeatureTypeToString(QEMU_FIRMWARE_FEATURE_SEC

Re: [libvirt] [PATCH v2 10/15] qemufirmwaretest: Test qemuFirmwareFetchConfigs()

[Laszlo Ersek]

(adding Dan)

On 03/07/19 10:29, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  tests/Makefile.am |  1 +
>  .../etc/qemu/firmware/40-ovmf-sb-keys.json|  1 +
>  .../etc/qemu/firmware/60-ovmf-sb.json |  0
>  .../user/.config/qemu/firmware/10-bios.json   |  0
>  .../share/qemu/firmware/40-bios.json} |  0
>  .../share/qemu/firmware/50-ovmf-sb-keys.json} |  0
>  .../share/qemu/firmware/60-ovmf-sb.json}  |  0
>  .../share/qemu/firmware/61-ovmf.json} |  0
>  .../share/qemu/firmware/70-aavmf.json}|  0
>  tests/qemufirmwaretest.c  | 72 +--
>  10 files changed, 68 insertions(+), 6 deletions(-)
>  create mode 12 
> tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json
>  create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json
>  create mode 100644 
> tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json
>  rename tests/qemufirmwaredata/{bios.json => 
> usr/share/qemu/firmware/40-bios.json} (100%)
>  rename tests/qemufirmwaredata/{ovmf-sb-keys.json => 
> usr/share/qemu/firmware/50-ovmf-sb-keys.json} (100%)
>  rename tests/qemufirmwaredata/{ovmf-sb.json => 
> usr/share/qemu/firmware/60-ovmf-sb.json} (100%)
>  rename tests/qemufirmwaredata/{ovmf.json => 
> usr/share/qemu/firmware/61-ovmf.json} (100%)
>  rename tests/qemufirmwaredata/{aavmf.json => 
> usr/share/qemu/firmware/70-aavmf.json} (100%)
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index b3449fa96b..b18b9e67ae 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -705,6 +705,7 @@ qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS)
>  qemufirmwaretest_SOURCES = \
>   qemufirmwaretest.c \
>   testutils.h testutils.c \
> + virfilewrapper.c virfilewrapper.h \
>   $(NULL)
>  qemufirmwaretest_LDADD = $(qemu_LDADDS) $(LDADDS)
>  
> diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json 
> b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json
> new file mode 12
> index 00..68e8cbbc2a
> --- /dev/null
> +++ b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json
> @@ -0,0 +1 @@
> +../../../usr/share/qemu/firmware/50-ovmf-sb-keys.json
> \ No newline at end of file
> diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json 
> b/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json
> new file mode 100644
> index 00..e69de29bb2
> diff --git 
> a/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json 
> b/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json
> new file mode 100644
> index 00..e69de29bb2
> diff --git a/tests/qemufirmwaredata/bios.json 
> b/tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json
> similarity index 100%
> rename from tests/qemufirmwaredata/bios.json
> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json
> diff --git a/tests/qemufirmwaredata/ovmf-sb-keys.json 
> b/tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json
> similarity index 100%
> rename from tests/qemufirmwaredata/ovmf-sb-keys.json
> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json
> diff --git a/tests/qemufirmwaredata/ovmf-sb.json 
> b/tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json
> similarity index 100%
> rename from tests/qemufirmwaredata/ovmf-sb.json
> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json
> diff --git a/tests/qemufirmwaredata/ovmf.json 
> b/tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json
> similarity index 100%
> rename from tests/qemufirmwaredata/ovmf.json
> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json
> diff --git a/tests/qemufirmwaredata/aavmf.json 
> b/tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json
> similarity index 100%
> rename from tests/qemufirmwaredata/aavmf.json
> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json
> diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c
> index 176cf0920d..cbf92f2689 100644
> --- a/tests/qemufirmwaretest.c
> +++ b/tests/qemufirmwaretest.c
> @@ -1,7 +1,9 @@
>  #include 
>  
>  #include "testutils.h"
> +#include "virfilewrapper.h"
>  #include "qemu/qemu_firmware.h"
> +#include "configmake.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
> @@ -50,11 +52,66 @@ testParseFormatFW(const void *opaque)
>  }
>  
>  
> +static int
> +testFWPrecedence(const void *opaque ATTRIBUTE_UNUSED)
> +{
> +VIR_AUTOFREE(char *) fakehome = NULL;
> +VIR_AUTOSTRINGLIST fwList = NULL;
> +size_t nfwList;
> +size_t i;
> +const char *expected[] = {
> +PREFIX "/share/qemu/firmware/40-bios.json",
> +SYSCONFDIR "/qemu/firmware/40-ovmf-sb-keys.json",
> +PREFIX "/share/qemu/firmware/50-ovmf-sb-keys.json",
> +PREFIX "/share/qemu/firmware/61-ovmf.json",
> +PREFIX "/share/qemu/firmware/70-aavmf.json",
> +

Re: [libvirt] [PATCH v2 09/15] qemu_firmware: Introduce qemuFirmwareFetchConfigs

[Laszlo Ersek]

On 03/07/19 13:46, Michal Privoznik wrote:
> On 3/7/19 12:55 PM, Daniel P. Berrangé wrote:
>> On Thu, Mar 07, 2019 at 10:29:19AM +0100, Michal Privoznik wrote:
>>> Implementation for yet another part of firmware description
>>> specification. This one covers selecting which files to parse.
>>>
>>> There are three locations from which description files can be
>>> loaded. In order of preference, from most generic to most
>>> specific these are:
>>>
>>>    /usr/share/qemu/firmware
>>>    /etc/qemu/firmware
>>>    $XDG_CONFIG_HOME/qemu/firmware
>>>
>>> If a file is found in two or more locations then the most specific
>>> one is used. Moreover, if file is empty then it means it is
>>> overriding some generic description and disabling it.
>>>
>>> Again, this is described in more details and with nice examples
>>> in firmware.json specification (qemu commit 3a0adfc9bf).
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>   src/qemu/qemu_firmware.c | 134 +++
>>>   src/qemu/qemu_firmware.h |   3 +
>>>   2 files changed, 137 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
>>> index 8f718ee2a6..a818f60c91 100644
>>> --- a/src/qemu/qemu_firmware.c
>>> +++ b/src/qemu/qemu_firmware.c
>>> @@ -21,9 +21,11 @@
>>>   #include 
>>>     #include "qemu_firmware.h"
>>> +#include "configmake.h"
>>>   #include "qemu_capabilities.h"
>>>   #include "virarch.h"
>>>   #include "virfile.h"
>>> +#include "virhash.h"
>>>   #include "virjson.h"
>>>   #include "virlog.h"
>>>   #include "virstring.h"
>>> @@ -899,3 +901,135 @@ qemuFirmwareFormat(qemuFirmwarePtr fw)
>>>     return virJSONValueToString(doc, true);
>>>   }
>>> +
>>> +
>>> +static int
>>> +qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir)
>>> +{
>>> +    DIR *dirp;
>>> +    struct dirent *ent = NULL;
>>> +    int rc;
>>> +    int ret = -1;
>>> +
>>> +    if ((rc = virDirOpenIfExists(, dir)) < 0)
>>> +    return -1;
>>> +
>>> +    if (rc == 0)
>>> +    return 0;
>>> +
>>> +    while ((rc = virDirRead(dirp, , dir)) > 0) {
>>> +    VIR_AUTOFREE(char *) filename = NULL;
>>> +    VIR_AUTOFREE(char *) path = NULL;
>>> +
>>> +    if (ent->d_type != DT_REG && ent->d_type != DT_LNK)
>>> +    continue;
>>> +
>>> +    if (STRPREFIX(ent->d_name, "."))
>>> +    continue;
>>> +
>>> +    if (VIR_STRDUP(filename, ent->d_name) < 0)
>>> +    goto cleanup;
>>> +
>>> +    if (virAsprintf(, "%s/%s", dir, filename) < 0)
>>> +    goto cleanup;
>>> +
>>> +    if (virHashUpdateEntry(files, filename, path) < 0)
>>> +    goto cleanup;
>>> +
>>> +    path = NULL;
>>> +    }
>>> +
>>> +    if (rc < 0)
>>> +    goto cleanup;
>>> +
>>> +    ret = 0;
>>> + cleanup:
>>> +    virDirClose();
>>> +    return ret;
>>> +}
>>> +
>>> +static int
>>> +qemuFirmwareFilesSorter(const virHashKeyValuePair *a,
>>> +    const virHashKeyValuePair *b)
>>> +{
>>> +    return strcmp(a->key, b->key);
>>> +}
>>> +
>>> +#define QEMU_FIRMWARE_SYSTEM_LOCATION PREFIX "/share/qemu/firmware"
>>> +#define QEMU_FIRMWARE_ETC_LOCATION SYSCONFDIR "/qemu/firmware"
>>> +
>>> +int
>>> +qemuFirmwareFetchConfigs(char ***firmwares)
>>> +{
>>> +    VIR_AUTOPTR(virHashTable) files = NULL;
>>> +    VIR_AUTOFREE(char *) homeConfig = NULL;
>>> +    VIR_AUTOFREE(char *) xdgConfig = NULL;
>>> +    VIR_AUTOFREE(virHashKeyValuePairPtr) pairs = NULL;
>>> +    virHashKeyValuePairPtr tmp = NULL;
>>> +
>>> +    *firmwares = NULL;
>>> +
>>> +    if (VIR_STRDUP(xdgConfig, virGetEnvBlockSUID("XDG_CONFIG_HOME"))
>>> < 0)
>>> +    return -1;
>>> +
>>> +    if (!xdgConfig) {
>>> +    VIR_AUTOFREE(char *) home = virGetUserDirectory();
>>> +
>>> +    if (!home)
>>> +    return -1;
>>> +
>>> +    if (virAsprintf(, "%s/.config", home) < 0)
>>> +    return -1;
>>> +    }
>>> +
>>> +    if (virAsprintf(, "%s/qemu/firmware", xdgConfig) < 0)
>>> +    return -1;
>>> +
>>> +    if (!(files = virHashCreate(10, virHashValueFree)))
>>> +    return -1;
>>> +
>>> +    if (qemuFirmwareBuildFileList(files,
>>> QEMU_FIRMWARE_SYSTEM_LOCATION) < 0)
>>> +    return -1;
>>> +
>>> +    if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_ETC_LOCATION)
>>> < 0)
>>> +    return -1;
>>> +
>>> +    if (qemuFirmwareBuildFileList(files, homeConfig) < 0)
>>> +    return -1;
>>
>> I wonder if it really makes sense to read /root/ for this. Normally we
>> would
>> only look at the home config for unprivileged daemon, eg we don't look in
>> /root for finding PKI x509 certs IIRC.
> 
> Fair enough. Root is able to put configs into /etc/qemu/firmware
> anyways. Will fix this locally for now.

Please carry forward my R-b, given up-thread, when you implement the
tweak suggested by Dan.

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 09/15] qemu_firmware: Introduce qemuFirmwareFetchConfigs

[Laszlo Ersek]

r)))
> +return -1;
> +
> +for (tmp = pairs; tmp->key; tmp++) {
> +const char *path = tmp->value;
> +off_t len;
> +
> +if ((len = virFileLength(path, -1)) < 0) {
> +virReportSystemError(errno,
> + _("unable to get size of '%s'"),
> + path);
> +return -1;
> +}
> +
> +VIR_DEBUG("firmware description path '%s' len=%jd",
> +  path, (intmax_t) len);
> +
> +if (len == 0) {
> +/* Empty files are used to mask less specific instances
> + * of the same file. */
> +continue;
> +}
> +
> +if (virStringListAdd(firmwares, path) < 0)
> +return -1;
> +}
> +
> +return 0;
> +}
> diff --git a/src/qemu/qemu_firmware.h b/src/qemu/qemu_firmware.h
> index 952615d42b..321169f56c 100644
> --- a/src/qemu/qemu_firmware.h
> +++ b/src/qemu/qemu_firmware.h
> @@ -37,4 +37,7 @@ qemuFirmwareParse(const char *path);
>  char *
>  qemuFirmwareFormat(qemuFirmwarePtr fw);
>  
> +int
> +qemuFirmwareFetchConfigs(char ***firmwares);
> +
>  #endif /* LIBVIRT_QEMU_FIRMWARE_H */
> 

Thanks for addressing my comments!

Reviewed-by: Laszlo Ersek 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 08/15] test: Introduce qemufirmwaretest

[Laszlo Ersek]

redata/bios.json
> new file mode 100644
> index 00..137ff70779
> --- /dev/null
> +++ b/tests/qemufirmwaredata/bios.json
> @@ -0,0 +1,35 @@
> +{
> +"description": "SeaBIOS",
> +"interface-types": [
> +"bios"
> +],
> +"mapping": {
> +"device": "memory",
> +"filename": "/usr/share/seabios/bios-256k.bin"
> +},
> +"targets": [
> +{
> +"architecture": "i386",
> +"machines": [
> +"pc-i440fx-*",
> +"pc-q35-*"
> +]
> +},
> +{
> +"architecture": "x86_64",
> +"machines": [
> +"pc-i440fx-*",
> +"pc-q35-*"
> +]
> +}
> +],
> +"features": [
> +"acpi-s3",
> +"acpi-s4"
> +],
> +"tags": [
> +"CONFIG_BOOTSPLASH=n",
> +"CONFIG_ROM_SIZE=256",
> +"CONFIG_USE_SMM=n"
> +]
> +}
> diff --git a/tests/qemufirmwaredata/ovmf-sb-keys.json 
> b/tests/qemufirmwaredata/ovmf-sb-keys.json
> new file mode 100644
> index 00..c804ac1038
> --- /dev/null
> +++ b/tests/qemufirmwaredata/ovmf-sb-keys.json
> @@ -0,0 +1,36 @@
> +{
> +"description": "OVMF with SB+SMM, SB enabled, MS certs enrolled",
> +"interface-types": [
> +"uefi"
> +],
> +"mapping": {
> +"device": "flash",
> +"executable": {
> +"filename": "/usr/share/OVMF/OVMF_CODE.secboot.fd",
> +"format": "raw"
> +},
> +"nvram-template": {
> +"filename": "/usr/share/OVMF/OVMF_VARS.secboot.fd",
> +"format": "raw"
> +}
> +},
> +"targets": [
> +{
> +"architecture": "x86_64",
> +"machines": [
> +"pc-q35-*"
> +]
> +}
> +],
> +"features": [
> +"acpi-s3",
> +"amd-sev",
> +"enrolled-keys",
> +"requires-smm",
> +"secure-boot",
> +"verbose-dynamic"
> +],
> +"tags": [
> +
> +]
> +}
> diff --git a/tests/qemufirmwaredata/ovmf-sb.json 
> b/tests/qemufirmwaredata/ovmf-sb.json
> new file mode 100644
> index 00..5e8a94ae78
> --- /dev/null
> +++ b/tests/qemufirmwaredata/ovmf-sb.json
> @@ -0,0 +1,35 @@
> +{
> +"description": "OVMF with SB+SMM, empty varstore",
> +"interface-types": [
> +"uefi"
> +],
> +"mapping": {
> +"device": "flash",
> +"executable": {
> +"filename": "/usr/share/OVMF/OVMF_CODE.secboot.fd",
> +"format": "raw"
> +},
> +"nvram-template": {
> +"filename": "/usr/share/OVMF/OVMF_VARS.fd",
> +"format": "raw"
> +}
> +    },
> +"targets": [
> +{
> +"architecture": "x86_64",
> +"machines": [
> +"pc-q35-*"
> +]
> +}
> +],
> +"features": [
> +"acpi-s3",
> +"amd-sev",
> +"requires-smm",
> +"secure-boot",
> +"verbose-dynamic"
> +],
> +"tags": [
> +
> +]
> +}
> diff --git a/tests/qemufirmwaredata/ovmf.json 
> b/tests/qemufirmwaredata/ovmf.json
> new file mode 100644
> index 00..9d53094778
> --- /dev/null
> +++ b/tests/qemufirmwaredata/ovmf.json
> @@ -0,0 +1,33 @@
> +{
> +"description": "OVMF with SB+SMM, empty varstore",

"SB", "SMM", and "empty varstore" are each either wrong or useless to
mention here.

(d) So I suggest retrofitting the AAVMF description instead -- just say
"UEFI firmware for x86_64 virtual machines".

> +"interface-types": [
> +"uefi"
> +],
> +"mapping&q

Re: [libvirt] [PATCH v2 04/15] virDomainLoaderDefParseXML: Allow loader path to be NULL

[Laszlo Ersek]

On 03/07/19 10:29, Michal Privoznik wrote:
> Except not really. At least for now.
> 
> In the future, the firmware will be selected automagically.
> Therefore, it makes no sense to require the pathname of a
> specific firmware binary in the domain XML. But since it is not
> implemented do not really allow the path to be NULL. Only move
> code around to prepare it for further expansion.
> 
> Signed-off-by: Michal Privoznik 
> Reviewed-by: Laszlo Ersek 
> ---
>  docs/schemas/domaincommon.rng |  4 +++-
>  src/conf/domain_conf.c| 29 +++--
>  2 files changed, 30 insertions(+), 3 deletions(-)

The commit message looks good. Thanks!
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 07/15] qemu: Introduce basic skeleton for parsing firmware description

[Laszlo Ersek]

On 03/05/19 15:38, Michal Privoznik wrote:
> On 2/28/19 10:31 AM, Laszlo Ersek wrote:
>> On 02/27/19 11:04, Michal Privoznik wrote:
>>> The firmware description is a JSON file which follows
>>> specification from qemu.git/docs/interop/firmware.json. The
>>> description file basically says: Firmware file X is {bios|uefi},
>>> supports these targets and machine types, requires these features
>>> to be enabled on qemu cmd line and this is how you put it onto
>>> qemu cmd line.
>>>
>>> The firmware.json specification covers more (i.e. how to select
>>> the right firmware) but that will be covered and implemented in
>>> next commits.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>   src/qemu/Makefile.inc.am |   2 +
>>>   src/qemu/qemu_firmware.c | 901 +++
>>>   src/qemu/qemu_firmware.h |  40 ++
>>>   3 files changed, 943 insertions(+)
>>>   create mode 100644 src/qemu/qemu_firmware.c
>>>   create mode 100644 src/qemu/qemu_firmware.h
>>
>> Wow. I assumed the parser for the schema could be auto-generated from
>> the schema JSON. Isn't that the case with the QMP commands and events?
> 
> Not really. We don't have any script to generate parser from qapi. We
> did not need it so far too as we have a lot of logic in our QMP command
> generation code.

Hmmm okay. If you generally interleave business logic and sanity checks
with QMP parsing (such as when processing events and command responses),
then I see the point. In the present case, the pattern is different
(first some larger-scale parsing, second heavy verification etc), but I
get the point of not adding a parser generator just for this.

Well, I'll let others review this patch then. :) Given the schema
definition, I think the patch can be verified regardless of the semantics.

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 08/15] test: Introduce qemufirmwaretest

[Laszlo Ersek]

On 03/05/19 15:38, Michal Privoznik wrote:
> On 2/28/19 10:42 AM, Laszlo Ersek wrote:
>> On 02/27/19 11:04, Michal Privoznik wrote:
>>> Test firmware description parsing so far.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>   tests/Makefile.am  |  9 
>>>   tests/qemufirmwaredata/40-bios.json    | 35 +
>>>   tests/qemufirmwaredata/50-ovmf-sb.json | 36 +
>>>   tests/qemufirmwaredata/60-ovmf.json    | 35 +
>>>   tests/qemufirmwaredata/70-aavmf.json   | 35 +
>>>   tests/qemufirmwaretest.c   | 70 ++
>>>   6 files changed, 220 insertions(+)
>>>   create mode 100644 tests/qemufirmwaredata/40-bios.json
>>>   create mode 100644 tests/qemufirmwaredata/50-ovmf-sb.json
>>>   create mode 100644 tests/qemufirmwaredata/60-ovmf.json
>>>   create mode 100644 tests/qemufirmwaredata/70-aavmf.json
>>>   create mode 100644 tests/qemufirmwaretest.c
>>
>> If the test data files added in this patch are from the examples in
>> QEMU's "docs/interop/firmware.json", I suggest stating so in the commit
>> message, also identifying the QEMU commit that added those examples.
> 
> It's a mixture. Some files (50-ovmf-sb.json and 60-ovmf.json) were taken
> from OVMF package from RHEL-7 and some were taken from firmware.json
> which has some examples in the comments. I'll put that into the commit
> message.
> 
>>
>> In addition, I wonder if the filenames should carry the double-digit
>> priority prefixes at once in this patch.
>>
>> Even if they do, I'm thinking that aavmf shouldn't be ordered (by
>> priority prefix) behind ovmf, given that the qemu targets / machine
>> types they are suitable for are mutually exclusive.
> 
> At this point, the code doesn't care about priority just yet. It is
> implemented in next patches. But I'm not sure I understand what you
> mean. You mean that 70-aavmf.json should be renamed to just 'aavmf.json'?

My first suggestion is to drop the prio prefixes (in this patch only).

If you prefer to keep them, then my second suggestion is to give the
same prio to the sole aavmf descriptor as to the main (highest prio)
ovmf descriptor.

> 
>>
>> Other than that, I have two superficial comments (questions) below,
>> because my knowledge of the libvirt test infrastructure is nonexistent
>> to minimal:
>>
> 
>>> diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c
>>> new file mode 100644
>>> index 00..0c5fb1e55a
>>> --- /dev/null
>>> +++ b/tests/qemufirmwaretest.c
>>> @@ -0,0 +1,70 @@
>>> +#include 
>>> +
>>> +#include "testutils.h"
>>> +#include "qemu/qemu_firmware.h"
>>> +
>>> +#define VIR_FROM_THIS VIR_FROM_QEMU
>>> +
>>> +static int
>>> +testParseFormatFW(const void *opaque)
>>> +{
>>> +    const char *filename = opaque;
>>> +    VIR_AUTOFREE(char *) path = NULL;
>>> +    VIR_AUTOPTR(qemuFirmware) fw = NULL;
>>> +    VIR_AUTOFREE(char *) buf = NULL;
>>> +    VIR_AUTOPTR(virJSONValue) json = NULL;
>>> +    VIR_AUTOFREE(char *) expected = NULL;
>>> +    VIR_AUTOFREE(char *) actual = NULL;
>>> +
>>> +    if (virAsprintf(, "%s/qemufirmwaredata/%s",
>>> +    abs_srcdir, filename) < 0)
>>> +    return -1;
>>> +
>>> +    if (!(fw = qemuFirmwareParse(path)))
>>> +    return -1;
>>> +
>>> +    if (virFileReadAll(path,
>>> +   1024 * 1024, /* 1MiB */
>>> +   ) < 0)
>>> +    return -1;
>>> +
>>> +    if (!(json = virJSONValueFromString(buf)))
>>> +    return -1;
>>> +
>>> +    /* Description and tags are not parsed. */
>>> +    if (virJSONValueObjectRemoveKey(json, "description", NULL) < 0 ||
>>> +    virJSONValueObjectRemoveKey(json, "tags", NULL) < 0)
>>> +    return -1;
>>> +
>>> +    if (!(expected = virJSONValueToString(json, true)))
>>> +    return -1;
>>> +
>>> +    if (!(actual = qemuFirmwareFormat(fw)))
>>> +    return -1;
>>> +
>>> +    return virTestCompareToString(expected, actual);
>>> +}
>>
>> Can you add a comment to the function about the general idea? Or is this
>> approach common for libvirt tests? I mean, to make heads or tails of
>> this function, I

Re: [libvirt] [PATCH v1 11/15] qemu_firmware: Introduce qemuFirmwareFillDomain()

[Laszlo Ersek]

On 03/05/19 15:38, Michal Privoznik wrote:
> On 2/28/19 12:15 PM, Laszlo Ersek wrote:
>> On 02/27/19 11:04, Michal Privoznik wrote:
>>> And finally the last missing piece. This is what puts it all
>>> together.
>>>
>>> At the beginning, qemuFirmwareFillDomain() loads all possible
>>> firmware description files based on algorithm described earlier.
>>> Then it tries to find description which matches given domain.
>>> The criteria are:
>>>
>>>    - firmware is the right type (e.g. it's bios when bios was
>>>  requested in domain XML)
>>>    - firmware is suitable for guest architecture/machine type
>>>    - firmware allows desired guest features to stay enabled (e.g.
>>>  if s3/s4 is enabled for guest then firmware has to support
>>>  it too)
>>>
>>> Once the desired description has been found it is then used to
>>> set various bits of virDomainDef so that proper qemu cmd line is
>>> constructed as demanded by the description file. For instance,
>>> secure boot enabled firmware might request SMM -> it will be
>>> enabled if needed.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>   src/qemu/qemu_firmware.c | 257 +++
>>>   src/qemu/qemu_firmware.h |   7 ++
>>>   2 files changed, 264 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
>>> index 509927b154..90c611db2d 100644
>>> --- a/src/qemu/qemu_firmware.c
>>> +++ b/src/qemu/qemu_firmware.c
>>> @@ -23,6 +23,8 @@
>>>   #include "qemu_firmware.h"
>>>   #include "configmake.h"
>>>   #include "qemu_capabilities.h"
>>> +#include "qemu_domain.h"
>>> +#include "qemu_process.h"
>>>   #include "virarch.h"
>>>   #include "virfile.h"
>>>   #include "virhash.h"
>>> @@ -1026,3 +1028,258 @@ qemuFirmwareFetchConfigs(char ***firmwares)
>>>     return 0;
>>>   }
>>> +
>>> +
>>> +static bool
>>> +qemuFirmwareMatchDomain(const virDomainDef *def,
>>> +    const qemuFirmware *fw)
>>> +{
>>> +    size_t i;
>>> +    bool supportsS3 = false;
>>> +    bool supportsS4 = false;
>>> +    bool supportsSecureBoot = false;
>>> +    bool supportsSEV = false;
>>> +
>>> +    for (i = 0; i < fw->ninterfaces; i++) {
>>> +    if ((def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS &&
>>> + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_BIOS) ||
>>> +    (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI &&
>>> + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_UEFI))
>>> +    break;
>>> +    }
>>> +
>>> +    if (i == fw->ninterfaces)
>>> +    return false;
>>> +
>>> +    for (i = 0; i < fw->ntargets; i++) {
>>> +    size_t j;
>>> +
>>> +    if (def->os.arch != fw->targets[i]->architecture)
>>> +    continue;
>>> +
>>> +    for (j = 0; j < fw->targets[i]->nmachines; j++) {
>>> +    if (virStringMatch(def->os.machine,
>>> fw->targets[i]->machines[j]))
>>> +    break;
>>> +    }
>>> +
>>> +    if (j == fw->targets[i]->nmachines)
>>> +    continue;
>>> +
>>> +    break;
>>> +    }
>>> +
>>> +    if (i == fw->ntargets)
>>> +    return false;
>>> +
>>> +    for (i = 0; i < fw->nfeatures; i++) {
>>> +    switch (fw->features[i]) {
>>> +    case QEMU_FIRMWARE_FEATURE_ACPI_S3:
>>> +    supportsS3 = true;
>>> +    break;
>>> +    case QEMU_FIRMWARE_FEATURE_ACPI_S4:
>>> +    supportsS4 = true;
>>> +    break;
>>> +    case QEMU_FIRMWARE_FEATURE_SECURE_BOOT:
>>> +    supportsSecureBoot = true;
>>> +    break;
>>> +    case QEMU_FIRMWARE_FEATURE_AMD_SEV:
>>> +    supportsSEV = true;
>>> +    break;
>>> +    case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS:
>>> +    case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM:
>>> +    case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC:
>>> +    case QEMU_FIRMWARE_FEATU

Re: [libvirt] [PATCH v1 13/15] qemuDomainDefValidate: Don't require SMM if automatic firmware selection enabled

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> The firmware selection code will enable the feature if needed.
> There's no need to require SMM to be enabled in that case.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index cc3a01397c..b25f26f8b0 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4157,7 +4157,9 @@ qemuDomainDefValidate(const virDomainDef *def,
>  goto cleanup;
>  }
>  
> -if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) 
> {
> +/* SMM will be enabled by qemuFirmwareFillDomain() if needed. */
> +if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE &&
> +def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) 
> {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Secure boot requires SMM feature enabled"));
>  goto cleanup;
> 

I'm not familiar with the ordering between the code in the previous
patch and the code in this patch. What is the order? Do we first
validate and then prepare/fill, or vice versa?

Also, what happens if the domain config explicitly disables SMM
emulation in , but the firmware requires it? See my point (2)
in message
.
Will qemuFirmwareEnableFeatures() simply overwrite the setting? I think
we should reject the firmware descriptor instead, in
qemuFirmwareMatchDomain().

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 11/15] qemu_firmware: Introduce qemuFirmwareFillDomain()

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> And finally the last missing piece. This is what puts it all
> together.
> 
> At the beginning, qemuFirmwareFillDomain() loads all possible
> firmware description files based on algorithm described earlier.
> Then it tries to find description which matches given domain.
> The criteria are:
> 
>   - firmware is the right type (e.g. it's bios when bios was
> requested in domain XML)
>   - firmware is suitable for guest architecture/machine type
>   - firmware allows desired guest features to stay enabled (e.g.
> if s3/s4 is enabled for guest then firmware has to support
> it too)
> 
> Once the desired description has been found it is then used to
> set various bits of virDomainDef so that proper qemu cmd line is
> constructed as demanded by the description file. For instance,
> secure boot enabled firmware might request SMM -> it will be
> enabled if needed.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_firmware.c | 257 +++
>  src/qemu/qemu_firmware.h |   7 ++
>  2 files changed, 264 insertions(+)
> 
> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
> index 509927b154..90c611db2d 100644
> --- a/src/qemu/qemu_firmware.c
> +++ b/src/qemu/qemu_firmware.c
> @@ -23,6 +23,8 @@
>  #include "qemu_firmware.h"
>  #include "configmake.h"
>  #include "qemu_capabilities.h"
> +#include "qemu_domain.h"
> +#include "qemu_process.h"
>  #include "virarch.h"
>  #include "virfile.h"
>  #include "virhash.h"
> @@ -1026,3 +1028,258 @@ qemuFirmwareFetchConfigs(char ***firmwares)
>  
>  return 0;
>  }
> +
> +
> +static bool
> +qemuFirmwareMatchDomain(const virDomainDef *def,
> +const qemuFirmware *fw)
> +{
> +size_t i;
> +bool supportsS3 = false;
> +bool supportsS4 = false;
> +bool supportsSecureBoot = false;
> +bool supportsSEV = false;
> +
> +for (i = 0; i < fw->ninterfaces; i++) {
> +if ((def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS &&
> + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_BIOS) ||
> +(def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI &&
> + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_UEFI))
> +break;
> +}
> +
> +if (i == fw->ninterfaces)
> +return false;
> +
> +for (i = 0; i < fw->ntargets; i++) {
> +size_t j;
> +
> +if (def->os.arch != fw->targets[i]->architecture)
> +continue;
> +
> +for (j = 0; j < fw->targets[i]->nmachines; j++) {
> +if (virStringMatch(def->os.machine, fw->targets[i]->machines[j]))
> +break;
> +}
> +
> +if (j == fw->targets[i]->nmachines)
> +continue;
> +
> +break;
> +}
> +
> +if (i == fw->ntargets)
> +return false;
> +
> +for (i = 0; i < fw->nfeatures; i++) {
> +switch (fw->features[i]) {
> +case QEMU_FIRMWARE_FEATURE_ACPI_S3:
> +supportsS3 = true;
> +break;
> +case QEMU_FIRMWARE_FEATURE_ACPI_S4:
> +supportsS4 = true;
> +break;
> +case QEMU_FIRMWARE_FEATURE_SECURE_BOOT:
> +supportsSecureBoot = true;
> +break;
> +case QEMU_FIRMWARE_FEATURE_AMD_SEV:
> +supportsSEV = true;
> +break;
> +case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS:
> +case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM:
> +case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC:
> +case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC:
> +case QEMU_FIRMWARE_FEATURE_NONE:
> +case QEMU_FIRMWARE_FEATURE_LAST:
> +break;
> +}
> +}
> +
> +if (def->pm.s3 == VIR_TRISTATE_BOOL_YES &&
> +!supportsS3)
> +return false;
> +
> +if (def->pm.s4 == VIR_TRISTATE_BOOL_YES &&
> +!supportsS4)
> +return false;
> +
> +if (def->os.loader &&
> +def->os.loader->secure == VIR_TRISTATE_BOOL_YES &&
> +!supportsSecureBoot)
> +return false;

This check doesn't seem correct. (Also the fact that
QEMU_FIRMWARE_FEATURE_REQUIRES_SMM is ignored above.)

The @secure attribute controls whether libvirtd generates the "-global
driver=cfi.pflash01,property=secure,value=on" cmdline option. See
qemuBuildDomainLoaderCommandLine(). That means that read/write accesses
("programming mode") to the pflash chips will be restricted to guest
code that runs in (guest) SMM.

In other words, @secure is connected to REQUIRES_SMM, not SECURE_BOOT.

Technically speaking, SECURE_BOOT is both independent of REQUIRES_SMM,
and irrelevant in itself for the QEMU command line. SECURE_BOOT is only
relevant to what firmware interfaces are exposed within the guest.

Now, security-wise, there *is* a connection between SECURE_BOOT and
REQUIRES_SMM. Namely, it is bad practice (for production uses) for
firmware to offer SECURE_BOOT without also specifying REQUIRES_SMM. See
the explanation in 

Re: [libvirt] [PATCH v1 10/15] qemufirmwaretest: Test qemuFirmwareFetchConfigs()

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  tests/Makefile.am |  1 +
>  .../etc/qemu/firmware/40-ovmf-sb.json |  1 +
>  .../etc/qemu/firmware/60-ovmf.json|  0
>  .../user/.config/qemu/firmware/10-bios.json   |  0
>  .../share/qemu/firmware}/40-bios.json |  0
>  .../share/qemu/firmware}/50-ovmf-sb.json  |  0
>  .../share/qemu/firmware}/60-ovmf.json |  0
>  .../share/qemu/firmware}/70-aavmf.json|  0
>  tests/qemufirmwaretest.c  | 69 +--
>  9 files changed, 66 insertions(+), 5 deletions(-)
>  create mode 12 tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb.json
>  create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf.json
>  create mode 100644 
> tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json
>  rename tests/qemufirmwaredata/{ => usr/share/qemu/firmware}/40-bios.json 
> (100%)
>  rename tests/qemufirmwaredata/{ => usr/share/qemu/firmware}/50-ovmf-sb.json 
> (100%)
>  rename tests/qemufirmwaredata/{ => usr/share/qemu/firmware}/60-ovmf.json 
> (100%)
>  rename tests/qemufirmwaredata/{ => usr/share/qemu/firmware}/70-aavmf.json 
> (100%)

Right, so I think it makes sense to introduce the double-digit priority
prefixes first in this patch, as part of the rename. They matter here,
but they don't matter in the previous test case.

No other comments for now regarding this patch; I hope that's not a problem.

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 09/15] qemu_firmware: Introduce qemuFirmwareFetchConfigs

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> Implementation for yet another part of firmware description
> specification. This one covers selecting which files to parse.
> 
> There are three locations from which description files can be
> loaded. In order of preference, from most generic to most
> specific these are:
> 
>   /usr/share/qemu/firmware
>   /etc/qemu/firmware
>   $XDG_CONFIG_HOME/qemu/firmware
> 
> If a file is find in two or more locations then the most specific

s/find/found/

> one is used. Moreover, if file is empty then it means it is
> overriding some generic description and disabling it.
> 
> Again, this is described in more details and with nice examples
> in firmware.json specification.

Please add a QEMU commit reference here as well.

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_firmware.c | 127 +++
>  src/qemu/qemu_firmware.h |   3 +
>  2 files changed, 130 insertions(+)
> 
> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
> index e709a5f608..509927b154 100644
> --- a/src/qemu/qemu_firmware.c
> +++ b/src/qemu/qemu_firmware.c
> @@ -21,9 +21,11 @@
>  #include 
>  
>  #include "qemu_firmware.h"
> +#include "configmake.h"
>  #include "qemu_capabilities.h"
>  #include "virarch.h"
>  #include "virfile.h"
> +#include "virhash.h"
>  #include "virjson.h"
>  #include "virlog.h"
>  #include "virstring.h"
> @@ -899,3 +901,128 @@ qemuFirmwareFormat(qemuFirmwarePtr fw)
>  
>  return virJSONValueToString(doc, true);
>  }
> +
> +
> +static int
> +qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir)
> +{
> +DIR *dirp;
> +struct dirent *ent = NULL;
> +int rc;
> +int ret = -1;
> +
> +if ((rc = virDirOpenIfExists(, dir)) < 0)
> +return -1;
> +
> +if (rc == 0)
> +return 0;
> +
> +while ((rc = virDirRead(dirp, , dir)) > 0) {
> +VIR_AUTOFREE(char *) filename = NULL;
> +VIR_AUTOFREE(char *) path = NULL;

(VIR_AUTOFREE() is very unusual to me, so I could be missing lifecycle
bugs.)

> +
> +if (ent->d_type != DT_REG && ent->d_type != DT_LNK)
> +continue;
> +
> +if (STRPREFIX(ent->d_name, "."))
> +continue;
> +
> +if (VIR_STRDUP(filename, ent->d_name) < 0)
> +goto cleanup;
> +
> +if (virAsprintf(, "%s/%s", dir, filename) < 0)
> +goto cleanup;
> +
> +if (virHashUpdateEntry(files, filename, path) < 0)
> +goto cleanup;
> +
> +path = NULL;
> +}
> +
> +if (rc < 0)
> +goto cleanup;
> +
> +ret = 0;
> + cleanup:
> +virDirClose();
> +return ret;
> +}
> +
> +static int
> +qemuFirmwareFilesSorter(const virHashKeyValuePair *a,
> +const virHashKeyValuePair *b)
> +{
> +return strcmp(a->key, b->key);
> +}
> +
> +#define QEMU_FIRMWARE_SYSTEM_LOCATION PREFIX "/share/qemu/firmware"
> +#define QEMU_FIRMWARE_ETC_LOCATION SYSCONFDIR "/qemu/firmware"

You could factor out "qemu/firmware" to a separate macro; you use it
three times in total.

> +
> +int
> +qemuFirmwareFetchConfigs(char ***firmwares)

Apparently, (char **) is a well known type in libvirt (a
"virStringList"). Shouldn't we have an actual typedef for that? It is
surprising not to have one, given the other verbose typedefs for
example, which basically just say "this is a pointer".

I'm also missing some invariants on this type. For example, an empty
string list could be represented by a 1-element array that is
immediately terminated with a NULL. But on failure this function doesn't
produce that, instead it sets *firmwares to NULL (no array at all).

Not saying this is wrong, just hard to understand, without docs / typedef.

> +{
> +VIR_AUTOPTR(virHashTable) files = NULL;
> +VIR_AUTOFREE(char *) homeConfig = NULL;
> +VIR_AUTOFREE(char *) xdgConfig = NULL;
> +VIR_AUTOFREE(virHashKeyValuePairPtr) pairs = NULL;
> +virHashKeyValuePairPtr tmp = NULL;
> +
> +*firmwares = NULL;
> +
> +if (VIR_STRDUP(xdgConfig, virGetEnvBlockSUID("XDG_CONFIG_HOME")) < 0)
> +return -1;
> +
> +if (!xdgConfig) {
> +VIR_AUTOFREE(char *) home = virGetUserDirectory();
> +
> +if (!home)
> +return -1;
> +
> +if (virAsprintf(, "%s/.config", home) < 0)
> +return -1;
> +}
> +
> +if (virAsprintf(, "%s/qemu/firmware", xdgConfig) < 0)
> +return -1;
> +
> +if (!(files = virHashCreate(10, virHashValueFree)))
> +return -1;
> +
> +if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_SYSTEM_LOCATION) < 0)
> +return -1;
> +
> +if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_ETC_LOCATION) < 0)
> +return -1;
> +
> +if (qemuFirmwareBuildFileList(files, homeConfig) < 0)
> +return -1;

OK, so at this point we have a unique set of filenames (one component
only) where each filename (as key) has the highest priority full
pathname associated with it. I'd add a comment 

Re: [libvirt] [PATCH v1 08/15] test: Introduce qemufirmwaretest

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> Test firmware description parsing so far.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tests/Makefile.am  |  9 
>  tests/qemufirmwaredata/40-bios.json| 35 +
>  tests/qemufirmwaredata/50-ovmf-sb.json | 36 +
>  tests/qemufirmwaredata/60-ovmf.json| 35 +
>  tests/qemufirmwaredata/70-aavmf.json   | 35 +
>  tests/qemufirmwaretest.c   | 70 ++
>  6 files changed, 220 insertions(+)
>  create mode 100644 tests/qemufirmwaredata/40-bios.json
>  create mode 100644 tests/qemufirmwaredata/50-ovmf-sb.json
>  create mode 100644 tests/qemufirmwaredata/60-ovmf.json
>  create mode 100644 tests/qemufirmwaredata/70-aavmf.json
>  create mode 100644 tests/qemufirmwaretest.c

If the test data files added in this patch are from the examples in
QEMU's "docs/interop/firmware.json", I suggest stating so in the commit
message, also identifying the QEMU commit that added those examples.

In addition, I wonder if the filenames should carry the double-digit
priority prefixes at once in this patch.

Even if they do, I'm thinking that aavmf shouldn't be ordered (by
priority prefix) behind ovmf, given that the qemu targets / machine
types they are suitable for are mutually exclusive.

Other than that, I have two superficial comments (questions) below,
because my knowledge of the libvirt test infrastructure is nonexistent
to minimal:

> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index c3f633cee0..d23a8c2812 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -132,6 +132,7 @@ EXTRA_DIST = \
>   qemuxml2xmloutdata \
>   qemustatusxml2xmldata \
>   qemumemlockdata \
> + qemufirmwaredata \
>   secretxml2xmlin \
>   securityselinuxhelperdata \
>   securityselinuxlabeldata \
> @@ -291,6 +292,7 @@ test_programs += qemuxml2argvtest qemuxml2xmltest \
>   qemublocktest \
>   qemumigparamstest \
>   qemusecuritytest \
> + qemufirmwaretest \
>   $(NULL)
>  test_helpers += qemucapsprobe
>  test_libraries += libqemumonitortestutils.la \
> @@ -698,6 +700,12 @@ qemusecuritytest_SOURCES = \
>   testutilsqemu.h testutilsqemu.c
>  qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS)
>  
> +qemufirmwaretest_SOURCES = \
> + qemufirmwaretest.c \
> + testutils.h testutils.c \
> + $(NULL)
> +qemufirmwaretest_LDADD = $(qemu_LDADDS) $(LDADDS)
> +
>  else ! WITH_QEMU
>  EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \
>   domainsnapshotxml2xmltest.c \
> @@ -711,6 +719,7 @@ EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c 
> qemuargv2xmltest.c \
>   qemumigparamstest.c \
>   qemusecuritytest.c qemusecuritytest.h \
>   qemusecuritymock.c \
> + qemufirmwaretest.c \
>   $(QEMUMONITORTESTUTILS_SOURCES)
>  endif ! WITH_QEMU
>  
> diff --git a/tests/qemufirmwaredata/40-bios.json 
> b/tests/qemufirmwaredata/40-bios.json
> new file mode 100644
> index 00..137ff70779
> --- /dev/null
> +++ b/tests/qemufirmwaredata/40-bios.json
> @@ -0,0 +1,35 @@
> +{
> +"description": "SeaBIOS",
> +"interface-types": [
> +"bios"
> +],
> +"mapping": {
> +"device": "memory",
> +"filename": "/usr/share/seabios/bios-256k.bin"
> +},
> +"targets": [
> +{
> +"architecture": "i386",
> +"machines": [
> +"pc-i440fx-*",
> +"pc-q35-*"
> +]
> +},
> +{
> +"architecture": "x86_64",
> +"machines": [
> +"pc-i440fx-*",
> +"pc-q35-*"
> +]
> +}
> +],
> +"features": [
> +"acpi-s3",
> +"acpi-s4"
> +],
> +"tags": [
> +"CONFIG_BOOTSPLASH=n",
> +"CONFIG_ROM_SIZE=256",
> +"CONFIG_USE_SMM=n"
> +]
> +}
> diff --git a/tests/qemufirmwaredata/50-ovmf-sb.json 
> b/tests/qemufirmwaredata/50-ovmf-sb.json
> new file mode 100644
> index 00..c804ac1038
> --- /dev/null
> +++ b/tests/qemufirmwaredata/50-ovmf-sb.json
> @@ -0,0 +1,36 @@
> +{
> +"description": "OVMF with SB+SMM, SB enabled, MS certs enrolled",
> +"interface-types": [
> +"uefi"
> +],
> +"mapping": {
> +"device": "flash",
> +"executable": {
> +"filename": "/usr/share/OVMF/OVMF_CODE.secboot.fd",
> +"format": "raw"
> +},
> +"nvram-template": {
> +"filename": "/usr/share/OVMF/OVMF_VARS.secboot.fd",
> +"format": "raw"
> +}
> +},
> +"targets": [
> +{
> +"architecture": "x86_64",
> +"machines": [
> +"pc-q35-*"
> +]
> +}
> +],
> +"features": [
> +"acpi-s3",
> +"amd-sev",
> +"enrolled-keys",
> +"requires-smm",
> +"secure-boot",
> +"verbose-dynamic"
> +],
> + 

Re: [libvirt] [PATCH v1 07/15] qemu: Introduce basic skeleton for parsing firmware description

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> The firmware description is a JSON file which follows
> specification from qemu.git/docs/interop/firmware.json. The
> description file basically says: Firmware file X is {bios|uefi},
> supports these targets and machine types, requires these features
> to be enabled on qemu cmd line and this is how you put it onto
> qemu cmd line.
> 
> The firmware.json specification covers more (i.e. how to select
> the right firmware) but that will be covered and implemented in
> next commits.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/Makefile.inc.am |   2 +
>  src/qemu/qemu_firmware.c | 901 +++
>  src/qemu/qemu_firmware.h |  40 ++
>  3 files changed, 943 insertions(+)
>  create mode 100644 src/qemu/qemu_firmware.c
>  create mode 100644 src/qemu/qemu_firmware.h

Wow. I assumed the parser for the schema could be auto-generated from
the schema JSON. Isn't that the case with the QMP commands and events?

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 06/15] conf: Introduce firmware attribute to

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> The idea is that using this attribute users enable libvirt to
> automagically select firmware image for their domain. For
> instance:
> 
>   
> hvm
> 
>   
> 
>   
> hvm
>   
> 
> (The automagic of selecting firmware image will be described in
> later commits.)
> 
> Accepted values are 'bios' and 'efi' to let libvirt select
> corresponding type of firmware.
> 
> I know it is a good sign to introduce xml2xml test case when
> changing XML config parser but that will have to come later.
> Firmware auto selection is not enabled for any driver just yet so
> any xml2xml test would fail right away.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  docs/formatdomain.html.in | 22 +-
>  docs/schemas/domaincommon.rng |  8 
>  src/conf/domain_conf.c| 75 ---
>  src/conf/domain_conf.h| 12 ++
>  src/libvirt_private.syms  |  2 +
>  5 files changed, 103 insertions(+), 16 deletions(-)

I prefer to leave the review of this patch to others; again because I'm
unsure if this is the desired syntax in the domain XML.

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 05/15] conf: Introduce VIR_DOMAIN_LOADER_TYPE_NONE

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> This is going to extend virDomainLoader enum. The reason is that
> once loader path is NULL its type makes no sense. However, since
> value of zero corresponds to VIR_DOMAIN_LOADER_TYPE_ROM the
> following XML would be produced:
> 
>   
> 
> ...
>   
> 
> To solve this, introduce VIR_DOMAIN_LOADER_TYPE_NONE which would
> correspond to value of zero and then use post parse callback to
> set the default loader type to 'rom' if needed.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/domain_conf.c  | 23 +--
>  src/conf/domain_conf.h  |  3 ++-
>  src/qemu/qemu_command.c |  1 +
>  src/qemu/qemu_domain.c  |  1 +
>  tests/domaincapsschemadata/full.xml |  1 +
>  5 files changed, 26 insertions(+), 3 deletions(-)

Sounds pretty complex, but I guess it makes sense. If both @type and the
pathname content are missing, then @type will default to NONE. (This
used not to be possible, given that pathname used to be required.) If
@type is absent but the pathname is present, then we flip the type
manually to ROM.

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f622a4dddf..b436b91c66 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1013,6 +1013,7 @@ VIR_ENUM_IMPL(virDomainMemoryAllocation, 
> VIR_DOMAIN_MEMORY_ALLOCATION_LAST,
>  
>  VIR_ENUM_IMPL(virDomainLoader,
>VIR_DOMAIN_LOADER_TYPE_LAST,
> +  "none",
>"rom",
>"pflash",
>  );
> @@ -4286,6 +4287,20 @@ virDomainDefPostParseMemory(virDomainDefPtr def,
>  }
>  
>  
> +static void
> +virDomainDefPostParseOs(virDomainDefPtr def)
> +{
> +if (!def->os.loader)
> +return;
> +
> +if (def->os.loader->path &&
> +def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_NONE) {
> +/* By default, loader is type of 'rom' */
> +def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
> +}
> +}
> +
> +
>  static void
>  virDomainDefPostParseMemtune(virDomainDefPtr def)
>  {
> @@ -5465,6 +5480,8 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
>  if (virDomainDefPostParseMemory(def, data->parseFlags) < 0)
>  return -1;
>  
> +virDomainDefPostParseOs(def);
> +
>  virDomainDefPostParseMemtune(def);
>  
>  if (virDomainDefRejectDuplicateControllers(def) < 0)
> @@ -18706,7 +18723,7 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>  
>  if (type_str) {
>  int type;
> -if ((type = virDomainLoaderTypeFromString(type_str)) < 0) {
> +if ((type = virDomainLoaderTypeFromString(type_str)) <= 0) {

Why is this change necessary? Hm... I assume, due to the enum
auto-generation in libvirt, the introduction of "none" will
automatically cause virDomainLoaderTypeFromString() to recognize "none"
as value 0. But we want to act like "none" isn't a valid value (it's
internal use only). Hence the same error message as before.

I reckon this is OK.

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo

>  virReportError(VIR_ERR_XML_DETAIL,
> _("unknown type value: %s"), type_str);
>  goto cleanup;
> @@ -27611,12 +27628,14 @@ virDomainLoaderDefFormat(virBufferPtr buf,
>  if (loader->secure)
>  virBufferAsprintf(buf, " secure='%s'", secure);
>  
> -virBufferAsprintf(buf, " type='%s'", type);
> +if (loader->type != VIR_DOMAIN_LOADER_TYPE_NONE)
> +virBufferAsprintf(buf, " type='%s'", type);
>  
>  if (loader->path)
>  virBufferEscapeString(buf, ">%s\n", loader->path);
>  else
>  virBufferAddLit(buf, "/>\n");
> +
>  if (loader->nvram || loader->templt) {
>  virBufferAddLit(buf, "  virBufferEscapeString(buf, " template='%s'", loader->templt);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 1f8454b38c..4e8c02b5e3 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1899,7 +1899,8 @@ struct _virDomainBIOSDef {
>  };
>  
>  typedef enum {
> -VIR_DOMAIN_LOADER_TYPE_ROM = 0,
> +VIR_DOMAIN_LOADER_TYPE_NONE = 0,
> +VIR_DOMAIN_LOADER_TYPE_ROM,
>  VIR_DOMAIN_LOADER_TYPE_PFLASH,
>  
>  VIR_DOMAIN_LOADER_TYPE_LAST
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 74f34af292..92729485ff 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9846,6 +9846,7 @@ qemuBuildDom

Re: [libvirt] [PATCH v1 04/15] virDomainLoaderDefParseXML: Allow loader path to be NULL

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> Except not really. At least for now.
> 
> In the future, the firmware will be selected automagically.
> Therefore, it makes no sense to require path to one in the domain

I suggest replacing "path to one" with "the pathname of a specific
firmware binary". "One" in the above context is meaningful, but it takes
some mental gymnastics to resolve.

> XML. But since it is not implemented do not really allow the path
> to be NULL. Only move code around to prepare it for further
> expansion.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  docs/schemas/domaincommon.rng |  4 +++-
>  src/conf/domain_conf.c| 29 +++--
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 5345e54342..bebe39de76 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -283,7 +283,9 @@
>  
>
>  
> -
> +
> +  
> +
>
>  
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 477deb777e..f622a4dddf 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6564,6 +6564,22 @@ virDomainDefMemtuneValidate(const virDomainDef *def)
>  }
>  
>  
> +static int
> +virDomainDefOSValidate(const virDomainDef *def)
> +{
> +if (!def->os.loader)
> +return 0;
> +
> +if (!def->os.loader->path) {
> +virReportError(VIR_ERR_XML_DETAIL, "%s",
> +   _("no loader path specified"));
> +return -1;
> +}
> +
> +return 0;
> +}
> +
> +
>  static int
>  virDomainDefValidateInternal(const virDomainDef *def)
>  {
> @@ -6602,6 +6618,9 @@ virDomainDefValidateInternal(const virDomainDef *def)
>  if (virDomainDefMemtuneValidate(def) < 0)
>  return -1;
>  
> +if (virDomainDefOSValidate(def) < 0)
> +return -1;
> +
>  return 0;
>  }
>  
> @@ -18668,6 +18687,9 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>  type_str = virXMLPropString(node, "type");
>  loader->path = (char *) xmlNodeGetContent(node);
>  
> +if (STREQ_NULLABLE(loader->path, ""))
> +VIR_FREE(loader->path);
> +
>  if (readonly_str &&
>  (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 
> 0) {
>  virReportError(VIR_ERR_XML_DETAIL,
> @@ -27589,9 +27611,12 @@ virDomainLoaderDefFormat(virBufferPtr buf,
>  if (loader->secure)
>  virBufferAsprintf(buf, " secure='%s'", secure);
>  
> -virBufferAsprintf(buf, " type='%s'>", type);
> +virBufferAsprintf(buf, " type='%s'", type);
>  
> -virBufferEscapeString(buf, "%s\n", loader->path);
> +if (loader->path)
> +virBufferEscapeString(buf, ">%s\n", loader->path);
> +else
> +virBufferAddLit(buf, "/>\n");
>  if (loader->nvram || loader->templt) {
>  virBufferAddLit(buf, "  virBufferEscapeString(buf, " template='%s'", loader->templt);
> 

I think this patch does what it says on the tin, but I'm not sure myself
if "what it says on the tin" is the right thing to do. (In other words,
whether the syntax proposed in the blurb is indeed our end-goal.) I
don't doubt it, I just don't have an opinion. So I'll defer to Dan on
this patch. Technically, it looks OK to me.

With the commit message update:

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 03/15] qemu_capabilities: Expose qemu <-> libvirt arch translators

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> In some cases, the string representing architecture is different
> in qemu and libvirt. That is the reason why we have
> virQEMUCapsArchFromString() and virQEMUCapsArchToString(). So
> far, we did not need them outside of qemu_capabilities code, but
> this will change shortly. Expose them then.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_capabilities.c | 4 ++--
>  src/qemu/qemu_capabilities.h | 3 +++
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index b48bcbebee..32e7a975a2 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -615,7 +615,7 @@ static int virQEMUCapsOnceInit(void)
>  
>  VIR_ONCE_GLOBAL_INIT(virQEMUCaps);
>  
> -static virArch virQEMUCapsArchFromString(const char *arch)
> +virArch virQEMUCapsArchFromString(const char *arch)
>  {
>  if (STREQ(arch, "i386"))
>  return VIR_ARCH_I686;
> @@ -628,7 +628,7 @@ static virArch virQEMUCapsArchFromString(const char *arch)
>  }
>  
>  
> -static const char *virQEMUCapsArchToString(virArch arch)
> +const char *virQEMUCapsArchToString(virArch arch)
>  {
>  if (arch == VIR_ARCH_I686)
>  return "i386";
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index ba84052bca..eb0fa5f3c0 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -642,4 +642,7 @@ bool virQEMUCapsCPUFilterFeatures(const char *name,
>  virSEVCapabilityPtr
>  virQEMUCapsGetSEVCapabilities(virQEMUCapsPtr qemuCaps);
>  
> +virArch virQEMUCapsArchFromString(const char *arch);
> +const char *virQEMUCapsArchToString(virArch arch);
> +
>  #endif /* LIBVIRT_QEMU_CAPABILITIES_H */
> 

Reviewed-by: Laszlo Ersek 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 02/15] qemu_domain: Separate NVRAM VAR store file name generation

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> Move the code that (possibly) generates filename of NVRAM VAR
> store into a single function so that it can be re-used later.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 26 ++
>  src/qemu/qemu_domain.h |  4 
>  2 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 59fe1eb401..cf7e650b34 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3876,14 +3876,8 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>  goto cleanup;
>  }
>  
> -if (def->os.loader &&
> -def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
> -def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
> -!def->os.loader->nvram) {
> -if (virAsprintf(>os.loader->nvram, "%s/%s_VARS.fd",
> -cfg->nvramDir, def->name) < 0)
> -goto cleanup;
> -}
> +if (qemuDomainNVRAMPathGenerate(cfg, def) < 0)
> +goto cleanup;
>  
>  if (qemuDomainDefAddDefaultDevices(def, qemuCaps) < 0)
>  goto cleanup;
> @@ -13944,3 +13938,19 @@ 
> qemuDomainDiskIsMissingLocalOptional(virDomainDiskDefPtr disk)
> virStorageSourceIsLocalStorage(disk->src) && disk->src->path &&
> !virFileExists(disk->src->path);
>  }
> +
> +
> +int
> +qemuDomainNVRAMPathGenerate(virQEMUDriverConfigPtr cfg,
> +virDomainDefPtr def)
> +{
> +if (def->os.loader &&
> +def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
> +def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
> +!def->os.loader->nvram) {
> +return virAsprintf(>os.loader->nvram, "%s/%s_VARS.fd",
> +   cfg->nvramDir, def->name);
> +}
> +
> +return 0;
> +}
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 7c6b50184c..136a7a7c72 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -1107,4 +1107,8 @@ qemuDomainIsUsingNoShutdown(qemuDomainObjPrivatePtr 
> priv);
>  bool
>  qemuDomainDiskIsMissingLocalOptional(virDomainDiskDefPtr disk);
>  
> +int
> +qemuDomainNVRAMPathGenerate(virQEMUDriverConfigPtr cfg,
> +virDomainDefPtr def);
> +
>  #endif /* LIBVIRT_QEMU_DOMAIN_H */
> 

I'm not familiar with restrictions on helper functions (e.g. if they are
supposed to sanity check input params for null pointers etc), but as a
normal code extraction patch, this looks OK to me.

Also presumably the other call will arrive from a different C file,
hence the external linkage and header file change.

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Configuring pflash devices for OVMF firmware

[Laszlo Ersek]

Hi Markus,

On 02/07/19 10:30, Markus Armbruster wrote:
> The thread got long, let me try to summarize, and elaborate a few
> points.
> 
> * The problem at hand is configuring firmware residing in flash memory
>   (OVMF requires this) without legacy -drive.
> 
> * The wider problem is configuring onboard devices.  Our general device
>   configuration interface doesn't cover them.  Instead, we have a zoo of
>   ad hoc interfaces that are much more limited.  Some of them we'd
>   rather deprecate (-drive, -net), but can't until we have a suitable
>   replacements.
> 
>   I think a board should be a composite object that exposes properties
>   of its own and its parts, just like other composite devices, so that
>   "create, set properties, realize" just works.  That would extend our
>   common device configuration mechanism naturally to onboard devices.
> 
>   A PC board's flash memory device would be just another part.  It could
>   be something like /machine/q35/cfi.pflash01/ in the QOM tree.  To
>   configure it, you'd set its properties, such as
>   /machine/q35/cfi.pflash01/drive.
> 
>   Note that this requires a way to set an existing device's properties.
>   Perhaps qom-set already works.
> 
> * While I do believe we should tackle the wider problem, I'd rather not
>   sit on the narrow problem until we crack it.  So, what can we do about
>   it?
> 
>   - Paolo proposed to add block backend properties to the PC machine,
> settable like -machine pflash0=BLOCK-BACKEND.
> 
> Possible drawback: if we add /machine/q35/pflash0 to the QOM tree
> now, and later replace it by /machine/q35/cfi.pflash01/drive, we'll
> have to deal with yet another machine type variation.  We'll live.
> 
>   - I proposed to sidestep our onboard device configuration problem by
> adding the cfi.pflash01 devices with our existing general device
> configuration interface: -device.  Possible since the onboard
> cfi.pflash01 devices are optional.  Requires a small extension to
> the firmware descriptors, and a bit of extra work in libvirt to
> process that extension.  I think it's workable, but Paolo's idea is
> simpler.
> 
>   I can give Paolo's idea a try.  Objections?
> 
> * A flash device supporting multiple regions is desirable, because it's
>   what physical hardware has.  We currently use multiple flash devices
>   instead.  We'll be stuck with them for existing machine types due to
>   guest ABI and migration compatibility.
> 
> * cfi.pflash01 currently requires users to opt out of "bad, do not use".
>   It should require opt in, to guard against accidental new uses of
>   "bad".
> 
> 
> PS: Big thanks to László, whose patient guidance helped me map this part
> of the jungle.
> 

I've read the above carefully.

At the QEMU design level, I don't have any opinion or preference; there
I simply don't know enough -- and don't suffer from bad decisions enough
-- to make sensible comments.

Regarding the choice betwen "-machine pflash0=BLOCK-BACKEND" and
"-device pflash": I don't object to exploring the former first.

I'd just like to note that "-machine pflash0=BLOCK-BACKEND" will also
require changes to the firmware descriptor schema. Not to the types that
the schema defines -- and therefore concrete descriptor *documents* that
already conform to the schema wouldn't be affected --, but to the
documentation that the schema directs at management applications.

The schema is supposed to specify (in the documentation) QEMU command
line options for management applications. If we add "-machine
pflash0=BLOCK-BACKEND", then even if the types in the schema stay the
same, some mappings to the QEMU cmdline will have to be re-documented.

Of course, that's still easier / less intrusive than changing the types!
... Which does make me prefer "-machine pflash0=BLOCK-BACKEND", if I'm
being honest.

(I hope my followup isn't totally useless. I certainly didn't want to
ignore your summary.)

Thanks,
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] Configuring pflash devices for OVMF firmware

[Laszlo Ersek]

On 01/31/19 10:37, Markus Armbruster wrote:
> Paolo Bonzini  writes:
> 
>> On 31/01/19 09:33, Markus Armbruster wrote:
>>> I thought secure=on affected only writes (and so wouldn't matter with
>>> readonly=on), but I was wrong:
>>>
>>> static MemTxResult pflash_mem_read_with_attrs(void *opaque, hwaddr 
>>> addr, uint64_t *value,
>>>   unsigned len, MemTxAttrs 
>>> attrs)
>>> {
>>> pflash_t *pfl = opaque;
>>> bool be = !!(pfl->features & (1 << PFLASH_BE));
>>>
>>> if ((pfl->features & (1 << PFLASH_SECURE)) && !attrs.secure) {
>>> *value = pflash_data_read(opaque, addr, len, be);
>>> } else {
>>> *value = pflash_read(opaque, addr, len, be);
>>> }
>>> return MEMTX_OK;
>>> }
>>>
>>> pflash_data_read() is what pflash_read() does when pfl->cmd is 0.
>>
>> Reads from flash actually do not go through here; this function executes
>> if the flash chip is already in MMIO mode, which happens after you
>> *write* a command to the memory area.  With secure=on, you just cannot
>> do a command write unless you're in SMM, in other words the flash chip
>> can only ever go in MMIO mode if you're in SMM.
>>
>>> Hmm, why is it okay to treat all pfl->cmd values the same when
>>> secure=on?
>>
>> But doesn't matter.  You just don't want MMIO mode to be active outside
>> SMM: all that non-SMM code want to do with the flash is read and execute
>> it, as far as they're concerned it's just ROM and the command mode is
>> nonexistent.
> 
> Out of curiosity: what effect does secure=on have when the device is
> read-only (pflash_t member ro non-zero)?
> 

It's hard to theorize about this comprehensively. What action and which
pflash unit do you have in mind?

(Interpreting your question as "what does the firmware see if...".
Another interpretation would be possible too, "what does QEMU do if...".)

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] Configuring pflash devices for OVMF firmware

[Laszlo Ersek]

On 01/31/19 10:19, Paolo Bonzini wrote:
> On 31/01/19 09:40, Markus Armbruster wrote:
>>> Maybe we should just add pflash block properties to the machine?  And
>>> then it can create the devices if the properties are set to a non-empty
>>> value.
>> What exactly do you have in mind?  Something like
>>
>> -machine q35,ovmf-code=OVMF-CODE-NODE,ovmf-data=OVMF-DATA-NODE
>>
>> where OVMF-CODE-NODE and OVMF-DATA-NODE are block backend node names,
>> i.e.
>>
>> -blockdev 
>> file,node-name=OVMF-CODE-NODE,read-only=on,filename=/usr/share/edk2/ovmf/OVMF_CODE.fd
>> -blockdev file,node-name=OVMF-DATA-NODE,read-only=on,filename=...
> 
> Yes, though I would call it pflash0 and pflash1.

(side-comment: read-only isn't correct for the varstore pflash)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] Configuring pflash devices for OVMF firmware

[Laszlo Ersek]

On 01/30/19 16:24, Peter Maydell wrote:

> Well, nobody who does anything with x86 has cared enough to
> make the pflash implementation actually correct.

I feel sort of included under this umbrella, so:

I haven't been aware of any particular pflash implementation errors. I
"didn't care" because it "worked fine" as much as I could tell.

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] Configuring pflash devices for OVMF firmware

[Laszlo Ersek]

On 01/30/19 15:33, Paolo Bonzini wrote:
> On 30/01/19 15:13, Markus Armbruster wrote:
>> -global driver=cfi.pflash01,property=secure,value=on
>>
>> Affects *all* such devices, but fortunately we have at most two, and
>> the one we don't want to affect happens to ignore the property value.
>
> Is this true?  I think both need secure=on, at least on x86.

commit f71e42a5c98722d6faa5be84a34fbad90d27dc04
Author: Paolo Bonzini 
Date:   Wed Apr 8 14:09:43 2015 +0200

pflash_cfi01: add secure property

When this property is set, MMIO accesses are only allowed with the
MEMTXATTRS_SECURE attribute.  This is used for secure access to UEFI
variables stored in flash.

Signed-off-by: Paolo Bonzini 

If you don't add "secure=on" to unit#0, then MMIO accesses will be
possible outside of SMM. From those, I'd hazard "MMIO reads" are
generally irrelevant. "MMIO writes" could succeed to the RAM image, but:

- they are never written back to the disk (due to readonly=on),

- the actual contents of unit#0 stops mattering as soon as the SEC phase
  decompresses the PEIFV and DXEFV firmware volumes from it, to DRAM.
  The SMM infrastructure is then constructed in SMRAM from DRAM. By the
  time a 3rd party UEFI application or driver, or an OS, is reached, the
  sensitive bits are all locked in SMRAM.

... But, I wonder if S3 resume would be under threat in this case. In
that case, SEC runs again (from pflash), and it re-decompresses
PEIFV/DXEFV from pflash to DRAM. If the in-memory image of pflash
doesn't revert to the (pristine, due to readonly=on) disk image at
platform reset, then I reckon there could be a problem; the SEC code and
the compressed FVs could have been tampered with in memory.

I guess it's best to apply secure=on to unit#0 as well, after all :)

Thanks!
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] Configuring pflash devices for OVMF firmware

[Laszlo Ersek]

On 01/28/19 15:58, Peter Maydell wrote:
> On Mon, 28 Jan 2019 at 14:56, Laszlo Ersek  wrote:
>> Regarding OVMF, I kept the flash driver intentionally in the dark about
>> split vs. unified pflash, so OVMF will not care, as long as the same
>> GPAs behave the same as before.
>>
>> Regarding ArmVirtQemu, I'm not so sure. It think it already depends on
>> two separate pflash chips, through the DTB that QEMU exposes. So
>> unifying the chips (albeit with multiple regions) might actually confuse
>> the firmware.
> 
> "virt" is a funny case because there is no underlying hardware
> that we're trying to match. So it is whatever we say it is
> and we've said it's two separate pflash chips. We don't need
> to change that I think. (IIRC this is derived partly from
> OVMF usecase requirements and partly from the vexpress devboards
> having 2 flash chips.)

That sounds great, but in that case, would the work be justified in
practice, to introduce single-ship-with-multiple-regions, if only x86
put it to use (for OVMF), but arm/aarch64 "virt" would have to stick
with the current layout?

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] Configuring pflash devices for OVMF firmware

[Laszlo Ersek]

On 01/28/19 14:06, Peter Maydell wrote:
> On Mon, 28 Jan 2019 at 12:40, Gerd Hoffmann  wrote:
>> The tricky part is the access control here.  On physical hardware you
>> typically have one flash rom, say 16M below 4G (on x86).
>>
>> Our pflash device doesn't allow to define multiple regions, so we use
>> multiple pflash devices instead, each with different access permissions
>> (code vs. vars).  Because of that they are more dynamic than they are on
>> phyiscal hardware, x86 sizes them according to the size of the firmware
>> images (arm is easier here, we have fixed size and location no matter
>> how big the firmware images are).
>>
>> So I think the options we have are:
>>   (a) leave pflash as-is, which pretty much implies physaddr and size
>>   must be user-configurable.
>>   (b) add support for multiple regions to pflash, so one can attach
>>   multiple blockdev at different offsets to a single pflash device.
> 
> The latter makes more sense to me -- we should model what the
> hardware actually has, because the guest can tell the difference.

Regarding OVMF, I kept the flash driver intentionally in the dark about
split vs. unified pflash, so OVMF will not care, as long as the same
GPAs behave the same as before.

Regarding ArmVirtQemu, I'm not so sure. It think it already depends on
two separate pflash chips, through the DTB that QEMU exposes. So
unifying the chips (albeit with multiple regions) might actually confuse
the firmware. I'm CC'ing Ard, he's done some work on the ARM NOR flash
driver recently, incl. updates to its usage in ArmVirtQemu:

* https://github.com/tianocore/edk2/commit/5e27deed438b

  [edk2] [PATCH v2 3/4]
  ArmVirtPkg/NorFlashQemuLib: disregard our primary FV

* https://github.com/tianocore/edk2/commit/51bb05c79595

  [edk2] [PATCH v2 4/4]
  ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping

Thanks
Laszlo

> (Migration compat is left as an exercise for the reader :-))
> 
> thanks
> -- PMM
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Configuring pflash devices for OVMF firmware

[Laszlo Ersek]

(+Peter, keeping the context)

On 01/25/19 16:03, Markus Armbruster wrote:
> We configure OVMF firmware for PC machine types with -drive if=pflash.
> This is pretty much the last remaining use of -drive in libvirt we can't
> yet replace by -blockdev.  Such a replacement is desirable, because
> -blockdev + -device is more flexible than -drive if=pflash.  Also, once
> we don't need -drive with new QEMU anymore, the path for deleting all
> -drive code in libvirt some day is open.  As with all desirables, the
> benefit needs to exceed the cost.
> 
> I'm going to describe the status quo, how we got there (briefly and much
> simplified), then sketch how to replace -drive if=pflash.  I'm afraid
> this is fairly long; sorry.  Please correct misunderstandings.  Beware,
> my libvirt and OVMF fu is much weaker than my QEMU fu.
> 
> In the beginning, board code read the BIOS from a fixed file and mapped
> it into the guest's address space.  Life was simple.
> 
> On physical hardware, the BIOS can persist a bit of state across (cold)
> reboots by storing it in (non-volatile) CMOS RAM.  We didn't bother.
> Simple.
> 
> Fast forward several years, and The Law of OS Envy (every program wants
> to grow into a full-blown operating system) has asserted itself: PC
> Firmware has grown from an 8KiB ROM using a few bytes of volatile and
> non-volatile RAM into a multi-megabyte beast with much more complex
> storage needs.
> 
> On today's physical PC hardware, firmware is stored in flash memory.
> There's code, and there's persistent data.  For obvious reasons, the
> code should be write-protected except when doing an upgrade.  "Secure
> boot" additionally needs to restrict data writes to system management
> mode (SMM).
> 
> Here's our first iteration of OVMF support, at QEMU level:
> 
> -drive if=pflash,format=raw,file=/where/ever/OVMF.fd
> 
> Generic code creates a block backend for it.  Magic board code picks up
> the backend, creates a frontend (a cfi.pflash01 device), and maps it
> into the guest's address space.
> 
> At libvirt level:
> 
>   /where/ever/OVMF.fd
> 
> Problem: while the flash device model provides read-only capability,
> it's all-or-nothing.  You can't tell it to write-protect just the part
> holding code.  The examples above don't write-protect anything.
> /where/ever/OVMF.fd better be writable exclusively.
> 
> The flash device model could be enhanced, but we went down a different
> path: we split the single OVMF image OVMF.fd ("unified build") into a
> code image OVMF_CODE.fd and a data image OVMF_VARS.fd ("split build").
> At QEMU level:
> 
> -drive if=pflash,format=raw,readonly,file=/usr/share/OVMF/OVMF_CODE.fd
> -drive if=pflash,format=raw,file=/where/ever/OVMF_VARS.fd
> 
> OVMF_CODE.fd must be unit 0, and OVMF_VARS.fd must be unit 1.
> 
> Generic code creates two block backends.  Magic board code picks them
> up, creates a frontend (a cfi.pflash01 device) for each, and maps them
> into the guest's address space.
> 
> Note there are *two* virtual flash devices now, whereas physical
> hardware commonly has just one.
> 
> At libvirt level:
> 
>   /usr/share/OVMF/OVMF_CODE.fd
>template="/usr/share/OVMF/OVMF_VARS.fd">/var/libvirt/nvram/${guest}_VARS.fd
> 
> This treats OVMF_VARS.fd as a read-only template, and gives each guest
> its own writable copy, which is nice.
> 
> The flash device model supports restricting writes to SMM (remember,
> that's required for secure boot).  It's controlled by cfi.pflash01
> property secure, off by default.  If we created the device model with
> -device, we'd simply pass secure=on.  But since we create it with -drive
> if=pflash, we can't.  Instead we have to use
> 
> -global driver=cfi.pflash01,property=secure,value=on
> 
> This flips the global default value.  Awkward, but works out okay,
> because (1) the flash device holding OVMF_VARS.fd wants this value, and
> (2) the flash device holding OVMF_CODE.fd doesn't care (it's read-only),
> and (3) there is no way to create additional flash devices.
> 
> At the libvirt level, we add secure='yes' to the loader element.
> 
> We also have to enable SMM emulation.  At QEMU level:
> 
> -machine smm=on
> 
> At libvirt level:
> 
> 
>   
> 
> 
> Note that the above configuration examples involve selecting OVMF
> images.  A bit of an inconvenience compared to BIOS, where the default
> "use the BIOS shipped with QEMU" pretty much just works.
> 
> To add annoyance to inconvenience, different distributions have
> different ideas on where to install OVMF images.  And because that's not
> complicated enough, we also have to pair code with data images.  And
> because that's still not complicated enough, any specific machine type
> may work only with a subset of the available firmwares.
> 
> The proposed way to deal with all that works as follows.
> 
> Each set of firmware images comes with a descriptor file.  These are
> JSON and conform to the QAPI schema docs/interop/firmware.json.
> 
> Among the 

Re: [libvirt] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Laszlo Ersek]

On 08/31/18 10:00, Igor Mammedov wrote:
> On Thu, 30 Aug 2018 17:51:13 +0200
> Laszlo Ersek  wrote:
> 
>> +Drew
>>
>> On 08/30/18 14:08, Igor Mammedov wrote:
>>> If VM has VCPUs plugged sparselly (for example a VM started with
>>> 3 VCPUs (cpu0, cpu1 and cpu2) and then cpu1 was hotunplugged so
>>> only cpu0 and cpu2 are present), QGA will rise a error
>>>   error: internal error: unable to execute QEMU agent command 
>>> 'guest-get-vcpus':
>>>   open("/sys/devices/system/cpu/cpu1/"): No such file or directory
>>> when
>>>   virsh vcpucount FOO --guest
>>> is executed.
>>> Fix it by ignoring non present CPUs when fetching CPUs status from sysfs.
>>>
>>> Signed-off-by: Igor Mammedov 
>>> ---
>>>  qga/commands-posix.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>>> index 37e8a2d..2929872 100644
>>> --- a/qga/commands-posix.c
>>> +++ b/qga/commands-posix.c
>>> @@ -2044,7 +2044,9 @@ static void transfer_vcpu(GuestLogicalProcessor 
>>> *vcpu, bool sys2vcpu,
>>>vcpu->logical_id);
>>>  dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
>>>  if (dirfd == -1) {
>>> -error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
>>> +if (!(sys2vcpu && errno == ENOENT)) {
>>> +error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
>>> +}
>>>  } else {
>>>  static const char fn[] = "online";
>>>  int fd;
>>>   
>>
>> Originally these guest agent commands (both getting and setting) were
>> meant to be used in the absence of real VCPU hot[un]plug, as a fallback
>> / place-holder.
>>
>> If the latter (= real VCPU hot(un)plug) works, then these guest agent
>> commands shouldn't be used at all.
> Technically there isn't reasons for "get" not to work in sparse  usecase
> hence the patch.
> 
>> Drew, do I remember correctly? ... The related RHBZ is
>> <https://bugzilla.redhat.com/show_bug.cgi?id=924684>. (It's a private
>> one, and I'm not at liberty to open it up, so my apologies to non-RH folks.)
>>
>> Anyway, given that "set" should be a subset of the "get" return value
>> (as documented in the command schema), if we fix up "get" to work with
>> sparse topologies, then "set" should work at once.
>>
>> However... as far as I understand, this change will allow
>> qmp_guest_get_vcpus() to produce a GuestLogicalProcessor object for the
>> missing (hot-unplugged) VCPU, with the following contents:
>> - @logical-id: populated by the loop,
>> - @online: false (from g_malloc0()),
>> - @can-offline: present (from the loop), and false (from g_malloc0()).
>>
>> The smaller problem with this might be that "online==false &&
>> can-offline==false" is nonsensical and has never been returned before. I
>> don't know how higher level apps will react.
>>
>> The larger problem might be that a higher level app could simply copy
>> this output structure into the next "set" call unchanged, and then that
>> "set" call will fail.
> Libvirt it seems that survives such outrageous output
> 
>> I wonder if, instead of this patch, we should rework
>> qmp_guest_get_vcpus(), to silently skip processors for which this
>> dirpath ENOENT condition arises (i.e., return a shorter list of
>> GuestLogicalProcessor objects).
> 
>> But, again, I wouldn't mix this guest agent command with real VCPU
>> hot(un)plug in the first place. The latter is much-much better, so if
>> it's available, use that exclusively?
> Agreed,
> 
> Maybe we can block invalid usecase on libvirt side with a more clear
> error message as libvirt sort of knows that sparse cpus are supported.

OK -- I see libvir-list is CC'd, so let's hear what they prefer :)

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 3/3] hw/vfio/display: add ramfb support

[Laszlo Ersek]

On 06/14/18 00:36, Gerd Hoffmann wrote:
> On Wed, Jun 13, 2018 at 01:50:47PM -0600, Alex Williamson wrote:

>> I suppose in the UEFI case runtime services can be used to continue
>> writing this display,
> 
> Yes.

Small clarification for the wording -- "UEFI runtime services" do not
include anything display- or graphics-related. Instead, the OS kernel
may inherit the framebuffer properties (base address, size, pixel
format), and continue accessing the framebuffer directly.

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/4] qemu: Add support for setting the TSEG size

[Laszlo Ersek]

On 06/07/18 23:14, Martin Kletzander wrote:

> @Laszlo: When thinking about it, even though it is a very stupid idea,
> technically there isn't really a difference between
> 'extended-tseg-mbytes=0' and
> 'extended-tseg-mbytes=8'

that's correct

> (of course the second option will still provide a
> fw_cfg info about the size etc., but the guest will behave the same
> way), right?

- it's not fw_cfg but a made-up register in PCI config space
- the guest won't behave *exactly* the same way, technically speaking,
  but the end result will be 8MB in both cases, yes.

> That brings me to another question (maybe even more stupid), what size
> does OVMF choose if I specify 'extended-tseg-mbytes=6'?

6MB.

Thanks,
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/4] qemu: Add support for setting the TSEG size

[Laszlo Ersek]

On 06/07/18 21:58, Ján Tomko wrote:

> Does a size of 0 MiB make sense? It's divisible by 1 MiB.

"-global mch.extended-tseg-mbytes=0" makes QEMU behave as if it lacked
the extended TSEG feature; the guest will believe that only the standard
1 / 2 / 8 MB TSEG sizes are available, and pick one of those.

Technically, in QEMU the extended TSEG feature is disabled for older
machine types by setting "mch.extended-tseg-mbytes" to zero:

[include/hw/i386/pc.h]

#define PC_COMPAT_2_9 \
HW_COMPAT_2_9 \
{\
.driver   = "mch",\
.property = "extended-tseg-mbytes",\
.value= stringify(0),\
},\

I'm unsure whether this -- i.e., disabling the feature -- is useful to
expose through the domain config. Probably not.

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] conf, schema, docs: Add support for TSEG size setting

[Laszlo Ersek]

}
> +
> +virXMLFormatElement(buf, "smm", , );
> +}
> +
> +break;
> +
>  case VIR_DOMAIN_FEATURE_APIC:
>  if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
>  virBufferAddLit(buf, " diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8a8121bf83b2..a41cdce6ff63 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2421,6 +2421,9 @@ struct _virDomainDef {
>  char *hyperv_vendor_id;
>  int apic_eoi;
>  
> +bool tseg_specified;
> +unsigned long long tseg_size;
> +
>  virDomainClockDef clock;
>  
>  size_t ngraphics;
> diff --git a/tests/genericxml2xmlindata/tseg.xml 
> b/tests/genericxml2xmlindata/tseg.xml
> new file mode 100644
> index ..49483f874cd4
> --- /dev/null
> +++ b/tests/genericxml2xmlindata/tseg.xml
> @@ -0,0 +1,23 @@
> +
> +  QEMUGuest1
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  219100
> +  219100
> +  1
> +  
> +hvm
> +
> +  
> +  
> +
> +  48
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +/usr/bin/qemu-system-x86_64
> +  
> +
> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
> index d8270a6cae82..daad6e0f78d8 100644
> --- a/tests/genericxml2xmltest.c
> +++ b/tests/genericxml2xmltest.c
> @@ -141,6 +141,8 @@ mymain(void)
>  DO_TEST_FULL("cachetune-colliding-types", false, true,
>   TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
>  
> +DO_TEST("tseg");
> +
>  virObjectUnref(caps);
>  virObjectUnref(xmlopt);
>  
> 

I checked the commit message and the docs; they look OK to me.

Acked-by: Laszlo Ersek 

Thanks,
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/5] qemu: Move checks for SMM from command-line creation into validation phase

[Laszlo Ersek]

adding qemu-devel, Paolo and Gerd; comments below:

On 05/30/18 23:08, Martin Kletzander wrote:
> On Wed, May 30, 2018 at 11:02:59AM -0400, John Ferlan wrote:
>>
>>
>> On 05/21/2018 11:00 AM, Martin Kletzander wrote:
>>> We are still hoping all of such checks will be moved there and this
>>> is one small
>>> step in that direction.
>>>
>>> One of the things that this is improving is the error message you get
>>> when
>>> starting a domain with SMM and i440fx, for example.  Instead of
>>> saying that the
>>> QEMU binary doesn't support that option, we correctly say that it is
>>> only
>>> supported with q35 machine type.
>>>
>>> Signed-off-by: Martin Kletzander 
>>> ---
>>>  src/qemu/qemu_capabilities.c | 21 +++--
>>>  src/qemu/qemu_capabilities.h |  4 ++--
>>>  src/qemu/qemu_command.c  | 12 ++--
>>>  src/qemu/qemu_domain.c   | 12 +---
>>>  4 files changed, 28 insertions(+), 21 deletions(-)
>>>
>>
>> I know it's outside the bounds of what you're doing; however,
>> qemuDomainDefValidateFeatures could check the capabilities for other
>> bits too...
>>
> 
> Probably, but I mostly wanted to do that because SMM is not only about the
> capability, but also about the machine.  Good idea for the future, though.
> 
>> [...]
>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index d3beee5d8760..881d0ea46a75 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -3430,7 +3430,8 @@ qemuDomainDefGetVcpuHotplugGranularity(const
>>> virDomainDef *def)
>>>
>>>
>>>  static int
>>> -qemuDomainDefValidateFeatures(const virDomainDef *def)
>>> +qemuDomainDefValidateFeatures(const virDomainDef *def,
>>> +  virQEMUCapsPtr qemuCaps)
>>>  {
>>>  size_t i;
>>>
>>> @@ -3477,6 +3478,12 @@ qemuDomainDefValidateFeatures(const
>>> virDomainDef *def)
>>>  }
>>>  break;
>>>
>>> +    case VIR_DOMAIN_FEATURE_SMM:
>>> +    if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
>>
>> Probably should change to != _ABSENT, since qemu_command will supply
>> smm={on|off}
>>
> 
> That makes sense, kind of.  For 'off' we only need to check if we can
> specify
> the smm= option.  The thing is that you can even specify smm=on with
> non-q35
> machine type, but it is unclear what that's going to mean since it doesn't
> really make sense.
> 
> @Laszlo: What would you say?  Should we allow users to specify smm=on
> for users?
> Or even better, does it makes sense to allow specifying smm=anything for
> non-q35
> machine types?  If not, we'll leave it like this, that is smm=anything is
> forbidden for non-q35 machine types.

Technically the "smm" machine type property is defined for both i440fx
and q35:

$ qemu-system-x86_64 -machine pc,\? 2>&1 | grep smm
pc-i440fx-2.11.smm=OnOffAuto (Enable SMM (pc & q35))

$ qemu-system-x86_64 -machine q35,\? 2>&1 | grep smm
pc-q35-2.11.smm=OnOffAuto (Enable SMM (pc & q35))

The "smm" property controls whether system management mode is emulated,
to my knowledge. That's an x86 processor operating mode, which isn't
specific to the Q35 board. What's specific to Q35 is the TSEG.

The primary reason why OVMF (built with the edk2 SMM driver stack)
requires Q35 is not that i440fx does not emulate SMM, it's that i440fx
doesn't have a large enough SMRAM area. (The legacy SMRAM areas are too
small for OVMF.)

For example, SeaBIOS too includes some SMM code (optionally, if it's
built like that), and that runs on i440fx just fine, because the legacy
SMRAM areas are large enough for SeaBIOS's purposes, AIUI.

(Now, there's at least one other reason why OVMF/SMM needs Q35: because
the SMI broadcast feature too is only implemented on Q35. But that's
rather a consequence of the above "primary reason", and not a
stand-alone reason itself -- we restricted the SMI broadcast feature to
Q35 *because* OVMF could only run on Q35. Anyhow, you need not care
about SMI broadcast because it's automatically negotiated between guest
fw and QEMU.)

Summary:

(1) For making Secure Boot actually secure in OVMF, OVMF needs to be
built with -D SMM_REQUIRE, so that it include the SMM driver stack.

(2) Booting such a firmware binary requires Q35, because only Q35 offers
TSEG, and the legacy -- non-TSEG -- SMRAM ranges are too small even for
a "basic boot" of OVMF.

(3) QEMU has to be configured with "-machine smm=on"; this is already
covered by libvirt via .

(4) QEMU has to be configured to restrict pflash writes to code that
executes in SMM, with "-global
driver=cfi.pflash01,property=secure,value=on". Libvirt covers this
already with the @secure=yes attribute in .

(5) There are two *quality of service* features related to SMM; with
both of them being Q35-only. The first feature is SMI broadcast; libvirt
need not care because it's auto-negotiated.

(6) The second QoS feature is *extended* TSEG (a paravirt feature on top
of the standard TSEG), which lets the edk2 SMM driver stack scale to
large RAM sizes and 

Re: [libvirt] [Qemu-devel] [qemu PATCH v2] docs/interop: add "firmware.json"

[Laszlo Ersek]

On 05/24/18 18:23, Paolo Bonzini wrote:
> On 24/05/2018 18:21, Laszlo Ersek wrote:
>> On 05/15/18 11:49, Gerd Hoffmann wrote:
>>> On Wed, May 09, 2018 at 05:26:08PM +0200, Laszlo Ersek wrote:
>>>> Add a schema that describes the different uses and properties of virtual
>>>> machine firmware.
>>>>
>>>> Each firmware executable installed on a host system should come with at
>>>> least one JSON file that conforms to this schema. Each file informs the
>>>> management applications about
>>>> - the firmware's properties and one possible use case / feature set,
>>>> - configuration bits that are required to run the firmware binary.
>>>>
>>>> In addition, define rules for management apps for picking the highest
>>>> priority firmware JSON file when multiple such files match the search
>>>> criteria.
>>>
>>> Reviewed-by: Gerd Hoffmann <kra...@redhat.com>
>>
>> Thanks, Gerd!
>>
>> Does anyone else intend to comment? Or should I ask for someone to queue
>> the patch for a pull request?
> 
> Since no one has done that so far I've queued it, thanks!

Awesome; huge kudos!
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [qemu PATCH v2] docs/interop: add "firmware.json"

[Laszlo Ersek]

On 05/15/18 11:49, Gerd Hoffmann wrote:
> On Wed, May 09, 2018 at 05:26:08PM +0200, Laszlo Ersek wrote:
>> Add a schema that describes the different uses and properties of virtual
>> machine firmware.
>>
>> Each firmware executable installed on a host system should come with at
>> least one JSON file that conforms to this schema. Each file informs the
>> management applications about
>> - the firmware's properties and one possible use case / feature set,
>> - configuration bits that are required to run the firmware binary.
>>
>> In addition, define rules for management apps for picking the highest
>> priority firmware JSON file when multiple such files match the search
>> criteria.
> 
> Reviewed-by: Gerd Hoffmann <kra...@redhat.com>

Thanks, Gerd!

Does anyone else intend to comment? Or should I ask for someone to queue
the patch for a pull request?

Thanks!
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [qemu PATCH v2] docs/interop: add "firmware.json"

[Laszlo Ersek]

Add a schema that describes the different uses and properties of virtual
machine firmware.

Each firmware executable installed on a host system should come with at
least one JSON file that conforms to this schema. Each file informs the
management applications about
- the firmware's properties and one possible use case / feature set,
- configuration bits that are required to run the firmware binary.

In addition, define rules for management apps for picking the highest
priority firmware JSON file when multiple such files match the search
criteria.

Cc: "Daniel P. Berrange" <berra...@redhat.com>
Cc: David Gibson <dgib...@redhat.com>
Cc: Eric Blake <ebl...@redhat.com>
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: Kashyap Chamarthy <kcham...@redhat.com>
Cc: Markus Armbruster <arm...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Thomas Huth <th...@redhat.com>
Signed-off-by: Laszlo Ersek <ler...@redhat.com>
---

Notes:
PATCHv2:

- previous version (PATCHv1) was posted at:
  <20180509150431.4979-1-lersek@redhat.com">http://mid.mail-archive.com/20180509150431.4979-1-lersek@redhat.com>
  <https://www.redhat.com/archives/libvir-list/2018-May/msg00612.html>

- drop @x-check-firmware for good

PATCH:

- previous version (RFCv3) was posted at:
  <20180420231246.23130-4-lersek@redhat.com">http://mid.mail-archive.com/20180420231246.23130-4-lersek@redhat.com>

- move "firmware.json" under docs/interop/; drop earlier development
  artifacts

- no changes to the schema (received no comments on RFCv3)

RFCv3:

- Previous version (RFCv2) was posted at:
  <20180417224054.26363-1-lersek@redhat.com">http://mid.mail-archive.com/20180417224054.26363-1-lersek@redhat.com>

- Trim the CC list a little.

- Wrap to a 72-char margin, for an easier reading. [Markus]

- Run spell-checker. [Markus]

- Rename @FirmwareType to @FirmwareOSInterface, to better express the
  concept. Update the enum constants accordingly (incl. documentation);
  in particular replace @slof with @openfirmware. Rename the @type field
  in @Firmware to @interface-type. [David, Gerd, Paolo, Thomas]

- Better yet, rename @interface-type to @interface-types (plural), and
  turn it into a list of @FirmwareOSInterface entries, to express an
  ordered compat list with multiple firmware<->OS interface types. [Dan,
  Eric, Gerd, Paolo]

- Drop @FirmwareArchitecture, and switch the type of
  @FirmwareTarget.@architecture to @SysEmuTarget (now defined in
  "common.json"). [Dan, Gerd, Markus]

- Switch elements of @FirmwareTarget.@machines from aliases ("pc",
  "q35", "virt") to concrete machine types, with glob patterns
  understood. [Dan, Gerd]

- Rename @secure-boot-enrolled-keys to @enrolled-keys, and drop the
  dependency on @secure-boot in its documentation. [Gerd]

- Describe the firmware JSON directories and the search rules to
  management apps, under the @Firmware element. Relatedly, remove the
  language on any concrete certificates under @enrolled-keys, as these
  can be customized through the overrides. Also refresh the commit
  message. [Dan, Gerd]

- Rename @pathname fields to @filename. [Markus]

- Don't replace "@executable.@filename" with "@executable.filename"
  (that is, keep the second @ sign), because removing the 2nd @ sign
  messes up the documentation ("filename" stops being type-set in
  monospace in the HTML, for example). [Markus]

- Rename @nvram_template to @nvram-template. [Thomas]

- Keep @x-check-firmware for a while longer. [Dan, Gerd, Markus, Paolo]

RFCv2:

- previous version (RFCv1) was posted at
  <20180407000117.25640-1-lersek@redhat.com">http://mid.mail-archive.com/20180407000117.25640-1-lersek@redhat.com>

- v2 is basically a rewrite from scratch, starting out with Dan's
  definitions from
  <20180410102033.GL5155@redhat.com">http://mid.mail-archive.com/20180410102033.GL5155@redhat.com> and
  <20180410110357.GP5155@redhat.com">http://mid.mail-archive.com/20180410110357.GP5155@redhat.com>,
  hopefully addressing others' feedback as well

RFCv1:

- Folks on the CC list, please try to see if the suggested schema is
  flexible enough to describe the virtual firmware(s) that you are
  familiar with. Thanks!

 docs/interop/firmware.json | 540 +
 1 file changed, 540 insertions(+)
 create mode 100644 docs/interop/firmware.json

diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
new file mode 100644
in

Re: [libvirt] [Qemu-devel] [qemu PATCH] docs/interop: add "firmware.json"

[Laszlo Ersek]

On 05/09/18 17:04, Laszlo Ersek wrote:
> Add a schema that describes the different uses and properties of virtual
> machine firmware.
> 
> Each firmware executable installed on a host system should come with at
> least one JSON file that conforms to this schema. Each file informs the
> management applications about
> - the firmware's properties and one possible use case / feature set,
> - configuration bits that are required to run the firmware binary.
> 
> In addition, define rules for management apps for picking the highest
> priority firmware JSON file when multiple such files match the search
> criteria.
> 
> Cc: "Daniel P. Berrange" <berra...@redhat.com>
> Cc: David Gibson <dgib...@redhat.com>
> Cc: Eric Blake <ebl...@redhat.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Cc: Kashyap Chamarthy <kcham...@redhat.com>
> Cc: Markus Armbruster <arm...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Thomas Huth <th...@redhat.com>
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
> 
> Notes:
> PATCH:
> 
> - previous version (RFCv3) was posted at:
>   <20180420231246.23130-4-lersek@redhat.com">http://mid.mail-archive.com/20180420231246.23130-4-lersek@redhat.com>
> 
> - move "firmware.json" under docs/interop/; drop earlier development
>   artifacts
> 
> - no changes to the schema (received no comments on RFCv3)

oops:

> +##
> +# @x-check-firmware:
> +#
> +# Accept a @Firmware object and do nothing, successfully. This command
> +# can be used in the QMP shell to validate @Firmware JSON against the
> +# schema.
> +#
> +# @fw: ignored
> +#
> +# Since: 2.13
> +##
> +{ 'command' : 'x-check-firmware',
> +  'data': { 'fw' : 'Firmware' } }
> 

missed this part (I should have removed it), I will post v2 soon.

Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [qemu PATCH] docs/interop: add "firmware.json"

[Laszlo Ersek]

Add a schema that describes the different uses and properties of virtual
machine firmware.

Each firmware executable installed on a host system should come with at
least one JSON file that conforms to this schema. Each file informs the
management applications about
- the firmware's properties and one possible use case / feature set,
- configuration bits that are required to run the firmware binary.

In addition, define rules for management apps for picking the highest
priority firmware JSON file when multiple such files match the search
criteria.

Cc: "Daniel P. Berrange" <berra...@redhat.com>
Cc: David Gibson <dgib...@redhat.com>
Cc: Eric Blake <ebl...@redhat.com>
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: Kashyap Chamarthy <kcham...@redhat.com>
Cc: Markus Armbruster <arm...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Thomas Huth <th...@redhat.com>
Signed-off-by: Laszlo Ersek <ler...@redhat.com>
---

Notes:
PATCH:

- previous version (RFCv3) was posted at:
  <20180420231246.23130-4-lersek@redhat.com">http://mid.mail-archive.com/20180420231246.23130-4-lersek@redhat.com>

- move "firmware.json" under docs/interop/; drop earlier development
  artifacts

- no changes to the schema (received no comments on RFCv3)

RFCv3:

- Previous version (RFCv2) was posted at:
  <20180417224054.26363-1-lersek@redhat.com">http://mid.mail-archive.com/20180417224054.26363-1-lersek@redhat.com>

- Trim the CC list a little.

- Wrap to a 72-char margin, for an easier reading. [Markus]

- Run spell-checker. [Markus]

- Rename @FirmwareType to @FirmwareOSInterface, to better express the
  concept. Update the enum constants accordingly (incl. documentation);
  in particular replace @slof with @openfirmware. Rename the @type field
  in @Firmware to @interface-type. [David, Gerd, Paolo, Thomas]

- Better yet, rename @interface-type to @interface-types (plural), and
  turn it into a list of @FirmwareOSInterface entries, to express an
  ordered compat list with multiple firmware<->OS interface types. [Dan,
  Eric, Gerd, Paolo]

- Drop @FirmwareArchitecture, and switch the type of
  @FirmwareTarget.@architecture to @SysEmuTarget (now defined in
  "common.json"). [Dan, Gerd, Markus]

- Switch elements of @FirmwareTarget.@machines from aliases ("pc",
  "q35", "virt") to concrete machine types, with glob patterns
  understood. [Dan, Gerd]

- Rename @secure-boot-enrolled-keys to @enrolled-keys, and drop the
  dependency on @secure-boot in its documentation. [Gerd]

- Describe the firmware JSON directories and the search rules to
  management apps, under the @Firmware element. Relatedly, remove the
  language on any concrete certificates under @enrolled-keys, as these
  can be customized through the overrides. Also refresh the commit
  message. [Dan, Gerd]

- Rename @pathname fields to @filename. [Markus]

- Don't replace "@executable.@filename" with "@executable.filename"
  (that is, keep the second @ sign), because removing the 2nd @ sign
  messes up the documentation ("filename" stops being type-set in
  monospace in the HTML, for example). [Markus]

- Rename @nvram_template to @nvram-template. [Thomas]

- Keep @x-check-firmware for a while longer. [Dan, Gerd, Markus, Paolo]

RFCv2:

- previous version (RFCv1) was posted at
  <20180407000117.25640-1-lersek@redhat.com">http://mid.mail-archive.com/20180407000117.25640-1-lersek@redhat.com>

- v2 is basically a rewrite from scratch, starting out with Dan's
  definitions from
  <20180410102033.GL5155@redhat.com">http://mid.mail-archive.com/20180410102033.GL5155@redhat.com> and
  <20180410110357.GP5155@redhat.com">http://mid.mail-archive.com/20180410110357.GP5155@redhat.com>,
  hopefully addressing others' feedback as well

RFCv1:

- Folks on the CC list, please try to see if the suggested schema is
  flexible enough to describe the virtual firmware(s) that you are
  familiar with. Thanks!

 docs/interop/firmware.json | 554 +
 1 file changed, 554 insertions(+)
 create mode 100644 docs/interop/firmware.json

diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
new file mode 100644
index 0000..2add6fd8afe7
--- /dev/null
+++ b/docs/interop/firmware.json
@@ -0,0 +1,554 @@
+# -*- Mode: Python -*-
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# Authors:
+#  Daniel P. Berrange <berra...@redhat.com>
+#  Laszlo Ersek <ler...@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, versio

Re: [libvirt] [Qemu-devel] [qemu RFC v3 3/3] qapi: add "firmware.json"

[Laszlo Ersek]

Ping:

On 04/21/18 01:12, Laszlo Ersek wrote:
> Add a schema that describes the different uses and properties of virtual
> machine firmware.
> 
> Each firmware executable installed on a host system should come with at
> least one JSON file that conforms to this schema. Each file informs the
> management applications about
> - the firmware's properties and one possible use case / feature set,
> - configuration bits that are required to run the firmware binary.
> 
> In addition, define rules for management apps for picking the highest
> priority firmware JSON file when multiple such files match the search
> criteria.
> 
> Cc: "Daniel P. Berrange" <berra...@redhat.com>
> Cc: David Gibson <dgib...@redhat.com>
> Cc: Eric Blake <ebl...@redhat.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Cc: Kashyap Chamarthy <kcham...@redhat.com>
> Cc: Markus Armbruster <arm...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Thomas Huth <th...@redhat.com>
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
> 
> Notes:
> RFCv3:
> 
> - Previous version (RFCv2) was posted at:
>   <20180417224054.26363-1-lersek@redhat.com">http://mid.mail-archive.com/20180417224054.26363-1-lersek@redhat.com>
> 
> - Trim the CC list a little.
> 
> - Wrap to a 72-char margin, for an easier reading. [Markus]
> 
> - Run spell-checker. [Markus]
> 
> - Rename @FirmwareType to @FirmwareOSInterface, to better express the
>   concept. Update the enum constants accordingly (incl. documentation);
>   in particular replace @slof with @openfirmware. Rename the @type field
>   in @Firmware to @interface-type. [David, Gerd, Paolo, Thomas]
> 
> - Better yet, rename @interface-type to @interface-types (plural), and
>   turn it into a list of @FirmwareOSInterface entries, to express an
>   ordered compat list with multiple firmware<->OS interface types. [Dan,
>   Eric, Gerd, Paolo]
> 
> - Drop @FirmwareArchitecture, and switch the type of
>   @FirmwareTarget.@architecture to @SysEmuTarget (now defined in
>   "common.json"). [Dan, Gerd, Markus]
> 
> - Switch elements of @FirmwareTarget.@machines from aliases ("pc",
>   "q35", "virt") to concrete machine types, with glob patterns
>   understood. [Dan, Gerd]
> 
> - Rename @secure-boot-enrolled-keys to @enrolled-keys, and drop the
>   dependency on @secure-boot in its documentation. [Gerd]
> 
> - Describe the firmware JSON directories and the search rules to
>   management apps, under the @Firmware element. Relatedly, remove the
>   language on any concrete certificates under @enrolled-keys, as these
>   can be customized through the overrides. Also refresh the commit
>   message. [Dan, Gerd]
> 
> - Rename @pathname fields to @filename. [Markus]
> 
> - Don't replace "@executable.@filename" with "@executable.filename"
>   (that is, keep the second @ sign), because removing the 2nd @ sign
>   messes up the documentation ("filename" stops being type-set in
>   monospace in the HTML, for example). [Markus]
> 
> - Rename @nvram_template to @nvram-template. [Thomas]
> 
> - Keep @x-check-firmware for a while longer. [Dan, Gerd, Markus, Paolo]

Any comments on this single patch?

Should I post just the schema, identically, for
"docs/interop/firmware.json", as a regular PATCH?

(Personally I wouldn't like to add any real QAPI integration for this
schema. I'm not saying it "shouldn't be done", just that it shouldn't be
done by me.)

Thanks,
Laszlo


> RFCv2:
> 
> - previous version (RFCv1) was posted at
>   <20180407000117.25640-1-lersek@redhat.com">http://mid.mail-archive.com/20180407000117.25640-1-lersek@redhat.com>
> 
> - v2 is basically a rewrite from scratch, starting out with Dan's
>   definitions from
>   <20180410102033.GL5155@redhat.com">http://mid.mail-archive.com/20180410102033.GL5155@redhat.com> and
>   <20180410110357.GP5155@redhat.com">http://mid.mail-archive.com/20180410110357.GP5155@redhat.com>,
>   hopefully addressing others' feedback as well
> 
> RFCv1:
> 
> - Folks on the CC list, please try to see if the suggested schema is
>   flexible enough to describe the virtual firmware(s) that you are
>   familiar with. Thanks!
> 
>  Makefile  |   9 +
>  Makefile.objs |   4 +
>  qapi/firmware.json| 554 
> +

Re: [libvirt] [Qemu-devel] [qemu RFC v3 2/3] qapi: change the type of TargetInfo.arch from string to enum SysEmuTarget

[Laszlo Ersek]

On 04/23/18 11:50, Markus Armbruster wrote:
> Laszlo Ersek <ler...@redhat.com> writes:
> 
>> Now that we have @SysEmuTarget, it makes sense to restict
>> @TargetInfo.@arch to valid sysemu targets at the schema level.
> 
> We could mention that supported targets become visible in QMP
> introspection.  But I can't see what QMP clients could do with this
> information, so let's not bother.
> 
>>
>> Cc: "Daniel P. Berrange" <berra...@redhat.com>
>> Cc: David Gibson <dgib...@redhat.com>
>> Cc: Eric Blake <ebl...@redhat.com>
>> Cc: Gerd Hoffmann <kra...@redhat.com>
>> Cc: Kashyap Chamarthy <kcham...@redhat.com>
>> Cc: Markus Armbruster <arm...@redhat.com>
>> Cc: Paolo Bonzini <pbonz...@redhat.com>
>> Cc: Thomas Huth <th...@redhat.com>
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>>
>> Notes:
>> RFCv3:
>> 
>> - The patch is new in this version. [Markus]
>>
>>  qapi/misc.json |  6 --
>>  arch_init.c| 18 +-
>>  2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index 5636f4a14997..3b4fbc534089 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json
>> @@ -5,6 +5,8 @@
>>  # = Miscellanea
>>  ##
>>  
>> +{ 'include': 'common.json' }
>> +
>>  ##
>>  # @qmp_capabilities:
>>  #
>> @@ -2449,12 +2451,12 @@
>>  #
>>  # Information describing the QEMU target.
>>  #
>> -# @arch: the target architecture (eg "x86_64", "i386", etc)
>> +# @arch: the target architecture
>>  #
>>  # Since: 1.2.0
>>  ##
>>  { 'struct': 'TargetInfo',
>> -  'data': { 'arch': 'str' } }
>> +  'data': { 'arch': 'SysEmuTarget' } }
>>  
>>  ##
>>  # @query-target:
>> diff --git a/arch_init.c b/arch_init.c
>> index 6ee07478bd11..4367f30291f8 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -29,6 +29,7 @@
>>  #include "hw/pci/pci.h"
>>  #include "hw/audio/soundhw.h"
>>  #include "qapi/qapi-commands-misc.h"
>> +#include "qapi/error.h"
>>  #include "qemu/config-file.h"
>>  #include "qemu/error-report.h"
>>  #include "hw/acpi/acpi.h"
>> @@ -111,8 +112,23 @@ int xen_available(void)
>>  TargetInfo *qmp_query_target(Error **errp)
>>  {
>>  TargetInfo *info = g_malloc0(sizeof(*info));
>> +Error *local_err = NULL;
>>  
>> -info->arch = g_strdup(TARGET_NAME);
>> +/*
>> + * The fallback enum value is irrelevant here (TARGET_NAME is a
>> + * macro and can never be NULL), so simply pass zero. Also, the
> 
> We pass -1 in similar cases elsewhere.
> 
>> + * lookup should never fail -- if it fails, then @SysEmuTarget needs
>> + * extending. Catch that with an assertion, but also handle it
>> + * gracefully, in case someone adventurous disables assertions.
>> + */
>> +info->arch = qapi_enum_parse(_lookup, TARGET_NAME, 0,
>> + _err);
>> +g_assert(!local_err);
>> +if (local_err) {
>> +error_propagate(errp, local_err);
>> +qapi_free_TargetInfo(info);
>> +return NULL;
>> +}
> 
> Simpler:
> 
>info->arch = qapi_enum_parse(_lookup, TARGET_NAME, 0,
> _abort);

OK, I'll use this form -- but should I use -1 or 0, after all, for
default value?

Thanks,
Laszlo

> 
>>  
>>  return info;
>>  }

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [qemu RFC v3 1/3] qapi: add SysEmuTarget to "common.json"

[Laszlo Ersek]

On 04/23/18 11:52, Markus Armbruster wrote:
> Laszlo Ersek <ler...@redhat.com> writes:
> 
>> We'll soon need an enumeration type that lists all the softmmu targets
>> that QEMU (the project) supports. Introduce @SysEmuTarget to
>> "common.json".
>>
>> Cc: "Daniel P. Berrange" <berra...@redhat.com>
>> Cc: David Gibson <dgib...@redhat.com>
>> Cc: Eric Blake <ebl...@redhat.com>
>> Cc: Gerd Hoffmann <kra...@redhat.com>
>> Cc: Kashyap Chamarthy <kcham...@redhat.com>
>> Cc: Markus Armbruster <arm...@redhat.com>
>> Cc: Paolo Bonzini <pbonz...@redhat.com>
>> Cc: Thomas Huth <th...@redhat.com>
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> 
> Reviewed-by: Markus Armbruster <arm...@redhat.com>
> 

Thanks! I think I'll post a PATCH (not RFC) series next, with patches #1
and #2 from this set (addressing your comments for patch #2), and I'll
try to add patch #3 for your earlier comments here:

87k1t1sujh.fsf@dusky.pond.sub.org">http://mid.mail-archive.com/87k1t1sujh.fsf@dusky.pond.sub.org

And we can keep the firmware.json RFC separate, and dependent on that set.

Thanks!
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/20/18 18:37, Markus Armbruster wrote:
> Laszlo Ersek <ler...@redhat.com> writes:
> 
>> On 04/20/18 14:53, Markus Armbruster wrote:
>>> Laszlo Ersek <ler...@redhat.com> writes:
>>
>> [snip]
>>
>>>> The targets with softmmu are: aarch64, alpha, arm, cris, hppa, i386,
>>>> lm32, m68k, microblaze, microblazeel, mips, mips64, mips64el, mipsel,
>>>> moxie, nios2, or1k, ppc, ppc64, ppcemb, riscv32, riscv64, s390x, sh4,
>>>> sh4eb, sparc, sparc64, tricore, unicore32, x86_64, xtensa, xtensaeb.
>>>>
>>>> That is, at least the following constants in CpuInfoArch have no
>>>> corresponding (identical) mapping -- I'll state to the right of the
>>>> arrow the emulation targets which I know or presume to be associated
>>>> with the CpuInfoArch constant:
>>>> - x86 -> i386, x86_64
>>>> - sparc -> sparc, sparc64
>>>> - ppc -> ppc, ppc64, ppcemb
>>>> - mips -> mips, mips64, mips64el, mipsel
>>>> - s390 -> s390x
>>>> - riscv -> riscv32, riscv64
>>>>
>>>> The only constant that seems to have a 1:1 mapping is "tricore", but I
>>>> could perfectly well be thinking even that just because I have no clue
>>>> about "tricore".
>>>>
>>>> So, I don't think CpuInfoArch can be updated so that it both remains
>>>> compatible with current QMP clients, and serves "firmware.json". In the
>>>> firmware schema we don't just need CPU architecture, but actual emulator
>>>> names (and board / machine types).
>>>
>>> The completely orthodox fix for CpuInfo would be:
>>>
>>> * Keep @arch unchanged.  In particular, keep "other" for all targets
>>>   other than 'x86', 'sparc', 'ppc', 'mips', 'tricore'.
>>>
>>> * Add a new member @target of new enum type Target, whose values match
>>>   configures idea of targets exactly.
>>>
>>> * Make the new member the union tag.
>>>
>>> It's too late for complete orthodoxy; we already changed @arch.
>>>
>>> A common enum type Target makes sense anyway, I think.
>>>
>>> Using it in CpuInfo as described above may make sense, too.  Could be
>>> left to a follow-up patch.
>>>
>>>> I grepped 'qapi/*json' for the whole word "x86_64", and the only thing
>>>> that remotely matches is the @TargetInfo structure, in which the @arch
>>>> field is a string, coming with the example "x86_64". The example also
>>>> names "i386" separately.
>>>
>>> Well spotted.
>>>
>>> TargetInfo member @arch is set to TARGET_NAME, which matches configure's
>>> idea of the target.  If we add enum Target, we should change @arch's
>>> type from str to Target.
>>>
>>>> So what might make sense is to introduce a separate enum in
>>>> "qapi/misc.json" with all the softmmu targets I listed above, and change
>>>> the type of @TargetInfo.@arch to that enum.
>>>
>>> I arrived at this conclusion, too.
>>>
>>>> In parallel,
>>>> qmp_query_target() would have to be updated to look up the enum value
>>>> matching TARGET_NAME. (I do think we can ignore linux-user etc emulators
>>>> for collecting the relevant arches here: @TargetInfo is only used in the
>>>> "query-target" QMP command, and Markus said above that QMP is only used
>>>> with system emulation.)
>>>>
>>>> Should I do this?
>>>
>>> Yes, please.
>>
>> OK, so here's my understanding:
>>
>> (1) I'll introduce a new @Target enum in misc.json. I'll inherit /
>> include it in firmware.json. This is compatible with what Dan said.
>>
>> (2) I'll change @TargetInfo.@arch to the new type. I believe this will
>> break the compilation of qmp_query_target(); I'll hack on that until it
>> builds again, with the lookup. :)
>>
>> (3) Regarding the addition of @target to CpuInfo (accompanying @arch)
>> doesn't look hard; what *does* look hard is changing the union
>> discriminator from @arch to @target. @target has many more values, and I
>> would have to map all of them to the (fewer) @arch values that currently
>> do *not* select @CpuInfoOther. Here's an example:
>>
>> - Both @i386 and @x86_64 from @target mean @x86 in @arch,
>> - because @x86 currently selects @CpuInfoX86, 

[libvirt] [qemu RFC v3 2/3] qapi: change the type of TargetInfo.arch from string to enum SysEmuTarget

[Laszlo Ersek]

Now that we have @SysEmuTarget, it makes sense to restict
@TargetInfo.@arch to valid sysemu targets at the schema level.

Cc: "Daniel P. Berrange" <berra...@redhat.com>
Cc: David Gibson <dgib...@redhat.com>
Cc: Eric Blake <ebl...@redhat.com>
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: Kashyap Chamarthy <kcham...@redhat.com>
Cc: Markus Armbruster <arm...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Thomas Huth <th...@redhat.com>
Signed-off-by: Laszlo Ersek <ler...@redhat.com>
---

Notes:
RFCv3:

- The patch is new in this version. [Markus]

 qapi/misc.json |  6 --
 arch_init.c| 18 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index 5636f4a14997..3b4fbc534089 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -5,6 +5,8 @@
 # = Miscellanea
 ##
 
+{ 'include': 'common.json' }
+
 ##
 # @qmp_capabilities:
 #
@@ -2449,12 +2451,12 @@
 #
 # Information describing the QEMU target.
 #
-# @arch: the target architecture (eg "x86_64", "i386", etc)
+# @arch: the target architecture
 #
 # Since: 1.2.0
 ##
 { 'struct': 'TargetInfo',
-  'data': { 'arch': 'str' } }
+  'data': { 'arch': 'SysEmuTarget' } }
 
 ##
 # @query-target:
diff --git a/arch_init.c b/arch_init.c
index 6ee07478bd11..4367f30291f8 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -29,6 +29,7 @@
 #include "hw/pci/pci.h"
 #include "hw/audio/soundhw.h"
 #include "qapi/qapi-commands-misc.h"
+#include "qapi/error.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "hw/acpi/acpi.h"
@@ -111,8 +112,23 @@ int xen_available(void)
 TargetInfo *qmp_query_target(Error **errp)
 {
 TargetInfo *info = g_malloc0(sizeof(*info));
+Error *local_err = NULL;
 
-info->arch = g_strdup(TARGET_NAME);
+/*
+ * The fallback enum value is irrelevant here (TARGET_NAME is a
+ * macro and can never be NULL), so simply pass zero. Also, the
+ * lookup should never fail -- if it fails, then @SysEmuTarget needs
+ * extending. Catch that with an assertion, but also handle it
+ * gracefully, in case someone adventurous disables assertions.
+ */
+info->arch = qapi_enum_parse(_lookup, TARGET_NAME, 0,
+ _err);
+g_assert(!local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+qapi_free_TargetInfo(info);
+return NULL;
+}
 
 return info;
 }
-- 
2.14.1.3.gb7cf6e02401b


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [qemu RFC v3 3/3] qapi: add "firmware.json"

[Laszlo Ersek]

Add a schema that describes the different uses and properties of virtual
machine firmware.

Each firmware executable installed on a host system should come with at
least one JSON file that conforms to this schema. Each file informs the
management applications about
- the firmware's properties and one possible use case / feature set,
- configuration bits that are required to run the firmware binary.

In addition, define rules for management apps for picking the highest
priority firmware JSON file when multiple such files match the search
criteria.

Cc: "Daniel P. Berrange" <berra...@redhat.com>
Cc: David Gibson <dgib...@redhat.com>
Cc: Eric Blake <ebl...@redhat.com>
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: Kashyap Chamarthy <kcham...@redhat.com>
Cc: Markus Armbruster <arm...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Thomas Huth <th...@redhat.com>
Signed-off-by: Laszlo Ersek <ler...@redhat.com>
---

Notes:
RFCv3:

- Previous version (RFCv2) was posted at:
  <20180417224054.26363-1-lersek@redhat.com">http://mid.mail-archive.com/20180417224054.26363-1-lersek@redhat.com>

- Trim the CC list a little.

- Wrap to a 72-char margin, for an easier reading. [Markus]

- Run spell-checker. [Markus]

- Rename @FirmwareType to @FirmwareOSInterface, to better express the
  concept. Update the enum constants accordingly (incl. documentation);
  in particular replace @slof with @openfirmware. Rename the @type field
  in @Firmware to @interface-type. [David, Gerd, Paolo, Thomas]

- Better yet, rename @interface-type to @interface-types (plural), and
  turn it into a list of @FirmwareOSInterface entries, to express an
  ordered compat list with multiple firmware<->OS interface types. [Dan,
  Eric, Gerd, Paolo]

- Drop @FirmwareArchitecture, and switch the type of
  @FirmwareTarget.@architecture to @SysEmuTarget (now defined in
  "common.json"). [Dan, Gerd, Markus]

- Switch elements of @FirmwareTarget.@machines from aliases ("pc",
  "q35", "virt") to concrete machine types, with glob patterns
  understood. [Dan, Gerd]

- Rename @secure-boot-enrolled-keys to @enrolled-keys, and drop the
  dependency on @secure-boot in its documentation. [Gerd]

- Describe the firmware JSON directories and the search rules to
  management apps, under the @Firmware element. Relatedly, remove the
  language on any concrete certificates under @enrolled-keys, as these
  can be customized through the overrides. Also refresh the commit
  message. [Dan, Gerd]

- Rename @pathname fields to @filename. [Markus]

- Don't replace "@executable.@filename" with "@executable.filename"
  (that is, keep the second @ sign), because removing the 2nd @ sign
  messes up the documentation ("filename" stops being type-set in
  monospace in the HTML, for example). [Markus]

- Rename @nvram_template to @nvram-template. [Thomas]

- Keep @x-check-firmware for a while longer. [Dan, Gerd, Markus, Paolo]

RFCv2:

- previous version (RFCv1) was posted at
  <20180407000117.25640-1-lersek@redhat.com">http://mid.mail-archive.com/20180407000117.25640-1-lersek@redhat.com>

- v2 is basically a rewrite from scratch, starting out with Dan's
  definitions from
  <20180410102033.GL5155@redhat.com">http://mid.mail-archive.com/20180410102033.GL5155@redhat.com> and
  <20180410110357.GP5155@redhat.com">http://mid.mail-archive.com/20180410110357.GP5155@redhat.com>,
  hopefully addressing others' feedback as well

RFCv1:

- Folks on the CC list, please try to see if the suggested schema is
  flexible enough to describe the virtual firmware(s) that you are
  familiar with. Thanks!

 Makefile  |   9 +
 Makefile.objs |   4 +
 qapi/firmware.json| 554 ++
 qapi/qapi-schema.json |   1 +
 qmp.c |   5 +
 .gitignore|   4 +
 6 files changed, 577 insertions(+)
 create mode 100644 qapi/firmware.json

diff --git a/Makefile b/Makefile
index d71dd5bea416..32034abe1583 100644
--- a/Makefile
+++ b/Makefile
@@ -97,6 +97,7 @@ GENERATED_FILES += qapi/qapi-types-block.h 
qapi/qapi-types-block.c
 GENERATED_FILES += qapi/qapi-types-char.h qapi/qapi-types-char.c
 GENERATED_FILES += qapi/qapi-types-common.h qapi/qapi-types-common.c
 GENERATED_FILES += qapi/qapi-types-crypto.h qapi/qapi-types-crypto.c
+GENERATED_FILES += qapi/qapi-types-firmware.h qapi/qapi-types-firmware.c
 GENERATED_FILES += qapi/qapi-types-introspect.h qapi/qapi-types-introspect.c
 GENERATED_FILES += qapi/qapi-types-migration.h qapi/qapi-types-migration.c
 GENERATED_FILES += qap

[libvirt] [qemu RFC v3 0/3] qapi: add "firmware.json"

[Laszlo Ersek]

This version seeks to address the RFCv2 feedback. Changes are noted per
patch.

Cc: "Daniel P. Berrange" <berra...@redhat.com>
Cc: David Gibson <dgib...@redhat.com>
Cc: Eric Blake <ebl...@redhat.com>
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: Kashyap Chamarthy <kcham...@redhat.com>
Cc: Markus Armbruster <arm...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Thomas Huth <th...@redhat.com>

Thanks,
Laszlo

Laszlo Ersek (3):
  qapi: add SysEmuTarget to "common.json"
  qapi: change the type of TargetInfo.arch from string to enum
SysEmuTarget
  qapi: add "firmware.json"

 Makefile  |   9 +
 Makefile.objs |   4 +
 qapi/common.json  |  19 ++
 qapi/firmware.json| 554 ++
 qapi/misc.json|   6 +-
 qapi/qapi-schema.json |   1 +
 arch_init.c   |  18 +-
 qmp.c |   5 +
 .gitignore|   4 +
 9 files changed, 617 insertions(+), 3 deletions(-)
 create mode 100644 qapi/firmware.json

-- 
2.14.1.3.gb7cf6e02401b

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [qemu RFC v3 1/3] qapi: add SysEmuTarget to "common.json"

[Laszlo Ersek]

We'll soon need an enumeration type that lists all the softmmu targets
that QEMU (the project) supports. Introduce @SysEmuTarget to
"common.json".

Cc: "Daniel P. Berrange" <berra...@redhat.com>
Cc: David Gibson <dgib...@redhat.com>
Cc: Eric Blake <ebl...@redhat.com>
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: Kashyap Chamarthy <kcham...@redhat.com>
Cc: Markus Armbruster <arm...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Thomas Huth <th...@redhat.com>
Signed-off-by: Laszlo Ersek <ler...@redhat.com>
---

Notes:
RFCv3:

- The patch is new in this version. [Dan, Markus]

- The original idea was to call the new enum @Target; however, @Target
  generates exactly the TARGET_AARCH64, TARGET_ALPHA, TARGET_ARM, ...
  enumeration constants that conflict with the poisoned preprocessing
  macros of the same names. Hence @SysEmuTarget -- it's more accurate
  anyway, since we want it to stand for system emulation targets.

- Also, we discussed defining the new type in either "common.json" or
  "misc.json". "misc.json" turned out to be a problem: "firmware.json"
  would then include "misc.json" for the new type's sake, but that
  inclusion would become the first appearance of "misc.json" -- within
  "firmware.json". That messed up the generated documentation. By adding
  the new type to "common.json", "misc.json" (see the 2nd patch) and
  "firmware.json" (see the 3rd patch) can both consume the new type
  without problems.

 qapi/common.json | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/qapi/common.json b/qapi/common.json
index d9b14dd429f3..1fd63172754a 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -126,3 +126,22 @@
 ##
 { 'enum': 'OffAutoPCIBAR',
   'data': [ 'off', 'auto', 'bar0', 'bar1', 'bar2', 'bar3', 'bar4', 'bar5' ] }
+
+##
+# @SysEmuTarget:
+#
+# The comprehensive enumeration of QEMU system emulation ("softmmu")
+# targets. Run "./configure --help" in the project root directory, and
+# look for the *-softmmu targets near the "--target-list" option. The
+# individual target constants are not documented here, for the time
+# being.
+#
+# Since: 2.13
+##
+{ 'enum' : 'SysEmuTarget',
+  'data' : [ 'aarch64', 'alpha', 'arm', 'cris', 'hppa', 'i386', 'lm32',
+ 'm68k', 'microblaze', 'microblazeel', 'mips', 'mips64',
+ 'mips64el', 'mipsel', 'moxie', 'nios2', 'or1k', 'ppc',
+ 'ppc64', 'ppcemb', 'riscv32', 'riscv64', 's390x', 'sh4',
+ 'sh4eb', 'sparc', 'sparc64', 'tricore', 'unicore32',
+ 'x86_64', 'xtensa', 'xtensaeb' ] }
-- 
2.14.1.3.gb7cf6e02401b


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/20/18 14:53, Markus Armbruster wrote:
> Laszlo Ersek <ler...@redhat.com> writes:

[snip]

>> The targets with softmmu are: aarch64, alpha, arm, cris, hppa, i386,
>> lm32, m68k, microblaze, microblazeel, mips, mips64, mips64el, mipsel,
>> moxie, nios2, or1k, ppc, ppc64, ppcemb, riscv32, riscv64, s390x, sh4,
>> sh4eb, sparc, sparc64, tricore, unicore32, x86_64, xtensa, xtensaeb.
>>
>> That is, at least the following constants in CpuInfoArch have no
>> corresponding (identical) mapping -- I'll state to the right of the
>> arrow the emulation targets which I know or presume to be associated
>> with the CpuInfoArch constant:
>> - x86 -> i386, x86_64
>> - sparc -> sparc, sparc64
>> - ppc -> ppc, ppc64, ppcemb
>> - mips -> mips, mips64, mips64el, mipsel
>> - s390 -> s390x
>> - riscv -> riscv32, riscv64
>>
>> The only constant that seems to have a 1:1 mapping is "tricore", but I
>> could perfectly well be thinking even that just because I have no clue
>> about "tricore".
>>
>> So, I don't think CpuInfoArch can be updated so that it both remains
>> compatible with current QMP clients, and serves "firmware.json". In the
>> firmware schema we don't just need CPU architecture, but actual emulator
>> names (and board / machine types).
> 
> The completely orthodox fix for CpuInfo would be:
> 
> * Keep @arch unchanged.  In particular, keep "other" for all targets
>   other than 'x86', 'sparc', 'ppc', 'mips', 'tricore'.
> 
> * Add a new member @target of new enum type Target, whose values match
>   configures idea of targets exactly.
> 
> * Make the new member the union tag.
> 
> It's too late for complete orthodoxy; we already changed @arch.
> 
> A common enum type Target makes sense anyway, I think.
> 
> Using it in CpuInfo as described above may make sense, too.  Could be
> left to a follow-up patch.
> 
>> I grepped 'qapi/*json' for the whole word "x86_64", and the only thing
>> that remotely matches is the @TargetInfo structure, in which the @arch
>> field is a string, coming with the example "x86_64". The example also
>> names "i386" separately.
> 
> Well spotted.
> 
> TargetInfo member @arch is set to TARGET_NAME, which matches configure's
> idea of the target.  If we add enum Target, we should change @arch's
> type from str to Target.
> 
>> So what might make sense is to introduce a separate enum in
>> "qapi/misc.json" with all the softmmu targets I listed above, and change
>> the type of @TargetInfo.@arch to that enum.
> 
> I arrived at this conclusion, too.
> 
>> In parallel,
>> qmp_query_target() would have to be updated to look up the enum value
>> matching TARGET_NAME. (I do think we can ignore linux-user etc emulators
>> for collecting the relevant arches here: @TargetInfo is only used in the
>> "query-target" QMP command, and Markus said above that QMP is only used
>> with system emulation.)
>>
>> Should I do this?
> 
> Yes, please.

OK, so here's my understanding:

(1) I'll introduce a new @Target enum in misc.json. I'll inherit /
include it in firmware.json. This is compatible with what Dan said.

(2) I'll change @TargetInfo.@arch to the new type. I believe this will
break the compilation of qmp_query_target(); I'll hack on that until it
builds again, with the lookup. :)

(3) Regarding the addition of @target to CpuInfo (accompanying @arch)
doesn't look hard; what *does* look hard is changing the union
discriminator from @arch to @target. @target has many more values, and I
would have to map all of them to the (fewer) @arch values that currently
do *not* select @CpuInfoOther. Here's an example:

- Both @i386 and @x86_64 from @target mean @x86 in @arch,
- because @x86 currently selects @CpuInfoX86, not @CpuInfoOther,
  both @i386 and @x86_64 must be assigned @CpuInfoX86.

This depends on the knowledge that "x86" actually *means* "i386 plus
x86_64", and I totally don't have that knowledge for the rest of the
arches / targets.

So, the modification of @CpuInfo I would indeed like to pass off to
someone else. (Well, if all the architecture maintainers follow up and
tell me what emulators exactly fall under the umbrella of each
individual @arch value, I can post the patch.) BTW... I wonder how
compatibility would be affected if we just added @target to @CpuInfo,
even without making it the new discriminator.

... Anyway, I think I've gotten a huge amount of useful and actionable
feedback; thanks everyone, let me work on RFCv3 and post it soon. :)

Thanks!
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/20/18 12:41, Gerd Hoffmann wrote:
 Since your schema is likely to end up just being a file in docs/specs,
> 
>>> I think it would be useful to have this as part of the schema.  Should
>>> be easy then to write up a small utility then which takes a json file as
>>> input and translates that into qemu command line options.
>>
>> Currently we have two distinct QAPI schemas, one covering the system
>> emulator for QMP and subset of CLI args, and one covering the guest
>> agent.
> 
> Yep, we can make it a third one, fine with me.
> But it likewise wouldn't end up in docs/ then.

Let's please focus on the schema itself for now, with the stupid little
test command included. Once the schema enjoys universal popularity, I'll
be glad to pass it on to QMP wizards -- because, if we ultimately wanted
to keep the schema code-enabled, then Markus's earlier suggestions apply
too, regarding QMP test cases. I'll admit I haven't dared look at those
yet (in particular because Paolo originally suggested to move this under
docs/interop). Looking at my todo list, I'd really like to focus on the
schema itself.

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/20/18 11:34, Daniel P. Berrangé wrote:
> On Fri, Apr 20, 2018 at 10:11:08AM +0200, Laszlo Ersek wrote:
>> On 04/19/18 11:12, Daniel P. Berrangé wrote:
>>> On Thu, Apr 19, 2018 at 10:39:32AM +0200, Laszlo Ersek wrote:
>>>> On 04/19/18 09:56, Daniel P. Berrangé wrote:
>>>>> On Thu, Apr 19, 2018 at 09:48:36AM +0200, Markus Armbruster wrote:
>>>>>> Laszlo Ersek <ler...@redhat.com> writes:
>>>>>>
>>>>>>> On 04/18/18 10:47, Markus Armbruster wrote:
>>>>>>>> Laszlo Ersek <ler...@redhat.com> writes:
>>>>>> Replacing CpuInfoArch by such an enum will change the discriminator
>>>>>> value from "other" to the real architecture, with the obvious
>>>>>> compatibility concerns.  But we've accepted similar changes twice
>>>>>> already: commit 9d0306dfdfb and commit 25fa194b7b1, both v2.12.0-rc0.
>>>>>>
>>>>>> "other" was a bad idea.  Hindsight 20/20.
>>>>>>
>>>>>> Getting rid of it in one go rather than piecemeal seems like the least
>>>>>> bad way out.  Too late for 2.12, though.  Eric, what do you think?
>>>>>
>>>>> Given the context in which this "other" value is used, I think it is
>>>>> reasonable to kill it and put a full arch list in there.
>>>>>
>>>>> No app is likely to be accessing the struct under "other" because it
>>>>> is just an empty placeholder.
>>>>
>>>> Commit 9d0306dfdfb added "s390" and "CpuInfoS390", which I guess had the
>>>> potential to confuse QMP clients that didn't expect "s390", but
>>>> otherwise it didn't mess with preexistent enum values / structures.
>>>
>>> NB, qemu-system-s390x would previously have returned "other" in
>>> this field, and now it returns "s390".  So while it didn't
>>> remove "other" from the list of things that could potentially
>>> exist, it did change what the s390x binary will actually report.
>>>
>>>> The same applies to commit 25fa194b7b1, just with "riscv" /
>>>> "CpuInfoRISCV" substituted.
>>>>
>>>> Removing "other" might confuse QMP clients that expect it, except
>>>> (according to Daniel) no such client exists, probably.
>>>
>>> When I say removing "other", I imply that we add an explicit arch
>>> for all those which we currently are missing. IOW, all qemu-system-XXX
>>> binaries which currently report "other" would change to report their
>>> respective "XXX" values.
>>>
>>> So in this way, it is exactly the same as what we did when we
>>> introduced "s390" as an option.
>>>
>>> The only difference is that once we have every binary reporting the
>>> correct arch, we can now also remove "other" from the schema itself
>>> as it will then be unused.
>>
>> Can we please translate this into more actionable items for me, because
>> I'm getting confused :)
>>
>> First, if I add "i386" and "x86_64" to the enum list, we'll have all
>> three of "i386", "x86_64" and "x86". Is that useful? How will that work?
> 
> Hmm, yes, on closer look this is a big mess as it is. We've been using
> generic terms for covering multiple architectures :-(  'x86' for both
> i386 and x86_64,  'sparc' for sparc and sparc64, etc. If we try to fix
> that we'll be entering a world of backcompat hurt :-(
> 
> Since your schema is likely to end up just being a file in docs/specs,
> rather than directly part of our existnig qapi schema, I suggest we just
> ignore whats there. Just define an arch enum in your spec which is right,
> and let someone else worry about fixing the mess

I can't tell you how much I love this idea. :)

Thanks!
Laszlo

> 
>> Second, assuming I add constants for the ~10 (?) softmmu arches, can I
>> still use @CpuInfoOther as the type for the corresponding new members in
>> @CpuInfo? What C code changes will be necessary?
> 
> Yes, we could still use the CpuInfoOther struct, since struct names are
> invisible to consumers, but as above, lets ignore the mess
> 
> Regards,
> Daniel
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/20/18 10:47, Paolo Bonzini wrote:
> On 20/04/2018 03:03, David Gibson wrote:
>>> This also implies I shouldn't add "openbios" separately, which was
>>> suggested earlier by Gerd -- according to
>>> , OpenBIOS is another
>>> implementation of OFW.
>>
>> Right.  Although I think OpenBIOS and SLOF support a disjoint set of
>> machines.  Openhackware which is (was?) used on some machines is yet
>> another (very partial) openfirmware implementation.
> 
> We can:
> 
> 1a) replace the architecture field with an ABI field (seems wrong to me)
> 
> 1b) add an "ABI" field (containing e.g. prep, spapr, etc.) to complement
> the architecture field
> 
> 2) we include directly a glob pattern for the QEMU machine types (also
> seems wrong).

Using globs in machine types was what Gerd and Daniel agreed upon, and I
was planning to do for v3. :(

> 3) just like we effectively moved the guest ABI to the features field,

No, we didn't -- while it's true that a single firmware image can
provide multiple guest ABIs, the implementation quality of those guest
ABIs is not identical, and there is a preference order between them. The
latest idea was to keep the @type field, but turn it into an ordered
list of enum constants. Whichever constant is near the top is the more
preferred ABI, and mgmt tools, when looking for "the" type of the
firware, would consider the topmost (first) element.

> we split the features into host-features and guest-features, and the
> host ABI (prep, spapr, etc.; but also OVMF's SMM requirement fits here
> for example) moves into host-features.  Again, just like 1a/1b, this can
> be replace or complement the architecture field, and I think I'd prefer
> the latter

I'd prefer if we could mostly stick with the structure of the schema as
seen in RFCv2 -- all the feedback until now implies that's possible,
with minor tweaks. Let me post an RFCv3 soon, so that we have a more
up-to-date basis for the discussion. (I've been waiting for some of the
architecture (emulation target) technicalities to clear up, but I guess
I had better postpone that now, because I see the discussion diverging.)

Thanks!
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/19/18 11:12, Daniel P. Berrangé wrote:
> On Thu, Apr 19, 2018 at 10:39:32AM +0200, Laszlo Ersek wrote:
>> On 04/19/18 09:56, Daniel P. Berrangé wrote:
>>> On Thu, Apr 19, 2018 at 09:48:36AM +0200, Markus Armbruster wrote:
>>>> Laszlo Ersek <ler...@redhat.com> writes:
>>>>
>>>>> On 04/18/18 10:47, Markus Armbruster wrote:
>>>>>> Laszlo Ersek <ler...@redhat.com> writes:
>>>> Replacing CpuInfoArch by such an enum will change the discriminator
>>>> value from "other" to the real architecture, with the obvious
>>>> compatibility concerns.  But we've accepted similar changes twice
>>>> already: commit 9d0306dfdfb and commit 25fa194b7b1, both v2.12.0-rc0.
>>>>
>>>> "other" was a bad idea.  Hindsight 20/20.
>>>>
>>>> Getting rid of it in one go rather than piecemeal seems like the least
>>>> bad way out.  Too late for 2.12, though.  Eric, what do you think?
>>>
>>> Given the context in which this "other" value is used, I think it is
>>> reasonable to kill it and put a full arch list in there.
>>>
>>> No app is likely to be accessing the struct under "other" because it
>>> is just an empty placeholder.
>>
>> Commit 9d0306dfdfb added "s390" and "CpuInfoS390", which I guess had the
>> potential to confuse QMP clients that didn't expect "s390", but
>> otherwise it didn't mess with preexistent enum values / structures.
> 
> NB, qemu-system-s390x would previously have returned "other" in
> this field, and now it returns "s390".  So while it didn't
> remove "other" from the list of things that could potentially
> exist, it did change what the s390x binary will actually report.
> 
>> The same applies to commit 25fa194b7b1, just with "riscv" /
>> "CpuInfoRISCV" substituted.
>>
>> Removing "other" might confuse QMP clients that expect it, except
>> (according to Daniel) no such client exists, probably.
> 
> When I say removing "other", I imply that we add an explicit arch
> for all those which we currently are missing. IOW, all qemu-system-XXX
> binaries which currently report "other" would change to report their
> respective "XXX" values.
> 
> So in this way, it is exactly the same as what we did when we
> introduced "s390" as an option.
> 
> The only difference is that once we have every binary reporting the
> correct arch, we can now also remove "other" from the schema itself
> as it will then be unused.

Can we please translate this into more actionable items for me, because
I'm getting confused :)

First, if I add "i386" and "x86_64" to the enum list, we'll have all
three of "i386", "x86_64" and "x86". Is that useful? How will that work?

Second, assuming I add constants for the ~10 (?) softmmu arches, can I
still use @CpuInfoOther as the type for the corresponding new members in
@CpuInfo? What C code changes will be necessary?

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/19/18 09:56, Daniel P. Berrangé wrote:
> On Thu, Apr 19, 2018 at 09:48:36AM +0200, Markus Armbruster wrote:
>> Laszlo Ersek <ler...@redhat.com> writes:
>>
>>> On 04/18/18 10:47, Markus Armbruster wrote:
>>>> Laszlo Ersek <ler...@redhat.com> writes:
>>>
>>>>> +##
>>>>> +# @FirmwareArchitecture:
>>>>> +#
>>>>> +# Defines the target architectures (emulator binaries) that firmware may
>>>>> +# execute on.
>>>>> +#
>>>>> +# @aarch64: The firmware can be executed by the qemu-system-aarch64 
>>>>> emulator.
>>>>> +#
>>>>> +# @arm: The firmware can be executed by the qemu-system-arm emulator.
>>>>> +#
>>>>> +# @i386: The firmware can be executed by the qemu-system-i386 emulator.
>>>>> +#
>>>>> +# @x86_64: The firmware can be executed by the qemu-system-x86_64 
>>>>> emulator.
>>>>> +#
>>>>> +# Since: 2.13
>>>>> +##
>>>>> +{ 'enum' : 'FirmwareArchitecture',
>>>>> +  'data' : [ 'aarch64', 'arm', 'i386', 'x86_64' ] }
>>>>
>>>> Partial dupe of CpuInfoArch from misc.json.  Neither covers all our
>>>> target architectures.  Should we have one that does in common.json
>>>> instead?
>>>
>>> If we had one there, I'd use it here :)
>>>
>>> For collecting the arch identifiers, is it OK to run "./configure
>>> --help", and look for the "*-softmmu" options under
>>> "--target-list=LIST"? Because that's what I need here; the qemu-system-*
>>> emulators.
>>
>> configure gets its list like this:
>>
>> default_target_list=""
>>
>> mak_wilds=""
>>
>> if [ "$softmmu" = "yes" ]; then
>> mak_wilds="${mak_wilds} $source_path/default-configs/*-softmmu.mak"
>> fi
>> if [ "$linux_user" = "yes" ]; then
>> mak_wilds="${mak_wilds} 
>> $source_path/default-configs/*-linux-user.mak"
>> fi
>> if [ "$bsd_user" = "yes" ]; then
>> mak_wilds="${mak_wilds} $source_path/default-configs/*-bsd-user.mak"
>> fi
>>
>> for config in $mak_wilds; do
>> default_target_list="${default_target_list} $(basename "$config" 
>> .mak)"
>> done
>>
>> Since we use QMP only in system emulation, a QAPI enum limited to the
>> system emulation targets makes sense.
>>
>> Replacing CpuInfoArch by such an enum will change the discriminator
>> value from "other" to the real architecture, with the obvious
>> compatibility concerns.  But we've accepted similar changes twice
>> already: commit 9d0306dfdfb and commit 25fa194b7b1, both v2.12.0-rc0.
>>
>> "other" was a bad idea.  Hindsight 20/20.
>>
>> Getting rid of it in one go rather than piecemeal seems like the least
>> bad way out.  Too late for 2.12, though.  Eric, what do you think?
> 
> Given the context in which this "other" value is used, I think it is
> reasonable to kill it and put a full arch list in there.
> 
> No app is likely to be accessing the struct under "other" because it
> is just an empty placeholder.

Commit 9d0306dfdfb added "s390" and "CpuInfoS390", which I guess had the
potential to confuse QMP clients that didn't expect "s390", but
otherwise it didn't mess with preexistent enum values / structures.

The same applies to commit 25fa194b7b1, just with "riscv" /
"CpuInfoRISCV" substituted.

Removing "other" might confuse QMP clients that expect it, except
(according to Daniel) no such client exists, probably.

However, replacing the current list of CpuInfoArch constants with the
system emulation target list would be more intrusive than all three of
the above. In particular there is no "x86" target; only i386 and x86_64
targets exist. For the firmware schema, this distinction is important,
but it would break QMP clients that expect "x86" (and such clients must
certainly exist).

The targets with softmmu are: aarch64, alpha, arm, cris, hppa, i386,
lm32, m68k, microblaze, microblazeel, mips, mips64, mips64el, mipsel,
moxie, nios2, or1k, ppc, ppc64, ppcemb, riscv32, riscv64, s390x, sh4,
sh4eb, sparc, sparc64, tricore, unicore32, x86_64, xtensa, xtensaeb.

That is, at least the following constants in CpuInfoArch have no
corresponding (identical) mapping -- I'll state to the rig

Re: [libvirt] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/19/18 02:09, David Gibson wrote:
> On Wed, 18 Apr 2018 10:32:06 +0200
> Laszlo Ersek <ler...@redhat.com> wrote:
> 
>> On 04/18/18 08:02, Gerd Hoffmann wrote:
>>  [...]  
>>  [...]  
>>>
>>> Looks good to me overall.
>>>  
>>>> +{ 'enum' : 'FirmwareType',
>>>> +  'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }  
>>>
>>> openbios missing.
>>>  
>>>> +{ 'enum' : 'FirmwareArchitecture',
>>>> +  'data' : [ 'aarch64', 'arm', 'i386', 'x86_64' ] }  
>>>
>>> ppc(64) missing (but you have slof above ;) ...
>>> s390 too.  
>>
>> I figured those would be contributed by people that actually use them,
>> as separate patches :) In fact I would rather prefer removing "slof" and
>> "uboot" from this initial version, because I have zero clue about them.
> 
> I've only been able to skim this discussion, so apologies if I've
> missed things.  I'm pretty unclear on the overall purpose of this, but
> in particular this FirmwareType field seems pretty weird.
> 
> Specifically the things in the list don't really seem comparable to
> each other: UEFI is a specified interface, BIOS is a de-facto
> interface.  So far so good.  But SLOF is a specific implementation of
> Open Firmware (of which we have a couple of other partial
> implementations used for other qemu platforms).

Thank you -- I will replace SLOF with "openfirmware".

This also implies I shouldn't add "openbios" separately, which was
suggested earlier by Gerd -- according to
<https://en.wikipedia.org/wiki/OpenBIOS>, OpenBIOS is another
implementation of OFW.

> U-Boot is somewhere in
> between the two, a specific implementation that defines a fair bunch of
> its own interfaces.

Right, this is about interfaces, so I'll keep "uboot".

Thank you!
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/18/18 17:27, Laszlo Ersek wrote:
> On 04/18/18 17:17, Eric Blake wrote:
>> On 04/18/2018 07:40 AM, Laszlo Ersek wrote:
>>
>>> Is it guaranteed that lists in JSON keep the order of the elements?
>>> Because, dictionaries definitely don't promise any ordering between the
>>> keys.
>>
>> Yes, JSON lists preserve order (and we have to explicitly document cases
>> where order within a list is not significant).
>>
> 
> Thanks.
> 
> After digesting the comments on @type for a while longer, from Dan, Gerd
> and Paolo, I think we *should* keep @type, but rather than having it be
> a simple enum, let's make it a list of enums, where order matters. Keep
> @features separate, and document that order does not matter there.
> 
> Here's why: my brain is crashing trying to come up with a
> human-parseable explanation why for *some* entries in @features, their
> relative order is important, and why for some others, it isn't.
> Discerning the subset for which order matters, from any specific
> grab-bag of @features, will be no fun for the human user.
> 
> Given that the "ordered features" idea was brought to life solely
> because a firmware can provide multiple interfaces (with a strong
> preference order between them), I guess we might as well be honest about
> that, and update @type accordingly.
> 
> Thoughts? :)

I mean I realize I just reinvented Paolo's point from
<c5e3f942-2c0e-9809-32ae-df97eaf2b7cd@redhat.com">http://mid.mail-archive.com/c5e3f942-2c0e-9809-32ae-df97eaf2b7cd@redhat.com>
:) I don't intend to claim the idea, I just needed some time to
understand it.

Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/18/18 17:17, Eric Blake wrote:
> On 04/18/2018 07:40 AM, Laszlo Ersek wrote:
> 
>> Is it guaranteed that lists in JSON keep the order of the elements?
>> Because, dictionaries definitely don't promise any ordering between the
>> keys.
> 
> Yes, JSON lists preserve order (and we have to explicitly document cases
> where order within a list is not significant).
> 

Thanks.

After digesting the comments on @type for a while longer, from Dan, Gerd
and Paolo, I think we *should* keep @type, but rather than having it be
a simple enum, let's make it a list of enums, where order matters. Keep
@features separate, and document that order does not matter there.

Here's why: my brain is crashing trying to come up with a
human-parseable explanation why for *some* entries in @features, their
relative order is important, and why for some others, it isn't.
Discerning the subset for which order matters, from any specific
grab-bag of @features, will be no fun for the human user.

Given that the "ordered features" idea was brought to life solely
because a firmware can provide multiple interfaces (with a strong
preference order between them), I guess we might as well be honest about
that, and update @type accordingly.

Thoughts? :)

Thanks,
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/18/18 16:06, Daniel P. Berrangé wrote:
> On Wed, Apr 18, 2018 at 04:03:03PM +0200, Laszlo Ersek wrote:

>> - Would you like me to capture the directory paths in the firmware.json
>>   file, or in the commit message for the patch?
> 
> Should be in whatever file ends up in the docs/specs directory eventually.
> 
>>
>> - Should we keep @secure-boot-enrolled-keys (or, as Gerd suggests,
>>   @enrolled-keys) in the schema, as a feature enum constant, but remove
>>   the documentation of the actual certificates? (I.e., say "an
>>   unspecified set of certificates has been enrolled and SB mode has been
>>   enabled".)
> 
> I think it is worth keeping the feature flag - we simply don't need
> to say /what/ keys.

All clear, thanks.
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/18/18 15:53, Daniel P. Berrangé wrote:
> On Wed, Apr 18, 2018 at 03:30:36PM +0200, Laszlo Ersek wrote:
>> On 04/18/18 15:06, Gerd Hoffmann wrote:
>>>>> Other question:  Do we want allow to specify which certs/keys are
>>>>> enrolled?  Which would probably mean to drop "enrolled-keys" from
>>>>> features and make it an optional string instead,
>>>>
>>>> Not an enum? "Microsoft" below should be an enum constant, shouldn't it?
>>>
>>> I don't think so.  If we want allow other certificate providers (not
>>> sure it makes sense as all physical hardware actually runs with the
>>> microsoft certificates), then we don't want a fixed list here.  So any
>>> CA can be listed, be it microsoft, redhat, canonical, verisign or
>>> kraxel.org ;)
>>
>> I agree (obviously), but then, at what detail do we stop?
>>
>> Because, if we go for full flexibility, then we should formalize:
>> - the certificate that goes into PK,
>> - the list of certificates that go into KEK,
>> - the list of certificates that go into db,
>>
>> and we should likely formalize "certificate" itself as the following pair:
>> - human-readable description (possibly the Common Name of the Subject),
>> - SHA256 digest (fingerprint) of the certificate.
>>
>> I do think this would totally be overkill, but I don't know where to
>> draw the line
>> - between the currently proposed @enrolled-keys (or similar enums that
>>   stand for well-defined "constellations")
>> - and the full details as described above.
>>
>> A simple string like "Microsoft" doesn't seem to me helpful for either
>> the user or management software -- the former won't know what
>> "Microsoft" stands for, and the latter won't want to look for free-form
>> strings. (Same issue as with @tags vs. @features.)
> 
> Ignoring secboot cert name for a minute, ultimately no matter what we do
> in terms of metadata there is always going to be the possibility that
> many firmware images all match the criteria libvirt is searching on.
> 
> Apps thus need rules to pick one of the many matches, and users need the
> ability to override distro defaults. We can achieve that as follows:
> 
> Recommend that firmware files are created with a double-digit prefix
> eg50-ovmf.json  50-seabios-256k.json, etc, etc, so they can be
> sorted in predictable order
> 
> Second, we should define three well known directory locations
> 
>  - /usr/share/qemu/firmware  (used by distros packages)
>  - /etc/qemu/firmware  (exclusively for sysadmin local additions)
>  - $HOME/.config/qemu/firmware (exclusively for per-user local additions)
> 
> Apps like libvirt should build list of files from all three locations,
> then sort by filename.  If a local directory has a file with same
> name as the distro directory, then it should replace the distro provided
> file. A zero length file should be simply hide the distro provided file
> 
> So for example, distro ships
> 
>/usr/share/qemu/firmware/50-ovmf.json
>/usr/share/qemu/firmware/50-seabios-256k.json
> 
> The sysadmin can now prevent the default ovmf being used at all with
> 
>   $ touch /etc/qemu/firmware/50-ovmf.json
> 
> The sysadmin can replace/alter the distro default ovmf with
> 
>   $ vim /etc/qemu/firmware/50-ovmf.json
> 
> Or they can provide a parallel ovmf with higher priority
> 
>   $ vim /etc/qemu/firmware/10-ovmf.json
> 
> Or they can provide a parallel ovmf with lower priority
> 
>   $ vim /etc/qemu/firmware/99-ovmf.json
> 
> $HOME/.config/qemu/firmware would take prior over /etc/qemu and
> /usr/share/qemu. 
> 
> 
> Getting back to the question of many ovmf images with various different
> secboot keys. The distro can now provide many ovmf images each with
> different keys, if desired and determine which one is picked by default.
> 
> The end user can provide their over ovmf with personal keys that replaces
> the distro microsoft enrolled one entirely, etc.
> 
> IOW,  don't think we need to record which certs are use for secboot in
> the JSON metadata. Just lets overrides & ordering take care of it.

OK, thank you. Three more questions:

- Would you like me to capture the directory paths in the firmware.json
  file, or in the commit message for the patch?

- Should we keep @secure-boot-enrolled-keys (or, as Gerd suggests,
  @enrolled-keys) in the schema, as a feature enum constant, but remove
  the documentation of the actual certificates? (I.e., say "an
  unspecified set of certificates has been enrolled and SB mode has been
  enabled".)

- Or else, should we drop the feature flag that stands for enrollment
  completely?

Thanks,
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/18/18 15:06, Gerd Hoffmann wrote:
>>> Other question:  Do we want allow to specify which certs/keys are
>>> enrolled?  Which would probably mean to drop "enrolled-keys" from
>>> features and make it an optional string instead,
>>
>> Not an enum? "Microsoft" below should be an enum constant, shouldn't it?
> 
> I don't think so.  If we want allow other certificate providers (not
> sure it makes sense as all physical hardware actually runs with the
> microsoft certificates), then we don't want a fixed list here.  So any
> CA can be listed, be it microsoft, redhat, canonical, verisign or
> kraxel.org ;)

I agree (obviously), but then, at what detail do we stop?

Because, if we go for full flexibility, then we should formalize:
- the certificate that goes into PK,
- the list of certificates that go into KEK,
- the list of certificates that go into db,

and we should likely formalize "certificate" itself as the following pair:
- human-readable description (possibly the Common Name of the Subject),
- SHA256 digest (fingerprint) of the certificate.

I do think this would totally be overkill, but I don't know where to
draw the line
- between the currently proposed @enrolled-keys (or similar enums that
  stand for well-defined "constellations")
- and the full details as described above.

A simple string like "Microsoft" doesn't seem to me helpful for either
the user or management software -- the former won't know what
"Microsoft" stands for, and the latter won't want to look for free-form
strings. (Same issue as with @tags vs. @features.)

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/18/18 14:32, Daniel P. Berrangé wrote:
> On Wed, Apr 18, 2018 at 02:30:02PM +0200, Gerd Hoffmann wrote:
>>   Hi,
>>
>>> So now the question is should this string be declared to be
>>> glob format, or regex format. Regex is more flexible, but
>>> regex syntax is ill-defined because every regex engine is
>>> slightly different.
>>
>> I'd go for glob.  It's good enough for the version number and
>> most likely we don't need anything else.
>>
>> And the machine types are a list, so there is still the option
>> to just have multiple entries in case it turns out we need
>> something glob can't cover with a single entry.
> 
> Ok, works for me.

Me too, thanks.
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/18/18 14:23, Gerd Hoffmann wrote:
>   Hi,
> 
>> Hmmm, I'm not sure I agree. One use case is that you want a domain
>> config in which well-known OS-es, signed by the MS UEFI certs, just boot
>> with SB enabled. (Some of our internal folks really want this.)
>>
>> Another use case is that you want a domain in which SB *can* be enabled,
>> but your installer is signed with a different certificate chain (or it
>> is unsigned), and with *just* the MS certs enrolled, it wouldn't boot at
>> all. So you want the SB *feature*, but definitely not the initial
>> enrollment / SB *operational mode*.
> 
> So "secure-boot-enrolled-keys" also has SB turned on.

Yes.

>> For me to understand you better, are you suggesting merely that I:
>>
>> - rename @secure-boot-enrolled-keys to @enrolled-keys, and
>>
>> - drop the reference to @secure-boot from the end of the @enrolled-keys
>>   documentation paragraph? (Namely, "@secure-boot-enrolled-keys is only
>>   valid if the firmware also supports @secure-boot").
> 
> Yes.  So "secure-boot" specifies "firmware binary supports secure-boot"
> and "enrolled-keys" specifies "firmware nvram template has keys enrolled
> (and SB enabled).

OK.

> Other question:  Do we want allow to specify which certs/keys are
> enrolled?  Which would probably mean to drop "enrolled-keys" from
> features and make it an optional string instead,

Not an enum? "Microsoft" below should be an enum constant, shouldn't it?

> then specify
> "'enrolled-keys' : 'Microsoft'" in the json file.

If this is really necessary (up to Dan :) ), I'm down with it.

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/18/18 14:03, Daniel P. Berrangé wrote:
> On Wed, Apr 18, 2018 at 01:52:19PM +0200, Laszlo Ersek wrote:
>> On 04/18/18 11:43, Paolo Bonzini wrote:
>>> On 18/04/2018 00:40, Laszlo Ersek wrote:
>>>> +#
>>>> +# Lists firmware types commonly used with QEMU virtual machines.
>>>> +#
>>>> +# @bios: The firmware was built from the SeaBIOS project.
>>>> +#
>>>> +# @slof: The firmware was built from the Slimline Open Firmware project.
>>>> +#
>>>> +# @uboot: The firmware was built from the U-Boot project.
>>>> +#
>>>> +# @uefi: The firmware was built from the edk2 (EFI Development Kit II) 
>>>> project.
>>>> +#
>>>> +# Since: 2.13
>>>> +##
>>>> +{ 'enum' : 'FirmwareType',
>>>> +  'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }
>>>
>>> A very basic question (so not likely a suggestion for change).  Why
>>> wouldn't these be features too?  For example a UEFI with CSM could
>>> expose both uefi and bios, a u-boot with UEFI support could expose both
>>> uboot and uefi, etc.
>>
>> Good point. I considered "type" to be a given, from the initial
>> brainstorming, but if Dan is OK with your suggestion, I can turn these
>> into features as well.
>>
>>> Perhaps there are two dimensions, the FirmwareType tells you how to
>>> configure it and the FirmwareFeature tells you the APIs it exposes to
>>> the guest?
>>
>> I don't know enough firmware types to answer this :) I believe I agree
>> about the FirmwareFeature statement (if we also include "platform
>> requirements" there), but I have no clue about any generalizations for
>> firmware configuration.
> 
> IIUC Paolo is basically suggesting
> 
>{
>"description": "OVMF firmware"
>"type": "uefi",
>"features": [
>   "uefi",
> "bios"
>],
>}
> 
> where 'bios' is only listed if CSM is enabled in the OVMF build. I tend
> to think that is redundant and we could just do
> 
> 
>{
>"description": "OVMF firmware"
>"features": [
>   "uefi",
> "bios"
>],
>}

Actually, this is how I interpreted Paolo's idea at once. I agree the
"type" member can be dropped.

> 
> And declare the order of "features" list is significant. ie
> 
>"features": [
>   "uefi",
> "bios"
>],
> 
> 
> means a UEFI firmware which has back compat for BIOS (ie OVMF with CSM)
> while
> 
>"features": [
> "bios"
>   "uefi",
>],
> 
> means a traditional BIOS firmware with compat for UEFI (thinking uboot
> being able to include uefi support in this case)

Is it guaranteed that lists in JSON keep the order of the elements?
Because, dictionaries definitely don't promise any ordering between the
keys.

Thanks,
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/18/18 11:43, Paolo Bonzini wrote:
> On 18/04/2018 00:40, Laszlo Ersek wrote:
>> +#
>> +# Lists firmware types commonly used with QEMU virtual machines.
>> +#
>> +# @bios: The firmware was built from the SeaBIOS project.
>> +#
>> +# @slof: The firmware was built from the Slimline Open Firmware project.
>> +#
>> +# @uboot: The firmware was built from the U-Boot project.
>> +#
>> +# @uefi: The firmware was built from the edk2 (EFI Development Kit II) 
>> project.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'enum' : 'FirmwareType',
>> +  'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }
> 
> A very basic question (so not likely a suggestion for change).  Why
> wouldn't these be features too?  For example a UEFI with CSM could
> expose both uefi and bios, a u-boot with UEFI support could expose both
> uboot and uefi, etc.

Good point. I considered "type" to be a given, from the initial
brainstorming, but if Dan is OK with your suggestion, I can turn these
into features as well.

> Perhaps there are two dimensions, the FirmwareType tells you how to
> configure it and the FirmwareFeature tells you the APIs it exposes to
> the guest?

I don't know enough firmware types to answer this :) I believe I agree
about the FirmwareFeature statement (if we also include "platform
requirements" there), but I have no clue about any generalizations for
firmware configuration.

Thanks,
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/18/18 11:04, Gerd Hoffmann wrote:
>> This surfaced in the RFCv1 discussion, but Daniel suggested ignoring
>> version numbers:
>>
>> 20180410093412.GI5155@redhat.com">http://mid.mail-archive.com/20180410093412.GI5155@redhat.com
>>
>> On 04/10/18 11:34, Daniel P. Berrangé wrote:
>>> IMHO it would be valid to just keep life simple and only record the
>>> base machine type name that can use the firmware ie "pc", "q35", and
>>> ignore the fact that in some cases the firmware might require a
>>> specific version of the machine type.
> 
> IIRC this bit referes to the fact that SMM requires qemu >= 2.x (don't
> remember which x) to work.  So smm-enabled edk2 would just say
> "pc-q35-*" instead of trying to specifying a version range somehow.

OK. I'm fine either way; Dan, can you please confirm you are OK with the
suggested wildcard format? (I.e., we still shouldn't include actual
version numbers in the supported machtypes list, but we should be more
specific than just "pc" and "q35" -- if the machine type is versioned,
use an asterisk for covering the version number.)

>> Continuing:
>>
>> On 04/18/18 08:02, Gerd Hoffmann wrote:
 +# @secure-boot: The firmware implements the software interfaces for UEFI 
 Secure
 +#   Boot, as defined in the UEFI specification. Note that 
 without
 +#   @requires-smm, guest code running with kernel privileges 
 can
 +#   undermine the security of Secure Boot.
 +#
 +# @secure-boot-enrolled-keys: The variable store (NVRAM) template 
 associated
>>>
>>> I think "enrolled-keys" should better be a separate feature.
>>
>> It's not possible from the edk2 side; without -D SECURE_BOOT_ENABLE, the
>> SB-related variables (SetupMode, PK, KEK, ...) don't work at all.
> 
> Sure.  The firmware builds will advertise both "secure-boot" and
> "enrolled-keys" features to specify that.
> 
> But I think it should be enough to check for "secure-boot" feature to
> figure whenever a given firmware build supports secure boot, not both
> "secure-boot" and "secure-boot-plus-something-else".

Hmmm, I'm not sure I agree. One use case is that you want a domain
config in which well-known OS-es, signed by the MS UEFI certs, just boot
with SB enabled. (Some of our internal folks really want this.)

Another use case is that you want a domain in which SB *can* be enabled,
but your installer is signed with a different certificate chain (or it
is unsigned), and with *just* the MS certs enrolled, it wouldn't boot at
all. So you want the SB *feature*, but definitely not the initial
enrollment / SB *operational mode*.

For me to understand you better, are you suggesting merely that I:

- rename @secure-boot-enrolled-keys to @enrolled-keys, and

- drop the reference to @secure-boot from the end of the @enrolled-keys
  documentation paragraph? (Namely, "@secure-boot-enrolled-keys is only
  valid if the firmware also supports @secure-boot").

Or are you suggesting something more?

Thanks!
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list