search

obhq / obliteration

Experimental PS4 emulator written in Rust for Windows, macOS and Linux
https://obliteration.net
MIT License
601 stars 17 forks source link

Refactors character devices #729

Closed SuchAFuriousDeath closed 4 months ago

SuchAFuriousDeath commented 4 months ago

@ultimaweapon I want to know your opinion on this. Keep in mind, this is just a show off of what I have in mind. I mentioned under some issue or PR that I checked a lot of devices and none of the seem to actually us FdOpen anywhere. We might be able to Rustify the devices.

SuchAFuriousDeath commented 4 months ago

With this approach, driver-specific fields that FreeBSD stores in cdev->si_drv1 and cdev->si_drv2 (I think) would be stored inside the type that implements the DeviceTrait and any global data for all instances of a device (I think those are stored in cdevsw->spare0 and cdevsw->spare1 ?) would be stored probably in a static OnceLock.

ultimaweapon commented 4 months ago

@ultimaweapon I want to know your opinion on this. Keep in mind, this is just a show off of what I have in mind. I mentioned under some issue or PR that I checked a lot of devices and none of the seem to actually us FdOpen anywhere. We might be able to Rustify the devices.

IIRC there is one place that call into this (can't remember where is this). It is the reason why I bring it into our code. When I was implemented this I want to remove it too due to it is so awkward.

SuchAFuriousDeath commented 4 months ago

Hmmm. Besides, we might be able to do fdopen in a single function, probably, right? Just pass a bool to it, or an enum, or something like that

ultimaweapon commented 4 months ago

If you don't want your idea to be missing better to start it as a discussion.

ultimaweapon commented 4 months ago

Hmmm. Besides, we might be able to do fdopen in a single function, probably, right? Just pass a bool to it, or an enum, or something like that

My idea is we should find an alternative solution where the PS4 call into this function. If we can fix at the call site then we can remove it.

SuchAFuriousDeath commented 4 months ago

@ultimaweapon I want to know your opinion on this. Keep in mind, this is just a show off of what I have in mind. I mentioned under some issue or PR that I checked a lot of devices and none of the seem to actually us FdOpen anywhere. We might be able to Rustify the devices.

IIRC there is one place that call into this (can't remember where is this). It is the reason why I bring it into our code. When I was implemented this I want to remove it too due to it is so awkward.

So... what now? Will you merge this with the idea that we'll refactor the devices later, once we start encountering them? It's kind of difficult to predict what will happen if we're not reaching any :D

SuchAFuriousDeath commented 4 months ago

Besides, didn't we jump ahead a bit too much with devfs? Maybe we should have stubbed it and done it later. Can it even happen that the file in VnodeBackend::open is null for us?

SuchAFuriousDeath commented 4 months ago

And also, I'm not sure if you're talking about fdopen being called in VnodeBackend::open, or you've seen it in an actual PS4 device. Like I said, in FreeBSD there's only one driver that actually uses it. I looked at a lot of PS4 devices and none of the use it too :D

ultimaweapon commented 4 months ago

@ultimaweapon I want to know your opinion on this. Keep in mind, this is just a show off of what I have in mind. I mentioned under some issue or PR that I checked a lot of devices and none of the seem to actually us FdOpen anywhere. We might be able to Rustify the devices.

IIRC there is one place that call into this (can't remember where is this). It is the reason why I bring it into our code. When I was implemented this I want to remove it too due to it is so awkward.

So... what now? Will you merge this with the idea that we'll refactor the devices later, once we start encountering them? It's kind of difficult to predict what will happen if we're not reaching any :D

I don't think putting all devices in a single place is a good idea. The fs/dev should be a foundation for creating a devices instead of implementing it own devices.

Besides, didn't we jump ahead a bit too much with devfs? Maybe we should have stubbed it and done it later. Can it even happen that the file in VnodeBackend::open is null for us?

Do you mean the devfs system or the device files?

SuchAFuriousDeath commented 4 months ago

The filesystem and vnodeops. Well, the devices too. I think we commited the code structure too soon

ultimaweapon commented 4 months ago

And also, I'm not sure if you're talking about fdopen being called in VnodeBackend::open, or you've seen it in an actual PS4 device. Like I said, in FreeBSD there's only one driver that actually uses it. I looked at a lot of PS4 devices and none of the use it too :D

Better to continue this at #730.

ultimaweapon commented 4 months ago

The filesystem and vnodeops

I think it is the right decision that we decided to rework on FS system. This allow us to implement anything that rely on FS correctly from the beginning without the need to come back later to rewrite it in the future.

If we keep going without fixing the root problem anything that depend on it will also goes wrong, which will cause the problem to accumulate and it will be much harder and harder to fix. Once we have too much problems instead of too many things that not yet implemented we will be exhausted because we don't want to fix the problem.

SuchAFuriousDeath commented 4 months ago

I didn't mean the FS rewrite, That was a good idea. I mean the devfs filesystem. If I put a todo inside VnodeBackend::open, it isn't reached. Why did we jump ahead and implement it, instead of solving the problem when we actually reach it?

SuchAFuriousDeath commented 4 months ago

Because now, there's an open issue for a 'problem', when it feels like the real problem is that fdopen shouldn't be in our code in the first place at this point in time.

SuchAFuriousDeath commented 4 months ago

I don't think putting all devices in a single place is a good idea. The fs/dev should be a foundation for creating a devices instead of implementing it own devices.

Really? For example. FreeBSD puts a lot of them in one directory. Where would you put them?

SuchAFuriousDeath commented 4 months ago

Source: http://fxr.watson.org/fxr/source/dev/?v=FREEBSD-9-STABLE

ultimaweapon commented 4 months ago

I don't think putting all devices in a single place is a good idea. The fs/dev should be a foundation for creating a devices instead of implementing it own devices.

Really? For example. FreeBSD puts a lot of them in one directory. Where would you put them?

Where did you see that? Here is what I saw.

ultimaweapon commented 4 months ago

Source: http://fxr.watson.org/fxr/source/dev/?v=FREEBSD-9-STABLE

It seems like those for hardware supports, not the device file.

SuchAFuriousDeath commented 4 months ago

To be clear, I'm not saying that all the devices should be in fs/dev, I'm saying that we should keep them together.

Where did you see that? Here is what I saw.

Here is what I saw: http://fxr.watson.org/fxr/source/dev/?v=FREEBSD-9-STABLE

SuchAFuriousDeath commented 4 months ago

Source: http://fxr.watson.org/fxr/source/dev/?v=FREEBSD-9-STABLE

It seems like those for hardware supports, not the device file.

They contain the kernel modules that make the devices, though: http://fxr.watson.org/fxr/ident?v=FREEBSD-9-STABLE&i=cdevsw

The /dev directory also contains /dev/random and /dev/null, for example, which are devices we'll likely have too.

ultimaweapon commented 4 months ago

We can try putting it in a directory on the top-level (not in the fs).

SuchAFuriousDeath commented 4 months ago

I moved them to dev in the kernel crate. I also removed the fdopen. This is enough changes for now I think.

SuchAFuriousDeath commented 4 months ago

Apparently, my brilliant idea isn't object safe :(

SuchAFuriousDeath commented 4 months ago

@ultimaweapon By the way, VThread holds a strong reference to VProc and VProc holds a vec of strong references to VThreads, doesn't that leak memory?

ultimaweapon commented 4 months ago

@ultimaweapon By the way, VThread holds a strong reference to VProc and VProc holds a vec of strong references to VThreads, doesn't that leak memory?

Nope. There is a RAII object that will remove it from the process when the thread is terminated.

SuchAFuriousDeath commented 4 months ago

@ultimaweapon I think that to do replace cdevsw open, we will need something like Box<dyn FnOnce() -> Box> or something like that.

SuchAFuriousDeath commented 4 months ago

@ultimaweapon I think I figured it out, take a look when you can.

ultimaweapon commented 4 months ago

@SuchAFuriousDeath do you know if we are going to need d_flags in the cdevsw? I'm worried about removing it right now because I'm not sure if we are going to need it in the future. If you are unsure better to keep it in each implementation for now.

SuchAFuriousDeath commented 4 months ago

Good point, I didn't even realize

SuchAFuriousDeath commented 4 months ago

@ultimaweapon Take a look