| Message ID | 1366978377-16823-4-git-send-email-dingel@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 26.04.2013, at 14:12, Dominik Dingel wrote: > Check for a kernel IPL entry and load kernel image if one was > specified. > If no kernel image was supplied and the first boot device > is not a virtio-ccw-blk device, print error message and exit. > > In case it is a virtio-ccw-blk device store the dev_no in reg[7] I thought we want a boot order, not a singular device to boot off of? Alex > > Signed-off-by: Christian Paro <cparo@us.ibm.com> > Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com> > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index ace5ff5..9758529 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -16,6 +16,8 @@ > #include "elf.h" > #include "hw/loader.h" > #include "hw/sysbus.h" > +#include "hw/s390x/virtio-ccw.h" > +#include "hw/s390x/css.h" > > #define KERN_IMAGE_START 0x010000UL > #define KERN_PARM_AREA 0x010480UL > @@ -56,14 +58,25 @@ typedef struct S390IPLState { > char *firmware; > } S390IPLState; > > +static void s390_ipl_kernel(uint64_t pswaddr) > +{ > + S390CPU *cpu = S390_CPU(qemu_get_cpu(0)); > + CPUS390XState *env = &cpu->env; > + > + env->psw.addr = pswaddr; > + env->psw.mask = IPL_PSW_MASK; > + s390_add_running_cpu(cpu); > +} > > -static void s390_ipl_cpu(uint64_t pswaddr) > +static void s390_ipl_from_disk(VirtIOBlkCcw *dev, uint64_t pswaddr) > { > S390CPU *cpu = S390_CPU(qemu_get_cpu(0)); > CPUS390XState *env = &cpu->env; > + VirtioCcwDevice *ccw_dev = &(dev->parent_obj); > > env->psw.addr = pswaddr; > env->psw.mask = IPL_PSW_MASK; > + env->regs[7] = ccw_dev->sch->devno; > s390_add_running_cpu(cpu); > } > > @@ -152,7 +165,18 @@ static void s390_ipl_reset(DeviceState *dev) > { > S390IPLState *ipl = S390_IPL(dev); > > - s390_ipl_cpu(ipl->start_addr); > + if (ipl->kernel) { > + return s390_ipl_kernel(ipl->start_addr); > + } > + > + DeviceState *boot_device = get_boot_device(0); > + if (object_dynamic_cast(OBJECT(boot_device), TYPE_VIRTIO_BLK) != NULL) { > + s390_ipl_from_disk(VIRTIO_BLK_CCW(boot_device->parent_obj.parent), > + ipl->start_addr); > + } else { > + fprintf(stderr, "qemu: s390x only supports boot from virtio-ccw-blk\n"); > + exit(1); > + } > } > > static void s390_ipl_class_init(ObjectClass *klass, void *data) > -- > 1.7.9.5 >
On Fri, 26 Apr 2013 17:22:08 +0200 Alexander Graf <agraf@suse.de> wrote: > > On 26.04.2013, at 14:12, Dominik Dingel wrote: > > > Check for a kernel IPL entry and load kernel image if one was > > specified. > > If no kernel image was supplied and the first boot device > > is not a virtio-ccw-blk device, print error message and exit. > > > > In case it is a virtio-ccw-blk device store the dev_no in reg[7] > > I thought we want a boot order, not a singular device to boot off of? > > Alex The current changes allow us to boot what we want (device and boot program selection). Later on we can think of fallback handling. I'm currently thinking about something like a hypercall to communicate to the host that we can not boot from the supplied device. Then the host could decide based on a policy if it tries the next device or stops or something like that. But that is a change we should discuss. Dominik > > > > > Signed-off-by: Christian Paro <cparo@us.ibm.com> > > Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com> > > > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > > index ace5ff5..9758529 100644 > > --- a/hw/s390x/ipl.c > > +++ b/hw/s390x/ipl.c > > @@ -16,6 +16,8 @@ > > #include "elf.h" > > #include "hw/loader.h" > > #include "hw/sysbus.h" > > +#include "hw/s390x/virtio-ccw.h" > > +#include "hw/s390x/css.h" > > > > #define KERN_IMAGE_START 0x010000UL > > #define KERN_PARM_AREA 0x010480UL > > @@ -56,14 +58,25 @@ typedef struct S390IPLState { > > char *firmware; > > } S390IPLState; > > > > +static void s390_ipl_kernel(uint64_t pswaddr) > > +{ > > + S390CPU *cpu = S390_CPU(qemu_get_cpu(0)); > > + CPUS390XState *env = &cpu->env; > > + > > + env->psw.addr = pswaddr; > > + env->psw.mask = IPL_PSW_MASK; > > + s390_add_running_cpu(cpu); > > +} > > > > -static void s390_ipl_cpu(uint64_t pswaddr) > > +static void s390_ipl_from_disk(VirtIOBlkCcw *dev, uint64_t pswaddr) > > { > > S390CPU *cpu = S390_CPU(qemu_get_cpu(0)); > > CPUS390XState *env = &cpu->env; > > + VirtioCcwDevice *ccw_dev = &(dev->parent_obj); > > > > env->psw.addr = pswaddr; > > env->psw.mask = IPL_PSW_MASK; > > + env->regs[7] = ccw_dev->sch->devno; > > s390_add_running_cpu(cpu); > > } > > > > @@ -152,7 +165,18 @@ static void s390_ipl_reset(DeviceState *dev) > > { > > S390IPLState *ipl = S390_IPL(dev); > > > > - s390_ipl_cpu(ipl->start_addr); > > + if (ipl->kernel) { > > + return s390_ipl_kernel(ipl->start_addr); > > + } > > + > > + DeviceState *boot_device = get_boot_device(0); > > + if (object_dynamic_cast(OBJECT(boot_device), TYPE_VIRTIO_BLK) != NULL) { > > + s390_ipl_from_disk(VIRTIO_BLK_CCW(boot_device->parent_obj.parent), > > + ipl->start_addr); > > + } else { > > + fprintf(stderr, "qemu: s390x only supports boot from virtio-ccw-blk\n"); > > + exit(1); > > + } > > } > > > > static void s390_ipl_class_init(ObjectClass *klass, void *data) > > -- > > 1.7.9.5 > > >
On 26/04/13 17:22, Alexander Graf wrote: > > On 26.04.2013, at 14:12, Dominik Dingel wrote: > >> Check for a kernel IPL entry and load kernel image if one was >> specified. >> If no kernel image was supplied and the first boot device >> is not a virtio-ccw-blk device, print error message and exit. >> >> In case it is a virtio-ccw-blk device store the dev_no in reg[7] > > I thought we want a boot order, not a singular device to boot off of? > > Alex First of all we want to be able to choose a boot device as a first step. With this patch the user is able to use libvirt and friends to choose the disk to boot from. The nice approach with this bios/ipl device is that we can update both in lock-step so this reg7 interface is not an ABI. So in a future version we actually could: a: implement diag 308 subcode 5/6, which would enable /sys/devices/firmware/[ipl|reipl] just like on z/VM or LPAR (this allows to reboot from a different device). b: implement a list. b looks nice, but I actually prefer a for two reasons: 1. be closer to the real hw 2. predictability but we can certainly discuss this. So I suggest to go with this patch and implement later on what we agree upon? Christian > >> >> Signed-off-by: Christian Paro <cparo@us.ibm.com> >> Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com> >> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >> index ace5ff5..9758529 100644 >> --- a/hw/s390x/ipl.c >> +++ b/hw/s390x/ipl.c >> @@ -16,6 +16,8 @@ >> #include "elf.h" >> #include "hw/loader.h" >> #include "hw/sysbus.h" >> +#include "hw/s390x/virtio-ccw.h" >> +#include "hw/s390x/css.h" >> >> #define KERN_IMAGE_START 0x010000UL >> #define KERN_PARM_AREA 0x010480UL >> @@ -56,14 +58,25 @@ typedef struct S390IPLState { >> char *firmware; >> } S390IPLState; >> >> +static void s390_ipl_kernel(uint64_t pswaddr) >> +{ >> + S390CPU *cpu = S390_CPU(qemu_get_cpu(0)); >> + CPUS390XState *env = &cpu->env; >> + >> + env->psw.addr = pswaddr; >> + env->psw.mask = IPL_PSW_MASK; >> + s390_add_running_cpu(cpu); >> +} >> >> -static void s390_ipl_cpu(uint64_t pswaddr) >> +static void s390_ipl_from_disk(VirtIOBlkCcw *dev, uint64_t pswaddr) >> { >> S390CPU *cpu = S390_CPU(qemu_get_cpu(0)); >> CPUS390XState *env = &cpu->env; >> + VirtioCcwDevice *ccw_dev = &(dev->parent_obj); >> >> env->psw.addr = pswaddr; >> env->psw.mask = IPL_PSW_MASK; >> + env->regs[7] = ccw_dev->sch->devno; >> s390_add_running_cpu(cpu); >> } >> >> @@ -152,7 +165,18 @@ static void s390_ipl_reset(DeviceState *dev) >> { >> S390IPLState *ipl = S390_IPL(dev); >> >> - s390_ipl_cpu(ipl->start_addr); >> + if (ipl->kernel) { >> + return s390_ipl_kernel(ipl->start_addr); >> + } >> + >> + DeviceState *boot_device = get_boot_device(0); >> + if (object_dynamic_cast(OBJECT(boot_device), TYPE_VIRTIO_BLK) != NULL) { >> + s390_ipl_from_disk(VIRTIO_BLK_CCW(boot_device->parent_obj.parent), >> + ipl->start_addr); >> + } else { >> + fprintf(stderr, "qemu: s390x only supports boot from virtio-ccw-blk\n"); >> + exit(1); >> + } >> } >> >> static void s390_ipl_class_init(ObjectClass *klass, void *data) >> -- >> 1.7.9.5 >> >
On 26.04.2013, at 17:42, Christian Borntraeger wrote: > On 26/04/13 17:22, Alexander Graf wrote: >> >> On 26.04.2013, at 14:12, Dominik Dingel wrote: >> >>> Check for a kernel IPL entry and load kernel image if one was >>> specified. >>> If no kernel image was supplied and the first boot device >>> is not a virtio-ccw-blk device, print error message and exit. >>> >>> In case it is a virtio-ccw-blk device store the dev_no in reg[7] >> >> I thought we want a boot order, not a singular device to boot off of? >> >> Alex > > First of all we want to be able to choose a boot device as a first step. > With this patch the user is able to use libvirt and friends to choose the > disk to boot from. > > The nice approach with this bios/ipl device is that we can update both > in lock-step so this reg7 interface is not an ABI. > > So in a future version we actually could: > a: implement diag 308 subcode 5/6, which would enable /sys/devices/firmware/[ipl|reipl] > just like on z/VM or LPAR (this allows to reboot from a different device). > b: implement a list. > > b looks nice, but I actually prefer a for two reasons: > 1. be closer to the real hw > 2. predictability > but we can certainly discuss this. > > So I suggest to go with this patch and implement later on what we > agree upon? I don't see how having "first device we find" is any better than a rushed interface we need to agree on right before 1.5 hard freeze. Let's just release 1.5 with the very simple one and then go for something awesome in the next version. Alex
On 26/04/13 17:48, Alexander Graf wrote: >> So I suggest to go with this patch and implement later on what we >> agree upon? > > I don't see how having "first device we find" is any better than a > rushed interface we need to agree on right before 1.5 hard freeze. Exactly, find first device isnt better ;-) See, the current code chooses the first subchannel that matches. This boils down to "a random disk" as soon as you have more than one. With this patch the user can at least specify the devno of the disk.It even works out of the box with libvirt. Let's just release 1.5 with the very simple one and then go for something awesome in the next version. Exactly - and having a list is more in the awesome area. Beiing able to specify the first disk and pass that in a register to the bios is of course not a full-features interface, but it works and can be changed. Just to double check: do you agree that we can change the interface between ipl device and bios at any time? Christian
On Fri, 26 Apr 2013 17:48:37 +0200 Alexander Graf <agraf@suse.de> wrote: > > On 26.04.2013, at 17:42, Christian Borntraeger wrote: > > > On 26/04/13 17:22, Alexander Graf wrote: > >> > >> On 26.04.2013, at 14:12, Dominik Dingel wrote: > >> > >>> Check for a kernel IPL entry and load kernel image if one was > >>> specified. > >>> If no kernel image was supplied and the first boot device > >>> is not a virtio-ccw-blk device, print error message and exit. > >>> > >>> In case it is a virtio-ccw-blk device store the dev_no in reg[7] > >> > >> I thought we want a boot order, not a singular device to boot off of? > >> > >> Alex > > > > First of all we want to be able to choose a boot device as a first step. > > With this patch the user is able to use libvirt and friends to choose the > > disk to boot from. > > > > The nice approach with this bios/ipl device is that we can update both > > in lock-step so this reg7 interface is not an ABI. > > > > So in a future version we actually could: > > a: implement diag 308 subcode 5/6, which would enable /sys/devices/firmware/[ipl|reipl] > > just like on z/VM or LPAR (this allows to reboot from a different device). > > b: implement a list. > > > > b looks nice, but I actually prefer a for two reasons: > > 1. be closer to the real hw > > 2. predictability > > but we can certainly discuss this. > > > > So I suggest to go with this patch and implement later on what we > > agree upon? > > I don't see how having "first device we find" is any better than a rushed interface we need to agree on right before 1.5 hard freeze. Let's just release 1.5 with the very simple one and then go for something awesome in the next version. > > > Alex Okay, I like more the evolution over the revolution kind of approach. So this patchset does include a few improvements over the :"boot the first blk device we see" system. We can specify explicitly with the boot index the device to boot and with loadparm even the boot entry. And I also see it like Christian that this is not really an interface, as it should be never exposed to the outside. We only enable the outside to use boot index, which is clearly stated in docs/bootindex.txt and in a later addition we will enable fallbacks. Dominik Dominik
On 26.04.2013, at 18:01, Christian Borntraeger wrote: > On 26/04/13 17:48, Alexander Graf wrote: > >>> So I suggest to go with this patch and implement later on what we >>> agree upon? >> >> I don't see how having "first device we find" is any better than a >> rushed interface we need to agree on right before 1.5 hard freeze. > > Exactly, find first device isnt better ;-) > See, the current code chooses the first subchannel that matches. This > boils down to "a random disk" as soon as you have more than one. > > With this patch the user can at least specify the devno of the disk.It > even works out of the box with libvirt. > > Let's just release 1.5 with the very simple one and then go for something > awesome in the next version. > > Exactly - and having a list is more in the awesome area. Beiing able to > specify the first disk and pass that in a register to the bios is of > course not a full-features interface, but it works and can be changed. My main concern is backwards compatibility. If we introduce a command line interface now, we have to support it forever. I'd rather only support one interface, rather than 2 out of which one is only legacy for 1.5 compatibility. > Just to double check: do you agree that we can change the interface between > ipl device and bios at any time? I think that's a fair thing to do, though I wouldn't encourage it. Alex
On 26.04.2013, at 18:04, Dominik Dingel wrote: > On Fri, 26 Apr 2013 17:48:37 +0200 > Alexander Graf <agraf@suse.de> wrote: > >> >> On 26.04.2013, at 17:42, Christian Borntraeger wrote: >> >>> On 26/04/13 17:22, Alexander Graf wrote: >>>> >>>> On 26.04.2013, at 14:12, Dominik Dingel wrote: >>>> >>>>> Check for a kernel IPL entry and load kernel image if one was >>>>> specified. >>>>> If no kernel image was supplied and the first boot device >>>>> is not a virtio-ccw-blk device, print error message and exit. >>>>> >>>>> In case it is a virtio-ccw-blk device store the dev_no in reg[7] >>>> >>>> I thought we want a boot order, not a singular device to boot off of? >>>> >>>> Alex >>> >>> First of all we want to be able to choose a boot device as a first step. >>> With this patch the user is able to use libvirt and friends to choose the >>> disk to boot from. >>> >>> The nice approach with this bios/ipl device is that we can update both >>> in lock-step so this reg7 interface is not an ABI. >>> >>> So in a future version we actually could: >>> a: implement diag 308 subcode 5/6, which would enable /sys/devices/firmware/[ipl|reipl] >>> just like on z/VM or LPAR (this allows to reboot from a different device). >>> b: implement a list. >>> >>> b looks nice, but I actually prefer a for two reasons: >>> 1. be closer to the real hw >>> 2. predictability >>> but we can certainly discuss this. >>> >>> So I suggest to go with this patch and implement later on what we >>> agree upon? >> >> I don't see how having "first device we find" is any better than a rushed interface we need to agree on right before 1.5 hard freeze. Let's just release 1.5 with the very simple one and then go for something awesome in the next version. >> >> >> Alex > > Okay, I like more the evolution over the revolution kind of approach. So this patchset does include a few improvements over the :"boot the first blk device we see" system. > We can specify explicitly with the boot index the device to boot and with loadparm even the boot entry. > > And I also see it like Christian that this is not really an interface, as it should be never exposed to the outside. We only enable the outside to use boot index, which is clearly stated in docs/bootindex.txt and in a later addition we will enable fallbacks. Is there any particular reason this can't wait a few weeks, go in early in the 1.6 development cycle and then we see where it carries us? Alex
On Fri, 26 Apr 2013 18:05:02 +0200 Alexander Graf <agraf@suse.de> wrote: > > On 26.04.2013, at 18:01, Christian Borntraeger wrote: > > > On 26/04/13 17:48, Alexander Graf wrote: > > > >>> So I suggest to go with this patch and implement later on what we > >>> agree upon? > >> > >> I don't see how having "first device we find" is any better than a > >> rushed interface we need to agree on right before 1.5 hard freeze. > > > > Exactly, find first device isnt better ;-) > > See, the current code chooses the first subchannel that matches. This > > boils down to "a random disk" as soon as you have more than one. > > > > With this patch the user can at least specify the devno of the disk.It > > even works out of the box with libvirt. > > > > Let's just release 1.5 with the very simple one and then go for something > > awesome in the next version. > > > > Exactly - and having a list is more in the awesome area. Beiing able to > > specify the first disk and pass that in a register to the bios is of > > course not a full-features interface, but it works and can be changed. > > My main concern is backwards compatibility. If we introduce a command line interface now, we have to support it forever. I'd rather only support one interface, rather than 2 out of which one is only legacy for 1.5 compatibility. We only enable, don't introduce the existing command line interface for bootindex. The loadparm thing is already kind of list, as the loadparm is stored with every device. But as I wrote in the cover letter, we also could just for the start only implement the bootindex. > > > Just to double check: do you agree that we can change the interface between > > ipl device and bios at any time? > > I think that's a fair thing to do, though I wouldn't encourage it. > > > Alex > Dominik
On 26/04/13 18:05, Alexander Graf wrote: > > On 26.04.2013, at 18:01, Christian Borntraeger wrote: > >> On 26/04/13 17:48, Alexander Graf wrote: >> >>>> So I suggest to go with this patch and implement later on what we >>>> agree upon? >>> >>> I don't see how having "first device we find" is any better than a >>> rushed interface we need to agree on right before 1.5 hard freeze. >> >> Exactly, find first device isnt better ;-) >> See, the current code chooses the first subchannel that matches. This >> boils down to "a random disk" as soon as you have more than one. >> >> With this patch the user can at least specify the devno of the disk.It >> even works out of the box with libvirt. >> >> Let's just release 1.5 with the very simple one and then go for something >> awesome in the next version. >> >> Exactly - and having a list is more in the awesome area. Beiing able to >> specify the first disk and pass that in a register to the bios is of >> course not a full-features interface, but it works and can be changed. > > My main concern is backwards compatibility. If we introduce a command line > interface now, we have to support it forever. I'd rather only support one > interface, rather than 2 out of which one is only legacy for 1.5 compatibility. The cool thing is, that we dont introduce a command line interface in this patch. We just use the existing bootindex.
On 26.04.2013, at 18:10, Dominik Dingel wrote: > On Fri, 26 Apr 2013 18:05:02 +0200 > Alexander Graf <agraf@suse.de> wrote: > >> >> On 26.04.2013, at 18:01, Christian Borntraeger wrote: >> >>> On 26/04/13 17:48, Alexander Graf wrote: >>> >>>>> So I suggest to go with this patch and implement later on what we >>>>> agree upon? >>>> >>>> I don't see how having "first device we find" is any better than a >>>> rushed interface we need to agree on right before 1.5 hard freeze. >>> >>> Exactly, find first device isnt better ;-) >>> See, the current code chooses the first subchannel that matches. This >>> boils down to "a random disk" as soon as you have more than one. >>> >>> With this patch the user can at least specify the devno of the disk.It >>> even works out of the box with libvirt. >>> >>> Let's just release 1.5 with the very simple one and then go for something >>> awesome in the next version. >>> >>> Exactly - and having a list is more in the awesome area. Beiing able to >>> specify the first disk and pass that in a register to the bios is of >>> course not a full-features interface, but it works and can be changed. >> >> My main concern is backwards compatibility. If we introduce a command line interface now, we have to support it forever. I'd rather only support one interface, rather than 2 out of which one is only legacy for 1.5 compatibility. > > We only enable, don't introduce the existing command line interface for bootindex. The loadparm thing is already kind of list, as the loadparm is stored with every device. > But as I wrote in the cover letter, we also could just for the start only implement the bootindex. The bootindex is the part that I'm reluctant about. I think it's the right way forward, but it touches generic code and generic infrastructure a few days before hard freeze. I don't think it's important enough to justify this. Alex
On 26.04.2013, at 18:11, Christian Borntraeger wrote: > On 26/04/13 18:05, Alexander Graf wrote: >> >> On 26.04.2013, at 18:01, Christian Borntraeger wrote: >> >>> On 26/04/13 17:48, Alexander Graf wrote: >>> >>>>> So I suggest to go with this patch and implement later on what we >>>>> agree upon? >>>> >>>> I don't see how having "first device we find" is any better than a >>>> rushed interface we need to agree on right before 1.5 hard freeze. >>> >>> Exactly, find first device isnt better ;-) >>> See, the current code chooses the first subchannel that matches. This >>> boils down to "a random disk" as soon as you have more than one. >>> >>> With this patch the user can at least specify the devno of the disk.It >>> even works out of the box with libvirt. >>> >>> Let's just release 1.5 with the very simple one and then go for something >>> awesome in the next version. >>> >>> Exactly - and having a list is more in the awesome area. Beiing able to >>> specify the first disk and pass that in a register to the bios is of >>> course not a full-features interface, but it works and can be changed. >> >> My main concern is backwards compatibility. If we introduce a command line >> interface now, we have to support it forever. I'd rather only support one >> interface, rather than 2 out of which one is only legacy for 1.5 compatibility. > > The cool thing is, that we dont introduce a command line interface in this patch. > We just use the existing bootindex. Get an ack from Anthony on the bootindex thing and I'm fine with pulling that in. Alex
Alexander Graf <agraf@suse.de> writes: > On 26.04.2013, at 18:11, Christian Borntraeger wrote: > >> On 26/04/13 18:05, Alexander Graf wrote: >>> >>> On 26.04.2013, at 18:01, Christian Borntraeger wrote: >>> >>>> On 26/04/13 17:48, Alexander Graf wrote: >>>> >>>>>> So I suggest to go with this patch and implement later on what we >>>>>> agree upon? >>>>> >>>>> I don't see how having "first device we find" is any better than a >>>>> rushed interface we need to agree on right before 1.5 hard freeze. >>>> >>>> Exactly, find first device isnt better ;-) >>>> See, the current code chooses the first subchannel that matches. This >>>> boils down to "a random disk" as soon as you have more than one. >>>> >>>> With this patch the user can at least specify the devno of the disk.It >>>> even works out of the box with libvirt. >>>> >>>> Let's just release 1.5 with the very simple one and then go for something >>>> awesome in the next version. >>>> >>>> Exactly - and having a list is more in the awesome area. Beiing able to >>>> specify the first disk and pass that in a register to the bios is of >>>> course not a full-features interface, but it works and can be changed. >>> >>> My main concern is backwards compatibility. If we introduce a command line >>> interface now, we have to support it forever. I'd rather only support one >>> interface, rather than 2 out of which one is only legacy for 1.5 compatibility. >> >> The cool thing is, that we dont introduce a command line interface in this patch. >> We just use the existing bootindex. > > Get an ack from Anthony on the bootindex thing and I'm fine with > pulling that in. So I don't see a problem with the no-fallback behavior (nor with it changing down the road), but the bootindex change would cause a regression on x86. Regards, Anthony Liguori > > > Alex
On 26.04.2013, at 14:12, Dominik Dingel wrote: > Check for a kernel IPL entry and load kernel image if one was > specified. > If no kernel image was supplied and the first boot device > is not a virtio-ccw-blk device, print error message and exit. > > In case it is a virtio-ccw-blk device store the dev_no in reg[7] > > Signed-off-by: Christian Paro <cparo@us.ibm.com> > Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com> > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index ace5ff5..9758529 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -16,6 +16,8 @@ > #include "elf.h" > #include "hw/loader.h" > #include "hw/sysbus.h" > +#include "hw/s390x/virtio-ccw.h" > +#include "hw/s390x/css.h" > > #define KERN_IMAGE_START 0x010000UL > #define KERN_PARM_AREA 0x010480UL > @@ -56,14 +58,25 @@ typedef struct S390IPLState { > char *firmware; > } S390IPLState; > > +static void s390_ipl_kernel(uint64_t pswaddr) > +{ > + S390CPU *cpu = S390_CPU(qemu_get_cpu(0)); > + CPUS390XState *env = &cpu->env; > + > + env->psw.addr = pswaddr; > + env->psw.mask = IPL_PSW_MASK; > + s390_add_running_cpu(cpu); > +} > > -static void s390_ipl_cpu(uint64_t pswaddr) > +static void s390_ipl_from_disk(VirtIOBlkCcw *dev, uint64_t pswaddr) > { > S390CPU *cpu = S390_CPU(qemu_get_cpu(0)); > CPUS390XState *env = &cpu->env; > + VirtioCcwDevice *ccw_dev = &(dev->parent_obj); > > env->psw.addr = pswaddr; > env->psw.mask = IPL_PSW_MASK; > + env->regs[7] = ccw_dev->sch->devno; > s390_add_running_cpu(cpu); > } > > @@ -152,7 +165,18 @@ static void s390_ipl_reset(DeviceState *dev) > { > S390IPLState *ipl = S390_IPL(dev); > > - s390_ipl_cpu(ipl->start_addr); > + if (ipl->kernel) { > + return s390_ipl_kernel(ipl->start_addr); The kernel/bootloader split is too high level. Please reuse more code. The flow should be something like if (ipl_from_disk) { regs[7] = devno; } env->psw.addr = pswaddr; mask = IPL_PSW_MASK; add_running_cpu(); Whether you split any of these into functions is your thing, but I don't want to have the PSW reset logic duplicated. Alex > + } > + > + DeviceState *boot_device = get_boot_device(0); > + if (object_dynamic_cast(OBJECT(boot_device), TYPE_VIRTIO_BLK) != NULL) { > + s390_ipl_from_disk(VIRTIO_BLK_CCW(boot_device->parent_obj.parent), > + ipl->start_addr); > + } else { > + fprintf(stderr, "qemu: s390x only supports boot from virtio-ccw-blk\n"); > + exit(1); > + } > } > > static void s390_ipl_class_init(ObjectClass *klass, void *data) > -- > 1.7.9.5 >
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index ace5ff5..9758529 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -16,6 +16,8 @@ #include "elf.h" #include "hw/loader.h" #include "hw/sysbus.h" +#include "hw/s390x/virtio-ccw.h" +#include "hw/s390x/css.h" #define KERN_IMAGE_START 0x010000UL #define KERN_PARM_AREA 0x010480UL @@ -56,14 +58,25 @@ typedef struct S390IPLState { char *firmware; } S390IPLState; +static void s390_ipl_kernel(uint64_t pswaddr) +{ + S390CPU *cpu = S390_CPU(qemu_get_cpu(0)); + CPUS390XState *env = &cpu->env; + + env->psw.addr = pswaddr; + env->psw.mask = IPL_PSW_MASK; + s390_add_running_cpu(cpu); +} -static void s390_ipl_cpu(uint64_t pswaddr) +static void s390_ipl_from_disk(VirtIOBlkCcw *dev, uint64_t pswaddr) { S390CPU *cpu = S390_CPU(qemu_get_cpu(0)); CPUS390XState *env = &cpu->env; + VirtioCcwDevice *ccw_dev = &(dev->parent_obj); env->psw.addr = pswaddr; env->psw.mask = IPL_PSW_MASK; + env->regs[7] = ccw_dev->sch->devno; s390_add_running_cpu(cpu); } @@ -152,7 +165,18 @@ static void s390_ipl_reset(DeviceState *dev) { S390IPLState *ipl = S390_IPL(dev); - s390_ipl_cpu(ipl->start_addr); + if (ipl->kernel) { + return s390_ipl_kernel(ipl->start_addr); + } + + DeviceState *boot_device = get_boot_device(0); + if (object_dynamic_cast(OBJECT(boot_device), TYPE_VIRTIO_BLK) != NULL) { + s390_ipl_from_disk(VIRTIO_BLK_CCW(boot_device->parent_obj.parent), + ipl->start_addr); + } else { + fprintf(stderr, "qemu: s390x only supports boot from virtio-ccw-blk\n"); + exit(1); + } } static void s390_ipl_class_init(ObjectClass *klass, void *data)