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

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

[Thomas Huth]

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);
         return NULL;
     }
 
     QLIST_FOREACH(tcet, &spapr_tce_tables, list) {
-        if (tcet->liobn == liobn) {
+        if (tcet->liobn == (uint32_t)liobn) {
             return tcet;
         }
     }
-- 
1.8.3.1

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]

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.

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.

>          return NULL;
>      }
>  
>      QLIST_FOREACH(tcet, &spapr_tce_tables, list) {
> -        if (tcet->liobn == liobn) {
> +        if (tcet->liobn == (uint32_t)liobn) {
>              return tcet;
>          }
>      }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Thomas Huth]

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