Re: [PATCH] hurd: Implement device memory mapping

[Samuel Thibault]

Applied, thanks!!

Samuel

Joan Lledó via Bug reports for the GNU Hurd, le mer. 05 janv. 2022 13:08:01 
+0100, a ecrit:
> From: Joan Lledó <jlledom@member.fsf.org>
> 
> * src/hurd_pci.c:
>       * Implement device memory mapping functions
>               * pci_device_hurd_map_range
>               * pci_device_hurd_unmap_range
>               * pci_device_hurd_map_legacy
>               * pci_device_hurd_unmap_legacy
> * src/x86_pci.h:
>       * Remove unused declarations
>               * pci_device_x86_map_range()
>               * pci_device_x86_unmap_range()
>               * pci_device_x86_map_legacy()
>               * pci_device_x86_unmap_legacy()
> * src/x86_pci.c:
>       * Fix port leaks
>       * Make mapping function static again
>       * map_dev_mem(): use device_map() support for offsets
> ---
>  src/hurd_pci.c | 153 +++++++++++++++++++++++++++++++++++++++++++------
>  src/x86_pci.c  |  23 +++++---
>  src/x86_pci.h  |   8 ---
>  3 files changed, 148 insertions(+), 36 deletions(-)
> 
> diff --git a/src/hurd_pci.c b/src/hurd_pci.c
> index 40c9925..ce96cbe 100644
> --- a/src/hurd_pci.c
> +++ b/src/hurd_pci.c
> @@ -55,6 +55,7 @@
>  
>  /* File names */
>  #define FILE_CONFIG_NAME "config"
> +#define FILE_REGION_NAME "region"
>  #define FILE_ROM_NAME "rom"
>  
>  /* Level in the fs tree */
> @@ -149,8 +150,119 @@ pci_device_hurd_probe(struct pci_device *dev)
>      return 0;
>  }
>  
> +static int
> +pci_device_hurd_map_range(struct pci_device *dev,
> +    struct pci_device_mapping *map)
> +{
> +    int err = 0;
> +    file_t file = MACH_PORT_NULL;
> +    memory_object_t robj, wobj, pager;
> +    vm_offset_t offset;
> +    vm_prot_t prot = VM_PROT_READ;
> +    const struct pci_mem_region * const region = &dev->regions[map->region];
> +    int flags = O_RDONLY;
> +    char server[NAME_MAX];
> +
> +    if (map->flags & PCI_DEV_MAP_FLAG_WRITABLE) {
> +        prot |= VM_PROT_WRITE;
> +        flags = O_RDWR;
> +    }
> +
> +    snprintf(server, NAME_MAX, "%s/%04x/%02x/%02x/%01u/%s%01u",
> +            _SERVERS_BUS_PCI, dev->domain, dev->bus, dev->dev, dev->func,
> +            FILE_REGION_NAME, map->region);
> +
> +    file = file_name_lookup (server, flags, 0);
> +    if (! MACH_PORT_VALID (file)) {
> +        return errno;
> +    }
> +
> +    err = io_map (file, &robj, &wobj);
> +    mach_port_deallocate (mach_task_self(), file);
> +    if (err)
> +        return err;
> +
> +    switch (prot & (VM_PROT_READ|VM_PROT_WRITE)) {
> +    case VM_PROT_READ:
> +        pager = robj;
> +        if (wobj != MACH_PORT_NULL)
> +            mach_port_deallocate (mach_task_self(), wobj);
> +        break;
> +    case VM_PROT_READ|VM_PROT_WRITE:
> +        if (robj == wobj) {
> +            if (robj == MACH_PORT_NULL)
> +                return EPERM;
> +
> +            pager = wobj;
> +            /* Remove extra reference.  */
> +            mach_port_deallocate (mach_task_self (), pager);
> +        }
> +        else {
> +            if (robj != MACH_PORT_NULL)
> +                mach_port_deallocate (mach_task_self (), robj);
> +            if (wobj != MACH_PORT_NULL)
> +                mach_port_deallocate (mach_task_self (), wobj);
> +
> +            return EPERM;
> +        }
> +        break;
> +    default:
> +        return EINVAL;
> +    }
> +
> +    offset = map->base - region->base_addr;
> +    err = vm_map (mach_task_self (), (vm_address_t *)&map->memory, map->size,
> +                  0, 1,
> +                  pager, /* a memory object proxy containing only the region 
> */
> +                  offset, /* offset from region start */
> +                  0, prot, VM_PROT_ALL, VM_INHERIT_SHARE);
> +    mach_port_deallocate (mach_task_self(), pager);
> +
> +    return err;
> +}
> +
> +static int
> +pci_device_hurd_unmap_range(struct pci_device *dev,
> +    struct pci_device_mapping *map)
> +{
> +    int err;
> +    err = pci_device_generic_unmap_range(dev, map);
> +    map->memory = NULL;
> +
> +    return err;
> +}
> +
> +static int
> +pci_device_hurd_map_legacy(struct pci_device *dev, pciaddr_t base,
> +    pciaddr_t size, unsigned map_flags, void **addr)
> +{
> +    struct pci_device_mapping map;
> +    int err;
> +
> +    map.base = base;
> +    map.size = size;
> +    map.flags = map_flags;
> +    err = pci_device_hurd_map_range(dev, &map);
> +    *addr = map.memory;
> +
> +    return err;
> +}
> +
> +static int
> +pci_device_hurd_unmap_legacy(struct pci_device *dev, void *addr,
> +    pciaddr_t size)
> +{
> +    struct pci_device_mapping map;
> +
> +    map.size = size;
> +    map.flags = 0;
> +    map.memory = addr;
> +
> +    return pci_device_hurd_unmap_range(dev, &map);
> +}
> +
>  /*
> - * Read `nbytes' bytes from `reg' in device's configuretion space
> + * Read `nbytes' bytes from `reg' in device's configuration space
>   * and store them in `buf'.
>   *
>   * It's assumed that `nbytes' bytes are allocated in `buf'
> @@ -339,7 +451,8 @@ enum_devices(mach_port_t pci_port, const char *parent, 
> int domain,
>      if (lev > LEVEL_FUNC + 1) {
>          return 0;
>      }
> -    cwd_port = file_name_lookup_under (pci_port, parent, O_DIRECTORY | 
> O_RDWR | O_EXEC, 0);
> +    cwd_port = file_name_lookup_under (pci_port, parent,
> +                                       O_DIRECTORY | O_RDONLY | O_EXEC, 0);
>      if (cwd_port == MACH_PORT_NULL) {
>          return 0;
>      }
> @@ -391,7 +504,7 @@ enum_devices(mach_port_t pci_port, const char *parent, 
> int domain,
>              snprintf(server, NAME_MAX, "./%04x/%02x/%02x/%01u/%s",
>                       domain, bus, dev, func,
>                       entry->d_name);
> -            device_port = file_name_lookup_under(pci_port, server, O_RDWR, 
> 0);
> +            device_port = file_name_lookup_under(pci_port, server, O_RDONLY, 
> 0);
>              if (device_port == MACH_PORT_NULL) {
>                  return 0;
>              }
> @@ -461,6 +574,7 @@ enum_devices(mach_port_t pci_port, const char *parent, 
> int domain,
>              pci_sys->num_devices++;
>          }
>      }
> +    mach_port_deallocate (mach_task_self (), cwd_port);
>  
>      return 0;
>  }
> @@ -470,8 +584,8 @@ static const struct pci_system_methods hurd_pci_methods = 
> {
>      .destroy_device = pci_device_hurd_destroy_device,
>      .read_rom = pci_device_hurd_read_rom,
>      .probe = pci_device_hurd_probe,
> -    .map_range = pci_device_x86_map_range,
> -    .unmap_range = pci_device_x86_unmap_range,
> +    .map_range = pci_device_hurd_map_range,
> +    .unmap_range = pci_device_hurd_unmap_range,
>      .read = pci_device_hurd_read,
>      .write = pci_device_hurd_write,
>      .fill_capabilities = pci_fill_capabilities_generic,
> @@ -483,8 +597,8 @@ static const struct pci_system_methods hurd_pci_methods = 
> {
>      .write32 = pci_device_x86_write32,
>      .write16 = pci_device_x86_write16,
>      .write8 = pci_device_x86_write8,
> -    .map_legacy = pci_device_x86_map_legacy,
> -    .unmap_legacy = pci_device_x86_unmap_legacy,
> +    .map_legacy = pci_device_hurd_map_legacy,
> +    .unmap_legacy = pci_device_hurd_unmap_legacy,
>  };
>  
>  /* Get the name of the server using libpciaccess if any */
> @@ -523,18 +637,18 @@ pci_system_hurd_create(void)
>  
>      pci_sys->num_devices = 0;
>  
> -    if ((err = get_privileged_ports (NULL, &device_master)) || 
> (device_master == MACH_PORT_NULL)) {
> -        pci_system_cleanup();
> -        return err;
> -    }
> -
> -    err = device_open (device_master, D_READ|D_WRITE, "pci", &pci_port);
> -    if (!err) {
> -        root = file_name_lookup_under (pci_port, ".", O_DIRECTORY | O_RDWR | 
> O_EXEC, 0);
> -    }
> -
> -    if (!root) {
> -        root = file_name_lookup (_SERVERS_BUS_PCI, O_RDWR, 0);
> +    if ((err = get_privileged_ports (NULL, &device_master))
> +            || (device_master == MACH_PORT_NULL)) {
> +        root = file_name_lookup (_SERVERS_BUS_PCI, O_RDONLY, 0);
> +    } else {
> +        err = device_open (device_master, D_READ, "pci", &pci_port);
> +        mach_port_deallocate (mach_task_self (), device_master);
> +        if (!err) {
> +            root = file_name_lookup_under (pci_port, ".",
> +                                           O_DIRECTORY | O_RDONLY | O_EXEC, 
> 0);
> +            device_close (pci_port);
> +            mach_port_deallocate (mach_task_self (), pci_port);
> +        }
>      }
>  
>      if (!root) {
> @@ -543,6 +657,7 @@ pci_system_hurd_create(void)
>      }
>  
>      err = enum_devices (root, ".", -1, -1, -1, -1, LEVEL_DOMAIN);
> +    mach_port_deallocate (mach_task_self (), root);
>      if (err) {
>          pci_system_cleanup();
>          return err;
> diff --git a/src/x86_pci.c b/src/x86_pci.c
> index 565dbc8..86be0e7 100644
> --- a/src/x86_pci.c
> +++ b/src/x86_pci.c
> @@ -263,6 +263,7 @@ map_dev_mem(void **dest, size_t mem_offset, size_t 
> mem_size, int write)
>      }
>  
>      err = device_open (master_device, mode, "mem", &devmem);
> +    mach_port_deallocate (mach_task_self (), master_device);
>      if (err)
>          return err;
>  
> @@ -270,17 +271,21 @@ map_dev_mem(void **dest, size_t mem_offset, size_t 
> mem_size, int write)
>      if (mem_size % pagesize)
>          mem_size += pagesize - (mem_size % pagesize);
>  
> -    /* XXX: Mach should be fixed into supporting non-zero offset */
> -    err = device_map (devmem, prot, 0x0, mem_offset + mem_size, &pager, 0);
> +    err = device_map (devmem, prot, mem_offset, mem_size, &pager, 0);
> +    device_close (devmem);
> +    mach_port_deallocate (mach_task_self (), devmem);
>      if (err)
>          return err;
>  
>      err = vm_map (mach_task_self (), (vm_address_t *)dest, mem_size,
>                    (vm_address_t) 0, /* mask */
>                    1, /* anywhere? */
> -                  pager, mem_offset,
> +                  pager, 0,
>                    0, /* copy */
>                    prot, VM_PROT_ALL, VM_INHERIT_SHARE);
> +    mach_port_deallocate (mach_task_self (), pager);
> +    if (err)
> +        return err;
>  
>      return err;
>  #else
> @@ -908,19 +913,19 @@ pci_device_x86_unmap_range(struct pci_device *dev,
>  
>  #else
>  
> -int
> +static int
>  pci_device_x86_map_range(struct pci_device *dev,
>      struct pci_device_mapping *map)
>  {
>      int err;
> -    if ( (err = map_dev_mem(&map->memory, map->base,
> -                            map->size, map->flags & 
> PCI_DEV_MAP_FLAG_WRITABLE)) )
> +    if ( (err = map_dev_mem(&map->memory, map->base, map->size,
> +                            map->flags & PCI_DEV_MAP_FLAG_WRITABLE)))
>          return err;
>  
>      return 0;
>  }
>  
> -int
> +static int
>  pci_device_x86_unmap_range(struct pci_device *dev,
>      struct pci_device_mapping *map)
>  {
> @@ -1099,7 +1104,7 @@ pci_device_x86_write8(struct pci_io_handle *handle, 
> uint32_t reg,
>      outb(data, reg + handle->base);
>  }
>  
> -int
> +static int
>  pci_device_x86_map_legacy(struct pci_device *dev, pciaddr_t base,
>      pciaddr_t size, unsigned map_flags, void **addr)
>  {
> @@ -1115,7 +1120,7 @@ pci_device_x86_map_legacy(struct pci_device *dev, 
> pciaddr_t base,
>      return err;
>  }
>  
> -int
> +static int
>  pci_device_x86_unmap_legacy(struct pci_device *dev, void *addr,
>      pciaddr_t size)
>  {
> diff --git a/src/x86_pci.h b/src/x86_pci.h
> index 22c9318..d36b48a 100644
> --- a/src/x86_pci.h
> +++ b/src/x86_pci.h
> @@ -63,10 +63,6 @@
>  int x86_enable_io(void);
>  int x86_disable_io(void);
>  void pci_system_x86_destroy(void);
> -int pci_device_x86_map_range(struct pci_device *dev,
> -    struct pci_device_mapping *map);
> -int pci_device_x86_unmap_range(struct pci_device *dev,
> -    struct pci_device_mapping *map);
>  struct pci_io_handle *pci_device_x86_open_legacy_io(struct pci_io_handle 
> *ret,
>      struct pci_device *dev, pciaddr_t base, pciaddr_t size);
>  void pci_device_x86_close_io(struct pci_device *dev,
> @@ -80,9 +76,5 @@ void pci_device_x86_write16(struct pci_io_handle *handle, 
> uint32_t reg,
>                      uint16_t data);
>  void pci_device_x86_write8(struct pci_io_handle *handle, uint32_t reg,
>                      uint8_t data);
> -int pci_device_x86_map_legacy(struct pci_device *dev, pciaddr_t base,
> -    pciaddr_t size, unsigned map_flags, void **addr);
> -int pci_device_x86_unmap_legacy(struct pci_device *dev, void *addr,
> -    pciaddr_t size);
>  
>  #endif /* X86_PCI_H */
> -- 
> 2.31.1
> 
> 

-- 
Samuel
/*
 * Oops. The kernel tried to access some bad page. We'll have to
 * terminate things with extreme prejudice.
*/
die_if_kernel("Oops", regs, error_code);
(From linux/arch/i386/mm/fault.c)