Re: [Qemu-devel] [PATCH 6/6] qga: RFC: guest-side retrieval of fw_cfg file

[Laszlo Ersek]

comments below

On 03/16/15 15:15, Gabriel L. Somlo wrote:
> Add -g (--get-fwcfg) client-mode option to qemu-ga, causing
> the named fw_cfg file to be retrieved and written to stdout.
> 
> Signed-off-by: Gabriel Somlo <address@hidden>
> ---
> 
> First off, I have NOT forgotten the suggestion to make this a
> standalone binary, and will do so when I submit it "for real".
> It's just more comfortable this way for quick-n-dirty testing :)
> 
> Two main issues I need help with before this would be ready to
> go upstream:
> 
>   1. I can't for the life of me figure out how to stop gcc -O2
>      from assuming the if() test below is ALWAYS FALSE, and thus
>      optimizing it out completely. For now I've forced -O0 on
>      the entire function, but for some reason fw_cfg_read(&fcfile, ...)
>      does not appear to count as potentially modifying fcfile...
> 
>   2. I'm toying with the idea of writing a kernel driver for fw_cfg
>      and thus having all this functionality reduced to
>      "cat /sys/firmware/fw_cfg/<filename> | grep <something>" :)
> 
>      Of course, I have no idea how that would work on Windows, so maybe
>      a binary spitting out a file is still the more portable way to go.
>      (not to mention that most of the code for that is already written
>      below).

Ignore windows, go for the Linux driver. Matt wants the kernel driver:
http://thread.gmane.org/gmane.comp.emulators.qemu/321875/focus=322263

Of course, for testing the feature, anything will do in the short term.
For the longer term, consider writing a test case. "tests/fw_cfg-test.c"
already exists, maybe you could extend that.

In any case, the "-g" option introduced here (for a one-shot invocation,
if I understand correctly) doesn't seem to match qga very well. Normally
you'd add a JSON/QMP API that would allow callers to interrogate the qga
daemon. But, again, the kernel driver is likely superior anyway.

