On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> Add support for a Hyper-V based vGPU implementation that exposes the
> DirectX API to Linux userspace.
Api questions:
> +struct d3dkmthandle {
> + union {
> + struct {
> + u32 instance : 6;
> + u32 index : 24;
> + u32 unique : 2;
What is the endian of this?
> + };
> + u32 v;
> + };
> +};
> +
> +extern const struct d3dkmthandle zerohandle;
> +
> +struct ntstatus {
> + union {
> + struct {
> + int code : 16;
> + int facility : 13;
> + int customer : 1;
> + int severity : 2;
Same here.
Are these things that cross the user/kernel boundry?
And why int on one and u32 on the other?
> + };
> + int v;
> + };
> +};
> +
> +struct winluid {
> + uint a;
> + uint b;
And now uint? Come on, be consistent please :)
thanks,
greg k-h
On 8/14/2020 5:57 AM, Greg KH wrote:
> On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> > Add support for a Hyper-V based vGPU implementation that exposes the
> > DirectX API to Linux userspace.
>
> Api questions:
>
> > +struct d3dkmthandle {
> > + union {
> > + struct {
> > + u32 instance : 6;
> > + u32 index : 24;
> > + u32 unique : 2;
>
> What is the endian of this?
>
> > + };
> > + u32 v;
> > + };
> > +};
> > +
> > +extern const struct d3dkmthandle zerohandle;
> > +
The definition is the same as on the Windows side. The driver
communicates with a Windows host, so I do not expect any issues with
endiannes. Windows currently runs only on the little endian platforms.
User mode applications see this as an opaque 32 bit value
(D3DKMTHANDLE). I prefer not to use the u32 definition to avoid mistakes
when another integer or a 64-bit handle is assigned to the handle or?
the handle is assigned to a 64 or 32 bit integer variable. There are
many handles in the driver model (shared NT handle, d3dkmthandle, etc).
Using a specific type allows to avoid assigning one handle to another.
> > +struct ntstatus {
> > + union {
> > + struct {
> > + int code : 16;
> > + int facility : 13;
> > + int customer : 1;
> > + int severity : 2;
>
> Same here.
>
> Are these things that cross the user/kernel boundry?
>
> And why int on one and u32 on the other?
>
> > + };
> > + int v;
> > + };
> > +};
> > +
"struct ntstatus" follows the definition for NTSTATUS on the Windows
side. NTSTATUS is an integer where the negative values indicate errors.
It is success otherwise. NTSTATUS is returned by the VM bus messages
from host. IOCTLs from the driver return Linux negative error code or
NTSTATUS positive success codes. DxCore applications expect certain
positive success codes. DxCore is a shared library, which translates the
D3DKMT* Windows interface to Linux ioctls. Applications link with DxCore
to use a paravirtualized GPU.
D3DKMTHANDLE is a 32-bit unsigned value (bitfield), not an integer.
> > +struct winluid {
> > + uint a;
> > + uint b;
>
> And now uint? Come on, be consistent please :)
Sorry about this. This came from the Windows size where we use UINT a
lot. All uints will be replaced by u32 in the next patch set.
>
> thanks,
>
> greg k-h
>
Thank you
Iouri
On Thu, Aug 27, 2020 at 04:45:55PM -0700, Iouri Tarassov wrote:
>
> On 8/14/2020 5:57 AM, Greg KH wrote:
> > On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> > > Add support for a Hyper-V based vGPU implementation that exposes the
> > > DirectX API to Linux userspace.
> >
> > Api questions:
> >
> > > +struct d3dkmthandle {
> > > + union {
> > > + struct {
> > > + u32 instance : 6;
> > > + u32 index : 24;
> > > + u32 unique : 2;
> >
> > What is the endian of this?
> >
> > > + };
> > > + u32 v;
> > > + };
> > > +};
> > > +
> > > +extern const struct d3dkmthandle zerohandle;
> > > +
> The definition is the same as on the Windows side. The driver communicates
> with a Windows host, so I do not expect any issues with endiannes. Windows
> currently runs only on the little endian platforms.
As I mentioned before, you need to document that somewhere (like maybe
preventing your code from being built on big endian systems?)
> User mode applications see this as an opaque 32 bit value (D3DKMTHANDLE). I
> prefer not to use the u32 definition to avoid mistakes when another integer
> or a 64-bit handle is assigned to the handle or? the handle is assigned to a
> 64 or 32 bit integer variable. There are many handles in the driver model
> (shared NT handle, d3dkmthandle, etc). Using a specific type allows to avoid
> assigning one handle to another.
Specific types are great, that is fine.
> > > +struct ntstatus {
> > > + union {
> > > + struct {
> > > + int code : 16;
> > > + int facility : 13;
> > > + int customer : 1;
> > > + int severity : 2;
> >
> > Same here.
> >
> > Are these things that cross the user/kernel boundry?
> >
> > And why int on one and u32 on the other?
> >
> > > + };
> > > + int v;
> > > + };
> > > +};
> > > +
> "struct ntstatus" follows the definition for NTSTATUS on the Windows side.
> NTSTATUS is an integer where the negative values indicate errors. It is
> success otherwise. NTSTATUS is returned by the VM bus messages from host.
> IOCTLs from the driver return Linux negative error code or NTSTATUS positive
> success codes. DxCore applications expect certain positive success codes.
> DxCore is a shared library, which translates the D3DKMT* Windows interface
> to Linux ioctls. Applications link with DxCore to use a paravirtualized GPU.
> D3DKMTHANDLE is a 32-bit unsigned value (bitfield), not an integer.
Ok, again, please document this, and as these fields are crossing the
kernel/user boundry, use the correct types (hint, 'int' is never that
type).
> > > +struct winluid {
> > > + uint a;
> > > + uint b;
> >
> > And now uint? Come on, be consistent please :)
> Sorry about this. This came from the Windows size where we use UINT a lot.
> All uints will be replaced by u32 in the next patch set.
thank you.
greg k-h