Re: [Xen-devel] [PATCH RESEND 1/2] hvc_xen: support PV on HVM consoles

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 8 Jun 2011, Ian Campbell wrote:
> > +static int xen_hvm_console_init(void)
> > +{
> > +   int r;
> > +   uint64_t v = 0;
> > +   unsigned long mfn;
> > +
> > +   if (!xen_hvm_domain())
> > +           return -ENODEV;
> > +
> > +   if (xencons_if != NULL)
> > +           return -EBUSY;
> > +
> > +   r = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v);
> > +   if (r < 0)
> > +           return -ENODEV;
> > +   console_evtchn = v;
> > +   hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
> > +   if (r < 0)
> > +           return -ENODEV;
> > +   mfn = v;
> > +   xencons_if = ioremap(mfn << PAGE_SHIFT, PAGE_SIZE);
> 
> mfn here is used equivalently to console_pfn, isn't it? Perhaps this
> ioremap could be done in xencons_interface for parity with the pv case?
 
Yes, it could be done that way, but see below.


> Or perhaps better the __va / mfn_to_virt stuff from xencons_interface
> should move into the init functions to initialise xencons_if, at which
> point the xencons_interface() function becomes a dumb wrapper (or even
> better goes away in favour of accessing the variable direct).

Exactly. In fact the next patch removes xencons_interface() completely
and mfn_to_virt for the PV case in done in xen_pv_console_init,
symmetrically to the HVM case.


 
> > +   if (xencons_if == NULL)
> > +           return -ENODEV;
> > +
> > +   return 0;
> > +}
> > +
> >  static int __init xen_hvc_init(void)
> >  {
> >     struct hvc_struct *hp;
> >     struct hv_ops *ops;
> > +   int r;
> >  
> > -   if (!xen_pv_domain())
> > +   if (!xen_domain())
> > +           return -ENODEV;
> > +
> > +   if (xen_pv_domain() && !xen_initial_domain() &&
> > +                   !xen_start_info->console.domU.evtchn)
> >             return -ENODEV;
> >  
> >     if (xen_initial_domain()) {
> >             ops = &dom0_hvc_ops;
> >             xencons_irq = bind_virq_to_irq(VIRQ_CONSOLE, 0);
> >     } else {
> > -           if (!xen_start_info->console.domU.evtchn)
> > -                   return -ENODEV;
> > -
> >             ops = &domU_hvc_ops;
> > -           xencons_irq = 
> > bind_evtchn_to_irq(xen_start_info->console.domU.evtchn);
> > +           if (xen_pv_domain()) {
> > +                   console_pfn = 
> > mfn_to_pfn(xen_start_info->console.domU.mfn);
> > +                   console_evtchn = xen_start_info->console.domU.evtchn;
> > +           } else {
> > +                   r = xen_hvm_console_init();
> > +                   if (r < 0)
> > +                           return r;
> > +           }
> > +           xencons_irq = bind_evtchn_to_irq(console_evtchn);
> >     }
> >     if (xencons_irq < 0)
> >             xencons_irq = 0; /* NO_IRQ */
> > @@ -186,15 +233,13 @@ static int __init xen_hvc_init(void)
> >  
> >     hvc = hp;
> >  
> > -   console_pfn = mfn_to_pfn(xen_start_info->console.domU.mfn);
> > -
> >     return 0;
> >  }
> >  
> >  void xen_console_resume(void)
> >  {
> >     if (xencons_irq)
> > -           rebind_evtchn_irq(xen_start_info->console.domU.evtchn, 
> > xencons_irq);
> > +           rebind_evtchn_irq(console_evtchn, xencons_irq);
> >  }
> >  
> >  static void __exit xen_hvc_fini(void)
> > @@ -205,16 +250,22 @@ static void __exit xen_hvc_fini(void)
> >  
> >  static int xen_cons_init(void)
> >  {
> > -   struct hv_ops *ops;
> > +   const struct hv_ops *ops;
> >  
> > -   if (!xen_pv_domain())
> > +   if (!xen_domain())
> >             return 0;
> >  
> >     if (xen_initial_domain())
> >             ops = &dom0_hvc_ops;
> > -   else
> > +   else {
> >             ops = &domU_hvc_ops;
> >  
> > +           if (xen_pv_domain())
> > +                   console_evtchn = xen_start_info->console.domU.evtchn;
> > +           else
> > +                   xen_hvm_console_init();
> > +   }
> 
> Might be cleaner to have xen_{pv,hvm}_console_init even if the pv one is
> just a one liner?

In the next patch, forced by the increasing number of consoles, I
refactored the code creating a xen_pv_console_init function and moving
everything useful in it. 


> xen_cons_init and xen_hvc_init seem to have a fair bit in common now,
> perhaps there is room for pulling some stuff up into a common function?

Everything is done by xen_{pv,hvm}_console_init so there isn't
much left in xen_cons_init at all.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel