This series adds driver support for:
- LSI53C1030
- SAS1068
- SAS1068E

These controllers are widely supported by QEMU, VirtualBox and VMWare.
This work is part of the more general agenda of enhancing OVMF boot
device support to have feature parity with SeaBIOS.

We have also developed support for PVSCSI which we will submit in a
separate patch series.

I pushed a copy of these patches to
https://github.com/nikital/edk2/tree/mptscsi_v3

Note that I didn't address Laszlo's comment on v2 about BSD vs
BSD+patent licensing, it needs some internal discussion. I would still
like move forward with the review so I'm submitting v3 with the old
license for now.

v2->v3:
- Change error handling style
- Add comments about target size and zero unused target bytes
- Remove internal Reviewed-by
- Fix problems reported by PatchCheck.py
- Use SetupGit.py

v1->v2:
- Map() DMAed buffers
- Fixed various code convention issues
- Newer debug macros
- Updated INF version

Thanks,
Nikita

Nikita Leshenko (13):
  OvmfPkg/MptScsiDxe: Create empty driver
  OvmfPkg/MptScsiDxe: Install DriverBinding Protocol
  OvmfPkg/MptScsiDxe: Report name of driver
  OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi
  OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU
  OvmfPkg/MptScsiDxe: Report one Target and one LUN
  OvmfPkg/MptScsiDxe: Build DevicePath for discovered devices
  OvmfPkg/MptScsiDxe: Implement GetTargetLun
  OvmfPkg/MptScsiDxe: Open PciIo protocol for later use
  OvmfPkg/MptScsiDxe: Set and restore PCI attributes
  OvmfPkg/MptScsiDxe: Initialize hardware
  OvmfPkg/MptScsiDxe: Implement the PassThru method
  OvmfPkg/MptScsiDxe: Report multiple targets

 .../Include/IndustryStandard/FusionMptScsi.h  |  162 +++
 OvmfPkg/MptScsiDxe/MptScsi.c                  | 1060 +++++++++++++++++
 OvmfPkg/MptScsiDxe/MptScsiDxe.inf             |   46 +
 OvmfPkg/OvmfPkg.dec                           |    7 +
 OvmfPkg/OvmfPkgIa32.dsc                       |    1 +
 OvmfPkg/OvmfPkgIa32.fdf                       |    1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                    |    1 +
 OvmfPkg/OvmfPkgIa32X64.fdf                    |    1 +
 OvmfPkg/OvmfPkgX64.dsc                        |    1 +
 OvmfPkg/OvmfPkgX64.fdf                        |    1 +
 10 files changed, 1281 insertions(+)
 create mode 100644 OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
 create mode 100644 OvmfPkg/MptScsiDxe/MptScsi.c
 create mode 100644 OvmfPkg/MptScsiDxe/MptScsiDxe.inf

-- 
2.20.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#55444): https://edk2.groups.io/g/devel/message/55444
Mute This Topic: https://groups.io/mt/71733479/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 03/04/20 20:22, Nikita Leshenko wrote:
> This series adds driver support for:
> - LSI53C1030
> - SAS1068
> - SAS1068E
>
> These controllers are widely supported by QEMU, VirtualBox and VMWare.
> This work is part of the more general agenda of enhancing OVMF boot
> device support to have feature parity with SeaBIOS.
>
> We have also developed support for PVSCSI which we will submit in a
> separate patch series.
>
> I pushed a copy of these patches to
> https://github.com/nikital/edk2/tree/mptscsi_v3
>
> Note that I didn't address Laszlo's comment on v2 about BSD vs
> BSD+patent licensing, it needs some internal discussion. I would still
> like move forward with the review so I'm submitting v3 with the old
> license for now.

Sorry, this doesn't work for me.

You seem to have removed the old "Contributed-under: TianoCore
Contribution Agreement 1.1" lines from the commit messages, and that's
great.

(My understanding is that those lines are now deal-breakers, because
said "Contribution Agreement" is no longer in effect, or even described
in the project, except in the "License-History.txt" file.)

What does not work for me is reviewing a patch set that the submitter
*knows* is unmergeable. I absolutely don't have time for that. Please
submit a patch set that you honestly believe can be merged as-is.

To be clear, the 2-Clause BSD License (SPDX short identifier:
BSD-2-Clause) *is* acceptable, according to "Readme.md"; and I'm not
trying to force you to contribute under "SPDX-License-Identifier:
BSD-2-Clause-Patent".

However, I explained at [1] that "Readme.md" contains the following
passage:

> The majority of the content in the EDK II open source project uses a
> [BSD-2-Clause Plus Patent License](License.txt).  The EDK II open source project
> contains the following components that are covered by additional licenses:
> * [...]
> * [OvmfPkg](OvmfPkg/License.txt)
> * [...]

