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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Roger,

> On 9 Mar 2022, at 4:06 pm, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> 
> On Wed, Mar 09, 2022 at 03:50:12PM +0000, Rahul Singh wrote:
>> Hi Jan,
>> 
>>> On 9 Mar 2022, at 10:17 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> 
>>> On 09.03.2022 11:08, Rahul Singh wrote:
>>>> Hi Jan,
>>>> 
>>>>> 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?
>> 
>> 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.
>> 
>> For x86 register_mmio_handler() will be called in init_msix() whereas for ARM
>> register_mmio_handler() will be called  in vpci_make_msix_hole(). In this 
>> case we
>> have to move the call to register_mmio_handler() also to arch-specific 
>> files. If we move
>> the register_mmio_handler()  to arch specific file there is no need to make 
>> the
>> function common.
> 
> So then for Arm you will need something akin to
> unregister_mmio_handler so the handler can be removed when memory
> decoding is disabled?
> 
> Or else you would keep adding new handlers every time the guest
> enables memory decoding for the device without having removed the
> stale ones?

Yes, when I will send the patches for ARM I will take care of this not to 
register the handler 
again if the memory decoding bit is changed. Before registering the handler 
will check 
if the handler is already for GPA if it is already registered no need to 
register.

Regards,
Rahul
> 
> Thanks, Roger.