On Mon, 2023-01-09 at 18:51 +0000, Dr. David Alan Gilbert wrote:
> * David Woodhouse (dwmw2@infradead.org) wrote:
> > 
> > Thanks. Taking option 'a' then...
> 
> Yes that's mostly right, some minor comments on it:
> 
> > +EvtchnInfoList *qmp_xen_event_list(Error **errp)
> > +{
> > +    error_setg(errp, "Xen event channel emulation not enabled\n");
> 
> No \n on error_setg (and below)

Bah, thought I'd caught all those... er, in fact I *had* caught those
but just sent the penultimate version. Definitely fixed now.

> > +void qmp_xen_event_inject(uint32_t port, Error **errp)
> > +{
> > +    XenEvtchnState *s = xen_evtchn_singleton;
> > +
> > +    if (!s) {
> > +        error_setg(errp, "Xen event channel emulation not enabled");
> 
> return;   ?
> 
> > +    }
> > +
> > +    if (!valid_port(port)) {
> > +        error_setg(errp, "Invalid port %u", port);
> 
> and there?

Fixed now; thanks.

> > +    }
> > +
> > +    QEMU_LOCK_GUARD(&s->port_lock);
> > +
> > +    if (set_port_pending(s, port)) {
> > +        error_setg(errp, "Failed to set port %d", port);
> 
> %u ?

Ack.
> 

> > +##
> > +# @EvtchnInfo:
> > +#
> > +# Information about a Xen event channel port
> > +#
> > +# @port: the port number
> > +#
> > +# @type: the port type
> > +#
> > +# @remote-domain: remote domain for interdomain ports
> 
> Missed vcpu?

Ah yes. In fact I originally missed it out *completely* but then the
hmp bit didn't build when I tried to print it... :)

> 
Should all be fixed in
https://git.infradead.org/users/dwmw2/qemu.git/commitdiff/5cf387fe307 
which is part of the xenfv branch there; I'll repost soonish after
fixing a MemoryRegion refcount leak in kvm_xen_get_vcpu_info_hva().

It might even be posted without the [RFC] tag, at *least* as far as the
parts I've been posting already. We're still working on the PV backend
driver stuff which comes in the next phase.

Thanks again for the feedback.