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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.05.16 at 05:41, <quan.xu@xxxxxxxxx> wrote:
> On May 10, 2016 12:14 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >>> On 06.05.16 at 10:54, <quan.xu@xxxxxxxxx> wrote:
>> > --- a/xen/drivers/passthrough/iommu.c
>> > +++ b/xen/drivers/passthrough/iommu.c
>> > @@ -240,21 +240,47 @@ int iommu_map_page(struct domain *d,
>> unsigned long gfn, unsigned long mfn,
>> >                     unsigned int flags)  {
>> >      const struct domain_iommu *hd = dom_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 ( unlikely(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);
>> > +    }
>> 
>> This still may spam the console in at least the case of Dom0.
> 
> I am afraid we may need a minor trade-off. What about:
> 
>        dprintk(XENLOG_ERR, "...");
> 
> to print out in debug mode.

And be silent in non-debug mode? That's not what we want.

>>  For DomU I'd
>> really expect you to state in the commit message why no spamming can occur
>> (of course assuming it really can't, which I'm not convinced of).
>>
> 
> In this v4, I think we will still spam the console in extreme cases :(:(..
> 
> For mapping:
> +                ret = iommu_map_page();
> +                if ( unlikely(ret) )
> +                {
> +                    while ( i-- )
> +                        iommu_unmap_page();
> +                }
> 
> We'll  stop map against any error and unmapping the previous mappings.  The 
> extreme case is error for unmapping the previous mappings.
> 
> Again -- I think dprintk is a better solution. Any suggestion?

For DomU the solution seems quite obvious: Only log a message if
the domain is not already marked crashed. For Dom0 you'll need to
get a little more creative (but by leveraging the fact that there's
only one in the system, this can't be too difficult a problem to solve:
e.g. "manually" rate limit these messages - see printk_ratelimit() et
al).

Jan


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