Message ID | 20220928195633.2348848-1-quic_eberman@quicinc.com |
---|---|
Headers | show |
Series | Drivers for gunyah hypervisor | expand |
On Sun, 2022-10-02 at 16:30 -0700, Jeff Johnson wrote: > On 9/29/2022 12:36 AM, Joe Perches wrote: > > On Wed, 2022-09-28 at 12:56 -0700, Elliot Berman wrote: > > > Add a sysfs node to list the features that the Gunyah hypervisor and > > > Linux supports. For now, Linux support cspace (capability IDs) and > > > message queues, so only report those.. > > [] > > > diff --git a/drivers/virt/gunyah/sysfs.c b/drivers/virt/gunyah/sysfs.c > > [] > > > @@ -25,9 +25,24 @@ static ssize_t variant_show(struct kobject *kobj, struct kobj_attribute *attr, c > > > } > > > static struct kobj_attribute variant_attr = __ATTR_RO(variant); > > > > > > +static ssize_t features_show(struct kobject *kobj, struct kobj_attribute *attr, char *buffer) > > > +{ > > > + int len = 0; > > > + > > > + if (GH_IDENTIFY_PARTITION_CSPACE(gunyah_api.flags)) > > > + len += sysfs_emit_at(buffer, len, "cspace "); > > > + if (GH_IDENTIFY_MSGQUEUE(gunyah_api.flags)) > > > + len += sysfs_emit_at(buffer, len, "message-queue "); > > > + > > > + len += sysfs_emit_at(buffer, len, "\n"); > > > + return len; > > > +} > > > > It's generally nicer to avoid unnecessary output spaces. > > > > Perhaps: > > > > { > > int len = 0; > > > > if (GH_IDENTIFY_PARTITION_CSPACE(gunyah_api.flags)) > > len += sysfs_emit_at(buffer, len, "cspace"); > > if (GH_IDENTIFY_MSGQUEUE(gunyah_api.flags)) { > > if (len) > > len += sysfs_emit_at(buffer, len, " "); > > len += sysfs_emit_at(buffer, len, "message-queue"); > > } > > > > len += sysfs_emit_at(buffer, len, "\n"); > > > > return len; > > } > > > > that approach seems ok for 2 features, but imo doesn't scale for more. > I like the original code with one exception: > > if (GH_IDENTIFY_PARTITION_CSPACE(gunyah_api.flags)) > len += sysfs_emit_at(buffer, len, "cspace "); > if (GH_IDENTIFY_MSGQUEUE(gunyah_api.flags)) > len += sysfs_emit_at(buffer, len, "message-queue "); > > /* overwrite last trailing space */ > if (len) > len--; > > len += sysfs_emit_at(buffer, len, "\n"); > return len; > That's fine as long as every formatted output uses a trailing space. A trivial negative would be that the linker would generally not be able to deduplicate these output strings with trailing spaces across the entire codebase.
On 9/30/2022 5:06 AM, Greg Kroah-Hartman wrote: > On Wed, Sep 28, 2022 at 12:56:29PM -0700, Elliot Berman wrote: >> Add a sysfs node to list the features that the Gunyah hypervisor and >> Linux supports. For now, Linux support cspace (capability IDs) and >> message queues, so only report those.. >> >> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> >> --- >> Documentation/ABI/testing/sysfs-hypervisor-gunyah | 15 +++++++++++++++ >> drivers/virt/gunyah/sysfs.c | 15 +++++++++++++++ >> 2 files changed, 30 insertions(+) >> >> diff --git a/Documentation/ABI/testing/sysfs-hypervisor-gunyah b/Documentation/ABI/testing/sysfs-hypervisor-gunyah >> index 7d74e74e9edd..6d0cde30355a 100644 >> --- a/Documentation/ABI/testing/sysfs-hypervisor-gunyah >> +++ b/Documentation/ABI/testing/sysfs-hypervisor-gunyah >> @@ -1,3 +1,18 @@ >> +What: /sys/hypervisor/gunyah/features >> +Date: October 2022 >> +KernelVersion: 6.1 >> +Contact: linux-arm-msm@vger.kernel.org >> +Description: If running under Gunyah: >> + Space separated list of features supported by Linux and Gunyah: >> + "cspace": Gunyah devices >> + "doorbell": Sending/receiving virtual interrupts via Gunyah doorbells >> + "message-queue": Sending/receiving messages via Gunyah message queues >> + "vic": Interrupt lending >> + "vpm": Virtual platform management >> + "vcpu": Virtual CPU management >> + "memextent": Memory lending/management >> + "trace": Gunyah hypervisor tracing > > Please no. Why do you really need this type of list? What hypervisor > will NOT have them all present already? Who will use this file and what > will it be used for? > > sysfs files should just be 1 value and not need to be parsed. Yes, we > have lists of features at times, but really, you need a very very good > reason why this is the only way this information can be exposed and who > is going to use it in order to be able to have this accepted. > We're currently at phase where all the features implemented so far as considered part of the "base" featureset. We're thinking of future features implemented in Gunyah: userspace might need to know that some hypervisor feature is present and that it should make use of the feature instead of some fallback behavior. I can drop this and it should be OK IMO to introduce it later if needed. The lack of the "gunyah/features" node would be sufficient for a userspace program to know that some new feature isn't present. > thanks, > > greg k-h