On Thu, Nov 25, 2021 at 01:02:43PM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <[email protected]>
>
> Add relevant vpci register handlers when assigning PCI device to a domain
> and remove those when de-assigning. This allows having different
> handlers for different domains, e.g. hwdom and other guests.
>
> Emulate guest BAR register values: this allows creating a guest view
> of the registers and emulates size and properties probe as it is done
> during PCI device enumeration by the guest.
>
> ROM BAR is only handled for the hardware domain and for guest domains
> there is a stub: at the moment PCI expansion ROM handling is supported
> for x86 only and it might not be used by other architectures without
> emulating x86. Other use-cases may include using that expansion ROM before
> Xen boots, hence no emulation is needed in Xen itself. Or when a guest
> wants to use the ROM code which seems to be rare.
>
> Signed-off-by: Oleksandr Andrushchenko <[email protected]>
> ---
> Since v4:
> - updated commit message
> - s/guest_addr/guest_reg
> Since v3:
> - squashed two patches: dynamic add/remove handlers and guest BAR
> handler implementation
> - fix guest BAR read of the high part of a 64bit BAR (Roger)
> - add error handling to vpci_assign_device
> - s/dom%pd/%pd
> - blank line before return
> Since v2:
> - remove unneeded ifdefs for CONFIG_HAS_VPCI_GUEST_SUPPORT as more code
> has been eliminated from being built on x86
> Since v1:
> - constify struct pci_dev where possible
> - do not open code is_system_domain()
> - simplify some code3. simplify
> - use gdprintk + error code instead of gprintk
> - gate vpci_bar_{add|remove}_handlers with CONFIG_HAS_VPCI_GUEST_SUPPORT,
> so these do not get compiled for x86
> - removed unneeded is_system_domain check
> - re-work guest read/write to be much simpler and do more work on write
> than read which is expected to be called more frequently
> - removed one too obvious comment
> ---
> xen/drivers/vpci/header.c | 72 +++++++++++++++++++++++++++++++++++----
> xen/include/xen/vpci.h | 3 ++
> 2 files changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index ba333fb2f9b0..8880d34ebf8e 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -433,6 +433,48 @@ static void bar_write(const struct pci_dev *pdev,
> unsigned int reg,
> pci_conf_write32(pdev->sbdf, reg, val);
> }
>
> +static void guest_bar_write(const struct pci_dev *pdev, unsigned int reg,
> + uint32_t val, void *data)
> +{
> + struct vpci_bar *bar = data;
> + bool hi = false;
> +
> + if ( bar->type == VPCI_BAR_MEM64_HI )
> + {
> + ASSERT(reg > PCI_BASE_ADDRESS_0);
> + bar--;
> + hi = true;
> + }
> + else
> + {
> + val &= PCI_BASE_ADDRESS_MEM_MASK;
> + val |= bar->type == VPCI_BAR_MEM32 ? PCI_BASE_ADDRESS_MEM_TYPE_32
> + : PCI_BASE_ADDRESS_MEM_TYPE_64;
> + val |= bar->prefetchable ? PCI_BASE_ADDRESS_MEM_PREFETCH : 0;
> + }
> +
> + bar->guest_reg &= ~(0xffffffffull << (hi ? 32 : 0));
> + bar->guest_reg |= (uint64_t)val << (hi ? 32 : 0);
> +
> + bar->guest_reg &= ~(bar->size - 1) | ~PCI_BASE_ADDRESS_MEM_MASK;
> +}
> +
> +static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg,
> + void *data)
> +{
> + const struct vpci_bar *bar = data;
> + bool hi = false;
> +
> + if ( bar->type == VPCI_BAR_MEM64_HI )
> + {
> + ASSERT(reg > PCI_BASE_ADDRESS_0);
> + bar--;
> + hi = true;
> + }
> +
> + return bar->guest_reg >> (hi ? 32 : 0);
> +}
> +
> static void rom_write(const struct pci_dev *pdev, unsigned int reg,
> uint32_t val, void *data)
> {
> @@ -481,6 +523,17 @@ static void rom_write(const struct pci_dev *pdev,
> unsigned int reg,
> rom->addr = val & PCI_ROM_ADDRESS_MASK;
> }
>
> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
> + uint32_t val, void *data)
> +{
> +}
> +
> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
> + void *data)
> +{
> + return 0xffffffff;
> +}
There should be no need for those handlers. As said elsewhere: for
guests registers not explicitly handled should return ~0 for reads and
drop writes, which is what you are proposing here.
> +
> static int init_bars(struct pci_dev *pdev)
> {
> uint16_t cmd;
> @@ -489,6 +542,7 @@ static int init_bars(struct pci_dev *pdev)
> struct vpci_header *header = &pdev->vpci->header;
> struct vpci_bar *bars = header->bars;
> int rc;
> + bool is_hwdom = is_hardware_domain(pdev->domain);
>
> switch ( pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f )
> {
> @@ -528,8 +582,10 @@ static int init_bars(struct pci_dev *pdev)
> if ( i && bars[i - 1].type == VPCI_BAR_MEM64_LO )
> {
> bars[i].type = VPCI_BAR_MEM64_HI;
> - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write,
> reg,
> - 4, &bars[i]);
> + rc = vpci_add_register(pdev->vpci,
> + is_hwdom ? vpci_hw_read32 :
> guest_bar_read,
> + is_hwdom ? bar_write : guest_bar_write,
> + reg, 4, &bars[i]);
> if ( rc )
> {
> pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> @@ -569,8 +625,10 @@ static int init_bars(struct pci_dev *pdev)
> bars[i].size = size;
> bars[i].prefetchable = val & PCI_BASE_ADDRESS_MEM_PREFETCH;
>
> - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg, 4,
> - &bars[i]);
> + rc = vpci_add_register(pdev->vpci,
> + is_hwdom ? vpci_hw_read32 : guest_bar_read,
> + is_hwdom ? bar_write : guest_bar_write,
> + reg, 4, &bars[i]);
> if ( rc )
> {
> pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> @@ -590,8 +648,10 @@ static int init_bars(struct pci_dev *pdev)
> header->rom_enabled = pci_conf_read32(pdev->sbdf, rom_reg) &
> PCI_ROM_ADDRESS_ENABLE;
>
> - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
> rom_reg,
> - 4, rom);
> + rc = vpci_add_register(pdev->vpci,
> + is_hwdom ? vpci_hw_read32 : guest_rom_read,
> + is_hwdom ? rom_write : guest_rom_write,
> + rom_reg, 4, rom);
This whole call should be made conditional to is_hwdom, as said above
there's no need for the guest_rom handlers.
Likewise I assume you expect IO BARs to simply return ~0 and drop
writes, as there's no explicit handler added for those?
> if ( rc )
> rom->type = VPCI_BAR_EMPTY;
> }
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index ed127a08a953..0a73b14a92dc 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -68,7 +68,10 @@ struct vpci {
> struct vpci_header {
> /* Information about the PCI BARs of this device. */
> struct vpci_bar {
> + /* Physical view of the BAR. */
No, that's not the physical view, it's the physical (host) address.
> uint64_t addr;
> + /* Guest view of the BAR: address and lower bits. */
> + uint64_t guest_reg;
I continue to think it would be clearer if you store the guest address
here (gaddr, without the low bits) and add those in guest_bar_read
based on bar->{type,prefetchable}. Then it would be equivalent to the
existing 'addr' field.
I wonder whether we need to protect the added code with
CONFIG_HAS_VPCI_GUEST_SUPPORT, this would effectively be dead code
otherwise. Long term I don't think we wish to differentiate between
dom0 and domU vPCI support at build time, so I'm unsure whether it's
helpful to pollute the code with CONFIG_HAS_VPCI_GUEST_SUPPORT when
the plan is to remove those long term.
Thanks, Roger.
