On 09.03.2022 11:08, Rahul Singh wrote:
> Hi Jan,
>
>> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeul...@suse.com> wrote:
>>
>> On 03.03.2022 17:31, Rahul Singh wrote:
>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeul...@suse.com> wrote:
>>>> On 01.03.2022 14:34, Rahul Singh wrote:
>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeul...@suse.com> 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?
Jan
