This extends the QemuVideoDxe driver to support the VMWare SVGA2 display
device implemented by Qemu. Drivers for this device exist for guest OSes
which do not support Qemu's other display adapters, so supporting it in
OVMF is useful in conjunction with those OSes.

I've tried to follow the existing pattern for device-specific code in
OVMF's QemuVideoDxe driver as much as possible, with the minimum of
additional code. I've marked this patch as RFC for 2 main reasons:

1. I've imported VMWare's own header file with device register constants
etc. verbatim. (patch 1/2) This doesn't follow any of the EDK2 coding
conventions, and it uses the MIT license, not BSD. Only a small percentage
of symbols are actually used in the driver. On the other hand, it's
obviously the authoritative source. I'm not sure what the correct
etiquette is here, define our own constants, or import the authoritative
header file?

2. For the functionality this driver uses, 2 I/O ports are used with
32-bit wide reads and writes. Unfortunately, one of them is not 32-bit
aligned. This is fine as far as x86/x86-64 is concerned, but neither
EDK2's IoLib nor other platforms support such an access pattern. It seems
this issue was already encountered/discussed on the edk2-devel list 4
years ago: http://edk2-devel.narkive.com/bwH3r0us/unaligned-i-o I couldn't
find any code resulting from that discussion, and Qemu definitely uses
unaligned port numbers for the SVGA2 device. (SVGA_IO_MUL is 1 in
hw/display/vmware_vga.c) It does not appear to make any provision for
non-x86 architectures, so I assume there's no sensible way to drive the
device in those cases. The patch therefore only detects the device on x86,
where it uses UnalignedIoWrite/Read32() helper functions which I've based
on IoLib's aligned ones. I have only tested the GCC version of these.
Feel free to suggest a better way of handling the issue.

Github feature branch: https://github.com/pmj/edk2/tree/ovmf_vmware_svga2_v1

Phil Dennis-Jordan (2):
  OvmfPkg: Add SVGA2 device register definition header from VMWare
  OvmfPkg: Add VMWare SVGA II support in QemuVideoDxe.

 OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf         |    6 +
 OvmfPkg/QemuVideoDxe/Qemu.h                   |   50 +
 OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h    |   51 +
 OvmfPkg/QemuVideoDxe/svga_reg.h               | 1558 ++++++++++++++++++++
 OvmfPkg/QemuVideoDxe/Driver.c                 |   67 +
 OvmfPkg/QemuVideoDxe/Gop.c                    |   71 +-
 OvmfPkg/QemuVideoDxe/Initialize.c             |   88 ++
 OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c         |   59 +
 OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c         |   79 +
 OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c         |   81 +
 OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c |   53 +
 11 files changed, 2162 insertions(+), 1 deletion(-)
 create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h
 create mode 100644 OvmfPkg/QemuVideoDxe/svga_reg.h
 create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
 create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
 create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c
 create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c

-- 
2.3.2 (Apple Git-55)

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 03/29/17 10:19, Phil Dennis-Jordan wrote:
> This extends the QemuVideoDxe driver to support the VMWare SVGA2 display
> device implemented by Qemu. Drivers for this device exist for guest OSes
> which do not support Qemu's other display adapters, so supporting it in
> OVMF is useful in conjunction with those OSes.
> 
> I've tried to follow the existing pattern for device-specific code in
> OVMF's QemuVideoDxe driver as much as possible, with the minimum of
> additional code. I've marked this patch as RFC for 2 main reasons:
> 
> 1. I've imported VMWare's own header file with device register constants
> etc. verbatim. (patch 1/2) This doesn't follow any of the EDK2 coding
> conventions, and it uses the MIT license, not BSD. Only a small percentage
> of symbols are actually used in the driver. On the other hand, it's
> obviously the authoritative source. I'm not sure what the correct
> etiquette is here, define our own constants, or import the authoritative
> header file?

The MIT license is OK (see "OvmfPkg/Contributions.txt").

I strongly prefer hand-crafted, minimal header files in OvmfPkg. I've
done that for all of the virtio stuff, for example.

At least one counter-example exists as well
("OvmfPkg/Include/IndustryStandard/Xen"), and I dislike that very much.

If hand-crafting the minimal required subset is not too much trouble,
and you don't expect frequent updates (header file syncs) from the
public (MIT-licensed) vmware-svga repo, I suggest that you write a brand
new header file, and place it under OvmfPkg/Include/IndustryStandard. As
reference you should indeed identify the original file (preferabily with
a commit hash / SVN revision identifier into the original repo, at which
the file looked like that). I think this new header file would still
qualify as derivative work, so it should be under MIT, and carry both
the original and your (C) notices. I think.

