Message-ID: <PAXPR02MB7310694F20F95CA446FFF22481729@PAXPR02MB7310.eurprd02.prod.outlook.com>
Date:   Thu, 25 Aug 2022 10:16:59 +0000
From:   "Czerwacki, Eial" <eial.czerwacki@....com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Arsh, Leonid" <leonid.arsh@....com>,
        "Twaig, Oren" <oren.twaig@....com>,
        SAP vSMP Linux Maintainer <linux.vsmp@....com>,
        Arnd Bergmann <arnd@...db.de>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Andra Paraschiv <andraprs@...zon.com>,
        Borislav Petkov <bp@...e.de>,
        Brijesh Singh <brijesh.singh@....com>,
        Eric Biggers <ebiggers@...gle.com>, Fei Li <fei1.li@...el.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Jens Axboe <axboe@...nel.dk>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Subject: Re: [PATCH v2] drivers/virt/vSMP: new driver

>On Thu, Aug 25, 2022 at 09:17:57AM +0000, Czerwacki, Eial wrote:
>> Greetings Greg,
>> 
>> thank you for the comments, see my comment below.
>> >On Thu, Aug 25, 2022 at 06:24:02AM +0000, Czerwacki, Eial wrote:
>> >> Introducing the vSMP guest driver which allows interaction with the
>> >> vSMP control device when running a Linux OS atop of the vSMP hypervisor.
>> >> vSMP is a resource aggregation hypervisor from SAP.
>> >> 
>> >> The driver comprises of api part which facilitates communication with
>> >> the hypervisor and version which displays the hypervisor's version.
>> >> 
>> >> This patch s based on previous patches sent to the staging tree mailing
>> >> lists
>> >> 
>> >> Signed-off-by: Eial Czerwacki <eial.czerwacki@....com>
>> >> Acked-by: Leonid Arsh <leonid.arsh@....com>
>> >> Acked-by: Oren Twaig <oren.twaig@....com>
>> >> CC: SAP vSMP Linux Maintainer <linux.vsmp@....com>
>> >> CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> >> CC: Arnd Bergmann <arnd@...db.de>
>> >> CC: Dan Carpenter <dan.carpenter@...cle.com>
>> >> CC: Andra Paraschiv <andraprs@...zon.com>
>> >> CC: Borislav Petkov <bp@...e.de>
>> >> CC: Brijesh Singh <brijesh.singh@....com>
>> >> CC: Eric Biggers <ebiggers@...gle.com>
>> >> CC: Fei Li <fei1.li@...el.com>
>> >> CC: Hans de Goede <hdegoede@...hat.com>
>> >> CC: Jens Axboe <axboe@...nel.dk>
>> >> CC: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
>> >> 
>> >> v1 -> v2:
>> >>        - fix -0 var init in add_sysfs_entries (pointed out by Dan Carpenter)
>> >> ---
>> >>  Documentation/ABI/stable/sysfs-driver-vsmp |   5 +
>> >>  MAINTAINERS                                |   6 +
>> >>  drivers/virt/Kconfig                       |   2 +
>> >>  drivers/virt/Makefile                      |   2 +
>> >>  drivers/virt/vsmp/Kconfig                  |  11 +
>> >>  drivers/virt/vsmp/Makefile                 |   7 +
>> >>  drivers/virt/vsmp/api/api.c                | 249 +++++++++++++++++++++
>> >>  drivers/virt/vsmp/api/api.h                |  69 ++++++
>> >>  drivers/virt/vsmp/include/registers.h      |  12 +
>> >>  drivers/virt/vsmp/version/version.c        | 118 ++++++++++
>> >>  drivers/virt/vsmp/version/version.h        |  14 ++
>> >>  drivers/virt/vsmp/vsmp_main.c              | 110 +++++++++
>> >>  12 files changed, 605 insertions(+)
>> >
>> >Why do you have all of these different .c and .h files for only 600
>> >lines of code?  Shouldn't this all just be in a single .c file?  Why
>> >have a subdir for just 300 lines?
>> >
>> >Please mush this all into a single .c file going forward, speading it
>> >out like this makes no sense.
>> the current driver has two modules, version and api, in later versions of the
>> driver support for additional features will be added.
>> at that time, this might cause the single file to inflate, so we thought to split it
>> to several files to make it more organized.
>> is there any way to keep it in different code files?
>
>Keep it small and together now.  If you need to change it in the future,
>wonderful, do it then.  Don't do things that we never know will happen
>or not.
>
>One file, and one module, is great to start with.  Please do that for
>now.
>
I understand, I'll restrucure it into one file

