[Qemu-devel] Re: [PATCH 2/7] pci: memory access API and IOMMU support
On Sat, Sep 04, 2010 at 09:01:06AM +, Blue Swirl wrote: On Thu, Sep 2, 2010 at 9:49 AM, Michael S. Tsirkin m...@redhat.com wrote: On Thu, Sep 02, 2010 at 11:40:58AM +0300, Eduard - Gabriel Munteanu wrote: On Thu, Sep 02, 2010 at 08:28:26AM +0300, Michael S. Tsirkin wrote: On Sat, Aug 28, 2010 at 05:54:53PM +0300, Eduard - Gabriel Munteanu wrote: PCI devices should access memory through pci_memory_*() instead of cpu_physical_memory_*(). This also provides support for translation and access checking in case an IOMMU is emulated. Memory maps are treated as remote IOTLBs (that is, translation caches belonging to the IOMMU-aware device itself). Clients (devices) must provide callbacks for map invalidation in case these maps are persistent beyond the current I/O context, e.g. AIO DMA transfers. Signed-off-by: Eduard - Gabriel Munteanu eduard.munte...@linux360.ro I am concerned about adding more pointer chaising on data path. Could we have 1. an iommu pointer in a device, inherited by secondary buses when they are created and by devices from buses when they are attached. 2. translation pointer in the iommu instead of the bus The first solution I proposed was based on qdev, that is, each DeviceState had an 'iommu' field. Translation would be done by recursively looking in the parent bus/devs for an IOMMU. But Anthony said we're better off with bus-specific APIs, mostly because (IIRC) there may be different types of addresses and it might be difficult to abstract those properly. Well we ended up with casting away types to make pci callbacks fit in ide structure, and silently assuming that all addresses are in fact 64 bit. So maybe it's hard to abstract addresses properly, but it appears we'll have to, to avoid even worse problems. I suppose I could revisit the idea by integrating the IOMMU in a PCIDevice as opposed to a DeviceState. Anthony, Paul, any thoughts on this? Just to clarify: this is an optimization idea: instead of a bus walk on each access, do the walk when device is attached to the bus, and copy the iommu from the root to the device itself. This will also make it possible to create DMADeviceState structure which would have this iommu field, and we'd use this structure instead of the void pointers all over. 3. pci_memory_XX functions inline, doing fast path for non-iommu case: if (__builtin_expect(!dev-iommu, 1) return cpu_memory_rw But isn't this some sort of 'if (likely(!dev-iommu))' from the Linux kernel? If so, it puts the IOMMU-enabled case at disadvantage. IOMMU has a ton of indirections anyway. I suppose most emulated systems would have at least some theoretical reasons to enable the IOMMU, e.g. as a GART replacement (say for 32-bit PCI devices) or for userspace drivers. So there are reasons to enable the IOMMU even when you don't have a real host IOMMU and you're not using nested guests. The time most people enable iommu for all devices in both real and virtualized systems appears distant, one of the reasons is because it has a lot of overhead. Let's start with not adding overhead for existing users, makes sense? I think the goal architecture (not for IOMMU, but in general) is one with zero copy DMA. This means we have stage one where the addresses are translated to host pointers and stage two where the read/write operation happens. The devices need to be converted. Now, let's consider the IOMMU in this zero copy architecture. It's one stage of address translation, for the access operation it will not matter. We can add translation caching at device level (or even at any intermediate levels), but that needs a cache invalidation callback system as discussed earlier. This can be implemented later, we need the zero copy stuff first. Given this overall picture, I think eliminating some pointer dereference overheads in non-zero copy architecture is a very premature optimization and it may even direct the architecture to wrong direction. If the performance degradation at this point is not acceptable, we could also postpone merging IOMMU until zero copy conversion has happened, or make IOMMU a compile time option. But it would be nice to back the decision by performance figures. I agree, a minimal benchmark showing no performance impact when disabled would put these concerns to rest. -- MST
[Qemu-devel] Re: [PATCH 00/14] trace: Add static tracing to QEMU
On Thu, Aug 12, 2010 at 11:36:21AM +0100, Stefan Hajnoczi wrote: 2. The built-in 'simple' trace backend writes binary traces to /tmp/trace-pid. Saving files with predictable names in /tmp is usually not a good idea, see e.g. http://en.wikipedia.org/wiki/Symlink_race. Put them in $HOME or something like that. -- MST
Re: [Qemu-devel] Unmaintained QEMU builds
On 04.09.2010, at 16:41, Andreas Färber wrote: Am 17.08.2010 um 21:56 schrieb Anthony Liguori: I think we have a lot of dump-and-run features in QEMU whereas someone writes the patches to implement something and then disappears. Often time, the feature is not generally useful so the code just rots. I think an awful lot of the PPC boards and devices also fall into this category. Considering that we're well over half a million lines of code today, I think we would do ourselves a favor by dropping some of the dead features we're carrying. The only really broken ppc sysemu should be PReP atm, no? Otherwise I'm just aware of some OpenBIOS bugs. Qemu currently tries to potentially emulate every CPU flavor that ever existed in the world. I have no idea if the original 601 implementation still works. Or 603. Not to speak of the halfway-implemented BookE CPUs. I'm also fairly sure that no emulated 64 bit CPU but the G5 work. I think the right thing to do for PPC would be to focus on a subset of CPUs we care about and make sure those work in combination. Also, the board emulation is ... eh ... suboptimal. The code is weirdly structured and I've already squashed a lot of bugs in there to make it barely work with Linux, but I have no idea about other OSs. The thing we _really_ should do there would be a from-scratch implementation of a U2 (32-bit) and a U3/U4 (64-bit) board and just forget about the old stuff. The big thing is that it costs time to do that and requires documentation, which all except for the U4 lack. I'm also fairly sure I won't get to it anytime soon :). Alex
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin m...@redhat.com wrote:
On Sun, Sep 05, 2010 at 09:06:10AM +, Blue Swirl wrote:
On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin m...@redhat.com wrote:
On Sat, Sep 04, 2010 at 05:21:24PM +, Blue Swirl wrote:
In the unsigned number space, the checks can be merged into one,
assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
could have:
- if (event 0 || event = BLKDBG_EVENT_MAX) {
+ if ((int)event 0 || event = BLKDBG_EVENT_MAX) {
This would also implement the check that the writer of this code was
trying to make.
The important thing to note is however is that the check as it is now
is not correct.
I agree. But it seems to indicate a bigger problem.
If we are trying to pass in a negative value, which is not one
of enum values, using BlkDebugEvent as type is just confusing,
we should just pass int instead.
AFAICT it's only possible to use the values listed in event_names in
blkdebug.c, other values are rejected. So the check should actually be
an assert() or it could even be removed.
Sounds good.
How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the
check? Then if the value changes, the need to add the comparison back
will be obvious.
This would work but it's weird. The thing is it's currently a correct
code and the check may be useless but it's the optimiser's task to
remove it, not ours. The compiler is not able to tell whether the
check makes sense or nott, because the compiler only has access to
preprocessed code. So why should you let the compiler have anything
to say on it.
Good point. I'll try to invent something better.
Use #pragma to supress the warning? Maybe we could wrap this in a macro ..
Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.
I think the assertion is still the best way, it ensures that something
will happen if OMAP_EMIFS_BASE changes. We could for example remove
OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
adding a new define could still forget to adjust the check anyway.
We could replace it with a macro
#define OMAP_EMIFS_VALID(addr) ((target_phys_addr_t)addr OMAP_EMIFF_BASE)
but all this does look artificial. And of course using type casts
is always scary ...
Would it help to have some inline functions that do the range checking
correctly?
We have a couple of range helpers in pci.h, these could be moved out
to range.h and we could add some more. As there act on u64 this will get
the type limits mostly automatically right.
That seems to be the best solution, I get no warnings with this:
diff --git a/hw/omap1.c b/hw/omap1.c
index b00f870..8bf88e7 100644
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -3672,14 +3672,25 @@ static int omap_validate_emiff_addr(struct
omap_mpu_state_s *s,
return addr = OMAP_EMIFF_BASE addr OMAP_EMIFF_BASE + s-sdram_size;
}
+/* Get last byte of a range from offset + length.
+ * Undefined for ranges that wrap around 0. */
+static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
+{
+return offset + len - 1;
+}
+
+/* Check whether a given range covers a given byte. */
+static inline int range_covers_byte(uint64_t offset, uint64_t len,
+uint64_t byte)
+{
+return offset = byte byte = range_get_last(offset, len);
+}
+
static int omap_validate_emifs_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-/* If OMAP_EMIFS_BASE ever becomes nonzero, adjust the check below
- to also include the lower bound check like
- addr = OMAP_EMIFS_BASE addr OMAP_EMIFF_BASE */
-assert(OMAP_EMIFS_BASE == 0);
-return addr OMAP_EMIFF_BASE;
+return range_covers_byte(OMAP_EMIFS_BASE,
+ OMAP_EMIFF_BASE - OMAP_EMIFS_BASE, addr);
}
static int omap_validate_imif_addr(struct omap_mpu_state_s *s,
I'll add range.h and respin the patches.
Re: [Qemu-devel] Unmaintained QEMU builds
On 09/04/2010 04:56 PM, Andreas Färber wrote: Maybe it's time to rethink the relation between QEMU and its frontends / management tools? If we want to compete with the commercial products (sic), we might agree on some official frontend per GUI-centric platform, with a Git-based repository (like qemu-kvm.git) and synchronized releases that may call themselves QEMU, linked from qemu.org, rather than having a variety of (outdated) Q* frontends per platform of which most are nothing more than a configuration window to spawn the regular qemu[-system-x86_64]. There is also virt-manager which is quite rich at this time. Currently what QEMU can point with is richer machine and hardware emulation and its license; if we want more users than that, we'll need to deliver what users usually want the most - stability, performance and ease of use... and good marketing. They may as well be merged into qemu.git directly, so long as: - the GUI has its own maintainer - the prepackaged GUI doesn't get access to internal APIs, compared to external tools -- error compiling committee.c: too many arguments to function
[Qemu-devel] Re: [PATCH 12/15] piix_pci: introduce a write_config notifier
On Fri, Aug 13, 2010 at 02:10:01PM +0100, Stefano Stabellini wrote: On Thu, 12 Aug 2010, Blue Swirl wrote: On Thu, Aug 12, 2010 at 2:09 PM, stefano.stabell...@eu.citrix.com wrote: From: Anthony PERARD anthony.per...@citrix.com Introduce a write config notifier in piix_pci, so that clients can be notified every time a pci config write happens. The patch also makes use of the notification mechanism in xen_machine_fv. Will the mechanism be used elsewhere? If not, I'd just add a call to xen_piix_pci_write_config_client() to piix_pci.c. It can be surrounded by Xen #ifdeffery, or you could introduce stubs like kvm-stub.c and friends. we were trying to avoid ifdef's in piix_pci, but if you are OK with just a couple of them we'll gladly remove the hook. I second this. Callbacks complicate code significantly. If there's a single user we are better off without. -- MST
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin m...@redhat.com wrote:
On Sat, Sep 04, 2010 at 05:21:24PM +, Blue Swirl wrote:
In the unsigned number space, the checks can be merged into one,
assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
could have:
- if (event 0 || event = BLKDBG_EVENT_MAX) {
+ if ((int)event 0 || event = BLKDBG_EVENT_MAX) {
This would also implement the check that the writer of this code was
trying to make.
The important thing to note is however is that the check as it is now
is not correct.
I agree. But it seems to indicate a bigger problem.
If we are trying to pass in a negative value, which is not one
of enum values, using BlkDebugEvent as type is just confusing,
we should just pass int instead.
AFAICT it's only possible to use the values listed in event_names in
blkdebug.c, other values are rejected. So the check should actually be
an assert() or it could even be removed.
How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the
check? Then if the value changes, the need to add the comparison back
will be obvious.
This would work but it's weird. The thing is it's currently a correct
code and the check may be useless but it's the optimiser's task to
remove it, not ours. The compiler is not able to tell whether the
check makes sense or nott, because the compiler only has access to
preprocessed code. So why should you let the compiler have anything
to say on it.
Good point. I'll try to invent something better.
Use #pragma to supress the warning? Maybe we could wrap this in a macro ..
Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.
I think the assertion is still the best way, it ensures that something
will happen if OMAP_EMIFS_BASE changes. We could for example remove
OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
adding a new define could still forget to adjust the check anyway.
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On Sat, Sep 04, 2010 at 05:21:24PM +, Blue Swirl wrote:
In the unsigned number space, the checks can be merged into one,
assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
could have:
-if (event 0 || event = BLKDBG_EVENT_MAX) {
+if ((int)event 0 || event = BLKDBG_EVENT_MAX) {
This would also implement the check that the writer of this code was
trying to make.
The important thing to note is however is that the check as it is now
is not correct.
I agree. But it seems to indicate a bigger problem.
If we are trying to pass in a negative value, which is not one
of enum values, using BlkDebugEvent as type is just confusing,
we should just pass int instead.
How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the
check? Then if the value changes, the need to add the comparison back
will be obvious.
This would work but it's weird. The thing is it's currently a correct
code and the check may be useless but it's the optimiser's task to
remove it, not ours. The compiler is not able to tell whether the
check makes sense or nott, because the compiler only has access to
preprocessed code. So why should you let the compiler have anything
to say on it.
Good point. I'll try to invent something better.
Use #pragma to supress the warning? Maybe we could wrap this in a macro ..
--
MST
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On Sun, Sep 05, 2010 at 09:44:01AM +, Blue Swirl wrote:
On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin m...@redhat.com wrote:
On Sun, Sep 05, 2010 at 09:06:10AM +, Blue Swirl wrote:
On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin m...@redhat.com wrote:
On Sat, Sep 04, 2010 at 05:21:24PM +, Blue Swirl wrote:
In the unsigned number space, the checks can be merged into one,
assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
could have:
- if (event 0 || event = BLKDBG_EVENT_MAX) {
+ if ((int)event 0 || event = BLKDBG_EVENT_MAX) {
This would also implement the check that the writer of this code was
trying to make.
The important thing to note is however is that the check as it is now
is not correct.
I agree. But it seems to indicate a bigger problem.
If we are trying to pass in a negative value, which is not one
of enum values, using BlkDebugEvent as type is just confusing,
we should just pass int instead.
AFAICT it's only possible to use the values listed in event_names in
blkdebug.c, other values are rejected. So the check should actually be
an assert() or it could even be removed.
Sounds good.
How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the
check? Then if the value changes, the need to add the comparison back
will be obvious.
This would work but it's weird. The thing is it's currently a correct
code and the check may be useless but it's the optimiser's task to
remove it, not ours. The compiler is not able to tell whether the
check makes sense or nott, because the compiler only has access to
preprocessed code. So why should you let the compiler have anything
to say on it.
Good point. I'll try to invent something better.
Use #pragma to supress the warning? Maybe we could wrap this in a macro
..
Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.
I think the assertion is still the best way, it ensures that something
will happen if OMAP_EMIFS_BASE changes. We could for example remove
OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
adding a new define could still forget to adjust the check anyway.
We could replace it with a macro
#define OMAP_EMIFS_VALID(addr) ((target_phys_addr_t)addr OMAP_EMIFF_BASE)
but all this does look artificial. And of course using type casts
is always scary ...
Would it help to have some inline functions that do the range checking
correctly?
We have a couple of range helpers in pci.h, these could be moved out
to range.h and we could add some more. As there act on u64 this will get
the type limits mostly automatically right.
That seems to be the best solution, I get no warnings with this:
diff --git a/hw/omap1.c b/hw/omap1.c
index b00f870..8bf88e7 100644
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -3672,14 +3672,25 @@ static int omap_validate_emiff_addr(struct
omap_mpu_state_s *s,
return addr = OMAP_EMIFF_BASE addr OMAP_EMIFF_BASE + s-sdram_size;
}
+/* Get last byte of a range from offset + length.
+ * Undefined for ranges that wrap around 0. */
+static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
+{
+return offset + len - 1;
+}
+
+/* Check whether a given range covers a given byte. */
+static inline int range_covers_byte(uint64_t offset, uint64_t len,
+uint64_t byte)
+{
+return offset = byte byte = range_get_last(offset, len);
+}
+
static int omap_validate_emifs_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-/* If OMAP_EMIFS_BASE ever becomes nonzero, adjust the check below
- to also include the lower bound check like
- addr = OMAP_EMIFS_BASE addr OMAP_EMIFF_BASE */
-assert(OMAP_EMIFS_BASE == 0);
-return addr OMAP_EMIFF_BASE;
+return range_covers_byte(OMAP_EMIFS_BASE,
+ OMAP_EMIFF_BASE - OMAP_EMIFS_BASE, addr);
}
static int omap_validate_imif_addr(struct omap_mpu_state_s *s,
I'll add range.h and respin the patches.
BTW, maybe we want a variant of range_covers_byte that gets
first and after last byte values, but so far we could not
come up with good name for that function, and 1 after last
semantics might be confusing.
One small comment: these are currenly wrong if the range
wraps around to 0 - and there's a comment that says so there.
This was never a problem for pci, but it might be
if we are making them generic.
--
MST
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On Sun, Sep 05, 2010 at 09:06:10AM +, Blue Swirl wrote:
On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin m...@redhat.com wrote:
On Sat, Sep 04, 2010 at 05:21:24PM +, Blue Swirl wrote:
In the unsigned number space, the checks can be merged into one,
assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
could have:
- if (event 0 || event = BLKDBG_EVENT_MAX) {
+ if ((int)event 0 || event = BLKDBG_EVENT_MAX) {
This would also implement the check that the writer of this code was
trying to make.
The important thing to note is however is that the check as it is now
is not correct.
I agree. But it seems to indicate a bigger problem.
If we are trying to pass in a negative value, which is not one
of enum values, using BlkDebugEvent as type is just confusing,
we should just pass int instead.
AFAICT it's only possible to use the values listed in event_names in
blkdebug.c, other values are rejected. So the check should actually be
an assert() or it could even be removed.
Sounds good.
How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the
check? Then if the value changes, the need to add the comparison back
will be obvious.
This would work but it's weird. The thing is it's currently a correct
code and the check may be useless but it's the optimiser's task to
remove it, not ours. The compiler is not able to tell whether the
check makes sense or nott, because the compiler only has access to
preprocessed code. So why should you let the compiler have anything
to say on it.
Good point. I'll try to invent something better.
Use #pragma to supress the warning? Maybe we could wrap this in a macro ..
Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.
I think the assertion is still the best way, it ensures that something
will happen if OMAP_EMIFS_BASE changes. We could for example remove
OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
adding a new define could still forget to adjust the check anyway.
We could replace it with a macro
#define OMAP_EMIFS_VALID(addr) ((target_phys_addr_t)addr OMAP_EMIFF_BASE)
but all this does look artificial. And of course using type casts
is always scary ...
Would it help to have some inline functions that do the range checking
correctly?
We have a couple of range helpers in pci.h, these could be moved out
to range.h and we could add some more. As there act on u64 this will get
the type limits mostly automatically right.
--
MST
[Qemu-devel] Re: [PATCH] ivshmem: remove unnecessary checks for unsigned
On 09/03/2010 05:54 AM, Hidetoshi Seto wrote: Oops, since I've registered qemu-kvm ML but not qemu-devel ML, I could not noticed that Jes have already posted same patch to qemu-devel. Now build of ivshmem is enabled only on KVM systems, please apply this patch to qemu-kvm.git asap. This was already applied to qemu.git; I'll pull it into qemu-kvm.git to fix the build. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Unmaintained QEMU builds
On Sun, Sep 5, 2010 at 2:17 PM, Avi Kivity a...@redhat.com wrote: On 09/05/2010 05:10 PM, Blue Swirl wrote: Easy to use GUI and integration to host system are important, but performance is also a big problem. QEMU/TCG can't compete with alternatives that use proprietary kernel modules. Someone should recreate kqemu by using KVM compatible interfaces. If someone is really willing to invest the effort to do that cleanly, I am willing to merge it into kvm. That would allow reuse of the mmu and some other logic that got a lot of effort in kvm. However, I doubt it is worth the effort, if anyone is interested in performance then they'd get a cpu that supports virtualization. That leaves non-Linux. Can qemu really compete there for x86-on-x86? I doubt it. Someone could also make a KVM compatible module for non-Linux hosts with virtualization capable CPUs. Wouldn't that solve most performance problems?
Re: [Qemu-devel] Unmaintained QEMU builds
Am 05.09.2010 um 13:19 schrieb Avi Kivity: On 09/04/2010 04:56 PM, Andreas Färber wrote: Maybe it's time to rethink the relation between QEMU and its frontends / management tools? If we want to compete with the commercial products (sic), we might agree on some official frontend per GUI-centric platform, with a Git-based repository (like qemu-kvm.git) and synchronized releases that may call themselves QEMU, linked from qemu.org, rather than having a variety of (outdated) Q* frontends per platform of which most are nothing more than a configuration window to spawn the regular qemu[- system-x86_64]. There is also virt-manager which is quite rich at this time. Seems I didn't get the point across too well: Standard users on Windows-PC and Mac expect a solution to their needs, not a forest of well-designed libraries and tools with .tgz downloads. QEMU has no such product identity, and there's no prominent binary download link for Win/Mac on the qemu.org frontpage. virt-manager is neither prominently advertised there either, nor does it have a Windows download. (Fwiw while it's certainly nice on Linux and to some limited degree on Solaris (ancient fork apparently), I wouldn't exactly describe the virt-install versions I've seen as rich... and setting up the VM is somewhat a prerequisite to using virt-manager's indeed nice features. Fedora's default security policies on top don't exactly make it easy to try out .isos or downloaded disk images with virt-manager, its German translation had a severe contentual error in the VM's menu and a felt 80% of the BRC bug reports get ignored and closed by a bot anyway, but that's another topic.) But sure, on Linux there's a plethora of simplistic Q* frontends, too. (n.b., virt-manager didn't match that regex ;) Choice and diversity isn't wrong per se, just the comparison of the available options on the two given platforms has shown not to make QEMU a common choice. Whining about lack of bugfix contributions is unlikely going to change that imo. As a baby step, is there any chance of publishing an automatic nightly Windows (cross-)build as a .zip file on qemu.org? That might give more users a chance of detecting runtime faults during the development cycle. Andreas Currently what QEMU can point with is richer machine and hardware emulation and its license; if we want more users than that, we'll need to deliver what users usually want the most - stability, performance and ease of use... and good marketing. They may as well be merged into qemu.git directly, so long as: - the GUI has its own maintainer - the prepackaged GUI doesn't get access to internal APIs, compared to external tools -- error compiling committee.c: too many arguments to function
[Qemu-devel] [PATCH 00/15] GCC warning flag update
In this version, I split some of the patches into more logical pieces. 5/15 adds range.h which changed a few patches. Blue Swirl (15): Check for errors during BIOS or kernel load linux-user: fix socklen_t comparisons linux-user: fix types in a comparison linux-user: improve flatload error checking Introduce range.h Use range_covers_byte pxa2xx: remove useless checks blkdebug: fix enum comparison PPC: Suppress gcc warnings with -Wtype-limits MIPS: fix yield handling pxa2xx: fix SSSR TFN logic Use gcc warning flag -Wtype-limits Use a few more gcc warning flags Use gcc warning flag -Wempty-body, fix warnings Use gcc warning flag -Wnested-externs, fix warnings block/blkdebug.c|4 +-- configure |5 +++- feature_to_c.sh |1 - gdbstub.c |1 - gdbstub.h |3 ++ hw/acpi_piix4.c |1 + hw/mips_fulong2e.c |2 +- hw/msix.c |1 + hw/omap1.c | 21 ++- hw/omap_i2c.c |5 ++- hw/omap_mmc.c |5 ++- hw/pci.c|1 + hw/pci.h| 29 --- hw/piix_pci.c |1 + hw/ppc405_boards.c | 23 - hw/ppc_newworld.c |3 +- hw/ppc_prep.c |3 +- hw/pxa2xx.c | 15 +++-- hw/sm501.c |5 ++- hw/soc_dma.c|5 ++- hw/vhost.c |3 +- linux-user/flatload.c |3 +- linux-user/mmap.c |2 +- linux-user/syscall.c| 20 -- range.h | 29 +++ target-cris/mmu.c |2 +- target-mips/op_helper.c |4 ++- target-ppc/op_helper.c | 50 +++--- 28 files changed, 140 insertions(+), 107 deletions(-) create mode 100644 range.h
[Qemu-devel] [PATCH 02/15] linux-user: fix socklen_t comparisons
On many systems, socklen_t is defined as unsigned. This means that
checks for negative values are not meaningful.
Fix by explicitly casting to a signed integer.
This also fixes some warnings with GCC flag -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
linux-user/syscall.c | 20 +---
1 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0ebe7e1..d44f512 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1551,8 +1551,9 @@ static abi_long do_bind(int sockfd, abi_ulong target_addr,
void *addr;
abi_long ret;
-if (addrlen 0)
+if ((int)addrlen 0) {
return -TARGET_EINVAL;
+}
addr = alloca(addrlen+1);
@@ -1570,8 +1571,9 @@ static abi_long do_connect(int sockfd, abi_ulong
target_addr,
void *addr;
abi_long ret;
-if (addrlen 0)
+if ((int)addrlen 0) {
return -TARGET_EINVAL;
+}
addr = alloca(addrlen);
@@ -1656,8 +1658,9 @@ static abi_long do_accept(int fd, abi_ulong target_addr,
if (get_user_u32(addrlen, target_addrlen_addr))
return -TARGET_EINVAL;
-if (addrlen 0)
+if ((int)addrlen 0) {
return -TARGET_EINVAL;
+}
if (!access_ok(VERIFY_WRITE, target_addr, addrlen))
return -TARGET_EINVAL;
@@ -1684,8 +1687,9 @@ static abi_long do_getpeername(int fd, abi_ulong
target_addr,
if (get_user_u32(addrlen, target_addrlen_addr))
return -TARGET_EFAULT;
-if (addrlen 0)
+if ((int)addrlen 0) {
return -TARGET_EINVAL;
+}
if (!access_ok(VERIFY_WRITE, target_addr, addrlen))
return -TARGET_EFAULT;
@@ -1712,8 +1716,9 @@ static abi_long do_getsockname(int fd, abi_ulong
target_addr,
if (get_user_u32(addrlen, target_addrlen_addr))
return -TARGET_EFAULT;
-if (addrlen 0)
+if ((int)addrlen 0) {
return -TARGET_EINVAL;
+}
if (!access_ok(VERIFY_WRITE, target_addr, addrlen))
return -TARGET_EFAULT;
@@ -1753,8 +1758,9 @@ static abi_long do_sendto(int fd, abi_ulong msg,
size_t len, int flags,
void *host_msg;
abi_long ret;
-if (addrlen 0)
+if ((int)addrlen 0) {
return -TARGET_EINVAL;
+}
host_msg = lock_user(VERIFY_READ, msg, len, 1);
if (!host_msg)
@@ -1792,7 +1798,7 @@ static abi_long do_recvfrom(int fd, abi_ulong
msg, size_t len, int flags,
ret = -TARGET_EFAULT;
goto fail;
}
-if (addrlen 0) {
+if ((int)addrlen 0) {
ret = -TARGET_EINVAL;
goto fail;
}
--
1.6.2.4
[Qemu-devel] [PATCH 01/15] Check for errors during BIOS or kernel load
Because of the use of unsigned types, possible errors during
BIOS or kernel load were ignored.
Fix by using a signed type.
This also fixes some warnings with GCC flag -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
hw/mips_fulong2e.c |2 +-
hw/ppc405_boards.c | 23 +--
hw/ppc_newworld.c |3 ++-
hw/ppc_prep.c |3 ++-
4 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c
index cbe7156..ac82067 100644
--- a/hw/mips_fulong2e.c
+++ b/hw/mips_fulong2e.c
@@ -258,7 +258,7 @@ static void mips_fulong2e_init(ram_addr_t
ram_size, const char *boot_device,
{
char *filename;
unsigned long ram_offset, bios_offset;
-unsigned long bios_size;
+long bios_size;
int64_t kernel_entry;
qemu_irq *i8259;
qemu_irq *cpu_exit_irq;
diff --git a/hw/ppc405_boards.c b/hw/ppc405_boards.c
index 662d7c4..db8e5ec 100644
--- a/hw/ppc405_boards.c
+++ b/hw/ppc405_boards.c
@@ -182,10 +182,12 @@ static void ref405ep_init (ram_addr_t ram_size,
qemu_irq *pic;
ram_addr_t sram_offset, bios_offset, bdloc;
target_phys_addr_t ram_bases[2], ram_sizes[2];
-target_ulong sram_size, bios_size;
+target_ulong sram_size;
+long bios_size;
//int phy_addr = 0;
//static int phy_addr = 1;
-target_ulong kernel_base, kernel_size, initrd_base, initrd_size;
+target_ulong kernel_base, initrd_base;
+long kernel_size, initrd_size;
int linux_boot;
int fl_idx, fl_sectors, len;
DriveInfo *dinfo;
@@ -221,8 +223,8 @@ static void ref405ep_init (ram_addr_t ram_size,
bios_offset = qemu_ram_alloc(NULL, ef405ep.bios, bios_size);
fl_sectors = (bios_size + 65535) 16;
#ifdef DEBUG_BOARD_INIT
-printf(Register parallel flash %d size TARGET_FMT_lx
-at offset %08lx addr TARGET_FMT_lx '%s' %d\n,
+printf(Register parallel flash %d size %lx
+at offset %08lx addr %lx '%s' %d\n,
fl_idx, bios_size, bios_offset, -bios_size,
bdrv_get_device_name(dinfo-bdrv), fl_sectors);
#endif
@@ -308,7 +310,7 @@ static void ref405ep_init (ram_addr_t ram_size,
kernel_filename);
exit(1);
}
-printf(Load kernel size TARGET_FMT_ld at TARGET_FMT_lx,
+printf(Load kernel size %ld at TARGET_FMT_lx,
kernel_size, kernel_base);
/* load initrd */
if (initrd_filename) {
@@ -503,8 +505,9 @@ static void taihu_405ep_init(ram_addr_t ram_size,
qemu_irq *pic;
ram_addr_t bios_offset;
target_phys_addr_t ram_bases[2], ram_sizes[2];
-target_ulong bios_size;
-target_ulong kernel_base, kernel_size, initrd_base, initrd_size;
+long bios_size;
+target_ulong kernel_base, initrd_base;
+long kernel_size, initrd_size;
int linux_boot;
int fl_idx, fl_sectors;
DriveInfo *dinfo;
@@ -534,8 +537,8 @@ static void taihu_405ep_init(ram_addr_t ram_size,
fl_sectors = (bios_size + 65535) 16;
bios_offset = qemu_ram_alloc(NULL, taihu_405ep.bios, bios_size);
#ifdef DEBUG_BOARD_INIT
-printf(Register parallel flash %d size TARGET_FMT_lx
-at offset %08lx addr TARGET_FMT_lx '%s' %d\n,
+printf(Register parallel flash %d size %lx
+at offset %08lx addr %lx '%s' %d\n,
fl_idx, bios_size, bios_offset, -bios_size,
bdrv_get_device_name(dinfo-bdrv), fl_sectors);
#endif
@@ -576,7 +579,7 @@ static void taihu_405ep_init(ram_addr_t ram_size,
bios_size = 32 * 1024 * 1024;
fl_sectors = (bios_size + 65535) 16;
#ifdef DEBUG_BOARD_INIT
-printf(Register parallel flash %d size TARGET_FMT_lx
+printf(Register parallel flash %d size %lx
at offset %08lx addr TARGET_FMT_lx '%s'\n,
fl_idx, bios_size, bios_offset, (target_ulong)0xfc00,
bdrv_get_device_name(dinfo-bdrv));
diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
index 809a1cf..fb07c83 100644
--- a/hw/ppc_newworld.c
+++ b/hw/ppc_newworld.c
@@ -135,7 +135,8 @@ static void ppc_core99_init (ram_addr_t ram_size,
int unin_memory;
int linux_boot, i;
ram_addr_t ram_offset, bios_offset, vga_bios_offset;
-uint32_t kernel_base, kernel_size, initrd_base, initrd_size;
+uint32_t kernel_base, initrd_base;
+long kernel_size, initrd_size;
PCIBus *pci_bus;
MacIONVRAMState *nvr;
int nvram_mem_index;
diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
index 52fa9b6..0e5b88c 100644
--- a/hw/ppc_prep.c
+++ b/hw/ppc_prep.c
@@ -572,7 +572,8 @@ static void ppc_prep_init (ram_addr_t ram_size,
int PPC_io_memory;
int linux_boot, i, nb_nics1, bios_size;
ram_addr_t ram_offset, bios_offset;
-uint32_t kernel_base, kernel_size, initrd_base, initrd_size;
+uint32_t kernel_base, initrd_base;
+long kernel_size, initrd_size;
PCIBus *pci_bus;
qemu_irq
[Qemu-devel] [PATCH 10/15] MIPS: fix yield handling
The parameter for yield should be handled as a signed integer
for the comparisons to have any effect.
This also fixes a gcc warning with -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
target-mips/op_helper.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index 50c65bd..41abd57 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -1598,8 +1598,10 @@ void helper_fork(target_ulong arg1, target_ulong arg2)
// TODO: store to TC register
}
-target_ulong helper_yield(target_ulong arg1)
+target_ulong helper_yield(target_ulong arg)
{
+target_long arg1 = arg;
+
if (arg1 0) {
/* No scheduling policy implemented. */
if (arg1 != -2) {
--
1.6.2.4
[Qemu-devel] [PATCH 03/15] linux-user: fix types in a comparison
-1ul is unsigned long, which does not necessarily match abi_ulong
type.
Fix by using abi_long instead.
This also fixes a warning with GCC flag -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
linux-user/mmap.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index e10a6ef..035dfbd 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -342,7 +342,7 @@ abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size)
munmap(ptr, size);
/* ENOMEM if we checked the whole of the target address space. */
-if (addr == -1ul) {
+if (addr == (abi_ulong)-1) {
return (abi_ulong)-1;
} else if (addr == 0) {
if (wrapped) {
--
1.6.2.4
[Qemu-devel] [PATCH 04/15] linux-user: improve flatload error checking
Because of the use of unsigned type, possible errors during
load were ignored.
Fix by using a signed type.
This also fixes a warning with GCC flag -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
linux-user/flatload.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index 8ad130a..8f9f4a5 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -383,7 +383,8 @@ static int load_flat_file(struct linux_binprm * bprm,
struct lib_info *libinfo, int id, abi_ulong *extra_stack)
{
struct flat_hdr * hdr;
-abi_ulong textpos = 0, datapos = 0, result;
+abi_ulong textpos = 0, datapos = 0;
+abi_long result;
abi_ulong realdatastart = 0;
abi_ulong text_len, data_len, bss_len, stack_len, flags;
abi_ulong memp = 0; /* for finding the brk area */
--
1.6.2.4
[Qemu-devel] [PATCH 05/15] Introduce range.h
Extract range functions from pci.h. These will be used by later patches
by non-PCI devices. Adjust current users.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
hw/acpi_piix4.c |1 +
hw/msix.c |1 +
hw/pci.c|1 +
hw/pci.h| 29 -
hw/piix_pci.c |1 +
hw/vhost.c |3 +--
range.h | 29 +
7 files changed, 34 insertions(+), 31 deletions(-)
create mode 100644 range.h
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index bfa1d9a..c8733e5 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -22,6 +22,7 @@
#include pci.h
#include acpi.h
#include sysemu.h
+#include range.h
//#define DEBUG
diff --git a/hw/msix.c b/hw/msix.c
index d99403a..b3bb92d 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -14,6 +14,7 @@
#include hw.h
#include msix.h
#include pci.h
+#include range.h
/* MSI-X capability structure */
#define MSIX_TABLE_OFFSET 4
diff --git a/hw/pci.c b/hw/pci.c
index a98d6f3..6d0934d 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -28,6 +28,7 @@
#include sysemu.h
#include loader.h
#include qemu-objects.h
+#include range.h
//#define DEBUG_PCI
#ifdef DEBUG_PCI
diff --git a/hw/pci.h b/hw/pci.h
index 1eab7e7..3d23f03 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -365,33 +365,4 @@ static inline uint32_t pci_config_size(const PCIDevice *d)
return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
}
-/* These are not pci specific. Should move into a separate header.
- * Only pci.c uses them, so keep them here for now.
- */
-
-/* Get last byte of a range from offset + length.
- * Undefined for ranges that wrap around 0. */
-static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
-{
-return offset + len - 1;
-}
-
-/* Check whether a given range covers a given byte. */
-static inline int range_covers_byte(uint64_t offset, uint64_t len,
-uint64_t byte)
-{
-return offset = byte byte = range_get_last(offset, len);
-}
-
-/* Check whether 2 given ranges overlap.
- * Undefined if ranges that wrap around 0. */
-static inline int ranges_overlap(uint64_t first1, uint64_t len1,
- uint64_t first2, uint64_t len2)
-{
-uint64_t last1 = range_get_last(first1, len1);
-uint64_t last2 = range_get_last(first2, len2);
-
-return !(last2 first1 || last1 first2);
-}
-
#endif
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index f152a0f..b5589b9 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -28,6 +28,7 @@
#include pci_host.h
#include isa.h
#include sysbus.h
+#include range.h
/*
* I440FX chipset data sheet.
diff --git a/hw/vhost.c b/hw/vhost.c
index 34c4745..1b8624d 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -13,8 +13,7 @@
#include sys/ioctl.h
#include vhost.h
#include hw/hw.h
-/* For range_get_last */
-#include pci.h
+#include range.h
#include linux/vhost.h
static void vhost_dev_sync_region(struct vhost_dev *dev,
diff --git a/range.h b/range.h
new file mode 100644
index 000..3502372
--- /dev/null
+++ b/range.h
@@ -0,0 +1,29 @@
+#ifndef QEMU_RANGE_H
+#define QEMU_RANGE_H
+
+/* Get last byte of a range from offset + length.
+ * Undefined for ranges that wrap around 0. */
+static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
+{
+return offset + len - 1;
+}
+
+/* Check whether a given range covers a given byte. */
+static inline int range_covers_byte(uint64_t offset, uint64_t len,
+uint64_t byte)
+{
+return offset = byte byte = range_get_last(offset, len);
+}
+
+/* Check whether 2 given ranges overlap.
+ * Undefined if ranges that wrap around 0. */
+static inline int ranges_overlap(uint64_t first1, uint64_t len1,
+ uint64_t first2, uint64_t len2)
+{
+uint64_t last1 = range_get_last(first1, len1);
+uint64_t last2 = range_get_last(first2, len2);
+
+return !(last2 first1 || last1 first2);
+}
+
+#endif
--
1.6.2.4
[Qemu-devel] [PATCH 11/15] pxa2xx: fix SSSR TFN logic
Fix SSSR TFN logic: TX FIFO is never filled, so it is always in
underrun condition if SSP is enabled.
This also fixes a gcc warning with -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
hw/pxa2xx.c |8
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index faa3d95..88f61c0 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -636,6 +636,7 @@ static void pxa2xx_ssp_fifo_update(PXA2xxSSPState *s)
{
s-sssr = ~(0xf 12); /* Clear RFL */
s-sssr = ~(0xf 8);/* Clear TFL */
+s-sssr = ~SSSR_TFS;
s-sssr = ~SSSR_TNF;
if (s-enable) {
s-sssr |= ((s-rx_level - 1) 0xf) 12;
@@ -643,14 +644,13 @@ static void pxa2xx_ssp_fifo_update(PXA2xxSSPState *s)
s-sssr |= SSSR_RFS;
else
s-sssr = ~SSSR_RFS;
-if (0 = SSCR1_TFT(s-sscr[1]))
-s-sssr |= SSSR_TFS;
-else
-s-sssr = ~SSSR_TFS;
if (s-rx_level)
s-sssr |= SSSR_RNE;
else
s-sssr = ~SSSR_RNE;
+/* TX FIFO is never filled, so it is always in underrun
+ condition if SSP is enabled */
+s-sssr |= SSSR_TFS;
s-sssr |= SSSR_TNF;
}
--
1.6.2.4
[Qemu-devel] [PATCH 13/15] Use a few more gcc warning flags
If the compiler supports the following warning flags, use them:
-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
-Wmissing-include-dirs -Wclobbered
Currently, these flags don't produce any warnings.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
configure |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/configure b/configure
index 47bbe39..6e4917a 100755
--- a/configure
+++ b/configure
@@ -138,7 +138,10 @@ QEMU_CFLAGS=-D_FORTIFY_SOURCE=2 $QEMU_CFLAGS
QEMU_CFLAGS=-I. -I\$(SRC_PATH) $QEMU_CFLAGS
LDFLAGS=-g $LDFLAGS
-gcc_flags=-Wold-style-declaration -Wold-style-definition
-Wtype-limits -fstack-protector-all
+gcc_flags=-Wold-style-declaration -Wold-style-definition -Wtype-limits
+gcc_flags=-Wformat-security -Wformat-y2k -Winit-self
-Wignored-qualifiers $gcc_flags
+gcc_flags=-Wmissing-include-dirs -Wclobbered $gcc_flags
+gcc_flags=-fstack-protector-all $gcc_flags
cat $TMPC EOF
int main(void) { return 0; }
EOF
--
1.6.2.4
[Qemu-devel] [PATCH 06/15] Use range_covers_byte
Use range_covers_byte() instead of comparisons.
This also fixes some warnings with GCC flag -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
hw/omap1.c | 21 +++--
hw/sm501.c |5 +++--
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/hw/omap1.c b/hw/omap1.c
index 06370b6..2dd62ec 100644
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -26,6 +26,7 @@
/* We use pc-style serial ports. */
#include pc.h
#include blockdev.h
+#include range.h
/* Should signal the TCMI/GPMC */
uint32_t omap_badwidth_read8(void *opaque, target_phys_addr_t addr)
@@ -3669,37 +3670,45 @@ static const struct dma_irq_map omap1_dma_irq_map[] = {
static int omap_validate_emiff_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-return addr = OMAP_EMIFF_BASE addr OMAP_EMIFF_BASE + s-sdram_size;
+return range_covers_byte(OMAP_EMIFF_BASE,
+ OMAP_EMIFF_BASE + s-sdram_size - OMAP_EMIFF_BASE,
+ addr);
}
static int omap_validate_emifs_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-return addr = OMAP_EMIFS_BASE addr OMAP_EMIFF_BASE;
+return range_covers_byte(OMAP_EMIFS_BASE, OMAP_EMIFF_BASE -
OMAP_EMIFS_BASE,
+ addr);
}
static int omap_validate_imif_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-return addr = OMAP_IMIF_BASE addr OMAP_IMIF_BASE + s-sram_size;
+return range_covers_byte(OMAP_IMIF_BASE,
+ OMAP_IMIF_BASE + s-sram_size - OMAP_IMIF_BASE,
+ addr);
}
static int omap_validate_tipb_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-return addr = 0xfffb addr 0x;
+return range_covers_byte(0xfffb, 0x - 0xfffb, addr);
}
static int omap_validate_local_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-return addr = OMAP_LOCALBUS_BASE addr OMAP_LOCALBUS_BASE + 0x100;
+return range_covers_byte(OMAP_LOCALBUS_BASE,
+ OMAP_LOCALBUS_BASE + 0x100 -
+ OMAP_LOCALBUS_BASE,
+ addr);
}
static int omap_validate_tipb_mpui_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-return addr = 0xe101 addr 0xe1020004;
+return range_covers_byte(0xe101, 0xe1020004 - 0xe101, addr);
}
struct omap_mpu_state_s *omap310_mpu_init(unsigned long sdram_size,
diff --git a/hw/sm501.c b/hw/sm501.c
index 8e6932d..705e0a5 100644
--- a/hw/sm501.c
+++ b/hw/sm501.c
@@ -29,6 +29,7 @@
#include devices.h
#include sysbus.h
#include qdev-addr.h
+#include range.h
/*
* Status: 2010/05/07
@@ -814,7 +815,7 @@ static uint32_t sm501_palette_read(void *opaque,
target_phys_addr_t addr)
/* TODO : consider BYTE/WORD access */
/* TODO : consider endian */
-assert(0 = addr addr 0x400 * 3);
+assert(range_covers_byte(0, 0x400 * 3, addr));
return *(uint32_t*)s-dc_palette[addr];
}
@@ -828,7 +829,7 @@ static void sm501_palette_write(void *opaque,
/* TODO : consider BYTE/WORD access */
/* TODO : consider endian */
-assert(0 = addr addr 0x400 * 3);
+assert(range_covers_byte(0, 0x400 * 3, addr));
*(uint32_t*)s-dc_palette[addr] = value;
}
--
1.6.2.4
[Qemu-devel] [PATCH 15/15] Use gcc warning flag -Wnested-externs, fix warnings
If the compiler supports the warning flag -Wnested-externs, use it.
Fix the only warning by moving the xml_builtin declaration to a more
proper place.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
configure |2 +-
feature_to_c.sh |1 -
gdbstub.c |1 -
gdbstub.h |3 +++
4 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/configure b/configure
index 61626b8..78829e9 100755
--- a/configure
+++ b/configure
@@ -140,7 +140,7 @@ LDFLAGS=-g $LDFLAGS
gcc_flags=-Wold-style-declaration -Wold-style-definition -Wtype-limits
gcc_flags=-Wformat-security -Wformat-y2k -Winit-self
-Wignored-qualifiers $gcc_flags
-gcc_flags=-Wmissing-include-dirs -Wclobbered -Wempty-body $gcc_flags
+gcc_flags=-Wmissing-include-dirs -Wclobbered -Wempty-body
-Wnested-externs $gcc_flags
gcc_flags=-fstack-protector-all $gcc_flags
cat $TMPC EOF
int main(void) { return 0; }
diff --git a/feature_to_c.sh b/feature_to_c.sh
index dbf9f19..0994d95 100644
--- a/feature_to_c.sh
+++ b/feature_to_c.sh
@@ -63,7 +63,6 @@ for input; do
done
echo $output
-echo extern const char *const xml_builtin[][2]; $output
echo const char *const xml_builtin[][2] = { $output
for input; do
diff --git a/gdbstub.c b/gdbstub.c
index 2b03ef2..0aa081b 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1504,7 +1504,6 @@ static int memtox(char *buf, const char *mem, int len)
static const char *get_feature_xml(const char *p, const char **newp)
{
-extern const char *const xml_builtin[][2];
size_t len;
int i;
const char *name;
diff --git a/gdbstub.h b/gdbstub.h
index 219abda..ce5fdcc 100644
--- a/gdbstub.h
+++ b/gdbstub.h
@@ -38,4 +38,7 @@ int gdbserver_start(int);
int gdbserver_start(const char *port);
#endif
+/* in gdbstub-xml.c, generated by feature_to_c.sh */
+extern const char *const xml_builtin[][2];
+
#endif
--
1.6.2.4
[Qemu-devel] [PATCH 08/15] blkdebug: fix enum comparison
The signedness of enum types depend on the compiler implementation.
Therefore the check for negative values may or may not be meaningful.
Fix by explicitly casting to a signed integer.
Since the values are also checked earlier against event_names
table, this is an internal error. Change the 'if' to 'assert'.
This also fixes a warning with GCC flag -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
block/blkdebug.c |4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 2a63df9..4d6ff0a 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -439,9 +439,7 @@ static void blkdebug_debug_event(BlockDriverState
*bs, BlkDebugEvent event)
struct BlkdebugRule *rule;
BlkdebugVars old_vars = s-vars;
-if (event 0 || event = BLKDBG_EVENT_MAX) {
-return;
-}
+assert((int)event = 0 event BLKDBG_EVENT_MAX);
QLIST_FOREACH(rule, s-rules[event], next) {
process_rule(bs, rule, old_vars);
--
1.6.2.4
[Qemu-devel] [PATCH 07/15] pxa2xx: remove useless checks
Remove checks which were made useless by r5849,
8da3ff180974732fc4272cb4433fef85c1822961.
This also fixes a warning with GCC flag -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
hw/pxa2xx.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index 26b9205..faa3d95 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -125,7 +125,7 @@ static void pxa2xx_pm_write(void *opaque,
target_phys_addr_t addr,
break;
default: /* Read-write registers */
-if (addr = PMCR addr = PCMD31 !(addr 3)) {
+if (!(addr 3)) {
s-pm_regs[addr 2] = value;
break;
}
--
1.6.2.4
[Qemu-devel] [PATCH 09/15] PPC: Suppress gcc warnings with -Wtype-limits
The hack added by c5b76b381081680633e2e0a91216507430409fb2 was not
enough to fix warnings with gcc flag -Wtype-limits. Add a new macro
to fix both problems.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
target-ppc/op_helper.c | 50
1 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 8cf34d4..3e6db85 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -1955,14 +1955,14 @@ target_ulong helper_dlmzb (target_ulong high,
target_ulong low, uint32_t update_
DO_HANDLE_NAN(result, x) DO_HANDLE_NAN(result, y) DO_HANDLE_NAN(result, z)
/* Saturating arithmetic helpers. */
-#define SATCVT(from, to, from_type, to_type, min, max, use_min, use_max) \
+#define SATCVT(from, to, from_type, to_type, min, max) \
static inline to_type cvt##from##to(from_type x, int *sat) \
{ \
to_type r; \
-if (use_min x min) { \
+if (x (from_type)min) { \
r = min;\
*sat = 1; \
-} else if (use_max x max) {\
+} else if (x (from_type)max) {\
r = max;\
*sat = 1; \
} else {\
@@ -1970,30 +1970,30 @@ target_ulong helper_dlmzb (target_ulong high,
target_ulong low, uint32_t update_
} \
return r; \
}
-SATCVT(sh, sb, int16_t, int8_t, INT8_MIN, INT8_MAX, 1, 1)
-SATCVT(sw, sh, int32_t, int16_t, INT16_MIN, INT16_MAX, 1, 1)
-SATCVT(sd, sw, int64_t, int32_t, INT32_MIN, INT32_MAX, 1, 1)
-
-/* Work around gcc problems with the macro version */
-static inline uint8_t cvtuhub(uint16_t x, int *sat)
-{
-uint8_t r;
-
-if (x UINT8_MAX) {
-r = UINT8_MAX;
-*sat = 1;
-} else {
-r = x;
+#define SATCVTU(from, to, from_type, to_type, min, max) \
+static inline to_type cvt##from##to(from_type x, int *sat) \
+{ \
+to_type r; \
+if (x (from_type)max) { \
+r = max;\
+*sat = 1; \
+} else {\
+r = x; \
+} \
+return r; \
}
-return r;
-}
-//SATCVT(uh, ub, uint16_t, uint8_t, 0, UINT8_MAX, 0, 1)
-SATCVT(uw, uh, uint32_t, uint16_t, 0, UINT16_MAX, 0, 1)
-SATCVT(ud, uw, uint64_t, uint32_t, 0, UINT32_MAX, 0, 1)
-SATCVT(sh, ub, int16_t, uint8_t, 0, UINT8_MAX, 1, 1)
-SATCVT(sw, uh, int32_t, uint16_t, 0, UINT16_MAX, 1, 1)
-SATCVT(sd, uw, int64_t, uint32_t, 0, UINT32_MAX, 1, 1)
+SATCVT(sh, sb, int16_t, int8_t, INT8_MIN, INT8_MAX)
+SATCVT(sw, sh, int32_t, int16_t, INT16_MIN, INT16_MAX)
+SATCVT(sd, sw, int64_t, int32_t, INT32_MIN, INT32_MAX)
+
+SATCVTU(uh, ub, uint16_t, uint8_t, 0, UINT8_MAX)
+SATCVTU(uw, uh, uint32_t, uint16_t, 0, UINT16_MAX)
+SATCVTU(ud, uw, uint64_t, uint32_t, 0, UINT32_MAX)
+SATCVT(sh, ub, int16_t, uint8_t, 0, UINT8_MAX)
+SATCVT(sw, uh, int32_t, uint16_t, 0, UINT16_MAX)
+SATCVT(sd, uw, int64_t, uint32_t, 0, UINT32_MAX)
#undef SATCVT
+#undef SATCVTU
#define LVE(name, access, swap, element)\
void helper_##name (ppc_avr_t *r, target_ulong addr)\
--
1.6.2.4
[Qemu-devel] [PATCH 12/15] Use gcc warning flag -Wtype-limits
If the compiler supports the warning flag -Wtype-limits, use it.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
configure |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/configure b/configure
index 146dac0..47bbe39 100755
--- a/configure
+++ b/configure
@@ -138,7 +138,7 @@ QEMU_CFLAGS=-D_FORTIFY_SOURCE=2 $QEMU_CFLAGS
QEMU_CFLAGS=-I. -I\$(SRC_PATH) $QEMU_CFLAGS
LDFLAGS=-g $LDFLAGS
-gcc_flags=-Wold-style-declaration -Wold-style-definition
-fstack-protector-all
+gcc_flags=-Wold-style-declaration -Wold-style-definition
-Wtype-limits -fstack-protector-all
cat $TMPC EOF
int main(void) { return 0; }
EOF
--
1.6.2.4
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On 5 September 2010 11:44, Blue Swirl blauwir...@gmail.com wrote:
On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin m...@redhat.com wrote:
On Sun, Sep 05, 2010 at 09:06:10AM +, Blue Swirl wrote:
On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin m...@redhat.com wrote:
On Sat, Sep 04, 2010 at 05:21:24PM +, Blue Swirl wrote:
In the unsigned number space, the checks can be merged into one,
assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
could have:
- if (event 0 || event = BLKDBG_EVENT_MAX) {
+ if ((int)event 0 || event = BLKDBG_EVENT_MAX) {
This would also implement the check that the writer of this code was
trying to make.
The important thing to note is however is that the check as it is now
is not correct.
I agree, assuming that an enum can reach 0x8000 different values,
perhaps the current code is not ideal. Still I think calling it
wrong is wrong, and calling your patch a fix is wrong. (Same as
calling patches that remove a warning a fix, they are workarounds)
I agree. But it seems to indicate a bigger problem.
If we are trying to pass in a negative value, which is not one
of enum values, using BlkDebugEvent as type is just confusing,
we should just pass int instead.
AFAICT it's only possible to use the values listed in event_names in
blkdebug.c, other values are rejected. So the check should actually be
an assert() or it could even be removed.
Sounds good.
How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the
check? Then if the value changes, the need to add the comparison back
will be obvious.
This would work but it's weird. The thing is it's currently a correct
code and the check may be useless but it's the optimiser's task to
remove it, not ours. The compiler is not able to tell whether the
check makes sense or nott, because the compiler only has access to
preprocessed code. So why should you let the compiler have anything
to say on it.
Good point. I'll try to invent something better.
Use #pragma to supress the warning? Maybe we could wrap this in a macro ..
Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.
I think the assertion is still the best way, it ensures that something
will happen if OMAP_EMIFS_BASE changes. We could for example remove
OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
adding a new define could still forget to adjust the check anyway.
We could replace it with a macro
#define OMAP_EMIFS_VALID(addr) ((target_phys_addr_t)addr OMAP_EMIFF_BASE)
but all this does look artificial. And of course using type casts
is always scary ...
Would it help to have some inline functions that do the range checking
correctly?
We have a couple of range helpers in pci.h, these could be moved out
to range.h and we could add some more. As there act on u64 this will get
the type limits mostly automatically right.
That seems to be the best solution, I get no warnings with this:
While the resulting code is clean (just as the current code), I think
it really shows that this warning should not be enabled. At this
point you find yourself working around your compiler and potentially
forcing other write some really strange code to work around the
problem caused by this.
There are many warnings that should not be enabled by default for this
reason (like the uninitialised variable warning) unless they are fixed
to be really intelligent (which is unlikely in this case).
Cheers
[Qemu-devel] [PATCH 14/15] Use gcc warning flag -Wempty-body, fix warnings
If the compiler supports the warning flag -Wempty-body, use it.
Adjust the code to avoid the warnings.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
configure |2 +-
hw/omap_i2c.c |5 +++--
hw/omap_mmc.c |5 +++--
hw/pxa2xx.c |5 +++--
hw/soc_dma.c |5 +++--
target-cris/mmu.c |2 +-
6 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/configure b/configure
index 6e4917a..61626b8 100755
--- a/configure
+++ b/configure
@@ -140,7 +140,7 @@ LDFLAGS=-g $LDFLAGS
gcc_flags=-Wold-style-declaration -Wold-style-definition -Wtype-limits
gcc_flags=-Wformat-security -Wformat-y2k -Winit-self
-Wignored-qualifiers $gcc_flags
-gcc_flags=-Wmissing-include-dirs -Wclobbered $gcc_flags
+gcc_flags=-Wmissing-include-dirs -Wclobbered -Wempty-body $gcc_flags
gcc_flags=-fstack-protector-all $gcc_flags
cat $TMPC EOF
int main(void) { return 0; }
diff --git a/hw/omap_i2c.c b/hw/omap_i2c.c
index d7c1888..d133977 100644
--- a/hw/omap_i2c.c
+++ b/hw/omap_i2c.c
@@ -190,8 +190,9 @@ static uint32_t omap_i2c_read(void *opaque,
target_phys_addr_t addr)
if (s-rxlen 2)
s-fifo = 16;
s-rxlen -= 2;
-} else
-/* XXX: remote access (qualifier) error - what's that? */;
+} else {
+/* XXX: remote access (qualifier) error - what's that? */
+}
if (!s-rxlen) {
s-stat = ~(1 3); /* RRDY */
if (((s-control 10) 1) /* MST */
diff --git a/hw/omap_mmc.c b/hw/omap_mmc.c
index 15cbf06..9d167ff 100644
--- a/hw/omap_mmc.c
+++ b/hw/omap_mmc.c
@@ -559,8 +559,9 @@ static void omap_mmc_cover_cb(void *opaque, int
line, int level)
if (!host-cdet_state level) {
host-status |= 0x0002;
omap_mmc_interrupts_update(host);
-if (host-cdet_wakeup)
-/* TODO: Assert wake-up */;
+if (host-cdet_wakeup) {
+/* TODO: Assert wake-up */
+}
}
if (host-cdet_state != level) {
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index 88f61c0..6e04645 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -1877,8 +1877,9 @@ static void pxa2xx_fir_write(void *opaque,
target_phys_addr_t addr,
s-control[0] = value;
if (!(value (1 4))) /* RXE */
s-rx_len = s-rx_start = 0;
-if (!(value (1 3))) /* TXE */
-/* Nop */;
+if (!(value (1 3))) { /* TXE */
+/* Nop */
+}
s-enable = value 1; /* ITR */
if (!s-enable)
s-status[0] = 0;
diff --git a/hw/soc_dma.c b/hw/soc_dma.c
index e116e63..23ec516 100644
--- a/hw/soc_dma.c
+++ b/hw/soc_dma.c
@@ -192,12 +192,13 @@ static void soc_dma_ch_freq_update(struct dma_s *s)
if (s-enabled_count)
/* We completely ignore channel priorities and stuff */
s-channel_freq = s-soc.freq / s-enabled_count;
-else
+else {
/* TODO: Signal that we want to disable the functional clock and let
* the platform code decide what to do with it, i.e. check that
* auto-idle is enabled in the clock controller and if we are stopping
* the clock, do the same with any parent clocks that had only one
- * user keeping them on and auto-idle enabled. */;
+ * user keeping them on and auto-idle enabled. */
+}
}
void soc_dma_set_request(struct soc_dma_ch_s *ch, int level)
diff --git a/target-cris/mmu.c b/target-cris/mmu.c
index 773438e..3f290ba 100644
--- a/target-cris/mmu.c
+++ b/target-cris/mmu.c
@@ -33,7 +33,7 @@
#define D(x) x
#define D_LOG(...) qemu_log(__VA_ARGS__)
#else
-#define D(x)
+#define D(x) do { } while (0)
#define D_LOG(...) do { } while (0)
#endif
--
1.6.2.4
Re: [Qemu-devel] Unmaintained QEMU builds
On 09/05/2010 06:01 PM, Andreas Färber wrote: Am 05.09.2010 um 13:19 schrieb Avi Kivity: On 09/04/2010 04:56 PM, Andreas Färber wrote: Maybe it's time to rethink the relation between QEMU and its frontends / management tools? If we want to compete with the commercial products (sic), we might agree on some official frontend per GUI-centric platform, with a Git-based repository (like qemu-kvm.git) and synchronized releases that may call themselves QEMU, linked from qemu.org, rather than having a variety of (outdated) Q* frontends per platform of which most are nothing more than a configuration window to spawn the regular qemu[-system-x86_64]. There is also virt-manager which is quite rich at this time. Seems I didn't get the point across too well: Standard users on Windows-PC and Mac expect a solution to their needs, not a forest of well-designed libraries and tools with .tgz downloads. QEMU has no such product identity, and there's no prominent binary download link for Win/Mac on the qemu.org frontpage. virt-manager is neither prominently advertised there either, nor does it have a Windows download. Definitely, virt-manager is not a solution for Windows/Mac. (Fwiw while it's certainly nice on Linux and to some limited degree on Solaris (ancient fork apparently), I wouldn't exactly describe the virt-install versions I've seen as rich... and setting up the VM is somewhat a prerequisite to using virt-manager's indeed nice features. I believe you can install a guest through virt-manager; virt-install is just a shortcut. Fedora's default security policies on top don't exactly make it easy to try out .isos or downloaded disk images with virt-manager, its German translation had a severe contentual error in the VM's menu and a felt 80% of the BRC bug reports get ignored and closed by a bot anyway, but that's another topic.) But sure, on Linux there's a plethora of simplistic Q* frontends, too. (n.b., virt-manager didn't match that regex ;) Choice and diversity isn't wrong per se, just the comparison of the available options on the two given platforms has shown not to make QEMU a common choice. Whining about lack of bugfix contributions is unlikely going to change that imo. As a baby step, is there any chance of publishing an automatic nightly Windows (cross-)build as a .zip file on qemu.org? That might give more users a chance of detecting runtime faults during the development cycle. That's doable and useful, yes. But I don't really see a path towards a competitive full fledged bundled/native qemu GUI. It's a huge amount of work, no one seems interested (or has an employer who's interested), and it requires talent we don't have. It will take a sustained effort by multiple people. Until then, Windows will be a second class host and we'll have to rely on external GUIs. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Unmaintained QEMU builds
On 09/05/2010 06:44 PM, Andreas Färber wrote: Am 05.09.2010 um 16:17 schrieb Avi Kivity: On 09/05/2010 05:10 PM, Blue Swirl wrote: Easy to use GUI and integration to host system are important, but performance is also a big problem. QEMU/TCG can't compete with alternatives that use proprietary kernel modules. Someone should recreate kqemu by using KVM compatible interfaces. If someone is really willing to invest the effort to do that cleanly, I am willing to merge it into kvm. That would allow reuse of the mmu and some other logic that got a lot of effort in kvm. I believe I already inquired about this when kqemu was dropped: KVM is GPL'ed iiuc. May we use it as a kernel extension with proprietary Mac OS X at all then? No idea. I thought there was some controversy on whether runtime-linking GPL modules to a closed-source kernel constitutes a GPL violation or not. (it would be news to me if Darwin/x86 was ever supported by kqemu) Having kqemu running as a userland service process (?) on Windows seems unproblematic by comparison. These things want to run in the kernel (or did you mean a kernel driver providing services to userland?) Don't know about the BSDs or how this would fit with OpenSolaris' CDDL. On Haiku new kernel code would probably be preferred under MIT/X11 License. In either case, I'm not aware of a clear documentation of what exactly is required to implement on the kernel side to replace kqemu or to provide a completely compatible implementation. It seems like a moving target. You can pick any recent snapshot and implement its interface. We don't force to upgrade their kernel as we go along. However, I doubt it is worth the effort, if anyone is interested in performance then they'd get a cpu that supports virtualization. That leaves non-Linux. This discussion was about non-Linux only. We can hardly call the Linux build unmaintained! :) Yeah - I though it diverged to whether we ship a bundled GUI or not - without that, performance doesn't really matter for non-Linux. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Unmaintained QEMU builds
On 09/05/2010 10:10 AM, Avi Kivity wrote: As a baby step, is there any chance of publishing an automatic nightly Windows (cross-)build as a .zip file on qemu.org? That might give more users a chance of detecting runtime faults during the development cycle. That's doable and useful, yes. I doubt it's useful. We don't have a massive pool of developers sitting on their hands waiting for something else to work on. We don't have myriads of users demanding better Windows support. Search the list, there's almost no one asking questions about Windows and considering that it's missing a ton of features and constantly broken, that strongly suggests that no one is actually using it. Windows support in QEMU is an academic exercise that's only of interest to developers. Regards, Anthony Liguori
Re: [Qemu-devel] Unmaintained QEMU builds
On 09/05/2010 06:57 PM, Anthony Liguori wrote: On 09/05/2010 10:10 AM, Avi Kivity wrote: As a baby step, is there any chance of publishing an automatic nightly Windows (cross-)build as a .zip file on qemu.org? That might give more users a chance of detecting runtime faults during the development cycle. That's doable and useful, yes. I doubt it's useful. We don't have a massive pool of developers sitting on their hands waiting for something else to work on. We don't have myriads of users demanding better Windows support. Search the list, there's almost no one asking questions about Windows and considering that it's missing a ton of features and constantly broken, that strongly suggests that no one is actually using it. Or maybe, real users don't use the git repository, so they aren't aware of the constant breakage. Windows support in QEMU is an academic exercise that's only of interest to developers. I'm perfectly fine with dropping it. btw, there are other features in qemu that seem to be academic exercises - *-user for example. What is it useful for? Most open source stuff is multiplatform, and serious commercial work needs something faster than tcg. I can understand cross-cpu system mode being very useful to embedded or kernel developers. x-on-x is only useful with virtualization, otherwise the performance penalty is too great. -- error compiling committee.c: too many arguments to function
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski balr...@gmail.com wrote:
On 5 September 2010 11:44, Blue Swirl blauwir...@gmail.com wrote:
On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin m...@redhat.com wrote:
On Sun, Sep 05, 2010 at 09:06:10AM +, Blue Swirl wrote:
On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin m...@redhat.com wrote:
On Sat, Sep 04, 2010 at 05:21:24PM +, Blue Swirl wrote:
In the unsigned number space, the checks can be merged into one,
assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
could have:
- if (event 0 || event = BLKDBG_EVENT_MAX) {
+ if ((int)event 0 || event = BLKDBG_EVENT_MAX) {
This would also implement the check that the writer of this code was
trying to make.
The important thing to note is however is that the check as it is now
is not correct.
I agree, assuming that an enum can reach 0x8000 different values,
perhaps the current code is not ideal. Still I think calling it
wrong is wrong, and calling your patch a fix is wrong. (Same as
calling patches that remove a warning a fix, they are workarounds)
On what basis do you still claim that? I think I explained the problem
at detail. There is a bug. I have a fix for the bug. The fix is not a
workaround, except maybe for committee-induced stupidity which created
the enum signedness ambiguity in the first place.
I agree. But it seems to indicate a bigger problem.
If we are trying to pass in a negative value, which is not one
of enum values, using BlkDebugEvent as type is just confusing,
we should just pass int instead.
AFAICT it's only possible to use the values listed in event_names in
blkdebug.c, other values are rejected. So the check should actually be
an assert() or it could even be removed.
Sounds good.
How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the
check? Then if the value changes, the need to add the comparison back
will be obvious.
This would work but it's weird. The thing is it's currently a correct
code and the check may be useless but it's the optimiser's task to
remove it, not ours. The compiler is not able to tell whether the
check makes sense or nott, because the compiler only has access to
preprocessed code. So why should you let the compiler have anything
to say on it.
Good point. I'll try to invent something better.
Use #pragma to supress the warning? Maybe we could wrap this in a macro
..
Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.
I think the assertion is still the best way, it ensures that something
will happen if OMAP_EMIFS_BASE changes. We could for example remove
OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
adding a new define could still forget to adjust the check anyway.
We could replace it with a macro
#define OMAP_EMIFS_VALID(addr) ((target_phys_addr_t)addr OMAP_EMIFF_BASE)
but all this does look artificial. And of course using type casts
is always scary ...
Would it help to have some inline functions that do the range checking
correctly?
We have a couple of range helpers in pci.h, these could be moved out
to range.h and we could add some more. As there act on u64 this will get
the type limits mostly automatically right.
That seems to be the best solution, I get no warnings with this:
While the resulting code is clean (just as the current code), I think
it really shows that this warning should not be enabled. At this
point you find yourself working around your compiler and potentially
forcing other write some really strange code to work around the
problem caused by this.
The warnings generated by -Wtype-limits are very useful, because with
it I have found several bugs in the code. Even the patches that are
not bugs fixes are cleanups, not 'some really strange code'. Please
take a look at the 15 piece patch set I sent last, the patches
identify the problems better than this one you are replying to. Which
ones do you still think are only workarounds? Please be more specific.
There are many warnings that should not be enabled by default for this
reason (like the uninitialised variable warning) unless they are fixed
to be really intelligent (which is unlikely in this case).
Please review the latest set of patches and provide hard facts to
support your claims.
Re: [Qemu-devel] Unmaintained QEMU builds
On Sun, Sep 5, 2010 at 4:05 PM, Avi Kivity a...@redhat.com wrote: On 09/05/2010 06:57 PM, Anthony Liguori wrote: On 09/05/2010 10:10 AM, Avi Kivity wrote: As a baby step, is there any chance of publishing an automatic nightly Windows (cross-)build as a .zip file on qemu.org? That might give more users a chance of detecting runtime faults during the development cycle. That's doable and useful, yes. I doubt it's useful. We don't have a massive pool of developers sitting on their hands waiting for something else to work on. We don't have myriads of users demanding better Windows support. Search the list, there's almost no one asking questions about Windows and considering that it's missing a ton of features and constantly broken, that strongly suggests that no one is actually using it. Or maybe, real users don't use the git repository, so they aren't aware of the constant breakage. Windows support in QEMU is an academic exercise that's only of interest to developers. I'm perfectly fine with dropping it. btw, there are other features in qemu that seem to be academic exercises - *-user for example. What is it useful for? Most open source stuff is multiplatform, and serious commercial work needs something faster than tcg. *-user can be used by developers to make specific tests with TCG more easily and faster than with system emulation. I think someone also used it to run Wine on PPC.
Re: [Qemu-devel] Unmaintained QEMU builds
On 09/05/2010 07:25 PM, Blue Swirl wrote: I'm perfectly fine with dropping it. btw, there are other features in qemu that seem to be academic exercises - *-user for example. What is it useful for? Most open source stuff is multiplatform, and serious commercial work needs something faster than tcg. *-user can be used by developers to make specific tests with TCG more easily and faster than with system emulation. Maybe we need a unit test framework instead? Translating system calls is a lot of work for testing a jitter. (of course, *-user exists, so why not use it) I think someone also used it to run Wine on PPC. That's dead now. And in any case, it would be too slow for production use of contemporary software. -- error compiling committee.c: too many arguments to function
[Qemu-devel] [PATCH] qemu: e1000 fix TOR math
Patch b0b900070c7cb29bbefb732ec00397abe5de6d73 made
TOR valuer incorrect: the spec says it should always
include the CRC field, while size does not include CRC now.
No one seems to use TOR field (which is likely why
current code works fine), but better to stick to spec.
Lightly tested with a linux guest.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
hw/e1000.c |8 ++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/e1000.c b/hw/e1000.c
index 80b78bc..eb9faf2 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -345,7 +345,7 @@ is_vlan_txd(uint32_t txd_lower)
/* FCS aka Ethernet CRC-32. We don't get it from backends and can't
* fill it in, just pad descriptor length by 4 bytes unless guest
- * told us to trip it off the packet. */
+ * told us to strip it off the packet. */
static inline int
fcs_len(E1000State *s)
{
@@ -690,8 +690,12 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf,
size_t size)
s-mac_reg[GPRC]++;
s-mac_reg[TPR]++;
+/* TOR - Total Octets Received:
+ * This register includes bytes received in a packet from the Destination
+ * Address field through the CRC field, inclusively.
+ */
n = s-mac_reg[TORL];
-if ((s-mac_reg[TORL] += size) n)
+if ((s-mac_reg[TORL] += size + 4 /* Always include FCS length. */) n)
s-mac_reg[TORH]++;
n = E1000_ICS_RXT0;
--
1.7.2.rc0.14.g41c1c
[Qemu-devel] Re: Unmaintained QEMU builds
Am 05.09.2010 um 17:41 schrieb Paolo Bonzini: The main thing is what you wrote in another message: what can QEMU offer on Windows and Darwin that semi-free Virtual Box and proprietary VMware cannot? I like to think that it can offer something, but maybe I'm wrong. :/ On Darwin/ppc64, I'm using QEMU for emulation of ppc, sparc and less common x86. There's no real competitor. My management tool is bash though. On Darwin/x86, VirtualBox has a foreign Qt-based UI. In terms of the machine's GUI itself it shouldn't be too hard to compete with some extensions to the Cocoa frontend (was going to look into that, QMP might make this less invasive). VMware Fusion and Parallels are not free. Never used it on Windows. I guess it's more the unique emulation capabilities it has to offer. Andreas
Re: [Qemu-devel] Guest cannot handle a PCI BAR 1GB
On 09/04/2010 01:22 AM, Cam Macdonell wrote: Hi, I'm trying to test 2 GB (and eventually larger) BARs with ivshmem and I get an error in the guest that it is able to find a mem resource for a BAR larger than 1GB. I'm using 64-bit BARs. when running with 6GB of RAM and a 1GB BAR for ivshmem, it finds a resource (and searches beyond 32-bit values to find it). Here is a log from printfs added to the loop that searches the resources from find_resource() in kernel/resource.c:363. This is a kernel question, not a qemu issue. Copying lkml. trying 'tmp.start' 1000 to 'tmp.end' fff trying 'tmp.start' 9f400 to 'tmp.end' 9f3ff trying 'tmp.start' a to 'tmp.end' e trying 'tmp.start' 10 to 'tmp.end' f trying 'tmp.start' dfffd000 to 'tmp.end' dfffcfff trying 'tmp.start' e000 to 'tmp.end' efff trying 'tmp.start' f200 to 'tmp.end' f1ff trying 'tmp.start' f2001000 to 'tmp.end' f200 trying 'tmp.start' f202 to 'tmp.end' f201 trying 'tmp.start' f2021000 to 'tmp.end' f202 trying 'tmp.start' f204 to 'tmp.end' f203 trying 'tmp.start' f2040100 to 'tmp.end' febf trying 'tmp.start' fec00400 to 'tmp.end' fffb trying 'tmp.start' 1 to 'tmp.end' trying 'tmp.start' 1a000 to 'tmp.end' pci :00:04.0: BAR 2: assigned [mem 0x1c000-0x1 64bit] pci :00:04.0: BAR 2: set to [mem 0x1c000-0x1 64bit] (PCI address [0x1c000-0x1] and you can see the BAR is successfully assigned. However, with a 2GB BAR (below), the search fails, but it also never searches beyong 32-bits. Again, all that's changed is the size of the ivshmem region. trying 'tmp.start' 1000 to 'tmp.end' fff trying 'tmp.start' 9f400 to 'tmp.end' 9f3ff trying 'tmp.start' a to 'tmp.end' e trying 'tmp.start' 10 to 'tmp.end' f trying 'tmp.start' dfffd000 to 'tmp.end' dfffcfff pci :00:04.0: BAR 2: can't assign mem (size 0x8000) Is there a limit to PCI BAR sizes or resources? Any pointers or further debugging tips are greatly appreciated. What kernel version are you looking at? Please add printks to the loop so we can see this-start and this-end. It smells like a truncation issue. -- error compiling committee.c: too many arguments to function
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On 5 September 2010 18:15, Blue Swirl blauwir...@gmail.com wrote:
On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski balr...@gmail.com wrote:
On 5 September 2010 11:44, Blue Swirl blauwir...@gmail.com wrote:
On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin m...@redhat.com wrote:
On Sun, Sep 05, 2010 at 09:06:10AM +, Blue Swirl wrote:
On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin m...@redhat.com
wrote:
On Sat, Sep 04, 2010 at 05:21:24PM +, Blue Swirl wrote:
In the unsigned number space, the checks can be merged into one,
assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
could have:
- if (event 0 || event = BLKDBG_EVENT_MAX) {
+ if ((int)event 0 || event = BLKDBG_EVENT_MAX) {
This would also implement the check that the writer of this code was
trying to make.
The important thing to note is however is that the check as it is now
is not correct.
I agree, assuming that an enum can reach 0x8000 different values,
perhaps the current code is not ideal. Still I think calling it
wrong is wrong, and calling your patch a fix is wrong. (Same as
calling patches that remove a warning a fix, they are workarounds)
On what basis do you still claim that?
I wanted to ask the same question. Without constants in the
definition, the values of an enum range from 0 to N-1. You explained
that if the enum had INT_MAX different values, then the signedness of
the values would matter (but for it to be signed would require it to
have constants again, which is not the case for enumerations of types
of an event). Can an enum even have INT_MAX values? It for sure
can't have UINT_MAX values. You failed to give an example value which
would make any difference in the result of the check. Perhaps I'm
misunderstanding where you see the bug.
I think I explained the problem
at detail. There is a bug. I have a fix for the bug. The fix is not a
workaround, except maybe for committee-induced stupidity which created
the enum signedness ambiguity in the first place.
I agree. But it seems to indicate a bigger problem.
If we are trying to pass in a negative value, which is not one
of enum values, using BlkDebugEvent as type is just confusing,
we should just pass int instead.
AFAICT it's only possible to use the values listed in event_names in
blkdebug.c, other values are rejected. So the check should actually be
an assert() or it could even be removed.
Sounds good.
How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the
check? Then if the value changes, the need to add the comparison
back
will be obvious.
This would work but it's weird. The thing is it's currently a
correct
code and the check may be useless but it's the optimiser's task to
remove it, not ours. The compiler is not able to tell whether the
check makes sense or nott, because the compiler only has access to
preprocessed code. So why should you let the compiler have anything
to say on it.
Good point. I'll try to invent something better.
Use #pragma to supress the warning? Maybe we could wrap this in a macro
..
Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.
I think the assertion is still the best way, it ensures that something
will happen if OMAP_EMIFS_BASE changes. We could for example remove
OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
adding a new define could still forget to adjust the check anyway.
We could replace it with a macro
#define OMAP_EMIFS_VALID(addr) ((target_phys_addr_t)addr OMAP_EMIFF_BASE)
but all this does look artificial. And of course using type casts
is always scary ...
Would it help to have some inline functions that do the range checking
correctly?
We have a couple of range helpers in pci.h, these could be moved out
to range.h and we could add some more. As there act on u64 this will get
the type limits mostly automatically right.
That seems to be the best solution, I get no warnings with this:
While the resulting code is clean (just as the current code), I think
it really shows that this warning should not be enabled. At this
point you find yourself working around your compiler and potentially
forcing other write some really strange code to work around the
problem caused by this.
The warnings generated by -Wtype-limits are very useful, because with
it I have found several bugs in the code.
Is that an argument for enabling a warning *by default*? Looking at
any specific part of the code you'll find bugs. If you enable some
warning, it'll hint on a given subset of the places in the code, some
of which are bugs and some are false-positives. Enable a different
warning and you get a different subset. Grep for any given keyword or
constant and you get a different subset.
Even the patches that are
not bugs fixes are cleanups, not 'some really strange code'. Please
take a look at the 15 piece patch set I sent
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On 5 September 2010 19:02, andrzej zaborowski balr...@gmail.com wrote: Patches 05, 06, 07, 09, 11, 14, 15 all replace one version of the code with a different that achieves the exact same functionality for all Sorry, patch 11 is a fix (unrelated to the warning though). Cheers
Re: [Qemu-devel] Unmaintained QEMU builds
On Sun, 5 Sep 2010, Avi Kivity wrote: On 09/05/2010 06:57 PM, Anthony Liguori wrote: On 09/05/2010 10:10 AM, Avi Kivity wrote: As a baby step, is there any chance of publishing an automatic nightly Windows (cross-)build as a .zip file on qemu.org? That might give more users a chance of detecting runtime faults during the development cycle. That's doable and useful, yes. I doubt it's useful. We don't have a massive pool of developers sitting on their hands waiting for something else to work on. We don't have myriads of users demanding better Windows support. Search the list, there's almost no one asking questions about Windows and considering that it's missing a ton of features and constantly broken, that strongly suggests that no one is actually using it. Or maybe, real users don't use the git repository, so they aren't aware of the constant breakage. Windows support in QEMU is an academic exercise that's only of interest to developers. I'm perfectly fine with dropping it. btw, there are other features in qemu that seem to be academic exercises - *-user for example. What is it useful for? Most open source stuff is multiplatform, and serious commercial work needs something faster than tcg. Riiight.. Here's a story, my work duties required me to fiddled with IP3K based board, thing is - even though their gdb patches were obviously freely available they were hopelessly endianness confused there were two options: a) fix it b) use linux-user to run their x86 version of gdb. I picked b) and it worked just fine. Actually i have two stories, story number two, since i do not have Adobe Flash but sometimes want to watch a non-youtube video i need somehow to figure out how .swf requests the file, giving that i didn't exactly want to learn Flash assembly and the only decompiler was binary only, i just used it with linux-user. I can understand cross-cpu system mode being very useful to embedded or kernel developers. x-on-x is only useful with virtualization, otherwise the performance penalty is too great. -- mailto:av1...@comtv.ru
Re: [Qemu-devel] Unmaintained QEMU builds
On 5 September 2010 17:05, Avi Kivity a...@redhat.com wrote: I'm perfectly fine with dropping it. btw, there are other features in qemu that seem to be academic exercises - *-user for example. What is it useful for? Most open source stuff is multiplatform, and serious commercial work needs something faster than tcg. I can understand cross-cpu system mode being very useful to embedded or kernel developers. x-on-x is only useful with virtualization, otherwise the performance penalty is too great. Cross-cpu -user mode gets used in embedded development environments like Scratchbox as a compilation environment, where it gets you a number of advantages over -system mode: * much closer integration with the host filesystem etc * you can use a chroot with a mix of native and target-architecture binaries (so eg bash is the fast native version, gcc is actually a cross compiler, but target binaries built and run by autoconf also work, via binfmt_misc) * it's faster than full system emulation * you get to use all your host system's resources rather than just (for example) the 256MB RAM the target system supports (There are disadvantages to doing it that way too, but I think it's a real, common qemu use case.) -- Peter Maydell
Re: [Qemu-devel] Unmaintained QEMU builds
On 5 September 2010 19:33, malc av1...@comtv.ru wrote: On Sun, 5 Sep 2010, Avi Kivity wrote: On 09/05/2010 06:57 PM, Anthony Liguori wrote: On 09/05/2010 10:10 AM, Avi Kivity wrote: As a baby step, is there any chance of publishing an automatic nightly Windows (cross-)build as a .zip file on qemu.org? That might give more users a chance of detecting runtime faults during the development cycle. That's doable and useful, yes. I doubt it's useful. We don't have a massive pool of developers sitting on their hands waiting for something else to work on. We don't have myriads of users demanding better Windows support. Search the list, there's almost no one asking questions about Windows and considering that it's missing a ton of features and constantly broken, that strongly suggests that no one is actually using it. Or maybe, real users don't use the git repository, so they aren't aware of the constant breakage. Windows support in QEMU is an academic exercise that's only of interest to developers. I'm perfectly fine with dropping it. btw, there are other features in qemu that seem to be academic exercises - *-user for example. What is it useful for? Most open source stuff is multiplatform, and serious commercial work needs something faster than tcg. Riiight.. Here's a story, my work duties required me to fiddled with More examples of industrial use are Nokia and Palm using OpenEmbedded building firmware for their phones, which afaik I relies for some parts on qemu (just some parts, so the tcg performance doesn't impact overall performance that much). There are many more users of OE, but these two have products in shops near me. Cheers
Re: [Qemu-devel] Unmaintained QEMU builds
On 09/05/2010 08:44 PM, andrzej zaborowski wrote: I'm perfectly fine with dropping it. btw, there are other features in qemu that seem to be academic exercises - *-user for example. What is it useful for? Most open source stuff is multiplatform, and serious commercial work needs something faster than tcg. Riiight.. Here's a story, my work duties required me to fiddled with More examples of industrial use are Nokia and Palm using OpenEmbedded building firmware for their phones, which afaik I relies for some parts on qemu (just some parts, so the tcg performance doesn't impact overall performance that much). There are many more users of OE, but these two have products in shops near me. Well, both these examples are very far from the typical end user or even typical developer. No doubt anything is useful for someone, given the are 6.7Gp of us on this planet. Are those examples worth the effort? I don't know, but I'm sceptical. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Re: [Qemu-devel] Unmaintained ppc features (was: Unmaintained QEMU builds)
Am 05.09.2010 um 12:03 schrieb Alexander Graf: On 04.09.2010, at 16:41, Andreas Färber wrote: Am 17.08.2010 um 21:56 schrieb Anthony Liguori: Often time, the feature is not generally useful so the code just rots. I think an awful lot of the PPC boards and devices also fall into this category. Considering that we're well over half a million lines of code today, I think we would do ourselves a favor by dropping some of the dead features we're carrying. The only really broken ppc sysemu should be PReP atm, no? Otherwise I'm just aware of some OpenBIOS bugs. Qemu currently tries to potentially emulate every CPU flavor that ever existed in the world. Yeah, noticed the diversity and the hardly comprehensible macros during the TCG conversion. Yet conditionalizing instructions based on CPU doesn't seem wrong when you notice it in the manuals, arm does it too and Paul insisted on contributors doing that right. I have no idea if the original 601 implementation still works. Or 603. Not to speak of the halfway-implemented BookE CPUs. I'm also fairly sure that no emulated 64 bit CPU but the G5 work. I think the right thing to do for PPC would be to focus on a subset of CPUs we care about and make sure those work in combination. Also, the board emulation is ... eh ... suboptimal. The code is weirdly structured and I've already squashed a lot of bugs in there to make it barely work with Linux, but I have no idea about other OSs. Haiku/ppc was not working yet, I'm tracking some ofmem issues in OpenBIOS. http://permalink.gmane.org/gmane.comp.bios.openbios/3090 The thing we _really_ should do there would be a from-scratch implementation of a U2 (32-bit) and a U3/U4 (64-bit) board and just forget about the old stuff. While the PowerMacs are my main focus, I would actually be curious about 'old' BeBox emulation (dual 603, proprietary boot ROM; a GSoC suggestion) and interested in some new AIX capable machine (possibly reusing the now untested POWER stuff?). And it seemed like Hollis were still using some ppcemb board at his new job. Andreas
Re: [Qemu-devel] Unmaintained QEMU builds
On 5 September 2010 19:51, Avi Kivity a...@redhat.com wrote: On 09/05/2010 08:44 PM, andrzej zaborowski wrote: I'm perfectly fine with dropping it. btw, there are other features in qemu that seem to be academic exercises - *-user for example. What is it useful for? Most open source stuff is multiplatform, and serious commercial work needs something faster than tcg. Riiight.. Here's a story, my work duties required me to fiddled with More examples of industrial use are Nokia and Palm using OpenEmbedded building firmware for their phones, which afaik I relies for some parts on qemu (just some parts, so the tcg performance doesn't impact overall performance that much). There are many more users of OE, but these two have products in shops near me. Well, both these examples are very far from the typical end user or even typical developer. Some of the industrial users include all of their app developers which count in big numbers. Now I haven't installed Nokia or Palm's SDKs but Poky's SDK (OE-based) does include qemu. So I wouldn't be surprised if the number of deployments is bigger than system mode qemu. Cheers
[Qemu-devel] Re: [PATCH 14/15] Use gcc warning flag -Wempty-body, fix warnings
On Sun, Sep 05, 2010 at 03:07:23PM +, Blue Swirl wrote:
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index 88f61c0..6e04645 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -1877,8 +1877,9 @@ static void pxa2xx_fir_write(void *opaque,
target_phys_addr_t addr,
s-control[0] = value;
if (!(value (1 4))) /* RXE */
s-rx_len = s-rx_start = 0;
-if (!(value (1 3))) /* TXE */
-/* Nop */;
+if (!(value (1 3))) { /* TXE */
+/* Nop */
+}
Should we change the surrounding code to use {} to keep it consistent?
It's annoying to have {} in one place only.
s-enable = value 1; /* ITR */
if (!s-enable)
s-status[0] = 0;
[Qemu-devel] Re: [PATCH 08/15] blkdebug: fix enum comparison
On Sun, Sep 05, 2010 at 03:06:32PM +, Blue Swirl wrote:
The signedness of enum types depend on the compiler implementation.
Therefore the check for negative values may or may not be meaningful.
Fix by explicitly casting to a signed integer.
Since the values are also checked earlier against event_names
table, this is an internal error. Change the 'if' to 'assert'.
This also fixes a warning with GCC flag -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
block/blkdebug.c |4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 2a63df9..4d6ff0a 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -439,9 +439,7 @@ static void blkdebug_debug_event(BlockDriverState
*bs, BlkDebugEvent event)
struct BlkdebugRule *rule;
BlkdebugVars old_vars = s-vars;
-if (event 0 || event = BLKDBG_EVENT_MAX) {
-return;
-}
+assert((int)event = 0 event BLKDBG_EVENT_MAX);
I am not sure all compilers must generate a negative value from
a very large unsigned integer cast to int.
assert((unsigned)event BLKDBG_EVENT_MAX);
will do the same but without integer overflow.
QLIST_FOREACH(rule, s-rules[event], next) {
process_rule(bs, rule, old_vars);
--
1.6.2.4
[Qemu-devel] Re: [PATCH 06/15] Use range_covers_byte
On Sun, Sep 05, 2010 at 03:06:07PM +, Blue Swirl wrote:
Use range_covers_byte() instead of comparisons.
This also fixes some warnings with GCC flag -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
To me (not a native english speaker)
this comment implies that there's a bugfix here. Is there?
---
hw/omap1.c | 21 +++--
hw/sm501.c |5 +++--
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/hw/omap1.c b/hw/omap1.c
index 06370b6..2dd62ec 100644
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -26,6 +26,7 @@
/* We use pc-style serial ports. */
#include pc.h
#include blockdev.h
+#include range.h
/* Should signal the TCMI/GPMC */
uint32_t omap_badwidth_read8(void *opaque, target_phys_addr_t addr)
@@ -3669,37 +3670,45 @@ static const struct dma_irq_map omap1_dma_irq_map[] =
{
static int omap_validate_emiff_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-return addr = OMAP_EMIFF_BASE addr OMAP_EMIFF_BASE + s-sdram_size;
+return range_covers_byte(OMAP_EMIFF_BASE,
+ OMAP_EMIFF_BASE + s-sdram_size -
OMAP_EMIFF_BASE,
+ addr);
same as
return range_covers_byte(OMAP_EMIFF_BASE,
s-sdram_size,
addr);
}
static int omap_validate_emifs_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-return addr = OMAP_EMIFS_BASE addr OMAP_EMIFF_BASE;
+return range_covers_byte(OMAP_EMIFS_BASE, OMAP_EMIFF_BASE -
OMAP_EMIFS_BASE,
+ addr);
}
static int omap_validate_imif_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-return addr = OMAP_IMIF_BASE addr OMAP_IMIF_BASE + s-sram_size;
+return range_covers_byte(OMAP_IMIF_BASE,
+ OMAP_IMIF_BASE + s-sram_size - OMAP_IMIF_BASE,
+ addr);
same as
return range_covers_byte(OMAP_IMIF_BASE,
s-sram_size,
addr);
?
}
static int omap_validate_tipb_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-return addr = 0xfffb addr 0x;
+return range_covers_byte(0xfffb, 0x - 0xfffb, addr);
}
repeating the constant 0xfffb is a bit ugly ... give them names?
static int omap_validate_local_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-return addr = OMAP_LOCALBUS_BASE addr OMAP_LOCALBUS_BASE +
0x100;
+return range_covers_byte(OMAP_LOCALBUS_BASE,
+ OMAP_LOCALBUS_BASE + 0x100 -
+ OMAP_LOCALBUS_BASE,
+ addr);
Same as
return range_covers_byte(OMAP_LOCALBUS_BASE,
0x100,
addr);
?
}
static int omap_validate_tipb_mpui_addr(struct omap_mpu_state_s *s,
target_phys_addr_t addr)
{
-return addr = 0xe101 addr 0xe1020004;
+return range_covers_byte(0xe101, 0xe1020004 - 0xe101, addr);
}
repeating the constants is a bit ugly ... give them names?
struct omap_mpu_state_s *omap310_mpu_init(unsigned long sdram_size,
diff --git a/hw/sm501.c b/hw/sm501.c
index 8e6932d..705e0a5 100644
--- a/hw/sm501.c
+++ b/hw/sm501.c
@@ -29,6 +29,7 @@
#include devices.h
#include sysbus.h
#include qdev-addr.h
+#include range.h
/*
* Status: 2010/05/07
@@ -814,7 +815,7 @@ static uint32_t sm501_palette_read(void *opaque,
target_phys_addr_t addr)
/* TODO : consider BYTE/WORD access */
/* TODO : consider endian */
-assert(0 = addr addr 0x400 * 3);
+assert(range_covers_byte(0, 0x400 * 3, addr));
return *(uint32_t*)s-dc_palette[addr];
}
@@ -828,7 +829,7 @@ static void sm501_palette_write(void *opaque,
/* TODO : consider BYTE/WORD access */
/* TODO : consider endian */
-assert(0 = addr addr 0x400 * 3);
+assert(range_covers_byte(0, 0x400 * 3, addr));
*(uint32_t*)s-dc_palette[addr] = value;
}
--
1.6.2.4
[Qemu-devel] [PATCH] ivshmem: fix build on a 32 bit system
/scm/qemu/hw/ivshmem.c: In function ‘check_shm_size’:
/scm/qemu/hw/ivshmem.c:356: error: format ‘%ld’ expects type ‘long int’,
but argument 4 has type ‘__off64_t’
Fix by casting to u64.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
hw/ivshmem.c |5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index bbb5cba..6f41383 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -352,8 +352,9 @@ static int check_shm_size(IVShmemState *s, int fd) {
if (s-ivshmem_size buf.st_size) {
fprintf(stderr, IVSHMEM ERROR: Requested memory size greater);
-fprintf(stderr, than shared object size (% PRIu64 %ld)\n,
- s-ivshmem_size, buf.st_size);
+fprintf(stderr, than shared object size
+(% PRIu64 % PRIu64 d)\n,
+s-ivshmem_size, (uint64_t)buf.st_size);
return -1;
} else {
return 0;
--
1.7.2.rc0.14.g41c1c
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On Sun, Sep 5, 2010 at 5:02 PM, andrzej zaborowski balr...@gmail.com wrote:
On 5 September 2010 18:15, Blue Swirl blauwir...@gmail.com wrote:
On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski balr...@gmail.com wrote:
On 5 September 2010 11:44, Blue Swirl blauwir...@gmail.com wrote:
On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin m...@redhat.com wrote:
On Sun, Sep 05, 2010 at 09:06:10AM +, Blue Swirl wrote:
On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin m...@redhat.com
wrote:
On Sat, Sep 04, 2010 at 05:21:24PM +, Blue Swirl wrote:
In the unsigned number space, the checks can be merged into one,
assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
could have:
- if (event 0 || event = BLKDBG_EVENT_MAX) {
+ if ((int)event 0 || event = BLKDBG_EVENT_MAX) {
This would also implement the check that the writer of this code was
trying to make.
The important thing to note is however is that the check as it is now
is not correct.
I agree, assuming that an enum can reach 0x8000 different values,
perhaps the current code is not ideal. Still I think calling it
wrong is wrong, and calling your patch a fix is wrong. (Same as
calling patches that remove a warning a fix, they are workarounds)
On what basis do you still claim that?
I wanted to ask the same question. Without constants in the
definition, the values of an enum range from 0 to N-1. You explained
that if the enum had INT_MAX different values, then the signedness of
the values would matter
I never said anything about INT_MAX different values, you did.
(but for it to be signed would require it to
have constants again, which is not the case for enumerations of types
of an event). Can an enum even have INT_MAX values? It for sure
can't have UINT_MAX values. You failed to give an example value which
would make any difference in the result of the check. Perhaps I'm
misunderstanding where you see the bug.
Yes, please read the discussion again. Especially my message with the
example program.
I think I explained the problem
at detail. There is a bug. I have a fix for the bug. The fix is not a
workaround, except maybe for committee-induced stupidity which created
the enum signedness ambiguity in the first place.
I agree. But it seems to indicate a bigger problem.
If we are trying to pass in a negative value, which is not one
of enum values, using BlkDebugEvent as type is just confusing,
we should just pass int instead.
AFAICT it's only possible to use the values listed in event_names in
blkdebug.c, other values are rejected. So the check should actually be
an assert() or it could even be removed.
Sounds good.
How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out
the
check? Then if the value changes, the need to add the comparison
back
will be obvious.
This would work but it's weird. The thing is it's currently a
correct
code and the check may be useless but it's the optimiser's task to
remove it, not ours. The compiler is not able to tell whether the
check makes sense or nott, because the compiler only has access to
preprocessed code. So why should you let the compiler have anything
to say on it.
Good point. I'll try to invent something better.
Use #pragma to supress the warning? Maybe we could wrap this in a
macro ..
Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.
I think the assertion is still the best way, it ensures that something
will happen if OMAP_EMIFS_BASE changes. We could for example remove
OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
adding a new define could still forget to adjust the check anyway.
We could replace it with a macro
#define OMAP_EMIFS_VALID(addr) ((target_phys_addr_t)addr
OMAP_EMIFF_BASE)
but all this does look artificial. And of course using type casts
is always scary ...
Would it help to have some inline functions that do the range checking
correctly?
We have a couple of range helpers in pci.h, these could be moved out
to range.h and we could add some more. As there act on u64 this will get
the type limits mostly automatically right.
That seems to be the best solution, I get no warnings with this:
While the resulting code is clean (just as the current code), I think
it really shows that this warning should not be enabled. At this
point you find yourself working around your compiler and potentially
forcing other write some really strange code to work around the
problem caused by this.
The warnings generated by -Wtype-limits are very useful, because with
it I have found several bugs in the code.
Is that an argument for enabling a warning *by default*? Looking at
any specific part of the code you'll find bugs. If you enable some
warning, it'll hint on a given subset of the places in the code, some
of which are bugs and some are false-positives. Enable a different
Re: [Qemu-devel] Unmaintained QEMU builds
On Sun, Sep 05, 2010 at 07:56:02PM +0200, andrzej zaborowski wrote: On 5 September 2010 19:51, Avi Kivity a...@redhat.com wrote: On 09/05/2010 08:44 PM, andrzej zaborowski wrote: I'm perfectly fine with dropping it. btw, there are other features in qemu that seem to be academic exercises - *-user for example. What is it useful for? Most open source stuff is multiplatform, and serious commercial work needs something faster than tcg. Riiight.. Here's a story, my work duties required me to fiddled with More examples of industrial use are Nokia and Palm using OpenEmbedded building firmware for their phones, which afaik I relies for some parts on qemu (just some parts, so the tcg performance doesn't impact overall performance that much). There are many more users of OE, but these two have products in shops near me. Well, both these examples are very far from the typical end user or even typical developer. Some of the industrial users include all of their app developers which count in big numbers. Now I haven't installed Nokia or Palm's SDKs but Poky's SDK (OE-based) does include qemu. So I wouldn't be surprised if the number of deployments is bigger than system mode qemu. I agree, removing linux-user emulation doesn't make any sense to me. Cheers
[Qemu-devel] Re: [PATCH 14/15] Use gcc warning flag -Wempty-body, fix warnings
On Sun, Sep 5, 2010 at 5:54 PM, Michael S. Tsirkin m...@redhat.com wrote:
On Sun, Sep 05, 2010 at 03:07:23PM +, Blue Swirl wrote:
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index 88f61c0..6e04645 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -1877,8 +1877,9 @@ static void pxa2xx_fir_write(void *opaque,
target_phys_addr_t addr,
s-control[0] = value;
if (!(value (1 4))) /* RXE */
s-rx_len = s-rx_start = 0;
- if (!(value (1 3))) /* TXE */
- /* Nop */;
+ if (!(value (1 3))) { /* TXE */
+ /* Nop */
+ }
Should we change the surrounding code to use {} to keep it consistent?
It's annoying to have {} in one place only.
In other reviews, changes like that have been called unrelated.
s-enable = value 1; /* ITR */
if (!s-enable)
s-status[0] = 0;
Re: [Qemu-devel] Unmaintained QEMU builds
On 09/05/2010 11:05 AM, Avi Kivity wrote: We don't have a massive pool of developers sitting on their hands waiting for something else to work on. We don't have myriads of users demanding better Windows support. Search the list, there's almost no one asking questions about Windows and considering that it's missing a ton of features and constantly broken, that strongly suggests that no one is actually using it. Or maybe, real users don't use the git repository, so they aren't aware of the constant breakage. We're not talking about has been broken for 6 months. Windows support has been shady in QEMU as long as I've been involved in the project which is pretty much as long as Windows support has existed. IOW, the Windows port is really a work in progress that hasn't made any progress. Windows support in QEMU is an academic exercise that's only of interest to developers. I'm perfectly fine with dropping it. btw, there are other features in qemu that seem to be academic exercises - *-user for example. What is it useful for? Most open source stuff is multiplatform, and serious commercial work needs something faster than tcg. A good example of where -user is being actively used is for doing cross builds. A lot of build systems have proper cross compiler support and many still generate special programs and expect to be able to run them. So an alternative to the traditional --cross-prefix is to setup a linux-user based root and replace GCC/LD with cross compilers. That let's the native build system be used and the lion share of the work is done with native code. Regards, Anthony Liguori I can understand cross-cpu system mode being very useful to embedded or kernel developers. x-on-x is only useful with virtualization, otherwise the performance penalty is too great.
Re: [Qemu-devel] Unmaintained QEMU builds
On 09/05/2010 12:51 PM, Avi Kivity wrote: On 09/05/2010 08:44 PM, andrzej zaborowski wrote: I'm perfectly fine with dropping it. btw, there are other features in qemu that seem to be academic exercises - *-user for example. What is it useful for? Most open source stuff is multiplatform, and serious commercial work needs something faster than tcg. Riiight.. Here's a story, my work duties required me to fiddled with More examples of industrial use are Nokia and Palm using OpenEmbedded building firmware for their phones, which afaik I relies for some parts on qemu (just some parts, so the tcg performance doesn't impact overall performance that much). There are many more users of OE, but these two have products in shops near me. Well, both these examples are very far from the typical end user or even typical developer. No doubt anything is useful for someone, given the are 6.7Gp of us on this planet. Are those examples worth the effort? I don't know, but I'm sceptical. -linux-user really doesn't impact much common code so the effort is pretty low. Regards, Anthony Liguori
[Qemu-devel] Re: [PATCH 08/15] blkdebug: fix enum comparison
On Sun, Sep 5, 2010 at 5:57 PM, Michael S. Tsirkin m...@redhat.com wrote:
On Sun, Sep 05, 2010 at 03:06:32PM +, Blue Swirl wrote:
The signedness of enum types depend on the compiler implementation.
Therefore the check for negative values may or may not be meaningful.
Fix by explicitly casting to a signed integer.
Since the values are also checked earlier against event_names
table, this is an internal error. Change the 'if' to 'assert'.
This also fixes a warning with GCC flag -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
block/blkdebug.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 2a63df9..4d6ff0a 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -439,9 +439,7 @@ static void blkdebug_debug_event(BlockDriverState
*bs, BlkDebugEvent event)
struct BlkdebugRule *rule;
BlkdebugVars old_vars = s-vars;
- if (event 0 || event = BLKDBG_EVENT_MAX) {
- return;
- }
+ assert((int)event = 0 event BLKDBG_EVENT_MAX);
I am not sure all compilers must generate a negative value from
a very large unsigned integer cast to int.
The enum rules seem to be vague. The type of enums may also be signed
(on GCC when the enum set includes negative values, on other compilers
in other cases). Do any machines or compilers exist (on which QEMU
runs) where this could happen?
assert((unsigned)event BLKDBG_EVENT_MAX);
will do the same but without integer overflow.
It's not the same if BLKDBG_EVENT_MAX = 0x8000 and the type of
the BlkDebugEvent is unsigned. It's probably more correct, though.
QLIST_FOREACH(rule, s-rules[event], next) {
process_rule(bs, rule, old_vars);
--
1.6.2.4
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On 5 September 2010 21:16, Blue Swirl blauwir...@gmail.com wrote:
On Sun, Sep 5, 2010 at 5:02 PM, andrzej zaborowski balr...@gmail.com wrote:
On 5 September 2010 18:15, Blue Swirl blauwir...@gmail.com wrote:
On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski balr...@gmail.com
wrote:
On 5 September 2010 11:44, Blue Swirl blauwir...@gmail.com wrote:
On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin m...@redhat.com
wrote:
On Sun, Sep 05, 2010 at 09:06:10AM +, Blue Swirl wrote:
On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin m...@redhat.com
wrote:
On Sat, Sep 04, 2010 at 05:21:24PM +, Blue Swirl wrote:
In the unsigned number space, the checks can be merged into one,
assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
could have:
- if (event 0 || event = BLKDBG_EVENT_MAX) {
+ if ((int)event 0 || event = BLKDBG_EVENT_MAX) {
This would also implement the check that the writer of this code was
trying to make.
The important thing to note is however is that the check as it is now
is not correct.
I agree, assuming that an enum can reach 0x8000 different values,
perhaps the current code is not ideal. Still I think calling it
wrong is wrong, and calling your patch a fix is wrong. (Same as
calling patches that remove a warning a fix, they are workarounds)
On what basis do you still claim that?
I wanted to ask the same question. Without constants in the
definition, the values of an enum range from 0 to N-1. You explained
that if the enum had INT_MAX different values, then the signedness of
the values would matter
I never said anything about INT_MAX different values, you did.
You said a BLKDBG_EVENT_MAX = 0x8000 and that the enum has to be
signed, how will that happen?
(but for it to be signed would require it to
have constants again, which is not the case for enumerations of types
of an event). Can an enum even have INT_MAX values? It for sure
can't have UINT_MAX values. You failed to give an example value which
would make any difference in the result of the check. Perhaps I'm
misunderstanding where you see the bug.
Yes, please read the discussion again. Especially my message with the
example program.
I've re-read it and confirmed that you failed to show a scenario that
the check does not address. I'm trying to understand why you call
this code buggy.
The line is if (event 0 || event = BLKDBG_EVENT_MAX) { and it
assures that an out-of-range value of event will not get used.
I think I explained the problem
at detail. There is a bug. I have a fix for the bug. The fix is not a
workaround, except maybe for committee-induced stupidity which created
the enum signedness ambiguity in the first place.
I agree. But it seems to indicate a bigger problem.
If we are trying to pass in a negative value, which is not one
of enum values, using BlkDebugEvent as type is just confusing,
we should just pass int instead.
AFAICT it's only possible to use the values listed in event_names in
blkdebug.c, other values are rejected. So the check should actually be
an assert() or it could even be removed.
Sounds good.
How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out
the
check? Then if the value changes, the need to add the comparison
back
will be obvious.
This would work but it's weird. The thing is it's currently a
correct
code and the check may be useless but it's the optimiser's task to
remove it, not ours. The compiler is not able to tell whether the
check makes sense or nott, because the compiler only has access to
preprocessed code. So why should you let the compiler have
anything
to say on it.
Good point. I'll try to invent something better.
Use #pragma to supress the warning? Maybe we could wrap this in a
macro ..
Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.
I think the assertion is still the best way, it ensures that something
will happen if OMAP_EMIFS_BASE changes. We could for example remove
OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
adding a new define could still forget to adjust the check anyway.
We could replace it with a macro
#define OMAP_EMIFS_VALID(addr) ((target_phys_addr_t)addr
OMAP_EMIFF_BASE)
but all this does look artificial. And of course using type casts
is always scary ...
Would it help to have some inline functions that do the range checking
correctly?
We have a couple of range helpers in pci.h, these could be moved out
to range.h and we could add some more. As there act on u64 this will get
the type limits mostly automatically right.
That seems to be the best solution, I get no warnings with this:
While the resulting code is clean (just as the current code), I think
it really shows that this warning should not be enabled. At this
point you find yourself working around your compiler and potentially
forcing other write some
[Qemu-devel] Re: [PATCH 08/15] blkdebug: fix enum comparison'
On Sun, Sep 05, 2010 at 07:37:54PM +, Blue Swirl wrote:
On Sun, Sep 5, 2010 at 5:57 PM, Michael S. Tsirkin m...@redhat.com wrote:
On Sun, Sep 05, 2010 at 03:06:32PM +, Blue Swirl wrote:
The signedness of enum types depend on the compiler implementation.
Therefore the check for negative values may or may not be meaningful.
Fix by explicitly casting to a signed integer.
Since the values are also checked earlier against event_names
table, this is an internal error. Change the 'if' to 'assert'.
This also fixes a warning with GCC flag -Wtype-limits.
Signed-off-by: Blue Swirl blauwir...@gmail.com
---
block/blkdebug.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 2a63df9..4d6ff0a 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -439,9 +439,7 @@ static void blkdebug_debug_event(BlockDriverState
*bs, BlkDebugEvent event)
struct BlkdebugRule *rule;
BlkdebugVars old_vars = s-vars;
- if (event 0 || event = BLKDBG_EVENT_MAX) {
- return;
- }
+ assert((int)event = 0 event BLKDBG_EVENT_MAX);
I am not sure all compilers must generate a negative value from
a very large unsigned integer cast to int.
The enum rules seem to be vague. The type of enums may also be signed
(on GCC when the enum set includes negative values, on other compilers
in other cases). Do any machines or compilers exist (on which QEMU
runs) where this could happen?
I remember reading that GCC sometimes assumes signed integers don't overflow,
and generates code behaves incorrectly if they do.
No idea whether this is ever the case for casts.
--
MST
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On Sun, Sep 5, 2010 at 8:32 PM, andrzej zaborowski balr...@gmail.com wrote:
On 5 September 2010 21:16, Blue Swirl blauwir...@gmail.com wrote:
On Sun, Sep 5, 2010 at 5:02 PM, andrzej zaborowski balr...@gmail.com wrote:
On 5 September 2010 18:15, Blue Swirl blauwir...@gmail.com wrote:
On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski balr...@gmail.com
wrote:
On 5 September 2010 11:44, Blue Swirl blauwir...@gmail.com wrote:
On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin m...@redhat.com
wrote:
On Sun, Sep 05, 2010 at 09:06:10AM +, Blue Swirl wrote:
On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin m...@redhat.com
wrote:
On Sat, Sep 04, 2010 at 05:21:24PM +, Blue Swirl wrote:
In the unsigned number space, the checks can be merged into one,
assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
could have:
- if (event 0 || event = BLKDBG_EVENT_MAX) {
+ if ((int)event 0 || event = BLKDBG_EVENT_MAX) {
This would also implement the check that the writer of this code was
trying to make.
The important thing to note is however is that the check as it is
now
is not correct.
I agree, assuming that an enum can reach 0x8000 different values,
perhaps the current code is not ideal. Still I think calling it
wrong is wrong, and calling your patch a fix is wrong. (Same as
calling patches that remove a warning a fix, they are workarounds)
On what basis do you still claim that?
I wanted to ask the same question. Without constants in the
definition, the values of an enum range from 0 to N-1. You explained
that if the enum had INT_MAX different values, then the signedness of
the values would matter
I never said anything about INT_MAX different values, you did.
You said a BLKDBG_EVENT_MAX = 0x8000 and that the enum has to be
signed, how will that happen?
Please be more careful with your attributions. I did not say that either.
The problem case is when BLKDBG_EVENT_MAX 0x8000 and the type of
enum is unsigned. This can happen easily by
typedef enum {
BLKDBG_EVENT_MAX = 0x8001,
} BlkDebugEvent;
(but for it to be signed would require it to
have constants again, which is not the case for enumerations of types
of an event). Can an enum even have INT_MAX values? It for sure
can't have UINT_MAX values. You failed to give an example value which
would make any difference in the result of the check. Perhaps I'm
misunderstanding where you see the bug.
Yes, please read the discussion again. Especially my message with the
example program.
I've re-read it and confirmed that you failed to show a scenario that
the check does not address. I'm trying to understand why you call
this code buggy.
The line is if (event 0 || event = BLKDBG_EVENT_MAX) { and it
assures that an out-of-range value of event will not get used.
The problem case is when BLKDBG_EVENT_MAX 0x8000 and the type of
enum is unsigned. Then the first check is ignored by the compiler and
the second does not catch values which are between 0x8000 and
BLKDBG_EVENT_MAX. This may not be what was desired by the check,
though.
Those values will be caught with the int cast, or if the compiler
still happens to make the enum signed (for example, because
BLKDBG_EVENT_MAX was changed to a #define in order to support
compilers which don't allow too large enum values).
I think I explained the problem
at detail. There is a bug. I have a fix for the bug. The fix is not a
workaround, except maybe for committee-induced stupidity which created
the enum signedness ambiguity in the first place.
I agree. But it seems to indicate a bigger problem.
If we are trying to pass in a negative value, which is not one
of enum values, using BlkDebugEvent as type is just confusing,
we should just pass int instead.
AFAICT it's only possible to use the values listed in event_names in
blkdebug.c, other values are rejected. So the check should actually be
an assert() or it could even be removed.
Sounds good.
How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out
the
check? Then if the value changes, the need to add the comparison
back
will be obvious.
This would work but it's weird. The thing is it's currently a
correct
code and the check may be useless but it's the optimiser's task to
remove it, not ours. The compiler is not able to tell whether the
check makes sense or nott, because the compiler only has access to
preprocessed code. So why should you let the compiler have
anything
to say on it.
Good point. I'll try to invent something better.
Use #pragma to supress the warning? Maybe we could wrap this in a
macro ..
Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.
I think the assertion is still the best way, it ensures that something
will happen if OMAP_EMIFS_BASE changes. We could for example remove
OMAP_EMIFS_BASE entirely (it's only used for the
[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits
On 5 September 2010 23:44, Blue Swirl blauwir...@gmail.com wrote: The problem case is when BLKDBG_EVENT_MAX 0x8000 and the type of enum is unsigned. Then the first check is ignored by the compiler and the second does not catch values which are between 0x8000 and BLKDBG_EVENT_MAX. This may not be what was desired by the check, though. Those values will be caught with the int cast, or if the compiler still happens to make the enum signed (for example, because BLKDBG_EVENT_MAX was changed to a #define in order to support compilers which don't allow too large enum values). So you're actually talking about INT_MAX + 1, not 0x8000, the number depends on the abi. Quite clearly BLKDBG_EVENT_MAX is the max value in the enum so that the values can be used as indices of an array of a known size. I think it's safe to say it is INT_MAX. I think I explained the problem at detail. There is a bug. I have a fix for the bug. The fix is not a workaround, except maybe for committee-induced stupidity which created the enum signedness ambiguity in the first place. I agree. But it seems to indicate a bigger problem. If we are trying to pass in a negative value, which is not one of enum values, using BlkDebugEvent as type is just confusing, we should just pass int instead. AFAICT it's only possible to use the values listed in event_names in blkdebug.c, other values are rejected. So the check should actually be an assert() or it could even be removed. Sounds good. How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the check? Then if the value changes, the need to add the comparison back will be obvious. This would work but it's weird. The thing is it's currently a correct code and the check may be useless but it's the optimiser's task to remove it, not ours. The compiler is not able to tell whether the check makes sense or nott, because the compiler only has access to preprocessed code. So why should you let the compiler have anything to say on it. Good point. I'll try to invent something better. Use #pragma to supress the warning? Maybe we could wrap this in a macro .. Those lines may also desynch silently with changes to OMAP_EMIFS_BASE. I think the assertion is still the best way, it ensures that something will happen if OMAP_EMIFS_BASE changes. We could for example remove OMAP_EMIFS_BASE entirely (it's only used for the check), but someone adding a new define could still forget to adjust the check anyway. We could replace it with a macro #define OMAP_EMIFS_VALID(addr) ((target_phys_addr_t)addr OMAP_EMIFF_BASE) but all this does look artificial. And of course using type casts is always scary ... Would it help to have some inline functions that do the range checking correctly? We have a couple of range helpers in pci.h, these could be moved out to range.h and we could add some more. As there act on u64 this will get the type limits mostly automatically right. That seems to be the best solution, I get no warnings with this: While the resulting code is clean (just as the current code), I think it really shows that this warning should not be enabled. At this point you find yourself working around your compiler and potentially forcing other write some really strange code to work around the problem caused by this. The warnings generated by -Wtype-limits are very useful, because with it I have found several bugs in the code. Is that an argument for enabling a warning *by default*? Looking at any specific part of the code you'll find bugs. If you enable some warning, it'll hint on a given subset of the places in the code, some of which are bugs and some are false-positives. Enable a different warning and you get a different subset. Grep for any given keyword or constant and you get a different subset. Right, so when we enable *by default* the warning, buggy code (and unfortunately the false positives, if any) will not be committed. Ok, so malloc causes memory leeks, let's forbid dynamic allocation, right? The questionable malloc policies of your employer have nothing to do with this. If you don't agree with them, you can argue for a change in the rules or seek employment in a company without those rules. First, the policy is almost identical to the policy you're introducing so it has everything to do with this. I'm pointing out what is an actual faulty generalisation. Avoiding malloc or avoiding strcat or avoiding if (0 = 1) is unlikely to reduce the number of bugs, quite the opposite. It's identical to arguing against file sharing on the internet because illegal file sharing is possible, it's a faulty generalisation. (Criminals use cars = cars are evil) See your statement above about buggy code not being committed. (I've never been employed by Nokia. But I know people who wanted to submit improvements to gpsd, which is the project that
[Qemu-devel] RE: [PATCH] ivshmem: remove unnecessary checks for unsigned
Avi Kivity wrote: On 09/03/2010 05:54 AM, Hidetoshi Seto wrote: Oops, since I've registered qemu-kvm ML but not qemu-devel ML, I could not noticed that Jes have already posted same patch to qemu-devel. Now build of ivshmem is enabled only on KVM systems, please apply this patch to qemu-kvm.git asap. This was already applied to qemu.git; I'll pull it into qemu-kvm.git to fix the build. Qemu-kvm 6ee63ef38696aa3b0518f6aa6b85bc111ba7ca4e can build successfully now, thanks all. -Xudong
[Qemu-devel] PCIe Qemu changes for Q35
Hi Isaku I believe you were working on the Q35 chipset and PCIe emulation for the same, and planned to check your code in to the main git. I can use some of that work for something I am working on. I have not been too active on the mailing list, and might have missed your reviews if you have sent already. In that case, please disregard my email (and let me know privately). Otherwise, If you can share your plan of sending the review and checking the code, it would give me some idea of when I can expect the PCIe changes in the git. I am mostly interested in your PCIe code (and not Q35) for now. My apologies if I am sounding pushy, I just want to see if your plan falls within my requirement schedule and I can use your code. Awaiting your response. Thanks, Adhyas Two types have compatible type if their types are the same. — ANSI C Standard, 3.1.2.6.
