Re: [Xen-devel] [PATCH for-next RFC 4/8] x86: factor out xen variants for hypervisor setup code

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

On Fri, Sep 27, 2019 at 12:30:58PM +0100, Wei Liu wrote:
> On Wed, Sep 25, 2019 at 12:23:54PM +0200, Roger Pau Monné wrote:
> > > -void __init probe_hypervisor(void)
> > > +static void __init probe_xen(void)
> > 
> > While here I would rename to xen_probe, to match with the other
> > functions (ie: xen_setup below for example).
> 
> Sure. I can do that. I always thought that strange too.
> 
> > 
> > >  {
> > >      if ( xen_guest )
> > >          return;
> > > @@ -87,6 +87,11 @@ void __init probe_hypervisor(void)
> > >      xen_guest = true;
> > >  }
> > >  
> > > +void __init probe_hypervisor(void)
> > 
> > Shouldn't this live in a separate file, like guest/guest.c or some
> > such?
> > 
> 
> It's done in a later patch. I believe you've already seen it.
> 
> > Also it might be nice to introduce something like:
> > 
> > enum guest_type {
> >     XEN_GUEST,
> > } guest_type;
> > 
> 
> This works for me.
> 
> > So that you can add a switch here, even if the only case is Xen ATM. I
> > guess this will be done in a later patch, or introduce an
> > hypervisor_ops struct that contain pointers to functions to allow for
> > different implementations.
> > 
> 
> I debated this. These changes require more code churn with no apparent
> benefit, but if people agree this is better I can make those changes.
> 
> > > +{
> > > +    probe_xen();
> > > +}
> > > +
> > >  static void map_shared_info(void)
> > >  {
> > >      mfn_t mfn;
> > > @@ -249,10 +254,8 @@ static void init_evtchn(void)
> > >      }
> > >  }
> > >  
> > > -void __init hypervisor_setup(void)
> > > +static void __init xen_setup(void)
> > >  {
> > > -    init_memmap();
> > > -
> > >      map_shared_info();
> > >  
> > >      set_vcpu_id();
> > > @@ -277,13 +280,25 @@ void __init hypervisor_setup(void)
> > >      init_evtchn();
> > >  }
> > >  
> > > -void hypervisor_ap_setup(void)
> > > +void __init hypervisor_setup(void)
> > > +{
> > > +    init_memmap();
> > 
> > I wonder, do you also require to map hypervisor data into the guest
> > physmap when running on HyperV?
> > 
> 
> Yes. There are a lot of comparable concepts in Hyper-V. For example,
> there is a page called VP assist page which is more or less the same as
> Xen's vcpuinfo. Its format, content and interfaces may be different, but
> conceptually it is the same as vcpuinfo.
> 
> > Is there a way when running on HyperV to request unused physical
> > address space ranges? What Xen currently does in init_memmap is quite
> > crappy.
> > 
> 
> Xen itself still needs to manage the address space, no?
>
> I agree the rangeset code in xen.c isn't pretty, but it does the job and
> is not too intrusive.

The problem with the current approach is that the nested Xen has no
way of knowing whether those holes are actually unused, or are MMIO
regions used by devices for example.

Hence I wondered if HyperV had a way to signal to guests that a
physical address range is safe to be used as scratch mapping space. We
had spoken avoid introducing something in Xen to be able to report to
guests safe ranges in the physmap to map stuff.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel