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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxx> 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