| Message ID | 20210930075223.860329-6-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On 30.09.2021 09:52, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > 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 is x86 only, so 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 <oleksandr_andrushchenko@epam.com> > Reviewed-by: Michal Orzel <michal.orzel@arm.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On Thu, Sep 30, 2021 at 10:52:17AM +0300, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > 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 is x86 only, so 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 <oleksandr_andrushchenko@epam.com> > Reviewed-by: Michal Orzel <michal.orzel@arm.com> > --- > Since v1: > - 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 > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > xen/drivers/vpci/header.c | 30 +++++++++++++++++++++++++++++- > xen/include/xen/vpci.h | 3 +++ > 2 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index 1ce98795fcca..ec4d215f36ff 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -400,12 +400,38 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, > 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_addr &= ~(0xffffffffull << (hi ? 32 : 0)); > + bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0); > + > + bar->guest_addr &= ~(bar->size - 1) | ~PCI_BASE_ADDRESS_MEM_MASK; > } > > static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg, > void *data) > { > - return 0xffffffff; > + const struct vpci_bar *bar = data; > + > + if ( bar->type == VPCI_BAR_MEM64_HI ) > + return bar->guest_addr >> 32; > + > + return bar->guest_addr; I think this is missing a check for whether the BAR is the high part of a 64bit one? Ie: 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_addr >> (hi ? 32 : 0); Or else when accessing the high part of a 64bit BAR you will always return 0s as it hasn't been setup by guest_bar_write. Thanks, Roger.
On 26.10.21 10:50, Roger Pau Monné wrote: > On Thu, Sep 30, 2021 at 10:52:17AM +0300, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> 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 is x86 only, so 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 <oleksandr_andrushchenko@epam.com> >> Reviewed-by: Michal Orzel <michal.orzel@arm.com> >> --- >> Since v1: >> - 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 >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> --- >> xen/drivers/vpci/header.c | 30 +++++++++++++++++++++++++++++- >> xen/include/xen/vpci.h | 3 +++ >> 2 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index 1ce98795fcca..ec4d215f36ff 100644 >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -400,12 +400,38 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, >> 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_addr &= ~(0xffffffffull << (hi ? 32 : 0)); >> + bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0); >> + >> + bar->guest_addr &= ~(bar->size - 1) | ~PCI_BASE_ADDRESS_MEM_MASK; >> } >> >> static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg, >> void *data) >> { >> - return 0xffffffff; >> + const struct vpci_bar *bar = data; >> + >> + if ( bar->type == VPCI_BAR_MEM64_HI ) >> + return bar->guest_addr >> 32; >> + >> + return bar->guest_addr; > I think this is missing a check for whether the BAR is the high part > of a 64bit one? Ie: > > 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_addr >> (hi ? 32 : 0); > > Or else when accessing the high part of a 64bit BAR you will always > return 0s as it hasn't been setup by guest_bar_write. Yes, you are right > Thanks, Roger. Thank you, Oleksandr
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index 1ce98795fcca..ec4d215f36ff 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -400,12 +400,38 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg, 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_addr &= ~(0xffffffffull << (hi ? 32 : 0)); + bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0); + + bar->guest_addr &= ~(bar->size - 1) | ~PCI_BASE_ADDRESS_MEM_MASK; } static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg, void *data) { - return 0xffffffff; + const struct vpci_bar *bar = data; + + if ( bar->type == VPCI_BAR_MEM64_HI ) + return bar->guest_addr >> 32; + + return bar->guest_addr; } static void rom_write(const struct pci_dev *pdev, unsigned int reg, @@ -522,6 +548,8 @@ static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom) if ( rc ) return rc; } + + bars[i].guest_addr = 0; } return 0; } diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index fd822c903af5..a0320b22cb36 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -75,7 +75,10 @@ struct vpci { struct vpci_header { /* Information about the PCI BARs of this device. */ struct vpci_bar { + /* Physical view of the BAR. */ uint64_t addr; + /* Guest view of the BAR. */ + uint64_t guest_addr; uint64_t size; enum { VPCI_BAR_EMPTY,