> 
> 2. For the functionality this driver uses, 2 I/O ports are used with
> 32-bit wide reads and writes. Unfortunately, one of them is not 32-bit
> aligned. This is fine as far as x86/x86-64 is concerned, but neither
> EDK2's IoLib nor other platforms support such an access pattern. It seems
> this issue was already encountered/discussed on the edk2-devel list 4
> years ago: http://edk2-devel.narkive.com/bwH3r0us/unaligned-i-o I couldn't
> find any code resulting from that discussion, and Qemu definitely uses
> unaligned port numbers for the SVGA2 device. (SVGA_IO_MUL is 1 in
> hw/display/vmware_vga.c) It does not appear to make any provision for
> non-x86 architectures, so I assume there's no sensible way to drive the
> device in those cases. The patch therefore only detects the device on x86,
> where it uses UnalignedIoWrite/Read32() helper functions which I've based
> on IoLib's aligned ones. I have only tested the GCC version of these.
> Feel free to suggest a better way of handling the issue.

Right now I can only say very generic things about patch 2:

- The idea to pull in & customize the primitives from IoLib matches
Jordan's idea from 4 years ago, so I think it's sane. Please do that in
a separate patch however.

- In the UnalignedIoRead32() and UnalignedIoWrite32() functions, you
should always return a value, even if you ASSERT(FALSE) first. Those
asserts can be compiled out.

