[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: PCI arbiter memory mapping

From: Joan Lledó
Subject: Re: PCI arbiter memory mapping
Date: Mon, 16 Aug 2021 10:53:47 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0


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?


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

I'll do.

[1] https://gitlab.freedesktop.org/jlledom/libpciaccess/-/blob/hurd-device-map/src/hurd_pci.c#L196

reply via email to

[Prev in Thread] Current Thread [Next in Thread]