and I asked that you please extend "OvmfPkg/License.txt", should you
prefer to make this contribution under "BSD-2-Clause".

[1] http://mid.mail-archive.com/a202d92e-61e1-187b-be47-e60ad282c575@redhat.com
    https://edk2.groups.io/g/devel/message/55049

This (v3) posting is under "BSD-2-Clause" (which is fine), but the
cumulative diffstat does not mention "OvmfPkg/License.txt", against my
express request. Similarly counter to my express request, you have not
adopted the SPDX notation even for "BSD-2-Clause".

I think you may have thought that we could make progress on the
technical details while you figured out your preferred license, and in
the end, you'd *unconditionally* repost the series (even if it were
technically perfect at v3), with one of the following modifications:

- you'd stick with "BSD-2-Clause", and extend "OvmfPkg/License.txt",

- or else you'd switch to "BSD-2-Clause-Patent".

To be clear, this approach does not work for me. I don't have time for
spurious reviews. When you post v(n+1) of the series, I have to:

- fetch that from your repo and/or apply it from the list,

- pull up my review notes that I had given for v(n),

- compare every single patch in the v(n+1) series against the v(n)
  counterpart, and verify that your changes are in sync with my requests
  -- even if my only feedback for v(n) was a "Reviewed-by",

- and generally page-in the whole topic against a "cold cache", possibly
  from a distance of a week or more.

I can't do this *spuriously*. The bottleneck is at the review side, not
at the contribution side.

Of course, people do sometimes post RFC patches (marked as such). That's
a great tool to discuss prototypes and new ideas. I give RFC series a
*fraction* of the attention that I give to real PATCH series. I might
ignore RFCs completely.

Please post v4 with either the license flipped to "BSD-2-Clause-Patent",
or with "OvmfPkg/License.txt" modified. If you need time to decide,
please post v4 when you have decided.

Per <https://bugzilla.tianocore.org/show_bug.cgi?id=2390>:

- Your v1 posting was in January 2019,

- you announced starting work on v2 in November 2019,

- you posted v2 in February 2020.

In response to every one of the v1 through v3 postings, I followed up in
at most 3 days, as much as I can tell. I think we can now wait for a
week or two until your Legal Department figures out the license under
which they would like you to make this contribution.

Thanks,
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#55559): https://edk2.groups.io/g/devel/message/55559
Mute This Topic: https://groups.io/mt/71733479/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Nikita,

On 03/04/20 20:22, Nikita Leshenko wrote:
> This series adds driver support for:
> - LSI53C1030
> - SAS1068
> - SAS1068E
> 
> These controllers are widely supported by QEMU, VirtualBox and VMWare.
> This work is part of the more general agenda of enhancing OVMF boot
> device support to have feature parity with SeaBIOS.
> 
> We have also developed support for PVSCSI which we will submit in a
> separate patch series.

I'd like to learn more of this general agenda ("feature parity with
SeaBIOS"). I have never felt the need for any SCSI controller offered by
QEMU other than virtio-scsi.

