Implementation of the FW CFG DMA interface.
When running a Linux guest on top of QEMU, using the -kernel options, this
is the timing improvement for x86:
QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
QEMU startup time: .078
BIOS startup time: .060
Kernel setup time: .578
Total time: .716
QEMU with this patch series and SeaBIOS with this patch series
QEMU startup time: .080
BIOS startup time: .039
Kernel setup time: .002
Total time: .121
QEMU startup time is the time between the start and the first kvm_entry.
BIOS startup time is the time between the first kvm_entry and the start of
function do_boot, in SeaBIOS.
Kernel setup time is the time between the start of the function do_boot in
SeaBIOS and the jump to the Linux kernel.
As you can see, both the BIOS (because of ACPI tables and other configurations)
and the Linux kernel boot (because of the copy to memory) are greatly
improved with this new interface.
Also, this new interface is an addon to the old interface. Both interfaces
are compatible and interchangeable.
Add fw_cfg DMA interface specfication in the fw_cfg documentation.
Signed-off-by: Marc Marí <[email protected]>
---
Documentation/devicetree/bindings/arm/fw-cfg.txt | 36 ++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
index 953fb64..c880eec 100644
--- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
+++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
@@ -49,6 +49,41 @@ The guest kernel is not expected to use these registers (although it is
certainly allowed to); the device tree bindings are documented here because
this is where device tree bindings reside in general.
+Starting from revision 2, a DMA interface has also been added. This can be used
+through a write-only, 64-bit wide address register.
+
+In this register, a pointer to a FWCfgDmaAccess structure can be written, in
+big endian mode. This is the format of the FWCfgDmaAccess structure:
+
+typedef struct FWCfgDmaAccess {
+ uint64_t address;
+ uint32_t length;
+ uint32_t control;
+} FWCfgDmaAccess;
+
+Once the address to this structure has been written, an DMA operation is
+started. If the "control" field has value 2, a read operation will be performed.
+"length" bytes for the current selector and offset will be mapped into the
+address specified by the "address" field.
+
+If the field "address" has value 0, the read is considered a skip, and
+the data is not copied anywhere, but the offset is still incremented.
+
+To check result, read the control register:
+ error bit set -> something went wrong.
+ all bits cleared -> transfer finished successfully.
+ otherwise -> transfer still in progress (doesn't happen
+ today due to implementation not being async,
+ but may in the future).
+
+Target address goes up and transfer length goes down as the transfer
+happens, so after a successful transfer the length register is zero
+and the address register points right after the memory block written.
+
+If a partial transfer happened before an error occured the address and
+length registers indicate how much data has been transfered
+successfully.
+
Required properties:
- compatible: "qemu,fw-cfg-mmio".
@@ -56,6 +91,7 @@ Required properties:
- reg: the MMIO region used by the device.
* Bytes 0x0 to 0x7 cover the data register.
* Bytes 0x8 to 0x9 cover the selector register.
+ * From revision 2: Bytes 0xa to 0x11 cover the DMA address register.
* Further registers may be appended to the region in case of future interface
revisions / feature bits.
--
2.4.3
On Thu, Aug 6, 2015 at 12:03 PM, Marc Marí <[email protected]> wrote:
> Add fw_cfg DMA interface specfication in the fw_cfg documentation.
>
> Signed-off-by: Marc Marí <[email protected]>
> ---
> Documentation/devicetree/bindings/arm/fw-cfg.txt | 36 ++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> index 953fb64..c880eec 100644
> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> @@ -49,6 +49,41 @@ The guest kernel is not expected to use these registers (although it is
Please update the "=== Revision (Key 0x0001, FW_CFG_ID) ===" section
to say that currently the revision is 2.
> certainly allowed to); the device tree bindings are documented here because
> this is where device tree bindings reside in general.
>
> +Starting from revision 2, a DMA interface has also been added. This can be used
> +through a write-only, 64-bit wide address register.
> +
> +In this register, a pointer to a FWCfgDmaAccess structure can be written, in
s/pointer/physical RAM address/ is clearer
> +big endian mode. This is the format of the FWCfgDmaAccess structure:
Please be explicit about the *order* of 32-bit writes to the 64-bit
DMA register.
Big-endian only defines the layout of bits but it doesn't say in which
order the two 32-bit sub-registers need to be written.
> +typedef struct FWCfgDmaAccess {
> + uint64_t address;
> + uint32_t length;
> + uint32_t control;
> +} FWCfgDmaAccess;
> +
> +Once the address to this structure has been written, an DMA operation is
> +started. If the "control" field has value 2, a read operation will be performed.
> +"length" bytes for the current selector and offset will be mapped into the
> +address specified by the "address" field.
"mapped" might be confusing. "Copied" or "DMAed" is clearer.
> +If the field "address" has value 0, the read is considered a skip, and
> +the data is not copied anywhere, but the offset is still incremented.
> +
> +To check result, read the control register:
FWCfgDmaAccess.control is not a register, it's a field.
s/register/field/
> + error bit set -> something went wrong.
Which bit number is the error bit?
> + all bits cleared -> transfer finished successfully.
> + otherwise -> transfer still in progress (doesn't happen
> + today due to implementation not being async,
> + but may in the future).
> +
> +Target address goes up and transfer length goes down as the transfer
> +happens, so after a successful transfer the length register is zero
> +and the address register points right after the memory block written.
s/register/field/
On 08/06/15 14:12, Stefan Hajnoczi wrote:
> On Thu, Aug 6, 2015 at 12:03 PM, Marc Marí <[email protected]> wrote:
>> Add fw_cfg DMA interface specfication in the fw_cfg documentation.
>>
>> Signed-off-by: Marc Marí <[email protected]>
>> ---
>> Documentation/devicetree/bindings/arm/fw-cfg.txt | 36 ++++++++++++++++++++++++
>> 1 file changed, 36 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
>> index 953fb64..c880eec 100644
>> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
>> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
>> @@ -49,6 +49,41 @@ The guest kernel is not expected to use these registers (although it is
>
> Please update the "=== Revision (Key 0x0001, FW_CFG_ID) ===" section
> to say that currently the revision is 2.
Sorry I haven't started reading the series yet, but this caught my eye
-- can we agree that this field should be a bitmap instead, and not a
counter-like value? I don't insist of course, because for the current
use case both approaches will work. But, for "future proofing", I think
it is useful to express features independently of each other. (See eg.
virtio feature flags.)
Just an idea.
Thanks
Laszlo
>
>> certainly allowed to); the device tree bindings are documented here because
>> this is where device tree bindings reside in general.
>>
>> +Starting from revision 2, a DMA interface has also been added. This can be used
>> +through a write-only, 64-bit wide address register.
>> +
>> +In this register, a pointer to a FWCfgDmaAccess structure can be written, in
>
> s/pointer/physical RAM address/ is clearer
>
>> +big endian mode. This is the format of the FWCfgDmaAccess structure:
>
> Please be explicit about the *order* of 32-bit writes to the 64-bit
> DMA register.
>
> Big-endian only defines the layout of bits but it doesn't say in which
> order the two 32-bit sub-registers need to be written.
>
>> +typedef struct FWCfgDmaAccess {
>> + uint64_t address;
>> + uint32_t length;
>> + uint32_t control;
>> +} FWCfgDmaAccess;
>> +
>> +Once the address to this structure has been written, an DMA operation is
>> +started. If the "control" field has value 2, a read operation will be performed.
>> +"length" bytes for the current selector and offset will be mapped into the
>> +address specified by the "address" field.
>
> "mapped" might be confusing. "Copied" or "DMAed" is clearer.
>
>> +If the field "address" has value 0, the read is considered a skip, and
>> +the data is not copied anywhere, but the offset is still incremented.
>> +
>> +To check result, read the control register:
>
> FWCfgDmaAccess.control is not a register, it's a field.
>
> s/register/field/
>
>> + error bit set -> something went wrong.
>
> Which bit number is the error bit?
>
>> + all bits cleared -> transfer finished successfully.
>> + otherwise -> transfer still in progress (doesn't happen
>> + today due to implementation not being async,
>> + but may in the future).
>> +
>> +Target address goes up and transfer length goes down as the transfer
>> +happens, so after a successful transfer the length register is zero
>> +and the address register points right after the memory block written.
>
> s/register/field/
>
On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <[email protected]> wrote:
> When running a Linux guest on top of QEMU, using the -kernel options, this
> is the timing improvement for x86:
>
> QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> QEMU startup time: .078
> BIOS startup time: .060
> Kernel setup time: .578
> Total time: .716
>
> QEMU with this patch series and SeaBIOS with this patch series
> QEMU startup time: .080
> BIOS startup time: .039
> Kernel setup time: .002
> Total time: .121
Impressive results!
Is this a fully-featured QEMU build or did you disable things?
Is this the default SeaBIOS build or did you disable things?
Stefan
On Thu, Aug 6, 2015 at 1:22 PM, Laszlo Ersek <[email protected]> wrote:
> On 08/06/15 14:12, Stefan Hajnoczi wrote:
>> On Thu, Aug 6, 2015 at 12:03 PM, Marc Marí <[email protected]> wrote:
>>> Add fw_cfg DMA interface specfication in the fw_cfg documentation.
>>>
>>> Signed-off-by: Marc Marí <[email protected]>
>>> ---
>>> Documentation/devicetree/bindings/arm/fw-cfg.txt | 36 ++++++++++++++++++++++++
>>> 1 file changed, 36 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
>>> index 953fb64..c880eec 100644
>>> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
>>> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
>>> @@ -49,6 +49,41 @@ The guest kernel is not expected to use these registers (although it is
>>
>> Please update the "=== Revision (Key 0x0001, FW_CFG_ID) ===" section
>> to say that currently the revision is 2.
>
> Sorry I haven't started reading the series yet, but this caught my eye
> -- can we agree that this field should be a bitmap instead, and not a
> counter-like value? I don't insist of course, because for the current
> use case both approaches will work. But, for "future proofing", I think
> it is useful to express features independently of each other. (See eg.
> virtio feature flags.)
Good idea.
Stefan
On Thu, 6 Aug 2015 13:27:16 +0100
Stefan Hajnoczi <[email protected]> wrote:
> On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <[email protected]> wrote:
> > When running a Linux guest on top of QEMU, using the -kernel
> > options, this is the timing improvement for x86:
> >
> > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> > QEMU startup time: .078
> > BIOS startup time: .060
> > Kernel setup time: .578
> > Total time: .716
> >
> > QEMU with this patch series and SeaBIOS with this patch series
> > QEMU startup time: .080
> > BIOS startup time: .039
> > Kernel setup time: .002
> > Total time: .121
>
> Impressive results!
>
> Is this a fully-featured QEMU build or did you disable things?
>
> Is this the default SeaBIOS build or did you disable things?
>
This is the default QEMU configuration I get for my system. It's not a
fully-featured QEMU, but it has a lot of things enabled. SeaBIOS
has a default configuration (with debugging disabled).
The QEMU configuration is:
[...]
tcg debug enabled no
gprof enabled no
sparse enabled no
strip binaries yes
profiler no
static build no
pixman system
SDL support yes
GTK support yes
GNUTLS support yes
GNUTLS hash yes
GNUTLS gcrypt no
GNUTLS nettle yes (2.7.1)
VTE support yes
curses support yes
curl support yes
mingw32 support no
Audio drivers oss
Block whitelist (rw)
Block whitelist (ro)
VirtFS support yes
VNC support yes
VNC TLS support yes
VNC SASL support yes
VNC JPEG support yes
VNC PNG support yes
xen support no
brlapi support yes
bluez support yes
Documentation no
GUEST_BASE yes
PIE yes
vde support yes
netmap support no
Linux AIO support yes
ATTR/XATTR support yes
Install blobs yes
KVM support yes
RDMA support yes
TCG interpreter no
fdt support yes
preadv support yes
fdatasync yes
madvise yes
posix_madvise yes
sigev_thread_id yes
uuid support yes
libcap-ng support yes
vhost-net support yes
vhost-scsi support yes
Trace backends nop
spice support yes (0.12.7/0.12.5)
rbd support yes
xfsctl support no
nss used yes
libusb yes
usb net redir yes
OpenGL support no
libiscsi support yes
libnfs support no
build guest agent yes
QGA VSS support no
QGA w32 disk info no
seccomp support yes
coroutine backend ucontext
coroutine pool yes
GlusterFS support yes
Archipelago support no
gcov gcov
gcov enabled no
TPM support yes
libssh2 support yes
TPM passthrough yes
QOM debugging yes
vhdx yes
lzo support yes
snappy support yes
bzip2 support yes
NUMA host support yes
tcmalloc support no
Marc
On Thu, Aug 6, 2015 at 1:37 PM, Marc Marí <[email protected]> wrote:
> On Thu, 6 Aug 2015 13:27:16 +0100
> Stefan Hajnoczi <[email protected]> wrote:
>
>> On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <[email protected]> wrote:
>> > When running a Linux guest on top of QEMU, using the -kernel
>> > options, this is the timing improvement for x86:
>> >
>> > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
>> > QEMU startup time: .078
>> > BIOS startup time: .060
>> > Kernel setup time: .578
>> > Total time: .716
>> >
>> > QEMU with this patch series and SeaBIOS with this patch series
>> > QEMU startup time: .080
>> > BIOS startup time: .039
>> > Kernel setup time: .002
>> > Total time: .121
>>
>> Impressive results!
>>
>> Is this a fully-featured QEMU build or did you disable things?
>>
>> Is this the default SeaBIOS build or did you disable things?
>>
>
> This is the default QEMU configuration I get for my system. It's not a
> fully-featured QEMU, but it has a lot of things enabled. SeaBIOS
> has a default configuration (with debugging disabled).
That's great.
Since SeaBIOS is a default configuration, the remaining BIOS startup
time is amenable to more optimizations in the future.
Stefan
On Thu, Aug 06, 2015 at 01:03:07PM +0200, Marc Mar? wrote:
> Add fw_cfg DMA interface specfication in the fw_cfg documentation.
>
> Signed-off-by: Marc Mar? <[email protected]>
> ---
> Documentation/devicetree/bindings/arm/fw-cfg.txt | 36 ++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> index 953fb64..c880eec 100644
> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> @@ -49,6 +49,41 @@ The guest kernel is not expected to use these registers (although it is
> certainly allowed to); the device tree bindings are documented here because
> this is where device tree bindings reside in general.
>
> +Starting from revision 2, a DMA interface has also been added. This can be used
> +through a write-only, 64-bit wide address register.
> +
> +In this register, a pointer to a FWCfgDmaAccess structure can be written, in
> +big endian mode. This is the format of the FWCfgDmaAccess structure:
s/mode/format/
> +
> +typedef struct FWCfgDmaAccess {
> + uint64_t address;
> + uint32_t length;
> + uint32_t control;
> +} FWCfgDmaAccess;
> +
> +Once the address to this structure has been written, an DMA operation is
> +started. If the "control" field has value 2, a read operation will be performed.
> +"length" bytes for the current selector and offset will be mapped into the
> +address specified by the "address" field.
> +
> +If the field "address" has value 0, the read is considered a skip, and
> +the data is not copied anywhere, but the offset is still incremented.
So we can't DMA to address == 0? That might not generally be a good
idea, but why limit ourselves? Can't we add another control input for skip
instead? Actually, what inputs are accepted now? READ == 2, WRITE? ??
> +
> +To check result, read the control register:
> + error bit set -> something went wrong.
echo Stefan's which bit question, and also what types of errors? If
there are many, then how about an error bit, plus field of bits for
an error code?
> + all bits cleared -> transfer finished successfully.
> + otherwise -> transfer still in progress (doesn't happen
> + today due to implementation not being async,
> + but may in the future).
I don't think we need to point out that this isn't implemented yet, but
may be in the future. If that's how it may work, then let's just document
it. And why not specify an in-progress bit?
> +
> +Target address goes up and transfer length goes down as the transfer
> +happens, so after a successful transfer the length register is zero
> +and the address register points right after the memory block written.
> +
> +If a partial transfer happened before an error occured the address and
> +length registers indicate how much data has been transfered
> +successfully.
> +
> Required properties:
>
> - compatible: "qemu,fw-cfg-mmio".
> @@ -56,6 +91,7 @@ Required properties:
> - reg: the MMIO region used by the device.
> * Bytes 0x0 to 0x7 cover the data register.
> * Bytes 0x8 to 0x9 cover the selector register.
> + * From revision 2: Bytes 0xa to 0x11 cover the DMA address register.
I think we should naturally align the register, i.e we can reserve bytes
0xa - 0xf, and then put the DMA register at 0x10.
> * Further registers may be appended to the region in case of future interface
> revisions / feature bits.
>
> --
> 2.4.3
>
Thanks,
drew
On Thu, 6 Aug 2015 16:08:49 +0200
Andrew Jones <[email protected]> wrote:
> On Thu, Aug 06, 2015 at 01:03:07PM +0200, Marc Marí wrote:
> > Add fw_cfg DMA interface specfication in the fw_cfg documentation.
> >
> > Signed-off-by: Marc Marí <[email protected]>
> > ---
> > Documentation/devicetree/bindings/arm/fw-cfg.txt | 36
> > ++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> > b/Documentation/devicetree/bindings/arm/fw-cfg.txt index
> > 953fb64..c880eec 100644 ---
> > a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++
> > b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -49,6 +49,41
> > @@ The guest kernel is not expected to use these registers
> > (although it is certainly allowed to); the device tree bindings are
> > documented here because this is where device tree bindings reside
> > in general. +Starting from revision 2, a DMA interface has also
> > been added. This can be used +through a write-only, 64-bit wide
> > address register. +
> > +In this register, a pointer to a FWCfgDmaAccess structure can be
> > written, in +big endian mode. This is the format of the
> > FWCfgDmaAccess structure:
> s/mode/format/
> > +
> > +typedef struct FWCfgDmaAccess {
> > + uint64_t address;
> > + uint32_t length;
> > + uint32_t control;
> > +} FWCfgDmaAccess;
> > +
> > +Once the address to this structure has been written, an DMA
> > operation is +started. If the "control" field has value 2, a read
> > operation will be performed. +"length" bytes for the current
> > selector and offset will be mapped into the +address specified by
> > the "address" field. +
> > +If the field "address" has value 0, the read is considered a skip,
> > and +the data is not copied anywhere, but the offset is still
> > incremented.
>
> So we can't DMA to address == 0? That might not generally be a good
> idea, but why limit ourselves? Can't we add another control input for
> skip instead? Actually, what inputs are accepted now? READ == 2,
> WRITE? ??
>
Write was already disabled for PIO:
static void fw_cfg_write(FWCfgState *s, uint8_t value)
{
/* nothing, write support removed in QEMU v2.4+ */
}
> > +
> > +To check result, read the control register:
> > + error bit set -> something went wrong.
> echo Stefan's which bit question, and also what types of errors? If
> there are many, then how about an error bit, plus field of bits for
> an error code?
>
Bit 0. At this moment the only error possible is DMA mapping failure.
But its true that it might be useful to have some bits in the control
field or another field to indicate the type of error, in case of future
extensions.
> > + all bits cleared -> transfer finished successfully.
> > + otherwise -> transfer still in progress (doesn't happen
> > + today due to implementation not being
> > async,
> > + but may in the future).
> I don't think we need to point out that this isn't implemented yet,
> but may be in the future. If that's how it may work, then let's just
> document it. And why not specify an in-progress bit?
Is this a feature we will want?
> > +
> > +Target address goes up and transfer length goes down as the
> > transfer +happens, so after a successful transfer the length
> > register is zero +and the address register points right after the
> > memory block written. +
> > +If a partial transfer happened before an error occured the address
> > and +length registers indicate how much data has been transfered
> > +successfully.
> > +
> > Required properties:
> >
> > - compatible: "qemu,fw-cfg-mmio".
> > @@ -56,6 +91,7 @@ Required properties:
> > - reg: the MMIO region used by the device.
> > * Bytes 0x0 to 0x7 cover the data register.
> > * Bytes 0x8 to 0x9 cover the selector register.
> > + * From revision 2: Bytes 0xa to 0x11 cover the DMA address
> > register.
>
> I think we should naturally align the register, i.e we can reserve
> bytes 0xa - 0xf, and then put the DMA register at 0x10.
>
> > * Further registers may be appended to the region in case of
> > future interface revisions / feature bits.
> >
> > --
> > 2.4.3
> >
>
Thanks
Marc
On Thu, Aug 06, 2015 at 04:19:18PM +0200, Marc Mar? wrote:
> On Thu, 6 Aug 2015 16:08:49 +0200
> Andrew Jones <[email protected]> wrote:
>
> > On Thu, Aug 06, 2015 at 01:03:07PM +0200, Marc Mar? wrote:
> > > Add fw_cfg DMA interface specfication in the fw_cfg documentation.
> > >
> > > Signed-off-by: Marc Mar? <[email protected]>
> > > ---
> > > Documentation/devicetree/bindings/arm/fw-cfg.txt | 36
> > > ++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> > > b/Documentation/devicetree/bindings/arm/fw-cfg.txt index
> > > 953fb64..c880eec 100644 ---
> > > a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++
> > > b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -49,6 +49,41
> > > @@ The guest kernel is not expected to use these registers
> > > (although it is certainly allowed to); the device tree bindings are
> > > documented here because this is where device tree bindings reside
> > > in general. +Starting from revision 2, a DMA interface has also
> > > been added. This can be used +through a write-only, 64-bit wide
> > > address register. +
> > > +In this register, a pointer to a FWCfgDmaAccess structure can be
> > > written, in +big endian mode. This is the format of the
> > > FWCfgDmaAccess structure:
> > s/mode/format/
> > > +
> > > +typedef struct FWCfgDmaAccess {
> > > + uint64_t address;
> > > + uint32_t length;
> > > + uint32_t control;
> > > +} FWCfgDmaAccess;
> > > +
> > > +Once the address to this structure has been written, an DMA
> > > operation is +started. If the "control" field has value 2, a read
> > > operation will be performed. +"length" bytes for the current
> > > selector and offset will be mapped into the +address specified by
> > > the "address" field. +
> > > +If the field "address" has value 0, the read is considered a skip,
> > > and +the data is not copied anywhere, but the offset is still
> > > incremented.
> >
> > So we can't DMA to address == 0? That might not generally be a good
> > idea, but why limit ourselves? Can't we add another control input for
> > skip instead? Actually, what inputs are accepted now? READ == 2,
> > WRITE? ??
> >
>
> Write was already disabled for PIO:
>
> static void fw_cfg_write(FWCfgState *s, uint8_t value)
> {
> /* nothing, write support removed in QEMU v2.4+ */
> }
>
> > > +
> > > +To check result, read the control register:
> > > + error bit set -> something went wrong.
> > echo Stefan's which bit question, and also what types of errors? If
> > there are many, then how about an error bit, plus field of bits for
> > an error code?
> >
>
> Bit 0. At this moment the only error possible is DMA mapping failure.
> But its true that it might be useful to have some bits in the control
> field or another field to indicate the type of error, in case of future
> extensions.
>
> > > + all bits cleared -> transfer finished successfully.
> > > + otherwise -> transfer still in progress (doesn't happen
> > > + today due to implementation not being
> > > async,
> > > + but may in the future).
> > I don't think we need to point out that this isn't implemented yet,
> > but may be in the future. If that's how it may work, then let's just
> > document it. And why not specify an in-progress bit?
>
> Is this a feature we will want?
I don't know. Need firmware person like Laszlo to give an opinion. Maybe
he'd want OVMF/AAVMF to be able to output progress while transferring,
to keep users more patient?
>
> > > +
> > > +Target address goes up and transfer length goes down as the
> > > transfer +happens, so after a successful transfer the length
> > > register is zero +and the address register points right after the
> > > memory block written. +
> > > +If a partial transfer happened before an error occured the address
> > > and +length registers indicate how much data has been transfered
> > > +successfully.
> > > +
> > > Required properties:
> > >
> > > - compatible: "qemu,fw-cfg-mmio".
> > > @@ -56,6 +91,7 @@ Required properties:
> > > - reg: the MMIO region used by the device.
> > > * Bytes 0x0 to 0x7 cover the data register.
> > > * Bytes 0x8 to 0x9 cover the selector register.
> > > + * From revision 2: Bytes 0xa to 0x11 cover the DMA address
> > > register.
> >
> > I think we should naturally align the register, i.e we can reserve
> > bytes 0xa - 0xf, and then put the DMA register at 0x10.
> >
> > > * Further registers may be appended to the region in case of
> > > future interface revisions / feature bits.
> > >
> > > --
> > > 2.4.3
> > >
> >
>
> Thanks
> Marc
On 08/06/15 16:28, Andrew Jones wrote:
> On Thu, Aug 06, 2015 at 04:19:18PM +0200, Marc Mar? wrote:
>> On Thu, 6 Aug 2015 16:08:49 +0200
>> Andrew Jones <[email protected]> wrote:
>>
>>> On Thu, Aug 06, 2015 at 01:03:07PM +0200, Marc Mar? wrote:
>>>> Add fw_cfg DMA interface specfication in the fw_cfg documentation.
>>>>
>>>> Signed-off-by: Marc Mar? <[email protected]>
>>>> ---
>>>> Documentation/devicetree/bindings/arm/fw-cfg.txt | 36
>>>> ++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt
>>>> b/Documentation/devicetree/bindings/arm/fw-cfg.txt index
>>>> 953fb64..c880eec 100644 ---
>>>> a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++
>>>> b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -49,6 +49,41
>>>> @@ The guest kernel is not expected to use these registers
>>>> (although it is certainly allowed to); the device tree bindings are
>>>> documented here because this is where device tree bindings reside
>>>> in general. +Starting from revision 2, a DMA interface has also
>>>> been added. This can be used +through a write-only, 64-bit wide
>>>> address register. +
>>>> +In this register, a pointer to a FWCfgDmaAccess structure can be
>>>> written, in +big endian mode. This is the format of the
>>>> FWCfgDmaAccess structure:
>>> s/mode/format/
>>>> +
>>>> +typedef struct FWCfgDmaAccess {
>>>> + uint64_t address;
>>>> + uint32_t length;
>>>> + uint32_t control;
>>>> +} FWCfgDmaAccess;
>>>> +
>>>> +Once the address to this structure has been written, an DMA
>>>> operation is +started. If the "control" field has value 2, a read
>>>> operation will be performed. +"length" bytes for the current
>>>> selector and offset will be mapped into the +address specified by
>>>> the "address" field. +
>>>> +If the field "address" has value 0, the read is considered a skip,
>>>> and +the data is not copied anywhere, but the offset is still
>>>> incremented.
>>>
>>> So we can't DMA to address == 0? That might not generally be a good
>>> idea, but why limit ourselves? Can't we add another control input for
>>> skip instead? Actually, what inputs are accepted now? READ == 2,
>>> WRITE? ??
>>>
>>
>> Write was already disabled for PIO:
>>
>> static void fw_cfg_write(FWCfgState *s, uint8_t value)
>> {
>> /* nothing, write support removed in QEMU v2.4+ */
>> }
>>
>>>> +
>>>> +To check result, read the control register:
>>>> + error bit set -> something went wrong.
>>> echo Stefan's which bit question, and also what types of errors? If
>>> there are many, then how about an error bit, plus field of bits for
>>> an error code?
>>>
>>
>> Bit 0. At this moment the only error possible is DMA mapping failure.
>> But its true that it might be useful to have some bits in the control
>> field or another field to indicate the type of error, in case of future
>> extensions.
>>
>>>> + all bits cleared -> transfer finished successfully.
>>>> + otherwise -> transfer still in progress (doesn't happen
>>>> + today due to implementation not being
>>>> async,
>>>> + but may in the future).
>>> I don't think we need to point out that this isn't implemented yet,
>>> but may be in the future. If that's how it may work, then let's just
>>> document it. And why not specify an in-progress bit?
>>
>> Is this a feature we will want?
>
> I don't know. Need firmware person like Laszlo to give an opinion. Maybe
> he'd want OVMF/AAVMF to be able to output progress while transferring,
> to keep users more patient?
I don't yet know how to use this interface from the firmware.
FWIW the library interface to fw_cfg that we use in OVMF & AAVMF is
synchronous. (It doesn't make sense to do "something else" until the
fw_cfg transfer completes "in the background".)
So I imagined the new interface would behave in one of the following ways:
(1) I perform the "final" MMIO register write, and bam, by the time the
next instruction is executed, the target memory area is populated.
(2) Or, I perform the same final MMIO register write, and then
busy-loop on reading some other status register, until it tells me
that the transfer is done, or there has been an error.
(Under (2), if there's an error, I'll just log an error message, and
hang the firmware. Our internal library interface doesn't return any
status codes (no errors are possible with the MMIO/PIO interfaces, on
ARM and x86 alike), and I don't think this change warrants updating a
zillion call sites, in order to propagate and handle such DMA errors in
all modules that use fw-cfg. (We always check return values -- unless
the return type is VOID. And it is now.))
Regarding progress info: aside from the kernel and the initrd, the
fw-cfg items (files included) are tiny. (Remember, they were meant as
*config* items :)) So, normally, I don't bother about progress info.
The kernel and the initrd are exceptions. So, at the moment the code
that reads them (which is specific Boot Device Selection library code,
not generic fw-cfg client libary code) breaks up the transfer into
chunks of 1MB, and logs progress info in between. That is, the progress
info is not emitted by the direct fw-cfg client, it's printed by the
specific module that relies on the fw-cfg library.
So, regardless of whether I'll have to write (1) or (2), I expect this
same approach to just work with the DMA interface. The DMA thing will be
hidden inside the library, the API will remain synchronous. At best the
module (in BDS) that downloads the kernel/initrd blobs will print those
1MB progress messages "super fast".
Summary: the only thing I need to know is when a transfer has finished,
successfully or unsuccessfully.
(If a transfer can terminate prematurely, but still successfully -- ie.
the firwmare should contine "with the rest" -- then the exact size of
the short transfer should also be returned. But I don't think short
transfers would be very useful.)
Thanks
Laszlo
>
>>
>>>> +
>>>> +Target address goes up and transfer length goes down as the
>>>> transfer +happens, so after a successful transfer the length
>>>> register is zero +and the address register points right after the
>>>> memory block written. +
>>>> +If a partial transfer happened before an error occured the address
>>>> and +length registers indicate how much data has been transfered
>>>> +successfully.
>>>> +
>>>> Required properties:
>>>>
>>>> - compatible: "qemu,fw-cfg-mmio".
>>>> @@ -56,6 +91,7 @@ Required properties:
>>>> - reg: the MMIO region used by the device.
>>>> * Bytes 0x0 to 0x7 cover the data register.
>>>> * Bytes 0x8 to 0x9 cover the selector register.
>>>> + * From revision 2: Bytes 0xa to 0x11 cover the DMA address
>>>> register.
>>>
>>> I think we should naturally align the register, i.e we can reserve
>>> bytes 0xa - 0xf, and then put the DMA register at 0x10.
>>>
>>>> * Further registers may be appended to the region in case of
>>>> future interface revisions / feature bits.
>>>>
>>>> --
>>>> 2.4.3
>>>>
>>>
>>
>> Thanks
>> Marc
On Thu, 6 Aug 2015 16:55:20 +0200
Laszlo Ersek <[email protected]> wrote:
> On 08/06/15 16:28, Andrew Jones wrote:
> > On Thu, Aug 06, 2015 at 04:19:18PM +0200, Marc Marí wrote:
> >> On Thu, 6 Aug 2015 16:08:49 +0200
> >> Andrew Jones <[email protected]> wrote:
> >>
> >>> On Thu, Aug 06, 2015 at 01:03:07PM +0200, Marc Marí wrote:
> >>>> Add fw_cfg DMA interface specfication in the fw_cfg
> >>>> documentation.
> >>>>
> >>>> Signed-off-by: Marc Marí <[email protected]>
> >>>> ---
> >>>> Documentation/devicetree/bindings/arm/fw-cfg.txt | 36
> >>>> ++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> >>>> b/Documentation/devicetree/bindings/arm/fw-cfg.txt index
> >>>> 953fb64..c880eec 100644 ---
> >>>> a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++
> >>>> b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -49,6
> >>>> +49,41 @@ The guest kernel is not expected to use these registers
> >>>> (although it is certainly allowed to); the device tree bindings
> >>>> are documented here because this is where device tree bindings
> >>>> reside in general. +Starting from revision 2, a DMA interface
> >>>> has also been added. This can be used +through a write-only,
> >>>> 64-bit wide address register. +
> >>>> +In this register, a pointer to a FWCfgDmaAccess structure can be
> >>>> written, in +big endian mode. This is the format of the
> >>>> FWCfgDmaAccess structure:
> >>> s/mode/format/
> >>>> +
> >>>> +typedef struct FWCfgDmaAccess {
> >>>> + uint64_t address;
> >>>> + uint32_t length;
> >>>> + uint32_t control;
> >>>> +} FWCfgDmaAccess;
> >>>> +
> >>>> +Once the address to this structure has been written, an DMA
> >>>> operation is +started. If the "control" field has value 2, a read
> >>>> operation will be performed. +"length" bytes for the current
> >>>> selector and offset will be mapped into the +address specified by
> >>>> the "address" field. +
> >>>> +If the field "address" has value 0, the read is considered a
> >>>> skip, and +the data is not copied anywhere, but the offset is
> >>>> still incremented.
> >>>
> >>> So we can't DMA to address == 0? That might not generally be a
> >>> good idea, but why limit ourselves? Can't we add another control
> >>> input for skip instead? Actually, what inputs are accepted now?
> >>> READ == 2, WRITE? ??
> >>>
> >>
> >> Write was already disabled for PIO:
> >>
> >> static void fw_cfg_write(FWCfgState *s, uint8_t value)
> >> {
> >> /* nothing, write support removed in QEMU v2.4+ */
> >> }
> >>
> >>>> +
> >>>> +To check result, read the control register:
> >>>> + error bit set -> something went wrong.
> >>> echo Stefan's which bit question, and also what types of errors?
> >>> If there are many, then how about an error bit, plus field of
> >>> bits for an error code?
> >>>
> >>
> >> Bit 0. At this moment the only error possible is DMA mapping
> >> failure. But its true that it might be useful to have some bits in
> >> the control field or another field to indicate the type of error,
> >> in case of future extensions.
> >>
> >>>> + all bits cleared -> transfer finished successfully.
> >>>> + otherwise -> transfer still in progress (doesn't
> >>>> happen
> >>>> + today due to implementation not being
> >>>> async,
> >>>> + but may in the future).
> >>> I don't think we need to point out that this isn't implemented
> >>> yet, but may be in the future. If that's how it may work, then
> >>> let's just document it. And why not specify an in-progress bit?
> >>
> >> Is this a feature we will want?
> >
> > I don't know. Need firmware person like Laszlo to give an opinion.
> > Maybe he'd want OVMF/AAVMF to be able to output progress while
> > transferring, to keep users more patient?
>
> I don't yet know how to use this interface from the firmware.
>
> FWIW the library interface to fw_cfg that we use in OVMF & AAVMF is
> synchronous. (It doesn't make sense to do "something else" until the
> fw_cfg transfer completes "in the background".)
>
> So I imagined the new interface would behave in one of the following
> ways: (1) I perform the "final" MMIO register write, and bam, by the
> time the next instruction is executed, the target memory area is
> populated. (2) Or, I perform the same final MMIO register write, and
> then busy-loop on reading some other status register, until it tells
> me that the transfer is done, or there has been an error.
>
It's (2).
If you have library functions such as "fw_cfg_read", just changing
them should leave everything working. You may want to look at the x86
guest (SeaBIOS) to check the few changes necessary:
http://www.seabios.org/pipermail/seabios/2015-August/009605.html
But it's important to note that the host part (QEMU) is implemented
using memcpy. When you implement this interface, you may want to
increase the chunk size.
Thanks
Marc
On Thu, Aug 06, 2015 at 02:37:45PM +0200, Marc Mar? wrote:
> On Thu, 6 Aug 2015 13:27:16 +0100
> Stefan Hajnoczi <[email protected]> wrote:
>
> > On Thu, Aug 6, 2015 at 12:00 PM, Marc Mar? <[email protected]> wrote:
> > > When running a Linux guest on top of QEMU, using the -kernel
> > > options, this is the timing improvement for x86:
> > >
> > > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> > > QEMU startup time: .078
> > > BIOS startup time: .060
> > > Kernel setup time: .578
> > > Total time: .716
> > >
> > > QEMU with this patch series and SeaBIOS with this patch series
> > > QEMU startup time: .080
> > > BIOS startup time: .039
> > > Kernel setup time: .002
> > > Total time: .121
> >
> > Impressive results!
> >
> > Is this a fully-featured QEMU build or did you disable things?
> >
> > Is this the default SeaBIOS build or did you disable things?
> >
>
> This is the default QEMU configuration I get for my system. It's not a
> fully-featured QEMU, but it has a lot of things enabled. SeaBIOS
> has a default configuration (with debugging disabled).
Thanks!
What qemu command-line did you use during testing? Also, do you have
a quick primer on how to use the kvm trace stuff to obtain timings?
-Kevin
On Thu, 6 Aug 2015 11:30:43 -0400
"Kevin O'Connor" <[email protected]> wrote:
> On Thu, Aug 06, 2015 at 02:37:45PM +0200, Marc Marí wrote:
> > On Thu, 6 Aug 2015 13:27:16 +0100
> > Stefan Hajnoczi <[email protected]> wrote:
> >
> > > On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <[email protected]>
> > > wrote:
> > > > When running a Linux guest on top of QEMU, using the -kernel
> > > > options, this is the timing improvement for x86:
> > > >
> > > > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> > > > QEMU startup time: .078
> > > > BIOS startup time: .060
> > > > Kernel setup time: .578
> > > > Total time: .716
> > > >
> > > > QEMU with this patch series and SeaBIOS with this patch series
> > > > QEMU startup time: .080
> > > > BIOS startup time: .039
> > > > Kernel setup time: .002
> > > > Total time: .121
> > >
> > > Impressive results!
> > >
> > > Is this a fully-featured QEMU build or did you disable things?
> > >
> > > Is this the default SeaBIOS build or did you disable things?
> > >
> >
> > This is the default QEMU configuration I get for my system. It's
> > not a fully-featured QEMU, but it has a lot of things enabled.
> > SeaBIOS has a default configuration (with debugging disabled).
>
> Thanks!
>
> What qemu command-line did you use during testing? Also, do you have
> a quick primer on how to use the kvm trace stuff to obtain timings?
>
The command line I used is:
x86_64-softmmu/qemu-system-x86_64 --enable-kvm \
-kernel /boot/vmlinuz-4.0.7-300.fc22.x86_64 \
-L pc-bios/optionrom/ \
-bios roms/seabios/out/bios.bin -nographic
And I used perf (and two out instructions in the BIOS) to measure the
times:
perf record -a -e kvm:\* -e sched:sched_process_exec
And searching for sched:sched_process_exec, kvm:kvm_entry, pio_write at
0xf5 and pio_write at 0xf4. Out at 0xf5 is the one in "do_boot"
function, and out at 0xf4 is the one just before the jump to the Linux
kernel.
I attach the script I've been using. It can be improved, but it works.
It can be run like this:
sudo ../../results/run_test.sh x86_64-softmmu/qemu-system-x86_64 \
--enable-kvm -kernel /boot/vmlinuz-4.0.8-300.fc22.x86_64 \
-L pc-bios/optionrom/ \
-bios roms/seabios/out/bios.bin -nographic
Thanks
Marc
On 08/06/15 13:03, Marc Marí wrote:
> Add fw_cfg DMA interface specfication in the fw_cfg documentation.
>
> Signed-off-by: Marc Marí <[email protected]>
> ---
> Documentation/devicetree/bindings/arm/fw-cfg.txt | 36 ++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> index 953fb64..c880eec 100644
> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> @@ -49,6 +49,41 @@ The guest kernel is not expected to use these registers (although it is
> certainly allowed to); the device tree bindings are documented here because
> this is where device tree bindings reside in general.
>
> +Starting from revision 2, a DMA interface has also been added.
- Should be updated to say "bitmap" etc.
- I think these additions should start one paragraph earlier in the
text file (ie. before the paragraph starting with "The guest kernel
is not expected...")
> This can be used
> +through a write-only, 64-bit wide address register.
Should be updated to two, 32-bit wide registers, defining which half is
most significant and least significant, then the byte order within each
half, and also which is the one that fires off the DMA.
(I think Drew mentioned the same, but I can't find the message.)
> +
> +In this register, a pointer to a FWCfgDmaAccess structure can be written, in
> +big endian mode. This is the format of the FWCfgDmaAccess structure:
Ah okay, big endian is mentioned here.
> +
> +typedef struct FWCfgDmaAccess {
> + uint64_t address;
> + uint32_t length;
> + uint32_t control;
> +} FWCfgDmaAccess;
> +
> +Once the address to this structure has been written, an DMA operation is
> +started. If the "control" field has value 2,
in what byte order?
> a read operation will be performed.
> +"length" bytes
in what byte order?
> for the current selector and offset will be mapped into the
> +address specified by the "address" field.
In what byte order? :)
(To wit, I think that the fw_cfg_dma_transfer() function in "[PATCH 3/5]
Implement fw_cfg DMA interface" lacks translation from guest endianness
to host endianness, for the fields of FWCfgDmaAccess.)
> +
> +If the field "address" has value 0, the read is considered a skip, and
> +the data is not copied anywhere, but the offset is still incremented.
> +
> +To check result, read the control register:
> + error bit set -> something went wrong.
> + all bits cleared -> transfer finished successfully.
> + otherwise -> transfer still in progress (doesn't happen
> + today due to implementation not being async,
> + but may in the future).
> +
> +Target address goes up and transfer length goes down as the transfer
> +happens, so after a successful transfer the length register is zero
> +and the address register points right after the memory block written.
> +
> +If a partial transfer happened before an error occured the address and
> +length registers indicate how much data has been transfered
> +successfully.
> +
> Required properties:
>
> - compatible: "qemu,fw-cfg-mmio".
> @@ -56,6 +91,7 @@ Required properties:
> - reg: the MMIO region used by the device.
> * Bytes 0x0 to 0x7 cover the data register.
> * Bytes 0x8 to 0x9 cover the selector register.
> + * From revision 2: Bytes 0xa to 0x11 cover the DMA address register.
I agree with Drew about padding being necessary here. I have proposed to
break up the address register into two 32-bit halves, but even for that,
0xa is not good enough. 0x0c..0x13, inclusive, seems better.
(Actually I wonder if we should instead add a separate region ("reg")
for this register, instead of padding. I don't know enough about device
trees to make a suggestion here. Let's go with this approach first, ie.
keep the single reg for now, just add the padding I guess.)
> * Further registers may be appended to the region in case of future interface
> revisions / feature bits.
>
>
The example DTB should also be refreshed. You can do it like this:
- apply your (updated) patch "[PATCH 4/5] Enable fw_cfg DMA interface
for ARM" to QEMU
- run
qemu-system-aarch64 -machine virt,dumpdtb=dumped.dtb ...
dtc -I dtb -O dts dumped.dtb >dumped.dts
- locate "fw-cfg" in "dumped.dts"
The example should be similar to:
fw-cfg@9020000 {
compatible = "qemu,fw-cfg-mmio";
reg = <0x0 0x9020000 0x0 0x14>;
};
Thanks
Laszlo