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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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

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

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

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

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.

Thanks, Roger.