Because you guys are contributing Fusion-MPT and PVSCSI to OVMF,
obviously such a practical need must exist ("feature parity with
SeaBIOS" is vague, I'm not really buying it :) ).

So I have two requests:

(1) please describe the actual use case (hypervisor, guest OS, maybe
performance, etc) for these drivers, in the associated bugzilla,

(2) please make the inclusion of these drivers in the OVMF DSC and FDF
files dependent on a new build flag (-D). It's up to you whether you
want to gate PVSCSI and Fusion-MPT with the same flag, or if you want to
assign separate flags to them.

It's fine if the default value is TRUE, for that flag (those flags). I'm
asking for them from a downstream perspective -- some distros follow a
"we ship it, we support it" model, and so they must be careful with
rebases to new usptream releases. I'd like to permit such downstreams to
easily disable these drivers, with just -D flags, without
downstream-only patches for the OVMF DSC and FDF files, that might need
repeated downstream rebasing and review.

Again, it's perfectly fine if the upstream defaults are TRUE.

... If you feel tempted to point out the Xen paravirt drivers: you are
entirely right, but those are already covered by
<https://bugzilla.tianocore.org/show_bug.cgi?id=2122>.

Thanks!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#55618): https://edk2.groups.io/g/devel/message/55618
Mute This Topic: https://groups.io/mt/71733479/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Lazlo,

On 06/03/2020 22:14, Laszlo Ersek wrote:
> Hi Nikita,
>
> On 03/04/20 20:22, Nikita Leshenko wrote:
>> This series adds driver support for:
>> - LSI53C1030
>> - SAS1068
>> - SAS1068E
>>
>> These controllers are widely supported by QEMU, VirtualBox and VMWare.
>> This work is part of the more general agenda of enhancing OVMF boot
>> device support to have feature parity with SeaBIOS.
>>
>> We have also developed support for PVSCSI which we will submit in a
>> separate patch series.
> I'd like to learn more of this general agenda ("feature parity with
> SeaBIOS"). I have never felt the need for any SCSI controller offered by
> QEMU other than virtio-scsi.
>
> Because you guys are contributing Fusion-MPT and PVSCSI to OVMF,
> obviously such a practical need must exist ("feature parity with
> SeaBIOS" is vague, I'm not really buying it :) ).

The motivation behind supporting booting from these devices is being 
able to run VMs that originally run on other hypervisors, such as VMware 
ESXi, on top of QEMU/KVM without any changes to the image (In contrast 
to approaches such as virt-v2v, Amazon CloudEndure or Google 
Velostrata). This technology was developed in Ravello Systems (Acquired 
by Oracle), which offered a product to run VMware-based images on top of 
any public cloud without any modification to the VMs.
This is also why you would also see QEMU PVSCSI device emulation and 
SeaBIOS PVSCSI driver were contributed by Ravello as-well.

Similar work to Ravello was done by Virtuozzo for Hyper-V based VMs (See 
their enhancements to QEMU of emulating Hyper-V PV devices). They also 
have EDK2 drivers for these devices that are not upstream.

Having said that, it is true that these two patch-series provide feature 
parity with SeaBIOS. I do believe that it makes sense OVMF will support 
booting from any storage device that QEMU is able to emulate. Especially 
if SeaBIOS is already able to boot from that device as-well.

>
> So I have two requests:
>
> (1) please describe the actual use case (hypervisor, guest OS, maybe
> performance, etc) for these drivers, in the associated bugzilla,

I don't think the description we have written in the relevant BugZilla 
tickets should change much. It's still true that these are contributed 
as part of feature-parity with SeaBIOS and allowing to boot from these 
devices. How one use this depends on it's own use-case. It could be to 
be able to boot VMware-based VMs as-is under QEMU/KVM, but it could also 
be used to boot a VM with a PVSCSI controller that was originally 
created with QEMU/KVM.

This is similar to upstream commit c137d9508169 ("OvmfPkg/QemuVideoDxe: 
VMWare SVGA device support") which added support for VMware-SVGA GPU.

>
> (2) please make the inclusion of these drivers in the OVMF DSC and FDF
> files dependent on a new build flag (-D). It's up to you whether you
> want to gate PVSCSI and Fusion-MPT with the same flag, or if you want to
> assign separate flags to them.
>
> It's fine if the default value is TRUE, for that flag (those flags). I'm
> asking for them from a downstream perspective -- some distros follow a
> "we ship it, we support it" model, and so they must be careful with
> rebases to new usptream releases. I'd like to permit such downstreams to
> easily disable these drivers, with just -D flags, without
> downstream-only patches for the OVMF DSC and FDF files, that might need
> repeated downstream rebasing and review.
>
> Again, it's perfectly fine if the upstream defaults are TRUE.
>
> ... If you feel tempted to point out the Xen paravirt drivers: you are
> entirely right, but those are already covered by
> <https://urldefense.com/v3/__https://bugzilla.tianocore.org/show_bug.cgi?id=2122__;!!GqivPVa7Brio!JJpn5PCAL9AX7xdt4qLhQJMF7L19-QEEbGF_o9bHTGsccK6a5AeDD0-_j3_UPCk$ >.

Funny. I was about to point out that the Xen PV drivers are not 
conditioned by any flag! :)

OK. I think it's appropriate we will add a separate flag for each device 
and make them both TRUE by default (As I think should be appropriate for 
any device built-in by default into QEMU).

Thanks,
-Liran

>
> Thanks!
> Laszlo
>

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#55620): https://edk2.groups.io/g/devel/message/55620
Mute This Topic: https://groups.io/mt/71733479/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 03/06/20 22:52, Liran Alon wrote:
> 
> Hi Lazlo,

("Laszlo")

> On 06/03/2020 22:14, Laszlo Ersek wrote:
>> Hi Nikita,
>>
>> On 03/04/20 20:22, Nikita Leshenko wrote:
>>> This series adds driver support for:
>>> - LSI53C1030
>>> - SAS1068
>>> - SAS1068E
>>>
>>> These controllers are widely supported by QEMU, VirtualBox and VMWare.
>>> This work is part of the more general agenda of enhancing OVMF boot
>>> device support to have feature parity with SeaBIOS.
>>>
>>> We have also developed support for PVSCSI which we will submit in a
>>> separate patch series.
>> I'd like to learn more of this general agenda ("feature parity with
>> SeaBIOS"). I have never felt the need for any SCSI controller offered by
>> QEMU other than virtio-scsi.
>>
>> Because you guys are contributing Fusion-MPT and PVSCSI to OVMF,
>> obviously such a practical need must exist ("feature parity with
>> SeaBIOS" is vague, I'm not really buying it :) ).
> 
> The motivation behind supporting booting from these devices is being
> able to run VMs that originally run on other hypervisors, such as VMware
> ESXi, on top of QEMU/KVM without any changes to the image (In contrast
> to approaches such as virt-v2v, Amazon CloudEndure or Google
> Velostrata). This technology was developed in Ravello Systems (Acquired
> by Oracle), which offered a product to run VMware-based images on top of
> any public cloud without any modification to the VMs.
> This is also why you would also see QEMU PVSCSI device emulation and
> SeaBIOS PVSCSI driver were contributed by Ravello as-well.
> 
> Similar work to Ravello was done by Virtuozzo for Hyper-V based VMs (See
> their enhancements to QEMU of emulating Hyper-V PV devices). They also
> have EDK2 drivers for these devices that are not upstream.

Thank you for the detailed explanation. Very helpful.

> 
> Having said that, it is true that these two patch-series provide feature
> parity with SeaBIOS. I do believe that it makes sense OVMF will support
> booting from any storage device that QEMU is able to emulate. Especially
> if SeaBIOS is already able to boot from that device as-well.

Yes, as long as a user can control relatively fine-grained what support
they'd like to include in the firmware binary.

> 
>>
>> So I have two requests:
>>
>> (1) please describe the actual use case (hypervisor, guest OS, maybe
>> performance, etc) for these drivers, in the associated bugzilla,
> 
> I don't think the description we have written in the relevant BugZilla
> tickets should change much. It's still true that these are contributed
> as part of feature-parity with SeaBIOS and allowing to boot from these
> devices. How one use this depends on it's own use-case. It could be to
> be able to boot VMware-based VMs as-is under QEMU/KVM, but it could also
> be used to boot a VM with a PVSCSI controller that was originally
> created with QEMU/KVM.

Right. All I'm going to do now is to link your message in the mailing
list archive in a new BZ comment (on both BZs).

> This is similar to upstream commit c137d9508169 ("OvmfPkg/QemuVideoDxe:
> VMWare SVGA device support") which added support for VMware-SVGA GPU.

Yes, I agree about the similarity.

(Two side comments about that commit:

- the commit explains the motivation in the message ("Drivers for this
device exist for some guest OSes which do not support Qemu's other
display adapters, so supporting it in OVMF is useful in conjunction with
those OSes").

- that commit ended up being reverted (even though not because the use
case got invalidated, but because it was replaced by a simpler solution
-- see commit range 7f3b0bad4bbb..d021868ccf49).)

> 
>>
>> (2) please make the inclusion of these drivers in the OVMF DSC and FDF
>> files dependent on a new build flag (-D). It's up to you whether you
>> want to gate PVSCSI and Fusion-MPT with the same flag, or if you want to
>> assign separate flags to them.
>>
>> It's fine if the default value is TRUE, for that flag (those flags). I'm
>> asking for them from a downstream perspective -- some distros follow a
>> "we ship it, we support it" model, and so they must be careful with
>> rebases to new usptream releases. I'd like to permit such downstreams to
>> easily disable these drivers, with just -D flags, without
>> downstream-only patches for the OVMF DSC and FDF files, that might need
>> repeated downstream rebasing and review.
>>
>> Again, it's perfectly fine if the upstream defaults are TRUE.
>>
>> ... If you feel tempted to point out the Xen paravirt drivers: you are
>> entirely right, but those are already covered by
>> <https://urldefense.com/v3/__https://bugzilla.tianocore.org/show_bug.cgi?id=2122__;!!GqivPVa7Brio!JJpn5PCAL9AX7xdt4qLhQJMF7L19-QEEbGF_o9bHTGsccK6a5AeDD0-_j3_UPCk$
>> >.
> 
> Funny. I was about to point out that the Xen PV drivers are not
> conditioned by any flag! :)

:)

> OK. I think it's appropriate we will add a separate flag for each device
> and make them both TRUE by default (As I think should be appropriate for
> any device built-in by default into QEMU).

Thank you very much!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#55619): https://edk2.groups.io/g/devel/message/55619
Mute This Topic: https://groups.io/mt/71733479/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-