Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Roger,

> On 13 Oct 2021, at 11:56, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> 
> On Wed, Oct 13, 2021 at 09:36:01AM +0000, Bertrand Marquis wrote:
>> Hi Roger,
>> 
>>> On 13 Oct 2021, at 09:30, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>>> 
>>> On Tue, Oct 12, 2021 at 12:38:35PM +0200, Michal Orzel wrote:
>>>> Hi Roger,
>>>> 
>>>> On 11.10.2021 11:27, Roger Pau Monné wrote:
>>>>> On Wed, Oct 06, 2021 at 06:40:33PM +0100, Rahul Singh wrote:
>>>>>> Introduce XEN_DOMCTL_CDF_vpci flag to enable VPCI support in XEN.
>>>>>> Reject the use of this new flag for x86 as VPCI is not supported for
>>>>>> DOMU guests for x86.
>>>>> 
>>>>> I don't like this approach, XEN_DOMCTL_CDF_vpci should be set for x86
>>>>> PVH dom0, like we do for any other CDF flags when Xen builds dom0.
>>>>> 
>>>>> Things like PVH vs PV get translated into CDF flags by create_dom0,
>>>>> and processed normally by the sanitise_domain_config logic, vPCI
>>>>> should be handled that way.
>>>>> 
>>>>> Do you think you could see about fixing this?
>>>>> 
>>>>> Thanks, Roger.
>>>>> 
>>>> 
>>>> I have one question about this fix.
>>>> If I set XEN_DOMCTL_CDF_vpci for dom0 pvh in create_dom0, then in
>>>> sanitise_domain_config or arch_sanitise_domain_config I have no
>>>> knowledge on whether I am dom0 or not. I can check if I'm PVH but not if 
>>>> dom0.
>>>> This would be needed to add a warning if this flag is set but we are not 
>>>> dom0 pvh.
>>>> 
>>>> Any ideas?
>>> 
>>> I've just realized this is more wrong that I thought. vPCI is
>>> signaled on x86 in xen_arch_domainconfig.emulation_flags, so
>>> introducing a top level option for it without removing the arch
>>> specific one is wrong, as then on x86 we have a duplicated option.
>>> 
>>> Then I'm also not sure whether we want to move it from
>>> emulation_flags, it seems like the more natural place for it to live
>>> on x86.
>>> 
>>> If we really want to make vPCI a CDF option we must deal with the
>>> removal of XEN_X86_EMU_VPCI, or else you could introduce an arch
>>> specific flag for vPCI on Arm.
>> 
>> First issue that we have here is that there is no emulation_flags right now 
>> on arm.
> 
> You don't explicitly need an emulation_flags field, you could add a
> uint8_t vpci or some such to xen_arch_domainconfig for Arm if you
> don't think there's a need to select more emulation. That's up to Arm
> folks.

One way or an other it is still changing the interface.

> 
>> So I think there are 2 solutions:
>> 
>> - introduce PHYSCAP. The problem here is that it is not a physical capacity 
>> and
>> I think that will hit us in the future for example if we want to use vpci 
>> for VIRTIO
>> even if there is not physical PCI on the platform.
> 
> Hm, PHYSCAP is a bit weird, for example Xen signals shadow paging
> support using PHYSCAP which doesn't depend on any hardware feature.
> 
> I think you could signal vPCI regardless of whether the underlying
> platform has PCI devices or not, as vPCI is purely a software
> component.

But in the x86 case this is something not supported in all cases and actually 
only by dom0 pvh.
So having something like that defined as a PHYSCAP is a bit weird.

> 
> Regarding VirtIO, won't it be implemented using ioreqs in user-space,
> and thus won't depend on vPCI?

Yes that’s a solution but you can list them through a PCI interface to let a 
guest discover them.
I am not saying that we will do that but that was an example of case where we 
could use vPCI without hardware PCI present.

> 
>> - introduce an emulation flag on arm. The problem here is that there is no 
>> emulation
>> flag right now on arm so adding this feature will require a change of 
>> interface in
>> arch-arm.h and in xen domctl interface. But if we introduce one on Arm, it 
>> would allow
>> the tools to check if CDF_vpci can be set or not which would solve current 
>> issues.
> 
> I'm not sure I follow. If we go the emulation flags route this will all
> be set in xen_arch_domainconfig, and CDF_vpci will completely go away.

This is a mistake on my side. You are right using emulation flags will require 
to remove CDF_vpci.

> 
>> I think the second solution is the right one but it cannot be done so near 
>> from the
>> feature freeze.
>> 
>> The CDF flag as introduced right now is not creating any issue and will be 
>> used once
>> the emulation flag will be introduce. We will be able at this stage to set 
>> this properly
>> also on x86 (for dom0 pvh).
>> Moreover keeping it will allow to continue to merge the remaining part of 
>> the PCI
>> passthrough which are otherwise not possible to be done as they are 
>> dependent on this flag.
>> 
>> Can we agree on keep the DOMCTL_CDF_vpci flag and introduce the emulation
>> flag on Arm after 4.16 release ?
> 
> If vPCI for Arm on 4.16 is not going to be functional, why so much
> pressure in pushing those patches so fast? I understand the need to
> remove stuff from the queue, but I don't think it's worth the cost of
> introducing a broken interface deliberately on a release.

We did not push that fast, those have been on the mailing list for a while 
(this is the 5th version of the serie).
PCI passthrough is a big change requiring lots of patches and we decided to 
push it piece by piece to make
the review easier.

> 
> I think we need to at least settle on whether we want to keep
> CDF_vpci or use an arch specific signal mechanism in order to decide
> what to do regarding the release.

Agree and I have no definitive idea on this so but switching will require to 
revert the patch adding CDF_vpci
and change the subsequent patches.

Regards
Bertrand

> 
> Thanks, Roger.