| Message ID | 20220928195633.2348848-11-quic_eberman@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Drivers for gunyah hypervisor | expand |
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; }
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; > } > Thanks! Applied for the next version.
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. thanks, greg k-h
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;
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
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 + What: /sys/hypervisor/gunyah/api Date: October 2022 KernelVersion: 6.1 diff --git a/drivers/virt/gunyah/sysfs.c b/drivers/virt/gunyah/sysfs.c index ec11510cbece..f8ec0553c197 100644 --- 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; +} +static struct kobj_attribute features_attr = __ATTR_RO(features); + static struct attribute *gunyah_attrs[] = { &api_attr.attr, &variant_attr.attr, + &features_attr.attr, NULL };
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(+)