From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH v1 for-4.6 1/2] xen/pvh: check permissions
 when adding MMIO regions
Date: Fri, 19 Dec 2014 09:06:33 +0000
Message-ID: <5493F8A90200007800050E8F@mail.emea.novell.com>
References: <1418927225-60266-1-git-send-email-roger.pau@citrix.com>
	<1418927225-60266-2-git-send-email-roger.pau@citrix.com>
	<54932565.2020801@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta4.messagelabs.com ([85.158.143.247])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <JBeulich@suse.com>) id 1Y1tW8-00050T-16
	for xen-devel@lists.xenproject.org; Fri, 19 Dec 2014 09:06:36 +0000
In-Reply-To: <54932565.2020801@citrix.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>, Roger Pau Monne <roger.pau@citrix.com>
Cc: xen-devel@lists.xenproject.org
List-Id: xen-devel@lists.xenproject.org

>>> On 18.12.14 at 20:05, <andrew.cooper3@citrix.com> wrote:
> On 18/12/14 18:27, Roger Pau Monne wrote:
>> --- a/xen/arch/x86/domain_build.c
>> +++ b/xen/arch/x86/domain_build.c
>> @@ -312,16 +312,47 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
>>                                         unsigned long mfn, unsigned long nr_mfns)
>>  {
>>      unsigned long i;
>> +    xenmem_access_t def_access;
>> +    bool_t r_only = false;
>>      int rc;
>>  
>>      for ( i = 0; i < nr_mfns; i++ )
>>      {
>> +        if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
>> +            goto next;
>> +
>> +        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) && !r_only )
>> +        {
>> +            rc = p2m_get_mem_access(d, ~0ul, &def_access);
>> +            BUG_ON(rc);
>> +            /* Set default access to read-only */
>> +            rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, XENMEM_access_r);
>> +            BUG_ON(rc);
>> +            r_only = true;
>> +        }
>> +        else if ( r_only )
>> +        {
>> +            /* Set the default back */
>> +            rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, def_access);
>> +            BUG_ON(rc);
>> +            r_only = false;
>> +        }
> 
> Am I missing something obvious, or would all this r_only juggling be far
> more easy if set_mmio_p2m_entry() had a ro/rw boolean parameter?
> 
> As these entries are done one at a time, it would seem to be far less
> error prone to explicitly choose a read-only or read-write mmio mapping,
> rather than playing with the entire default.

+1

>> +
>>          if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i))) )
>>              panic("pvh_add_mem_mapping: gfn:%lx mfn:%lx i:%ld rc:%d\n",
>>                    gfn, mfn, i, rc);
>> +
>> + next:
>>          if ( !(i & 0xfffff) )
>>                  process_pending_softirqs();
> 
> You could remove the next label by moving this clause to the top and
> adding an i != 0 check.

Or really just make the respective goto a continue - we know we
don't hide overly large regions from Dom0.

Jan