[Qemu-devel] QEMU fw_cfg DMA interface

[Qemu-devel] QEMU fw_cfg DMA interface

[Marc Marí]

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 090d0bf and SeaBIOS commit 2fc20dc
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.

Changes from v1:
 - Take into account order of fields in the FWCfgDmaAccess structure
 - Check and change endianness of FWCfgDmaAccess fields
 - Change order of fields in the FWCfgDmaAccess structure
 - Add FW_CFG_DMA_CTL_SKIP feature for control field
 - Split FW_CFG_SIZE in QEMU
 - Make FW_CFG_ID a bitmap of features
 - Add 64 bit address support for the transfer. Trigger when writing the low
   address, and address is 0 by default and at the end of each transfer.
 - Align ports and addresses.
 - Preserve old fw_cfg_comb_valid behaviour in QEMU
 - Update documentation to reflect all these changes

[Qemu-devel] [PATCH v2 0/5] fw_cfg DMA interface

[Marc Marí]

Implement host-side of the FW CFG DMA interface both for x86 and ARM.

Based on Gerd Hoffman's initial implementation.

Gabriel L. Somlo (1):
  fw_cfg: document fw_cfg_modify_iXX() update functions

Marc Marí (4):
  fw_cfg DMA interface documentation
  Implement fw_cfg DMA interface
  Enable fw_cfg DMA interface for ARM
  Enable fw_cfg DMA interface for x86

 docs/specs/fw_cfg.txt     |  79 +++++++++++++-
 hw/arm/virt.c             |  13 ++-
 hw/i386/pc.c              |  11 +-
 hw/nvram/fw_cfg.c         | 261 +++++++++++++++++++++++++++++++++++++++++++---
 include/hw/nvram/fw_cfg.h |  15 ++-
 5 files changed, 351 insertions(+), 28 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Marc Marí]

From: "Gabriel L. Somlo" <somlo@cmu.edu>

Document the behavior of fw_cfg_modify_iXX() for leak-less updating
of integer-type blobs.

Currently only fw_cfg_modify_i16() is coded, but 32- and 64-bit versions
may be added later if necessary..

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 docs/specs/fw_cfg.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 74351dd..5bc7b96 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -159,6 +159,17 @@ will convert a 16-, 32-, or 64-bit integer to little-endian, then add
 a dynamically allocated copy of the appropriately sized item to fw_cfg
 under the given selector key value.
 
+== fw_cfg_modify_iXX() ==
+
+Modify the value of an XX-bit item (where XX may be 16, 32, or 64).
+Similarly to the corresponding fw_cfg_add_iXX() function set, convert
+a 16-, 32-, or 64-bit integer to little endian, create a dynamically
+allocated copy of the required size, and replace the existing item at
+the given selector key value with the newly allocated one. The previous
+item, assumed to have been allocated during an earlier call to
+fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed
+before the function returns.
+
 == fw_cfg_add_file() ==
 
 Given a filename (i.e., fw_cfg item name), starting pointer, and size,
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Peter Maydell]

On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote:
> From: "Gabriel L. Somlo" <somlo@cmu.edu>
>
> Document the behavior of fw_cfg_modify_iXX() for leak-less updating
> of integer-type blobs.
>
> Currently only fw_cfg_modify_i16() is coded, but 32- and 64-bit versions
> may be added later if necessary..
>
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  docs/specs/fw_cfg.txt | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 74351dd..5bc7b96 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -159,6 +159,17 @@ will convert a 16-, 32-, or 64-bit integer to little-endian, then add
>  a dynamically allocated copy of the appropriately sized item to fw_cfg
>  under the given selector key value.
>
> +== fw_cfg_modify_iXX() ==
> +
> +Modify the value of an XX-bit item (where XX may be 16, 32, or 64).
> +Similarly to the corresponding fw_cfg_add_iXX() function set, convert
> +a 16-, 32-, or 64-bit integer to little endian, create a dynamically
> +allocated copy of the required size, and replace the existing item at
> +the given selector key value with the newly allocated one. The previous
> +item, assumed to have been allocated during an earlier call to
> +fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed
> +before the function returns.
> +
>  == fw_cfg_add_file() ==
>
>  Given a filename (i.e., fw_cfg item name), starting pointer, and size,

This doesn't cover fw_cfg_modify_file(); is that intentional?

As an aside, shouldn't this function-level documentation be done
via doc-comments in the header file where the prototypes are
declared? (You don't need to move the docs around in this series,
but it might be nice to do at some point.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Gabriel L. Somlo]

On Tue, Sep 01, 2015 at 06:33:25PM +0100, Peter Maydell wrote:
> On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote:
> > From: "Gabriel L. Somlo" <somlo@cmu.edu>
> >
> > Document the behavior of fw_cfg_modify_iXX() for leak-less updating
> > of integer-type blobs.
> >
> > Currently only fw_cfg_modify_i16() is coded, but 32- and 64-bit versions
> > may be added later if necessary..
> >
> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  docs/specs/fw_cfg.txt | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> > index 74351dd..5bc7b96 100644
> > --- a/docs/specs/fw_cfg.txt
> > +++ b/docs/specs/fw_cfg.txt
> > @@ -159,6 +159,17 @@ will convert a 16-, 32-, or 64-bit integer to little-endian, then add
> >  a dynamically allocated copy of the appropriately sized item to fw_cfg
> >  under the given selector key value.
> >
> > +== fw_cfg_modify_iXX() ==
> > +
> > +Modify the value of an XX-bit item (where XX may be 16, 32, or 64).
> > +Similarly to the corresponding fw_cfg_add_iXX() function set, convert
> > +a 16-, 32-, or 64-bit integer to little endian, create a dynamically
> > +allocated copy of the required size, and replace the existing item at
> > +the given selector key value with the newly allocated one. The previous
> > +item, assumed to have been allocated during an earlier call to
> > +fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed
> > +before the function returns.
> > +
> >  == fw_cfg_add_file() ==
> >
> >  Given a filename (i.e., fw_cfg item name), starting pointer, and size,
> 
> This doesn't cover fw_cfg_modify_file(); is that intentional?

There should already be a paragraph in there (further down from where
this is supposed to go in) describing fw_cfg_modify_file(), which
isn't affected by this. 
> 
> As an aside, shouldn't this function-level documentation be done
> via doc-comments in the header file where the prototypes are
> declared? (You don't need to move the docs around in this series,
> but it might be nice to do at some point.)

You mean, leave docs/specs/fw_cfg.txt to deal with the guest-side
port/mmio api only, and have the host-side functions simply documented
as comments in include/hw/nvram/fw_cfg.h ?

That should be relatively painless, if that's the agreed-upon
convention...

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Peter Maydell]

On 1 September 2015 at 18:45, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> On Tue, Sep 01, 2015 at 06:33:25PM +0100, Peter Maydell wrote:
>> This doesn't cover fw_cfg_modify_file(); is that intentional?
>
> There should already be a paragraph in there (further down from where
> this is supposed to go in) describing fw_cfg_modify_file(), which
> isn't affected by this.

Sorry, so there is.

>> As an aside, shouldn't this function-level documentation be done
>> via doc-comments in the header file where the prototypes are
>> declared? (You don't need to move the docs around in this series,
>> but it might be nice to do at some point.)
>
> You mean, leave docs/specs/fw_cfg.txt to deal with the guest-side
> port/mmio api only, and have the host-side functions simply documented
> as comments in include/hw/nvram/fw_cfg.h ?
>
> That should be relatively painless, if that's the agreed-upon
> convention...

Yes. I think at the point this file was written we probably hadn't
started using doc-comments for our APIs. (My usual place to crib
the formatting for doc-comments is the extract/deposit APIs in
bitops.h.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Gabriel L. Somlo]

On Tue, Sep 01, 2015 at 07:45:27PM +0100, Peter Maydell wrote:
> On 1 September 2015 at 18:45, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> > On Tue, Sep 01, 2015 at 06:33:25PM +0100, Peter Maydell wrote:
> >> As an aside, shouldn't this function-level documentation be done
> >> via doc-comments in the header file where the prototypes are
> >> declared? (You don't need to move the docs around in this series,
> >> but it might be nice to do at some point.)
> >
> > You mean, leave docs/specs/fw_cfg.txt to deal with the guest-side
> > port/mmio api only, and have the host-side functions simply documented
> > as comments in include/hw/nvram/fw_cfg.h ?
> >
> > That should be relatively painless, if that's the agreed-upon
> > convention...
> 
> Yes. I think at the point this file was written we probably hadn't
> started using doc-comments for our APIs. (My usual place to crib
> the formatting for doc-comments is the extract/deposit APIs in
> bitops.h.)

