From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 9 May 2019 20:26:00 +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> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Message-Id: <20190509202600.4fd6aebe.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 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. > However we never use more than 64 bits, do we ever have an adapter > having more than 64 different interrupts? These are one per queue. The number of queues used to be limited to 64 but it ain't no more. If the driver uses classic notifiers, only the first 64 can be used. > > May be we can state than we use a maximal number of AISB of 64 and > therefor use indicators with a size of unsigned long, or __u64 or > whatever is appropriate. Please clear this. > I think you are mixing up adapter interrupts as defined by the architecture, with virtio indicators which are kind of a special case at best: the two stage stuff is modeled after AISB and AIBV. > With this cleared: I hope, I managed to clear this up a bit. If not please try to re-state your concern in different words. > Reviewed-by: Pierre Morel > Thanks for your review! 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: Thu, 9 May 2019 20:26:00 +0200 Message-ID: <20190509202600.4fd6aebe.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> 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 T24gVGh1LCA5IE1heSAyMDE5IDE0OjAxOjAxICswMjAwClBpZXJyZSBNb3JlbCA8cG1vcmVsQGxp bnV4LmlibS5jb20+IHdyb3RlOgoKPiBPbiAwOC8wNS8yMDE5IDE2OjMxLCBQaWVycmUgTW9yZWwg d3JvdGU6Cj4gPiBPbiAyNi8wNC8yMDE5IDIwOjMyLCBIYWxpbCBQYXNpYyB3cm90ZToKPiA+PiBU aGlzIHdpbGwgY29tZSBpbiBoYW5keSBzb29uIHdoZW4gd2UgcHVsbCBvdXQgdGhlIGluZGljYXRv cnMgZnJvbQo+ID4+IHZpcnRpb19jY3dfZGV2aWNlIHRvIGEgbWVtb3J5IGFyZWEgdGhhdCBpcyBz aGFyZWQgd2l0aCB0aGUgaHlwZXJ2aXNvcgo+ID4+IChpbiBwYXJ0aWN1bGFyIGZvciBwcm90ZWN0 ZWQgdmlydHVhbGl6YXRpb24gZ3Vlc3RzKS4KPiA+Pgo+ID4+IFNpZ25lZC1vZmYtYnk6IEhhbGls IFBhc2ljIDxwYXNpY0BsaW51eC5pYm0uY29tPgo+ID4+IC0tLQo+ID4+IMKgIGRyaXZlcnMvczM5 MC92aXJ0aW8vdmlydGlvX2Njdy5jIHwgNDAgCj4gPj4gKysrKysrKysrKysrKysrKysrKysrKysr Ky0tLS0tLS0tLS0tLS0tLQo+ID4+IMKgIDEgZmlsZSBjaGFuZ2VkLCAyNSBpbnNlcnRpb25zKCsp LCAxNSBkZWxldGlvbnMoLSkKPiA+Pgo+ID4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3MzOTAvdmly dGlvL3ZpcnRpb19jY3cuYyAKPiA+PiBiL2RyaXZlcnMvczM5MC92aXJ0aW8vdmlydGlvX2Njdy5j Cj4gPj4gaW5kZXggYmI3YTkyMzE2ZmM4Li4xZjNlN2Q1NjkyNGYgMTAwNjQ0Cj4gPj4gLS0tIGEv ZHJpdmVycy9zMzkwL3ZpcnRpby92aXJ0aW9fY2N3LmMKPiA+PiArKysgYi9kcml2ZXJzL3MzOTAv dmlydGlvL3ZpcnRpb19jY3cuYwo+ID4+IEBAIC02OCw2ICs2OCwxNiBAQCBzdHJ1Y3QgdmlydGlv X2Njd19kZXZpY2Ugewo+ID4+IMKgwqDCoMKgwqAgdm9pZCAqYWlycV9pbmZvOwo+ID4+IMKgIH07 Cj4gPj4gK3N0YXRpYyBpbmxpbmUgdW5zaWduZWQgbG9uZyAqaW5kaWNhdG9ycyhzdHJ1Y3Qgdmly dGlvX2Njd19kZXZpY2UgKnZjZGV2KQo+ID4+ICt7Cj4gPj4gK8KgwqDCoCByZXR1cm4gJnZjZGV2 LT5pbmRpY2F0b3JzOwo+ID4+ICt9Cj4gPj4gKwo+ID4+ICtzdGF0aWMgaW5saW5lIHVuc2lnbmVk IGxvbmcgKmluZGljYXRvcnMyKHN0cnVjdCB2aXJ0aW9fY2N3X2RldmljZSAKPiA+PiAqdmNkZXYp Cj4gPj4gK3sKPiA+PiArwqDCoMKgIHJldHVybiAmdmNkZXYtPmluZGljYXRvcnMyOwo+ID4+ICt9 Cj4gPj4gKwo+ID4+IMKgIHN0cnVjdCB2cV9pbmZvX2Jsb2NrX2xlZ2FjeSB7Cj4gPj4gwqDCoMKg wqDCoCBfX3U2NCBxdWV1ZTsKPiA+PiDCoMKgwqDCoMKgIF9fdTMyIGFsaWduOwo+ID4+IEBAIC0z MzcsMTcgKzM0NywxNyBAQCBzdGF0aWMgdm9pZCB2aXJ0aW9fY2N3X2Ryb3BfaW5kaWNhdG9yKHN0 cnVjdCAKPiA+PiB2aXJ0aW9fY2N3X2RldmljZSAqdmNkZXYsCj4gPj4gwqDCoMKgwqDCoMKgwqDC oMKgIGNjdy0+Y2RhID0gKF9fdTMyKSh1bnNpZ25lZCBsb25nKSB0aGluaW50X2FyZWE7Cj4gPj4g wqDCoMKgwqDCoCB9IGVsc2Ugewo+ID4+IMKgwqDCoMKgwqDCoMKgwqDCoCAvKiBwYXlsb2FkIGlz IHRoZSBhZGRyZXNzIG9mIHRoZSBpbmRpY2F0b3JzICovCj4gPj4gLcKgwqDCoMKgwqDCoMKgIGlu ZGljYXRvcnAgPSBrbWFsbG9jKHNpemVvZigmdmNkZXYtPmluZGljYXRvcnMpLAo+ID4+ICvCoMKg wqDCoMKgwqDCoCBpbmRpY2F0b3JwID0ga21hbGxvYyhzaXplb2YoaW5kaWNhdG9ycyh2Y2Rldikp LAo+ID4+IMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIEdGUF9E TUEgfCBHRlBfS0VSTkVMKTsKPiA+PiDCoMKgwqDCoMKgwqDCoMKgwqAgaWYgKCFpbmRpY2F0b3Jw KQo+ID4+IMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIHJldHVybjsKPiA+PiDCoMKgwqDCoMKg wqDCoMKgwqAgKmluZGljYXRvcnAgPSAwOwo+ID4+IMKgwqDCoMKgwqDCoMKgwqDCoCBjY3ctPmNt ZF9jb2RlID0gQ0NXX0NNRF9TRVRfSU5EOwo+ID4+IC3CoMKgwqDCoMKgwqDCoCBjY3ctPmNvdW50 ID0gc2l6ZW9mKCZ2Y2Rldi0+aW5kaWNhdG9ycyk7Cj4gPj4gK8KgwqDCoMKgwqDCoMKgIGNjdy0+ Y291bnQgPSBzaXplb2YoaW5kaWNhdG9ycyh2Y2RldikpOwo+ID4gCj4gPiBUaGlzIGxvb2tzIHN0 cmFuZ2UgdG8gbWUuIFdhcyBhbHJlYWR5IHdlaXJkIGJlZm9yZS4KPiA+IEx1Y2t5IHdlIGFyZSBp bmRpY2F0b3JzIGFyZSBsb25nLi4uCj4gPiBtYXkgYmUganVzdCBzaXplb2YobG9uZykKPiAKCkkn bSBub3Qgc3VyZSBJIHVuZGVyc3RhbmQgd2hlcmUgYXJlIHlvdSBjb21pbmcgZnJvbS4uLgoKV2l0 aCBDQ1dfQ01EX1NFVF9JTkQgd2UgdGVsbCB0aGUgaHlwZXJ2aXNvciB0aGUgZ3Vlc3QgcGh5c2lj YWwgYWRkcmVzcwphdCB3aGljaCB0aGUgc28gY2FsbGVkIGNsYXNzaWMgaW5kaWNhdG9ycy4gVGhl cmUgaXMgYSBjb21tZW50IHRoYXQKbWFrZXMgdGhpcyBvYnZpb3VzLiBUaGUgYXJndW1lbnQgb2Yg dGhlIHNpemVvZiB3YXMgYW5kIHJlbWFpbmVkIGEKcG9pbnRlciB0eXBlLiBBRkFJVSB0aGlzIGlz IHdoYXQgYm90aGVycyB5b3UuIAo+IAo+IEFGQUlLIHRoZSBzaXplIG9mIHRoZSBpbmRpY2F0b3Jz IChBSVYvQUlTKSBpcyBub3QgcmVzdHJpY3RlZCBieSB0aGUgCj4gYXJjaGl0ZWN0dXJlLgoKVGhl IHNpemUgb2YgdmNkZXYtPmluZGljYXRvcnMgaXMgcmVzdHJpY3RlZCBvciBkZWZpbmVkIGJ5IHRo ZSB2aXJ0aW8Kc3BlY2lmaWNhdGlvbi4gUGxlYXNlIGhhdmUgYSBsb29rIGF0ICc0LjMuMi42LjEg U2V0dGluZyBVcCBDbGFzc2ljIFF1ZXVlCkluZGljYXRvcnMnIGhlcmU6Cmh0dHBzOi8vZG9jcy5v YXNpcy1vcGVuLm9yZy92aXJ0aW8vdmlydGlvL3YxLjEvY3MwMS92aXJ0aW8tdjEuMS1jczAxLmh0 bWwjeDEtMTYzMDAwMgoKU2luY2Ugd2l0aCBMaW51eCBvbiBzMzkwIG9ubHkgNjQgYml0IGlzIHN1 cHBvcnRlZCwgYm90aCB0aGUgc2l6ZXMgYXJlIGluCmxpbmUgd2l0aCB0aGUgc3BlY2lmaWNhdGlv bi4gVXNpbmcgdTY0IHdvdWxkIHNlbWFudGljYWxseSBtYXRjaCB0aGUgc3BlYwpiZXR0ZXIsIG1v ZHVsbyBwcmUgdmlydGlvIDEuMCB3aGljaCBhaW4ndCBzcGVjaWZpZWQuIEkgZGlkIG5vdCB3YW50 IHRvCmRvIGNoYW5nZXMgdGhhdCBhcmUgbm90IG5lY2Vzc2FyeSBmb3Igd2hhdCBJJ20gdHJ5aW5n IHRvIGFjY29tcGxpc2guIElmCndlIHdhbnQgd2UgY2FuIGNoYW5nZSB0aGVzZSB0byB1NjQgd2l0 aCBhIHBhdGNoIG9uIHRvcC4KCj4gSG93ZXZlciB3ZSBuZXZlciB1c2UgbW9yZSB0aGFuIDY0IGJp dHMsIGRvIHdlIGV2ZXIgaGF2ZSBhbiBhZGFwdGVyIAo+IGhhdmluZyBtb3JlIHRoYW4gNjQgZGlm ZmVyZW50IGludGVycnVwdHM/CgpUaGVzZSBhcmUgb25lIHBlciBxdWV1ZS4gVGhlIG51bWJlciBv ZiBxdWV1ZXMgdXNlZCB0byBiZSBsaW1pdGVkIHRvIDY0CmJ1dCBpdCBhaW4ndCBubyBtb3JlLiBJ ZiB0aGUgZHJpdmVyIHVzZXMgY2xhc3NpYyBub3RpZmllcnMsIG9ubHkgdGhlCmZpcnN0IDY0IGNh biBiZSB1c2VkLgoKPiAKPiBNYXkgYmUgd2UgY2FuIHN0YXRlIHRoYW4gd2UgdXNlIGEgbWF4aW1h bCBudW1iZXIgb2YgQUlTQiBvZiA2NCBhbmQgCj4gdGhlcmVmb3IgdXNlIGluZGljYXRvcnMgd2l0 aCBhIHNpemUgb2YgdW5zaWduZWQgbG9uZywgb3IgX191NjQgb3IgCj4gd2hhdGV2ZXIgaXMgYXBw cm9wcmlhdGUuIFBsZWFzZSBjbGVhciB0aGlzLgo+IAoKSSB0aGluayB5b3UgYXJlIG1peGluZyB1 cCBhZGFwdGVyIGludGVycnVwdHMgYXMgZGVmaW5lZCBieSB0aGUKYXJjaGl0ZWN0dXJlLCB3aXRo IHZpcnRpbyBpbmRpY2F0b3JzIHdoaWNoIGFyZSBraW5kIG9mIGEgc3BlY2lhbCBjYXNlCmF0IGJl c3Q6IHRoZSB0d28gc3RhZ2Ugc3R1ZmYgaXMgbW9kZWxlZCBhZnRlciBBSVNCIGFuZCBBSUJWLiAg Cgo+IFdpdGggdGhpcyBjbGVhcmVkOgoKSSBob3BlLCBJIG1hbmFnZWQgdG8gY2xlYXIgdGhpcyB1 cCBhIGJpdC4gSWYgbm90IHBsZWFzZSB0cnkgdG8gcmUtc3RhdGUKeW91ciBjb25jZXJuIGluIGRp ZmZlcmVudCB3b3Jkcy4KCj4gUmV2aWV3ZWQtYnk6IFBpZXJyZSBNb3JlbDxwbW9yZWxAbGludXgu aWJtLmNvbT4KPiAKClRoYW5rcyBmb3IgeW91ciByZXZpZXchCgpIYWxpbAogCgpfX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpWaXJ0dWFsaXphdGlvbiBtYWls aW5nIGxpc3QKVmlydHVhbGl6YXRpb25AbGlzdHMubGludXgtZm91bmRhdGlvbi5vcmcKaHR0cHM6 Ly9saXN0cy5saW51eGZvdW5kYXRpb24ub3JnL21haWxtYW4vbGlzdGluZm8vdmlydHVhbGl6YXRp b24=