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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

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.

If you are ok if we can make vpci_make_msix_hole() function arch-specific 
something like
vpci_msix_arch_check_mmio() and get this patch merged.

Regards,
Rahul
> 
> I'm adding Paul to Cc in case he wants to comment, as this would
> touch his territory on the x86 side.
> 
> Jan
>