OK, I guess I'll wait until after the dust settles on fw_cfg/DMA
before further mucking with the doc or header files, but again, this
shouldn't be too painless...

Also, since I'll be tinkering with fw_cfg again, and you mentioned
using DT on arm and ACPI on x86 to auto-detect the presence (and location)
of fw_cfg from the guest-side in a related thread:

Right now, I don't think fw_cfg is listed in ACPI, and I would like it
to be. So what should I do, simply figure out how to add a new node
somewhere in the SSDT and submit a patch for that ? Could it be that
simple, or am I missing something ? :)

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Peter Maydell]

On 1 September 2015 at 20:13, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> Also, since I'll be tinkering with fw_cfg again, and you mentioned
> using DT on arm and ACPI on x86 to auto-detect the presence (and location)
> of fw_cfg from the guest-side in a related thread:

I meant DT or ACPI on ARM, actually. I don't know what the x86
approach is for fw_cfg but I think it's just "known address".
(For ARM we don't I think currently expose fw_cfg in the ACPI
tables, but we probably ought to; only needed for the edge
case where you actually wanted to read fw_cfg in the guest
kernel rather than just the guest firmware, though.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Gabriel L. Somlo]

On Tue, Sep 01, 2015 at 09:10:23PM +0100, Peter Maydell wrote:
> On 1 September 2015 at 20:13, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> > Also, since I'll be tinkering with fw_cfg again, and you mentioned
> > using DT on arm and ACPI on x86 to auto-detect the presence (and location)
> > of fw_cfg from the guest-side in a related thread:
> 
> I meant DT or ACPI on ARM, actually. I don't know what the x86
> approach is for fw_cfg but I think it's just "known address".
> (For ARM we don't I think currently expose fw_cfg in the ACPI
> tables, but we probably ought to; only needed for the edge
> case where you actually wanted to read fw_cfg in the guest
> kernel rather than just the guest firmware, though.)

Right, that's what I'm trying to do -- expose fw_cfg in
/sys/firmware/... on the guest -- which involves figuring out how
to determine if it is present (DT on ARM, blindly poking port 0x510
on x86 for now, hence my interest in having it listed in ACPI).

Now, if we did expose fw_cfg in ACPI on *both* ARM and x86, I wonder
I could get away with an ACPI-only detection scheme across both
architectures... But ACPI is all we have on x86, so on the QEMU side
of things it looks like I'll have to add it regardless...

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Peter Maydell]

On 1 September 2015 at 21:27, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> Right, that's what I'm trying to do -- expose fw_cfg in
> /sys/firmware/... on the guest -- which involves figuring out how
> to determine if it is present (DT on ARM, blindly poking port 0x510
> on x86 for now, hence my interest in having it listed in ACPI).
>
> Now, if we did expose fw_cfg in ACPI on *both* ARM and x86, I wonder
> I could get away with an ACPI-only detection scheme across both
> architectures...

Nope, because there's no requirement for ACPI on ARM -- you could
be booting the kernel via device tree.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Gerd Hoffmann]

On Di, 2015-09-01 at 21:10 +0100, Peter Maydell wrote:
> On 1 September 2015 at 20:13, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> > Also, since I'll be tinkering with fw_cfg again, and you mentioned
> > using DT on arm and ACPI on x86 to auto-detect the presence (and location)
> > of fw_cfg from the guest-side in a related thread:
> 
> I meant DT or ACPI on ARM, actually. I don't know what the x86
> approach is for fw_cfg but I think it's just "known address".

Yes, "known address".  And given that the firmware actually loads the
acpi tables via fw_cfg that is very unlikely to change.  Adding it to
the acpi tables might be useful nevertheless so the guest os knows the
ioports are in use.

I think on arm the acpi situation is the same, but I think firmware
detecting the location via DT should work without chicken&egg problems.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Laszlo Ersek]

On 09/02/15 10:08, Gerd Hoffmann wrote:
> On Di, 2015-09-01 at 21:10 +0100, Peter Maydell wrote:
>> On 1 September 2015 at 20:13, Gabriel L. Somlo <somlo@cmu.edu> wrote:
>>> Also, since I'll be tinkering with fw_cfg again, and you mentioned
>>> using DT on arm and ACPI on x86 to auto-detect the presence (and location)
>>> of fw_cfg from the guest-side in a related thread:
>>
>> I meant DT or ACPI on ARM, actually. I don't know what the x86
>> approach is for fw_cfg but I think it's just "known address".
> 
> Yes, "known address".  And given that the firmware actually loads the
> acpi tables via fw_cfg that is very unlikely to change.  Adding it to
> the acpi tables might be useful nevertheless so the guest os knows the
> ioports are in use.
> 
> I think on arm the acpi situation is the same, but I think firmware
> detecting the location via DT should work without chicken&egg problems.

First of all I apologize for lagging severely behind this thread; in
practice I can't read emails longer than 20 lines or so. This one
qualifies, so I'll respond:

Yes, on qemu-system-(arm|aarch64) -M virt, detecting the fw_cfg
registers via DT should continue to work without any dependencies.

Thanks
Laszlo

[Qemu-devel] [PATCH v2 2/5] fw_cfg DMA interface documentation

[Marc Marí]

Add fw_cfg DMA interface specification in the documentation.

Based on Gerd Hoffman's initial implementation.

Signed-off-by: Marc Marí <markmb@redhat.com>
---
 docs/specs/fw_cfg.txt | 68 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 4 deletions(-)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 5bc7b96..06302f6 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -76,6 +76,13 @@ increasing address order, similar to memcpy().
 
 Selector Register IOport: 0x510
 Data Register IOport:     0x511
+DMA Address IOport:       0x514
+
+=== ARM Register Locations ===
+
+Selector Register address: 0x09020000
+Data Register address:     0x09020008
+DMA Address address:       0x0902000c
 
 == Firmware Configuration Items ==
 
@@ -86,11 +93,12 @@ by selecting the "signature" item using key 0x0000 (FW_CFG_SIGNATURE),
 and reading four bytes from the data register. If the fw_cfg device is
 present, the four bytes read will contain the characters "QEMU".
 
-=== Revision (Key 0x0001, FW_CFG_ID) ===
+=== Revision / feature bitmap (Key 0x0001, FW_CFG_ID) ===
 
-A 32-bit little-endian unsigned int, this item is used as an interface
-revision number, and is currently set to 1 by QEMU when fw_cfg is
-initialized.
+A 32-bit little-endian unsigned int, this item is used to check for enabled
+features.
+ - Bit 0: traditional interface. Always set.
+ - Bit 1: DMA interface.
 
 === File Directory (Key 0x0019, FW_CFG_FILE_DIR) ===
 
@@ -132,6 +140,58 @@ Selector Reg.    Range Usage
 In practice, the number of allowed firmware configuration items is given
 by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
 
+= Guest-side DMA Interface =
+
+If bit 1 of the feature bitmap is set, the DMA interface is present. This does
+not replace the existing fw_cfg interface, it is an add-on. This interface
+can be used through the 64-bit wide address register.
+
+The address register, as the selector register, is in little-endian format
+when using IOports, and in big-endian format when using MMIO. The value for
+the register is 0 at startup and after an operation. A write to the lower
+half triggers an operation. This means, that operations with 32-bit addresses
+can be triggered with just one write, whereas operations with 64-bit addresses
+can be triggered with one 64-bit write or two 32-bit writes, starting with the
+higher part.
+
+In this register, a physical RAM address to a FWCfgDmaAccess structure should
+be written. This is the format of the FWCfgDmaAccess structure:
+
+typedef struct FWCfgDmaAccess {
+    uint32_t control;
+    uint32_t length;
+    uint64_t address;
+} FWCfgDmaAccess;
+
+The fields of the structure are in big endian mode, and the field at the lowest
+address is the "control" field.
+
+The "control" field has the following bits:
+ - Bit 0: Error
+ - Bit 1: Read
+ - Bit 2: Skip
+
+When an operation is triggered, if the "control" field has bit 1 set, a read
+operation will be performed. "length" bytes for the current selector and
+offset will be copied into the address specified by the "address" field.
+
+If the control field has only bit 2 set, a skip operation will be perfomed.
+The offset for the current selector will be advanced "length" bytes.
+
+To check result, read the "control" field:
+   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 field is zero and the address field
+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.
+
 = Host-side API =
 
 The following functions are available to the QEMU programmer for adding
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 2/5] fw_cfg DMA interface documentation

