igvm: pad with zero byte instead of zero ascii

[katexochen]

https://github.com/microsoft/igvm/blame/igvm_defs-v0.3.4/igvm_defs/src/lib.rs#L58-L59

[katexochen]

@liuw can this be merged, or does it need review of someone else?

[MahatiC]

@katexochen could you make sure the tests are passing for this to be merged? The CI run has failed for this

[katexochen]

@microsoft-github-policy-service agree company="Edgeless Systems"

[katexochen]

@MahatiC Fixed the tests.

[KenGordon]

@katexochen I think I understand what you are trying to do but to be sure I'd like you to explicitly say so. Linking to a couple of lines of description in a doc and expecting me to read the diff and work it out is not something I am comfortable to approve. Have you discussed this with Wei and so you are assuming it is golden? Have you booted a UVM made with the change? In a Linux VM? In a Windows VM?

[katexochen]

I think I understand what you are trying to do but to be sure I'd like you to explicitly say so.

igvm-tooling uses an ascii '0' for padding, but the spec specifies padding as zero, which means zero bytes (as usually used for padding), not ascii.

Have you discussed this with Wei and so you are assuming it is golden?

Not sure what you mean, there hasn't been any out-of-band communication.

Have you booted a UVM made with the change? In a Linux VM? In a Windows VM?

We are running this change with cloud hypervisor on AKS and can boot Linux VMs without problem. Please test Windows VMs yourself.

[jinankjain]

@katexochen @KenGordon I have tested the changes, and the guest is booting fine with these changes on the CloudHypervisor side.

[KenGordon]

@katexochen @KenGordon I have tested the changes, and the guest is booting fine with these changes on the CloudHypervisor side.

I need it tested with Windows. I might happen to do that in the next few days. It looks like an obvious fix so I don't expect it to be a problem but I would rather find out now that be puzzled randomly later. I am not going to drop what I or anyone in my team is doing to test a cosmetic fix though. I might prefer to edit Chris's doc to say that the padding is ignored and can bet any old nonsense, which seems to be the actual truth. :)

[KenGordon]

To be absolutely clear. Do not merge this until I have approved it. I will only approve it having seen a UVM boot on Windows.

[katexochen]

I might prefer to edit Chris's doc

(assuming "Chris's doc" is referring to the IGVM spec) That is a somewhat weird understanding of a public specification that has multiple parties depending on it outside of Microsoft.

[KenGordon]

If someone makes the loader insist on zero padding then it seems to me it will break all previous igvm files made with ASCII "0" padding. Generally padding is ignored, it is an unnecessary burden to insist it is set to any particular value. I think that the documentation you link to is actually wrong using the work MUST and ought to have used the word SHOULD. Now, clearly it is nicer to fix the places that generate this to generate the value intended (binary zero) but if people expect to take the MUST literally and reject files with non zero padding then things will fail needlessly.

Is your motivation for the change that you have noticed a difference between spec and implementation and want to tidy it or do you have an actual failure case you are trying to fix?

This goes to the business with how to present the reasons behind a PR such that the people who need to review it and give proper consideration to the implications can be best informed.

[katexochen]

Generally padding is ignored

It is part of the checksum!