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

Re: [PATCH v2 5/6] x86/iommu: switch hwdom IOMMU to use a rangeset


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 5 Dec 2023 16:47:15 +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, 05 Dec 2023 15:47:27 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 05.12.2023 16:43, Roger Pau Monné wrote:
> On Tue, Dec 05, 2023 at 04:27:05PM +0100, Jan Beulich wrote:
>> On 04.12.2023 10:43, Roger Pau Monne wrote:
>>> @@ -476,58 +406,55 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain 
>>> *d)
>>>      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);
>>> +    if ( iommu_hwdom_inclusive )
>>> +    {
>>> +        /* Add the whole range below 4GB, UNUSABLE regions will be 
>>> removed. */
>>> +        rc = rangeset_add_range(map, 0, max_pfn);
>>> +        if ( rc )
>>> +            panic("IOMMU inclusive mappings can't be added: %d\n",
>>> +                  rc);
>>> +    }
>>>  
>>> -    for ( i = 0, start = 0, count = 0; i < top; )
>>> +    for ( i = 0; i < e820.nr_map; i++ )
>>>      {
>>> -        unsigned long pfn = pdx_to_pfn(i);
>>> -        unsigned int perms = hwdom_iommu_map(d, pfn, max_pfn);
>>> +        struct e820entry entry = e820.map[i];
>>>  
>>> -        if ( !perms )
>>> -            /* nothing */;
>>> -        else if ( paging_mode_translate(d) )
>>> +        switch ( entry.type )
>>>          {
>>> -            int rc;
>>> +        case E820_UNUSABLE:
>>> +            if ( !iommu_hwdom_inclusive || PFN_DOWN(entry.addr) > max_pfn )
>>> +                continue;
>>
>> The !iommu_hwdom_inclusive part isn't really needed here, is it? The ...
> 
> Nor the PFN_DOWN(entry.addr) > max_pfn.

Hmm, I couldn't convince myself that could also be dropped.

>>> -            rc = p2m_add_identity_entry(d, pfn,
>>> -                                        perms & IOMMUF_writable ? 
>>> p2m_access_rw
>>> -                                                                : 
>>> p2m_access_r,
>>> -                                        0);
>>> +            rc = rangeset_remove_range(map, PFN_DOWN(entry.addr),
>>> +                                       PFN_DOWN(entry.addr + entry.size - 
>>> 1));
>>
>> ... call here would then simply be a no-op, as it looks. And things would
>> overall look more safe if the removal was skipped for fewer reasons.
> 
> OK, the removal can be done unconditionally if so desired.
> 
>>> @@ -605,7 +532,7 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain 
>>> *d)
>>>      rangeset_destroy(map);
>>>  
>>>      /* Use if to avoid compiler warning */
>>> -    if ( iommu_iotlb_flush_all(d, flush_flags) )
>>> +    if ( iommu_iotlb_flush_all(d, map_data.flush_flags) )
>>>          return;
>>>  }
>>
>> Ah yes, here is said change. But I think for correctness this wants
>> moving to the earlier patch.
> 
> OK, so something like:
> 
> map_data.flush_flags |= flush_flags;

Or simply drop flush_flags here right away (read: replace by map.flush_flags).

Jan

> And adjusting the iommu_iotlb_flush_all() would be fine in this patch
> context.
> 
> Thanks, Roger.




 


Rackspace

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