[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Rahul Singh <Rahul.Singh@xxxxxxx>
  • Date: Wed, 9 Mar 2022 15:50:12 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Z5Nvwym6EwBCaonhBcDZqUhNcEBgme2f9STe12xdYp4=; b=Cim1xvN9K+sAB7XM4WFAnSjxldeNnc0QgfJdALrxkaVwMVQJFJAZ7yldIWjdSkZSjqftyrUH/LMPAn74bsHSBjlSgB4efZjjUN9ECtY2eXJiw8bhfaDR6K6jEGUnt3n80VO4TsltrRBakU1Emf14SEuWL50U54obBqxAPp3FgLmDk0dl8JlEVYMe39Pu9fGD/55Lmn4H5tGYbZZn7vvPoMNESXNsGbQC2jerc3xYblwO0sE8bEEvgcf+sACVDd3EErJ7hOcPfZUWuIP3jz9E4iniM9kMTKDvOobFJYmF1xAZgr5bvNVALxWHqaxbfo6AwK6VamYcDoxdjqTAhMsqjw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=MM1SpwYrbn0OBI7xBcBTb6kZVcl3/lhCnyGrhnZ0CunvThPp2QKZY57MsfVRHvMgy3Mlp/1himSVdBa14GeaLInvWBLpoR3Dgsrb3NlVOKprZzNekur0QAMXChr0bkdPResrgM1wPbPGpJmTx15CREVnbcM/Oo8cC8WBZ1m3RHYS24KwdxL/BtS0UL+8ySzfZQvIjVktarshaMFlAPYC2/8r85Xm9+rrXwl4DQGwUEKEHlW2kHZkAYqgt0s2yCDGfF86frgWuJjfT3mi031QYwaojCN1q+Yh3B/hod3RwHcfMOl3jhNjRfuRV1kezrFc0erPTpRTJNsonxD7uRBtbA==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Paul Durrant <paul@xxxxxxx>
  • Delivery-date: Wed, 09 Mar 2022 15:51:10 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHYIoBoEZVl6/zkE0mK7dGMpvA2Zqyi2K6AgAfEbQCAAAXLAIADUEkAgAD5MYCACAnKAIAAAqaAgABc6AA=
  • Thread-topic: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.