[PATCH kvmtool 0/3] Change what --nodefaults does and a revert

[PATCH kvmtool 0/3] Change what --nodefaults does and a revert

[Alexandru Elisei]

The first two patches revert "virtio-net: Don't print the compat warning
for the default device" because using --network mode=none disables the
device and lets the user know that it can use that to disable the default
virtio-net device. I don't think the changes are controversial.

And the last patch is there to get the conversation going about changing
what --nodefaults does. Details in the patch.

Alexandru Elisei (3):
  Revert "virtio-net: Don't print the compat warning for the default
    device"
  builtin-run: Document mode=none for -n/--network
  builtin-run: Have --nodefaults disable the default virtio-net device

 builtin-run.c |  6 +++---
 virtio/net.c  | 11 ++++++-----
 2 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.42.0

[PATCH kvmtool 1/3] Revert "virtio-net: Don't print the compat warning for the default device"

[Alexandru Elisei]

This reverts commit 15757e8e6441d83757c39046a6cdd3e4d74200ce.

Turns out there's a way to disable the default virtio-net device: pass
--network mode=none when running a VM.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 virtio/net.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/virtio/net.c b/virtio/net.c
index 77f7c9a7a788..f09dd0a48b53 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -847,7 +847,7 @@ done:
 	return 0;
 }
 
-static int virtio_net__init_one(struct virtio_net_params *params, bool suppress_compat)
+static int virtio_net__init_one(struct virtio_net_params *params)
 {
 	enum virtio_trans trans = params->kvm->cfg.virtio_transport;
 	struct net_dev *ndev;
@@ -913,7 +913,7 @@ static int virtio_net__init_one(struct virtio_net_params *params, bool suppress_
 	if (params->vhost)
 		virtio_net__vhost_init(params->kvm, ndev);
 
-	if (compat_id == -1 && !suppress_compat)
+	if (compat_id == -1)
 		compat_id = virtio_compat_add_message("virtio-net", "CONFIG_VIRTIO_NET");
 
 	return 0;
@@ -925,7 +925,7 @@ int virtio_net__init(struct kvm *kvm)
 
 	for (i = 0; i < kvm->cfg.num_net_devices; i++) {
 		kvm->cfg.net_params[i].kvm = kvm;
-		r = virtio_net__init_one(&kvm->cfg.net_params[i], false);
+		r = virtio_net__init_one(&kvm->cfg.net_params[i]);
 		if (r < 0)
 			goto cleanup;
 	}
@@ -943,7 +943,7 @@ int virtio_net__init(struct kvm *kvm)
 		str_to_mac(kvm->cfg.guest_mac, net_params.guest_mac);
 		str_to_mac(kvm->cfg.host_mac, net_params.host_mac);
 
-		r = virtio_net__init_one(&net_params, true);
+		r = virtio_net__init_one(&net_params);
 		if (r < 0)
 			goto cleanup;
 	}
-- 
2.42.0

[PATCH kvmtool 2/3] builtin-run: Document mode=none for -n/--network

[Alexandru Elisei]

It can be useful to disable all network devices, for example, to remove the
compat warning for the default network device when the guest does not
initialize it. This can be done by passing mode=none to the --network
command line option, but without in-depth knowledge of the code, there is
no way for the user to know this. Update the help message for -n/--network
to explain what mode=none does.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 builtin-run.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin-run.c b/builtin-run.c
index 21373d41edd6..c26184ea7fc0 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -252,7 +252,8 @@ static int loglevel_parser(const struct option *opt, const char *arg, int unset)
 									\
 	OPT_GROUP("Networking options:"),				\
 	OPT_CALLBACK_DEFAULT('n', "network", NULL, "network params",	\
-		     "Create a new guest NIC",				\
+		     "Create a new guest NIC. Pass mode=none to disable"\
+		     " all network devices",				\
 		     netdev_parser, NULL, kvm),				\
 	OPT_BOOLEAN('\0', "no-dhcp", &(cfg)->no_dhcp, "Disable kernel"	\
 			" DHCP in rootfs mode"),			\
-- 
2.42.0

[PATCH RFC kvmtool 3/3] builtin-run: Have --nodefaults disable the default virtio-net device

[Alexandru Elisei]

Knowing which configuration options can only be disabled via --nodefaults
requires the user to have knowlegde of the code. Asking that from someone
who just wants to run a VM isn't completely fair, so just change what
--nodefaults does to disable all the configuration that kvmtool does
without being explicitely instructed to do so.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---

I'm not 100% sure about this one, when first looking at this it was
surprising to me that --nodefaults doesn't disable all the defaults (and I
was the one that added the command line parameter!), but I don't if
we should be changing the semantics of a command line parameter. So this is
more for people to talk it over.

