Re: [PATCH 6/6] xen/hybrid: Enable grant table and xenbus

[Sheng Yang <sheng@linux.intel.com>]

On Tuesday 02 February 2010 19:33:10 Ian Campbell wrote:
> On Tue, 2010-02-02 at 08:19 +0000, Sheng Yang wrote:
> > Notice one memory region(0xfbfe0000ul - 0xfc000000ul) would be reserved
> > in the bios E820 table. This memory region would be used as grant table.
> 
> I queried this requirement in a reply to the hypervisor patch.
> 
> > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > index 05a31e5..d7dfba9 100644
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -1071,6 +1071,9 @@ static int __init xlblk_init(void)
> >  	if (!xen_domain())
> >  		return -ENODEV;
> >
> > +	if (xen_hybrid_domain() && !xen_hybrid_evtchn_enabled())
> > +		return -ENODEV;
> > +
> 
> This seems ugly, at the very least the check should be something like
> xen_evtchn_enabled() 

Yeah, seems indeed ugly... 

> but preferable would be to hook up evtchn's by
> demuxing the PCI device IRQ (the exiting PVonHVM drivers mechanism) in
> the case where hybrid evtchn's are not available and encapsulating the
> differences inside the evtchn code, there should be no need to scatter
> these sorts of checks throughout every driver.
> 
> If you don't want to demux the PCI device IRQ for the non-hybrid case
> another option might be simply return failure from the evtchn operations
> if hybrid evtchns are not available and to ensure that the drivers
> handle that sort of error gracefully (which they should in any case). At
> least the difference in mode would be encapsulated that way.
> 
> (same for netfront).

I am not sure if I understand you right, but I think the issue is, there is no 
PVonHVM drivers in Linux upstream. The drivers are currently maintained by 
OSVs, and the one in Xen upstream code only support 2.6.18. So I didn't take 
them into consideration at the time.

I think the "xen_evtchn_enable()" looks much better. Would replace these ugly 
lines in the next version. 
> 
> >  	if (register_blkdev(XENVBD_MAJOR, DEV_NAME)) {
> >  		printk(KERN_WARNING "xen_blk: can't get major %d with name %s\n",
> >  		       XENVBD_MAJOR, DEV_NAME);
> > diff --git a/drivers/input/xen-kbdfront.c b/drivers/input/xen-kbdfront.c
> > index c721c0a..74cbb25 100644
> > --- a/drivers/input/xen-kbdfront.c
> > +++ b/drivers/input/xen-kbdfront.c
> > @@ -341,6 +341,10 @@ static int __init xenkbd_init(void)
> >  	if (!xen_domain())
> >  		return -ENODEV;
> >
> > +	/* Xen Hybrid domain don't need vkbd */
> > +	if (xen_hybrid_domain())
> > +		return -ENODEV;
> > +
> 
> Why disallow it if the platform has specified it (same for fbfront)?

Well.. The direct reason is I didn't test them and don't know what would 
happen... I would give it a try later.
> 
> > diff --git a/include/xen/xen.h b/include/xen/xen.h
> > index aace9cc..632e76f 100644
> > --- a/include/xen/xen.h
> > +++ b/include/xen/xen.h
> > @@ -5,6 +5,7 @@ enum xen_domain_type {
> >  	XEN_NATIVE,		/* running on bare hardware    */
> >  	XEN_PV_DOMAIN,		/* running in a PV domain      */
> >  	XEN_HVM_DOMAIN,		/* running in a Xen hvm domain */
> > +	XEN_HYBRID_DOMAIN,	/* running in a Xen hybrid hvm domain */
> >  };
> 
> I don't think you should need to distinguish HYBRID from HVM mode...

The only purpose is for event channel of hybrid... Yes, I think I can find 
other way to indicate the availability of event channel. :)

-- 
regards
Yang, Sheng