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 <so...@cmu.edu> > --- > > 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 <so...@cmu.edu> > + * > + * 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 <mdr...@linux.vnet.ibm.com>\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; >
