Re: [PATCH v3 hurd] pci: Add RPCs for taking and freeing io ports by region

[Joan Lledó]

Hi,

Thank you Damien and Samuel for your explanations.

On 22/7/23 3:31, Damien Zammit wrote:
> diff --git a/pci-arbiter/netfs_impl.c b/pci-arbiter/netfs_impl.c
> index 4bb5c97a..6b3d6918 100644
> --- a/pci-arbiter/netfs_impl.c
> +++ b/pci-arbiter/netfs_impl.c
> @@ -522,6 +522,14 @@ netfs_attempt_read (struct iouser * cred, struct node * 
> node,
>         /* Update atime */
>         UPDATE_TIMES (node->nn->ln, TOUCH_ATIME);
>       }
> +  else if (!strncmp
> +          (node->nn->ln->name, FILE_IO_NAME, strlen (FILE_IO_NAME)))
> +    {
> +      err = io_region_file (node->nn->ln, offset, len, data, 1);
> +      if (!err)
> +       /* Update atime */
> +       UPDATE_TIMES (node->nn->ln, TOUCH_ATIME);
> +    }

This block code is repeated from the condition above. Why not modifying 
the condition to:

else if (
!strncmp (node->nn->ln->name, FILE_REGION_NAME, strlen 
(FILE_REGION_NAME)) ||
!strncmp (node->nn->ln->name, FILE_IO_NAME, strlen (FILE_IO_NAME))
)

And avoid the code duplication?

> @@ -556,6 +564,14 @@ netfs_attempt_write (struct iouser * cred, struct node * 
> node,
>         /* Update atime */
>         UPDATE_TIMES (node->nn->ln, TOUCH_MTIME | TOUCH_CTIME);
>       }
> +  else if (!strncmp
> +          (node->nn->ln->name, FILE_IO_NAME, strlen (FILE_IO_NAME)))
> +    {
> +      err = io_region_file (node->nn->ln, offset, len, (void*) data, 0);
> +      if (!err)
> +       /* Update atime */
> +       UPDATE_TIMES (node->nn->ln, TOUCH_MTIME | TOUCH_CTIME);
> +    }

Same code duplication here.

>     else
>       return EOPNOTSUPP;
>   
> diff --git a/pci-arbiter/pci-ops.c b/pci-arbiter/pci-ops.c
> index 2f9f5296..aa92f991 100644
> --- a/pci-arbiter/pci-ops.c
> +++ b/pci-arbiter/pci-ops.c
> @@ -273,3 +274,61 @@ S_pci_get_dev_rom (struct protid * master, char **data, 
> mach_msg_type_number_t *
>   
>     return 0;
>   }
> +
> +error_t
> +request_io_ports (struct pcifs_dirent *e, io_perm_t *io_perm)
> +{
> +  int bar;
> +  uint64_t iobase, iosize;
> +  io_port_t from, to;
> +  error_t err;
> +  mach_port_t device_master;
> +
> +  if (get_privileged_ports (0, &device_master))
> +    return EPERM;

Correct me if I'm wrong: nested arbiters would fail here, right? An 
arbiter launched from a user (non-root) having permissions over the IO 
file should be able to get the port and handle it to it's clients. Then 
we need the fallback mechanism to RPC the master arbiter when this is a 
nested arbiter. Just mentioning, no need to be in this patch, though.

> +
> +  bar = strtol(&e->name[2], NULL, 10);

Better:

&e->name[strlen (FILE_IO_NAME)]

WDYT?

The rest of the patch looks good to me.