NVIDIA: SAUCE: hw/arm/virt: Define MEC as shared or private

[ianm-nv]

Kernel MEC support has landed, reintroduce this updated patch which enables shared MEC functionality in QEMU. This patch also contains functionality for handling kernels that don't have MEC support. Below is a short summary of MEC behavior:

Set MEC in QEMU:

• If no shareability is requested, use the kernel default.
• If private MEC is requested, abort initialization if setting MEC fails.
• If shared MEC is requested, try setting MEC. If there is no kernel support, continue.

[nvmochs]

Confirmed the branch builds with this commit.

Couple of comments...

I think the commit message could be a bit clearer. For example, it was not immediately obvious to me that mec_specified is used to indicate when shareability is or is not requested. I suspect these patches will cleaned up further during upstreaming, so I don’t see this as a blocker but wanted to mention this feedback.

---

  if (ret) {
        if (private_mec_requested) {
            error_setg_errno(errp, EINVAL,
                "Private MECID requested but MEC query failed - kernel may not support MEC");
            return ret;
        }

Should the errno be set to “ret” instead of EINVAL?

---

   /* Check if MEC is supported */
    if (!mec_query.mec_supported) {
        if (private_mec_requested) {
            error_setg_errno(errp, EINVAL,
                "Private MECID requested but MEC is not supported by the kernel");
            return -EINVAL;
        }
        /* For shared MEC, we can continue without MEC support */
        warn_report("MEC is not supported by the kernel - continuing without MEC configuration\n");
        return 0;
    }

Not a big deal, but this block is very similar to the one before it and they could likely be combined.

e.g.

if (ret || !mec_query.mec_supported) {
...

---

General question, I see various differences in what is returned / logged: “ret”, “-ret”, “EINVAL”, “-EINVAL”. Shouldn’t the code be consistent with everything using negative (or positive) returns codes?

[ianm-nv]

Confirmed the branch builds with this commit.

Couple of comments...

I think the commit message could be a bit clearer. For example, it was not immediately obvious to me that mec_specified is used to indicate when shareability is or is not requested. I suspect these patches will cleaned up further during upstreaming, so I don’t see this as a blocker but wanted to mention this feedback.

  if (ret) {
        if (private_mec_requested) {
            error_setg_errno(errp, EINVAL,
                "Private MECID requested but MEC query failed - kernel may not support MEC");
            return ret;
        }

Should the errno be set to “ret” instead of EINVAL?

   /* Check if MEC is supported */
    if (!mec_query.mec_supported) {
        if (private_mec_requested) {
            error_setg_errno(errp, EINVAL,
                "Private MECID requested but MEC is not supported by the kernel");
            return -EINVAL;
        }
        /* For shared MEC, we can continue without MEC support */
        warn_report("MEC is not supported by the kernel - continuing without MEC configuration\n");
        return 0;
    }

Not a big deal, but this block is very similar to the one before it and they could likely be combined.

e.g.

if (ret || !mec_query.mec_supported) {
...

General question, I see various differences in what is returned / logged: “ret”, “-ret”, “EINVAL”, “-EINVAL”. Shouldn’t the code be consistent with everything using negative (or positive) returns codes?

Thanks @nvmochs, I agree with your feedback. I think I need to take a different approach here, I was putting more weight on implementing the functionality and not enough on patch correctness. I'll sync with the author to get this cleaned up.

[ianm-nv]

@nvmochs I've pushed an updated patch, which I think properly addresses feedback.

The commit message should contain more detail in describing the 'shared-mec' property as well as what private/shared provide. I also added a section briefly explaining the conditional checking based on feedback from the kernel.

Also adjusted the error handling to where 'error_setg_errno' should take a positive error code and the 'rme_configure_mec' function will return an negative error code, which appears to follow QEMU standards.

I also changed the 'EINVAL' to 'ENOSUP' when the kernel mec query reports that mec is not supported.

[nvmochs]

Thanks for the updated commit message, it is much clearer how this is supposed to flow and how it is invoked. Regarding the errnos, I appreciate your explaining as to why the code is negating the value in some places while using it directly in others.

No further issues from me.

Acked-by: Matthew R. Ochs <mochs@nvidia.com>

[shamiali2008]

Looks good to me.
Acked-by: Shameer Kolothum skolothumtho@nvidia.com

[nvmochs]

Merged, closing PR.