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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@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 (?).

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,
>