Re: [V8 PATCH 5/8] pvh dom0: Add and remove foreign pages

["Jan Beulich" <JBeulich@suse.com>]

>>> On 08.04.14 at 03:11, <mukesh.rathor@oracle.com> wrote:
> On Mon, 07 Apr 2014 07:57:56 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
>> In the end I think we'll want to make them all report proper error
>> codes...
> 
> Ok, how about the following patch then? If it's OK, I'd like to submit
> independently.

Yes, I just now stumbled across this oddity too. Looks generally
like what we want - a couple of comments below.

> @@ -696,7 +695,7 @@ long arch_do_domctl(
>  
>              if ( paging_mode_translate(d) )
>                  for ( i = 0; i < nr_mfns; i++ )
> -                    add |= !clear_mmio_p2m_entry(d, gfn + i);
> +                    add |= !!clear_mmio_p2m_entry(d, gfn + i);

No need for the double !!, and in fact ...

>              ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
>              if ( !ret && add )
>                  ret = -EIO;

... you should be propagating the error code here anyway rather
than making one up.

> @@ -124,15 +124,16 @@ nestedhap_fix_p2m(struct vcpu *v, struct p2m_domain *p2m,
>          gfn = (L2_gpa >> PAGE_SHIFT) & mask;
>          mfn = _mfn((L0_gpa >> PAGE_SHIFT) & mask);
>  
> -        rv = set_p2m_entry(p2m, gfn, mfn, page_order, p2mt, p2ma);
> +        rc = set_p2m_entry(p2m, gfn, mfn, page_order, p2mt, p2ma);
>      }
>  
>      p2m_unlock(p2m);
>  
> -    if (rv == 0) {
> +    if ( rc )
> +    {
>          gdprintk(XENLOG_ERR,
> -		"failed to set entry for %#"PRIx64" -> %#"PRIx64"\n",
> -		L2_gpa, L0_gpa);
> +		"failed to set entry for %#"PRIx64" -> %#"PRIx64" rc:%d\n",
> +		L2_gpa, L0_gpa, rc);

Together with the other coding style cleanup you should also get rid
of the hard tabs here.

> @@ -769,26 +769,28 @@ set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
>      }
>  
>      P2M_DEBUG("set mmio %lx %lx\n", gfn, mfn_x(mfn));
> -    rc = set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_mmio_direct, p2m->default_access);
> +    rc = set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_mmio_direct,
> +                       p2m->default_access);
>      gfn_unlock(p2m, gfn, 0);
> -    if ( 0 == rc )
> +    if ( rc )
>          gdprintk(XENLOG_ERR,
> -            "set_mmio_p2m_entry: set_p2m_entry failed! mfn=%08lx\n",
> -            mfn_x(get_gfn_query_unlocked(p2m->domain, gfn, &ot)));
> +            "set_mmio_p2m_entry: set_p2m_entry failed! mfn=%08lx rc:%d\n",
> +            mfn_x(get_gfn_query_unlocked(p2m->domain, gfn, &ot)), rc);

Again, if you already alter this and do minor cleanup at once, this
should become

	printk(XENLOG_G_ERR "d%d: ...", ...);

or the "set_mmio_p2m_entry:" prefix should be dropped.

> @@ -835,12 +839,13 @@ set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
>          set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
>  
>      P2M_DEBUG("set shared %lx %lx\n", gfn, mfn_x(mfn));
> -    rc = set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_ram_shared, p2m->default_access);
> +    rc = set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_ram_shared, 
> +                       p2m->default_access);
>      gfn_unlock(p2m, gfn, 0);
> -    if ( 0 == rc )
> +    if ( rc )
>          gdprintk(XENLOG_ERR,
> -            "set_shared_p2m_entry: set_p2m_entry failed! mfn=%08lx\n",
> -            mfn_x(get_gfn_query_unlocked(p2m->domain, gfn, &ot)));
> +            "set_shared_p2m_entry: set_p2m_entry failed! mfn=%08lx rc:%d\n",
> +            mfn_x(get_gfn_query_unlocked(p2m->domain, gfn, &ot)), rc);

Similarly here.

Jan