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

From: Jan Beulich <jbeulich@suse.com>
To: Rahul Singh <Rahul.Singh@arm.com>
Cc: "Bertrand Marquis" <Bertrand.Marquis@arm.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Paul Durrant" <paul@xen.org>
Subject: Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file
Date: Wed, 9 Mar 2022 11:17:41 +0100	[thread overview]
Message-ID: <35ad9273-956b-86d6-55a3-c737575a7880@suse.com> (raw)
In-Reply-To: <1205992D-ACE4-4E94-B554-3B14F6D9697E@arm.com>

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