Hi,
El 5/8/21 a les 1:26, Samuel Thibault ha escrit:
Is it not possible to avoid having to call memory_object_proxy_valid?
maybe better fix device_map into supporting non-zero offset,
I think it'd be a better solution to move the call to
memory_object_proxy_valid() and the start value overwrite to
memory_object_create_proxy(), that way all logic related to proxies is
kept inside memory_object_proxy.c and other modules of the kernel don't
need to be aware of proxies.
Does pci_device_hurd_unmap_range not need to close the pager?
Yes.
Also, map_dev_mem needs to release the pager when dev == NULL? otherwise
pci_device_x86_read_rom would leak the pager?
Yes. Or not passing a pager to device_map() if dev == NULL, is not going
to be used anyway.
I'm a bit afraid of the struct pci_user_data passing between
libpciaccess and pci-arbiter
Me too.
I'd rather see a
hurd-specific libpciaccess function to get the pager.
That's better, but we'd have to add that function in both hurd_pci.c and
x86_pci.c. I don't like the idea of adding Hurd specific code to
x86_module but there's already a lot of it and we could avoid the
existence of struct pci_user_data, which I like even less.
Apart from that, I also found a bug in hurd_pci.c:196 [1]. When the user
asks for read/write permissions but only read is allowed, we should either:
- Deallocate robj and return EPERM
- Do nothing and map the range read-only which is not what the user
asked for.
The current code deallocates wobj which is null, leaks robj and returns
EPERM. That wrong, since it doesn't make much sense, but which of two
above is correct?
I think pci_device_hurd_map_range() should behave the same way
pci_device_x86_map_range() does in the x86 module. But the
implementation of map_dev_mem() is not clear about what happens in that
case, I guess vm_map() handles that.
BTW, you can directly push "fix indentation" commits, so that after
rebasing your branch only contains actual code changes, for easier
review.
I'll do.
------
[1]
https://gitlab.freedesktop.org/jlledom/libpciaccess/-/blob/hurd-device-map/src/hurd_pci.c#L196