- QemuVideDxe is used by ArmVirtPkg as well (it works OK on x86 TCG --
it's broken on aarch64 KVM); have you build tested the change with that
platform?

More on this later I hope.
Laszlo

> 
> Github feature branch: https://github.com/pmj/edk2/tree/ovmf_vmware_svga2_v1
> 
> Phil Dennis-Jordan (2):
>   OvmfPkg: Add SVGA2 device register definition header from VMWare
>   OvmfPkg: Add VMWare SVGA II support in QemuVideoDxe.
> 
>  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf         |    6 +
>  OvmfPkg/QemuVideoDxe/Qemu.h                   |   50 +
>  OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h    |   51 +
>  OvmfPkg/QemuVideoDxe/svga_reg.h               | 1558 ++++++++++++++++++++
>  OvmfPkg/QemuVideoDxe/Driver.c                 |   67 +
>  OvmfPkg/QemuVideoDxe/Gop.c                    |   71 +-
>  OvmfPkg/QemuVideoDxe/Initialize.c             |   88 ++
>  OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c         |   59 +
>  OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c         |   79 +
>  OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c         |   81 +
>  OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c |   53 +
>  11 files changed, 2162 insertions(+), 1 deletion(-)
>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h
>  create mode 100644 OvmfPkg/QemuVideoDxe/svga_reg.h
>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c
>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On Thu, Mar 30, 2017 at 2:33 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/29/17 10:19, Phil Dennis-Jordan wrote:
>> This extends the QemuVideoDxe driver to support the VMWare SVGA2 display
>> device implemented by Qemu. Drivers for this device exist for guest OSes
>> which do not support Qemu's other display adapters, so supporting it in
>> OVMF is useful in conjunction with those OSes.
>>
>> I've tried to follow the existing pattern for device-specific code in
>> OVMF's QemuVideoDxe driver as much as possible, with the minimum of
>> additional code. I've marked this patch as RFC for 2 main reasons:
>>
>> 1. I've imported VMWare's own header file with device register constants
>> etc. verbatim. (patch 1/2) This doesn't follow any of the EDK2 coding
>> conventions, and it uses the MIT license, not BSD. Only a small percentage
>> of symbols are actually used in the driver. On the other hand, it's
>> obviously the authoritative source. I'm not sure what the correct
>> etiquette is here, define our own constants, or import the authoritative
>> header file?
>
> The MIT license is OK (see "OvmfPkg/Contributions.txt").
>
> I strongly prefer hand-crafted, minimal header files in OvmfPkg. I've
> done that for all of the virtio stuff, for example.
>
> At least one counter-example exists as well
> ("OvmfPkg/Include/IndustryStandard/Xen"), and I dislike that very much.
>
> If hand-crafting the minimal required subset is not too much trouble,
> and you don't expect frequent updates (header file syncs) from the
> public (MIT-licensed) vmware-svga repo, I suggest that you write a brand
> new header file, and place it under OvmfPkg/Include/IndustryStandard. As
> reference you should indeed identify the original file (preferabily with
> a commit hash / SVN revision identifier into the original repo, at which
> the file looked like that). I think this new header file would still
> qualify as derivative work, so it should be under MIT, and carry both
> the original and your (C) notices. I think.

OK, I will do exactly that. The number of constants/macros used in the
driver is minimal.

>>
>> 2. For the functionality this driver uses, 2 I/O ports are used with
>> 32-bit wide reads and writes. Unfortunately, one of them is not 32-bit
>> aligned. This is fine as far as x86/x86-64 is concerned, but neither
>> EDK2's IoLib nor other platforms support such an access pattern. It seems
>> this issue was already encountered/discussed on the edk2-devel list 4
>> years ago: http://edk2-devel.narkive.com/bwH3r0us/unaligned-i-o I couldn't
>> find any code resulting from that discussion, and Qemu definitely uses
>> unaligned port numbers for the SVGA2 device. (SVGA_IO_MUL is 1 in
>> hw/display/vmware_vga.c) It does not appear to make any provision for
>> non-x86 architectures, so I assume there's no sensible way to drive the
>> device in those cases. The patch therefore only detects the device on x86,
>> where it uses UnalignedIoWrite/Read32() helper functions which I've based
>> on IoLib's aligned ones. I have only tested the GCC version of these.
>> Feel free to suggest a better way of handling the issue.
>
> Right now I can only say very generic things about patch 2:
>
> - The idea to pull in & customize the primitives from IoLib matches
> Jordan's idea from 4 years ago, so I think it's sane. Please do that in
> a separate patch however.

Sure, makes sense.

> - In the UnalignedIoRead32() and UnalignedIoWrite32() functions, you
> should always return a value, even if you ASSERT(FALSE) first. Those
> asserts can be compiled out.
>
> - QemuVideDxe is used by ArmVirtPkg as well (it works OK on x86 TCG --
> it's broken on aarch64 KVM); have you build tested the change with that
> platform?

Thanks for the pointer, I'll check out the ArmVirtPkg tutorial and try
to get a buildchain and testing environment set up for that. aarch64
TCG should hopefully suffice, I don't have access to an ARM system
capable of running KVM.

> More on this later I hope.

I'll fix up these high level issues and submit a new patchset in a few
days, no need to review the driver in depth until I've done that.

Thanks,
Phil


> Laszlo
>
>>
>> Github feature branch: https://github.com/pmj/edk2/tree/ovmf_vmware_svga2_v1
>>
>> Phil Dennis-Jordan (2):
>>   OvmfPkg: Add SVGA2 device register definition header from VMWare
>>   OvmfPkg: Add VMWare SVGA II support in QemuVideoDxe.
>>
>>  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf         |    6 +
>>  OvmfPkg/QemuVideoDxe/Qemu.h                   |   50 +
>>  OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h    |   51 +
>>  OvmfPkg/QemuVideoDxe/svga_reg.h               | 1558 ++++++++++++++++++++
>>  OvmfPkg/QemuVideoDxe/Driver.c                 |   67 +
>>  OvmfPkg/QemuVideoDxe/Gop.c                    |   71 +-
>>  OvmfPkg/QemuVideoDxe/Initialize.c             |   88 ++
>>  OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c         |   59 +
>>  OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c         |   79 +
>>  OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c         |   81 +
>>  OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c |   53 +
>>  11 files changed, 2162 insertions(+), 1 deletion(-)
>>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h
>>  create mode 100644 OvmfPkg/QemuVideoDxe/svga_reg.h
>>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
>>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
>>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c
>>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c
>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 03/30/17 12:51, Phil Dennis-Jordan wrote:
> On Thu, Mar 30, 2017 at 2:33 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/29/17 10:19, Phil Dennis-Jordan wrote:
>>> This extends the QemuVideoDxe driver to support the VMWare SVGA2 display
>>> device implemented by Qemu. Drivers for this device exist for guest OSes
>>> which do not support Qemu's other display adapters, so supporting it in
>>> OVMF is useful in conjunction with those OSes.
>>>
>>> I've tried to follow the existing pattern for device-specific code in
>>> OVMF's QemuVideoDxe driver as much as possible, with the minimum of
>>> additional code. I've marked this patch as RFC for 2 main reasons:
>>>
>>> 1. I've imported VMWare's own header file with device register constants
>>> etc. verbatim. (patch 1/2) This doesn't follow any of the EDK2 coding
>>> conventions, and it uses the MIT license, not BSD. Only a small percentage
>>> of symbols are actually used in the driver. On the other hand, it's
>>> obviously the authoritative source. I'm not sure what the correct
>>> etiquette is here, define our own constants, or import the authoritative
>>> header file?
>>
>> The MIT license is OK (see "OvmfPkg/Contributions.txt").
>>
>> I strongly prefer hand-crafted, minimal header files in OvmfPkg. I've
>> done that for all of the virtio stuff, for example.
>>
>> At least one counter-example exists as well
>> ("OvmfPkg/Include/IndustryStandard/Xen"), and I dislike that very much.
>>
>> If hand-crafting the minimal required subset is not too much trouble,
>> and you don't expect frequent updates (header file syncs) from the
>> public (MIT-licensed) vmware-svga repo, I suggest that you write a brand
>> new header file, and place it under OvmfPkg/Include/IndustryStandard. As
>> reference you should indeed identify the original file (preferabily with
>> a commit hash / SVN revision identifier into the original repo, at which
>> the file looked like that). I think this new header file would still
>> qualify as derivative work, so it should be under MIT, and carry both
>> the original and your (C) notices. I think.
> 
> OK, I will do exactly that. The number of constants/macros used in the
> driver is minimal.
> 
>>>
>>> 2. For the functionality this driver uses, 2 I/O ports are used with
>>> 32-bit wide reads and writes. Unfortunately, one of them is not 32-bit
>>> aligned. This is fine as far as x86/x86-64 is concerned, but neither
>>> EDK2's IoLib nor other platforms support such an access pattern. It seems
>>> this issue was already encountered/discussed on the edk2-devel list 4
>>> years ago: http://edk2-devel.narkive.com/bwH3r0us/unaligned-i-o I couldn't
>>> find any code resulting from that discussion, and Qemu definitely uses
>>> unaligned port numbers for the SVGA2 device. (SVGA_IO_MUL is 1 in
>>> hw/display/vmware_vga.c) It does not appear to make any provision for
>>> non-x86 architectures, so I assume there's no sensible way to drive the
>>> device in those cases. The patch therefore only detects the device on x86,
>>> where it uses UnalignedIoWrite/Read32() helper functions which I've based
>>> on IoLib's aligned ones. I have only tested the GCC version of these.
>>> Feel free to suggest a better way of handling the issue.
>>
>> Right now I can only say very generic things about patch 2:
>>
>> - The idea to pull in & customize the primitives from IoLib matches
>> Jordan's idea from 4 years ago, so I think it's sane. Please do that in
>> a separate patch however.
> 
> Sure, makes sense.
> 
>> - In the UnalignedIoRead32() and UnalignedIoWrite32() functions, you
>> should always return a value, even if you ASSERT(FALSE) first. Those
>> asserts can be compiled out.
>>
>> - QemuVideDxe is used by ArmVirtPkg as well (it works OK on x86 TCG --
>> it's broken on aarch64 KVM); have you build tested the change with that
>> platform?
> 
> Thanks for the pointer, I'll check out the ArmVirtPkg tutorial and try
> to get a buildchain and testing environment set up for that. aarch64
> TCG should hopefully suffice, I don't have access to an ARM system
> capable of running KVM.
> 
>> More on this later I hope.
> 
> I'll fix up these high level issues and submit a new patchset in a few
> days, no need to review the driver in depth until I've done that.

Thank you for that; I secretly wished you would do this. I'm grasping at
straws to catch a break in my review load, and any cleanup in these
driver additions before I get to look at them in a bit more depth will
be highly appreciated.

Thanks
Laszlo


> 
> Thanks,
> Phil
> 
> 
>> Laszlo
>>
>>>
>>> Github feature branch: https://github.com/pmj/edk2/tree/ovmf_vmware_svga2_v1
>>>
>>> Phil Dennis-Jordan (2):
>>>   OvmfPkg: Add SVGA2 device register definition header from VMWare
>>>   OvmfPkg: Add VMWare SVGA II support in QemuVideoDxe.
>>>
>>>  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf         |    6 +
>>>  OvmfPkg/QemuVideoDxe/Qemu.h                   |   50 +
>>>  OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h    |   51 +
>>>  OvmfPkg/QemuVideoDxe/svga_reg.h               | 1558 ++++++++++++++++++++
>>>  OvmfPkg/QemuVideoDxe/Driver.c                 |   67 +
>>>  OvmfPkg/QemuVideoDxe/Gop.c                    |   71 +-
>>>  OvmfPkg/QemuVideoDxe/Initialize.c             |   88 ++
>>>  OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c         |   59 +
>>>  OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c         |   79 +
>>>  OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c         |   81 +
>>>  OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c |   53 +
>>>  11 files changed, 2162 insertions(+), 1 deletion(-)
>>>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h
>>>  create mode 100644 OvmfPkg/QemuVideoDxe/svga_reg.h
>>>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
>>>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
>>>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c
>>>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c
>>>
>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel