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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.16 at 10:04, <quan.xu@xxxxxxxxx> wrote:
> On May 12, 2016 11:06 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >>> On 12.05.16 at 16:28, <quan.xu@xxxxxxxxx> wrote:
>> > On May 10, 2016 2:54 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >> >>> 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:
> Try it again, I hope I have got it. If not, could you write some sample code 
> for me as an exception? :)

Well, this would be acceptable (albeit not ideal) to me as a first
cut (and instead of continuing this apparently fruitless discussion
I'd then see to provide a follow-up patch improving it), but won't
(I'm afraid) please Kevin: You now again don't log anything for
DomU-s. That's one half of the "not ideal" part; the other is that
of two far apart events for Dom0, only the first one would get
logged.

In any event there's stylistic cleanup necessary:

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -240,21 +240,63 @@ int iommu_map_page(struct domain *d, unsigned long gfn, 
> unsigned long mfn,
>                     unsigned int flags)
>  {
>      const struct domain_iommu *hd = dom_iommu(d);
> +    static int printed = 0;

This doesn't need an initializer, should be bool_t, and should get
moved ...

> +    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) )
> +    {
> +        if ( is_hardware_domain(d) )
> +        {

... here.

> +            if ( !printed )
> +            {
> +                printk(XENLOG_ERR
> +                       "d%d: IOMMU mapping gfn %#lx mfn %#lx failed %d.",
> +                       d->domain_id, gfn, mfn, rc);
> +
> +                printed = 1;
> +            }
> +        }
> +        else
> +            domain_crash(d);
> +    }

What I think might at least come close to what Kevin and I would
like to see is something like

        if ( !d->is_shutting_down && printk_ratelimit() )
                printk(...);
        if ( !is_hardware_domain(d) )
                domain_crash(d);

For Dom0 that'll still be more verbose than we'd really like, but it
at least wouldn't outright flood the console.

Jan


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