[Kevin O'Connor]

On Mon, Aug 31, 2015 at 11:10:14AM +0200, Marc Marí wrote:
> Add fw_cfg DMA interface specification in the documentation.
> 
> Based on Gerd Hoffman's initial implementation.

Thanks for working on this.

[...]
> += Guest-side DMA Interface =
> +
> +If bit 1 of the feature bitmap is set, the DMA interface is present. This does
> +not replace the existing fw_cfg interface, it is an add-on. This interface
> +can be used through the 64-bit wide address register.
> +
> +The address register, as the selector register, is in little-endian format
> +when using IOports, and in big-endian format when using MMIO. The value for
> +the register is 0 at startup and after an operation. A write to the lower
> +half triggers an operation. This means, that operations with 32-bit addresses
> +can be triggered with just one write, whereas operations with 64-bit addresses
> +can be triggered with one 64-bit write or two 32-bit writes, starting with the
> +higher part.
> +
> +In this register, a physical RAM address to a FWCfgDmaAccess structure should
> +be written. This is the format of the FWCfgDmaAccess structure:
> +
> +typedef struct FWCfgDmaAccess {
> +    uint32_t control;
> +    uint32_t length;
> +    uint64_t address;
> +} FWCfgDmaAccess;
> +
> +The fields of the structure are in big endian mode, and the field at the lowest
> +address is the "control" field.
> +
> +The "control" field has the following bits:
> + - Bit 0: Error
> + - Bit 1: Read
> + - Bit 2: Skip
> +
> +When an operation is triggered, if the "control" field has bit 1 set, a read
> +operation will be performed. "length" bytes for the current selector and
> +offset will be copied into the address specified by the "address" field.
> +
> +If the control field has only bit 2 set, a skip operation will be perfomed.
> +The offset for the current selector will be advanced "length" bytes.
> +
> +To check result, read the "control" field:
> +   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 field is zero and the address field
> +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.

If the interface will support asynchronous transfers, then it is
necessary to document which field QEMU will update last.  That way,
the guest will know which particular field to wait on.  It should
probably also be documented that that particular field must have
native alignment (eg, alignment of 4 for a 32bit field).

The reason this is important, is because of a subtle race condition.
For example, if QEMU updates "length" and then "control", but the
guest spins on "length" then it is possible for the guest to exit its
spin loop and use the memory containing "control" for something else
before QEMU finishes updating "control".  This would result in
corruption of that memory when QEMU later overwrites it.

My suggestion would be for QEMU to only ever update "control" and not
modify any other parts of the FWCfgDmaAccess struct.  This makes the
interface and documentation simpler and it sidesteps this issue.

-Kevin

Re: [Qemu-devel] [PATCH v2 2/5] fw_cfg DMA interface documentation

[Peter Maydell]

On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote:
> Add fw_cfg DMA interface specification in the documentation.
>
> Based on Gerd Hoffman's initial implementation.
>
> Signed-off-by: Marc Marí <markmb@redhat.com>
> ---
>  docs/specs/fw_cfg.txt | 68 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 5bc7b96..06302f6 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -76,6 +76,13 @@ increasing address order, similar to memcpy().
>
>  Selector Register IOport: 0x510
>  Data Register IOport:     0x511
> +DMA Address IOport:       0x514
> +
> +=== ARM Register Locations ===
> +
> +Selector Register address: 0x09020000
> +Data Register address:     0x09020008
> +DMA Address address:       0x0902000c

These addresses shouldn't be documented -- the correct API is that the guest
needs to find the base address of the fw_cfg device via device tree
or ACPI table. You can document the layout of the registers within the
device, obviously (ie +0, +4, +8).

>  == Firmware Configuration Items ==
>
> @@ -86,11 +93,12 @@ by selecting the "signature" item using key 0x0000 (FW_CFG_SIGNATURE),
>  and reading four bytes from the data register. If the fw_cfg device is
>  present, the four bytes read will contain the characters "QEMU".
>
> -=== Revision (Key 0x0001, FW_CFG_ID) ===
> +=== Revision / feature bitmap (Key 0x0001, FW_CFG_ID) ===
>
> -A 32-bit little-endian unsigned int, this item is used as an interface
> -revision number, and is currently set to 1 by QEMU when fw_cfg is
> -initialized.
> +A 32-bit little-endian unsigned int, this item is used to check for enabled
> +features.
> + - Bit 0: traditional interface. Always set.
> + - Bit 1: DMA interface.
>
>  === File Directory (Key 0x0019, FW_CFG_FILE_DIR) ===
>
> @@ -132,6 +140,58 @@ Selector Reg.    Range Usage
>  In practice, the number of allowed firmware configuration items is given
>  by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
>
> += Guest-side DMA Interface =
> +
> +If bit 1 of the feature bitmap is set, the DMA interface is present. This does
> +not replace the existing fw_cfg interface, it is an add-on. This interface
> +can be used through the 64-bit wide address register.
> +
> +The address register, as the selector register, is in little-endian format
> +when using IOports, and in big-endian format when using MMIO. The value for
> +the register is 0 at startup and after an operation. A write to the lower
> +half triggers an operation. This means, that operations with 32-bit addresses

Delete this comma.

> +can be triggered with just one write, whereas operations with 64-bit addresses
> +can be triggered with one 64-bit write or two 32-bit writes, starting with the
> +higher part.
> +
> +In this register, a physical RAM address to a FWCfgDmaAccess structure should

"the physical address of a FWCfgDmaAccess structure in RAM"

> +be written. This is the format of the FWCfgDmaAccess structure:
> +
> +typedef struct FWCfgDmaAccess {
> +    uint32_t control;
> +    uint32_t length;
> +    uint64_t address;
> +} FWCfgDmaAccess;
> +
> +The fields of the structure are in big endian mode, and the field at the lowest
> +address is the "control" field.
> +
> +The "control" field has the following bits:
> + - Bit 0: Error
> + - Bit 1: Read
> + - Bit 2: Skip
> +
> +When an operation is triggered, if the "control" field has bit 1 set, a read
> +operation will be performed. "length" bytes for the current selector and
> +offset will be copied into the address specified by the "address" field.
> +
> +If the control field has only bit 2 set, a skip operation will be perfomed.
> +The offset for the current selector will be advanced "length" bytes.

The implication here is that the operation completes before the
guest write to the address register returns,

> +To check result, read the "control" field:
> +   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).

Is there much point in having an async transfer interface which
requires the guest to busy-wait polling the control field?

> +
> +Target address goes up and transfer length goes down as the transfer happens,
> +so after a successful transfer the length field is zero and the address field
> +points right after the memory block written.
> +
> +If a partial transfer happened before an error occured the address and

"occurred".

> +length registers indicate how much data has been transfered successfully.

"transferred".

> +
>  = Host-side API =
>
>  The following functions are available to the QEMU programmer for adding

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/5] fw_cfg DMA interface documentation

[Peter Maydell]

On 1 September 2015 at 18:47, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote:
>> +=== ARM Register Locations ===
>> +
>> +Selector Register address: 0x09020000
>> +Data Register address:     0x09020008
>> +DMA Address address:       0x0902000c
>
> These addresses shouldn't be documented -- the correct API is that the guest
> needs to find the base address of the fw_cfg device via device tree
> or ACPI table. You can document the layout of the registers within the
> device, obviously (ie +0, +4, +8).

...and this is not what your patchset implements anyway. This should be:

 offset 0: data register (8 bytes wide)
 offset 8: selector register (4 bytes wide)
 offset 12: reserved
 offset 16: DMA address register (8 bytes wide)

thanks
-- PMM

[Qemu-devel] [PATCH v2 3/5] Implement fw_cfg DMA interface

[Marc Marí]

Based on the specifications on docs/specs/fw_cfg.txt

This interface is an addon. The old interface can still be used as usual.

Based on Gerd Hoffman's initial implementation.

Signed-off-by: Marc Marí <markmb@redhat.com>
---
 hw/arm/virt.c             |   2 +-
 hw/nvram/fw_cfg.c         | 261 +++++++++++++++++++++++++++++++++++++++++++---
 include/hw/nvram/fw_cfg.h |  15 ++-
 3 files changed, 260 insertions(+), 18 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d5a8417..b88c104 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -620,7 +620,7 @@ static void create_fw_cfg(const VirtBoardInfo *vbi)
     hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
     char *nodename;
 
-    fw_cfg_init_mem_wide(base + 8, base, 8);
+    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
 
     nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 88481b7..7e5ba96 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -23,6 +23,7 @@
  */
 #include "hw/hw.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/dma.h"
 #include "hw/isa/isa.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/sysbus.h"
@@ -30,7 +31,8 @@
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 
-#define FW_CFG_SIZE 2
+#define FW_CFG_IO_SIZE 12 /* Address aligned to 4 bytes */
+#define FW_CFG_CTL_SIZE 2
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
@@ -42,6 +44,15 @@
 #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
 #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
 
