Messages in this thread | | | Date | Mon, 17 Apr 2023 15:55:26 -0700 | Subject | Re: [PATCH v11 24/26] virt: gunyah: Add irqfd interface | From | Elliot Berman <> |
| |
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?
| |