Re: PCI arbiter memory mapping

[Joan Lledó]

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