+/* FW_CFG_VERSION bits */
+#define FW_CFG_VERSION      0x01
+#define FW_CFG_VERSION_DMA  0x02
+
+/* FW_CFG_DMA_CONTROL bits */
+#define FW_CFG_DMA_CTL_ERROR   0x01
+#define FW_CFG_DMA_CTL_READ    0x02
+#define FW_CFG_DMA_CTL_SKIP    0x04
+
 typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
@@ -59,6 +70,10 @@ struct FWCfgState {
     uint16_t cur_entry;
     uint32_t cur_offset;
     Notifier machine_ready;
+
+    bool dma_enabled;
+    AddressSpace *dma_as;
+    dma_addr_t dma_addr;
 };
 
 struct FWCfgIoState {
@@ -75,7 +90,7 @@ struct FWCfgMemState {
     FWCfgState parent_obj;
     /*< public >*/
 
-    MemoryRegion ctl_iomem, data_iomem;
+    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
     uint32_t data_width;
     MemoryRegionOps wide_data_ops;
 };
@@ -294,6 +309,142 @@ static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
     } while (i);
 }
 
+static void fw_cfg_dma_transfer(FWCfgState *s)
+{
+    dma_addr_t len;
+    uint8_t *ptr;
+    void *addr;
+    FWCfgDmaAccess dma;
+    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
+    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
+
+    len = sizeof(dma);
+    addr = dma_memory_map(s->dma_as, s->dma_addr, &len,
+                                DMA_DIRECTION_FROM_DEVICE);
+
+    s->dma_addr = 0;
+
+    if (!addr || !len) {
+        return;
+    }
+
+    dma.address = be64_to_cpu(*(uint64_t *)(addr +
+                    offsetof(FWCfgDmaAccess, address)));
+    dma.length = be32_to_cpu(*(uint32_t *)(addr +
+                    offsetof(FWCfgDmaAccess, length)));
+    dma.control = be32_to_cpu(*(uint32_t *)(addr +
+                    offsetof(FWCfgDmaAccess, control)));
+
+    if (dma.control & FW_CFG_DMA_CTL_ERROR) {
+        goto out;
+    }
+
+    if (!(dma.control & (FW_CFG_DMA_CTL_READ | FW_CFG_DMA_CTL_SKIP))) {
+        goto out;
+    }
+
+    while (dma.length > 0) {
+        if (s->cur_entry == FW_CFG_INVALID || !e->data ||
+                                s->cur_offset >= e->len) {
+            len = dma.length;
+
+            /* If the access is not a read access, it will be a skip access,
+             * tested before.
+             */
+            if (dma.control & FW_CFG_DMA_CTL_READ) {
+                ptr = dma_memory_map(s->dma_as, dma.address, &len,
+                                     DMA_DIRECTION_FROM_DEVICE);
+                if (!ptr || !len) {
+                    dma.control |= FW_CFG_DMA_CTL_ERROR;
+                    goto out;
+                }
+
+                memset(ptr, 0, len);
+
+                dma_memory_unmap(s->dma_as, ptr, len,
+                                 DMA_DIRECTION_FROM_DEVICE, len);
+            }
+
+        } else {
+            if (dma.length <= e->len) {
+                len = dma.length;
+            } else {
+                len = e->len;
+            }
+
+            if (e->read_callback) {
+                e->read_callback(e->callback_opaque, s->cur_offset);
+            }
+
+            /* If the access is not a read access, it will be a skip access,
+             * tested before.
+             */
+            if (dma.control & FW_CFG_DMA_CTL_READ) {
+                ptr = dma_memory_map(s->dma_as, dma.address, &len,
+                                     DMA_DIRECTION_FROM_DEVICE);
+                if (!ptr || !len) {
+                    dma.control |= FW_CFG_DMA_CTL_ERROR;
+                    goto out;
+                }
+
+                memcpy(ptr, &e->data[s->cur_offset], len);
+
+                dma_memory_unmap(s->dma_as, ptr, len,
+                                 DMA_DIRECTION_FROM_DEVICE, len);
+            }
+
+            s->cur_offset += len;
+
+        }
+
+        dma.address += len;
+        dma.length  -= len;
+        dma.control = 0;
+
+        *(uint64_t *)(addr + offsetof(FWCfgDmaAccess, address)) =
+                                                cpu_to_be64(dma.address);
+        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, length)) =
+                                                cpu_to_be32(dma.length);
+        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, control)) =
+                                                cpu_to_be32(dma.control);
+    }
+
+    trace_fw_cfg_read(s, 0);
+
+out:
+    dma_memory_unmap(s->dma_as, addr, sizeof(dma),
+                                DMA_DIRECTION_FROM_DEVICE, sizeof(dma));
+    return;
+
+}
+
+static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
+                                 uint64_t value, unsigned size)
+{
+    FWCfgState *s = opaque;
+
+    if (size == 4) {
+        if (addr == 0) {
+            /* FWCfgDmaAccess high address */
+            s->dma_addr = (0xFFFFFFFF & value) << 32;
+        } else if (addr == 4) {
+            /* FWCfgDmaAccess low address */
+            s->dma_addr |= value;
+            fw_cfg_dma_transfer(s);
+        }
+    } else if (size == 8 && addr == 0) {
+        s->dma_addr = value;
+        fw_cfg_dma_transfer(s);
+    }
+}
+
+static bool fw_cfg_dma_mem_valid(void *opaque, hwaddr addr,
+                                  unsigned size, bool is_write)
+{
+    return is_write && ((size == 4 && (addr == 0 || addr == 4)) ||
+                        (size == 8 && addr == 0));
+}
+
 static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
@@ -321,20 +472,35 @@ static uint64_t fw_cfg_comb_read(void *opaque, hwaddr addr,
 static void fw_cfg_comb_write(void *opaque, hwaddr addr,
                               uint64_t value, unsigned size)
 {
-    switch (size) {
+    FWCfgState *s;
+
+    switch (addr) {
+    case 0:
+        fw_cfg_select(opaque, (uint16_t)value);
+        break;
     case 1:
         fw_cfg_write(opaque, (uint8_t)value);
         break;
-    case 2:
-        fw_cfg_select(opaque, (uint16_t)value);
+    case 4:
+        /* FWCfgDmaAccess low address */
+        s = opaque;
+        s->dma_addr |= value;
+        fw_cfg_dma_transfer(s);
         break;
+    case 8:
+        /* FWCfgDmaAccess high address */
+        s = opaque;
+        s->dma_addr = (0xFFFFFFFF & value) << 32;
     }
 }
 
 static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
-    return (size == 1) || (is_write && size == 2);
+    FWCfgState *s = opaque;
+
+    return (size == 1) || (is_write && size == 2) ||
+            (s->dma_enabled && is_write && addr >= 4);
 }
 
 static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
@@ -361,6 +527,12 @@ static const MemoryRegionOps fw_cfg_comb_mem_ops = {
     .valid.accepts = fw_cfg_comb_valid,
 };
 
+static const MemoryRegionOps fw_cfg_dma_mem_ops = {
+    .write = fw_cfg_dma_mem_write,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .valid.accepts = fw_cfg_dma_mem_valid,
+};
+
 static void fw_cfg_reset(DeviceState *d)
 {
     FWCfgState *s = FW_CFG(d);
@@ -401,6 +573,22 @@ static bool is_version_1(void *opaque, int version_id)
     return version_id == 1;
 }
 
+static bool fw_cfg_dma_enabled(void *opaque)
+{
+    FWCfgState *s = opaque;
+
+    return s->dma_enabled;
+}
+
+static VMStateDescription vmstate_fw_cfg_dma = {
+    .name = "fw_cfg/dma",
+    .needed = fw_cfg_dma_enabled,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(dma_addr, FWCfgState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static const VMStateDescription vmstate_fw_cfg = {
     .name = "fw_cfg",
     .version_id = 2,
@@ -410,6 +598,10 @@ static const VMStateDescription vmstate_fw_cfg = {
         VMSTATE_UINT16_HACK(cur_offset, FWCfgState, is_version_1),
         VMSTATE_UINT32_V(cur_offset, FWCfgState, 2),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription*[]) {
+        &vmstate_fw_cfg_dma,
+        NULL,
     }
 };
 
@@ -595,7 +787,6 @@ static void fw_cfg_init1(DeviceState *dev)
     qdev_init_nofail(dev);
 
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
-    fw_cfg_add_i32(s, FW_CFG_ID, 1);
     fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));
     fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
@@ -607,22 +798,45 @@ static void fw_cfg_init1(DeviceState *dev)
     qemu_add_machine_init_done_notifier(&s->machine_ready);
 }
 
-FWCfgState *fw_cfg_init_io(uint32_t iobase)
+FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, AddressSpace *dma_as)
 {
     DeviceState *dev;
+    FWCfgState *s;
+    uint32_t version = FW_CFG_VERSION;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_IO);
     qdev_prop_set_uint32(dev, "iobase", iobase);