>
>> 
>> >
>> >>  create mode 100644 Documentation/ABI/stable/sysfs-driver-vsmp
>> >>  create mode 100644 drivers/virt/vsmp/Kconfig
>> >>  create mode 100644 drivers/virt/vsmp/Makefile
>> >>  create mode 100644 drivers/virt/vsmp/api/api.c
>> >>  create mode 100644 drivers/virt/vsmp/api/api.h
>> >>  create mode 100644 drivers/virt/vsmp/include/registers.h
>> >>  create mode 100644 drivers/virt/vsmp/version/version.c
>> >>  create mode 100644 drivers/virt/vsmp/version/version.h
>> >>  create mode 100644 drivers/virt/vsmp/vsmp_main.c
>> >> 
>> >> diff --git a/Documentation/ABI/stable/sysfs-driver-vsmp b/Documentation/ABI/stable/sysfs-driver-vsmp
>> >> new file mode 100644
>> >> index 000000000000..18a0a62f40ed
>> >> --- /dev/null
>> >> +++ b/Documentation/ABI/stable/sysfs-driver-vsmp
>> >> @@ -0,0 +1,5 @@
>> >> +What:           /sys/hypervisor/vsmp/version
>> >> +Date:           Aug 2022
>> >
>> >August is almost over :(
>> will fix, thank!
>> 
>> >
>> >> +Contact:        Eial Czerwacki <eial.czerwacki@....com>
>> >> +             linux-vsmp@....com
>> >
>> >No need for an alias here.
>> it's not an alias, it is a shard mailbox for the team so others can
>> view the mails history.
>> I saw that the same methodology is done with mailing lists.
>
>That's a mailing list?  Ah, didn't realize that, sorry, was not obvious.
>
>> >> diff --git a/MAINTAINERS b/MAINTAINERS
>> >> index f512b430c7cb..cf74089c4d19 100644
>> >> --- a/MAINTAINERS
>> >> +++ b/MAINTAINERS
>> >> @@ -21783,6 +21783,12 @@ F:   lib/test_printf.c
>> >>  F:   lib/test_scanf.c
>> >>  F:   lib/vsprintf.c
>> >> 
>> >> +VSMP GUEST DRIVER
>> >> +M:   Eial Czerwacki <eial.czerwacki@....com>
>> >> +M:   linux-vsmp@....com
>> >
>> >Again, no random aliases please, stick to a person as a contact.
>> see above comment
>
>If this is a mailing list, mark it as such, you did not do so.
I'll mark it accordingly.

>
>
>> >> +/*
>> >> + * Register the vSMP sysfs object for user space interaction
>> >> + */
>> >> +int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr)
>> >> +{
>> >> +     int error = -EINVAL;
>> >> +
>> >> +     if (vsmp_sysfs_kobj && bin_attr) {
>> >> +             error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr);
>> >
>> >You raced userspace and lost :(
>> not sure I understand, can you elaborate more please?
>
>Fix up your sysfs usage first, and then we can revisit this.
will do.

>
>
>> >
>> >And why is your version file a binary file?  It should just be a small
>> >text string, right?
>> not so small, it can reach up to 512kb.
>
>That was not obvious at all.  Please document this.
where should the document be?
in the code as a comment or in another file?

>
>And how in the world is a "version" that big?  What exactly does this
>contain?
it 's size depends on the number of resources it uses.
here is an example:
:~> cat /sys/hypervisor/vsmp/version  
SAP vSMP Foundation: 10.6.2862.0 (Aug 22 2022 15:21:02)
System configuration:
   Boards:      2
      1 x Proc. + I/O + Memory
      1 x NVM devices (Amazon.com Amazon EC2 NVMe Instance Storage)
   Processors:  1, Cores: 2, Threads: 4
       Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz Stepping 04
   Memory (MB): 30976 (of 103192), Cache: 7527, Private: 64689
      1 x  6400MB    [ 7825/ 321/ 1104]      
      1 x 24576MB    [95367/7206/63585]       00:1f.0#1
   Boot device: [HDD] NVMe: Amazon Elastic Block Store        
Supported until: Aug 22 2024

>
>> that is why I decided to go with binary, I understood that the text is rather limited.
>
>That is true, sysfs is "one value per file", this can not be a file that
>you parse.
should I keep it as bin then?

>
>
>> >> +int open_cfg_addr(struct pci_dev *pdev)
>> >> +{
>> >> +     u64 cfg_start;
>> >> +     u32 cfg_len;
>> >> +
>> >> +     vsmp_dev_obj = pdev;
>> >> +     cfg_start = pci_resource_start(vsmp_dev_obj, 0);
>> >> +     cfg_len = pci_resource_len(vsmp_dev_obj, 0);
>> >> +
>> >> +     dev_dbg(get_dev(), "Mapping bar 0: [0x%llx,0x%llx]\n",
>> >> +             cfg_start, cfg_start + cfg_len);
>> >
>> >Again, you have a device, use that.  Goes for the whole driver.
>> I think I've missed something, please correct me if I'm wrong,
>> I don't need to save the pdev because there is an existing framework
>> that provides it?
>
>You have pdev, use that in your dev_dbg() call.
I have pdev when the probe cb is called, however in other funcs I
don't have it.
I'll look into other drivers see what they are doing instead
of wasting your time

>
>> >> +#define FILE_PREM 0444
>> >
>> >File permission of what?  And shouldn't it be "PERM", not "PREM"?  And
>> >why do you need it at all?  Just use the proper sysfs macros and you
>> >never need it.  See below.
>> all the sysfs files (1 for now) should be read for all users.
>
>Then use the proper sysfs macros for that, do not set it yourself.
(Y)

>
>> I'll submit a new version after a proper file structure will be decided
>> thanks again for you comments.
>
>Again, stick with one file to start with.  You can always change that
>later once it is merged if you really need it.
>
>thanks,
>
>greg k-h

great, thanks.

Eial

Powered by blists - more mailing lists