On Wed, Jul 03, 2024 at 12:05:44PM +0100, Roy Hopkins wrote: > The function sev_encrypt_flash() checks to see if the return value of > launch_update_data() < 0, but the function returns a non-zero (and not > necessarily negative) result on error. This means that some errors in > updating launch data will result in the function returning success.
I'm not sure that positive return values are intended as an error ? The sev_launch_update_data method does: if (!addr || !len) { return 1; } which seems to suggest that passing a NULL addr / zero-length, was intended to be treated as a no-op, rather than an error. If that interpretation is wrong, then, I'd suggest that sev_launch_update_data be changed to return '-1' in this case, since QEMU standard is to consider negative values as errors. > > In addition, the function takes an Error parameter which is not used > when an error is actually returned. > > The return value is now checked for non-zero to indicate an error and > a suitable error message is logged. > > Signed-off-by: Roy Hopkins <roy.hopk...@suse.com> > --- > target/i386/sev.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 3ab8b3c28b..491ca5369e 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -1542,12 +1542,9 @@ sev_encrypt_flash(hwaddr gpa, uint8_t *ptr, uint64_t > len, Error **errp) > > /* if SEV is in update state then encrypt the data else do nothing */ > if (sev_check_state(sev_common, SEV_STATE_LAUNCH_UPDATE)) { > - int ret; > - > - ret = klass->launch_update_data(sev_common, gpa, ptr, len); > - if (ret < 0) { > - error_setg(errp, "SEV: Failed to encrypt pflash rom"); > - return ret; > + if (klass->launch_update_data(sev_common, gpa, ptr, len)) { > + error_setg(errp, "SEV: Failed to encrypt flash"); > + return -1; sev_launch_update_data() calls error_report with the real error details, and here we file a information-less error message. Can we add "Error **errp" to the 'launch_update_data' API contract, and change the error_report() calls to error_setg(), so we have the useful information propagated in the Error object. 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 :|