Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.03.2022 16:50, Rahul Singh wrote:
>> On 9 Mar 2022, at 10:17 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 09.03.2022 11:08, Rahul Singh wrote:
>>>> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 03.03.2022 17:31, Rahul Singh wrote:
>>>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>>> On 01.03.2022 14:34, Rahul Singh wrote:
>>>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>>>>> On 15.02.2022 16:25, Rahul Singh wrote:
>>>>>>>>> --- a/xen/arch/x86/hvm/vmsi.c
>>>>>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>>>>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix 
>>>>>>>>> *msix)
>>>>>>>>>
>>>>>>>>>  return 0;
>>>>>>>>> }
>>>>>>>>> +
>>>>>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
>>>>>>>>> +{
>>>>>>>>> +    struct domain *d = pdev->domain;
>>>>>>>>> +    unsigned int i;
>>>>>>>>> +
>>>>>>>>> +    if ( !pdev->vpci->msix )
>>>>>>>>> +        return 0;
>>>>>>>>> +
>>>>>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
>>>>>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
>>>>>>>>> +    {
>>>>>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, 
>>>>>>>>> i));
>>>>>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) 
>>>>>>>>> +
>>>>>>>>> +                                     vmsix_table_size(pdev->vpci, i) 
>>>>>>>>> - 1);
>>>>>>>>> +
>>>>>>>>> +        for ( ; start <= end; start++ )
>>>>>>>>> +        {
>>>>>>>>> +            p2m_type_t t;
>>>>>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
>>>>>>>>> +
>>>>>>>>> +            switch ( t )
>>>>>>>>> +            {
>>>>>>>>> +            case p2m_mmio_dm:
>>>>>>>>> +            case p2m_invalid:
>>>>>>>>> +                break;
>>>>>>>>> +            case p2m_mmio_direct:
>>>>>>>>> +                if ( mfn_x(mfn) == start )
>>>>>>>>> +                {
>>>>>>>>> +                    clear_identity_p2m_entry(d, start);
>>>>>>>>> +                    break;
>>>>>>>>> +                }
>>>>>>>>> +                /* fallthrough. */
>>>>>>>>> +            default:
>>>>>>>>> +                put_gfn(d, start);
>>>>>>>>> +                gprintk(XENLOG_WARNING,
>>>>>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
>>>>>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO 
>>>>>>>>> area\n",
>>>>>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
>>>>>>>>> +                return -EEXIST;
>>>>>>>>> +            }
>>>>>>>>> +            put_gfn(d, start);
>>>>>>>>> +        }
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    return 0;
>>>>>>>>> +}
>>>>>>>>
>>>>>>>> ... nothing in this function looks to be x86-specific, except maybe
>>>>>>>> functions like clear_identity_p2m_entry() may not currently be 
>>>>>>>> available
>>>>>>>> on Arm. But this doesn't make the code x86-specific.
>>>>>>>
>>>>>>> I will maybe be wrong but what I understand from the code is that for 
>>>>>>> x86 
>>>>>>> if there is no p2m entries setup for the region, accesses to them will 
>>>>>>> be trapped 
>>>>>>> into the hypervisor and can be handled by specific MMIO handler.
>>>>>>>
>>>>>>> But for ARM when we are registering the MMIO handler we have to provide 
>>>>>>> the GPA also for the MMIO handler. 
>>>>>>
>>>>>> Question is: Is this just an effect resulting from different 
>>>>>> implementation,
>>>>>> or an inherent requirement? In the former case, harmonizing things may 
>>>>>> be an
>>>>>> alternative option.
>>>>>
>>>>> This is an inherent requirement to provide a GPA when registering the 
>>>>> MMIO handler.
>>>>
>>>> So you first say yes to my "inherent" question, but then ...
>>>>
>>>>> For x86 msix mmio handlers is registered in init_msix(..) function as 
>>>>> there is no requirement
>>>>> on x86 to provide GPA when registering the handler. Later point of time 
>>>>> when BARs are configured
>>>>> and memory decoding bit is enabled vpci_make_msix_hole() will clear the 
>>>>> identity mapping for msix
>>>>> base table address so that access to msix tables will be trapped.
>>>>>
>>>>> On ARM we need to provide GPA to register the mmio handler and MSIX table 
>>>>> base
>>>>> address is not valid when init_msix() is called as BAR will be configured 
>>>>> later point in time.
>>>>> Therefore on ARM mmio handler will be registered in function 
>>>>> vpci_make_msix_hole() when
>>>>> memory decoding bit is enabled.
>>>>
>>>> ... you explain it's an implementation detail. I'm inclined to
>>>> suggest that x86 also pass the GPA where possible. Handler lookup
>>>> really would benefit from not needing to iterate over all registered
>>>> handlers, until one claims the access. The optimization part of this
>>>> of course doesn't need to be done right here, but harmonizing
>>>> register_mmio_handler() between both architectures would seem to be
>>>> a reasonable prereq step.
>>>
>>> I agree with you that if we modify the register_mmio_handler() for x86 to 
>>> pass GPA
>>> we can have the common code for x86 and ARM and also we can optimize the 
>>> MMIO
>>> trap handling for x86.
>>>
>>> What I understand from the code is that modifying the 
>>> register_mmio_handler() for
>>> x86 to pass GPA requires a lot of effort and testing.
>>>
>>> Unfortunately, I have another high priority task that I have to complete I 
>>> don’t have time
>>> to optimise the register_mmio_handler() for x86 at this time.
>>
>> Actually making use of the parameter is nothing I would expect you to
>> do. But is just adding the extra parameter similarly out of scope for
>> you?
>>
> 
> If I understand correctly you are asking to make register_mmio_handler() 
> declaration
> same for x86 and ARM. You are not asking to modify the x86 MMIO handler logic 
> to
> use GPA to find the handler?

Yes, but ...

> As Roger also mentioned that vpci_make_msix_hole() is required only for x86 
> to clear
> the identity mapping. If we make the vpci_make_msix_hole() arch-specific 
> there is no
> need to modify the parameter for register_mmio_handler(), as for x86 and ARM
> register_mmio_handler() will be called in different places.

... with Roger agreeing with this plan, that other alternative is
likely dead now. Provided other stuff which isn't obviously arch-
specific remains in common code.

Jan