From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36298) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YjHgb-0007pP-NK for qemu-devel@nongnu.org; Fri, 17 Apr 2015 21:36:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YjHgY-0008NZ-Gi for qemu-devel@nongnu.org; Fri, 17 Apr 2015 21:36:45 -0400 Received: from mail-wi0-f175.google.com ([209.85.212.175]:34709) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YjHgY-0008NN-7o for qemu-devel@nongnu.org; Fri, 17 Apr 2015 21:36:42 -0400 Received: by wicmx19 with SMTP id mx19so1295654wic.1 for ; Fri, 17 Apr 2015 18:36:41 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1429272826-4145-1-git-send-email-shannon.zhao@linaro.org> <1429272826-4145-2-git-send-email-shannon.zhao@linaro.org> Date: Sat, 18 Apr 2015 09:36:41 +0800 Message-ID: From: Shannon Zhao Content-Type: multipart/alternative; boundary=f46d043c096e91e9170513f5baf9 Subject: Re: [Qemu-devel] [PATCH 1/4] hw/s390x/virtio-ccw: Add virtio_ccw_device_plugged for virtio-ccw List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: "Michael S. Tsirkin" , "Huangpeng (Peter)" , QEMU Developers , Shannon Zhao , Paolo Bonzini , Christoffer Dall --f46d043c096e91e9170513f5baf9 Content-Type: text/plain; charset=UTF-8 On Friday, 17 April 2015, Peter Maydell wrote: > On 17 April 2015 at 13:13, Shannon Zhao wrote: >> Add virtio_ccw_device_plugged, it can be used to get backend's features. >> >> Signed-off-by: Shannon Zhao >> Signed-off-by: Shannon Zhao >> --- >> hw/s390x/virtio-ccw.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c >> index 130535c..30ca377 100644 >> --- a/hw/s390x/virtio-ccw.c >> +++ b/hw/s390x/virtio-ccw.c >> @@ -1395,6 +1395,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f) >> return 0; >> } >> >> +/* This is called by virtio-bus just after the device is plugged. */ >> +static void virtio_ccw_device_plugged(DeviceState *d) >> +{ >> + VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); >> + >> + /* Only the first 32 feature bits are used. */ >> + dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus, >> + dev->host_features[0]); >> +} > > This means that this transport now calls virtio_bus_get_vdev_features > twice, which doesn't look right. In particular, we call it from > realize to set dev->host_features[0], and then add some features to > dev->host_features[0]. Then I think we will call the 'plugged' > method which will throw away those extra features. > > So I think that if we need to call this from 'plugged' > rather than 'realize' we need to move all the code for > setting host_features from 'realize' to here. > So sorry, when I reply this mail I'm using mobile phone, no codes on hand. So I didn't confirm that. > But I'm confused about why this change is necessary -- > don't the blk backends already use the "properties are > on the backend" approach, and they work with this transport? > Ok, maybe I missed that the transport already get the features when realized. If so, this change is not necessary. --f46d043c096e91e9170513f5baf9 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On Friday, 17 April 2015, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 17 = April 2015 at 13:13, Shannon Zhao <shannon.zhao@linaro.org> wrote:
>> Add virtio_ccw_dev= ice_plugged, it can be used to get backend's features.
>>
&= gt;> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shanno= n Zhao <shannon.zhao@linaro.o= rg>
>> ---
>>=C2=A0 hw/s390x/virtio-ccw.c | 11 +++= ++++++++
>>=C2=A0 1 file changed, 11 insertions(+)
>>
= >> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>= > index 130535c..30ca377 100644
>> --- a/hw/s390x/virtio-ccw.c<= br>>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -1395,6 +1395,16 @@ = static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
>>= =C2=A0 =C2=A0 =C2=A0 return 0;
>>=C2=A0 }
>>
>> = +/* This is called by virtio-bus just after the device is plugged. */
&g= t;> +static void virtio_ccw_device_plugged(DeviceState *d)
>> += {
>> +=C2=A0 =C2=A0 VirtioCcwDevice *dev =3D VIRTIO_CCW_DEVICE(d);=
>> +
>> +=C2=A0 =C2=A0 /* Only the first 32 feature bits= are used. */
>> +=C2=A0 =C2=A0 dev->host_features[0] =3D virti= o_bus_get_vdev_features(&dev->bus,
>> +=C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev->host_features[0]);
>> +}=
>
> This means that this transport now calls virtio_bus_get_vd= ev_features
> twice, which doesn't look right. In particular, we = call it from
> realize to set dev->host_features[0], and then add = some features to
> dev->host_features[0]. Then I think we will cal= l the 'plugged'
> method which will throw away those extra fe= atures.
>
> So I think that if we need to call this from 'p= lugged'
> rather than 'realize' we need to move all the c= ode for
> setting host_features from 'realize' to here.
&g= t;

So sorry, when I reply this mail I'm using mobile phone, no c= odes on hand.=C2=A0 So I didn't confirm that.

> But I'm c= onfused about why this change is necessary --
> don't the blk bac= kends already use the "properties are
> on the backend" app= roach, and they work with this transport?
>

Ok, maybe I missed= that the transport already get the features when realized. If so, this cha= nge is not necessary.
--f46d043c096e91e9170513f5baf9--