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