WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

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