| Message ID | 1441012217-8213-2-git-send-email-markmb@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote: > From: "Gabriel L. Somlo" <somlo@cmu.edu> > > Document the behavior of fw_cfg_modify_iXX() for leak-less updating > of integer-type blobs. > > Currently only fw_cfg_modify_i16() is coded, but 32- and 64-bit versions > may be added later if necessary.. > > Signed-off-by: Gabriel Somlo <somlo@cmu.edu> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > --- > docs/specs/fw_cfg.txt | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt > index 74351dd..5bc7b96 100644 > --- a/docs/specs/fw_cfg.txt > +++ b/docs/specs/fw_cfg.txt > @@ -159,6 +159,17 @@ will convert a 16-, 32-, or 64-bit integer to little-endian, then add > a dynamically allocated copy of the appropriately sized item to fw_cfg > under the given selector key value. > > +== fw_cfg_modify_iXX() == > + > +Modify the value of an XX-bit item (where XX may be 16, 32, or 64). > +Similarly to the corresponding fw_cfg_add_iXX() function set, convert > +a 16-, 32-, or 64-bit integer to little endian, create a dynamically > +allocated copy of the required size, and replace the existing item at > +the given selector key value with the newly allocated one. The previous > +item, assumed to have been allocated during an earlier call to > +fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed > +before the function returns. > + > == fw_cfg_add_file() == > > Given a filename (i.e., fw_cfg item name), starting pointer, and size, This doesn't cover fw_cfg_modify_file(); is that intentional? As an aside, shouldn't this function-level documentation be done via doc-comments in the header file where the prototypes are declared? (You don't need to move the docs around in this series, but it might be nice to do at some point.) thanks -- PMM
On Tue, Sep 01, 2015 at 06:33:25PM +0100, Peter Maydell wrote: > On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote: > > From: "Gabriel L. Somlo" <somlo@cmu.edu> > > > > Document the behavior of fw_cfg_modify_iXX() for leak-less updating > > of integer-type blobs. > > > > Currently only fw_cfg_modify_i16() is coded, but 32- and 64-bit versions > > may be added later if necessary.. > > > > Signed-off-by: Gabriel Somlo <somlo@cmu.edu> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > --- > > docs/specs/fw_cfg.txt | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt > > index 74351dd..5bc7b96 100644 > > --- a/docs/specs/fw_cfg.txt > > +++ b/docs/specs/fw_cfg.txt > > @@ -159,6 +159,17 @@ will convert a 16-, 32-, or 64-bit integer to little-endian, then add > > a dynamically allocated copy of the appropriately sized item to fw_cfg > > under the given selector key value. > > > > +== fw_cfg_modify_iXX() == > > + > > +Modify the value of an XX-bit item (where XX may be 16, 32, or 64). > > +Similarly to the corresponding fw_cfg_add_iXX() function set, convert > > +a 16-, 32-, or 64-bit integer to little endian, create a dynamically > > +allocated copy of the required size, and replace the existing item at > > +the given selector key value with the newly allocated one. The previous > > +item, assumed to have been allocated during an earlier call to > > +fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed > > +before the function returns. > > + > > == fw_cfg_add_file() == > > > > Given a filename (i.e., fw_cfg item name), starting pointer, and size, > > This doesn't cover fw_cfg_modify_file(); is that intentional? There should already be a paragraph in there (further down from where this is supposed to go in) describing fw_cfg_modify_file(), which isn't affected by this. > > As an aside, shouldn't this function-level documentation be done > via doc-comments in the header file where the prototypes are > declared? (You don't need to move the docs around in this series, > but it might be nice to do at some point.) You mean, leave docs/specs/fw_cfg.txt to deal with the guest-side port/mmio api only, and have the host-side functions simply documented as comments in include/hw/nvram/fw_cfg.h ? That should be relatively painless, if that's the agreed-upon convention... Thanks, --Gabriel
On 1 September 2015 at 18:45, Gabriel L. Somlo <somlo@cmu.edu> wrote: > On Tue, Sep 01, 2015 at 06:33:25PM +0100, Peter Maydell wrote: >> This doesn't cover fw_cfg_modify_file(); is that intentional? > > There should already be a paragraph in there (further down from where > this is supposed to go in) describing fw_cfg_modify_file(), which > isn't affected by this. Sorry, so there is. >> As an aside, shouldn't this function-level documentation be done >> via doc-comments in the header file where the prototypes are >> declared? (You don't need to move the docs around in this series, >> but it might be nice to do at some point.) > > You mean, leave docs/specs/fw_cfg.txt to deal with the guest-side > port/mmio api only, and have the host-side functions simply documented > as comments in include/hw/nvram/fw_cfg.h ? > > That should be relatively painless, if that's the agreed-upon > convention... Yes. I think at the point this file was written we probably hadn't started using doc-comments for our APIs. (My usual place to crib the formatting for doc-comments is the extract/deposit APIs in bitops.h.) thanks -- PMM
On Tue, Sep 01, 2015 at 07:45:27PM +0100, Peter Maydell wrote: > On 1 September 2015 at 18:45, Gabriel L. Somlo <somlo@cmu.edu> wrote: > > On Tue, Sep 01, 2015 at 06:33:25PM +0100, Peter Maydell wrote: > >> As an aside, shouldn't this function-level documentation be done > >> via doc-comments in the header file where the prototypes are > >> declared? (You don't need to move the docs around in this series, > >> but it might be nice to do at some point.) > > > > You mean, leave docs/specs/fw_cfg.txt to deal with the guest-side > > port/mmio api only, and have the host-side functions simply documented > > as comments in include/hw/nvram/fw_cfg.h ? > > > > That should be relatively painless, if that's the agreed-upon > > convention... > > Yes. I think at the point this file was written we probably hadn't > started using doc-comments for our APIs. (My usual place to crib > the formatting for doc-comments is the extract/deposit APIs in > bitops.h.) OK, I guess I'll wait until after the dust settles on fw_cfg/DMA before further mucking with the doc or header files, but again, this shouldn't be too painless... Also, since I'll be tinkering with fw_cfg again, and you mentioned using DT on arm and ACPI on x86 to auto-detect the presence (and location) of fw_cfg from the guest-side in a related thread: Right now, I don't think fw_cfg is listed in ACPI, and I would like it to be. So what should I do, simply figure out how to add a new node somewhere in the SSDT and submit a patch for that ? Could it be that simple, or am I missing something ? :) Thanks, --Gabriel
On 1 September 2015 at 20:13, Gabriel L. Somlo <somlo@cmu.edu> wrote: > Also, since I'll be tinkering with fw_cfg again, and you mentioned > using DT on arm and ACPI on x86 to auto-detect the presence (and location) > of fw_cfg from the guest-side in a related thread: I meant DT or ACPI on ARM, actually. I don't know what the x86 approach is for fw_cfg but I think it's just "known address". (For ARM we don't I think currently expose fw_cfg in the ACPI tables, but we probably ought to; only needed for the edge case where you actually wanted to read fw_cfg in the guest kernel rather than just the guest firmware, though.) thanks -- PMM
On Tue, Sep 01, 2015 at 09:10:23PM +0100, Peter Maydell wrote: > On 1 September 2015 at 20:13, Gabriel L. Somlo <somlo@cmu.edu> wrote: > > Also, since I'll be tinkering with fw_cfg again, and you mentioned > > using DT on arm and ACPI on x86 to auto-detect the presence (and location) > > of fw_cfg from the guest-side in a related thread: > > I meant DT or ACPI on ARM, actually. I don't know what the x86 > approach is for fw_cfg but I think it's just "known address". > (For ARM we don't I think currently expose fw_cfg in the ACPI > tables, but we probably ought to; only needed for the edge > case where you actually wanted to read fw_cfg in the guest > kernel rather than just the guest firmware, though.) Right, that's what I'm trying to do -- expose fw_cfg in /sys/firmware/... on the guest -- which involves figuring out how to determine if it is present (DT on ARM, blindly poking port 0x510 on x86 for now, hence my interest in having it listed in ACPI). Now, if we did expose fw_cfg in ACPI on *both* ARM and x86, I wonder I could get away with an ACPI-only detection scheme across both architectures... But ACPI is all we have on x86, so on the QEMU side of things it looks like I'll have to add it regardless... Thanks, --Gabriel
On 1 September 2015 at 21:27, Gabriel L. Somlo <somlo@cmu.edu> wrote: > Right, that's what I'm trying to do -- expose fw_cfg in > /sys/firmware/... on the guest -- which involves figuring out how > to determine if it is present (DT on ARM, blindly poking port 0x510 > on x86 for now, hence my interest in having it listed in ACPI). > > Now, if we did expose fw_cfg in ACPI on *both* ARM and x86, I wonder > I could get away with an ACPI-only detection scheme across both > architectures... Nope, because there's no requirement for ACPI on ARM -- you could be booting the kernel via device tree. thanks -- PMM
On Di, 2015-09-01 at 21:10 +0100, Peter Maydell wrote: > On 1 September 2015 at 20:13, Gabriel L. Somlo <somlo@cmu.edu> wrote: > > Also, since I'll be tinkering with fw_cfg again, and you mentioned > > using DT on arm and ACPI on x86 to auto-detect the presence (and location) > > of fw_cfg from the guest-side in a related thread: > > I meant DT or ACPI on ARM, actually. I don't know what the x86 > approach is for fw_cfg but I think it's just "known address". Yes, "known address". And given that the firmware actually loads the acpi tables via fw_cfg that is very unlikely to change. Adding it to the acpi tables might be useful nevertheless so the guest os knows the ioports are in use. I think on arm the acpi situation is the same, but I think firmware detecting the location via DT should work without chicken&egg problems. cheers, Gerd
On 09/02/15 10:08, Gerd Hoffmann wrote: > On Di, 2015-09-01 at 21:10 +0100, Peter Maydell wrote: >> On 1 September 2015 at 20:13, Gabriel L. Somlo <somlo@cmu.edu> wrote: >>> Also, since I'll be tinkering with fw_cfg again, and you mentioned >>> using DT on arm and ACPI on x86 to auto-detect the presence (and location) >>> of fw_cfg from the guest-side in a related thread: >> >> I meant DT or ACPI on ARM, actually. I don't know what the x86 >> approach is for fw_cfg but I think it's just "known address". > > Yes, "known address". And given that the firmware actually loads the > acpi tables via fw_cfg that is very unlikely to change. Adding it to > the acpi tables might be useful nevertheless so the guest os knows the > ioports are in use. > > I think on arm the acpi situation is the same, but I think firmware > detecting the location via DT should work without chicken&egg problems. First of all I apologize for lagging severely behind this thread; in practice I can't read emails longer than 20 lines or so. This one qualifies, so I'll respond: Yes, on qemu-system-(arm|aarch64) -M virt, detecting the fw_cfg registers via DT should continue to work without any dependencies. Thanks Laszlo
diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt index 74351dd..5bc7b96 100644 --- a/docs/specs/fw_cfg.txt +++ b/docs/specs/fw_cfg.txt @@ -159,6 +159,17 @@ will convert a 16-, 32-, or 64-bit integer to little-endian, then add a dynamically allocated copy of the appropriately sized item to fw_cfg under the given selector key value. +== fw_cfg_modify_iXX() == + +Modify the value of an XX-bit item (where XX may be 16, 32, or 64). +Similarly to the corresponding fw_cfg_add_iXX() function set, convert +a 16-, 32-, or 64-bit integer to little endian, create a dynamically +allocated copy of the required size, and replace the existing item at +the given selector key value with the newly allocated one. The previous +item, assumed to have been allocated during an earlier call to +fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed +before the function returns. + == fw_cfg_add_file() == Given a filename (i.e., fw_cfg item name), starting pointer, and size,