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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Oct 13, 2021 at 12:11:30PM +0000, Bertrand Marquis wrote:
> Hi Roger,
> 
> > On 13 Oct 2021, at 11:56, Roger Pau Monné <roger.pau@citrix.com> 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@citrix.com> 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.

Well, I don't want to sound rude, but you already changed the
interface first and very recently by introducing CDF_vpci. Complaining
that you don't want to change it anymore just after that seems
slightly weird.

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

Well, even if vPCI was fully supported on x86 for both domU and dom0
it would only apply to PVH and maybe HVM guests, but classic PV will
never get vPCI. It's kind of trying to create a PV guest with HAP
support.

One option would be to defer the introduction of the CDF_vpci flag
until the vpci work has been finished - then we could decide if the
current code can also be used by x86 so maybe it could be enabled for
both arches, and there would be no problem in setting the PHYSCAP.

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

First version of this patch I have on my inbox is from 23rd of
September, that's less than 3 weeks ago.

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

I think it's fair to expect changes to the interface when things are
reviewed, that's part of the review process, changes to subsequent
patches can be painful, but we have all been there and dealt with
them.

I'm not saying we must remove the CDF_vpci flag. I have to admit I was
fine using emulation_flags, but I could also be fine using CDF_vpci if
the x86 side is fixed and XEN_X86_EMU_VPCI is removed.

Thanks, Roger.