|
[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 Fri, Sep 5, 2014 at 6:34 AM, Anthony PERARD
<anthony.perard@xxxxxxxxxx> wrote:
> 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.
I don't believe we have any feature differences like this between IA32 and X64.
I often just merge changes across OvmfPkg/*.dsc and *.fdf, so I try to
keep the differences to a minimum. In fact, I've often thought about
adding that pesky --keepexceptiontable section to the IA32 file just
for consistency. :)
> 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.
Can the Xen include files use #pragma pack(1)? I think this has been
supported on GCC for quite some time.
> For Ia32X64, I probably just don't understand what it's suppose to mean.
For the early PEI (Pre-EFI Initialization) boot phase we use IA32, and
then transition to X64 when transitioning to DXE. This is used often
in real-world UEFI systems, because IA32 uses less space. (Code is
smaller, and doesn't have a page tables requirement for flat mode so
IA32 flat mode can easily run from flash.)
The important note for Ia32X64 is that data structures communicated
between PEI and DXE cannot use pointers or UINTN since the size be
different.
> 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)
No, I'm pretty sure I saw MIT license headers on the .h files. By the
way, this series is why I pushed for us to update the
Contributions.txt files to include that MIT contributions can be
accepted.
If you aren't sure about the licenses on the files, then I'm not going
to feel very confident in your series... :)
> are BSD-2 clause licenses. I will check that.
Could you note in the commit message when non-2-clause BSD code is being added?
Could you add any non-2-clause BSD licenses under License.txt?
You might be able to use wildcards to point out the files. Or maybe we
could just add a note that some files are MIT licensed as indicated in
the files' contents.
-Jordan
>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |