diff mbox

[v3,0/5] fw_cfg DMA interface

Message ID 20150918182509.GA13450@morn.lan
State New
Headers show

Commit Message

Kevin O'Connor Sept. 18, 2015, 6:25 p.m. UTC 
On Fri, Sep 18, 2015 at 10:58:44AM +0200, Marc Marí wrote:
> Implement host-side of the FW CFG DMA interface both for x86 and ARM.
> 
> Based on Gerd Hoffman's initial implementation.

Thanks for working on this Marc!

Any chance you could add the patch below to the series (or merge it
into your series)?

The patch adds a signature to the DMA address IO register.  With the
current implementation, a future firmware would have to implement the
V1 fw_cfg interface just to probe for the dma interface.  It might be
useful if future firmwares (that don't care about backwards
compatibility with old versions of qemu) could probe for the dma
fw_cfg interface by just checking for a signature (and therefore not
require all the V1 code just to probe).

-Kevin


commit ae6d8df012ef9b21ae17bfb0383d116f71ba1d58
Author: Kevin O'Connor <kevin@koconnor.net>
Date:   Fri Sep 18 14:14:55 2015 -0400

    fw_cfg: Define a static signature to be returned on DMA port reads
    
    Return a static signature ("QEMU CFG") if the guest does a read to the
    DMA address io register.
    
    Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

Comments

Marc Marí Sept. 18, 2015, 7:14 p.m. UTC | #1 
On Fri, 18 Sep 2015 14:25:09 -0400
"Kevin O'Connor" <kevin@koconnor.net> wrote:

> On Fri, Sep 18, 2015 at 10:58:44AM +0200, Marc Marí wrote:
> > Implement host-side of the FW CFG DMA interface both for x86 and
> > ARM.
> > 
> > Based on Gerd Hoffman's initial implementation.
> 
> Thanks for working on this Marc!
> 
> Any chance you could add the patch below to the series (or merge it
> into your series)?

Unless it is decided to merge the series as is, I'll send another
version with the little nitpicks corrected. I'll add this patch too.

Thank you also for all the comments!

Marc

> The patch adds a signature to the DMA address IO register.  With the
> current implementation, a future firmware would have to implement the
> V1 fw_cfg interface just to probe for the dma interface.  It might be
> useful if future firmwares (that don't care about backwards
> compatibility with old versions of qemu) could probe for the dma
> fw_cfg interface by just checking for a signature (and therefore not
> require all the V1 code just to probe).
> 
> -Kevin
> 
> 
> commit ae6d8df012ef9b21ae17bfb0383d116f71ba1d58
> Author: Kevin O'Connor <kevin@koconnor.net>
> Date:   Fri Sep 18 14:14:55 2015 -0400
> 
>     fw_cfg: Define a static signature to be returned on DMA port reads
>     
>     Return a static signature ("QEMU CFG") if the guest does a read
> to the DMA address io register.
>     
>     Signed-off-by: Kevin O'Connor <kevin@koconnor.net>
> 
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index d5f9ddd..5bf3f65 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -93,6 +93,10 @@ by selecting the "signature" item using key 0x0000
> (FW_CFG_SIGNATU RE),
>  and reading four bytes from the data register. If the fw_cfg device
> is present, the four bytes read will contain the characters "QEMU".
>  
> +Additionaly, if the DMA interface is available then a read to the DMA
> +Address will return 0x51454d5520434647 ("QEMU CFG" in big-endian
> +format).
> +
>  === Revision / feature bitmap (Key 0x0001, FW_CFG_ID) ===
>  
>  A 32-bit little-endian unsigned int, this item is used to check for
> enabled diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index d11d8c5..d95075d 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -53,6 +53,8 @@
>  #define FW_CFG_DMA_CTL_SKIP    0x04
>  #define FW_CFG_DMA_CTL_SELECT  0x08
>  
> +#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647 /* "QEMU CFG" */
> +
>  typedef struct FWCfgEntry {
>      uint32_t len;
>      uint8_t *data;
> @@ -393,6 +395,12 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
>      trace_fw_cfg_read(s, 0);
>  }
>  
> +static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr,
> +                                    unsigned size)
> +{
> +    return FW_CFG_DMA_SIGNATURE >> ((8 - addr - size) * 8);
> +}
> +
>  static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
>                                   uint64_t value, unsigned size)
>  {
> @@ -416,8 +424,8 @@ static void fw_cfg_dma_mem_write(void *opaque,
> hwaddr addr, 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));
> +    return !is_write || ((size == 4 && (addr == 0 || addr == 4)) ||
> +                         (size == 8 && addr == 0));
>  }
>  
>  static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
> @@ -488,6 +496,7 @@ static const MemoryRegionOps fw_cfg_comb_mem_ops
> = { };
>  
>  static const MemoryRegionOps fw_cfg_dma_mem_ops = {
> +    .read = fw_cfg_dma_mem_read,
>      .write = fw_cfg_dma_mem_write,
>      .endianness = DEVICE_BIG_ENDIAN,
>      .valid.accepts = fw_cfg_dma_mem_valid,
>
Peter Maydell Sept. 18, 2015, 10:47 p.m. UTC | #2 
On 18 September 2015 at 19:25, Kevin O'Connor <kevin@koconnor.net> wrote:
> On Fri, Sep 18, 2015 at 10:58:44AM +0200, Marc Marí wrote:
>> Implement host-side of the FW CFG DMA interface both for x86 and ARM.
>>
>> Based on Gerd Hoffman's initial implementation.
>
> Thanks for working on this Marc!
>
> Any chance you could add the patch below to the series (or merge it
> into your series)?
>
> The patch adds a signature to the DMA address IO register.  With the
> current implementation, a future firmware would have to implement the
> V1 fw_cfg interface just to probe for the dma interface.  It might be
> useful if future firmwares (that don't care about backwards
> compatibility with old versions of qemu) could probe for the dma
> fw_cfg interface by just checking for a signature (and therefore not
> require all the V1 code just to probe).
>
> -Kevin
>
>
> commit ae6d8df012ef9b21ae17bfb0383d116f71ba1d58
> Author: Kevin O'Connor <kevin@koconnor.net>
> Date:   Fri Sep 18 14:14:55 2015 -0400
>
>     fw_cfg: Define a static signature to be returned on DMA port reads
>
>     Return a static signature ("QEMU CFG") if the guest does a read to the
>     DMA address io register.
>
>     Signed-off-by: Kevin O'Connor <kevin@koconnor.net>
>
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index d5f9ddd..5bf3f65 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -93,6 +93,10 @@ by selecting the "signature" item using key 0x0000 (FW_CFG_SIGNATU
> RE),
>  and reading four bytes from the data register. If the fw_cfg device is
>  present, the four bytes read will contain the characters "QEMU".
>
> +Additionaly, if the DMA interface is available then a read to the DMA
> +Address will return 0x51454d5520434647 ("QEMU CFG" in big-endian
> +format).
> +

I don't think I understand this. If you know the DMA Address
port or register exists, then you know (by definition) that
the DMA interface is available. If you don't know that the
DMA interface is available then you can't read from the DMA
Address port or register because it might not exist and could
therefore cause you to blow up.

If you want to be able to tell without doing the "use the
old-style interface to query the version" thing, then you
need to look in the ACPI or device tree tables (and those
tables need to be such that you can tell the difference,
which is the case for at least device tree; haven't checked
ACPI.)

thanks
-- PMM
Kevin O'Connor Sept. 18, 2015, 11:43 p.m. UTC | #3 
On Fri, Sep 18, 2015 at 11:47:52PM +0100, Peter Maydell wrote:
> On 18 September 2015 at 19:25, Kevin O'Connor <kevin@koconnor.net> wrote:
> > +Additionaly, if the DMA interface is available then a read to the DMA
> > +Address will return 0x51454d5520434647 ("QEMU CFG" in big-endian
> > +format).
> > +
> 
> I don't think I understand this. If you know the DMA Address
> port or register exists, then you know (by definition) that
> the DMA interface is available. If you don't know that the
> DMA interface is available then you can't read from the DMA
> Address port or register because it might not exist and could
> therefore cause you to blow up.
> 
> If you want to be able to tell without doing the "use the
> old-style interface to query the version" thing, then you
> need to look in the ACPI or device tree tables (and those
> tables need to be such that you can tell the difference,
> which is the case for at least device tree; haven't checked
> ACPI.)

Hi Peter,

On x86 the firmware can't use acpi (nor device tree) to find fw_cfg
because fw_cfg is what is used to transfer acpi to the firmware.  So,
the firmware just hard codes the address.  As a "sanity check", the
firmware currently checks for a signature before using fw_cfg to
verify everything is working correctly (outw(0x0000, 0x510);
inb(0x511) == 'Q'; inb(0x511) == 'E'; ...).  A check for the new dma
interface involves an additional query (outw(0x0001, 0x510);
inb(0x511) == 3; ...).

I'm proposing that a future firmware (that didn't need to support old
versions of QEMU) could use a simpler sanity check instead (inl(0x514)
== "QEMU"; inl(0x518) == " CFG").

Granted, both the old check and the new proposed check would not be
needed on platforms that have a device tree transmitted separately
from fw_cfg.  Though, even on those platforms, there is no harm in
defining what happens on a read event.

-Kevin
Peter Maydell Sept. 19, 2015, 9:48 a.m. UTC | #4 
On 19 September 2015 at 00:43, Kevin O'Connor <kevin@koconnor.net> wrote:
> On x86 the firmware can't use acpi (nor device tree) to find fw_cfg
> because fw_cfg is what is used to transfer acpi to the firmware.  So,
> the firmware just hard codes the address.  As a "sanity check", the
> firmware currently checks for a signature before using fw_cfg to
> verify everything is working correctly (outw(0x0000, 0x510);
> inb(0x511) == 'Q'; inb(0x511) == 'E'; ...).  A check for the new dma
> interface involves an additional query (outw(0x0001, 0x510);
> inb(0x511) == 3; ...).
>
> I'm proposing that a future firmware (that didn't need to support old
> versions of QEMU) could use a simpler sanity check instead (inl(0x514)
> == "QEMU"; inl(0x518) == " CFG").

But what happens if you try this on an old QEMU? Won't it not
have the newer ports present and so do bad things? At least
on ARM trying to read from something you don't know for certain
to exist is a bad idea because you're likely to get a fault.

thanks
-- PMM
Kevin O'Connor Sept. 19, 2015, 3:15 p.m. UTC | #5 
On Sat, Sep 19, 2015 at 10:48:37AM +0100, Peter Maydell wrote:
> On 19 September 2015 at 00:43, Kevin O'Connor <kevin@koconnor.net> wrote:
> > On x86 the firmware can't use acpi (nor device tree) to find fw_cfg
> > because fw_cfg is what is used to transfer acpi to the firmware.  So,
> > the firmware just hard codes the address.  As a "sanity check", the
> > firmware currently checks for a signature before using fw_cfg to
> > verify everything is working correctly (outw(0x0000, 0x510);
> > inb(0x511) == 'Q'; inb(0x511) == 'E'; ...).  A check for the new dma
> > interface involves an additional query (outw(0x0001, 0x510);
> > inb(0x511) == 3; ...).
> >
> > I'm proposing that a future firmware (that didn't need to support old
> > versions of QEMU) could use a simpler sanity check instead (inl(0x514)
> > == "QEMU"; inl(0x518) == " CFG").
> 
> But what happens if you try this on an old QEMU? Won't it not
> have the newer ports present and so do bad things? At least
> on ARM trying to read from something you don't know for certain
> to exist is a bad idea because you're likely to get a fault.

Not on x86 - it used to be the norm to probe for old ISA devices via
io port reads and writes (eg, serial ports and lpt ports were detected
that way).  Here's what adding this to seabios:

  dprintf(1, "outl: %x %x\n", inl(0x514), inl(0x518));

reports on qemu v2.3 and earlier:

  outl: ffffffff ffffffff

on latest qemu with Marc's patches:

  outl: 0 0

and with my additional patch:

  outl: 554d4551 47464320

It's not a huge deal if you don't want to include the additional
signature.  It's not required as the v1 signature check still works
(see docs/specs/fw_cfg.txt), but the v1 check is a bit ugly and a new
additional simpler signature didn't seem like it would hurt.

-Kevin
diff mbox

Patch

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index d5f9ddd..5bf3f65 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -93,6 +93,10 @@  by selecting the "signature" item using key 0x0000 (FW_CFG_SIGNATU
RE),
 and reading four bytes from the data register. If the fw_cfg device is
 present, the four bytes read will contain the characters "QEMU".
 
+Additionaly, if the DMA interface is available then a read to the DMA
+Address will return 0x51454d5520434647 ("QEMU CFG" in big-endian
+format).
+
 === Revision / feature bitmap (Key 0x0001, FW_CFG_ID) ===
 
 A 32-bit little-endian unsigned int, this item is used to check for enabled
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index d11d8c5..d95075d 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -53,6 +53,8 @@ 
 #define FW_CFG_DMA_CTL_SKIP    0x04
 #define FW_CFG_DMA_CTL_SELECT  0x08
 
+#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647 /* "QEMU CFG" */
+
 typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
@@ -393,6 +395,12 @@  static void fw_cfg_dma_transfer(FWCfgState *s)
     trace_fw_cfg_read(s, 0);
 }
 
+static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr,
+                                    unsigned size)
+{
+    return FW_CFG_DMA_SIGNATURE >> ((8 - addr - size) * 8);
+}
+
 static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
                                  uint64_t value, unsigned size)
 {
@@ -416,8 +424,8 @@  static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
 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));
+    return !is_write || ((size == 4 && (addr == 0 || addr == 4)) ||
+                         (size == 8 && addr == 0));
 }
 
 static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
@@ -488,6 +496,7 @@  static const MemoryRegionOps fw_cfg_comb_mem_ops = {
 };
 
 static const MemoryRegionOps fw_cfg_dma_mem_ops = {
+    .read = fw_cfg_dma_mem_read,
     .write = fw_cfg_dma_mem_write,
     .endianness = DEVICE_BIG_ENDIAN,
     .valid.accepts = fw_cfg_dma_mem_valid,