| Message ID | 1433367941-119488-1-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
On 3 June 2015 at 22:45, Alexander Graf <agraf@suse.de> wrote: > Hi Peter, > > This is my current patch queue for ppc. Please pull. > > Alex > > > The following changes since commit 3fc827d591679f3e262b9d1f8b34528eabfca8c0: > > target-arm: Correct check for non-EL3 (2015-06-02 13:22:29 +0100) > > are available in the git repository at: > > git://github.com/agraf/qemu.git tags/signed-ppc-for-upstream > > for you to fetch changes up to e229d3cc64420204cdb40b983ce08eae657812f9: > > softmmu: support up to 12 MMU modes (2015-06-03 23:42:13 +0200) > > ---------------------------------------------------------------- > Patch queue for ppc - 2015-06-03 > > Highlights this time around: > > - sPAPR: endian fixes, speedups, bug fixes, hotplug basics > - add default ram size capability for machines (sPAPR defaults to 512MB now) > > ---------------------------------------------------------------- Applied (the updated version), thanks. -- PMM
On 3 June 2015 at 22:45, Alexander Graf <agraf@suse.de> wrote: > Hi Peter, > > This is my current patch queue for ppc. Please pull. This is applied, but can you fix the clang sanitizer warnings, please? hw/ppc/spapr_drc.c:59:24: runtime error: left shift of negative value -1 hw/ppc/spapr_drc.c:587:19: runtime error: left shift of negative value -1 Problem looks to be in: #define DRC_INDEX_ID_MASK (~(~0 << DRC_INDEX_TYPE_SHIFT)) which is doing left shifts on a negative signed number. thanks -- PMM
On 05/06/2015 15:33, Peter Maydell wrote: > This is applied, but can you fix the clang sanitizer warnings, > please? > > hw/ppc/spapr_drc.c:59:24: runtime error: left shift of negative value -1 > hw/ppc/spapr_drc.c:587:19: runtime error: left shift of negative value -1 > > Problem looks to be in: > #define DRC_INDEX_ID_MASK (~(~0 << DRC_INDEX_TYPE_SHIFT)) > > which is doing left shifts on a negative signed number. Speaking in general, I find that this makes code worse. If you're using ~0 you probably want the value to extend with infinite ones. Using ~0u instead of ~0ull may cause problems down the line, and ~0ul is even worse because it is not 64-bit safe. Paolo
On 5 June 2015 at 15:35, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 05/06/2015 15:33, Peter Maydell wrote: >> This is applied, but can you fix the clang sanitizer warnings, >> please? >> >> hw/ppc/spapr_drc.c:59:24: runtime error: left shift of negative value -1 >> hw/ppc/spapr_drc.c:587:19: runtime error: left shift of negative value -1 >> >> Problem looks to be in: >> #define DRC_INDEX_ID_MASK (~(~0 << DRC_INDEX_TYPE_SHIFT)) >> >> which is doing left shifts on a negative signed number. > > Speaking in general, I find that this makes code worse. If you're using > ~0 you probably want the value to extend with infinite ones. > > Using ~0u instead of ~0ull may cause problems down the line, and ~0ul > is even worse because it is not 64-bit safe. I agree that C's semantics are terrible here (ideally left shift of negative values should Just Work in the 2s complement style, and right shift of negative values should be an arithmetic shift). Unfortunately we're stuck with the standard, which says this is undefined behaviour :-( -- PMM
On 05/06/2015 16:40, Peter Maydell wrote: > > Speaking in general, I find that this makes code worse. If you're using > > ~0 you probably want the value to extend with infinite ones. > > > > Using ~0u instead of ~0ull may cause problems down the line, and ~0ul > > is even worse because it is not 64-bit safe. > > I agree that C's semantics are terrible here (ideally > left shift of negative values should Just Work in the 2s > complement style, and right shift of negative values should > be an arithmetic shift). Unfortunately we're stuck with > the standard, which says this is undefined behaviour :-( But this is not something that C compiler writers can reasonably change. Can someone add a checkpatch rule that forbids shifting left U or UL constants (i.e. only ULL)? That would alleviate my concerns with these ubsan warnings. Paolo
On 5 June 2015 at 16:02, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 05/06/2015 16:40, Peter Maydell wrote: >> > Speaking in general, I find that this makes code worse. If you're using >> > ~0 you probably want the value to extend with infinite ones. >> > >> > Using ~0u instead of ~0ull may cause problems down the line, and ~0ul >> > is even worse because it is not 64-bit safe. >> >> I agree that C's semantics are terrible here (ideally >> left shift of negative values should Just Work in the 2s >> complement style, and right shift of negative values should >> be an arithmetic shift). Unfortunately we're stuck with >> the standard, which says this is undefined behaviour :-( > > But this is not something that C compiler writers can reasonably change. Right, which is why we need to change our code to not invoke undefined behaviour. (More generally, C compiler writers can agitate with the C standards bodies to get ideas like "friendly C" dialects accepted, http://blog.regehr.org/archives/1180, and they can provide them as vendor extensions too.) > Can someone add a checkpatch rule that forbids shifting left U or UL > constants (i.e. only ULL)? That would alleviate my concerns with these > ubsan warnings. ...but things like "(1U << 31)" are entirely valid. That's the reason these warnings are runtime rather than compile time in the first place... thanks -- PMM
On 05/06/2015 17:08, Peter Maydell wrote: >> > Can someone add a checkpatch rule that forbids shifting left U or UL >> > constants (i.e. only ULL)? That would alleviate my concerns with these >> > ubsan warnings. > > ...but things like "(1U << 31)" are entirely valid. They're only valid until someone does a ~ on them. I think it's reasonable to forbid them in our coding standards, if we want to fix ubsan's warning of (1 << 31). I don't think it's reasonable for compiler writers to exploit the undefinedness of (1 << 31) anyway, and if it were possible to shut up ubsan about this particular kind of undefined behavior, I would prefer it. Paolo
On 06/05/2015 07:33 AM, Peter Maydell wrote: > On 3 June 2015 at 22:45, Alexander Graf <agraf@suse.de> wrote: >> Hi Peter, >> >> This is my current patch queue for ppc. Please pull. > > This is applied, but can you fix the clang sanitizer warnings, > please? > > hw/ppc/spapr_drc.c:59:24: runtime error: left shift of negative value -1 > hw/ppc/spapr_drc.c:587:19: runtime error: left shift of negative value -1 > > Problem looks to be in: > #define DRC_INDEX_ID_MASK (~(~0 << DRC_INDEX_TYPE_SHIFT)) > > which is doing left shifts on a negative signed number. Might be as simple as using ~0U instead of ~0.
On 5 June 2015 at 16:20, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 05/06/2015 17:08, Peter Maydell wrote: >>> > Can someone add a checkpatch rule that forbids shifting left U or UL >>> > constants (i.e. only ULL)? That would alleviate my concerns with these >>> > ubsan warnings. >> >> ...but things like "(1U << 31)" are entirely valid. > > They're only valid until someone does a ~ on them. I think it's > reasonable to forbid them in our coding standards, if we want to fix > ubsan's warning of (1 << 31). > > I don't think it's reasonable for compiler writers to exploit the > undefinedness of (1 << 31) anyway, and if it were possible to shut up > ubsan about this particular kind of undefined behavior, I would prefer it. I don't think it's reasonable for compiler writers to exploit undefined behaviour either, but historically they absolutely have done. Absent a guarantee from gcc that it will never do so, I think we should avoid any UB in our code. There's also the usual rationale that it's worth fixing the borderline things that provoke complaints from lint-like tools in order that new warnings don't get lost in the pile of old uninteresting warnings. thanks -- PMM
On 05/06/2015 17:45, Peter Maydell wrote: >>> ...but things like "(1U << 31)" are entirely valid. >> >> They're only valid until someone does a ~ on them. I think it's >> reasonable to forbid them in our coding standards, if we want to fix >> ubsan's warning of (1 << 31). >> >> I don't think it's reasonable for compiler writers to exploit the >> undefinedness of (1 << 31) anyway, and if it were possible to shut up >> ubsan about this particular kind of undefined behavior, I would prefer it. > > I don't think it's reasonable for compiler writers to exploit > undefined behaviour either, but historically they absolutely > have done. Most cases of undefined behavior are rooted in "you should never do that anyway". This is not the case for bitwise operations, since they are not mathematical concepts and the representation of integers as bits is only implementation-defined. > Absent a guarantee from gcc that it will never do > so, I think we should avoid any UB in our code. The GCC manual says "GCC does not use the latitude given in C99 and C11 only to treat certain aspects of signed '<<' as undefined, but this is subject to change". It would certainly be nice if they removed the "this is subject to change" part. Paolo
On Fri, 5 Jun 2015, Paolo Bonzini wrote: > The GCC manual says "GCC does not use the latitude given in C99 and C11 > only to treat certain aspects of signed '<<' as undefined, but this is > subject to change". It would certainly be nice if they removed the > "this is subject to change" part. The correct statement would be more complicated. That is: the value returned is as documented, without that latitude being used for *optimization*, but (a) -fsanitize=undefined (and its subcase -fsanitize=shift) intends to follow exactly what the different standards specify when giving runtime errors and (b) the cases that are undefined are thereby not considered integer constant expressions (with consequent pedwarns-if-pedantic in various cases, and corner case effects on what's a null pointer constant). (The only "subject to change" would be that if there are still missing cases from the runtime detection or the not treating as integer constant expressions, then those missing cases may be fixed. I don't think it would be a good idea to add optimizations on this basis - for example, optimizations of x * 2 based on undefined overflow should not be applied to x << 1.)
On 5 June 2015 at 16:55, Paolo Bonzini <pbonzini@redhat.com> wrote: > The GCC manual says "GCC does not use the latitude given in C99 and C11 > only to treat certain aspects of signed '<<' as undefined, but this is > subject to change". It would certainly be nice if they removed the > "this is subject to change" part. Does clang provide a similar guarantee? I couldn't find one in a quick scan through the docs, but I might be looking in the wrong place. thanks -- PMM
On 5 June 2015 at 16:20, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 05/06/2015 17:08, Peter Maydell wrote: >>> > Can someone add a checkpatch rule that forbids shifting left U or UL >>> > constants (i.e. only ULL)? That would alleviate my concerns with these >>> > ubsan warnings. >> >> ...but things like "(1U << 31)" are entirely valid. > > They're only valid until someone does a ~ on them. I think it's > reasonable to forbid them in our coding standards, if we want to fix > ubsan's warning of (1 << 31). > > I don't think it's reasonable for compiler writers to exploit the > undefinedness of (1 << 31) anyway, and if it were possible to shut up > ubsan about this particular kind of undefined behavior, I would prefer it. I just checked, and it isn't possible to suppress this particular shift warning without also turning off others that we would want to retain. The ppc code is still provoking this sanitizer warning in my test runs :-( thanks -- PMM
Quoting Peter Maydell (2015-07-21 06:42:41) > On 5 June 2015 at 16:20, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 05/06/2015 17:08, Peter Maydell wrote: > >>> > Can someone add a checkpatch rule that forbids shifting left U or UL > >>> > constants (i.e. only ULL)? That would alleviate my concerns with these > >>> > ubsan warnings. > >> > >> ...but things like "(1U << 31)" are entirely valid. > > > > They're only valid until someone does a ~ on them. I think it's > > reasonable to forbid them in our coding standards, if we want to fix > > ubsan's warning of (1 << 31). > > > > I don't think it's reasonable for compiler writers to exploit the > > undefinedness of (1 << 31) anyway, and if it were possible to shut up > > ubsan about this particular kind of undefined behavior, I would prefer it. > > I just checked, and it isn't possible to suppress this particular > shift warning without also turning off others that we would want > to retain. > > The ppc code is still provoking this sanitizer warning in my > test runs :-( Was I being silly by not just doing this is the first place? -#define DRC_INDEX_ID_MASK (~(~0 << DRC_INDEX_TYPE_SHIFT)) +#define DRC_INDEX_ID_MASK ((1 << DRC_INDEX_TYPE_SHIFT) - 1) Will send a proper patch to list in a moment. > > thanks > -- PMM >
Hi Peter, This is my current patch queue for ppc. Please pull. Alex The following changes since commit 3fc827d591679f3e262b9d1f8b34528eabfca8c0: target-arm: Correct check for non-EL3 (2015-06-02 13:22:29 +0100) are available in the git repository at: git://github.com/agraf/qemu.git tags/signed-ppc-for-upstream for you to fetch changes up to e229d3cc64420204cdb40b983ce08eae657812f9: softmmu: support up to 12 MMU modes (2015-06-03 23:42:13 +0200) ---------------------------------------------------------------- Patch queue for ppc - 2015-06-03 Highlights this time around: - sPAPR: endian fixes, speedups, bug fixes, hotplug basics - add default ram size capability for machines (sPAPR defaults to 512MB now) ---------------------------------------------------------------- Alexey Kardashevskiy (10): spapr_iommu: Disable in-kernel IOMMU tables for >4GB windows spapr_iommu: Make H_PUT_TCE_INDIRECT endian-safe spapr_pci: Introduce a liobn number generating macros spapr_vio: Introduce a liobn number generating macros spapr_pci: Define default DMA window size as a macro spapr_iommu: Add separate trace points for PCI DMA operations spapr_pci: Make find_phb()/find_dev() public spapr_iommu: Make spapr_tce_find_by_liobn() public spapr_pci: Rework device-tree rendering spapr_iommu: Give unique QOM name to TCE table David Gibson (4): spapr_pci: Fix unsafe signed/unsigned comparisons pseries: Add pseries-2.4 machine type pseries: Enable in-kernel H_LOGICAL_CI_{LOAD, STORE} implementations Add David Gibson for sPAPR in MAINTAINERS file Markus Armbruster (1): macio: Convert to realize() Michael Roth (9): docs: add sPAPR hotplug/dynamic-reconfiguration documentation spapr_drc: initial implementation of sPAPRDRConnector device spapr: add rtas_st_buffer_direct() helper spapr_rtas: add ibm, configure-connector RTAS interface spapr_drc: add spapr_drc_populate_dt() spapr_pci: add dynamic-reconfiguration option for spapr-pci-host-bridge spapr_pci: create DRConnectors for each PCI slot during PHB realize pci: make pci_bar useable outside pci.c spapr_pci: enable basic hotplug operations Mike Day (2): spapr_rtas: add set-indicator RTAS interface spapr_rtas: add get-sensor-state RTAS interface Nathan Fontenot (2): spapr_rtas: add get/set-power-level RTAS interfaces spapr_events: re-use EPOW event infrastructure for hotplug events Nikunj A Dadhania (2): machine: add default_ram_size to machine class spapr: override default ram size to 512MB Paolo Bonzini (3): tci: do not use CPUArchState in tcg-target.h tcg: add TCG_TARGET_TLB_DISPLACEMENT_BITS softmmu: support up to 12 MMU modes Thomas Huth (5): dtc: Update dtc / libfdt submodule to version 1.4.0 configure: Check for libfdt version 1.4.0 hw/ppc/spapr_iommu: Fix the check for invalid upper bits in liobn hw/ppc/spapr: Fix error message when firmware could not be loaded hw/ppc/spapr: Use error_report() instead of hw_error() Tyrel Datwyler (2): spapr_events: event-scan RTAS interface spapr_pci: emit hotplug add/remove events during hotplug MAINTAINERS | 3 +- configure | 6 +- docs/specs/ppc-spapr-hotplug.txt | 287 +++++++++++++++ dtc | 2 +- hw/core/machine.c | 9 + hw/misc/macio/macio.c | 71 ++-- hw/pci/pci.c | 2 +- hw/ppc/Makefile.objs | 2 +- hw/ppc/spapr.c | 49 ++- hw/ppc/spapr_drc.c | 744 +++++++++++++++++++++++++++++++++++++++ hw/ppc/spapr_events.c | 338 +++++++++++++++--- hw/ppc/spapr_iommu.c | 46 ++- hw/ppc/spapr_pci.c | 513 +++++++++++++++++++++++---- hw/ppc/spapr_rtas.c | 361 +++++++++++++++++++ hw/ppc/spapr_vio.c | 2 +- include/exec/cpu-defs.h | 35 +- include/exec/cpu_ldst.h | 104 +++++- include/hw/boards.h | 1 + include/hw/pci-host/spapr.h | 7 + include/hw/pci/pci.h | 6 + include/hw/ppc/spapr.h | 59 +++- include/hw/ppc/spapr_drc.h | 201 +++++++++++ include/qemu-common.h | 6 + target-ppc/kvm.c | 17 + target-ppc/kvm_ppc.h | 5 + tcg/aarch64/tcg-target.h | 1 + tcg/arm/tcg-target.h | 1 + tcg/i386/tcg-target.h | 1 + tcg/ia64/tcg-target.h | 2 + tcg/mips/tcg-target.h | 1 + tcg/ppc/tcg-target.h | 1 + tcg/s390/tcg-target.h | 1 + tcg/sparc/tcg-target.h | 1 + tcg/tcg.h | 4 +- tcg/tci/tcg-target.h | 4 +- trace-events | 4 + vl.c | 30 +- 37 files changed, 2704 insertions(+), 223 deletions(-) create mode 100644 docs/specs/ppc-spapr-hotplug.txt create mode 100644 hw/ppc/spapr_drc.c create mode 100644 include/hw/ppc/spapr_drc.h