+
     fw_cfg_init1(dev);
+    s = FW_CFG(dev);
+
+    if (dma_as) {
+        /* 64 bits for the address field */
+        s->dma_as = dma_as;
+        s->dma_enabled = true;
+        s->dma_addr = 0;
+
+        version |= FW_CFG_VERSION_DMA;
+    }
+
+    fw_cfg_add_i32(s, FW_CFG_ID, version);
 
-    return FW_CFG(dev);
+    return s;
 }
 
-FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
-                                 uint32_t data_width)
+FWCfgState *fw_cfg_init_io(uint32_t iobase)
+{
+    return fw_cfg_init_io_dma(iobase, NULL);
+}
+
+FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
+                                 hwaddr data_addr, uint32_t data_width,
+                                 hwaddr dma_addr, AddressSpace *dma_as)
 {
     DeviceState *dev;
     SysBusDevice *sbd;
+    FWCfgState *s;
+    uint32_t version = FW_CFG_VERSION;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
     qdev_prop_set_uint32(dev, "data_width", data_width);
@@ -633,13 +847,26 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
     sysbus_mmio_map(sbd, 0, ctl_addr);
     sysbus_mmio_map(sbd, 1, data_addr);
 
-    return FW_CFG(dev);
+    s = FW_CFG(dev);
+
+    if (dma_addr && dma_as) {
+        s->dma_as = dma_as;
+        s->dma_enabled = true;
+        s->dma_addr = 0;
+        sysbus_mmio_map(sbd, 2, dma_addr);
+        version |= FW_CFG_VERSION_DMA;
+    }
+
+    fw_cfg_add_i32(s, FW_CFG_ID, version);
+
+    return s;
 }
 
 FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
 {
     return fw_cfg_init_mem_wide(ctl_addr, data_addr,
-                                fw_cfg_data_mem_ops.valid.max_access_size);
+                                fw_cfg_data_mem_ops.valid.max_access_size,
+                                0, NULL);
 }
 
 
@@ -675,7 +902,7 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
     memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
-                          FW_CFG(s), "fwcfg", FW_CFG_SIZE);
+                          FW_CFG(s), "fwcfg", FW_CFG_IO_SIZE);
     sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
 }
 
@@ -707,7 +934,7 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
     const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
 
     memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
-                          FW_CFG(s), "fwcfg.ctl", FW_CFG_SIZE);
+                          FW_CFG(s), "fwcfg.ctl", FW_CFG_CTL_SIZE);
     sysbus_init_mmio(sbd, &s->ctl_iomem);
 
     if (s->data_width > data_ops->valid.max_access_size) {
@@ -725,6 +952,10 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
     memory_region_init_io(&s->data_iomem, OBJECT(s), data_ops, FW_CFG(s),
                           "fwcfg.data", data_ops->valid.max_access_size);
     sysbus_init_mmio(sbd, &s->data_iomem);
+
+    memory_region_init_io(&s->dma_iomem, OBJECT(s), &fw_cfg_dma_mem_ops,
+                          FW_CFG(s), "fwcfg.dma", sizeof(dma_addr_t));
+    sysbus_init_mmio(sbd, &s->dma_iomem);
 }
 
 static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index e60d3ca..d0cbc88 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -61,6 +61,15 @@ typedef struct FWCfgFiles {
     FWCfgFile f[];
 } FWCfgFiles;
 
+/* Control as first field allows for different structures selected by this
+ * field, which might be useful in the future
+ */
+typedef struct FWCfgDmaAccess {
+    uint32_t control;
+    uint32_t length;
+    uint64_t address;
+} QEMU_PACKED FWCfgDmaAccess;
+
 typedef void (*FWCfgCallback)(void *opaque, uint8_t *data);
 typedef void (*FWCfgReadCallback)(void *opaque, uint32_t offset);
 
@@ -77,10 +86,12 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
                               void *data, size_t len);
 void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
                          size_t len);
+FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, AddressSpace *dma_as);
 FWCfgState *fw_cfg_init_io(uint32_t iobase);
 FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr);
-FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
-                                 uint32_t data_width);
+FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
+                                 hwaddr data_addr, uint32_t data_width,
+                                 hwaddr dma_addr, AddressSpace *dma_as);
 
 FWCfgState *fw_cfg_find(void);
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 3/5] Implement fw_cfg DMA interface

[Kevin O'Connor]

On Mon, Aug 31, 2015 at 11:10:15AM +0200, Marc Marí wrote:
> Based on the specifications on docs/specs/fw_cfg.txt
> 
> This interface is an addon. The old interface can still be used as usual.
> 
> Based on Gerd Hoffman's initial implementation.
> 
> Signed-off-by: Marc Marí <markmb@redhat.com>
> ---
>  hw/arm/virt.c             |   2 +-
>  hw/nvram/fw_cfg.c         | 261 +++++++++++++++++++++++++++++++++++++++++++---
>  include/hw/nvram/fw_cfg.h |  15 ++-
>  3 files changed, 260 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d5a8417..b88c104 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -620,7 +620,7 @@ static void create_fw_cfg(const VirtBoardInfo *vbi)
>      hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
>      char *nodename;
>  
> -    fw_cfg_init_mem_wide(base + 8, base, 8);
> +    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
>  
>      nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>      qemu_fdt_add_subnode(vbi->fdt, nodename);
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 88481b7..7e5ba96 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -23,6 +23,7 @@
>   */
>  #include "hw/hw.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/dma.h"
>  #include "hw/isa/isa.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/sysbus.h"
> @@ -30,7 +31,8 @@
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
>  
> -#define FW_CFG_SIZE 2
> +#define FW_CFG_IO_SIZE 12 /* Address aligned to 4 bytes */
> +#define FW_CFG_CTL_SIZE 2
>  #define FW_CFG_NAME "fw_cfg"
>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>  
> @@ -42,6 +44,15 @@
>  #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
>  #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
>  
> +/* FW_CFG_VERSION bits */
> +#define FW_CFG_VERSION      0x01
> +#define FW_CFG_VERSION_DMA  0x02
> +
> +/* FW_CFG_DMA_CONTROL bits */
> +#define FW_CFG_DMA_CTL_ERROR   0x01
> +#define FW_CFG_DMA_CTL_READ    0x02
> +#define FW_CFG_DMA_CTL_SKIP    0x04
> +
>  typedef struct FWCfgEntry {
>      uint32_t len;
>      uint8_t *data;
> @@ -59,6 +70,10 @@ struct FWCfgState {
>      uint16_t cur_entry;
>      uint32_t cur_offset;
>      Notifier machine_ready;
> +
> +    bool dma_enabled;
> +    AddressSpace *dma_as;
> +    dma_addr_t dma_addr;
>  };
>  
>  struct FWCfgIoState {
> @@ -75,7 +90,7 @@ struct FWCfgMemState {
>      FWCfgState parent_obj;
>      /*< public >*/
>  
> -    MemoryRegion ctl_iomem, data_iomem;
> +    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
>      uint32_t data_width;
>      MemoryRegionOps wide_data_ops;
>  };
> @@ -294,6 +309,142 @@ static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
>      } while (i);
>  }
>  
> +static void fw_cfg_dma_transfer(FWCfgState *s)
> +{
> +    dma_addr_t len;
> +    uint8_t *ptr;
> +    void *addr;
> +    FWCfgDmaAccess dma;
> +    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> +    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
> +
> +    len = sizeof(dma);
> +    addr = dma_memory_map(s->dma_as, s->dma_addr, &len,
> +                                DMA_DIRECTION_FROM_DEVICE);
> +
> +    s->dma_addr = 0;
> +
> +    if (!addr || !len) {
> +        return;
> +    }
> +
> +    dma.address = be64_to_cpu(*(uint64_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, address)));
> +    dma.length = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, length)));
> +    dma.control = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, control)));

I am not that familiar with QEMU, but shouldn't that be
DMA_DIRECTION_TO_DEVICE?

It looks like other drivers use dma_memory_read() which would simplify
this code:

    dma_memory_read(s->dma_as, s->dma_addr, &dma, sizeof(dma));
    dma.address = be64_to_cpu(dma.address);
    dma.length = be32_to_cpu(dma.length);
    dma.control = be32_to_cpu(dma.control);

