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

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 18 Oct 2021 10:38:34 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.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=fndire85mAACL4pbJGncAywmLzcxt7aJCh+9xcbCQdo=; b=Yxa084fsfn+PtIKAI7yv8xcke6wtvx+O96/Vqx3F9B1m65AqSewUtecvjQ91FAr26zUzmdCkRQBK0sSKoCNymmEDrk0AIMq8UknYDKmkh+L1V95GsQIN3HdKmf059mMEzEDmXypLdDa8jlzxY2HnKGsI2ih1XRaZGre1kyyHECZRgQsDImCMQ22s1e654vp6oTfuoJG5kp8GMHY4lDMB5jsikMSGl1IlOsib+pNhfjry8GP4672agdHf93WCrg136ua6qh038onlqCe3deaq/BEeDLhoDvx4HA4MaY8q0B9NZ9hYLbwBQRrLJgQ7f/BVniWmGzApok+eNHgb63ze0g==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=TK0ps7+4B4M5/44isJrjLK4unrcopgRHIW61ARhWfBg2xfgt2IqyrQ2Af3u3A3Rwicsx6fZisLhWG/KX3Cbvk4k3HP47ALrHyQErU5FwPfcK+xoR97N7QA+JpiCtKPexQipKix3OR49/d6OjN5MrC3BReBI0JSuj5cvxme+uEFHjK3bkKJVkM3v86j5xI0y21YPw1Oo4o8wm5ILG0f+o5DNcYGNAhWNXtjvNYhy65fDqh6gAgijCjXAxob1rTSu0rFTrKt0rE+GW8nVoVVls5mCwYzBDwW4vtlJX17zL/iKLx7CEpT2fvNNvGFnhdbC12ZKfEXTMAWHAHbDbMDhp5w==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Bertrand Marquis <bertrand.marquis@xxxxxxx>, Rahul Singh <rahul.singh@xxxxxxx>, iwj@xxxxxxxxxxxxxx, sstabellini@xxxxxxxxxx, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Paul Durrant <paul@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 18 Oct 2021 08:38:47 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 18.10.2021 10:03, Roger Pau Monné wrote:
> On Mon, Oct 18, 2021 at 09:47:06AM +0200, Jan Beulich wrote:
>> On 15.10.2021 18:51, Bertrand Marquis wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/arm/vpci.c
>>> @@ -0,0 +1,77 @@
>>> +/*
>>> + * xen/arch/arm/vpci.c
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +#include <xen/sched.h>
>>> +#include <xen/vpci.h>
>>> +
>>> +#include <asm/mmio.h>
>>> +
>>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>>> +                          register_t *r, void *p)
>>> +{
>>> +    pci_sbdf_t sbdf;
>>> +    /* data is needed to prevent a pointer cast on 32bit */
>>> +    unsigned long data;
>>> +
>>> +    /* We ignore segment part and always handle segment 0 */
>>> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
>>> +
>>> +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>>> +                        1U << info->dabt.size, &data) )
>>> +    {
>>
>> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
>> the virtual one. The function then calls vpci_read(), which in turn
>> will call vpci_read_hw() in a number of situations (first and foremost
>> whenever pci_get_pdev_by_domain() returns NULL). That function as well
>> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
>> physical one; I'm unable to spot any translation. Yet I do recall
>> seeing assignment of a virtual device and function number somewhere
>> (perhaps another of the related series), so the model also doesn't
>> look to assume 1:1 mapping of SBDF.
>>
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>      if ( !pdev->domain )
>>>      {
>>>          pdev->domain = hardware_domain;
>>> +#ifdef CONFIG_ARM
>>> +        /*
>>> +         * On ARM PCI devices discovery will be done by Dom0. Add vpci 
>>> handler
>>> +         * when Dom0 inform XEN to add the PCI devices in XEN.
>>> +         */
>>> +        ret = vpci_add_handlers(pdev);
>>> +        if ( ret )
>>> +        {
>>> +            printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
>>> +            pdev->domain = NULL;
>>> +            goto out;
>>> +        }
>>> +#endif
>>>          ret = iommu_add_device(pdev);
>>>          if ( ret )
>>>          {
>>
>> Upon failure, vpci_add_handlers() will itself call vpci_remove_handlers().
>> What about iommu_add_device() failure? The device will have ->domain
>> zapped, but all vPCI handlers still in place. This aspect of insufficient
>> error cleanup was pointed out already in review of v1.
>>
>> Furthermore already in v1 I questioned why this would be Arm-specific: On
>> x86 this code path is going to be taken for all devices Xen wasn't able
>> to discover at boot (anything on segments we wouldn't consider config
>> space access safe on without reassurance by Dom0 plus SR-IOV VFs, at the
>> very least).
> 
> My original plans for SR-IOV VFs on PVH dom0 involved trapping
> accesses to the SR-IOV capability and detecting the creation of VFs
> without the need for dom0 to notify them to Xen. This would avoid dom0
> having to call PHYSDEVOP_pci_device_add for that case.
> 
> I might be confused, but I think we also spoke about other (non SR-IOV
> related) cases where PCI devices might appear after certain actions by
> dom0, so I think we need to keep the PHYSDEVOP_pci_device_add for PVH
> dom0.

Right, hotplugged ones, which I forgot to mention in my earlier reply.

>> Hence it is my understanding that something along these
>> lines is actually also needed for PVH Dom0. I've just checked, and
>> according to my mailbox that comment was actually left unresponded to.
>>
>> Roger, am I missing anything here as to PVH Dom0 getting handlers put in
>> place?
> 
> No, I think we will need those, likewise for run-time reported MCFG
> regions.

Yes, this was what I referred to by "without reassurance by Dom0".
Thanks for confirming.

Jan




 


Rackspace

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