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 <[email protected]> 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.
