[erjiaqing]

Add repeated acpi_table, device_tree_overlay (DeviceTreeOverlayArtifact), and
file_backed_mapping (FileBackedMapping) fields to message DiskArtifacts in
load_config.proto, along with regenerated Go bindings.

In cf_disk_configs.cpp, parse these fields and emit the corresponding
--crosvm_acpi_table, --crosvm_device_tree_overlay, and
--crosvm_file_backed_mapping_base64 flags. Given current usage of these flags,
validate that at most one entry per instance is provided while keeping the
proto schema as repeated for future improvements.

In the proto, only the minimal arguments for the functions are added.

Bug: 544996625
Bug: 539399873

Test: bazel test //cuttlefish/host/commands/cvd/cli/parser/...
Assisted-by: Jetski:Gemini
TAG=agy
CONV=9d324de2-493b-495b-8490-a20d8e687190

[bridadan]

It looks like these changes assume that the ACPI table file, the device tree overlay, and/or the file backed memory file are already present in the local file system. This would work for local builds, but how does this work in CI? How are these artifacts fetched? In #3386 I added logic to the fetch handler to pull the artifacts from the build server. Is that required here as well?

[erjiaqing]

It looks like these changes assume that the ACPI table file, the device tree overlay, and/or the file backed memory file are already present in the local file system. This would work for local builds, but how does this work in CI? How are these artifacts fetched? In #3386 I added logic to the fetch handler to pull the artifacts from the build server. Is that required here as well?

Thanks for the reviewing effort, I looked through #3386 again, and feel there are some feature gaps between cvd fetch and the requirement of this PR.

This PR is to provide extra parameters, instead of overriding standard images for Android. I confirmed that we can put the artifacts directly on CI.

I don't feel it is a good idea to add --dice-handover-build, --acpi-table-build, --dtb-build to cvd fetch, instead, I would prefer something like an array of --extra-artifact-build and control the file names in load_config.

Do you have any ideas on this?

[bridadan]

Based on our offline discussion, the expectation of the path for all of these fields is that they reference a file in the default_build, right?

@rmuthiah just heads up this differs from the vendor_boot and eventually the system fields since those accept a build string. Maybe a comment in each of the relevant fields would be helpful to show the difference.

[erjiaqing]

Yes, these files should be located in the system_image_dir, which is the extracted default_build zip (Assuming #3234 is merged). Added a comment.

[bridadan]

After discussing offline, these paths can either be:

• An absolute local path
• A local path relative to system_image_dir (once assemble_cvd: support relative paths for crosvm_acpi_table, crosvm_device_tree_overlay, and crosvm_file_backed_mapping #3234 is merged)
• An remote build path, including a file path (e.g. @ab/<branch>/<target>{<file path>})

[bridadan]

It looks like these changes assume that the ACPI table file, the device tree overlay, and/or the file backed memory file are already present in the local file system. This would work for local builds, but how does this work in CI? How are these artifacts fetched? In #3386 I added logic to the fetch handler to pull the artifacts from the build server. Is that required here as well?

Thanks for the reviewing effort, I looked through #3386 again, and feel there are some feature gaps between cvd fetch and the requirement of this PR.

This PR is to provide extra parameters, instead of overriding standard images for Android. I confirmed that we can put the artifacts directly on CI.

I see what you mean, it does seem different from #3386 in that way.

I don't feel it is a good idea to add --dice-handover-build, --acpi-table-build, --dtb-build to cvd fetch, instead, I would prefer something like an array of --extra-artifact-build and control the file names in load_config.

Do you have any ideas on this?

From our discussion before, it sounds like all of the artifacts that would be used are tied to the target build (what goes in the default_build field). My understanding is the goal of this effort is to be able to select individual artifacts from that build, not from a different build. So I don't know that something like --extra-artifact-build flag is necessary in this case.

I think it makes sense for these fields to reference "build local" files. It's not a pattern I had considered before but I think its ok.

@rmuthiah any thoughts?

[bridadan]

I think build instead of path would be more consistent with the other fields in this protofield (builds support @ab/... remote links and local paths.

[erjiaqing]

I realized that in the near future, we will use the same DTB and ACPI table within a group of VMs, so maybe we don't have to support fetching them from ab immediately.

So for now, we only need to support fetching the file backed mapping.

[bridadan]

Same comment here: path -> build