VAX: Mask QBus addresses to 22 bits

[crwolff]

Fixes issue #409
Ultrix-32M v1.2 has a hardcoded bdp value (buffered data path) or'd into the memory address sent to an MSCP device. The memory address is subsequently used as a destination for the disk read, but since it is outside of the memory range, the device reports an I/O error causing the OS to fail.

This patch masks all QBus transfer addresses to 22 bits which allows Ultrix-32M v1.2 to install and run correctly.

[crwolff]

Per section 2.3.7.1 of the STORAGE SYSTEMS PORT ARCHITECTURE, bits 24-30 of the buffer address are used for the adapter channel. This change masks these bits during read and write operations.

[pkoning2]

But what does the actual hardware that's emulated here do? That standard is interesting but the goal is to emulate the actual hardware, not the standard if it happens to be different.

[crwolff]

But what does the actual hardware that's emulated here do? That standard is interesting but the goal is to emulate the actual hardware, not the standard if it happens to be different.

The QBus has only 22 address lines, so the hardware masks off the upper 10 bits.

[markpizz]

Masking to 22bits somewhere (either in the vax630_io.c or in pdp11_rq.c) is appropriate. That this first has happened with this Ultrix version after many other OS driver code doesn't make it clear which is the appropriate place. Making the change in pdp11_rq.c addresses code which might try to do this on any system with a Qbus without any other system specific accommodations.

Also, the multiple commits in this PR should be combined AND the commit messages should follow the project commit message standards that have a first line which identifies the affected simulators and a 1 line summary of the fix along with potential additional explanation on subsequent commit message lines.

Additionally, we want to have the actual name of the person making the commit which can be either in the github profile or in the email address of the commit. Crwolff doesn't really identify who's suggesting this change.

[pkoning2]

Ok, and those upper bits don't do anything else in the real hardware? They are just ignored? Are they read/write but ignored, or unimplemented read-as-zero?

[markpizz]

An interesting comparison should be made in the logic of that boot code when it runs on a MicroVAX I and when it runs on a MicroVAX 3900. Yes, this version of Ultrix probably won't completely work on the MicroVAX 3900, but I'd guess that the boot code might succeed until something further along in the loaded kernel doesn't recognize the newer CPU type.

Additionally, since later Ultrix (or BSD) versions run on unmodified MicroVAX II simulators, there must be something changed in that boot code for those versions. I wonder why those changes were made...

[crwolff]

Ok, and those upper bits don't do anything else in the real hardware? They are just ignored? Are they read/write but ignored, or unimplemented read-as-zero?

The RQDX3 uses the buffer address in the command packet to perform a DMA transfer back to the main memory. Since the DMA is using the QBus address lines, the extra bits are just ignored.

[crwolff]

Additionally, since later Ultrix (or BSD) versions run on unmodified MicroVAX II simulators, there must be something changed in that boot code for those versions. I wonder why those changes were made...

In Ultrix 4.2 ubasetup.c the code checks the machine and skips allocating a BDP for machines that don't require it, resulting in the field being zero for more recent releases.

    if (uh->uba_type & (UBA730|UBAUVI|UBAUVII))
        flags &= ~UBA_NEEDBDP;

UBAUVI is ka610
UBAUVII is kn210, kn220, ka60, ka630 and ka650.

[crwolff]

Converted QBus mask to an existing symbolic based on VAX-11/730 implementation
Added masking for for Microvax 3900

[markpizz]

Changing the title of the PR is nice, but the commit comments should not only contain a 1 line summary, but the justification for what/why this is being done. The reason for this is due to the fact that commit comments travel with every clone/copy of the repo completely independent of where these explanations exist in github's Issue/PR system. If you gather your changes into a single commit which is then "git push -f" to your github repo you can solve the single commit question and the extended commit message at one time. Likewise that single commit could contain your email address (and name) if you want, or you can add your full name to your github profile.

I've suggested the simplest form of the change should be in pdp11_rq.c due to the fact this is the ONLY Qbus DMA device which gets the buffer address as a 32bit value. All other Qbus DMA devices have to construct the 22bit Qbus address by gathering pieces of that 22bit address from several places (A BA register plus some extra bits provided in another register).

[crwolff]

I've suggested the simplest form of the change should be in pdp11_rq.c due to the fact this is the ONLY Qbus DMA device which gets the buffer address as a 32bit value.

All of the big VAXen mask the address to 18 bits in Map_ReadX/Map_WriteX functions located in vax###_uba.c. This PR extends the existing design to include the 630 and 650 CPUs.

[crwolff]

Changing the title of the PR is nice, but the commit comments should not only contain a 1 line summary, but the justification for what/why this is being done. The reason for this is due to the fact that commit comments travel with every clone/copy of the repo completely independent of where these explanations exist in github's Issue/PR system. If you gather your changes into a single commit which is then "git push -f" to your github repo you can solve the single commit question and the extended commit message at one time. Likewise that single commit could contain your email address (and name) if you want, or you can add your full name to your github profile.

I have not located these requirements in any of the project documentation. Please provide a link.

[markpizz]

Changing the title of the PR is nice, but the commit comments should not only contain a 1 line summary, but the justification for what/why this is being done. The reason for this is due to the fact that commit comments travel with every clone/copy of the repo completely independent of where these explanations exist in github's Issue/PR system. If you gather your changes into a single commit which is then "git push -f" to your github repo you can solve the single commit question and the extended commit message at one time. Likewise that single commit could contain your email address (and name) if you want, or you can add your full name to your github profile.

I have not located these requirements in any of the project documentation. Please provide a link.

You can dig through the github notes for all prior Pull Requests here in open-simh/simh and simh/simh and find precisely the same input provided. Sometimes here in open-simh/simh multiple commits (non squashed) and/or inconsistent commit comment formatting have been merged probably by accident and in early days this happened in simh/simh as well. Meanwhile, here is an example of a PR where the author chose to remain anonymous and that PR hasn't been merged in the last 18+ months: #157

You did the necessary digging to actually solve the reported problem in #409.

Meanwhile, your PR has 3 commits which repeatedly change essentially the same lines in a couple of files and have separate non project form commit messages in 2 of the 3 commits. Please explain why this needs to be merged as 3 commits.

Additionally, you very correctly described the justification for what you've done here in this PR discussion. Please explain why the actual justification can't be part of the commit comments.

You may say that it is hard for you to find the repeated times the same info has been provided in previous PR discussions and that may be true, but it provides a precise example of why the information I'm suggesting belongs in the repository content (commit comments or files in the repo) rather than PR discussions.