[PATCH v2 2/5] ACRN:DM:PCI: Load rom_file and map it into PCI ROMbar


Zhao, Yakui
 

PCI ROM is the firmware specific to PCI device and it is provided by
the device vendor. The PCI rom resides in 0x30 offset of PCI config space.
This can be used to check whether the PCI rom exists. And when it exists,
it can load the firmware from the addr that is obtained from ROM bar addr.

For the user-vm, it will try to load the rom_file for the given PCI device and
enable the VM to access the firmware that is defined in rom_file.

BTW: The emulated rom_file is converted from efi image by using EfiRom. It has
no dependency on the ROM bar of physical PCI devices. Of course if the physical
PCI devices supports the ROM bar, the rom_file can also be dumped from the PCI
rom.

Now this is limited to PCI display device.

Signed-off-by: Zhao Yakui <yakui.zhao@...>
---
devicemodel/hw/pci/passthrough.c | 94 ++++++++++++++++++++++++++++++++
devicemodel/include/passthru.h | 4 +-
2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/passthrough.c b/devicemodel/hw/pci/passthrough.c
index 4c227aea9..9590d95bc 100644
--- a/devicemodel/hw/pci/passthrough.c
+++ b/devicemodel/hw/pci/passthrough.c
@@ -600,6 +600,13 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
struct acrn_pcidev pcidev = {};
uint16_t vendor = 0, device = 0;
uint8_t class = 0;
+ char *rom_buffer;
+ uint64_t bar_addr;
+ FILE *fp;
+ int file_size, rom_size;
+ uint8_t rom_header[4];
+ size_t read_size;
+ char rom_file[256];

ptdev = NULL;
error = -EINVAL;
@@ -615,6 +622,7 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
return -EINVAL;
}

+ memset(rom_file, 0, sizeof(rom_file));
while ((opt = strsep(&opts, ",")) != NULL) {
if (!strncmp(opt, "keep_gsi", 8))
keep_gsi = true;
@@ -711,6 +719,72 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
error = ACRN_PTDEV_IRQ_MSI;
}

