| Message ID | e2b10f56043155e4bb8eae824723045ccc042f8e.1634315461.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI devices passthrough on Arm | expand |
Hi Bertrand, On 15/10/2021 17:51, Bertrand Marquis wrote: > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 3aa8c3175f..35e0190796 100644 > --- 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); I don't seem to find the code to remove __init_hwdom in this series. Are you intending to fix it separately? With that addressed: Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
Hi Julien, > On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > On 15/10/2021 17:51, Bertrand Marquis wrote: >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >> index 3aa8c3175f..35e0190796 100644 >> --- 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); > > I don't seem to find the code to remove __init_hwdom in this series. Are you intending to fix it separately? Yes I think it is better to fix that in a new patch as it will require some discussion as it will impact the x86 code if I just remove the flag now. > > With that addressed: > > Acked-by: Julien Grall <jgrall@amazon.com> > Thanks Bertrand > Cheers, > > > -- > Julien Grall
On 15/10/2021 18:33, Bertrand Marquis wrote: > Hi Julien, Hi Bertrand, > >> On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote: >> >> Hi Bertrand, >> >> On 15/10/2021 17:51, Bertrand Marquis wrote: >>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >>> index 3aa8c3175f..35e0190796 100644 >>> --- 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); >> >> I don't seem to find the code to remove __init_hwdom in this series. Are you intending to fix it separately? > > Yes I think it is better to fix that in a new patch as it will require some discussion as it will impact the x86 code if I just remove the flag now. For the future patch series, may I ask to keep track of outstanding issues in the commit message (if you don't plan to address them before commiting) or after --- (if they are meant to be addressed before commiting)? In this case, the impact on Arm is this would result to an hypervisor crash if called. If we drop __init_hwdom, the impact on x86 is Xen text will slightly be bigger after the boot time. AFAICT, the code is not reachable on Arm (?). Therefore, one could argue we this can wait after the week-end as this is a latent bug. Yet, I am not really comfortable to see knowningly buggy code merged. Stefano, would you be willing to remove __init_hwdom while committing it? If not, can you update the commit message and mention this patch doesn't work as intended? Cheers,
On Fri, 15 Oct 2021, Julien Grall wrote: > On 15/10/2021 18:33, Bertrand Marquis wrote: > > > On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote: > > > > > > Hi Bertrand, > > > > > > On 15/10/2021 17:51, Bertrand Marquis wrote: > > > > diff --git a/xen/drivers/passthrough/pci.c > > > > b/xen/drivers/passthrough/pci.c > > > > index 3aa8c3175f..35e0190796 100644 > > > > --- 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); > > > > > > I don't seem to find the code to remove __init_hwdom in this series. Are > > > you intending to fix it separately? > > > > Yes I think it is better to fix that in a new patch as it will require some > > discussion as it will impact the x86 code if I just remove the flag now. > For the future patch series, may I ask to keep track of outstanding issues in > the commit message (if you don't plan to address them before commiting) or > after --- (if they are meant to be addressed before commiting)? > > In this case, the impact on Arm is this would result to an hypervisor crash if > called. If we drop __init_hwdom, the impact on x86 is Xen text will slightly > be bigger after the boot time. > > AFAICT, the code is not reachable on Arm (?). Therefore, one could argue we > this can wait after the week-end as this is a latent bug. Yet, I am not really > comfortable to see knowningly buggy code merged. > > Stefano, would you be willing to remove __init_hwdom while committing it? If > not, can you update the commit message and mention this patch doesn't work as > intended? I prefer not to do a change like this on commit as it affects x86. I added a note in the commit message about it. I also added Roger's ack that was given to the previous version. FYI this is the only outstanding TODO as far as I am aware (there are other pending patch series of course). After reviewing the whole series again, checking it against all the reviewers comments, and making it go through gitlab-ci, I committed the series. Thank you all for all the efforts that went into this. We made it :-)
On Fri, Oct 15, 2021 at 12:47:17PM -0700, Stefano Stabellini wrote: > On Fri, 15 Oct 2021, Julien Grall wrote: > > On 15/10/2021 18:33, Bertrand Marquis wrote: > > > > On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote: > > > > > > > > Hi Bertrand, > > > > > > > > On 15/10/2021 17:51, Bertrand Marquis wrote: > > > > > diff --git a/xen/drivers/passthrough/pci.c > > > > > b/xen/drivers/passthrough/pci.c > > > > > index 3aa8c3175f..35e0190796 100644 > > > > > --- 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); > > > > > > > > I don't seem to find the code to remove __init_hwdom in this series. Are > > > > you intending to fix it separately? > > > > > > Yes I think it is better to fix that in a new patch as it will require some > > > discussion as it will impact the x86 code if I just remove the flag now. > > For the future patch series, may I ask to keep track of outstanding issues in > > the commit message (if you don't plan to address them before commiting) or > > after --- (if they are meant to be addressed before commiting)? > > > > In this case, the impact on Arm is this would result to an hypervisor crash if > > called. If we drop __init_hwdom, the impact on x86 is Xen text will slightly > > be bigger after the boot time. > > > > AFAICT, the code is not reachable on Arm (?). Therefore, one could argue we > > this can wait after the week-end as this is a latent bug. Yet, I am not really > > comfortable to see knowningly buggy code merged. > > > > Stefano, would you be willing to remove __init_hwdom while committing it? If > > not, can you update the commit message and mention this patch doesn't work as > > intended? > > I prefer not to do a change like this on commit as it affects x86. > > I added a note in the commit message about it. I also added Roger's ack > that was given to the previous version. FYI this is the only outstanding > TODO as far as I am aware (there are other pending patch series of > course). > > After reviewing the whole series again, checking it against all the > reviewers comments, and making it go through gitlab-ci, I committed the > series. Hello, Maybe I'm being pedantic, or there was some communication outside the mailing list, but I think strictly speaking you are missing an Ack from either Jan or Paul for the xen/drivers/passthrough/pci.c change. IMHO seeing how that chunk moved from 3 different places in just one afternoon also doesn't give me a lot of confidence. It's Arm only code at the end, so it's not going to effect the existing x86 support and I'm not specially worried, but I would like to avoid having to move it again. Regards, Roger.
On 16.10.2021 12:28, Roger Pau Monné wrote: > On Fri, Oct 15, 2021 at 12:47:17PM -0700, Stefano Stabellini wrote: >> On Fri, 15 Oct 2021, Julien Grall wrote: >>> On 15/10/2021 18:33, Bertrand Marquis wrote: >>>>> On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote: >>>>> >>>>> Hi Bertrand, >>>>> >>>>> On 15/10/2021 17:51, Bertrand Marquis wrote: >>>>>> diff --git a/xen/drivers/passthrough/pci.c >>>>>> b/xen/drivers/passthrough/pci.c >>>>>> index 3aa8c3175f..35e0190796 100644 >>>>>> --- 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); >>>>> >>>>> I don't seem to find the code to remove __init_hwdom in this series. Are >>>>> you intending to fix it separately? >>>> >>>> Yes I think it is better to fix that in a new patch as it will require some >>>> discussion as it will impact the x86 code if I just remove the flag now. >>> For the future patch series, may I ask to keep track of outstanding issues in >>> the commit message (if you don't plan to address them before commiting) or >>> after --- (if they are meant to be addressed before commiting)? >>> >>> In this case, the impact on Arm is this would result to an hypervisor crash if >>> called. If we drop __init_hwdom, the impact on x86 is Xen text will slightly >>> be bigger after the boot time. >>> >>> AFAICT, the code is not reachable on Arm (?). Therefore, one could argue we >>> this can wait after the week-end as this is a latent bug. Yet, I am not really >>> comfortable to see knowningly buggy code merged. >>> >>> Stefano, would you be willing to remove __init_hwdom while committing it? If >>> not, can you update the commit message and mention this patch doesn't work as >>> intended? >> >> I prefer not to do a change like this on commit as it affects x86. >> >> I added a note in the commit message about it. I also added Roger's ack >> that was given to the previous version. FYI this is the only outstanding >> TODO as far as I am aware (there are other pending patch series of >> course). >> >> After reviewing the whole series again, checking it against all the >> reviewers comments, and making it go through gitlab-ci, I committed the >> series. > > Hello, > > Maybe I'm being pedantic, or there was some communication outside the > mailing list, but I think strictly speaking you are missing an Ack > from either Jan or Paul for the xen/drivers/passthrough/pci.c change. > > IMHO seeing how that chunk moved from 3 different places in just one > afternoon also doesn't give me a lot of confidence. It's Arm only code > at the end, so it's not going to effect the existing x86 support and > I'm not specially worried, but I would like to avoid having to move it > again. +1 I'll be replying to the patch itself for the technical aspects. As per context still visible above this code path is supposedly unreachable right now, which makes me wonder even more: Why the rush? Depending on the answer plus considering the __hwdom_init issue, Ian, I'm inclined to suggest a revert. Jan
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). 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? Of course as soon as the CONFIG_ARM conditionals were dropped, the __hwdom_init issue would become an "active" one. Jan
On 15.10.2021 20:38, Julien Grall wrote: > > > On 15/10/2021 18:33, Bertrand Marquis wrote: >> Hi Julien, > > Hi Bertrand, > >> >>> On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Bertrand, >>> >>> On 15/10/2021 17:51, Bertrand Marquis wrote: >>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >>>> index 3aa8c3175f..35e0190796 100644 >>>> --- 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); >>> >>> I don't seem to find the code to remove __init_hwdom in this series. Are you intending to fix it separately? >> >> Yes I think it is better to fix that in a new patch as it will require some discussion as it will impact the x86 code if I just remove the flag now. > For the future patch series, may I ask to keep track of outstanding > issues in the commit message (if you don't plan to address them before > commiting) or after --- (if they are meant to be addressed before > commiting)? > > In this case, the impact on Arm is this would result to an hypervisor > crash if called. If we drop __init_hwdom, the impact on x86 is Xen text > will slightly be bigger after the boot time. > > AFAICT, the code is not reachable on Arm (?). Which re-raises my question towards testing of what is being added in this series. Supported also by the typo in v7 patch 1, which suggests that version wasn't even build-tested. Jan > Therefore, one could argue > we this can wait after the week-end as this is a latent bug. Yet, I am > not really comfortable to see knowningly buggy code merged. > > Stefano, would you be willing to remove __init_hwdom while committing > it? If not, can you update the commit message and mention this patch > doesn't work as intended? > > Cheers, >
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. > 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. Thanks, Roger.
Hi Jan, > On 18 Oct 2021, at 08:51, Jan Beulich <jbeulich@suse.com> wrote: > > On 15.10.2021 20:38, Julien Grall wrote: >> >> >> On 15/10/2021 18:33, Bertrand Marquis wrote: >>> Hi Julien, >> >> Hi Bertrand, >> >>> >>>> On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote: >>>> >>>> Hi Bertrand, >>>> >>>> On 15/10/2021 17:51, Bertrand Marquis wrote: >>>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >>>>> index 3aa8c3175f..35e0190796 100644 >>>>> --- 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); >>>> >>>> I don't seem to find the code to remove __init_hwdom in this series. Are you intending to fix it separately? >>> >>> Yes I think it is better to fix that in a new patch as it will require some discussion as it will impact the x86 code if I just remove the flag now. >> For the future patch series, may I ask to keep track of outstanding >> issues in the commit message (if you don't plan to address them before >> commiting) or after --- (if they are meant to be addressed before >> commiting)? >> >> In this case, the impact on Arm is this would result to an hypervisor >> crash if called. If we drop __init_hwdom, the impact on x86 is Xen text >> will slightly be bigger after the boot time. >> >> AFAICT, the code is not reachable on Arm (?). > > Which re-raises my question towards testing of what is being added in > this series. Supported also by the typo in v7 patch 1, which suggests > that version wasn't even build-tested. This was an honest mistake, we did build locally but without VPCI activated. Once I discovered this by rerunning all tests (including one modifying the code to activate VPCI), I signalled it on the mailing list and it was fixed in v8. We did a lot of tests and tried to be as careful as possible but on the last rush before the feature freeze deadline those can happen. Regards Bertrand > > Jan > >> Therefore, one could argue >> we this can wait after the week-end as this is a latent bug. Yet, I am >> not really comfortable to see knowningly buggy code merged. >> >> Stefano, would you be willing to remove __init_hwdom while committing >> it? If not, can you update the commit message and mention this patch >> doesn't work as intended? >> >> Cheers,
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
Hi, Jan! On 18.10.21 10:47, 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. Not really yet > 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. > At the time of this patch we do not yet have a virtual topology implemented which is added at a later stage. So, when a DomU performs PCI enumeration it sees all the real PCI HW and thus discovers all PCI devices with their *real SBDFs*, e.g. just like we pass through all the topology to the guest. So, in the question, SBDFs are physical
Hi Jan, > On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@suse.com> 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. This question was answered by Oleksandr I think. > >> --- 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. Yes a call to vpci_remove_device should be made on the error path out if iommu_add_device is failing. This should also be done in fact in pci_remove_device before cleanup the msi. We will push a patch with a proposal for a fix for this. > > 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). 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? From Roger answer I understood that it will be needed (in the future). When and if this is needed, the ifdef CONFIG_ARM can be removed but this would change x86 code behaviour so I do not think it would have been right to do that in this serie. > > Of course as soon as the CONFIG_ARM conditionals were dropped, the > __hwdom_init issue would become an "active" one. We will push a proposal for a fix for that. If I understand Roger right, vpci_add_handler will also be needed in runtime on x86 in the future so maybe it would even be right to remove the flag altogether ? Regards Bertrand > > Jan >
On 18.10.2021 12:11, Bertrand Marquis wrote: >> On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@suse.com> 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. > > This question was answered by Oleksandr I think. Yes; I continue to be sure though that I saw devfn allocation logic in one of the many patches that are related here. And I'm relatively sure that there no adjustment to logic here was made (but since it's hard to pick the right patch out of the huge pile without knowing its title, I can't reasonably go check). >>> --- 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. > > Yes a call to vpci_remove_device should be made on the error path out if > iommu_add_device is failing. This should also be done in fact in > pci_remove_device before cleanup the msi. > We will push a patch with a proposal for a fix for this. > >> >> 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). 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? > > From Roger answer I understood that it will be needed (in the future). > When and if this is needed, the ifdef CONFIG_ARM can be removed > but this would change x86 code behaviour so I do not think it would > have been right to do that in this serie. I view this differently: This change {c,sh}ould have been broken out into one changing x86 behavior first, which Arm then would simply have adopted. I don't find it unusual for issues to be found in existing code when making that fit for another architecture. As a result ... >> Of course as soon as the CONFIG_ARM conditionals were dropped, the >> __hwdom_init issue would become an "active" one. > > We will push a proposal for a fix for that. > If I understand Roger right, vpci_add_handler will also be needed in runtime > on x86 in the future so maybe it would even be right to remove the flag altogether ? ... I view these as going hand in hand: Removing the annotation altogether is the way to go, yes, because on x86 the call will also need to be made. Ian, considering that PVH Dom0 is still experimental on x86, and considering that the patch here was committed prematurely anyway, would you be willing to release-ack a patch dropping the "#ifdef CONFIG_ARM" alongside other necessary adjustments here, provided maintainers have reached agreement? Jan
Julien Grall writes ("Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM"):
> AFAICT, the code is not reachable on Arm (?). Therefore, one could argue
> we this can wait after the week-end as this is a latent bug. Yet, I am
> not really comfortable to see knowningly buggy code merged.
I agree that merging something that is known to be wrong would be
quite irregular, at least without a compelling reason.
Jan Beulich writes ("Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM"):
> On 16.10.2021 12:28, Roger Pau Monné wrote:
> > Maybe I'm being pedantic, or there was some communication outside the
> > mailing list, but I think strictly speaking you are missing an Ack
> > from either Jan or Paul for the xen/drivers/passthrough/pci.c change.
> >
> > IMHO seeing how that chunk moved from 3 different places in just one
> > afternoon also doesn't give me a lot of confidence. It's Arm only code
> > at the end, so it's not going to effect the existing x86 support and
> > I'm not specially worried, but I would like to avoid having to move it
> > again.
>
> +1
>
> I'll be replying to the patch itself for the technical aspects. As per
> context still visible above this code path is supposedly unreachable
> right now, which makes me wonder even more: Why the rush? Depending on
> the answer plus considering the __hwdom_init issue, Ian, I'm inclined
> to suggest a revert.
I don't want to be waving hammers about at this stage, and I haven't
looked at the technical details myself, but:
Can I ask the ARM folks to make sure that this situation is sorted out
ASAP ? Say, by the end of Thursday ?
By sorted out I mean that the __init_hwdom issue is fixed and that
the overall changes to xen/drivers/passthrough/pci.c have been
properly approved.
Furthermore, I think these followup patches should go in all in one
go, as a small series, when everyone is OK with it, rather than
dribbling in. That will make it easier to see the wood for the trees
(and it would also make a revert less complicated).
Jan, are you OK with this approach ?
Thanks,
Ian.
Hi Ian, > On 18 Oct 2021, at 11:38, Ian Jackson <iwj@xenproject.org> wrote: > > Julien Grall writes ("Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM"): >> AFAICT, the code is not reachable on Arm (?). Therefore, one could argue >> we this can wait after the week-end as this is a latent bug. Yet, I am >> not really comfortable to see knowningly buggy code merged. > > I agree that merging something that is known to be wrong would be > quite irregular, at least without a compelling reason. > > Jan Beulich writes ("Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM"): >> On 16.10.2021 12:28, Roger Pau Monn� wrote: >>> Maybe I'm being pedantic, or there was some communication outside the >>> mailing list, but I think strictly speaking you are missing an Ack >>> from either Jan or Paul for the xen/drivers/passthrough/pci.c change. >>> >>> IMHO seeing how that chunk moved from 3 different places in just one >>> afternoon also doesn't give me a lot of confidence. It's Arm only code >>> at the end, so it's not going to effect the existing x86 support and >>> I'm not specially worried, but I would like to avoid having to move it >>> again. >> >> +1 >> >> I'll be replying to the patch itself for the technical aspects. As per >> context still visible above this code path is supposedly unreachable >> right now, which makes me wonder even more: Why the rush? Depending on >> the answer plus considering the __hwdom_init issue, Ian, I'm inclined >> to suggest a revert. > > I don't want to be waving hammers about at this stage, and I haven't > looked at the technical details myself, but: > > Can I ask the ARM folks to make sure that this situation is sorted out > ASAP ? Say, by the end of Thursday ? Sure. Cheers Bertrand > > By sorted out I mean that the __init_hwdom issue is fixed and that > the overall changes to xen/drivers/passthrough/pci.c have been > properly approved. > > Furthermore, I think these followup patches should go in all in one > go, as a small series, when everyone is OK with it, rather than > dribbling in. That will make it easier to see the wood for the trees > (and it would also make a revert less complicated). > > Jan, are you OK with this approach ? > > Thanks, > Ian.
On 18.10.2021 12:38, Ian Jackson wrote: > Julien Grall writes ("Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM"): >> AFAICT, the code is not reachable on Arm (?). Therefore, one could argue >> we this can wait after the week-end as this is a latent bug. Yet, I am >> not really comfortable to see knowningly buggy code merged. > > I agree that merging something that is known to be wrong would be > quite irregular, at least without a compelling reason. > > Jan Beulich writes ("Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM"): >> On 16.10.2021 12:28, Roger Pau Monné wrote: >>> Maybe I'm being pedantic, or there was some communication outside the >>> mailing list, but I think strictly speaking you are missing an Ack >>> from either Jan or Paul for the xen/drivers/passthrough/pci.c change. >>> >>> IMHO seeing how that chunk moved from 3 different places in just one >>> afternoon also doesn't give me a lot of confidence. It's Arm only code >>> at the end, so it's not going to effect the existing x86 support and >>> I'm not specially worried, but I would like to avoid having to move it >>> again. >> >> +1 >> >> I'll be replying to the patch itself for the technical aspects. As per >> context still visible above this code path is supposedly unreachable >> right now, which makes me wonder even more: Why the rush? Depending on >> the answer plus considering the __hwdom_init issue, Ian, I'm inclined >> to suggest a revert. > > I don't want to be waving hammers about at this stage, and I haven't > looked at the technical details myself, but: > > Can I ask the ARM folks to make sure that this situation is sorted out > ASAP ? Say, by the end of Thursday ? > > By sorted out I mean that the __init_hwdom issue is fixed and that > the overall changes to xen/drivers/passthrough/pci.c have been > properly approved. > > Furthermore, I think these followup patches should go in all in one > go, as a small series, when everyone is OK with it, rather than > dribbling in. That will make it easier to see the wood for the trees > (and it would also make a revert less complicated). > > Jan, are you OK with this approach ? Yes. FTR I'm not fussed about "all in one go" vs "dribbling in". Jan
On 18.10.21 13:29, Jan Beulich wrote: > On 18.10.2021 12:11, Bertrand Marquis wrote: >>> On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@suse.com> 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. >> This question was answered by Oleksandr I think. > Yes; I continue to be sure though that I saw devfn allocation logic in > one of the many patches that are related here. And I'm relatively sure > that there no adjustment to logic here was made (but since it's hard > to pick the right patch out of the huge pile without knowing its title, > I can't reasonably go check). Offtop: I was somehow dropped from the Cc.. Please see: [PATCH v3 10/11] vpci: Add initial support for virtual PCI bus topology [PATCH v3 11/11] xen/arm: Translate virtual PCI bus topology for guests [1] https://patchwork.kernel.org/project/xen-devel/list/?series=555481
On 18.10.2021 16:07, Oleksandr Andrushchenko wrote: > On 18.10.21 13:29, Jan Beulich wrote: >> On 18.10.2021 12:11, Bertrand Marquis wrote: >>>> On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@suse.com> 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. >>> This question was answered by Oleksandr I think. >> Yes; I continue to be sure though that I saw devfn allocation logic in >> one of the many patches that are related here. And I'm relatively sure >> that there no adjustment to logic here was made (but since it's hard >> to pick the right patch out of the huge pile without knowing its title, >> I can't reasonably go check). > Offtop: I was somehow dropped from the Cc.. > > Please see: > > [PATCH v3 10/11] vpci: Add initial support for virtual PCI bus topology > [PATCH v3 11/11] xen/arm: Translate virtual PCI bus topology for guests > > [1] https://patchwork.kernel.org/project/xen-devel/list/?series=555481 Ah yes, this way I can find them in my mailbox. And indeed - no translation from virtual to physical SBDF on the config space read/write paths afaics. Thanks for the pointer, Jan
On 18.10.21 17:19, Jan Beulich wrote: > On 18.10.2021 16:07, Oleksandr Andrushchenko wrote: >> On 18.10.21 13:29, Jan Beulich wrote: >>> On 18.10.2021 12:11, Bertrand Marquis wrote: >>>>> On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@suse.com> 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. >>>> This question was answered by Oleksandr I think. >>> Yes; I continue to be sure though that I saw devfn allocation logic in >>> one of the many patches that are related here. And I'm relatively sure >>> that there no adjustment to logic here was made (but since it's hard >>> to pick the right patch out of the huge pile without knowing its title, >>> I can't reasonably go check). >> Offtop: I was somehow dropped from the Cc.. >> >> Please see: >> >> [PATCH v3 10/11] vpci: Add initial support for virtual PCI bus topology >> [PATCH v3 11/11] xen/arm: Translate virtual PCI bus topology for guests >> >> [1] https://urldefense.com/v3/__https://patchwork.kernel.org/project/xen-devel/list/?series=555481__;!!GF_29dbcQIUBPA!mM8A39p8nk4UMK3YeKMMW9ua9BHj1UOWzaQcyx7G46YPdudxMpD3huqZfih0Uc8S-GyWXD_mPg$ [patchwork[.]kernel[.]org] > Ah yes, this way I can find them in my mailbox. And indeed - no translation > from virtual to physical SBDF on the config space read/write paths afaics. There are translations for both read and write [2] such as: + /* + * For the passed through devices we need to map their virtual SBDF + * to the physical PCI device being passed through. + */ + if ( priv->is_virt_ecam && !pci_translate_virtual_device(v->domain, &sbdf) ) + return 1; + > > Thanks for the pointer, > Jan > [2] https://patchwork.kernel.org/project/xen-devel/patch/20210930075223.860329-12-andr2000@gmail.com/
On 18.10.2021 16:37, Oleksandr Andrushchenko wrote: > > > On 18.10.21 17:19, Jan Beulich wrote: >> On 18.10.2021 16:07, Oleksandr Andrushchenko wrote: >>> On 18.10.21 13:29, Jan Beulich wrote: >>>> On 18.10.2021 12:11, Bertrand Marquis wrote: >>>>>> On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@suse.com> 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. >>>>> This question was answered by Oleksandr I think. >>>> Yes; I continue to be sure though that I saw devfn allocation logic in >>>> one of the many patches that are related here. And I'm relatively sure >>>> that there no adjustment to logic here was made (but since it's hard >>>> to pick the right patch out of the huge pile without knowing its title, >>>> I can't reasonably go check). >>> Offtop: I was somehow dropped from the Cc.. >>> >>> Please see: >>> >>> [PATCH v3 10/11] vpci: Add initial support for virtual PCI bus topology >>> [PATCH v3 11/11] xen/arm: Translate virtual PCI bus topology for guests >>> >>> [1] https://urldefense.com/v3/__https://patchwork.kernel.org/project/xen-devel/list/?series=555481__;!!GF_29dbcQIUBPA!mM8A39p8nk4UMK3YeKMMW9ua9BHj1UOWzaQcyx7G46YPdudxMpD3huqZfih0Uc8S-GyWXD_mPg$ [patchwork[.]kernel[.]org] >> Ah yes, this way I can find them in my mailbox. And indeed - no translation >> from virtual to physical SBDF on the config space read/write paths afaics. > There are translations for both read and write [2] such as: > > + /* > + * For the passed through devices we need to map their virtual SBDF > + * to the physical PCI device being passed through. > + */ > + if ( priv->is_virt_ecam && !pci_translate_virtual_device(v->domain, &sbdf) ) > + return 1; > + Oh, that's before you even call vpci_read(). I would have expected this to live in common vPCI code ... Jan
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 64518848b2..07f634508e 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -7,6 +7,7 @@ ifneq ($(CONFIG_NO_PLAT),y) obj-y += platforms/ endif obj-$(CONFIG_TEE) += tee/ +obj-$(CONFIG_HAS_VPCI) += vpci.o obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o obj-y += bootfdt.init.o diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index eef0661beb..96e1b23550 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -39,6 +39,7 @@ #include <asm/vgic.h> #include <asm/vtimer.h> +#include "vpci.h" #include "vuart.h" DEFINE_PER_CPU(struct vcpu *, curr_vcpu); @@ -773,6 +774,9 @@ int arch_domain_create(struct domain *d, if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) goto fail; + if ( (rc = domain_vpci_init(d)) != 0 ) + goto fail; + return 0; fail: diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c new file mode 100644 index 0000000000..8f40a0dec6 --- /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) ) + { + *r = data; + return 1; + } + + *r = ~0ul; + + return 0; +} + +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info, + register_t r, void *p) +{ + pci_sbdf_t sbdf; + + /* We ignore segment part and always handle segment 0 */ + sbdf.sbdf = VPCI_ECAM_BDF(info->gpa); + + return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa), + 1U << info->dabt.size, r); +} + +static const struct mmio_handler_ops vpci_mmio_handler = { + .read = vpci_mmio_read, + .write = vpci_mmio_write, +}; + +int domain_vpci_init(struct domain *d) +{ + if ( !has_vpci(d) ) + return 0; + + register_mmio_handler(d, &vpci_mmio_handler, + GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL); + + return 0; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ + diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h new file mode 100644 index 0000000000..d8a7b0e3e8 --- /dev/null +++ b/xen/arch/arm/vpci.h @@ -0,0 +1,36 @@ +/* + * xen/arch/arm/vpci.h + * + * 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. + */ + +#ifndef __ARCH_ARM_VPCI_H__ +#define __ARCH_ARM_VPCI_H__ + +#ifdef CONFIG_HAS_VPCI +int domain_vpci_init(struct domain *d); +#else +static inline int domain_vpci_init(struct domain *d) +{ + return 0; +} +#endif + +#endif /* __ARCH_ARM_VPCI_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 3aa8c3175f..35e0190796 100644 --- 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 ) { diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index f8cd55e7c0..40ff79c33f 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -373,7 +373,7 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, /* If the value written is the current one avoid printing a warning. */ if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) gprintk(XENLOG_WARNING, - "%pp: ignored BAR %lu write with memory decoding enabled\n", + "%pp: ignored BAR %zu write with memory decoding enabled\n", &pdev->sbdf, bar - pdev->vpci->header.bars + hi); return; } diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index ef690f15a9..decf7d87a1 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -485,6 +485,12 @@ bool vpci_access_allowed(unsigned int reg, unsigned int len) if ( len != 1 && len != 2 && len != 4 && len != 8 ) return false; +#ifndef CONFIG_64BIT + /* Prevent 64bit accesses on 32bit */ + if ( len == 8 ) + return false; +#endif + /* Check that access is size aligned. */ if ( (reg & (len - 1)) ) return false; @@ -500,8 +506,10 @@ bool vpci_ecam_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, return false; vpci_write(sbdf, reg, min(4u, len), data); +#ifdef CONFIG_64BIT if ( len == 8 ) vpci_write(sbdf, reg + 4, 4, data >> 32); +#endif return true; } @@ -526,8 +534,10 @@ bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, * 4byte accesses. */ *data = vpci_read(sbdf, reg, min(4u, len)); +#ifdef CONFIG_64BIT if ( len == 8 ) *data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32; +#endif return true; } diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 14e575288f..9b3647587a 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -263,6 +263,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {} #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) +/* vPCI is not available on Arm */ #define has_vpci(d) ({ (void)(d); false; }) #endif /* __ASM_DOMAIN_H__ */ diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index 96ead3de07..b4c615bcdf 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -418,6 +418,13 @@ typedef uint64_t xen_callback_t; #define GUEST_GICV3_GICR0_BASE xen_mk_ullong(0x03020000) /* vCPU0..127 */ #define GUEST_GICV3_GICR0_SIZE xen_mk_ullong(0x01000000) +/* + * 256 MB is reserved for VPCI configuration space based on calculation + * 256 buses x 32 devices x 8 functions x 4 KB = 256 MB + */ +#define GUEST_VPCI_ECAM_BASE xen_mk_ullong(0x10000000) +#define GUEST_VPCI_ECAM_SIZE xen_mk_ullong(0x10000000) + /* ACPI tables physical address */ #define GUEST_ACPI_BASE xen_mk_ullong(0x20000000) #define GUEST_ACPI_SIZE xen_mk_ullong(0x02000000) diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 70ac25345c..b6d7e454f8 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -40,6 +40,8 @@ #define PCI_SBDF3(s,b,df) \ ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) }) +#define ECAM_REG_OFFSET(addr) ((addr) & 0x00000fff) + typedef union { uint32_t sbdf; struct {