On 4/18/2023 10:31 AM, Alex Elder wrote:
On 4/18/23 12:18 PM, Elliot Berman wrote:
On 4/17/2023 3:41 PM, Elliot Berman wrote:
On 3/31/2023 7:27 AM, Alex Elder wrote:
On 3/3/23 7:06 PM, Elliot Berman wrote:
[snip]
diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h
index caeb3b3a3e9a..e52265fa5715 100644
--- a/include/uapi/linux/gunyah.h
+++ b/include/uapi/linux/gunyah.h
@@ -62,8 +62,32 @@ struct gh_vm_dtb_config {
#define GH_VM_START _IO(GH_IOCTL_TYPE, 0x3)
+/**
+ * GH_FN_VCPU - create a vCPU instance to control a vCPU
+ *
+ * gh_fn_desc is filled with &struct gh_fn_vcpu_arg
+ *
+ * The vcpu type will register with the VM Manager to expect to
control
+ * vCPU number `vcpu_id`. It returns a file descriptor allowing
interaction with
+ * the vCPU. See the Gunyah vCPU API description sections for
interacting with
+ * the Gunyah vCPU file descriptors.
+ *
+ * Return: file descriptor to manipulate the vcpu. See GH_VCPU_*
ioctls
+ */
+#define GH_FN_VCPU 1
I think you should define GH_VN_VCPU, GN_FN_IRQFD, and GN_FN_IOEVENTFD
in an enumerated type. Each has a type associated with it, and you can
add the explanation for the function in the kernel-doc comments above
thosse type definitions.
I'd like to enumify the GH_FN_* macros, but one challenge I'm facing
is that it breaks the module alias implementation in patch 19.
MODULE_ALIAS("ghfunc:"__stringify(_type))
When the GH_FN_* are regular preprocessor macros backed by an
integer, the preprocessor will make the module alias ghfunc:0 (or
ghfunc:1, etc). This works well because I can do
request_module("ghfunc:%d", type);
If the function hasn't been registered and then gunyah_vcpu.ko gets
loaded automatically.
With enum, compiler knows the value of GH_FN_VCPU and preprocessor
will make the module alias like ghfunc:GH_FN_VCPU.
I still like the idea of having enum for documentation and clarity. I
noticed that nfnetlink.h saw the same problem for NFNL_SUBSYS_*.
Is this compromise terrible and I should give up on the enum?
You know, I've seen this pattern in the kernel and never thought
too much about why it was done. Maybe this is exactly the reason.
It sure *seems* like there might be some macro magic that might
cause the enum symbol's numeric value to be used but I think the
problem is that enums are C tokens, which are not evaluated at
preprocessor time.
You could probably skip the leading underscore, and do this as
it's done for nfnetlink_groups in that same header file.
Maybe somebody else can confirm, or has a better suggestion.
In the preprocessor macro case, the preprocessor macro GH_FN_VCPU
expands to the GH_FN_VCPU enum value and stuck back as if I didn't have
the preprocessor macro in first place. I'm not sure why the preprocessor
macros are done for nfnetlink_groups. I saw one case where enum
kvm_device_type does the same, but that might be done because it was
converting preprocessor macro to enum.
Just a guess -- maybe the preprocessor macro was preserved to support
userspace code doing this?
#ifdef KVM_DEV_TYPE_FSL_MPIC_20
...
#endif
-Alex
enum gh_fn_type {
/* _GH_FN_* macro required for MODULE_ALIAS, otherwise __stringify()
trick
* won't work anymore */
#define _GH_FN_VCPU 1
GH_FN_VCPU = _GH_FN_VCPU,
#define _GH_FN_IRQFD 2
GH_FN_IRQFD = _GH_FN_IRQFD,
#define _GH_FN_IOEVENTFD 3
GH_FN_IOEVENTFD = _GH_FN_IOEVENTFD,
};
[snip]
+
+/*
+ * Gunyah presently sends max 4 bytes of exit_reason.
+ * If that changes, this macro can be safely increased without
breaking
+ * userspace so long as struct gh_vcpu_run < PAGE_SIZE.
Is PAGE_SIZE allowed to be anything other than 4096 bytes? Do you
expect this driver to work properly if the page size were configured
to be 16384 bytes? In other words, is this a Gunyah constant, or
is it *really* the page size configured for Linux?
Our implementations are only doing 4096 bytes. I expect the driver to
work properly when using 16k pages. This really is a Linux page. It's
a reflection of the alloc_page in gh_vcpu_bind().
The exit reason is copied from hypervisor into field accessible by
userspace directly. Gunyah makes the exit reason size dynamic --
there's no architectural limitation preventing the exit reason from
being a string or some lengthy data.
As I was writing this response, I realized that I should be able to
make this a zero-length array and ensure that reason[] doesn't
overflow PAGE_SIZE...
The comment was trying to explain that Linux itself imposes a
limitation on the maximum exit reason size. If we need to support
longer exit reason, we're OK to do so long as the total size doesn't
overrun PAGE_SIZE. There aren't any plans to need longer exit reasons
than the 8 bytes mentioned today.
Thanks,
Elliot