+ if ((class != 3) || (dev->bus != 0)) {
+ if (ptdev->need_rombar) {
+ pr_warn("PCI rom is only supported on Bus 0 and GPU device\n");
+ ptdev->need_rombar = false;
+ }
+ }
+
+ if (ptdev->need_rombar) {
+
+ fp = fopen(rom_file, "rb");
+ if (fp == NULL) {
+ pr_warn("Fail to open %s rom_file\n", rom_file);
+ ptdev->need_rombar = false;
+ } else {
+ rom_buffer = NULL;
+ read_size = fread(rom_header, 1, 4, fp);
+ if (read_size !=4)
+ pr_warn("%s: Fail to read rom_header\n", __func__);
+
+ if ((rom_header[0] != 0x55) || (rom_header[1] != 0xaa)) {
+ pr_warn("Incorrect Rom file format\n");
+ ptdev->need_rombar = false;
+ } else {
+ /* check file size */
+ fseek(fp, 0, SEEK_END);
+ file_size = ftell(fp);
+ fseek(fp, 0, SEEK_SET);
+ if (file_size > (2048 * 1024)) {
+ pr_warn("Rom file is too big.\n");
+ ptdev->need_rombar = false;
+ }
+ }
+ if (ptdev->need_rombar) {
+ rom_size = ALIGN_UP(file_size, 4096);
+ /* round up to a power of 2 */
+ if ((rom_size & (rom_size - 1)) != 0)
+ rom_size = 1UL << fls(rom_size);
+
+ if (posix_memalign((void **)&rom_buffer, 4096, rom_size)) {
+ ptdev->need_rombar = false;
+ pr_err("Fail to allocate buffer for rom_file\n");
+ } else {
+ ptdev->rom_buffer = rom_buffer;
+ read_size = fread(rom_buffer, 1, file_size, fp);
+ if (read_size != file_size) {
+ pr_warn("%s: read size is different with rom_size\n", __func__);
+ }
+ pci_emul_alloc_bar(dev, PCI_ROMBAR, PCIBAR_ROM, rom_size);
+
+ /* Setup the EPT mapping for ROM bar */
+ bar_addr = dev->bar[PCI_ROMBAR].addr;
+ if (vm_map_memseg_vma(ctx, rom_size, bar_addr,
+ (uint64_t)rom_buffer, PROT_READ)) {
+ pr_err("%s: Fail to map ROM bar\n", __func__);
+ free(rom_buffer);
+ ptdev->need_rombar = false;
+ } else {
+ /* Update the enable flag */
+ pci_set_cfgdata32(dev, PCIR_BIOS, (bar_addr | PCIM_BIOS_ENABLE));
+ }
+ }
+ }
+ fclose(fp);
+ }
+ }
+
if (is_intel_graphics_dev(dev)) {
if (is_rtvm) {
pr_err("%s RTVM doesn't support GVT-D.", __func__);
@@ -805,6 +879,26 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
vm_reset_ptdev_intx_info(ctx, virt_bdf, ptdev->phys_bdf, dev->lintr.ioapic_irq, false);
}

+ if (ptdev->need_rombar) {
+ int error;
+ struct acrn_vm_memmap rom_unmap;
+
+ rom_unmap.user_vm_pa = dev->bar[PCI_ROMBAR].addr;
+ rom_unmap.len = dev->bar[PCI_ROMBAR].size;
+ /* remove the EPT mapping for PCI_rombar.
+ * WA: Use ACRN_MEMMAP_MMIO to del the mapping
+ */
+ rom_unmap.type = ACRN_MEMMAP_MMIO;
+ error = ioctl(ctx->fd, ACRN_IOCTL_UNSET_MEMSEG, &rom_unmap);
+ if (error) {
+ pr_err("%s: unset_memseg ioctl() returned an error: %s\n",
+ __func__, errormsg(errno));
+ }
+ free(ptdev->rom_buffer);
+ ptdev->rom_buffer = NULL;
+ ptdev->need_rombar = false;
+ }
+
if (ptdev)
phys_bdf = ptdev->phys_bdf;

diff --git a/devicemodel/include/passthru.h b/devicemodel/include/passthru.h
index 7f693913e..655a17000 100644
--- a/devicemodel/include/passthru.h
+++ b/devicemodel/include/passthru.h
@@ -16,7 +16,7 @@

struct passthru_dev {
struct pci_vdev *dev;
- struct pcibar bar[PCI_BARMAX + 1];
+ struct pcibar bar[PCI_BARMAX + 2];
struct {
int capoff;
} msi;
@@ -36,6 +36,8 @@ struct passthru_dev {
*/
bool need_reset;
bool d3hot_reset;
+ bool need_rombar;
+ char *rom_buffer;
bool (*has_virt_pcicfg_regs)(int offset);
};

--
2.25.1


Yu Wang
 

On Mon, Sep 19, 2022 at 09:52:40AM +0800, Zhao Yakui wrote:
PCI ROM is the firmware specific to PCI device and it is provided by
the device vendor. The PCI rom resides in 0x30 offset of PCI config space.
This can be used to check whether the PCI rom exists. And when it exists,
it can load the firmware from the addr that is obtained from ROM bar addr.

For the user-vm, it will try to load the rom_file for the given PCI device and
enable the VM to access the firmware that is defined in rom_file.

BTW: The emulated rom_file is converted from efi image by using EfiRom. It has
no dependency on the ROM bar of physical PCI devices. Of course if the physical
PCI devices supports the ROM bar, the rom_file can also be dumped from the PCI
rom.

Now this is limited to PCI display device.

Signed-off-by: Zhao Yakui <yakui.zhao@...>
---
devicemodel/hw/pci/passthrough.c | 94 ++++++++++++++++++++++++++++++++
devicemodel/include/passthru.h | 4 +-
2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/passthrough.c b/devicemodel/hw/pci/passthrough.c
index 4c227aea9..9590d95bc 100644
--- a/devicemodel/hw/pci/passthrough.c
+++ b/devicemodel/hw/pci/passthrough.c
@@ -600,6 +600,13 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
struct acrn_pcidev pcidev = {};
uint16_t vendor = 0, device = 0;
uint8_t class = 0;
+ char *rom_buffer;
+ uint64_t bar_addr;
+ FILE *fp;
+ int file_size, rom_size;
+ uint8_t rom_header[4];
+ size_t read_size;
+ char rom_file[256];

ptdev = NULL;
error = -EINVAL;
@@ -615,6 +622,7 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
return -EINVAL;
}

+ memset(rom_file, 0, sizeof(rom_file));
while ((opt = strsep(&opts, ",")) != NULL) {
if (!strncmp(opt, "keep_gsi", 8))
keep_gsi = true;
@@ -711,6 +719,72 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
error = ACRN_PTDEV_IRQ_MSI;
}

+ if ((class != 3) || (dev->bus != 0)) {
+ if (ptdev->need_rombar) {
+ pr_warn("PCI rom is only supported on Bus 0 and GPU device\n");
PCI ROM only supports Intel iGPU.

+ ptdev->need_rombar = false;
+ }
+ }
+
+ if (ptdev->need_rombar) {
+
+ fp = fopen(rom_file, "rb");
+ if (fp == NULL) {
+ pr_warn("Fail to open %s rom_file\n", rom_file);
+ ptdev->need_rombar = false;
+ } else {
+ rom_buffer = NULL;
+ read_size = fread(rom_header, 1, 4, fp);
+ if (read_size !=4)
+ pr_warn("%s: Fail to read rom_header\n", __func__);
+
+ if ((rom_header[0] != 0x55) || (rom_header[1] != 0xaa)) {
+ pr_warn("Incorrect Rom file format\n");
+ ptdev->need_rombar = false;
+ } else {
+ /* check file size */
+ fseek(fp, 0, SEEK_END);
+ file_size = ftell(fp);
+ fseek(fp, 0, SEEK_SET);
+ if (file_size > (2048 * 1024)) {
+ pr_warn("Rom file is too big.\n");
+ ptdev->need_rombar = false;
+ }
+ }
Can we swap the logic to positive flow.

if ((rom_header[0] == 0x55) && (rom_header[1] == 0xaa)) {
...
} else {
pr_warn("Incorrect Rom file format\n");
ptdev->need_rombar = false;
}

+ if (ptdev->need_rombar) {
+ rom_size = ALIGN_UP(file_size, 4096);
+ /* round up to a power of 2 */
+ if ((rom_size & (rom_size - 1)) != 0)
+ rom_size = 1UL << fls(rom_size);
+
+ if (posix_memalign((void **)&rom_buffer, 4096, rom_size)) {
+ ptdev->need_rombar = false;
+ pr_err("Fail to allocate buffer for rom_file\n");
+ } else {
+ ptdev->rom_buffer = rom_buffer;
+ read_size = fread(rom_buffer, 1, file_size, fp);
+ if (read_size != file_size) {
+ pr_warn("%s: read size is different with rom_size\n", __func__);
+ }
+ pci_emul_alloc_bar(dev, PCI_ROMBAR, PCIBAR_ROM, rom_size);
+
+ /* Setup the EPT mapping for ROM bar */
+ bar_addr = dev->bar[PCI_ROMBAR].addr;
+ if (vm_map_memseg_vma(ctx, rom_size, bar_addr,
+ (uint64_t)rom_buffer, PROT_READ)) {
+ pr_err("%s: Fail to map ROM bar\n", __func__);
+ free(rom_buffer);
+ ptdev->need_rombar = false;
+ } else {
+ /* Update the enable flag */
+ pci_set_cfgdata32(dev, PCIR_BIOS, (bar_addr | PCIM_BIOS_ENABLE));
+ }
+ }
+ }
+ fclose(fp);
Please pack these code into a function, it can be named as create_rombar

+ }
+ }
+
if (is_intel_graphics_dev(dev)) {
if (is_rtvm) {
pr_err("%s RTVM doesn't support GVT-D.", __func__);
@@ -805,6 +879,26 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
vm_reset_ptdev_intx_info(ctx, virt_bdf, ptdev->phys_bdf, dev->lintr.ioapic_irq, false);
}

+ if (ptdev->need_rombar) {
+ int error;
+ struct acrn_vm_memmap rom_unmap;
+
+ rom_unmap.user_vm_pa = dev->bar[PCI_ROMBAR].addr;
+ rom_unmap.len = dev->bar[PCI_ROMBAR].size;
+ /* remove the EPT mapping for PCI_rombar.
+ * WA: Use ACRN_MEMMAP_MMIO to del the mapping
+ */
+ rom_unmap.type = ACRN_MEMMAP_MMIO;
+ error = ioctl(ctx->fd, ACRN_IOCTL_UNSET_MEMSEG, &rom_unmap);
+ if (error) {
+ pr_err("%s: unset_memseg ioctl() returned an error: %s\n",
+ __func__, errormsg(errno));
+ }
+ free(ptdev->rom_buffer);
+ ptdev->rom_buffer = NULL;
+ ptdev->need_rombar = false;
+ }
release_rombar

+
if (ptdev)
phys_bdf = ptdev->phys_bdf;

diff --git a/devicemodel/include/passthru.h b/devicemodel/include/passthru.h
index 7f693913e..655a17000 100644
--- a/devicemodel/include/passthru.h
+++ b/devicemodel/include/passthru.h
@@ -16,7 +16,7 @@

struct passthru_dev {
struct pci_vdev *dev;
- struct pcibar bar[PCI_BARMAX + 1];
+ struct pcibar bar[PCI_BARMAX + 2];
struct {
int capoff;
} msi;
@@ -36,6 +36,8 @@ struct passthru_dev {
*/
bool need_reset;
bool d3hot_reset;
+ bool need_rombar;
+ char *rom_buffer;
bool (*has_virt_pcicfg_regs)(int offset);
};

--
2.25.1


Zhao, Yakui
 

On 2022/9/19 15:21, Yu Wang wrote:
On Mon, Sep 19, 2022 at 09:52:40AM +0800, Zhao Yakui wrote:
PCI ROM is the firmware specific to PCI device and it is provided by
the device vendor. The PCI rom resides in 0x30 offset of PCI config space.
This can be used to check whether the PCI rom exists. And when it exists,
it can load the firmware from the addr that is obtained from ROM bar addr.

For the user-vm, it will try to load the rom_file for the given PCI device and
enable the VM to access the firmware that is defined in rom_file.

BTW: The emulated rom_file is converted from efi image by using EfiRom. It has
no dependency on the ROM bar of physical PCI devices. Of course if the physical
PCI devices supports the ROM bar, the rom_file can also be dumped from the PCI
rom.

Now this is limited to PCI display device.

Signed-off-by: Zhao Yakui <yakui.zhao@...>
---
devicemodel/hw/pci/passthrough.c | 94 ++++++++++++++++++++++++++++++++
devicemodel/include/passthru.h | 4 +-
2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/passthrough.c b/devicemodel/hw/pci/passthrough.c
index 4c227aea9..9590d95bc 100644
--- a/devicemodel/hw/pci/passthrough.c
+++ b/devicemodel/hw/pci/passthrough.c
@@ -600,6 +600,13 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
struct acrn_pcidev pcidev = {};
uint16_t vendor = 0, device = 0;
uint8_t class = 0;
+ char *rom_buffer;
+ uint64_t bar_addr;
+ FILE *fp;
+ int file_size, rom_size;
+ uint8_t rom_header[4];
+ size_t read_size;
+ char rom_file[256];
ptdev = NULL;
error = -EINVAL;
@@ -615,6 +622,7 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
return -EINVAL;
}
+ memset(rom_file, 0, sizeof(rom_file));
while ((opt = strsep(&opts, ",")) != NULL) {
if (!strncmp(opt, "keep_gsi", 8))
keep_gsi = true;
@@ -711,6 +719,72 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
error = ACRN_PTDEV_IRQ_MSI;
}
+ if ((class != 3) || (dev->bus != 0)) {
+ if (ptdev->need_rombar) {
+ pr_warn("PCI rom is only supported on Bus 0 and GPU device\n");
PCI ROM only supports Intel iGPU.
The ROM bar is not restricted to iGPU.
Instead it is fine to add the ROM bar for discrete GPU.


+ ptdev->need_rombar = false;
+ }
+ }
+
+ if (ptdev->need_rombar) {
+
+ fp = fopen(rom_file, "rb");
+ if (fp == NULL) {
+ pr_warn("Fail to open %s rom_file\n", rom_file);
+ ptdev->need_rombar = false;
+ } else {
+ rom_buffer = NULL;
+ read_size = fread(rom_header, 1, 4, fp);
+ if (read_size !=4)
+ pr_warn("%s: Fail to read rom_header\n", __func__);
+
+ if ((rom_header[0] != 0x55) || (rom_header[1] != 0xaa)) {
+ pr_warn("Incorrect Rom file format\n");
+ ptdev->need_rombar = false;
+ } else {
+ /* check file size */
+ fseek(fp, 0, SEEK_END);
+ file_size = ftell(fp);
+ fseek(fp, 0, SEEK_SET);
+ if (file_size > (2048 * 1024)) {
+ pr_warn("Rom file is too big.\n");
+ ptdev->need_rombar = false;
+ }
+ }
Can we swap the logic to positive flow.
if ((rom_header[0] == 0x55) && (rom_header[1] == 0xaa)) {
...
} else {
pr_warn("Incorrect Rom file format\n");
ptdev->need_rombar = false;
}
It is also ok to use the above logic.
It will be updated in next version.

+ if (ptdev->need_rombar) {
+ rom_size = ALIGN_UP(file_size, 4096);
+ /* round up to a power of 2 */
+ if ((rom_size & (rom_size - 1)) != 0)
+ rom_size = 1UL << fls(rom_size);
+
+ if (posix_memalign((void **)&rom_buffer, 4096, rom_size)) {
+ ptdev->need_rombar = false;
+ pr_err("Fail to allocate buffer for rom_file\n");
+ } else {
+ ptdev->rom_buffer = rom_buffer;
+ read_size = fread(rom_buffer, 1, file_size, fp);
+ if (read_size != file_size) {
+ pr_warn("%s: read size is different with rom_size\n", __func__);
+ }
+ pci_emul_alloc_bar(dev, PCI_ROMBAR, PCIBAR_ROM, rom_size);
+
+ /* Setup the EPT mapping for ROM bar */
+ bar_addr = dev->bar[PCI_ROMBAR].addr;
+ if (vm_map_memseg_vma(ctx, rom_size, bar_addr,
+ (uint64_t)rom_buffer, PROT_READ)) {
+ pr_err("%s: Fail to map ROM bar\n", __func__);
+ free(rom_buffer);
+ ptdev->need_rombar = false;
+ } else {
+ /* Update the enable flag */
+ pci_set_cfgdata32(dev, PCIR_BIOS, (bar_addr | PCIM_BIOS_ENABLE));
+ }
+ }
+ }
+ fclose(fp);
Please pack these code into a function, it can be named as create_rombar
OK. The above code will be put into one function in next version.


+ }
+ }
+
if (is_intel_graphics_dev(dev)) {
if (is_rtvm) {
pr_err("%s RTVM doesn't support GVT-D.", __func__);
@@ -805,6 +879,26 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
vm_reset_ptdev_intx_info(ctx, virt_bdf, ptdev->phys_bdf, dev->lintr.ioapic_irq, false);
}
+ if (ptdev->need_rombar) {
+ int error;
+ struct acrn_vm_memmap rom_unmap;
+
+ rom_unmap.user_vm_pa = dev->bar[PCI_ROMBAR].addr;
+ rom_unmap.len = dev->bar[PCI_ROMBAR].size;
+ /* remove the EPT mapping for PCI_rombar.
+ * WA: Use ACRN_MEMMAP_MMIO to del the mapping
+ */
+ rom_unmap.type = ACRN_MEMMAP_MMIO;
+ error = ioctl(ctx->fd, ACRN_IOCTL_UNSET_MEMSEG, &rom_unmap);
+ if (error) {
+ pr_err("%s: unset_memseg ioctl() returned an error: %s\n",
+ __func__, errormsg(errno));
+ }
+ free(ptdev->rom_buffer);
+ ptdev->rom_buffer = NULL;
+ ptdev->need_rombar = false;
+ }
release_rombar

+
if (ptdev)
phys_bdf = ptdev->phys_bdf;
diff --git a/devicemodel/include/passthru.h b/devicemodel/include/passthru.h
index 7f693913e..655a17000 100644
--- a/devicemodel/include/passthru.h
+++ b/devicemodel/include/passthru.h
@@ -16,7 +16,7 @@
struct passthru_dev {
struct pci_vdev *dev;
- struct pcibar bar[PCI_BARMAX + 1];
+ struct pcibar bar[PCI_BARMAX + 2];
struct {
int capoff;
} msi;
@@ -36,6 +36,8 @@ struct passthru_dev {
*/
bool need_reset;
bool d3hot_reset;
+ bool need_rombar;
+ char *rom_buffer;
bool (*has_virt_pcicfg_regs)(int offset);
};
--
2.25.1


Yu Wang
 

On Mon, Sep 19, 2022 at 03:54:48PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:21, Yu Wang wrote:
On Mon, Sep 19, 2022 at 09:52:40AM +0800, Zhao Yakui wrote:
PCI ROM is the firmware specific to PCI device and it is provided by
the device vendor. The PCI rom resides in 0x30 offset of PCI config space.
This can be used to check whether the PCI rom exists. And when it exists,
it can load the firmware from the addr that is obtained from ROM bar addr.

For the user-vm, it will try to load the rom_file for the given PCI device and
enable the VM to access the firmware that is defined in rom_file.

BTW: The emulated rom_file is converted from efi image by using EfiRom. It has
no dependency on the ROM bar of physical PCI devices. Of course if the physical
PCI devices supports the ROM bar, the rom_file can also be dumped from the PCI
rom.

Now this is limited to PCI display device.

Signed-off-by: Zhao Yakui <yakui.zhao@...>
---
devicemodel/hw/pci/passthrough.c | 94 ++++++++++++++++++++++++++++++++
devicemodel/include/passthru.h | 4 +-
2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/passthrough.c b/devicemodel/hw/pci/passthrough.c
index 4c227aea9..9590d95bc 100644
--- a/devicemodel/hw/pci/passthrough.c
+++ b/devicemodel/hw/pci/passthrough.c
@@ -600,6 +600,13 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
struct acrn_pcidev pcidev = {};
uint16_t vendor = 0, device = 0;
uint8_t class = 0;
+ char *rom_buffer;
+ uint64_t bar_addr;
+ FILE *fp;
+ int file_size, rom_size;
+ uint8_t rom_header[4];
+ size_t read_size;
+ char rom_file[256];
ptdev = NULL;
error = -EINVAL;
@@ -615,6 +622,7 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
return -EINVAL;
}
+ memset(rom_file, 0, sizeof(rom_file));
while ((opt = strsep(&opts, ",")) != NULL) {
if (!strncmp(opt, "keep_gsi", 8))
keep_gsi = true;
@@ -711,6 +719,72 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
error = ACRN_PTDEV_IRQ_MSI;
}
+ if ((class != 3) || (dev->bus != 0)) {
+ if (ptdev->need_rombar) {
+ pr_warn("PCI rom is only supported on Bus 0 and GPU device\n");
PCI ROM only supports Intel iGPU.
The ROM bar is not restricted to iGPU.
Instead it is fine to add the ROM bar for discrete GPU.
The GPU on Bus0 is the iGPU only, right?..

And usually the discrete GPU has the physical ROMBAR, then it should be
done by ROMBAR passthrough instead of ROMBAR virtualization?..




+ ptdev->need_rombar = false;
+ }
+ }
+
+ if (ptdev->need_rombar) {
+
+ fp = fopen(rom_file, "rb");
+ if (fp == NULL) {
+ pr_warn("Fail to open %s rom_file\n", rom_file);
+ ptdev->need_rombar = false;
+ } else {
+ rom_buffer = NULL;
+ read_size = fread(rom_header, 1, 4, fp);
+ if (read_size !=4)
+ pr_warn("%s: Fail to read rom_header\n", __func__);
+
+ if ((rom_header[0] != 0x55) || (rom_header[1] != 0xaa)) {
+ pr_warn("Incorrect Rom file format\n");
+ ptdev->need_rombar = false;
+ } else {
+ /* check file size */
+ fseek(fp, 0, SEEK_END);
+ file_size = ftell(fp);
+ fseek(fp, 0, SEEK_SET);
+ if (file_size > (2048 * 1024)) {
+ pr_warn("Rom file is too big.\n");
+ ptdev->need_rombar = false;
+ }
+ }
Can we swap the logic to positive flow.

if ((rom_header[0] == 0x55) && (rom_header[1] == 0xaa)) {
...
} else {
pr_warn("Incorrect Rom file format\n");
ptdev->need_rombar = false;
}
It is also ok to use the above logic.
It will be updated in next version.

+ if (ptdev->need_rombar) {
+ rom_size = ALIGN_UP(file_size, 4096);
+ /* round up to a power of 2 */
+ if ((rom_size & (rom_size - 1)) != 0)
+ rom_size = 1UL << fls(rom_size);
+
+ if (posix_memalign((void **)&rom_buffer, 4096, rom_size)) {
+ ptdev->need_rombar = false;
+ pr_err("Fail to allocate buffer for rom_file\n");
+ } else {
+ ptdev->rom_buffer = rom_buffer;
+ read_size = fread(rom_buffer, 1, file_size, fp);
+ if (read_size != file_size) {
+ pr_warn("%s: read size is different with rom_size\n", __func__);
+ }
+ pci_emul_alloc_bar(dev, PCI_ROMBAR, PCIBAR_ROM, rom_size);
+
+ /* Setup the EPT mapping for ROM bar */
+ bar_addr = dev->bar[PCI_ROMBAR].addr;
+ if (vm_map_memseg_vma(ctx, rom_size, bar_addr,
+ (uint64_t)rom_buffer, PROT_READ)) {
+ pr_err("%s: Fail to map ROM bar\n", __func__);
+ free(rom_buffer);
+ ptdev->need_rombar = false;
+ } else {
+ /* Update the enable flag */
+ pci_set_cfgdata32(dev, PCIR_BIOS, (bar_addr | PCIM_BIOS_ENABLE));
+ }
+ }
+ }
+ fclose(fp);
Please pack these code into a function, it can be named as create_rombar
OK. The above code will be put into one function in next version.


+ }
+ }
+
if (is_intel_graphics_dev(dev)) {
if (is_rtvm) {
pr_err("%s RTVM doesn't support GVT-D.", __func__);
@@ -805,6 +879,26 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
vm_reset_ptdev_intx_info(ctx, virt_bdf, ptdev->phys_bdf, dev->lintr.ioapic_irq, false);
}
+ if (ptdev->need_rombar) {
+ int error;
+ struct acrn_vm_memmap rom_unmap;
+
+ rom_unmap.user_vm_pa = dev->bar[PCI_ROMBAR].addr;
+ rom_unmap.len = dev->bar[PCI_ROMBAR].size;
+ /* remove the EPT mapping for PCI_rombar.
+ * WA: Use ACRN_MEMMAP_MMIO to del the mapping
+ */
+ rom_unmap.type = ACRN_MEMMAP_MMIO;
+ error = ioctl(ctx->fd, ACRN_IOCTL_UNSET_MEMSEG, &rom_unmap);
+ if (error) {
+ pr_err("%s: unset_memseg ioctl() returned an error: %s\n",
+ __func__, errormsg(errno));
+ }
+ free(ptdev->rom_buffer);
+ ptdev->rom_buffer = NULL;
+ ptdev->need_rombar = false;
+ }
release_rombar

+
if (ptdev)
phys_bdf = ptdev->phys_bdf;
diff --git a/devicemodel/include/passthru.h b/devicemodel/include/passthru.h
index 7f693913e..655a17000 100644
--- a/devicemodel/include/passthru.h
+++ b/devicemodel/include/passthru.h
@@ -16,7 +16,7 @@
struct passthru_dev {
struct pci_vdev *dev;
- struct pcibar bar[PCI_BARMAX + 1];
+ struct pcibar bar[PCI_BARMAX + 2];
struct {
int capoff;
} msi;
@@ -36,6 +36,8 @@ struct passthru_dev {
*/
bool need_reset;
bool d3hot_reset;
+ bool need_rombar;
+ char *rom_buffer;
bool (*has_virt_pcicfg_regs)(int offset);
};
--
2.25.1


Zhao, Yakui
 

On 2022/9/19 15:41, Yu Wang wrote:
On Mon, Sep 19, 2022 at 03:54:48PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:21, Yu Wang wrote:
On Mon, Sep 19, 2022 at 09:52:40AM +0800, Zhao Yakui wrote:
PCI ROM is the firmware specific to PCI device and it is provided by
the device vendor. The PCI rom resides in 0x30 offset of PCI config space.
This can be used to check whether the PCI rom exists. And when it exists,
it can load the firmware from the addr that is obtained from ROM bar addr.

For the user-vm, it will try to load the rom_file for the given PCI device and
enable the VM to access the firmware that is defined in rom_file.

BTW: The emulated rom_file is converted from efi image by using EfiRom. It has
no dependency on the ROM bar of physical PCI devices. Of course if the physical
PCI devices supports the ROM bar, the rom_file can also be dumped from the PCI
rom.

Now this is limited to PCI display device.

Signed-off-by: Zhao Yakui <yakui.zhao@...>
---
devicemodel/hw/pci/passthrough.c | 94 ++++++++++++++++++++++++++++++++
devicemodel/include/passthru.h | 4 +-
2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/passthrough.c b/devicemodel/hw/pci/passthrough.c
index 4c227aea9..9590d95bc 100644
--- a/devicemodel/hw/pci/passthrough.c
+++ b/devicemodel/hw/pci/passthrough.c
@@ -600,6 +600,13 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
struct acrn_pcidev pcidev = {};
uint16_t vendor = 0, device = 0;
uint8_t class = 0;
+ char *rom_buffer;
+ uint64_t bar_addr;
+ FILE *fp;
+ int file_size, rom_size;
+ uint8_t rom_header[4];
+ size_t read_size;
+ char rom_file[256];
ptdev = NULL;
error = -EINVAL;
@@ -615,6 +622,7 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
return -EINVAL;
}
+ memset(rom_file, 0, sizeof(rom_file));
while ((opt = strsep(&opts, ",")) != NULL) {
if (!strncmp(opt, "keep_gsi", 8))
keep_gsi = true;
@@ -711,6 +719,72 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
error = ACRN_PTDEV_IRQ_MSI;
}
+ if ((class != 3) || (dev->bus != 0)) {
+ if (ptdev->need_rombar) {
+ pr_warn("PCI rom is only supported on Bus 0 and GPU device\n");
PCI ROM only supports Intel iGPU.
The ROM bar is not restricted to iGPU.
Instead it is fine to add the ROM bar for discrete GPU.
The GPU on Bus0 is the iGPU only, right?..
This is the device for Guest VM. It doesn't mean that it should only be iGPU.

And usually the discrete GPU has the physical ROMBAR, then it should be
done by ROMBAR passthrough instead of ROMBAR virtualization?..
The discrete GPU maybe has the ROM bar. Some devices have no ROM bar. When it has the ROM bar, we don't need to virtualize it. Instead we can dump the ROM file from the pci_device or download it from device_vendor.
And it needs the specifc code to dump the rom_file.(It is convenient to dump it by using script or linux_cmd).

For the GPU device for guest_vm, we only need to provide the rom_file for the emulated PCI rombar.




+ ptdev->need_rombar = false;
+ }
+ }
+
+ if (ptdev->need_rombar) {
+
+ fp = fopen(rom_file, "rb");
+ if (fp == NULL) {
+ pr_warn("Fail to open %s rom_file\n", rom_file);
+ ptdev->need_rombar = false;
+ } else {
+ rom_buffer = NULL;
+ read_size = fread(rom_header, 1, 4, fp);
+ if (read_size !=4)
+ pr_warn("%s: Fail to read rom_header\n", __func__);
+
+ if ((rom_header[0] != 0x55) || (rom_header[1] != 0xaa)) {
+ pr_warn("Incorrect Rom file format\n");
+ ptdev->need_rombar = false;
+ } else {
+ /* check file size */
+ fseek(fp, 0, SEEK_END);
+ file_size = ftell(fp);
+ fseek(fp, 0, SEEK_SET);
+ if (file_size > (2048 * 1024)) {
+ pr_warn("Rom file is too big.\n");
+ ptdev->need_rombar = false;
+ }
+ }
Can we swap the logic to positive flow.

if ((rom_header[0] == 0x55) && (rom_header[1] == 0xaa)) {
...
} else {
pr_warn("Incorrect Rom file format\n");
ptdev->need_rombar = false;
}
It is also ok to use the above logic.
It will be updated in next version.

+ if (ptdev->need_rombar) {
+ rom_size = ALIGN_UP(file_size, 4096);
+ /* round up to a power of 2 */
+ if ((rom_size & (rom_size - 1)) != 0)
+ rom_size = 1UL << fls(rom_size);
+
+ if (posix_memalign((void **)&rom_buffer, 4096, rom_size)) {
+ ptdev->need_rombar = false;
+ pr_err("Fail to allocate buffer for rom_file\n");
+ } else {
+ ptdev->rom_buffer = rom_buffer;
+ read_size = fread(rom_buffer, 1, file_size, fp);
+ if (read_size != file_size) {
+ pr_warn("%s: read size is different with rom_size\n", __func__);
+ }
+ pci_emul_alloc_bar(dev, PCI_ROMBAR, PCIBAR_ROM, rom_size);
+
+ /* Setup the EPT mapping for ROM bar */
+ bar_addr = dev->bar[PCI_ROMBAR].addr;
+ if (vm_map_memseg_vma(ctx, rom_size, bar_addr,
+ (uint64_t)rom_buffer, PROT_READ)) {
+ pr_err("%s: Fail to map ROM bar\n", __func__);
+ free(rom_buffer);
+ ptdev->need_rombar = false;
+ } else {
+ /* Update the enable flag */
+ pci_set_cfgdata32(dev, PCIR_BIOS, (bar_addr | PCIM_BIOS_ENABLE));
+ }
+ }
+ }
+ fclose(fp);
Please pack these code into a function, it can be named as create_rombar
OK. The above code will be put into one function in next version.


+ }
+ }
+
if (is_intel_graphics_dev(dev)) {
if (is_rtvm) {
pr_err("%s RTVM doesn't support GVT-D.", __func__);
@@ -805,6 +879,26 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
vm_reset_ptdev_intx_info(ctx, virt_bdf, ptdev->phys_bdf, dev->lintr.ioapic_irq, false);
}
+ if (ptdev->need_rombar) {
+ int error;
+ struct acrn_vm_memmap rom_unmap;
+
+ rom_unmap.user_vm_pa = dev->bar[PCI_ROMBAR].addr;
+ rom_unmap.len = dev->bar[PCI_ROMBAR].size;
+ /* remove the EPT mapping for PCI_rombar.
+ * WA: Use ACRN_MEMMAP_MMIO to del the mapping
+ */
+ rom_unmap.type = ACRN_MEMMAP_MMIO;
+ error = ioctl(ctx->fd, ACRN_IOCTL_UNSET_MEMSEG, &rom_unmap);
+ if (error) {
+ pr_err("%s: unset_memseg ioctl() returned an error: %s\n",
+ __func__, errormsg(errno));
+ }
+ free(ptdev->rom_buffer);
+ ptdev->rom_buffer = NULL;
+ ptdev->need_rombar = false;
+ }
release_rombar

+
if (ptdev)
phys_bdf = ptdev->phys_bdf;
diff --git a/devicemodel/include/passthru.h b/devicemodel/include/passthru.h
index 7f693913e..655a17000 100644
--- a/devicemodel/include/passthru.h
+++ b/devicemodel/include/passthru.h
@@ -16,7 +16,7 @@
struct passthru_dev {
struct pci_vdev *dev;
- struct pcibar bar[PCI_BARMAX + 1];
+ struct pcibar bar[PCI_BARMAX + 2];
struct {
int capoff;
} msi;
@@ -36,6 +36,8 @@ struct passthru_dev {
*/
bool need_reset;
bool d3hot_reset;
+ bool need_rombar;
+ char *rom_buffer;
bool (*has_virt_pcicfg_regs)(int offset);
};
--
2.25.1


Yu Wang
 

On Mon, Sep 19, 2022 at 04:16:11PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:41, Yu Wang wrote:
On Mon, Sep 19, 2022 at 03:54:48PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:21, Yu Wang wrote:
On Mon, Sep 19, 2022 at 09:52:40AM +0800, Zhao Yakui wrote:
PCI ROM is the firmware specific to PCI device and it is provided by
the device vendor. The PCI rom resides in 0x30 offset of PCI config space.
This can be used to check whether the PCI rom exists. And when it exists,
it can load the firmware from the addr that is obtained from ROM bar addr.

For the user-vm, it will try to load the rom_file for the given PCI device and
enable the VM to access the firmware that is defined in rom_file.

BTW: The emulated rom_file is converted from efi image by using EfiRom. It has
no dependency on the ROM bar of physical PCI devices. Of course if the physical
PCI devices supports the ROM bar, the rom_file can also be dumped from the PCI
rom.

Now this is limited to PCI display device.

Signed-off-by: Zhao Yakui <yakui.zhao@...>
---
devicemodel/hw/pci/passthrough.c | 94 ++++++++++++++++++++++++++++++++
devicemodel/include/passthru.h | 4 +-
2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/passthrough.c b/devicemodel/hw/pci/passthrough.c
index 4c227aea9..9590d95bc 100644
--- a/devicemodel/hw/pci/passthrough.c
+++ b/devicemodel/hw/pci/passthrough.c
@@ -600,6 +600,13 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
struct acrn_pcidev pcidev = {};
uint16_t vendor = 0, device = 0;
uint8_t class = 0;
+ char *rom_buffer;
+ uint64_t bar_addr;
+ FILE *fp;
+ int file_size, rom_size;
+ uint8_t rom_header[4];
+ size_t read_size;
+ char rom_file[256];
ptdev = NULL;
error = -EINVAL;
@@ -615,6 +622,7 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
return -EINVAL;
}
+ memset(rom_file, 0, sizeof(rom_file));
while ((opt = strsep(&opts, ",")) != NULL) {
if (!strncmp(opt, "keep_gsi", 8))
keep_gsi = true;
@@ -711,6 +719,72 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
error = ACRN_PTDEV_IRQ_MSI;
}
+ if ((class != 3) || (dev->bus != 0)) {
+ if (ptdev->need_rombar) {
+ pr_warn("PCI rom is only supported on Bus 0 and GPU device\n");
PCI ROM only supports Intel iGPU.
The ROM bar is not restricted to iGPU.
Instead it is fine to add the ROM bar for discrete GPU.
The GPU on Bus0 is the iGPU only, right?..
This is the device for Guest VM. It doesn't mean that it should only be
iGPU.
Then this log doesn't make sense...
Why we need to mention the virtual BDF here? Checking the class is
enough?



And usually the discrete GPU has the physical ROMBAR, then it should be
done by ROMBAR passthrough instead of ROMBAR virtualization?..
The discrete GPU maybe has the ROM bar. Some devices have no ROM bar. When
it has the ROM bar, we don't need to virtualize it. Instead we can dump the
ROM file from the pci_device or download it from device_vendor.
And it needs the specifc code to dump the rom_file.(It is convenient to dump
it by using script or linux_cmd).

For the GPU device for guest_vm, we only need to provide the rom_file for
the emulated PCI rombar.
Ok. this part is OK to me.






+ ptdev->need_rombar = false;
+ }
+ }
+
+ if (ptdev->need_rombar) {
+
+ fp = fopen(rom_file, "rb");
+ if (fp == NULL) {
+ pr_warn("Fail to open %s rom_file\n", rom_file);
+ ptdev->need_rombar = false;
+ } else {
+ rom_buffer = NULL;
+ read_size = fread(rom_header, 1, 4, fp);
+ if (read_size !=4)
+ pr_warn("%s: Fail to read rom_header\n", __func__);
+
+ if ((rom_header[0] != 0x55) || (rom_header[1] != 0xaa)) {
+ pr_warn("Incorrect Rom file format\n");
+ ptdev->need_rombar = false;
+ } else {
+ /* check file size */
+ fseek(fp, 0, SEEK_END);
+ file_size = ftell(fp);
+ fseek(fp, 0, SEEK_SET);
+ if (file_size > (2048 * 1024)) {
+ pr_warn("Rom file is too big.\n");
+ ptdev->need_rombar = false;
+ }
+ }
Can we swap the logic to positive flow.

if ((rom_header[0] == 0x55) && (rom_header[1] == 0xaa)) {
...
} else {
pr_warn("Incorrect Rom file format\n");
ptdev->need_rombar = false;
}
It is also ok to use the above logic.
It will be updated in next version.

+ if (ptdev->need_rombar) {
+ rom_size = ALIGN_UP(file_size, 4096);
+ /* round up to a power of 2 */
+ if ((rom_size & (rom_size - 1)) != 0)
+ rom_size = 1UL << fls(rom_size);
+
+ if (posix_memalign((void **)&rom_buffer, 4096, rom_size)) {
+ ptdev->need_rombar = false;
+ pr_err("Fail to allocate buffer for rom_file\n");
+ } else {
+ ptdev->rom_buffer = rom_buffer;
+ read_size = fread(rom_buffer, 1, file_size, fp);
+ if (read_size != file_size) {
+ pr_warn("%s: read size is different with rom_size\n", __func__);
+ }
+ pci_emul_alloc_bar(dev, PCI_ROMBAR, PCIBAR_ROM, rom_size);
+
+ /* Setup the EPT mapping for ROM bar */
+ bar_addr = dev->bar[PCI_ROMBAR].addr;
+ if (vm_map_memseg_vma(ctx, rom_size, bar_addr,
+ (uint64_t)rom_buffer, PROT_READ)) {
+ pr_err("%s: Fail to map ROM bar\n", __func__);
+ free(rom_buffer);
+ ptdev->need_rombar = false;
+ } else {
+ /* Update the enable flag */
+ pci_set_cfgdata32(dev, PCIR_BIOS, (bar_addr | PCIM_BIOS_ENABLE));
+ }
+ }
+ }
+ fclose(fp);
Please pack these code into a function, it can be named as create_rombar
OK. The above code will be put into one function in next version.


+ }
+ }
+
if (is_intel_graphics_dev(dev)) {
if (is_rtvm) {
pr_err("%s RTVM doesn't support GVT-D.", __func__);
@@ -805,6 +879,26 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
vm_reset_ptdev_intx_info(ctx, virt_bdf, ptdev->phys_bdf, dev->lintr.ioapic_irq, false);
}
+ if (ptdev->need_rombar) {
+ int error;
+ struct acrn_vm_memmap rom_unmap;
+
+ rom_unmap.user_vm_pa = dev->bar[PCI_ROMBAR].addr;
+ rom_unmap.len = dev->bar[PCI_ROMBAR].size;
+ /* remove the EPT mapping for PCI_rombar.
+ * WA: Use ACRN_MEMMAP_MMIO to del the mapping
+ */
+ rom_unmap.type = ACRN_MEMMAP_MMIO;
+ error = ioctl(ctx->fd, ACRN_IOCTL_UNSET_MEMSEG, &rom_unmap);
+ if (error) {
+ pr_err("%s: unset_memseg ioctl() returned an error: %s\n",
+ __func__, errormsg(errno));
+ }
+ free(ptdev->rom_buffer);
+ ptdev->rom_buffer = NULL;
+ ptdev->need_rombar = false;
+ }
release_rombar

+
if (ptdev)
phys_bdf = ptdev->phys_bdf;
diff --git a/devicemodel/include/passthru.h b/devicemodel/include/passthru.h
index 7f693913e..655a17000 100644
--- a/devicemodel/include/passthru.h
+++ b/devicemodel/include/passthru.h
@@ -16,7 +16,7 @@
struct passthru_dev {
struct pci_vdev *dev;
- struct pcibar bar[PCI_BARMAX + 1];
+ struct pcibar bar[PCI_BARMAX + 2];
struct {
int capoff;
} msi;
@@ -36,6 +36,8 @@ struct passthru_dev {
*/
bool need_reset;
bool d3hot_reset;
+ bool need_rombar;
+ char *rom_buffer;
bool (*has_virt_pcicfg_regs)(int offset);
};
--
2.25.1


Zhao, Yakui
 

On 2022/9/19 15:59, Yu Wang wrote:
On Mon, Sep 19, 2022 at 04:16:11PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:41, Yu Wang wrote:
On Mon, Sep 19, 2022 at 03:54:48PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:21, Yu Wang wrote:
On Mon, Sep 19, 2022 at 09:52:40AM +0800, Zhao Yakui wrote:
PCI ROM is the firmware specific to PCI device and it is provided by
the device vendor. The PCI rom resides in 0x30 offset of PCI config space.
This can be used to check whether the PCI rom exists. And when it exists,
it can load the firmware from the addr that is obtained from ROM bar addr.

For the user-vm, it will try to load the rom_file for the given PCI device and
enable the VM to access the firmware that is defined in rom_file.

BTW: The emulated rom_file is converted from efi image by using EfiRom. It has
no dependency on the ROM bar of physical PCI devices. Of course if the physical
PCI devices supports the ROM bar, the rom_file can also be dumped from the PCI
rom.

Now this is limited to PCI display device.

Signed-off-by: Zhao Yakui <yakui.zhao@...>
---
devicemodel/hw/pci/passthrough.c | 94 ++++++++++++++++++++++++++++++++
devicemodel/include/passthru.h | 4 +-
2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/passthrough.c b/devicemodel/hw/pci/passthrough.c
index 4c227aea9..9590d95bc 100644
--- a/devicemodel/hw/pci/passthrough.c
+++ b/devicemodel/hw/pci/passthrough.c
@@ -600,6 +600,13 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
struct acrn_pcidev pcidev = {};
uint16_t vendor = 0, device = 0;
uint8_t class = 0;
+ char *rom_buffer;
+ uint64_t bar_addr;
+ FILE *fp;
+ int file_size, rom_size;
+ uint8_t rom_header[4];
+ size_t read_size;
+ char rom_file[256];
ptdev = NULL;
error = -EINVAL;
@@ -615,6 +622,7 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
return -EINVAL;
}
+ memset(rom_file, 0, sizeof(rom_file));
while ((opt = strsep(&opts, ",")) != NULL) {
if (!strncmp(opt, "keep_gsi", 8))
keep_gsi = true;
@@ -711,6 +719,72 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
error = ACRN_PTDEV_IRQ_MSI;
}
+ if ((class != 3) || (dev->bus != 0)) {
+ if (ptdev->need_rombar) {
+ pr_warn("PCI rom is only supported on Bus 0 and GPU device\n");
PCI ROM only supports Intel iGPU.
The ROM bar is not restricted to iGPU.
Instead it is fine to add the ROM bar for discrete GPU.
The GPU on Bus0 is the iGPU only, right?..
This is the device for Guest VM. It doesn't mean that it should only be
iGPU.
Then this log doesn't make sense...
Why we need to mention the virtual BDF here? Checking the class is
enough?
For the device with ROM on Bus1/Bus2, maybe it needs to add some specific code to handle the bridge_window related with ROM_bar region.
To make it simple, we add the limitation that the ROM bar is only supported for the PCI devices on Bus0.




And usually the discrete GPU has the physical ROMBAR, then it should be
done by ROMBAR passthrough instead of ROMBAR virtualization?..
The discrete GPU maybe has the ROM bar. Some devices have no ROM bar. When
it has the ROM bar, we don't need to virtualize it. Instead we can dump the
ROM file from the pci_device or download it from device_vendor.
And it needs the specifc code to dump the rom_file.(It is convenient to dump
it by using script or linux_cmd).

For the GPU device for guest_vm, we only need to provide the rom_file for
the emulated PCI rombar.
Ok. this part is OK to me.






+ ptdev->need_rombar = false;
+ }
+ }
+
+ if (ptdev->need_rombar) {
+
+ fp = fopen(rom_file, "rb");
+ if (fp == NULL) {
+ pr_warn("Fail to open %s rom_file\n", rom_file);
+ ptdev->need_rombar = false;
+ } else {
+ rom_buffer = NULL;
+ read_size = fread(rom_header, 1, 4, fp);
+ if (read_size !=4)
+ pr_warn("%s: Fail to read rom_header\n", __func__);
+
+ if ((rom_header[0] != 0x55) || (rom_header[1] != 0xaa)) {
+ pr_warn("Incorrect Rom file format\n");
+ ptdev->need_rombar = false;
+ } else {
+ /* check file size */
+ fseek(fp, 0, SEEK_END);
+ file_size = ftell(fp);
+ fseek(fp, 0, SEEK_SET);
+ if (file_size > (2048 * 1024)) {
+ pr_warn("Rom file is too big.\n");
+ ptdev->need_rombar = false;
+ }
+ }
Can we swap the logic to positive flow.

if ((rom_header[0] == 0x55) && (rom_header[1] == 0xaa)) {
...
} else {
pr_warn("Incorrect Rom file format\n");
ptdev->need_rombar = false;
}
It is also ok to use the above logic.
It will be updated in next version.

+ if (ptdev->need_rombar) {
+ rom_size = ALIGN_UP(file_size, 4096);
+ /* round up to a power of 2 */
+ if ((rom_size & (rom_size - 1)) != 0)
+ rom_size = 1UL << fls(rom_size);
+
+ if (posix_memalign((void **)&rom_buffer, 4096, rom_size)) {
+ ptdev->need_rombar = false;
+ pr_err("Fail to allocate buffer for rom_file\n");
+ } else {
+ ptdev->rom_buffer = rom_buffer;
+ read_size = fread(rom_buffer, 1, file_size, fp);
+ if (read_size != file_size) {
+ pr_warn("%s: read size is different with rom_size\n", __func__);
+ }
+ pci_emul_alloc_bar(dev, PCI_ROMBAR, PCIBAR_ROM, rom_size);
+
+ /* Setup the EPT mapping for ROM bar */
+ bar_addr = dev->bar[PCI_ROMBAR].addr;
+ if (vm_map_memseg_vma(ctx, rom_size, bar_addr,
+ (uint64_t)rom_buffer, PROT_READ)) {
+ pr_err("%s: Fail to map ROM bar\n", __func__);
+ free(rom_buffer);
+ ptdev->need_rombar = false;
+ } else {
+ /* Update the enable flag */
+ pci_set_cfgdata32(dev, PCIR_BIOS, (bar_addr | PCIM_BIOS_ENABLE));
+ }
+ }
+ }
+ fclose(fp);
Please pack these code into a function, it can be named as create_rombar
OK. The above code will be put into one function in next version.


+ }
+ }
+
if (is_intel_graphics_dev(dev)) {
if (is_rtvm) {
pr_err("%s RTVM doesn't support GVT-D.", __func__);
@@ -805,6 +879,26 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
vm_reset_ptdev_intx_info(ctx, virt_bdf, ptdev->phys_bdf, dev->lintr.ioapic_irq, false);
}
+ if (ptdev->need_rombar) {
+ int error;
+ struct acrn_vm_memmap rom_unmap;
+
+ rom_unmap.user_vm_pa = dev->bar[PCI_ROMBAR].addr;
+ rom_unmap.len = dev->bar[PCI_ROMBAR].size;
+ /* remove the EPT mapping for PCI_rombar.
+ * WA: Use ACRN_MEMMAP_MMIO to del the mapping
+ */
+ rom_unmap.type = ACRN_MEMMAP_MMIO;
+ error = ioctl(ctx->fd, ACRN_IOCTL_UNSET_MEMSEG, &rom_unmap);
+ if (error) {
+ pr_err("%s: unset_memseg ioctl() returned an error: %s\n",
+ __func__, errormsg(errno));
+ }
+ free(ptdev->rom_buffer);
+ ptdev->rom_buffer = NULL;
+ ptdev->need_rombar = false;
+ }
release_rombar

+
if (ptdev)
phys_bdf = ptdev->phys_bdf;
diff --git a/devicemodel/include/passthru.h b/devicemodel/include/passthru.h
index 7f693913e..655a17000 100644
--- a/devicemodel/include/passthru.h
+++ b/devicemodel/include/passthru.h
@@ -16,7 +16,7 @@
struct passthru_dev {
struct pci_vdev *dev;
- struct pcibar bar[PCI_BARMAX + 1];
+ struct pcibar bar[PCI_BARMAX + 2];
struct {
int capoff;
} msi;
@@ -36,6 +36,8 @@ struct passthru_dev {
*/
bool need_reset;
bool d3hot_reset;
+ bool need_rombar;
+ char *rom_buffer;
bool (*has_virt_pcicfg_regs)(int offset);
};
--
2.25.1


Yu Wang
 

On Mon, Sep 19, 2022 at 04:32:55PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:59, Yu Wang wrote:
On Mon, Sep 19, 2022 at 04:16:11PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:41, Yu Wang wrote:
On Mon, Sep 19, 2022 at 03:54:48PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:21, Yu Wang wrote:
On Mon, Sep 19, 2022 at 09:52:40AM +0800, Zhao Yakui wrote:
PCI ROM is the firmware specific to PCI device and it is provided by
the device vendor. The PCI rom resides in 0x30 offset of PCI config space.
This can be used to check whether the PCI rom exists. And when it exists,
it can load the firmware from the addr that is obtained from ROM bar addr.

For the user-vm, it will try to load the rom_file for the given PCI device and
enable the VM to access the firmware that is defined in rom_file.

BTW: The emulated rom_file is converted from efi image by using EfiRom. It has
no dependency on the ROM bar of physical PCI devices. Of course if the physical
PCI devices supports the ROM bar, the rom_file can also be dumped from the PCI
rom.

Now this is limited to PCI display device.

Signed-off-by: Zhao Yakui <yakui.zhao@...>
---
devicemodel/hw/pci/passthrough.c | 94 ++++++++++++++++++++++++++++++++
devicemodel/include/passthru.h | 4 +-
2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/passthrough.c b/devicemodel/hw/pci/passthrough.c
index 4c227aea9..9590d95bc 100644
--- a/devicemodel/hw/pci/passthrough.c
+++ b/devicemodel/hw/pci/passthrough.c
@@ -600,6 +600,13 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
struct acrn_pcidev pcidev = {};
uint16_t vendor = 0, device = 0;
uint8_t class = 0;
+ char *rom_buffer;
+ uint64_t bar_addr;
+ FILE *fp;
+ int file_size, rom_size;
+ uint8_t rom_header[4];
+ size_t read_size;
+ char rom_file[256];
ptdev = NULL;
error = -EINVAL;
@@ -615,6 +622,7 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
return -EINVAL;
}
+ memset(rom_file, 0, sizeof(rom_file));
while ((opt = strsep(&opts, ",")) != NULL) {
if (!strncmp(opt, "keep_gsi", 8))
keep_gsi = true;
@@ -711,6 +719,72 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
error = ACRN_PTDEV_IRQ_MSI;
}
+ if ((class != 3) || (dev->bus != 0)) {
+ if (ptdev->need_rombar) {
+ pr_warn("PCI rom is only supported on Bus 0 and GPU device\n");
PCI ROM only supports Intel iGPU.
The ROM bar is not restricted to iGPU.
Instead it is fine to add the ROM bar for discrete GPU.
The GPU on Bus0 is the iGPU only, right?..
This is the device for Guest VM. It doesn't mean that it should only be
iGPU.
Then this log doesn't make sense...
Why we need to mention the virtual BDF here? Checking the class is
enough?
For the device with ROM on Bus1/Bus2, maybe it needs to add some specific
code to handle the bridge_window related with ROM_bar region.
Do we have bus1/bus2 for virtual pci device? I think that we emulate all
devices as RCiEPs?

To make it simple, we add the limitation that the ROM bar is only supported
for the PCI devices on Bus0.




And usually the discrete GPU has the physical ROMBAR, then it should be
done by ROMBAR passthrough instead of ROMBAR virtualization?..
The discrete GPU maybe has the ROM bar. Some devices have no ROM bar. When
it has the ROM bar, we don't need to virtualize it. Instead we can dump the
ROM file from the pci_device or download it from device_vendor.
And it needs the specifc code to dump the rom_file.(It is convenient to dump
it by using script or linux_cmd).

For the GPU device for guest_vm, we only need to provide the rom_file for
the emulated PCI rombar.
Ok. this part is OK to me.






+ ptdev->need_rombar = false;
+ }
+ }
+
+ if (ptdev->need_rombar) {
+
+ fp = fopen(rom_file, "rb");
+ if (fp == NULL) {
+ pr_warn("Fail to open %s rom_file\n", rom_file);
+ ptdev->need_rombar = false;
+ } else {
+ rom_buffer = NULL;
+ read_size = fread(rom_header, 1, 4, fp);
+ if (read_size !=4)
+ pr_warn("%s: Fail to read rom_header\n", __func__);
+
+ if ((rom_header[0] != 0x55) || (rom_header[1] != 0xaa)) {
+ pr_warn("Incorrect Rom file format\n");
+ ptdev->need_rombar = false;
+ } else {
+ /* check file size */
+ fseek(fp, 0, SEEK_END);
+ file_size = ftell(fp);
+ fseek(fp, 0, SEEK_SET);
+ if (file_size > (2048 * 1024)) {
+ pr_warn("Rom file is too big.\n");
+ ptdev->need_rombar = false;
+ }
+ }
Can we swap the logic to positive flow.

if ((rom_header[0] == 0x55) && (rom_header[1] == 0xaa)) {
...
} else {
pr_warn("Incorrect Rom file format\n");
ptdev->need_rombar = false;
}
It is also ok to use the above logic.
It will be updated in next version.

+ if (ptdev->need_rombar) {
+ rom_size = ALIGN_UP(file_size, 4096);
+ /* round up to a power of 2 */
+ if ((rom_size & (rom_size - 1)) != 0)
+ rom_size = 1UL << fls(rom_size);
+
+ if (posix_memalign((void **)&rom_buffer, 4096, rom_size)) {
+ ptdev->need_rombar = false;
+ pr_err("Fail to allocate buffer for rom_file\n");
+ } else {
+ ptdev->rom_buffer = rom_buffer;
+ read_size = fread(rom_buffer, 1, file_size, fp);
+ if (read_size != file_size) {
+ pr_warn("%s: read size is different with rom_size\n", __func__);
+ }
+ pci_emul_alloc_bar(dev, PCI_ROMBAR, PCIBAR_ROM, rom_size);
+
+ /* Setup the EPT mapping for ROM bar */
+ bar_addr = dev->bar[PCI_ROMBAR].addr;
+ if (vm_map_memseg_vma(ctx, rom_size, bar_addr,
+ (uint64_t)rom_buffer, PROT_READ)) {
+ pr_err("%s: Fail to map ROM bar\n", __func__);
+ free(rom_buffer);
+ ptdev->need_rombar = false;
+ } else {
+ /* Update the enable flag */
+ pci_set_cfgdata32(dev, PCIR_BIOS, (bar_addr | PCIM_BIOS_ENABLE));
+ }
+ }
+ }
+ fclose(fp);
Please pack these code into a function, it can be named as create_rombar
OK. The above code will be put into one function in next version.


+ }
+ }
+
if (is_intel_graphics_dev(dev)) {
if (is_rtvm) {
pr_err("%s RTVM doesn't support GVT-D.", __func__);
@@ -805,6 +879,26 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
vm_reset_ptdev_intx_info(ctx, virt_bdf, ptdev->phys_bdf, dev->lintr.ioapic_irq, false);
}
+ if (ptdev->need_rombar) {
+ int error;
+ struct acrn_vm_memmap rom_unmap;
+
+ rom_unmap.user_vm_pa = dev->bar[PCI_ROMBAR].addr;
+ rom_unmap.len = dev->bar[PCI_ROMBAR].size;
+ /* remove the EPT mapping for PCI_rombar.
+ * WA: Use ACRN_MEMMAP_MMIO to del the mapping
+ */
+ rom_unmap.type = ACRN_MEMMAP_MMIO;
+ error = ioctl(ctx->fd, ACRN_IOCTL_UNSET_MEMSEG, &rom_unmap);
+ if (error) {
+ pr_err("%s: unset_memseg ioctl() returned an error: %s\n",
+ __func__, errormsg(errno));
+ }
+ free(ptdev->rom_buffer);
+ ptdev->rom_buffer = NULL;
+ ptdev->need_rombar = false;
+ }
release_rombar

+
if (ptdev)
phys_bdf = ptdev->phys_bdf;
diff --git a/devicemodel/include/passthru.h b/devicemodel/include/passthru.h
index 7f693913e..655a17000 100644
--- a/devicemodel/include/passthru.h
+++ b/devicemodel/include/passthru.h
@@ -16,7 +16,7 @@
struct passthru_dev {
struct pci_vdev *dev;
- struct pcibar bar[PCI_BARMAX + 1];
+ struct pcibar bar[PCI_BARMAX + 2];
struct {
int capoff;
} msi;
@@ -36,6 +36,8 @@ struct passthru_dev {
*/
bool need_reset;
bool d3hot_reset;
+ bool need_rombar;
+ char *rom_buffer;
bool (*has_virt_pcicfg_regs)(int offset);
};
--
2.25.1


Zhao, Yakui
 

On 2022/9/19 16:10, Yu Wang wrote:
On Mon, Sep 19, 2022 at 04:32:55PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:59, Yu Wang wrote:
On Mon, Sep 19, 2022 at 04:16:11PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:41, Yu Wang wrote:
On Mon, Sep 19, 2022 at 03:54:48PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:21, Yu Wang wrote:
On Mon, Sep 19, 2022 at 09:52:40AM +0800, Zhao Yakui wrote:
PCI ROM is the firmware specific to PCI device and it is provided by
the device vendor. The PCI rom resides in 0x30 offset of PCI config space.
This can be used to check whether the PCI rom exists. And when it exists,
it can load the firmware from the addr that is obtained from ROM bar addr.

For the user-vm, it will try to load the rom_file for the given PCI device and
enable the VM to access the firmware that is defined in rom_file.

BTW: The emulated rom_file is converted from efi image by using EfiRom. It has
no dependency on the ROM bar of physical PCI devices. Of course if the physical
PCI devices supports the ROM bar, the rom_file can also be dumped from the PCI
rom.

Now this is limited to PCI display device.

Signed-off-by: Zhao Yakui <yakui.zhao@...>
---
devicemodel/hw/pci/passthrough.c | 94 ++++++++++++++++++++++++++++++++
devicemodel/include/passthru.h | 4 +-
2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/passthrough.c b/devicemodel/hw/pci/passthrough.c
index 4c227aea9..9590d95bc 100644
--- a/devicemodel/hw/pci/passthrough.c
+++ b/devicemodel/hw/pci/passthrough.c
@@ -600,6 +600,13 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
struct acrn_pcidev pcidev = {};
uint16_t vendor = 0, device = 0;
uint8_t class = 0;
+ char *rom_buffer;
+ uint64_t bar_addr;
+ FILE *fp;
+ int file_size, rom_size;
+ uint8_t rom_header[4];
+ size_t read_size;
+ char rom_file[256];
ptdev = NULL;
error = -EINVAL;
@@ -615,6 +622,7 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
return -EINVAL;
}
+ memset(rom_file, 0, sizeof(rom_file));
while ((opt = strsep(&opts, ",")) != NULL) {
if (!strncmp(opt, "keep_gsi", 8))
keep_gsi = true;
@@ -711,6 +719,72 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
error = ACRN_PTDEV_IRQ_MSI;
}
+ if ((class != 3) || (dev->bus != 0)) {
+ if (ptdev->need_rombar) {
+ pr_warn("PCI rom is only supported on Bus 0 and GPU device\n");
PCI ROM only supports Intel iGPU.
The ROM bar is not restricted to iGPU.
Instead it is fine to add the ROM bar for discrete GPU.
The GPU on Bus0 is the iGPU only, right?..
This is the device for Guest VM. It doesn't mean that it should only be
iGPU.
Then this log doesn't make sense...
Why we need to mention the virtual BDF here? Checking the class is
enough?
For the device with ROM on Bus1/Bus2, maybe it needs to add some specific
code to handle the bridge_window related with ROM_bar region.
Do we have bus1/bus2 for virtual pci device? I think that we emulate all
devices as RCiEPs?
Currently the pci devices defined in launch_script mainly exist on the Bus0.
But based on the code of parse_bdf, it seems that it is possible to define the devices on Bus1/Bus2.

If we don't need to consider this scenario, I can remove the check of dev->bus.



To make it simple, we add the limitation that the ROM bar is only supported
for the PCI devices on Bus0.




And usually the discrete GPU has the physical ROMBAR, then it should be
done by ROMBAR passthrough instead of ROMBAR virtualization?..
The discrete GPU maybe has the ROM bar. Some devices have no ROM bar. When
it has the ROM bar, we don't need to virtualize it. Instead we can dump the
ROM file from the pci_device or download it from device_vendor.
And it needs the specifc code to dump the rom_file.(It is convenient to dump
it by using script or linux_cmd).

For the GPU device for guest_vm, we only need to provide the rom_file for
the emulated PCI rombar.
Ok. this part is OK to me.






+ ptdev->need_rombar = false;
+ }
+ }
+
+ if (ptdev->need_rombar) {
+
+ fp = fopen(rom_file, "rb");
+ if (fp == NULL) {
+ pr_warn("Fail to open %s rom_file\n", rom_file);
+ ptdev->need_rombar = false;
+ } else {
+ rom_buffer = NULL;
+ read_size = fread(rom_header, 1, 4, fp);
+ if (read_size !=4)
+ pr_warn("%s: Fail to read rom_header\n", __func__);
+
+ if ((rom_header[0] != 0x55) || (rom_header[1] != 0xaa)) {
+ pr_warn("Incorrect Rom file format\n");
+ ptdev->need_rombar = false;
+ } else {
+ /* check file size */
+ fseek(fp, 0, SEEK_END);
+ file_size = ftell(fp);
+ fseek(fp, 0, SEEK_SET);
+ if (file_size > (2048 * 1024)) {
+ pr_warn("Rom file is too big.\n");
+ ptdev->need_rombar = false;
+ }
+ }
Can we swap the logic to positive flow.

if ((rom_header[0] == 0x55) && (rom_header[1] == 0xaa)) {
...
} else {
pr_warn("Incorrect Rom file format\n");
ptdev->need_rombar = false;
}
It is also ok to use the above logic.
It will be updated in next version.

+ if (ptdev->need_rombar) {
+ rom_size = ALIGN_UP(file_size, 4096);
+ /* round up to a power of 2 */
+ if ((rom_size & (rom_size - 1)) != 0)
+ rom_size = 1UL << fls(rom_size);
+
+ if (posix_memalign((void **)&rom_buffer, 4096, rom_size)) {
+ ptdev->need_rombar = false;
+ pr_err("Fail to allocate buffer for rom_file\n");
+ } else {
+ ptdev->rom_buffer = rom_buffer;
+ read_size = fread(rom_buffer, 1, file_size, fp);
+ if (read_size != file_size) {
+ pr_warn("%s: read size is different with rom_size\n", __func__);
+ }
+ pci_emul_alloc_bar(dev, PCI_ROMBAR, PCIBAR_ROM, rom_size);
+
+ /* Setup the EPT mapping for ROM bar */
+ bar_addr = dev->bar[PCI_ROMBAR].addr;
+ if (vm_map_memseg_vma(ctx, rom_size, bar_addr,
+ (uint64_t)rom_buffer, PROT_READ)) {
+ pr_err("%s: Fail to map ROM bar\n", __func__);
+ free(rom_buffer);
+ ptdev->need_rombar = false;
+ } else {
+ /* Update the enable flag */
+ pci_set_cfgdata32(dev, PCIR_BIOS, (bar_addr | PCIM_BIOS_ENABLE));
+ }
+ }
+ }
+ fclose(fp);
Please pack these code into a function, it can be named as create_rombar
OK. The above code will be put into one function in next version.


+ }
+ }
+
if (is_intel_graphics_dev(dev)) {
if (is_rtvm) {
pr_err("%s RTVM doesn't support GVT-D.", __func__);
@@ -805,6 +879,26 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
vm_reset_ptdev_intx_info(ctx, virt_bdf, ptdev->phys_bdf, dev->lintr.ioapic_irq, false);
}
+ if (ptdev->need_rombar) {
+ int error;
+ struct acrn_vm_memmap rom_unmap;
+
+ rom_unmap.user_vm_pa = dev->bar[PCI_ROMBAR].addr;
+ rom_unmap.len = dev->bar[PCI_ROMBAR].size;
+ /* remove the EPT mapping for PCI_rombar.
+ * WA: Use ACRN_MEMMAP_MMIO to del the mapping
+ */
+ rom_unmap.type = ACRN_MEMMAP_MMIO;
+ error = ioctl(ctx->fd, ACRN_IOCTL_UNSET_MEMSEG, &rom_unmap);
+ if (error) {
+ pr_err("%s: unset_memseg ioctl() returned an error: %s\n",
+ __func__, errormsg(errno));
+ }
+ free(ptdev->rom_buffer);
+ ptdev->rom_buffer = NULL;
+ ptdev->need_rombar = false;
+ }
release_rombar

+
if (ptdev)
phys_bdf = ptdev->phys_bdf;
diff --git a/devicemodel/include/passthru.h b/devicemodel/include/passthru.h
index 7f693913e..655a17000 100644
--- a/devicemodel/include/passthru.h
+++ b/devicemodel/include/passthru.h
@@ -16,7 +16,7 @@
struct passthru_dev {
struct pci_vdev *dev;
- struct pcibar bar[PCI_BARMAX + 1];
+ struct pcibar bar[PCI_BARMAX + 2];
struct {
int capoff;
} msi;
@@ -36,6 +36,8 @@ struct passthru_dev {
*/
bool need_reset;
bool d3hot_reset;
+ bool need_rombar;
+ char *rom_buffer;
bool (*has_virt_pcicfg_regs)(int offset);
};
--
2.25.1


Yu Wang
 

On Mon, Sep 19, 2022 at 04:50:37PM +0800, Zhao, Yakui wrote:


On 2022/9/19 16:10, Yu Wang wrote:
On Mon, Sep 19, 2022 at 04:32:55PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:59, Yu Wang wrote:
On Mon, Sep 19, 2022 at 04:16:11PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:41, Yu Wang wrote:
On Mon, Sep 19, 2022 at 03:54:48PM +0800, Zhao, Yakui wrote:


On 2022/9/19 15:21, Yu Wang wrote:
On Mon, Sep 19, 2022 at 09:52:40AM +0800, Zhao Yakui wrote:
PCI ROM is the firmware specific to PCI device and it is provided by
the device vendor. The PCI rom resides in 0x30 offset of PCI config space.
This can be used to check whether the PCI rom exists. And when it exists,
it can load the firmware from the addr that is obtained from ROM bar addr.

For the user-vm, it will try to load the rom_file for the given PCI device and
enable the VM to access the firmware that is defined in rom_file.

BTW: The emulated rom_file is converted from efi image by using EfiRom. It has
no dependency on the ROM bar of physical PCI devices. Of course if the physical
PCI devices supports the ROM bar, the rom_file can also be dumped from the PCI
rom.

Now this is limited to PCI display device.

Signed-off-by: Zhao Yakui <yakui.zhao@...>
---
devicemodel/hw/pci/passthrough.c | 94 ++++++++++++++++++++++++++++++++
devicemodel/include/passthru.h | 4 +-
2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/passthrough.c b/devicemodel/hw/pci/passthrough.c
index 4c227aea9..9590d95bc 100644
--- a/devicemodel/hw/pci/passthrough.c
+++ b/devicemodel/hw/pci/passthrough.c
@@ -600,6 +600,13 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
struct acrn_pcidev pcidev = {};
uint16_t vendor = 0, device = 0;
uint8_t class = 0;
+ char *rom_buffer;
+ uint64_t bar_addr;
+ FILE *fp;
+ int file_size, rom_size;
+ uint8_t rom_header[4];
+ size_t read_size;
+ char rom_file[256];
ptdev = NULL;
error = -EINVAL;
@@ -615,6 +622,7 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
return -EINVAL;
}
+ memset(rom_file, 0, sizeof(rom_file));
while ((opt = strsep(&opts, ",")) != NULL) {
if (!strncmp(opt, "keep_gsi", 8))
keep_gsi = true;
@@ -711,6 +719,72 @@ passthru_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
error = ACRN_PTDEV_IRQ_MSI;
}
+ if ((class != 3) || (dev->bus != 0)) {
+ if (ptdev->need_rombar) {
+ pr_warn("PCI rom is only supported on Bus 0 and GPU device\n");
PCI ROM only supports Intel iGPU.
The ROM bar is not restricted to iGPU.
Instead it is fine to add the ROM bar for discrete GPU.
The GPU on Bus0 is the iGPU only, right?..
This is the device for Guest VM. It doesn't mean that it should only be
iGPU.
Then this log doesn't make sense...
Why we need to mention the virtual BDF here? Checking the class is
enough?
For the device with ROM on Bus1/Bus2, maybe it needs to add some specific
code to handle the bridge_window related with ROM_bar region.
Do we have bus1/bus2 for virtual pci device? I think that we emulate all
devices as RCiEPs?
Currently the pci devices defined in launch_script mainly exist on the Bus0.
But based on the code of parse_bdf, it seems that it is possible to define
the devices on Bus1/Bus2.
We don't emulate virtual root port for user VM. It is only used for SOS
to prevent physical ACS be disabled by SOS then do P2P attack for
Pre-launched VM.


If we don't need to consider this scenario, I can remove the check of
dev->bus.
Yes. Let's remove it. And the log can be changed to
"Virtual PCI ROM is only supported for GPU device."




To make it simple, we add the limitation that the ROM bar is only supported
for the PCI devices on Bus0.




And usually the discrete GPU has the physical ROMBAR, then it should be
done by ROMBAR passthrough instead of ROMBAR virtualization?..
The discrete GPU maybe has the ROM bar. Some devices have no ROM bar. When
it has the ROM bar, we don't need to virtualize it. Instead we can dump the
ROM file from the pci_device or download it from device_vendor.
And it needs the specifc code to dump the rom_file.(It is convenient to dump
it by using script or linux_cmd).

For the GPU device for guest_vm, we only need to provide the rom_file for
the emulated PCI rombar.
Ok. this part is OK to me.






+ ptdev->need_rombar = false;
+ }
+ }
+
+ if (ptdev->need_rombar) {
+
+ fp = fopen(rom_file, "rb");
+ if (fp == NULL) {
+ pr_warn("Fail to open %s rom_file\n", rom_file);
+ ptdev->need_rombar = false;
+ } else {
+ rom_buffer = NULL;
+ read_size = fread(rom_header, 1, 4, fp);
+ if (read_size !=4)
+ pr_warn("%s: Fail to read rom_header\n", __func__);
+
+ if ((rom_header[0] != 0x55) || (rom_header[1] != 0xaa)) {
+ pr_warn("Incorrect Rom file format\n");
+ ptdev->need_rombar = false;
+ } else {
+ /* check file size */
+ fseek(fp, 0, SEEK_END);
+ file_size = ftell(fp);
+ fseek(fp, 0, SEEK_SET);
+ if (file_size > (2048 * 1024)) {
+ pr_warn("Rom file is too big.\n");
+ ptdev->need_rombar = false;
+ }
+ }
Can we swap the logic to positive flow.

if ((rom_header[0] == 0x55) && (rom_header[1] == 0xaa)) {
...
} else {
pr_warn("Incorrect Rom file format\n");
ptdev->need_rombar = false;
}
It is also ok to use the above logic.
It will be updated in next version.

+ if (ptdev->need_rombar) {
+ rom_size = ALIGN_UP(file_size, 4096);
+ /* round up to a power of 2 */
+ if ((rom_size & (rom_size - 1)) != 0)
+ rom_size = 1UL << fls(rom_size);
+
+ if (posix_memalign((void **)&rom_buffer, 4096, rom_size)) {
+ ptdev->need_rombar = false;
+ pr_err("Fail to allocate buffer for rom_file\n");
+ } else {
+ ptdev->rom_buffer = rom_buffer;
+ read_size = fread(rom_buffer, 1, file_size, fp);
+ if (read_size != file_size) {
+ pr_warn("%s: read size is different with rom_size\n", __func__);
+ }
+ pci_emul_alloc_bar(dev, PCI_ROMBAR, PCIBAR_ROM, rom_size);
+
+ /* Setup the EPT mapping for ROM bar */
+ bar_addr = dev->bar[PCI_ROMBAR].addr;
+ if (vm_map_memseg_vma(ctx, rom_size, bar_addr,
+ (uint64_t)rom_buffer, PROT_READ)) {
+ pr_err("%s: Fail to map ROM bar\n", __func__);
+ free(rom_buffer);
+ ptdev->need_rombar = false;
+ } else {
+ /* Update the enable flag */
+ pci_set_cfgdata32(dev, PCIR_BIOS, (bar_addr | PCIM_BIOS_ENABLE));
+ }
+ }
+ }
+ fclose(fp);
Please pack these code into a function, it can be named as create_rombar
OK. The above code will be put into one function in next version.


+ }
+ }
+
if (is_intel_graphics_dev(dev)) {
if (is_rtvm) {
pr_err("%s RTVM doesn't support GVT-D.", __func__);
@@ -805,6 +879,26 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
vm_reset_ptdev_intx_info(ctx, virt_bdf, ptdev->phys_bdf, dev->lintr.ioapic_irq, false);
}
+ if (ptdev->need_rombar) {
+ int error;
+ struct acrn_vm_memmap rom_unmap;
+
+ rom_unmap.user_vm_pa = dev->bar[PCI_ROMBAR].addr;
+ rom_unmap.len = dev->bar[PCI_ROMBAR].size;
+ /* remove the EPT mapping for PCI_rombar.
+ * WA: Use ACRN_MEMMAP_MMIO to del the mapping
+ */
+ rom_unmap.type = ACRN_MEMMAP_MMIO;
+ error = ioctl(ctx->fd, ACRN_IOCTL_UNSET_MEMSEG, &rom_unmap);
+ if (error) {
+ pr_err("%s: unset_memseg ioctl() returned an error: %s\n",
+ __func__, errormsg(errno));
+ }
+ free(ptdev->rom_buffer);
+ ptdev->rom_buffer = NULL;
+ ptdev->need_rombar = false;
+ }
release_rombar

+
if (ptdev)
phys_bdf = ptdev->phys_bdf;
diff --git a/devicemodel/include/passthru.h b/devicemodel/include/passthru.h
index 7f693913e..655a17000 100644
--- a/devicemodel/include/passthru.h
+++ b/devicemodel/include/passthru.h
@@ -16,7 +16,7 @@
struct passthru_dev {
struct pci_vdev *dev;
- struct pcibar bar[PCI_BARMAX + 1];
+ struct pcibar bar[PCI_BARMAX + 2];
struct {
int capoff;
} msi;
@@ -36,6 +36,8 @@ struct passthru_dev {
*/
bool need_reset;
bool d3hot_reset;
+ bool need_rombar;
+ char *rom_buffer;
bool (*has_virt_pcicfg_regs)(int offset);
};
--
2.25.1