OpenVPN / tap-windows6

Windows TAP driver (NDIS 6)
Other
788 stars 237 forks source link

Add ARM64 build support #56

Closed jkunkee closed 5 years ago

jkunkee commented 6 years ago

This change simply adds one more build architecture configuration.

jkunkee commented 6 years ago

Rebased onto master

cron2 commented 6 years ago

Hi,

On Tue, May 08, 2018 at 05:08:01PM -0700, Jon Kunkee wrote:

This change simply adds one more build architecture configuration. You can view, comment on, or merge this pull request online at:

https://github.com/OpenVPN/tap-windows6/pull/56

Briefly looked into this. I neither do python nor do I understand Windows, but this one looks like "case-insensitive filesystem alert"

so, should it be "arm64":"ARM64" in self.architecture_platform_folder_map?

(Most likely not relevant on NTFS, but in case someone comes up with a case-sensitive windows filesystem, if this does what I think it does, it will be an avoidable surprise)

Besides this, this seems to be fairly trivial and straightforward - so if it passes building on Samuli's build rig, go for it (ACK).

(It used to be a much larger patch set, but the NDIS 6.20 stuff has been merged so not much is left here)

gert

-- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany gert@greenie.muc.de

jkunkee commented 6 years ago

Working here I learned that NTFS is case-sensitive, but Windows passes a flag to the driver on every name lookup telling it to be case-insensitive. Principle of least surprise: I'll do it. It's pretty easy.

jkunkee commented 6 years ago

Since it looks like the VS tools create an arm64 folder, lowercase, I updated the other references to match this one.

mattock commented 5 years ago

@jkunkee I'm not sure if this is the correct PR to note this bug, but the Attestion signing process revealed a possible issue in the INF file:

amd64\oemvista.inf does not have NTARM64 decorated model sections.
amd64\oemvista.inf does not have NTARM64 decorated model sections.

It is noteworthy that it is complaining about NTARM64 while processing the amd64 INF file. The submited cabinet file contains all the architectures in separate directories (as shown here). I also checked all the checkboxes in "Requested signatures" section when submitting the cabinet file. Perhaps that kind of multi-architecture cabinet is not supported, and I should submit each architecture as separate cabinet file, checking only the relevant signature sections.

Any thoughts?

jkunkee commented 5 years ago

Huh. It's odd that it's asking for .NTARM64 in an AMD64 folder, but this seems to be a continuation of an old problem:

https://social.msdn.microsoft.com/Forums/en-US/2203c567-7483-4438-8f86-9590252768b3/x86-driver-submission-for-attestation-signing-fails-with-does-not-have-ntamd64-decorated-model

I kind of hope this is due to this from the Attestation Signing page you linked:

All driver folders in your CAB file must support the same set of architectures. For example, they all must support x86, x64, or they all must support both x86 and x64.

That means either a) it requires all INFs to list all archs in the Manufacturer section or b) you're missing a quirk in your CAB's folder structure.

a) was broken by https://github.com/OpenVPN/tap-windows6/pull/53/files#diff-11126c9effa069f82717db3853df87e9 where each arch's INF is tailored to just that arch. This is easy but ugly to fix.

b) is more promising but less certain. What does your CAB's folder structure look like? I learned here that it has to be pretty specific:

\driver\foo.inf
\driver\foo.pdb
\driver\foo.sys

I haven't found good docs yet for the multiarch case, but I think that each folder in the root of the CAB is considered a separate driver and the Attestation Signing pipeline requires that they all have the same set of archs in them. This means that this structure may get past this error:

\driver\x86\foo.inf
\driver\x86\foo.pdb
\driver\x86\foo.sys
\driver\arm64\foo.inf
\driver\arm64\foo.pdb
\driver\arm64\foo.sys
\driver\amd64\foo.inf
\driver\amd64\foo.pdb
\driver\amd64\foo.sys

...but again, I'm not certain.

Note that multi-arch support is present according to this interview, I just can't find any docs that detail how to do so.

mattock commented 5 years ago

It was possible to get attestation signatures by uploading each architecture in a separate file and by checking only the applicable signatures on the "Required signatures" page.

mattock commented 5 years ago

I'll ACK and merge this in preparation for the 9.23.2 release. This PR was included when building the tap-windows6 9.23.1 test installers and worked fine.