Re: [Xen-devel] [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.04.14 at 13:51, <roger.pau@xxxxxxxxxx> wrote:
> On 04/04/14 17:35, Jan Beulich wrote:
>>>>> On 04.04.14 at 16:37, <roger.pau@xxxxxxxxxx> wrote:
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -749,6 +749,22 @@ __gnttab_map_grant_ref(
>>>              goto undo_out;
>>>          }
>>>      }
>>> +    else if ( need_iommu(ld) )
>>> +    {
>>> +        int err;
>>> +
>>> +        BUG_ON(!paging_mode_translate(ld));
>>> +        err = iommu_map_page(ld, op->host_addr >> PAGE_SHIFT, frame,
>>> +                             op->flags & GNTMAP_readonly ?
>>> +                                IOMMUF_readable :
>>> +                                IOMMUF_readable|IOMMUF_writable);
>>> +        if ( err )
>>> +        {
>>> +            double_gt_unlock(lgt, rgt);
>>> +            rc = GNTST_general_error;
>>> +            goto undo_out;
>>> +        }
>>> +    }
>> 
>> As much of this as possible should be folded with the if() branch.
>> And looking at the PV code, I think it makes no sense for the
>> conditions whether to map the page r/o or r/w to be different
>> between PV and non-PV.
>> 
>> Plus - wouldn't it be better to have this taken care of via
>> create_grant_host_mapping(), by not blindly calling
>> iommu_unmap_page() on anything other than p2m_ram_rw in
>> ept_set_entry() and p2m_set_entry(), the more that this should
>> already be taken care of in the iommu_hap_pt_share case.
> 
> Thanks for the comment, it indeed makes much more sense to fix this in 
> ept_set_entry/p2m_set_entry, the following patch adds IOMMU support for 
> all page types except p2m_mmio_direct (when the hap memory map is not 
> shared with IOMMU):

This seems to be going a little too far - I'm not sure we want to include
all types here: p2m_mmio_dm, p2m_ram_paging_*, p2m_ram_paged,
and p2m_ram_broken all might require different treatment. Please do
this via some sort of switch() setting the permissions instead, calling
iommu_unmap_page() when the permissions remain zero.

Jan

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -475,18 +475,22 @@ out:
>              iommu_pte_flush(d, gfn, (u64*)ept_entry, order, 
> vtd_pte_present);
>          else
>          {
> -            if ( p2mt == p2m_ram_rw )
> +            if ( (p2mt != p2m_invalid) && (p2mt != p2m_mmio_direct) )
>              {
> +                unsigned int flags;
> +
> +                flags = ( (p2mt == p2m_grant_map_ro) || (p2mt == 
> p2m_ram_ro) ) ?
> +                        IOMMUF_readable : IOMMUF_readable | 
> IOMMUF_writable;
>                  if ( order > 0 )
>                  {
>                      for ( i = 0; i < (1 << order); i++ )
>                          iommu_map_page(
>                              p2m->domain, gfn - offset + i, mfn_x(mfn) - 
> offset 
> + i,
> -                            IOMMUF_readable | IOMMUF_writable);
> +                            flags);
>                  }
>                  else if ( !order )
>                      iommu_map_page(
> -                        p2m->domain, gfn, mfn_x(mfn), IOMMUF_readable | 
> IOMMUF_writable);
> +                        p2m->domain, gfn, mfn_x(mfn), flags);
>              }
>              else
>              {
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index 766fd67..c564cb6 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -450,11 +450,15 @@ p2m_set_entry(struct p2m_domain *p2m, unsigned long 
> gfn, mfn_t mfn,
>          }
>          else
>          {
> -            if ( p2mt == p2m_ram_rw )
> +            if ( (p2mt != p2m_invalid) && (p2mt != p2m_mmio_direct) )
> +            {
> +                unsigned int flags;
> +
> +                flags = ( (p2mt == p2m_grant_map_ro) || (p2mt == 
> p2m_ram_ro) ) ?
> +                        IOMMUF_readable : IOMMUF_readable | 
> IOMMUF_writable;
>                  for ( i = 0; i < (1UL << page_order); i++ )
> -                    iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i,
> -                                   IOMMUF_readable|IOMMUF_writable);
> -            else
> +                    iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, flags);
> +            } else
>                  for ( int i = 0; i < (1UL << page_order); i++ )
>                      iommu_unmap_page(p2m->domain, gfn+i);
>          }



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel