[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 10:08: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=1NqrgC1THxgaW5C43pa9FzKLabTMVFbpvPR9iPIaIW8=; b=M029LXHAqoID5CIiw8O3A1I7w1lhYAo9lh5d3sEnUNsLogX44X3Cm19npPNkOoQrWbyVXCVaa5Y3GljtM4lkH1xC6+BrB9BaoVwWycB07D5HWs1QeMfMdOWmXwHEjpydaT8WbvJqF6lNUOWgGS9xVMmEmrChU9dtnoYqqNLfjns1yGt3RWFzBAfuVtPqSQFuyoXEdj9dodfBzts7ndjLS+/oMzY0nBfhVw/1TYG/1WraUrqtuZiIeJYJ4wLNPluaf5KdWlSwMXbfyubcmJA6S738l9lONPkOnU5h/HUsl5ZdLoomG2aDJjMIEENcg/PspvR8CHH51EkAN4taDTSeYg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=lBV540qrPSxiKGWKFm3X7oXplc2djqWjqGZ1WDstXYMR9eeFrOk1fWcvVKa37TN8wHtrtRhl5x+bSNxrgUF1yH31L8tD7LUBYQSrk0dG19d36Ca+LRB1tzhqEqIpYWJOCt3JGlcm4+oU1rGsCogjtsqfqRG3/ott2EBeeme3v6QeCIp4DK0aASzSPPpt6UkKV4WZZJmOeyNRyQtmnSl9lSLVn9J98oJ8+m5yKciE5Qs1ktwUsdaK3pwwm+9ZllVwWEjgGYcY8eBGoKU7m9idQMnbx/7rw0iD4j1ja83lKYE49PIWBVeBYNPwHPBGhq9pysGNZuIsjaKt/QZJ9DOG/g==
  • 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 10:08:47 +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/zkE0mK7dGMpvA2Zqyi2K6AgAfEbQCAAAXLAIADUEkAgAD5MYCACAnKAA==
  • Thread-topic: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

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
> 


 


Rackspace

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