|
[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 Thu, Sep 04, 2014 at 08:01:07PM +0200, Laszlo Ersek wrote:
> 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.
I understand that this patch series is big. I can always try to split
some patches, if it can help.
They are already some documentation about the interface in every headers
imported from the Xen Project, in OvmfPkg/Include/IndustryStandard/Xen/.
Also, some of the .c have a description of what it is used for (in the
file header).
I'm not sure what you are asking for. So, what more is needed? Where
this documentation should be? And what do you mean by Xen internals?
> 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.
I forgot to talk about this in the first mail. I tried to make 32bit
build works, but I run into an issue, which is due to the way OVMF is
built.
OVMF, at least on Linux, use the gcc options "-malign-double". This
makes the alignment of some field in struct been different than what
Xen and backends expect. To fix this, we would need to go through every
struct in IndustryStandard/Xen/*.h and check that -malign-double would
not change the alignment. I haven't done that yet. That's for Ia32.
For Ia32X64, I probably just don't understand what it's suppose to mean.
But if you say that a "64bit DXE", then I guess that the driver will
work fine. I will give a try.
> - 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).
OK, will do.
> - 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.
OK, I will see what I can do. I think most of the licenses (if not all)
are BSD-2 clause licenses. I will check that.
> Once these are cleared, you have my Acked-by. (And Jordan will decide if
> that's worth anything ;))
Thanks,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |