| Message ID | 20240304194331.1586191-14-nifan.cxl@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enabling DCD emulation support in Qemu | expand |
On Mon, 4 Mar 2024 11:34:08 -0800 nifan.cxl@gmail.com wrote: > From: Fan Ni <fan.ni@samsung.com> > > With the change, we add the following two QMP interfaces to print out > extents information in the device, > 1. cxl-display-accepted-dc-extents: print out the accepted DC extents in > the device; > 2. cxl-display-pending-to-add-dc-extents: print out the pending-to-add > DC extents in the device; > The output is appended to a file passed to the command and by default > it is /tmp/dc-extent.txt. Hi Fan, Is there precedence for this sort of logging to a file from a qmp command? I can see something like this being useful. A few comments inline. Jonathan > > Signed-off-by: Fan Ni <fan.ni@samsung.com> > --- > hw/mem/cxl_type3.c | 80 ++++++++++++++++++++++++++++++++++++++++ > hw/mem/cxl_type3_stubs.c | 12 ++++++ > qapi/cxl.json | 32 ++++++++++++++++ > 3 files changed, 124 insertions(+) > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index 5bd64e604e..6a08e7ae40 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -2089,6 +2089,86 @@ void qmp_cxl_release_dynamic_capacity(const char *path, uint8_t region_id, > region_id, records, errp); > } > > +static void cxl_dcd_display_extent_list(const CXLType3Dev *dcd, const char *f, > + bool accepted_list, Error **errp) > +{ > + const CXLDCExtentList *list; > + CXLDCExtent *ent; > + FILE *fp = NULL; > + int i = 0; > + > + if (!dcd->dc.num_regions) { > + error_setg(errp, "No dynamic capacity support from the device"); > + return; > + } > + > + if (!f) { > + fp = fopen("/tmp/dc-extent.txt", "a+"); > + } else { > + fp = fopen(f, "a+"); > + } > + > + if (!fp) { > + error_setg(errp, "Open log file failed"); > + return; > + } > + if (accepted_list) { > + list = &dcd->dc.extents; > + fprintf(fp, "Print accepted extent info:\n"); > + } else { > + list = &dcd->dc.extents_pending_to_add; > + fprintf(fp, "Print pending-to-add extent info:\n"); > + } > + > + QTAILQ_FOREACH(ent, list, node) { > + fprintf(fp, "%d: [0x%lx - 0x%lx]\n", i++, ent->start_dpa, > + ent->start_dpa + ent->len); > + } > + fprintf(fp, "In total, %d extents printed!\n", i); > + fclose(fp); > +} > +void qmp_cxl_display_pending_to_add_dc_extents(const char *path, const char *f, > + Error **errp) > +{ > + Object *obj; > + CXLType3Dev *dcd; > + > + obj = object_resolve_path(path, NULL); As an aside, we could probably flatten a lot of these cases into object_resolve_path_type() > + if (!obj) { > + error_setg(errp, "Unable to resolve path"); > + return; > + } > + if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) { > + error_setg(errp, "Path not point to a valid CXL type3 device"); > + return; > + } > + > + > + dcd = CXL_TYPE3(obj); > + cxl_dcd_display_extent_list(dcd, f, false, errp); > +} > + > static void ct3_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > diff --git a/qapi/cxl.json b/qapi/cxl.json > index 2645004666..6f10300ec6 100644 > --- a/qapi/cxl.json > +++ b/qapi/cxl.json > @@ -420,3 +420,35 @@ > 'extents': [ 'CXLDCExtentRecord' ] > } > } > + > +## > +# @cxl-display-accepted-dc-extents: > +# > +# Command to print out all the accepted DC extents in the device > +# > +# @path: CXL DCD canonical QOM path > +# @output: path of output file to dump the results to We take a path, but dump to the same file whatever this is set to? I'm not sure what precedence there is for qom commands that dump to a debug log. Perhaps reference any other cases in the patch description. > +# > +# Since : 9.0 > +## > +{ 'command': 'cxl-display-accepted-dc-extents', > + 'data': { 'path': 'str', > + 'output': 'str' > + } > +} > + > +## > +# @cxl-display-pending-to-add-dc-extents: > +# > +# Command to print out all the pending-to-add DC extents in the device > +# > +# @path: CXL DCD canonical QOM path > +# @output: path of output file to dump the results to > +# > +# Since : 9.0 > +## > +{ 'command': 'cxl-display-pending-to-add-dc-extents', > + 'data': { 'path': 'str', > + 'output': 'str' > + } > +}
On Tue, Mar 05, 2024 at 04:09:08PM +0000, Jonathan Cameron via wrote: > On Mon, 4 Mar 2024 11:34:08 -0800 > nifan.cxl@gmail.com wrote: > > > From: Fan Ni <fan.ni@samsung.com> > > > > With the change, we add the following two QMP interfaces to print out > > extents information in the device, > > 1. cxl-display-accepted-dc-extents: print out the accepted DC extents in > > the device; > > 2. cxl-display-pending-to-add-dc-extents: print out the pending-to-add > > DC extents in the device; > > The output is appended to a file passed to the command and by default > > it is /tmp/dc-extent.txt. > Hi Fan, > > Is there precedence for this sort of logging to a file from a qmp > command? I can see something like this being useful. This is pretty unusual. For runtime debugging information our strong preference is to integrate 'trace' probes throughout the code: https://www.qemu.org/docs/master/devel/tracing.html#tracing With regards, Daniel
On Tue, Mar 05, 2024 at 04:15:30PM +0000, Daniel P. Berrangé wrote: > On Tue, Mar 05, 2024 at 04:09:08PM +0000, Jonathan Cameron via wrote: > > On Mon, 4 Mar 2024 11:34:08 -0800 > > nifan.cxl@gmail.com wrote: > > > > > From: Fan Ni <fan.ni@samsung.com> > > > > > > With the change, we add the following two QMP interfaces to print out > > > extents information in the device, > > > 1. cxl-display-accepted-dc-extents: print out the accepted DC extents in > > > the device; > > > 2. cxl-display-pending-to-add-dc-extents: print out the pending-to-add > > > DC extents in the device; > > > The output is appended to a file passed to the command and by default > > > it is /tmp/dc-extent.txt. > > Hi Fan, > > > > Is there precedence for this sort of logging to a file from a qmp > > command? I can see something like this being useful. > > This is pretty unusual. Yeah. I cannot find anything similar in existing code, my initial plan was to print out to the screen directly, however, cannot find out how to do it nicely, so decided to go with a file. Is there a reason why we do not want to go with this approach? > > For runtime debugging information our strong preference is to integrate > 'trace' probes throughout the code: > > https://www.qemu.org/docs/master/devel/tracing.html#tracing I am not familiar with the trace mechanism. However, I think the approach in this patch may be useful not only for debugging purpose. Although not tried yet, maybe we can also use the approach to set some parameters at runtime like what procfs does? Just a rough thought. Fan > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On Tue, Mar 05, 2024 at 09:09:05AM -0800, fan wrote: > On Tue, Mar 05, 2024 at 04:15:30PM +0000, Daniel P. Berrangé wrote: > > On Tue, Mar 05, 2024 at 04:09:08PM +0000, Jonathan Cameron via wrote: > > > On Mon, 4 Mar 2024 11:34:08 -0800 > > > nifan.cxl@gmail.com wrote: > > > > > > > From: Fan Ni <fan.ni@samsung.com> > > > > > > > > With the change, we add the following two QMP interfaces to print out > > > > extents information in the device, > > > > 1. cxl-display-accepted-dc-extents: print out the accepted DC extents in > > > > the device; > > > > 2. cxl-display-pending-to-add-dc-extents: print out the pending-to-add > > > > DC extents in the device; > > > > The output is appended to a file passed to the command and by default > > > > it is /tmp/dc-extent.txt. > > > Hi Fan, > > > > > > Is there precedence for this sort of logging to a file from a qmp > > > command? I can see something like this being useful. > > > > This is pretty unusual. > > Yeah. I cannot find anything similar in existing code, my initial plan > was to print out to the screen directly, however, cannot find out how to > do it nicely, so decided to go with a file. > > Is there a reason why we do not want to go with this approach? > > > > > For runtime debugging information our strong preference is to integrate > > 'trace' probes throughout the code: > > > > https://www.qemu.org/docs/master/devel/tracing.html#tracing > > I am not familiar with the trace mechanism. However, I think the > approach in this patch may be useful not only for debugging purpose. > Although not tried yet, maybe we can also use the approach to set > some parameters at runtime like what procfs does? Please don't invent something new unless you can show why QEMU's existing tracing system isn't sufficiently good for the problem. QEMU's tracing can dump to the terminal directly, or integrate with a variety of other backends, and data can be turned off/on at runtime per-trace point. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Mar 05, 2024 at 09:09:05AM -0800, fan wrote: >> On Tue, Mar 05, 2024 at 04:15:30PM +0000, Daniel P. Berrangé wrote: >> > On Tue, Mar 05, 2024 at 04:09:08PM +0000, Jonathan Cameron via wrote: >> > > On Mon, 4 Mar 2024 11:34:08 -0800 >> > > nifan.cxl@gmail.com wrote: >> > > >> > > > From: Fan Ni <fan.ni@samsung.com> >> > > > >> > > > With the change, we add the following two QMP interfaces to print out >> > > > extents information in the device, >> > > > 1. cxl-display-accepted-dc-extents: print out the accepted DC extents in >> > > > the device; >> > > > 2. cxl-display-pending-to-add-dc-extents: print out the pending-to-add >> > > > DC extents in the device; >> > > > The output is appended to a file passed to the command and by default >> > > > it is /tmp/dc-extent.txt. >> > > Hi Fan, >> > > >> > > Is there precedence for this sort of logging to a file from a qmp >> > > command? I can see something like this being useful. >> > >> > This is pretty unusual. >> >> Yeah. I cannot find anything similar in existing code, my initial plan >> was to print out to the screen directly, however, cannot find out how to >> do it nicely, so decided to go with a file. >> >> Is there a reason why we do not want to go with this approach? >> >> > >> > For runtime debugging information our strong preference is to integrate >> > 'trace' probes throughout the code: >> > >> > https://www.qemu.org/docs/master/devel/tracing.html#tracing >> >> I am not familiar with the trace mechanism. However, I think the >> approach in this patch may be useful not only for debugging purpose. >> Although not tried yet, maybe we can also use the approach to set >> some parameters at runtime like what procfs does? > > Please don't invent something new unless you can show why QEMU's existing > tracing system isn't sufficiently good for the problem. QEMU's tracing > can dump to the terminal directly, or integrate with a variety of other > backends, and data can be turned off/on at runtime per-trace point. Seconded.
On Wed, Apr 24, 2024 at 03:12:34PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Tue, Mar 05, 2024 at 09:09:05AM -0800, fan wrote: > >> On Tue, Mar 05, 2024 at 04:15:30PM +0000, Daniel P. Berrangé wrote: > >> > On Tue, Mar 05, 2024 at 04:09:08PM +0000, Jonathan Cameron via wrote: > >> > > On Mon, 4 Mar 2024 11:34:08 -0800 > >> > > nifan.cxl@gmail.com wrote: > >> > > > >> > > > From: Fan Ni <fan.ni@samsung.com> > >> > > > > >> > > > With the change, we add the following two QMP interfaces to print out > >> > > > extents information in the device, > >> > > > 1. cxl-display-accepted-dc-extents: print out the accepted DC extents in > >> > > > the device; > >> > > > 2. cxl-display-pending-to-add-dc-extents: print out the pending-to-add > >> > > > DC extents in the device; > >> > > > The output is appended to a file passed to the command and by default > >> > > > it is /tmp/dc-extent.txt. > >> > > Hi Fan, > >> > > > >> > > Is there precedence for this sort of logging to a file from a qmp > >> > > command? I can see something like this being useful. > >> > > >> > This is pretty unusual. > >> > >> Yeah. I cannot find anything similar in existing code, my initial plan > >> was to print out to the screen directly, however, cannot find out how to > >> do it nicely, so decided to go with a file. > >> > >> Is there a reason why we do not want to go with this approach? > >> > >> > > >> > For runtime debugging information our strong preference is to integrate > >> > 'trace' probes throughout the code: > >> > > >> > https://www.qemu.org/docs/master/devel/tracing.html#tracing > >> > >> I am not familiar with the trace mechanism. However, I think the > >> approach in this patch may be useful not only for debugging purpose. > >> Although not tried yet, maybe we can also use the approach to set > >> some parameters at runtime like what procfs does? > > > > Please don't invent something new unless you can show why QEMU's existing > > tracing system isn't sufficiently good for the problem. QEMU's tracing > > can dump to the terminal directly, or integrate with a variety of other > > backends, and data can be turned off/on at runtime per-trace point. > > Seconded. > Thanks. This patch is removed from the latest version (v7): https://lore.kernel.org/linux-cxl/ZiaFYUB6FC9NR7W4@memverge.com/T/#t Fan
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index 5bd64e604e..6a08e7ae40 100644 --- a/hw/mem/cxl_type3.c +++ b/hw/mem/cxl_type3.c @@ -2089,6 +2089,86 @@ void qmp_cxl_release_dynamic_capacity(const char *path, uint8_t region_id, region_id, records, errp); } +static void cxl_dcd_display_extent_list(const CXLType3Dev *dcd, const char *f, + bool accepted_list, Error **errp) +{ + const CXLDCExtentList *list; + CXLDCExtent *ent; + FILE *fp = NULL; + int i = 0; + + if (!dcd->dc.num_regions) { + error_setg(errp, "No dynamic capacity support from the device"); + return; + } + + if (!f) { + fp = fopen("/tmp/dc-extent.txt", "a+"); + } else { + fp = fopen(f, "a+"); + } + + if (!fp) { + error_setg(errp, "Open log file failed"); + return; + } + if (accepted_list) { + list = &dcd->dc.extents; + fprintf(fp, "Print accepted extent info:\n"); + } else { + list = &dcd->dc.extents_pending_to_add; + fprintf(fp, "Print pending-to-add extent info:\n"); + } + + QTAILQ_FOREACH(ent, list, node) { + fprintf(fp, "%d: [0x%lx - 0x%lx]\n", i++, ent->start_dpa, + ent->start_dpa + ent->len); + } + fprintf(fp, "In total, %d extents printed!\n", i); + fclose(fp); +} + +void qmp_cxl_display_accepted_dc_extents(const char *path, const char *f, + Error **errp) +{ + Object *obj; + CXLType3Dev *dcd; + + obj = object_resolve_path(path, NULL); + if (!obj) { + error_setg(errp, "Unable to resolve path"); + return; + } + if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) { + error_setg(errp, "Path not point to a valid CXL type3 device"); + return; + } + + dcd = CXL_TYPE3(obj); + cxl_dcd_display_extent_list(dcd, f, true, errp); +} + +void qmp_cxl_display_pending_to_add_dc_extents(const char *path, const char *f, + Error **errp) +{ + Object *obj; + CXLType3Dev *dcd; + + obj = object_resolve_path(path, NULL); + if (!obj) { + error_setg(errp, "Unable to resolve path"); + return; + } + if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) { + error_setg(errp, "Path not point to a valid CXL type3 device"); + return; + } + + + dcd = CXL_TYPE3(obj); + cxl_dcd_display_extent_list(dcd, f, false, errp); +} + static void ct3_class_init(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c index d913b11b4d..d896758301 100644 --- a/hw/mem/cxl_type3_stubs.c +++ b/hw/mem/cxl_type3_stubs.c @@ -81,3 +81,15 @@ void qmp_cxl_release_dynamic_capacity(const char *path, uint8_t region_id, { error_setg(errp, "CXL Type 3 support is not compiled in"); } + +void qmp_cxl_display_accepted_dc_extents(const char *path, const char *f, + Error **errp) +{ + error_setg(errp, "CXL Type 3 support is not compiled in"); +} + +void qmp_cxl_display_pending_to_add_dc_extents(const char *path, const char *f, + Error **errp) +{ + error_setg(errp, "CXL Type 3 support is not compiled in"); +} diff --git a/qapi/cxl.json b/qapi/cxl.json index 2645004666..6f10300ec6 100644 --- a/qapi/cxl.json +++ b/qapi/cxl.json @@ -420,3 +420,35 @@ 'extents': [ 'CXLDCExtentRecord' ] } } + +## +# @cxl-display-accepted-dc-extents: +# +# Command to print out all the accepted DC extents in the device +# +# @path: CXL DCD canonical QOM path +# @output: path of output file to dump the results to +# +# Since : 9.0 +## +{ 'command': 'cxl-display-accepted-dc-extents', + 'data': { 'path': 'str', + 'output': 'str' + } +} + +## +# @cxl-display-pending-to-add-dc-extents: +# +# Command to print out all the pending-to-add DC extents in the device +# +# @path: CXL DCD canonical QOM path +# @output: path of output file to dump the results to +# +# Since : 9.0 +## +{ 'command': 'cxl-display-pending-to-add-dc-extents', + 'data': { 'path': 'str', + 'output': 'str' + } +}