[Qemu-devel] qemu-system-arm icp_control error
Hi I am testing an ARM Linux 2.6.10 Image(zImage). When I run qemu-system-arm -kernel zImage -nographic the following error happened. Could anybody give what is wrong? Thanks Terry qemu: hardware error: icp_control_write: Bad offset 7fffe8 CPU #0: R00= R01=0113 R02=0100 R03= R04= R05= R06= R07= R08=cb80 R09= R10= R11= R12= R13= R14=00010088 R15=0001 PSR=81db N--- A und32 Aborted (core dumped)
[Qemu-devel] Re: [PATCH] block: Make BSG detection more sane in hdev_open()
On Sat, 2010-08-21 at 16:01 -0700, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger
>
> Greetings hch, tomo and Co,
>
> This patch changes the Linux BSG backstore detect logic in hdev_open()
> in order to determine when to actually set 'bs->sg = BDS_BSG;' by obtaining
> the BSG major from a SysFS attribute in /sys/class/bsg/$H:C:T:L/dev,
> instead of the original hardcoded 254 major check.
>
> This patch has been tested with Linux KVM guest v2.6.26 using the megasas
> HBA emulation SGL passthrough from a TCM_Loop IBLOCK backstore running on
> v2.6.35 x86_64.
>
> Thanks to Mark Harvey for initially contributing code for doing this with
> tgt.git/usr/bs_sg.c!
>
> Thanks!
>
> Signed-off-by: Nicholas A. Bellinger
> ---
> block/raw-posix.c | 51 +++
> 1 files changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index e7afc4a..487e7f0 100644
> +++ b/block/raw-posix.c
> @@ -842,7 +842,10 @@ static int hdev_open(BlockDriverState *bs, const char
> *filename, int flags)
> {
> BDRVRawState *s = bs->opaque;
> #if defined(__linux__)
> -struct stat st;
> +struct stat st, st2;
> +FILE *file;
> +char major[8], dev[64], path[128], *p, *buf;
> +int ch, i;
> #endif
>
> #ifdef CONFIG_COCOA
> @@ -881,11 +884,51 @@ static int hdev_open(BlockDriverState *bs, const char
> *filename, int flags)
> if (S_ISCHR(st.st_mode)) {
> if (major(st.st_rdev) == SCSI_GENERIC_MAJOR) {
> bs->sg = BDS_SCSI_GENERIC;
> -} else if (major(st.st_rdev) == 254) {
> -/* This is not yet defined in include/linux/major.h.. */
> -bs->sg = BDS_BSG;
> +} else { /* Extract major for BSG backstore usage */
> +memset(major, 0, 8);
> +memset(dev, 0, 64);
> +memset(path, 0, 128);
> +
> +buf = strdup(filename);
> +if (!(buf))
> +goto out;
Ugh, one quick followup on this to address missing free(buf) calls from
the usage of buf = strdup(filename).
http://git.kernel.org/?p=virt/kvm/nab/qemu-kvm.git;a=commitdiff;h=9cc774b1766c07065dee4637febe9f7d9237ff7e
Thanks,
--nab
> +/*
> + * Locate the device name from the path, we are interested
> + * in the last strsep() token..
> + */
> +while ((p = strsep(&buf, "/")))
> +snprintf(dev, 64, "%s", p);
> +/*
> + * Check to sure the sysfs entry exists before calling open
> + */
> +snprintf(path, 128, "/sys/class/bsg/%s/dev", dev);
> +if (stat(path, &st2) < 0)
> +goto out;
> +
> +file = fopen(path, "r");
> +if (!(file)) {
> +printf("fopen() failed for BSG sysfs path: %s\n", path);
> +goto out;
> +}
> +ch = fgetc(file);
> +for (i = 0; i < 7; i++) {
> +if (ch == ':') {
> +major[i] = '\0';
> +break;
> +}
> +major[i] = ch;
> +ch = fgetc(file);
> +}
> +fclose(file);
> +/*
> + * If the major returned by /sys/class/bsg/$H:C:T:L/dev matches
> + * stat(), then we signal BDS_BSG usage.
> + */
> +if (major(st.st_rdev) == atoi(major))
> +bs->sg = BDS_BSG;
> }
> }
> +out:
> #endif
>
> return raw_open_common(bs, filename, flags, 0);
> --
> 1.5.6.5
>
[Qemu-devel] [PATCH] block: Make BSG detection more sane in hdev_open()
From: Nicholas Bellinger
Greetings hch, tomo and Co,
This patch changes the Linux BSG backstore detect logic in hdev_open()
in order to determine when to actually set 'bs->sg = BDS_BSG;' by obtaining
the BSG major from a SysFS attribute in /sys/class/bsg/$H:C:T:L/dev,
instead of the original hardcoded 254 major check.
This patch has been tested with Linux KVM guest v2.6.26 using the megasas
HBA emulation SGL passthrough from a TCM_Loop IBLOCK backstore running on
v2.6.35 x86_64.
Thanks to Mark Harvey for initially contributing code for doing this with
tgt.git/usr/bs_sg.c!
Thanks!
Signed-off-by: Nicholas A. Bellinger
---
block/raw-posix.c | 51 +++
1 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index e7afc4a..487e7f0 100644
+++ b/block/raw-posix.c
@@ -842,7 +842,10 @@ static int hdev_open(BlockDriverState *bs, const char
*filename, int flags)
{
BDRVRawState *s = bs->opaque;
#if defined(__linux__)
-struct stat st;
+struct stat st, st2;
+FILE *file;
+char major[8], dev[64], path[128], *p, *buf;
+int ch, i;
#endif
#ifdef CONFIG_COCOA
@@ -881,11 +884,51 @@ static int hdev_open(BlockDriverState *bs, const char
*filename, int flags)
if (S_ISCHR(st.st_mode)) {
if (major(st.st_rdev) == SCSI_GENERIC_MAJOR) {
bs->sg = BDS_SCSI_GENERIC;
-} else if (major(st.st_rdev) == 254) {
-/* This is not yet defined in include/linux/major.h.. */
-bs->sg = BDS_BSG;
+} else { /* Extract major for BSG backstore usage */
+memset(major, 0, 8);
+memset(dev, 0, 64);
+memset(path, 0, 128);
+
+buf = strdup(filename);
+if (!(buf))
+goto out;
+/*
+ * Locate the device name from the path, we are interested
+ * in the last strsep() token..
+ */
+while ((p = strsep(&buf, "/")))
+snprintf(dev, 64, "%s", p);
+/*
+ * Check to sure the sysfs entry exists before calling open
+ */
+snprintf(path, 128, "/sys/class/bsg/%s/dev", dev);
+if (stat(path, &st2) < 0)
+goto out;
+
+file = fopen(path, "r");
+if (!(file)) {
+printf("fopen() failed for BSG sysfs path: %s\n", path);
+goto out;
+}
+ch = fgetc(file);
+for (i = 0; i < 7; i++) {
+if (ch == ':') {
+major[i] = '\0';
+break;
+}
+major[i] = ch;
+ch = fgetc(file);
+}
+fclose(file);
+/*
+ * If the major returned by /sys/class/bsg/$H:C:T:L/dev matches
+ * stat(), then we signal BDS_BSG usage.
+ */
+if (major(st.st_rdev) == atoi(major))
+bs->sg = BDS_BSG;
}
}
+out:
#endif
return raw_open_common(bs, filename, flags, 0);
--
1.5.6.5
[Qemu-devel] [PATCH] fat_chksum(): fix access above array bounds
Signed-off-by: Loïc Minier
---
block/vvfat.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index 6d61c2e..365332a 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -512,7 +512,7 @@ static inline uint8_t fat_chksum(const direntry_t* entry)
for(i=0;i<11;i++) {
unsigned char c;
-c = (i <= 8) ? entry->name[i] : entry->extension[i-8];
+c = (i < 8) ? entry->name[i] : entry->extension[i-8];
chksum=(((chksum&0xfe)>>1)|((chksum&0x01)?0x80:0)) + c;
}
--
1.7.1
[Qemu-devel] [Bug 621950] Re: qemu not able to run 64 bit OS when -enable-kvm is used with a 64 bit processor
Because you're using a 32-bit OS in the host. ** Changed in: qemu Status: New => Invalid -- qemu not able to run 64 bit OS when -enable-kvm is used with a 64 bit processor https://bugs.launchpad.net/bugs/621950 You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. Status in QEMU: Invalid Bug description: Hi, The host processor is a 64 bit processor (as below) When I run the emulated mode of 64 bit (full emulation) I can run the 64 bit OS fine. when I start using -enable-kvm, I get error message that "Your CPU does not support long mode. Use a 32bit distribution." trying to install SuSe linux enterprise server 64 bit Information -- -- processor cat /proc/cpuinfo processor : 3 vendor_id : GenuineIntel cpu family : 6 model : 15 model name : Intel(R) Core(TM)2 Quad CPU @ 2.66GHz stepping: 7 cpu MHz : 2671.246 cache size : 4096 KB physical id : 0 siblings: 4 core id : 3 cpu cores : 4 apicid : 3 initial apicid : 3 fdiv_bug: no hlt_bug : no f00f_bug: no coma_bug: no fpu : yes fpu_exception : yes cpuid level : 10 wp : yes flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe lm constant_tsc arch_perfmon pebs bts aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm tpr_shadow bogomips: 5342.46 clflush size: 64 cache_alignment : 64 address sizes : 36 bits physical, 48 bits virtual power management: -- command line used qemu-system-x86_64 -enable-kvm -cdrom /img/cd.iso why is kvm not able to use 64 bit capability in a host which is a 64 bit processor ?
[Qemu-devel] [Bug 621950] [NEW] qemu not able to run 64 bit OS when -enable-kvm is used with a 64 bit processor
Public bug reported: Hi, The host processor is a 64 bit processor (as below) When I run the emulated mode of 64 bit (full emulation) I can run the 64 bit OS fine. when I start using -enable-kvm, I get error message that "Your CPU does not support long mode. Use a 32bit distribution." trying to install SuSe linux enterprise server 64 bit Information -- -- processor cat /proc/cpuinfo processor : 3 vendor_id : GenuineIntel cpu family : 6 model : 15 model name : Intel(R) Core(TM)2 Quad CPU @ 2.66GHz stepping: 7 cpu MHz : 2671.246 cache size : 4096 KB physical id : 0 siblings: 4 core id : 3 cpu cores : 4 apicid : 3 initial apicid : 3 fdiv_bug: no hlt_bug : no f00f_bug: no coma_bug: no fpu : yes fpu_exception : yes cpuid level : 10 wp : yes flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe lm constant_tsc arch_perfmon pebs bts aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm tpr_shadow bogomips: 5342.46 clflush size: 64 cache_alignment : 64 address sizes : 36 bits physical, 48 bits virtual power management: -- command line used qemu-system-x86_64 -enable-kvm -cdrom /img/cd.iso why is kvm not able to use 64 bit capability in a host which is a 64 bit processor ? ** Affects: qemu Importance: Undecided Status: New -- qemu not able to run 64 bit OS when -enable-kvm is used with a 64 bit processor https://bugs.launchpad.net/bugs/621950 You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. Status in QEMU: New Bug description: Hi, The host processor is a 64 bit processor (as below) When I run the emulated mode of 64 bit (full emulation) I can run the 64 bit OS fine. when I start using -enable-kvm, I get error message that "Your CPU does not support long mode. Use a 32bit distribution." trying to install SuSe linux enterprise server 64 bit Information -- -- processor cat /proc/cpuinfo processor : 3 vendor_id : GenuineIntel cpu family : 6 model : 15 model name : Intel(R) Core(TM)2 Quad CPU @ 2.66GHz stepping: 7 cpu MHz : 2671.246 cache size : 4096 KB physical id : 0 siblings: 4 core id : 3 cpu cores : 4 apicid : 3 initial apicid : 3 fdiv_bug: no hlt_bug : no f00f_bug: no coma_bug: no fpu : yes fpu_exception : yes cpuid level : 10 wp : yes flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe lm constant_tsc arch_perfmon pebs bts aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm tpr_shadow bogomips: 5342.46 clflush size: 64 cache_alignment : 64 address sizes : 36 bits physical, 48 bits virtual power management: -- command line used qemu-system-x86_64 -enable-kvm -cdrom /img/cd.iso why is kvm not able to use 64 bit capability in a host which is a 64 bit processor ?
Re: [Qemu-devel] qemu-system-arm aborted
2010/8/21 S W > Hi there > > I made an ARM Linux2.6.10 image(zImage) and I tried to run it on QEMU using > the following command. > > qemu-system-arm -kernel zImage -nographic > > I got an error message like: > > CPU #0: > R00= R01=0113 R02=0100 R03= > R04= R05= R06= R07= > R08=cb80 R09= R10= R11= > R12= R13= R14=00010088 R15=0001 > PSR=81db N--- A und32 > > Is it an Edian problem? I checked my Linux configuration file. > The setting is Big Endian. > You should use --target-list=armeb-softmmu when using Big endian in configuration But armeb can' be built in my system. So you may try > > Would you please give me any ideas? > > Thanks! > Terry > > >
Re: [Qemu-devel] [PATCH] qdev: Reset hotplugged devices
On 08/21/2010 05:07 AM, Markus Armbruster wrote:
diff --git a/vl.c b/vl.c
index b3e3676..5de1688 100644
--- a/vl.c
+++ b/vl.c
@@ -2968,6 +2968,9 @@ int main(int argc, char **argv, char **envp)
}
qemu_system_reset();
+
+qemu_register_reset((void *)qbus_reset_all, sysbus_get_default());
+
This is inconsistent with qdev_create(). qdev_create() uses null.
I agree with the N.B. in your commit message: the root of the tree
should be explicit. Implicit is too much magic. But you create a
second kind of magic. I don't object to how that works, only to having
two different kinds.
I'd suggest you either make your qemu_reset_all() work like existing
qdev_create(), i.e. null means root. Or change qdev_create() to work
like your qemu_reset_all(), i.e. use sysbus_get_default() instead of
null.
I'm getting rid of the NULL crap too although it's lower on my qdev
TODO.. sysbus_get_default() is a heck of a lot easier to grep for
though than NULL so I'd prefer to use this for now.
Regards,
Anthony Liguori
if (loadvm) {
if (load_vmstate(loadvm)< 0) {
autostart = 0;
qemu-devel@nongnu.org
On Sat, Aug 21, 2010 at 1:19 PM, Markus Armbruster wrote:
> Blue Swirl writes:
>
>> Combining bitwise AND and logical NOT is suspicious.
>>
>> Fixed by this Coccinelle script:
>> // From http://article.gmane.org/gmane.linux.kernel/646367
>> @@ expression E1,E2; @@
>> (
>> !E1 & !E2
>> |
>> - !E1 & E2
>> + !(E1 & E2)
>> )
>>
>> Signed-off-by: Blue Swirl
>> ---
>>
>> Maybe the middle hunk should be fixed this way instead:
>> - } else if ((rw == 1) & !matching->d) {
>> + } else if ((rw == 1) && !matching->d) {
>>
>> ---
>> hw/etraxfs_eth.c | 2 +-
>> target-sh4/helper.c | 4 ++--
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
>> index b897c9c..ade96f1 100644
>> --- a/hw/etraxfs_eth.c
>> +++ b/hw/etraxfs_eth.c
>> @@ -464,7 +464,7 @@ static int eth_match_groupaddr(struct fs_eth *eth,
>> const unsigned char *sa)
>>
>> /* First bit on the wire of a MAC address signals multicast or
>> physical address. */
>> - if (!m_individual && !sa[0] & 1)
>> + if (!m_individual && !(sa[0] & 1))
>
> Doesn't this fix a bug? If yes, then the commit message should state
> that, and assess impact.
I don't know, the patch wasn't a result from any specific bug hunting
but from syntax analysis.
>> return 0;
>>
>> /* Calculate the hash index for the GA registers. */
>> diff --git a/target-sh4/helper.c b/target-sh4/helper.c
>> index 9e70352..e457904 100644
>> --- a/target-sh4/helper.c
>> +++ b/target-sh4/helper.c
>> @@ -357,7 +357,7 @@ static int get_mmu_address(CPUState * env,
>> target_ulong * physical,
>> MMU_DTLB_VIOLATION_READ;
>> } else if ((rw == 1) && !(matching->pr & 1)) {
>> n = MMU_DTLB_VIOLATION_WRITE;
>> - } else if ((rw == 1) & !matching->d) {
>> + } else if (!(matching->d & (rw == 1))) {
>> n = MMU_DTLB_INITIAL_WRITE;
>> } else {
>> *prot = PAGE_READ;
>
> Way too clever for my taste. I'd prefer the alternative fix you
> mentioned above.
The effect is different, though.
>> @@ -407,7 +407,7 @@ static int get_physical_address(CPUState * env,
>> target_ulong * physical,
>> }
>>
>> /* If MMU is disabled, return the corresponding physical page */
>> - if (!env->mmucr & MMUCR_AT) {
>> + if (!(env->mmucr & MMUCR_AT)) {
>> *physical = address & 0x1FFF;
>> *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>> return MMU_OK;
>
>
> Doesn't this fix a bug?
Probably.
Re: [Qemu-devel] [PATCH - V3] Port codes from qemu-kvm to support booting from SCSI disk image
On Fri, Aug 20, 2010 at 09:39:10AM -0500, Anthony Liguori wrote: > On 08/20/2010 09:09 AM, Gerd Hoffmann wrote: > >Been there, tried that. It isn't *that* easy. The PCI ID in the > >option rom header doesn't match the PCI ID of the emulated lsi, so > >seabios refuses to load it from the rom bar. > > Heh, I was wondering why it didn't work unless I put rombar=0 :-) > > Is this fixable in a reasonable way or does PCI ID in the option rom > represent a much newer device that would trigger issues with guest > drivers? The PCI spec requires that the PCI IDs match - this is done so that a single rom can store multiple optionroms. One could place the optionrom in fw_cfg with a "file" name of "pci,.rom". SeaBIOS will then deploy that rom for every device that has the given vendor/devid. (It wont require matching PCI IDs ids in the rom.) -Kevin
[Qemu-devel] [Bug 621780] Re: 160 unused but set variables in QEMU
** Attachment added: "make.txt" https://bugs.launchpad.net/bugs/621780/+attachment/1508292/+files/make.txt -- 160 unused but set variables in QEMU https://bugs.launchpad.net/bugs/621780 You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. Status in QEMU: New Bug description: I use gcc from SVN on my desktop: aus...@midna:~/gits/qemu$ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/x86_64-unknown-linux-gnu/4.6.0/lto-wrapper Target: x86_64-unknown-linux-gnu Configured with: ./configure --enable-languages=c,c++ Thread model: posix gcc version 4.6.0 20100726 (experimental) (GCC) which enables a new warning, 'unused-but-set'. Normally, this wouldn't be a big deal, just an annoyance, but since QEMU builds with -Werror, it causes problems. Easy enough to workaround with 'make -k'. To make it a bit easier on you, I ran 'make -k ; make -k &> make.txt', and attached that here, so you can easily find the 160 cases that should be fixed in qemu without needing gcc 4.6.
[Qemu-devel] [Bug 621780] Re: 160 unused but set variables in QEMU
Forgot to add, this is with git head, commit cc597832119dd1504f1c1536bb5f903d8970af2a. -- 160 unused but set variables in QEMU https://bugs.launchpad.net/bugs/621780 You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. Status in QEMU: New Bug description: I use gcc from SVN on my desktop: aus...@midna:~/gits/qemu$ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/x86_64-unknown-linux-gnu/4.6.0/lto-wrapper Target: x86_64-unknown-linux-gnu Configured with: ./configure --enable-languages=c,c++ Thread model: posix gcc version 4.6.0 20100726 (experimental) (GCC) which enables a new warning, 'unused-but-set'. Normally, this wouldn't be a big deal, just an annoyance, but since QEMU builds with -Werror, it causes problems. Easy enough to workaround with 'make -k'. To make it a bit easier on you, I ran 'make -k ; make -k &> make.txt', and attached that here, so you can easily find the 160 cases that should be fixed in qemu without needing gcc 4.6.
[Qemu-devel] [Bug 621780] [NEW] 160 unused but set variables in QEMU
Public bug reported: I use gcc from SVN on my desktop: aus...@midna:~/gits/qemu$ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/x86_64-unknown-linux-gnu/4.6.0/lto-wrapper Target: x86_64-unknown-linux-gnu Configured with: ./configure --enable-languages=c,c++ Thread model: posix gcc version 4.6.0 20100726 (experimental) (GCC) which enables a new warning, 'unused-but-set'. Normally, this wouldn't be a big deal, just an annoyance, but since QEMU builds with -Werror, it causes problems. Easy enough to workaround with 'make -k'. To make it a bit easier on you, I ran 'make -k ; make -k &> make.txt', and attached that here, so you can easily find the 160 cases that should be fixed in qemu without needing gcc 4.6. ** Affects: qemu Importance: Undecided Status: New -- 160 unused but set variables in QEMU https://bugs.launchpad.net/bugs/621780 You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. Status in QEMU: New Bug description: I use gcc from SVN on my desktop: aus...@midna:~/gits/qemu$ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/x86_64-unknown-linux-gnu/4.6.0/lto-wrapper Target: x86_64-unknown-linux-gnu Configured with: ./configure --enable-languages=c,c++ Thread model: posix gcc version 4.6.0 20100726 (experimental) (GCC) which enables a new warning, 'unused-but-set'. Normally, this wouldn't be a big deal, just an annoyance, but since QEMU builds with -Werror, it causes problems. Easy enough to workaround with 'make -k'. To make it a bit easier on you, I ran 'make -k ; make -k &> make.txt', and attached that here, so you can easily find the 160 cases that should be fixed in qemu without needing gcc 4.6.
Re: [Qemu-devel] [PATCH 0/5] CODING_STYLE amendments
On Sat, Aug 21, 2010 at 12:24 PM, Markus Armbruster wrote:
> Blue Swirl writes:
>
>> On Sat, Aug 21, 2010 at 9:54 AM, Markus Armbruster wrote:
>>> Blue Swirl writes:
>>>
On Fri, Aug 20, 2010 at 6:44 PM, Blue Swirl wrote:
> On Fri, Aug 20, 2010 at 1:47 PM, Markus Armbruster
> wrote:
>> Anthony Liguori writes:
>>> To be perfectly honest, we have enough hard problems to solve in QEMU.
>>> We're spending a lot more time on coding style than we probably need
>>> to :-)
>>
>> In my not so humble opinion, that's because the current CODING_STYLE is
>> idiosyncratic, widely disliked (follows from idiosyncratic pretty much
>> inevitably), widely violated by existing code, and only haphazardly
>> enforced for new code.
>
> I think Coccinelle could help us here, it can check for some of the
> CODING_STYLE issues. We only need to include it to our build system
> and add git hooks if possible. It can also perform mechanical
> conversions (if desired).
This Coccinelle script seems to do the job:
>>> [...]
There are some formatting issues though, I get changes like:
- for (i=0; i<5; i++)
+ for(i = 0;i < 5;i++) {
Reformatting the expressions with more spaces is nice, but removing
the spaces between 'for' and '(' and especially after ';' is not.
>>>
>>> Please make sure that patch submitters can easily check their patches
>>> with your tool. Depending on coccinelle isn't a problem for me, but it
>>> may well be for others.
>>
>> Coccinelle depends on OCaml and lots of other stuff, but just I
>> installed it with 'aptitude install coccinelle'. These days you can
>> also check Linux kernel sources with the included Coccinelle scripts.
>
> Could be "fun" for developers using Windows. If they exist.
At least OCaml site offers binary download for Windows. I didn't
compile Coccinelle myself, so I don't know how much that helps.
>> But depending on Coccinelle for the build wouldn't be nice.
>>
>>> Unless you mass-convert existing code to your style, tools working on
>>> source files won't cut it, because reports of the patch's style
>>> violations are prone to drown in a sea of reports of preexisting style
>>> violations. There's a reason why Linux's scrtips/checkpatch.pl works on
>>> patch files.
>>
>> Mass conversion would have the benefit that submitters, who use old
>> code as their reference, are more likely to use the correct style.
>>
>>> Even a working patch checking tool can only address the last issue
>>> (haphazard enforcement), not the other ones. You may not care.
>>
>> Which other ones?
>
> Quoting myself:
>
> [...] the current CODING_STYLE is
> idiosyncratic,
Personal preference. I liked Fabrice's style but I also like current
style. I would probably like Linux style except for the LISPisms. I
don't like GNU or Java style.
> widely disliked (follows from idiosyncratic pretty much
> inevitably),
How widely? How do you know that?
> widely violated by existing code,
The mechanical conversion would address that. We could of course
convert to some other style, or declare that we don't care about the
style after all. Or claim that we care, don't fix old code and whine
randomly at some submitters.
> and only haphazardly
> enforced for new code.
I'd like to fix that.
>>> I still think inventing yet another idiosyncratic coding style plus
>>> tools to enforce it is a waste of time.
>>
>> There are historical reasons for the style used in the current code
>> base. There are also reasons why CODING_STYLE was written like it
>> stands now.
>
> While wasting time for historical reasons is certainly better than
> wasting time for the heck of it, it's arguably worse than stopping the
> waste.
But how would you do that? Drop the CODING_STYLE (and accept
anything)? Switch to a new CODING_STYLE that is widely appreciated and
so all bikeshedding will cease? Enforce current style?
[Qemu-devel] patch: use correct output format in wm8750_set_format()
Looks like a typo. Found when compiling with gcc 4.6, which sets -Werror=unused-but-set-variable. Please cc me on any replies. -- -Austin diff --git a/hw/wm8750.c b/hw/wm8750.c index ce43c23..4c864e4 100644 --- a/hw/wm8750.c +++ b/hw/wm8750.c @@ -223,7 +223,7 @@ static void wm8750_set_format(WM8750State *s) CODEC ".headphone", s, wm8750_audio_out_cb, &out_fmt); /* MONOMIX is also in stereo for simplicity */ s->dac_voice[2] = AUD_open_out(&s->card, s->dac_voice[2], -CODEC ".monomix", s, wm8750_audio_out_cb, &out_fmt); +CODEC ".monomix", s, wm8750_audio_out_cb, &monoout_fmt); /* no sense emulating OUT3 which is a mix of other outputs */ wm8750_vol_update(s);
qemu-devel@nongnu.org
Blue Swirl writes:
> Combining bitwise AND and logical NOT is suspicious.
>
> Fixed by this Coccinelle script:
> // From http://article.gmane.org/gmane.linux.kernel/646367
> @@ expression E1,E2; @@
> (
> !E1 & !E2
> |
> - !E1 & E2
> + !(E1 & E2)
> )
>
> Signed-off-by: Blue Swirl
> ---
>
> Maybe the middle hunk should be fixed this way instead:
> -} else if ((rw == 1) & !matching->d) {
> +} else if ((rw == 1) && !matching->d) {
>
> ---
> hw/etraxfs_eth.c|2 +-
> target-sh4/helper.c |4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
> index b897c9c..ade96f1 100644
> --- a/hw/etraxfs_eth.c
> +++ b/hw/etraxfs_eth.c
> @@ -464,7 +464,7 @@ static int eth_match_groupaddr(struct fs_eth *eth,
> const unsigned char *sa)
>
> /* First bit on the wire of a MAC address signals multicast or
> physical address. */
> - if (!m_individual && !sa[0] & 1)
> + if (!m_individual && !(sa[0] & 1))
Doesn't this fix a bug? If yes, then the commit message should state
that, and assess impact.
> return 0;
>
> /* Calculate the hash index for the GA registers. */
> diff --git a/target-sh4/helper.c b/target-sh4/helper.c
> index 9e70352..e457904 100644
> --- a/target-sh4/helper.c
> +++ b/target-sh4/helper.c
> @@ -357,7 +357,7 @@ static int get_mmu_address(CPUState * env,
> target_ulong * physical,
> MMU_DTLB_VIOLATION_READ;
> } else if ((rw == 1) && !(matching->pr & 1)) {
> n = MMU_DTLB_VIOLATION_WRITE;
> -} else if ((rw == 1) & !matching->d) {
> +} else if (!(matching->d & (rw == 1))) {
> n = MMU_DTLB_INITIAL_WRITE;
> } else {
> *prot = PAGE_READ;
Way too clever for my taste. I'd prefer the alternative fix you
mentioned above.
> @@ -407,7 +407,7 @@ static int get_physical_address(CPUState * env,
> target_ulong * physical,
> }
>
> /* If MMU is disabled, return the corresponding physical page */
> -if (!env->mmucr & MMUCR_AT) {
> +if (!(env->mmucr & MMUCR_AT)) {
> *physical = address & 0x1FFF;
> *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> return MMU_OK;
Doesn't this fix a bug?
Re: [Qemu-devel] [PATCH 0/5] CODING_STYLE amendments
Blue Swirl writes:
> On Sat, Aug 21, 2010 at 9:54 AM, Markus Armbruster wrote:
>> Blue Swirl writes:
>>
>>> On Fri, Aug 20, 2010 at 6:44 PM, Blue Swirl wrote:
On Fri, Aug 20, 2010 at 1:47 PM, Markus Armbruster
wrote:
> Anthony Liguori writes:
>> To be perfectly honest, we have enough hard problems to solve in QEMU.
>> We're spending a lot more time on coding style than we probably need
>> to :-)
>
> In my not so humble opinion, that's because the current CODING_STYLE is
> idiosyncratic, widely disliked (follows from idiosyncratic pretty much
> inevitably), widely violated by existing code, and only haphazardly
> enforced for new code.
I think Coccinelle could help us here, it can check for some of the
CODING_STYLE issues. We only need to include it to our build system
and add git hooks if possible. It can also perform mechanical
conversions (if desired).
>>>
>>> This Coccinelle script seems to do the job:
>> [...]
>>> There are some formatting issues though, I get changes like:
>>> - for (i=0; i<5; i++)
>>> + for(i = 0;i < 5;i++) {
>>>
>>> Reformatting the expressions with more spaces is nice, but removing
>>> the spaces between 'for' and '(' and especially after ';' is not.
>>
>> Please make sure that patch submitters can easily check their patches
>> with your tool. Depending on coccinelle isn't a problem for me, but it
>> may well be for others.
>
> Coccinelle depends on OCaml and lots of other stuff, but just I
> installed it with 'aptitude install coccinelle'. These days you can
> also check Linux kernel sources with the included Coccinelle scripts.
Could be "fun" for developers using Windows. If they exist.
> But depending on Coccinelle for the build wouldn't be nice.
>
>> Unless you mass-convert existing code to your style, tools working on
>> source files won't cut it, because reports of the patch's style
>> violations are prone to drown in a sea of reports of preexisting style
>> violations. There's a reason why Linux's scrtips/checkpatch.pl works on
>> patch files.
>
> Mass conversion would have the benefit that submitters, who use old
> code as their reference, are more likely to use the correct style.
>
>> Even a working patch checking tool can only address the last issue
>> (haphazard enforcement), not the other ones. You may not care.
>
> Which other ones?
Quoting myself:
[...] the current CODING_STYLE is
idiosyncratic, widely disliked (follows from idiosyncratic pretty much
inevitably), widely violated by existing code, and only haphazardly
enforced for new code.
>> I still think inventing yet another idiosyncratic coding style plus
>> tools to enforce it is a waste of time.
>
> There are historical reasons for the style used in the current code
> base. There are also reasons why CODING_STYLE was written like it
> stands now.
While wasting time for historical reasons is certainly better than
wasting time for the heck of it, it's arguably worse than stopping the
waste.
Re: [Qemu-devel] [PATCH 0/5] CODING_STYLE amendments
On Sat, Aug 21, 2010 at 9:54 AM, Markus Armbruster wrote:
> Blue Swirl writes:
>
>> On Fri, Aug 20, 2010 at 6:44 PM, Blue Swirl wrote:
>>> On Fri, Aug 20, 2010 at 1:47 PM, Markus Armbruster
>>> wrote:
Anthony Liguori writes:
> To be perfectly honest, we have enough hard problems to solve in QEMU.
> We're spending a lot more time on coding style than we probably need
> to :-)
In my not so humble opinion, that's because the current CODING_STYLE is
idiosyncratic, widely disliked (follows from idiosyncratic pretty much
inevitably), widely violated by existing code, and only haphazardly
enforced for new code.
>>>
>>> I think Coccinelle could help us here, it can check for some of the
>>> CODING_STYLE issues. We only need to include it to our build system
>>> and add git hooks if possible. It can also perform mechanical
>>> conversions (if desired).
>>
>> This Coccinelle script seems to do the job:
> [...]
>> There are some formatting issues though, I get changes like:
>> - for (i=0; i<5; i++)
>> + for(i = 0;i < 5;i++) {
>>
>> Reformatting the expressions with more spaces is nice, but removing
>> the spaces between 'for' and '(' and especially after ';' is not.
>
> Please make sure that patch submitters can easily check their patches
> with your tool. Depending on coccinelle isn't a problem for me, but it
> may well be for others.
Coccinelle depends on OCaml and lots of other stuff, but just I
installed it with 'aptitude install coccinelle'. These days you can
also check Linux kernel sources with the included Coccinelle scripts.
But depending on Coccinelle for the build wouldn't be nice.
> Unless you mass-convert existing code to your style, tools working on
> source files won't cut it, because reports of the patch's style
> violations are prone to drown in a sea of reports of preexisting style
> violations. There's a reason why Linux's scrtips/checkpatch.pl works on
> patch files.
Mass conversion would have the benefit that submitters, who use old
code as their reference, are more likely to use the correct style.
> Even a working patch checking tool can only address the last issue
> (haphazard enforcement), not the other ones. You may not care.
Which other ones?
> I still think inventing yet another idiosyncratic coding style plus
> tools to enforce it is a waste of time.
There are historical reasons for the style used in the current code
base. There are also reasons why CODING_STYLE was written like it
stands now.
Re: [Qemu-devel] segfault due to missing qdev_create()?
Hollis Blanchard writes:
> I am able to run qemu with the following commandline:
> /usr/local/bin/qemu-system-ppcemb -enable-kvm -kernel uImage.bamboo
> -nographic -M bamboo ppc440-angstrom-linux.img
>
> However, when I try to use virtio instead, I get this segfault:
> /usr/local/bin/qemu-system-ppcemb -enable-kvm -kernel uImage.bamboo
> -drive file=ppc440-angstrom-linux.img,if=virtio -nographic -M bamboo
>
> #0 0x1009864c in qbus_find_recursive (bus=0x0, name=0x0, info=0x10287238)
> at /home/hollisb/work/qemu.git/hw/qdev.c:461
> #1 0x10099cc4 in qdev_device_add (opts=0x108a07a0)
> at /home/hollisb/work/qemu.git/hw/qdev.c:229
> #2 0x101a4220 in device_init_func (opts=,
> opaque=) at /home/hollisb/work/qemu.git/vl.c:1519
> #3 0x1002baf8 in qemu_opts_foreach (list=,
> func=0x101a4204 , opaque=0x0,
> abort_on_failure=) at qemu-option.c:978
> #4 0x101a68e0 in main (argc=,
> argv=, envp=)
> at /home/hollisb/work/qemu.git/vl.c:2890
>
> This patch avoids the segfault, but just gives me this message: No
> 'PCI' bus found for device 'virtio-blk-pci'
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index e99c73f..8fe4f06 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -455,6 +455,9 @@ static BusState *qbus_find_recursive(BusState *bus, const
> ch
> BusState *child, *ret;
> int match = 1;
>
> + if (!bus)
> + return NULL;
> +
> if (name && (strcmp(bus->name, name) != 0)) {
> match = 0;
> }
Is your main_system_bus still null?
We should report the problem, not crash, of course.
>
> FWIW, hw/ppc4xx_pci.c is my PCI controller. Do I need to add some qdev
> magic to that file to make this work?
Yes, you need to convert the device providing your PCI bus to qdev
before you can use any qdevified PCI devices.
piix_pci.c could serve as an example. qdev works starts with commit
8a14daa5.
My KVM Forum slides might help with the basics:
http://www.linux-kvm.org/wiki/images/f/fe/2010-forum-armbru-qdev.pdf
I can try to assist and advice, just ask.
Re: [Qemu-devel] [PATCH] qdev: Reset hotplugged devices
Anthony Liguori writes:
> On 08/20/2010 11:14 AM, Markus Armbruster wrote:
>>> The real problem is how we do reset. We shouldn't register a reset
>>> handler for every qdev device but rather register a single reset
>>> handler that walks the device tree and calls reset on every reachable
>>> device.
>>>
>>> Then we can always call reset in init() and there's no need to have a
>>> dev->hotplugged check. The qdev device tree reset handler should not
>>> be registered until *after* we call qemu_system_reset() after creating
>>> the device model which will ensure that we don't do a double reset.
>>>
>> Fine with me.
>>
>> But we need to merge something short term (pre 0.13) to fix hot plug of
>> e1000 et al. Use Alex's patch as such a stop-gap?
>>
>
> No, we're accumulating crud in base qdev at an alarming rate. It's
> important to fix these things now before it gets prohibitively hard to
> take care of.
>
> Can you and Alex review/try the following patch? It seems to work for
> me although I'm not sure how to trigger the original bug.
>
> Regards,
>
> Anthony Liguori
Looks good to me, except I dislike one little thing:
>>From df719f1cc6ae2cd430e1cc47896a13d25af81e67 Mon Sep 17 00:00:00 2001
> From: Anthony Liguori
> Date: Fri, 20 Aug 2010 13:06:22 -0500
> Subject: [PATCH] qdev: fix reset with hotplug
>
> Devices expect to be reset after being initialized. Today, we achieve this by
> registering a reset handler in each qdev device. We then rely on this reset
> handler getting called after device init but before CPU execution runs.
>
> Since hot plug results in a device being initialized outside of the normal
> system reset, things go badly today.
>
> This patch changes the reset handling so that qdev has no knowledge of the
> global system reset. Instead, qdev devices are reset after initialization and
> then a new bus level function is introduced that allows all devices on the bus
> to be reset using a depth first transversal.
>
> We still need to do a system_reset before CPU init to preserve behavior of
> non-qdev devices so we make sure to register the qdev-based reset handler
> after
> that reset.
>
> N.B. we have to expose the implicit system bus because we have various hacks
> that result in an implicit system bus existing. Instead, we ought to have an
> explicitly created system bus that we can trigger reset from. That's a topic
> for a future patch though.
>
> Signed-off-by: Anthony Liguori
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index e99c73f..dfd91d7 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
[...]
> +BusState *sysbus_get_default(void)
> +{
> +return main_system_bus;
> +}
> +
> +void qbus_reset_all(BusState *bus)
> +{
> +qbus_walk_children(bus, qdev_reset_one, NULL);
> +}
> +
> /* can be used as ->unplug() callback for the simple cases */
> int qdev_simple_unplug_cb(DeviceState *dev)
> {
[...]
> diff --git a/vl.c b/vl.c
> index b3e3676..5de1688 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2968,6 +2968,9 @@ int main(int argc, char **argv, char **envp)
> }
>
> qemu_system_reset();
> +
> +qemu_register_reset((void *)qbus_reset_all, sysbus_get_default());
> +
This is inconsistent with qdev_create(). qdev_create() uses null.
I agree with the N.B. in your commit message: the root of the tree
should be explicit. Implicit is too much magic. But you create a
second kind of magic. I don't object to how that works, only to having
two different kinds.
I'd suggest you either make your qemu_reset_all() work like existing
qdev_create(), i.e. null means root. Or change qdev_create() to work
like your qemu_reset_all(), i.e. use sysbus_get_default() instead of
null.
> if (loadvm) {
> if (load_vmstate(loadvm) < 0) {
> autostart = 0;
Re: [Qemu-devel] [PATCH 0/5] CODING_STYLE amendments
Blue Swirl writes:
> On Fri, Aug 20, 2010 at 6:44 PM, Blue Swirl wrote:
>> On Fri, Aug 20, 2010 at 1:47 PM, Markus Armbruster wrote:
>>> Anthony Liguori writes:
To be perfectly honest, we have enough hard problems to solve in QEMU.
We're spending a lot more time on coding style than we probably need
to :-)
>>>
>>> In my not so humble opinion, that's because the current CODING_STYLE is
>>> idiosyncratic, widely disliked (follows from idiosyncratic pretty much
>>> inevitably), widely violated by existing code, and only haphazardly
>>> enforced for new code.
>>
>> I think Coccinelle could help us here, it can check for some of the
>> CODING_STYLE issues. We only need to include it to our build system
>> and add git hooks if possible. It can also perform mechanical
>> conversions (if desired).
>
> This Coccinelle script seems to do the job:
[...]
> There are some formatting issues though, I get changes like:
> -for (i=0; i<5; i++)
> +for(i = 0;i < 5;i++) {
>
> Reformatting the expressions with more spaces is nice, but removing
> the spaces between 'for' and '(' and especially after ';' is not.
Please make sure that patch submitters can easily check their patches
with your tool. Depending on coccinelle isn't a problem for me, but it
may well be for others.
Unless you mass-convert existing code to your style, tools working on
source files won't cut it, because reports of the patch's style
violations are prone to drown in a sea of reports of preexisting style
violations. There's a reason why Linux's scrtips/checkpatch.pl works on
patch files.
Even a working patch checking tool can only address the last issue
(haphazard enforcement), not the other ones. You may not care.
I still think inventing yet another idiosyncratic coding style plus
tools to enforce it is a waste of time.
qemu-devel@nongnu.org
Combining bitwise AND and logical NOT is suspicious.
Fixed by this Coccinelle script:
// From http://article.gmane.org/gmane.linux.kernel/646367
@@ expression E1,E2; @@
(
!E1 & !E2
|
- !E1 & E2
+ !(E1 & E2)
)
Signed-off-by: Blue Swirl
---
Maybe the middle hunk should be fixed this way instead:
-} else if ((rw == 1) & !matching->d) {
+} else if ((rw == 1) && !matching->d) {
---
hw/etraxfs_eth.c|2 +-
target-sh4/helper.c |4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
index b897c9c..ade96f1 100644
--- a/hw/etraxfs_eth.c
+++ b/hw/etraxfs_eth.c
@@ -464,7 +464,7 @@ static int eth_match_groupaddr(struct fs_eth *eth,
const unsigned char *sa)
/* First bit on the wire of a MAC address signals multicast or
physical address. */
- if (!m_individual && !sa[0] & 1)
+ if (!m_individual && !(sa[0] & 1))
return 0;
/* Calculate the hash index for the GA registers. */
diff --git a/target-sh4/helper.c b/target-sh4/helper.c
index 9e70352..e457904 100644
--- a/target-sh4/helper.c
+++ b/target-sh4/helper.c
@@ -357,7 +357,7 @@ static int get_mmu_address(CPUState * env,
target_ulong * physical,
MMU_DTLB_VIOLATION_READ;
} else if ((rw == 1) && !(matching->pr & 1)) {
n = MMU_DTLB_VIOLATION_WRITE;
-} else if ((rw == 1) & !matching->d) {
+} else if (!(matching->d & (rw == 1))) {
n = MMU_DTLB_INITIAL_WRITE;
} else {
*prot = PAGE_READ;
@@ -407,7 +407,7 @@ static int get_physical_address(CPUState * env,
target_ulong * physical,
}
/* If MMU is disabled, return the corresponding physical page */
-if (!env->mmucr & MMUCR_AT) {
+if (!(env->mmucr & MMUCR_AT)) {
*physical = address & 0x1FFF;
*prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
return MMU_OK;
--
1.6.2.4