> 
> Thanks much for any additional clue...
>   Gabriel
> 
>  qga/Makefile.objs      |  1 +
>  qga/get-fwcfg.c        | 92 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/guest-agent-core.h |  2 ++
>  qga/main.c             |  6 +++-
>  4 files changed, 100 insertions(+), 1 deletion(-)
>  create mode 100644 qga/get-fwcfg.c
> 
> diff --git a/qga/Makefile.objs b/qga/Makefile.objs
> index 1c5986c..ef53841 100644
> --- a/qga/Makefile.objs
> +++ b/qga/Makefile.objs
> @@ -4,5 +4,6 @@ qga-obj-$(CONFIG_WIN32) += commands-win32.o channel-win32.o 
> service-win32.o
>  qga-obj-$(CONFIG_WIN32) += vss-win32.o
>  qga-obj-y += qapi-generated/qga-qapi-types.o qapi-generated/qga-qapi-visit.o
>  qga-obj-y += qapi-generated/qga-qmp-marshal.o
> +qga-obj-y += get-fwcfg.o
>  
>  qga-vss-dll-obj-$(CONFIG_QGA_VSS) += vss-win32/
> diff --git a/qga/get-fwcfg.c b/qga/get-fwcfg.c
> new file mode 100644
> index 0000000..1928698
> --- /dev/null
> +++ b/qga/get-fwcfg.c
> @@ -0,0 +1,92 @@
> +/*
> + * QEMU Guest Agent: retrieve blob from fw_cfg device by name
> + *
> + * Copyright Carnegie Mellon University 2015

Hm? I thought... Well, nevermind, that's for another discussion. :)

> + *
> + * Author:
> + *  Gabriel L. Somlo  <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <sys/io.h>
> +#include "qemu/bswap.h"
> +#include "hw/nvram/fw_cfg.h"
> +#include "qga/guest-agent-core.h"
> +
> +#define PORT_FW_CFG_CTL       0x0510
> +#define PORT_FW_CFG_DATA      0x0511
> +
> +static void
> +fw_cfg_select(uint16_t f)
> +{
> +    outw(f, PORT_FW_CFG_CTL);
> +}

This won't fly for an ARM guest, of course. But, the kernel driver
should take over this burden anyway.

> +
> +static void
> +fw_cfg_read(void *buf, int len)
> +{
> +    insb(PORT_FW_CFG_DATA, buf, len);
> +}
> +
> +static void
> +fw_cfg_read_entry(void *buf, int e, int len)
> +{
> +    fw_cfg_select(e);
> +    fw_cfg_read(buf, len);
> +}
> +
> +int
> +__attribute__((optimize("O0"))) //FIXME: "gcc -O2" wrongfully optimizes 
> "if"!!!
> +ga_get_fwcfg(const char *filename)
> +{
> +    int i;
> +    uint32_t count, len = 0;
> +    uint16_t sel;
> +    uint8_t sig[] = "QEMU";
> +    FWCfgFile fcfile;
> +    void *buf;
> +
> +    /* ensure access to the fw_cfg device */
> +    if (ioperm(PORT_FW_CFG_CTL, 2, 1) != 0) {
> +        perror("ioperm failed");
> +        return EXIT_FAILURE;
> +    }
> +
> +    /* verify presence of fw_cfg device */
> +    fw_cfg_select(FW_CFG_SIGNATURE);
> +    for (i = 0; i < sizeof(sig) - 1; i++) {
> +        sig[i] = inb(PORT_FW_CFG_DATA);
> +    }
> +    if (memcmp(sig, "QEMU", sizeof(sig)) != 0) {
> +        fprintf(stderr, "fw_cfg signature not found!\n");
> +        return EXIT_FAILURE;
> +    }
> +
> +    /* read number of fw_cfg entries, then scan for requested entry by name 
> */
> +    fw_cfg_read_entry(&count, FW_CFG_FILE_DIR, sizeof(count));
> +    count = be32_to_cpu(count);
> +    for (i = 0; i < count; i++) {
> +        fw_cfg_read(&fcfile, sizeof(fcfile));
> +        //FIXME: why does gcc -O2 optimize away the whole if {} block 
> below?!?
> +        if (!strcmp(fcfile.name, filename)) {

So, according to your description at the top, and here, this test is
"always false", according to gcc, which means that strcmp() always
returns nonzero (according to gcc), meaning the two strings always differ.

My idea is that gcc optimizes this block away because it thinks it
detects undefined behavior somewhere. For example, if you omitted the
fw_cfg_read() call, that would indeed be the case, because the contents
of "fcfile" would be indeterminate then.

Hmmm. Look at this:

http://man7.org/linux/man-pages/man2/outb.2.html

       You must compile with -O or -O2 or similar.  The functions are
       defined as inline macros, and will not be substituted in without
       optimization enabled, causing unresolved references at link time.

I think fw_cfg_read() is inlined under -O2, and the insb() from that
function is somehow confusing gcc.

>From "/usr/include/sys/io.h", on my RHEL-7.1 laptop:

static __inline void
insb (unsigned short int __port, void *__addr, unsigned long int __count)
{
  __asm__ __volatile__ ("cld ; rep ; insb":"=D" (__addr), "=c" (__count)
                        :"d" (__port), "0" (__addr), "1" (__count));
}

Honestly, I don't understand the output operand list. INSB takes the
port in DX (which is '"d" (__port)' I think, in the input operand list).
For the REP prefix, CX should be set to the number of bytes, and that's
'"1" (__count)' in the input list (because #1 refers to the earlier
"=c"). The address is passed in ES:DI, ie. '"0" (__addr)', where #0
refers back to the earlier "=D".

But that doesn't explain why __addr and __count are in the *output*
operand list at all! If anything, CX and DI should be on the *clobber*
list instead (which is completely absent here). It makes no sense to
write the changed values of CX and DI back to the __addr and __count
function parameters (which are supposed to be local variables, sure, but
this function is also inlined, and that could cause some confusion).

... Now this is interesting! The Linux kernel used to use the same
definition for insb *until 2002*. Then it was rewritten. You won't even
find that commit in Linus's git repo -- you can find it in Thomas
Gleixner's "history" repo, which is -- as far as I remember -- a git
repo constructed from some other version control system's history, and
it carries patches that predate the kernel's move to git. Check it out:

http://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/?id=6b1ad46e

Notice how the prepatch code had matched what we have in glibc to this
day, and it was replaced with the following operand lists:

output:  "+D"(addr), "+c"(count)
input:   "d"(port)

According to the gcc documentation
<https://gcc.gnu.org/onlinedocs/gcc/Modifiers.html>, there's a *big*
difference between the constraint modifiers "=" and "+". Namely:

    ‘=’ identifies an operand which is only written;
    ‘+’ identifies an operand that is both read and written

Which means that the glibc version is *broken*. If ES:DI were never
read, then the processor wouldn't know where in memory to place the
datum from the port, and *that* seems consistent with gcc thinking that
"fcfile" is never modified (and remains indeterminate therefore)!

In short, I think you caught either a gcc or (much more likely) a glibc
bug. The glibc version dates back to glibc commit 40ff7949, 2002.

What do you think, Paolo?

Thanks
Laszlo


> +            len = be32_to_cpu(fcfile.size);
> +            sel = be16_to_cpu(fcfile.select);
> +            buf = g_malloc(len);
> +            fw_cfg_read_entry(buf, sel, len);
> +            if (write(STDOUT_FILENO, buf, len) != len) {
> +                fprintf(stderr, "Failed to write %s to stdout\n", filename);
> +                return EXIT_FAILURE;
> +            }
> +            return 0;;
> +        }
> +    }
> +
> +    /* requested entry not present in fw_cfg */
> +    fprintf(stderr, "File %s not found in fw_cfg!\n", filename);
> +    return EXIT_FAILURE;
> +}
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index e92c6ab..b859e08 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -41,3 +41,5 @@ int64_t ga_get_fd_handle(GAState *s, Error **errp);
>  #ifndef _WIN32
>  void reopen_fd_to_null(int fd);
>  #endif
> +
> +int ga_get_fwcfg(const char *file);
> diff --git a/qga/main.c b/qga/main.c
> index 9939a2b..f9c1ece 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -215,6 +215,7 @@ static void usage(const char *cmd)
>  #endif
>  "  -b, --blacklist   comma-separated list of RPCs to disable (no spaces, 
> \"?\"\n"
>  "                    to list available RPCs)\n"
> +"  -g, --get-fwcfg   dump the content of a given fw_cfg file to stdout\n"
>  "  -h, --help        display this help and exit\n"
>  "\n"
>  "Report bugs to <address@hidden>\n"
> @@ -923,7 +924,7 @@ static void ga_print_cmd(QmpCommand *cmd, void *opaque)
>  
>  int main(int argc, char **argv)
>  {
> -    const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> +    const char *sopt = "hVvdm:p:l:f:F::b:s:t:g:";
>      const char *method = NULL, *path = NULL;
>      const char *log_filepath = NULL;
>      const char *pid_filepath;
> @@ -951,6 +952,7 @@ int main(int argc, char **argv)
>          { "service", 1, NULL, 's' },
>  #endif
>          { "statedir", 1, NULL, 't' },
> +        { "get-fwcfg", 1, NULL, 'g' },
>          { NULL, 0, NULL, 0 }
>      };
>      int opt_ind = 0, ch, daemonize = 0, i, j, len;
> @@ -1042,6 +1044,8 @@ int main(int argc, char **argv)
>              }
>              break;
>  #endif
> +        case 'g':
> +            return ga_get_fwcfg(optarg);
>          case 'h':
>              usage(argv[0]);
>              return 0;
>