Re: [PATCH 08/10] virtio/s390: add indirection to indicators access

Pierre Morel
2019-05-10T07:43:08Z

On 09/05/2019 20:26, Halil Pasic wrote:
> On Thu, 9 May 2019 14:01:01 +0200
> Pierre Morel <pmorel@linux.ibm.com> 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 <pasic@linux.ibm.com>
>>>> ---
>>>>   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?

Pierre

--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany