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

[Sheng Yang <sheng@xxxxxxxxxxxxxxx>]

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

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