[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 4/7] x86/iommu: introduce a rangeset to perform hwdom IOMMU setup


  • To: Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 19 Dec 2023 17:06:32 +0100
  • Autocrypt: addr=jbeulich@xxxxxxxx; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL
  • Cc: Paul Durrant <paul@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Tue, 19 Dec 2023 16:06:36 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 15.12.2023 15:18, Roger Pau Monne wrote:
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -370,10 +370,88 @@ static unsigned int __hwdom_init hwdom_iommu_map(const 
> struct domain *d,
>      return perms;
>  }
>  
> +struct map_data {
> +    struct domain *d;
> +    unsigned int flush_flags;
> +    bool mmio_ro;
> +};
> +
> +static int __hwdom_init cf_check identity_map(unsigned long s, unsigned long 
> e,
> +                                              void *data)
> +{
> +    struct map_data *info = data;
> +    struct domain *d = info->d;
> +    long rc;
> +
> +    if ( iommu_verbose )
> +        printk(XENLOG_INFO " [%010lx, %010lx] R%c\n",
> +               s, e, info->mmio_ro ? 'O' : 'W');
> +
> +    if ( paging_mode_translate(d) )
> +    {
> +        if ( info->mmio_ro )
> +        {
> +            ASSERT_UNREACHABLE();
> +            return 0;

Is this meant to be in line with the main return statement's comment?
I'm inclined to ask for an actual error code (-EOPNOTSUPP?) here.

> +        }
> +        while ( (rc = map_mmio_regions(d, _gfn(s), e - s + 1, _mfn(s))) > 0 )
> +        {
> +            s += rc;
> +            process_pending_softirqs();
> +        }
> +    }
> +    else
> +    {
> +        const unsigned int perms = IOMMUF_readable | IOMMUF_preempt |
> +                                   (info->mmio_ro ? 0 : IOMMUF_writable);
> +
> +        /*
> +         * Read-only ranges are only created based on the contents of mmio
> +         * read-only rangeset, and hence need the additional iomem 
> permissions
> +         * check.
> +         */
> +        while( info->mmio_ro && s <= e && !iomem_access_permitted(d, s, e) )

Nit: Missing blank after "while".

> +        {
> +            /*
> +             * Consume a frame per iteration until the reminder is accessible

Nit: remainder?

> +             * or there's nothing left to map.
> +             */
> +            if ( iomem_access_permitted(d, s, s) )
> +            {
> +                rc = iommu_map(d, _dfn(s), _mfn(s), 1, perms,
> +                               &info->flush_flags);
> +                if ( rc < 0 )
> +                    break;
> +                /* Must map a frame at least, which is what we request for. 
> */
> +                ASSERT(rc == 1);
> +                process_pending_softirqs();
> +            }
> +            s++;
> +        }
> +        while ( (rc = iommu_map(d, _dfn(s), _mfn(s), e - s + 1,
> +                                perms, &info->flush_flags)) > 0 )
> +        {
> +            s += rc;
> +            process_pending_softirqs();
> +        }
> +    }
> +    ASSERT(rc <= 0);
> +    if ( rc )
> +        printk(XENLOG_WARNING
> +               "IOMMU identity mapping of [%lx, %lx] failed: %ld\n",
> +               s, e, rc);
> +
> +    /* Ignore errors and attempt to map the remaining regions. */
> +    return 0;
> +}
> +
>  void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>  {
>      unsigned long i, top, max_pfn, start, count;
>      unsigned int flush_flags = 0, start_perms = 0;
> +    struct rangeset *map;
> +    struct map_data map_data = { .d = d };
> +    int rc;
>  
>      BUG_ON(!is_hardware_domain(d));
>  
> @@ -397,6 +475,10 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>      if ( iommu_hwdom_passthrough )
>          return;
>  
> +    map = rangeset_new(NULL, NULL, 0);
> +    if ( !map )
> +        panic("IOMMU init: unable to allocate rangeset\n");
> +
>      max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
>      top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
>  
> @@ -451,8 +533,26 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>              goto commit;
>      }
>  
> +    if ( iommu_verbose )
> +        printk(XENLOG_INFO "%pd: identity mappings for IOMMU:\n", d);
> +
> +    rc = rangeset_report_ranges(map, 0, ~0UL, identity_map, &map_data);
> +    if ( rc )
> +        panic("IOMMU unable to create mappings: %d\n", rc);
> +    rangeset_destroy(map);
> +
> +    if ( is_pv_domain(d) )
> +    {
> +        map_data.mmio_ro = true;
> +        rc = rangeset_report_ranges(mmio_ro_ranges, 0, ~0UL, identity_map,
> +                                    &map_data);
> +        if ( rc )
> +            panic("IOMMU unable to create read-only mappings: %d\n", rc);
> +    }

As it stands identity_map() deliberately returns no error. Yet here
you panic() in case of receiving an error, despite that being impossible?

Also if we want/need to panic() here, can we avoid having two instances
of almost the same string literal in .rodata? Along the lines of

    rc = rangeset_report_ranges(map, 0, ~0UL, identity_map, &map_data);
    rangeset_destroy(map);
    if ( !rc && is_pv_domain(d) )
    {
        map_data.mmio_ro = true;
        rc = rangeset_report_ranges(mmio_ro_ranges, 0, ~0UL, identity_map,
                                    &map_data);
    }
    if ( rc )
        panic("IOMMU unable to create %smappings: %d\n",
              map_data.mmio_ro ? "read-only " : "", rc);

?

> +    map_data.flush_flags |= flush_flags;

So you decided to still keep the standalone "flush_flags" around. Is
there a particular reason?

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.