Re: [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH

[Julien Grall <julien@xen.org>]

Hi Jan,

On 18/05/2021 16:13, Jan Beulich wrote:
> On 18.05.2021 16:06, Julien Grall wrote:
>>
>>
>> On 18/05/2021 07:27, Jan Beulich wrote:
>>> On 18.05.2021 06:11, Connor Davis wrote:
>>>>
>>>> On 5/17/21 9:42 AM, Julien Grall wrote:
>>>>> Hi Jan,
>>>>>
>>>>> On 17/05/2021 12:16, Jan Beulich wrote:
>>>>>> On 14.05.2021 20:53, Connor Davis wrote:
>>>>>>> --- a/xen/common/memory.c
>>>>>>> +++ b/xen/common/memory.c
>>>>>>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned
>>>>>>> long gmfn)
>>>>>>>         p2m_type_t p2mt;
>>>>>>>     #endif
>>>>>>>         mfn_t mfn;
>>>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>>         bool *dont_flush_p, dont_flush;
>>>>>>> +#endif
>>>>>>>         int rc;
>>>>>>>       #ifdef CONFIG_X86
>>>>>>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d,
>>>>>>> unsigned long gmfn)
>>>>>>>          * Since we're likely to free the page below, we need to suspend
>>>>>>>          * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes.
>>>>>>>          */
>>>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>>         dont_flush_p = &this_cpu(iommu_dont_flush_iotlb);
>>>>>>>         dont_flush = *dont_flush_p;
>>>>>>>         *dont_flush_p = false;
>>>>>>> +#endif
>>>>>>>           rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>>>>>>>     +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>>         *dont_flush_p = dont_flush;
>>>>>>> +#endif
>>>>>>>           /*
>>>>>>>          * With the lack of an IOMMU on some platforms, domains with
>>>>>>> DMA-capable
>>>>>>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d,
>>>>>>> struct xen_add_to_physmap *xatp,
>>>>>>>         xatp->gpfn += start;
>>>>>>>         xatp->size -= start;
>>>>>>>     +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>>         if ( is_iommu_enabled(d) )
>>>>>>>         {
>>>>>>>            this_cpu(iommu_dont_flush_iotlb) = 1;
>>>>>>>            extra.ppage = &pages[0];
>>>>>>>         }
>>>>>>> +#endif
>>>>>>>           while ( xatp->size > done )
>>>>>>>         {
>>>>>>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d,
>>>>>>> struct xen_add_to_physmap *xatp,
>>>>>>>             }
>>>>>>>         }
>>>>>>>     +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>>         if ( is_iommu_enabled(d) )
>>>>>>>         {
>>>>>>>             int ret;
>>>>>>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d,
>>>>>>> struct xen_add_to_physmap *xatp,
>>>>>>>             if ( unlikely(ret) && rc >= 0 )
>>>>>>>                 rc = ret;
>>>>>>>         }
>>>>>>> +#endif
>>>>>>>           return rc;
>>>>>>>     }
>>>>>>
>>>>>> I wonder whether all of these wouldn't better become CONFIG_X86:
>>>>>> ISTR Julien indicating that he doesn't see the override getting used
>>>>>> on Arm. (Julien, please correct me if I'm misremembering.)
>>>>>
>>>>> Right, so far, I haven't been in favor to introduce it because:
>>>>>      1) The P2M code may free some memory. So you can't always ignore
>>>>> the flush (I think this is wrong for the upper layer to know when this
>>>>> can happen).
>>>>>      2) It is unclear what happen if the IOMMU TLBs and the PT contains
>>>>> different mappings (I received conflicted advice).
>>>>>
>>>>> So it is better to always flush and as early as possible.
>>>>
>>>> So keep it as is or switch to CONFIG_X86?
>>>
>>> Please switch, unless anyone else voices a strong opinion towards
>>> keeping as is.
>>
>> I would like to avoid adding more #ifdef CONFIG_X86 in the common code.
>> Can we instead provide a wrapper for them?
> 
> Doable, sure, but I don't know whether Connor is up to going this
> more extensive route.

That's a fair point. If that the case, then I prefer the #ifdef 
CONFIG_HAS_PASSTHROUGH version.

I can add an item in my todo list to introduce some helpers.

Cheers,

-- 
Julien Grall