Re: [Qemu-devel] [PATCH] hw/ppc/spapr_iommu: Fix the check for invalid upper bits in liobn

[Thomas Huth <thuth@redhat.com>]

Am Tue, 21 Apr 2015 10:24:48 +1000
schrieb David Gibson <david@gibson.dropbear.id.au>:

> On Mon, Apr 20, 2015 at 05:34:56PM +0200, Thomas Huth wrote:
> > The check "liobn & 0xFFFFFFFF00000000ULL" in spapr_tce_find_by_liobn()
> > is completely useless since liobn is only declared as an uint32_t
> > parameter. Fix this by using target_ulong instead (this is what most
> > of the callers of this function are using, too).
> > And while we're at it, change the error message printing into a proper
> > qemu_log_mask(LOG_GUEST_ERROR, ...) call so that it is also possible
> > to enable this warning without recompiling the binary.
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> > ---
> >  hw/ppc/spapr_iommu.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> > index f3990fd..cd26a06 100644
> > --- a/hw/ppc/spapr_iommu.c
> > +++ b/hw/ppc/spapr_iommu.c
> > @@ -41,18 +41,19 @@ enum sPAPRTCEAccess {
> >  
> >  static QLIST_HEAD(spapr_tce_tables, sPAPRTCETable) spapr_tce_tables;
> >  
> > -static sPAPRTCETable *spapr_tce_find_by_liobn(uint32_t liobn)
> > +static sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn)
> >  {
> >      sPAPRTCETable *tcet;
> >  
> >      if (liobn & 0xFFFFFFFF00000000ULL) {
> > -        hcall_dprintf("Request for out-of-bounds LIOBN 0x" TARGET_FMT_lx "\n",
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "Request for out-of-bounds LIOBN 0x" TARGET_FMT_lx "\n",
> >                        liobn);
> 
> I'd actually prefer to see this left for the time being.  The dprintf
> stuff may be ugly, but it's used throughout this stuff, whereas the
> qemu logging is not.  Plus I've found the qemu logging stuff (as
> opposed to the tracepoint stuff) to be a pain to actually use.

I used it a couple of times already and once you get used to it, I
think it's quite useful. IMHO it's at least easier than to recompile
the binary for detecting such issues.

> So I'd prefer that you just fix the mask bug, and we look at a
> different patch to change the logging of the spapr stuff en masse.

Ok, fair, I'll change my patch accordingly.

 Thomas