Re: [PATCH v9 04/14] arch/arm: unmap partially-mapped I/O-memory regions

[Julien Grall <julien.grall@linaro.org>]

On 07/03/2014 03:40 PM, Ian Campbell wrote:
> On Wed, 2014-07-02 at 20:42 +0200, Arianna Avanzini wrote:
>> This commit changes the function apply_p2m_changes() to unwind changes
>> performed while mapping an I/O-memory region, if errors are seen during
>> the operation. This is useful to avoid that I/O-memory areas are
>> partially accessible to guests.
> 
> I think it won't quite unwind, since it will actually destroy whatever
> was there before we tried to put something new.
> 
> Is there anything here which is specific to I/O memory areas? It seems
> to affect all mapping types.

I think it's fine to unmap for mapping types.

>> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
>> Cc: Dario Faggioli <dario.faggioli@citrix.com>
>> Cc: Paolo Valente <paolo.valente@unimore.it>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Julien Grall <julien.grall@citrix.com>
>> Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Tim Deegan <tim@xen.org>
>> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: Eric Trudeau <etrudeau@broadcom.com>
>> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>>
>> ---
>>
>>     v9:
>>         - Let apply_p2m_ranges() unwind its own progress instead of relying on
>>           the caller to unmap partially-mapped I/O-memory regions.
>>
>> ---
>>  xen/arch/arm/p2m.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 22646e9..92fc4ec 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -314,7 +314,7 @@ static int apply_p2m_changes(struct domain *d,
>>      unsigned long cur_first_page = ~0,
>>                    cur_first_offset = ~0,
>>                    cur_second_offset = ~0;
>> -    unsigned long count = 0;
>> +    unsigned long count = 0, inserted = 0;
>>      unsigned int flush = 0;
>>      bool_t populate = (op == INSERT || op == ALLOCATE);
>>      lpae_t pte;
>> @@ -328,6 +328,7 @@ static int apply_p2m_changes(struct domain *d,
>>  
>>      spin_lock(&p2m->lock);
>>  
>> +unwind:
> 
> I think I'd prefer a recursive call to the use of goto.
> 
> In particular the use of goto will result in the overall function
> appearing to succeed, won't it? Because the REMOVE will have succeeded.
> 
>>      addr = start_gpaddr;
>>      while ( addr < end_gpaddr )
>>      {
>> @@ -338,7 +339,9 @@ static int apply_p2m_changes(struct domain *d,
>>              if ( !first )
>>              {
>>                  rc = -EINVAL;
>> -                goto out;
>> +                end_gpaddr = start_gpaddr + inserted * PAGE_SIZE;
> 
> Isn't start_gpaddr + inserted * PAGE_SIZE == addr at any given moment?
> 
> Or you could just recurse on the entire region, which is much simpler I
> think, especially when you consider that you are clearing the region
> rather than unwinding it.
> 
>> @@ -384,7 +389,9 @@ static int apply_p2m_changes(struct domain *d,
>>                                    flush_pt);
>>              if ( rc < 0 ) {
>>                  printk("p2m_populate_ram: L2 failed\n");
>> -                goto out;
>> +                end_gpaddr = start_gpaddr + inserted * PAGE_SIZE;
>> +                op = REMOVE;
>> +                goto unwind;
>>              }
> 
> What about the L3 ALLOCATE case? Or is you intention only to handle
> INSERT (but you partially handle ALLOCATE)?
> 
>>          }
>>  
>> @@ -441,6 +448,7 @@ static int apply_p2m_changes(struct domain *d,
>>                      pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr, t);
>>                      p2m_write_pte(&third[third_table_offset(addr)],
>>                                    pte, flush_pt);
>> +                    inserted++;
>>                  }
>>                  break;
>>              case REMOVE:
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 


-- 
Julien Grall