linux-kernel - Re: [PATCH 18/52] virtio-fs: Map cache using the values from the capabilities

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0f1b43f6-57e3-c6d2-7ffe-cf783e125a7b@redhat.com>
Date:   Thu, 13 Dec 2018 13:24:31 +0100
From:   David Hildenbrand <david@...hat.com>
To:     "Dr. David Alan Gilbert" <dgilbert@...hat.com>,
        Cornelia Huck <cohuck@...hat.com>
Cc:     Vivek Goyal <vgoyal@...hat.com>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        miklos@...redi.hu, stefanha@...hat.com, sweil@...hat.com,
        swhiteho@...hat.com
Subject: Re: [PATCH 18/52] virtio-fs: Map cache using the values from the
 capabilities

On 13.12.18 13:15, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@...hat.com) wrote:
>> On 13.12.18 11:00, Dr. David Alan Gilbert wrote:
>>> * David Hildenbrand (david@...hat.com) wrote:
>>>> On 13.12.18 10:13, Dr. David Alan Gilbert wrote:
>>>>> * David Hildenbrand (david@...hat.com) wrote:
>>>>>> On 10.12.18 18:12, Vivek Goyal wrote:
>>>>>>> Instead of assuming we had the fixed bar for the cache, use the
>>>>>>> value from the capabilities.
>>>>>>>
>>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@...hat.com>
>>>>>>> ---
>>>>>>>  fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
>>>>>>>  1 file changed, 17 insertions(+), 15 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
>>>>>>> index 60d496c16841..55bac1465536 100644
>>>>>>> --- a/fs/fuse/virtio_fs.c
>>>>>>> +++ b/fs/fuse/virtio_fs.c
>>>>>>> @@ -14,11 +14,6 @@
>>>>>>>  #include <uapi/linux/virtio_pci.h>
>>>>>>>  #include "fuse_i.h"
>>>>>>>  
>>>>>>> -enum {
>>>>>>> -	/* PCI BAR number of the virtio-fs DAX window */
>>>>>>> -	VIRTIO_FS_WINDOW_BAR = 2,
>>>>>>> -};
>>>>>>> -
>>>>>>>  /* List of virtio-fs device instances and a lock for the list */
>>>>>>>  static DEFINE_MUTEX(virtio_fs_mutex);
>>>>>>>  static LIST_HEAD(virtio_fs_instances);
>>>>>>> @@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
>>>>>>>  	struct dev_pagemap *pgmap;
>>>>>>>  	struct pci_dev *pci_dev;
>>>>>>>  	phys_addr_t phys_addr;
>>>>>>> -	size_t len;
>>>>>>> +	size_t bar_len;
>>>>>>>  	int ret;
>>>>>>>  	u8 have_cache, cache_bar;
>>>>>>>  	u64 cache_offset, cache_len;
>>>>>>> @@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
>>>>>>>          }
>>>>>>>  
>>>>>>>  	/* TODO handle case where device doesn't expose BAR? */
>>>>>>
>>>>>> For virtio-pmem we decided to not go via BARs as this would effectively
>>>>>> make it only usable for virtio-pci implementers. Instead, we are going
>>>>>> to export the applicable physical device region directly (e.g.
>>>>>> phys_start, phys_size in virtio config), so it is decoupled from PCI
>>>>>> details. Doing the same for virtio-fs would allow e.g. also virtio-ccw
>>>>>> to make eventually use of this.
>>>>>
>>>>> That makes it a very odd looking PCI device;  I can see that with
>>>>> virtio-pmem it makes some sense, given that it's job is to expose
>>>>> arbitrary chunks of memory.
>>>>>
>>>>> Dave
>>>>
>>>> Well, the fact that your are
>>>>
>>>> - including <uapi/linux/virtio_pci.h>
>>>> - adding pci related code
>>>>
>>>> in/to fs/fuse/virtio_fs.c
>>>>
>>>> tells me that these properties might be better communicated on the
>>>> virtio layer, not on the PCI layer.
>>>>
>>>> Or do you really want to glue virtio-fs to virtio-pci for all eternity?
>>>
>>> No, these need cleaning up; and the split within the bar
>>> is probably going to change to be communicated via virtio layer
>>> rather than pci capabilities.  However, I don't want to make our PCI
>>> device look odd, just to make portability to non-PCI devices - so it's
>>> right to make the split appropriately, but still to use PCI bars
>>> for what they were designed for.
>>>
>>> Dave
>>
>> Let's discuss after the cleanup. In general I am not convinced this is
>> the right thing to do. Using virtio-pci for anything else than pure
>> transport smells like bad design to me (well, I am no virtio expert
>> after all ;) ). No matter what PCI bars were designed for. If we can't
>> get the same running with e.g. virtio-ccw or virtio-whatever, it is
>> broken by design (or an addon that is tightly glued to virtio-pci, if
>> that is the general idea).
> 
> I'm sure we can find alternatives for virtio-*, so I wouldn't expect
> it to be glued to virtio-pci.
> 
> Dave

As s390x does not have the concept of memory mapped io (RAM is RAM,
nothing else), this is not architectured. vitio-ccw can therefore not
define anything similar like that. However, in virtual environments we
can do whatever we want on top of the pure transport (e.g. on the virtio
layer).

Conny can correct me if I am wrong.

-- 

Thanks,

David / dhildenb