|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [edk2] [PATCH v2 00/18] Introducing Xen PV block driver to OVMF
On 09/04/14 18:50, Anthony PERARD wrote:
> Hi all,
>
> This patch series is implementing the necessary in order to access a PV block
> device. For that, one need a XenStore client, a XenBus client, and the PV
> block
> driver.
>
> There are two new drivers, XenbusDxe and XenPvBlkDxe. The first one implement
> a
> bus drivers, and the second is a block drivers.
>
> The GUID for the drivers XenBusDxe and XenPvBlkDxe and for the protocol XenBus
> have been genereted using the UefiDriverWizard.
>
> There are quite a lot of changes by patches since this series first hit the
> mailing-list, all changes are listed in every patches, after a '---' line.
>
> Anthony PERARD (18):
> OvmfPkg: Add public headers from Xen Project.
> OvmfPkg: Add basic skeleton for the XenBus bus driver.
> OvmfPkg/XenBusDxe: Add device state struct and create an ExitBoot
> services event.
> OvmfPkg/XenBusDxe: Add support to make Xen Hypercalls.
> OvmfPkg/XenBusDxe: Open PciIo protocol.
> OvmfPkg: Introduce XenBus Protocol.
> OvmfPkg/XenBusDxe: Add InterlockedCompareExchange16.
> OvmfPkg/XenBusDxe: Add Grant Table functions.
> OvmfPkg/XenBusDxe: Add Event Channel Notify.
> OvmfPkg/XenBusDxe: Add TestAndClearBit.
> OvmfPkg/XenBusDxe: Add XenStore client implementation
> OvmfPkg/XenBusDxe: Add an helper AsciiStrDup.
> OvmfPkg/XenBusDxe: Add XenStore function into the XenBus protocol
> OvmfPkg/XenBusDxe: Indroduce XenBus support itself.
> OvmfPkg/XenBusDxe: Add Event Channel into XenBus protocol.
> OvmfPkg/XenPvBlkDxe: Xen PV Block device, initial skeleton
> OvmfPkg/XenPvBlkDxe: Add BlockFront client.
> OvmfPkg/XenPvBlkDxe: Add BlockIo.
>
> .../IndustryStandard/Xen/arch-x86/xen-x86_32.h | 171 +++
> .../IndustryStandard/Xen/arch-x86/xen-x86_64.h | 202 +++
> .../Include/IndustryStandard/Xen/arch-x86/xen.h | 273 ++++
> .../Include/IndustryStandard/Xen/event_channel.h | 381 +++++
> OvmfPkg/Include/IndustryStandard/Xen/grant_table.h | 662 +++++++++
> OvmfPkg/Include/IndustryStandard/Xen/hvm/hvm_op.h | 275 ++++
> OvmfPkg/Include/IndustryStandard/Xen/hvm/params.h | 150 ++
> OvmfPkg/Include/IndustryStandard/Xen/io/blkif.h | 608 ++++++++
> .../Include/IndustryStandard/Xen/io/protocols.h | 40 +
> OvmfPkg/Include/IndustryStandard/Xen/io/ring.h | 312 +++++
> OvmfPkg/Include/IndustryStandard/Xen/io/xenbus.h | 80 ++
> OvmfPkg/Include/IndustryStandard/Xen/io/xs_wire.h | 138 ++
> OvmfPkg/Include/IndustryStandard/Xen/memory.h | 480 +++++++
> OvmfPkg/Include/IndustryStandard/Xen/sched.h | 174 +++
> OvmfPkg/Include/IndustryStandard/Xen/trace.h | 310 ++++
> OvmfPkg/Include/IndustryStandard/Xen/xen-compat.h | 44 +
> OvmfPkg/Include/IndustryStandard/Xen/xen.h | 897 ++++++++++++
> OvmfPkg/Include/Protocol/XenBus.h | 254 ++++
> OvmfPkg/OvmfPkg.dec | 1 +
> OvmfPkg/OvmfPkgX64.dsc | 2 +
> OvmfPkg/OvmfPkgX64.fdf | 2 +
> OvmfPkg/XenBusDxe/ComponentName.c | 190 +++
> OvmfPkg/XenBusDxe/ComponentName.h | 110 ++
> OvmfPkg/XenBusDxe/DriverBinding.h | 144 ++
> OvmfPkg/XenBusDxe/EventChannel.c | 104 ++
> OvmfPkg/XenBusDxe/EventChannel.h | 98 ++
> OvmfPkg/XenBusDxe/GrantTable.c | 217 +++
> OvmfPkg/XenBusDxe/GrantTable.h | 68 +
> OvmfPkg/XenBusDxe/Helpers.c | 9 +
> OvmfPkg/XenBusDxe/InterlockedCompareExchange16.h | 15 +
> .../XenBusDxe/InterlockedCompareExchange16Intel.c | 32 +
> .../XenBusDxe/X64/InterlockedCompareExchange16.asm | 41 +
> .../XenBusDxe/X64/InterlockedCompareExchange16.c | 41 +
> OvmfPkg/XenBusDxe/X64/TestAndClearBit.S | 13 +
> OvmfPkg/XenBusDxe/X64/TestAndClearBit.asm | 16 +
> OvmfPkg/XenBusDxe/X64/hypercall.S | 23 +
> OvmfPkg/XenBusDxe/X64/hypercall.asm | 27 +
> OvmfPkg/XenBusDxe/XenBus.c | 371 +++++
> OvmfPkg/XenBusDxe/XenBus.h | 67 +
> OvmfPkg/XenBusDxe/XenBusDxe.c | 482 +++++++
> OvmfPkg/XenBusDxe/XenBusDxe.h | 159 +++
> OvmfPkg/XenBusDxe/XenBusDxe.inf | 78 ++
> OvmfPkg/XenBusDxe/XenHypercall.c | 134 ++
> OvmfPkg/XenBusDxe/XenHypercall.h | 100 ++
> OvmfPkg/XenBusDxe/XenStore.c | 1480
> ++++++++++++++++++++
> OvmfPkg/XenBusDxe/XenStore.h | 379 +++++
> OvmfPkg/XenPvBlkDxe/BlockFront.c | 624 +++++++++
> OvmfPkg/XenPvBlkDxe/BlockFront.h | 87 ++
> OvmfPkg/XenPvBlkDxe/BlockIo.c | 292 ++++
> OvmfPkg/XenPvBlkDxe/BlockIo.h | 124 ++
> OvmfPkg/XenPvBlkDxe/ComponentName.c | 192 +++
> OvmfPkg/XenPvBlkDxe/ComponentName.h | 110 ++
> OvmfPkg/XenPvBlkDxe/DriverBinding.h | 159 +++
> OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.c | 413 ++++++
> OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.h | 99 ++
> OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf | 80 ++
> 56 files changed, 12034 insertions(+)
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/arch-x86/xen-x86_32.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/arch-x86/xen-x86_64.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/arch-x86/xen.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/event_channel.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/grant_table.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/hvm/hvm_op.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/hvm/params.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/io/blkif.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/io/protocols.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/io/ring.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/io/xenbus.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/io/xs_wire.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/memory.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/sched.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/trace.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/xen-compat.h
> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/xen.h
> create mode 100644 OvmfPkg/Include/Protocol/XenBus.h
> create mode 100644 OvmfPkg/XenBusDxe/ComponentName.c
> create mode 100644 OvmfPkg/XenBusDxe/ComponentName.h
> create mode 100644 OvmfPkg/XenBusDxe/DriverBinding.h
> create mode 100644 OvmfPkg/XenBusDxe/EventChannel.c
> create mode 100644 OvmfPkg/XenBusDxe/EventChannel.h
> create mode 100644 OvmfPkg/XenBusDxe/GrantTable.c
> create mode 100644 OvmfPkg/XenBusDxe/GrantTable.h
> create mode 100644 OvmfPkg/XenBusDxe/Helpers.c
> create mode 100644 OvmfPkg/XenBusDxe/InterlockedCompareExchange16.h
> create mode 100644 OvmfPkg/XenBusDxe/InterlockedCompareExchange16Intel.c
> create mode 100644 OvmfPkg/XenBusDxe/X64/InterlockedCompareExchange16.asm
> create mode 100644 OvmfPkg/XenBusDxe/X64/InterlockedCompareExchange16.c
> create mode 100644 OvmfPkg/XenBusDxe/X64/TestAndClearBit.S
> create mode 100644 OvmfPkg/XenBusDxe/X64/TestAndClearBit.asm
> create mode 100644 OvmfPkg/XenBusDxe/X64/hypercall.S
> create mode 100644 OvmfPkg/XenBusDxe/X64/hypercall.asm
> create mode 100644 OvmfPkg/XenBusDxe/XenBus.c
> create mode 100644 OvmfPkg/XenBusDxe/XenBus.h
> create mode 100644 OvmfPkg/XenBusDxe/XenBusDxe.c
> create mode 100644 OvmfPkg/XenBusDxe/XenBusDxe.h
> create mode 100644 OvmfPkg/XenBusDxe/XenBusDxe.inf
> create mode 100644 OvmfPkg/XenBusDxe/XenHypercall.c
> create mode 100644 OvmfPkg/XenBusDxe/XenHypercall.h
> create mode 100644 OvmfPkg/XenBusDxe/XenStore.c
> create mode 100644 OvmfPkg/XenBusDxe/XenStore.h
> create mode 100644 OvmfPkg/XenPvBlkDxe/BlockFront.c
> create mode 100644 OvmfPkg/XenPvBlkDxe/BlockFront.h
> create mode 100644 OvmfPkg/XenPvBlkDxe/BlockIo.c
> create mode 100644 OvmfPkg/XenPvBlkDxe/BlockIo.h
> create mode 100644 OvmfPkg/XenPvBlkDxe/ComponentName.c
> create mode 100644 OvmfPkg/XenPvBlkDxe/ComponentName.h
> create mode 100644 OvmfPkg/XenPvBlkDxe/DriverBinding.h
> create mode 100644 OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.c
> create mode 100644 OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.h
> create mode 100644 OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
While explicitly deferring to Jordan's judgement, I think:
Obviously, this patchset is unreviewable due to its sheer size. In
addition, as I said before, technical documentation about Xen internals
would have been nice. (I mentioned the virtio spec earlier as an
example.) But I won't pretend such documentation would make me review
the series, and Xen developers don't need that documentation.
OVMF is now at a point where not all of its developers can verify/test
all of its users' use cases. Which should be fine I think.
The patchset looks modular (based on the pathnames it touches); it
doesn't seem to modify "common" or "foundational" (platform or generic
qemu) code. I think it has minimal capacity to cause regressions in
other code, and even if it should, excluding the new code from the build
would be trivial for downstreams, and easily addressable in upstream
too, with a new build flag and some !ifdefs. (Anyway let's hope that
will never happen.) I do see the importance of getting this into OVMF
and to allow Xen developers to refine it incrementally if necessary.
I can see the VendorId and DeviceId check in
XenBusDxeDriverBindingSupported() (patch 02/18), and the
gXenBusProtocolGuid dependency in XenPvBlkDxeDriverBindingSupported()
(patch 16/18); hence I trust that there won't be any regressions in
driver binding either.
I have the following requests:
- Please justify why only OvmfPkg/OvmfPkgX64.{dsc,fdf} are modified,
and why the code is not built for the "32-bit PEI and DXE" (Ia32) and
the "32-bit PEI and 64-bit DXE" (Ia32X64) cases.
- In patch 06/18, where you introduce XENBUS_PROTOCOL, please consider
adding a banner comment to file "OvmfPkg/Include/Protocol/XenBus.h",
and right above "struct _XENBUS_PROTOCOL", stating that this protocol
is non-portable and internal to edk2. Please see
"OvmfPkg/Include/Protocol/VirtioDevice.h" for an example; when that
file was committed, Jordan insisted on such a disclaimer (and he was
right).
- Please distill the list of the unique copyright licenses that cover
this work. That list should be verified against
OvmfPkg/Contributions.txt. If some of the license types covering this
work are not named explicitly in "OvmfPkg/Contributions.txt" (and are
different from "OvmfPkg/License.txt" as well), then those licenses
will have to be checked by a professional.
Once these are cleared, you have my Acked-by. (And Jordan will decide if
that's worth anything ;))
Thanks,
Laszlo
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |