Re: [Xen-devel] [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.16 at 16:26, <quan.xu@xxxxxxxxx> wrote:
> On April 25, 2016 5:27 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >>> On 18.04.16 at 16:00, <quan.xu@xxxxxxxxx> wrote:
>> > --- a/xen/drivers/passthrough/iommu.c
>> > +++ b/xen/drivers/passthrough/iommu.c
>> > @@ -243,21 +243,33 @@ int iommu_map_page(struct domain *d,
>> unsigned long gfn, unsigned long mfn,
>> >                     unsigned int flags)  {
>> >      struct hvm_iommu *hd = domain_hvm_iommu(d);
>> > +    int rc;
>> >
>> >      if ( !iommu_enabled || !hd->platform_ops )
>> >          return 0;
>> >
>> > -    return hd->platform_ops->map_page(d, gfn, mfn, flags);
>> > +    rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
>> > +
>> > +    if ( rc && !is_hardware_domain(d) )
>> > +        domain_crash(d);
>> > +
>> > +    return rc;
>> >  }
>> 
>> As said before - letting this go completely silently for the hardware domain 
> is
>> bad. At least the first instance of such an event needs a message to be 
> logged.
>> Advanced variants where a message gets logged once in a while if the issue 
> re-
>> occurs would be nice, but aren't strictly necessary imo. And note that even
>> logging all occurrences would not be a security issue, but just a usability 
> one
>> (but I still recommend against this).
>> 
> 
> IMO the IOMMU map/unmap failures  are the small probability events.  I think 
> the log message wouldn't 
> spam log message.

That's when things work right. The case to consider is when something
goes wrong, leading to a myriad of failures. To see what went wrong
first it is necessary to not write overly many messages when many
failures occur in close succession.

> I'd like modify it as following:
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -243,11 +243,24 @@ int iommu_map_page(struct domain *d, unsigned long gfn, 
> unsigned long mfn,
>                     unsigned int flags)
>  {
>      struct hvm_iommu *hd = domain_hvm_iommu(d);
> +    int rc;
> 
>      if ( !iommu_enabled || !hd->platform_ops )
>          return 0;
> 
> -    return hd->platform_ops->map_page(d, gfn, mfn, flags);
> +    rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
> +
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR
> +               "iommu_map_page: IOMMU mapping gfn %#lx mfn %#lx failed for 
> dom%d.",
> +               gfn, mfn, d->domain_id);
> +
> +        if ( !is_hardware_domain(d) )
> +            domain_crash(d);
> +    }
> +
> +    return rc;
>  }

That's okay as long as no code path would ever trigger hundreds
of these messages before getting back to the pointer where the
now dead domain gets unscheduled. And note that this can't just
be "I don't think this can happen", but it needs to be guaranteed,
or else you introduce a security issue.

> btw, in case  I have print-out for mapping failure, I think it is no need  
> to print out an extra message for the hardware domain.
> If this is still not what you want, I'll continue to enhance it. Also I am 
> not good at description, so I try to send out code modification directly. 

I.e. I continue to think that

    if ( is_hardware_domain() )
        printk();
    else
        domain_crash();

is the better approach (and still subject to preventing spamming
the log as per the first part of my reply).

Jan


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