Patchwork [09/10] S390: Pass per-device loadparm values for CCW blk and net devs.

login
register
mail settings
Submitter Dominik Dingel
Date April 26, 2013, 12:12 p.m.
Message ID <1366978377-16823-10-git-send-email-dingel@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/239837/
State New
Headers show

Comments

Dominik Dingel - April 26, 2013, 12:12 p.m.
From: Christian Paro <cparo@us.ibm.com>

Provide a loadparm property which can be used to pass IPL load
parameters on a per-device basis for VirtioCcwDevice instances
representing block and network devices.

Signed-off-by: Christian Paro <cparo@us.ibm.com>
Alexander Graf - April 26, 2013, 4:52 p.m.
On 26.04.2013, at 14:12, Dominik Dingel wrote:

> From: Christian Paro <cparo@us.ibm.com>
> 
> Provide a loadparm property which can be used to pass IPL load
> parameters on a per-device basis for VirtioCcwDevice instances
> representing block and network devices.
> 
> Signed-off-by: Christian Paro <cparo@us.ibm.com>
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 56539d3..23d042f 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -791,6 +791,7 @@ static const VirtIOBindings virtio_ccw_bindings = {
> 
> static Property virtio_ccw_net_properties[] = {
>     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> +    DEFINE_PROP_UINT32("loadparm", VirtioCcwDevice, loadparm, 0),

We don't support netboot yet, right? Also is "loadparm" an architected name? Wouldn't "bootmap_id" or something that actually tells the user what's going on fit better?


Alex

>     DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]),
>     DEFINE_VIRTIO_NET_PROPERTIES(VirtIONetCcw, vdev.net_conf),
>     DEFINE_NIC_PROPERTIES(VirtIONetCcw, vdev.nic_conf),
> @@ -818,6 +819,7 @@ static const TypeInfo virtio_ccw_net = {
> 
> static Property virtio_ccw_blk_properties[] = {
>     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> +    DEFINE_PROP_UINT32("loadparm", VirtioCcwDevice, loadparm, 0),
>     DEFINE_VIRTIO_BLK_FEATURES(VirtioCcwDevice, host_features[0]),
>     DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, blk),
>     DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
> index 84055e7..a3f706a 100644
> --- a/hw/s390x/virtio-ccw.h
> +++ b/hw/s390x/virtio-ccw.h
> @@ -76,6 +76,7 @@ struct VirtioCcwDevice {
>     SubchDev *sch;
>     VirtIODevice *vdev;
>     char *bus_id;
> +    uint32_t loadparm;
>     uint32_t host_features[VIRTIO_CCW_FEATURE_SIZE];
>     VirtIORNGConf rng;
>     VirtioBusState bus;
> -- 
> 1.7.9.5
>
Dominik Dingel - April 26, 2013, 6:08 p.m.
On Fri, 26 Apr 2013 18:52:48 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> On 26.04.2013, at 14:12, Dominik Dingel wrote:
> 
> > From: Christian Paro <cparo@us.ibm.com>
> > 
> > Provide a loadparm property which can be used to pass IPL load
> > parameters on a per-device basis for VirtioCcwDevice instances
> > representing block and network devices.
> > 
> > Signed-off-by: Christian Paro <cparo@us.ibm.com>
> > 
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index 56539d3..23d042f 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -791,6 +791,7 @@ static const VirtIOBindings virtio_ccw_bindings = {
> > 
> > static Property virtio_ccw_net_properties[] = {
> >     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> > +    DEFINE_PROP_UINT32("loadparm", VirtioCcwDevice, loadparm, 0),
> 
> We don't support netboot yet, right? Also is "loadparm" an architected name? Wouldn't "bootmap_id" or something that actually tells the user what's going on fit better?
> 
> 
> Alex

Christian had the idea to name it subindex, but I don't know, to be honest, I like the general open solution more, especially for the commandline interface. What if we later on want to change the behaviour, loadparm, or better biosparameter is so general... where bootmap_id is narrowed down.

Dominik 


> >     DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]),
> >     DEFINE_VIRTIO_NET_PROPERTIES(VirtIONetCcw, vdev.net_conf),
> >     DEFINE_NIC_PROPERTIES(VirtIONetCcw, vdev.nic_conf),
> > @@ -818,6 +819,7 @@ static const TypeInfo virtio_ccw_net = {
> > 
> > static Property virtio_ccw_blk_properties[] = {
> >     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> > +    DEFINE_PROP_UINT32("loadparm", VirtioCcwDevice, loadparm, 0),
> >     DEFINE_VIRTIO_BLK_FEATURES(VirtioCcwDevice, host_features[0]),
> >     DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, blk),
> >     DEFINE_PROP_END_OF_LIST(),
> > diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
> > index 84055e7..a3f706a 100644
> > --- a/hw/s390x/virtio-ccw.h
> > +++ b/hw/s390x/virtio-ccw.h
> > @@ -76,6 +76,7 @@ struct VirtioCcwDevice {
> >     SubchDev *sch;
> >     VirtIODevice *vdev;
> >     char *bus_id;
> > +    uint32_t loadparm;
> >     uint32_t host_features[VIRTIO_CCW_FEATURE_SIZE];
> >     VirtIORNGConf rng;
> >     VirtioBusState bus;
> > -- 
> > 1.7.9.5
> > 
>
Alexander Graf - April 26, 2013, 6:14 p.m.
On 26.04.2013, at 20:08, Dominik Dingel wrote:

> On Fri, 26 Apr 2013 18:52:48 +0200
> Alexander Graf <agraf@suse.de> wrote:
> 
>> 
>> On 26.04.2013, at 14:12, Dominik Dingel wrote:
>> 
>>> From: Christian Paro <cparo@us.ibm.com>
>>> 
>>> Provide a loadparm property which can be used to pass IPL load
>>> parameters on a per-device basis for VirtioCcwDevice instances
>>> representing block and network devices.
>>> 
>>> Signed-off-by: Christian Paro <cparo@us.ibm.com>
>>> 
>>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>>> index 56539d3..23d042f 100644
>>> --- a/hw/s390x/virtio-ccw.c
>>> +++ b/hw/s390x/virtio-ccw.c
>>> @@ -791,6 +791,7 @@ static const VirtIOBindings virtio_ccw_bindings = {
>>> 
>>> static Property virtio_ccw_net_properties[] = {
>>>    DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>>> +    DEFINE_PROP_UINT32("loadparm", VirtioCcwDevice, loadparm, 0),
>> 
>> We don't support netboot yet, right? Also is "loadparm" an architected name? Wouldn't "bootmap_id" or something that actually tells the user what's going on fit better?
>> 
>> 
>> Alex
> 
> Christian had the idea to name it subindex, but I don't know, to be honest, I like the general open solution more, especially for the commandline interface. What if we later on want to change the behaviour, loadparm, or better biosparameter is so general... where bootmap_id is narrowed down.

How about you guys make up your mind first what you really want to describe and which use cases you want to cover? :)


Alex
Christian Borntraeger - April 26, 2013, 7:54 p.m.
On 26/04/13 20:14, Alexander Graf wrote:
> 
> On 26.04.2013, at 20:08, Dominik Dingel wrote:
> 
>> On Fri, 26 Apr 2013 18:52:48 +0200
>> Alexander Graf <agraf@suse.de> wrote:
>>
>>>
>>> On 26.04.2013, at 14:12, Dominik Dingel wrote:
>>>
>>>> From: Christian Paro <cparo@us.ibm.com>
>>>>
>>>> Provide a loadparm property which can be used to pass IPL load
>>>> parameters on a per-device basis for VirtioCcwDevice instances
>>>> representing block and network devices.
>>>>
>>>> Signed-off-by: Christian Paro <cparo@us.ibm.com>
>>>>
>>>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>>>> index 56539d3..23d042f 100644
>>>> --- a/hw/s390x/virtio-ccw.c
>>>> +++ b/hw/s390x/virtio-ccw.c
>>>> @@ -791,6 +791,7 @@ static const VirtIOBindings virtio_ccw_bindings = {
>>>>
>>>> static Property virtio_ccw_net_properties[] = {
>>>>    DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>>>> +    DEFINE_PROP_UINT32("loadparm", VirtioCcwDevice, loadparm, 0),
>>>
>>> We don't support netboot yet, right? Also is "loadparm" an architected name? Wouldn't "bootmap_id" or something that actually tells the user what's going on fit better?
>>>
>>>
>>> Alex
>>
>> Christian had the idea to name it subindex, but I don't know, to be honest, I like the general open solution more, especially for the commandline interface. What if we later on want to change the behaviour, loadparm, or better biosparameter is so general... where bootmap_id is narrowed down.

You should probably specify the Christian.. ;-)
My vote was for loadparm, because thats how the parameter is called in HMC and in
z/VM as well as under /sys/firmware/reipl/ccw/loadparm etc. On the other hand
its only loadparm for ccw disk devices. fcp has wwpn,lun etc.

Its getting late in the cycle, so lets focus on the bootindex and defer this
after 1.5.

Patch

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 56539d3..23d042f 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -791,6 +791,7 @@  static const VirtIOBindings virtio_ccw_bindings = {
 
 static Property virtio_ccw_net_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
+    DEFINE_PROP_UINT32("loadparm", VirtioCcwDevice, loadparm, 0),
     DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]),
     DEFINE_VIRTIO_NET_PROPERTIES(VirtIONetCcw, vdev.net_conf),
     DEFINE_NIC_PROPERTIES(VirtIONetCcw, vdev.nic_conf),
@@ -818,6 +819,7 @@  static const TypeInfo virtio_ccw_net = {
 
 static Property virtio_ccw_blk_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
+    DEFINE_PROP_UINT32("loadparm", VirtioCcwDevice, loadparm, 0),
     DEFINE_VIRTIO_BLK_FEATURES(VirtioCcwDevice, host_features[0]),
     DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, blk),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index 84055e7..a3f706a 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -76,6 +76,7 @@  struct VirtioCcwDevice {
     SubchDev *sch;
     VirtIODevice *vdev;
     char *bus_id;
+    uint32_t loadparm;
     uint32_t host_features[VIRTIO_CCW_FEATURE_SIZE];
     VirtIORNGConf rng;
     VirtioBusState bus;