From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 10 May 2019 13:54:21 +0200 From: Halil Pasic Subject: Re: [PATCH 08/10] virtio/s390: add indirection to indicators access In-Reply-To: References: <20190426183245.37939-1-pasic@linux.ibm.com> <20190426183245.37939-9-pasic@linux.ibm.com> <716d47ca-016f-e8f4-6d78-7746a7d9f6ba@linux.ibm.com> <20190509202600.4fd6aebe.pasic@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Message-Id: <20190510135421.5363f14a.pasic@linux.ibm.com> Sender: kvm-owner@vger.kernel.org List-Archive: List-Post: To: Pierre Morel Cc: kvm@vger.kernel.org, linux-s390@vger.kernel.org, Cornelia Huck , Martin Schwidefsky , Sebastian Ott , virtualization@lists.linux-foundation.org, "Michael S. Tsirkin" , Christoph Hellwig , Thomas Huth , Christian Borntraeger , Viktor Mihajlovski , Vasily Gorbik , Janosch Frank , Claudio Imbrenda , Farhan Ali , Eric Farman List-ID: On Fri, 10 May 2019 09:43:08 +0200 Pierre Morel wrote: > On 09/05/2019 20:26, Halil Pasic wrote: > > On Thu, 9 May 2019 14:01:01 +0200 > > Pierre Morel wrote: > > > >> On 08/05/2019 16:31, Pierre Morel wrote: > >>> On 26/04/2019 20:32, Halil Pasic wrote: > >>>> This will come in handy soon when we pull out the indicators from > >>>> virtio_ccw_device to a memory area that is shared with the hypervisor > >>>> (in particular for protected virtualization guests). > >>>> > >>>> Signed-off-by: Halil Pasic > >>>> --- > >>>>   drivers/s390/virtio/virtio_ccw.c | 40 > >>>> +++++++++++++++++++++++++--------------- > >>>>   1 file changed, 25 insertions(+), 15 deletions(-) > >>>> > >>>> diff --git a/drivers/s390/virtio/virtio_ccw.c > >>>> b/drivers/s390/virtio/virtio_ccw.c > >>>> index bb7a92316fc8..1f3e7d56924f 100644 > >>>> --- a/drivers/s390/virtio/virtio_ccw.c > >>>> +++ b/drivers/s390/virtio/virtio_ccw.c > >>>> @@ -68,6 +68,16 @@ struct virtio_ccw_device { > >>>>       void *airq_info; > >>>>   }; > >>>> +static inline unsigned long *indicators(struct virtio_ccw_device *vcdev) > >>>> +{ > >>>> +    return &vcdev->indicators; > >>>> +} > >>>> + > >>>> +static inline unsigned long *indicators2(struct virtio_ccw_device > >>>> *vcdev) > >>>> +{ > >>>> +    return &vcdev->indicators2; > >>>> +} > >>>> + > >>>>   struct vq_info_block_legacy { > >>>>       __u64 queue; > >>>>       __u32 align; > >>>> @@ -337,17 +347,17 @@ static void virtio_ccw_drop_indicator(struct > >>>> virtio_ccw_device *vcdev, > >>>>           ccw->cda = (__u32)(unsigned long) thinint_area; > >>>>       } else { > >>>>           /* payload is the address of the indicators */ > >>>> -        indicatorp = kmalloc(sizeof(&vcdev->indicators), > >>>> +        indicatorp = kmalloc(sizeof(indicators(vcdev)), > >>>>                        GFP_DMA | GFP_KERNEL); > >>>>           if (!indicatorp) > >>>>               return; > >>>>           *indicatorp = 0; > >>>>           ccw->cmd_code = CCW_CMD_SET_IND; > >>>> -        ccw->count = sizeof(&vcdev->indicators); > >>>> +        ccw->count = sizeof(indicators(vcdev)); > >>> > >>> This looks strange to me. Was already weird before. > >>> Lucky we are indicators are long... > >>> may be just sizeof(long) > >> > > > > I'm not sure I understand where are you coming from... > > > > With CCW_CMD_SET_IND we tell the hypervisor the guest physical address > > at which the so called classic indicators. There is a comment that > > makes this obvious. The argument of the sizeof was and remained a > > pointer type. AFAIU this is what bothers you. > >> > >> AFAIK the size of the indicators (AIV/AIS) is not restricted by the > >> architecture. > > > > The size of vcdev->indicators is restricted or defined by the virtio > > specification. Please have a look at '4.3.2.6.1 Setting Up Classic Queue > > Indicators' here: > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-1630002 > > > > Since with Linux on s390 only 64 bit is supported, both the sizes are in > > line with the specification. Using u64 would semantically match the spec > > better, modulo pre virtio 1.0 which ain't specified. I did not want to > > do changes that are not necessary for what I'm trying to accomplish. If > > we want we can change these to u64 with a patch on top. > > I mean you are changing these line already, so why not doing it right > while at it? > This patch is about adding the indirection so we can move the member painlessly. Mixing in different stuff would be a bad practice. BTW I just explained that it ain't wrong, so I really do not understand what do you mean by 'why not doing it right'. Can you please explain? Did you agree with the rest of my comment? I mean there was more to it. Regards, Halil From mboxrd@z Thu Jan 1 00:00:00 1970 From: Halil Pasic Subject: Re: [PATCH 08/10] virtio/s390: add indirection to indicators access Date: Fri, 10 May 2019 13:54:21 +0200 Message-ID: <20190510135421.5363f14a.pasic@linux.ibm.com> References: <20190426183245.37939-1-pasic@linux.ibm.com> <20190426183245.37939-9-pasic@linux.ibm.com> <716d47ca-016f-e8f4-6d78-7746a7d9f6ba@linux.ibm.com> <20190509202600.4fd6aebe.pasic@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Pierre Morel Cc: Vasily Gorbik , linux-s390@vger.kernel.org, Thomas Huth , Claudio Imbrenda , kvm@vger.kernel.org, Sebastian Ott , "Michael S. Tsirkin" , Cornelia Huck , Eric Farman , virtualization@lists.linux-foundation.org, Christoph Hellwig , Martin Schwidefsky , Farhan Ali , Viktor Mihajlovski , Janosch Frank List-Id: virtualization@lists.linuxfoundation.org T24gRnJpLCAxMCBNYXkgMjAxOSAwOTo0MzowOCArMDIwMApQaWVycmUgTW9yZWwgPHBtb3JlbEBs aW51eC5pYm0uY29tPiB3cm90ZToKCj4gT24gMDkvMDUvMjAxOSAyMDoyNiwgSGFsaWwgUGFzaWMg d3JvdGU6Cj4gPiBPbiBUaHUsIDkgTWF5IDIwMTkgMTQ6MDE6MDEgKzAyMDAKPiA+IFBpZXJyZSBN b3JlbCA8cG1vcmVsQGxpbnV4LmlibS5jb20+IHdyb3RlOgo+ID4gCj4gPj4gT24gMDgvMDUvMjAx OSAxNjozMSwgUGllcnJlIE1vcmVsIHdyb3RlOgo+ID4+PiBPbiAyNi8wNC8yMDE5IDIwOjMyLCBI YWxpbCBQYXNpYyB3cm90ZToKPiA+Pj4+IFRoaXMgd2lsbCBjb21lIGluIGhhbmR5IHNvb24gd2hl biB3ZSBwdWxsIG91dCB0aGUgaW5kaWNhdG9ycyBmcm9tCj4gPj4+PiB2aXJ0aW9fY2N3X2Rldmlj ZSB0byBhIG1lbW9yeSBhcmVhIHRoYXQgaXMgc2hhcmVkIHdpdGggdGhlIGh5cGVydmlzb3IKPiA+ Pj4+IChpbiBwYXJ0aWN1bGFyIGZvciBwcm90ZWN0ZWQgdmlydHVhbGl6YXRpb24gZ3Vlc3RzKS4K PiA+Pj4+Cj4gPj4+PiBTaWduZWQtb2ZmLWJ5OiBIYWxpbCBQYXNpYyA8cGFzaWNAbGludXguaWJt LmNvbT4KPiA+Pj4+IC0tLQo+ID4+Pj4gIMKgIGRyaXZlcnMvczM5MC92aXJ0aW8vdmlydGlvX2Nj dy5jIHwgNDAKPiA+Pj4+ICsrKysrKysrKysrKysrKysrKysrKysrKystLS0tLS0tLS0tLS0tLS0K PiA+Pj4+ICDCoCAxIGZpbGUgY2hhbmdlZCwgMjUgaW5zZXJ0aW9ucygrKSwgMTUgZGVsZXRpb25z KC0pCj4gPj4+Pgo+ID4+Pj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvczM5MC92aXJ0aW8vdmlydGlv X2Njdy5jCj4gPj4+PiBiL2RyaXZlcnMvczM5MC92aXJ0aW8vdmlydGlvX2Njdy5jCj4gPj4+PiBp bmRleCBiYjdhOTIzMTZmYzguLjFmM2U3ZDU2OTI0ZiAxMDA2NDQKPiA+Pj4+IC0tLSBhL2RyaXZl cnMvczM5MC92aXJ0aW8vdmlydGlvX2Njdy5jCj4gPj4+PiArKysgYi9kcml2ZXJzL3MzOTAvdmly dGlvL3ZpcnRpb19jY3cuYwo+ID4+Pj4gQEAgLTY4LDYgKzY4LDE2IEBAIHN0cnVjdCB2aXJ0aW9f Y2N3X2RldmljZSB7Cj4gPj4+PiAgwqDCoMKgwqDCoCB2b2lkICphaXJxX2luZm87Cj4gPj4+PiAg wqAgfTsKPiA+Pj4+ICtzdGF0aWMgaW5saW5lIHVuc2lnbmVkIGxvbmcgKmluZGljYXRvcnMoc3Ry dWN0IHZpcnRpb19jY3dfZGV2aWNlICp2Y2RldikKPiA+Pj4+ICt7Cj4gPj4+PiArwqDCoMKgIHJl dHVybiAmdmNkZXYtPmluZGljYXRvcnM7Cj4gPj4+PiArfQo+ID4+Pj4gKwo+ID4+Pj4gK3N0YXRp YyBpbmxpbmUgdW5zaWduZWQgbG9uZyAqaW5kaWNhdG9yczIoc3RydWN0IHZpcnRpb19jY3dfZGV2 aWNlCj4gPj4+PiAqdmNkZXYpCj4gPj4+PiArewo+ID4+Pj4gK8KgwqDCoCByZXR1cm4gJnZjZGV2 LT5pbmRpY2F0b3JzMjsKPiA+Pj4+ICt9Cj4gPj4+PiArCj4gPj4+PiAgwqAgc3RydWN0IHZxX2lu Zm9fYmxvY2tfbGVnYWN5IHsKPiA+Pj4+ICDCoMKgwqDCoMKgIF9fdTY0IHF1ZXVlOwo+ID4+Pj4g IMKgwqDCoMKgwqAgX191MzIgYWxpZ247Cj4gPj4+PiBAQCAtMzM3LDE3ICszNDcsMTcgQEAgc3Rh dGljIHZvaWQgdmlydGlvX2Njd19kcm9wX2luZGljYXRvcihzdHJ1Y3QKPiA+Pj4+IHZpcnRpb19j Y3dfZGV2aWNlICp2Y2RldiwKPiA+Pj4+ICDCoMKgwqDCoMKgwqDCoMKgwqAgY2N3LT5jZGEgPSAo X191MzIpKHVuc2lnbmVkIGxvbmcpIHRoaW5pbnRfYXJlYTsKPiA+Pj4+ICDCoMKgwqDCoMKgIH0g ZWxzZSB7Cj4gPj4+PiAgwqDCoMKgwqDCoMKgwqDCoMKgIC8qIHBheWxvYWQgaXMgdGhlIGFkZHJl c3Mgb2YgdGhlIGluZGljYXRvcnMgKi8KPiA+Pj4+IC3CoMKgwqDCoMKgwqDCoCBpbmRpY2F0b3Jw ID0ga21hbGxvYyhzaXplb2YoJnZjZGV2LT5pbmRpY2F0b3JzKSwKPiA+Pj4+ICvCoMKgwqDCoMKg wqDCoCBpbmRpY2F0b3JwID0ga21hbGxvYyhzaXplb2YoaW5kaWNhdG9ycyh2Y2RldikpLAo+ID4+ Pj4gIMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIEdGUF9ETUEg fCBHRlBfS0VSTkVMKTsKPiA+Pj4+ICDCoMKgwqDCoMKgwqDCoMKgwqAgaWYgKCFpbmRpY2F0b3Jw KQo+ID4+Pj4gIMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIHJldHVybjsKPiA+Pj4+ICDCoMKg wqDCoMKgwqDCoMKgwqAgKmluZGljYXRvcnAgPSAwOwo+ID4+Pj4gIMKgwqDCoMKgwqDCoMKgwqDC oCBjY3ctPmNtZF9jb2RlID0gQ0NXX0NNRF9TRVRfSU5EOwo+ID4+Pj4gLcKgwqDCoMKgwqDCoMKg IGNjdy0+Y291bnQgPSBzaXplb2YoJnZjZGV2LT5pbmRpY2F0b3JzKTsKPiA+Pj4+ICvCoMKgwqDC oMKgwqDCoCBjY3ctPmNvdW50ID0gc2l6ZW9mKGluZGljYXRvcnModmNkZXYpKTsKPiA+Pj4KPiA+ Pj4gVGhpcyBsb29rcyBzdHJhbmdlIHRvIG1lLiBXYXMgYWxyZWFkeSB3ZWlyZCBiZWZvcmUuCj4g Pj4+IEx1Y2t5IHdlIGFyZSBpbmRpY2F0b3JzIGFyZSBsb25nLi4uCj4gPj4+IG1heSBiZSBqdXN0 IHNpemVvZihsb25nKQo+ID4+Cj4gPiAKPiA+IEknbSBub3Qgc3VyZSBJIHVuZGVyc3RhbmQgd2hl cmUgYXJlIHlvdSBjb21pbmcgZnJvbS4uLgo+ID4gCj4gPiBXaXRoIENDV19DTURfU0VUX0lORCB3 ZSB0ZWxsIHRoZSBoeXBlcnZpc29yIHRoZSBndWVzdCBwaHlzaWNhbCBhZGRyZXNzCj4gPiBhdCB3 aGljaCB0aGUgc28gY2FsbGVkIGNsYXNzaWMgaW5kaWNhdG9ycy4gVGhlcmUgaXMgYSBjb21tZW50 IHRoYXQKPiA+IG1ha2VzIHRoaXMgb2J2aW91cy4gVGhlIGFyZ3VtZW50IG9mIHRoZSBzaXplb2Yg d2FzIGFuZCByZW1haW5lZCBhCj4gPiBwb2ludGVyIHR5cGUuIEFGQUlVIHRoaXMgaXMgd2hhdCBi b3RoZXJzIHlvdS4KPiA+Pgo+ID4+IEFGQUlLIHRoZSBzaXplIG9mIHRoZSBpbmRpY2F0b3JzIChB SVYvQUlTKSBpcyBub3QgcmVzdHJpY3RlZCBieSB0aGUKPiA+PiBhcmNoaXRlY3R1cmUuCj4gPiAK PiA+IFRoZSBzaXplIG9mIHZjZGV2LT5pbmRpY2F0b3JzIGlzIHJlc3RyaWN0ZWQgb3IgZGVmaW5l ZCBieSB0aGUgdmlydGlvCj4gPiBzcGVjaWZpY2F0aW9uLiBQbGVhc2UgaGF2ZSBhIGxvb2sgYXQg JzQuMy4yLjYuMSBTZXR0aW5nIFVwIENsYXNzaWMgUXVldWUKPiA+IEluZGljYXRvcnMnIGhlcmU6 Cj4gPiBodHRwczovL2RvY3Mub2FzaXMtb3Blbi5vcmcvdmlydGlvL3ZpcnRpby92MS4xL2NzMDEv dmlydGlvLXYxLjEtY3MwMS5odG1sI3gxLTE2MzAwMDIKPiA+IAo+ID4gU2luY2Ugd2l0aCBMaW51 eCBvbiBzMzkwIG9ubHkgNjQgYml0IGlzIHN1cHBvcnRlZCwgYm90aCB0aGUgc2l6ZXMgYXJlIGlu Cj4gPiBsaW5lIHdpdGggdGhlIHNwZWNpZmljYXRpb24uIFVzaW5nIHU2NCB3b3VsZCBzZW1hbnRp Y2FsbHkgbWF0Y2ggdGhlIHNwZWMKPiA+IGJldHRlciwgbW9kdWxvIHByZSB2aXJ0aW8gMS4wIHdo aWNoIGFpbid0IHNwZWNpZmllZC4gSSBkaWQgbm90IHdhbnQgdG8KPiA+IGRvIGNoYW5nZXMgdGhh dCBhcmUgbm90IG5lY2Vzc2FyeSBmb3Igd2hhdCBJJ20gdHJ5aW5nIHRvIGFjY29tcGxpc2guIElm Cj4gPiB3ZSB3YW50IHdlIGNhbiBjaGFuZ2UgdGhlc2UgdG8gdTY0IHdpdGggYSBwYXRjaCBvbiB0 b3AuCj4gCj4gSSBtZWFuIHlvdSBhcmUgY2hhbmdpbmcgdGhlc2UgbGluZSBhbHJlYWR5LCBzbyB3 aHkgbm90IGRvaW5nIGl0IHJpZ2h0IAo+IHdoaWxlIGF0IGl0Pwo+IAoKVGhpcyBwYXRjaCBpcyBh Ym91dCBhZGRpbmcgdGhlIGluZGlyZWN0aW9uIHNvIHdlIGNhbiBtb3ZlIHRoZSBtZW1iZXIKcGFp bmxlc3NseS4gTWl4aW5nIGluIGRpZmZlcmVudCBzdHVmZiB3b3VsZCBiZSBhIGJhZCBwcmFjdGlj ZS4KCkJUVyBJIGp1c3QgZXhwbGFpbmVkIHRoYXQgaXQgYWluJ3Qgd3JvbmcsIHNvIEkgcmVhbGx5 IGRvIG5vdCB1bmRlcnN0YW5kCndoYXQgZG8geW91IG1lYW4gYnkgICd3aHkgbm90IGRvaW5nIGl0 IHJpZ2h0Jy4gQ2FuIHlvdSBwbGVhc2UgZXhwbGFpbj8KCkRpZCB5b3UgYWdyZWUgd2l0aCB0aGUg cmVzdCBvZiBteSBjb21tZW50PyBJIG1lYW4gdGhlcmUgd2FzIG1vcmUgdG8gaXQuCgpSZWdhcmRz LApIYWxpbAoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18K VmlydHVhbGl6YXRpb24gbWFpbGluZyBsaXN0ClZpcnR1YWxpemF0aW9uQGxpc3RzLmxpbnV4LWZv dW5kYXRpb24ub3JnCmh0dHBzOi8vbGlzdHMubGludXhmb3VuZGF0aW9uLm9yZy9tYWlsbWFuL2xp c3RpbmZvL3ZpcnR1YWxpemF0aW9u