Re: [RFC PATCH v2 02/22] xen: add CONFIG_XENFV_MACHINE and CONFIG_XEN_EMU options for Xen emulation

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3779 bytes --]

On Tue, 2022-12-13 at 01:39 +0100, Paolo Bonzini wrote:
> 
> 
> Il lun 12 dic 2022, 23:23 David Woodhouse <dwmw2@infradead.org> ha
> scritto:
> > On Mon, 2022-12-12 at 18:07 +0100, Paolo Bonzini wrote:
> > > On 12/9/22 10:55, David Woodhouse wrote:
> > > >   config KVM
> > > >       bool
> > > > +    imply XEN_EMU if (I386 || X86_64)
> > > 
> > > No need for the "imply", just make it "default y" below and it
> > will have 
> > > the same effect.
> > > 
> > > > 
> > > > diff --git a/target/Kconfig b/target/Kconfig
> > > > index 83da0bd293..e19c9d77b5 100644
> > > > --- a/target/Kconfig
> > > > +++ b/target/Kconfig
> > > > @@ -18,3 +18,7 @@ source sh4/Kconfig
> > > >  source sparc/Kconfig
> > > >  source tricore/Kconfig
> > > >  source xtensa/Kconfig
> > > > +
> > > > +config XEN_EMU
> > > > +    bool
> > > > +    depends on KVM && (I386 || X86_64)
> > > 
> > > Please place it in hw/xen/Kconfig.
> > 
> > Perhaps I misunderstand, but I'm not sure that is consistent with
> > what
> > Philippe was asking for in  
> > https://lore.kernel.org/qemu-devel/d203e13d-e2f9-5816-030d-c1449bde364d@linaro.org/
> > specifically:
> > 
> > >> I rather have hw/ and target/ features disentangled, so I'd use
> > >> CONFIG_XEN_EMU under target/ and CONFIG_XENFV_MACHINE under hw/,
> > >> eventually having CONFIG_XEN_EMU depending on
> > CONFIG_XENFV_MACHINE
> > >> and -- for now -- CONFIG_KVM.
> 
> 
> However the dependency of the xenfv machine is misguided. In
> principle there is no reason to depend on KVM as opposed to TCG, too,
> which is why I didn't suggest hw/i386/kvm for either the devices or
> the Kconfig file.
> 

That was my initial thought too.

But those devices are there primarily as hooks to save/restore state,
and that means they want to actually program that state back into KVM
on restore; they *have* ended up using KVM directly, which is why I
moved them into hw/i386/kvm.

Contriving some pretence at a "generic" way for the target to set those
features seemed a bit like overengineering.

Supporting TCG (at least if we want to run on non-x86 hosts) isn't just
a case of reimplementing the bits that are already done in-kernel,
either. The structure layouts may differ too (it's bad enough between
i686 and x86_64 with the alignment of uint64_t). So I just don't see
TCG support happening any time soon.

> > The idea there seems to be that XEN_EMU is a *target* feature since
> > it covers the support in target/i386/kvm.
> > 
> > But yes, it *also* covers the devices I'm adding to hw/i386/kvm. Do
> > I want a *separate* config symbol for that? Or just make those
> > depend on XENFV_MACHINE && XEN_EMU ? 
> > 
> > I'll move XEN_EMU to hw/i386/Kconfig for now, thereby doing what
> > *neither* of you said (I don't think hw/xen/Kconfig is the best
> > choice when the *code* it enables is under hw/i386/kvm?)
> 
> 
> Yeah there is no especially better match. I guess hw/i386 is fine,
> since there will be code in mc->kvm_type. It would be either there or
> in target/i386/Kconfig, but not target/Kconfig.

I put pc_machine_kvm_type into hw/i386/pc.c and made DEFINE_PC_MACHINE
add it unconditionally. Then it registers those devices if
xen_mode==XEN_EMULATE. Which is *almost* pretending to be generic,
apart from the hook being KVM-specific.

There was *also* a call to xen_emulated_machine_init() added to
pc_init1() by the 'pc_piix: handle XEN_EMULATE backend init' patch.
I've dropped that for now; once we are ready to hook up the xenbus and
PV drivers, that seems like it can go into mc->kvm_type too. Or maybe I
should have kept that, and initialised the overlay and evtchn devices
from xen_emulated_machine_init() instead of mc->kvm_type() ?



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]