Messages in this thread | | | From | "Czerwacki, Eial" <> | Subject | Re: [PATCH v2] drivers/virt/vSMP: new driver | Date | Thu, 25 Aug 2022 10:16:59 +0000 |
| |
>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@sap.com> >> >> Acked-by: Leonid Arsh <leonid.arsh@sap.com> >> >> Acked-by: Oren Twaig <oren.twaig@sap.com> >> >> CC: SAP vSMP Linux Maintainer <linux.vsmp@sap.com> >> >> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> >> CC: Arnd Bergmann <arnd@arndb.de> >> >> CC: Dan Carpenter <dan.carpenter@oracle.com> >> >> CC: Andra Paraschiv <andraprs@amazon.com> >> >> CC: Borislav Petkov <bp@suse.de> >> >> CC: Brijesh Singh <brijesh.singh@amd.com> >> >> CC: Eric Biggers <ebiggers@google.com> >> >> CC: Fei Li <fei1.li@intel.com> >> >> CC: Hans de Goede <hdegoede@redhat.com> >> >> CC: Jens Axboe <axboe@kernel.dk> >> >> CC: Mauro Carvalho Chehab <mchehab+huawei@kernel.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@sap.com> >> >> + linux-vsmp@sap.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@sap.com> >> >> +M: linux-vsmp@sap.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
| |