> +    if (dma.control & FW_CFG_DMA_CTL_ERROR) {
> +        goto out;
> +    }
> +
> +    if (!(dma.control & (FW_CFG_DMA_CTL_READ | FW_CFG_DMA_CTL_SKIP))) {
> +        goto out;
> +    }
> +
> +    while (dma.length > 0) {
> +        if (s->cur_entry == FW_CFG_INVALID || !e->data ||
> +                                s->cur_offset >= e->len) {
> +            len = dma.length;
> +
> +            /* If the access is not a read access, it will be a skip access,
> +             * tested before.
> +             */
> +            if (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memset(ptr, 0, len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);
> +            }
> +
> +        } else {
> +            if (dma.length <= e->len) {
> +                len = dma.length;
> +            } else {
> +                len = e->len;
> +            }
> +
> +            if (e->read_callback) {
> +                e->read_callback(e->callback_opaque, s->cur_offset);
> +            }
> +
> +            /* If the access is not a read access, it will be a skip access,
> +             * tested before.
> +             */
> +            if (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memcpy(ptr, &e->data[s->cur_offset], len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);
> +            }
> +
> +            s->cur_offset += len;
> +
> +        }
> +
> +        dma.address += len;
> +        dma.length  -= len;
> +        dma.control = 0;
> +
> +        *(uint64_t *)(addr + offsetof(FWCfgDmaAccess, address)) =
> +                                                cpu_to_be64(dma.address);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, length)) =
> +                                                cpu_to_be32(dma.length);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, control)) =
> +                                                cpu_to_be32(dma.control);
> +    }

I don't think it makes sense for this update to be performed within
the loop.

As I mentioned in another email, I think just updating control would
be sufficient.  Looks like include/sysemu/dma.h defines a stl_be_dma()
macro for performing a single 32bit dma write.

Thanks,
-Kevin

Re: [Qemu-devel] [PATCH v2 3/5] Implement fw_cfg DMA interface

[Peter Maydell]

On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote:
> Based on the specifications on docs/specs/fw_cfg.txt
>
> This interface is an addon. The old interface can still be used as usual.
>
> Based on Gerd Hoffman's initial implementation.
>
> Signed-off-by: Marc Marí <markmb@redhat.com>

> @@ -294,6 +309,142 @@ static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
>      } while (i);
>  }
>
> +static void fw_cfg_dma_transfer(FWCfgState *s)
> +{
> +    dma_addr_t len;
> +    uint8_t *ptr;
> +    void *addr;
> +    FWCfgDmaAccess dma;
> +    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> +    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
> +
> +    len = sizeof(dma);
> +    addr = dma_memory_map(s->dma_as, s->dma_addr, &len,
> +                                DMA_DIRECTION_FROM_DEVICE);
> +
> +    s->dma_addr = 0;
> +
> +    if (!addr || !len) {
> +        return;
> +    }
> +
> +    dma.address = be64_to_cpu(*(uint64_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, address)));
> +    dma.length = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, length)));
> +    dma.control = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, control)));

There are only three fields in this struct, so rather than
using dma_memory_map/unmap and manual byteswapping, how about:

 dma.control = ldl_be_dma(s->dma_as, s->dma_addr);
 dma.length = ldl_be_dma(s->dma_as, s->dma_addr + 4);
 dma.address = ldq_be_dma(s->dma_as, s->dma_addr + 8);

Kevin's suggestion to use dma_memory_read() would be OK as well,
if you really want to check for the return value from the memory
transaction. But you probably don't care because if the guest has
pointed the dma address register into nowhere then it deserves
whatever it gets as long as we don't fall over; and we need to
not fall over whatever the values of control/length/address are.
In any case, please don't use dma_memory_map/unmap.

There are st*_ versions for writing back updated values.

> +
> +    if (dma.control & FW_CFG_DMA_CTL_ERROR) {
> +        goto out;
> +    }
> +
> +    if (!(dma.control & (FW_CFG_DMA_CTL_READ | FW_CFG_DMA_CTL_SKIP))) {
> +        goto out;
> +    }
> +
> +    while (dma.length > 0) {
> +        if (s->cur_entry == FW_CFG_INVALID || !e->data ||
> +                                s->cur_offset >= e->len) {
> +            len = dma.length;
> +
> +            /* If the access is not a read access, it will be a skip access,
> +             * tested before.
> +             */
> +            if (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memset(ptr, 0, len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);

dma_memory_write() would be better than map-zero-unmap I think,
though you would need to have a handy buffer of zeroes to pass
to that function, so maybe not.

> +            }
> +
> +        } else {
> +            if (dma.length <= e->len) {
> +                len = dma.length;
> +            } else {
> +                len = e->len;
> +            }
> +
> +            if (e->read_callback) {
> +                e->read_callback(e->callback_opaque, s->cur_offset);
> +            }
> +
> +            /* If the access is not a read access, it will be a skip access,
> +             * tested before.
> +             */
> +            if (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memcpy(ptr, &e->data[s->cur_offset], len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);

...and dma_memory_write() is definitely better than
map-memcpy-unmap.

> +            }
> +
> +            s->cur_offset += len;
> +
> +        }
> +
> +        dma.address += len;
> +        dma.length  -= len;
> +        dma.control = 0;
> +
> +        *(uint64_t *)(addr + offsetof(FWCfgDmaAccess, address)) =
> +                                                cpu_to_be64(dma.address);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, length)) =
> +                                                cpu_to_be32(dma.length);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, control)) =
> +                                                cpu_to_be32(dma.control);
> +    }
> +
> +    trace_fw_cfg_read(s, 0);
> +
> +out:
> +    dma_memory_unmap(s->dma_as, addr, sizeof(dma),
> +                                DMA_DIRECTION_FROM_DEVICE, sizeof(dma));
> +    return;
> +
> +}
> +
> +static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
> +                                 uint64_t value, unsigned size)
> +{
> +    FWCfgState *s = opaque;
> +
> +    if (size == 4) {
> +        if (addr == 0) {
> +            /* FWCfgDmaAccess high address */
> +            s->dma_addr = (0xFFFFFFFF & value) << 32;

The mask here is unnecessary, since you're shifting by 32 bits.

> +        } else if (addr == 4) {
> +            /* FWCfgDmaAccess low address */
> +            s->dma_addr |= value;
> +            fw_cfg_dma_transfer(s);
> +        }
> +    } else if (size == 8 && addr == 0) {
> +        s->dma_addr = value;
> +        fw_cfg_dma_transfer(s);
> +    }
> +}
> +
> +static bool fw_cfg_dma_mem_valid(void *opaque, hwaddr addr,
> +                                  unsigned size, bool is_write)
> +{
> +    return is_write && ((size == 4 && (addr == 0 || addr == 4)) ||
> +                        (size == 8 && addr == 0));
> +}
> +
>  static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> @@ -321,20 +472,35 @@ static uint64_t fw_cfg_comb_read(void *opaque, hwaddr addr,
>  static void fw_cfg_comb_write(void *opaque, hwaddr addr,
>                                uint64_t value, unsigned size)
>  {
> -    switch (size) {
> +    FWCfgState *s;
> +
> +    switch (addr) {
> +    case 0:
> +        fw_cfg_select(opaque, (uint16_t)value);
> +        break;
>      case 1:
>          fw_cfg_write(opaque, (uint8_t)value);
>          break;
> -    case 2:
> -        fw_cfg_select(opaque, (uint16_t)value);
> +    case 4:
> +        /* FWCfgDmaAccess low address */
> +        s = opaque;
> +        s->dma_addr |= value;
> +        fw_cfg_dma_transfer(s);
>          break;
> +    case 8:
> +        /* FWCfgDmaAccess high address */
> +        s = opaque;
> +        s->dma_addr = (0xFFFFFFFF & value) << 32;
>      }
>  }
>
>  static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> -    return (size == 1) || (is_write && size == 2);
> +    FWCfgState *s = opaque;
> +
> +    return (size == 1) || (is_write && size == 2) ||
> +            (s->dma_enabled && is_write && addr >= 4);

This doesn't look right, since it will allow 1 and 2 byte
writes to the dma_address port, which doesn't seem like
a great idea.

Maybe it would be easier to make the dma port its own
memory region (and then init and sysbus_add_io() it in
fw_cfg_io_realize()) ? Then you wouldn't need to mess with
the already confusing logic for handling combined config/data
ports (which have to be handled together because of the weird
"1 byte accesses are data and 2 byte accesses are config"
semantics).

thanks
-- PMM

[Qemu-devel] [PATCH v2 4/5] Enable fw_cfg DMA interface for ARM

[Marc Marí]

Enable the fw_cfg DMA interface for the ARM virt machine.

Based on Gerd Hoffman's initial implementation.

Signed-off-by: Marc Marí <markmb@redhat.com>
---
 hw/arm/virt.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b88c104..54d5f54 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -111,7 +111,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
-    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
+    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -614,13 +614,13 @@ static void create_flash(const VirtBoardInfo *vbi)
     g_free(nodename);
 }
 
-static void create_fw_cfg(const VirtBoardInfo *vbi)
+static void create_fw_cfg(AddressSpace *as, const VirtBoardInfo *vbi)
 {
     hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
     hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
     char *nodename;
 
-    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
+    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
 
     nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
@@ -808,6 +808,7 @@ static void machvirt_init(MachineState *machine)
     VirtGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
     VirtGuestInfo *guest_info = &guest_info_state->info;
     char **cpustr;
+    AddressSpace *as = NULL;
 
     if (!cpu_model) {
         cpu_model = "cortex-a15";
@@ -845,6 +846,10 @@ static void machvirt_init(MachineState *machine)
         }
         cpuobj = object_new(object_class_get_name(oc));
 
+        if (!as) {
+            as = CPU(cpuobj)->as;
+        }
+
         /* Handle any CPU options specified by the user */
         cc->parse_features(CPU(cpuobj), cpuopts, &err);
         g_free(cpuopts);
@@ -897,7 +902,7 @@ static void machvirt_init(MachineState *machine)
      */
     create_virtio_devices(vbi, pic);
 
-    create_fw_cfg(vbi);
+    create_fw_cfg(as, vbi);
     rom_set_fw(fw_cfg_find());
 
     guest_info->smp_cpus = smp_cpus;
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 4/5] Enable fw_cfg DMA interface for ARM

[Peter Maydell]

On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote:
> Enable the fw_cfg DMA interface for the ARM virt machine.
>
> Based on Gerd Hoffman's initial implementation.
>
> Signed-off-by: Marc Marí <markmb@redhat.com>
> ---
>  hw/arm/virt.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b88c104..54d5f54 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -111,7 +111,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
>      [VIRT_UART] =               { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
> -    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
> +    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -614,13 +614,13 @@ static void create_flash(const VirtBoardInfo *vbi)
>      g_free(nodename);
>  }
>
> -static void create_fw_cfg(const VirtBoardInfo *vbi)
> +static void create_fw_cfg(AddressSpace *as, const VirtBoardInfo *vbi)

Please keep vbi as the first argument; this matches the other functions
in this file.

Calling the argument dma_as would probably be a little more informative.

>  {
>      hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
>      hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
>      char *nodename;
>
> -    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
> +    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
>
>      nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>      qemu_fdt_add_subnode(vbi->fdt, nodename);
> @@ -808,6 +808,7 @@ static void machvirt_init(MachineState *machine)
>      VirtGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
>      VirtGuestInfo *guest_info = &guest_info_state->info;
>      char **cpustr;
> +    AddressSpace *as = NULL;
>
>      if (!cpu_model) {
>          cpu_model = "cortex-a15";
> @@ -845,6 +846,10 @@ static void machvirt_init(MachineState *machine)
>          }
>          cpuobj = object_new(object_class_get_name(oc));
>
> +        if (!as) {
> +            as = CPU(cpuobj)->as;
> +        }

This seems like a weird thing to set the fw_cfg DMA address
space to. (Either the fw_cfg device shouldn't care which CPU
it is being accessing by and shouldn't use any particular CPU's
address space, or it needs to really care about the CPU
that's doing any particular write to it and use that exact
CPU's address space, selected at runtime. The former is the
most likely and matches what actual DMA hardware devices
will do.)

Why not just use address_space_memory ?

> +
>          /* Handle any CPU options specified by the user */
>          cc->parse_features(CPU(cpuobj), cpuopts, &err);
>          g_free(cpuopts);
> @@ -897,7 +902,7 @@ static void machvirt_init(MachineState *machine)
>       */
>      create_virtio_devices(vbi, pic);
>
> -    create_fw_cfg(vbi);
> +    create_fw_cfg(as, vbi);
>      rom_set_fw(fw_cfg_find());
>
>      guest_info->smp_cpus = smp_cpus;
> --
> 2.4.3

thanks
-- PMM

[Qemu-devel] [PATCH v2 5/5] Enable fw_cfg DMA interface for x86

[Marc Marí]

Enable the fw_cfg DMA interface for all the x86 platforms.

Based on Gerd Hoffman's initial implementation.

Signed-off-by: Marc Marí <markmb@redhat.com>
---
 hw/i386/pc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 9f2924e..c6dc84f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -753,14 +753,15 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
     }
 }
 
