| Message ID | 1344263246-28036-10-git-send-email-stefano.stabellini@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote: > bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not > an error. > > If Linux is running as an HVM domain and is running as Dom0, use > xenstored_local_init to initialize the xenstore page and event channel. > > Changes in v2: > > - refactor xenbus_init. Thank you. Lets also CC our friend at NSA who has been doing some work in that area. Daniel are you OK with this change - will it still make PV initial domain with with the MiniOS XenBus driver? Thanks. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > --- > drivers/xen/xenbus/xenbus_comms.c | 2 +- > drivers/xen/xenbus/xenbus_probe.c | 62 +++++++++++++++++++++++++----------- > drivers/xen/xenbus/xenbus_xs.c | 1 + > 3 files changed, 45 insertions(+), 20 deletions(-) > > diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c > index 52fe7ad..c5aa55c 100644 > --- a/drivers/xen/xenbus/xenbus_comms.c > +++ b/drivers/xen/xenbus/xenbus_comms.c > @@ -224,7 +224,7 @@ int xb_init_comms(void) > int err; > err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting, > 0, "xenbus", &xb_waitq); > - if (err <= 0) { > + if (err < 0) { > printk(KERN_ERR "XENBUS request irq failed %i\n", err); > return err; > } > diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c > index b793723..a67ccc0 100644 > --- a/drivers/xen/xenbus/xenbus_probe.c > +++ b/drivers/xen/xenbus/xenbus_probe.c > @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void) > return err; > } > > +enum xenstore_init { > + UNKNOWN, > + PV, > + HVM, > + LOCAL, > +}; > static int __init xenbus_init(void) > { > int err = 0; > + enum xenstore_init usage = UNKNOWN; > + uint64_t v = 0; > > if (!xen_domain()) > return -ENODEV; > > xenbus_ring_ops_init(); > > - if (xen_hvm_domain()) { > - uint64_t v = 0; > - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); > - if (err) > - goto out_error; > - xen_store_evtchn = (int)v; > - err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); > - if (err) > - goto out_error; > - xen_store_mfn = (unsigned long)v; > - xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); > - } else { > - xen_store_evtchn = xen_start_info->store_evtchn; > - xen_store_mfn = xen_start_info->store_mfn; > - if (xen_store_evtchn) > - xenstored_ready = 1; > - else { > + if (xen_pv_domain()) > + usage = PV; > + if (xen_hvm_domain()) > + usage = HVM; > + if (xen_hvm_domain() && xen_initial_domain()) > + usage = LOCAL; > + if (xen_pv_domain() && !xen_start_info->store_evtchn) > + usage = LOCAL; > + if (xen_pv_domain() && xen_start_info->store_evtchn) > + xenstored_ready = 1; > + > + switch (usage) { > + case LOCAL: > err = xenstored_local_init(); > if (err) > goto out_error; > - } > - xen_store_interface = mfn_to_virt(xen_store_mfn); > + xen_store_interface = mfn_to_virt(xen_store_mfn); > + break; > + case PV: > + xen_store_evtchn = xen_start_info->store_evtchn; > + xen_store_mfn = xen_start_info->store_mfn; > + xen_store_interface = mfn_to_virt(xen_store_mfn); > + break; > + case HVM: > + err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); > + if (err) > + goto out_error; > + xen_store_evtchn = (int)v; > + err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); > + if (err) > + goto out_error; > + xen_store_mfn = (unsigned long)v; > + xen_store_interface = > + ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); > + break; > + default: > + pr_warn("Xenstore state unknown\n"); > + break; > } > > /* Initialize the interface to xenstore. */ > diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c > index d1c217b..f7feb3d 100644 > --- a/drivers/xen/xenbus/xenbus_xs.c > +++ b/drivers/xen/xenbus/xenbus_xs.c > @@ -44,6 +44,7 @@ > #include <linux/rwsem.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <asm/xen/hypervisor.h> > #include <xen/xenbus.h> > #include <xen/xen.h> > #include "xenbus_comms.h" > -- > 1.7.2.5
On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote: > On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote: >> bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not >> an error. >> >> If Linux is running as an HVM domain and is running as Dom0, use >> xenstored_local_init to initialize the xenstore page and event channel. >> >> Changes in v2: >> >> - refactor xenbus_init. > > Thank you. Lets also CC our friend at NSA who has been doing some work > in that area. Daniel are you OK with this change - will it still make > PV initial domain with with the MiniOS XenBus driver? > > Thanks. That case will work, but what this will break is launching the initial domain with a Xenstore stub domain already running (see below). >> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> --- >> drivers/xen/xenbus/xenbus_comms.c | 2 +- >> drivers/xen/xenbus/xenbus_probe.c | 62 +++++++++++++++++++++++++----------- >> drivers/xen/xenbus/xenbus_xs.c | 1 + >> 3 files changed, 45 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c >> index 52fe7ad..c5aa55c 100644 >> --- a/drivers/xen/xenbus/xenbus_comms.c >> +++ b/drivers/xen/xenbus/xenbus_comms.c >> @@ -224,7 +224,7 @@ int xb_init_comms(void) >> int err; >> err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting, >> 0, "xenbus", &xb_waitq); >> - if (err <= 0) { >> + if (err < 0) { >> printk(KERN_ERR "XENBUS request irq failed %i\n", err); >> return err; >> } > >> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c >> index b793723..a67ccc0 100644 >> --- a/drivers/xen/xenbus/xenbus_probe.c >> +++ b/drivers/xen/xenbus/xenbus_probe.c >> @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void) >> return err; >> } >> >> +enum xenstore_init { >> + UNKNOWN, >> + PV, >> + HVM, >> + LOCAL, >> +}; >> static int __init xenbus_init(void) >> { >> int err = 0; >> + enum xenstore_init usage = UNKNOWN; >> + uint64_t v = 0; >> >> if (!xen_domain()) >> return -ENODEV; >> >> xenbus_ring_ops_init(); >> >> - if (xen_hvm_domain()) { >> - uint64_t v = 0; >> - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); >> - if (err) >> - goto out_error; >> - xen_store_evtchn = (int)v; >> - err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); >> - if (err) >> - goto out_error; >> - xen_store_mfn = (unsigned long)v; >> - xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); >> - } else { >> - xen_store_evtchn = xen_start_info->store_evtchn; >> - xen_store_mfn = xen_start_info->store_mfn; >> - if (xen_store_evtchn) >> - xenstored_ready = 1; >> - else { >> + if (xen_pv_domain()) >> + usage = PV; >> + if (xen_hvm_domain()) >> + usage = HVM; The above is correct for domUs, and is overridden for dom0s: >> + if (xen_hvm_domain() && xen_initial_domain()) >> + usage = LOCAL; >> + if (xen_pv_domain() && !xen_start_info->store_evtchn) >> + usage = LOCAL; Instead of these checks, I think it should just be: if (!xen_start_info->store_evtchn) usage = LOCAL; Any domain started after xenstore will have store_evtchn set, so if you don't have this set, you are either going to be running xenstore locally, or will use the ioctl to change it later (and so should still set up everything as if it will be running locally). >> + if (xen_pv_domain() && xen_start_info->store_evtchn) >> + xenstored_ready = 1; This part can now just be moved unconditionally into case PV. >> + >> + switch (usage) { >> + case LOCAL: >> err = xenstored_local_init(); >> if (err) >> goto out_error; >> - } >> - xen_store_interface = mfn_to_virt(xen_store_mfn); >> + xen_store_interface = mfn_to_virt(xen_store_mfn); >> + break; >> + case PV: >> + xen_store_evtchn = xen_start_info->store_evtchn; >> + xen_store_mfn = xen_start_info->store_mfn; >> + xen_store_interface = mfn_to_virt(xen_store_mfn); >> + break; >> + case HVM: >> + err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); >> + if (err) >> + goto out_error; >> + xen_store_evtchn = (int)v; >> + err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); >> + if (err) >> + goto out_error; >> + xen_store_mfn = (unsigned long)v; >> + xen_store_interface = >> + ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); >> + break; >> + default: >> + pr_warn("Xenstore state unknown\n"); >> + break; >> } >> >> /* Initialize the interface to xenstore. */ >> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c >> index d1c217b..f7feb3d 100644 >> --- a/drivers/xen/xenbus/xenbus_xs.c >> +++ b/drivers/xen/xenbus/xenbus_xs.c >> @@ -44,6 +44,7 @@ >> #include <linux/rwsem.h> >> #include <linux/module.h> >> #include <linux/mutex.h> >> +#include <asm/xen/hypervisor.h> >> #include <xen/xenbus.h> >> #include <xen/xen.h> >> #include "xenbus_comms.h" >> -- >> 1.7.2.5
On Tue, 7 Aug 2012, Daniel De Graaf wrote: > On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote: > > On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote: > >> bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not > >> an error. > >> > >> If Linux is running as an HVM domain and is running as Dom0, use > >> xenstored_local_init to initialize the xenstore page and event channel. > >> > >> Changes in v2: > >> > >> - refactor xenbus_init. > > > > Thank you. Lets also CC our friend at NSA who has been doing some work > > in that area. Daniel are you OK with this change - will it still make > > PV initial domain with with the MiniOS XenBus driver? > > > > Thanks. > > That case will work, but what this will break is launching the initial domain > with a Xenstore stub domain already running (see below). > > >> > >> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > >> --- > >> drivers/xen/xenbus/xenbus_comms.c | 2 +- > >> drivers/xen/xenbus/xenbus_probe.c | 62 +++++++++++++++++++++++++----------- > >> drivers/xen/xenbus/xenbus_xs.c | 1 + > >> 3 files changed, 45 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c > >> index 52fe7ad..c5aa55c 100644 > >> --- a/drivers/xen/xenbus/xenbus_comms.c > >> +++ b/drivers/xen/xenbus/xenbus_comms.c > >> @@ -224,7 +224,7 @@ int xb_init_comms(void) > >> int err; > >> err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting, > >> 0, "xenbus", &xb_waitq); > >> - if (err <= 0) { > >> + if (err < 0) { > >> printk(KERN_ERR "XENBUS request irq failed %i\n", err); > >> return err; > >> } > > > >> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c > >> index b793723..a67ccc0 100644 > >> --- a/drivers/xen/xenbus/xenbus_probe.c > >> +++ b/drivers/xen/xenbus/xenbus_probe.c > >> @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void) > >> return err; > >> } > >> > >> +enum xenstore_init { > >> + UNKNOWN, > >> + PV, > >> + HVM, > >> + LOCAL, > >> +}; > >> static int __init xenbus_init(void) > >> { > >> int err = 0; > >> + enum xenstore_init usage = UNKNOWN; > >> + uint64_t v = 0; > >> > >> if (!xen_domain()) > >> return -ENODEV; > >> > >> xenbus_ring_ops_init(); > >> > >> - if (xen_hvm_domain()) { > >> - uint64_t v = 0; > >> - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); > >> - if (err) > >> - goto out_error; > >> - xen_store_evtchn = (int)v; > >> - err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); > >> - if (err) > >> - goto out_error; > >> - xen_store_mfn = (unsigned long)v; > >> - xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); > >> - } else { > >> - xen_store_evtchn = xen_start_info->store_evtchn; > >> - xen_store_mfn = xen_start_info->store_mfn; > >> - if (xen_store_evtchn) > >> - xenstored_ready = 1; > >> - else { > >> + if (xen_pv_domain()) > >> + usage = PV; > >> + if (xen_hvm_domain()) > >> + usage = HVM; > > The above is correct for domUs, and is overridden for dom0s: > > >> + if (xen_hvm_domain() && xen_initial_domain()) > >> + usage = LOCAL; > >> + if (xen_pv_domain() && !xen_start_info->store_evtchn) > >> + usage = LOCAL; > > Instead of these checks, I think it should just be: > > if (!xen_start_info->store_evtchn) > usage = LOCAL; > > Any domain started after xenstore will have store_evtchn set, so if you don't > have this set, you are either going to be running xenstore locally, or will > use the ioctl to change it later (and so should still set up everything as if > it will be running locally). That would be wrong for an HVM dom0 domain (at least on ARM), because we don't have a start_info page at all. > >> + if (xen_pv_domain() && xen_start_info->store_evtchn) > >> + xenstored_ready = 1; > > This part can now just be moved unconditionally into case PV. What about: if (xen_pv_domain()) usage = PV; if (xen_hvm_domain()) usage = HVM; if (!xen_store_evtchn) usage = LOCAL; and moving xenstored_ready in case PV, like you suggested.
On 08/08/2012 12:51 PM, Stefano Stabellini wrote: > On Tue, 7 Aug 2012, Daniel De Graaf wrote: >> On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote: >>> On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote: >>>> bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not >>>> an error. >>>> >>>> If Linux is running as an HVM domain and is running as Dom0, use >>>> xenstored_local_init to initialize the xenstore page and event channel. >>>> >>>> Changes in v2: >>>> >>>> - refactor xenbus_init. >>> >>> Thank you. Lets also CC our friend at NSA who has been doing some work >>> in that area. Daniel are you OK with this change - will it still make >>> PV initial domain with with the MiniOS XenBus driver? >>> >>> Thanks. >> >> That case will work, but what this will break is launching the initial domain >> with a Xenstore stub domain already running (see below). >> >>>> >>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>>> --- >>>> drivers/xen/xenbus/xenbus_comms.c | 2 +- >>>> drivers/xen/xenbus/xenbus_probe.c | 62 +++++++++++++++++++++++++----------- >>>> drivers/xen/xenbus/xenbus_xs.c | 1 + >>>> 3 files changed, 45 insertions(+), 20 deletions(-) >>>> >>>> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c >>>> index 52fe7ad..c5aa55c 100644 >>>> --- a/drivers/xen/xenbus/xenbus_comms.c >>>> +++ b/drivers/xen/xenbus/xenbus_comms.c >>>> @@ -224,7 +224,7 @@ int xb_init_comms(void) >>>> int err; >>>> err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting, >>>> 0, "xenbus", &xb_waitq); >>>> - if (err <= 0) { >>>> + if (err < 0) { >>>> printk(KERN_ERR "XENBUS request irq failed %i\n", err); >>>> return err; >>>> } >>> >>>> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c >>>> index b793723..a67ccc0 100644 >>>> --- a/drivers/xen/xenbus/xenbus_probe.c >>>> +++ b/drivers/xen/xenbus/xenbus_probe.c >>>> @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void) >>>> return err; >>>> } >>>> >>>> +enum xenstore_init { >>>> + UNKNOWN, >>>> + PV, >>>> + HVM, >>>> + LOCAL, >>>> +}; >>>> static int __init xenbus_init(void) >>>> { >>>> int err = 0; >>>> + enum xenstore_init usage = UNKNOWN; >>>> + uint64_t v = 0; >>>> >>>> if (!xen_domain()) >>>> return -ENODEV; >>>> >>>> xenbus_ring_ops_init(); >>>> >>>> - if (xen_hvm_domain()) { >>>> - uint64_t v = 0; >>>> - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); >>>> - if (err) >>>> - goto out_error; >>>> - xen_store_evtchn = (int)v; >>>> - err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); >>>> - if (err) >>>> - goto out_error; >>>> - xen_store_mfn = (unsigned long)v; >>>> - xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); >>>> - } else { >>>> - xen_store_evtchn = xen_start_info->store_evtchn; >>>> - xen_store_mfn = xen_start_info->store_mfn; >>>> - if (xen_store_evtchn) >>>> - xenstored_ready = 1; >>>> - else { >>>> + if (xen_pv_domain()) >>>> + usage = PV; >>>> + if (xen_hvm_domain()) >>>> + usage = HVM; >> >> The above is correct for domUs, and is overridden for dom0s: >> >>>> + if (xen_hvm_domain() && xen_initial_domain()) >>>> + usage = LOCAL; >>>> + if (xen_pv_domain() && !xen_start_info->store_evtchn) >>>> + usage = LOCAL; >> >> Instead of these checks, I think it should just be: >> >> if (!xen_start_info->store_evtchn) >> usage = LOCAL; >> >> Any domain started after xenstore will have store_evtchn set, so if you don't >> have this set, you are either going to be running xenstore locally, or will >> use the ioctl to change it later (and so should still set up everything as if >> it will be running locally). > > That would be wrong for an HVM dom0 domain (at least on ARM), because > we don't have a start_info page at all. > > >>>> + if (xen_pv_domain() && xen_start_info->store_evtchn) >>>> + xenstored_ready = 1; >> >> This part can now just be moved unconditionally into case PV. > > What about: > > if (xen_pv_domain()) > usage = PV; > if (xen_hvm_domain()) > usage = HVM; > if (!xen_store_evtchn) > usage = LOCAL; > > and moving xenstored_ready in case PV, like you suggested. > That looks correct, but you'd need to split up the switch statement in order to populate xen_store_evtchn before that last condition, which ends up pretty much eliminating the usage variable.
On Wed, 8 Aug 2012, Daniel De Graaf wrote: > On 08/08/2012 12:51 PM, Stefano Stabellini wrote: > > On Tue, 7 Aug 2012, Daniel De Graaf wrote: > >> On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote: > >>> On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote: > >>>> bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not > >>>> an error. > >>>> > >>>> If Linux is running as an HVM domain and is running as Dom0, use > >>>> xenstored_local_init to initialize the xenstore page and event channel. > >>>> > >>>> Changes in v2: > >>>> > >>>> - refactor xenbus_init. > >>> > >>> Thank you. Lets also CC our friend at NSA who has been doing some work > >>> in that area. Daniel are you OK with this change - will it still make > >>> PV initial domain with with the MiniOS XenBus driver? > >>> > >>> Thanks. > >> > >> That case will work, but what this will break is launching the initial domain > >> with a Xenstore stub domain already running (see below). > >> > >>>> > >>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > >>>> --- > >>>> drivers/xen/xenbus/xenbus_comms.c | 2 +- > >>>> drivers/xen/xenbus/xenbus_probe.c | 62 +++++++++++++++++++++++++----------- > >>>> drivers/xen/xenbus/xenbus_xs.c | 1 + > >>>> 3 files changed, 45 insertions(+), 20 deletions(-) > >>>> > >>>> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c > >>>> index 52fe7ad..c5aa55c 100644 > >>>> --- a/drivers/xen/xenbus/xenbus_comms.c > >>>> +++ b/drivers/xen/xenbus/xenbus_comms.c > >>>> @@ -224,7 +224,7 @@ int xb_init_comms(void) > >>>> int err; > >>>> err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting, > >>>> 0, "xenbus", &xb_waitq); > >>>> - if (err <= 0) { > >>>> + if (err < 0) { > >>>> printk(KERN_ERR "XENBUS request irq failed %i\n", err); > >>>> return err; > >>>> } > >>> > >>>> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c > >>>> index b793723..a67ccc0 100644 > >>>> --- a/drivers/xen/xenbus/xenbus_probe.c > >>>> +++ b/drivers/xen/xenbus/xenbus_probe.c > >>>> @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void) > >>>> return err; > >>>> } > >>>> > >>>> +enum xenstore_init { > >>>> + UNKNOWN, > >>>> + PV, > >>>> + HVM, > >>>> + LOCAL, > >>>> +}; > >>>> static int __init xenbus_init(void) > >>>> { > >>>> int err = 0; > >>>> + enum xenstore_init usage = UNKNOWN; > >>>> + uint64_t v = 0; > >>>> > >>>> if (!xen_domain()) > >>>> return -ENODEV; > >>>> > >>>> xenbus_ring_ops_init(); > >>>> > >>>> - if (xen_hvm_domain()) { > >>>> - uint64_t v = 0; > >>>> - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); > >>>> - if (err) > >>>> - goto out_error; > >>>> - xen_store_evtchn = (int)v; > >>>> - err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); > >>>> - if (err) > >>>> - goto out_error; > >>>> - xen_store_mfn = (unsigned long)v; > >>>> - xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); > >>>> - } else { > >>>> - xen_store_evtchn = xen_start_info->store_evtchn; > >>>> - xen_store_mfn = xen_start_info->store_mfn; > >>>> - if (xen_store_evtchn) > >>>> - xenstored_ready = 1; > >>>> - else { > >>>> + if (xen_pv_domain()) > >>>> + usage = PV; > >>>> + if (xen_hvm_domain()) > >>>> + usage = HVM; > >> > >> The above is correct for domUs, and is overridden for dom0s: > >> > >>>> + if (xen_hvm_domain() && xen_initial_domain()) > >>>> + usage = LOCAL; > >>>> + if (xen_pv_domain() && !xen_start_info->store_evtchn) > >>>> + usage = LOCAL; > >> > >> Instead of these checks, I think it should just be: > >> > >> if (!xen_start_info->store_evtchn) > >> usage = LOCAL; > >> > >> Any domain started after xenstore will have store_evtchn set, so if you don't > >> have this set, you are either going to be running xenstore locally, or will > >> use the ioctl to change it later (and so should still set up everything as if > >> it will be running locally). > > > > That would be wrong for an HVM dom0 domain (at least on ARM), because > > we don't have a start_info page at all. > > > > > >>>> + if (xen_pv_domain() && xen_start_info->store_evtchn) > >>>> + xenstored_ready = 1; > >> > >> This part can now just be moved unconditionally into case PV. > > > > What about: > > > > if (xen_pv_domain()) > > usage = PV; > > if (xen_hvm_domain()) > > usage = HVM; > > if (!xen_store_evtchn) > > usage = LOCAL; > > > > and moving xenstored_ready in case PV, like you suggested. > > > > That looks correct, but you'd need to split up the switch statement in > order to populate xen_store_evtchn before that last condition, which > ends up pretty much eliminating the usage variable. Going back to what you wrote in the previous email, in what way this patch breaks the case when an initial domain is started after a Xenstore stub domain? Assuming that we are talking about a PV initial domain on x86, the following check if (xen_pv_domain() && !xen_start_info->store_evtchn) usage = LOCAL; will return false (because store_evtchn is set), therefore usage will remain set to PV. And the check: if (xen_pv_domain() && xen_start_info->store_evtchn) xenstored_ready = 1; will return true so xenstored_ready is going to be set to 1.
On 08/08/2012 01:19 PM, Stefano Stabellini wrote: > On Wed, 8 Aug 2012, Daniel De Graaf wrote: >> On 08/08/2012 12:51 PM, Stefano Stabellini wrote: >>> On Tue, 7 Aug 2012, Daniel De Graaf wrote: >>>> On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote: >>>>> On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote: >>>>>> bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not >>>>>> an error. >>>>>> >>>>>> If Linux is running as an HVM domain and is running as Dom0, use >>>>>> xenstored_local_init to initialize the xenstore page and event channel. >>>>>> >>>>>> Changes in v2: >>>>>> >>>>>> - refactor xenbus_init. >>>>> >>>>> Thank you. Lets also CC our friend at NSA who has been doing some work >>>>> in that area. Daniel are you OK with this change - will it still make >>>>> PV initial domain with with the MiniOS XenBus driver? >>>>> >>>>> Thanks. >>>> >>>> That case will work, but what this will break is launching the initial domain >>>> with a Xenstore stub domain already running (see below). >>>> >>>>>> >>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>>>>> --- >>>>>> drivers/xen/xenbus/xenbus_comms.c | 2 +- >>>>>> drivers/xen/xenbus/xenbus_probe.c | 62 +++++++++++++++++++++++++----------- >>>>>> drivers/xen/xenbus/xenbus_xs.c | 1 + >>>>>> 3 files changed, 45 insertions(+), 20 deletions(-) >>>>>> >>>>>> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c >>>>>> index 52fe7ad..c5aa55c 100644 >>>>>> --- a/drivers/xen/xenbus/xenbus_comms.c >>>>>> +++ b/drivers/xen/xenbus/xenbus_comms.c >>>>>> @@ -224,7 +224,7 @@ int xb_init_comms(void) >>>>>> int err; >>>>>> err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting, >>>>>> 0, "xenbus", &xb_waitq); >>>>>> - if (err <= 0) { >>>>>> + if (err < 0) { >>>>>> printk(KERN_ERR "XENBUS request irq failed %i\n", err); >>>>>> return err; >>>>>> } >>>>> >>>>>> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c >>>>>> index b793723..a67ccc0 100644 >>>>>> --- a/drivers/xen/xenbus/xenbus_probe.c >>>>>> +++ b/drivers/xen/xenbus/xenbus_probe.c >>>>>> @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void) >>>>>> return err; >>>>>> } >>>>>> >>>>>> +enum xenstore_init { >>>>>> + UNKNOWN, >>>>>> + PV, >>>>>> + HVM, >>>>>> + LOCAL, >>>>>> +}; >>>>>> static int __init xenbus_init(void) >>>>>> { >>>>>> int err = 0; >>>>>> + enum xenstore_init usage = UNKNOWN; >>>>>> + uint64_t v = 0; >>>>>> >>>>>> if (!xen_domain()) >>>>>> return -ENODEV; >>>>>> >>>>>> xenbus_ring_ops_init(); >>>>>> >>>>>> - if (xen_hvm_domain()) { >>>>>> - uint64_t v = 0; >>>>>> - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); >>>>>> - if (err) >>>>>> - goto out_error; >>>>>> - xen_store_evtchn = (int)v; >>>>>> - err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); >>>>>> - if (err) >>>>>> - goto out_error; >>>>>> - xen_store_mfn = (unsigned long)v; >>>>>> - xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); >>>>>> - } else { >>>>>> - xen_store_evtchn = xen_start_info->store_evtchn; >>>>>> - xen_store_mfn = xen_start_info->store_mfn; >>>>>> - if (xen_store_evtchn) >>>>>> - xenstored_ready = 1; >>>>>> - else { >>>>>> + if (xen_pv_domain()) >>>>>> + usage = PV; >>>>>> + if (xen_hvm_domain()) >>>>>> + usage = HVM; >>>> >>>> The above is correct for domUs, and is overridden for dom0s: >>>> >>>>>> + if (xen_hvm_domain() && xen_initial_domain()) >>>>>> + usage = LOCAL; >>>>>> + if (xen_pv_domain() && !xen_start_info->store_evtchn) >>>>>> + usage = LOCAL; >>>> >>>> Instead of these checks, I think it should just be: >>>> >>>> if (!xen_start_info->store_evtchn) >>>> usage = LOCAL; >>>> >>>> Any domain started after xenstore will have store_evtchn set, so if you don't >>>> have this set, you are either going to be running xenstore locally, or will >>>> use the ioctl to change it later (and so should still set up everything as if >>>> it will be running locally). >>> >>> That would be wrong for an HVM dom0 domain (at least on ARM), because >>> we don't have a start_info page at all. >>> >>> >>>>>> + if (xen_pv_domain() && xen_start_info->store_evtchn) >>>>>> + xenstored_ready = 1; >>>> >>>> This part can now just be moved unconditionally into case PV. >>> >>> What about: >>> >>> if (xen_pv_domain()) >>> usage = PV; >>> if (xen_hvm_domain()) >>> usage = HVM; >>> if (!xen_store_evtchn) >>> usage = LOCAL; >>> >>> and moving xenstored_ready in case PV, like you suggested. >>> >> >> That looks correct, but you'd need to split up the switch statement in >> order to populate xen_store_evtchn before that last condition, which >> ends up pretty much eliminating the usage variable. > > Going back to what you wrote in the previous email, in what way this > patch breaks the case when an initial domain is started after a Xenstore > stub domain? > > Assuming that we are talking about a PV initial domain on x86, the > following check > > if (xen_pv_domain() && !xen_start_info->store_evtchn) > usage = LOCAL; > > will return false (because store_evtchn is set), therefore usage will > remain set to PV. > And the check: > > if (xen_pv_domain() && xen_start_info->store_evtchn) > xenstored_ready = 1; > > will return true so xenstored_ready is going to be set to 1. > Right, the original patch didn't break anything with PV domains. The case it doesn't handle is an HVM initial domain with an already-running Xenstore domain; I think this applies both to ARM and hybrid/PVH on x86. In that case, usage would be set to LOCAL instead of HVM. As a side note: the value of xen_initial_domain() shouldn't be connected to determining if xenstore is running locally or not.
On Wed, 8 Aug 2012, Daniel De Graaf wrote: > On 08/08/2012 01:19 PM, Stefano Stabellini wrote: > > On Wed, 8 Aug 2012, Daniel De Graaf wrote: > >> On 08/08/2012 12:51 PM, Stefano Stabellini wrote: > >>> On Tue, 7 Aug 2012, Daniel De Graaf wrote: > >>>> On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote: > >>>>> On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote: > >>>>>> bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not > >>>>>> an error. > >>>>>> > >>>>>> If Linux is running as an HVM domain and is running as Dom0, use > >>>>>> xenstored_local_init to initialize the xenstore page and event channel. > >>>>>> > >>>>>> Changes in v2: > >>>>>> > >>>>>> - refactor xenbus_init. > >>>>> > >>>>> Thank you. Lets also CC our friend at NSA who has been doing some work > >>>>> in that area. Daniel are you OK with this change - will it still make > >>>>> PV initial domain with with the MiniOS XenBus driver? > >>>>> > >>>>> Thanks. > >>>> > >>>> That case will work, but what this will break is launching the initial domain > >>>> with a Xenstore stub domain already running (see below). > >>>> > >>>>>> > >>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > >>>>>> --- > >>>>>> drivers/xen/xenbus/xenbus_comms.c | 2 +- > >>>>>> drivers/xen/xenbus/xenbus_probe.c | 62 +++++++++++++++++++++++++----------- > >>>>>> drivers/xen/xenbus/xenbus_xs.c | 1 + > >>>>>> 3 files changed, 45 insertions(+), 20 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c > >>>>>> index 52fe7ad..c5aa55c 100644 > >>>>>> --- a/drivers/xen/xenbus/xenbus_comms.c > >>>>>> +++ b/drivers/xen/xenbus/xenbus_comms.c > >>>>>> @@ -224,7 +224,7 @@ int xb_init_comms(void) > >>>>>> int err; > >>>>>> err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting, > >>>>>> 0, "xenbus", &xb_waitq); > >>>>>> - if (err <= 0) { > >>>>>> + if (err < 0) { > >>>>>> printk(KERN_ERR "XENBUS request irq failed %i\n", err); > >>>>>> return err; > >>>>>> } > >>>>> > >>>>>> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c > >>>>>> index b793723..a67ccc0 100644 > >>>>>> --- a/drivers/xen/xenbus/xenbus_probe.c > >>>>>> +++ b/drivers/xen/xenbus/xenbus_probe.c > >>>>>> @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void) > >>>>>> return err; > >>>>>> } > >>>>>> > >>>>>> +enum xenstore_init { > >>>>>> + UNKNOWN, > >>>>>> + PV, > >>>>>> + HVM, > >>>>>> + LOCAL, > >>>>>> +}; > >>>>>> static int __init xenbus_init(void) > >>>>>> { > >>>>>> int err = 0; > >>>>>> + enum xenstore_init usage = UNKNOWN; > >>>>>> + uint64_t v = 0; > >>>>>> > >>>>>> if (!xen_domain()) > >>>>>> return -ENODEV; > >>>>>> > >>>>>> xenbus_ring_ops_init(); > >>>>>> > >>>>>> - if (xen_hvm_domain()) { > >>>>>> - uint64_t v = 0; > >>>>>> - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); > >>>>>> - if (err) > >>>>>> - goto out_error; > >>>>>> - xen_store_evtchn = (int)v; > >>>>>> - err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); > >>>>>> - if (err) > >>>>>> - goto out_error; > >>>>>> - xen_store_mfn = (unsigned long)v; > >>>>>> - xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); > >>>>>> - } else { > >>>>>> - xen_store_evtchn = xen_start_info->store_evtchn; > >>>>>> - xen_store_mfn = xen_start_info->store_mfn; > >>>>>> - if (xen_store_evtchn) > >>>>>> - xenstored_ready = 1; > >>>>>> - else { > >>>>>> + if (xen_pv_domain()) > >>>>>> + usage = PV; > >>>>>> + if (xen_hvm_domain()) > >>>>>> + usage = HVM; > >>>> > >>>> The above is correct for domUs, and is overridden for dom0s: > >>>> > >>>>>> + if (xen_hvm_domain() && xen_initial_domain()) > >>>>>> + usage = LOCAL; > >>>>>> + if (xen_pv_domain() && !xen_start_info->store_evtchn) > >>>>>> + usage = LOCAL; > >>>> > >>>> Instead of these checks, I think it should just be: > >>>> > >>>> if (!xen_start_info->store_evtchn) > >>>> usage = LOCAL; > >>>> > >>>> Any domain started after xenstore will have store_evtchn set, so if you don't > >>>> have this set, you are either going to be running xenstore locally, or will > >>>> use the ioctl to change it later (and so should still set up everything as if > >>>> it will be running locally). > >>> > >>> That would be wrong for an HVM dom0 domain (at least on ARM), because > >>> we don't have a start_info page at all. > >>> > >>> > >>>>>> + if (xen_pv_domain() && xen_start_info->store_evtchn) > >>>>>> + xenstored_ready = 1; > >>>> > >>>> This part can now just be moved unconditionally into case PV. > >>> > >>> What about: > >>> > >>> if (xen_pv_domain()) > >>> usage = PV; > >>> if (xen_hvm_domain()) > >>> usage = HVM; > >>> if (!xen_store_evtchn) > >>> usage = LOCAL; > >>> > >>> and moving xenstored_ready in case PV, like you suggested. > >>> > >> > >> That looks correct, but you'd need to split up the switch statement in > >> order to populate xen_store_evtchn before that last condition, which > >> ends up pretty much eliminating the usage variable. > > > > Going back to what you wrote in the previous email, in what way this > > patch breaks the case when an initial domain is started after a Xenstore > > stub domain? > > > > Assuming that we are talking about a PV initial domain on x86, the > > following check > > > > if (xen_pv_domain() && !xen_start_info->store_evtchn) > > usage = LOCAL; > > > > will return false (because store_evtchn is set), therefore usage will > > remain set to PV. > > And the check: > > > > if (xen_pv_domain() && xen_start_info->store_evtchn) > > xenstored_ready = 1; > > > > will return true so xenstored_ready is going to be set to 1. > > > > Right, the original patch didn't break anything with PV domains. The case > it doesn't handle is an HVM initial domain with an already-running > Xenstore domain; I think this applies both to ARM and hybrid/PVH on x86. > In that case, usage would be set to LOCAL instead of HVM. Right, however if I am not mistaken there is no such thing as an HVM dom0 right now on x86 and hybrid/PVH is probably going to return true on xen_pv_domain() and false on xen_hvm_domain(). In the ARM case, given that we don't have a start_info page, we would need another way to figure out whether a xenstore stub domain is already running, so I think we can just postpone the solution of that problem for now.
> > Right, the original patch didn't break anything with PV domains. The case > > it doesn't handle is an HVM initial domain with an already-running > > Xenstore domain; I think this applies both to ARM and hybrid/PVH on x86. > > In that case, usage would be set to LOCAL instead of HVM. > > > Right, however if I am not mistaken there is no such thing as an HVM > dom0 right now on x86 and hybrid/PVH is probably going to return true on > xen_pv_domain() and false on xen_hvm_domain(). The other way around. HVM = true, PV = false. Mukesh, correct me if I am wrong pls. > > In the ARM case, given that we don't have a start_info page, we would > need another way to figure out whether a xenstore stub domain is already > running, so I think we can just postpone the solution of that problem > for now.
diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c index 52fe7ad..c5aa55c 100644 --- a/drivers/xen/xenbus/xenbus_comms.c +++ b/drivers/xen/xenbus/xenbus_comms.c @@ -224,7 +224,7 @@ int xb_init_comms(void) int err; err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting, 0, "xenbus", &xb_waitq); - if (err <= 0) { + if (err < 0) { printk(KERN_ERR "XENBUS request irq failed %i\n", err); return err; } diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index b793723..a67ccc0 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void) return err; } +enum xenstore_init { + UNKNOWN, + PV, + HVM, + LOCAL, +}; static int __init xenbus_init(void) { int err = 0; + enum xenstore_init usage = UNKNOWN; + uint64_t v = 0; if (!xen_domain()) return -ENODEV; xenbus_ring_ops_init(); - if (xen_hvm_domain()) { - uint64_t v = 0; - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); - if (err) - goto out_error; - xen_store_evtchn = (int)v; - err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); - if (err) - goto out_error; - xen_store_mfn = (unsigned long)v; - xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); - } else { - xen_store_evtchn = xen_start_info->store_evtchn; - xen_store_mfn = xen_start_info->store_mfn; - if (xen_store_evtchn) - xenstored_ready = 1; - else { + if (xen_pv_domain()) + usage = PV; + if (xen_hvm_domain()) + usage = HVM; + if (xen_hvm_domain() && xen_initial_domain()) + usage = LOCAL; + if (xen_pv_domain() && !xen_start_info->store_evtchn) + usage = LOCAL; + if (xen_pv_domain() && xen_start_info->store_evtchn) + xenstored_ready = 1; + + switch (usage) { + case LOCAL: err = xenstored_local_init(); if (err) goto out_error; - } - xen_store_interface = mfn_to_virt(xen_store_mfn); + xen_store_interface = mfn_to_virt(xen_store_mfn); + break; + case PV: + xen_store_evtchn = xen_start_info->store_evtchn; + xen_store_mfn = xen_start_info->store_mfn; + xen_store_interface = mfn_to_virt(xen_store_mfn); + break; + case HVM: + err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); + if (err) + goto out_error; + xen_store_evtchn = (int)v; + err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); + if (err) + goto out_error; + xen_store_mfn = (unsigned long)v; + xen_store_interface = + ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); + break; + default: + pr_warn("Xenstore state unknown\n"); + break; } /* Initialize the interface to xenstore. */ diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c index d1c217b..f7feb3d 100644 --- a/drivers/xen/xenbus/xenbus_xs.c +++ b/drivers/xen/xenbus/xenbus_xs.c @@ -44,6 +44,7 @@ #include <linux/rwsem.h> #include <linux/module.h> #include <linux/mutex.h> +#include <asm/xen/hypervisor.h> #include <xen/xenbus.h> #include <xen/xen.h> #include "xenbus_comms.h"
bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not an error. If Linux is running as an HVM domain and is running as Dom0, use xenstored_local_init to initialize the xenstore page and event channel. Changes in v2: - refactor xenbus_init. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- drivers/xen/xenbus/xenbus_comms.c | 2 +- drivers/xen/xenbus/xenbus_probe.c | 62 +++++++++++++++++++++++++----------- drivers/xen/xenbus/xenbus_xs.c | 1 + 3 files changed, 45 insertions(+), 20 deletions(-)