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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 18 Oct 2021, at 08:51, Jan Beulich <jbeulich@xxxxxxxx> 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@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.

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,