-static FWCfgState *bochs_bios_init(void)
+static FWCfgState *bochs_bios_init(AddressSpace *as)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;
     int i, j;
     unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
 
-    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
+    fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, as);
+
     /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
      *
      * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU hotplug
@@ -1316,6 +1317,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
     MemoryRegion *ram_below_4g, *ram_above_4g;
     FWCfgState *fw_cfg;
     MachineState *machine = MACHINE(pcms);
+    AddressSpace *as;
 
     assert(machine->ram_size == pcms->below_4g_mem_size +
                                 pcms->above_4g_mem_size);
@@ -1407,7 +1409,10 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = bochs_bios_init();
+    as = g_malloc(sizeof(*as));
+    address_space_init(as, ram_below_4g, "pc.as");
+    fw_cfg = bochs_bios_init(as);
+
     rom_set_fw(fw_cfg);
 
     if (guest_info->has_reserved_memory && pcms->hotplug_memory.base) {
-- 
2.4.3

[Qemu-devel] QEMU fw_cfg DMA interface

[Marc Marí]

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 b2312c6 and SeaBIOS commit 423542e
QEMU startup time: .080
BIOS startup time: .060
Kernel setup time: .586
Total time: .726

QEMU with this patch series and SeaBIOS with this patch series
QEMU startup time: .080
BIOS startup time: .039
Kernel setup time: .005
Total time: .126

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.

Changes from v1:
 - Take into account order of fields in the FWCfgDmaAccess structure
 - Check and change endianness of FWCfgDmaAccess fields
 - Change order of fields in the FWCfgDmaAccess structure
 - Add FW_CFG_DMA_CTL_SKIP feature for control field
 - Split FW_CFG_SIZE in QEMU
 - Make FW_CFG_ID a bitmap of features
 - Add 64 bit address support for the transfer. Trigger when writing the low
   address, and address is 0 by default and at the end of each transfer.
 - Align ports and addresses.
 - Preserve old fw_cfg_comb_valid behaviour in QEMU
 - Update documentation to reflect all these changes

Changes from v2:
 - Make IOports fw_cfg DMA region a different IO region.
 - Reuse everything for MMIO and IOport DMA regions
 - Make transfer status only based on control field
 - Use DMA helpers instead of direct map/unmap
 - Change ARM fw_cfg DMA address space
 - Change Linux boot process to match linuxboot.S
 - Add select capabilities in the FWCfgDmaAccess struct
 - Update documentation to reflect all these changes

Changes from v3:
 - Set properly fw_cfg DMA fields in ARM
 - Set fw_cfg DMA boot process properly (by Laszlo Ersek)
 - Add signature to fw_cfg DMA address field (by Kevin O'Connor)
 - Minor nitpicks

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

[meta-comment]

On 10/01/2015 06:14 AM, Marc Marí wrote:
> Implementation of the FW CFG DMA interface.

The subject line is missing "v4" and "0/7". Also, the cover letter is
missing a diffstat.  That makes it harder to see from the cover letter
what the rest of the series is about.  'git format-patch/send-email
--cover-letter' does what you want; you can even 'git config
format.coverletter=auto' to always include a decent cover letter on any
multi-patch series.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]

On 10/01/2015 10:03 AM, Eric Blake wrote:
> [meta-comment]
> 
> On 10/01/2015 06:14 AM, Marc Marí wrote:
>> Implementation of the FW CFG DMA interface.
> 
> The subject line is missing "v4" and "0/7". Also, the cover letter is
> missing a diffstat.  That makes it harder to see from the cover letter
> what the rest of the series is about.  'git format-patch/send-email
> --cover-letter' does what you want; you can even 'git config
> format.coverletter=auto' to always include a decent cover letter on any
> multi-patch series.

Oh, I see - you sent a meta-cover letter (the one I replied to in this
subthread), and then a patch series including a cover letter (the real
0/7, then 1/7 and friends in-reply-to the 0/7) as a child of the
meta-cover.  It's still a bit awkward for tools that expect the 0/7 as
the start of the thread, and part of my confusion was caused by
out-of-order mail delivery due to the nongnu.org mail server still
recovering from its mail delays.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Laszlo Ersek]

