On 3/31/2023 7:27 AM, Alex Elder wrote:
On 3/3/23 7:06 PM, Elliot Berman wrote:
[snip]
+
+static int irqfd_wakeup(wait_queue_entry_t *wait, unsigned int mode,
int sync, void *key)
+{
+ struct gh_irqfd *irqfd = container_of(wait, struct gh_irqfd, wait);
+ __poll_t flags = key_to_poll(key);
+ u64 enable_mask = GH_BELL_NONBLOCK;
+ u64 old_flags;
+ int ret = 0;
+
+ if (flags & EPOLLIN) {
+ if (irqfd->ghrsc) {
+ ret = gh_hypercall_bell_send(irqfd->ghrsc->capid,
enable_mask, &old_flags);
I commented elsewhere that you might support passing a null
pointer as the last argument above (since you don't use the
result).
+ if (ret)
+ pr_err_ratelimited("Failed to inject interrupt %d:
%d\n",
+ irqfd->ticket.label, ret);
+ } else
+ pr_err_ratelimited("Premature injection of interrupt\n");
+ }
+
+ return 0;
+}
+
+static void irqfd_ptable_queue_proc(struct file *file,
wait_queue_head_t *wqh, poll_table *pt)
+{
+ struct gh_irqfd *irq_ctx = container_of(pt, struct gh_irqfd, pt);
+
+ add_wait_queue(wqh, &irq_ctx->wait);
+}
+
+static int gh_irqfd_populate(struct gh_vm_resource_ticket *ticket,
struct gh_resource *ghrsc)
+{
+ struct gh_irqfd *irqfd = container_of(ticket, struct gh_irqfd,
ticket);
+ u64 enable_mask = GH_BELL_NONBLOCK;
+ u64 ack_mask = ~0;
Why is the ACK mask ~0?
I guess I don't know details about this hypercall (do you document
them somewhere?), so it's hard to judge whether or why this is the
right thing to use. The enable_mask is just GH_BELL_NONBLOCK,
which is just BIT(32).
I talked to our hypervisor folks and they mentioned we can simplify
this. In v12, enable_mask and ack_mask can just be "1" (BIT(0)). We had
chosen bit 32 arbitrarily.
[snip]
+ }
+
+ irqfd->ghrsc = ghrsc;
+ if (irqfd->level) {
I think I don't understand this part of the code well
enough to know this. What happens if level is false?
If level is false, then guest is assumed to set up IRQ on its side as
edge-triggered. In that case, we don't need to configure the enable
mask/ack mask because the doorbell flags aren't polled.
[snip]
+/**
+ * struct gh_fn_irqfd_arg - Arguments to create an irqfd function
+ * @fd: an eventfd which when written to will raise a doorbell
+ * @label: Label of the doorbell created on the guest VM
+ * @flags: GH_IRQFD_LEVEL configures the corresponding doorbell to
behave
+ * like a level triggered interrupt.
+ * @padding: padding bytes
+ */
+struct gh_fn_irqfd_arg {
+ __u32 fd;
Should the "fd" field be signed? Should it be an int? (Perhaps
you're trying to define a fixed kernel API, so __s32 if signed would
be better.)
It looked to me like some interfaces use __u32 and some use __s32. Is
one technically correct?
