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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

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.

Regards,
Rahul 

> Jan