On 10/01/15 18:11, Eric Blake wrote:
> On 10/01/2015 10:03 AM, Eric Blake wrote:
>> [meta-comment]
>>
>> On 10/01/2015 06:14 AM, Marc Marí wrote:
>>> Implementation of the FW CFG DMA interface.
>>
>> The subject line is missing "v4" and "0/7". Also, the cover letter is
>> missing a diffstat.  That makes it harder to see from the cover letter
>> what the rest of the series is about.  'git format-patch/send-email
>> --cover-letter' does what you want; you can even 'git config
>> format.coverletter=auto' to always include a decent cover letter on any
>> multi-patch series.
> 
> Oh, I see - you sent a meta-cover letter (the one I replied to in this
> subthread), and then a patch series including a cover letter (the real
> 0/7, then 1/7 and friends in-reply-to the 0/7) as a child of the
> meta-cover.  It's still a bit awkward for tools that expect the 0/7 as
> the start of the thread,

Yep, the pattern I just described doesn't consider those tools. Is that
a bad problem? Maybe the pattern is not so clever then. :)

(I'm allowed to say bad things about it, because I "invented" it
"independently". :))

> and part of my confusion was caused by
> out-of-order mail delivery due to the nongnu.org mail server still
> recovering from its mail delays.
> 

Right, those delays are not helping.

Thanks
Laszlo

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Laszlo Ersek]

On 10/01/15 18:03, Eric Blake wrote:
> [meta-comment]
> 
> On 10/01/2015 06:14 AM, Marc Marí wrote:
>> Implementation of the FW CFG DMA interface.
> 
> The subject line is missing "v4" and "0/7". Also, the cover letter is
> missing a diffstat.  That makes it harder to see from the cover letter
> what the rest of the series is about.  'git format-patch/send-email
> --cover-letter' does what you want; you can even 'git config
> format.coverletter=auto' to always include a decent cover letter on any
> multi-patch series.
> 

This posting follows a little bit different pattern, one that I myself
follow when posting patches for two (or more) components that must work
in sync.

Usually, a top-level blurb is manually cross-posted to all relevant
mailing lists. Then, each separate patch series is posted only to the
relevant mailing list, with its own cover letter (as usual with git),
*in response* to the manually posted blurb.

This has the following benefits:

- in mailing list archives that organize messages into threads *across*
  mailing lists (like Gmane does, for example), the top-level manual
  blurb is a good "root" for referencing the entire posting.

- The same is true for personal mailboxes, if a recipient is explicitly
  CC'd on all of the messages.

Because the top level blurb is parent to several patch series, and those
child series can all have different version numbers (due to different
numbers of respinds), it is not always straightforward to assign a
version number to the top blurb.

Thanks
Laszlo

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

On 10/01/2015 10:17 AM, Laszlo Ersek wrote:
> On 10/01/15 18:03, Eric Blake wrote:
>> [meta-comment]
>>
>> On 10/01/2015 06:14 AM, Marc Marí wrote:
>>> Implementation of the FW CFG DMA interface.
>>
>> The subject line is missing "v4" and "0/7". Also, the cover letter is
>> missing a diffstat.  That makes it harder to see from the cover letter
>> what the rest of the series is about.  'git format-patch/send-email
>> --cover-letter' does what you want; you can even 'git config
>> format.coverletter=auto' to always include a decent cover letter on any
>> multi-patch series.
>>
> 
> This posting follows a little bit different pattern, one that I myself
> follow when posting patches for two (or more) components that must work
> in sync.

Ok, makes sense. Maybe the only additional suggestions would be to make
it more obvious in the subject line (put the text 'cross-post'
somewhere?) or have the first paragraph of the meta-cover be more
explicit that there are going to be multiple sub-threads, one per
project, where all subthreads must be applied to their corresponding
project for the overall feature to be complete?

[And maybe I should wait a few minutes for the full thread to appear in
my inbox, rather than immediately replying to the first mail while the
series is still incomplete due to mail delays...]

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Laszlo Ersek]

On 10/01/15 18:21, Eric Blake wrote:
> On 10/01/2015 10:17 AM, Laszlo Ersek wrote:
>> On 10/01/15 18:03, Eric Blake wrote:
>>> [meta-comment]
>>>
>>> On 10/01/2015 06:14 AM, Marc Marí wrote:
>>>> Implementation of the FW CFG DMA interface.
>>>
>>> The subject line is missing "v4" and "0/7". Also, the cover letter is
>>> missing a diffstat.  That makes it harder to see from the cover letter
>>> what the rest of the series is about.  'git format-patch/send-email
>>> --cover-letter' does what you want; you can even 'git config
>>> format.coverletter=auto' to always include a decent cover letter on any
>>> multi-patch series.
>>>
>>
>> This posting follows a little bit different pattern, one that I myself
>> follow when posting patches for two (or more) components that must work
>> in sync.
> 
> Ok, makes sense. Maybe the only additional suggestions would be to make
> it more obvious in the subject line (put the text 'cross-post'
> somewhere?) or have the first paragraph of the meta-cover be more
> explicit that there are going to be multiple sub-threads, one per
> project, where all subthreads must be applied to their corresponding
> project for the overall feature to be complete?

That's a good idea. I think prefixing the main blurb's subject with
[cross-post], and a "standard" first paragraph based on your above
suggestion, would be helpful.

> [And maybe I should wait a few minutes for the full thread to appear in
> my inbox, rather than immediately replying to the first mail while the
> series is still incomplete due to mail delays...]

I'm not patient; it would be unfair from me to expect others to be... :)

Laszlo

[Qemu-devel] QEMU fw_cfg DMA interface

[Marc Marí]

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 16a1b6e and SeaBIOS commit e4d2b8c
QEMU startup time: .080
BIOS startup time: .060
Kernel setup time: .586
Total time: .726

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.

Changes from v1:
 - Take into account order of fields in the FWCfgDmaAccess structure
 - Check and change endianness of FWCfgDmaAccess fields
 - Change order of fields in the FWCfgDmaAccess structure
 - Add FW_CFG_DMA_CTL_SKIP feature for control field
 - Split FW_CFG_SIZE in QEMU
 - Make FW_CFG_ID a bitmap of features
 - Add 64 bit address support for the transfer. Trigger when writing the low
   address, and address is 0 by default and at the end of each transfer.
 - Align ports and addresses.
 - Preserve old fw_cfg_comb_valid behaviour in QEMU
 - Update documentation to reflect all these changes

Changes from v2:
 - Make IOports fw_cfg DMA region a different IO region.
 - Reuse everything for MMIO and IOport DMA regions
 - Make transfer status only based on control field
 - Use DMA helpers instead of direct map/unmap
 - Change ARM fw_cfg DMA address space
 - Change Linux boot process to match linuxboot.S
 - Add select capabilities in the FWCfgDmaAccess struct
 - Update documentation to reflect all these changes

[Qemu-devel] QEMU fw_cfg DMA interface

[Marc Marí]

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.

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Stefan Hajnoczi]

On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com> 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

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Marc Marí]

On Thu, 6 Aug 2015 13:27:16 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com> 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

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Stefan Hajnoczi]

On Thu, Aug 6, 2015 at 1:37 PM, Marc Marí <markmb@redhat.com> wrote:
> On Thu, 6 Aug 2015 13:27:16 +0100
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
>> On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com> 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

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Kevin O'Connor]

On Thu, Aug 06, 2015 at 02:37:45PM +0200, Marc Marí wrote:
> On Thu, 6 Aug 2015 13:27:16 +0100
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> > On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com> 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

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Marc Marí]

[-- Attachment #1: Type: text/plain, Size: 2332 bytes --]

On Thu, 6 Aug 2015 11:30:43 -0400
"Kevin O'Connor" <kevin@koconnor.net> 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 <stefanha@gmail.com> wrote:
> > 
> > > On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com>
> > > 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

[-- Attachment #2: run_test.sh --]
[-- Type: application/x-shellscript, Size: 1401 bytes --]

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Kevin O'Connor]

Trimming CC list.

On Thu, Aug 06, 2015 at 05:53:26PM +0200, Marc Marí wrote:
> On Thu, 6 Aug 2015 11:30:43 -0400
> "Kevin O'Connor" <kevin@koconnor.net> 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 <stefanha@gmail.com> wrote:
> > > 
> > > > On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com>
> > > > 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

Thanks.  One thing that immediately pops up is that even though
"-nographic" is used, it looks like QEMU still emulates a VGA device
and that device has an option rom that takes several milliseconds to
init the display hardware.  Adding "-device VGA,romfile=" to the qemu
command line will further reduce the boot time by avoiding this
hardware init delay.

-Kevin

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Gerd Hoffmann]

  Hi,

> > 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
> 
> Thanks.  One thing that immediately pops up is that even though
> "-nographic" is used, it looks like QEMU still emulates a VGA device
> and that device has an option rom that takes several milliseconds to
> init the display hardware.  Adding "-device VGA,romfile=" to the qemu
> command line will further reduce the boot time by avoiding this
> hardware init delay.

Same goes for the NIC btw.

cheers,
  Gerd