Also, if we're changing what --nodefaults does, did I miss other default
behaviour that the option should be disabling?

 builtin-run.c | 3 +--
 virtio/net.c  | 3 ++-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-run.c b/builtin-run.c
index c26184ea7fc0..50f1ae02f8f9 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -207,8 +207,7 @@ static int loglevel_parser(const struct option *opt, const char *arg, int unset)
 	OPT_BOOLEAN('\0', "rng", &(cfg)->virtio_rng, "Enable virtio"	\
 			" Random Number Generator"),			\
 	OPT_BOOLEAN('\0', "nodefaults", &(cfg)->nodefaults, "Disable"   \
-			" implicit configuration that cannot be"	\
-			" disabled otherwise"),				\
+			" implicit configuration options"),		\
 	OPT_CALLBACK('\0', "9p", NULL, "dir_to_share,tag_name",		\
 		     "Enable virtio 9p to share files between host and"	\
 		     " guest", virtio_9p_rootdir_parser, kvm),		\
diff --git a/virtio/net.c b/virtio/net.c
index f09dd0a48b53..651344bd7710 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -930,7 +930,8 @@ int virtio_net__init(struct kvm *kvm)
 			goto cleanup;
 	}
 
-	if (kvm->cfg.num_net_devices == 0 && kvm->cfg.no_net == 0) {
+	if (kvm->cfg.num_net_devices == 0 && kvm->cfg.no_net == 0 &&
+	    !kvm->cfg.nodefaults) {
 		static struct virtio_net_params net_params;
 
 		net_params = (struct virtio_net_params) {
-- 
2.42.0

Re: [PATCH kvmtool 0/3] Change what --nodefaults does and a revert

[Will Deacon]

On Thu, 7 Sep 2023 18:16:52 +0100, Alexandru Elisei wrote:
> The first two patches revert "virtio-net: Don't print the compat warning
> for the default device" because using --network mode=none disables the
> device and lets the user know that it can use that to disable the default
> virtio-net device. I don't think the changes are controversial.
> 
> And the last patch is there to get the conversation going about changing
> what --nodefaults does. Details in the patch.
> 
> [...]

Applied first two to kvmtool (master), thanks!

[1/3] Revert "virtio-net: Don't print the compat warning for the default device"
      https://git.kernel.org/will/kvmtool/c/4498eb7400c6
[2/3] builtin-run: Document mode=none for -n/--network
      https://git.kernel.org/will/kvmtool/c/c7b7a542cdcd

I'm also not sure about the final RFC patch:

[3/3] builtin-run: Have --nodefaults disable the default virtio-net device

so it would be great to hear if anybody else has an opinion on that. IIRC,
we introduced this for some EFI work, so perhaps those folks might have
an opinion?

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

Re: [PATCH kvmtool 0/3] Change what --nodefaults does and a revert

[Alexandru Elisei]

Hi Will,

Sorry for the late reply, was on holiday.

On Mon, Sep 18, 2023 at 12:05:14PM +0100, Will Deacon wrote:
> On Thu, 7 Sep 2023 18:16:52 +0100, Alexandru Elisei wrote:
> > The first two patches revert "virtio-net: Don't print the compat warning
> > for the default device" because using --network mode=none disables the
> > device and lets the user know that it can use that to disable the default
> > virtio-net device. I don't think the changes are controversial.
> > 
> > And the last patch is there to get the conversation going about changing
> > what --nodefaults does. Details in the patch.
> > 
> > [...]
> 
> Applied first two to kvmtool (master), thanks!
> 
> [1/3] Revert "virtio-net: Don't print the compat warning for the default device"
>       https://git.kernel.org/will/kvmtool/c/4498eb7400c6
> [2/3] builtin-run: Document mode=none for -n/--network
>       https://git.kernel.org/will/kvmtool/c/c7b7a542cdcd
> 
> I'm also not sure about the final RFC patch:
> 
> [3/3] builtin-run: Have --nodefaults disable the default virtio-net device
> 
> so it would be great to hear if anybody else has an opinion on that. IIRC,
> we introduced this for some EFI work, so perhaps those folks might have
> an opinion?

It was actually introduced to allow kvmtool to load a kvm-unit-tests test
file using --kernel instead of --firmware [1].

The user can specify parameters for a test using two methods: using the
kernel command line (for selecting a specific test from a test file with
multiple tests, for example) and from an initrd (for environment variables,
in a key=value format). --firmware cannot load an initrd, so the only
option is to use --kernel. But kvmtool mangles the kernel command line by
prepending several kernel options, which breaks kvm-unit-tests' command
line parser. Until --defaults there was no way to disable this behaviour.

To the point of running kvm-unit-tests, it makes no functional difference
if a test is run with --nodefaults --network mode=none or with --nodefaults
only. It's just that I think that --nodefaults disabling the default
configuration options is slightly more intuitive, but I'm not sure the
effort of changing the semantics is worth it (need to inspect all the code
that sets the default configuration options, then possibly adding new
kvmtool command line parameters to bring back those options if there's no
other way of changing them - I suspect this can get rather tedious).

[1] https://lore.kernel.org/all/20210923144505.60776-1-alexandru.elisei@arm.com/

Thanks,
Alex

> 
> Cheers,
> -- 
> Will
> 
> https://fixes.arm64.dev
> https://next.arm64.dev
> https://will.arm64.dev

Re: [PATCH kvmtool 0/3] Change what --nodefaults does and a revert

[Suzuki K Poulose]

On 11/10/2023 15:56, Alexandru Elisei wrote:
> Hi Will,
> 
> Sorry for the late reply, was on holiday.
> 
> On Mon, Sep 18, 2023 at 12:05:14PM +0100, Will Deacon wrote:
>> On Thu, 7 Sep 2023 18:16:52 +0100, Alexandru Elisei wrote:
>>> The first two patches revert "virtio-net: Don't print the compat warning
>>> for the default device" because using --network mode=none disables the
>>> device and lets the user know that it can use that to disable the default
>>> virtio-net device. I don't think the changes are controversial.
>>>
>>> And the last patch is there to get the conversation going about changing
>>> what --nodefaults does. Details in the patch.
>>>
>>> [...]
>>
>> Applied first two to kvmtool (master), thanks!
>>
>> [1/3] Revert "virtio-net: Don't print the compat warning for the default device"
>>        https://git.kernel.org/will/kvmtool/c/4498eb7400c6
>> [2/3] builtin-run: Document mode=none for -n/--network
>>        https://git.kernel.org/will/kvmtool/c/c7b7a542cdcd
>>
>> I'm also not sure about the final RFC patch:
>>
>> [3/3] builtin-run: Have --nodefaults disable the default virtio-net device
>>
>> so it would be great to hear if anybody else has an opinion on that. IIRC,
>> we introduced this for some EFI work, so perhaps those folks might have
>> an opinion?
> 
> It was actually introduced to allow kvmtool to load a kvm-unit-tests test
> file using --kernel instead of --firmware [1].
> 
> The user can specify parameters for a test using two methods: using the
> kernel command line (for selecting a specific test from a test file with
> multiple tests, for example) and from an initrd (for environment variables,
> in a key=value format). --firmware cannot load an initrd, so the only
> option is to use --kernel. But kvmtool mangles the kernel command line by
> prepending several kernel options, which breaks kvm-unit-tests' command
> line parser. Until --defaults there was no way to disable this behaviour.
> 
> To the point of running kvm-unit-tests, it makes no functional difference
> if a test is run with --nodefaults --network mode=none or with --nodefaults
> only. It's just that I think that --nodefaults disabling the default
> configuration options is slightly more intuitive, but I'm not sure the
> effort of changing the semantics is worth it (need to inspect all the code
> that sets the default configuration options, then possibly adding new
> kvmtool command line parameters to bring back those options if there's no
> other way of changing them - I suspect this can get rather tedious).

I personally prefer "--nodefaults" doing what it says. i.e., kvmtool
not adding *anything* by default on its own.

Suzuki


> 
> [1] https://lore.kernel.org/all/20210923144505.60776-1-alexandru.elisei@arm.com/
> 
> Thanks,
> Alex
> 
>>
>> Cheers,
>> -- 
>> Will
>>
>> https://fixes.arm64.dev
>> https://next.arm64.dev
>> https://will.arm64.dev