On Mon, May 15, 2023 at 05:36:00PM +0530, Viresh Kumar wrote:
On 12-05-23, 11:43, Anthony PERARD wrote:
On Thu, May 11, 2023 at 01:20:43PM +0530, Viresh Kumar wrote:
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index 24ac92718288..0405f6efe62a 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -1619,6 +1619,18 @@ hexadecimal format, without the "0x" prefix and all in lower case, like Specifies the transport mechanism for the Virtio device, only "mmio" is supported for now. +=item B<grant_usage=STRING>
+Specifies the grant usage details for the Virtio device. This can be set to +following values:
+- "default": The default grant setting will be used, enable grants if
- backend-domid != 0.
I don't think this "default" setting is useful. We could just describe what the default is when "grant_usage" setting is missing from the configuration.
This is what I suggested earlier [1], maybe I misunderstood what Juergen said.
To me, as a user of any program, setting default to a configuration option is when we don't select a configuration option. Like when starting a program for the first time and let it set things up based on the environment if that make senses. But I guess sometime when there's multiple choice, we can select default.
Anyway, I've looked in the xl.cfg man page and there's already plenty of example where "default" is an option, so I guess it doesn't really hurt to have the option to choose to not choose. You still need to write in the man page that "default" is the default option, as in the absence of the option in the configuration the default option will be used (unless you managed somehow to make the option mandatory, but is they a reason for that?).
In any case, there's going to be a 3-state option between xl and libxl which are going to be default, false, true. It doesn't matter whether a user can write default or not.
+- "enabled": The grants are always enabled.
+- "disabled": The grants are always disabled.
if ((virtio->grant_usage == LIBXL_VIRTIO_GRANT_USAGE_ENABLED) ||((virtio->grant_usage == LIBXL_VIRTIO_GRANT_USAGE_DEFAULT) &&(virtio->backend_domid != LIBXL_TOOLSTACK_DOMID))) {I think libxl can select what the default value should be replace with before we start to setup the guest. There's a *_setdefault() phase were we set the correct value when a configuration value hasn't been set and thus a default value is used. I think this can be done in libxl__device_virtio_setdefault(). After that, virtio->grant_usage will be true or false, and that's the value that should be given to the virtio backend via xenstore.
I am not great with Xen's flow of stuff and so would like to clarify a few things here since I am confused.
In my case, parse_virtio_config() gets called first followed by libxl__prepare_dtb(), where I need to use the "grant_usage" field. Later libxl__device_virtio_setdefault() gets called, anything done there isn't of much use in my case I guess.
:-(, I feel like something is missing. I would think that libxl__prepare_dtb() would be called after any _setdefault() function. Maybe something isn't calling setdefault for virtio devices soon enough in libxl.
Setting the default value of grant_usage in libxl__device_virtio_setdefault() doesn't work for me (since libxl__prepare_dtb() is already called), and I need to set this in parse_virtio_config() only.
I don't think that `xl` should set any default, that would be better done in libxl. libxl could be used by another program, such as `libvirt`.
Currently, virtio->backend_domid is getting set via libxl__resolve_domid() in libxl__device_virtio_setdefault(), which is too late for me, but is working fine, accidentally I think, since the default value of the field is 0, which is same as domain id in my case. I would like to understand though how it works for Disk device for Oleksandr, since they must also face similar issues. I must be doing something wrong here :)
No, I think something is missing for virtio devices.
For disk, there's code in initiate_domain_create() which call libxl__disk_devtype.set_default() for every disk, and this happen before libxl__prepare_dtb(). I don't know how other device types are doing this defaulting, I need to search.
There's also a special case for nic, a call of libxl__device_nic_set_devids() does call set_default for nics. Otherwise, I think set_default() is called whenever something calls add().
So, for virtio devices in libxl, I think we will also want to call set_default() early. Add a call to libxl__virtio_devtype.set_default() in libxl__domain_config_setdefault() similar to the one done for disk. (For disk, at the moment it is done in initiate_domain_create() but let's use the new libxl__domain_config_setdefault() function for virtio.)
This means that libxl__device_virtio_setdefault() would be called twice for each device, but that shouldn't be an issue.
Would that work?
Lastly, libxl__virtio_from_xenstore() is never called in my case. Should I just remove it ? I copied it from some other implementation.
I don't think from_xenstore() is normally called when creating a guest, but if we had an `xl` command called "virtio-list", like there's "block-list", then from_xenstore() would be use I think. It could also be use when doing *-detach, even if virtio doesn't have the option.
Cheers,