Re: [PATCH v2 1/3] tools/libs/evtchn: decouple more from mini-os

[Juergen Gross <jgross@xxxxxxxx>]

On 11.01.22 20:56, Andrew Cooper wrote:
> On 11/01/2022 15:03, Juergen Gross wrote:
> > diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c
> > index e5dfdc5ef5..c3a5ce3b98 100644
> > --- a/tools/libs/evtchn/minios.c
> > +++ b/tools/libs/evtchn/minios.c
> > @@ -38,29 +38,40 @@
> >   
> >   #include "private.h"
> >   
> > -extern void minios_evtchn_close_fd(int fd);
> > +LIST_HEAD(port_list, port_info);
> > +
> > +struct port_info {
> > +    LIST_ENTRY(port_info) list;
> > +    evtchn_port_t port;
> > +    bool pending;
> > +    bool bound;
> > +};
> >   
> >   extern struct wait_queue_head event_queue;
> Yuck.  This should come from minios's evtchn header, rather than being
> extern'd like this, but lets consider that future cleanup work.
I think I should do that rather sooner than later.

> > +int minios_evtchn_close_fd(int fd);
> You don't need this forward declaration, because nothing in this file
> calls minios_evtchn_close_fd().  The extern should simply be deleted,
> and it removes a hunk from your later xen.git series.
Without it I get a build error due to no prototype defined.

> > @@ -69,18 +80,54 @@ static void port_dealloc(struct evtchn_port_info *port_info)
> >       free(port_info);
> >   }
> >   
> > +int minios_evtchn_close_fd(int fd)
> > +{
> > +    struct port_info *port_info, *tmp;
> > +    struct file *file = get_file_from_fd(fd);
> > +    struct port_list *port_list = file->dev;
> Looking at this, the file_ops don't need to have the C ABI.
>
> The single caller already needs access to the file structure, so could
> pass both file and fd in to the ops->close() function.  Thoughts?
If we do this for close(), we should do it for all callbacks. I think we
don't need fd at all in the callbacks, so switching to file seems to be
the way to go.

> > +
> > +    LIST_FOREACH_SAFE(port_info, port_list, list, tmp)
> > +        port_dealloc(port_info);
> > +    free(port_list);
> > +
> > +    return 0;
> > +}
> > +
> > +static struct file_ops evtchn_ops = {
> This wants to become const, when alloc_file_type() has been
> appropriately const'd.
Yes.

> > +    .name = "evtchn",
> > +    .close = minios_evtchn_close_fd,
> > +    .select_rd = select_read_flag,
> > +};
> > +
> >   /*
> >    * XENEVTCHN_NO_CLOEXEC is being ignored, as there is no exec() call supported
> >    * in Mini-OS.
> >    */
> >   int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags)
> >   {
> > -    int fd = alloc_fd(FTYPE_EVTCHN);
> > +    int fd;
> > +    struct file *file;
> > +    struct port_list *list;
> > +    static unsigned int ftype_evtchn;
> >   
> > -    if ( fd == -1 )
> > +    if ( !ftype_evtchn )
> > +        ftype_evtchn = alloc_file_type(&evtchn_ops);
> Hmm.  MiniOS doesn't appear to support __attribute__((constructor)) but
> this would be an ideal candidate.
>
> It would remove a non-threadsafe singleton from a (largely unrelated)
> codepath.
>
> Should be very simple to add to MiniOS.  See Xen's init_constructors(),
> and add CONSTRUCTORS to the linker file.
I'll look into this.

> > @@ -134,42 +171,43 @@ int xenevtchn_notify(xenevtchn_handle *xce, evtchn_port_t 
> > port)
> >   
> >   static void evtchn_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
> >   {
> > -    int fd = (int)(intptr_t)data;
> > -    struct evtchn_port_info *port_info;
> > +    xenevtchn_handle *xce = data;
> > +    struct file *file = get_file_from_fd(xce->fd);
> > +    struct port_info *port_info;
> > +    struct port_list *port_list;
> >   
> > -    assert(files[fd].type == FTYPE_EVTCHN);
> > +    assert(file);
> > +    port_list = file->dev;
> >       mask_evtchn(port);
> > -    LIST_FOREACH(port_info, &files[fd].evtchn.ports, list)
> > +    LIST_FOREACH(port_info, port_list, list)
> >       {
> >           if ( port_info->port == port )
> >               goto found;
> >       }
> >   
> > -    printk("Unknown port for handle %d\n", fd);
> > +    printk("Unknown port for handle %d\n", xce->fd);
> As you're editing this line anyway, it really wants to become "Unknown
> port %d for handle %d\n".
Okay.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature